Add support for AWS user tags from PlatformStatus

- 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]>
openshift · Oct 7, 2024 · f6bd4f6 · f6bd4f6
1 parent 721cf7c
commit f6bd4f6
Show file tree

Hide file tree

Showing 7 changed files with 1,130 additions and 37 deletions.
diff --git a/Makefile b/Makefile
@@ -120,6 +120,9 @@ help: ## Display this help.
 
 ##@ Development
 
+.PHONY: update
+update: update-vendored-crds manifests generate
+
 .PHONY: manifests
 manifests: ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
 	$(CONTROLLER_GEN) rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
@@ -130,6 +133,11 @@ manifests: ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefin
 generate: iamctl-gen iam-gen## Generate code containing DeepCopy, DeepCopyInto, DeepCopyObject method implementations and iamctl policies.
 	$(CONTROLLER_GEN) object:headerFile="hack/boilerplate.go.txt" paths="./..."
 
+.PHONY: update-vendored-crds
+update-vendored-crds:
+	## Copy infrastructure CRD from openshift/api
+	cp vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_infrastructure-Default.crd.yaml ./pkg/utils/test/crd/infrastructure-Default.yaml
+
 .PHONY: fmt
 fmt: ## Run go fmt against code.
 	go fmt -mod=vendor ./...
@@ -301,8 +309,12 @@ catalog-build: catalog
 catalog-push:
 	$(MAKE) image-push IMG=$(CATALOG_IMG)
 
+.PHONY: verify-vendored-crds
+verify-vendored-crds:
+	diff vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_infrastructure-Default.crd.yaml ./pkg/utils/test/crd/infrastructure-Default.yaml
+
 .PHONY: verify
-verify:
+verify: verify-vendored-crds
 	hack/verify-deps.sh
 	hack/verify-generated.sh
 	hack/verify-gofmt.sh

diff --git a/pkg/controllers/awsloadbalancercontroller/controller.go b/pkg/controllers/awsloadbalancercontroller/controller.go
@@ -31,6 +31,7 @@ import (
 
 	cco "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1"
 
+	configv1 "github.com/openshift/api/config/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -46,6 +47,8 @@ import (
 const (
 	// the name of the AWSLoadBalancerController resource which will be reconciled
 	controllerName = "cluster"
+	// clusterInfrastructureName is the name of the 'cluster' infrastructure object.
+	clusterInfrastructureName = "cluster"
 	// the port on which controller metrics are served
 	controllerMetricsPort = 8080
 	// the port on which the controller webhook is served
@@ -120,6 +123,15 @@ func (r *AWSLoadBalancerControllerReconciler) Reconcile(ctx context.Context, req
 		}
 	}
 
+	infraConfig := &configv1.Infrastructure{}
+	if err := r.Client.Get(ctx, types.NamespacedName{Name: clusterInfrastructureName}, infraConfig); err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to get infrastructure %q: %w", clusterInfrastructureName, err)
+	}
+	platformStatus := infraConfig.Status.PlatformStatus
+	if platformStatus == nil {
+		return ctrl.Result{}, fmt.Errorf("failed to determine infrastructure platform status: status.platformStatus is nil")
+	}
+
 	if err := r.ensureIngressClass(ctx, lbController); err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to ensure default IngressClass for AWSLoadBalancerController %q: %v", req.Name, err)
 	}
@@ -187,7 +199,7 @@ func (r *AWSLoadBalancerControllerReconciler) Reconcile(ctx context.Context, req
 		return ctrl.Result{}, fmt.Errorf("failed to ensure ClusterRole and Binding for AWSLoadBalancerController %q: %w", req.Name, err)
 	}
 
-	deployment, err := r.ensureDeployment(ctx, sa, credSecretNsName.Name, servingSecretName, lbController, trustCAConfigMap)
+	deployment, err := r.ensureDeployment(ctx, sa, credSecretNsName.Name, servingSecretName, lbController, platformStatus, trustCAConfigMap)
 	if err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to ensure Deployment for AWSLoadbalancerController %q: %w", req.Name, err)
 	}
@@ -247,17 +259,17 @@ func (r *AWSLoadBalancerControllerReconciler) BuildManagedController(mgr ctrl.Ma
 		Owns(&arv1.ValidatingWebhookConfiguration{}).
 		Owns(&arv1.MutatingWebhookConfiguration{})
 
-	if r.TrustedCAConfigMapName != "" {
-		clusterALBCInstance := func(ctx context.Context, o client.Object) []reconcile.Request {
-			return []reconcile.Request{
-				{
-					NamespacedName: types.NamespacedName{
-						Name: controllerName,
-					},
+	clusterALBCInstance := func(ctx context.Context, o client.Object) []reconcile.Request {
+		return []reconcile.Request{
+			{
+				NamespacedName: types.NamespacedName{
+					Name: controllerName,
 				},
-			}
+			},
 		}
+	}
 
+	if r.TrustedCAConfigMapName != "" {
 		// Requeue the only (cluster) instance of AWSLoadBalancerController
 		// so that the main reconciliation loop can detect the changes in the trusted CA configmap's contents
 		// and redeploy the controller if needed.
@@ -270,6 +282,12 @@ func (r *AWSLoadBalancerControllerReconciler) BuildManagedController(mgr ctrl.Ma
 				predicate.NewPredicateFuncs(inNamespace(r.Namespace))),
 				predicate.NewPredicateFuncs(hasName(r.TrustedCAConfigMapName))))
 	}
+	// Watch Infrastructure object to detect changes in AWS user tags
+	bldr = bldr.Watches(&configv1.Infrastructure{},
+		handler.EnqueueRequestsFromMapFunc(clusterALBCInstance),
+		builder.WithPredicates(
+			predicate.NewPredicateFuncs(hasName(clusterInfrastructureName))))
+
 	return bldr
 }
 

diff --git a/pkg/controllers/awsloadbalancercontroller/deployment.go b/pkg/controllers/awsloadbalancercontroller/deployment.go
@@ -21,6 +21,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
+	configv1 "github.com/openshift/api/config/v1"
+
 	albo "github.com/openshift/aws-load-balancer-operator/api/v1"
 )
 
@@ -69,7 +71,7 @@ const (
 	allCapabilities = "ALL"
 )
 
-func (r *AWSLoadBalancerControllerReconciler) ensureDeployment(ctx context.Context, sa *corev1.ServiceAccount, crSecretName, servingSecretName string, controller *albo.AWSLoadBalancerController, trustCAConfigMap *corev1.ConfigMap) (*appsv1.Deployment, error) {
+func (r *AWSLoadBalancerControllerReconciler) ensureDeployment(ctx context.Context, sa *corev1.ServiceAccount, crSecretName, servingSecretName string, controller *albo.AWSLoadBalancerController, platformStatus *configv1.PlatformStatus, trustCAConfigMap *corev1.ConfigMap) (*appsv1.Deployment, error) {
 	deploymentName := fmt.Sprintf("%s-%s", controllerResourcePrefix, controller.Name)
 
 	reqLogger := log.FromContext(ctx).WithValues("deployment", deploymentName)
@@ -90,7 +92,11 @@ func (r *AWSLoadBalancerControllerReconciler) ensureDeployment(ctx context.Conte
 		trustCAConfigMapHash = configMapHash
 	}
 
-	desired := r.desiredDeployment(deploymentName, crSecretName, servingSecretName, controller, sa, trustCAConfigMapName, trustCAConfigMapHash)
+	desired, err := r.desiredDeployment(deploymentName, crSecretName, servingSecretName, controller, platformStatus, sa, trustCAConfigMapName, trustCAConfigMapHash)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get desired deployment %s: %w", deploymentName, err)
+	}
+
 	err = controllerutil.SetControllerReference(controller, desired, r.Scheme)
 	if err != nil {
 		return nil, fmt.Errorf("failed to set owner reference on deployment %s: %w", deploymentName, err)
@@ -120,7 +126,11 @@ func (r *AWSLoadBalancerControllerReconciler) ensureDeployment(ctx context.Conte
 	return current, nil
 }
 
-func (r *AWSLoadBalancerControllerReconciler) desiredDeployment(name, credentialsRequestSecretName, servingSecret string, controller *albo.AWSLoadBalancerController, sa *corev1.ServiceAccount, trustedCAConfigMapName, trustedCAConfigMapHash string) *appsv1.Deployment {
+func (r *AWSLoadBalancerControllerReconciler) desiredDeployment(name, credentialsRequestSecretName, servingSecret string, controller *albo.AWSLoadBalancerController, platformStatus *configv1.PlatformStatus, sa *corev1.ServiceAccount, trustedCAConfigMapName, trustedCAConfigMapHash string) (*appsv1.Deployment, error) {
+	containerArgs, err := desiredContainerArgs(controller, platformStatus, r.ClusterName, r.VPCID)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get container args: %w", err)
+	}
 	d := &appsv1.Deployment{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name,
@@ -145,7 +155,7 @@ func (r *AWSLoadBalancerControllerReconciler) desiredDeployment(name, credential
 						{
 							Name:  awsLoadBalancerControllerContainerName,
 							Image: r.Image,
-							Args:  desiredContainerArgs(controller, r.ClusterName, r.VPCID),
+							Args:  containerArgs,
 							Env: append([]corev1.EnvVar{
 								{
 									Name:  awsRegionEnvVarName,
@@ -257,21 +267,23 @@ func (r *AWSLoadBalancerControllerReconciler) desiredDeployment(name, credential
 			})
 		}
 	}
-	return d
+	return d, nil
 }
 
-func desiredContainerArgs(controller *albo.AWSLoadBalancerController, clusterName, vpcID string) []string {
+func desiredContainerArgs(controller *albo.AWSLoadBalancerController, platformStatus *configv1.PlatformStatus, clusterName, vpcID string) ([]string, error) {
 	var args []string
 	args = append(args, fmt.Sprintf("--webhook-cert-dir=%s", webhookTLSDir))
 	args = append(args, fmt.Sprintf("--aws-vpc-id=%s", vpcID))
 	args = append(args, fmt.Sprintf("--cluster-name=%s", clusterName))
 
-	// if additional keys are present then sort them and append it to the arguments
-	if controller.Spec.AdditionalResourceTags != nil {
-		var tags []string
-		for _, t := range controller.Spec.AdditionalResourceTags {
-			tags = append(tags, fmt.Sprintf("%s=%s", t.Key, t.Value))
-		}
+	tags := mergeTags(controller, platformStatus)
+	// `--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 {
+		return nil, fmt.Errorf("exceeded maximum of 24 allowed tags, got %d", len(tags))
+	}
+	if len(tags) > 0 {
 		sort.Strings(tags)
 		args = append(args, fmt.Sprintf(`--default-tags=%s`, strings.Join(tags, ",")))
 	}
@@ -302,7 +314,7 @@ func desiredContainerArgs(controller *albo.AWSLoadBalancerController, clusterNam
 	args = append(args, fmt.Sprintf("--ingress-class=%s", controller.Spec.IngressClass))
 	args = append(args, "--feature-gates=EnableIPTargetType=false")
 	sort.Strings(args)
-	return args
+	return args, nil
 }
 
 func (r *AWSLoadBalancerControllerReconciler) currentDeployment(ctx context.Context, name string, namespace string) (bool, *appsv1.Deployment, error) {
@@ -542,3 +554,37 @@ func buildMapHash(data map[string]string) (string, error) {
 	}
 	return hex.EncodeToString(hash.Sum(nil)), nil
 }
+
+// mergeTags merges tags from an AWSLoadBalancerController and PlatformStatus into a slice of strings.
+// Tags from `controller.Spec.AdditionalResourceTags` take precedence over those in `platformStatus.AWS.ResourceTags`.
+// If a tag key already exists, the value from AdditionalResourceTags will overwrite the one from ResourceTags.
+func mergeTags(controller *albo.AWSLoadBalancerController, platformStatus *configv1.PlatformStatus) []string {
+	// tagMap holds tags with unique keys
+	tagMap := make(map[string]string)
+
+	// Add tags from controller.Spec.AdditionalResourceTags since operator tags has higher precedence
+	if controller.Spec.AdditionalResourceTags != nil {
+		for _, t := range controller.Spec.AdditionalResourceTags {
+			tagMap[t.Key] = t.Value
+		}
+	}
+
+	// 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 {
+				if _, exists := tagMap[t.Key]; !exists {
+					tagMap[t.Key] = t.Value
+				}
+			}
+		}
+	}
+
+	// Convert map back to a slice
+	var tags []string
+	for key, value := range tagMap {
+		tags = append(tags, fmt.Sprintf("%s=%s", key, value))
+	}
+
+	return tags
+}