feat(tags): filter AWS tags and ACK tags during reconciliation #170
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Skipping CI for Draft Pull Request. |
ack-prow
bot
added
the
do-not-merge/work-in-progress
ack-prow
bot
removed
the
do-not-merge/work-in-progress
michaelhtm
force-pushed
the
adress/tag-issue
branch
2 times, most recently
from
2b71b27 to
795a5e1
Compare
a-hilaly
reviewed
Feb 14, 2025
michaelhtm
force-pushed
the
adress/tag-issue
branch
from
795a5e1 to
9a75aec
Compare
Add utility to automatically preserve immutable AWS-managed tags (aws:*) when modifying resources. Prevents tag operation errors with CloudFormation and Service Catalog managed resources.
michaelhtm
changed the title
michaelhtm
force-pushed
the
adress/tag-issue
branch
from
e14f58b to
06e1268
Compare
michaelhtm
changed the title
michaelhtm
changed the title
a-hilaly
reviewed
Feb 14, 2025
a-hilaly
reviewed
Feb 14, 2025
michaelhtm
force-pushed
the
adress/tag-issue
branch
2 times, most recently
from
65a917f to
9868c97
Compare
This funtion will filter out tags injected by AWS and ACK.
michaelhtm
force-pushed
the
adress/tag-issue
branch
from
9868c97 to
2748a4e
Compare
|
@michaelhtm: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, michaelhtm The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
michaelhtm
added a commit
to michaelhtm/ack-code-generator
that referenced
this pull request
Mar 6, 2025
When converting between ACK tags (map[string]string) and resource tags(which could be either a list of Tag struct or map[string]*string) can be tricky. One issue that we found was order preservation, especially when converting from `map[string]string` to list of `Tag`. The changed order, when patched to the resource in the cluster, would trigger a reconciliation, for an unpredictable amount of times (at least until we luckily convert to the list with a preserved order.) This change introduces two new functions, similar to the existing ones. The `toACKTagsWithKeyOrder` has the same function as `ToACKTags`, but besides returning the ACK tags, it also returns a list of keys in the same order it is in the resource. The `fromACKTagsWithKeyOrder`, also similar to `FromACKTags`, accepts the keyOrder returned by `toACKTagsWithKeyOrder`, and rebuilds the resource tags, if it's a list, in the same order as it was before. Any extra tags will be added in random order. These two new functions will be called before and after `mirrowAWSTags` and `FilterSystemTags`. The extra tags mentioned earlier are the AWS tags that are mirrored from the latest resource (more explanation on what they are [here](aws-controllers-k8s/runtime#170)) and will be filtered before getting patched with [this](aws-controllers-k8s/runtime#172) change. This will ensure that the tags in desired will maintain their order and avoid triggering unnecessary reconciliations.
michaelhtm
added a commit
to michaelhtm/ack-code-generator
that referenced
this pull request
Mar 6, 2025
When converting between ACK tags (map[string]string) and resource tags(which could be either a list of Tag struct or map[string]*string) can be tricky. One issue that we found was order preservation, especially when converting from `map[string]string` to list of `Tag`. The changed order, when patched to the resource in the cluster, would trigger a reconciliation, for an unpredictable amount of times (at least until we luckily convert to the list with a preserved order.) This change introduces two new functions, similar to the existing ones. The `toACKTagsWithKeyOrder` has the same function as `ToACKTags`, but besides returning the ACK tags, it also returns a list of keys in the same order it is in the resource. The `fromACKTagsWithKeyOrder`, also similar to `FromACKTags`, accepts the keyOrder returned by `toACKTagsWithKeyOrder`, and rebuilds the resource tags, if it's a list, in the same order as it was before. Any extra tags will be added in random order. These two new functions will be called before and after `mirrowAWSTags` and `FilterSystemTags`. The extra tags mentioned earlier are the AWS tags that are mirrored from the latest resource (more explanation on what they are [here](aws-controllers-k8s/runtime#170)) and will be filtered before getting patched with [this](aws-controllers-k8s/runtime#172) change. This will ensure that the tags in desired will maintain their order and avoid triggering unnecessary reconciliations.
michaelhtm
added a commit
to michaelhtm/ack-code-generator
that referenced
this pull request
Mar 7, 2025
When converting between ACK tags (map[string]string) and resource tags(which could be either a list of Tag struct or map[string]*string) can be tricky. One issue that we found was order preservation, especially when converting from `map[string]string` to list of `Tag`. The changed order, when patched to the resource in the cluster, would trigger a reconciliation, for an unpredictable amount of times (at least until we luckily convert to the list with a preserved order.) This change introduces two new functions, similar to the existing ones. The `toACKTagsWithKeyOrder` has the same function as `ToACKTags`, but besides returning the ACK tags, it also returns a list of keys in the same order it is in the resource. The `fromACKTagsWithKeyOrder`, also similar to `FromACKTags`, accepts the keyOrder returned by `toACKTagsWithKeyOrder`, and rebuilds the resource tags, if it's a list, in the same order as it was before. Any extra tags will be added in random order. These two new functions will be called before and after `mirrowAWSTags` and `FilterSystemTags`. The extra tags mentioned earlier are the AWS tags that are mirrored from the latest resource (more explanation on what they are [here](aws-controllers-k8s/runtime#170)) and will be filtered before getting patched with [this](aws-controllers-k8s/runtime#172) change. This will ensure that the tags in desired will maintain their order and avoid triggering unnecessary reconciliations.
ack-prow bot
pushed a commit
to aws-controllers-k8s/code-generator
that referenced
this pull request
Mar 10, 2025
…#572) Description of changes: When converting between ACK tags (map[string]string) and resource tags(which could be either a list of Tag struct or map[string]*string) can be tricky. One issue that we found was order preservation, especially when converting from `map[string]string` to list of `Tag`. The changed order, when patched to the resource in the cluster, would trigger a reconciliation, for an unpredictable amount of times (at least until we luckily convert to the list with a preserved order.) This change introduces two new functions, similar to the existing ones. The `toACKTagsWithKeyOrder` has the same function as `ToACKTags`, but besides returning the ACK tags, it also returns a list of keys in the same order it is in the resource. The `fromACKTagsWithKeyOrder`, also similar to `FromACKTags`, accepts the keyOrder returned by `toACKTagsWithKeyOrder`, and rebuilds the resource tags, if it's a list, in the same order as it was before. Any extra tags will be added in random order. These two new functions will be called before and after `mirrowAWSTags` and `FilterSystemTags`. The extra tags mentioned earlier are the AWS tags that are mirrored from the latest resource (more explanation on what they are [here](aws-controllers-k8s/runtime#170)) and will be filtered before getting patched with [this](aws-controllers-k8s/runtime#172) change. This will ensure that the tags in desired will maintain their order and avoid triggering unnecessary reconciliations. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #2248
Description of changes:
When we adopt ACK resources, if those resources are injected by AWS,
we are trying to avoid from patching them into the resource.
For this reason we are adding two new methods for the AWSResourceManager
interface.
MirrowAWSTags:
This method ensures the AWS tags returned by the API after a read
operation are mirrored in the desired resource. Mirroring the tags in the desired
makes them undetected by the controller.
New FilterSystemTags Function:
Removes System tags injected by AWS (cloudformation aws:)
or the controller (services.k8s.aws). This method is called during
adoption, to ensure these tags are not patched into the controller.
Also removing EnsureTag operations during adoption to avoid ACK from patching controller tags
to the resource.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.