diff --git a/api/v1beta2/network_types.go b/api/v1beta2/network_types.go index 85ae45cbc2..55cb919cdc 100644 --- a/api/v1beta2/network_types.go +++ b/api/v1beta2/network_types.go @@ -457,10 +457,13 @@ func (s Subnets) IDs() []string { } // FindByID returns a single subnet matching the given id or nil. +// +// The returned pointer can be used to write back into the original slice. func (s Subnets) FindByID(id string) *SubnetSpec { - for _, x := range s { + for i := range s { + x := &(s[i]) // pointer to original structure if x.GetResourceID() == id { - return &x + return x } } return nil @@ -469,12 +472,15 @@ func (s Subnets) FindByID(id string) *SubnetSpec { // FindEqual returns a subnet spec that is equal to the one passed in. // Two subnets are defined equal to each other if their id is equal // or if they are in the same vpc and the cidr block is the same. +// +// The returned pointer can be used to write back into the original slice. func (s Subnets) FindEqual(spec *SubnetSpec) *SubnetSpec { - for _, x := range s { + for i := range s { + x := &(s[i]) // pointer to original structure if (spec.GetResourceID() != "" && x.GetResourceID() == spec.GetResourceID()) || (spec.CidrBlock == x.CidrBlock) || (spec.IPv6CidrBlock != "" && spec.IPv6CidrBlock == x.IPv6CidrBlock) { - return &x + return x } } return nil diff --git a/cmd/clusterawsadm/converters/cloudformation.go b/cmd/clusterawsadm/converters/cloudformation.go index d079d1d202..ac7bd4e104 100644 --- a/cmd/clusterawsadm/converters/cloudformation.go +++ b/cmd/clusterawsadm/converters/cloudformation.go @@ -17,6 +17,8 @@ limitations under the License. package converters import ( + "sort" + "github.com/awslabs/goformation/v4/cloudformation/tags" infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" @@ -35,5 +37,8 @@ func MapToCloudFormationTags(src infrav1.Tags) []tags.Tag { cfnTags = append(cfnTags, tag) } + // Sort so that unit tests can expect a stable order + sort.Slice(cfnTags, func(i, j int) bool { return cfnTags[i].Key < cfnTags[j].Key }) + return cfnTags } diff --git a/controllers/awscluster_controller_test.go b/controllers/awscluster_controller_test.go index d97e18d4db..c16116a463 100644 --- a/controllers/awscluster_controller_test.go +++ b/controllers/awscluster_controller_test.go @@ -23,6 +23,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/elb" "github.com/golang/mock/gomock" . "github.com/onsi/gomega" "github.com/pkg/errors" @@ -72,13 +73,13 @@ func TestAWSClusterReconcilerIntegrationTests(t *testing.T) { ec2Mock := mocks.NewMockEC2API(mockCtrl) expect := func(m *mocks.MockEC2APIMockRecorder) { // First iteration, when the AWS Cluster is missing a valid Control Plane Endpoint - mockedCreateVPCCalls(m) - mockedCreateSGCalls(false, m) + mockedVPCCallsForExistingVPCAndSubnets(m) + mockedCreateSGCalls(false, "vpc-exists", m) mockedDescribeInstanceCall(m) // Second iteration: the AWS Cluster object has been patched, // thus a valid Control Plane Endpoint has been provided - mockedCreateVPCCalls(m) - mockedCreateSGCalls(false, m) + mockedVPCCallsForExistingVPCAndSubnets(m) + mockedCreateSGCalls(false, "vpc-exists", m) mockedDescribeInstanceCall(m) } expect(ec2Mock.EXPECT()) @@ -184,8 +185,8 @@ func TestAWSClusterReconcilerIntegrationTests(t *testing.T) { ec2Mock := mocks.NewMockEC2API(mockCtrl) elbMock := mocks.NewMockELBAPI(mockCtrl) expect := func(m *mocks.MockEC2APIMockRecorder, e *mocks.MockELBAPIMockRecorder) { - mockedCreateVPCCalls(m) - mockedCreateSGCalls(false, m) + mockedVPCCallsForExistingVPCAndSubnets(m) + mockedCreateSGCalls(false, "vpc-exists", m) mockedCreateLBCalls(t, e) mockedDescribeInstanceCall(m) } @@ -293,8 +294,8 @@ func TestAWSClusterReconcilerIntegrationTests(t *testing.T) { } expect := func(m *mocks.MockEC2APIMockRecorder, e *mocks.MockELBV2APIMockRecorder) { - mockedCreateVPCCalls(m) - mockedCreateSGCalls(true, m) + mockedVPCCallsForExistingVPCAndSubnets(m) + mockedCreateSGCalls(true, "vpc-exists", m) mockedCreateLBV2Calls(t, e) mockedDescribeInstanceCall(m) } @@ -374,8 +375,116 @@ func TestAWSClusterReconcilerIntegrationTests(t *testing.T) { {conditionType: infrav1.SubnetsReadyCondition, status: corev1.ConditionTrue, severity: "", reason: ""}, }) }) + t.Run("Should successfully reconcile AWSCluster creation with managed VPC", func(t *testing.T) { + g := NewWithT(t) + mockCtrl = gomock.NewController(t) + ec2Mock := mocks.NewMockEC2API(mockCtrl) + elbMock := mocks.NewMockELBAPI(mockCtrl) + expect := func(m *mocks.MockEC2APIMockRecorder, e *mocks.MockELBAPIMockRecorder) { + mockedCallsForMissingEverything(m, e, "my-managed-subnet-priv", "my-managed-subnet-pub") + mockedCreateSGCalls(false, "vpc-new", m) + mockedDescribeInstanceCall(m) + } + expect(ec2Mock.EXPECT(), elbMock.EXPECT()) + + setup(t) + controllerIdentity := createControllerIdentity(g) + ns, err := testEnv.CreateNamespace(ctx, fmt.Sprintf("integ-test-%s", util.RandomString(5))) + g.Expect(err).To(BeNil()) + + awsCluster := getAWSCluster("test", ns.Name) + awsCluster.Spec.ControlPlaneLoadBalancer = &infrav1.AWSLoadBalancerSpec{ + LoadBalancerType: infrav1.LoadBalancerTypeClassic, + } + + // Make controller manage resources + awsCluster.Spec.NetworkSpec.VPC.ID = "" + awsCluster.Spec.NetworkSpec.Subnets[0].ID = "my-managed-subnet-priv" + awsCluster.Spec.NetworkSpec.Subnets[1].ID = "my-managed-subnet-pub" + + // NAT gateway of the public subnet will be accessed by the private subnet in the same zone, + // so use same zone for the 2 test subnets + awsCluster.Spec.NetworkSpec.Subnets[0].AvailabilityZone = "us-east-1a" + awsCluster.Spec.NetworkSpec.Subnets[1].AvailabilityZone = "us-east-1a" + + g.Expect(testEnv.Create(ctx, &awsCluster)).To(Succeed()) + g.Eventually(func() bool { + cluster := &infrav1.AWSCluster{} + key := client.ObjectKey{ + Name: awsCluster.Name, + Namespace: ns.Name, + } + err := testEnv.Get(ctx, key, cluster) + return err == nil + }, 10*time.Second).Should(BeTrue()) + + defer teardown() + defer t.Cleanup(func() { + g.Expect(testEnv.Cleanup(ctx, &awsCluster, controllerIdentity, ns)).To(Succeed()) + }) - t.Run("Should fail on AWSCluster reconciliation if `VPC limit exceeded`", func(t *testing.T) { + cs, err := getClusterScope(awsCluster) + g.Expect(err).To(BeNil()) + networkSvc := network.NewService(cs) + networkSvc.EC2Client = ec2Mock + reconciler.networkServiceFactory = func(clusterScope scope.ClusterScope) services.NetworkInterface { + return networkSvc + } + + ec2Svc := ec2Service.NewService(cs) + ec2Svc.EC2Client = ec2Mock + reconciler.ec2ServiceFactory = func(scope scope.EC2Scope) services.EC2Interface { + return ec2Svc + } + testSecurityGroupRoles := []infrav1.SecurityGroupRole{ + infrav1.SecurityGroupBastion, + infrav1.SecurityGroupAPIServerLB, + infrav1.SecurityGroupLB, + infrav1.SecurityGroupControlPlane, + infrav1.SecurityGroupNode, + } + sgSvc := securitygroup.NewService(cs, testSecurityGroupRoles) + sgSvc.EC2Client = ec2Mock + + reconciler.securityGroupFactory = func(clusterScope scope.ClusterScope) services.SecurityGroupInterface { + return sgSvc + } + elbSvc := elbService.NewService(cs) + elbSvc.EC2Client = ec2Mock + elbSvc.ELBClient = elbMock + + reconciler.elbServiceFactory = func(elbScope scope.ELBScope) services.ELBInterface { + return elbSvc + } + _, err = reconciler.reconcileNormal(cs) + g.Expect(err).To(BeNil()) + g.Expect(cs.VPC().ID).To(Equal("vpc-new")) + expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{ + {conditionType: infrav1.ClusterSecurityGroupsReadyCondition, status: corev1.ConditionTrue, severity: "", reason: ""}, + {conditionType: infrav1.BastionHostReadyCondition, status: corev1.ConditionTrue, severity: "", reason: ""}, + {conditionType: infrav1.VpcReadyCondition, status: corev1.ConditionTrue, severity: "", reason: ""}, + {conditionType: infrav1.SubnetsReadyCondition, status: corev1.ConditionTrue, severity: "", reason: ""}, + }) + + // Information should get written back into the `ClusterScope` object. Keeping it up to date means that + // reconciliation functionality will always work on the latest-known status of AWS cloud resources. + + // Private subnet + g.Expect(cs.Subnets()[0].ID).To(Equal("my-managed-subnet-priv")) + g.Expect(cs.Subnets()[0].ResourceID).To(Equal("subnet-1")) + g.Expect(cs.Subnets()[0].IsPublic).To(BeFalse()) + g.Expect(cs.Subnets()[0].NatGatewayID).To(BeNil()) + g.Expect(cs.Subnets()[0].RouteTableID).To(Equal(aws.String("rtb-1"))) + + // Public subnet + g.Expect(cs.Subnets()[1].ID).To(Equal("my-managed-subnet-pub")) + g.Expect(cs.Subnets()[1].ResourceID).To(Equal("subnet-2")) + g.Expect(cs.Subnets()[1].IsPublic).To(BeTrue()) + g.Expect(cs.Subnets()[1].NatGatewayID).To(Equal(aws.String("nat-01"))) + g.Expect(cs.Subnets()[1].RouteTableID).To(Equal(aws.String("rtb-2"))) + }) + + t.Run("Should fail on AWSCluster reconciliation if VPC limit exceeded", func(t *testing.T) { // Assuming the max VPC limit is 2 and when two VPCs are created, the creation of 3rd VPC throws mocked error from EC2 API g := NewWithT(t) mockCtrl = gomock.NewController(t) @@ -632,7 +741,7 @@ func mockedDeleteInstanceCalls(m *mocks.MockEC2APIMockRecorder) { ).Return(nil, nil) } -func mockedCreateVPCCalls(m *mocks.MockEC2APIMockRecorder) { +func mockedVPCCallsForExistingVPCAndSubnets(m *mocks.MockEC2APIMockRecorder) { m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{ Resources: aws.StringSlice([]string{"subnet-1"}), Tags: []*ec2.Tag{ @@ -765,6 +874,507 @@ func mockedCreateVPCCalls(m *mocks.MockEC2APIMockRecorder) { }, nil) } +// mockedCallsForMissingEverything mocks most of the AWSCluster reconciliation calls to the AWS API, +// except for what other functions provide (see `mockedCreateSGCalls` and `mockedDescribeInstanceCall`). +func mockedCallsForMissingEverything(m *mocks.MockEC2APIMockRecorder, e *mocks.MockELBAPIMockRecorder, privateSubnetName string, publicSubnetName string) { + describeVPCByNameCall := m.DescribeVpcsWithContext(context.TODO(), gomock.Eq(&ec2.DescribeVpcsInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("tag:Name"), + Values: aws.StringSlice([]string{"test-cluster-vpc"}), + }, + }, + })).Return(&ec2.DescribeVpcsOutput{Vpcs: []*ec2.Vpc{}}, nil) + m.CreateVpcWithContext(context.TODO(), gomock.Eq(&ec2.CreateVpcInput{ + CidrBlock: aws.String("10.0.0.0/8"), + TagSpecifications: []*ec2.TagSpecification{ + { + ResourceType: aws.String("vpc"), + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("test-cluster-vpc"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("common"), + }, + }, + }, + }, + })).After(describeVPCByNameCall).Return(&ec2.CreateVpcOutput{ + Vpc: &ec2.Vpc{ + State: aws.String("available"), + VpcId: aws.String("vpc-new"), + CidrBlock: aws.String("10.0.0.0/8"), + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("test-cluster-vpc"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("common"), + }, + }, + }, + }, nil) + + m.DescribeVpcAttributeWithContext(context.TODO(), gomock.Eq(&ec2.DescribeVpcAttributeInput{ + VpcId: aws.String("vpc-new"), + Attribute: aws.String("enableDnsHostnames"), + })).Return(&ec2.DescribeVpcAttributeOutput{ + EnableDnsHostnames: &ec2.AttributeBooleanValue{Value: aws.Bool(true)}, + }, nil) + + m.DescribeVpcAttributeWithContext(context.TODO(), gomock.Eq(&ec2.DescribeVpcAttributeInput{ + VpcId: aws.String("vpc-new"), + Attribute: aws.String("enableDnsSupport"), + })).Return(&ec2.DescribeVpcAttributeOutput{ + EnableDnsSupport: &ec2.AttributeBooleanValue{Value: aws.Bool(true)}, + }, nil) + + m.DescribeSubnetsWithContext(context.TODO(), gomock.Eq(&ec2.DescribeSubnetsInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("state"), + Values: aws.StringSlice([]string{ec2.VpcStatePending, ec2.VpcStateAvailable}), + }, + { + Name: aws.String("vpc-id"), + Values: aws.StringSlice([]string{"vpc-new"}), + }, + }})).Return(&ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{}, + }, nil) + + m.CreateSubnetWithContext(context.TODO(), gomock.Eq(&ec2.CreateSubnetInput{ + VpcId: aws.String("vpc-new"), + CidrBlock: aws.String("10.0.10.0/24"), + AvailabilityZone: aws.String("us-east-1a"), + TagSpecifications: []*ec2.TagSpecification{ + { + ResourceType: aws.String("subnet"), + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String(privateSubnetName), + }, + { + Key: aws.String("kubernetes.io/cluster/test-cluster"), + Value: aws.String("shared"), + }, + { + Key: aws.String("kubernetes.io/role/internal-elb"), + Value: aws.String("1"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("private"), + }, + }, + }, + }, + })).Return(&ec2.CreateSubnetOutput{ + Subnet: &ec2.Subnet{ + VpcId: aws.String("vpc-new"), + SubnetId: aws.String("subnet-1"), + CidrBlock: aws.String("10.0.10.0/24"), + AvailabilityZone: aws.String("us-east-1a"), + MapPublicIpOnLaunch: aws.Bool(false), + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String(privateSubnetName), + }, + { + Key: aws.String("kubernetes.io/cluster/test-cluster"), + Value: aws.String("shared"), + }, + { + Key: aws.String("kubernetes.io/role/internal-elb"), + Value: aws.String("1"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("private"), + }, + }, + }, + }, nil) + + m.WaitUntilSubnetAvailableWithContext(context.TODO(), gomock.Eq(&ec2.DescribeSubnetsInput{ + SubnetIds: aws.StringSlice([]string{"subnet-1"}), + })).Return(nil) + + m.CreateSubnetWithContext(context.TODO(), gomock.Eq(&ec2.CreateSubnetInput{ + VpcId: aws.String("vpc-new"), + CidrBlock: aws.String("10.0.11.0/24"), + AvailabilityZone: aws.String("us-east-1a"), + TagSpecifications: []*ec2.TagSpecification{ + { + ResourceType: aws.String("subnet"), + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String(publicSubnetName), + }, + { + Key: aws.String("kubernetes.io/cluster/test-cluster"), + Value: aws.String("shared"), + }, + { + Key: aws.String("kubernetes.io/role/elb"), + Value: aws.String("1"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("public"), + }, + }, + }, + }, + })).Return(&ec2.CreateSubnetOutput{ + Subnet: &ec2.Subnet{ + VpcId: aws.String("vpc-new"), + SubnetId: aws.String("subnet-2"), + CidrBlock: aws.String("10.0.11.0/24"), + AvailabilityZone: aws.String("us-east-1a"), + MapPublicIpOnLaunch: aws.Bool(false), + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String(publicSubnetName), + }, + { + Key: aws.String("kubernetes.io/cluster/test-cluster"), + Value: aws.String("shared"), + }, + { + Key: aws.String("kubernetes.io/role/elb"), + Value: aws.String("1"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("public"), + }, + }, + }, + }, nil) + + m.WaitUntilSubnetAvailableWithContext(context.TODO(), gomock.Eq(&ec2.DescribeSubnetsInput{ + SubnetIds: aws.StringSlice([]string{"subnet-2"}), + })).Return(nil) + + m.ModifySubnetAttributeWithContext(context.TODO(), gomock.Eq(&ec2.ModifySubnetAttributeInput{ + SubnetId: aws.String("subnet-2"), + MapPublicIpOnLaunch: &ec2.AttributeBooleanValue{ + Value: aws.Bool(true), + }, + })).Return(&ec2.ModifySubnetAttributeOutput{}, nil) + + m.DescribeRouteTablesWithContext(context.TODO(), gomock.Eq(&ec2.DescribeRouteTablesInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("vpc-id"), + Values: aws.StringSlice([]string{"vpc-new"}), + }, + { + Name: aws.String("tag-key"), + Values: aws.StringSlice([]string{"sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"}), + }, + }})).Return(&ec2.DescribeRouteTablesOutput{ + RouteTables: []*ec2.RouteTable{ + { + Routes: []*ec2.Route{ + { + GatewayId: aws.String("igw-12345"), + }, + }, + }, + }, + }, nil).MinTimes(1).MaxTimes(2) + + m.DescribeInternetGatewaysWithContext(context.TODO(), gomock.Eq(&ec2.DescribeInternetGatewaysInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("attachment.vpc-id"), + Values: aws.StringSlice([]string{"vpc-new"}), + }, + }, + })).Return(&ec2.DescribeInternetGatewaysOutput{ + InternetGateways: []*ec2.InternetGateway{}, + }, nil) + + m.CreateInternetGatewayWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.CreateInternetGatewayInput{})). + Return(&ec2.CreateInternetGatewayOutput{ + InternetGateway: &ec2.InternetGateway{ + InternetGatewayId: aws.String("igw-1"), + Tags: []*ec2.Tag{ + { + Key: aws.String(infrav1.ClusterTagKey("test-cluster")), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("common"), + }, + { + Key: aws.String("Name"), + Value: aws.String("test-cluster-igw"), + }, + }, + }, + }, nil) + + m.AttachInternetGatewayWithContext(context.TODO(), gomock.Eq(&ec2.AttachInternetGatewayInput{ + InternetGatewayId: aws.String("igw-1"), + VpcId: aws.String("vpc-new"), + })). + Return(&ec2.AttachInternetGatewayOutput{}, nil) + + m.DescribeNatGatewaysPagesWithContext(context.TODO(), gomock.Eq(&ec2.DescribeNatGatewaysInput{ + Filter: []*ec2.Filter{ + { + Name: aws.String("vpc-id"), + Values: []*string{aws.String("vpc-new")}, + }, + { + Name: aws.String("state"), + Values: aws.StringSlice([]string{ec2.VpcStatePending, ec2.VpcStateAvailable}), + }, + }}), gomock.Any()).Return(nil).MinTimes(1).MaxTimes(2) + + m.DescribeAddressesWithContext(context.TODO(), 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"}), + }, + { + Name: aws.String("tag:sigs.k8s.io/cluster-api-provider-aws/role"), + Values: aws.StringSlice([]string{"apiserver"}), + }, + }, + })).Return(&ec2.DescribeAddressesOutput{ + Addresses: []*ec2.Address{}, + }, nil) + + m.AllocateAddressWithContext(context.TODO(), gomock.Eq(&ec2.AllocateAddressInput{ + Domain: aws.String("vpc"), + TagSpecifications: []*ec2.TagSpecification{ + { + ResourceType: aws.String("elastic-ip"), + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("test-cluster-eip-apiserver"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("apiserver"), + }, + }, + }, + }, + })).Return(&ec2.AllocateAddressOutput{ + AllocationId: aws.String("1234"), + }, nil) + + m.CreateNatGatewayWithContext(context.TODO(), gomock.Eq(&ec2.CreateNatGatewayInput{ + AllocationId: aws.String("1234"), + SubnetId: aws.String("subnet-2"), + TagSpecifications: []*ec2.TagSpecification{ + { + ResourceType: aws.String("natgateway"), + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("test-cluster-nat"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("common"), + }, + }, + }, + }, + })).Return(&ec2.CreateNatGatewayOutput{ + NatGateway: &ec2.NatGateway{ + NatGatewayId: aws.String("nat-01"), + SubnetId: aws.String("subnet-2"), + }, + }, nil) + + m.WaitUntilNatGatewayAvailableWithContext(context.TODO(), &ec2.DescribeNatGatewaysInput{ + NatGatewayIds: []*string{aws.String("nat-01")}, + }).Return(nil) + + m.CreateRouteTableWithContext(context.TODO(), gomock.Eq(&ec2.CreateRouteTableInput{ + TagSpecifications: []*ec2.TagSpecification{ + { + ResourceType: aws.String("route-table"), + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("test-cluster-rt-private-us-east-1a"), + }, + { + Key: aws.String("kubernetes.io/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("common"), + }, + }, + }, + }, + VpcId: aws.String("vpc-new"), + })).Return(&ec2.CreateRouteTableOutput{ + RouteTable: &ec2.RouteTable{ + RouteTableId: aws.String("rtb-1"), + }, + }, nil) + + m.CreateRouteWithContext(context.TODO(), gomock.Eq(&ec2.CreateRouteInput{ + DestinationCidrBlock: aws.String("0.0.0.0/0"), + NatGatewayId: aws.String("nat-01"), + RouteTableId: aws.String("rtb-1"), + })).Return(&ec2.CreateRouteOutput{}, nil) + + m.AssociateRouteTableWithContext(context.TODO(), gomock.Eq(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String("rtb-1"), + SubnetId: aws.String("subnet-1"), + })).Return(&ec2.AssociateRouteTableOutput{}, nil) + + m.CreateRouteTableWithContext(context.TODO(), gomock.Eq(&ec2.CreateRouteTableInput{ + TagSpecifications: []*ec2.TagSpecification{ + { + ResourceType: aws.String("route-table"), + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("test-cluster-rt-public-us-east-1a"), + }, + { + Key: aws.String("kubernetes.io/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("common"), + }, + }, + }, + }, + VpcId: aws.String("vpc-new"), + })).Return(&ec2.CreateRouteTableOutput{ + RouteTable: &ec2.RouteTable{ + RouteTableId: aws.String("rtb-2"), + }, + }, nil) + + m.CreateRouteWithContext(context.TODO(), gomock.Eq(&ec2.CreateRouteInput{ + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-1"), + RouteTableId: aws.String("rtb-2"), + })).Return(&ec2.CreateRouteOutput{}, nil) + + m.AssociateRouteTableWithContext(context.TODO(), gomock.Eq(&ec2.AssociateRouteTableInput{ + RouteTableId: aws.String("rtb-2"), + SubnetId: aws.String("subnet-2"), + })).Return(&ec2.AssociateRouteTableOutput{}, nil) + + e.DescribeLoadBalancers(gomock.Eq(&elb.DescribeLoadBalancersInput{ + LoadBalancerNames: aws.StringSlice([]string{"test-cluster-apiserver"}), + })).Return(&elb.DescribeLoadBalancersOutput{ + LoadBalancerDescriptions: []*elb.LoadBalancerDescription{}, + }, nil) + + e.CreateLoadBalancer(gomock.Eq(&elb.CreateLoadBalancerInput{ + Listeners: []*elb.Listener{ + { + InstancePort: aws.Int64(6443), + InstanceProtocol: aws.String("TCP"), + LoadBalancerPort: aws.Int64(6443), + Protocol: aws.String("TCP"), + }, + }, + LoadBalancerName: aws.String("test-cluster-apiserver"), + Scheme: aws.String("internet-facing"), + SecurityGroups: aws.StringSlice([]string{"sg-apiserver-lb"}), + Subnets: aws.StringSlice([]string{"subnet-2"}), + Tags: []*elb.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("test-cluster-apiserver"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("apiserver"), + }, + }, + })).Return(&elb.CreateLoadBalancerOutput{ + DNSName: aws.String("unittest24.de"), + }, nil) + + e.ConfigureHealthCheck(gomock.Eq(&elb.ConfigureHealthCheckInput{ + LoadBalancerName: aws.String("test-cluster-apiserver"), + HealthCheck: &elb.HealthCheck{ + Target: aws.String("SSL:6443"), + Interval: aws.Int64(10), + Timeout: aws.Int64(5), + HealthyThreshold: aws.Int64(5), + UnhealthyThreshold: aws.Int64(3), + }, + })).Return(&elb.ConfigureHealthCheckOutput{}, nil) +} + func mockedCreateMaximumVPCCalls(m *mocks.MockEC2APIMockRecorder) { describeVPCByNameCall := m.DescribeVpcsWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeVpcsInput{})).Return(&ec2.DescribeVpcsOutput{ Vpcs: []*ec2.Vpc{}, @@ -987,12 +1597,12 @@ func mockedDeleteVPCCalls(m *mocks.MockEC2APIMockRecorder) { })) } -func mockedCreateSGCalls(recordLBV2 bool, m *mocks.MockEC2APIMockRecorder) { +func mockedCreateSGCalls(recordLBV2 bool, vpcID string, m *mocks.MockEC2APIMockRecorder) { m.DescribeSecurityGroupsWithContext(context.TODO(), gomock.Eq(&ec2.DescribeSecurityGroupsInput{ Filters: []*ec2.Filter{ { Name: aws.String("vpc-id"), - Values: aws.StringSlice([]string{"vpc-exists"}), + Values: aws.StringSlice([]string{vpcID}), }, { Name: aws.String("tag-key"), @@ -1009,7 +1619,7 @@ func mockedCreateSGCalls(recordLBV2 bool, m *mocks.MockEC2APIMockRecorder) { }, }, nil) m.CreateSecurityGroupWithContext(context.TODO(), gomock.Eq(&ec2.CreateSecurityGroupInput{ - VpcId: aws.String("vpc-exists"), + VpcId: aws.String(vpcID), GroupName: aws.String("test-cluster-bastion"), Description: aws.String("Kubernetes cluster test-cluster: bastion"), TagSpecifications: []*ec2.TagSpecification{ @@ -1034,7 +1644,7 @@ func mockedCreateSGCalls(recordLBV2 bool, m *mocks.MockEC2APIMockRecorder) { })). Return(&ec2.CreateSecurityGroupOutput{GroupId: aws.String("sg-bastion")}, nil) m.CreateSecurityGroupWithContext(context.TODO(), gomock.Eq(&ec2.CreateSecurityGroupInput{ - VpcId: aws.String("vpc-exists"), + VpcId: aws.String(vpcID), GroupName: aws.String("test-cluster-apiserver-lb"), Description: aws.String("Kubernetes cluster test-cluster: apiserver-lb"), TagSpecifications: []*ec2.TagSpecification{ @@ -1059,7 +1669,7 @@ func mockedCreateSGCalls(recordLBV2 bool, m *mocks.MockEC2APIMockRecorder) { })). Return(&ec2.CreateSecurityGroupOutput{GroupId: aws.String("sg-apiserver-lb")}, nil) m.CreateSecurityGroupWithContext(context.TODO(), gomock.Eq(&ec2.CreateSecurityGroupInput{ - VpcId: aws.String("vpc-exists"), + VpcId: aws.String(vpcID), GroupName: aws.String("test-cluster-lb"), Description: aws.String("Kubernetes cluster test-cluster: lb"), TagSpecifications: []*ec2.TagSpecification{ @@ -1088,7 +1698,7 @@ func mockedCreateSGCalls(recordLBV2 bool, m *mocks.MockEC2APIMockRecorder) { })). Return(&ec2.CreateSecurityGroupOutput{GroupId: aws.String("sg-lb")}, nil) securityGroupControl := m.CreateSecurityGroupWithContext(context.TODO(), gomock.Eq(&ec2.CreateSecurityGroupInput{ - VpcId: aws.String("vpc-exists"), + VpcId: aws.String(vpcID), GroupName: aws.String("test-cluster-controlplane"), Description: aws.String("Kubernetes cluster test-cluster: controlplane"), TagSpecifications: []*ec2.TagSpecification{ @@ -1113,7 +1723,7 @@ func mockedCreateSGCalls(recordLBV2 bool, m *mocks.MockEC2APIMockRecorder) { })). Return(&ec2.CreateSecurityGroupOutput{GroupId: aws.String("sg-controlplane")}, nil) securityGroupNode := m.CreateSecurityGroupWithContext(context.TODO(), gomock.Eq(&ec2.CreateSecurityGroupInput{ - VpcId: aws.String("vpc-exists"), + VpcId: aws.String(vpcID), GroupName: aws.String("test-cluster-node"), Description: aws.String("Kubernetes cluster test-cluster: node"), TagSpecifications: []*ec2.TagSpecification{ diff --git a/pkg/cloud/converters/tags.go b/pkg/cloud/converters/tags.go index 7bec800206..c46c412d7c 100644 --- a/pkg/cloud/converters/tags.go +++ b/pkg/cloud/converters/tags.go @@ -17,6 +17,8 @@ limitations under the License. package converters import ( + "sort" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/autoscaling" "github.com/aws/aws-sdk-go/service/ec2" @@ -64,6 +66,9 @@ func MapToTags(src infrav1.Tags) []*ec2.Tag { tags = append(tags, tag) } + // Sort so that unit tests can expect a stable order + sort.Slice(tags, func(i, j int) bool { return *tags[i].Key < *tags[j].Key }) + return tags } @@ -102,6 +107,9 @@ func MapToELBTags(src infrav1.Tags) []*elb.Tag { tags = append(tags, tag) } + // Sort so that unit tests can expect a stable order + sort.Slice(tags, func(i, j int) bool { return *tags[i].Key < *tags[j].Key }) + return tags } @@ -118,6 +126,9 @@ func MapToV2Tags(src infrav1.Tags) []*elbv2.Tag { tags = append(tags, tag) } + // Sort so that unit tests can expect a stable order + sort.Slice(tags, func(i, j int) bool { return *tags[i].Key < *tags[j].Key }) + return tags } @@ -134,6 +145,9 @@ func MapToSecretsManagerTags(src infrav1.Tags) []*secretsmanager.Tag { tags = append(tags, tag) } + // Sort so that unit tests can expect a stable order + sort.Slice(tags, func(i, j int) bool { return *tags[i].Key < *tags[j].Key }) + return tags } @@ -150,6 +164,9 @@ func MapToSSMTags(src infrav1.Tags) []*ssm.Tag { tags = append(tags, tag) } + // Sort so that unit tests can expect a stable order + sort.Slice(tags, func(i, j int) bool { return *tags[i].Key < *tags[j].Key }) + return tags } @@ -166,6 +183,9 @@ func MapToIAMTags(src infrav1.Tags) []*iam.Tag { tags = append(tags, tag) } + // Sort so that unit tests can expect a stable order + sort.Slice(tags, func(i, j int) bool { return *tags[i].Key < *tags[j].Key }) + return tags } diff --git a/pkg/cloud/services/autoscaling/autoscalinggroup.go b/pkg/cloud/services/autoscaling/autoscalinggroup.go index 73838b06cb..6e24cf22f9 100644 --- a/pkg/cloud/services/autoscaling/autoscalinggroup.go +++ b/pkg/cloud/services/autoscaling/autoscalinggroup.go @@ -19,6 +19,7 @@ package asg import ( "context" "fmt" + "sort" "strings" "github.com/aws/aws-sdk-go/aws" @@ -422,6 +423,9 @@ func BuildTagsFromMap(asgName string, inTags map[string]string) []*autoscaling.T }) } + // Sort so that unit tests can expect a stable order + sort.Slice(tags, func(i, j int) bool { return *tags[i].Key < *tags[j].Key }) + return tags } @@ -500,6 +504,10 @@ func mapToTags(input map[string]string, resourceID *string) []*autoscaling.Tag { Value: aws.String(v), }) } + + // Sort so that unit tests can expect a stable order + sort.Slice(tags, func(i, j int) bool { return *tags[i].Key < *tags[j].Key }) + return tags } diff --git a/pkg/cloud/services/ec2/launchtemplate.go b/pkg/cloud/services/ec2/launchtemplate.go index 1bb54c47b8..356433ed91 100644 --- a/pkg/cloud/services/ec2/launchtemplate.go +++ b/pkg/cloud/services/ec2/launchtemplate.go @@ -899,6 +899,8 @@ func (s *Service) buildLaunchTemplateTagSpecificationRequest(scope scope.LaunchT Value: aws.String(value), }) } + // Sort so that unit tests can expect a stable order + sort.Slice(spec.Tags, func(i, j int) bool { return *spec.Tags[i].Key < *spec.Tags[j].Key }) tagSpecifications = append(tagSpecifications, spec) } @@ -911,6 +913,8 @@ func (s *Service) buildLaunchTemplateTagSpecificationRequest(scope scope.LaunchT Value: aws.String(value), }) } + // Sort so that unit tests can expect a stable order + sort.Slice(spec.Tags, func(i, j int) bool { return *spec.Tags[i].Key < *spec.Tags[j].Key }) tagSpecifications = append(tagSpecifications, spec) } diff --git a/pkg/cloud/services/eks/iam/iam.go b/pkg/cloud/services/eks/iam/iam.go index 3c05d6120e..e8b13e4747 100644 --- a/pkg/cloud/services/eks/iam/iam.go +++ b/pkg/cloud/services/eks/iam/iam.go @@ -23,6 +23,7 @@ import ( "encoding/json" "net/http" "net/url" + "sort" "strings" "github.com/aws/aws-sdk-go/aws" @@ -175,6 +176,10 @@ func RoleTags(key string, additionalTags infrav1.Tags) []*iam.Tag { Value: aws.String(v), }) } + + // Sort so that unit tests can expect a stable order + sort.Slice(tags, func(i, j int) bool { return *tags[i].Key < *tags[j].Key }) + return tags } diff --git a/pkg/cloud/services/elb/loadbalancer.go b/pkg/cloud/services/elb/loadbalancer.go index d962b7aefe..3e9e6c304d 100644 --- a/pkg/cloud/services/elb/loadbalancer.go +++ b/pkg/cloud/services/elb/loadbalancer.go @@ -758,20 +758,18 @@ func (s *Service) RegisterInstanceWithAPIServerELB(i *infrav1.Instance) error { } // Validate that the subnets associated with the load balancer has the instance AZ. - subnet := s.scope.Subnets().FindByID(i.SubnetID) - if subnet == nil { + subnets := s.scope.Subnets() + instanceSubnet := subnets.FindByID(i.SubnetID) + if instanceSubnet == nil { return errors.Errorf("failed to attach load balancer subnets, could not find subnet %q description in AWSCluster", i.SubnetID) } - instanceAZ := subnet.AvailabilityZone + instanceAZ := instanceSubnet.AvailabilityZone - var subnets infrav1.Subnets if s.scope.ControlPlaneLoadBalancer() != nil && len(s.scope.ControlPlaneLoadBalancer().Subnets) > 0 { subnets, err = s.getControlPlaneLoadBalancerSubnets() if err != nil { return err } - } else { - subnets = s.scope.Subnets() } found := false diff --git a/pkg/cloud/services/network/natgateways.go b/pkg/cloud/services/network/natgateways.go index abdf1c3c0a..8038b42290 100644 --- a/pkg/cloud/services/network/natgateways.go +++ b/pkg/cloud/services/network/natgateways.go @@ -114,8 +114,12 @@ func (s *Service) reconcileNatGateways() error { } ngws, err := s.createNatGateways(subnetIDs) + subnets := s.scope.Subnets() + defer func() { + s.scope.SetSubnets(subnets) + }() for _, ng := range ngws { - subnet := s.scope.Subnets().FindByID(*ng.SubnetId) + subnet := subnets.FindByID(*ng.SubnetId) subnet.NatGatewayID = ng.NatGatewayId } diff --git a/pkg/cloud/services/network/routetables.go b/pkg/cloud/services/network/routetables.go index b9ec4fb7b2..777c12fd64 100644 --- a/pkg/cloud/services/network/routetables.go +++ b/pkg/cloud/services/network/routetables.go @@ -53,8 +53,12 @@ func (s *Service) reconcileRouteTables() error { } subnets := s.scope.Subnets() + defer func() { + s.scope.SetSubnets(subnets) + }() + for i := range subnets { - sn := subnets[i] + sn := &subnets[i] // We need to compile the minimum routes for this subnet first, so we can compare it or create them. var routes []*ec2.Route if sn.IsPublic { @@ -66,7 +70,7 @@ func (s *Service) reconcileRouteTables() error { routes = append(routes, s.getGatewayPublicIPv6Route()) } } else { - natGatewayID, err := s.getNatGatewayForSubnet(&sn) + natGatewayID, err := s.getNatGatewayForSubnet(sn) if err != nil { return err } diff --git a/pkg/cloud/services/network/subnets.go b/pkg/cloud/services/network/subnets.go index d2b1ee1c63..c4b7a0c44f 100644 --- a/pkg/cloud/services/network/subnets.go +++ b/pkg/cloud/services/network/subnets.go @@ -167,7 +167,7 @@ func (s *Service) reconcileSubnets() error { } else if unmanagedVPC { // If there is no existing subnet and we have an umanaged vpc report an error record.Warnf(s.scope.InfraCluster(), "FailedMatchSubnet", "Using unmanaged VPC and failed to find existing subnet for specified subnet id %d, cidr %q", sub.GetResourceID(), sub.CidrBlock) - return errors.New(fmt.Errorf("usign unmanaged vpc and subnet %s (cidr %s) specified but it doesn't exist in vpc %s", sub.GetResourceID(), sub.CidrBlock, s.scope.VPC().ID).Error()) + return errors.New(fmt.Errorf("using unmanaged vpc and subnet %s (cidr %s) specified but it doesn't exist in vpc %s", sub.GetResourceID(), sub.CidrBlock, s.scope.VPC().ID).Error()) } }