From 164adc78fcf17788ff8dde0e6625e0075671c728 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Fri, 16 Sep 2022 14:28:19 -0700 Subject: [PATCH 01/35] add option to manifest, redirect to https in cf, forward http to alb --- internal/pkg/deploy/cloudformation/stack/env.go | 1 + internal/pkg/manifest/env.go | 6 ++++-- internal/pkg/template/env.go | 1 + .../environment/partials/cdn-resources.yml | 14 +++++++++++--- 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index a0e83c9a370..c2053c19cfc 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -392,6 +392,7 @@ func (e *EnvStackConfig) cdnConfig() *template.CDNConfig { } return &template.CDNConfig{ ImportedCertificate: e.in.Mft.CDNConfig.Config.Certificate, + TerminateTLS: aws.BoolValue(e.in.Mft.CDNConfig.Config.TerminateTLS), } } diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index cf4666ae1f1..58e3910e095 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -191,7 +191,9 @@ type environmentCDNConfig struct { // advancedCDNConfig represents an advanced configuration for a Content Delivery Network. type advancedCDNConfig struct { - Certificate *string `yaml:"certificate"` + // TODO does this need omitempty? + Certificate *string `yaml:"certificate"` + TerminateTLS *bool `yaml:"tls_termination"` } // IsEmpty returns whether environmentCDNConfig is empty. @@ -201,7 +203,7 @@ func (cfg *environmentCDNConfig) IsEmpty() bool { // isEmpty returns whether advancedCDNConfig is empty. func (cfg *advancedCDNConfig) isEmpty() bool { - return cfg.Certificate == nil + return cfg.Certificate == nil && cfg.TerminateTLS == nil } // CDNEnabled returns whether a CDN configuration has been enabled in the environment manifest. diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index 74d858f1017..c8bb833f2f7 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -146,6 +146,7 @@ func (elb *ELBAccessLogs) ShouldCreateBucket() bool { // CDNConfig represents a Content Delivery Network deployed by CloudFront. type CDNConfig struct { ImportedCertificate *string + TerminateTLS bool } type VPCConfig struct { diff --git a/internal/pkg/template/templates/environment/partials/cdn-resources.yml b/internal/pkg/template/templates/environment/partials/cdn-resources.yml index 87cd18372b2..1eb59550af0 100644 --- a/internal/pkg/template/templates/environment/partials/cdn-resources.yml +++ b/internal/pkg/template/templates/environment/partials/cdn-resources.yml @@ -58,14 +58,22 @@ CloudFrontDistribution: CachePolicyId: 4135ea2d-6df8-44a3-9df3-4b5a84be39ad # See https://go.aws/3bJid3k TargetOriginId: !Sub 'copilot-${AppName}-${EnvironmentName}-origin' OriginRequestPolicyId: 216adef6-5c7f-47e4-b989-5492eafa07d3 # See https://go.aws/3BIE8CP + {{- if .CDNConfig.TerminateTLS}} + ViewerProtocolPolicy: redirect-to-https + {{- else}} ViewerProtocolPolicy: allow-all + {{- end}} Enabled: true IPV6Enabled: true Origins: - - CustomOriginConfig: - OriginProtocolPolicy: match-viewer + - Id: !Sub 'copilot-${AppName}-${EnvironmentName}-origin' DomainName: !GetAtt PublicLoadBalancer.DNSName - Id: !Sub 'copilot-${AppName}-${EnvironmentName}-origin' + CustomOriginConfig: + {{- if .CDNConfig.TerminateTLS}} + OriginProtocolPolicy: http-only + {{- else}} + OriginProtocolPolicy: match-viewer + {{- end}} {{- if .CDNConfig.ImportedCertificate}} ViewerCertificate: AcmCertificateArn: {{.CDNConfig.ImportedCertificate}} From 3f683bcf2ba582e89e7c606924480a0bd1a9b88f Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Tue, 20 Sep 2022 10:47:17 -0700 Subject: [PATCH 02/35] manifest validation --- internal/pkg/manifest/env.go | 5 ++--- internal/pkg/manifest/validate_env.go | 6 ++++++ internal/pkg/manifest/validate_env_test.go | 16 ++++++++++++++++ 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 58e3910e095..c42ffcb5b01 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -191,9 +191,8 @@ type environmentCDNConfig struct { // advancedCDNConfig represents an advanced configuration for a Content Delivery Network. type advancedCDNConfig struct { - // TODO does this need omitempty? - Certificate *string `yaml:"certificate"` - TerminateTLS *bool `yaml:"tls_termination"` + Certificate *string `yaml:"certificate,omitempty"` + TerminateTLS *bool `yaml:"terminate_tls,omitempty"` } // IsEmpty returns whether environmentCDNConfig is empty. diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 9c0f774a2bc..e4ee23aa30e 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -367,6 +367,12 @@ func (i RestrictiveIngress) validate() error { // validate returns nil if advancedCDNConfig is configured correctly. func (cfg advancedCDNConfig) validate() error { if cfg.Certificate == nil { + if aws.BoolValue(cfg.TerminateTLS) == true { + return &errFieldMustBeSpecified{ + missingField: "certificate", + conditionalFields: []string{"terminate_tls"}, + } + } return nil } certARN, err := arn.Parse(*cfg.Certificate) diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index e08f2e23ba9..3769161bfe6 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -767,6 +767,22 @@ func TestCDNConfiguration_validate(t *testing.T) { }, wantedError: errors.New("cdn certificate must be in region us-east-1"), }, + "error if terminate tls set without cert": { + in: environmentCDNConfig{ + Config: advancedCDNConfig{ + TerminateTLS: aws.Bool(true), + }, + }, + wantedError: errors.New(`"certificate" must be specified if "terminate_tls" is specified`), + }, + "success with cert and terminate tls": { + in: environmentCDNConfig{ + Config: advancedCDNConfig{ + Certificate: aws.String("arn:aws:acm:us-east-1:1111111:certificate/look-like-a-good-arn"), + TerminateTLS: aws.Bool(true), + }, + }, + }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { From a7d93446e47a539544eb8f68e580aab9e44f9d4c Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Tue, 20 Sep 2022 14:32:38 -0700 Subject: [PATCH 03/35] verify services will work with `terminate_tls` --- internal/pkg/aws/elbv2/elbv2.go | 21 ++++++ internal/pkg/cli/deploy/env.go | 125 ++++++++++++++++++++++++++++++-- internal/pkg/cli/env_deploy.go | 34 ++------- internal/pkg/cli/interfaces.go | 2 + 4 files changed, 147 insertions(+), 35 deletions(-) diff --git a/internal/pkg/aws/elbv2/elbv2.go b/internal/pkg/aws/elbv2/elbv2.go index d08062abcb1..0fe30d03c60 100644 --- a/internal/pkg/aws/elbv2/elbv2.go +++ b/internal/pkg/aws/elbv2/elbv2.go @@ -5,6 +5,7 @@ package elbv2 import ( + "context" "fmt" "sort" @@ -73,6 +74,26 @@ func (e *ELBV2) ListenerRuleHostHeaders(ruleARN string) ([]string, error) { return hostHeaders, nil } +func (e *ELBV2) ListenerRuleIsRedirect(ctx context.Context, ruleARN string) (bool, error) { + resp, err := e.client.DescribeRules(&elbv2.DescribeRulesInput{ + RuleArns: aws.StringSlice([]string{ruleARN}), + }) + switch { + case err != nil: + return false, fmt.Errorf("get listener rule for %s: %w", ruleARN, err) + case len(resp.Rules) == 0: + return false, fmt.Errorf("cannot find listener rule %s", ruleARN) + } + + rule := resp.Rules[0] + for _, action := range rule.Actions { + if aws.StringValue(action.Type) == "redirect" { + return true, nil + } + } + return false, nil +} + // TargetHealth wraps up elbv2.TargetHealthDescription. type TargetHealth elbv2.TargetHealthDescription diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index d5f21d47942..34e3577d000 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -4,13 +4,19 @@ package deploy import ( + "context" "fmt" "io" "os" + "strings" + "sync" + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/session" awscfn "github.com/aws/aws-sdk-go/service/cloudformation" "github.com/aws/copilot-cli/internal/pkg/aws/cloudformation" "github.com/aws/copilot-cli/internal/pkg/aws/ec2" + "github.com/aws/copilot-cli/internal/pkg/aws/elbv2" "github.com/aws/copilot-cli/internal/pkg/aws/partitions" "github.com/aws/copilot-cli/internal/pkg/aws/s3" "github.com/aws/copilot-cli/internal/pkg/aws/sessions" @@ -18,16 +24,20 @@ import ( "github.com/aws/copilot-cli/internal/pkg/config" "github.com/aws/copilot-cli/internal/pkg/deploy" deploycfn "github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation" - "github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation/stack" + cfnstack "github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation/stack" "github.com/aws/copilot-cli/internal/pkg/deploy/upload/customresource" + "github.com/aws/copilot-cli/internal/pkg/describe" + "github.com/aws/copilot-cli/internal/pkg/describe/stack" "github.com/aws/copilot-cli/internal/pkg/manifest" "github.com/aws/copilot-cli/internal/pkg/template" "github.com/aws/copilot-cli/internal/pkg/term/log" termprogress "github.com/aws/copilot-cli/internal/pkg/term/progress" + "github.com/dustin/go-humanize/english" + "golang.org/x/sync/errgroup" ) type appResourcesGetter interface { - GetAppResourcesByRegion(app *config.Application, region string) (*stack.AppRegionalResources, error) + GetAppResourcesByRegion(app *config.Application, region string) (*cfnstack.AppRegionalResources, error) } type environmentDeployer interface { @@ -44,6 +54,11 @@ type prefixListGetter interface { CloudFrontManagedPrefixListID() (string, error) } +type envDescriber interface { + ValidateCFServiceDomainAliases() error + Params() (map[string]string, error) +} + type envDeployer struct { app *config.Application env *config.Environment @@ -57,9 +72,11 @@ type envDeployer struct { envDeployer environmentDeployer patcher patcher newStackSerializer func(input *deploy.CreateEnvironmentInput, forceUpdateID string, prevParams []*awscfn.Parameter) stackSerializer + envDescriber envDescriber + envManagerSession *session.Session // Cached variables. - appRegionalResources *stack.AppRegionalResources + appRegionalResources *cfnstack.AppRegionalResources } // NewEnvDeployerInput contains information needed to construct an environment deployer. @@ -67,6 +84,7 @@ type NewEnvDeployerInput struct { App *config.Application Env *config.Environment SessionProvider *sessions.Provider + ConfigStore describe.ConfigStoreSvc } // NewEnvDeployer constructs an environment deployer. @@ -83,6 +101,11 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { if err != nil { return nil, fmt.Errorf("get env session: %w", err) } + envDescriber, err := describe.NewEnvDescriber(describe.NewEnvDescriberConfig{ + App: in.App.Name, + Env: in.Env.Name, + ConfigStore: in.ConfigStore, + }) cfnClient := deploycfn.New(envManagerSession, deploycfn.WithProgressTracker(os.Stderr)) return &envDeployer{ app: in.App, @@ -100,11 +123,101 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { Env: in.Env, }, newStackSerializer: func(in *deploy.CreateEnvironmentInput, lastForceUpdateID string, oldParams []*awscfn.Parameter) stackSerializer { - return stack.NewEnvConfigFromExistingStack(in, lastForceUpdateID, oldParams) + return cfnstack.NewEnvConfigFromExistingStack(in, lastForceUpdateID, oldParams) }, + envDescriber: envDescriber, + envManagerSession: envManagerSession, }, nil } +func (d *envDeployer) Verify(ctx context.Context, mft *manifest.Environment) error { + if mft.CDNEnabled() && mft.HTTPConfig.Public.Certificates == nil && d.app.Domain != "" { + // With managed domain, if the customer isn't using `alias` the A-records are inserted in the service stack as each service domain is unique. + // However, when clients enable CloudFront, they would need to update all their existing records to now point to the distribution. + // Hence, we force users to use `alias` and let the records be written in the environment stack instead. + if err := d.envDescriber.ValidateCFServiceDomainAliases(); err != nil { + return err + } + } + + if mft.CDNEnabled() && aws.BoolValue(mft.CDNConfig.Config.TerminateTLS) { + // ensure all services _are not_ doing http->https redirect + if err := d.verifyALBWorkloadsDontRedirect(ctx); err != nil { + return fmt.Errorf("can't enable TLS termination on CDN: %w", err) + } + } + + return nil +} + +func (d *envDeployer) verifyALBWorkloadsDontRedirect(ctx context.Context) error { + params, err := d.envDescriber.Params() + if err != nil { + return fmt.Errorf("get env params: %w", err) + } + + services := strings.Split(params[cfnstack.EnvParamALBWorkloadsKey], ",") + g, ctx := errgroup.WithContext(ctx) + + var badServices []string + var badServicesMu sync.Mutex + + for i := range services { + svc := services[i] + g.Go(func() error { + allowsHTTP, err := d.lbServiceAllowsHTTP(ctx, svc) + switch { + case err != nil: + return fmt.Errorf("verify service %q: %w", svc, err) + case !allowsHTTP: + badServicesMu.Lock() + defer badServicesMu.Unlock() + badServices = append(badServices, svc) + } + + return nil + }) + } + + if err := g.Wait(); err != nil { + return err + } + if len(badServices) > 0 { + return fmt.Errorf("HTTP traffic redirects to HTTPS in services %v.\nSet http.redirect_to_https to false for those services and redeploy them.", english.OxfordWordSeries(badServices, "and")) + } + + return nil +} + +const ( + svcStackResourceHTTPListenerRuleLogicalID = "HTTPListenerRuleWithDomain" +) + +func (d *envDeployer) lbServiceAllowsHTTP(ctx context.Context, svc string) (bool, error) { + cfn := stack.NewStackDescriber(cfnstack.NameForService(d.app.Name, d.env.Name, svc), d.envManagerSession) + resources, err := cfn.Resources() + if err != nil { + return false, fmt.Errorf("get stack resources: %w", err) + } + + ruleARN := "" + for _, res := range resources { + if res.LogicalID == svcStackResourceHTTPListenerRuleLogicalID { + ruleARN = res.PhysicalID + } + } + if ruleARN == "" { + return false, fmt.Errorf("resource %q not present", svcStackResourceHTTPListenerRuleLogicalID) + } + + elbv2 := elbv2.New(d.envManagerSession) + isRedirect, err := elbv2.ListenerRuleIsRedirect(ctx, ruleARN) + if err != nil { + return false, fmt.Errorf("verify rule isn't redirect: %w", err) + } + return !isRedirect, nil +} + // UploadArtifacts uploads the deployment artifacts for the environment. func (d *envDeployer) UploadArtifacts() (map[string]string, error) { resources, err := d.getAppRegionalResources() @@ -208,11 +321,11 @@ func (d *envDeployer) DeployEnvironment(in *DeployEnvironmentInput) error { if err != nil { return fmt.Errorf("retrieve environment stack force update ID: %w", err) } - conf := stack.NewEnvConfigFromExistingStack(stackInput, lastForceUpdateID, oldParams) + conf := cfnstack.NewEnvConfigFromExistingStack(stackInput, lastForceUpdateID, oldParams) return d.envDeployer.UpdateAndRenderEnvironment(conf, stackInput.ArtifactBucketARN, cloudformation.WithRoleARN(d.env.ExecutionRoleARN)) } -func (d *envDeployer) getAppRegionalResources() (*stack.AppRegionalResources, error) { +func (d *envDeployer) getAppRegionalResources() (*cfnstack.AppRegionalResources, error) { if d.appRegionalResources != nil { return d.appRegionalResources, nil } diff --git a/internal/pkg/cli/env_deploy.go b/internal/pkg/cli/env_deploy.go index c1db57016bb..fb2ba74791e 100644 --- a/internal/pkg/cli/env_deploy.go +++ b/internal/pkg/cli/env_deploy.go @@ -4,6 +4,7 @@ package cli import ( + "context" "errors" "fmt" "os" @@ -16,7 +17,6 @@ import ( "github.com/aws/copilot-cli/internal/pkg/aws/sessions" "github.com/aws/copilot-cli/internal/pkg/cli/deploy" "github.com/aws/copilot-cli/internal/pkg/config" - "github.com/aws/copilot-cli/internal/pkg/describe" "github.com/aws/copilot-cli/internal/pkg/manifest" "github.com/aws/copilot-cli/internal/pkg/term/color" "github.com/aws/copilot-cli/internal/pkg/term/log" @@ -47,7 +47,6 @@ type deployEnvOpts struct { identity identityService newInterpolator func(app, env string) interpolator newEnvDeployer func() (envDeployer, error) - newEnvDescriber func() (envDescriber, error) // Cached variables. targetApp *config.Application @@ -79,17 +78,6 @@ func newEnvDeployOpts(vars deployEnvVars) (*deployEnvOpts, error) { opts.newEnvDeployer = func() (envDeployer, error) { return newEnvDeployer(opts) } - opts.newEnvDescriber = func() (envDescriber, error) { - envDescriber, err := describe.NewEnvDescriber(describe.NewEnvDescriberConfig{ - App: opts.appName, - Env: opts.name, - ConfigStore: opts.store, - }) - if err != nil { - return nil, err - } - return envDescriber, nil - } return opts, nil } @@ -106,6 +94,7 @@ func newEnvDeployer(opts *deployEnvOpts) (envDeployer, error) { App: app, Env: env, SessionProvider: opts.sessionProvider, + ConfigStore: opts.store, }) } @@ -126,10 +115,6 @@ func (o *deployEnvOpts) Ask() error { return o.validateOrAskEnvName() } -func (o *deployEnvOpts) isManagedCDNEnabled(mft *manifest.Environment) bool { - return mft.CDNEnabled() && mft.HTTPConfig.Public.Certificates == nil && o.targetApp.Domain != "" -} - // Execute deploys an environment given a manifest. func (o *deployEnvOpts) Execute() error { rawMft, err := o.ws.ReadEnvironmentManifest(o.name) @@ -140,18 +125,6 @@ func (o *deployEnvOpts) Execute() error { if err != nil { return err } - if o.isManagedCDNEnabled(mft) { - describer, err := o.newEnvDescriber() - if err != nil { - return err - } - // With managed domain, if the customer isn't using `alias` the A-records are inserted in the service stack as each service domain is unique. - // However, when clients enable CloudFront, they would need to update all their existing records to now point to the distribution. - // Hence, we force users to use `alias` and let the records be written in the environment stack instead. - if err := describer.ValidateCFServiceDomainAliases(); err != nil { - return err - } - } caller, err := o.identity.Get() if err != nil { return fmt.Errorf("get identity: %w", err) @@ -160,6 +133,9 @@ func (o *deployEnvOpts) Execute() error { if err != nil { return err } + if err := deployer.Verify(context.Background(), mft); err != nil { + return err + } urls, err := deployer.UploadArtifacts() if err != nil { return fmt.Errorf("upload artifacts for environment %s: %w", o.name, err) diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index 59d294106df..793b5161204 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -4,6 +4,7 @@ package cli import ( + "context" "encoding" "io" @@ -648,6 +649,7 @@ type runner interface { type envDeployer interface { DeployEnvironment(in *clideploy.DeployEnvironmentInput) error + Verify(ctx context.Context, mft *manifest.Environment) error UploadArtifacts() (map[string]string, error) } From 35eed74ae50061712cb27daf847f10b36cf2182f Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Tue, 20 Sep 2022 14:41:08 -0700 Subject: [PATCH 04/35] testing in package elbv2 --- internal/pkg/aws/elbv2/elbv2.go | 2 +- internal/pkg/aws/elbv2/elbv2_test.go | 85 ++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/internal/pkg/aws/elbv2/elbv2.go b/internal/pkg/aws/elbv2/elbv2.go index 0fe30d03c60..6fff0c33ac6 100644 --- a/internal/pkg/aws/elbv2/elbv2.go +++ b/internal/pkg/aws/elbv2/elbv2.go @@ -87,7 +87,7 @@ func (e *ELBV2) ListenerRuleIsRedirect(ctx context.Context, ruleARN string) (boo rule := resp.Rules[0] for _, action := range rule.Actions { - if aws.StringValue(action.Type) == "redirect" { + if aws.StringValue(action.Type) == elbv2.ActionTypeEnumRedirect { return true, nil } } diff --git a/internal/pkg/aws/elbv2/elbv2_test.go b/internal/pkg/aws/elbv2/elbv2_test.go index 4532322567f..4221b2301a7 100644 --- a/internal/pkg/aws/elbv2/elbv2_test.go +++ b/internal/pkg/aws/elbv2/elbv2_test.go @@ -4,6 +4,7 @@ package elbv2 import ( + "context" "errors" "fmt" "testing" @@ -219,6 +220,90 @@ func TestELBV2_ListenerRuleHostHeaders(t *testing.T) { } } +func TestELBV2_ListenerRuleIsRedirect(t *testing.T) { + mockARN := "mockListenerRuleARN" + testCases := map[string]struct { + setUpMock func(m *mocks.Mockapi) + + expectedErr string + expected bool + }{ + "fail to describe rules": { + setUpMock: func(m *mocks.Mockapi) { + m.EXPECT().DescribeRules(&elbv2.DescribeRulesInput{ + RuleArns: aws.StringSlice([]string{mockARN}), + }).Return(nil, errors.New("some error")) + }, + expectedErr: fmt.Sprintf("get listener rule for mockListenerRuleARN: some error"), + }, + "cannot find listener rule": { + setUpMock: func(m *mocks.Mockapi) { + m.EXPECT().DescribeRules(&elbv2.DescribeRulesInput{ + RuleArns: aws.StringSlice([]string{mockARN}), + }).Return(&elbv2.DescribeRulesOutput{}, nil) + }, + expectedErr: fmt.Sprintf("cannot find listener rule mockListenerRuleARN"), + }, + "rule is redirect": { + setUpMock: func(m *mocks.Mockapi) { + m.EXPECT().DescribeRules(&elbv2.DescribeRulesInput{ + RuleArns: aws.StringSlice([]string{mockARN}), + }).Return(&elbv2.DescribeRulesOutput{ + Rules: []*elbv2.Rule{ + { + Actions: []*elbv2.Action{ + { + Type: aws.String(elbv2.ActionTypeEnumRedirect), + }, + }, + }, + }, + }, nil) + }, + expected: true, + }, + "rule is not redirect": { + setUpMock: func(m *mocks.Mockapi) { + m.EXPECT().DescribeRules(&elbv2.DescribeRulesInput{ + RuleArns: aws.StringSlice([]string{mockARN}), + }).Return(&elbv2.DescribeRulesOutput{ + Rules: []*elbv2.Rule{ + { + Actions: []*elbv2.Action{ + { + Type: aws.String(elbv2.ActionTypeEnumForward), + }, + }, + }, + }, + }, nil) + }, + expected: false, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockAPI := mocks.NewMockapi(ctrl) + tc.setUpMock(mockAPI) + + elbv2Client := ELBV2{ + client: mockAPI, + } + + actual, err := elbv2Client.ListenerRuleIsRedirect(context.Background(), mockARN) + if tc.expectedErr != "" { + require.EqualError(t, err, tc.expectedErr) + } else { + require.Equal(t, tc.expected, actual) + } + }) + } +} + func TestTargetHealth_HealthStatus(t *testing.T) { testCases := map[string]struct { inTargetHealth *TargetHealth From ef68646085c16443283c63d8e6684f755538e898 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Tue, 20 Sep 2022 14:48:03 -0700 Subject: [PATCH 05/35] gen mocks --- internal/pkg/cli/mocks/mock_interfaces.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/internal/pkg/cli/mocks/mock_interfaces.go b/internal/pkg/cli/mocks/mock_interfaces.go index 7d30f3aa3a7..ac29fc246b5 100644 --- a/internal/pkg/cli/mocks/mock_interfaces.go +++ b/internal/pkg/cli/mocks/mock_interfaces.go @@ -5,6 +5,7 @@ package mocks import ( + context "context" encoding "encoding" io "io" reflect "reflect" @@ -6847,6 +6848,20 @@ func (mr *MockenvDeployerMockRecorder) UploadArtifacts() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UploadArtifacts", reflect.TypeOf((*MockenvDeployer)(nil).UploadArtifacts)) } +// Verify mocks base method. +func (m *MockenvDeployer) Verify(ctx context.Context, mft *manifest.Environment) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Verify", ctx, mft) + ret0, _ := ret[0].(error) + return ret0 +} + +// Verify indicates an expected call of Verify. +func (mr *MockenvDeployerMockRecorder) Verify(ctx, mft interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Verify", reflect.TypeOf((*MockenvDeployer)(nil).Verify), ctx, mft) +} + // MockenvPackager is a mock of envPackager interface. type MockenvPackager struct { ctrl *gomock.Controller From eda434e5198c3a61800e230185c374bf3c1bbeb2 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Tue, 20 Sep 2022 14:55:28 -0700 Subject: [PATCH 06/35] env_deploy tests --- internal/pkg/cli/env_deploy_test.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/internal/pkg/cli/env_deploy_test.go b/internal/pkg/cli/env_deploy_test.go index e74034fc5b6..6cf15775e27 100644 --- a/internal/pkg/cli/env_deploy_test.go +++ b/internal/pkg/cli/env_deploy_test.go @@ -5,7 +5,6 @@ package cli import ( "errors" - "fmt" "testing" "github.com/aws/aws-sdk-go/aws" @@ -143,7 +142,6 @@ type deployEnvExecuteMocks struct { deployer *mocks.MockenvDeployer identity *mocks.MockidentityService interpolator *mocks.Mockinterpolator - describer *mocks.MockenvDescriber } func TestDeployEnvOpts_Execute(t *testing.T) { @@ -172,14 +170,6 @@ func TestDeployEnvOpts_Execute(t *testing.T) { }, wantedErr: errors.New(`unmarshal environment manifest for "mockEnv"`), }, - "fail to validate cdn": { - setUpMocks: func(m *deployEnvExecuteMocks) { - m.ws.EXPECT().ReadEnvironmentManifest(gomock.Any()).Return([]byte("name: mockEnv\ntype: Environment\ncdn: true\n"), nil) - m.interpolator.EXPECT().Interpolate(gomock.Any()).Return("name: mockEnv\ntype: Environment\ncdn: true\n", nil) - m.describer.EXPECT().ValidateCFServiceDomainAliases().Return(fmt.Errorf("mock error")) - }, - wantedErr: errors.New("mock error"), - }, "fail to get caller identity": { setUpMocks: func(m *deployEnvExecuteMocks) { m.ws.EXPECT().ReadEnvironmentManifest(gomock.Any()).Return([]byte("name: mockEnv\ntype: Environment\n"), nil) @@ -188,6 +178,17 @@ func TestDeployEnvOpts_Execute(t *testing.T) { }, wantedErr: errors.New("get identity: some error"), }, + "fail to verify env": { + setUpMocks: func(m *deployEnvExecuteMocks) { + m.ws.EXPECT().ReadEnvironmentManifest(gomock.Any()).Return([]byte("name: mockEnv\ntype: Environment\ncdn: true\n"), nil) + m.interpolator.EXPECT().Interpolate(gomock.Any()).Return("name: mockEnv\ntype: Environment\ncdn: true\n", nil) + m.identity.EXPECT().Get().Return(identity.Caller{ + RootUserARN: "mockRootUserARN", + }, nil) + m.deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(errors.New("mock error")) + }, + wantedErr: errors.New("mock error"), + }, "fail to upload manifest": { setUpMocks: func(m *deployEnvExecuteMocks) { m.ws.EXPECT().ReadEnvironmentManifest(gomock.Any()).Return([]byte("name: mockEnv\ntype: Environment\n"), nil) @@ -195,6 +196,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) + m.deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(nil, errors.New("some error")) }, wantedErr: errors.New("upload artifacts for environment mockEnv: some error"), @@ -206,6 +208,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) + m.deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(map[string]string{ "mockResource": "mockURL", }, nil) @@ -222,6 +225,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) + m.deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(map[string]string{ "mockResource": "mockURL", }, nil) @@ -250,7 +254,6 @@ func TestDeployEnvOpts_Execute(t *testing.T) { deployer: mocks.NewMockenvDeployer(ctrl), identity: mocks.NewMockidentityService(ctrl), interpolator: mocks.NewMockinterpolator(ctrl), - describer: mocks.NewMockenvDescriber(ctrl), } tc.setUpMocks(m) opts := deployEnvOpts{ @@ -265,9 +268,6 @@ func TestDeployEnvOpts_Execute(t *testing.T) { newInterpolator: func(s string, s2 string) interpolator { return m.interpolator }, - newEnvDescriber: func() (envDescriber, error) { - return m.describer, nil - }, targetApp: &config.Application{ Name: "mockApp", Domain: "mockDomain", From 410b803b33a1a87f39bc58a6803656737e2a044b Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Tue, 20 Sep 2022 16:24:08 -0700 Subject: [PATCH 07/35] export some CDN manifest types --- internal/pkg/manifest/env.go | 16 +++++----- internal/pkg/manifest/env_test.go | 20 ++++++------- internal/pkg/manifest/transform.go | 6 ++-- internal/pkg/manifest/transform_test.go | 26 ++++++++--------- internal/pkg/manifest/validate_env.go | 4 +-- internal/pkg/manifest/validate_env_test.go | 34 +++++++++++----------- 6 files changed, 53 insertions(+), 53 deletions(-) diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index c42ffcb5b01..31b665cbe54 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -94,7 +94,7 @@ type EnvironmentConfig struct { Network environmentNetworkConfig `yaml:"network,omitempty,flow"` Observability environmentObservability `yaml:"observability,omitempty,flow"` HTTPConfig EnvironmentHTTPConfig `yaml:"http,omitempty,flow"` - CDNConfig environmentCDNConfig `yaml:"cdn,omitempty,flow"` + CDNConfig EnvironmentCDNConfig `yaml:"cdn,omitempty,flow"` } // IsIngressRestrictedToCDN returns whether or not an environment has its @@ -184,24 +184,24 @@ func (cfg *EnvironmentConfig) EnvSecurityGroup() (*securityGroupConfig, bool) { return nil, false } -type environmentCDNConfig struct { +type EnvironmentCDNConfig struct { Enabled *bool - Config advancedCDNConfig // mutually exclusive with Enabled + Config AdvancedCDNConfig // mutually exclusive with Enabled } -// advancedCDNConfig represents an advanced configuration for a Content Delivery Network. -type advancedCDNConfig struct { +// AdvancedCDNConfig represents an advanced configuration for a Content Delivery Network. +type AdvancedCDNConfig struct { Certificate *string `yaml:"certificate,omitempty"` TerminateTLS *bool `yaml:"terminate_tls,omitempty"` } // IsEmpty returns whether environmentCDNConfig is empty. -func (cfg *environmentCDNConfig) IsEmpty() bool { +func (cfg *EnvironmentCDNConfig) IsEmpty() bool { return cfg.Enabled == nil && cfg.Config.isEmpty() } // isEmpty returns whether advancedCDNConfig is empty. -func (cfg *advancedCDNConfig) isEmpty() bool { +func (cfg *AdvancedCDNConfig) isEmpty() bool { return cfg.Certificate == nil && cfg.TerminateTLS == nil } @@ -216,7 +216,7 @@ func (cfg *EnvironmentConfig) CDNEnabled() bool { // UnmarshalYAML overrides the default YAML unmarshaling logic for the environmentCDNConfig // struct, allowing it to perform more complex unmarshaling behavior. // This method implements the yaml.Unmarshaler (v3) interface. -func (cfg *environmentCDNConfig) UnmarshalYAML(value *yaml.Node) error { +func (cfg *EnvironmentCDNConfig) UnmarshalYAML(value *yaml.Node) error { if err := value.Decode(&cfg.Config); err != nil { var yamlTypeErr *yaml.TypeError if !errors.As(err, &yamlTypeErr) { diff --git a/internal/pkg/manifest/env_test.go b/internal/pkg/manifest/env_test.go index 0fc698b4e79..d3f74f9a9ee 100644 --- a/internal/pkg/manifest/env_test.go +++ b/internal/pkg/manifest/env_test.go @@ -468,7 +468,7 @@ cdn: true Type: aws.String("Environment"), }, EnvironmentConfig: EnvironmentConfig{ - CDNConfig: environmentCDNConfig{ + CDNConfig: EnvironmentCDNConfig{ Enabled: aws.Bool(true), }, }, @@ -855,22 +855,22 @@ func TestEnvironmentObservability_IsEmpty(t *testing.T) { func TestEnvironmentCDNConfig_IsEmpty(t *testing.T) { testCases := map[string]struct { - in environmentCDNConfig + in EnvironmentCDNConfig wanted bool }{ "empty": { - in: environmentCDNConfig{}, + in: EnvironmentCDNConfig{}, wanted: true, }, "not empty": { - in: environmentCDNConfig{ + in: EnvironmentCDNConfig{ Enabled: aws.Bool(false), }, wanted: false, }, "advanced not empty": { - in: environmentCDNConfig{ - Config: advancedCDNConfig{ + in: EnvironmentCDNConfig{ + Config: AdvancedCDNConfig{ Certificate: aws.String("arn:aws:acm:us-east-1:1111111:certificate/look-like-a-good-arn"), }, }, @@ -893,7 +893,7 @@ func TestEnvironmentConfig_CDNEnabled(t *testing.T) { }{ "enabled via bool": { in: EnvironmentConfig{ - CDNConfig: environmentCDNConfig{ + CDNConfig: EnvironmentCDNConfig{ Enabled: aws.Bool(true), }, }, @@ -901,8 +901,8 @@ func TestEnvironmentConfig_CDNEnabled(t *testing.T) { }, "enabled via config": { in: EnvironmentConfig{ - CDNConfig: environmentCDNConfig{ - Config: advancedCDNConfig{ + CDNConfig: EnvironmentCDNConfig{ + Config: AdvancedCDNConfig{ Certificate: aws.String("arn:aws:acm:us-east-1:1111111:certificate/look-like-a-good-arn"), }, }, @@ -915,7 +915,7 @@ func TestEnvironmentConfig_CDNEnabled(t *testing.T) { }, "not enabled via bool": { in: EnvironmentConfig{ - CDNConfig: environmentCDNConfig{ + CDNConfig: EnvironmentCDNConfig{ Enabled: aws.Bool(false), }, }, diff --git a/internal/pkg/manifest/transform.go b/internal/pkg/manifest/transform.go index 375fd55d2c9..b28b294109b 100644 --- a/internal/pkg/manifest/transform.go +++ b/internal/pkg/manifest/transform.go @@ -522,19 +522,19 @@ type environmentCDNConfigTransformer struct{} // Transformer returns custom merge logic for environmentCDNConfig's fields. func (t environmentCDNConfigTransformer) Transformer(typ reflect.Type) func(dst, src reflect.Value) error { - if typ != reflect.TypeOf(environmentCDNConfig{}) { + if typ != reflect.TypeOf(EnvironmentCDNConfig{}) { return nil } return func(dst, src reflect.Value) error { - dstStruct, srcStruct := dst.Interface().(environmentCDNConfig), src.Interface().(environmentCDNConfig) + dstStruct, srcStruct := dst.Interface().(EnvironmentCDNConfig), src.Interface().(EnvironmentCDNConfig) if !srcStruct.Config.isEmpty() { dstStruct.Enabled = nil } if srcStruct.Enabled != nil { - dstStruct.Config = advancedCDNConfig{} + dstStruct.Config = AdvancedCDNConfig{} } if dst.CanSet() { // For extra safety to prevent panicking. diff --git a/internal/pkg/manifest/transform_test.go b/internal/pkg/manifest/transform_test.go index d0e07fffe44..0286bfbeaa4 100644 --- a/internal/pkg/manifest/transform_test.go +++ b/internal/pkg/manifest/transform_test.go @@ -1197,34 +1197,34 @@ func TestSecretTransformer_Transformer(t *testing.T) { func TestEnvironmentCDNConfigTransformer_Transformer(t *testing.T) { testCases := map[string]struct { - original func(cfg *environmentCDNConfig) - override func(cfg *environmentCDNConfig) - wanted func(cfg *environmentCDNConfig) + original func(cfg *EnvironmentCDNConfig) + override func(cfg *EnvironmentCDNConfig) + wanted func(cfg *EnvironmentCDNConfig) }{ "cdnconfig set to empty if enabled is not nil": { - original: func(cfg *environmentCDNConfig) { - cfg.Config = advancedCDNConfig{ + original: func(cfg *EnvironmentCDNConfig) { + cfg.Config = AdvancedCDNConfig{ Certificate: aws.String("arn:aws:acm:us-east-1:1111111:certificate/look-like-a-good-arn"), } }, - override: func(cfg *environmentCDNConfig) { + override: func(cfg *EnvironmentCDNConfig) { cfg.Enabled = aws.Bool(true) }, - wanted: func(cfg *environmentCDNConfig) { + wanted: func(cfg *EnvironmentCDNConfig) { cfg.Enabled = aws.Bool(true) }, }, "enabled set to nil if cdnconfig is not empty": { - original: func(cfg *environmentCDNConfig) { + original: func(cfg *EnvironmentCDNConfig) { cfg.Enabled = aws.Bool(true) }, - override: func(cfg *environmentCDNConfig) { - cfg.Config = advancedCDNConfig{ + override: func(cfg *EnvironmentCDNConfig) { + cfg.Config = AdvancedCDNConfig{ Certificate: aws.String("arn:aws:acm:us-east-1:1111111:certificate/look-like-a-good-arn"), } }, - wanted: func(cfg *environmentCDNConfig) { - cfg.Config = advancedCDNConfig{ + wanted: func(cfg *EnvironmentCDNConfig) { + cfg.Config = AdvancedCDNConfig{ Certificate: aws.String("arn:aws:acm:us-east-1:1111111:certificate/look-like-a-good-arn"), } }, @@ -1233,7 +1233,7 @@ func TestEnvironmentCDNConfigTransformer_Transformer(t *testing.T) { for name, tc := range testCases { t.Run(name, func(t *testing.T) { - var dst, override, wanted environmentCDNConfig + var dst, override, wanted EnvironmentCDNConfig tc.original(&dst) tc.override(&override) diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index e4ee23aa30e..519c078bd49 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -347,7 +347,7 @@ func (cfg securityGroupsConfig) validate() error { } // validate returns nil if environmentCDNConfig is configured correctly. -func (cfg environmentCDNConfig) validate() error { +func (cfg EnvironmentCDNConfig) validate() error { if cfg.Config.isEmpty() { return nil } @@ -365,7 +365,7 @@ func (i RestrictiveIngress) validate() error { } // validate returns nil if advancedCDNConfig is configured correctly. -func (cfg advancedCDNConfig) validate() error { +func (cfg AdvancedCDNConfig) validate() error { if cfg.Certificate == nil { if aws.BoolValue(cfg.TerminateTLS) == true { return &errFieldMustBeSpecified{ diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index 3769161bfe6..da4d62d3c64 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -170,7 +170,7 @@ func TestEnvironmentConfig_validate(t *testing.T) { }, "error if security group ingress is limited to a cdn distribution not enabled": { in: EnvironmentConfig{ - CDNConfig: environmentCDNConfig{ + CDNConfig: EnvironmentCDNConfig{ Enabled: aws.Bool(false), }, HTTPConfig: EnvironmentHTTPConfig{ @@ -216,8 +216,8 @@ func TestEnvironmentConfig_validate(t *testing.T) { }, "error if cdn cert specified but public certs not specified": { in: EnvironmentConfig{ - CDNConfig: environmentCDNConfig{ - Config: advancedCDNConfig{ + CDNConfig: EnvironmentCDNConfig{ + Config: AdvancedCDNConfig{ Certificate: aws.String("arn:aws:acm:us-east-1:1111111:certificate/look-like-a-good-arn"), }, }, @@ -226,7 +226,7 @@ func TestEnvironmentConfig_validate(t *testing.T) { }, "error if cdn cert not specified but public certs imported": { in: EnvironmentConfig{ - CDNConfig: environmentCDNConfig{ + CDNConfig: EnvironmentCDNConfig{ Enabled: aws.Bool(true), }, HTTPConfig: EnvironmentHTTPConfig{ @@ -732,52 +732,52 @@ func TestSubnetsConfiguration_validate(t *testing.T) { func TestCDNConfiguration_validate(t *testing.T) { testCases := map[string]struct { - in environmentCDNConfig + in EnvironmentCDNConfig wantedError error wantedErrorMsgPrefix string }{ "valid if empty": { - in: environmentCDNConfig{}, + in: EnvironmentCDNConfig{}, }, "valid if bool specified": { - in: environmentCDNConfig{ + in: EnvironmentCDNConfig{ Enabled: aws.Bool(false), }, }, "valid if advanced config configured correctly": { - in: environmentCDNConfig{ - Config: advancedCDNConfig{ + in: EnvironmentCDNConfig{ + Config: AdvancedCDNConfig{ Certificate: aws.String("arn:aws:acm:us-east-1:1111111:certificate/look-like-a-good-arn"), }, }, }, "error if certificate invalid": { - in: environmentCDNConfig{ - Config: advancedCDNConfig{ + in: EnvironmentCDNConfig{ + Config: AdvancedCDNConfig{ Certificate: aws.String("arn:aws:weird-little-arn"), }, }, wantedErrorMsgPrefix: "parse cdn certificate:", }, "error if certificate in invalid region": { - in: environmentCDNConfig{ - Config: advancedCDNConfig{ + in: EnvironmentCDNConfig{ + Config: AdvancedCDNConfig{ Certificate: aws.String("arn:aws:acm:us-west-2:1111111:certificate/look-like-a-good-arn"), }, }, wantedError: errors.New("cdn certificate must be in region us-east-1"), }, "error if terminate tls set without cert": { - in: environmentCDNConfig{ - Config: advancedCDNConfig{ + in: EnvironmentCDNConfig{ + Config: AdvancedCDNConfig{ TerminateTLS: aws.Bool(true), }, }, wantedError: errors.New(`"certificate" must be specified if "terminate_tls" is specified`), }, "success with cert and terminate tls": { - in: environmentCDNConfig{ - Config: advancedCDNConfig{ + in: EnvironmentCDNConfig{ + Config: AdvancedCDNConfig{ Certificate: aws.String("arn:aws:acm:us-east-1:1111111:certificate/look-like-a-good-arn"), TerminateTLS: aws.Bool(true), }, From 7241bdc4ebab7c55c3e027ddcb9af8c3b7a1471b Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Tue, 20 Sep 2022 16:24:47 -0700 Subject: [PATCH 08/35] better error, Verify() tests cases --- internal/pkg/cli/deploy/env.go | 55 +++- internal/pkg/cli/deploy/env_test.go | 367 +++++++++++++++++++--- internal/pkg/cli/deploy/mocks/mock_env.go | 130 ++++++++ 3 files changed, 485 insertions(+), 67 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 34e3577d000..fd1cface173 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "os" + "sort" "strings" "sync" @@ -59,6 +60,18 @@ type envDescriber interface { Params() (map[string]string, error) } +type lbDescriber interface { + ListenerRuleIsRedirect(context.Context, string) (bool, error) +} + +type stackDescriber interface { + Resources() ([]*stack.Resource, error) +} + +const ( + svcStackResourceHTTPListenerRuleLogicalID = "HTTPListenerRuleWithDomain" +) + type envDeployer struct { app *config.Application env *config.Environment @@ -68,12 +81,14 @@ type envDeployer struct { s3 uploader prefixListGetter prefixListGetter // Dependencies to deploy an environment. - appCFN appResourcesGetter - envDeployer environmentDeployer - patcher patcher - newStackSerializer func(input *deploy.CreateEnvironmentInput, forceUpdateID string, prevParams []*awscfn.Parameter) stackSerializer - envDescriber envDescriber - envManagerSession *session.Session + appCFN appResourcesGetter + envDeployer environmentDeployer + patcher patcher + newStackSerializer func(input *deploy.CreateEnvironmentInput, forceUpdateID string, prevParams []*awscfn.Parameter) stackSerializer + envDescriber envDescriber + envManagerSession *session.Session + newLBDescriber func(*session.Session) lbDescriber + newServiceStackDescriber func(string, *session.Session) stackDescriber // Cached variables. appRegionalResources *cfnstack.AppRegionalResources @@ -127,6 +142,12 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { }, envDescriber: envDescriber, envManagerSession: envManagerSession, + newLBDescriber: func(sess *session.Session) lbDescriber { + return elbv2.New(sess) + }, + newServiceStackDescriber: func(svc string, sess *session.Session) stackDescriber { + return stack.NewStackDescriber(cfnstack.NameForService(in.App.Name, in.Env.Name, svc), sess) + }, }, nil } @@ -183,19 +204,21 @@ func (d *envDeployer) verifyALBWorkloadsDontRedirect(ctx context.Context) error return err } if len(badServices) > 0 { - return fmt.Errorf("HTTP traffic redirects to HTTPS in services %v.\nSet http.redirect_to_https to false for those services and redeploy them.", english.OxfordWordSeries(badServices, "and")) + sort.Strings(badServices) + return fmt.Errorf("HTTP traffic redirects to HTTPS in %v %v.\nSet http.redirect_to_https to false for %v and redeploy %v.", + english.PluralWord(len(badServices), "service", "services"), + english.OxfordWordSeries(badServices, "and"), + english.PluralWord(len(badServices), "that service", "those services"), + english.PluralWord(len(badServices), "it", "them"), + ) } return nil } -const ( - svcStackResourceHTTPListenerRuleLogicalID = "HTTPListenerRuleWithDomain" -) - func (d *envDeployer) lbServiceAllowsHTTP(ctx context.Context, svc string) (bool, error) { - cfn := stack.NewStackDescriber(cfnstack.NameForService(d.app.Name, d.env.Name, svc), d.envManagerSession) - resources, err := cfn.Resources() + stackDescriber := d.newServiceStackDescriber(svc, d.envManagerSession) + resources, err := stackDescriber.Resources() if err != nil { return false, fmt.Errorf("get stack resources: %w", err) } @@ -210,10 +233,10 @@ func (d *envDeployer) lbServiceAllowsHTTP(ctx context.Context, svc string) (bool return false, fmt.Errorf("resource %q not present", svcStackResourceHTTPListenerRuleLogicalID) } - elbv2 := elbv2.New(d.envManagerSession) - isRedirect, err := elbv2.ListenerRuleIsRedirect(ctx, ruleARN) + lbDescriber := d.newLBDescriber(d.envManagerSession) + isRedirect, err := lbDescriber.ListenerRuleIsRedirect(ctx, ruleARN) if err != nil { - return false, fmt.Errorf("verify rule isn't redirect: %w", err) + return false, fmt.Errorf("verify http listener doesn't redirect: %w", err) } return !isRedirect, nil } diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index a4374beabea..609c55eac4d 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -4,6 +4,7 @@ package deploy import ( + "context" "errors" "fmt" "io" @@ -11,21 +12,29 @@ import ( "testing" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/session" awscfn "github.com/aws/aws-sdk-go/service/cloudformation" "github.com/aws/copilot-cli/internal/pkg/cli/deploy/mocks" "github.com/aws/copilot-cli/internal/pkg/config" "github.com/aws/copilot-cli/internal/pkg/deploy" - "github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation/stack" + cfnstack "github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation/stack" "github.com/aws/copilot-cli/internal/pkg/deploy/upload/customresource" + "github.com/aws/copilot-cli/internal/pkg/describe/stack" "github.com/aws/copilot-cli/internal/pkg/manifest" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" ) -type uploadArtifactsMock struct { - appCFN *mocks.MockappResourcesGetter - s3 *mocks.Mockuploader - patcher *mocks.Mockpatcher +type envDeployerMocks struct { + s3 *mocks.Mockuploader + prefixListGetter *mocks.MockprefixListGetter + appCFN *mocks.MockappResourcesGetter + envDeployer *mocks.MockenvironmentDeployer + patcher *mocks.Mockpatcher + stackSerializer *mocks.MockstackSerializer + envDescriber *mocks.MockenvDescriber + lbDescriber *mocks.MocklbDescriber + stackDescribers map[string]*mocks.MockstackDescriber } func TestEnvDeployer_UploadArtifacts(t *testing.T) { @@ -34,25 +43,25 @@ func TestEnvDeployer_UploadArtifacts(t *testing.T) { ) mockApp := &config.Application{} testCases := map[string]struct { - setUpMocks func(m *uploadArtifactsMock) + setUpMocks func(m *envDeployerMocks) wantedOut map[string]string wantedError error }{ "fail to get app resource by region": { - setUpMocks: func(m *uploadArtifactsMock) { + setUpMocks: func(m *envDeployerMocks) { m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(nil, errors.New("some error")) }, wantedError: fmt.Errorf("get app resources in region %s: some error", mockEnvRegion), }, "fail to find S3 bucket in the region": { - setUpMocks: func(m *uploadArtifactsMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{}, nil) + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&cfnstack.AppRegionalResources{}, nil) }, wantedError: fmt.Errorf("cannot find the S3 artifact bucket in region %s", mockEnvRegion), }, "fail to patch the environment": { - setUpMocks: func(m *uploadArtifactsMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.patcher.EXPECT().EnsureManagerRoleIsAllowedToUpload("mockS3Bucket").Return(errors.New("some error")) @@ -60,8 +69,8 @@ func TestEnvDeployer_UploadArtifacts(t *testing.T) { wantedError: errors.New("ensure env manager role has permissions to upload: some error"), }, "fail to upload artifacts": { - setUpMocks: func(m *uploadArtifactsMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.patcher.EXPECT().EnsureManagerRoleIsAllowedToUpload("mockS3Bucket").Return(nil) @@ -70,8 +79,8 @@ func TestEnvDeployer_UploadArtifacts(t *testing.T) { wantedError: errors.New("upload custom resources to bucket mockS3Bucket"), }, "success with URL returned": { - setUpMocks: func(m *uploadArtifactsMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.patcher.EXPECT().EnsureManagerRoleIsAllowedToUpload("mockS3Bucket").Return(nil) @@ -101,7 +110,7 @@ func TestEnvDeployer_UploadArtifacts(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - m := &uploadArtifactsMock{ + m := &envDeployerMocks{ appCFN: mocks.NewMockappResourcesGetter(ctrl), s3: mocks.NewMockuploader(ctrl), patcher: mocks.NewMockpatcher(ctrl), @@ -134,13 +143,6 @@ func TestEnvDeployer_UploadArtifacts(t *testing.T) { } } -type deployEnvironmentMock struct { - appCFN *mocks.MockappResourcesGetter - envDeployer *mocks.MockenvironmentDeployer - stackSerializer *mocks.MockstackSerializer - prefixListGetter *mocks.MockprefixListGetter -} - func TestEnvDeployer_GenerateCloudFormationTemplate(t *testing.T) { const ( mockEnvRegion = "us-west-2" @@ -151,22 +153,22 @@ func TestEnvDeployer_GenerateCloudFormationTemplate(t *testing.T) { Name: mockAppName, } testCases := map[string]struct { - setUpMocks func(m *deployEnvironmentMock) + setUpMocks func(m *envDeployerMocks) wantedTemplate string wantedParams string wantedError error }{ "fail to get app resources by region": { - setUpMocks: func(m *deployEnvironmentMock) { + setUpMocks: func(m *envDeployerMocks) { m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()). Return(nil, errors.New("some error")) }, wantedError: errors.New("get app resources in region us-west-2: some error"), }, "fail to get existing parameters": { - setUpMocks: func(m *deployEnvironmentMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, errors.New("some error")) @@ -174,8 +176,8 @@ func TestEnvDeployer_GenerateCloudFormationTemplate(t *testing.T) { wantedError: errors.New("describe environment stack parameters: some error"), }, "fail to get existing force update ID": { - setUpMocks: func(m *deployEnvironmentMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) @@ -184,8 +186,8 @@ func TestEnvDeployer_GenerateCloudFormationTemplate(t *testing.T) { wantedError: errors.New("retrieve environment stack force update ID: some error"), }, "fail to generate stack template": { - setUpMocks: func(m *deployEnvironmentMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) @@ -195,8 +197,8 @@ func TestEnvDeployer_GenerateCloudFormationTemplate(t *testing.T) { wantedError: errors.New("generate stack template: some error"), }, "fail to generate stack parameters": { - setUpMocks: func(m *deployEnvironmentMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) @@ -207,8 +209,8 @@ func TestEnvDeployer_GenerateCloudFormationTemplate(t *testing.T) { wantedError: errors.New("generate stack template parameters: some error"), }, "successfully return templates environment deployment": { - setUpMocks: func(m *deployEnvironmentMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.envDeployer.EXPECT().DeployedEnvironmentParameters(mockAppName, mockEnvName).Return(nil, nil) @@ -226,7 +228,7 @@ func TestEnvDeployer_GenerateCloudFormationTemplate(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - m := &deployEnvironmentMock{ + m := &envDeployerMocks{ appCFN: mocks.NewMockappResourcesGetter(ctrl), envDeployer: mocks.NewMockenvironmentDeployer(ctrl), stackSerializer: mocks.NewMockstackSerializer(ctrl), @@ -267,20 +269,20 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { Name: mockAppName, } testCases := map[string]struct { - setUpMocks func(m *deployEnvironmentMock) + setUpMocks func(m *envDeployerMocks) inManifest *manifest.Environment wantedError error }{ "fail to get app resources by region": { - setUpMocks: func(m *deployEnvironmentMock) { + setUpMocks: func(m *envDeployerMocks) { m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion). Return(nil, errors.New("some error")) }, wantedError: fmt.Errorf("get app resources in region %s: some error", mockEnvRegion), }, "fail to get prefix list id": { - setUpMocks: func(m *deployEnvironmentMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return("", errors.New("some error")) @@ -303,8 +305,8 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { wantedError: fmt.Errorf("retrieve CloudFront managed prefix list id: some error"), }, "prefix list not retrieved when manifest not present": { - setUpMocks: func(m *deployEnvironmentMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) @@ -315,8 +317,8 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { inManifest: nil, }, "fail to get existing parameters": { - setUpMocks: func(m *deployEnvironmentMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, errors.New("some error")) @@ -324,8 +326,8 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { wantedError: errors.New("describe environment stack parameters: some error"), }, "fail to get existing force update ID": { - setUpMocks: func(m *deployEnvironmentMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.envDeployer.EXPECT().DeployedEnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) @@ -334,8 +336,8 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { wantedError: errors.New("retrieve environment stack force update ID: some error"), }, "fail to deploy environment": { - setUpMocks: func(m *deployEnvironmentMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return("mockPrefixListID", nil).Times(0) @@ -346,8 +348,8 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { wantedError: errors.New("some error"), }, "successful environment deployment": { - setUpMocks: func(m *deployEnvironmentMock) { - m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ + setUpMocks: func(m *envDeployerMocks) { + m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&cfnstack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return("mockPrefixListID", nil).Times(0) @@ -362,7 +364,7 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - m := &deployEnvironmentMock{ + m := &envDeployerMocks{ appCFN: mocks.NewMockappResourcesGetter(ctrl), envDeployer: mocks.NewMockenvironmentDeployer(ctrl), prefixListGetter: mocks.NewMockprefixListGetter(ctrl), @@ -395,3 +397,266 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { }) } } + +func TestEnvDeployer_Verify(t *testing.T) { + tests := map[string]struct { + app *config.Application + mft *manifest.Environment + setUpMocks func(*envDeployerMocks, *gomock.Controller) + expected string + }{ + "cdn enabled, domain imported, no public http certs and validate aliases fails": { + app: &config.Application{ + Domain: "example.com", + }, + mft: &manifest.Environment{ + EnvironmentConfig: manifest.EnvironmentConfig{ + CDNConfig: manifest.EnvironmentCDNConfig{ + Enabled: aws.Bool(true), + }, + }, + }, + setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { + m.envDescriber.EXPECT().ValidateCFServiceDomainAliases().Return(errors.New("some error")) + }, + expected: "some error", + }, + "cdn enabled, domain imported, no public http certs and validate aliases succeeds": { + app: &config.Application{ + Domain: "example.com", + }, + mft: &manifest.Environment{ + EnvironmentConfig: manifest.EnvironmentConfig{ + CDNConfig: manifest.EnvironmentCDNConfig{ + Enabled: aws.Bool(true), + }, + }, + }, + setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { + m.envDescriber.EXPECT().ValidateCFServiceDomainAliases().Return(nil) + }, + }, + "cdn tls termination enabled, fail to get env stack params": { + app: &config.Application{}, + mft: &manifest.Environment{ + EnvironmentConfig: manifest.EnvironmentConfig{ + CDNConfig: manifest.EnvironmentCDNConfig{ + Config: manifest.AdvancedCDNConfig{ + TerminateTLS: aws.Bool(true), + }, + }, + }, + }, + setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { + m.envDescriber.EXPECT().Params().Return(nil, errors.New("some error")) + }, + expected: "can't enable TLS termination on CDN: get env params: some error", + }, + "cdn tls termination enabled, fail to get service resources": { + app: &config.Application{}, + mft: &manifest.Environment{ + EnvironmentConfig: manifest.EnvironmentConfig{ + CDNConfig: manifest.EnvironmentCDNConfig{ + Config: manifest.AdvancedCDNConfig{ + TerminateTLS: aws.Bool(true), + }, + }, + }, + }, + setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { + m.stackDescribers = map[string]*mocks.MockstackDescriber{ + "svc1": mocks.NewMockstackDescriber(ctrl), + } + + m.envDescriber.EXPECT().Params().Return(map[string]string{ + "ALBWorkloads": "svc1", + }, nil) + m.stackDescribers["svc1"].EXPECT().Resources().Return(nil, errors.New("some error")) + }, + expected: `can't enable TLS termination on CDN: verify service "svc1": get stack resources: some error`, + }, + "cdn tls termination enabled, fail to check listener rule": { + app: &config.Application{}, + mft: &manifest.Environment{ + EnvironmentConfig: manifest.EnvironmentConfig{ + CDNConfig: manifest.EnvironmentCDNConfig{ + Config: manifest.AdvancedCDNConfig{ + TerminateTLS: aws.Bool(true), + }, + }, + }, + }, + setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { + m.stackDescribers = map[string]*mocks.MockstackDescriber{ + "svc1": mocks.NewMockstackDescriber(ctrl), + } + + m.envDescriber.EXPECT().Params().Return(map[string]string{ + "ALBWorkloads": "svc1", + }, nil) + m.stackDescribers["svc1"].EXPECT().Resources().Return([]*stack.Resource{ + { + LogicalID: "HTTPListenerRuleWithDomain", + PhysicalID: "svc1RuleARN", + }, + }, nil) + m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc1RuleARN").Return(false, errors.New("some error")) + }, + expected: `can't enable TLS termination on CDN: verify service "svc1": verify http listener doesn't redirect: some error`, + }, + "cdn tls termination enabled, fail with one service that redirects": { + app: &config.Application{}, + mft: &manifest.Environment{ + EnvironmentConfig: manifest.EnvironmentConfig{ + CDNConfig: manifest.EnvironmentCDNConfig{ + Config: manifest.AdvancedCDNConfig{ + TerminateTLS: aws.Bool(true), + }, + }, + }, + }, + setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { + m.stackDescribers = map[string]*mocks.MockstackDescriber{ + "svc1": mocks.NewMockstackDescriber(ctrl), + } + + m.envDescriber.EXPECT().Params().Return(map[string]string{ + "ALBWorkloads": "svc1", + }, nil) + m.stackDescribers["svc1"].EXPECT().Resources().Return([]*stack.Resource{ + { + LogicalID: "HTTPListenerRuleWithDomain", + PhysicalID: "svc1RuleARN", + }, + }, nil) + m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc1RuleARN").Return(true, nil) + }, + expected: "can't enable TLS termination on CDN: HTTP traffic redirects to HTTPS in service svc1.\nSet http.redirect_to_https to false for that service and redeploy it.", + }, + "cdn tls termination enabled, fail with one service that doesn't redirects, two that do redirect": { + app: &config.Application{}, + mft: &manifest.Environment{ + EnvironmentConfig: manifest.EnvironmentConfig{ + CDNConfig: manifest.EnvironmentCDNConfig{ + Config: manifest.AdvancedCDNConfig{ + TerminateTLS: aws.Bool(true), + }, + }, + }, + }, + setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { + m.stackDescribers = map[string]*mocks.MockstackDescriber{ + "svc1": mocks.NewMockstackDescriber(ctrl), + "svc2": mocks.NewMockstackDescriber(ctrl), + "svc3": mocks.NewMockstackDescriber(ctrl), + } + + m.envDescriber.EXPECT().Params().Return(map[string]string{ + "ALBWorkloads": "svc1,svc2,svc3", + }, nil) + m.stackDescribers["svc1"].EXPECT().Resources().Return([]*stack.Resource{ + { + LogicalID: "HTTPListenerRuleWithDomain", + PhysicalID: "svc1RuleARN", + }, + }, nil) + m.stackDescribers["svc2"].EXPECT().Resources().Return([]*stack.Resource{ + { + LogicalID: "HTTPListenerRuleWithDomain", + PhysicalID: "svc2RuleARN", + }, + }, nil) + m.stackDescribers["svc3"].EXPECT().Resources().Return([]*stack.Resource{ + { + LogicalID: "HTTPListenerRuleWithDomain", + PhysicalID: "svc3RuleARN", + }, + }, nil) + m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc1RuleARN").Return(true, nil) + m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc2RuleARN").Return(false, nil) + m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc3RuleARN").Return(true, nil) + }, + expected: "can't enable TLS termination on CDN: HTTP traffic redirects to HTTPS in services svc1 and svc3.\nSet http.redirect_to_https to false for those services and redeploy them.", + }, + "cdn tls termination enabled, success with three services that don't redirect": { + app: &config.Application{}, + mft: &manifest.Environment{ + EnvironmentConfig: manifest.EnvironmentConfig{ + CDNConfig: manifest.EnvironmentCDNConfig{ + Config: manifest.AdvancedCDNConfig{ + TerminateTLS: aws.Bool(true), + }, + }, + }, + }, + setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { + m.stackDescribers = map[string]*mocks.MockstackDescriber{ + "svc1": mocks.NewMockstackDescriber(ctrl), + "svc2": mocks.NewMockstackDescriber(ctrl), + "svc3": mocks.NewMockstackDescriber(ctrl), + } + + m.envDescriber.EXPECT().Params().Return(map[string]string{ + "ALBWorkloads": "svc1,svc2,svc3", + }, nil) + m.stackDescribers["svc1"].EXPECT().Resources().Return([]*stack.Resource{ + { + LogicalID: "HTTPListenerRuleWithDomain", + PhysicalID: "svc1RuleARN", + }, + }, nil) + m.stackDescribers["svc2"].EXPECT().Resources().Return([]*stack.Resource{ + { + LogicalID: "HTTPListenerRuleWithDomain", + PhysicalID: "svc2RuleARN", + }, + }, nil) + m.stackDescribers["svc3"].EXPECT().Resources().Return([]*stack.Resource{ + { + LogicalID: "HTTPListenerRuleWithDomain", + PhysicalID: "svc3RuleARN", + }, + }, nil) + m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc1RuleARN").Return(false, nil) + m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc2RuleARN").Return(false, nil) + m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc3RuleARN").Return(false, nil) + }, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + m := &envDeployerMocks{ + envDescriber: mocks.NewMockenvDescriber(ctrl), + lbDescriber: mocks.NewMocklbDescriber(ctrl), + } + if tc.setUpMocks != nil { + tc.setUpMocks(m, ctrl) + } + + d := &envDeployer{ + app: tc.app, + env: &config.Environment{ + Name: aws.StringValue(tc.mft.Name), + }, + envDescriber: m.envDescriber, + newLBDescriber: func(*session.Session) lbDescriber { + return m.lbDescriber + }, + newServiceStackDescriber: func(svc string, sess *session.Session) stackDescriber { + return m.stackDescribers[svc] + }, + } + + err := d.Verify(context.Background(), tc.mft) + if tc.expected != "" { + require.EqualError(t, err, tc.expected) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/internal/pkg/cli/deploy/mocks/mock_env.go b/internal/pkg/cli/deploy/mocks/mock_env.go index 84bd252619c..41159f575b8 100644 --- a/internal/pkg/cli/deploy/mocks/mock_env.go +++ b/internal/pkg/cli/deploy/mocks/mock_env.go @@ -5,6 +5,7 @@ package mocks import ( + context "context" reflect "reflect" cloudformation "github.com/aws/aws-sdk-go/service/cloudformation" @@ -12,6 +13,7 @@ import ( config "github.com/aws/copilot-cli/internal/pkg/config" cloudformation1 "github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation" stack "github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation/stack" + stack0 "github.com/aws/copilot-cli/internal/pkg/describe/stack" gomock "github.com/golang/mock/gomock" ) @@ -199,3 +201,131 @@ func (mr *MockprefixListGetterMockRecorder) CloudFrontManagedPrefixListID() *gom mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CloudFrontManagedPrefixListID", reflect.TypeOf((*MockprefixListGetter)(nil).CloudFrontManagedPrefixListID)) } + +// MockenvDescriber is a mock of envDescriber interface. +type MockenvDescriber struct { + ctrl *gomock.Controller + recorder *MockenvDescriberMockRecorder +} + +// MockenvDescriberMockRecorder is the mock recorder for MockenvDescriber. +type MockenvDescriberMockRecorder struct { + mock *MockenvDescriber +} + +// NewMockenvDescriber creates a new mock instance. +func NewMockenvDescriber(ctrl *gomock.Controller) *MockenvDescriber { + mock := &MockenvDescriber{ctrl: ctrl} + mock.recorder = &MockenvDescriberMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockenvDescriber) EXPECT() *MockenvDescriberMockRecorder { + return m.recorder +} + +// Params mocks base method. +func (m *MockenvDescriber) Params() (map[string]string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Params") + ret0, _ := ret[0].(map[string]string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Params indicates an expected call of Params. +func (mr *MockenvDescriberMockRecorder) Params() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Params", reflect.TypeOf((*MockenvDescriber)(nil).Params)) +} + +// ValidateCFServiceDomainAliases mocks base method. +func (m *MockenvDescriber) ValidateCFServiceDomainAliases() error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ValidateCFServiceDomainAliases") + ret0, _ := ret[0].(error) + return ret0 +} + +// ValidateCFServiceDomainAliases indicates an expected call of ValidateCFServiceDomainAliases. +func (mr *MockenvDescriberMockRecorder) ValidateCFServiceDomainAliases() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateCFServiceDomainAliases", reflect.TypeOf((*MockenvDescriber)(nil).ValidateCFServiceDomainAliases)) +} + +// MocklbDescriber is a mock of lbDescriber interface. +type MocklbDescriber struct { + ctrl *gomock.Controller + recorder *MocklbDescriberMockRecorder +} + +// MocklbDescriberMockRecorder is the mock recorder for MocklbDescriber. +type MocklbDescriberMockRecorder struct { + mock *MocklbDescriber +} + +// NewMocklbDescriber creates a new mock instance. +func NewMocklbDescriber(ctrl *gomock.Controller) *MocklbDescriber { + mock := &MocklbDescriber{ctrl: ctrl} + mock.recorder = &MocklbDescriberMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MocklbDescriber) EXPECT() *MocklbDescriberMockRecorder { + return m.recorder +} + +// ListenerRuleIsRedirect mocks base method. +func (m *MocklbDescriber) ListenerRuleIsRedirect(arg0 context.Context, arg1 string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ListenerRuleIsRedirect", arg0, arg1) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ListenerRuleIsRedirect indicates an expected call of ListenerRuleIsRedirect. +func (mr *MocklbDescriberMockRecorder) ListenerRuleIsRedirect(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListenerRuleIsRedirect", reflect.TypeOf((*MocklbDescriber)(nil).ListenerRuleIsRedirect), arg0, arg1) +} + +// MockstackDescriber is a mock of stackDescriber interface. +type MockstackDescriber struct { + ctrl *gomock.Controller + recorder *MockstackDescriberMockRecorder +} + +// MockstackDescriberMockRecorder is the mock recorder for MockstackDescriber. +type MockstackDescriberMockRecorder struct { + mock *MockstackDescriber +} + +// NewMockstackDescriber creates a new mock instance. +func NewMockstackDescriber(ctrl *gomock.Controller) *MockstackDescriber { + mock := &MockstackDescriber{ctrl: ctrl} + mock.recorder = &MockstackDescriberMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockstackDescriber) EXPECT() *MockstackDescriberMockRecorder { + return m.recorder +} + +// Resources mocks base method. +func (m *MockstackDescriber) Resources() ([]*stack0.Resource, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Resources") + ret0, _ := ret[0].([]*stack0.Resource) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Resources indicates an expected call of Resources. +func (mr *MockstackDescriberMockRecorder) Resources() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Resources", reflect.TypeOf((*MockstackDescriber)(nil).Resources)) +} From 97d9e21057e913e872829d6c1a697070926767e6 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Tue, 20 Sep 2022 16:40:56 -0700 Subject: [PATCH 09/35] add `Verify()` to `env package` --- internal/pkg/cli/deploy/env.go | 10 +++++--- internal/pkg/cli/env_package.go | 5 +++- internal/pkg/cli/env_package_test.go | 31 +++++++++++++++++++++++ internal/pkg/cli/interfaces.go | 1 + internal/pkg/cli/mocks/mock_interfaces.go | 14 ++++++++++ 5 files changed, 56 insertions(+), 5 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index fd1cface173..cef75954a65 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -151,6 +151,8 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { }, nil } +// Verify checks that the manifest configuration is valid to deploy alongside currently +// deployed resources in this environment. func (d *envDeployer) Verify(ctx context.Context, mft *manifest.Environment) error { if mft.CDNEnabled() && mft.HTTPConfig.Public.Certificates == nil && d.app.Domain != "" { // With managed domain, if the customer isn't using `alias` the A-records are inserted in the service stack as each service domain is unique. @@ -186,11 +188,11 @@ func (d *envDeployer) verifyALBWorkloadsDontRedirect(ctx context.Context) error for i := range services { svc := services[i] g.Go(func() error { - allowsHTTP, err := d.lbServiceAllowsHTTP(ctx, svc) + redirects, err := d.lbServiceRedirects(ctx, svc) switch { case err != nil: return fmt.Errorf("verify service %q: %w", svc, err) - case !allowsHTTP: + case redirects: badServicesMu.Lock() defer badServicesMu.Unlock() badServices = append(badServices, svc) @@ -216,7 +218,7 @@ func (d *envDeployer) verifyALBWorkloadsDontRedirect(ctx context.Context) error return nil } -func (d *envDeployer) lbServiceAllowsHTTP(ctx context.Context, svc string) (bool, error) { +func (d *envDeployer) lbServiceRedirects(ctx context.Context, svc string) (bool, error) { stackDescriber := d.newServiceStackDescriber(svc, d.envManagerSession) resources, err := stackDescriber.Resources() if err != nil { @@ -238,7 +240,7 @@ func (d *envDeployer) lbServiceAllowsHTTP(ctx context.Context, svc string) (bool if err != nil { return false, fmt.Errorf("verify http listener doesn't redirect: %w", err) } - return !isRedirect, nil + return isRedirect, nil } // UploadArtifacts uploads the deployment artifacts for the environment. diff --git a/internal/pkg/cli/env_package.go b/internal/pkg/cli/env_package.go index 44796423b31..d404bcffaa8 100644 --- a/internal/pkg/cli/env_package.go +++ b/internal/pkg/cli/env_package.go @@ -4,6 +4,7 @@ package cli import ( + "context" "fmt" "io" "os" @@ -154,7 +155,9 @@ func (o *packageEnvOpts) Execute() error { if err != nil { return err } - + if err := deployer.Verify(context.Background(), mft); err != nil { + return err + } var urls map[string]string if o.uploadAssets { urls, err = deployer.UploadArtifacts() diff --git a/internal/pkg/cli/env_package_test.go b/internal/pkg/cli/env_package_test.go index 3cd60c03ffd..e0349f760ad 100644 --- a/internal/pkg/cli/env_package_test.go +++ b/internal/pkg/cli/env_package_test.go @@ -192,6 +192,34 @@ func TestPackageEnvOpts_Execute(t *testing.T) { }, wantedErr: errors.New(`get caller principal identity: some error`), }, + "should return a wrapped error when fails to verify env": { + mockedCmd: func(ctrl *gomock.Controller) *packageEnvOpts { + ws := mocks.NewMockwsEnvironmentReader(ctrl) + ws.EXPECT().ReadEnvironmentManifest(gomock.Any()).Return([]byte("name: test\ntype: Environment\n"), nil) + interop := mocks.NewMockinterpolator(ctrl) + interop.EXPECT().Interpolate(gomock.Any()).Return("name: test\ntype: Environment\n", nil) + caller := mocks.NewMockidentityService(ctrl) + caller.EXPECT().Get().Return(identity.Caller{}, nil) + deployer := mocks.NewMockenvPackager(ctrl) + deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(errors.New("mock error")) + + return &packageEnvOpts{ + packageEnvVars: packageEnvVars{ + envName: "test", + }, + ws: ws, + caller: caller, + newInterpolator: func(_, _ string) interpolator { + return interop + }, + newEnvDeployer: func() (envPackager, error) { + return deployer, nil + }, + envCfg: &config.Environment{Name: "test"}, + } + }, + wantedErr: errors.New(`mock error`), + }, "should return a wrapped error when uploading assets fails": { mockedCmd: func(ctrl *gomock.Controller) *packageEnvOpts { ws := mocks.NewMockwsEnvironmentReader(ctrl) @@ -201,6 +229,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) + deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(nil) deployer.EXPECT().UploadArtifacts().Return(nil, errors.New("some error")) return &packageEnvOpts{ @@ -230,6 +259,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) + deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(nil) deployer.EXPECT().GenerateCloudFormationTemplate(gomock.Any()).Return(nil, errors.New("some error")) return &packageEnvOpts{ @@ -258,6 +288,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) + deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(nil) deployer.EXPECT().GenerateCloudFormationTemplate(gomock.Any()).Return(&deploy.GenerateCloudFormationTemplateOutput{ Template: "template", Parameters: "parameters", diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index 793b5161204..3d46c471469 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -655,5 +655,6 @@ type envDeployer interface { type envPackager interface { GenerateCloudFormationTemplate(in *clideploy.DeployEnvironmentInput) (*clideploy.GenerateCloudFormationTemplateOutput, error) + Verify(ctx context.Context, mft *manifest.Environment) error UploadArtifacts() (map[string]string, error) } diff --git a/internal/pkg/cli/mocks/mock_interfaces.go b/internal/pkg/cli/mocks/mock_interfaces.go index ac29fc246b5..510efbbbe16 100644 --- a/internal/pkg/cli/mocks/mock_interfaces.go +++ b/internal/pkg/cli/mocks/mock_interfaces.go @@ -6914,3 +6914,17 @@ func (mr *MockenvPackagerMockRecorder) UploadArtifacts() *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UploadArtifacts", reflect.TypeOf((*MockenvPackager)(nil).UploadArtifacts)) } + +// Verify mocks base method. +func (m *MockenvPackager) Verify(ctx context.Context, mft *manifest.Environment) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Verify", ctx, mft) + ret0, _ := ret[0].(error) + return ret0 +} + +// Verify indicates an expected call of Verify. +func (mr *MockenvPackagerMockRecorder) Verify(ctx, mft interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Verify", reflect.TypeOf((*MockenvPackager)(nil).Verify), ctx, mft) +} From ce1cafe5d906f25af4fb770f3a766ba3bfae66bd Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Tue, 20 Sep 2022 16:49:14 -0700 Subject: [PATCH 10/35] function docs --- internal/pkg/aws/elbv2/elbv2.go | 1 + internal/pkg/cli/deploy/env.go | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/pkg/aws/elbv2/elbv2.go b/internal/pkg/aws/elbv2/elbv2.go index 6fff0c33ac6..045f3f642a3 100644 --- a/internal/pkg/aws/elbv2/elbv2.go +++ b/internal/pkg/aws/elbv2/elbv2.go @@ -74,6 +74,7 @@ func (e *ELBV2) ListenerRuleHostHeaders(ruleARN string) ([]string, error) { return hostHeaders, nil } +// ListenerRuleIsRedirect returns true if ruleARN has an action to redirect. func (e *ELBV2) ListenerRuleIsRedirect(ctx context.Context, ruleARN string) (bool, error) { resp, err := e.client.DescribeRules(&elbv2.DescribeRulesInput{ RuleArns: aws.StringSlice([]string{ruleARN}), diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index cef75954a65..bdb7b2bfced 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -151,8 +151,8 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { }, nil } -// Verify checks that the manifest configuration is valid to deploy alongside currently -// deployed resources in this environment. +// Verify checks that the manifest configuration is valid to deploy alongside +// the currently deployed resources. func (d *envDeployer) Verify(ctx context.Context, mft *manifest.Environment) error { if mft.CDNEnabled() && mft.HTTPConfig.Public.Certificates == nil && d.app.Domain != "" { // With managed domain, if the customer isn't using `alias` the A-records are inserted in the service stack as each service domain is unique. @@ -173,6 +173,9 @@ func (d *envDeployer) Verify(ctx context.Context, mft *manifest.Environment) err return nil } +// verifyALBWorkloadsDontRedirect verifies that none of the ALB Workloads +// in this environment have a redirect in their HTTPWithDomain listener. +// If any services redirect, an error is returned. func (d *envDeployer) verifyALBWorkloadsDontRedirect(ctx context.Context) error { params, err := d.envDescriber.Params() if err != nil { @@ -218,6 +221,7 @@ func (d *envDeployer) verifyALBWorkloadsDontRedirect(ctx context.Context) error return nil } +// lbServiceRedirects returns true if svc's HTTP listener rule redirects. func (d *envDeployer) lbServiceRedirects(ctx context.Context, svc string) (bool, error) { stackDescriber := d.newServiceStackDescriber(svc, d.envManagerSession) resources, err := stackDescriber.Resources() From ff705ceabe59d14981ad2b711116f927a0108d0e Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Tue, 20 Sep 2022 17:06:37 -0700 Subject: [PATCH 11/35] fix lint --- internal/pkg/aws/elbv2/elbv2_test.go | 4 ++-- internal/pkg/cli/deploy/env.go | 3 +++ internal/pkg/manifest/validate_env.go | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/pkg/aws/elbv2/elbv2_test.go b/internal/pkg/aws/elbv2/elbv2_test.go index 4221b2301a7..c2da1896f4b 100644 --- a/internal/pkg/aws/elbv2/elbv2_test.go +++ b/internal/pkg/aws/elbv2/elbv2_test.go @@ -234,7 +234,7 @@ func TestELBV2_ListenerRuleIsRedirect(t *testing.T) { RuleArns: aws.StringSlice([]string{mockARN}), }).Return(nil, errors.New("some error")) }, - expectedErr: fmt.Sprintf("get listener rule for mockListenerRuleARN: some error"), + expectedErr: "get listener rule for mockListenerRuleARN: some error", }, "cannot find listener rule": { setUpMock: func(m *mocks.Mockapi) { @@ -242,7 +242,7 @@ func TestELBV2_ListenerRuleIsRedirect(t *testing.T) { RuleArns: aws.StringSlice([]string{mockARN}), }).Return(&elbv2.DescribeRulesOutput{}, nil) }, - expectedErr: fmt.Sprintf("cannot find listener rule mockListenerRuleARN"), + expectedErr: "cannot find listener rule mockListenerRuleARN", }, "rule is redirect": { setUpMock: func(m *mocks.Mockapi) { diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index fda3623922c..b0b562f4850 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -121,6 +121,9 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { Env: in.Env.Name, ConfigStore: in.ConfigStore, }) + if err != nil { + return nil, fmt.Errorf("get env describer: %w", err) + } cfnClient := deploycfn.New(envManagerSession, deploycfn.WithProgressTracker(os.Stderr)) return &envDeployer{ app: in.App, diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 519c078bd49..3081fa9c22d 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -367,7 +367,7 @@ func (i RestrictiveIngress) validate() error { // validate returns nil if advancedCDNConfig is configured correctly. func (cfg AdvancedCDNConfig) validate() error { if cfg.Certificate == nil { - if aws.BoolValue(cfg.TerminateTLS) == true { + if aws.BoolValue(cfg.TerminateTLS) { return &errFieldMustBeSpecified{ missingField: "certificate", conditionalFields: []string{"terminate_tls"}, From d942ac5a05c9436164aaae2d03d8b8825e938b3b Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 21 Sep 2022 16:53:19 -0700 Subject: [PATCH 12/35] change rule api --- internal/pkg/aws/elbv2/elbv2.go | 23 +++++--- internal/pkg/aws/elbv2/elbv2_test.go | 71 ++++++++++++++--------- internal/pkg/cli/deploy/env.go | 27 +++++---- internal/pkg/cli/deploy/env_test.go | 41 ++++++++----- internal/pkg/cli/deploy/mocks/mock_env.go | 15 ++--- 5 files changed, 105 insertions(+), 72 deletions(-) diff --git a/internal/pkg/aws/elbv2/elbv2.go b/internal/pkg/aws/elbv2/elbv2.go index 045f3f642a3..6092ae1c60c 100644 --- a/internal/pkg/aws/elbv2/elbv2.go +++ b/internal/pkg/aws/elbv2/elbv2.go @@ -6,6 +6,7 @@ package elbv2 import ( "context" + "errors" "fmt" "sort" @@ -74,25 +75,31 @@ func (e *ELBV2) ListenerRuleHostHeaders(ruleARN string) ([]string, error) { return hostHeaders, nil } -// ListenerRuleIsRedirect returns true if ruleARN has an action to redirect. -func (e *ELBV2) ListenerRuleIsRedirect(ctx context.Context, ruleARN string) (bool, error) { +type Rule elbv2.Rule + +// DescribeRule returns the Rule with ruleARN. +func (e *ELBV2) DescribeRule(ctx context.Context, ruleARN string) (Rule, error) { resp, err := e.client.DescribeRules(&elbv2.DescribeRulesInput{ RuleArns: aws.StringSlice([]string{ruleARN}), }) switch { case err != nil: - return false, fmt.Errorf("get listener rule for %s: %w", ruleARN, err) + return Rule{}, err case len(resp.Rules) == 0: - return false, fmt.Errorf("cannot find listener rule %s", ruleARN) + return Rule{}, errors.New("not found") } - rule := resp.Rules[0] - for _, action := range rule.Actions { + return Rule(*resp.Rules[0]), nil +} + +// HasRedirectAction returns true if the rule has a redirect action. +func (r *Rule) HasRedirectAction() bool { + for _, action := range r.Actions { if aws.StringValue(action.Type) == elbv2.ActionTypeEnumRedirect { - return true, nil + return true } } - return false, nil + return false } // TargetHealth wraps up elbv2.TargetHealthDescription. diff --git a/internal/pkg/aws/elbv2/elbv2_test.go b/internal/pkg/aws/elbv2/elbv2_test.go index c2da1896f4b..88d612384fd 100644 --- a/internal/pkg/aws/elbv2/elbv2_test.go +++ b/internal/pkg/aws/elbv2/elbv2_test.go @@ -220,13 +220,13 @@ func TestELBV2_ListenerRuleHostHeaders(t *testing.T) { } } -func TestELBV2_ListenerRuleIsRedirect(t *testing.T) { +func TestELBV2_DescribeRule(t *testing.T) { mockARN := "mockListenerRuleARN" testCases := map[string]struct { setUpMock func(m *mocks.Mockapi) expectedErr string - expected bool + expected Rule }{ "fail to describe rules": { setUpMock: func(m *mocks.Mockapi) { @@ -242,43 +242,23 @@ func TestELBV2_ListenerRuleIsRedirect(t *testing.T) { RuleArns: aws.StringSlice([]string{mockARN}), }).Return(&elbv2.DescribeRulesOutput{}, nil) }, - expectedErr: "cannot find listener rule mockListenerRuleARN", + expectedErr: `listener rule "mockListenerRuleARN" not found`, }, - "rule is redirect": { - setUpMock: func(m *mocks.Mockapi) { - m.EXPECT().DescribeRules(&elbv2.DescribeRulesInput{ - RuleArns: aws.StringSlice([]string{mockARN}), - }).Return(&elbv2.DescribeRulesOutput{ - Rules: []*elbv2.Rule{ - { - Actions: []*elbv2.Action{ - { - Type: aws.String(elbv2.ActionTypeEnumRedirect), - }, - }, - }, - }, - }, nil) - }, - expected: true, - }, - "rule is not redirect": { + "success": { setUpMock: func(m *mocks.Mockapi) { m.EXPECT().DescribeRules(&elbv2.DescribeRulesInput{ RuleArns: aws.StringSlice([]string{mockARN}), }).Return(&elbv2.DescribeRulesOutput{ Rules: []*elbv2.Rule{ { - Actions: []*elbv2.Action{ - { - Type: aws.String(elbv2.ActionTypeEnumForward), - }, - }, + RuleArn: aws.String(mockARN), }, }, }, nil) }, - expected: false, + expected: Rule(elbv2.Rule{ + RuleArn: aws.String(mockARN), + }), }, } @@ -294,7 +274,7 @@ func TestELBV2_ListenerRuleIsRedirect(t *testing.T) { client: mockAPI, } - actual, err := elbv2Client.ListenerRuleIsRedirect(context.Background(), mockARN) + actual, err := elbv2Client.DescribeRule(context.Background(), mockARN) if tc.expectedErr != "" { require.EqualError(t, err, tc.expectedErr) } else { @@ -304,6 +284,39 @@ func TestELBV2_ListenerRuleIsRedirect(t *testing.T) { } } +func TestELBV2Rule_HasRedirectAction(t *testing.T) { + testCases := map[string]struct { + rule Rule + expected bool + }{ + "rule doesn't have redirect": { + rule: Rule(elbv2.Rule{ + Actions: []*elbv2.Action{ + { + Type: aws.String(elbv2.ActionTypeEnumForward), + }, + }, + }), + }, + "rule has redirect": { + rule: Rule(elbv2.Rule{ + Actions: []*elbv2.Action{ + { + Type: aws.String(elbv2.ActionTypeEnumRedirect), + }, + }, + }), + expected: true, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + require.Equal(t, tc.expected, tc.rule.HasRedirectAction()) + }) + } +} + func TestTargetHealth_HealthStatus(t *testing.T) { testCases := map[string]struct { inTargetHealth *TargetHealth diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index b0b562f4850..2a794b579a3 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -61,7 +61,7 @@ type envDescriber interface { } type lbDescriber interface { - ListenerRuleIsRedirect(context.Context, string) (bool, error) + DescribeRule(context.Context, string) (elbv2.Rule, error) } type stackDescriber interface { @@ -87,8 +87,8 @@ type envDeployer struct { newStackSerializer func(input *deploy.CreateEnvironmentInput, forceUpdateID string, prevParams []*awscfn.Parameter) stackSerializer envDescriber envDescriber envManagerSession *session.Session - newLBDescriber func(*session.Session) lbDescriber - newServiceStackDescriber func(string, *session.Session) stackDescriber + lbDescriber lbDescriber + newServiceStackDescriber func(string) stackDescriber // Cached variables. appRegionalResources *cfnstack.AppRegionalResources @@ -145,11 +145,9 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { }, envDescriber: envDescriber, envManagerSession: envManagerSession, - newLBDescriber: func(sess *session.Session) lbDescriber { - return elbv2.New(sess) - }, - newServiceStackDescriber: func(svc string, sess *session.Session) stackDescriber { - return stack.NewStackDescriber(cfnstack.NameForService(in.App.Name, in.Env.Name, svc), sess) + lbDescriber: elbv2.New(envManagerSession), + newServiceStackDescriber: func(svc string) stackDescriber { + return stack.NewStackDescriber(cfnstack.NameForService(in.App.Name, in.Env.Name, svc), envManagerSession) }, }, nil } @@ -226,7 +224,7 @@ func (d *envDeployer) verifyALBWorkloadsDontRedirect(ctx context.Context) error // lbServiceRedirects returns true if svc's HTTP listener rule redirects. func (d *envDeployer) lbServiceRedirects(ctx context.Context, svc string) (bool, error) { - stackDescriber := d.newServiceStackDescriber(svc, d.envManagerSession) + stackDescriber := d.newServiceStackDescriber(svc) resources, err := stackDescriber.Resources() if err != nil { return false, fmt.Errorf("get stack resources: %w", err) @@ -239,15 +237,16 @@ func (d *envDeployer) lbServiceRedirects(ctx context.Context, svc string) (bool, } } if ruleARN == "" { - return false, fmt.Errorf("resource %q not present", svcStackResourceHTTPListenerRuleLogicalID) + // TODO + // return false, fmt.Errorf("resource %q not present", svcStackResourceHTTPListenerRuleLogicalID) + return false, fmt.Errorf("http listener not found on service %q", svc) } - lbDescriber := d.newLBDescriber(d.envManagerSession) - isRedirect, err := lbDescriber.ListenerRuleIsRedirect(ctx, ruleARN) + rule, err := d.lbDescriber.DescribeRule(ctx, ruleARN) if err != nil { - return false, fmt.Errorf("verify http listener doesn't redirect: %w", err) + return false, fmt.Errorf("get listener rule %q: %w", ruleARN, err) } - return isRedirect, nil + return rule.HasRedirectAction(), nil } // UploadArtifacts uploads the deployment artifacts for the environment. diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 609c55eac4d..13d6733c3e8 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -12,8 +12,9 @@ import ( "testing" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/session" awscfn "github.com/aws/aws-sdk-go/service/cloudformation" + awselb "github.com/aws/aws-sdk-go/service/elbv2" + "github.com/aws/copilot-cli/internal/pkg/aws/elbv2" "github.com/aws/copilot-cli/internal/pkg/cli/deploy/mocks" "github.com/aws/copilot-cli/internal/pkg/config" "github.com/aws/copilot-cli/internal/pkg/deploy" @@ -399,6 +400,20 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { } func TestEnvDeployer_Verify(t *testing.T) { + listenerRuleNoRedirect := elbv2.Rule{ + Actions: []*awselb.Action{ + { + Type: aws.String(awselb.ActionTypeEnumForward), + }, + }, + } + listenerRuleWithRedirect := elbv2.Rule{ + Actions: []*awselb.Action{ + { + Type: aws.String(awselb.ActionTypeEnumRedirect), + }, + }, + } tests := map[string]struct { app *config.Application mft *manifest.Environment @@ -500,9 +515,9 @@ func TestEnvDeployer_Verify(t *testing.T) { PhysicalID: "svc1RuleARN", }, }, nil) - m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc1RuleARN").Return(false, errors.New("some error")) + m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc1RuleARN").Return(elbv2.Rule{}, errors.New("some error")) }, - expected: `can't enable TLS termination on CDN: verify service "svc1": verify http listener doesn't redirect: some error`, + expected: `can't enable TLS termination on CDN: verify service "svc1": get listener rule "svc1RuleARN": some error`, }, "cdn tls termination enabled, fail with one service that redirects": { app: &config.Application{}, @@ -529,7 +544,7 @@ func TestEnvDeployer_Verify(t *testing.T) { PhysicalID: "svc1RuleARN", }, }, nil) - m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc1RuleARN").Return(true, nil) + m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc1RuleARN").Return(listenerRuleWithRedirect, nil) }, expected: "can't enable TLS termination on CDN: HTTP traffic redirects to HTTPS in service svc1.\nSet http.redirect_to_https to false for that service and redeploy it.", }, @@ -572,9 +587,9 @@ func TestEnvDeployer_Verify(t *testing.T) { PhysicalID: "svc3RuleARN", }, }, nil) - m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc1RuleARN").Return(true, nil) - m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc2RuleARN").Return(false, nil) - m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc3RuleARN").Return(true, nil) + m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc1RuleARN").Return(listenerRuleWithRedirect, nil) + m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc2RuleARN").Return(listenerRuleNoRedirect, nil) + m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc3RuleARN").Return(listenerRuleWithRedirect, nil) }, expected: "can't enable TLS termination on CDN: HTTP traffic redirects to HTTPS in services svc1 and svc3.\nSet http.redirect_to_https to false for those services and redeploy them.", }, @@ -617,9 +632,9 @@ func TestEnvDeployer_Verify(t *testing.T) { PhysicalID: "svc3RuleARN", }, }, nil) - m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc1RuleARN").Return(false, nil) - m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc2RuleARN").Return(false, nil) - m.lbDescriber.EXPECT().ListenerRuleIsRedirect(gomock.Any(), "svc3RuleARN").Return(false, nil) + m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc1RuleARN").Return(listenerRuleNoRedirect, nil) + m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc2RuleARN").Return(listenerRuleNoRedirect, nil) + m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc3RuleARN").Return(listenerRuleNoRedirect, nil) }, }, } @@ -643,10 +658,8 @@ func TestEnvDeployer_Verify(t *testing.T) { Name: aws.StringValue(tc.mft.Name), }, envDescriber: m.envDescriber, - newLBDescriber: func(*session.Session) lbDescriber { - return m.lbDescriber - }, - newServiceStackDescriber: func(svc string, sess *session.Session) stackDescriber { + lbDescriber: m.lbDescriber, + newServiceStackDescriber: func(svc string) stackDescriber { return m.stackDescribers[svc] }, } diff --git a/internal/pkg/cli/deploy/mocks/mock_env.go b/internal/pkg/cli/deploy/mocks/mock_env.go index 41159f575b8..6448448d8b9 100644 --- a/internal/pkg/cli/deploy/mocks/mock_env.go +++ b/internal/pkg/cli/deploy/mocks/mock_env.go @@ -10,6 +10,7 @@ import ( cloudformation "github.com/aws/aws-sdk-go/service/cloudformation" cloudformation0 "github.com/aws/copilot-cli/internal/pkg/aws/cloudformation" + elbv2 "github.com/aws/copilot-cli/internal/pkg/aws/elbv2" config "github.com/aws/copilot-cli/internal/pkg/config" cloudformation1 "github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation" stack "github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation/stack" @@ -277,19 +278,19 @@ func (m *MocklbDescriber) EXPECT() *MocklbDescriberMockRecorder { return m.recorder } -// ListenerRuleIsRedirect mocks base method. -func (m *MocklbDescriber) ListenerRuleIsRedirect(arg0 context.Context, arg1 string) (bool, error) { +// DescribeRule mocks base method. +func (m *MocklbDescriber) DescribeRule(arg0 context.Context, arg1 string) (elbv2.Rule, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ListenerRuleIsRedirect", arg0, arg1) - ret0, _ := ret[0].(bool) + ret := m.ctrl.Call(m, "DescribeRule", arg0, arg1) + ret0, _ := ret[0].(elbv2.Rule) ret1, _ := ret[1].(error) return ret0, ret1 } -// ListenerRuleIsRedirect indicates an expected call of ListenerRuleIsRedirect. -func (mr *MocklbDescriberMockRecorder) ListenerRuleIsRedirect(arg0, arg1 interface{}) *gomock.Call { +// DescribeRule indicates an expected call of DescribeRule. +func (mr *MocklbDescriberMockRecorder) DescribeRule(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListenerRuleIsRedirect", reflect.TypeOf((*MocklbDescriber)(nil).ListenerRuleIsRedirect), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeRule", reflect.TypeOf((*MocklbDescriber)(nil).DescribeRule), arg0, arg1) } // MockstackDescriber is a mock of stackDescriber interface. From 034806625d4c731c14640a3d76363f6a816fe0ab Mon Sep 17 00:00:00 2001 From: danny randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 21 Sep 2022 16:56:18 -0700 Subject: [PATCH 13/35] Update internal/pkg/cli/deploy/env.go Co-authored-by: Efe Karakus --- internal/pkg/cli/deploy/env.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 2a794b579a3..a88ef634173 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -152,9 +152,8 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { }, nil } -// Verify checks that the manifest configuration is valid to deploy alongside -// the currently deployed resources. -func (d *envDeployer) Verify(ctx context.Context, mft *manifest.Environment) error { +// Validate returns an error if the environment manifest is incompatible with services and application configurations. +func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment) error { if mft.CDNEnabled() && mft.HTTPConfig.Public.Certificates == nil && d.app.Domain != "" { // With managed domain, if the customer isn't using `alias` the A-records are inserted in the service stack as each service domain is unique. // However, when clients enable CloudFront, they would need to update all their existing records to now point to the distribution. From d084e3472aceba3db455d6cc3b9ee63ae455844d Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 21 Sep 2022 16:58:52 -0700 Subject: [PATCH 14/35] rename to validate --- internal/pkg/cli/deploy/env_test.go | 4 ++-- internal/pkg/cli/env_deploy.go | 2 +- internal/pkg/cli/env_deploy_test.go | 8 ++++---- internal/pkg/cli/env_package.go | 2 +- internal/pkg/cli/env_package_test.go | 8 ++++---- internal/pkg/cli/interfaces.go | 4 ++-- internal/pkg/cli/mocks/mock_interfaces.go | 24 +++++++++++------------ 7 files changed, 26 insertions(+), 26 deletions(-) diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 13d6733c3e8..4d63e3f19ac 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -399,7 +399,7 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { } } -func TestEnvDeployer_Verify(t *testing.T) { +func TestEnvDeployer_Validate(t *testing.T) { listenerRuleNoRedirect := elbv2.Rule{ Actions: []*awselb.Action{ { @@ -664,7 +664,7 @@ func TestEnvDeployer_Verify(t *testing.T) { }, } - err := d.Verify(context.Background(), tc.mft) + err := d.Validate(context.Background(), tc.mft) if tc.expected != "" { require.EqualError(t, err, tc.expected) } else { diff --git a/internal/pkg/cli/env_deploy.go b/internal/pkg/cli/env_deploy.go index fdfbac5d6dc..54e77f2029a 100644 --- a/internal/pkg/cli/env_deploy.go +++ b/internal/pkg/cli/env_deploy.go @@ -133,7 +133,7 @@ func (o *deployEnvOpts) Execute() error { if err != nil { return err } - if err := deployer.Verify(context.Background(), mft); err != nil { + if err := deployer.Validate(context.Background(), mft); err != nil { return err } urls, err := deployer.UploadArtifacts() diff --git a/internal/pkg/cli/env_deploy_test.go b/internal/pkg/cli/env_deploy_test.go index 6cf15775e27..4eed9cf71fb 100644 --- a/internal/pkg/cli/env_deploy_test.go +++ b/internal/pkg/cli/env_deploy_test.go @@ -185,7 +185,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(errors.New("mock error")) + m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(errors.New("mock error")) }, wantedErr: errors.New("mock error"), }, @@ -196,7 +196,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(nil) + m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(nil, errors.New("some error")) }, wantedErr: errors.New("upload artifacts for environment mockEnv: some error"), @@ -208,7 +208,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(nil) + m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(map[string]string{ "mockResource": "mockURL", }, nil) @@ -225,7 +225,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(nil) + m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(map[string]string{ "mockResource": "mockURL", }, nil) diff --git a/internal/pkg/cli/env_package.go b/internal/pkg/cli/env_package.go index e2c7593c3a5..81610b783e3 100644 --- a/internal/pkg/cli/env_package.go +++ b/internal/pkg/cli/env_package.go @@ -155,7 +155,7 @@ func (o *packageEnvOpts) Execute() error { if err != nil { return err } - if err := deployer.Verify(context.Background(), mft); err != nil { + if err := deployer.Validate(context.Background(), mft); err != nil { return err } var urls map[string]string diff --git a/internal/pkg/cli/env_package_test.go b/internal/pkg/cli/env_package_test.go index 1dcf7c7a003..5f2bbb47120 100644 --- a/internal/pkg/cli/env_package_test.go +++ b/internal/pkg/cli/env_package_test.go @@ -204,7 +204,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(errors.New("mock error")) + deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(errors.New("mock error")) return &packageEnvOpts{ packageEnvVars: packageEnvVars{ @@ -232,7 +232,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(nil) + deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) deployer.EXPECT().UploadArtifacts().Return(nil, errors.New("some error")) return &packageEnvOpts{ @@ -262,7 +262,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(nil) + deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) deployer.EXPECT().GenerateCloudFormationTemplate(gomock.Any()).Return(nil, errors.New("some error")) return &packageEnvOpts{ @@ -292,7 +292,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Verify(gomock.Any(), gomock.Any()).Return(nil) + deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) deployer.EXPECT().GenerateCloudFormationTemplate(&deploy.DeployEnvironmentInput{ RootUserARN: "", CustomResourcesURLs: nil, diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index 3d46c471469..b668a36a269 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -649,12 +649,12 @@ type runner interface { type envDeployer interface { DeployEnvironment(in *clideploy.DeployEnvironmentInput) error - Verify(ctx context.Context, mft *manifest.Environment) error + Validate(ctx context.Context, mft *manifest.Environment) error UploadArtifacts() (map[string]string, error) } type envPackager interface { GenerateCloudFormationTemplate(in *clideploy.DeployEnvironmentInput) (*clideploy.GenerateCloudFormationTemplateOutput, error) - Verify(ctx context.Context, mft *manifest.Environment) error + Validate(ctx context.Context, mft *manifest.Environment) error UploadArtifacts() (map[string]string, error) } diff --git a/internal/pkg/cli/mocks/mock_interfaces.go b/internal/pkg/cli/mocks/mock_interfaces.go index 510efbbbe16..3273262f737 100644 --- a/internal/pkg/cli/mocks/mock_interfaces.go +++ b/internal/pkg/cli/mocks/mock_interfaces.go @@ -6848,18 +6848,18 @@ func (mr *MockenvDeployerMockRecorder) UploadArtifacts() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UploadArtifacts", reflect.TypeOf((*MockenvDeployer)(nil).UploadArtifacts)) } -// Verify mocks base method. -func (m *MockenvDeployer) Verify(ctx context.Context, mft *manifest.Environment) error { +// Validate mocks base method. +func (m *MockenvDeployer) Validate(ctx context.Context, mft *manifest.Environment) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Verify", ctx, mft) + ret := m.ctrl.Call(m, "Validate", ctx, mft) ret0, _ := ret[0].(error) return ret0 } -// Verify indicates an expected call of Verify. -func (mr *MockenvDeployerMockRecorder) Verify(ctx, mft interface{}) *gomock.Call { +// Validate indicates an expected call of Validate. +func (mr *MockenvDeployerMockRecorder) Validate(ctx, mft interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Verify", reflect.TypeOf((*MockenvDeployer)(nil).Verify), ctx, mft) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvDeployer)(nil).Validate), ctx, mft) } // MockenvPackager is a mock of envPackager interface. @@ -6915,16 +6915,16 @@ func (mr *MockenvPackagerMockRecorder) UploadArtifacts() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UploadArtifacts", reflect.TypeOf((*MockenvPackager)(nil).UploadArtifacts)) } -// Verify mocks base method. -func (m *MockenvPackager) Verify(ctx context.Context, mft *manifest.Environment) error { +// Validate mocks base method. +func (m *MockenvPackager) Validate(ctx context.Context, mft *manifest.Environment) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Verify", ctx, mft) + ret := m.ctrl.Call(m, "Validate", ctx, mft) ret0, _ := ret[0].(error) return ret0 } -// Verify indicates an expected call of Verify. -func (mr *MockenvPackagerMockRecorder) Verify(ctx, mft interface{}) *gomock.Call { +// Validate indicates an expected call of Validate. +func (mr *MockenvPackagerMockRecorder) Validate(ctx, mft interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Verify", reflect.TypeOf((*MockenvPackager)(nil).Verify), ctx, mft) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvPackager)(nil).Validate), ctx, mft) } From 8bae8a80ec65ace523dc2b26e2c3c339bfe0215c Mon Sep 17 00:00:00 2001 From: danny randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 21 Sep 2022 16:59:59 -0700 Subject: [PATCH 15/35] Update internal/pkg/cli/deploy/env.go Co-authored-by: Efe Karakus --- internal/pkg/cli/deploy/env.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index a88ef634173..f0910dcba11 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -173,7 +173,7 @@ func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment) e return nil } -// verifyALBWorkloadsDontRedirect verifies that none of the ALB Workloads +// verifyALBWorkloadsDontRedirect verifies that none of the public ALB Workloads // in this environment have a redirect in their HTTPWithDomain listener. // If any services redirect, an error is returned. func (d *envDeployer) verifyALBWorkloadsDontRedirect(ctx context.Context) error { From dc7bde099199c4641d9fef37e864863a9b44f089 Mon Sep 17 00:00:00 2001 From: danny randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 21 Sep 2022 17:00:46 -0700 Subject: [PATCH 16/35] Update internal/pkg/cli/deploy/env.go Co-authored-by: Wanxian Yang <79273084+Lou1415926@users.noreply.github.com> --- internal/pkg/cli/deploy/env.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index f0910dcba11..28a4f3d8d1b 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -229,7 +229,7 @@ func (d *envDeployer) lbServiceRedirects(ctx context.Context, svc string) (bool, return false, fmt.Errorf("get stack resources: %w", err) } - ruleARN := "" + var ruleARN string for _, res := range resources { if res.LogicalID == svcStackResourceHTTPListenerRuleLogicalID { ruleARN = res.PhysicalID From 026df8c4cc9af6f8916bddf0ddc25f60749f46c0 Mon Sep 17 00:00:00 2001 From: danny randall <10566468+dannyrandall@users.noreply.github.com> Date: Wed, 21 Sep 2022 17:01:09 -0700 Subject: [PATCH 17/35] Update internal/pkg/cli/deploy/env.go Co-authored-by: Wanxian Yang <79273084+Lou1415926@users.noreply.github.com> --- internal/pkg/cli/deploy/env.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 28a4f3d8d1b..8e8852926b0 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -233,6 +233,7 @@ func (d *envDeployer) lbServiceRedirects(ctx context.Context, svc string) (bool, for _, res := range resources { if res.LogicalID == svcStackResourceHTTPListenerRuleLogicalID { ruleARN = res.PhysicalID + break } } if ruleARN == "" { From 63650d670b2f96a9153ce5f3e699390ceba1a87f Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Thu, 22 Sep 2022 09:56:33 -0700 Subject: [PATCH 18/35] don't leak manifest internals --- internal/pkg/cli/deploy/env.go | 6 +++--- internal/pkg/manifest/env.go | 8 ++++++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 8e8852926b0..c7a6093d26b 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -12,7 +12,6 @@ import ( "strings" "sync" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/session" awscfn "github.com/aws/aws-sdk-go/service/cloudformation" "github.com/aws/copilot-cli/internal/pkg/aws/cloudformation" @@ -154,7 +153,8 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { // Validate returns an error if the environment manifest is incompatible with services and application configurations. func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment) error { - if mft.CDNEnabled() && mft.HTTPConfig.Public.Certificates == nil && d.app.Domain != "" { + isManagedCDNEnabled := mft.CDNEnabled() && !mft.HasImportedPublicALBCerts() && d.app.Domain != "" + if isManagedCDNEnabled { // With managed domain, if the customer isn't using `alias` the A-records are inserted in the service stack as each service domain is unique. // However, when clients enable CloudFront, they would need to update all their existing records to now point to the distribution. // Hence, we force users to use `alias` and let the records be written in the environment stack instead. @@ -163,7 +163,7 @@ func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment) e } } - if mft.CDNEnabled() && aws.BoolValue(mft.CDNConfig.Config.TerminateTLS) { + if mft.CDNEnabled() && mft.CDNDoesTLSTermination() { // ensure all services _are not_ doing http->https redirect if err := d.verifyALBWorkloadsDontRedirect(ctx); err != nil { return fmt.Errorf("can't enable TLS termination on CDN: %w", err) diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 31b665cbe54..493111e661d 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -213,6 +213,14 @@ func (cfg *EnvironmentConfig) CDNEnabled() bool { return aws.BoolValue(cfg.CDNConfig.Enabled) } +func (cfg *EnvironmentConfig) HasImportedPublicALBCerts() bool { + return cfg.HTTPConfig.Public.Certificates != nil +} + +func (cfg *EnvironmentConfig) CDNDoesTLSTermination() bool { + return aws.BoolValue(cfg.CDNConfig.Config.TerminateTLS) +} + // UnmarshalYAML overrides the default YAML unmarshaling logic for the environmentCDNConfig // struct, allowing it to perform more complex unmarshaling behavior. // This method implements the yaml.Unmarshaler (v3) interface. From b3c148d90242cda8396b907f984f93432d591bac Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Thu, 22 Sep 2022 12:58:20 -0700 Subject: [PATCH 19/35] change error to just be a warning --- internal/pkg/cli/deploy/env.go | 60 ++++++++++++++++++---- internal/pkg/manifest/validate_env_test.go | 2 +- 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index c7a6093d26b..200295bb916 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -9,6 +9,7 @@ import ( "io" "os" "sort" + "strconv" "strings" "sync" @@ -30,6 +31,7 @@ import ( "github.com/aws/copilot-cli/internal/pkg/describe/stack" "github.com/aws/copilot-cli/internal/pkg/manifest" "github.com/aws/copilot-cli/internal/pkg/template" + "github.com/aws/copilot-cli/internal/pkg/term/color" "github.com/aws/copilot-cli/internal/pkg/term/log" termprogress "github.com/aws/copilot-cli/internal/pkg/term/progress" "github.com/dustin/go-humanize/english" @@ -67,6 +69,10 @@ type stackDescriber interface { Resources() ([]*stack.Resource, error) } +type Warner interface { + Warning() string +} + const ( svcStackResourceHTTPListenerRuleLogicalID = "HTTPListenerRuleWithDomain" ) @@ -166,13 +172,49 @@ func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment) e if mft.CDNEnabled() && mft.CDNDoesTLSTermination() { // ensure all services _are not_ doing http->https redirect if err := d.verifyALBWorkloadsDontRedirect(ctx); err != nil { - return fmt.Errorf("can't enable TLS termination on CDN: %w", err) + if warn, ok := err.(Warner); ok { + fmt.Println(warn.Warning()) + } else { + return fmt.Errorf("can't enable TLS termination on CDN: %w", err) + } } } return nil } +type errEnvHasPublicServicesWithRedirect struct { + services []string +} + +func (e *errEnvHasPublicServicesWithRedirect) Error() string { + quoted := make([]string, len(e.services)) + for i := range e.services { + quoted[i] = strconv.Quote(e.services[i]) + } + return fmt.Sprintf("%s %s %s HTTP traffic to HTTPS", + english.PluralWord(len(e.services), "Service", "Services"), + english.OxfordWordSeries(quoted, "and"), + english.PluralWord(len(e.services), "redirects", "redirect")) +} + +func (e *errEnvHasPublicServicesWithRedirect) Warning() string { + return fmt.Sprintf(`%s. +%s +To fix this, set the following field in %s manifest: +%s +and run %s. +If you'd like to use %s without a CDN, ensure %s A record is pointed to the ALB. + `, e.Error(), + color.Emphasize(english.PluralWord(len(e.services), "This service", "These services")+" will not be reachable through the CDN."), + english.PluralWord(len(e.services), "its", "each"), + color.HighlightCodeBlock("http:\n redirect_to_https: true"), + color.HighlightCode("copilot svc deploy"), + english.PluralWord(len(e.services), "this service", "these services"), + english.PluralWord(len(e.services), "its", "each service's"), + ) +} + // verifyALBWorkloadsDontRedirect verifies that none of the public ALB Workloads // in this environment have a redirect in their HTTPWithDomain listener. // If any services redirect, an error is returned. @@ -210,18 +252,18 @@ func (d *envDeployer) verifyALBWorkloadsDontRedirect(ctx context.Context) error } if len(badServices) > 0 { sort.Strings(badServices) - return fmt.Errorf("HTTP traffic redirects to HTTPS in %v %v.\nSet http.redirect_to_https to false for %v and redeploy %v.", - english.PluralWord(len(badServices), "service", "services"), - english.OxfordWordSeries(badServices, "and"), - english.PluralWord(len(badServices), "that service", "those services"), - english.PluralWord(len(badServices), "it", "them"), - ) + return &errEnvHasPublicServicesWithRedirect{ + services: badServices, + } } return nil } -// lbServiceRedirects returns true if svc's HTTP listener rule redirects. +// lbServiceRedirects returns true if svc's HTTP listener rule redirects. We only check +// HTTPListenerRuleWithDomain because HTTPListenerRule: +// a) doesn't ever redirect +// b) won't work with cloudfront anyways (can't point ALB default DNS to CF) func (d *envDeployer) lbServiceRedirects(ctx context.Context, svc string) (bool, error) { stackDescriber := d.newServiceStackDescriber(svc) resources, err := stackDescriber.Resources() @@ -237,8 +279,6 @@ func (d *envDeployer) lbServiceRedirects(ctx context.Context, svc string) (bool, } } if ruleARN == "" { - // TODO - // return false, fmt.Errorf("resource %q not present", svcStackResourceHTTPListenerRuleLogicalID) return false, fmt.Errorf("http listener not found on service %q", svc) } diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index da4d62d3c64..e89bc0efcc8 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -744,7 +744,7 @@ func TestCDNConfiguration_validate(t *testing.T) { Enabled: aws.Bool(false), }, }, - "valid if advanced config configured correctly": { + "success with cert without tls termination": { in: EnvironmentCDNConfig{ Config: AdvancedCDNConfig{ Certificate: aws.String("arn:aws:acm:us-east-1:1111111:certificate/look-like-a-good-arn"), From 1db265035f45e6acc1f4ce576c0ac6f21fe72bc7 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Thu, 22 Sep 2022 13:36:31 -0700 Subject: [PATCH 20/35] move constant --- internal/pkg/cli/deploy/env.go | 15 +++++---------- internal/pkg/template/workload.go | 5 +++++ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 200295bb916..2d186739d22 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -73,10 +73,6 @@ type Warner interface { Warning() string } -const ( - svcStackResourceHTTPListenerRuleLogicalID = "HTTPListenerRuleWithDomain" -) - type envDeployer struct { app *config.Application env *config.Environment @@ -169,9 +165,8 @@ func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment) e } } - if mft.CDNEnabled() && mft.CDNDoesTLSTermination() { - // ensure all services _are not_ doing http->https redirect - if err := d.verifyALBWorkloadsDontRedirect(ctx); err != nil { + if mft.CDNEnabled() && mft.CDNDoesTLSTermination() && mft.HasImportedPublicALBCerts() { + if err := d.validateALBWorkloadsDontRedirect(ctx); err != nil { if warn, ok := err.(Warner); ok { fmt.Println(warn.Warning()) } else { @@ -215,10 +210,10 @@ If you'd like to use %s without a CDN, ensure %s A record is pointed to the ALB. ) } -// verifyALBWorkloadsDontRedirect verifies that none of the public ALB Workloads +// validateALBWorkloadsDontRedirect verifies that none of the public ALB Workloads // in this environment have a redirect in their HTTPWithDomain listener. // If any services redirect, an error is returned. -func (d *envDeployer) verifyALBWorkloadsDontRedirect(ctx context.Context) error { +func (d *envDeployer) validateALBWorkloadsDontRedirect(ctx context.Context) error { params, err := d.envDescriber.Params() if err != nil { return fmt.Errorf("get env params: %w", err) @@ -273,7 +268,7 @@ func (d *envDeployer) lbServiceRedirects(ctx context.Context, svc string) (bool, var ruleARN string for _, res := range resources { - if res.LogicalID == svcStackResourceHTTPListenerRuleLogicalID { + if res.LogicalID == template.LogicalIDHTTPListenerRuleWithDomain { ruleARN = res.PhysicalID break } diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index 9207aa2fe79..966a4d3b9a8 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -62,6 +62,11 @@ const ( snsARNPattern = "arn:%s:sns:%s:%s:%s-%s-%s-%s" ) +// Constants for stack resource logical ID's +const ( + LogicalIDHTTPListenerRuleWithDomain = "HTTPListenerRuleWithDomain" +) + var ( // Template names under "workloads/partials/cf/". partialsWorkloadCFTemplateNames = []string{ From c52c836b67f2a82fa6a5b1dc669437389b013ef0 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Thu, 22 Sep 2022 14:12:50 -0700 Subject: [PATCH 21/35] warn sometimes, error sometimes --- internal/pkg/cli/deploy/env.go | 54 +++++++------ internal/pkg/cli/deploy/env_test.go | 113 ++++++++-------------------- internal/pkg/cli/env_deploy.go | 2 +- internal/pkg/cli/env_package.go | 2 +- internal/pkg/cli/interfaces.go | 4 +- internal/pkg/manifest/env.go | 4 + 6 files changed, 73 insertions(+), 106 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 2d186739d22..2b893eb0183 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -5,6 +5,7 @@ package deploy import ( "context" + "errors" "fmt" "io" "os" @@ -69,10 +70,6 @@ type stackDescriber interface { Resources() ([]*stack.Resource, error) } -type Warner interface { - Warning() string -} - type envDeployer struct { app *config.Application env *config.Environment @@ -154,7 +151,7 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { } // Validate returns an error if the environment manifest is incompatible with services and application configurations. -func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment) error { +func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment, output io.Writer) error { isManagedCDNEnabled := mft.CDNEnabled() && !mft.HasImportedPublicALBCerts() && d.app.Domain != "" if isManagedCDNEnabled { // With managed domain, if the customer isn't using `alias` the A-records are inserted in the service stack as each service domain is unique. @@ -166,12 +163,15 @@ func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment) e } if mft.CDNEnabled() && mft.CDNDoesTLSTermination() && mft.HasImportedPublicALBCerts() { - if err := d.validateALBWorkloadsDontRedirect(ctx); err != nil { - if warn, ok := err.(Warner); ok { - fmt.Println(warn.Warning()) - } else { - return fmt.Errorf("can't enable TLS termination on CDN: %w", err) - } + err := d.validateALBWorkloadsDontRedirect(ctx) + var redirErr *errEnvHasPublicServicesWithRedirect + switch { + case errors.As(err, &redirErr) && mft.ALBIngressRestrictedToCDN(): + return err + case errors.As(err, &redirErr): + fmt.Fprintf(output, redirErr.Warning()) + case err != nil: + return fmt.Errorf("can't enable TLS termination on CDN: %w", err) } } @@ -183,28 +183,38 @@ type errEnvHasPublicServicesWithRedirect struct { } func (e *errEnvHasPublicServicesWithRedirect) Error() string { + return e.message() +} + +func (e *errEnvHasPublicServicesWithRedirect) message() string { + n := len(e.services) quoted := make([]string, len(e.services)) for i := range e.services { quoted[i] = strconv.Quote(e.services[i]) } - return fmt.Sprintf("%s %s %s HTTP traffic to HTTPS", - english.PluralWord(len(e.services), "Service", "Services"), - english.OxfordWordSeries(quoted, "and"), - english.PluralWord(len(e.services), "redirects", "redirect")) -} -func (e *errEnvHasPublicServicesWithRedirect) Warning() string { - return fmt.Sprintf(`%s. + return fmt.Sprintf(`%s %s %s HTTP traffic to HTTPS. %s To fix this, set the following field in %s manifest: %s and run %s. -If you'd like to use %s without a CDN, ensure %s A record is pointed to the ALB. - `, e.Error(), - color.Emphasize(english.PluralWord(len(e.services), "This service", "These services")+" will not be reachable through the CDN."), - english.PluralWord(len(e.services), "its", "each"), + `, + english.PluralWord(n, "Service", "Services"), + english.OxfordWordSeries(quoted, "and"), + english.PluralWord(n, "redirects", "redirect"), + color.Emphasize(english.PluralWord(n, "This service", "These services")+" will not be reachable through the CDN."), + english.PluralWord(n, "its", "each"), color.HighlightCodeBlock("http:\n redirect_to_https: true"), color.HighlightCode("copilot svc deploy"), + ) + +} + +func (e *errEnvHasPublicServicesWithRedirect) Warning() string { + return fmt.Sprintf(`%s +If you'd like to use %s without a CDN, ensure %s A record is pointed to the ALB. +`, + e.message(), english.PluralWord(len(e.services), "this service", "these services"), english.PluralWord(len(e.services), "its", "each service's"), ) diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 4d63e3f19ac..9ac714f8b68 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -4,6 +4,7 @@ package deploy import ( + "bytes" "context" "errors" "fmt" @@ -414,11 +415,26 @@ func TestEnvDeployer_Validate(t *testing.T) { }, }, } + mftCDNTerminateTLS := &manifest.Environment{ + EnvironmentConfig: manifest.EnvironmentConfig{ + HTTPConfig: manifest.EnvironmentHTTPConfig{ + Public: manifest.PublicHTTPConfig{ + Certificates: []string{"mockCertARN"}, + }, + }, + CDNConfig: manifest.EnvironmentCDNConfig{ + Config: manifest.AdvancedCDNConfig{ + TerminateTLS: aws.Bool(true), + }, + }, + }, + } tests := map[string]struct { - app *config.Application - mft *manifest.Environment - setUpMocks func(*envDeployerMocks, *gomock.Controller) - expected string + app *config.Application + mft *manifest.Environment + setUpMocks func(*envDeployerMocks, *gomock.Controller) + expected string + expectedStdErr string }{ "cdn enabled, domain imported, no public http certs and validate aliases fails": { app: &config.Application{ @@ -453,15 +469,7 @@ func TestEnvDeployer_Validate(t *testing.T) { }, "cdn tls termination enabled, fail to get env stack params": { app: &config.Application{}, - mft: &manifest.Environment{ - EnvironmentConfig: manifest.EnvironmentConfig{ - CDNConfig: manifest.EnvironmentCDNConfig{ - Config: manifest.AdvancedCDNConfig{ - TerminateTLS: aws.Bool(true), - }, - }, - }, - }, + mft: mftCDNTerminateTLS, setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { m.envDescriber.EXPECT().Params().Return(nil, errors.New("some error")) }, @@ -469,15 +477,7 @@ func TestEnvDeployer_Validate(t *testing.T) { }, "cdn tls termination enabled, fail to get service resources": { app: &config.Application{}, - mft: &manifest.Environment{ - EnvironmentConfig: manifest.EnvironmentConfig{ - CDNConfig: manifest.EnvironmentCDNConfig{ - Config: manifest.AdvancedCDNConfig{ - TerminateTLS: aws.Bool(true), - }, - }, - }, - }, + mft: mftCDNTerminateTLS, setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { m.stackDescribers = map[string]*mocks.MockstackDescriber{ "svc1": mocks.NewMockstackDescriber(ctrl), @@ -492,15 +492,7 @@ func TestEnvDeployer_Validate(t *testing.T) { }, "cdn tls termination enabled, fail to check listener rule": { app: &config.Application{}, - mft: &manifest.Environment{ - EnvironmentConfig: manifest.EnvironmentConfig{ - CDNConfig: manifest.EnvironmentCDNConfig{ - Config: manifest.AdvancedCDNConfig{ - TerminateTLS: aws.Bool(true), - }, - }, - }, - }, + mft: mftCDNTerminateTLS, setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { m.stackDescribers = map[string]*mocks.MockstackDescriber{ "svc1": mocks.NewMockstackDescriber(ctrl), @@ -519,46 +511,9 @@ func TestEnvDeployer_Validate(t *testing.T) { }, expected: `can't enable TLS termination on CDN: verify service "svc1": get listener rule "svc1RuleARN": some error`, }, - "cdn tls termination enabled, fail with one service that redirects": { + "cdn tls termination enabled, warn with one service that doesn't redirect, two that do redirect": { app: &config.Application{}, - mft: &manifest.Environment{ - EnvironmentConfig: manifest.EnvironmentConfig{ - CDNConfig: manifest.EnvironmentCDNConfig{ - Config: manifest.AdvancedCDNConfig{ - TerminateTLS: aws.Bool(true), - }, - }, - }, - }, - setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { - m.stackDescribers = map[string]*mocks.MockstackDescriber{ - "svc1": mocks.NewMockstackDescriber(ctrl), - } - - m.envDescriber.EXPECT().Params().Return(map[string]string{ - "ALBWorkloads": "svc1", - }, nil) - m.stackDescribers["svc1"].EXPECT().Resources().Return([]*stack.Resource{ - { - LogicalID: "HTTPListenerRuleWithDomain", - PhysicalID: "svc1RuleARN", - }, - }, nil) - m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc1RuleARN").Return(listenerRuleWithRedirect, nil) - }, - expected: "can't enable TLS termination on CDN: HTTP traffic redirects to HTTPS in service svc1.\nSet http.redirect_to_https to false for that service and redeploy it.", - }, - "cdn tls termination enabled, fail with one service that doesn't redirects, two that do redirect": { - app: &config.Application{}, - mft: &manifest.Environment{ - EnvironmentConfig: manifest.EnvironmentConfig{ - CDNConfig: manifest.EnvironmentCDNConfig{ - Config: manifest.AdvancedCDNConfig{ - TerminateTLS: aws.Bool(true), - }, - }, - }, - }, + mft: mftCDNTerminateTLS, setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { m.stackDescribers = map[string]*mocks.MockstackDescriber{ "svc1": mocks.NewMockstackDescriber(ctrl), @@ -595,15 +550,7 @@ func TestEnvDeployer_Validate(t *testing.T) { }, "cdn tls termination enabled, success with three services that don't redirect": { app: &config.Application{}, - mft: &manifest.Environment{ - EnvironmentConfig: manifest.EnvironmentConfig{ - CDNConfig: manifest.EnvironmentCDNConfig{ - Config: manifest.AdvancedCDNConfig{ - TerminateTLS: aws.Bool(true), - }, - }, - }, - }, + mft: mftCDNTerminateTLS, setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { m.stackDescribers = map[string]*mocks.MockstackDescriber{ "svc1": mocks.NewMockstackDescriber(ctrl), @@ -664,12 +611,18 @@ func TestEnvDeployer_Validate(t *testing.T) { }, } - err := d.Validate(context.Background(), tc.mft) + buf := &bytes.Buffer{} + + err := d.Validate(context.Background(), tc.mft, buf) if tc.expected != "" { require.EqualError(t, err, tc.expected) } else { require.NoError(t, err) } + + if tc.expectedStdErr != "" { + require.Equal(t, tc.expectedStdErr, buf.String()) + } }) } } diff --git a/internal/pkg/cli/env_deploy.go b/internal/pkg/cli/env_deploy.go index 54e77f2029a..b8a40a15057 100644 --- a/internal/pkg/cli/env_deploy.go +++ b/internal/pkg/cli/env_deploy.go @@ -133,7 +133,7 @@ func (o *deployEnvOpts) Execute() error { if err != nil { return err } - if err := deployer.Validate(context.Background(), mft); err != nil { + if err := deployer.Validate(context.Background(), mft, os.Stderr); err != nil { return err } urls, err := deployer.UploadArtifacts() diff --git a/internal/pkg/cli/env_package.go b/internal/pkg/cli/env_package.go index 81610b783e3..7152dd48d2c 100644 --- a/internal/pkg/cli/env_package.go +++ b/internal/pkg/cli/env_package.go @@ -155,7 +155,7 @@ func (o *packageEnvOpts) Execute() error { if err != nil { return err } - if err := deployer.Validate(context.Background(), mft); err != nil { + if err := deployer.Validate(context.Background(), mft, os.Stderr); err != nil { return err } var urls map[string]string diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index b668a36a269..e6621070585 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -649,12 +649,12 @@ type runner interface { type envDeployer interface { DeployEnvironment(in *clideploy.DeployEnvironmentInput) error - Validate(ctx context.Context, mft *manifest.Environment) error + Validate(context.Context, *manifest.Environment, io.Writer) error UploadArtifacts() (map[string]string, error) } type envPackager interface { GenerateCloudFormationTemplate(in *clideploy.DeployEnvironmentInput) (*clideploy.GenerateCloudFormationTemplateOutput, error) - Validate(ctx context.Context, mft *manifest.Environment) error + Validate(context.Context, *manifest.Environment, io.Writer) error UploadArtifacts() (map[string]string, error) } diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 493111e661d..4be8ff51309 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -504,6 +504,10 @@ type Ingress struct { VPCIngress *bool `yaml:"from_vpc"` } +func (cfg *EnvironmentConfig) ALBIngressRestrictedToCDN() bool { + return aws.BoolValue(cfg.HTTPConfig.Public.SecurityGroupConfig.Ingress.RestrictiveIngress.CDNIngress) +} + // RestrictiveIngress represents ingress fields which restrict // default behavior of allowing all public ingress. type RestrictiveIngress struct { From 4086a653e7ff6d0b4d70fd0e337a7ea38db4c600 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Thu, 22 Sep 2022 14:31:28 -0700 Subject: [PATCH 22/35] error testing --- internal/pkg/cli/deploy/env.go | 6 +-- internal/pkg/cli/deploy/env_test.go | 76 ++++++++++++++++++++++++++++- 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 2b893eb0183..2fae76acbc9 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -197,8 +197,7 @@ func (e *errEnvHasPublicServicesWithRedirect) message() string { %s To fix this, set the following field in %s manifest: %s -and run %s. - `, +and run %s.`, english.PluralWord(n, "Service", "Services"), english.OxfordWordSeries(quoted, "and"), english.PluralWord(n, "redirects", "redirect"), @@ -212,8 +211,7 @@ and run %s. func (e *errEnvHasPublicServicesWithRedirect) Warning() string { return fmt.Sprintf(`%s -If you'd like to use %s without a CDN, ensure %s A record is pointed to the ALB. -`, +If you'd like to use %s without a CDN, ensure %s A record is pointed to the ALB.`, e.message(), english.PluralWord(len(e.services), "this service", "these services"), english.PluralWord(len(e.services), "its", "each service's"), diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 9ac714f8b68..2afd08b671d 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -546,7 +546,81 @@ func TestEnvDeployer_Validate(t *testing.T) { m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc2RuleARN").Return(listenerRuleNoRedirect, nil) m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc3RuleARN").Return(listenerRuleWithRedirect, nil) }, - expected: "can't enable TLS termination on CDN: HTTP traffic redirects to HTTPS in services svc1 and svc3.\nSet http.redirect_to_https to false for those services and redeploy them.", + expectedStdErr: fmt.Sprintf(`Services "svc1" and "svc3" redirect HTTP traffic to HTTPS. +These services will not be reachable through the CDN. +To fix this, set the following field in each manifest: +%s +http: + redirect_to_https: true +%s +and run %scopilot svc deploy%s. +If you'd like to use these services without a CDN, ensure each service's A record is pointed to the ALB.`, + "```", "```", "`", "`"), // ugh + }, + "cdn tls termination enabled, warn with one service that doesn't redirect, two that do redirect, alb ingress restricted to cdn": { + app: &config.Application{}, + mft: &manifest.Environment{ + EnvironmentConfig: manifest.EnvironmentConfig{ + HTTPConfig: manifest.EnvironmentHTTPConfig{ + Public: manifest.PublicHTTPConfig{ + Certificates: []string{"mockCertARN"}, + SecurityGroupConfig: manifest.ALBSecurityGroupsConfig{ + Ingress: manifest.Ingress{ + RestrictiveIngress: manifest.RestrictiveIngress{ + CDNIngress: aws.Bool(true), + }, + }, + }, + }, + }, + CDNConfig: manifest.EnvironmentCDNConfig{ + Config: manifest.AdvancedCDNConfig{ + TerminateTLS: aws.Bool(true), + }, + }, + }, + }, + setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { + m.stackDescribers = map[string]*mocks.MockstackDescriber{ + "svc1": mocks.NewMockstackDescriber(ctrl), + "svc2": mocks.NewMockstackDescriber(ctrl), + "svc3": mocks.NewMockstackDescriber(ctrl), + } + + m.envDescriber.EXPECT().Params().Return(map[string]string{ + "ALBWorkloads": "svc1,svc2,svc3", + }, nil) + m.stackDescribers["svc1"].EXPECT().Resources().Return([]*stack.Resource{ + { + LogicalID: "HTTPListenerRuleWithDomain", + PhysicalID: "svc1RuleARN", + }, + }, nil) + m.stackDescribers["svc2"].EXPECT().Resources().Return([]*stack.Resource{ + { + LogicalID: "HTTPListenerRuleWithDomain", + PhysicalID: "svc2RuleARN", + }, + }, nil) + m.stackDescribers["svc3"].EXPECT().Resources().Return([]*stack.Resource{ + { + LogicalID: "HTTPListenerRuleWithDomain", + PhysicalID: "svc3RuleARN", + }, + }, nil) + m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc1RuleARN").Return(listenerRuleWithRedirect, nil) + m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc2RuleARN").Return(listenerRuleNoRedirect, nil) + m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc3RuleARN").Return(listenerRuleWithRedirect, nil) + }, + expected: fmt.Sprintf(`Services "svc1" and "svc3" redirect HTTP traffic to HTTPS. +These services will not be reachable through the CDN. +To fix this, set the following field in each manifest: +%s +http: + redirect_to_https: true +%s +and run %scopilot svc deploy%s.`, + "```", "```", "`", "`"), }, "cdn tls termination enabled, success with three services that don't redirect": { app: &config.Application{}, From 9ac84d31392ab27b69d996c2659422fdf5d5a307 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Thu, 22 Sep 2022 14:32:35 -0700 Subject: [PATCH 23/35] delete extra func --- internal/pkg/cli/deploy/env.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 2fae76acbc9..d62cd8b9f46 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -183,10 +183,6 @@ type errEnvHasPublicServicesWithRedirect struct { } func (e *errEnvHasPublicServicesWithRedirect) Error() string { - return e.message() -} - -func (e *errEnvHasPublicServicesWithRedirect) message() string { n := len(e.services) quoted := make([]string, len(e.services)) for i := range e.services { @@ -206,13 +202,12 @@ and run %s.`, color.HighlightCodeBlock("http:\n redirect_to_https: true"), color.HighlightCode("copilot svc deploy"), ) - } func (e *errEnvHasPublicServicesWithRedirect) Warning() string { return fmt.Sprintf(`%s If you'd like to use %s without a CDN, ensure %s A record is pointed to the ALB.`, - e.message(), + e.Error(), english.PluralWord(len(e.services), "this service", "these services"), english.PluralWord(len(e.services), "its", "each service's"), ) From 556fc9667625019b7a1c0b643fc7486a0895ea71 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Thu, 22 Sep 2022 14:59:07 -0700 Subject: [PATCH 24/35] fix interfaces --- internal/pkg/cli/deploy/env.go | 2 +- internal/pkg/cli/env_deploy_test.go | 8 ++++---- internal/pkg/cli/env_package_test.go | 8 ++++---- internal/pkg/cli/mocks/mock_interfaces.go | 16 ++++++++-------- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index d62cd8b9f46..a2aa513f3ba 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -162,7 +162,7 @@ func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment, o } } - if mft.CDNEnabled() && mft.CDNDoesTLSTermination() && mft.HasImportedPublicALBCerts() { + if mft.CDNEnabled() && mft.CDNDoesTLSTermination() && (mft.HasImportedPublicALBCerts() || d.app.Domain != "") { err := d.validateALBWorkloadsDontRedirect(ctx) var redirErr *errEnvHasPublicServicesWithRedirect switch { diff --git a/internal/pkg/cli/env_deploy_test.go b/internal/pkg/cli/env_deploy_test.go index 4eed9cf71fb..44a9f02197a 100644 --- a/internal/pkg/cli/env_deploy_test.go +++ b/internal/pkg/cli/env_deploy_test.go @@ -185,7 +185,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(errors.New("mock error")) + m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("mock error")) }, wantedErr: errors.New("mock error"), }, @@ -196,7 +196,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(nil, errors.New("some error")) }, wantedErr: errors.New("upload artifacts for environment mockEnv: some error"), @@ -208,7 +208,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(map[string]string{ "mockResource": "mockURL", }, nil) @@ -225,7 +225,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(map[string]string{ "mockResource": "mockURL", }, nil) diff --git a/internal/pkg/cli/env_package_test.go b/internal/pkg/cli/env_package_test.go index 5f2bbb47120..0305325c74c 100644 --- a/internal/pkg/cli/env_package_test.go +++ b/internal/pkg/cli/env_package_test.go @@ -204,7 +204,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(errors.New("mock error")) + deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("mock error")) return &packageEnvOpts{ packageEnvVars: packageEnvVars{ @@ -232,7 +232,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) deployer.EXPECT().UploadArtifacts().Return(nil, errors.New("some error")) return &packageEnvOpts{ @@ -262,7 +262,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) deployer.EXPECT().GenerateCloudFormationTemplate(gomock.Any()).Return(nil, errors.New("some error")) return &packageEnvOpts{ @@ -292,7 +292,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) deployer.EXPECT().GenerateCloudFormationTemplate(&deploy.DeployEnvironmentInput{ RootUserARN: "", CustomResourcesURLs: nil, diff --git a/internal/pkg/cli/mocks/mock_interfaces.go b/internal/pkg/cli/mocks/mock_interfaces.go index 3273262f737..2c1e906486d 100644 --- a/internal/pkg/cli/mocks/mock_interfaces.go +++ b/internal/pkg/cli/mocks/mock_interfaces.go @@ -6849,17 +6849,17 @@ func (mr *MockenvDeployerMockRecorder) UploadArtifacts() *gomock.Call { } // Validate mocks base method. -func (m *MockenvDeployer) Validate(ctx context.Context, mft *manifest.Environment) error { +func (m *MockenvDeployer) Validate(arg0 context.Context, arg1 *manifest.Environment, arg2 io.Writer) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Validate", ctx, mft) + ret := m.ctrl.Call(m, "Validate", arg0, arg1, arg2) ret0, _ := ret[0].(error) return ret0 } // Validate indicates an expected call of Validate. -func (mr *MockenvDeployerMockRecorder) Validate(ctx, mft interface{}) *gomock.Call { +func (mr *MockenvDeployerMockRecorder) Validate(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvDeployer)(nil).Validate), ctx, mft) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvDeployer)(nil).Validate), arg0, arg1, arg2) } // MockenvPackager is a mock of envPackager interface. @@ -6916,15 +6916,15 @@ func (mr *MockenvPackagerMockRecorder) UploadArtifacts() *gomock.Call { } // Validate mocks base method. -func (m *MockenvPackager) Validate(ctx context.Context, mft *manifest.Environment) error { +func (m *MockenvPackager) Validate(arg0 context.Context, arg1 *manifest.Environment, arg2 io.Writer) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Validate", ctx, mft) + ret := m.ctrl.Call(m, "Validate", arg0, arg1, arg2) ret0, _ := ret[0].(error) return ret0 } // Validate indicates an expected call of Validate. -func (mr *MockenvPackagerMockRecorder) Validate(ctx, mft interface{}) *gomock.Call { +func (mr *MockenvPackagerMockRecorder) Validate(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvPackager)(nil).Validate), ctx, mft) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvPackager)(nil).Validate), arg0, arg1, arg2) } From abfc83265d24822d18d2b5b65eed25a2132acb5b Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Thu, 22 Sep 2022 15:05:15 -0700 Subject: [PATCH 25/35] fix elbv2 tests --- internal/pkg/aws/elbv2/elbv2_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/aws/elbv2/elbv2_test.go b/internal/pkg/aws/elbv2/elbv2_test.go index 88d612384fd..45220b92195 100644 --- a/internal/pkg/aws/elbv2/elbv2_test.go +++ b/internal/pkg/aws/elbv2/elbv2_test.go @@ -234,7 +234,7 @@ func TestELBV2_DescribeRule(t *testing.T) { RuleArns: aws.StringSlice([]string{mockARN}), }).Return(nil, errors.New("some error")) }, - expectedErr: "get listener rule for mockListenerRuleARN: some error", + expectedErr: "some error", }, "cannot find listener rule": { setUpMock: func(m *mocks.Mockapi) { @@ -242,7 +242,7 @@ func TestELBV2_DescribeRule(t *testing.T) { RuleArns: aws.StringSlice([]string{mockARN}), }).Return(&elbv2.DescribeRulesOutput{}, nil) }, - expectedErr: `listener rule "mockListenerRuleARN" not found`, + expectedErr: `not found`, }, "success": { setUpMock: func(m *mocks.Mockapi) { From 90a2126e36f9fe058492e0fc4d2beff675b9f4f1 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Thu, 22 Sep 2022 15:12:51 -0700 Subject: [PATCH 26/35] fix lint --- internal/pkg/cli/deploy/env.go | 2 +- internal/pkg/cli/deploy/env_test.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index a2aa513f3ba..765d2ebf3c2 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -169,7 +169,7 @@ func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment, o case errors.As(err, &redirErr) && mft.ALBIngressRestrictedToCDN(): return err case errors.As(err, &redirErr): - fmt.Fprintf(output, redirErr.Warning()) + fmt.Fprintln(output, redirErr.Warning()) case err != nil: return fmt.Errorf("can't enable TLS termination on CDN: %w", err) } diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 2afd08b671d..7387611a592 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -554,7 +554,8 @@ http: redirect_to_https: true %s and run %scopilot svc deploy%s. -If you'd like to use these services without a CDN, ensure each service's A record is pointed to the ALB.`, +If you'd like to use these services without a CDN, ensure each service's A record is pointed to the ALB. +`, "```", "```", "`", "`"), // ugh }, "cdn tls termination enabled, warn with one service that doesn't redirect, two that do redirect, alb ingress restricted to cdn": { From 5fc137a382a44404b828a510fa11cc9f16859162 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Thu, 22 Sep 2022 15:20:36 -0700 Subject: [PATCH 27/35] typo --- internal/pkg/template/workload.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/template/workload.go b/internal/pkg/template/workload.go index 79657f666dd..f85c838f79d 100644 --- a/internal/pkg/template/workload.go +++ b/internal/pkg/template/workload.go @@ -62,7 +62,7 @@ const ( snsARNPattern = "arn:%s:sns:%s:%s:%s-%s-%s-%s" ) -// Constants for stack resource logical ID's +// Constants for stack resource logical IDs const ( LogicalIDHTTPListenerRuleWithDomain = "HTTPListenerRuleWithDomain" ) From 544159b845bed10063052570f0720e79a12a2395 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Fri, 23 Sep 2022 10:04:42 -0700 Subject: [PATCH 28/35] use `elbv2.DescribeRulesWithContext` --- internal/pkg/aws/elbv2/elbv2.go | 8 +++-- internal/pkg/aws/elbv2/elbv2_test.go | 6 ++-- internal/pkg/aws/elbv2/mocks/mock_elbv2.go | 38 +++++++++++++++++----- 3 files changed, 38 insertions(+), 14 deletions(-) diff --git a/internal/pkg/aws/elbv2/elbv2.go b/internal/pkg/aws/elbv2/elbv2.go index 6092ae1c60c..0a622db4a8b 100644 --- a/internal/pkg/aws/elbv2/elbv2.go +++ b/internal/pkg/aws/elbv2/elbv2.go @@ -11,6 +11,7 @@ import ( "sort" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/request" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/elbv2" ) @@ -21,8 +22,9 @@ const ( ) type api interface { - DescribeTargetHealth(input *elbv2.DescribeTargetHealthInput) (*elbv2.DescribeTargetHealthOutput, error) - DescribeRules(input *elbv2.DescribeRulesInput) (*elbv2.DescribeRulesOutput, error) + DescribeTargetHealth(*elbv2.DescribeTargetHealthInput) (*elbv2.DescribeTargetHealthOutput, error) + DescribeRules(*elbv2.DescribeRulesInput) (*elbv2.DescribeRulesOutput, error) + DescribeRulesWithContext(context.Context, *elbv2.DescribeRulesInput, ...request.Option) (*elbv2.DescribeRulesOutput, error) } // ELBV2 wraps an AWS ELBV2 client. @@ -79,7 +81,7 @@ type Rule elbv2.Rule // DescribeRule returns the Rule with ruleARN. func (e *ELBV2) DescribeRule(ctx context.Context, ruleARN string) (Rule, error) { - resp, err := e.client.DescribeRules(&elbv2.DescribeRulesInput{ + resp, err := e.client.DescribeRulesWithContext(ctx, &elbv2.DescribeRulesInput{ RuleArns: aws.StringSlice([]string{ruleARN}), }) switch { diff --git a/internal/pkg/aws/elbv2/elbv2_test.go b/internal/pkg/aws/elbv2/elbv2_test.go index 45220b92195..2686e05cfac 100644 --- a/internal/pkg/aws/elbv2/elbv2_test.go +++ b/internal/pkg/aws/elbv2/elbv2_test.go @@ -230,7 +230,7 @@ func TestELBV2_DescribeRule(t *testing.T) { }{ "fail to describe rules": { setUpMock: func(m *mocks.Mockapi) { - m.EXPECT().DescribeRules(&elbv2.DescribeRulesInput{ + m.EXPECT().DescribeRulesWithContext(gomock.Any(), &elbv2.DescribeRulesInput{ RuleArns: aws.StringSlice([]string{mockARN}), }).Return(nil, errors.New("some error")) }, @@ -238,7 +238,7 @@ func TestELBV2_DescribeRule(t *testing.T) { }, "cannot find listener rule": { setUpMock: func(m *mocks.Mockapi) { - m.EXPECT().DescribeRules(&elbv2.DescribeRulesInput{ + m.EXPECT().DescribeRulesWithContext(gomock.Any(), &elbv2.DescribeRulesInput{ RuleArns: aws.StringSlice([]string{mockARN}), }).Return(&elbv2.DescribeRulesOutput{}, nil) }, @@ -246,7 +246,7 @@ func TestELBV2_DescribeRule(t *testing.T) { }, "success": { setUpMock: func(m *mocks.Mockapi) { - m.EXPECT().DescribeRules(&elbv2.DescribeRulesInput{ + m.EXPECT().DescribeRulesWithContext(gomock.Any(), &elbv2.DescribeRulesInput{ RuleArns: aws.StringSlice([]string{mockARN}), }).Return(&elbv2.DescribeRulesOutput{ Rules: []*elbv2.Rule{ diff --git a/internal/pkg/aws/elbv2/mocks/mock_elbv2.go b/internal/pkg/aws/elbv2/mocks/mock_elbv2.go index 9246dbfa9e4..43b238ff8f9 100644 --- a/internal/pkg/aws/elbv2/mocks/mock_elbv2.go +++ b/internal/pkg/aws/elbv2/mocks/mock_elbv2.go @@ -5,8 +5,10 @@ package mocks import ( + context "context" reflect "reflect" + request "github.com/aws/aws-sdk-go/aws/request" elbv2 "github.com/aws/aws-sdk-go/service/elbv2" gomock "github.com/golang/mock/gomock" ) @@ -35,31 +37,51 @@ func (m *Mockapi) EXPECT() *MockapiMockRecorder { } // DescribeRules mocks base method. -func (m *Mockapi) DescribeRules(input *elbv2.DescribeRulesInput) (*elbv2.DescribeRulesOutput, error) { +func (m *Mockapi) DescribeRules(arg0 *elbv2.DescribeRulesInput) (*elbv2.DescribeRulesOutput, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DescribeRules", input) + ret := m.ctrl.Call(m, "DescribeRules", arg0) ret0, _ := ret[0].(*elbv2.DescribeRulesOutput) ret1, _ := ret[1].(error) return ret0, ret1 } // DescribeRules indicates an expected call of DescribeRules. -func (mr *MockapiMockRecorder) DescribeRules(input interface{}) *gomock.Call { +func (mr *MockapiMockRecorder) DescribeRules(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeRules", reflect.TypeOf((*Mockapi)(nil).DescribeRules), input) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeRules", reflect.TypeOf((*Mockapi)(nil).DescribeRules), arg0) +} + +// DescribeRulesWithContext mocks base method. +func (m *Mockapi) DescribeRulesWithContext(arg0 context.Context, arg1 *elbv2.DescribeRulesInput, arg2 ...request.Option) (*elbv2.DescribeRulesOutput, error) { + m.ctrl.T.Helper() + varargs := []interface{}{arg0, arg1} + for _, a := range arg2 { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "DescribeRulesWithContext", varargs...) + ret0, _ := ret[0].(*elbv2.DescribeRulesOutput) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// DescribeRulesWithContext indicates an expected call of DescribeRulesWithContext. +func (mr *MockapiMockRecorder) DescribeRulesWithContext(arg0, arg1 interface{}, arg2 ...interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]interface{}{arg0, arg1}, arg2...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeRulesWithContext", reflect.TypeOf((*Mockapi)(nil).DescribeRulesWithContext), varargs...) } // DescribeTargetHealth mocks base method. -func (m *Mockapi) DescribeTargetHealth(input *elbv2.DescribeTargetHealthInput) (*elbv2.DescribeTargetHealthOutput, error) { +func (m *Mockapi) DescribeTargetHealth(arg0 *elbv2.DescribeTargetHealthInput) (*elbv2.DescribeTargetHealthOutput, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DescribeTargetHealth", input) + ret := m.ctrl.Call(m, "DescribeTargetHealth", arg0) ret0, _ := ret[0].(*elbv2.DescribeTargetHealthOutput) ret1, _ := ret[1].(error) return ret0, ret1 } // DescribeTargetHealth indicates an expected call of DescribeTargetHealth. -func (mr *MockapiMockRecorder) DescribeTargetHealth(input interface{}) *gomock.Call { +func (mr *MockapiMockRecorder) DescribeTargetHealth(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeTargetHealth", reflect.TypeOf((*Mockapi)(nil).DescribeTargetHealth), input) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeTargetHealth", reflect.TypeOf((*Mockapi)(nil).DescribeTargetHealth), arg0) } From 8c94bf2fb1bb0d6eccd9141abec3014881f20dde Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Fri, 23 Sep 2022 10:27:49 -0700 Subject: [PATCH 29/35] address some feedback --- internal/pkg/aws/elbv2/elbv2.go | 1 + internal/pkg/cli/deploy/env.go | 42 ++--------------------------- internal/pkg/cli/deploy/env_test.go | 2 +- internal/pkg/cli/deploy/errors.go | 37 +++++++++++++++++++++++++ 4 files changed, 41 insertions(+), 41 deletions(-) diff --git a/internal/pkg/aws/elbv2/elbv2.go b/internal/pkg/aws/elbv2/elbv2.go index 0a622db4a8b..012ec9a6cf0 100644 --- a/internal/pkg/aws/elbv2/elbv2.go +++ b/internal/pkg/aws/elbv2/elbv2.go @@ -77,6 +77,7 @@ func (e *ELBV2) ListenerRuleHostHeaders(ruleARN string) ([]string, error) { return hostHeaders, nil } +// Rule wraps an elbv2.Rule to add some nice functionality to it. type Rule elbv2.Rule // DescribeRule returns the Rule with ruleARN. diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 765d2ebf3c2..17656327a27 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -10,7 +10,6 @@ import ( "io" "os" "sort" - "strconv" "strings" "sync" @@ -32,10 +31,8 @@ import ( "github.com/aws/copilot-cli/internal/pkg/describe/stack" "github.com/aws/copilot-cli/internal/pkg/manifest" "github.com/aws/copilot-cli/internal/pkg/template" - "github.com/aws/copilot-cli/internal/pkg/term/color" "github.com/aws/copilot-cli/internal/pkg/term/log" termprogress "github.com/aws/copilot-cli/internal/pkg/term/progress" - "github.com/dustin/go-humanize/english" "golang.org/x/sync/errgroup" ) @@ -120,7 +117,7 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { ConfigStore: in.ConfigStore, }) if err != nil { - return nil, fmt.Errorf("get env describer: %w", err) + return nil, fmt.Errorf("initialize env describer: %w", err) } cfnClient := deploycfn.New(envManagerSession, deploycfn.WithProgressTracker(os.Stderr)) return &envDeployer{ @@ -178,41 +175,6 @@ func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment, o return nil } -type errEnvHasPublicServicesWithRedirect struct { - services []string -} - -func (e *errEnvHasPublicServicesWithRedirect) Error() string { - n := len(e.services) - quoted := make([]string, len(e.services)) - for i := range e.services { - quoted[i] = strconv.Quote(e.services[i]) - } - - return fmt.Sprintf(`%s %s %s HTTP traffic to HTTPS. -%s -To fix this, set the following field in %s manifest: -%s -and run %s.`, - english.PluralWord(n, "Service", "Services"), - english.OxfordWordSeries(quoted, "and"), - english.PluralWord(n, "redirects", "redirect"), - color.Emphasize(english.PluralWord(n, "This service", "These services")+" will not be reachable through the CDN."), - english.PluralWord(n, "its", "each"), - color.HighlightCodeBlock("http:\n redirect_to_https: true"), - color.HighlightCode("copilot svc deploy"), - ) -} - -func (e *errEnvHasPublicServicesWithRedirect) Warning() string { - return fmt.Sprintf(`%s -If you'd like to use %s without a CDN, ensure %s A record is pointed to the ALB.`, - e.Error(), - english.PluralWord(len(e.services), "this service", "these services"), - english.PluralWord(len(e.services), "its", "each service's"), - ) -} - // validateALBWorkloadsDontRedirect verifies that none of the public ALB Workloads // in this environment have a redirect in their HTTPWithDomain listener. // If any services redirect, an error is returned. @@ -282,7 +244,7 @@ func (d *envDeployer) lbServiceRedirects(ctx context.Context, svc string) (bool, rule, err := d.lbDescriber.DescribeRule(ctx, ruleARN) if err != nil { - return false, fmt.Errorf("get listener rule %q: %w", ruleARN, err) + return false, fmt.Errorf("describe listener rule %q: %w", ruleARN, err) } return rule.HasRedirectAction(), nil } diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 7387611a592..a66596ee7d0 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -509,7 +509,7 @@ func TestEnvDeployer_Validate(t *testing.T) { }, nil) m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc1RuleARN").Return(elbv2.Rule{}, errors.New("some error")) }, - expected: `can't enable TLS termination on CDN: verify service "svc1": get listener rule "svc1RuleARN": some error`, + expected: `can't enable TLS termination on CDN: verify service "svc1": describe listener rule "svc1RuleARN": some error`, }, "cdn tls termination enabled, warn with one service that doesn't redirect, two that do redirect": { app: &config.Application{}, diff --git a/internal/pkg/cli/deploy/errors.go b/internal/pkg/cli/deploy/errors.go index a3911288f6c..73a03c6fe40 100644 --- a/internal/pkg/cli/deploy/errors.go +++ b/internal/pkg/cli/deploy/errors.go @@ -5,9 +5,11 @@ package deploy import ( "fmt" + "strconv" "strings" "github.com/aws/copilot-cli/internal/pkg/term/color" + "github.com/dustin/go-humanize/english" ) type errSvcWithNoALBAliasDeployingToEnvWithImportedCerts struct { @@ -38,3 +40,38 @@ func (e *errSvcWithALBAliasHostedZoneWithCDNEnabled) RecommendActions() string { } return strings.Join(msgs, "\n") } + +type errEnvHasPublicServicesWithRedirect struct { + services []string +} + +func (e *errEnvHasPublicServicesWithRedirect) Error() string { + n := len(e.services) + quoted := make([]string, len(e.services)) + for i := range e.services { + quoted[i] = strconv.Quote(e.services[i]) + } + + return fmt.Sprintf(`%s %s %s HTTP traffic to HTTPS. +%s +To fix this, set the following field in %s manifest: +%s +and run %s.`, + english.PluralWord(n, "Service", "Services"), + english.OxfordWordSeries(quoted, "and"), + english.PluralWord(n, "redirects", "redirect"), + color.Emphasize(english.PluralWord(n, "This service", "These services")+" will not be reachable through the CDN."), + english.PluralWord(n, "its", "each"), + color.HighlightCodeBlock("http:\n redirect_to_https: true"), + color.HighlightCode("copilot svc deploy"), + ) +} + +func (e *errEnvHasPublicServicesWithRedirect) Warning() string { + return fmt.Sprintf(`%s +If you'd like to use %s without a CDN, ensure %s A record is pointed to the ALB.`, + e.Error(), + english.PluralWord(len(e.services), "this service", "these services"), + english.PluralWord(len(e.services), "its", "each service's"), + ) +} From d63d204289a26e1cebed1b3edd6ae7e914bc63c6 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Fri, 23 Sep 2022 10:31:39 -0700 Subject: [PATCH 30/35] =?UTF-8?q?bye=20friend=20=F0=9F=AB=A1?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/pkg/aws/elbv2/elbv2.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/pkg/aws/elbv2/elbv2.go b/internal/pkg/aws/elbv2/elbv2.go index 012ec9a6cf0..8eb1bfc90de 100644 --- a/internal/pkg/aws/elbv2/elbv2.go +++ b/internal/pkg/aws/elbv2/elbv2.go @@ -85,10 +85,9 @@ func (e *ELBV2) DescribeRule(ctx context.Context, ruleARN string) (Rule, error) resp, err := e.client.DescribeRulesWithContext(ctx, &elbv2.DescribeRulesInput{ RuleArns: aws.StringSlice([]string{ruleARN}), }) - switch { - case err != nil: + if err != nil { return Rule{}, err - case len(resp.Rules) == 0: + } else if len(resp.Rules) == 0 { return Rule{}, errors.New("not found") } From a9e4c330e84e7bc950125c82ce3f11c677ecbc3c Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Fri, 23 Sep 2022 10:41:13 -0700 Subject: [PATCH 31/35] it was fun while it lasted --- internal/pkg/cli/deploy/env.go | 12 ++++++++---- internal/pkg/cli/deploy/env_test.go | 3 +-- internal/pkg/cli/env_deploy.go | 3 +-- internal/pkg/cli/env_deploy_test.go | 8 ++++---- internal/pkg/cli/env_package.go | 3 +-- internal/pkg/cli/env_package_test.go | 8 ++++---- internal/pkg/cli/interfaces.go | 5 ++--- internal/pkg/cli/mocks/mock_interfaces.go | 17 ++++++++--------- 8 files changed, 29 insertions(+), 30 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 17656327a27..26f7bb19a7b 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -148,7 +148,11 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { } // Validate returns an error if the environment manifest is incompatible with services and application configurations. -func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment, output io.Writer) error { +func (d *envDeployer) Validate(mft *manifest.Environment, output io.Writer) error { + return d.validateCDN(mft, output) +} + +func (d *envDeployer) validateCDN(mft *manifest.Environment, output io.Writer) error { isManagedCDNEnabled := mft.CDNEnabled() && !mft.HasImportedPublicALBCerts() && d.app.Domain != "" if isManagedCDNEnabled { // With managed domain, if the customer isn't using `alias` the A-records are inserted in the service stack as each service domain is unique. @@ -160,7 +164,7 @@ func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment, o } if mft.CDNEnabled() && mft.CDNDoesTLSTermination() && (mft.HasImportedPublicALBCerts() || d.app.Domain != "") { - err := d.validateALBWorkloadsDontRedirect(ctx) + err := d.validateALBWorkloadsDontRedirect() var redirErr *errEnvHasPublicServicesWithRedirect switch { case errors.As(err, &redirErr) && mft.ALBIngressRestrictedToCDN(): @@ -178,14 +182,14 @@ func (d *envDeployer) Validate(ctx context.Context, mft *manifest.Environment, o // validateALBWorkloadsDontRedirect verifies that none of the public ALB Workloads // in this environment have a redirect in their HTTPWithDomain listener. // If any services redirect, an error is returned. -func (d *envDeployer) validateALBWorkloadsDontRedirect(ctx context.Context) error { +func (d *envDeployer) validateALBWorkloadsDontRedirect() error { params, err := d.envDescriber.Params() if err != nil { return fmt.Errorf("get env params: %w", err) } services := strings.Split(params[cfnstack.EnvParamALBWorkloadsKey], ",") - g, ctx := errgroup.WithContext(ctx) + g, ctx := errgroup.WithContext(context.Background()) var badServices []string var badServicesMu sync.Mutex diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index a66596ee7d0..fa08ced235e 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -5,7 +5,6 @@ package deploy import ( "bytes" - "context" "errors" "fmt" "io" @@ -688,7 +687,7 @@ and run %scopilot svc deploy%s.`, buf := &bytes.Buffer{} - err := d.Validate(context.Background(), tc.mft, buf) + err := d.Validate(tc.mft, buf) if tc.expected != "" { require.EqualError(t, err, tc.expected) } else { diff --git a/internal/pkg/cli/env_deploy.go b/internal/pkg/cli/env_deploy.go index b8a40a15057..ac712fe16a9 100644 --- a/internal/pkg/cli/env_deploy.go +++ b/internal/pkg/cli/env_deploy.go @@ -4,7 +4,6 @@ package cli import ( - "context" "errors" "fmt" "os" @@ -133,7 +132,7 @@ func (o *deployEnvOpts) Execute() error { if err != nil { return err } - if err := deployer.Validate(context.Background(), mft, os.Stderr); err != nil { + if err := deployer.Validate(mft, os.Stderr); err != nil { return err } urls, err := deployer.UploadArtifacts() diff --git a/internal/pkg/cli/env_deploy_test.go b/internal/pkg/cli/env_deploy_test.go index 44a9f02197a..4eed9cf71fb 100644 --- a/internal/pkg/cli/env_deploy_test.go +++ b/internal/pkg/cli/env_deploy_test.go @@ -185,7 +185,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("mock error")) + m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(errors.New("mock error")) }, wantedErr: errors.New("mock error"), }, @@ -196,7 +196,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(nil, errors.New("some error")) }, wantedErr: errors.New("upload artifacts for environment mockEnv: some error"), @@ -208,7 +208,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(map[string]string{ "mockResource": "mockURL", }, nil) @@ -225,7 +225,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(map[string]string{ "mockResource": "mockURL", }, nil) diff --git a/internal/pkg/cli/env_package.go b/internal/pkg/cli/env_package.go index 7152dd48d2c..7c549bbfa0b 100644 --- a/internal/pkg/cli/env_package.go +++ b/internal/pkg/cli/env_package.go @@ -4,7 +4,6 @@ package cli import ( - "context" "fmt" "io" "os" @@ -155,7 +154,7 @@ func (o *packageEnvOpts) Execute() error { if err != nil { return err } - if err := deployer.Validate(context.Background(), mft, os.Stderr); err != nil { + if err := deployer.Validate(mft, os.Stderr); err != nil { return err } var urls map[string]string diff --git a/internal/pkg/cli/env_package_test.go b/internal/pkg/cli/env_package_test.go index 0305325c74c..5f2bbb47120 100644 --- a/internal/pkg/cli/env_package_test.go +++ b/internal/pkg/cli/env_package_test.go @@ -204,7 +204,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("mock error")) + deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(errors.New("mock error")) return &packageEnvOpts{ packageEnvVars: packageEnvVars{ @@ -232,7 +232,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) deployer.EXPECT().UploadArtifacts().Return(nil, errors.New("some error")) return &packageEnvOpts{ @@ -262,7 +262,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) deployer.EXPECT().GenerateCloudFormationTemplate(gomock.Any()).Return(nil, errors.New("some error")) return &packageEnvOpts{ @@ -292,7 +292,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) deployer.EXPECT().GenerateCloudFormationTemplate(&deploy.DeployEnvironmentInput{ RootUserARN: "", CustomResourcesURLs: nil, diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index e6621070585..82beafb2ff2 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -4,7 +4,6 @@ package cli import ( - "context" "encoding" "io" @@ -649,12 +648,12 @@ type runner interface { type envDeployer interface { DeployEnvironment(in *clideploy.DeployEnvironmentInput) error - Validate(context.Context, *manifest.Environment, io.Writer) error + Validate(*manifest.Environment, io.Writer) error UploadArtifacts() (map[string]string, error) } type envPackager interface { GenerateCloudFormationTemplate(in *clideploy.DeployEnvironmentInput) (*clideploy.GenerateCloudFormationTemplateOutput, error) - Validate(context.Context, *manifest.Environment, io.Writer) error + Validate(*manifest.Environment, io.Writer) error UploadArtifacts() (map[string]string, error) } diff --git a/internal/pkg/cli/mocks/mock_interfaces.go b/internal/pkg/cli/mocks/mock_interfaces.go index 2c1e906486d..c8acc04f08e 100644 --- a/internal/pkg/cli/mocks/mock_interfaces.go +++ b/internal/pkg/cli/mocks/mock_interfaces.go @@ -5,7 +5,6 @@ package mocks import ( - context "context" encoding "encoding" io "io" reflect "reflect" @@ -6849,17 +6848,17 @@ func (mr *MockenvDeployerMockRecorder) UploadArtifacts() *gomock.Call { } // Validate mocks base method. -func (m *MockenvDeployer) Validate(arg0 context.Context, arg1 *manifest.Environment, arg2 io.Writer) error { +func (m *MockenvDeployer) Validate(arg0 *manifest.Environment, arg1 io.Writer) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Validate", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "Validate", arg0, arg1) ret0, _ := ret[0].(error) return ret0 } // Validate indicates an expected call of Validate. -func (mr *MockenvDeployerMockRecorder) Validate(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockenvDeployerMockRecorder) Validate(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvDeployer)(nil).Validate), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvDeployer)(nil).Validate), arg0, arg1) } // MockenvPackager is a mock of envPackager interface. @@ -6916,15 +6915,15 @@ func (mr *MockenvPackagerMockRecorder) UploadArtifacts() *gomock.Call { } // Validate mocks base method. -func (m *MockenvPackager) Validate(arg0 context.Context, arg1 *manifest.Environment, arg2 io.Writer) error { +func (m *MockenvPackager) Validate(arg0 *manifest.Environment, arg1 io.Writer) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Validate", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "Validate", arg0, arg1) ret0, _ := ret[0].(error) return ret0 } // Validate indicates an expected call of Validate. -func (mr *MockenvPackagerMockRecorder) Validate(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockenvPackagerMockRecorder) Validate(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvPackager)(nil).Validate), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvPackager)(nil).Validate), arg0, arg1) } From dc8ba3d97b165f83733baf9583f79ed3703852c1 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Mon, 26 Sep 2022 13:59:05 -0700 Subject: [PATCH 32/35] address some fb --- internal/pkg/cli/deploy/env.go | 17 +++++++---------- internal/pkg/cli/deploy/env_test.go | 4 +++- internal/pkg/cli/deploy/errors.go | 2 +- internal/pkg/cli/env_deploy.go | 2 +- internal/pkg/cli/env_deploy_test.go | 8 ++++---- internal/pkg/cli/env_package.go | 2 +- internal/pkg/cli/env_package_test.go | 8 ++++---- internal/pkg/cli/interfaces.go | 4 ++-- internal/pkg/cli/mocks/mock_interfaces.go | 16 ++++++++-------- 9 files changed, 31 insertions(+), 32 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 26f7bb19a7b..d250d1aacbb 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -13,7 +13,6 @@ import ( "strings" "sync" - "github.com/aws/aws-sdk-go/aws/session" awscfn "github.com/aws/aws-sdk-go/service/cloudformation" "github.com/aws/copilot-cli/internal/pkg/aws/cloudformation" "github.com/aws/copilot-cli/internal/pkg/aws/ec2" @@ -81,7 +80,6 @@ type envDeployer struct { patcher patcher newStackSerializer func(input *deploy.CreateEnvironmentInput, forceUpdateID string, prevParams []*awscfn.Parameter) stackSerializer envDescriber envDescriber - envManagerSession *session.Session lbDescriber lbDescriber newServiceStackDescriber func(string) stackDescriber @@ -138,9 +136,8 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { newStackSerializer: func(in *deploy.CreateEnvironmentInput, lastForceUpdateID string, oldParams []*awscfn.Parameter) stackSerializer { return cfnstack.NewEnvConfigFromExistingStack(in, lastForceUpdateID, oldParams) }, - envDescriber: envDescriber, - envManagerSession: envManagerSession, - lbDescriber: elbv2.New(envManagerSession), + envDescriber: envDescriber, + lbDescriber: elbv2.New(envManagerSession), newServiceStackDescriber: func(svc string) stackDescriber { return stack.NewStackDescriber(cfnstack.NameForService(in.App.Name, in.Env.Name, svc), envManagerSession) }, @@ -148,11 +145,11 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { } // Validate returns an error if the environment manifest is incompatible with services and application configurations. -func (d *envDeployer) Validate(mft *manifest.Environment, output io.Writer) error { - return d.validateCDN(mft, output) +func (d *envDeployer) Validate(mft *manifest.Environment) error { + return d.validateCDN(mft) } -func (d *envDeployer) validateCDN(mft *manifest.Environment, output io.Writer) error { +func (d *envDeployer) validateCDN(mft *manifest.Environment) error { isManagedCDNEnabled := mft.CDNEnabled() && !mft.HasImportedPublicALBCerts() && d.app.Domain != "" if isManagedCDNEnabled { // With managed domain, if the customer isn't using `alias` the A-records are inserted in the service stack as each service domain is unique. @@ -170,9 +167,9 @@ func (d *envDeployer) validateCDN(mft *manifest.Environment, output io.Writer) e case errors.As(err, &redirErr) && mft.ALBIngressRestrictedToCDN(): return err case errors.As(err, &redirErr): - fmt.Fprintln(output, redirErr.Warning()) + log.Warningln(redirErr.warning()) case err != nil: - return fmt.Errorf("can't enable TLS termination on CDN: %w", err) + return fmt.Errorf("enable TLS termination on CDN: %w", err) } } diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index fa08ced235e..1237ea4d96d 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -22,6 +22,7 @@ import ( "github.com/aws/copilot-cli/internal/pkg/deploy/upload/customresource" "github.com/aws/copilot-cli/internal/pkg/describe/stack" "github.com/aws/copilot-cli/internal/pkg/manifest" + "github.com/aws/copilot-cli/internal/pkg/term/log" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" ) @@ -686,8 +687,9 @@ and run %scopilot svc deploy%s.`, } buf := &bytes.Buffer{} + log.DiagnosticWriter = buf - err := d.Validate(tc.mft, buf) + err := d.Validate(tc.mft) if tc.expected != "" { require.EqualError(t, err, tc.expected) } else { diff --git a/internal/pkg/cli/deploy/errors.go b/internal/pkg/cli/deploy/errors.go index 73a03c6fe40..ed31736d537 100644 --- a/internal/pkg/cli/deploy/errors.go +++ b/internal/pkg/cli/deploy/errors.go @@ -67,7 +67,7 @@ and run %s.`, ) } -func (e *errEnvHasPublicServicesWithRedirect) Warning() string { +func (e *errEnvHasPublicServicesWithRedirect) warning() string { return fmt.Sprintf(`%s If you'd like to use %s without a CDN, ensure %s A record is pointed to the ALB.`, e.Error(), diff --git a/internal/pkg/cli/env_deploy.go b/internal/pkg/cli/env_deploy.go index ac712fe16a9..ddc6e239057 100644 --- a/internal/pkg/cli/env_deploy.go +++ b/internal/pkg/cli/env_deploy.go @@ -132,7 +132,7 @@ func (o *deployEnvOpts) Execute() error { if err != nil { return err } - if err := deployer.Validate(mft, os.Stderr); err != nil { + if err := deployer.Validate(mft); err != nil { return err } urls, err := deployer.UploadArtifacts() diff --git a/internal/pkg/cli/env_deploy_test.go b/internal/pkg/cli/env_deploy_test.go index 4eed9cf71fb..f888fb792d7 100644 --- a/internal/pkg/cli/env_deploy_test.go +++ b/internal/pkg/cli/env_deploy_test.go @@ -185,7 +185,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(errors.New("mock error")) + m.deployer.EXPECT().Validate(gomock.Any()).Return(errors.New("mock error")) }, wantedErr: errors.New("mock error"), }, @@ -196,7 +196,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + m.deployer.EXPECT().Validate(gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(nil, errors.New("some error")) }, wantedErr: errors.New("upload artifacts for environment mockEnv: some error"), @@ -208,7 +208,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + m.deployer.EXPECT().Validate(gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(map[string]string{ "mockResource": "mockURL", }, nil) @@ -225,7 +225,7 @@ func TestDeployEnvOpts_Execute(t *testing.T) { m.identity.EXPECT().Get().Return(identity.Caller{ RootUserARN: "mockRootUserARN", }, nil) - m.deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + m.deployer.EXPECT().Validate(gomock.Any()).Return(nil) m.deployer.EXPECT().UploadArtifacts().Return(map[string]string{ "mockResource": "mockURL", }, nil) diff --git a/internal/pkg/cli/env_package.go b/internal/pkg/cli/env_package.go index 7c549bbfa0b..9c1ea684c41 100644 --- a/internal/pkg/cli/env_package.go +++ b/internal/pkg/cli/env_package.go @@ -154,7 +154,7 @@ func (o *packageEnvOpts) Execute() error { if err != nil { return err } - if err := deployer.Validate(mft, os.Stderr); err != nil { + if err := deployer.Validate(mft); err != nil { return err } var urls map[string]string diff --git a/internal/pkg/cli/env_package_test.go b/internal/pkg/cli/env_package_test.go index 5f2bbb47120..3253f4cc2f7 100644 --- a/internal/pkg/cli/env_package_test.go +++ b/internal/pkg/cli/env_package_test.go @@ -204,7 +204,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(errors.New("mock error")) + deployer.EXPECT().Validate(gomock.Any()).Return(errors.New("mock error")) return &packageEnvOpts{ packageEnvVars: packageEnvVars{ @@ -232,7 +232,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + deployer.EXPECT().Validate(gomock.Any()).Return(nil) deployer.EXPECT().UploadArtifacts().Return(nil, errors.New("some error")) return &packageEnvOpts{ @@ -262,7 +262,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + deployer.EXPECT().Validate(gomock.Any()).Return(nil) deployer.EXPECT().GenerateCloudFormationTemplate(gomock.Any()).Return(nil, errors.New("some error")) return &packageEnvOpts{ @@ -292,7 +292,7 @@ func TestPackageEnvOpts_Execute(t *testing.T) { caller := mocks.NewMockidentityService(ctrl) caller.EXPECT().Get().Return(identity.Caller{}, nil) deployer := mocks.NewMockenvPackager(ctrl) - deployer.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + deployer.EXPECT().Validate(gomock.Any()).Return(nil) deployer.EXPECT().GenerateCloudFormationTemplate(&deploy.DeployEnvironmentInput{ RootUserARN: "", CustomResourcesURLs: nil, diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index 82beafb2ff2..85734437452 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -648,12 +648,12 @@ type runner interface { type envDeployer interface { DeployEnvironment(in *clideploy.DeployEnvironmentInput) error - Validate(*manifest.Environment, io.Writer) error + Validate(*manifest.Environment) error UploadArtifacts() (map[string]string, error) } type envPackager interface { GenerateCloudFormationTemplate(in *clideploy.DeployEnvironmentInput) (*clideploy.GenerateCloudFormationTemplateOutput, error) - Validate(*manifest.Environment, io.Writer) error + Validate(*manifest.Environment) error UploadArtifacts() (map[string]string, error) } diff --git a/internal/pkg/cli/mocks/mock_interfaces.go b/internal/pkg/cli/mocks/mock_interfaces.go index c8acc04f08e..ed151f1aa8f 100644 --- a/internal/pkg/cli/mocks/mock_interfaces.go +++ b/internal/pkg/cli/mocks/mock_interfaces.go @@ -6848,17 +6848,17 @@ func (mr *MockenvDeployerMockRecorder) UploadArtifacts() *gomock.Call { } // Validate mocks base method. -func (m *MockenvDeployer) Validate(arg0 *manifest.Environment, arg1 io.Writer) error { +func (m *MockenvDeployer) Validate(arg0 *manifest.Environment) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Validate", arg0, arg1) + ret := m.ctrl.Call(m, "Validate", arg0) ret0, _ := ret[0].(error) return ret0 } // Validate indicates an expected call of Validate. -func (mr *MockenvDeployerMockRecorder) Validate(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockenvDeployerMockRecorder) Validate(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvDeployer)(nil).Validate), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvDeployer)(nil).Validate), arg0) } // MockenvPackager is a mock of envPackager interface. @@ -6915,15 +6915,15 @@ func (mr *MockenvPackagerMockRecorder) UploadArtifacts() *gomock.Call { } // Validate mocks base method. -func (m *MockenvPackager) Validate(arg0 *manifest.Environment, arg1 io.Writer) error { +func (m *MockenvPackager) Validate(arg0 *manifest.Environment) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Validate", arg0, arg1) + ret := m.ctrl.Call(m, "Validate", arg0) ret0, _ := ret[0].(error) return ret0 } // Validate indicates an expected call of Validate. -func (mr *MockenvPackagerMockRecorder) Validate(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockenvPackagerMockRecorder) Validate(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvPackager)(nil).Validate), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockenvPackager)(nil).Validate), arg0) } From f9614b43ee31083fe91504411ddc70fef2cb8d2c Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Mon, 26 Sep 2022 14:08:51 -0700 Subject: [PATCH 33/35] docstrings --- internal/pkg/manifest/env.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 4be8ff51309..eff2b5f60ff 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -184,6 +184,7 @@ func (cfg *EnvironmentConfig) EnvSecurityGroup() (*securityGroupConfig, bool) { return nil, false } +// EnvironmentCDNConfig represents configuration of a CDN. type EnvironmentCDNConfig struct { Enabled *bool Config AdvancedCDNConfig // mutually exclusive with Enabled @@ -213,10 +214,14 @@ func (cfg *EnvironmentConfig) CDNEnabled() bool { return aws.BoolValue(cfg.CDNConfig.Enabled) } +// HasImportedPublicALBCerts returns true when the environment's ALB +// is configured with certs for the public listener. func (cfg *EnvironmentConfig) HasImportedPublicALBCerts() bool { return cfg.HTTPConfig.Public.Certificates != nil } +// CDNDoesTLSTermination returns true when the environment's CDN +// is configured to terminate incoming TLS connections. func (cfg *EnvironmentConfig) CDNDoesTLSTermination() bool { return aws.BoolValue(cfg.CDNConfig.Config.TerminateTLS) } @@ -504,6 +509,8 @@ type Ingress struct { VPCIngress *bool `yaml:"from_vpc"` } +// ALBIngressRestrictedToCDN returns true when the environment is configured +// to only allow ALB ingress from the CDN. func (cfg *EnvironmentConfig) ALBIngressRestrictedToCDN() bool { return aws.BoolValue(cfg.HTTPConfig.Public.SecurityGroupConfig.Ingress.RestrictiveIngress.CDNIngress) } From b3eee7f2e5660a930e304d7b230afa19237d15b0 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Mon, 26 Sep 2022 14:25:06 -0700 Subject: [PATCH 34/35] change error message to a single line --- internal/pkg/cli/deploy/env_test.go | 18 +++++------------- internal/pkg/cli/deploy/errors.go | 6 +++++- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 1237ea4d96d..b60fc81dd10 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -473,7 +473,7 @@ func TestEnvDeployer_Validate(t *testing.T) { setUpMocks: func(m *envDeployerMocks, ctrl *gomock.Controller) { m.envDescriber.EXPECT().Params().Return(nil, errors.New("some error")) }, - expected: "can't enable TLS termination on CDN: get env params: some error", + expected: "enable TLS termination on CDN: get env params: some error", }, "cdn tls termination enabled, fail to get service resources": { app: &config.Application{}, @@ -488,7 +488,7 @@ func TestEnvDeployer_Validate(t *testing.T) { }, nil) m.stackDescribers["svc1"].EXPECT().Resources().Return(nil, errors.New("some error")) }, - expected: `can't enable TLS termination on CDN: verify service "svc1": get stack resources: some error`, + expected: `enable TLS termination on CDN: verify service "svc1": get stack resources: some error`, }, "cdn tls termination enabled, fail to check listener rule": { app: &config.Application{}, @@ -509,7 +509,7 @@ func TestEnvDeployer_Validate(t *testing.T) { }, nil) m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc1RuleARN").Return(elbv2.Rule{}, errors.New("some error")) }, - expected: `can't enable TLS termination on CDN: verify service "svc1": describe listener rule "svc1RuleARN": some error`, + expected: `enable TLS termination on CDN: verify service "svc1": describe listener rule "svc1RuleARN": some error`, }, "cdn tls termination enabled, warn with one service that doesn't redirect, two that do redirect": { app: &config.Application{}, @@ -546,7 +546,7 @@ func TestEnvDeployer_Validate(t *testing.T) { m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc2RuleARN").Return(listenerRuleNoRedirect, nil) m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc3RuleARN").Return(listenerRuleWithRedirect, nil) }, - expectedStdErr: fmt.Sprintf(`Services "svc1" and "svc3" redirect HTTP traffic to HTTPS. + expectedStdErr: fmt.Sprintf(`Note: Services "svc1" and "svc3" redirect HTTP traffic to HTTPS. These services will not be reachable through the CDN. To fix this, set the following field in each manifest: %s @@ -613,15 +613,7 @@ If you'd like to use these services without a CDN, ensure each service's A recor m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc2RuleARN").Return(listenerRuleNoRedirect, nil) m.lbDescriber.EXPECT().DescribeRule(gomock.Any(), "svc3RuleARN").Return(listenerRuleWithRedirect, nil) }, - expected: fmt.Sprintf(`Services "svc1" and "svc3" redirect HTTP traffic to HTTPS. -These services will not be reachable through the CDN. -To fix this, set the following field in each manifest: -%s -http: - redirect_to_https: true -%s -and run %scopilot svc deploy%s.`, - "```", "```", "`", "`"), + expected: "2 services redirect HTTP to HTTPS", }, "cdn tls termination enabled, success with three services that don't redirect": { app: &config.Application{}, diff --git a/internal/pkg/cli/deploy/errors.go b/internal/pkg/cli/deploy/errors.go index ed31736d537..1fcd1d24be4 100644 --- a/internal/pkg/cli/deploy/errors.go +++ b/internal/pkg/cli/deploy/errors.go @@ -46,6 +46,10 @@ type errEnvHasPublicServicesWithRedirect struct { } func (e *errEnvHasPublicServicesWithRedirect) Error() string { + return fmt.Sprintf("%v services redirect HTTP to HTTPS", len(e.services)) +} + +func (e *errEnvHasPublicServicesWithRedirect) RecommendActions() string { n := len(e.services) quoted := make([]string, len(e.services)) for i := range e.services { @@ -70,7 +74,7 @@ and run %s.`, func (e *errEnvHasPublicServicesWithRedirect) warning() string { return fmt.Sprintf(`%s If you'd like to use %s without a CDN, ensure %s A record is pointed to the ALB.`, - e.Error(), + e.RecommendActions(), english.PluralWord(len(e.services), "this service", "these services"), english.PluralWord(len(e.services), "its", "each service's"), ) From 5e13e9436ef84812204c02dfcddddac7982a45d8 Mon Sep 17 00:00:00 2001 From: Daniel Randall <10566468+dannyrandall@users.noreply.github.com> Date: Mon, 26 Sep 2022 15:02:21 -0700 Subject: [PATCH 35/35] docstring for `RecommendActions` --- internal/pkg/cli/deploy/errors.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/pkg/cli/deploy/errors.go b/internal/pkg/cli/deploy/errors.go index 1fcd1d24be4..b2ed47b31a0 100644 --- a/internal/pkg/cli/deploy/errors.go +++ b/internal/pkg/cli/deploy/errors.go @@ -49,6 +49,8 @@ func (e *errEnvHasPublicServicesWithRedirect) Error() string { return fmt.Sprintf("%v services redirect HTTP to HTTPS", len(e.services)) } +// RecommendActions returns recommended actions to be taken after the error. +// Implements main.actionRecommender interface. func (e *errEnvHasPublicServicesWithRedirect) RecommendActions() string { n := len(e.services) quoted := make([]string, len(e.services))