Merge pull request openshift#1625 from enxebre/nodepool-aws-tags

Enforce aws cluster cloud provider tag in NodePool controller
gnufied · Jul 29, 2022 · d74698b · d74698b
2 parents e567cca + 17a1a2a
commit d74698b
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 13 deletions.
diff --git a/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
@@ -4247,6 +4247,9 @@ func (r *HostedClusterReconciler) reconcileAWSResourceTags(ctx context.Context,
 	if hcluster.Spec.Platform.AWS == nil {
 		return nil
 	}
+	if hcluster.Spec.InfraID == "" {
+		return nil
+	}
 
 	var existing *hyperv1.AWSResourceTag
 	for idx, tag := range hcluster.Spec.Platform.AWS.ResourceTags {

diff --git a/hypershift-operator/controllers/nodepool/aws.go b/hypershift-operator/controllers/nodepool/aws.go
@@ -8,6 +8,18 @@ import (
 	capiaws "sigs.k8s.io/cluster-api-provider-aws/api/v1beta1"
 )
 
+const (
+	// infraLifecycleOwned is the value we use when tagging infra resources to indicate
+	// that the resource is considered owned and managed by the cluster.
+	infraLifecycleOwned = "owned"
+)
+
+// awsClusterCloudProviderTagKey generates the key for infra resources associated to a cluster.
+// https://github.com/kubernetes/cloud-provider-aws/blob/5f394ba297bf280ceb3edfc38922630b4bd83f46/pkg/providers/v2/tags.go#L31-L37
+func awsClusterCloudProviderTagKey(id string) string {
+	return fmt.Sprintf("kubernetes.io/cluster/%s", id)
+}
+
 func awsMachineTemplateSpec(infraName, ami string, hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool) *capiaws.AWSMachineTemplateSpec {
 	subnet := &capiaws.AWSResourceReference{}
 	if nodePool.Spec.Platform.AWS.Subnet != nil {
@@ -61,14 +73,18 @@ func awsMachineTemplateSpec(infraName, ami string, hostedCluster *hyperv1.Hosted
 
 	instanceType := nodePool.Spec.Platform.AWS.InstanceType
 
-	var tags capiaws.Tags
+	tags := capiaws.Tags{}
 	for _, tag := range append(nodePool.Spec.Platform.AWS.ResourceTags, hostedCluster.Spec.Platform.AWS.ResourceTags...) {
-		if tags == nil {
-			tags = capiaws.Tags{}
-		}
 		tags[tag.Key] = tag.Value
 	}
 
+	// We enforce the AWS cluster cloud provider tag here.
+	// Otherwise, this would race with the HC defaulting itself hostedCluster.Spec.Platform.AWS.ResourceTags.
+	key := awsClusterCloudProviderTagKey(infraName)
+	if _, ok := tags[key]; !ok {
+		tags[key] = infraLifecycleOwned
+	}
+
 	awsMachineTemplateSpec := &capiaws.AWSMachineTemplateSpec{
 		Template: capiaws.AWSMachineTemplateResource{
 			Spec: capiaws.AWSMachineSpec{

diff --git a/hypershift-operator/controllers/nodepool/aws_test.go b/hypershift-operator/controllers/nodepool/aws_test.go
@@ -11,6 +11,7 @@ import (
 )
 
 const amiName = "ami"
+const infraName = "test"
 
 var volume = hyperv1.Volume{
 	Size: 16,
@@ -19,6 +20,7 @@ var volume = hyperv1.Volume{
 }
 
 func TestAWSMachineTemplate(t *testing.T) {
+	infraName := "test"
 	testCases := []struct {
 		name     string
 		cluster  hyperv1.HostedClusterSpec
@@ -53,7 +55,7 @@ func TestAWSMachineTemplate(t *testing.T) {
 			}}},
 
 			expected: defaultAWSMachineTemplate(func(tmpl *capiaws.AWSMachineTemplate) {
-				tmpl.Spec.Template.Spec.AdditionalTags = capiaws.Tags{"key": "value"}
+				tmpl.Spec.Template.Spec.AdditionalTags["key"] = "value"
 			}),
 		},
 		{
@@ -65,7 +67,7 @@ func TestAWSMachineTemplate(t *testing.T) {
 			}}},
 
 			expected: defaultAWSMachineTemplate(func(tmpl *capiaws.AWSMachineTemplate) {
-				tmpl.Spec.Template.Spec.AdditionalTags = capiaws.Tags{"key": "value"}
+				tmpl.Spec.Template.Spec.AdditionalTags["key"] = "value"
 			}),
 		},
 		{
@@ -84,11 +86,9 @@ func TestAWSMachineTemplate(t *testing.T) {
 			}}},
 
 			expected: defaultAWSMachineTemplate(func(tmpl *capiaws.AWSMachineTemplate) {
-				tmpl.Spec.Template.Spec.AdditionalTags = capiaws.Tags{
-					"cluster-only":         "value",
-					"cluster-and-nodepool": "cluster",
-					"nodepool-only":        "value",
-				}
+				tmpl.Spec.Template.Spec.AdditionalTags["cluster-only"] = "value"
+				tmpl.Spec.Template.Spec.AdditionalTags["cluster-and-nodepool"] = "cluster"
+				tmpl.Spec.Template.Spec.AdditionalTags["nodepool-only"] = "value"
 			}),
 		},
 	}
@@ -100,7 +100,7 @@ func TestAWSMachineTemplate(t *testing.T) {
 			if tc.nodePool.Platform.AWS == nil {
 				tc.nodePool.Platform.AWS = &hyperv1.AWSNodePoolPlatform{}
 			}
-			result := awsMachineTemplateSpec("testi", amiName, &hyperv1.HostedCluster{Spec: tc.cluster}, &hyperv1.NodePool{Spec: tc.nodePool})
+			result := awsMachineTemplateSpec(infraName, amiName, &hyperv1.HostedCluster{Spec: tc.cluster}, &hyperv1.NodePool{Spec: tc.nodePool})
 			if !equality.Semantic.DeepEqual(&tc.expected.Spec, result) {
 				t.Errorf(cmp.Diff(tc.expected.Spec, result))
 			}
@@ -126,7 +126,10 @@ func defaultAWSMachineTemplate(modify ...func(*capiaws.AWSMachineTemplate)) *cap
 					AMI: capiaws.AMIReference{
 						ID: k8sutilspointer.StringPtr(amiName),
 					},
-					IAMInstanceProfile:       "testi-worker-profile",
+					AdditionalTags: capiaws.Tags{
+						awsClusterCloudProviderTagKey(infraName): infraLifecycleOwned,
+					},
+					IAMInstanceProfile:       infraName + "-worker-profile",
 					AdditionalSecurityGroups: []capiaws.AWSResourceReference{},
 					Subnet:                   &capiaws.AWSResourceReference{},
 					UncompressedUserData:     k8sutilspointer.BoolPtr(true),

diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller_test.go b/hypershift-operator/controllers/nodepool/nodepool_controller_test.go
@@ -1154,6 +1154,9 @@ func RunTestMachineTemplateBuilders(t *testing.T, preCreateMachineTemplate bool)
 						InsecureSkipSecretsManager: true,
 						SecureSecretsBackend:       "secrets-manager",
 					},
+					AdditionalTags: capiaws.Tags{
+						awsClusterCloudProviderTagKey(infraID): infraLifecycleOwned,
+					},
 					RootVolume: &capiaws.Volume{
 						Size: 16,
 						Type: "io1",