From 5a8e339c97e80069fb4b6f8b28fa015b35be1525 Mon Sep 17 00:00:00 2001 From: Ilya Dmitrichenko Date: Thu, 8 Nov 2018 10:53:27 +0000 Subject: [PATCH] Fix subnet ordering assumption - pass full provider to stack manager and cluster stack builder --- pkg/cfn/builder/api_test.go | 185 +++++++++++++++++++++++------------ pkg/cfn/builder/cluster.go | 20 ++-- pkg/cfn/manager/api.go | 31 +++--- pkg/cfn/manager/cluster.go | 2 +- pkg/cfn/manager/nodegroup.go | 2 +- pkg/cfn/manager/waiters.go | 8 +- 6 files changed, 151 insertions(+), 97 deletions(-) diff --git a/pkg/cfn/builder/api_test.go b/pkg/cfn/builder/api_test.go index d0b7ab1741..cdbff466b4 100644 --- a/pkg/cfn/builder/api_test.go +++ b/pkg/cfn/builder/api_test.go @@ -3,11 +3,14 @@ package builder_test import ( "encoding/base64" "encoding/json" + "fmt" "net" "path/filepath" "strings" cfn "github.com/aws/aws-sdk-go/service/cloudformation" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/stretchr/testify/mock" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -15,6 +18,7 @@ import ( "github.com/weaveworks/eksctl/pkg/cloudconfig" "github.com/weaveworks/eksctl/pkg/eks/api" "github.com/weaveworks/eksctl/pkg/nodebootstrap" + "github.com/weaveworks/eksctl/pkg/testutils" "github.com/weaveworks/eksctl/pkg/vpc" ) @@ -24,6 +28,10 @@ const ( endpoint = "https://DE37D8AFB23F7275D2361AD6B2599143.yl4.us-west-2.eks.amazonaws.com" caCert = "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUN5RENDQWJDZ0F3SUJBZ0lCQURBTkJna3Foa2lHOXcwQkFRc0ZBREFWTVJNd0VRWURWUVFERXdwcmRXSmwKY201bGRHVnpNQjRYRFRFNE1EWXdOekExTlRBMU5Wb1hEVEk0TURZd05EQTFOVEExTlZvd0ZURVRNQkVHQTFVRQpBeE1LYTNWaVpYSnVaWFJsY3pDQ0FTSXdEUVlKS29aSWh2Y05BUUVCQlFBRGdnRVBBRENDQVFvQ2dnRUJBTWJoCnpvZElYR0drckNSZE1jUmVEN0YvMnB1NFZweTdvd3FEVDgrdk9zeGs2bXFMNWxQd3ZicFhmYkE3R0xzMDVHa0wKaDdqL0ZjcU91cnMwUFZSK3N5REtuQXltdDFORWxGNllGQktSV1dUQ1hNd2lwN1pweW9XMXdoYTlJYUlPUGxCTQpPTEVlckRabFVrVDFVV0dWeVdsMmxPeFgxa2JhV2gvakptWWdkeW5jMXhZZ3kxa2JybmVMSkkwLzVUVTRCajJxClB1emtrYW5Xd3lKbGdXQzhBSXlpWW82WFh2UVZmRzYrM3RISE5XM1F1b3ZoRng2MTFOYnl6RUI3QTdtZGNiNmgKR0ZpWjdOeThHZnFzdjJJSmI2Nk9FVzBSdW9oY1k3UDZPdnZmYnlKREhaU2hqTStRWFkxQXN5b3g4Ri9UelhHSgpQUWpoWUZWWEVhZU1wQmJqNmNFQ0F3RUFBYU1qTUNFd0RnWURWUjBQQVFIL0JBUURBZ0trTUE4R0ExVWRFd0VCCi93UUZNQU1CQWY4d0RRWUpLb1pJaHZjTkFRRUxCUUFEZ2dFQkFCa2hKRVd4MHk1LzlMSklWdXJ1c1hZbjN6Z2EKRkZ6V0JsQU44WTlqUHB3S2t0Vy9JNFYyUGg3bWY2Z3ZwZ3Jhc2t1Slk1aHZPcDdBQmcxSTFhaHUxNUFpMUI0ZApuMllRaDlOaHdXM2pKMmhuRXk0VElpb0gza2JFdHRnUVB2bWhUQzNEYUJreEpkbmZJSEJCV1RFTTU1czRwRmxUClpzQVJ3aDc1Q3hYbjdScVU0akpKcWNPaTRjeU5qeFVpRDBqR1FaTmNiZWEyMkRCeTJXaEEzUWZnbGNScGtDVGUKRDVPS3NOWlF4MW9MZFAwci9TSmtPT1NPeUdnbVJURTIrODQxN21PRW02Z3RPMCszdWJkbXQ0aENsWEtFTTZYdwpuQWNlK0JxVUNYblVIN2ZNS3p2TDE5UExvMm5KbFU1TnlCbU1nL1pNVHVlUy80eFZmKy94WnpsQ0Q1WT0KLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo=" arn = "arn:aws:eks:us-west-2:376248598259:cluster/" + clusterName + + vpcID = "vpc-0e265ad953062b94b" + subnetsPublic = "subnet-0f98135715dfcf55f,subnet-0ade11bad78dced9e,subnet-0e2e63ff1712bf6ef" + subnetsPrivate = "subnet-0f98135715dfcf55a,subnet-0ade11bad78dced9f,subnet-0e2e63ff1712bf6ea" ) type Template struct { @@ -83,6 +91,72 @@ users: ` } +func testVPC() *api.ClusterVPC { + return &api.ClusterVPC{ + Network: api.Network{ + ID: vpcID, + CIDR: &net.IPNet{ + IP: []byte{192, 168, 0, 0}, + Mask: []byte{255, 255, 0, 0}, + }, + }, + SecurityGroup: "sg-0b44c48bcba5b7362", + Subnets: map[api.SubnetTopology]map[string]api.Network{ + "Public": map[string]api.Network{ + "us-west-2b": { + ID: "subnet-0f98135715dfcf55f", + CIDR: &net.IPNet{ + IP: []byte{192, 168, 0, 0}, + Mask: []byte{255, 255, 224, 0}, + }, + }, + "us-west-2a": { + ID: "subnet-0ade11bad78dced9e", + CIDR: &net.IPNet{ + IP: []byte{192, 168, 32, 0}, + Mask: []byte{255, 255, 224, 0}, + }, + }, + "us-west-2c": { + ID: "subnet-0e2e63ff1712bf6ef", + CIDR: &net.IPNet{ + IP: []byte{192, 168, 64, 0}, + Mask: []byte{255, 255, 224, 0}, + }, + }, + }, + "Private": map[string]api.Network{ + "us-west-2b": { + ID: "subnet-0f98135715dfcf55a", + CIDR: &net.IPNet{ + IP: []byte{192, 168, 96, 0}, + Mask: []byte{255, 255, 224, 0}, + }, + }, + "us-west-2a": { + ID: "subnet-0ade11bad78dced9f", + CIDR: &net.IPNet{ + IP: []byte{192, 168, 128, 0}, + Mask: []byte{255, 255, 224, 0}, + }, + }, + "us-west-2c": { + ID: "subnet-0e2e63ff1712bf6ea", + CIDR: &net.IPNet{ + IP: []byte{192, 168, 160, 0}, + Mask: []byte{255, 255, 224, 0}, + }, + }, + }, + }, + } +} + +var subnetLists = map[api.SubnetTopology]string{ + "Public": subnetsPublic, + "Private": subnetsPrivate, +} + func newStackWithOutputs(outputs map[string]string) cfn.Stack { s := cfn.Stack{} for k, v := range outputs { @@ -116,6 +190,46 @@ var _ = Describe("CloudFormation template builder API", func() { return cfg } + p := testutils.NewMockProvider() + + { + joinCompare := func(input *ec2.DescribeSubnetsInput, compare string) bool { + ids := make([]string, len(input.SubnetIds)) + for x, id := range input.SubnetIds { + ids[x] = *id + } + return strings.Join(ids, ",") == compare + } + testVPC := testVPC() + for t := range subnetLists { + func(list string, subnetsByAz map[string]api.Network) { + subnets := strings.Split(list, ",") + + output := &ec2.DescribeSubnetsOutput{ + Subnets: make([]*ec2.Subnet, len(subnets)), + } + + for i := range subnets { + subnet := &ec2.Subnet{} + subnet.SetSubnetId(subnets[i]) + subnet.SetVpcId(vpcID) + for az := range subnetsByAz { + if subnetsByAz[az].ID == subnets[i] { + subnet.SetAvailabilityZone(az) + subnet.SetCidrBlock(subnetsByAz[az].CIDR.String()) + } + } + output.Subnets[i] = subnet + + } + p.MockEC2().On("DescribeSubnets", mock.MatchedBy(func(input *ec2.DescribeSubnetsInput) bool { + fmt.Fprintf(GinkgoWriter, "%s subnets = %#v\n", t, output) + return joinCompare(input, list) + })).Return(output, nil) + }(subnetLists[t], testVPC.Subnets[t]) + } + } + Describe("GetAllOutputsFromClusterStack", func() { caCertData, err := base64.StdEncoding.DecodeString(caCert) It("should not error", func() { Expect(err).ShouldNot(HaveOccurred()) }) @@ -129,65 +243,7 @@ var _ = Describe("CloudFormation template builder API", func() { CertificateAuthorityData: caCertData, ARN: arn, AvailabilityZones: testAZs, - - VPC: &api.ClusterVPC{ - Network: api.Network{ - ID: "vpc-0e265ad953062b94b", - CIDR: &net.IPNet{ - IP: []byte{192, 168, 0, 0}, - Mask: []byte{255, 255, 0, 0}, - }, - }, - SecurityGroup: "sg-0b44c48bcba5b7362", - Subnets: map[api.SubnetTopology]map[string]api.Network{ - "Public": map[string]api.Network{ - "us-west-2b": { - ID: "subnet-0f98135715dfcf55f", - CIDR: &net.IPNet{ - IP: []byte{192, 168, 0, 0}, - Mask: []byte{255, 255, 224, 0}, - }, - }, - "us-west-2a": { - ID: "subnet-0ade11bad78dced9e", - CIDR: &net.IPNet{ - IP: []byte{192, 168, 32, 0}, - Mask: []byte{255, 255, 224, 0}, - }, - }, - "us-west-2c": { - ID: "subnet-0e2e63ff1712bf6ef", - CIDR: &net.IPNet{ - IP: []byte{192, 168, 64, 0}, - Mask: []byte{255, 255, 224, 0}, - }, - }, - }, - "Private": map[string]api.Network{ - "us-west-2b": { - ID: "subnet-0f98135715dfcf55a", - CIDR: &net.IPNet{ - IP: []byte{192, 168, 96, 0}, - Mask: []byte{255, 255, 224, 0}, - }, - }, - "us-west-2a": { - ID: "subnet-0ade11bad78dced9f", - CIDR: &net.IPNet{ - IP: []byte{192, 168, 128, 0}, - Mask: []byte{255, 255, 224, 0}, - }, - }, - "us-west-2c": { - ID: "subnet-0e2e63ff1712bf6ea", - CIDR: &net.IPNet{ - IP: []byte{192, 168, 160, 0}, - Mask: []byte{255, 255, 224, 0}, - }, - }, - }, - }, - }, + VPC: testVPC(), NodeGroups: []*api.NodeGroup{ { AMI: "", @@ -199,7 +255,6 @@ var _ = Describe("CloudFormation template builder API", func() { } cfg := newClusterConfig() - //ctl := eks.New(testutils.ProviderConfig, cfg) It("should not error when calling SetSubnets", func() { err := vpc.SetSubnets(cfg) @@ -214,7 +269,7 @@ var _ = Describe("CloudFormation template builder API", func() { } }) - rs := NewClusterResourceSet(cfg) + rs := NewClusterResourceSet(p, cfg) It("should add all resources without error", func() { err := rs.AddAllResources() Expect(err).ShouldNot(HaveOccurred()) @@ -222,9 +277,9 @@ var _ = Describe("CloudFormation template builder API", func() { sampleOutputs := map[string]string{ "SecurityGroup": "sg-0b44c48bcba5b7362", - "SubnetsPublic": "subnet-0f98135715dfcf55f,subnet-0ade11bad78dced9e,subnet-0e2e63ff1712bf6ef", - "SubnetsPrivate": "subnet-0f98135715dfcf55a,subnet-0ade11bad78dced9f,subnet-0e2e63ff1712bf6ea", - "VPC": "vpc-0e265ad953062b94b", + "SubnetsPublic": subnetsPublic, + "SubnetsPrivate": subnetsPrivate, + "VPC": vpcID, "Endpoint": endpoint, "CertificateAuthorityData": caCert, "ARN": arn, @@ -470,7 +525,7 @@ var _ = Describe("CloudFormation template builder API", func() { cfg.AvailabilityZones = testAZs cfg.VPC = &api.ClusterVPC{ Network: api.Network{ - ID: "vpc-0e265ad953062b94b", + ID: vpcID, }, SecurityGroup: "sg-0b44c48bcba5b7362", Subnets: map[api.SubnetTopology]map[string]api.Network{ diff --git a/pkg/cfn/builder/cluster.go b/pkg/cfn/builder/cluster.go index 07f92a9e27..81a051a196 100644 --- a/pkg/cfn/builder/cluster.go +++ b/pkg/cfn/builder/cluster.go @@ -5,12 +5,14 @@ import ( gfn "github.com/awslabs/goformation/cloudformation" "github.com/weaveworks/eksctl/pkg/eks/api" + "github.com/weaveworks/eksctl/pkg/vpc" ) // ClusterResourceSet stores the resource information of the cluster type ClusterResourceSet struct { rs *resourceSet spec *api.ClusterConfig + provider api.ClusterProvider vpc *gfn.Value subnets map[api.SubnetTopology][]*gfn.Value securityGroups []*gfn.Value @@ -18,11 +20,12 @@ type ClusterResourceSet struct { } // NewClusterResourceSet returns a resource set for the new cluster -func NewClusterResourceSet(spec *api.ClusterConfig) *ClusterResourceSet { +func NewClusterResourceSet(provider api.ClusterProvider, spec *api.ClusterConfig) *ClusterResourceSet { return &ClusterResourceSet{ - rs: newResourceSet(), - spec: spec, - outputs: &ClusterStackOutputs{}, + rs: newResourceSet(), + spec: spec, + provider: provider, + outputs: &ClusterStackOutputs{}, } } @@ -99,13 +102,12 @@ func (c *ClusterResourceSet) GetAllOutputs(stack cfn.Stack) error { c.spec.VPC.ID = c.outputs.VPC c.spec.VPC.SecurityGroup = c.outputs.SecurityGroup - // TODO: shouldn't assume the order - https://github.com/weaveworks/eksctl/issues/293 - for i, subnet := range c.outputs.SubnetsPrivate { - c.spec.ImportSubnet(api.SubnetTopologyPrivate, c.spec.AvailabilityZones[i], subnet, "") + if err := vpc.UseSubnets(c.provider, c.spec, api.SubnetTopologyPrivate, c.outputs.SubnetsPrivate); err != nil { + return err } - for i, subnet := range c.outputs.SubnetsPublic { - c.spec.ImportSubnet(api.SubnetTopologyPublic, c.spec.AvailabilityZones[i], subnet, "") + if err := vpc.UseSubnets(c.provider, c.spec, api.SubnetTopologyPublic, c.outputs.SubnetsPublic); err != nil { + return err } c.spec.ClusterStackName = c.outputs.ClusterStackName diff --git a/pkg/cfn/manager/api.go b/pkg/cfn/manager/api.go index f7011ff130..2142cdf2b0 100644 --- a/pkg/cfn/manager/api.go +++ b/pkg/cfn/manager/api.go @@ -11,7 +11,6 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/cloudformation" - "github.com/aws/aws-sdk-go/service/cloudformation/cloudformationiface" "github.com/kubicorn/kubicorn/pkg/logger" ) @@ -36,10 +35,9 @@ type ChangeSet = cloudformation.DescribeChangeSetOutput // StackCollection stores the CloudFormation stack information type StackCollection struct { - cfn cloudformationiface.CloudFormationAPI - waitTimeout time.Duration - spec *api.ClusterConfig - tags []*cloudformation.Tag + provider api.ClusterProvider + spec *api.ClusterConfig + tags []*cloudformation.Tag } func newTag(key, value string) *cloudformation.Tag { @@ -56,10 +54,9 @@ func NewStackCollection(provider api.ClusterProvider, spec *api.ClusterConfig) * } logger.Debug("tags = %#v", tags) return &StackCollection{ - cfn: provider.CloudFormation(), - waitTimeout: provider.WaitTimeout(), - spec: spec, - tags: tags, + provider: provider, + spec: spec, + tags: tags, } } @@ -84,7 +81,7 @@ func (c *StackCollection) doCreateStackRequest(i *Stack, templateBody []byte, pa } logger.Debug("input = %#v", input) - s, err := c.cfn.CreateStack(input) + s, err := c.provider.CloudFormation().CreateStack(input) if err != nil { return errors.Wrapf(err, "creating CloudFormation stack %q", *i.StackName) } @@ -146,7 +143,7 @@ func (c *StackCollection) describeStack(i *Stack) (*Stack, error) { if i.StackId != nil { input.StackName = i.StackId } - resp, err := c.cfn.DescribeStacks(input) + resp, err := c.provider.CloudFormation().DescribeStacks(input) if err != nil { return nil, errors.Wrapf(err, "describing CloudFormation stack %q", *i.StackName) } @@ -182,7 +179,7 @@ func (c *StackCollection) ListStacks(nameRegex string, statusFilters ...string) } return true } - if err := c.cfn.ListStacksPages(input, pager); err != nil { + if err := c.provider.CloudFormation().ListStacksPages(input, pager); err != nil { return nil, err } if subErr != nil { @@ -210,7 +207,7 @@ func (c *StackCollection) DeleteStack(name string) (*Stack, error) { StackName: i.StackId, } - if _, err := c.cfn.DeleteStack(input); err != nil { + if _, err := c.provider.CloudFormation().DeleteStack(input); err != nil { return nil, errors.Wrapf(err, "not able to delete stack %q", name) } logger.Info("will delete stack %q", name) @@ -249,7 +246,7 @@ func (c *StackCollection) DescribeStackEvents(i *Stack) ([]*cloudformation.Stack StackName: i.StackId, } - resp, err := c.cfn.DescribeStackEvents(input) + resp, err := c.provider.CloudFormation().DescribeStackEvents(input) if err != nil { return nil, errors.Wrapf(err, "describing CloudFormation stack %q events", *i.StackName) } @@ -285,7 +282,7 @@ func (c *StackCollection) doCreateChangeSetRequest(i *Stack, action string, desc } logger.Debug("creating changeSet, input = %#v", input) - s, err := c.cfn.CreateChangeSet(input) + s, err := c.provider.CloudFormation().CreateChangeSet(input) if err != nil { return "", errors.Wrap(err, fmt.Sprintf("creating ChangeSet %q for stack %q", changeSetName, *i.StackName)) } @@ -301,7 +298,7 @@ func (c *StackCollection) doExecuteChangeSet(stackName string, changeSetName str logger.Debug("executing changeSet, input = %#v", input) - if _, err := c.cfn.ExecuteChangeSet(input); err != nil { + if _, err := c.provider.CloudFormation().ExecuteChangeSet(input); err != nil { return errors.Wrapf(err, "executing CloudFormation ChangeSet %q for stack %q", changeSetName, stackName) } return nil @@ -315,7 +312,7 @@ func (c *StackCollection) describeStackChangeSet(i *Stack, changeSetName *string if i.StackId != nil { input.StackName = i.StackId } - resp, err := c.cfn.DescribeChangeSet(input) + resp, err := c.provider.CloudFormation().DescribeChangeSet(input) if err != nil { return nil, errors.Wrapf(err, "describing CloudFormation ChangeSet %s for stack %s", *changeSetName, *i.StackName) } diff --git a/pkg/cfn/manager/cluster.go b/pkg/cfn/manager/cluster.go index 2b0a68857a..4d0b84b993 100644 --- a/pkg/cfn/manager/cluster.go +++ b/pkg/cfn/manager/cluster.go @@ -14,7 +14,7 @@ func (c *StackCollection) makeClusterStackName() string { func (c *StackCollection) CreateCluster(errs chan error, _ interface{}) error { name := c.makeClusterStackName() logger.Info("creating cluster stack %q", name) - stack := builder.NewClusterResourceSet(c.spec) + stack := builder.NewClusterResourceSet(c.provider, c.spec) if err := stack.AddAllResources(); err != nil { return err } diff --git a/pkg/cfn/manager/nodegroup.go b/pkg/cfn/manager/nodegroup.go index 78c5f23b0a..f8ef387bba 100644 --- a/pkg/cfn/manager/nodegroup.go +++ b/pkg/cfn/manager/nodegroup.go @@ -142,7 +142,7 @@ func (c *StackCollection) getStackTemplate(stackName string) (string, error) { StackName: aws.String(stackName), } - output, err := c.cfn.GetTemplate(input) + output, err := c.provider.CloudFormation().GetTemplate(input) if err != nil { return "", err } diff --git a/pkg/cfn/manager/waiters.go b/pkg/cfn/manager/waiters.go index afaa71e0a5..1f3c47920c 100644 --- a/pkg/cfn/manager/waiters.go +++ b/pkg/cfn/manager/waiters.go @@ -73,7 +73,7 @@ func (c *StackCollection) waitWithAcceptors(i *Stack, acceptors []request.Waiter desiredStatus := fmt.Sprintf("%v", acceptors[0].Expected) msg := fmt.Sprintf("waiting for CloudFormation stack %q to reach %q status", *i.StackName, desiredStatus) - ctx, cancel := context.WithTimeout(context.Background(), c.waitTimeout) + ctx, cancel := context.WithTimeout(context.Background(), c.provider.WaitTimeout()) defer cancel() startTime := time.Now() @@ -91,7 +91,7 @@ func (c *StackCollection) waitWithAcceptors(i *Stack, acceptors []request.Waiter input.StackName = i.StackId } logger.Debug(msg) - req, _ := c.cfn.DescribeStacksRequest(input) + req, _ := c.provider.CloudFormation().DescribeStacksRequest(input) req.SetContext(ctx) return req, nil }, @@ -118,7 +118,7 @@ func (c *StackCollection) waitWithAcceptors(i *Stack, acceptors []request.Waiter func (c *StackCollection) waitWithAcceptorsChangeSet(i *Stack, changesetName *string, acceptors []request.WaiterAcceptor) error { desiredStatus := fmt.Sprintf("%v", acceptors[0].Expected) msg := fmt.Sprintf("waiting for CloudFormation changeset %q for stack %q to reach %q status", *changesetName, *i.StackName, desiredStatus) - ctx, cancel := context.WithTimeout(context.Background(), c.waitTimeout) + ctx, cancel := context.WithTimeout(context.Background(), c.provider.WaitTimeout()) defer cancel() startTime := time.Now() w := request.Waiter{ @@ -132,7 +132,7 @@ func (c *StackCollection) waitWithAcceptorsChangeSet(i *Stack, changesetName *st ChangeSetName: changesetName, } logger.Debug(msg) - req, _ := c.cfn.DescribeChangeSetRequest(input) + req, _ := c.provider.CloudFormation().DescribeChangeSetRequest(input) req.SetContext(ctx) return req, nil },