CFE-1133: Watch Infrastructure and update AWS tags #137

chiragkyal · 2024-09-24T08:12:51Z

This PR adds support for AWS user tags specified in the platform status. Tags can be managed centrally in the PlatformStatus and have them automatically applied to resources managed by the ALBC.

Adds a watch on the Infrastructure object to detect changes in the platformStatus.AWS.ResourceTags field.
Merges tags from both the AWSLoadBalancerController.Spec.AdditionalResourceTags and the PlatformStatus.AWS.ResourceTags into a single list, prioritizing tags from the operator spec.
Ensures that the combined tag list does not exceed the maximum allowed limit of 24 tags for the ALBC's --default-tags argument.

Implements CFE-1133

openshift-ci-robot · 2024-09-24T08:12:54Z

@chiragkyal: This pull request references CFE-1133 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

chiragkyal · 2024-09-26T08:40:56Z

/test images

chiragkyal · 2024-09-26T08:51:14Z

/retest-required

chiragkyal · 2024-09-30T09:12:51Z

/test all

openshift-ci-robot · 2024-09-30T11:03:13Z

@chiragkyal: This pull request references CFE-1133 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

This PR adds support for AWS user tags specified in the platform status. Tags can be managed centrally in the PlatformStatus and have them automatically applied to resources managed by the ALBC.

Adds a watch on the Infrastructure object to detect changes in the platformStatus.AWS.ResourceTags field.

Merges tags from both the AWSLoadBalancerController.Spec.AdditionalResourceTags and the PlatformStatus.AWS.ResourceTags into a single list, prioritizing tags from the operator spec.

Ensures that the combined tag list does not exceed the maximum allowed limit of 24 tags for the ALBC's --default-tags argument.

Implements CFE-1133

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 openshift-eng/jira-lifecycle-plugin repository.

chiragkyal · 2024-10-01T14:23:04Z

/retest-required

pkg/controllers/awsloadbalancercontroller/controller.go

- Added watch on Infrastructure object to detect changes in PlatformStatus.AWS.ResourceTags. - Merged tags from AWSLoadBalancerController.Spec.AdditionalResourceTags and PlatformStatus.AWS.ResourceTags, prioritizing operator spec tags. Signed-off-by: chiragkyal <[email protected]>

chiragkyal · 2024-10-07T11:49:15Z

/retest

chiragkyal · 2024-10-17T12:07:30Z

/assign @alebedev87

chiragkyal · 2024-10-17T18:24:06Z

/retest

openshift-ci · 2024-11-03T12:44:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from alebedev87. 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

chiragkyal · 2024-11-04T06:23:34Z

/retest-required

chiragkyal · 2024-11-05T08:32:23Z

/retest-required

chiragkyal · 2024-11-05T13:43:45Z

/retest-required

chiragkyal · 2024-11-06T05:51:07Z

/retest-required

Signed-off-by: chiragkyal <[email protected]>

chiragkyal · 2024-11-10T12:16:54Z

/retest-required

chiragkyal · 2024-11-11T08:25:45Z

test/e2e/operator_test.go

+	managed, err := isManagedServiceCluster(context.TODO(), kubeClientSet)
+	if err != nil {
+		t.Fatalf("failed to check managed cluster %v", err)
+	}
+	if managed {
+		t.Skip("Infrastructure status cannot be directly updated on managed cluster, skipping...")
+	}


Infrastructure status cannot be directly updated on ROSA clusters. Hence skipping the test.

chiragkyal · 2024-11-11T08:29:36Z

test/e2e/operator_test.go

+func isManagedServiceCluster(ctx context.Context, adminClient kubernetes.Interface) (bool, error) {
+	_, err := adminClient.CoreV1().Namespaces().Get(ctx, "openshift-backplane", v1.GetOptions{})
+	if err == nil {
+		return true, nil
+	}
+
+	if !errors.IsNotFound(err) {
+		return false, err
+	}
+
+	return false, nil
+}


