Enforce aws cluster cloud provider tag in NodePool controller

Without enforcing this in the NodePool controller there's race with HC defaulting itself in the backend. The race might result in unncessary MachineDeployment rolling uprades caused by generating different awsmachinetemplates (with / without the tag). This relax the coupling.
gnufied · Jul 28, 2022 · 17a1a2a · 17a1a2a
1 parent 579d83b
commit 17a1a2a
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",