Skip to content

Commit

Permalink
fix: ensure EnvManagerRole template has permissions to upload artifac…
Browse files Browse the repository at this point in the history 
…ts (#3567)

Related #3556 

Tested `env upgrade` from the following versions:
- [x] `copilot v1.0` 
- [x] `copilot v1.13`
- [x] `copilot v1.17`  

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
  • Loading branch information
@efekarakus
efekarakus authored May 12, 2022
1 parent c9760fc commit 61a05a6
Show file tree
Hide file tree
Showing 2 changed files with 126 additions and 14 deletions.
67 changes: 64 additions & 3 deletions internal/pkg/cli/env_upgrade.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package cli
import (
"errors"
"fmt"
"strings"

"gopkg.in/yaml.v3"

Expand All @@ -15,6 +16,7 @@ import (

"github.com/aws/aws-sdk-go/aws/endpoints"

awscfn "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/aws/sessions"
"github.com/aws/copilot-cli/internal/pkg/config"
Expand Down Expand Up @@ -241,9 +243,7 @@ func (o *envUpgradeOpts) ensureManagerRoleIsAllowedToUpload(env *config.Environm
if ok {
return nil
}
// TODO(efekarakus): if the role isn't allowed to upload, modify the template to add the permission and call cfn.UpdateEnvironmentTemplate
// TODO(efekarakus): remove this error.
return errors.New("cannot upload artifacts")
return o.grantManagerRolePermissionToUpload(cfn, env.App, env.Name, env.ExecutionRoleARN, body, s3.FormatARN(endpoints.AwsPartitionID, bucketName))
}

func (o *envUpgradeOpts) isManagerRoleAllowedToUpload(body string) (bool, error) {
Expand All @@ -266,6 +266,67 @@ func (o *envUpgradeOpts) isManagerRoleAllowedToUpload(body string) (bool, error)
return true, nil
}

func (o *envUpgradeOpts) grantManagerRolePermissionToUpload(cfn environmentDeployer, app, env, execRole, body, bucketARN string) error {
// Detect which line number the EnvironmentManagerRole's PolicyDocument Statement is at.
// We will add additional permissions after that line.
type Template struct {
Resources struct {
ManagerRole struct {
Properties struct {
Policies []struct {
Document struct {
Statements yaml.Node `yaml:"Statement"`
} `yaml:"PolicyDocument"`
} `yaml:"Policies"`
} `yaml:"Properties"`
} `yaml:"EnvironmentManagerRole"`
} `yaml:"Resources"`
}

var tpl Template
if err := yaml.Unmarshal([]byte(body), &tpl); err != nil {
return fmt.Errorf("unmarshal environment template to find EnvironmentManagerRole policy statement: %v", err)
}
if len(tpl.Resources.ManagerRole.Properties.Policies) == 0 {
return errors.New("unable to find policies for the EnvironmentManagerRole")
}
// lines and columns are 1-indexed, so we have to substract one from each.
statementLineIndex := tpl.Resources.ManagerRole.Properties.Policies[0].Document.Statements.Line - 1
numSpaces := tpl.Resources.ManagerRole.Properties.Policies[0].Document.Statements.Column - 1
pad := strings.Repeat(" ", numSpaces)

// Create the additional permissions needed with the appropriate indentation.
permissions := fmt.Sprintf(`- Sid: PatchPutObjectsToArtifactBucket
Effect: Allow
Action:
- s3:PutObject
- s3:PutObjectAcl
Resource:
- %s
- %s/*`, bucketARN, bucketARN)
permissions = pad + strings.Replace(permissions, "\n", "\n"+pad, -1)

// Add the new permissions to the body.
lines := strings.Split(body, "\n")
linesBefore := lines[:statementLineIndex]
linesAfter := lines[statementLineIndex:]
updatedLines := append(linesBefore, append(strings.Split(permissions, "\n"), linesAfter...)...)
updatedBody := strings.Join(updatedLines, "\n")

// Update the Environment template with the new content.
// CloudFormation is the only entity that's allowed to update the EnvManagerRole so we have to go through this route.
// See #3556.
var errEmptyChangeSet *awscfn.ErrChangeSetEmpty
o.prog.Start("Update the environment's manager role with permission to upload artifacts to S3")
err := cfn.UpdateEnvironmentTemplate(app, env, updatedBody, execRole)
if err != nil && !errors.As(err, &errEmptyChangeSet) {
o.prog.Stop(log.Serrorln("Unable to update the environment's manager role with upload artifacts permission"))
return fmt.Errorf("update environment template with PutObject permissions: %v", err)
}
o.prog.Stop(log.Ssuccessln("Updated the environment's manager role with permissions to upload artifacts to S3"))
return nil
}

func (o *envUpgradeOpts) upgrade(env *config.Environment, app *config.Application,
artifactBucketARN, artifactBucketKeyARN string, customResourcesURLs map[string]string) (err error) {
version, err := o.envVersion(env.Name)
Expand Down
73 changes: 62 additions & 11 deletions internal/pkg/cli/env_upgrade_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ package cli

import (
"errors"
"fmt"
"testing"

"github.com/aws/copilot-cli/internal/pkg/aws/cloudformation"

"github.com/aws/copilot-cli/internal/pkg/cli/mocks"
"github.com/aws/copilot-cli/internal/pkg/config"
"github.com/aws/copilot-cli/internal/pkg/deploy"
Expand Down Expand Up @@ -201,14 +204,15 @@ func TestEnvUpgradeOpts_Execute(t *testing.T) {
given func(ctrl *gomock.Controller) *envUpgradeOpts
wantedErr error
}{
"should return an error if the environment template version cannot upload artifacts": {
"should return an error if the environment template cannot be updated to grant upload artifacts permissions": {
given: func(ctrl *gomock.Controller) *envUpgradeOpts {
mockStore := mocks.NewMockstore(ctrl)
mockStore.EXPECT().ListEnvironments("phonetool").Return([]*config.Environment{
{
Name: "test",
App: "phonetool",
Region: "us-west-2",
Name: "test",
App: "phonetool",
Region: "us-west-2",
ExecutionRoleARN: "execRoleARN",
},
}, nil)
mockStore.EXPECT().GetApplication("phonetool").Return(&config.Application{Name: "phonetool"}, nil)
Expand All @@ -218,18 +222,50 @@ func TestEnvUpgradeOpts_Execute(t *testing.T) {
S3Bucket: "mockBucket",
}, nil)

mockProg := mocks.NewMockprogress(ctrl)
mockProg.EXPECT().Start(gomock.Any())
mockProg.EXPECT().Stop(gomock.Any())

mockEnvDeployer := mocks.NewMockenvironmentDeployer(ctrl)
mockEnvDeployer.EXPECT().EnvironmentTemplate("phonetool", gomock.Any()).Return(`
mockEnvDeployer.EXPECT().EnvironmentTemplate("phonetool", "test").Return(`
Metadata:
Version: v1.7.0
Resources:
EnvironmentManagerRole:
Properties:
Policies:
- PolicyDocument:
Statement:
- Sid: CloudwatchLogs
OtherResource:
`, nil)

mockEnvDeployer.EXPECT().UpdateEnvironmentTemplate("phonetool", "test", `
Metadata:
Version: v1.7.0
Resources:
EnvironmentManagerRole:
Properties:
Policies:
- PolicyDocument:
Statement:
- Sid: PatchPutObjectsToArtifactBucket
Effect: Allow
Action:
- s3:PutObject
- s3:PutObjectAcl
Resource:
- arn:aws:s3:::mockBucket
- arn:aws:s3:::mockBucket/*
- Sid: CloudwatchLogs
OtherResource:
`, "execRoleARN").Return(errors.New("some unexpected error"))
return &envUpgradeOpts{
envUpgradeVars: envUpgradeVars{
appName: "phonetool",
all: true,
},
store: mockStore,
prog: mockProg,
appCFN: mockAppCFN,
newEnvDeployer: func(_ *config.Environment) (environmentDeployer, error) {
return mockEnvDeployer, nil
Expand All @@ -239,7 +275,7 @@ Metadata:
},
}
},
wantedErr: errors.New("cannot upload artifacts"),
wantedErr: errors.New("update environment template with PutObject permissions: some unexpected error"),
},
"should skip upgrading if the environment version is already at least latest": {
given: func(ctrl *gomock.Controller) *envUpgradeOpts {
Expand Down Expand Up @@ -298,14 +334,14 @@ Metadata:
}
},
},
"should upgrade non-legacy environments with UpgradeEnvironment call": {
"should upgrade non-legacy environments and ignore ErrChangeSet if the environment template already has permissions to upload artifacts": {
given: func(ctrl *gomock.Controller) *envUpgradeOpts {
mockEnvTpl := mocks.NewMockversionGetter(ctrl)
mockEnvTpl.EXPECT().Version().Return("v0.1.0", nil) // Legacy versions are v0.0.0

mockProg := mocks.NewMockprogress(ctrl)
mockProg.EXPECT().Start(gomock.Any())
mockProg.EXPECT().Stop(gomock.Any())
mockProg.EXPECT().Start(gomock.Any()).AnyTimes()
mockProg.EXPECT().Stop(gomock.Any()).AnyTimes()

mockStore := mocks.NewMockstore(ctrl)
mockStore.EXPECT().GetEnvironment("phonetool", "test").
Expand Down Expand Up @@ -333,8 +369,23 @@ Metadata:
mockEnvDeployer := mocks.NewMockenvironmentDeployer(ctrl)
mockEnvDeployer.EXPECT().EnvironmentTemplate("phonetool", "test").Return(`
Metadata:
Version: v1.9.0
Version: v1.1.0
Resources:
EnvironmentManagerRole:
Properties:
Policies:
- PolicyDocument:
Statement:
- Sid: PatchPutObjectsToArtifactBucket
Effect: Allow
Action:
- s3:PutObject
- s3:PutObjectAcl
Resource:
- arn:aws:s3:::mockBucket
- arn:aws:s3:::mockBucket/*
`, nil)
mockEnvDeployer.EXPECT().UpdateEnvironmentTemplate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("wrapped err: %w", &cloudformation.ErrChangeSetEmpty{}))

mockUploader := mocks.NewMockcustomResourcesUploader(ctrl)
mockUploader.EXPECT().UploadEnvironmentCustomResources(gomock.Any()).Return(map[string]string{"mockCustomResource": "mockURL"}, nil)
Expand Down

0 comments on commit 61a05a6

Please sign in to comment.