This check is inspired by https://github.com/openshift/origin/pull/29216/files#diff-35a89a7a7362642eebb559fb8564e857b00d6f7dd6322c3adabaf1adbd609d35R2267-R2278 implementation.
xRef: Slack discussion

Can you please add this link as a comment so that it would be easier to track this later?

Sure, added the PR link as code comment

alebedev87 · 2024-11-14T13:21:12Z

pkg/controllers/awsloadbalancercontroller/deployment.go

 }

-func desiredContainerArgs(controller *albo.AWSLoadBalancerController, clusterName, vpcID string) []string {
+func desiredContainerArgs(controller *albo.AWSLoadBalancerController, platformStatus *configv1.PlatformStatus, clusterName, vpcID string) ([]string, error) {


Cluster name and vpcID are still more fundamental inputs for the flags than the resource tags:

Suggested change

func desiredContainerArgs(controller *albo.AWSLoadBalancerController, platformStatus *configv1.PlatformStatus, clusterName, vpcID string) ([]string, error) {

func desiredContainerArgs(controller *albo.AWSLoadBalancerController, clusterName, vpcID string, platformStatus *configv1.PlatformStatus) ([]string, error) {

Sounds good! Updated

alebedev87 · 2024-11-14T13:37:39Z

pkg/controllers/awsloadbalancercontroller/controller.go

+	platformStatus := infraConfig.Status.PlatformStatus
+	if platformStatus == nil {
+		return ctrl.Result{}, fmt.Errorf("failed to determine infrastructure platform status: status.platformStatus is nil")
+	}


Can the platform status ever be nil? If it can, I don't think we have to block the reconciliation - we just don't add platform tags in that case.

platform status should not be nil for clusters deployed on AWS. We need the AWS region as well from the platform status for the operator to function correctly.

aws-load-balancer-operator/main.go

Lines 230 to 233 in 59f0616

if infra.Status.PlatformStatus == nil || infra.Status.PlatformStatus.AWS == nil || infra.Status.PlatformStatus.AWS.Region == "" {

err = fmt.Errorf("could not get AWS region from Infrastructure %q status", clusterInfrastructureName)

return

}

So, I think we should block the reconciliation if it is nil

We need the AWS region as well from the platform status for the operator to function correctly.

Right, that's what we ensure at the startup of the operator. The region is fundamental for the operator, without it the operator cannot function. Unlike resource tags (platform or additional).

I don't see why a cluster may not have any platform resource tags. So, I consider platformStatus==nil as the same use case as "customer didn't set any resource tags during installation".

alebedev87 · 2024-11-14T13:53:25Z

pkg/controllers/awsloadbalancercontroller/deployment.go

+	// `--default-tags` arg allows a maximum of 24 user tags, but the combination of
+	// controller.Spec.AdditionalResourceTags and platformStatus.AWS.ResourceTags can result in more.
+	// Ensure a maximum of 24 merged tags are added.
+	if len(tags) > 24 {


Can this be a constant? And can you please add some links which confirm this number. AWS limitation is 50 tags per resource so I don't quite get where 24 is coming from.

24 is coming from the validation added for the AdditionalResourceTags field.

aws-load-balancer-operator/api/v1/awsloadbalancercontroller_types.go

Lines 61 to 77 in 59f0616

// additionalResourceTags are the AWS tags that will be applied to all AWS resources managed by this

// controller. The managed AWS resources don't include the cluster subnets which are tagged by the operator.

// The addition of new tags as well as the update or removal of any existing tags

// will be propagated to the AWS resources. The controller owns all the tags of the managed AWS resources,

// unsolicited tags are removed. The controller doesn't watch for changes on AWS, so the removal of the unsolicited

// tags can only be triggered by an event coming from OpenShift. AWS supports a maximum of 50 tags per resource.

// AWSLoadBalancerController reserves 3 tags for its use, the rest is split between the tag annotation

// which can be set on the ingress and this field: 23 and 24, respectively. Each tag key must be unique.

//

// +kubebuilder:validation:MaxItems=24

// +kubebuilder:validation:Optional

// +optional

// +listType=map

// +listMapKey=key

// +patchMergeKey=key

// +patchStrategy=merge

AdditionalResourceTags []AWSResourceTag `json:"additionalResourceTags,omitempty" patchStrategy:"merge" patchMergeKey:"key"`

Since we are now merging these tags with the Infrastructure tags, isn't this number still valid?

alebedev87 · 2024-11-14T14:16:53Z

pkg/controllers/awsloadbalancercontroller/deployment.go

+	// Add tags from platformStatus.AWS.ResourceTags to the map, only if the key doesn't exist
+	if platformStatus.AWS != nil && len(platformStatus.AWS.ResourceTags) > 0 {
+		for _, t := range platformStatus.AWS.ResourceTags {
+			if len(t.Key) > 0 {


platformStatus.AWS.ResourceTags.Key has OpenAPI schema validations which exclude empty value:

// +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=128 // +kubebuilder:validation:Pattern=`^[0-9A-Za-z_.:/=+-@]+$`

So, this is redundant.

Thanks for the catch. Removed the if block.

alebedev87 · 2024-11-14T14:23:16Z

pkg/controllers/awsloadbalancercontroller/deployment_test.go

+			if tc.platformStatus == nil {
+				tc.platformStatus = &configv1.PlatformStatus{}
+			}


I think we should handle the nil case inside the function. That is, if platformStatus is nil we just don't add platform tags.

Due to #137 (comment) reason, I think having nil platformStatus won't even start the operator.

alebedev87 · 2024-11-14T14:24:42Z

pkg/controllers/awsloadbalancercontroller/deployment_test.go

@@ -854,11 +962,15 @@ func TestEnsureDeploymentEnvVars(t *testing.T) {
 				VPCID:       "test-vpc",
 				AWSRegion:   testAWSRegion,
 			}
-			_, err := r.ensureDeployment(context.Background(), tc.serviceAccount, "test-credentials", "test-serving", tc.controller, nil)
+			_, err := r.ensureDeployment(context.Background(), tc.serviceAccount, "test-credentials", "test-serving", tc.controller, &configv1.PlatformStatus{}, nil)


Same comment about nil value being handled inside the function as other args of this function.

Due to the same reason, I think we won't even reach upto ensureDeployment if platformStatus is nil.

alebedev87 · 2024-11-14T15:13:07Z

test/e2e/operator_test.go

+	alb.Spec.AdditionalResourceTags = []albo.AWSResourceTag{
+		{Key: "op-key1", Value: "op-value1"},
+		{Key: "conflict-key1", Value: "op-value2"},
+		{Key: "conflict-key2", Value: "op-value3"},
+	}


Can you add this to albcBuilder? Something like withTags(map[string]string).

I just now observed that there is already a similar function: withResourceTags(map[string]string). I've updated it to use this function instead. Thanks for the suggestion.

alebedev87 · 2024-11-14T15:23:23Z

test/e2e/operator_test.go

+
+	// Check `--default-tags` arg for tags present in alb instance and infra status (initialInfraTags)
+	expectedTagValue := "conflict-key1=op-value2,conflict-key2=op-value3,op-key1=op-value1,plat-key1=plat-value1"
+	assertContainerArgFromDeployment(t, dep, awsLoadBalancerControllerContainerName, "--default-tags", expectedTagValue)


This test is not quite different from the unit tests for desired deployment. We already assure the deployment gets the right flag values. I think the e2e test should check the the result of ALBC's job - load balancers. You can use https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2 to check the infrastructure tags are propagated to a load balancer.

I was thinking differently. The task of the ALBO is to set the flags correctly, next it's the job of ALBC to consume those flags and take necessary actions (adding tags in this case). If we can validate that the --default-tags flag is getting populated correctly, I hope we can trust ALBC to propagate them to a load balancer.

I would expect ALBC's e2e to query the tags using https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2

In case of CIO also, we did a similar check. We're checking whether the operator has correctly set the annotations or not. https://github.com/openshift/cluster-ingress-operator/pull/1148/files#diff-cf4b4e5424070a666a364d1d1b04011478d888d6d3d65c943ca7783333b7a6e4R1356-R1358

The task of the ALBO is to set the flags correctly, next it's the job of ALBC to consume those flags and take necessary actions (adding tags in this case). If we can validate that the --default-tags flag is getting populated correctly, I hope we can trust ALBC to propagate them to a load balancer.

We don't have much control over what's validated upstream. The e2e tests for our addon operators verify use cases we think are nominal for our customers. And the idea here is to go from one end (API) to the other end (load balancers). That's why all the other e2e tests send real traffic to check the load balancers are in the shape we expect them to be. The validation of the expected values for --default-tags flag is what a unit test can do even without envtest, a fake Kube client would be enough.

I would expect ALBC's e2e to query the tags using https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2

I would expect too. But I don't think we know this for sure.

In case of CIO also, we did a similar check. We're checking whether the operator has correctly set the annotations or not. https://github.com/openshift/cluster-ingress-operator/pull/1148/files#diff-cf4b4e5424070a666a364d1d1b04011478d888d6d3d65c943ca7783333b7a6e4R1356-R1358

C-I-O case is different. The creation of load balancers for Services is out of control of C-I-O, it's not done by anything C-I-O manages or being responsible for.

Frankly speaking, I would be ok if this feature would come with a set of unit tests only. However since you already into the e2e stage, we should be consistent with the rest of the tests and go to very end (AWS).

alebedev87 · 2024-11-14T15:24:24Z

test/e2e/operator_test.go

+func isManagedServiceCluster(ctx context.Context, adminClient kubernetes.Interface) (bool, error) {
+	_, err := adminClient.CoreV1().Namespaces().Get(ctx, "openshift-backplane", v1.GetOptions{})
+	if err == nil {
+		return true, nil
+	}
+
+	if !errors.IsNotFound(err) {
+		return false, err
+	}
+
+	return false, nil
+}


Can you please add this link as a comment so that it would be easier to track this later?

Signed-off-by: chiragkyal <[email protected]>

openshift-ci · 2024-11-15T15:05:11Z

@chiragkyal: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ci-index-aws-load-balancer-operator-bundle	`e9077da`	link	true	`/test ci-index-aws-load-balancer-operator-bundle`

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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 24, 2024

openshift-ci bot requested review from alebedev87 and miheer September 24, 2024 08:13

chiragkyal force-pushed the aws-tags branch 3 times, most recently from 3d5641f to f413d74 Compare September 25, 2024 09:29

chiragkyal force-pushed the aws-tags branch from afad1c2 to deb501d Compare September 30, 2024 10:55

chiragkyal changed the title ~~CFE-1133: [WIP] Watch infrastructure and update AWS tags~~ CFE-1133: Watch Infrastructure and update AWS tags Sep 30, 2024

chiragkyal force-pushed the aws-tags branch 2 times, most recently from c177a9c to 48c649a Compare October 1, 2024 06:34

Miciah reviewed Oct 3, 2024

View reviewed changes

pkg/controllers/awsloadbalancercontroller/controller.go Outdated Show resolved Hide resolved

Miciah reviewed Oct 3, 2024

View reviewed changes

pkg/controllers/awsloadbalancercontroller/controller.go Outdated Show resolved Hide resolved

chiragkyal force-pushed the aws-tags branch from 48c649a to f6bd4f6 Compare October 7, 2024 08:31

openshift-ci bot assigned alebedev87 Oct 17, 2024

chiragkyal mentioned this pull request Oct 21, 2024

CFE-1162: Updates enhancement to reflect hcp usecase and with latest information on aws tags support openshift/enhancements#1700

Open

chiragkyal force-pushed the aws-tags branch from 1879fb2 to a9aea28 Compare November 3, 2024 12:44

chiragkyal force-pushed the aws-tags branch 2 times, most recently from 54edf38 to 650dd1b Compare November 5, 2024 08:19

chiragkyal force-pushed the aws-tags branch from 650dd1b to 17289ec Compare November 5, 2024 11:40

add e2e test

2712b01

Signed-off-by: chiragkyal <[email protected]>

chiragkyal force-pushed the aws-tags branch from 17289ec to 2712b01 Compare November 8, 2024 17:26

chiragkyal commented Nov 11, 2024

View reviewed changes

alebedev87 reviewed Nov 14, 2024

View reviewed changes

Address review comments

e9077da

Signed-off-by: chiragkyal <[email protected]>

chiragkyal requested a review from alebedev87 November 15, 2024 12:12

	func desiredContainerArgs(controller albo.AWSLoadBalancerController, platformStatus configv1.PlatformStatus, clusterName, vpcID string) ([]string, error) {
	func desiredContainerArgs(controller albo.AWSLoadBalancerController, clusterName, vpcID string, platformStatus configv1.PlatformStatus) ([]string, error) {

	if infra.Status.PlatformStatus == nil \|\| infra.Status.PlatformStatus.AWS == nil \|\| infra.Status.PlatformStatus.AWS.Region == "" {
	err = fmt.Errorf("could not get AWS region from Infrastructure %q status", clusterInfrastructureName)
	return
	}

	// additionalResourceTags are the AWS tags that will be applied to all AWS resources managed by this
	// controller. The managed AWS resources don't include the cluster subnets which are tagged by the operator.
	// The addition of new tags as well as the update or removal of any existing tags
	// will be propagated to the AWS resources. The controller owns all the tags of the managed AWS resources,
	// unsolicited tags are removed. The controller doesn't watch for changes on AWS, so the removal of the unsolicited
	// tags can only be triggered by an event coming from OpenShift. AWS supports a maximum of 50 tags per resource.
	// AWSLoadBalancerController reserves 3 tags for its use, the rest is split between the tag annotation
	// which can be set on the ingress and this field: 23 and 24, respectively. Each tag key must be unique.
	//
	// +kubebuilder:validation:MaxItems=24
	// +kubebuilder:validation:Optional
	// +optional
	// +listType=map
	// +listMapKey=key
	// +patchMergeKey=key
	// +patchStrategy=merge
	AdditionalResourceTags []AWSResourceTag `json:"additionalResourceTags,omitempty" patchStrategy:"merge" patchMergeKey:"key"`

CFE-1133: Watch Infrastructure and update AWS tags #137

Are you sure you want to change the base?

CFE-1133: Watch Infrastructure and update AWS tags #137

Conversation

chiragkyal commented Sep 24, 2024 • edited Loading

openshift-ci-robot commented Sep 24, 2024 • edited by openshift-ci bot Loading

chiragkyal commented Sep 26, 2024

chiragkyal commented Sep 26, 2024

chiragkyal commented Sep 30, 2024

openshift-ci-robot commented Sep 30, 2024 • edited by openshift-ci bot Loading

chiragkyal commented Oct 1, 2024

chiragkyal commented Oct 7, 2024

chiragkyal commented Oct 17, 2024

chiragkyal commented Oct 17, 2024

openshift-ci bot commented Nov 3, 2024

chiragkyal commented Nov 4, 2024

chiragkyal commented Nov 5, 2024

chiragkyal commented Nov 5, 2024

chiragkyal commented Nov 6, 2024

chiragkyal commented Nov 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiragkyal Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

alebedev87 Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiragkyal Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alebedev87 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiragkyal Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 15, 2024

chiragkyal commented Sep 24, 2024 •

edited

Loading

openshift-ci-robot commented Sep 24, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Sep 30, 2024 •

edited by openshift-ci bot

Loading

chiragkyal Nov 15, 2024 •

edited

Loading

alebedev87 Nov 15, 2024 •

edited

Loading

chiragkyal Nov 15, 2024 •

edited

Loading

alebedev87 Nov 14, 2024 •

edited

Loading

chiragkyal Nov 15, 2024 •

edited

Loading