From f2868ae1a0383a57758f65adcaecd2158fdbf828 Mon Sep 17 00:00:00 2001 From: Ilya Dmitrichenko Date: Fri, 9 Nov 2018 12:41:51 +0000 Subject: [PATCH 1/2] Improve stack template descriptions, fix hardcoded values for nodegroup features --- pkg/cfn/builder/api.go | 10 +++------ pkg/cfn/builder/api_test.go | 39 +++++++++++++++++++++++++++++------- pkg/cfn/builder/cluster.go | 20 +++++++++--------- pkg/cfn/builder/nodegroup.go | 8 +++++--- 4 files changed, 51 insertions(+), 26 deletions(-) diff --git a/pkg/cfn/builder/api.go b/pkg/cfn/builder/api.go index 495b14829c..6d268a35a9 100644 --- a/pkg/cfn/builder/api.go +++ b/pkg/cfn/builder/api.go @@ -9,13 +9,9 @@ import ( ) const ( - clusterTemplateDescription = "EKS cluster" - clusterTemplateDescriptionDefaultFeatures = " (with dedicated VPC & IAM role) " - - nodeGroupTemplateDescription = "EKS nodes" - nodeGroupTemplateDescriptionDefaultFeatures = " (Amazon Linux 2 with SSH) " - - templateDescriptionSuffix = " [created and managed by eksctl]" + clusterTemplateDescription = "EKS cluster" + nodeGroupTemplateDescription = "EKS nodes" + templateDescriptionSuffix = "[created and managed by eksctl]" ) type awsCloudFormationResource struct { diff --git a/pkg/cfn/builder/api_test.go b/pkg/cfn/builder/api_test.go index cdbff466b4..00135d2d9b 100644 --- a/pkg/cfn/builder/api_test.go +++ b/pkg/cfn/builder/api_test.go @@ -35,7 +35,8 @@ const ( ) type Template struct { - Resources map[string]struct { + Description string + Resources map[string]struct { Properties struct { Tags []struct { Key interface{} @@ -319,7 +320,6 @@ var _ = Describe("CloudFormation template builder API", func() { Expect(err).ShouldNot(HaveOccurred()) Expect(templateBody).ShouldNot(BeEmpty()) }) - obj := Template{} It("should parse JSON withon errors", func() { err := json.Unmarshal(templateBody, &obj) @@ -412,7 +412,7 @@ var _ = Describe("CloudFormation template builder API", func() { }) }) - Describe("NodeGroup{PrivateNetworking=true}", func() { + Describe("NodeGroup{PrivateNetworking=true AllowSSH=true}", func() { cfg := api.NewClusterConfig() ng := cfg.NewNodeGroup() @@ -422,6 +422,7 @@ var _ = Describe("CloudFormation template builder API", func() { ng.AllowSSH = true ng.InstanceType = "t2.medium" ng.PrivateNetworking = true + ng.AMIFamily = "AmazonLinux2" rs := NewNodeGroupResourceSet(cfg, "eksctl-test-private-ng", 0) rs.AddAllResources() @@ -436,6 +437,12 @@ var _ = Describe("CloudFormation template builder API", func() { Expect(err).ShouldNot(HaveOccurred()) }) + It("should have correct description", func() { + Expect(obj.Description).To(ContainSubstring("AMI family: AmazonLinux2")) + Expect(obj.Description).To(ContainSubstring("SSH access: true")) + Expect(obj.Description).To(ContainSubstring("subnet topology: Private")) + }) + It("should have correct resources and attributes", func() { Expect(len(obj.Resources)).ToNot(Equal(0)) @@ -473,6 +480,7 @@ var _ = Describe("CloudFormation template builder API", func() { ng.AllowSSH = true ng.InstanceType = "t2.medium" ng.PrivateNetworking = false + ng.AMIFamily = "AmazonLinux2" rs := NewNodeGroupResourceSet(cfg, "eksctl-test-public-ng", 0) rs.AddAllResources() @@ -487,6 +495,12 @@ var _ = Describe("CloudFormation template builder API", func() { Expect(err).ShouldNot(HaveOccurred()) }) + It("should have correct description", func() { + Expect(obj.Description).To(ContainSubstring("AMI family: AmazonLinux2")) + Expect(obj.Description).To(ContainSubstring("SSH access: true")) + Expect(obj.Description).To(ContainSubstring("subnet topology: Public")) + }) + It("should have correct resources and attributes", func() { Expect(len(obj.Resources)).ToNot(Equal(0)) @@ -557,6 +571,7 @@ var _ = Describe("CloudFormation template builder API", func() { ng.AllowSSH = false ng.InstanceType = "t2.medium" ng.PrivateNetworking = false + ng.AMIFamily = "AmazonLinux2" It("should have 1 AZ for the nodegroup", func() { Expect(ng.AvailabilityZones).To(Equal([]string{"us-west-2a"})) @@ -575,6 +590,12 @@ var _ = Describe("CloudFormation template builder API", func() { Expect(err).ShouldNot(HaveOccurred()) }) + It("should have correct description", func() { + Expect(obj.Description).To(ContainSubstring("AMI family: AmazonLinux2")) + Expect(obj.Description).To(ContainSubstring("SSH access: false")) + Expect(obj.Description).To(ContainSubstring("subnet topology: Public")) + }) + It("should have correct resources and attributes", func() { Expect(len(obj.Resources)).ToNot(Equal(0)) @@ -646,9 +667,8 @@ var _ = Describe("CloudFormation template builder API", func() { It("should serialise JSON without errors", func() { Expect(err).ShouldNot(HaveOccurred()) }) - + obj := Template{} It("should parse JSON withon errors and extract valid cloud-config using our implementation", func() { - obj := Template{} err = json.Unmarshal(template, &obj) Expect(err).ShouldNot(HaveOccurred()) Expect(len(obj.Resources)).ToNot(Equal(0)) @@ -713,9 +733,8 @@ var _ = Describe("CloudFormation template builder API", func() { It("should serialise JSON without errors", func() { Expect(err).ShouldNot(HaveOccurred()) }) - + obj := Template{} It("should parse JSON withon errors and extract valid cloud-config using our implementation", func() { - obj := Template{} err = json.Unmarshal(template, &obj) Expect(err).ShouldNot(HaveOccurred()) Expect(len(obj.Resources)).ToNot(Equal(0)) @@ -727,6 +746,12 @@ var _ = Describe("CloudFormation template builder API", func() { Expect(err).ShouldNot(HaveOccurred()) }) + It("should have correct description", func() { + Expect(obj.Description).To(ContainSubstring("AMI family: Ubuntu1804")) + Expect(obj.Description).To(ContainSubstring("SSH access: false")) + Expect(obj.Description).To(ContainSubstring("subnet topology: Public")) + }) + It("should have packages, scripts and commands in cloud-config", func() { Expect(c).ToNot(BeNil()) diff --git a/pkg/cfn/builder/cluster.go b/pkg/cfn/builder/cluster.go index 81a051a196..68476f46b2 100644 --- a/pkg/cfn/builder/cluster.go +++ b/pkg/cfn/builder/cluster.go @@ -1,6 +1,8 @@ package builder import ( + "fmt" + cfn "github.com/aws/aws-sdk-go/service/cloudformation" gfn "github.com/awslabs/goformation/cloudformation" @@ -31,18 +33,22 @@ func NewClusterResourceSet(provider api.ClusterProvider, spec *api.ClusterConfig // AddAllResources adds all the information about the cluster to the resource set func (c *ClusterResourceSet) AddAllResources() error { + dedicatedVPC := c.spec.VPC.ID == "" - templateDescriptionFeatures := clusterTemplateDescriptionDefaultFeatures + c.rs.template.Description = fmt.Sprintf( + "%s (dedicated VPC: %v, dedicatd IAM: %v) %s", + clusterTemplateDescription, + dedicatedVPC, true, + templateDescriptionSuffix) if err := c.spec.HasSufficientSubnets(); err != nil { return err } - if c.spec.VPC.ID != "" { - c.importResourcesForVPC() - templateDescriptionFeatures = " (with shared VPC and dedicated IAM role) " - } else { + if dedicatedVPC { c.addResourcesForVPC() + } else { + c.importResourcesForVPC() } c.addOutputsForVPC() @@ -52,10 +58,6 @@ func (c *ClusterResourceSet) AddAllResources() error { c.rs.newOutput(cfnOutputClusterStackName, gfn.RefStackName, false) - c.rs.template.Description = clusterTemplateDescription - c.rs.template.Description += templateDescriptionFeatures - c.rs.template.Description += templateDescriptionSuffix - return nil } diff --git a/pkg/cfn/builder/nodegroup.go b/pkg/cfn/builder/nodegroup.go index 2b49d509db..47585c4c34 100644 --- a/pkg/cfn/builder/nodegroup.go +++ b/pkg/cfn/builder/nodegroup.go @@ -41,9 +41,11 @@ func NewNodeGroupResourceSet(spec *api.ClusterConfig, clusterStackName string, i // AddAllResources adds all the information about the node group to the resource set func (n *NodeGroupResourceSet) AddAllResources() error { - n.rs.template.Description = nodeGroupTemplateDescription - n.rs.template.Description += nodeGroupTemplateDescriptionDefaultFeatures - n.rs.template.Description += templateDescriptionSuffix + n.rs.template.Description = fmt.Sprintf( + "%s (AMI family: %s, SSH access: %v, subnet topology: %s) %s", + nodeGroupTemplateDescription, + n.spec.AMIFamily, n.spec.AllowSSH, n.spec.SubnetTopology(), + templateDescriptionSuffix) n.vpc = makeImportValue(n.clusterStackName, cfnOutputClusterVPC) From 7786872a0868ca028e848739af5f881be25a2d4a Mon Sep 17 00:00:00 2001 From: Ilya Dmitrichenko Date: Fri, 9 Nov 2018 13:00:52 +0000 Subject: [PATCH 2/2] Fix panic when VPC CIDR is unset in tests --- pkg/nodebootstrap/userdata.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/nodebootstrap/userdata.go b/pkg/nodebootstrap/userdata.go index 4908fb18ea..13fda4afbe 100644 --- a/pkg/nodebootstrap/userdata.go +++ b/pkg/nodebootstrap/userdata.go @@ -86,7 +86,7 @@ func makeClientConfigData(spec *api.ClusterConfig, nodeGroupID int) ([]byte, err func clusterDNS(spec *api.ClusterConfig) string { // Default service network is 10.100.0.0, but it gets set 172.20.0.0 automatically when pod network // is anywhere within 10.0.0.0/8 - if spec.VPC.CIDR.IP[0] == 10 { + if spec.VPC.CIDR != nil && spec.VPC.CIDR.IP[0] == 10 { return "172.20.0.10" } return "10.100.0.10"