From a830133d6615247bff5e3da4562e19936e2d29f8 Mon Sep 17 00:00:00 2001 From: Wanxian Yang <79273084+Lou1415926@users.noreply.github.com> Date: Tue, 30 Aug 2022 16:35:20 -0700 Subject: [PATCH] chore: env manager patcher and service discovery namespace should work with bootstrap stack (#3966) Previously, we have two fixes: 1. #3956 which patches the S3 permissions to the environment manager role if it's needed. 2. #3949 which preserves service discovery endpoint namespace from a previously deployed env stack. Fix 1 performs an environment template version check to infer whether the necessary permissions are present. This version check fails to take into account the `"bootstrap"` version, which is present when the environment is only bootstrapped (by running `env init` and not having run `env deploy`) Fix 2 grabs the old parameter from a deployed environment stack. It fails to take into consideration that, while an env stack that's on `"bootstrap"` version is not considered a "deployed environment" in Copilot, it is a deployed stack in CFN. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License. --- internal/pkg/cli/deploy/env.go | 6 ++--- internal/pkg/cli/deploy/env_test.go | 20 +++++++------- internal/pkg/cli/deploy/mocks/mock_env.go | 12 ++++----- internal/pkg/cli/deploy/patch/env.go | 5 ++++ .../deploy/cloudformation/cloudformation.go | 1 + .../cloudformation_integration_test.go | 2 +- internal/pkg/deploy/cloudformation/env.go | 27 +++++++++++++++++-- .../pkg/deploy/cloudformation/env_test.go | 25 +++++++++++++++-- .../mocks/mock_cloudformation.go | 15 +++++++++++ internal/pkg/deploy/env.go | 3 ++- 10 files changed, 91 insertions(+), 25 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 1ec7f4e11de..a8db6ab3457 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -32,7 +32,7 @@ type appResourcesGetter interface { type environmentDeployer interface { UpdateAndRenderEnvironment(conf deploycfn.StackConfiguration, bucketARN string, opts ...cloudformation.StackOption) error - EnvironmentParameters(app, env string) ([]*awscfn.Parameter, error) + DeployedEnvironmentParameters(app, env string) ([]*awscfn.Parameter, error) ForceUpdateOutputID(app, env string) (string, error) } @@ -171,7 +171,7 @@ func (d *envDeployer) GenerateCloudFormationTemplate(in *DeployEnvironmentInput) if err != nil { return nil, err } - oldParams, err := d.envDeployer.EnvironmentParameters(d.app.Name, d.env.Name) + oldParams, err := d.envDeployer.DeployedEnvironmentParameters(d.app.Name, d.env.Name) if err != nil { return nil, fmt.Errorf("describe environment stack parameters: %w", err) } @@ -200,7 +200,7 @@ func (d *envDeployer) DeployEnvironment(in *DeployEnvironmentInput) error { if err != nil { return err } - oldParams, err := d.envDeployer.EnvironmentParameters(d.app.Name, d.env.Name) + oldParams, err := d.envDeployer.DeployedEnvironmentParameters(d.app.Name, d.env.Name) if err != nil { return fmt.Errorf("describe environment stack parameters: %w", err) } diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 87a4b87b310..a2ffe660257 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -169,7 +169,7 @@ func TestEnvDeployer_GenerateCloudFormationTemplate(t *testing.T) { m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) - m.envDeployer.EXPECT().EnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, errors.New("some error")) + m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, errors.New("some error")) }, wantedError: errors.New("describe environment stack parameters: some error"), }, @@ -178,7 +178,7 @@ func TestEnvDeployer_GenerateCloudFormationTemplate(t *testing.T) { m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) - m.envDeployer.EXPECT().EnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) + m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) m.envDeployer.EXPECT().ForceUpdateOutputID(gomock.Any(), gomock.Any()).Return("", errors.New("some error")) }, wantedError: errors.New("retrieve environment stack force update ID: some error"), @@ -188,7 +188,7 @@ func TestEnvDeployer_GenerateCloudFormationTemplate(t *testing.T) { m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) - m.envDeployer.EXPECT().EnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) + m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) m.envDeployer.EXPECT().ForceUpdateOutputID(gomock.Any(), gomock.Any()).Return("", nil) m.stackSerializer.EXPECT().Template().Return("", errors.New("some error")) }, @@ -199,7 +199,7 @@ func TestEnvDeployer_GenerateCloudFormationTemplate(t *testing.T) { m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) - m.envDeployer.EXPECT().EnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) + m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) m.envDeployer.EXPECT().ForceUpdateOutputID(gomock.Any(), gomock.Any()).Return("", nil) m.stackSerializer.EXPECT().Template().Return("", nil) m.stackSerializer.EXPECT().SerializedParameters().Return("", errors.New("some error")) @@ -211,7 +211,7 @@ func TestEnvDeployer_GenerateCloudFormationTemplate(t *testing.T) { m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) - m.envDeployer.EXPECT().EnvironmentParameters(mockAppName, mockEnvName).Return(nil, nil) + m.envDeployer.EXPECT().DeployedEnvironmentParameters(mockAppName, mockEnvName).Return(nil, nil) m.envDeployer.EXPECT().ForceUpdateOutputID(gomock.Any(), gomock.Any()).Return("", nil) m.stackSerializer.EXPECT().Template().Return("aloo", nil) m.stackSerializer.EXPECT().SerializedParameters().Return("gobi", nil) @@ -307,7 +307,7 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) - m.envDeployer.EXPECT().EnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) + m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) m.envDeployer.EXPECT().ForceUpdateOutputID(gomock.Any(), gomock.Any()).Return("", nil) m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return("mockPrefixListID", nil).Times(0) m.envDeployer.EXPECT().UpdateAndRenderEnvironment(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) @@ -319,7 +319,7 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) - m.envDeployer.EXPECT().EnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, errors.New("some error")) + m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, errors.New("some error")) }, wantedError: errors.New("describe environment stack parameters: some error"), }, @@ -328,7 +328,7 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) - m.envDeployer.EXPECT().EnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) + m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) m.envDeployer.EXPECT().ForceUpdateOutputID(gomock.Any(), gomock.Any()).Return("", errors.New("some error")) }, wantedError: errors.New("retrieve environment stack force update ID: some error"), @@ -339,7 +339,7 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { S3Bucket: "mockS3Bucket", }, nil) m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return("mockPrefixListID", nil).Times(0) - m.envDeployer.EXPECT().EnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) + m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) m.envDeployer.EXPECT().ForceUpdateOutputID(gomock.Any(), gomock.Any()).Return("", nil) m.envDeployer.EXPECT().UpdateAndRenderEnvironment(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("some error")) }, @@ -351,7 +351,7 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { S3Bucket: "mockS3Bucket", }, nil) m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return("mockPrefixListID", nil).Times(0) - m.envDeployer.EXPECT().EnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) + m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) m.envDeployer.EXPECT().ForceUpdateOutputID(gomock.Any(), gomock.Any()).Return("", nil) m.envDeployer.EXPECT().UpdateAndRenderEnvironment(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) }, diff --git a/internal/pkg/cli/deploy/mocks/mock_env.go b/internal/pkg/cli/deploy/mocks/mock_env.go index 7e87c9f7dbe..84bd252619c 100644 --- a/internal/pkg/cli/deploy/mocks/mock_env.go +++ b/internal/pkg/cli/deploy/mocks/mock_env.go @@ -76,19 +76,19 @@ func (m *MockenvironmentDeployer) EXPECT() *MockenvironmentDeployerMockRecorder return m.recorder } -// EnvironmentParameters mocks base method. -func (m *MockenvironmentDeployer) EnvironmentParameters(app, env string) ([]*cloudformation.Parameter, error) { +// DeployedEnvironmentParameters mocks base method. +func (m *MockenvironmentDeployer) DeployedEnvironmentParameters(app, env string) ([]*cloudformation.Parameter, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "EnvironmentParameters", app, env) + ret := m.ctrl.Call(m, "DeployedEnvironmentParameters", app, env) ret0, _ := ret[0].([]*cloudformation.Parameter) ret1, _ := ret[1].(error) return ret0, ret1 } -// EnvironmentParameters indicates an expected call of EnvironmentParameters. -func (mr *MockenvironmentDeployerMockRecorder) EnvironmentParameters(app, env interface{}) *gomock.Call { +// DeployedEnvironmentParameters indicates an expected call of DeployedEnvironmentParameters. +func (mr *MockenvironmentDeployerMockRecorder) DeployedEnvironmentParameters(app, env interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EnvironmentParameters", reflect.TypeOf((*MockenvironmentDeployer)(nil).EnvironmentParameters), app, env) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeployedEnvironmentParameters", reflect.TypeOf((*MockenvironmentDeployer)(nil).DeployedEnvironmentParameters), app, env) } // ForceUpdateOutputID mocks base method. diff --git a/internal/pkg/cli/deploy/patch/env.go b/internal/pkg/cli/deploy/patch/env.go index 248fdfaa458..65d28b7021b 100644 --- a/internal/pkg/cli/deploy/patch/env.go +++ b/internal/pkg/cli/deploy/patch/env.go @@ -12,6 +12,7 @@ import ( "github.com/aws/copilot-cli/internal/pkg/aws/cloudformation" "github.com/aws/copilot-cli/internal/pkg/aws/s3" "github.com/aws/copilot-cli/internal/pkg/config" + "github.com/aws/copilot-cli/internal/pkg/deploy" "github.com/aws/copilot-cli/internal/pkg/term/log" "golang.org/x/mod/semver" "gopkg.in/yaml.v3" @@ -122,6 +123,10 @@ func isManagerRoleAllowedToUpload(body string) (bool, error) { if err := yaml.Unmarshal([]byte(body), &tpl); err != nil { return false, fmt.Errorf("unmarshal environment template to detect Metadata.Version: %v", err) } + if tpl.Metadata.Version == deploy.EnvTemplateVersionBootstrap { + // "bootstrap" version is introduced after v1.9.0. The environment manager roles must have had the permissions. + return true, nil + } if !semver.IsValid(tpl.Metadata.Version) { // The template doesn't contain a version. return false, nil } diff --git a/internal/pkg/deploy/cloudformation/cloudformation.go b/internal/pkg/deploy/cloudformation/cloudformation.go index 51d20ab1f22..e395bde6e43 100644 --- a/internal/pkg/deploy/cloudformation/cloudformation.go +++ b/internal/pkg/deploy/cloudformation/cloudformation.go @@ -79,6 +79,7 @@ type cfnClient interface { ErrorEvents(stackName string) ([]cloudformation.StackEvent, error) Outputs(stack *cloudformation.Stack) (map[string]string, error) StackResources(name string) ([]*cloudformation.StackResource, error) + Metadata(opts cloudformation.MetadataOpts) (string, error) // Methods vended by the aws sdk struct. DescribeStackEvents(*sdkcloudformation.DescribeStackEventsInput) (*sdkcloudformation.DescribeStackEventsOutput, error) diff --git a/internal/pkg/deploy/cloudformation/cloudformation_integration_test.go b/internal/pkg/deploy/cloudformation/cloudformation_integration_test.go index 93a14609442..4ea4ac27cf0 100644 --- a/internal/pkg/deploy/cloudformation/cloudformation_integration_test.go +++ b/internal/pkg/deploy/cloudformation/cloudformation_integration_test.go @@ -501,7 +501,7 @@ func Test_Environment_Deployment_Integration(t *testing.T) { } // Deploy the environment and wait for it to be complete. - oldParams, err := deployer.EnvironmentParameters(environmentToDeploy.App.Name, environmentToDeploy.Name) + oldParams, err := deployer.DeployedEnvironmentParameters(environmentToDeploy.App.Name, environmentToDeploy.Name) require.NoError(t, err) lastForceUpdateID, err := deployer.ForceUpdateOutputID(environmentToDeploy.App.Name, environmentToDeploy.Name) require.NoError(t, err) diff --git a/internal/pkg/deploy/cloudformation/env.go b/internal/pkg/deploy/cloudformation/env.go index 040e2457d4b..551c762c8a8 100644 --- a/internal/pkg/deploy/cloudformation/env.go +++ b/internal/pkg/deploy/cloudformation/env.go @@ -10,6 +10,7 @@ import ( "github.com/aws/aws-sdk-go/aws/arn" "github.com/aws/copilot-cli/internal/pkg/template" + "gopkg.in/yaml.v3" "github.com/aws/aws-sdk-go/aws" awscfn "github.com/aws/aws-sdk-go/service/cloudformation" @@ -116,8 +117,15 @@ func (cf CloudFormation) ForceUpdateOutputID(app, env string) (string, error) { return "", nil } -// EnvironmentParameters returns the environment stack's parameters. -func (cf CloudFormation) EnvironmentParameters(appName, envName string) ([]*awscfn.Parameter, error) { +// DeployedEnvironmentParameters returns the environment stack's parameters. +func (cf CloudFormation) DeployedEnvironmentParameters(appName, envName string) ([]*awscfn.Parameter, error) { + isInitial, err := cf.isInitialDeployment(appName, envName) + if err != nil { + return nil, err + } + if isInitial { + return nil, nil + } out, err := cf.cachedStack(stack.NameForEnv(appName, envName)) if err != nil { return nil, err @@ -188,3 +196,18 @@ func (cf CloudFormation) cachedStack(stackName string) (*cloudformation.StackDes cf.cachedDeployedStack = stackDescr return cf.cachedDeployedStack, nil } + +// isInitialDeployment returns whether this is the first deployment of the environment stack. +func (cf CloudFormation) isInitialDeployment(appName, envName string) (bool, error) { + raw, err := cf.cfnClient.Metadata(cloudformation.MetadataWithStackName(stack.NameForEnv(appName, envName))) + if err != nil { + return false, fmt.Errorf("get metadata of stack %q: %w", stack.NameForEnv(appName, envName), err) + } + metadata := struct { + Version string `yaml:"Version"` + }{} + if err := yaml.Unmarshal([]byte(raw), &metadata); err != nil { + return false, fmt.Errorf("unmarshal Metadata property to read Version: %w", err) + } + return metadata.Version == deploy.EnvTemplateVersionBootstrap, nil +} diff --git a/internal/pkg/deploy/cloudformation/env_test.go b/internal/pkg/deploy/cloudformation/env_test.go index 631472bfb96..171e5f038dc 100644 --- a/internal/pkg/deploy/cloudformation/env_test.go +++ b/internal/pkg/deploy/cloudformation/env_test.go @@ -48,7 +48,7 @@ func TestCloudFormation_EnvironmentTemplate(t *testing.T) { } } -func TestCloudFormation_EnvironmentParameters(t *testing.T) { +func TestCloudFormation_DeployedEnvironmentParameters(t *testing.T) { testCases := map[string]struct { inAppName string inEnvName string @@ -57,11 +57,31 @@ func TestCloudFormation_EnvironmentParameters(t *testing.T) { wantedParams []*awscfn.Parameter wantedErr error }{ + "error retrieving metadata": { + inAppName: "phonetool", + inEnvName: "test", + inClient: func(ctrl *gomock.Controller) *mocks.MockcfnClient { + m := mocks.NewMockcfnClient(ctrl) + m.EXPECT().Metadata(gomock.Any()).Return("", errors.New("some error")) + return m + }, + wantedErr: errors.New("get metadata of stack \"phonetool-test\": some error"), + }, + "returns nil if the version is bootstrap": { + inAppName: "phonetool", + inEnvName: "test", + inClient: func(ctrl *gomock.Controller) *mocks.MockcfnClient { + m := mocks.NewMockcfnClient(ctrl) + m.EXPECT().Metadata(gomock.Any()).Return(`Version: bootstrap`, nil) + return m + }, + }, "should return stack parameters from a stack description": { inAppName: "phonetool", inEnvName: "test", inClient: func(ctrl *gomock.Controller) *mocks.MockcfnClient { m := mocks.NewMockcfnClient(ctrl) + m.EXPECT().Metadata(gomock.Any()).Return(`Version: `, nil) m.EXPECT().Describe("phonetool-test").Return(&cloudformation.StackDescription{ Parameters: []*awscfn.Parameter{ { @@ -85,6 +105,7 @@ func TestCloudFormation_EnvironmentParameters(t *testing.T) { inEnvName: "test", inClient: func(ctrl *gomock.Controller) *mocks.MockcfnClient { m := mocks.NewMockcfnClient(ctrl) + m.EXPECT().Metadata(gomock.Any()).Return(`Version: v1.21.0`, nil) m.EXPECT().Describe(gomock.Any()).Return(nil, errors.New("some error")) return m }, @@ -102,7 +123,7 @@ func TestCloudFormation_EnvironmentParameters(t *testing.T) { } // WHEN - actual, err := cf.EnvironmentParameters(tc.inAppName, tc.inEnvName) + actual, err := cf.DeployedEnvironmentParameters(tc.inAppName, tc.inEnvName) if tc.wantedErr != nil { require.EqualError(t, err, tc.wantedErr.Error()) } else { diff --git a/internal/pkg/deploy/cloudformation/mocks/mock_cloudformation.go b/internal/pkg/deploy/cloudformation/mocks/mock_cloudformation.go index 9d6cac5bec8..e62356185eb 100644 --- a/internal/pkg/deploy/cloudformation/mocks/mock_cloudformation.go +++ b/internal/pkg/deploy/cloudformation/mocks/mock_cloudformation.go @@ -334,6 +334,21 @@ func (mr *MockcfnClientMockRecorder) ListStacksWithTags(tags interface{}) *gomoc return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListStacksWithTags", reflect.TypeOf((*MockcfnClient)(nil).ListStacksWithTags), tags) } +// Metadata mocks base method. +func (m *MockcfnClient) Metadata(opts cloudformation0.MetadataOpts) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Metadata", opts) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Metadata indicates an expected call of Metadata. +func (mr *MockcfnClientMockRecorder) Metadata(opts interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Metadata", reflect.TypeOf((*MockcfnClient)(nil).Metadata), opts) +} + // Outputs mocks base method. func (m *MockcfnClient) Outputs(stack *cloudformation0.Stack) (map[string]string, error) { m.ctrl.T.Helper() diff --git a/internal/pkg/deploy/env.go b/internal/pkg/deploy/env.go index d6ab1ff2e7f..566947f11d4 100644 --- a/internal/pkg/deploy/env.go +++ b/internal/pkg/deploy/env.go @@ -14,7 +14,8 @@ const ( // LegacyEnvTemplateVersion is the version associated with the environment template before we started versioning. LegacyEnvTemplateVersion = "v0.0.0" // LatestEnvTemplateVersion is the latest version number available for environment templates. - LatestEnvTemplateVersion = "v1.12.2" + LatestEnvTemplateVersion = "v1.12.2" + EnvTemplateVersionBootstrap = "bootstrap" ) // CreateEnvironmentInput holds the fields required to deploy an environment.