Skip to content

Provide a more flexible way to adopt AWS resources that are part of a cloudformation stack #2248

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

Closed
adriananeci opened this issue Jan 21, 2025 · 4 comments
Assignees
Labels
area/runtime Issues or PRs as related to controller runtime, common reconciliation logic, etc kind/bug Categorizes issue or PR as related to a bug. target/q1-2025 Issues scheduled for Q1 in 2025

Comments

@adriananeci
Copy link

adriananeci commented Jan 21, 2025

Is your feature request related to a problem?

Hey team, we are currently trying to adopt some AWS resources that were initially created via a cloudformation stack. Our ACK objects are packed into a helm chart and we have a helm value that decides if we adopt or apply the helm spec for a particular ACK object, something like:

---
apiVersion: ec2.services.k8s.aws/v1alpha1
kind: VPC
metadata:
  name: {{ $name }}
  namespace: {{ $namespace }}
  annotations:
    services.k8s.aws/region: {{ .Values.global.location }}
{{- if .Values.global.adoptExistingResources }}
    services.k8s.aws/adoption-fields: |
      {
        "vpcID": "{{ .Values.network.vpc.id }}"
      }
    services.k8s.aws/adoption-policy: adopt
{{- else }}
spec:
  cidrBlocks:
........................

Our plan is to initially adopt the AWS resources (adoptExistingResources: true) and once adopted, just flip the adoptExistingResources: false so that we can manage the AWS resource via our helm chart moving forward.
The curent problem we are facing is that once we set the adoptExistingResources: false , the ACK object is failing the reconciliation because of:

status:
  conditions:
  - message: "Error syncing property 'Tagging': InvalidTag: System tags cannot be removed by requester\n\tstatus code: 400 ..."
    status: "True"
    type: ACK.Recoverable
  - lastTransitionTime: "2025-01-17T11:34:14Z"
    message: Unable to determine if desired resource state matches latest observed state
    reason: "Error syncing property 'Tagging': InvalidTag: System tags cannot be removed by requester\n\tstatus code: 400 ..."
    status: Unknown
    type: ACK.ResourceSynced

I guess this error is coming from AWS API since ACK controller is trying to remove the system tags (the ones that start with aws:) given our ACK object spec from the helm chart doesn't have these system tags defined.
Is there an easy way we can overcome this situation without the need of extracting and keeping the system tags inside the helm values?

Also, in the current state, once an AWS object that is part of a cloudformation stack is adopted an dthen someone is trying to adjust the system tags, it will get a similar error like the one above.
Describe the solution you'd like

I'm thinking to something like:

  • Get the AWS object details first
  • Merge any system tags with the ones defined in the ACK object spec
  • Post the request during reconciliation with the merged tags list
    In this way, system tags will be transparent for ACK users, won't be stored in ACK object spec (not even during adoption) and will allow a smoother ACK adoption. This feature can be put behind a feature flag.

Describe alternatives you've considered

Initial approach was to piggy back the cloudformation tags and save them as helm values for every AWS object we are looking to adopt into ACK, but we found out pretty quickly that given the multitude of AWS objects we need to import, we'll get into a sppagetti situation.

Slack thread with the initial discussion: https://kubernetes.slack.com/archives/C0402D8JJS1/p1737388913108039
cc @amine Hilaly @michaelhtm

@a-hilaly a-hilaly added kind/bug Categorizes issue or PR as related to a bug. area/runtime Issues or PRs as related to controller runtime, common reconciliation logic, etc labels Jan 21, 2025
@eqe-aws eqe-aws added the q1-2025 label Feb 5, 2025
@ack-bot ack-bot added target/q1-2025 Issues scheduled for Q1 in 2025 and removed q1-2025 labels Feb 6, 2025
ack-prow bot pushed a commit to aws-controllers-k8s/code-generator that referenced this issue Feb 17, 2025
Issue [#2248](aws-controllers-k8s/community#2248)

Description of changes:
By generating these two functions, we ensure that AWS tags will not 
be present in resource Spec during adoption/regular reconciliations.

FilterSystemTags will be called right after the ReadOne operation during
adoption, and it will ensure we don't patch AWS (aws:cloudformation..)
or Controller tags (services.aws.k8s/...) into the resource.

MirrowAWSTags instead, is called during the regular reconciliation loop,
after each ReadOne operation. It ensures that if the resource in AWS has
AWS tags, they need to be reflected in the desired Spec. This will ensure
that those tags will not be detected in the Delta and trigger an update. 
Since the controller only patches the difference between the latest and 
desired resource, mirroring the AWS tags ensures we don't include them
in our Spec.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
ack-prow bot pushed a commit to aws-controllers-k8s/s3-controller that referenced this issue Feb 17, 2025
Issue [#2248](aws-controllers-k8s/community#2248)

Description of changes:
This test expects the ACK controller to always ignore AWS tags.
 It also attempts updating the tags and ensures the AWS tags 
injected by cloudformation are still ignored.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@michaelhtm
Copy link
Member

michaelhtm commented Feb 17, 2025

Hey @adriananeci we are making releases addressing this issue today. Will post here once all releases are completed.

@michaelhtm
Copy link
Member

Hey @adriananeci, all controllers have now been released with this feature!
Closing this issue for now. Feel free to reopen it if you find any issues. Thanks!

@michaelhtm
Copy link
Member

/close

Copy link

ack-prow bot commented Feb 18, 2025

@michaelhtm: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ack-prow ack-prow bot closed this as completed Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/runtime Issues or PRs as related to controller runtime, common reconciliation logic, etc kind/bug Categorizes issue or PR as related to a bug. target/q1-2025 Issues scheduled for Q1 in 2025
Projects
None yet
Development

No branches or pull requests

5 participants