Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AWS Secrets Manager Provider Constantly Adds and Deletes Regional Replication #1079

Closed
shelby-moore opened this issue Jan 11, 2024 · 5 comments · Fixed by #1107
Closed

AWS Secrets Manager Provider Constantly Adds and Deletes Regional Replication #1079

shelby-moore opened this issue Jan 11, 2024 · 5 comments · Fixed by #1107
Labels
bug Something isn't working is:triaged Indicates that an issue has been reviewed.

Comments

@shelby-moore
Copy link

What happened?

On upgrade to 0.47.1 (also observed the same behaviour in 0.46.0), the AWS Secrets Manager provider constantly removes regional replication for secrets and then adds it back. This results in a steep increase in billing costs for the AWS Secrets Manager API. Tailing the logs for the provider will show the constant creation and deletion of replication. Downgrading the provider to 0.43.0 resolved the issue.

How can we reproduce it?

Create a Secret resource with the replica config block set to replicate the secret to an additional region, ex:

---
apiVersion: secretsmanager.aws.upbound.io/v1beta1
kind: Secret
metadata:
  name: my-secret
spec:
  deletionPolicy: Delete
  forProvider:
    name: my-secret
    region: us-east-1
    replica:
    - region: us-east-2

What environment did it happen in?

  • Crossplane Version: 0.14.5
  • Provider Version: 0.47.1 (also observed for 0.46.0)
  • Kubernetes Version: 1.28
  • Kubernetes Distribution: EKS
@shelby-moore shelby-moore added bug Something isn't working needs:triage labels Jan 11, 2024
@turkenf
Copy link
Collaborator

turkenf commented Jan 11, 2024

Thank you for bringing up this issue @shelby-moore, could you please share the logs with us?

@shelby-moore
Copy link
Author

For sure, here's an excerpt of what gets output repeatedly for each secret with replication being managed:

2024-01-11T22:21:01Z	DEBUG	provider-aws	Diff detected	{"uid": "REDACTED_UUID", "name": "REDACTED_SECRET_NAME", "gvk": "secretsmanager.aws.upbound.io/v1beta1, Kind=Secret", "instanceDiff": "*terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff{\"replica.REDACTED_REPLICA_ID.kms_key_id\":*terraform.ResourceAttrDiff{Old:\"\", New:\"\", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"replica.REDACTED_REPLICA_ID.last_accessed_date\":*terraform.ResourceAttrDiff{Old:\"\", New:\"\", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"replica.REDACTED_REPLICA_ID.region\":*terraform.ResourceAttrDiff{Old:\"\", New:\"us-east-2\", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"replica.REDACTED_REPLICA_ID.status\":*terraform.ResourceAttrDiff{Old:\"\", New:\"\", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"replica.REDACTED_REPLICA_ID.status_message\":*terraform.ResourceAttrDiff{Old:\"\", New:\"\", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"replica.REDACTED_REPLICA_ID.region\":*terraform.ResourceAttrDiff{Old:\"us-east-2\", New:\"\", NewComputed:false, NewRemoved:true, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}}, Destroy:false, DestroyDeposed:false, DestroyTainted:false, RawConfig:cty.NilVal, RawState:cty.NilVal, RawPlan:cty.NilVal, Meta:map[string]interface {}(nil)}"}
2024-01-11T22:21:01Z	DEBUG	provider-aws	Successfully requested update of external resource	{"controller": "managed/secretsmanager.aws.upbound.io/v1beta1, kind=secret", "request": {"name":"REDACTED_SECRET_NAME"}, "uid": "REDACTED_UUID", "version": "38518484", "external-name": "REDACTED_SECRET_ARN", "requeue-after": "2024-01-11T22:30:44Z"}
2024-01-11T22:21:01Z	DEBUG	provider-aws	Async update starting...	{"trackerUID": "REDACTED_UUID", "resourceName": "REDACTED_SECRET_NAME", "tfID": "REDACTED_SECRET_ARN"}
2024-01-11T22:21:01Z	DEBUG	provider-aws	Updating the external resource	{"uid": "REDACTED_UUID", "name": "REDACTED_SECRET_NAME", "gvk": "secretsmanager.aws.upbound.io/v1beta1, Kind=Secret"}
2024/01/11 22:21:01 [DEBUG] Removing Secrets Manager Secret Replicas: {
  RemoveReplicaRegions: ["us-east-2"],
  SecretId: "REDACTED_SECRET_ARN"
}
2024-01-11T22:21:01Z	DEBUG	events	Successfully requested update of external resource	{"type": "Normal", "object": {"kind":"Secret","name":"REDACTED_SECRET_NAME","uid":"REDACTED_UUID","apiVersion":"secretsmanager.aws.upbound.io/v1beta1","resourceVersion":"38518484"}, "reason": "UpdatedExternalResource"}
2024/01/11 22:21:01 [DEBUG] Waiting for state to become: [success]
2024/01/11 22:21:01 [DEBUG] Waiting for state to become: [success]
2024/01/11 22:21:01 [DEBUG] Waiting for state to become: [success]
2024/01/11 22:21:01 [DEBUG] Removing Secrets Manager Secret Replica: {
  AddReplicaRegions: [{
      Region: "us-east-2"
    }],
  ForceOverwriteReplicaSecret: false,
  SecretId: "REDACTED_SECRET_ARN"
}

@turkenf
Copy link
Collaborator

turkenf commented Jan 12, 2024

The issue can be reproduced with the provided information:

 2024-01-12T20:00:15+03:00	DEBUG	provider-aws	Diff detected	{"uid": "042c468f-94ce-4019-95c6-dc5ea89fe8b4", "name": "test-my", "gvk": "secretsmanager.aws.upbound.io/v1beta1, Kind=Secret", "instanceDiff": "*terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff{\"replica.1166113449.kms_key_id\":*terraform.ResourceAttrDiff{Old:\"\", New:\"\", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"replica.1166113449.last_accessed_date\":*terraform.ResourceAttrDiff{Old:\"\", New:\"\", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"replica.1166113449.region\":*terraform.ResourceAttrDiff{Old:\"\", New:\"us-east-2\", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"replica.1166113449.status\":*terraform.ResourceAttrDiff{Old:\"\", New:\"\", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"replica.1166113449.status_message\":*terraform.ResourceAttrDiff{Old:\"\", New:\"\", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"replica.4155765772.region\":*terraform.ResourceAttrDiff{Old:\"us-east-2\", New:\"\", NewComputed:false, NewRemoved:true, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}}, Destroy:false, DestroyDeposed:false, DestroyTainted:false, RawConfig:cty.NilVal, RawState:cty.NilVal, RawPlan:cty.NilVal, Meta:map[string]interface {}(nil)}"}

And UpToDate condition does not come when use the field replica

@turkenf turkenf added is:triaged Indicates that an issue has been reviewed. and removed needs:triage labels Jan 12, 2024
@mbbush
Copy link
Collaborator

mbbush commented Jan 13, 2024

It looks like the crossplane provider is setting a bunch of unspecified properties to "" in the New state, which are null in the Old state, and that's causing the terraform provider to replace the entire list element.

It seems like we need to do a better job distinguishing between null and "zero" values.

@erhancagirici
Copy link
Collaborator

Hi all, I've created #1107 and explained the situation on why it happens in the PR description.

@mbbush btw, for the record, this is a different issue than #1071, I'll create a separate fix for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working is:triaged Indicates that an issue has been reviewed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants