Skip to content

fix: Use PublicSubnet from stack to run task #2024

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mergify merged 13 commits into from
Mar 10, 2021
Merged

fix: Use PublicSubnet from stack to run task #2024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ac0168d
feat: load VPC environment form stack outputs
ota42y Mar 5, 2021
60edf43
fix: get public subnets from stack outputs
ota42y Mar 5, 2021
11bf888
Update internal/pkg/cli/task_run.go
ota42y Mar 9, 2021
0d8d31f
Update internal/pkg/describe/env.go
ota42y Mar 9, 2021
73156a3
Update internal/pkg/cli/task_run.go
ota42y Mar 9, 2021
59743f9
Update internal/pkg/task/env_runner.go
ota42y Mar 9, 2021
8a868c6
use fmtErrDescribeEnvironment
ota42y Mar 9, 2021
925fa23
use Output name in internal/pkg/deploy/cloudformation/stack/env.go
ota42y Mar 9, 2021
d169ce0
fix error message in test
ota42y Mar 9, 2021
5a411a0
Update internal/pkg/cli/task_run.go
ota42y Mar 10, 2021
483ff58
use aws.StringValue to check Stack data
ota42y Mar 10, 2021
393e294
don't use pointer for EnvironmentVPC
ota42y Mar 10, 2021
0db2968
Merge branch 'mainline' into feature/use_subnets_from_stack
mergify[bot] Mar 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/pkg/cli/env_show_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ Resources
)
},

wantedContent: "{\"environment\":{\"app\":\"testApp\",\"name\":\"testEnv\",\"region\":\"us-west-2\",\"accountID\":\"123456789012\",\"prod\":false,\"registryURL\":\"\",\"executionRoleARN\":\"\",\"managerRoleARN\":\"\"},\"services\":[{\"app\":\"testApp\",\"name\":\"testSvc1\",\"type\":\"load-balanced\"},{\"app\":\"testApp\",\"name\":\"testSvc2\",\"type\":\"load-balanced\"},{\"app\":\"testApp\",\"name\":\"testSvc3\",\"type\":\"load-balanced\"}],\"tags\":{\"copilot-application\":\"testApp\",\"copilot-environment\":\"testEnv\",\"key1\":\"value1\",\"key2\":\"value2\"},\"resources\":[{\"type\":\"AWS::IAM::Role\",\"physicalID\":\"testApp-testEnv-CFNExecutionRole\"},{\"type\":\"testApp-testEnv-Cluster\",\"physicalID\":\"AWS::ECS::Cluster-jI63pYBWU6BZ\"}]}\n",
wantedContent: "{\"environment\":{\"app\":\"testApp\",\"name\":\"testEnv\",\"region\":\"us-west-2\",\"accountID\":\"123456789012\",\"prod\":false,\"registryURL\":\"\",\"executionRoleARN\":\"\",\"managerRoleARN\":\"\"},\"services\":[{\"app\":\"testApp\",\"name\":\"testSvc1\",\"type\":\"load-balanced\"},{\"app\":\"testApp\",\"name\":\"testSvc2\",\"type\":\"load-balanced\"},{\"app\":\"testApp\",\"name\":\"testSvc3\",\"type\":\"load-balanced\"}],\"tags\":{\"copilot-application\":\"testApp\",\"copilot-environment\":\"testEnv\",\"key1\":\"value1\",\"key2\":\"value2\"},\"resources\":[{\"type\":\"AWS::IAM::Role\",\"physicalID\":\"testApp-testEnv-CFNExecutionRole\"},{\"type\":\"testApp-testEnv-Cluster\",\"physicalID\":\"AWS::ECS::Cluster-jI63pYBWU6BZ\"}],\"environment_vpc\":{\"ID\":\"\",\"PublicSubnetIDs\":null,\"PrivateSubnetIDs\":null}}\n",
},
}

Expand Down
35 changes: 28 additions & 7 deletions internal/pkg/cli/task_run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"strings"

