From f528a7586d90af237e820b5210aeb935b9ea0544 Mon Sep 17 00:00:00 2001 From: Phil Bracikowski Date: Thu, 21 Sep 2023 16:18:50 -0700 Subject: [PATCH 1/3] fix: propagate AdditionalTags from AWSCluster to storage volumes * see kubernetes-sigs/cluster-api-provider-aws#4511 --- controllers/awsmachine_controller.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/controllers/awsmachine_controller.go b/controllers/awsmachine_controller.go index 7629552240..9f84621c98 100644 --- a/controllers/awsmachine_controller.go +++ b/controllers/awsmachine_controller.go @@ -589,7 +589,7 @@ func (r *AWSMachineReconciler) reconcileNormal(_ 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 { @@ -1111,20 +1111,20 @@ func (r *AWSMachineReconciler) indexAWSMachineByInstanceID(o client.Object) []st return nil } -func (r *AWSMachineReconciler) ensureStorageTags(ec2svc services.EC2Interface, instance *infrav1.Instance, machine *infrav1.AWSMachine) { +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 annotations for volume tags") } for _, volumeID := range instance.VolumeIDs { if subAnnotation, ok := annotations[volumeID].(map[string]interface{}); ok { - newAnnotation, err := r.ensureVolumeTags(ec2svc, aws.String(volumeID), subAnnotation, machine.Spec.AdditionalTags) + 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") } 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") } From 577bcda9a354b1f4c97640c761b5809cb8e7c9fa Mon Sep 17 00:00:00 2001 From: Phil Bracikowski Date: Fri, 22 Sep 2023 10:05:27 -0700 Subject: [PATCH 2/3] tests(awsmachine controller): fix test that matched bug behavior --- .../awsmachine_controller_unit_test.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/controllers/awsmachine_controller_unit_test.go b/controllers/awsmachine_controller_unit_test.go index 471f016247..f1e2205df7 100644 --- a/controllers/awsmachine_controller_unit_test.go +++ b/controllers/awsmachine_controller_unit_test.go @@ -481,7 +481,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) @@ -493,22 +493,29 @@ func TestAWSMachineReconciler(t *testing.T) { cs.AWSCluster.Spec.AdditionalTags = infrav1.Tags{"colour": "lavender"} 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": "lavender", + "kind": "alicorn", }, map[string]string{}, - ).Return(nil).Times(2) + ).Return(nil) ec2Svc.EXPECT().UpdateResourceTags( - PointsTo("myMachine"), + gomock.Any(), + //create map[string]string{ "colour": "lavender", "kind": "alicorn", }, + //remove map[string]string{}, - ).Return(nil) + ).Return(nil).Times(2) _, err := reconciler.reconcileNormal(context.Background(), ms, cs, cs, cs, cs) g.Expect(err).To(BeNil()) From b805082d60433e267cc6bbf93a4882224d8c4a40 Mon Sep 17 00:00:00 2001 From: Phil Bracikowski Date: Fri, 22 Sep 2023 10:21:24 -0700 Subject: [PATCH 3/3] tests(awsmachine controller): improve test for precedence of tags --- controllers/awsmachine_controller_unit_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/controllers/awsmachine_controller_unit_test.go b/controllers/awsmachine_controller_unit_test.go index f1e2205df7..cb953bcd72 100644 --- a/controllers/awsmachine_controller_unit_test.go +++ b/controllers/awsmachine_controller_unit_test.go @@ -489,8 +489,8 @@ 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) @@ -500,7 +500,8 @@ func TestAWSMachineReconciler(t *testing.T) { ec2Svc.EXPECT().UpdateResourceTags( PointsTo("myMachine"), map[string]string{ - "colour": "lavender", + "colour": "pink", + "shape": "round", "kind": "alicorn", }, map[string]string{}, @@ -508,12 +509,11 @@ func TestAWSMachineReconciler(t *testing.T) { ec2Svc.EXPECT().UpdateResourceTags( gomock.Any(), - //create map[string]string{ - "colour": "lavender", + "colour": "pink", + "shape": "round", "kind": "alicorn", }, - //remove map[string]string{}, ).Return(nil).Times(2)