Revert "Merge pull request #905 from spectrocloud/pcp-3592"

This reverts commit 8010300, reversing changes made to 93b0586.
spectrocloud · Oct 21, 2024 · 0d500f0 · 0d500f0
1 parent 8010300
commit 0d500f0
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 56 deletions.
diff --git 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(ctx, cs)
+		_, err = reconciler.reconcileNormal(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(ctx, cs)
+		_, err = reconciler.reconcileNormal(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,8 +282,7 @@ 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},
@@ -503,8 +502,7 @@ 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"),
@@ -546,8 +544,7 @@ 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{
@@ -568,8 +565,7 @@ 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"),
@@ -617,8 +613,7 @@ 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"),
@@ -639,8 +634,7 @@ 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{
@@ -687,15 +681,13 @@ 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
@@ -57,8 +57,7 @@ func TestAWSClusterReconciler_Reconcile(t *testing.T) {
 					Kind:       "Cluster",
 					Name:       "capi-fail-test",
 					UID:        "1",
-				},
-			}}},
+				}}}},
 			expectError: true,
 		},
 		{
@@ -242,7 +241,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 						IsPublic:         false,
 					},
 				})
-				_, err = reconciler.reconcileNormal(context.Background(), cs)
+				_, err = reconciler.reconcileNormal(cs)
 				g.Expect(err).To(BeNil())
 				expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.LoadBalancerReadyCondition, corev1.ConditionTrue, "", ""}})
 				g.Expect(awsCluster.GetFinalizers()).To(ContainElement(infrav1.ClusterFinalizer))
@@ -267,7 +266,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				_, err = reconciler.reconcileNormal(context.Background(), cs)
+				_, err = reconciler.reconcileNormal(cs)
 				g.Expect(err).Should(Equal(expectedErr))
 			})
 			t.Run("Should fail AWSCluster create with ClusterSecurityGroupsReadyCondition status false", func(t *testing.T) {
@@ -288,7 +287,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				_, err = reconciler.reconcileNormal(context.Background(), cs)
+				_, err = reconciler.reconcileNormal(cs)
 				g.Expect(err).ToNot(BeNil())
 				expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.ClusterSecurityGroupsReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityWarning, infrav1.ClusterSecurityGroupReconciliationFailedReason}})
 			})
@@ -311,7 +310,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				_, err = reconciler.reconcileNormal(context.Background(), cs)
+				_, err = reconciler.reconcileNormal(cs)
 				g.Expect(err).ToNot(BeNil())
 				expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.BastionHostReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityWarning, infrav1.BastionHostFailedReason}})
 			})
@@ -335,7 +334,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				_, err = reconciler.reconcileNormal(context.Background(), cs)
+				_, err = reconciler.reconcileNormal(cs)
 				g.Expect(err).ToNot(BeNil())
 				expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.LoadBalancerReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityWarning, infrav1.LoadBalancerFailedReason}})
 			})
@@ -359,7 +358,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				_, err = reconciler.reconcileNormal(context.Background(), cs)
+				_, err = reconciler.reconcileNormal(cs)
 				g.Expect(err).To(BeNil())
 				expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.LoadBalancerReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, infrav1.WaitForDNSNameReason}})
 			})
@@ -384,7 +383,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(context.Background(), cs)
+				_, err = reconciler.reconcileNormal(cs)
 				g.Expect(err).To(BeNil())
 				expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.LoadBalancerReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, infrav1.WaitForDNSNameResolveReason}})
 			})
@@ -557,17 +556,15 @@ 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,
 		},
 		{
 			name: "Should not create reconcile request if AWSCluster not found",
 			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,
 		},
 	}

diff --git 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, machineScope.AdditionalTags())
+		r.ensureStorageTags(ec2svc, instance, machineScope.AWSMachine)
 	}
 
 	if err := r.reconcileLBAttachment(machineScope, elbScope, instance); err != nil {
@@ -1042,28 +1042,29 @@ func (r *AWSMachineReconciler) indexAWSMachineByInstanceID(o client.Object) []st
 	return nil
 }
 