awscloudformation "github.com/aws/copilot-cli/internal/pkg/aws/cloudformation"
"github.com/aws/copilot-cli/internal/pkg/describe"
"github.com/aws/copilot-cli/internal/pkg/logging"
"github.com/aws/copilot-cli/internal/pkg/term/prompt"

Expand Down Expand Up @@ -134,7 +135,10 @@ func newTaskRunOpts(vars runTaskVars) (*runTaskOpts, error) {
}

opts.configureRuntimeOpts = func() error {
opts.runner = opts.configureRunner()
opts.runner, err = opts.configureRunner()
if err != nil {
return fmt.Errorf("configure task runner: %w", err)
}
opts.deployer = cloudformation.New(opts.sess)
opts.defaultClusterGetter = awsecs.New(opts.sess)
return nil
Expand All @@ -157,22 +161,39 @@ func newTaskRunOpts(vars runTaskVars) (*runTaskOpts, error) {
return &opts, nil
}

func (o *runTaskOpts) configureRunner() taskRunner {
func (o *runTaskOpts) configureRunner() (taskRunner, error) {
vpcGetter := ec2.New(o.sess)
ecsService := awsecs.New(o.sess)

if o.env != "" {
deployStore, err := deploy.NewStore(o.store)
if err != nil {
return nil, fmt.Errorf("connect to copilot deploy store: %w", err)
}

d, err := describe.NewEnvDescriber(describe.NewEnvDescriberConfig{
App: o.appName,
Env: o.env,
ConfigStore: o.store,
DeployStore: deployStore,
EnableResources: false, // We don't need to show detailed resources.
})
if err != nil {
return nil, fmt.Errorf("create describer for environment %s in application %s: %w", o.env, o.appName, err)
}

return &task.EnvRunner{
Count: o.count,
GroupName: o.groupName,

App: o.appName,
Env: o.env,

VPCGetter: vpcGetter,
ClusterGetter: ecs.New(o.sess),
Starter: ecsService,
}
VPCGetter: vpcGetter,
ClusterGetter: ecs.New(o.sess),
Starter: ecsService,
EnvironmentDescriber: d,
}, nil
}

return &task.NetworkConfigRunner{
Expand All @@ -185,7 +206,7 @@ func (o *runTaskOpts) configureRunner() taskRunner {
VPCGetter: vpcGetter,
ClusterGetter: ecsService,
Starter: ecsService,
}
}, nil

}

Expand Down
59 changes: 39 additions & 20 deletions internal/pkg/describe/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"strings"
"text/tabwriter"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/copilot-cli/internal/pkg/aws/sessions"
"github.com/aws/copilot-cli/internal/pkg/config"
"github.com/aws/copilot-cli/internal/pkg/deploy"
Expand All @@ -21,10 +22,18 @@ import (

// EnvDescription contains the information about an environment.
type EnvDescription struct {
Environment *config.Environment `json:"environment"`
Services []*config.Workload `json:"services"`
Tags map[string]string `json:"tags,omitempty"`
Resources []*CfnResource `json:"resources,omitempty"`
Environment *config.Environment `json:"environment"`
Services []*config.Workload `json:"services"`
Tags map[string]string `json:"tags,omitempty"`
Resources []*CfnResource `json:"resources,omitempty"`
EnvironmentVPC EnvironmentVPC `json:"environment_vpc"`
}

// EnvironmentVPC holds the ID of the environment's VPC configuration.
type EnvironmentVPC struct {
ID string
PublicSubnetIDs []string
PrivateSubnetIDs []string
}

// EnvDescriber retrieves information about an environment.
Expand Down Expand Up @@ -76,9 +85,9 @@ func (d *EnvDescriber) Describe() (*EnvDescription, error) {
return nil, err
}

tags, err := d.stackTags()
tags, environmentVPC, err := d.loadStackInfo()
if err != nil {
return nil, fmt.Errorf("retrieve environment tags: %w", err)
return nil, err
}

var stackResources []*CfnResource
Expand All @@ -90,10 +99,11 @@ func (d *EnvDescriber) Describe() (*EnvDescription, error) {
}

return &EnvDescription{
Environment: d.env,
Services: svcs,
Tags: tags,
Resources: stackResources,
Environment: d.env,
Services: svcs,
Tags: tags,
Resources: stackResources,
EnvironmentVPC: environmentVPC,
}, nil
}

Expand All @@ -119,23 +129,32 @@ func (d *EnvDescriber) Version() (string, error) {
return metadata.Version, nil
}

// EnvironmentVPC holds the ID of the environment's VPC configuration.
type EnvironmentVPC struct {
ID string
PublicSubnetIDs []string
PrivateSubnetIDs []string
}

func (d *EnvDescriber) stackTags() (map[string]string, error) {
func (d *EnvDescriber) loadStackInfo() (map[string]string, EnvironmentVPC, error) {
environmentVPC := EnvironmentVPC{}
tags := make(map[string]string)

envStack, err := d.stackDescriber.Stack(stack.NameForEnv(d.app, d.env.Name))
if err != nil {
return nil, err
return nil, environmentVPC, fmt.Errorf("retrieve environment stack: %w", err)
}
for _, tag := range envStack.Tags {
tags[*tag.Key] = *tag.Value
}
return tags, nil

for _, out := range envStack.Outputs {
value := aws.StringValue(out.OutputValue)

switch aws.StringValue(out.OutputKey) {
case stack.EnvOutputVPCID:
environmentVPC.ID = value
case stack.EnvOutputPublicSubnets:
environmentVPC.PublicSubnetIDs = strings.Split(value, ",")
case stack.EnvOutputPrivateSubnets:
environmentVPC.PrivateSubnetIDs = strings.Split(value, ",")
}
}

return tags, environmentVPC, nil
}

func (d *EnvDescriber) filterDeployedSvcs() ([]*config.Workload, error) {
Expand Down
47 changes: 37 additions & 10 deletions internal/pkg/describe/env_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,20 @@ func TestEnvDescriber_Describe(t *testing.T) {
Value: aws.String("testEnv"),
},
}
stackOutputs := []*cloudformation.Output{
{
OutputKey: aws.String("VpcId"),
OutputValue: aws.String("vpc-012abcd345"),
},
{
OutputKey: aws.String("PublicSubnets"),
OutputValue: aws.String("subnet-0789ab,subnet-0123cd"),
},
{
OutputKey: aws.String("PrivateSubnets"),
OutputValue: aws.String("subnet-023ff,subnet-04af"),
},
}
mockResource1 := &cloudformation.StackResource{
PhysicalResourceId: aws.String("testApp-testEnv-CFNExecutionRole"),
ResourceType: aws.String("AWS::IAM::Role"),
Expand Down Expand Up @@ -121,7 +135,7 @@ func TestEnvDescriber_Describe(t *testing.T) {
m.stackDescriber.EXPECT().Stack("testApp-testEnv").Return(nil, mockError),
)
},
wantedError: fmt.Errorf("retrieve environment tags: some error"),
wantedError: fmt.Errorf("retrieve environment stack: some error"),
},
"error if fail to get env resources": {
shouldOutputResources: true,
Expand All @@ -133,7 +147,8 @@ func TestEnvDescriber_Describe(t *testing.T) {
m.deployStoreSvc.EXPECT().ListDeployedServices(testApp, testEnv.Name).
Return([]string{"testSvc1", "testSvc2"}, nil),
m.stackDescriber.EXPECT().Stack("testApp-testEnv").Return(&cloudformation.Stack{
Tags: stackTags,
Tags: stackTags,
Outputs: stackOutputs,
}, nil),
m.stackDescriber.EXPECT().StackResources("testApp-testEnv").Return(nil, mockError),
)
Expand All @@ -150,14 +165,20 @@ func TestEnvDescriber_Describe(t *testing.T) {
m.deployStoreSvc.EXPECT().ListDeployedServices(testApp, testEnv.Name).
Return([]string{"testSvc1", "testSvc2"}, nil),
m.stackDescriber.EXPECT().Stack("testApp-testEnv").Return(&cloudformation.Stack{
Tags: stackTags,
Tags: stackTags,
Outputs: stackOutputs,
}, nil),
)
},
wantedEnv: &EnvDescription{
Environment: testEnv,
Services: envSvcs,
Tags: map[string]string{"copilot-application": "testApp", "copilot-environment": "testEnv"},
EnvironmentVPC: EnvironmentVPC{
ID: "vpc-012abcd345",
PublicSubnetIDs: []string{"subnet-0789ab", "subnet-0123cd"},
PrivateSubnetIDs: []string{"subnet-023ff", "subnet-04af"},
},
},
},
"success with resources": {
Expand All @@ -170,7 +191,8 @@ func TestEnvDescriber_Describe(t *testing.T) {
m.deployStoreSvc.EXPECT().ListDeployedServices(testApp, testEnv.Name).
Return([]string{"testSvc1", "testSvc2"}, nil),
m.stackDescriber.EXPECT().Stack("testApp-testEnv").Return(&cloudformation.Stack{
Tags: stackTags,
Tags: stackTags,
Outputs: stackOutputs,
}, nil),
m.stackDescriber.EXPECT().StackResources("testApp-testEnv").Return([]*cloudformation.StackResource{
mockResource1,
Expand All @@ -183,10 +205,14 @@ func TestEnvDescriber_Describe(t *testing.T) {
Services: envSvcs,
Tags: map[string]string{"copilot-application": "testApp", "copilot-environment": "testEnv"},
Resources: wantedResources,
EnvironmentVPC: EnvironmentVPC{
ID: "vpc-012abcd345",
PublicSubnetIDs: []string{"subnet-0789ab", "subnet-0123cd"},
PrivateSubnetIDs: []string{"subnet-023ff", "subnet-04af"},
},
},
},
}

for name, tc := range testCases {
t.Run(name, func(t *testing.T) {
// GIVEN
Expand Down Expand Up @@ -315,17 +341,18 @@ func TestEnvDescription_JSONString(t *testing.T) {
Type: "load-balanced",
}
allSvcs := []*config.Workload{testSvc1, testSvc2, testSvc3}
wantedContent := "{\"environment\":{\"app\":\"testApp\",\"name\":\"testEnv\",\"region\":\"us-west-2\",\"accountID\":\"123456789012\",\"prod\":false,\"registryURL\":\"\",\"executionRoleARN\":\"\",\"managerRoleARN\":\"\",\"customConfig\":{}},\"services\":[{\"app\":\"testApp\",\"name\":\"testSvc1\",\"type\":\"load-balanced\"},{\"app\":\"testApp\",\"name\":\"testSvc2\",\"type\":\"load-balanced\"},{\"app\":\"testApp\",\"name\":\"testSvc3\",\"type\":\"load-balanced\"}],\"tags\":{\"key1\":\"value1\",\"key2\":\"value2\"},\"resources\":[{\"type\":\"AWS::IAM::Role\",\"physicalID\":\"testApp-testEnv-CFNExecutionRole\"},{\"type\":\"testApp-testEnv-Cluster\",\"physicalID\":\"AWS::ECS::Cluster-jI63pYBWU6BZ\"}]}\n"
wantedContent := "{\"environment\":{\"app\":\"testApp\",\"name\":\"testEnv\",\"region\":\"us-west-2\",\"accountID\":\"123456789012\",\"prod\":false,\"registryURL\":\"\",\"executionRoleARN\":\"\",\"managerRoleARN\":\"\",\"customConfig\":{}},\"services\":[{\"app\":\"testApp\",\"name\":\"testSvc1\",\"type\":\"load-balanced\"},{\"app\":\"testApp\",\"name\":\"testSvc2\",\"type\":\"load-balanced\"},{\"app\":\"testApp\",\"name\":\"testSvc3\",\"type\":\"load-balanced\"}],\"tags\":{\"key1\":\"value1\",\"key2\":\"value2\"},\"resources\":[{\"type\":\"AWS::IAM::Role\",\"physicalID\":\"testApp-testEnv-CFNExecutionRole\"},{\"type\":\"testApp-testEnv-Cluster\",\"physicalID\":\"AWS::ECS::Cluster-jI63pYBWU6BZ\"}],\"environment_vpc\":{\"ID\":\"\",\"PublicSubnetIDs\":null,\"PrivateSubnetIDs\":null}}\n"

// GIVEN
ctrl := gomock.NewController(t)
defer ctrl.Finish()

d := &EnvDescription{
Environment: testEnv,
Services: allSvcs,
Tags: testApp.Tags,
Resources: wantedResources,
Environment: testEnv,
EnvironmentVPC: EnvironmentVPC{},
Services: allSvcs,
Tags: testApp.Tags,
Resources: wantedResources,
}

// WHEN
Expand Down
20 changes: 11 additions & 9 deletions internal/pkg/task/env_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ import (
)

const (
fmtErrPublicSubnetsFromEnv = "get public subnet IDs from environment %s: %w"
fmtErrSecurityGroupsFromEnv = "get security groups from environment %s: %w"
fmtErrDescribeEnvironment = "describe environment %s: %w"

envSecurityGroupCFNLogicalIDTagKey = "aws:cloudformation:logical-id"
envSecurityGroupCFNLogicalIDTagValue = "EnvironmentSecurityGroup"
Expand All @@ -37,9 +37,10 @@ type EnvRunner struct {
Env string

// Interfaces to interact with dependencies. Must not be nil.
VPCGetter VPCGetter
ClusterGetter ClusterGetter
Starter Runner
VPCGetter VPCGetter
ClusterGetter ClusterGetter
Starter Runner
EnvironmentDescriber EnvironmentDescriber
}

// Run runs tasks in the environment of the application, and returns the tasks.
Expand All @@ -53,16 +54,17 @@ func (r *EnvRunner) Run() ([]*Task, error) {
return nil, fmt.Errorf("get cluster for environment %s: %w", r.Env, err)
}

filters := r.filtersForVPCFromAppEnv()

subnets, err := r.VPCGetter.PublicSubnetIDs(filters...)
description, err := r.EnvironmentDescriber.Describe()
if err != nil {
return nil, fmt.Errorf(fmtErrPublicSubnetsFromEnv, r.Env, err)
return nil, fmt.Errorf(fmtErrDescribeEnvironment, r.Env, err)
}
if len(subnets) == 0 {
if len(description.EnvironmentVPC.PublicSubnetIDs) == 0 {
return nil, errNoSubnetFound
Copy link
Copy Markdown
Contributor

@bvtujo bvtujo Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a more descriptive error here, since this is wrapping multiple failure modes? Perhaps we could break it out into separate descriptive errors.

Copy link
Copy Markdown
Contributor Author

@ota42y ota42y Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need to check description.
Because when description is nil, we'll get error and return above line.

And I don't use pointer for description.EnvironmentVPC .
Because when we can't get data from Stack, we return error so we can set this struct always.

Copy link
Copy Markdown
Contributor

@bvtujo bvtujo Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Thanks for the follow up!

}

subnets := description.EnvironmentVPC.PublicSubnetIDs

filters := r.filtersForVPCFromAppEnv()
// Use only environment security group https://github.com/aws/copilot-cli/issues/1882.
securityGroups, err := r.VPCGetter.SecurityGroups(append(filters, ec2.Filter{
Name: fmt.Sprintf(ec2.TagFilterName, envSecurityGroupCFNLogicalIDTagKey),
Expand Down
Loading