Tags support for elbv2-controller resources #42

delinak · 2025-02-28T21:15:47Z

Issue #2349

Added update logic of tags in the load_balancer resource for elbv2-controller.
Supported tags for other elbv2-controller resources: listener, rule, and target_group.

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

ack-prow · 2025-02-28T21:15:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: delinak
Once this PR has been reviewed and has the lgtm label, please assign a-hilaly for approval by writing /assign @a-hilaly in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ack-prow · 2025-02-28T21:15:57Z

Hi @delinak. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

michaelhtm

Hi @delinak,
Thanks for the contribution!! 🚀 🚀
Left a few inline comments.
Also a nit: would you be able to separate out these changes and make one PR per resource?

michaelhtm · 2025-03-03T20:58:24Z

pkg/resource/listener/sdk.go

-
 	rm.setStatusDefaults(ko)
+	if ko.Spec.Tags != nil {
+		return nil, ackrequeue.NeededAfter(fmt.Errorf("Requing due to tags in CREATE"), RequeueAfterUpdateDuration)


nit: rename RequeueAfterUpdateDuration to RequeueAfterDuration
Since it's not being specifically used after an UpdateCall

same for the other RequeueAfterUpdateDuration's

michaelhtm · 2025-03-03T20:59:35Z

pkg/resource/listener/sdk.go

+			return nil, err
+		}
+	}
+	if !delta.DifferentAt("Spec.Tags") {


Suggested change

if !delta.DifferentAt("Spec.Tags") {

if !delta.DifferentExcept("Spec.Tags") {

pkg/resource/load_balancer/hooks.go

michaelhtm · 2025-03-03T21:05:48Z

pkg/resource/load_balancer/hooks.go

+	return tags.GetResourceTags(ctx, rm.sdkapi, rm.metrics, resourceARN )
+}
+
+func (rm *resourceManager) updateTags(


where is this used?

michaelhtm · 2025-03-03T21:06:50Z

pkg/resource/tags/sync.go

+var (
+	_             = svcapitypes.Tag{}
+	_             = acktags.NewTags()
+	ACKSystemTags = []string{"services.k8s.aws/namespace", "services.k8s.aws/controller-version"}
+)


not sure if this is needed here

michaelhtm · 2025-03-03T21:11:56Z

pkg/resource/tags/sync.go

+mainLoop:
+	for _, aElement := range a {
+		visitedIndexes = append(visitedIndexes, *aElement.Key)
+		for _, bElement := range b {
+			if equalStrings(aElement.Key, bElement.Key) {
+				if !equalStrings(aElement.Value, bElement.Value) {
+					addedOrUpdated = append(addedOrUpdated, bElement)
+				}
+				continue mainLoop
+			}
+		}
+		removed = append(removed, *aElement.Key)
+	}
+	for _, bElement := range b {
+		if !ackutil.InStrings(*bElement.Key, visitedIndexes) {
+			addedOrUpdated = append(addedOrUpdated, bElement)
+		}
+	}


nit: I know we have examples in ACK that uses this method, but I have since learned that readability and maintainability is something i wanted to pay more attention to, especially in these cases.

Here's an example i try to follow, feel free to implement it, or not :)

michaelhtm · 2025-03-03T21:12:59Z

templates/hooks/listener/sdk_create_post_set_output.go.tpl

@@ -0,0 +1,3 @@
+    if desired.ko.Spec.Tags != nil {
+        return nil, ackrequeue.NeededAfter(fmt.Errorf("Requing due to tags in UPDATE"), RequeueAfterUpdateDuration)


Suggested change

return nil, ackrequeue.NeededAfter(fmt.Errorf("Requing due to tags in UPDATE"), RequeueAfterUpdateDuration)

return nil, ackrequeue.NeededAfter(fmt.Errorf("Requeuing to sync tags in UPDATE"), RequeueAfterUpdateDuration)

ditto for others below

michaelhtm · 2025-03-03T21:14:12Z

templates/hooks/listener/sdk_read_many_post_set_output.go.tpl

@@ -0,0 +1 @@
+    Spec.Tags, err = rm.getTags(ctx, string(*ko.Status.ACKResourceMetadata.ARN))


i see a diff between this and the line in listner/sdk.go..

ditto for other resources

michaelhtm · 2025-03-03T21:16:55Z

templates/hooks/target_group/sdk_update_pre_build_request.go.tpl

+	if delta.DifferentAt("Spec.Priority") {
+		if err = rm.setRulePriority(ctx, desired); err != nil {
+			return nil, err
+		}
+	} else if !delta.DifferentExcept("Spec.Priority") {
+		return desired, nil


are we also supporting Priority updates?

michaelhtm · 2025-03-03T21:19:06Z

templates/hooks/load_balancer/sdk_update_pre_build_request.go.tpl

+    if delta.DifferentAt("Spec.Tags") {
+        err = rm.updateTags(ctx, desired, latest)
+        if err != nil {
+            return nil, err
+        }
+    }
+    if !delta.DifferentAt("Spec.Tags") {
+        return desired, nil
+    }


seems like this is not needed since it's written in hooks.go

ack-bot · 2025-08-30T21:48:02Z

Issues go stale after 180d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 60d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

support read and update tags for elbv2 resources

023d958

ack-prow bot requested review from a-hilaly and michaelhtm February 28, 2025 21:15

ack-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 28, 2025

michaelhtm reviewed Mar 3, 2025

View reviewed changes

ack-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 30, 2025

	if !delta.DifferentAt("Spec.Tags") {
	if !delta.DifferentExcept("Spec.Tags") {

		@@ -0,0 +1,3 @@
		if desired.ko.Spec.Tags != nil {
		return nil, ackrequeue.NeededAfter(fmt.Errorf("Requing due to tags in UPDATE"), RequeueAfterUpdateDuration)

		@@ -0,0 +1 @@
		Spec.Tags, err = rm.getTags(ctx, string(*ko.Status.ACKResourceMetadata.ARN)) No newline at end of file

Tags support for elbv2-controller resources #42

Are you sure you want to change the base?

Tags support for elbv2-controller resources #42

Uh oh!

Conversation

delinak commented Feb 28, 2025

Uh oh!

ack-prow bot commented Feb 28, 2025

Uh oh!

ack-prow bot commented Feb 28, 2025

Uh oh!

michaelhtm left a comment

Choose a reason for hiding this comment

Uh oh!

michaelhtm Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ack-bot commented Aug 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michaelhtm Mar 3, 2025 •

edited

Loading