-func (r *AWSMachineReconciler) ensureStorageTags(ec2svc services.EC2Interface, instance *infrav1.Instance, machine *infrav1.AWSMachine, additionalTags map[string]string) {
-	annotations, err := r.machineAnnotationJSON(machine, VolumeTagsLastAppliedAnnotation)
+func (r *AWSMachineReconciler) ensureStorageTags(ec2svc services.EC2Interface, instance *infrav1.Instance, machine *infrav1.AWSMachine) {
+	machineAnnotations, 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 := annotations[volumeID].(map[string]interface{}); ok {
-			newAnnotation, err := r.ensureVolumeTags(ec2svc, aws.String(volumeID), subAnnotation, additionalTags)
+		if subAnnotation, ok := machineAnnotations[volumeID].(map[string]interface{}); ok {
+			newAnnotation, err := r.ensureVolumeTags(ec2svc, aws.String(volumeID), subAnnotation, machine.Spec.AdditionalTags)
 			if err != nil {
 				r.Log.Error(err, "Failed to fetch the changed volume tags in EC2 instance")
 			}
-			annotations[volumeID] = newAnnotation
+			machineAnnotations[volumeID] = newAnnotation
 		} else {
-			newAnnotation, err := r.ensureVolumeTags(ec2svc, aws.String(volumeID), make(map[string]interface{}), additionalTags)
+			newAnnotation, err := r.ensureVolumeTags(ec2svc, aws.String(volumeID), make(map[string]interface{}), machine.Spec.AdditionalTags)
 			if err != nil {
 				r.Log.Error(err, "Failed to fetch the changed volume tags in EC2 instance")
 			}
-			annotations[volumeID] = newAnnotation
+			machineAnnotations[volumeID] = newAnnotation
 		}
 
 		// We also need to update the annotation if anything changed.
-		err = r.updateMachineAnnotationJSON(machine, VolumeTagsLastAppliedAnnotation, annotations)
+		err = r.updateMachineAnnotationJSON(machine, VolumeTagsLastAppliedAnnotation, machineAnnotations)
 		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
@@ -471,41 +471,34 @@ func TestAWSMachineReconciler(t *testing.T) {
 					}
 				})
 
-				t.Run("should tag instances and volumes with machine and cluster tags", func(t *testing.T) {
+				t.Run("should tag instances from machine and cluster tags", func(t *testing.T) {
 					g := NewWithT(t)
 					awsMachine := getAWSMachine()
 					setup(t, g, awsMachine)
 					defer teardown(t, g)
 					instanceCreate(t, g)
 					getCoreSecurityGroups(t, g)
 
-					ms.AWSMachine.Spec.AdditionalTags = infrav1.Tags{"kind": "alicorn", "colour": "pink"} // takes precedence
-					cs.AWSCluster.Spec.AdditionalTags = infrav1.Tags{"colour": "lavender", "shape": "round"}
+					ms.AWSMachine.Spec.AdditionalTags = infrav1.Tags{"kind": "alicorn"}
+					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(
-						PointsTo("myMachine"),
+						gomock.Any(),
 						map[string]string{
-							"colour": "pink",
-							"shape":  "round",
-							"kind":   "alicorn",
+							"kind": "alicorn",
 						},
 						map[string]string{},
-					).Return(nil)
+					).Return(nil).Times(2)
 
 					ec2Svc.EXPECT().UpdateResourceTags(
-						gomock.Any(),
+						PointsTo("myMachine"),
 						map[string]string{
-							"colour": "pink",
-							"shape":  "round",
+							"colour": "lavender",
 							"kind":   "alicorn",
 						},
 						map[string]string{},
-					).Return(nil).Times(2)
+					).Return(nil)
 
 					_, err := reconciler.reconcileNormal(context.Background(), ms, cs, cs, cs, cs)
 					g.Expect(err).To(BeNil())