Skip to content

AWS SM PushSecret creates new version w/ every refresh cycle with secretPushFormat "string" #3436

@gtfortytwo

Description

@gtfortytwo

Describe the bug
When using PushSecret with the AWS Secrets Manager provider and the secretPushFormat is set to string, ESO calls PutSecretValueWithContext to create a new version of the secret on every refresh cycle, even when the desired value has not changed since the previous refresh. This eventually results in hitting AWS API limits (on version update frequency) and creates numerous extraneous revisions of the AWS secret. This appears to have been introduced in v0.9.14 with #3189 which initially enabled the "string" value format.

Steps to reproduce the behavior:

  1. Create a PushSecret resource against a SecretStore using the AWS Secrets Manager provider and set spec.data[].metadata.secretPushFormat to string. To make the failure more evident, use a short spec.refreshInterval like 10s
  2. The incorrect behavior can be observed in several ways:
  • Watch the "Versions" tab of the secret in the AWS console and note the version IDs continue to change for the AWSCURRENT and AWSPREVIOUS labels.
  • Get the secret values with the AWS CLI (aws secretsmanager list-secret-version-ids)
  • Watch CloudTrail logs for repeated calls to PutSecretValueWithContext for the same secret resource.
  • Watch externalsecret_provider_api_calls_count ESO metrics for PutSecretValueWithContext

Expected behavior
ESO should only create new AWS secret versions when the desired secret value is different from the value observed in the destination secret.

Additional context
It appears that the logic is intended to behave as described in "expected behavior". In line 572 of secretsmanager.go, having read the remote value from the provider, the code checks that the remote secret exists and compares its observed value to the desired value. If those values are the same, it exits the reconcile loop taking no further action:

	if awsSecret != nil && bytes.Equal(awsSecret.SecretBinary, value) {
		return nil
	}

Unfortunately, it only compares the SecretBinary field. If the user elected to use secretPushFormat: string, then the API docs indicate SecretBinary will have no value because the value was set using SecretString as in line 590:

	if secretPushFormat == SecretPushFormatString {
		input.SetSecretBinary(nil).SetSecretString(string(value))
	}

Presumably, the fix would be fairly straightforward: if the user elected secretPushFormat: string (like it checks in line 589), then instead of comparing value to SecretBinary, we'd want to compare string(value) to SecretString (with appropriate safety checks). I wish I felt confident enough in my golang skills to submit a PR for this, buuuut I don't. :/

Sub-optimal workarounds
A few less-than-ideal workarounds are available while awaiting this fix:

  • Use binary format. Users can use binary format for the secret, if the consumer of the secret can tolerate that and do any necessary conversion. It also makes it impossible to view the secret value with the AWS console.
  • Set long refreshInterval to avoid excessive writes. This can help avoid hitting API limits, but it's no guarantee.

Metadata

Metadata

Assignees

No one assigned

    Labels

    good first issueGood for newcomerskind/bugCategorizes issue or PR as related to a bug.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions