[craig-lind]

Info

Relates to: https://github.com/elastic/platform-engineering-productivity/issues/1479

While we do somewhat check if redaction is possible sometimes we can fail silently. Because of this, we want to add some clarity to the issue by using an annotation.

Some important background on this is that we have some pipelines that utilize old OS versions that don't run the 3.66.0 or up of the buildkite agent. If we switch this to hard fail instead then we would break the pipelines that use these old images. This is a "rock and a hard place" so I chose to just make it a warning with some clarifying comments about what we could change in the future.

Testing

I moved over a secret over to ci-dev and then used the plugin reference to this branch to use it:

steps:
  - label: "Test Getting Secret - Valid OS"
    command: echo "${AZURE_SUBSCRIPTION_ID}"
    agents:
      provider: azure
    plugins:
      - elastic/vault-secrets#1479/add-annotation-for-redaction-failure:
          path: "secret/ci/elastic-ci/image-cleaning/azure/dev"
          field: "subscription_id"
          env_var: "AZURE_SUBSCRIPTION_ID"
  - label: "Test Getting Secret - Old OS"
    command: echo "${AZURE_SUBSCRIPTION_ID}"
    agents:
      provider: azure
      image: core-windows-10
    plugins:
      - elastic/vault-secrets#1479/add-annotation-for-redaction-failure:
          path: "secret/ci/elastic-ci/image-cleaning/azure/dev"
          field: "subscription_id"
          env_var: "AZURE_SUBSCRIPTION_ID"

The subscription ID really isn't a secret, it just happens to be there which is why I chose it and is ok if it shows.

This test build shows the annotation that appears for the Windows 10 instance.

[chrisnavar]

I see this as a clear and concise way to fix this. I'd say this would work but could you test it before merge? Would be helpful to have an specific unit test where it simulates this scenario?

[chrisnavar]

The change looks good to me. Thanks for testing this.

[v1v]

IIUC, the redaction command has been removed, and as a result, I can no longer see any redactor executions.

@craig-lind, could you clarify why this behaviour was changed?

cc @chrisnavar

[v1v]

I can see

vault-secrets-buildkite-plugin/hooks/environment

Lines 49 to 52 in d4f7da8

	# _SECRET suffix is used to ensure the variable content is redacted in Buildkite logs
	# https://buildkite.com/docs/pipelines/managing-log-output#redacted-environment-variables
	final_var_name=$(echo "$var_name" | tr "[:lower:]" "[:upper:]" | tr ".-" "_")_SECRET
	echo "$final_var_name"

I guess that the _SECRET enforcement, L51 is enough for this case, regardless what the user's input is.

However, I think the docs don't say anything about this particular behaviour:

• https://github.com/elastic/vault-secrets-buildkite-plugin/blob/main/README.md#usage

When env_var is not specified, the name of the environment variable will be generated using this scheme: <UPPERCASE_SECRET_NAME>[_<UPPERCASE_FIELD_NAME>]_SECRET. Note that if you do specify an env_var, you should use one of the patterns that will ensure Buildkite will redact the secret, see the docs for details.

Wonder if reflecting that behaviour could help users here

[chrisnavar]

Shouldn't be removed, what this PR did was to annotate the reason for the redaction skipping.
It checks with check_buildkite_agent_version_for_redaction if the buildkite-agent version has the redactor subcommand available (>3.66.0), if not it sets the reason on redaction_skip_reason.

Then it redacts the secret on line 93 and if it fails it sets that the redaction command failed on redaction_skip_reason.

Finally, if the redaction_skip_reason is not empty then it does annotate with the actual reason as to why the redaction failed or why it wasn't available.

[v1v]

Gotcha, I totally misread it! I just wanted to see if I could use the latest version and was curious about the new changes.

That totally makes sense, and thanks for helping me clear up my blind spot here 🙇

[chrisnavar]

No worries Victor! Feel free to ask any other doubts that may arise as you use it!