From f32bcbe38954dba1777390e2b21fea4130875727 Mon Sep 17 00:00:00 2001 From: Nick Fischer Date: Wed, 8 Apr 2020 11:16:03 -0700 Subject: [PATCH 1/5] Validate instance type before CloudFormation CloudFormation will try to validate the Default ECSInstanceType parameter based on the AllowedValues field which is problematic in regions that do not support our default t2.micro instance type such as eu-north-1. Resolves #1006 --- ecs-cli/modules/cli/cluster/cluster_app.go | 60 +++++++++++++++++-- .../modules/cli/cluster/cluster_app_test.go | 55 ++++++++++++++++- .../aws/cloudformation/cluster_template.go | 12 +--- 3 files changed, 110 insertions(+), 17 deletions(-) diff --git a/ecs-cli/modules/cli/cluster/cluster_app.go b/ecs-cli/modules/cli/cluster/cluster_app.go index fd43114d8..3053c41fa 100644 --- a/ecs-cli/modules/cli/cluster/cluster_app.go +++ b/ecs-cli/modules/cli/cluster/cluster_app.go @@ -71,6 +71,11 @@ const ( ParameterKeySpotPrice = "SpotPrice" ) +const ( + invalidInstanceTypeFmt = "instance type %s not found in list of supported instance types %s" + instanceTypeUnsupportedFmt = "instance type %s not supported in region %s: %w" +) + var flagNamesToStackParameterKeys map[string]string var requiredParameters []string = []string{ParameterKeyCluster} @@ -314,6 +319,24 @@ func createCluster(context *cli.Context, awsClients *AWSClients, commandConfig * } if launchType == config.LaunchTypeEC2 { + instanceType, err := populateInstanceTypeParameter(cfnParams) + if err != nil { + return err + } + supportedInstanceTypes, err := awsClients.EC2Client.DescribeInstanceTypeOfferings(commandConfig.Region()) + if err != nil { + return errors.Wrapf(err, "No instance type found in region %s", commandConfig.Region()) + } + + if err = validateInstanceType(instanceType, supportedInstanceTypes); err != nil { + // if we detect the default value is unsupported then we'll suggest to the user overriding the value with the appropriate flag + if instanceType == cloudformation.DefaultECSInstanceType { + logrus.Warnf("Default instance type %s not supported in region %s. Override the default instance type with the --%s flag and provide a supported value.", + instanceType, commandConfig.Region(), flags.InstanceTypeFlag) + } + return fmt.Errorf(instanceTypeUnsupportedFmt, instanceType, commandConfig.Region(), err) + } + // Check if image id was supplied, else populate _, err = cfnParams.GetParameter(ParameterKeyAmiId) if err == cloudformation.ParameterNotFoundError { @@ -345,11 +368,7 @@ func createCluster(context *cli.Context, awsClients *AWSClients, commandConfig * } } // Create cfn stack - instanceTypes, err := awsClients.EC2Client.DescribeInstanceTypeOfferings(commandConfig.Region()) - if err != nil { - return errors.Wrapf(err, "No instance type found in region %s", commandConfig.Region()) - } - template, err := cloudformation.GetClusterTemplate(tags, stackName, instanceTypes) + template, err := cloudformation.GetClusterTemplate(tags, stackName) if err != nil { return errors.Wrapf(err, "Error building cloudformation template") } @@ -391,7 +410,6 @@ func retrieveInstanceType(cfnParams *cloudformation.CfnStackParams) (string, err param, err := cfnParams.GetParameter(ParameterKeyInstanceType) if err == cloudformation.ParameterNotFoundError { - logrus.Infof("Defaulting instance type to %s", cloudformation.DefaultECSInstanceType) return cloudformation.DefaultECSInstanceType, nil } if err != nil { @@ -400,6 +418,36 @@ func retrieveInstanceType(cfnParams *cloudformation.CfnStackParams) (string, err return aws.StringValue(param.ParameterValue), nil } +func populateInstanceTypeParameter(cfnParams *cloudformation.CfnStackParams) (string, error) { + param, err := cfnParams.GetParameter(ParameterKeyInstanceType) + if err == cloudformation.ParameterNotFoundError { + logrus.Infof("Defaulting instance type to %s", cloudformation.DefaultECSInstanceType) + + cfnParams.Add(ParameterKeyInstanceType, cloudformation.DefaultECSInstanceType) + + return cloudformation.DefaultECSInstanceType, nil + } else if err != nil { + return "", err + } + + return aws.StringValue(param.ParameterValue), err +} + +func validateInstanceType(instanceType string, supportedInstanceTypes []string) error { + found := false + for _, it := range supportedInstanceTypes { + if it == instanceType { + found = true + break + } + } + if !found { + return fmt.Errorf(invalidInstanceTypeFmt, instanceType, supportedInstanceTypes) + } + + return nil +} + func populateAMIID(cfnParams *cloudformation.CfnStackParams, client amimetadata.Client) error { instanceType, err := retrieveInstanceType(cfnParams) if err != nil { diff --git a/ecs-cli/modules/cli/cluster/cluster_app_test.go b/ecs-cli/modules/cli/cluster/cluster_app_test.go index fbba74044..bbb9c7288 100644 --- a/ecs-cli/modules/cli/cluster/cluster_app_test.go +++ b/ecs-cli/modules/cli/cluster/cluster_app_test.go @@ -18,6 +18,7 @@ import ( "bytes" "errors" "flag" + "fmt" "os" "testing" @@ -1024,7 +1025,7 @@ func TestClusterUpARM64(t *testing.T) { ) gomock.InOrder( - mockEC2.EXPECT().DescribeInstanceTypeOfferings("us-west-1").Return([]string{"t2.micro"}, nil), + mockEC2.EXPECT().DescribeInstanceTypeOfferings("us-west-1").Return([]string{"t2.micro", "a1.medium"}, nil), ) flagSet := flag.NewFlagSet("ecs-cli-up", 0) @@ -1041,6 +1042,58 @@ func TestClusterUpARM64(t *testing.T) { assert.NoError(t, err, "Unexpected error bringing up cluster") } +func TestClusterUpWithUnsupportedInstanceType(t *testing.T) { + defer os.Clearenv() + mockECS, mockCloudformation, mockSSM, mockEC2 := setupTest(t) + awsClients := &AWSClients{mockECS, mockCloudformation, mockSSM, mockEC2} + + instanceType := "a1.medium" + region := "us-west-1" + supportedInstanceTypes := []string{"t2.micro"} + + invalidInstanceTypeErr := fmt.Errorf(invalidInstanceTypeFmt, instanceType, supportedInstanceTypes) + expectedError := fmt.Errorf(instanceTypeUnsupportedFmt, + instanceType, region, invalidInstanceTypeErr) + + gomock.InOrder( + mockECS.EXPECT().CreateCluster(clusterName, gomock.Any()).Return(clusterName, nil), + ) + + gomock.InOrder( + mockSSM.EXPECT().GetRecommendedECSLinuxAMI(instanceType).Return(amiMetadata(armAMIID), nil), + ) + + gomock.InOrder( + mockCloudformation.EXPECT().ValidateStackExists(stackName).Return(errors.New("error")), + mockCloudformation.EXPECT().CreateStack(gomock.Any(), stackName, true, gomock.Any(), gomock.Any()).Do(func(v, w, x, y, z interface{}) { + capabilityIAM := x.(bool) + cfnParams := y.(*cloudformation.CfnStackParams) + amiIDParam, err := cfnParams.GetParameter(ParameterKeyAmiId) + assert.NoError(t, err, "Unexpected error getting cfn parameter") + assert.Equal(t, armAMIID, aws.StringValue(amiIDParam.ParameterValue), "Expected ami ID to be set to recommended for arm64") + assert.True(t, capabilityIAM, "Expected capability capabilityIAM to be true") + }).Return("", nil), + mockCloudformation.EXPECT().WaitUntilCreateComplete(stackName).Return(nil), + ) + + gomock.InOrder( + mockEC2.EXPECT().DescribeInstanceTypeOfferings(region).Return(supportedInstanceTypes, nil), + ) + + flagSet := flag.NewFlagSet("ecs-cli-up", 0) + flagSet.Bool(flags.CapabilityIAMFlag, true, "") + flagSet.String(flags.KeypairNameFlag, "default", "") + flagSet.String(flags.InstanceTypeFlag, instanceType, "") + + context := cli.NewContext(nil, flagSet, nil) + rdwr := newMockReadWriter() + commandConfig, err := newCommandConfig(context, rdwr) + assert.NoError(t, err, "Unexpected error creating CommandConfig") + + err = createCluster(context, awsClients, commandConfig) + assert.Equal(t, err, expectedError) +} + func TestClusterUpWithTags(t *testing.T) { defer os.Clearenv() mockECS, mockCloudformation, mockSSM, mockEC2 := setupTest(t) diff --git a/ecs-cli/modules/clients/aws/cloudformation/cluster_template.go b/ecs-cli/modules/clients/aws/cloudformation/cluster_template.go index fd39b8f12..c2149648c 100644 --- a/ecs-cli/modules/clients/aws/cloudformation/cluster_template.go +++ b/ecs-cli/modules/clients/aws/cloudformation/cluster_template.go @@ -21,7 +21,7 @@ import ( "github.com/aws/aws-sdk-go/service/ecs" ) -func GetClusterTemplate(tags []*ecs.Tag, stackName string, instanceTypes []string) (string, error) { +func GetClusterTemplate(tags []*ecs.Tag, stackName string) (string, error) { tagJSON, err := json.Marshal(tags) if err != nil { return "", err @@ -33,12 +33,7 @@ func GetClusterTemplate(tags []*ecs.Tag, stackName string, instanceTypes []strin return "", err } - instanceTypesJSON, err := json.Marshal(instanceTypes) - if err != nil { - return "", err - } - - return fmt.Sprintf(clusterTemplate, string(tagJSON), string(asgTagJSON), string(instanceTypesJSON)), nil + return fmt.Sprintf(clusterTemplate, string(tagJSON), string(asgTagJSON)), nil } // Autoscaling CFN tags have an additional field that determines if they are @@ -115,9 +110,6 @@ var clusterTemplate = ` "EcsInstanceType": { "Type": "String", "Description": "ECS EC2 instance type", - "Default": "` + DefaultECSInstanceType + `", - "AllowedValues": %[3]s, - "ConstraintDescription": "must be a valid EC2 instance type." }, "SpotPrice": { "Type": "Number", From 6a1d7139a9d966874dc999e5a6b3afc5660bc638 Mon Sep 17 00:00:00 2001 From: Nick Fischer Date: Wed, 8 Apr 2020 13:37:40 -0700 Subject: [PATCH 2/5] Update wrapped message for DescribeInstanceTypeOfferings error --- ecs-cli/modules/cli/cluster/cluster_app.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ecs-cli/modules/cli/cluster/cluster_app.go b/ecs-cli/modules/cli/cluster/cluster_app.go index 3053c41fa..330514672 100644 --- a/ecs-cli/modules/cli/cluster/cluster_app.go +++ b/ecs-cli/modules/cli/cluster/cluster_app.go @@ -325,7 +325,7 @@ func createCluster(context *cli.Context, awsClients *AWSClients, commandConfig * } supportedInstanceTypes, err := awsClients.EC2Client.DescribeInstanceTypeOfferings(commandConfig.Region()) if err != nil { - return errors.Wrapf(err, "No instance type found in region %s", commandConfig.Region()) + return fmt.Errorf("describe instance type offerings: %w", err) } if err = validateInstanceType(instanceType, supportedInstanceTypes); err != nil { From 06c39473e8db7b4fc65937e2563195de4444df7b Mon Sep 17 00:00:00 2001 From: Nick Fischer Date: Wed, 8 Apr 2020 15:36:51 -0700 Subject: [PATCH 3/5] Remove duplication of InstanceType parameter retrieval --- ecs-cli/modules/cli/cluster/cluster_app.go | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/ecs-cli/modules/cli/cluster/cluster_app.go b/ecs-cli/modules/cli/cluster/cluster_app.go index 330514672..c45ad0b21 100644 --- a/ecs-cli/modules/cli/cluster/cluster_app.go +++ b/ecs-cli/modules/cli/cluster/cluster_app.go @@ -319,7 +319,7 @@ func createCluster(context *cli.Context, awsClients *AWSClients, commandConfig * } if launchType == config.LaunchTypeEC2 { - instanceType, err := populateInstanceTypeParameter(cfnParams) + instanceType, err := getInstanceType(cfnParams) if err != nil { return err } @@ -406,19 +406,7 @@ func canEnableContainerInstanceTagging(client ecsclient.ECSClient) (bool, error) return false, nil } -func retrieveInstanceType(cfnParams *cloudformation.CfnStackParams) (string, error) { - param, err := cfnParams.GetParameter(ParameterKeyInstanceType) - - if err == cloudformation.ParameterNotFoundError { - return cloudformation.DefaultECSInstanceType, nil - } - if err != nil { - return "", err - } - return aws.StringValue(param.ParameterValue), nil -} - -func populateInstanceTypeParameter(cfnParams *cloudformation.CfnStackParams) (string, error) { +func getInstanceType(cfnParams *cloudformation.CfnStackParams) (string, error) { param, err := cfnParams.GetParameter(ParameterKeyInstanceType) if err == cloudformation.ParameterNotFoundError { logrus.Infof("Defaulting instance type to %s", cloudformation.DefaultECSInstanceType) @@ -430,7 +418,7 @@ func populateInstanceTypeParameter(cfnParams *cloudformation.CfnStackParams) (st return "", err } - return aws.StringValue(param.ParameterValue), err + return aws.StringValue(param.ParameterValue), nil } func validateInstanceType(instanceType string, supportedInstanceTypes []string) error { @@ -449,7 +437,7 @@ func validateInstanceType(instanceType string, supportedInstanceTypes []string) } func populateAMIID(cfnParams *cloudformation.CfnStackParams, client amimetadata.Client) error { - instanceType, err := retrieveInstanceType(cfnParams) + instanceType, err := getInstanceType(cfnParams) if err != nil { return err } From 31b15a809be742e0b0fae62ba31ba175527f3eb4 Mon Sep 17 00:00:00 2001 From: Nick Fischer Date: Tue, 14 Apr 2020 13:44:29 -0700 Subject: [PATCH 4/5] Remove extraneous comma in CloudFormation template --- ecs-cli/modules/clients/aws/cloudformation/cluster_template.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ecs-cli/modules/clients/aws/cloudformation/cluster_template.go b/ecs-cli/modules/clients/aws/cloudformation/cluster_template.go index c2149648c..c1a9a9ab8 100644 --- a/ecs-cli/modules/clients/aws/cloudformation/cluster_template.go +++ b/ecs-cli/modules/clients/aws/cloudformation/cluster_template.go @@ -109,7 +109,7 @@ var clusterTemplate = ` }, "EcsInstanceType": { "Type": "String", - "Description": "ECS EC2 instance type", + "Description": "ECS EC2 instance type" }, "SpotPrice": { "Type": "Number", From 1acd67ca10f079974d0e9400a8b2e88d5979ade0 Mon Sep 17 00:00:00 2001 From: Nick Fischer Date: Fri, 17 Apr 2020 15:03:15 -0700 Subject: [PATCH 5/5] Add Default value for EcsInstanceType CF Parameter --- ecs-cli/modules/clients/aws/cloudformation/cluster_template.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ecs-cli/modules/clients/aws/cloudformation/cluster_template.go b/ecs-cli/modules/clients/aws/cloudformation/cluster_template.go index c1a9a9ab8..8471b8347 100644 --- a/ecs-cli/modules/clients/aws/cloudformation/cluster_template.go +++ b/ecs-cli/modules/clients/aws/cloudformation/cluster_template.go @@ -109,7 +109,8 @@ var clusterTemplate = ` }, "EcsInstanceType": { "Type": "String", - "Description": "ECS EC2 instance type" + "Description": "ECS EC2 instance type", + "Default": "" }, "SpotPrice": { "Type": "Number",