From d5d5cd74bcb4bf137c6dea44af44135a967258fe Mon Sep 17 00:00:00 2001 From: Kubernetes Prow Robot Date: Sat, 23 Sep 2023 14:04:56 -0700 Subject: [PATCH 1/2] 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()) From bfbce146670977c07154a984b5075578c7f0dfd0 Mon Sep 17 00:00:00 2001 From: Rafael Polanco <6497491+rrpolanco@users.noreply.github.com> Date: Thu, 17 Oct 2024 09:06:01 -0500 Subject: [PATCH 2/2] fix unit test errors --- controllers/awscluster_controller_test.go | 28 ++++++++++++------- .../awscluster_controller_unit_test.go | 23 ++++++++------- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/controllers/awscluster_controller_test.go b/controllers/awscluster_controller_test.go index c60aba40ba..bbd95ff2f9 100644 --- a/controllers/awscluster_controller_test.go +++ b/controllers/awscluster_controller_test.go @@ -149,7 +149,7 @@ func TestAWSClusterReconciler_IntegrationTests(t *testing.T) { IsPublic: false, }, }) - _, err = reconciler.reconcileNormal(cs) + _, err = reconciler.reconcileNormal(ctx, cs) g.Expect(err).To(BeNil()) g.Expect(cs.VPC().ID).To(Equal("vpc-exists")) expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{ @@ -205,7 +205,7 @@ func TestAWSClusterReconciler_IntegrationTests(t *testing.T) { reconciler.networkServiceFactory = func(clusterScope scope.ClusterScope) services.NetworkInterface { return s } - _, err = reconciler.reconcileNormal(cs) + _, err = reconciler.reconcileNormal(ctx, cs) g.Expect(err.Error()).To(ContainSubstring("The maximum number of VPCs has been reached")) }) t.Run("Should successfully delete AWSCluster with managed VPC", func(t *testing.T) { @@ -282,7 +282,8 @@ func TestAWSClusterReconciler_IntegrationTests(t *testing.T) { _, err = reconciler.reconcileDelete(ctx, cs) g.Expect(err).To(BeNil()) - expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.LoadBalancerReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, clusterv1.DeletedReason}, + expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{ + {infrav1.LoadBalancerReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, clusterv1.DeletedReason}, {infrav1.BastionHostReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, clusterv1.DeletedReason}, {infrav1.SecondaryCidrsReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, clusterv1.DeletingReason}, {infrav1.RouteTablesReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, clusterv1.DeletedReason}, @@ -502,7 +503,8 @@ func mockedCreateVPCCalls(m *mocks.MockEC2APIMockRecorder) { Name: aws.String("vpc-id"), Values: aws.StringSlice([]string{"vpc-exists"}), }, - }})).Return(&ec2.DescribeSubnetsOutput{ + }, + })).Return(&ec2.DescribeSubnetsOutput{ Subnets: []*ec2.Subnet{ { VpcId: aws.String("vpc-exists"), @@ -544,7 +546,8 @@ func mockedCreateVPCCalls(m *mocks.MockEC2APIMockRecorder) { Name: aws.String("vpc-id"), Values: aws.StringSlice([]string{"vpc-exists"}), }, - }})).Return(&ec2.DescribeRouteTablesOutput{ + }, + })).Return(&ec2.DescribeRouteTablesOutput{ RouteTables: []*ec2.RouteTable{ { Routes: []*ec2.Route{ @@ -565,7 +568,8 @@ func mockedCreateVPCCalls(m *mocks.MockEC2APIMockRecorder) { Name: aws.String("state"), Values: aws.StringSlice([]string{ec2.VpcStatePending, ec2.VpcStateAvailable}), }, - }}), gomock.Any()).Return(nil) + }, + }), gomock.Any()).Return(nil) m.DescribeVpcs(gomock.Eq(&ec2.DescribeVpcsInput{ VpcIds: []*string{ aws.String("vpc-exists"), @@ -613,7 +617,8 @@ func mockedDeleteVPCCalls(m *mocks.MockEC2APIMockRecorder) { Name: aws.String("vpc-id"), Values: aws.StringSlice([]string{"vpc-exists"}), }, - }})).Return(&ec2.DescribeSubnetsOutput{ + }, + })).Return(&ec2.DescribeSubnetsOutput{ Subnets: []*ec2.Subnet{ { VpcId: aws.String("vpc-exists"), @@ -634,7 +639,8 @@ func mockedDeleteVPCCalls(m *mocks.MockEC2APIMockRecorder) { Name: aws.String("tag-key"), Values: aws.StringSlice([]string{"sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"}), }, - }})).Return(&ec2.DescribeRouteTablesOutput{ + }, + })).Return(&ec2.DescribeRouteTablesOutput{ RouteTables: []*ec2.RouteTable{ { Routes: []*ec2.Route{ @@ -681,13 +687,15 @@ func mockedDeleteVPCCalls(m *mocks.MockEC2APIMockRecorder) { Name: aws.String("state"), Values: aws.StringSlice([]string{ec2.VpcStatePending, ec2.VpcStateAvailable}), }, - }}), gomock.Any()).Return(nil).AnyTimes() + }, + }), gomock.Any()).Return(nil).AnyTimes() m.DescribeAddresses(gomock.Eq(&ec2.DescribeAddressesInput{ Filters: []*ec2.Filter{ { Name: aws.String("tag-key"), Values: aws.StringSlice([]string{"sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"}), - }}, + }, + }, })).Return(&ec2.DescribeAddressesOutput{ Addresses: []*ec2.Address{ { diff --git a/controllers/awscluster_controller_unit_test.go b/controllers/awscluster_controller_unit_test.go index e1db773596..21e11b6170 100644 --- a/controllers/awscluster_controller_unit_test.go +++ b/controllers/awscluster_controller_unit_test.go @@ -57,7 +57,8 @@ func TestAWSClusterReconciler_Reconcile(t *testing.T) { Kind: "Cluster", Name: "capi-fail-test", UID: "1", - }}}}, + }, + }}}, expectError: true, }, { @@ -241,7 +242,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) { IsPublic: false, }, }) - _, err = reconciler.reconcileNormal(cs) + _, err = reconciler.reconcileNormal(context.Background(), cs) g.Expect(err).To(BeNil()) expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.LoadBalancerReadyCondition, corev1.ConditionTrue, "", ""}}) g.Expect(awsCluster.GetFinalizers()).To(ContainElement(infrav1.ClusterFinalizer)) @@ -266,7 +267,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) { }, ) g.Expect(err).To(BeNil()) - _, err = reconciler.reconcileNormal(cs) + _, err = reconciler.reconcileNormal(context.Background(), cs) g.Expect(err).Should(Equal(expectedErr)) }) t.Run("Should fail AWSCluster create with ClusterSecurityGroupsReadyCondition status false", func(t *testing.T) { @@ -287,7 +288,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) { }, ) g.Expect(err).To(BeNil()) - _, err = reconciler.reconcileNormal(cs) + _, err = reconciler.reconcileNormal(context.Background(), cs) g.Expect(err).ToNot(BeNil()) expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.ClusterSecurityGroupsReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityWarning, infrav1.ClusterSecurityGroupReconciliationFailedReason}}) }) @@ -310,7 +311,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) { }, ) g.Expect(err).To(BeNil()) - _, err = reconciler.reconcileNormal(cs) + _, err = reconciler.reconcileNormal(context.Background(), cs) g.Expect(err).ToNot(BeNil()) expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.BastionHostReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityWarning, infrav1.BastionHostFailedReason}}) }) @@ -334,7 +335,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) { }, ) g.Expect(err).To(BeNil()) - _, err = reconciler.reconcileNormal(cs) + _, err = reconciler.reconcileNormal(context.Background(), cs) g.Expect(err).ToNot(BeNil()) expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.LoadBalancerReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityWarning, infrav1.LoadBalancerFailedReason}}) }) @@ -358,7 +359,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) { }, ) g.Expect(err).To(BeNil()) - _, err = reconciler.reconcileNormal(cs) + _, err = reconciler.reconcileNormal(context.Background(), cs) g.Expect(err).To(BeNil()) expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.LoadBalancerReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, infrav1.WaitForDNSNameReason}}) }) @@ -383,7 +384,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) { ) awsCluster.Status.Network.APIServerELB.DNSName = "test-apiserver.us-east-1.aws" g.Expect(err).To(BeNil()) - _, err = reconciler.reconcileNormal(cs) + _, err = reconciler.reconcileNormal(context.Background(), cs) g.Expect(err).To(BeNil()) expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.LoadBalancerReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, infrav1.WaitForDNSNameResolveReason}}) }) @@ -556,7 +557,8 @@ func TestAWSClusterReconciler_RequeueAWSClusterForUnpausedCluster(t *testing.T) ownerCluster: &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: "capi-test"}, Spec: clusterv1.ClusterSpec{InfrastructureRef: &corev1.ObjectReference{ APIVersion: clusterv1.GroupVersion.String(), Kind: "Cluster", - Name: "aws-test"}}}, + Name: "aws-test", + }}}, requeue: false, }, { @@ -564,7 +566,8 @@ func TestAWSClusterReconciler_RequeueAWSClusterForUnpausedCluster(t *testing.T) ownerCluster: &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: "capi-test"}, Spec: clusterv1.ClusterSpec{InfrastructureRef: &corev1.ObjectReference{ APIVersion: clusterv1.GroupVersion.String(), Kind: "AWSCluster", - Name: "aws-test"}}}, + Name: "aws-test", + }}}, requeue: false, }, }