From d5d5cd74bcb4bf137c6dea44af44135a967258fe Mon Sep 17 00:00:00 2001
From: Kubernetes Prow Robot <k8s-ci-robot@users.noreply.github.com>
Date: Sat, 23 Sep 2023 14:04:56 -0700
Subject: [PATCH] Merge pull request #4512 from
 philjb/pjb-4511-additionaltags-root-volumes

fix: propagate AdditionalTags from AWSCluster to storage volumes
---
 controllers/awsmachine_controller.go          | 19 +++++++-------
 .../awsmachine_controller_unit_test.go        | 25 ++++++++++++-------
 2 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/controllers/awsmachine_controller.go b/controllers/awsmachine_controller.go
index aad20ca383..8b716ef6c4 100644
--- a/controllers/awsmachine_controller.go
+++ b/controllers/awsmachine_controller.go
@@ -587,7 +587,7 @@ func (r *AWSMachineReconciler) knownStateTasks(_ context.Context, machineScope *
 	}
 
 	if instance != nil {
-		r.ensureStorageTags(ec2svc, instance, machineScope.AWSMachine)
+		r.ensureStorageTags(ec2svc, instance, machineScope.AWSMachine, machineScope.AdditionalTags())
 	}
 
 	if err := r.reconcileLBAttachment(machineScope, elbScope, instance); err != nil {
@@ -1042,29 +1042,28 @@ func (r *AWSMachineReconciler) indexAWSMachineByInstanceID(o client.Object) []st
 	return nil
 }
 
-func (r *AWSMachineReconciler) ensureStorageTags(ec2svc services.EC2Interface, instance *infrav1.Instance, machine *infrav1.AWSMachine) {
-	machineAnnotations, err := r.machineAnnotationJSON(machine, VolumeTagsLastAppliedAnnotation)
+func (r *AWSMachineReconciler) ensureStorageTags(ec2svc services.EC2Interface, instance *infrav1.Instance, machine *infrav1.AWSMachine, additionalTags map[string]string) {
+	annotations, err := r.machineAnnotationJSON(machine, VolumeTagsLastAppliedAnnotation)
 	if err != nil {
-
 		r.Log.Error(err, "Failed to fetch the machineAnnotations for volume tags")
 	}
 	for _, volumeID := range instance.VolumeIDs {
-		if subAnnotation, ok := machineAnnotations[volumeID].(map[string]interface{}); ok {
-			newAnnotation, err := r.ensureVolumeTags(ec2svc, aws.String(volumeID), subAnnotation, machine.Spec.AdditionalTags)
+		if subAnnotation, ok := annotations[volumeID].(map[string]interface{}); ok {
+			newAnnotation, err := r.ensureVolumeTags(ec2svc, aws.String(volumeID), subAnnotation, additionalTags)
 			if err != nil {
 				r.Log.Error(err, "Failed to fetch the changed volume tags in EC2 instance")
 			}
-			machineAnnotations[volumeID] = newAnnotation
+			annotations[volumeID] = newAnnotation
 		} else {
-			newAnnotation, err := r.ensureVolumeTags(ec2svc, aws.String(volumeID), make(map[string]interface{}), machine.Spec.AdditionalTags)
+			newAnnotation, err := r.ensureVolumeTags(ec2svc, aws.String(volumeID), make(map[string]interface{}), additionalTags)
 			if err != nil {
 				r.Log.Error(err, "Failed to fetch the changed volume tags in EC2 instance")
 			}
-			machineAnnotations[volumeID] = newAnnotation
+			annotations[volumeID] = newAnnotation
 		}
 
 		// We also need to update the annotation if anything changed.
-		err = r.updateMachineAnnotationJSON(machine, VolumeTagsLastAppliedAnnotation, machineAnnotations)
+		err = r.updateMachineAnnotationJSON(machine, VolumeTagsLastAppliedAnnotation, annotations)
 		if err != nil {
 			r.Log.Error(err, "Failed to fetch the changed volume tags in EC2 instance")
 		}
diff --git a/controllers/awsmachine_controller_unit_test.go b/controllers/awsmachine_controller_unit_test.go
index 67bf646279..5b05648566 100644
--- a/controllers/awsmachine_controller_unit_test.go
+++ b/controllers/awsmachine_controller_unit_test.go
@@ -471,7 +471,7 @@ func TestAWSMachineReconciler(t *testing.T) {
 					}
 				})
 
-				t.Run("should tag instances from machine and cluster tags", func(t *testing.T) {
+				t.Run("should tag instances and volumes with machine and cluster tags", func(t *testing.T) {
 					g := NewWithT(t)
 					awsMachine := getAWSMachine()
 					setup(t, g, awsMachine)
@@ -479,26 +479,33 @@ func TestAWSMachineReconciler(t *testing.T) {
 					instanceCreate(t, g)
 					getCoreSecurityGroups(t, g)
 
-					ms.AWSMachine.Spec.AdditionalTags = infrav1.Tags{"kind": "alicorn"}
-					cs.AWSCluster.Spec.AdditionalTags = infrav1.Tags{"colour": "lavender"}
+					ms.AWSMachine.Spec.AdditionalTags = infrav1.Tags{"kind": "alicorn", "colour": "pink"} // takes precedence
+					cs.AWSCluster.Spec.AdditionalTags = infrav1.Tags{"colour": "lavender", "shape": "round"}
 
 					ec2Svc.EXPECT().GetAdditionalSecurityGroupsIDs(gomock.Any()).Return(nil, nil)
+
+					// expect one call first to tag the instance and two calls for tagging each of two volumes
+					// the volumes get the tags from the AWSCluster _and_ the AWSMachine
+
 					ec2Svc.EXPECT().UpdateResourceTags(
-						gomock.Any(),
+						PointsTo("myMachine"),
 						map[string]string{
-							"kind": "alicorn",
+							"colour": "pink",
+							"shape":  "round",
+							"kind":   "alicorn",
 						},
 						map[string]string{},
-					).Return(nil).Times(2)
+					).Return(nil)
 
 					ec2Svc.EXPECT().UpdateResourceTags(
-						PointsTo("myMachine"),
+						gomock.Any(),
 						map[string]string{
-							"colour": "lavender",
+							"colour": "pink",
+							"shape":  "round",
 							"kind":   "alicorn",
 						},
 						map[string]string{},
-					).Return(nil)
+					).Return(nil).Times(2)
 
 					_, err := reconciler.reconcileNormal(context.Background(), ms, cs, cs, cs, cs)
 					g.Expect(err).To(BeNil())