From c3cc8dfc2dd96b32c257c4615690fe5151275ec9 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Mon, 27 Jun 2022 10:25:41 -0700 Subject: [PATCH 01/27] chore(manifest): add cdn bool to env manifest --- internal/pkg/manifest/env.go | 21 +++++++++++++++++++++ internal/pkg/manifest/env_test.go | 18 ++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 32e223813b4..7b2dec1fb4d 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -57,6 +57,7 @@ type EnvironmentConfig struct { Network environmentNetworkConfig `yaml:"network,omitempty,flow"` Observability environmentObservability `yaml:"observability,omitempty,flow"` HTTPConfig environmentHTTPConfig `yaml:"http,omitempty,flow"` + CDN environmentCDNConfig `yaml:"cdn,omitempty,flow"` } type environmentNetworkConfig struct { @@ -69,6 +70,26 @@ type environmentVPCConfig struct { Subnets subnetsConfiguration `yaml:"subnets,omitempty"` } +type environmentCDNConfig struct { + EnableCDN *bool +} + +// UnmarshalYAML overrides the default YAML unmarshaling logic for the ScalingConfigOrT +// struct, allowing it to perform more complex unmarshaling behavior. +// This method implements the yaml.Unmarshaler (v3) interface. +func (e *environmentCDNConfig) UnmarshalYAML(value *yaml.Node) error { + if err := value.Decode(&e.EnableCDN); err != nil { + switch err.(type) { + case *yaml.TypeError: + break + default: + return err + } + } + + return nil +} + func (cfg *environmentVPCConfig) loadVPCConfig(env *config.CustomizeEnv) { if env.IsEmpty() { return diff --git a/internal/pkg/manifest/env_test.go b/internal/pkg/manifest/env_test.go index da09115328c..536c9cdf013 100644 --- a/internal/pkg/manifest/env_test.go +++ b/internal/pkg/manifest/env_test.go @@ -400,6 +400,24 @@ observability: }, }, }, + "unmarshal with content delivery network": { + inContent: `name: prod +type: Environment + +cdn: true +`, + wantedStruct: &Environment{ + Workload: Workload{ + Name: aws.String("prod"), + Type: aws.String("Environment"), + }, + EnvironmentConfig: EnvironmentConfig{ + CDN: environmentCDNConfig{ + aws.Bool(true), + }, + }, + }, + }, "unmarshal with http": { inContent: `name: prod type: Environment From d85dc7c31b85a9c43ac6d377081b38bbbd988412 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Mon, 27 Jun 2022 10:26:07 -0700 Subject: [PATCH 02/27] chore(manifest): add cdn bool to env manifest --- internal/pkg/manifest/env.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 7b2dec1fb4d..1f2107d9b12 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -74,7 +74,7 @@ type environmentCDNConfig struct { EnableCDN *bool } -// UnmarshalYAML overrides the default YAML unmarshaling logic for the ScalingConfigOrT +// 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 (e *environmentCDNConfig) UnmarshalYAML(value *yaml.Node) error { From b03957d035c0287c3414fbe4800501ac3e0339f3 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Wed, 29 Jun 2022 13:13:36 -0700 Subject: [PATCH 03/27] allow cfn to create barebones cf dist via stack --- .../pkg/deploy/cloudformation/stack/env.go | 7 ++++++ internal/pkg/template/env.go | 7 ++++++ .../pkg/template/templates/environment/cf.yml | 3 +++ .../environment/partials/cdn-resources.yml | 22 +++++++++++++++++++ 4 files changed, 39 insertions(+) create mode 100644 internal/pkg/template/templates/environment/partials/cdn-resources.yml diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index a52d49d2829..f542c88b64d 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -115,6 +115,7 @@ func (e *EnvStackConfig) Template() (string, error) { CustomInternalALBSubnets: e.internalALBSubnets(), AllowVPCIngress: e.in.AllowVPCIngress, // TODO(jwh): fetch AllowVPCIngress from Manifest or SSM. Telemetry: e.telemetryConfig(), + CDNConfig: e.cdnConfig(), // VERY TEMP LOL Version: e.in.Version, LatestVersion: deploy.LatestEnvTemplateVersion, @@ -136,6 +137,12 @@ func (e *EnvStackConfig) vpcConfig() template.VPCConfig { } } +func (e *EnvStackConfig) cdnConfig() template.CDNConfig { + return template.CDNConfig{ + EnableCDN: true, //TEMP, ENABLING WON'T ALWAYS BE SET TO TRUE + } +} + func (e *EnvStackConfig) importVPC() *template.ImportVPC { // If a manifest is present, it is the only place we look at. if e.in.Mft != nil { diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index 525eb09153c..c31e78056bb 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -58,6 +58,7 @@ func LeastVersionForFeature(feature string) string { var ( // Template names under "environment/partials/". envCFSubTemplateNames = []string{ + "cdn-resources", "cfn-execution-role", "custom-resources", "custom-resources-role", @@ -92,10 +93,16 @@ type EnvOpts struct { AllowVPCIngress bool Telemetry *Telemetry + CDNConfig CDNConfig + LatestVersion string Manifest string // Serialized manifest used to render the environment template. } +type CDNConfig struct { + EnableCDN bool +} + type VPCConfig struct { Imported *ImportVPC // If not-nil, use the imported VPC resources instead of the Managed VPC. Managed ManagedVPC diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index 75e5558045b..a78690be7a9 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -56,6 +56,9 @@ Conditions: HasAliases: !Not [!Equals [ !Ref Aliases, "" ]] Resources: +{{- if .CDNConfig.EnableCDN}} +{{include "cdn-resources" . | indent 2}} +{{- end}} {{- if not .VPCConfig.Imported}} {{include "vpc-resources" .VPCConfig.Managed | indent 2}} {{include "nat-gateways" .VPCConfig.Managed | indent 2}} diff --git a/internal/pkg/template/templates/environment/partials/cdn-resources.yml b/internal/pkg/template/templates/environment/partials/cdn-resources.yml new file mode 100644 index 00000000000..5d62b2d08e8 --- /dev/null +++ b/internal/pkg/template/templates/environment/partials/cdn-resources.yml @@ -0,0 +1,22 @@ +cloudfrontdistribution: + Type: AWS::CloudFront::Distribution + Properties: + DistributionConfig: + # DefaultCacheBehavior: + # CachePolicyId: !Sub 'copilot-${AppName}-${EnvironmentName}-cache' + # TargetOriginId: !Sub 'copilot-${AppName}-${EnvironmentName}-origin' + # ViewerProtocolPolicy: allow-all + Enabled: false + ForwardedValues: + QueryString: false + IPV6Enabled: true + Origins: + - CustomOriginConfig: + OriginProtocolPolicy: match-viewer + DomainName: copilot-cf.penghaoh.com # This needs to be the ALB DNS + Id: !Sub 'copilot-${AppName}-${EnvironmentName}-origin' + Tags: + - Key: copilot-application + Value: {{.AppName}} + - Key: copilot-environment + Value: {{.EnvName}} From 5161cc8fffba41e0f0ac483cb9c7b64d5983fcd5 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Thu, 30 Jun 2022 11:10:57 -0700 Subject: [PATCH 04/27] Update CDN stack template --- .../environment/partials/cdn-resources.yml | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/internal/pkg/template/templates/environment/partials/cdn-resources.yml b/internal/pkg/template/templates/environment/partials/cdn-resources.yml index 5d62b2d08e8..3ffa1c1e7b7 100644 --- a/internal/pkg/template/templates/environment/partials/cdn-resources.yml +++ b/internal/pkg/template/templates/environment/partials/cdn-resources.yml @@ -1,22 +1,24 @@ cloudfrontdistribution: + Metadata: + 'aws:copilot:description': 'A CloudFront distribution for global content delivery' + Condition: CreateALB Type: AWS::CloudFront::Distribution Properties: DistributionConfig: - # DefaultCacheBehavior: - # CachePolicyId: !Sub 'copilot-${AppName}-${EnvironmentName}-cache' - # TargetOriginId: !Sub 'copilot-${AppName}-${EnvironmentName}-origin' - # ViewerProtocolPolicy: allow-all - Enabled: false - ForwardedValues: - QueryString: false + DefaultCacheBehavior: + AllowedMethods: ["GET", "HEAD", "OPTIONS", "PUT", "PATCH", "POST", "DELETE"] + CachePolicyId: 4135ea2d-6df8-44a3-9df3-4b5a84be39ad + TargetOriginId: !Sub 'copilot-${AppName}-${EnvironmentName}-origin' + ViewerProtocolPolicy: allow-all # Will need to change with TLS termination and Alias usage + Enabled: true IPV6Enabled: true Origins: - CustomOriginConfig: - OriginProtocolPolicy: match-viewer - DomainName: copilot-cf.penghaoh.com # This needs to be the ALB DNS + OriginProtocolPolicy: http-only # Will need to change with TLS termination and Alias usage + DomainName: !GetAtt PublicLoadBalancer.DNSName Id: !Sub 'copilot-${AppName}-${EnvironmentName}-origin' - Tags: - - Key: copilot-application - Value: {{.AppName}} - - Key: copilot-environment - Value: {{.EnvName}} + # ViewerCertificate: # Used for Aliases + # AcmCertificateArn: # Imported certificate goes here + # CloudFrontDefaultCertificate: true # If importing certificate, set to false + # MinimumProtocolVersion: TLSv1 # Default for now, may need to change later + # SsLSupportMethod: sni-only # Changing this can incur costs to the user From ad38e535969e2fc2ff3a76474a34834eda0fc847 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Thu, 30 Jun 2022 12:36:45 -0700 Subject: [PATCH 05/27] add test, change cf public lb ingress --- internal/pkg/template/env_test.go | 1 + internal/pkg/template/templates/environment/cf.yml | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/internal/pkg/template/env_test.go b/internal/pkg/template/env_test.go index cc89b643160..808d5a81a8f 100644 --- a/internal/pkg/template/env_test.go +++ b/internal/pkg/template/env_test.go @@ -15,6 +15,7 @@ func TestTemplate_ParseEnv(t *testing.T) { fs: &mockReadFileFS{ fs: map[string][]byte{ "templates/environment/cf.yml": []byte("test"), + "templates/environment/partials/cdn-resources.yml": []byte("cdn-resources"), "templates/environment/partials/cfn-execution-role.yml": []byte("cfn-execution-role"), "templates/environment/partials/custom-resources.yml": []byte("custom-resources"), "templates/environment/partials/custom-resources-role.yml": []byte("custom-resources-role"), diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index a78690be7a9..f55535e1e2d 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -102,6 +102,11 @@ Resources: Type: AWS::EC2::SecurityGroup Properties: GroupDescription: Access to the public facing load balancer + # Change load balancer to only accept ingress from CloudFront when CDN Enabled +{{- if .CDNConfig.EnableCDN}} + SecurityGroupIngress: + SourcePrefixListId: pl-82a045eb +{{- else}} SecurityGroupIngress: - CidrIp: 0.0.0.0/0 Description: Allow from anyone on port 80 @@ -113,6 +118,7 @@ Resources: FromPort: 443 IpProtocol: tcp ToPort: 443 +{{- end}} {{- if .VPCConfig.Imported}} VpcId: {{.VPCConfig.Imported.ID}} {{- else}} From edebc10a3d1a11cf98a536066a38afeea68cc29c Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Tue, 5 Jul 2022 14:00:52 -0700 Subject: [PATCH 06/27] get prefix list for public lb security group --- internal/pkg/cli/deploy/env.go | 30 +++++++++ .../pkg/deploy/cloudformation/stack/env.go | 25 +++++++- internal/pkg/deploy/env.go | 1 + internal/pkg/manifest/env.go | 37 ++++++++++- internal/pkg/manifest/env_test.go | 64 ++++++++++++++++++- internal/pkg/manifest/validate_env.go | 13 ++++ internal/pkg/manifest/validate_env_test.go | 34 ++++++++++ internal/pkg/template/env.go | 3 +- .../pkg/template/templates/environment/cf.yml | 4 +- 9 files changed, 203 insertions(+), 8 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 15e65bc53c9..0445e7ecb69 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -13,6 +13,7 @@ import ( "github.com/aws/copilot-cli/internal/pkg/deploy/upload/customresource" "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/s3" "github.com/aws/copilot-cli/internal/pkg/aws/sessions" "github.com/aws/copilot-cli/internal/pkg/config" @@ -37,6 +38,10 @@ type environmentDeployer interface { UpdateAndRenderEnvironment(out termprogress.FileWriter, env *deploy.CreateEnvironmentInput, opts ...cloudformation.StackOption) error } +type prefixListGetter interface { + CloudFrontManagedPrefixListID() (*string, error) +} + type envDeployer struct { app *config.Application env *config.Environment @@ -47,6 +52,7 @@ type envDeployer struct { uploader customResourcesUploader // Deprecated: after legacy is removed. templateFS template.Reader s3 uploader + ec2 prefixListGetter // Dependencies to deploy an environment. envDeployer environmentDeployer @@ -86,6 +92,7 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { templateFS: template.New(), uploader: template.New(), s3: s3.New(envRegionSession), + ec2: ec2.New(envRegionSession), envDeployer: deploycfn.New(envManagerSession), }, nil @@ -104,6 +111,24 @@ func (d *envDeployer) UploadArtifacts() (map[string]string, error) { return d.legacyUploadCustomResources(resources.S3Bucket) } +func (d *envDeployer) getPrefixListId(in *DeployEnvironmentInput) (*string, error) { + if in.Manifest != nil { + if !in.Manifest.CDN.CDNEnabled() { + return nil, nil + } + } + + if d.ec2 == nil { + return nil, nil + } + + id, err := d.ec2.CloudFrontManagedPrefixListID() + if err != nil { + return nil, err + } + return id, nil +} + func (d *envDeployer) legacyUploadCustomResources(bucket string) (map[string]string, error) { urls, err := d.uploader.UploadEnvironmentCustomResources(func(key string, objects ...s3.NamedBinary) (string, error) { return d.s3.ZipAndUpload(bucket, key, objects...) @@ -145,6 +170,10 @@ func (d *envDeployer) DeployEnvironment(in *DeployEnvironmentInput) error { if err != nil { return err } + prefixListId, err := d.getPrefixListId(in) + if err != nil { + return err + } deployEnvInput := &deploy.CreateEnvironmentInput{ Name: d.env.Name, App: deploy.AppInformation{ @@ -156,6 +185,7 @@ func (d *envDeployer) DeployEnvironment(in *DeployEnvironmentInput) error { CustomResourcesURLs: in.CustomResourcesURLs, ArtifactBucketARN: s3.FormatARN(partition.ID(), resources.S3Bucket), ArtifactBucketKeyARN: resources.KMSKeyARN, + PrefixListID: prefixListId, Mft: in.Manifest, Version: deploy.LatestEnvTemplateVersion, } diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index f542c88b64d..eb2f155686a 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -115,7 +115,7 @@ func (e *EnvStackConfig) Template() (string, error) { CustomInternalALBSubnets: e.internalALBSubnets(), AllowVPCIngress: e.in.AllowVPCIngress, // TODO(jwh): fetch AllowVPCIngress from Manifest or SSM. Telemetry: e.telemetryConfig(), - CDNConfig: e.cdnConfig(), // VERY TEMP LOL + CDNConfig: e.cdnConfig(), Version: e.in.Version, LatestVersion: deploy.LatestEnvTemplateVersion, @@ -138,8 +138,29 @@ func (e *EnvStackConfig) vpcConfig() template.VPCConfig { } func (e *EnvStackConfig) cdnConfig() template.CDNConfig { + var ( + enabled bool + prefixListId *string + ) + + if e.in.Mft != nil { + // CDN Config specified by AdvancedCDNConfig + if !e.in.Mft.CDN.CDNConfig.IsEmpty() { + enabled = true + prefixListId = e.in.PrefixListID + } + + // CDN Config specified by basic boolean enabler + if e.in.Mft.CDN.EnableCDN != nil { + if *e.in.Mft.CDN.EnableCDN { + enabled = true + } + } + } + return template.CDNConfig{ - EnableCDN: true, //TEMP, ENABLING WON'T ALWAYS BE SET TO TRUE + EnableCDN: enabled, // TODO(CaptainCarpensir): create CDN config with manifest-specified settings + PrefixListID: prefixListId, } } diff --git a/internal/pkg/deploy/env.go b/internal/pkg/deploy/env.go index c1644fe1e8a..b5e42f8f996 100644 --- a/internal/pkg/deploy/env.go +++ b/internal/pkg/deploy/env.go @@ -35,6 +35,7 @@ type CreateEnvironmentInput struct { ImportCertARNs []string // Optional configuration if users want to import certificates. InternalALBSubnets []string // Optional configuration if users want to specify internal ALB placement. AllowVPCIngress bool // Optional configuration to allow access to internal ALB from ports 80/443. + PrefixListID *string // Optional configuration to specify public security group ingress based on prefix lists Telemetry *config.Telemetry // Optional observability and monitoring configuration. Mft *manifest.Environment diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 1f2107d9b12..62dbb649f8e 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -4,6 +4,7 @@ package manifest import ( + "errors" "fmt" "sort" @@ -72,13 +73,39 @@ type environmentVPCConfig struct { type environmentCDNConfig struct { EnableCDN *bool + CDNConfig AdvancedCDNConfig // mutually exclusive with EnableCDN +} + +// AdvancedCDNConfig represents an advanced configuration for a Content Delivery Network. +type AdvancedCDNConfig struct { + PrefixListIngress *bool `yaml:"public_ingress_enabled"` +} + +// IsEmpty returns whether AdvancedCDNConfig is empty. +func (a *AdvancedCDNConfig) IsEmpty() bool { + return a.PrefixListIngress == nil +} + +// CDNEnabled returns whether a CDN configuration has been enabled in the environment manifest. +func (e *environmentCDNConfig) CDNEnabled() bool { + if e.EnableCDN != nil { + if *e.EnableCDN { + return true + } + } + + if !e.CDNConfig.IsEmpty() { + return true + } + + return false } // 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 (e *environmentCDNConfig) UnmarshalYAML(value *yaml.Node) error { - if err := value.Decode(&e.EnableCDN); err != nil { + if err := value.Decode(&e.CDNConfig); err != nil { switch err.(type) { case *yaml.TypeError: break @@ -87,6 +114,14 @@ func (e *environmentCDNConfig) UnmarshalYAML(value *yaml.Node) error { } } + if !e.CDNConfig.IsEmpty() { + // Successfully unmarshalled CDNConfig fields, return + return nil + } + + if err := value.Decode(&e.EnableCDN); err != nil { + return errors.New(`unable to unmarshal into bool or composite-style map`) + } return nil } diff --git a/internal/pkg/manifest/env_test.go b/internal/pkg/manifest/env_test.go index 536c9cdf013..7199faa8b1e 100644 --- a/internal/pkg/manifest/env_test.go +++ b/internal/pkg/manifest/env_test.go @@ -400,7 +400,7 @@ observability: }, }, }, - "unmarshal with content delivery network": { + "unmarshal with content delivery network bool": { inContent: `name: prod type: Environment @@ -413,7 +413,28 @@ cdn: true }, EnvironmentConfig: EnvironmentConfig{ CDN: environmentCDNConfig{ - aws.Bool(true), + EnableCDN: aws.Bool(true), + }, + }, + }, + }, + "unmarshal with content delivery network map": { + inContent: `name: prod +type: Environment + +cdn: + public_ingress_enabled: false +`, + wantedStruct: &Environment{ + Workload: Workload{ + Name: aws.String("prod"), + Type: aws.String("Environment"), + }, + EnvironmentConfig: EnvironmentConfig{ + CDN: environmentCDNConfig{ + CDNConfig: AdvancedCDNConfig{ + PrefixListIngress: aws.Bool(false), + }, }, }, }, @@ -635,3 +656,42 @@ func TestEnvironmentObservability_IsEmpty(t *testing.T) { }) } } + +func TestEnvironmentCDNConfig_CDNEnabled(t *testing.T) { + testCases := map[string]struct { + in environmentCDNConfig + wanted bool + }{ + "enabled via bool": { + in: environmentCDNConfig{ + EnableCDN: aws.Bool(true), + }, + wanted: true, + }, + "enabled via struct": { + in: environmentCDNConfig{ + CDNConfig: AdvancedCDNConfig{ + PrefixListIngress: aws.Bool(false), + }, + }, + wanted: true, + }, + "not enabled because empty": { + in: environmentCDNConfig{}, + wanted: false, + }, + "not enabled via bool": { + in: environmentCDNConfig{ + EnableCDN: aws.Bool(false), + }, + wanted: false, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + got := tc.in.CDNEnabled() + require.Equal(t, tc.wanted, got) + }) + } +} diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 00b0aed3c6c..bcbf020ad43 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -208,6 +208,19 @@ func (o privateHTTPConfig) Validate() error { return nil } +// Validate returns nil if environmentCDNConfig is configured correctly. +func (cfg environmentCDNConfig) Validate() error { + if cfg.CDNConfig.IsEmpty() { + return nil + } + return cfg.CDNConfig.Validate() +} + +// Validate is a no-op for AdvancedCDNConfig. +func (a AdvancedCDNConfig) Validate() error { + return nil +} + func (c EnvironmentConfig) validateInternalALBSubnets() error { isImported := make(map[string]bool) for _, placementSubnet := range c.HTTPConfig.Private.InternalALBSubnets { diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index 12678c67615..8520b730b7d 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -556,6 +556,40 @@ func TestSubnetsConfiguration_Validate(t *testing.T) { } } +func TestCDNConfiguration_Validate(t *testing.T) { + testCases := map[string]struct { + in environmentCDNConfig + wantedError error + }{ + "valid if empty": { + in: environmentCDNConfig{}, + }, + "valid if bool specified": { + in: environmentCDNConfig{ + EnableCDN: aws.Bool(false), + }, + }, + "valid if advanced config configured correctly": { + in: environmentCDNConfig{ + CDNConfig: AdvancedCDNConfig{ + PrefixListIngress: aws.Bool(false), + }, + }, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + gotErr := tc.in.Validate() + if tc.wantedError != nil { + require.Error(t, gotErr) + require.EqualError(t, tc.wantedError, gotErr.Error()) + } else { + require.NoError(t, gotErr) + } + }) + } +} + func TestSubnetConfiguration_Validate(t *testing.T) { mockCIDR := IPNet("10.0.0.0/24") testCases := map[string]struct { diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index c31e78056bb..8a79dda1730 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -100,7 +100,8 @@ type EnvOpts struct { } type CDNConfig struct { - EnableCDN bool + EnableCDN bool + PrefixListID *string } type VPCConfig struct { diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index f55535e1e2d..d253a5388e2 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -103,9 +103,9 @@ Resources: Properties: GroupDescription: Access to the public facing load balancer # Change load balancer to only accept ingress from CloudFront when CDN Enabled -{{- if .CDNConfig.EnableCDN}} +{{- if .CDNConfig.PrefixListID}} SecurityGroupIngress: - SourcePrefixListId: pl-82a045eb + SourcePrefixListId: {{.CDNConfig.PrefixListID}} {{- else}} SecurityGroupIngress: - CidrIp: 0.0.0.0/0 From fbc6ba66dd5a3dbf3c1c50c1014cc06b4e2a2d59 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Tue, 5 Jul 2022 15:16:14 -0700 Subject: [PATCH 07/27] correct ingress logic --- internal/pkg/cli/deploy/env.go | 2 +- internal/pkg/manifest/env.go | 13 +++++++ internal/pkg/manifest/env_test.go | 37 +++++++++++++++++++ .../pkg/template/templates/environment/cf.yml | 3 ++ 4 files changed, 54 insertions(+), 1 deletion(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 0445e7ecb69..663bce15097 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -113,7 +113,7 @@ func (d *envDeployer) UploadArtifacts() (map[string]string, error) { func (d *envDeployer) getPrefixListId(in *DeployEnvironmentInput) (*string, error) { if in.Manifest != nil { - if !in.Manifest.CDN.CDNEnabled() { + if in.Manifest.CDN.PublicIngressEnabled() { return nil, nil } } diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 62dbb649f8e..2bab471ec39 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -86,6 +86,19 @@ func (a *AdvancedCDNConfig) IsEmpty() bool { return a.PrefixListIngress == nil } +// PublicIngressEnabled returns whether the cloud front facing security group allows public access. +func (e *environmentCDNConfig) PublicIngressEnabled() bool { + if e.CDNConfig.PrefixListIngress != nil { + return *e.CDNConfig.PrefixListIngress + } + if e.EnableCDN != nil { + if *e.EnableCDN { + return true + } + } + return false +} + // CDNEnabled returns whether a CDN configuration has been enabled in the environment manifest. func (e *environmentCDNConfig) CDNEnabled() bool { if e.EnableCDN != nil { diff --git a/internal/pkg/manifest/env_test.go b/internal/pkg/manifest/env_test.go index 7199faa8b1e..a90a240bd1e 100644 --- a/internal/pkg/manifest/env_test.go +++ b/internal/pkg/manifest/env_test.go @@ -657,6 +657,43 @@ func TestEnvironmentObservability_IsEmpty(t *testing.T) { } } +func TestEnvironmentCDNConfig_PublicIngressEnabled(t *testing.T) { + testCases := map[string]struct { + in environmentCDNConfig + wanted bool + }{ + "enabled via bool": { + in: environmentCDNConfig{ + CDNConfig: AdvancedCDNConfig{ + PrefixListIngress: aws.Bool(true), + }, + }, + wanted: true, + }, + "disabled via bool": { + in: environmentCDNConfig{ + CDNConfig: AdvancedCDNConfig{ + PrefixListIngress: aws.Bool(false), + }, + }, + wanted: false, + }, + "enabled by default": { + in: environmentCDNConfig{ + EnableCDN: aws.Bool(true), + }, + wanted: true, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + got := tc.in.PublicIngressEnabled() + require.Equal(t, tc.wanted, got) + }) + } +} + func TestEnvironmentCDNConfig_CDNEnabled(t *testing.T) { testCases := map[string]struct { in environmentCDNConfig diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index d253a5388e2..a03f4c1437a 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -106,6 +106,9 @@ Resources: {{- if .CDNConfig.PrefixListID}} SecurityGroupIngress: SourcePrefixListId: {{.CDNConfig.PrefixListID}} + FromPort: 0 + IpProtocol: tcp + ToPort: 0 {{- else}} SecurityGroupIngress: - CidrIp: 0.0.0.0/0 From 507f86ea49f7338bb99bafb27d6ba2a8e750507e Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Tue, 5 Jul 2022 15:39:52 -0700 Subject: [PATCH 08/27] rename manifest tag to be more clear --- internal/pkg/cli/deploy/env.go | 2 +- internal/pkg/manifest/env.go | 6 +++--- internal/pkg/manifest/env_test.go | 4 ++-- internal/pkg/template/templates/environment/cf.yml | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 663bce15097..2c71f99e3f9 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -113,7 +113,7 @@ func (d *envDeployer) UploadArtifacts() (map[string]string, error) { func (d *envDeployer) getPrefixListId(in *DeployEnvironmentInput) (*string, error) { if in.Manifest != nil { - if in.Manifest.CDN.PublicIngressEnabled() { + if in.Manifest.CDN.PublicIngressAllowed() { return nil, nil } } diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 2bab471ec39..b622e7d62a2 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -78,7 +78,7 @@ type environmentCDNConfig struct { // AdvancedCDNConfig represents an advanced configuration for a Content Delivery Network. type AdvancedCDNConfig struct { - PrefixListIngress *bool `yaml:"public_ingress_enabled"` + PrefixListIngress *bool `yaml:"public_ingress_allowed"` } // IsEmpty returns whether AdvancedCDNConfig is empty. @@ -86,8 +86,8 @@ func (a *AdvancedCDNConfig) IsEmpty() bool { return a.PrefixListIngress == nil } -// PublicIngressEnabled returns whether the cloud front facing security group allows public access. -func (e *environmentCDNConfig) PublicIngressEnabled() bool { +// PublicIngressAllowed returns whether the cloud front facing security group allows public access. +func (e *environmentCDNConfig) PublicIngressAllowed() bool { if e.CDNConfig.PrefixListIngress != nil { return *e.CDNConfig.PrefixListIngress } diff --git a/internal/pkg/manifest/env_test.go b/internal/pkg/manifest/env_test.go index a90a240bd1e..9c9abce34de 100644 --- a/internal/pkg/manifest/env_test.go +++ b/internal/pkg/manifest/env_test.go @@ -657,7 +657,7 @@ func TestEnvironmentObservability_IsEmpty(t *testing.T) { } } -func TestEnvironmentCDNConfig_PublicIngressEnabled(t *testing.T) { +func TestEnvironmentCDNConfig_PublicIngressAllowed(t *testing.T) { testCases := map[string]struct { in environmentCDNConfig wanted bool @@ -688,7 +688,7 @@ func TestEnvironmentCDNConfig_PublicIngressEnabled(t *testing.T) { for name, tc := range testCases { t.Run(name, func(t *testing.T) { - got := tc.in.PublicIngressEnabled() + got := tc.in.PublicIngressAllowed() require.Equal(t, tc.wanted, got) }) } diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index a03f4c1437a..a27ac33f043 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -102,7 +102,7 @@ Resources: Type: AWS::EC2::SecurityGroup Properties: GroupDescription: Access to the public facing load balancer - # Change load balancer to only accept ingress from CloudFront when CDN Enabled + # Change load balancer to only accept ingress from CloudFront when prefix list specified {{- if .CDNConfig.PrefixListID}} SecurityGroupIngress: SourcePrefixListId: {{.CDNConfig.PrefixListID}} From 58e48790ca74f0f34e94939cdb2bf1e30790c3c5 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Tue, 5 Jul 2022 16:00:59 -0700 Subject: [PATCH 09/27] fix unit test manifest example --- internal/pkg/manifest/env_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/manifest/env_test.go b/internal/pkg/manifest/env_test.go index 9c9abce34de..ac6d6530e45 100644 --- a/internal/pkg/manifest/env_test.go +++ b/internal/pkg/manifest/env_test.go @@ -423,7 +423,7 @@ cdn: true type: Environment cdn: - public_ingress_enabled: false + public_ingress_allowed: false `, wantedStruct: &Environment{ Workload: Workload{ From c6a8b4d88b810a97d1611f8e1b9169ea00525dbd Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Wed, 6 Jul 2022 10:19:34 -0700 Subject: [PATCH 10/27] prefix-list changes --- .../pkg/deploy/cloudformation/stack/env.go | 27 +++---------------- internal/pkg/manifest/env.go | 22 +++------------ internal/pkg/template/env.go | 7 +++-- 3 files changed, 10 insertions(+), 46 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index 76052e5127c..a788743bfbe 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -297,31 +297,12 @@ func (e *BootstrapEnvStackConfig) ToEnv(stack *cloudformation.Stack) (*config.En }, nil } -func (e *EnvStackConfig) cdnConfig() template.CDNConfig { - var ( - enabled bool - prefixListId *string - ) - - if e.in.Mft != nil { - // CDN Config specified by AdvancedCDNConfig - if !e.in.Mft.CDN.CDNConfig.IsEmpty() { - enabled = true - prefixListId = e.in.PrefixListID - } - - // CDN Config specified by basic boolean enabler - if e.in.Mft.CDN.EnableCDN != nil { - if *e.in.Mft.CDN.EnableCDN { - enabled = true - } - } +func (e *EnvStackConfig) cdnConfig() *template.CDNConfig { + if !e.in.Mft.EnvironmentConfig.CDNConfig.CDNEnabled() { + return nil } - return template.CDNConfig{ - EnableCDN: enabled, - PrefixListID: prefixListId, - } + return &template.CDNConfig{} } func (e *EnvStackConfig) vpcConfig() template.VPCConfig { diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index b622e7d62a2..43d86147ec5 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -58,7 +58,7 @@ type EnvironmentConfig struct { Network environmentNetworkConfig `yaml:"network,omitempty,flow"` Observability environmentObservability `yaml:"observability,omitempty,flow"` HTTPConfig environmentHTTPConfig `yaml:"http,omitempty,flow"` - CDN environmentCDNConfig `yaml:"cdn,omitempty,flow"` + CDNConfig environmentCDNConfig `yaml:"cdn,omitempty,flow"` } type environmentNetworkConfig struct { @@ -78,33 +78,17 @@ type environmentCDNConfig struct { // AdvancedCDNConfig represents an advanced configuration for a Content Delivery Network. type AdvancedCDNConfig struct { - PrefixListIngress *bool `yaml:"public_ingress_allowed"` } // IsEmpty returns whether AdvancedCDNConfig is empty. func (a *AdvancedCDNConfig) IsEmpty() bool { - return a.PrefixListIngress == nil -} - -// PublicIngressAllowed returns whether the cloud front facing security group allows public access. -func (e *environmentCDNConfig) PublicIngressAllowed() bool { - if e.CDNConfig.PrefixListIngress != nil { - return *e.CDNConfig.PrefixListIngress - } - if e.EnableCDN != nil { - if *e.EnableCDN { - return true - } - } - return false + return true } // CDNEnabled returns whether a CDN configuration has been enabled in the environment manifest. func (e *environmentCDNConfig) CDNEnabled() bool { if e.EnableCDN != nil { - if *e.EnableCDN { - return true - } + return *e.EnableCDN } if !e.CDNConfig.IsEmpty() { diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index faf8ed486e0..e49dec2cf51 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -104,15 +104,14 @@ type EnvOpts struct { AllowVPCIngress bool Telemetry *Telemetry - CDNConfig CDNConfig + CDNConfig *CDNConfig - LatestVersion string + LatestVersion string SerializedManifest string // Serialized manifest used to render the environment template. } +// CDNConfig represents configuration settings for a Content Delivery Network with CloudFront. type CDNConfig struct { - EnableCDN bool - PrefixListID *string } type VPCConfig struct { From 613b61961046d3099572b556326e3f7ed00421ee Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Wed, 6 Jul 2022 13:55:31 -0700 Subject: [PATCH 11/27] refactor pr scope add transformer, remove prefix-list logic, address efekarakus and iamhopaul's feedback --- internal/pkg/cli/deploy/env.go | 30 --------- .../pkg/deploy/cloudformation/stack/env.go | 28 ++------ internal/pkg/deploy/env.go | 1 - internal/pkg/manifest/env.go | 50 +++++--------- internal/pkg/manifest/env_test.go | 67 +++++-------------- internal/pkg/manifest/transform.go | 27 ++++++++ internal/pkg/manifest/transform_test.go | 52 ++++++++++++++ internal/pkg/manifest/validate_env.go | 2 +- internal/pkg/manifest/validate_env_test.go | 6 +- internal/pkg/template/env.go | 7 +- .../pkg/template/templates/environment/cf.yml | 13 +--- .../environment/partials/cdn-resources.yml | 4 +- 12 files changed, 129 insertions(+), 158 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 2c71f99e3f9..15e65bc53c9 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -13,7 +13,6 @@ import ( "github.com/aws/copilot-cli/internal/pkg/deploy/upload/customresource" "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/s3" "github.com/aws/copilot-cli/internal/pkg/aws/sessions" "github.com/aws/copilot-cli/internal/pkg/config" @@ -38,10 +37,6 @@ type environmentDeployer interface { UpdateAndRenderEnvironment(out termprogress.FileWriter, env *deploy.CreateEnvironmentInput, opts ...cloudformation.StackOption) error } -type prefixListGetter interface { - CloudFrontManagedPrefixListID() (*string, error) -} - type envDeployer struct { app *config.Application env *config.Environment @@ -52,7 +47,6 @@ type envDeployer struct { uploader customResourcesUploader // Deprecated: after legacy is removed. templateFS template.Reader s3 uploader - ec2 prefixListGetter // Dependencies to deploy an environment. envDeployer environmentDeployer @@ -92,7 +86,6 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { templateFS: template.New(), uploader: template.New(), s3: s3.New(envRegionSession), - ec2: ec2.New(envRegionSession), envDeployer: deploycfn.New(envManagerSession), }, nil @@ -111,24 +104,6 @@ func (d *envDeployer) UploadArtifacts() (map[string]string, error) { return d.legacyUploadCustomResources(resources.S3Bucket) } -func (d *envDeployer) getPrefixListId(in *DeployEnvironmentInput) (*string, error) { - if in.Manifest != nil { - if in.Manifest.CDN.PublicIngressAllowed() { - return nil, nil - } - } - - if d.ec2 == nil { - return nil, nil - } - - id, err := d.ec2.CloudFrontManagedPrefixListID() - if err != nil { - return nil, err - } - return id, nil -} - func (d *envDeployer) legacyUploadCustomResources(bucket string) (map[string]string, error) { urls, err := d.uploader.UploadEnvironmentCustomResources(func(key string, objects ...s3.NamedBinary) (string, error) { return d.s3.ZipAndUpload(bucket, key, objects...) @@ -170,10 +145,6 @@ func (d *envDeployer) DeployEnvironment(in *DeployEnvironmentInput) error { if err != nil { return err } - prefixListId, err := d.getPrefixListId(in) - if err != nil { - return err - } deployEnvInput := &deploy.CreateEnvironmentInput{ Name: d.env.Name, App: deploy.AppInformation{ @@ -185,7 +156,6 @@ func (d *envDeployer) DeployEnvironment(in *DeployEnvironmentInput) error { CustomResourcesURLs: in.CustomResourcesURLs, ArtifactBucketARN: s3.FormatARN(partition.ID(), resources.S3Bucket), ArtifactBucketKeyARN: resources.KMSKeyARN, - PrefixListID: prefixListId, Mft: in.Manifest, Version: deploy.LatestEnvTemplateVersion, } diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index 76052e5127c..efbe7af2d2b 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -297,31 +297,17 @@ func (e *BootstrapEnvStackConfig) ToEnv(stack *cloudformation.Stack) (*config.En }, nil } -func (e *EnvStackConfig) cdnConfig() template.CDNConfig { - var ( - enabled bool - prefixListId *string - ) - +func (e *EnvStackConfig) cdnConfig() *template.CDNConfig { + // If a manifest is present, check there, otherwise cdn cannot be enabled if e.in.Mft != nil { - // CDN Config specified by AdvancedCDNConfig - if !e.in.Mft.CDN.CDNConfig.IsEmpty() { - enabled = true - prefixListId = e.in.PrefixListID - } - - // CDN Config specified by basic boolean enabler - if e.in.Mft.CDN.EnableCDN != nil { - if *e.in.Mft.CDN.EnableCDN { - enabled = true - } + if !e.in.Mft.CDNConfig.CDNEnabled() { + return nil } + } else { + return nil } - return template.CDNConfig{ - EnableCDN: enabled, - PrefixListID: prefixListId, - } + return &template.CDNConfig{} } func (e *EnvStackConfig) vpcConfig() template.VPCConfig { diff --git a/internal/pkg/deploy/env.go b/internal/pkg/deploy/env.go index 7eb093efa4b..72949b47392 100644 --- a/internal/pkg/deploy/env.go +++ b/internal/pkg/deploy/env.go @@ -35,7 +35,6 @@ type CreateEnvironmentInput struct { ImportCertARNs []string // Optional configuration if users want to import certificates. InternalALBSubnets []string // Optional configuration if users want to specify internal ALB placement. AllowVPCIngress bool // Optional configuration to allow access to internal ALB from ports 80/443. - PrefixListID *string // Optional configuration to specify public security group ingress based on prefix lists Telemetry *config.Telemetry // Optional observability and monitoring configuration. Mft *manifest.Environment diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index b622e7d62a2..2eac073da76 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -58,7 +58,7 @@ type EnvironmentConfig struct { Network environmentNetworkConfig `yaml:"network,omitempty,flow"` Observability environmentObservability `yaml:"observability,omitempty,flow"` HTTPConfig environmentHTTPConfig `yaml:"http,omitempty,flow"` - CDN environmentCDNConfig `yaml:"cdn,omitempty,flow"` + CDNConfig environmentCDNConfig `yaml:"cdn,omitempty,flow"` } type environmentNetworkConfig struct { @@ -72,53 +72,37 @@ type environmentVPCConfig struct { } type environmentCDNConfig struct { - EnableCDN *bool - CDNConfig AdvancedCDNConfig // mutually exclusive with EnableCDN + Enabled *bool + CDNConfig AdvancedCDNConfig // mutually exclusive with Enabled } // AdvancedCDNConfig represents an advanced configuration for a Content Delivery Network. -type AdvancedCDNConfig struct { - PrefixListIngress *bool `yaml:"public_ingress_allowed"` -} +type AdvancedCDNConfig struct{} -// IsEmpty returns whether AdvancedCDNConfig is empty. -func (a *AdvancedCDNConfig) IsEmpty() bool { - return a.PrefixListIngress == nil +// IsEmpty returns whether environmentCDNConfig is empty. +func (cfg *environmentCDNConfig) IsEmpty() bool { + return cfg.Enabled == nil && cfg.CDNConfig.IsEmpty() } -// PublicIngressAllowed returns whether the cloud front facing security group allows public access. -func (e *environmentCDNConfig) PublicIngressAllowed() bool { - if e.CDNConfig.PrefixListIngress != nil { - return *e.CDNConfig.PrefixListIngress - } - if e.EnableCDN != nil { - if *e.EnableCDN { - return true - } - } - return false +// IsEmpty returns whether AdvancedCDNConfig is empty. +func (cfg *AdvancedCDNConfig) IsEmpty() bool { + return true } // CDNEnabled returns whether a CDN configuration has been enabled in the environment manifest. -func (e *environmentCDNConfig) CDNEnabled() bool { - if e.EnableCDN != nil { - if *e.EnableCDN { - return true - } - } - - if !e.CDNConfig.IsEmpty() { +func (cfg *environmentCDNConfig) CDNEnabled() bool { + if !cfg.CDNConfig.IsEmpty() { return true } - return false + return aws.BoolValue(cfg.Enabled) } // 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 (e *environmentCDNConfig) UnmarshalYAML(value *yaml.Node) error { - if err := value.Decode(&e.CDNConfig); err != nil { +func (cfg *environmentCDNConfig) UnmarshalYAML(value *yaml.Node) error { + if err := value.Decode(&cfg.CDNConfig); err != nil { switch err.(type) { case *yaml.TypeError: break @@ -127,12 +111,12 @@ func (e *environmentCDNConfig) UnmarshalYAML(value *yaml.Node) error { } } - if !e.CDNConfig.IsEmpty() { + if !cfg.CDNConfig.IsEmpty() { // Successfully unmarshalled CDNConfig fields, return return nil } - if err := value.Decode(&e.EnableCDN); err != nil { + if err := value.Decode(&cfg.Enabled); err != nil { return errors.New(`unable to unmarshal into bool or composite-style map`) } return nil diff --git a/internal/pkg/manifest/env_test.go b/internal/pkg/manifest/env_test.go index ac6d6530e45..356558bd7a4 100644 --- a/internal/pkg/manifest/env_test.go +++ b/internal/pkg/manifest/env_test.go @@ -412,29 +412,8 @@ cdn: true Type: aws.String("Environment"), }, EnvironmentConfig: EnvironmentConfig{ - CDN: environmentCDNConfig{ - EnableCDN: aws.Bool(true), - }, - }, - }, - }, - "unmarshal with content delivery network map": { - inContent: `name: prod -type: Environment - -cdn: - public_ingress_allowed: false -`, - wantedStruct: &Environment{ - Workload: Workload{ - Name: aws.String("prod"), - Type: aws.String("Environment"), - }, - EnvironmentConfig: EnvironmentConfig{ - CDN: environmentCDNConfig{ - CDNConfig: AdvancedCDNConfig{ - PrefixListIngress: aws.Bool(false), - }, + CDNConfig: environmentCDNConfig{ + Enabled: aws.Bool(true), }, }, }, @@ -657,38 +636,26 @@ func TestEnvironmentObservability_IsEmpty(t *testing.T) { } } -func TestEnvironmentCDNConfig_PublicIngressAllowed(t *testing.T) { +func TestEnvironmentCDNConfig_IsEmpty(t *testing.T) { testCases := map[string]struct { in environmentCDNConfig wanted bool }{ - "enabled via bool": { - in: environmentCDNConfig{ - CDNConfig: AdvancedCDNConfig{ - PrefixListIngress: aws.Bool(true), - }, - }, + "empty": { + in: environmentCDNConfig{}, wanted: true, }, - "disabled via bool": { + "not empty": { in: environmentCDNConfig{ - CDNConfig: AdvancedCDNConfig{ - PrefixListIngress: aws.Bool(false), - }, + Enabled: aws.Bool(false), }, wanted: false, }, - "enabled by default": { - in: environmentCDNConfig{ - EnableCDN: aws.Bool(true), - }, - wanted: true, - }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - got := tc.in.PublicIngressAllowed() + got := tc.in.IsEmpty() require.Equal(t, tc.wanted, got) }) } @@ -701,25 +668,23 @@ func TestEnvironmentCDNConfig_CDNEnabled(t *testing.T) { }{ "enabled via bool": { in: environmentCDNConfig{ - EnableCDN: aws.Bool(true), - }, - wanted: true, - }, - "enabled via struct": { - in: environmentCDNConfig{ - CDNConfig: AdvancedCDNConfig{ - PrefixListIngress: aws.Bool(false), - }, + Enabled: aws.Bool(true), }, wanted: true, }, + // "enabled via struct": { + // in: environmentCDNConfig{ + // CDNConfig: AdvancedCDNConfig{}, + // }, + // wanted: true, + // }, "not enabled because empty": { in: environmentCDNConfig{}, wanted: false, }, "not enabled via bool": { in: environmentCDNConfig{ - EnableCDN: aws.Bool(false), + Enabled: aws.Bool(false), }, wanted: false, }, diff --git a/internal/pkg/manifest/transform.go b/internal/pkg/manifest/transform.go index e2eb9dc3dae..37aaf731ec7 100644 --- a/internal/pkg/manifest/transform.go +++ b/internal/pkg/manifest/transform.go @@ -35,6 +35,7 @@ var defaultTransformers = []mergo.Transformers{ sqsQueueOrBoolTransformer{}, routingRuleConfigOrBoolTransformer{}, secretTransformer{}, + environmentCDNConfigTransformer{}, } // See a complete list of `reflect.Kind` here: https://pkg.go.dev/reflect#Kind. @@ -463,6 +464,32 @@ func (t secretTransformer) Transformer(typ reflect.Type) func(dst, src reflect.V } } +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{}) { + return nil + } + + return func(dst, src reflect.Value) error { + dstStruct, srcStruct := dst.Interface().(environmentCDNConfig), src.Interface().(environmentCDNConfig) + + if !srcStruct.CDNConfig.IsEmpty() { + dstStruct.Enabled = nil + } + + if srcStruct.Enabled != nil { + dstStruct.CDNConfig = AdvancedCDNConfig{} + } + + if dst.CanSet() { // For extra safety to prevent panicking. + dst.Set(reflect.ValueOf(dstStruct)) + } + return nil + } +} + type basicTransformer struct{} // Transformer returns custom merge logic for volume's fields. diff --git a/internal/pkg/manifest/transform_test.go b/internal/pkg/manifest/transform_test.go index 07811ef7848..00cf6a05808 100644 --- a/internal/pkg/manifest/transform_test.go +++ b/internal/pkg/manifest/transform_test.go @@ -1124,3 +1124,55 @@ 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) + }{ + // "enabled set to nil if cdnconfig is not empty": { + // original: func(cfg *environmentCDNConfig) { + // cfg.Enabled = aws.Bool(true) + // }, + // override: func(cfg *environmentCDNConfig) { + // cfg.CDNConfig = AdvancedCDNConfig{} // Need to update with advanced fields when AdvancedCDNConfig struct is not empty + // }, + // wanted: func(cfg *environmentCDNConfig) { + // cfg.CDNConfig = AdvancedCDNConfig{} // Need to update with advanced fields when AdvancedCDNConfig struct is not empty + // }, + // }, + "cdnconfig set to empty if enabled is not nil": { + original: func(cfg *environmentCDNConfig) { + cfg.CDNConfig = AdvancedCDNConfig{} // Need to update with advanced fields when AdvancedCDNConfig struct is not empty + }, + override: func(cfg *environmentCDNConfig) { + cfg.Enabled = aws.Bool(true) + }, + wanted: func(cfg *environmentCDNConfig) { + cfg.Enabled = aws.Bool(true) + }, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + var dst, override, wanted environmentCDNConfig + + tc.original(&dst) + tc.override(&override) + tc.wanted(&wanted) + + // Perform default merge. + err := mergo.Merge(&dst, override, mergo.WithOverride) + require.NoError(t, err) + + // Use custom transformer. + err = mergo.Merge(&dst, override, mergo.WithOverride, mergo.WithTransformers(environmentCDNConfigTransformer{})) + require.NoError(t, err) + + require.NoError(t, err) + require.Equal(t, wanted, dst) + }) + } +} diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index bcbf020ad43..8b32453f790 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -217,7 +217,7 @@ func (cfg environmentCDNConfig) Validate() error { } // Validate is a no-op for AdvancedCDNConfig. -func (a AdvancedCDNConfig) Validate() error { +func (cfg AdvancedCDNConfig) Validate() error { return nil } diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index 8520b730b7d..f0a4edbf3f3 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -566,14 +566,12 @@ func TestCDNConfiguration_Validate(t *testing.T) { }, "valid if bool specified": { in: environmentCDNConfig{ - EnableCDN: aws.Bool(false), + Enabled: aws.Bool(false), }, }, "valid if advanced config configured correctly": { in: environmentCDNConfig{ - CDNConfig: AdvancedCDNConfig{ - PrefixListIngress: aws.Bool(false), - }, + CDNConfig: AdvancedCDNConfig{}, }, }, } diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index faf8ed486e0..2f8263d41b5 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -104,15 +104,14 @@ type EnvOpts struct { AllowVPCIngress bool Telemetry *Telemetry - CDNConfig CDNConfig + CDNConfig *CDNConfig // If nil, no cdn is to be used - LatestVersion string + LatestVersion string SerializedManifest string // Serialized manifest used to render the environment template. } +// CDNConfig represents a Content Delivery Network deployed by CloudFront. type CDNConfig struct { - EnableCDN bool - PrefixListID *string } type VPCConfig struct { diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index 23cea3787fb..b590c748822 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -56,10 +56,10 @@ Conditions: HasAliases: !Not [!Equals [ !Ref Aliases, "" ]] Resources: -{{- if .CDNConfig.EnableCDN}} +{{include "bootstrap-resources" . | indent 2}} +{{- if .CDNConfig}} {{include "cdn-resources" . | indent 2}} {{- end}} -{{include "bootstrap-resources" . | indent 2}} {{- if not .VPCConfig.Imported}} {{include "vpc-resources" .VPCConfig.Managed | indent 2}} {{include "nat-gateways" .VPCConfig.Managed | indent 2}} @@ -103,14 +103,6 @@ Resources: Type: AWS::EC2::SecurityGroup Properties: GroupDescription: Access to the public facing load balancer - # Change load balancer to only accept ingress from CloudFront when prefix list specified -{{- if .CDNConfig.PrefixListID}} - SecurityGroupIngress: - SourcePrefixListId: {{.CDNConfig.PrefixListID}} - FromPort: 0 - IpProtocol: tcp - ToPort: 0 -{{- else}} SecurityGroupIngress: - CidrIp: 0.0.0.0/0 Description: Allow from anyone on port 80 @@ -122,7 +114,6 @@ Resources: FromPort: 443 IpProtocol: tcp ToPort: 443 -{{- end}} {{- if .VPCConfig.Imported}} VpcId: {{.VPCConfig.Imported.ID}} {{- else}} diff --git a/internal/pkg/template/templates/environment/partials/cdn-resources.yml b/internal/pkg/template/templates/environment/partials/cdn-resources.yml index 3ffa1c1e7b7..237d96004b4 100644 --- a/internal/pkg/template/templates/environment/partials/cdn-resources.yml +++ b/internal/pkg/template/templates/environment/partials/cdn-resources.yml @@ -1,4 +1,4 @@ -cloudfrontdistribution: +CloudFrontDistribution: Metadata: 'aws:copilot:description': 'A CloudFront distribution for global content delivery' Condition: CreateALB @@ -7,7 +7,7 @@ cloudfrontdistribution: DistributionConfig: DefaultCacheBehavior: AllowedMethods: ["GET", "HEAD", "OPTIONS", "PUT", "PATCH", "POST", "DELETE"] - CachePolicyId: 4135ea2d-6df8-44a3-9df3-4b5a84be39ad + CachePolicyId: 4135ea2d-6df8-44a3-9df3-4b5a84be39ad # See https://go.aws/3bJid3k TargetOriginId: !Sub 'copilot-${AppName}-${EnvironmentName}-origin' ViewerProtocolPolicy: allow-all # Will need to change with TLS termination and Alias usage Enabled: true From 3e46ef1bf9afa50a9603523210648ad8f2324256 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Wed, 6 Jul 2022 14:09:22 -0700 Subject: [PATCH 12/27] fix missed merge overwrite conflict --- internal/pkg/manifest/env_test.go | 2 +- internal/pkg/manifest/validate_env.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/manifest/env_test.go b/internal/pkg/manifest/env_test.go index 49acd8b1f4e..3d402e74972 100644 --- a/internal/pkg/manifest/env_test.go +++ b/internal/pkg/manifest/env_test.go @@ -413,7 +413,7 @@ cdn: true Name: aws.String("prod"), Type: aws.String("Environment"), }, - EnvironmentConfig: EnvironmentConfig{ + environmentConfig: environmentConfig{ CDNConfig: environmentCDNConfig{ Enabled: aws.Bool(true), }, diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 460a44c2860..667327bfffa 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -221,7 +221,7 @@ func (cfg AdvancedCDNConfig) Validate() error { return nil } -func (c EnvironmentConfig) validateInternalALBSubnets() error { +func (c environmentConfig) validateInternalALBSubnets() error { isImported := make(map[string]bool) for _, placementSubnet := range c.HTTPConfig.Private.InternalALBSubnets { for _, subnet := range append(c.Network.VPC.Subnets.Private, c.Network.VPC.Subnets.Public...) { From c500169abc91ab0f144c8fb77c525ec48db519e5 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Fri, 8 Jul 2022 11:05:41 -0700 Subject: [PATCH 13/27] allow env manifest to restrict ingress to cf --- internal/pkg/cli/deploy/env.go | 44 ++++++++++++++++++ internal/pkg/cli/deploy/env_test.go | 35 ++++++++++++-- .../pkg/deploy/cloudformation/stack/env.go | 33 ++++++------- .../stack/env_integration_test.go | 1 + ...late-with-imported-certs-observability.yml | 2 +- internal/pkg/deploy/env.go | 1 + internal/pkg/manifest/env.go | 29 ++++++------ internal/pkg/manifest/env_test.go | 46 +++++++++---------- internal/pkg/manifest/validate_env.go | 20 ++++---- internal/pkg/manifest/validate_env_test.go | 43 +++++++++++------ internal/pkg/template/env.go | 13 +++--- .../pkg/template/templates/environment/cf.yml | 10 ++++ 12 files changed, 191 insertions(+), 86 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 15e65bc53c9..4ef8325e68a 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -8,11 +8,13 @@ import ( "io" "os" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/copilot-cli/internal/pkg/template" "github.com/aws/copilot-cli/internal/pkg/deploy/upload/customresource" "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/s3" "github.com/aws/copilot-cli/internal/pkg/aws/sessions" "github.com/aws/copilot-cli/internal/pkg/config" @@ -37,6 +39,10 @@ type environmentDeployer interface { UpdateAndRenderEnvironment(out termprogress.FileWriter, env *deploy.CreateEnvironmentInput, opts ...cloudformation.StackOption) error } +type prefixListGetter interface { + CloudFrontManagedPrefixListID() (*string, error) +} + type envDeployer struct { app *config.Application env *config.Environment @@ -47,6 +53,7 @@ type envDeployer struct { uploader customResourcesUploader // Deprecated: after legacy is removed. templateFS template.Reader s3 uploader + ec2 prefixListGetter // Dependencies to deploy an environment. envDeployer environmentDeployer @@ -86,6 +93,7 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { templateFS: template.New(), uploader: template.New(), s3: s3.New(envRegionSession), + ec2: ec2.New(envRegionSession), envDeployer: deploycfn.New(envManagerSession), }, nil @@ -128,6 +136,37 @@ func (d *envDeployer) uploadCustomResources(bucket string) (map[string]string, e return urls, nil } +func (d *envDeployer) prefixLists(in *DeployEnvironmentInput) ([]string, error) { + var prefixListIDs []string + + cfManagedPrefixListId, err := d.cfManagedPrefixListId(in) + if err != nil { + return nil, err + } + if cfManagedPrefixListId != nil { + prefixListIDs = append(prefixListIDs, *cfManagedPrefixListId) + } + + return prefixListIDs, nil +} + +func (d *envDeployer) cfManagedPrefixListId(in *DeployEnvironmentInput) (*string, error) { + // Check if ingress is allowed from cloudfront + if in.Manifest != nil { + if !aws.BoolValue(in.Manifest.HTTPConfig.Public.LimitToCFIngress) { + return nil, nil + } + } else { + return nil, nil + } + + id, err := d.ec2.CloudFrontManagedPrefixListID() + if err != nil { + return nil, fmt.Errorf("retrieve CloudFront managed prefix list id: %s", err) + } + return id, nil +} + // DeployEnvironmentInput contains information used to deploy the environment. type DeployEnvironmentInput struct { RootUserARN string @@ -145,6 +184,10 @@ func (d *envDeployer) DeployEnvironment(in *DeployEnvironmentInput) error { if err != nil { return err } + prefixListIDs, err := d.prefixLists(in) + if err != nil { + return err + } deployEnvInput := &deploy.CreateEnvironmentInput{ Name: d.env.Name, App: deploy.AppInformation{ @@ -156,6 +199,7 @@ func (d *envDeployer) DeployEnvironment(in *DeployEnvironmentInput) error { CustomResourcesURLs: in.CustomResourcesURLs, ArtifactBucketARN: s3.FormatARN(partition.ID(), resources.S3Bucket), ArtifactBucketKeyARN: resources.KMSKeyARN, + PrefixListIDs: prefixListIDs, Mft: in.Manifest, Version: deploy.LatestEnvTemplateVersion, } diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 96d7fdb8c19..bda280bfbd7 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -10,7 +10,9 @@ import ( "strings" "testing" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/copilot-cli/internal/pkg/deploy/upload/customresource" + "github.com/aws/copilot-cli/internal/pkg/manifest" "github.com/aws/copilot-cli/internal/pkg/aws/cloudformation" "github.com/aws/copilot-cli/internal/pkg/cli/deploy/mocks" @@ -204,8 +206,9 @@ func TestEnvDeployer_UploadArtifacts(t *testing.T) { } type deployEnvironmentMock struct { - appCFN *mocks.MockappResourcesGetter - envDeployer *mocks.MockenvironmentDeployer + appCFN *mocks.MockappResourcesGetter + envDeployer *mocks.MockenvironmentDeployer + prefixListGetter *mocks.MockprefixListGetter } func TestEnvDeployer_DeployEnvironment(t *testing.T) { @@ -220,6 +223,7 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { } testCases := map[string]struct { setUpMocks func(m *deployEnvironmentMock) + inManifest *manifest.Environment wantedError error }{ "fail to get app resources by region": { @@ -229,11 +233,30 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { }, 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{ + S3Bucket: "mockS3Bucket", + }, nil) + m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return(nil, errors.New("some error")) + }, + inManifest: &manifest.Environment{ + EnvironmentConfig: manifest.EnvironmentConfig{ + HTTPConfig: manifest.EnvironmentHTTPConfig{ + Public: manifest.PublicHTTPConfig{ + LimitToCFIngress: aws.Bool(true), + }, + }, + }, + }, + wantedError: fmt.Errorf("retrieve CloudFront managed prefix list id: some error"), + }, "fail to deploy environment": { setUpMocks: func(m *deployEnvironmentMock) { m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) + m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return(aws.String("mockPrefixListID"), nil).Times(0) m.envDeployer.EXPECT().UpdateAndRenderEnvironment(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("some error")) }, wantedError: errors.New("some error"), @@ -243,6 +266,7 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) + m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return(aws.String("mockPrefixListID"), nil).Times(0) m.envDeployer.EXPECT().UpdateAndRenderEnvironment(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( func(_ progress.FileWriter, in *deploy.CreateEnvironmentInput, opts ...cloudformation.StackOption) error { require.Equal(t, mockEnvName, in.Name) @@ -262,8 +286,9 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { defer ctrl.Finish() m := &deployEnvironmentMock{ - appCFN: mocks.NewMockappResourcesGetter(ctrl), - envDeployer: mocks.NewMockenvironmentDeployer(ctrl), + appCFN: mocks.NewMockappResourcesGetter(ctrl), + envDeployer: mocks.NewMockenvironmentDeployer(ctrl), + prefixListGetter: mocks.NewMockprefixListGetter(ctrl), } tc.setUpMocks(m) d := envDeployer{ @@ -275,12 +300,14 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { }, appCFN: m.appCFN, envDeployer: m.envDeployer, + ec2: m.prefixListGetter, } mockIn := &DeployEnvironmentInput{ RootUserARN: "mockRootUserARN", CustomResourcesURLs: map[string]string{ "mockResource": "mockURL", }, + Manifest: tc.inManifest, } gotErr := d.DeployEnvironment(mockIn) if tc.wantedError != nil { diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index efbe7af2d2b..bfb89b4178d 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -100,22 +100,23 @@ func (e *EnvStackConfig) Template() (string, error) { mft = string(out) } content, err := e.parser.ParseEnv(&template.EnvOpts{ - AppName: e.in.App.Name, - EnvName: e.in.Name, - CustomResources: crs, - DNSCertValidatorLambda: dnsCertValidator, - DNSDelegationLambda: dnsDelegation, - CustomDomainLambda: customDomain, - ScriptBucketName: bucket, - ArtifactBucketARN: e.in.ArtifactBucketARN, - ArtifactBucketKeyARN: e.in.ArtifactBucketKeyARN, - PublicImportedCertARNs: e.importPublicCertARNs(), - PrivateImportedCertARNs: e.importPrivateCertARNs(), - VPCConfig: e.vpcConfig(), - CustomInternalALBSubnets: e.internalALBSubnets(), - AllowVPCIngress: e.in.AllowVPCIngress, // TODO(jwh): fetch AllowVPCIngress from Manifest or SSM. - Telemetry: e.telemetryConfig(), - CDNConfig: e.cdnConfig(), + AppName: e.in.App.Name, + EnvName: e.in.Name, + CustomResources: crs, + DNSCertValidatorLambda: dnsCertValidator, + DNSDelegationLambda: dnsDelegation, + CustomDomainLambda: customDomain, + ScriptBucketName: bucket, + ArtifactBucketARN: e.in.ArtifactBucketARN, + ArtifactBucketKeyARN: e.in.ArtifactBucketKeyARN, + PublicFacingPrefixListIDs: e.in.PrefixListIDs, + PublicImportedCertARNs: e.importPublicCertARNs(), + PrivateImportedCertARNs: e.importPrivateCertARNs(), + VPCConfig: e.vpcConfig(), + CustomInternalALBSubnets: e.internalALBSubnets(), + AllowVPCIngress: e.in.AllowVPCIngress, // TODO(jwh): fetch AllowVPCIngress from Manifest or SSM. + Telemetry: e.telemetryConfig(), + CDNConfig: e.cdnConfig(), Version: e.in.Version, LatestVersion: deploy.LatestEnvTemplateVersion, diff --git a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go index 4af22fda307..3acca875f5e 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go @@ -36,6 +36,7 @@ type: Environment # All these comments should be deleted. http: public: + restrict_alb_ingress_to_cf: false certificates: - cert-1 - cert-2 diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml index 3687ae15d0d..4a0ab39e2a8 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml @@ -6,7 +6,7 @@ Metadata: name: test type: Environment observability: {container_insights: true} - http: {public: {certificates: [cert-1, cert-2]}} + http: {public: {restrict_alb_ingress_to_cf: false, certificates: [cert-1, cert-2]}} Parameters: AppName: diff --git a/internal/pkg/deploy/env.go b/internal/pkg/deploy/env.go index 72949b47392..622fe7dd001 100644 --- a/internal/pkg/deploy/env.go +++ b/internal/pkg/deploy/env.go @@ -35,6 +35,7 @@ type CreateEnvironmentInput struct { ImportCertARNs []string // Optional configuration if users want to import certificates. InternalALBSubnets []string // Optional configuration if users want to specify internal ALB placement. AllowVPCIngress bool // Optional configuration to allow access to internal ALB from ports 80/443. + PrefixListIDs []string // Optional configuration to specify public security group ingress based on prefix lists Telemetry *config.Telemetry // Optional observability and monitoring configuration. Mft *manifest.Environment diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index fc94e7eb442..3a25e899e0b 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -23,7 +23,7 @@ var environmentManifestPath = "environment/manifest.yml" // Environment is the manifest configuration for an environment. type Environment struct { Workload `yaml:",inline"` - environmentConfig `yaml:",inline"` + EnvironmentConfig `yaml:",inline"` parser template.Parser } @@ -49,7 +49,7 @@ func FromEnvConfig(cfg *config.Environment, parser template.Parser) *Environment var vpc environmentVPCConfig vpc.loadVPCConfig(cfg.CustomConfig) - var http environmentHTTPConfig + var http EnvironmentHTTPConfig http.loadLBConfig(cfg.CustomConfig) var obs environmentObservability @@ -60,7 +60,7 @@ func FromEnvConfig(cfg *config.Environment, parser template.Parser) *Environment Name: stringP(cfg.Name), Type: stringP(EnvironmentManifestType), }, - environmentConfig: environmentConfig{ + EnvironmentConfig: EnvironmentConfig{ Network: environmentNetworkConfig{ VPC: vpc, }, @@ -83,10 +83,11 @@ func (e *Environment) MarshalBinary() ([]byte, error) { return content.Bytes(), nil } -type environmentConfig struct { +// EnvironmentConfig defines the configuration settings for an environment manifest +type EnvironmentConfig struct { Network environmentNetworkConfig `yaml:"network,omitempty,flow"` Observability environmentObservability `yaml:"observability,omitempty,flow"` - HTTPConfig environmentHTTPConfig `yaml:"http,omitempty,flow"` + HTTPConfig EnvironmentHTTPConfig `yaml:"http,omitempty,flow"` CDNConfig environmentCDNConfig `yaml:"cdn,omitempty,flow"` } @@ -300,17 +301,18 @@ func (o *environmentObservability) loadObsConfig(tele *config.Telemetry) { o.ContainerInsights = &tele.EnableContainerInsights } -type environmentHTTPConfig struct { - Public publicHTTPConfig `yaml:"public,omitempty"` +// EnvironmentHTTPConfig defines the configuration settings for an environment group's HTTP connections +type EnvironmentHTTPConfig struct { + Public PublicHTTPConfig `yaml:"public,omitempty"` Private privateHTTPConfig `yaml:"private,omitempty"` } // IsEmpty returns true if neither the public ALB nor the internal ALB is configured. -func (cfg environmentHTTPConfig) IsEmpty() bool { +func (cfg EnvironmentHTTPConfig) IsEmpty() bool { return cfg.Public.IsEmpty() && cfg.Private.IsEmpty() } -func (cfg *environmentHTTPConfig) loadLBConfig(env *config.CustomizeEnv) { +func (cfg *EnvironmentHTTPConfig) loadLBConfig(env *config.CustomizeEnv) { if env.IsEmpty() { return } @@ -322,13 +324,14 @@ func (cfg *environmentHTTPConfig) loadLBConfig(env *config.CustomizeEnv) { cfg.Public.Certificates = env.ImportCertARNs } -type publicHTTPConfig struct { - Certificates []string `yaml:"certificates,omitempty"` +type PublicHTTPConfig struct { + LimitToCFIngress *bool `yaml:"restrict_alb_ingress_to_cf"` + Certificates []string `yaml:"certificates,omitempty"` } // IsEmpty returns true if there is no customization to the public ALB. -func (cfg publicHTTPConfig) IsEmpty() bool { - return len(cfg.Certificates) == 0 +func (cfg PublicHTTPConfig) IsEmpty() bool { + return len(cfg.Certificates) == 0 && cfg.LimitToCFIngress == nil } type privateHTTPConfig struct { diff --git a/internal/pkg/manifest/env_test.go b/internal/pkg/manifest/env_test.go index 3d402e74972..a5e5214f428 100644 --- a/internal/pkg/manifest/env_test.go +++ b/internal/pkg/manifest/env_test.go @@ -39,7 +39,7 @@ func TestFromEnvConfig(t *testing.T) { Name: stringP("test"), Type: stringP("Environment"), }, - environmentConfig: environmentConfig{ + EnvironmentConfig: EnvironmentConfig{ Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ CIDR: ipNetP("10.0.0.0/16"), @@ -96,7 +96,7 @@ func TestFromEnvConfig(t *testing.T) { Name: stringP("test"), Type: stringP("Environment"), }, - environmentConfig: environmentConfig{ + EnvironmentConfig: EnvironmentConfig{ Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ CIDR: ipNetP("10.0.0.0/16"), @@ -146,7 +146,7 @@ func TestFromEnvConfig(t *testing.T) { Name: stringP("test"), Type: stringP("Environment"), }, - environmentConfig: environmentConfig{ + EnvironmentConfig: EnvironmentConfig{ Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ ID: stringP("vpc-3f139646"), @@ -196,7 +196,7 @@ func TestFromEnvConfig(t *testing.T) { Name: stringP("test"), Type: stringP("Environment"), }, - environmentConfig: environmentConfig{ + EnvironmentConfig: EnvironmentConfig{ Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ Subnets: subnetsConfiguration{ @@ -211,8 +211,8 @@ func TestFromEnvConfig(t *testing.T) { }, }, }, - HTTPConfig: environmentHTTPConfig{ - Public: publicHTTPConfig{ + HTTPConfig: EnvironmentHTTPConfig{ + Public: PublicHTTPConfig{ Certificates: []string{"arn:aws:acm:region:account:certificate/certificate_ID_1", "arn:aws:acm:region:account:certificate/certificate_ID_2"}, }, }, @@ -233,9 +233,9 @@ func TestFromEnvConfig(t *testing.T) { Name: stringP("test"), Type: stringP("Environment"), }, - environmentConfig: environmentConfig{ - HTTPConfig: environmentHTTPConfig{ - Public: publicHTTPConfig{ + EnvironmentConfig: EnvironmentConfig{ + HTTPConfig: EnvironmentHTTPConfig{ + Public: PublicHTTPConfig{ Certificates: []string{"arn:aws:acm:region:account:certificate/certificate_ID_1", "arn:aws:acm:region:account:certificate/certificate_ID_2"}, }, }, @@ -260,7 +260,7 @@ func TestFromEnvConfig(t *testing.T) { Name: stringP("test"), Type: stringP("Environment"), }, - environmentConfig: environmentConfig{ + EnvironmentConfig: EnvironmentConfig{ Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ Subnets: subnetsConfiguration{ @@ -276,7 +276,7 @@ func TestFromEnvConfig(t *testing.T) { }, }, }, - HTTPConfig: environmentHTTPConfig{ + HTTPConfig: EnvironmentHTTPConfig{ Private: privateHTTPConfig{ InternalALBSubnets: []string{"subnet2"}, Certificates: []string{"arn:aws:acm:region:account:certificate/certificate_ID_1", "arn:aws:acm:region:account:certificate/certificate_ID_2"}, @@ -299,7 +299,7 @@ func TestFromEnvConfig(t *testing.T) { Name: stringP("test"), Type: stringP("Environment"), }, - environmentConfig: environmentConfig{ + EnvironmentConfig: EnvironmentConfig{ Observability: environmentObservability{ ContainerInsights: aws.Bool(false), }, @@ -352,7 +352,7 @@ network: Name: aws.String("test"), Type: aws.String("Environment"), }, - environmentConfig: environmentConfig{ + EnvironmentConfig: EnvironmentConfig{ Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ CIDR: &mockVPCCIDR, @@ -395,7 +395,7 @@ observability: Name: aws.String("prod"), Type: aws.String("Environment"), }, - environmentConfig: environmentConfig{ + EnvironmentConfig: EnvironmentConfig{ Observability: environmentObservability{ ContainerInsights: aws.Bool(true), }, @@ -413,7 +413,7 @@ cdn: true Name: aws.String("prod"), Type: aws.String("Environment"), }, - environmentConfig: environmentConfig{ + EnvironmentConfig: EnvironmentConfig{ CDNConfig: environmentCDNConfig{ Enabled: aws.Bool(true), }, @@ -435,9 +435,9 @@ http: Name: aws.String("prod"), Type: aws.String("Environment"), }, - environmentConfig: environmentConfig{ - HTTPConfig: environmentHTTPConfig{ - Public: publicHTTPConfig{ + EnvironmentConfig: EnvironmentConfig{ + HTTPConfig: EnvironmentHTTPConfig{ + Public: PublicHTTPConfig{ Certificates: []string{"cert-1", "cert-2"}, }, }, @@ -729,15 +729,15 @@ func TestSubnetsConfiguration_IsEmpty(t *testing.T) { func TestEnvironmentHTTPConfig_IsEmpty(t *testing.T) { testCases := map[string]struct { - in environmentHTTPConfig + in EnvironmentHTTPConfig wanted bool }{ "empty": { wanted: true, }, "not empty": { - in: environmentHTTPConfig{ - Public: publicHTTPConfig{ + in: EnvironmentHTTPConfig{ + Public: PublicHTTPConfig{ Certificates: []string{"mock-cert"}, }, }, @@ -753,14 +753,14 @@ func TestEnvironmentHTTPConfig_IsEmpty(t *testing.T) { func TestPublicHTTPConfig_IsEmpty(t *testing.T) { testCases := map[string]struct { - in publicHTTPConfig + in PublicHTTPConfig wanted bool }{ "empty": { wanted: true, }, "not empty": { - in: publicHTTPConfig{ + in: PublicHTTPConfig{ Certificates: []string{"mock-cert-1"}, }, }, diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 667327bfffa..79835aaa130 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -19,14 +19,14 @@ var ( // Validate returns nil if Environment is configured correctly. func (e Environment) Validate() error { - if err := e.environmentConfig.Validate(); err != nil { + if err := e.EnvironmentConfig.Validate(); err != nil { return fmt.Errorf(`validate "network": %w`, err) } return nil } -// Validate returns nil if environmentConfig is configured correctly. -func (e environmentConfig) Validate() error { +// Validate returns nil if EnvironmentConfig is configured correctly. +func (e EnvironmentConfig) Validate() error { if err := e.Network.Validate(); err != nil { return fmt.Errorf(`validate "network": %w`, err) } @@ -37,6 +37,10 @@ func (e environmentConfig) Validate() error { return fmt.Errorf(`validate "http config": %w`, err) } + if aws.BoolValue(e.HTTPConfig.Public.LimitToCFIngress) && !e.CDNConfig.CDNEnabled() { + return errors.New("CDN must be enabled to limit security group ingress to CloudFront") + } + if e.HTTPConfig.Private.InternalALBSubnets != nil { if !e.Network.VPC.imported() { return errors.New("in order to specify internal ALB subnet placement, subnets must be imported") @@ -177,8 +181,8 @@ func (o environmentObservability) Validate() error { return nil } -// Validate returns nil if environmentHTTPConfig is configured correctly. -func (cfg environmentHTTPConfig) Validate() error { +// Validate returns nil if EnvironmentHTTPConfig is configured correctly. +func (cfg EnvironmentHTTPConfig) Validate() error { if err := cfg.Public.Validate(); err != nil { return fmt.Errorf(`validate "public": %w`, err) } @@ -188,8 +192,8 @@ func (cfg environmentHTTPConfig) Validate() error { return nil } -// Validate returns nil if publicHTTPConfig is configured correctly. -func (cfg publicHTTPConfig) Validate() error { +// Validate returns nil if PublicHTTPConfig is configured correctly. +func (cfg PublicHTTPConfig) Validate() error { for idx, certARN := range cfg.Certificates { if _, err := arn.Parse(certARN); err != nil { return fmt.Errorf(`parse "certificates[%d]": %w`, idx, err) @@ -221,7 +225,7 @@ func (cfg AdvancedCDNConfig) Validate() error { return nil } -func (c environmentConfig) validateInternalALBSubnets() error { +func (c EnvironmentConfig) validateInternalALBSubnets() error { isImported := make(map[string]bool) for _, placementSubnet := range c.HTTPConfig.Private.InternalALBSubnets { for _, subnet := range append(c.Network.VPC.Subnets.Private, c.Network.VPC.Subnets.Public...) { diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index 6544eca6104..199f4f966e1 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -19,7 +19,7 @@ func TestEnvironment_Validate(t *testing.T) { }{ "malformed network": { in: Environment{ - environmentConfig: environmentConfig{ + EnvironmentConfig: EnvironmentConfig{ Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ ID: stringP("vpc-123"), @@ -51,11 +51,11 @@ func TestEnvironmentConfig_Validate(t *testing.T) { mockPrivateSubnet1CIDR := IPNet("10.0.3.0/24") mockPrivateSubnet2CIDR := IPNet("10.0.4.0/24") testCases := map[string]struct { - in environmentConfig + in EnvironmentConfig wantedError string }{ "error if internal ALB subnet placement specified with adjusted vpc": { - in: environmentConfig{ + in: EnvironmentConfig{ Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ CIDR: ipNetP("apple cider"), @@ -84,7 +84,7 @@ func TestEnvironmentConfig_Validate(t *testing.T) { }, }, - HTTPConfig: environmentHTTPConfig{ + HTTPConfig: EnvironmentHTTPConfig{ Private: privateHTTPConfig{ InternalALBSubnets: []string{"mockSubnet"}, }, @@ -92,8 +92,21 @@ func TestEnvironmentConfig_Validate(t *testing.T) { }, wantedError: "in order to specify internal ALB subnet placement, subnets must be imported", }, + "error if security group ingress is limited to a cdn distribution not enabled": { + in: EnvironmentConfig{ + CDNConfig: environmentCDNConfig{ + Enabled: aws.Bool(false), + }, + HTTPConfig: EnvironmentHTTPConfig{ + Public: PublicHTTPConfig{ + LimitToCFIngress: aws.Bool(true), + }, + }, + }, + wantedError: "CDN must be enabled to limit security group ingress to CloudFront", + }, "error if subnets specified for internal ALB placement don't exist": { - in: environmentConfig{ + in: EnvironmentConfig{ Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ ID: aws.String("mockID"), @@ -105,7 +118,7 @@ func TestEnvironmentConfig_Validate(t *testing.T) { }, }, }, - HTTPConfig: environmentHTTPConfig{ + HTTPConfig: EnvironmentHTTPConfig{ Private: privateHTTPConfig{ InternalALBSubnets: []string{"nonexistentSubnet"}, }, @@ -114,7 +127,7 @@ func TestEnvironmentConfig_Validate(t *testing.T) { wantedError: "subnet(s) specified for internal ALB placement not imported", }, "valid case with internal ALB placement": { - in: environmentConfig{ + in: EnvironmentConfig{ Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ ID: aws.String("mockID"), @@ -130,7 +143,7 @@ func TestEnvironmentConfig_Validate(t *testing.T) { }, }, }, - HTTPConfig: environmentHTTPConfig{ + HTTPConfig: EnvironmentHTTPConfig{ Private: privateHTTPConfig{ InternalALBSubnets: []string{"existentSubnet", "anotherExistentSubnet"}, }, @@ -653,19 +666,19 @@ func TestSubnetConfiguration_Validate(t *testing.T) { func TestEnvironmentHTTPConfig_Validate(t *testing.T) { testCases := map[string]struct { - in environmentHTTPConfig + in EnvironmentHTTPConfig wantedErrorMsgPrefix string }{ "malformed public certificate": { - in: environmentHTTPConfig{ - Public: publicHTTPConfig{ + in: EnvironmentHTTPConfig{ + Public: PublicHTTPConfig{ Certificates: []string{"arn:aws:weird-little-arn"}, }, }, wantedErrorMsgPrefix: `parse "certificates[0]": `, }, "malformed private certificate": { - in: environmentHTTPConfig{ + in: EnvironmentHTTPConfig{ Private: privateHTTPConfig{ Certificates: []string{"arn:aws:weird-little-arn"}, }, @@ -673,14 +686,14 @@ func TestEnvironmentHTTPConfig_Validate(t *testing.T) { wantedErrorMsgPrefix: `parse "certificates[0]": `, }, "success with public cert": { - in: environmentHTTPConfig{ - Public: publicHTTPConfig{ + in: EnvironmentHTTPConfig{ + Public: PublicHTTPConfig{ Certificates: []string{"arn:aws:acm:us-east-1:1111111:certificate/look-like-a-good-arn"}, }, }, }, "success with private cert": { - in: environmentHTTPConfig{ + in: EnvironmentHTTPConfig{ Private: privateHTTPConfig{ Certificates: []string{"arn:aws:acm:us-east-1:1111111:certificate/look-like-a-good-arn"}, }, diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index 15228ca4608..bdb3424bfec 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -97,12 +97,13 @@ type EnvOpts struct { ArtifactBucketARN string ArtifactBucketKeyARN string - VPCConfig VPCConfig - PublicImportedCertARNs []string - PrivateImportedCertARNs []string - CustomInternalALBSubnets []string - AllowVPCIngress bool - Telemetry *Telemetry + VPCConfig VPCConfig + PublicFacingPrefixListIDs []string + PublicImportedCertARNs []string + PrivateImportedCertARNs []string + CustomInternalALBSubnets []string + AllowVPCIngress bool + Telemetry *Telemetry CDNConfig *CDNConfig // If nil, no cdn is to be used diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index b590c748822..00384db9267 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -104,6 +104,15 @@ Resources: Properties: GroupDescription: Access to the public facing load balancer SecurityGroupIngress: +{{- if .PublicFacingPrefixListIDs}} + {{- range $id := .PublicFacingPrefixListIDs}} + - SourcePrefixListId: {{$id}} + Description: Allow ingress from prefix list {{$id}} on all ports + FromPort: 0 + IpProtocol: tcp + ToPort: 0 + {{- end}} +{{- else}} - CidrIp: 0.0.0.0/0 Description: Allow from anyone on port 80 FromPort: 80 @@ -114,6 +123,7 @@ Resources: FromPort: 443 IpProtocol: tcp ToPort: 443 +{{- end}} {{- if .VPCConfig.Imported}} VpcId: {{.VPCConfig.Imported.ID}} {{- else}} From 539f30fb0e6e3fd4c82e9b7c4013de836a80deea Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Fri, 8 Jul 2022 14:31:06 -0700 Subject: [PATCH 14/27] fix prefix list port specification --- internal/pkg/deploy/cloudformation/stack/env.go | 11 +---------- internal/pkg/template/templates/environment/cf.yml | 6 +++--- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index bfb89b4178d..5982b2ba996 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -299,16 +299,7 @@ func (e *BootstrapEnvStackConfig) ToEnv(stack *cloudformation.Stack) (*config.En } func (e *EnvStackConfig) cdnConfig() *template.CDNConfig { - // If a manifest is present, check there, otherwise cdn cannot be enabled - if e.in.Mft != nil { - if !e.in.Mft.CDNConfig.CDNEnabled() { - return nil - } - } else { - return nil - } - - return &template.CDNConfig{} + return nil // no-op - return &template.CDNConfig{} when feature is ready } func (e *EnvStackConfig) vpcConfig() template.VPCConfig { diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index 4579525fe8e..ed625df2224 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -108,9 +108,9 @@ Resources: {{- range $id := .PublicFacingPrefixListIDs}} - SourcePrefixListId: {{$id}} Description: Allow ingress from prefix list {{$id}} on all ports - FromPort: 0 - IpProtocol: tcp - ToPort: 0 + FromPort: -1 + IpProtocol: -1 + ToPort: -1 {{- end}} {{- else}} - CidrIp: 0.0.0.0/0 From 516bf6254321656750286b3b2f1133fd5db62405 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Fri, 8 Jul 2022 15:33:57 -0700 Subject: [PATCH 15/27] split http/https security groups to allow secure port usage with cf --- .../pkg/template/templates/environment/cf.yml | 62 ++++++++++++++----- 1 file changed, 48 insertions(+), 14 deletions(-) diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index ed625df2224..005baa3d4b3 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -96,34 +96,60 @@ Resources: Value: disabled {{- end}} {{- end}} - PublicLoadBalancerSecurityGroup: + PublicHTTPLoadBalancerSecurityGroup: Metadata: - 'aws:copilot:description': 'A security group for your load balancer allowing HTTP and HTTPS traffic' + 'aws:copilot:description': 'A security group for your load balancer allowing HTTP traffic' Condition: CreateALB Type: AWS::EC2::SecurityGroup Properties: - GroupDescription: Access to the public facing load balancer + GroupDescription: HTTP access to the public facing load balancer SecurityGroupIngress: -{{- if .PublicFacingPrefixListIDs}} + {{- if .PublicFacingPrefixListIDs}} {{- range $id := .PublicFacingPrefixListIDs}} - SourcePrefixListId: {{$id}} - Description: Allow ingress from prefix list {{$id}} on all ports - FromPort: -1 - IpProtocol: -1 - ToPort: -1 + Description: Allow ingress from prefix list {{$id}} on port 80 + FromPort: 80 + IpProtocol: tcp + ToPort: 80 {{- end}} -{{- else}} + {{- else}} - CidrIp: 0.0.0.0/0 Description: Allow from anyone on port 80 FromPort: 80 IpProtocol: tcp ToPort: 80 + {{- end}} +{{- if .VPCConfig.Imported}} + VpcId: {{.VPCConfig.Imported.ID}} +{{- else}} + VpcId: !Ref VPC +{{- end}} + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-lb' + PublicHTTPSLoadBalancerSecurityGroup: + Metadata: + 'aws:copilot:description': 'A security group for your load balancer allowing HTTPS traffic' + Condition: CreateALB + Type: AWS::EC2::SecurityGroup + Properties: + GroupDescription: HTTPS access to the public facing load balancer + SecurityGroupIngress: + {{- if .PublicFacingPrefixListIDs}} + {{- range $id := .PublicFacingPrefixListIDs}} + - SourcePrefixListId: {{$id}} + Description: Allow ingress from prefix list {{$id}} on port 443 + FromPort: 443 + IpProtocol: tcp + ToPort: 443 + {{- end}} + {{- else}} - CidrIp: 0.0.0.0/0 Description: Allow from anyone on port 443 FromPort: 443 IpProtocol: tcp ToPort: 443 -{{- end}} + {{- end}} {{- if .VPCConfig.Imported}} VpcId: {{.VPCConfig.Imported.ID}} {{- else}} @@ -162,14 +188,22 @@ Resources: Tags: - Key: Name Value: !Sub 'copilot-${AppName}-${EnvironmentName}-env' - EnvironmentSecurityGroupIngressFromPublicALB: + EnvironmentHTTPSecurityGroupIngressFromPublicALB: + Type: AWS::EC2::SecurityGroupIngress + Condition: CreateALB + Properties: + Description: HTTP ingress from the public ALB + GroupId: !Ref EnvironmentSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref PublicHTTPLoadBalancerSecurityGroup + EnvironmentHTTPSSecurityGroupIngressFromPublicALB: Type: AWS::EC2::SecurityGroupIngress Condition: CreateALB Properties: - Description: Ingress from the public ALB + Description: HTTPS ingress from the public ALB GroupId: !Ref EnvironmentSecurityGroup IpProtocol: -1 - SourceSecurityGroupId: !Ref PublicLoadBalancerSecurityGroup + SourceSecurityGroupId: !Ref PublicHTTPSLoadBalancerSecurityGroup EnvironmentSecurityGroupIngressFromInternalALB: Type: AWS::EC2::SecurityGroupIngress Condition: CreateInternalALB @@ -226,7 +260,7 @@ Resources: Type: AWS::ElasticLoadBalancingV2::LoadBalancer Properties: Scheme: internet-facing - SecurityGroups: [ !GetAtt PublicLoadBalancerSecurityGroup.GroupId ] + SecurityGroups: [ !GetAtt PublicHTTPLoadBalancerSecurityGroup.GroupId, !GetAtt PublicHTTPSLoadBalancerSecurityGroup.GroupId] {{- if .VPCConfig.Imported}} Subnets: [ {{range $id := .VPCConfig.Imported.PublicSubnetIDs}}{{$id}}, {{end}} ] {{- else}} From 1c9dfa75b33a703d593f0b7ae02cfe939cd77d5f Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Fri, 8 Jul 2022 16:09:36 -0700 Subject: [PATCH 16/27] add mocks --- internal/pkg/cli/deploy/mocks/mock_env.go | 77 +++++++++++------------ 1 file changed, 38 insertions(+), 39 deletions(-) diff --git a/internal/pkg/cli/deploy/mocks/mock_env.go b/internal/pkg/cli/deploy/mocks/mock_env.go index 8501f3681a3..0404f38e00e 100644 --- a/internal/pkg/cli/deploy/mocks/mock_env.go +++ b/internal/pkg/cli/deploy/mocks/mock_env.go @@ -8,7 +8,6 @@ import ( reflect "reflect" cloudformation "github.com/aws/copilot-cli/internal/pkg/aws/cloudformation" - s3 "github.com/aws/copilot-cli/internal/pkg/aws/s3" config "github.com/aws/copilot-cli/internal/pkg/config" deploy "github.com/aws/copilot-cli/internal/pkg/deploy" stack "github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation/stack" @@ -16,44 +15,6 @@ import ( gomock "github.com/golang/mock/gomock" ) -// MockcustomResourcesUploader is a mock of customResourcesUploader interface. -type MockcustomResourcesUploader struct { - ctrl *gomock.Controller - recorder *MockcustomResourcesUploaderMockRecorder -} - -// MockcustomResourcesUploaderMockRecorder is the mock recorder for MockcustomResourcesUploader. -type MockcustomResourcesUploaderMockRecorder struct { - mock *MockcustomResourcesUploader -} - -// NewMockcustomResourcesUploader creates a new mock instance. -func NewMockcustomResourcesUploader(ctrl *gomock.Controller) *MockcustomResourcesUploader { - mock := &MockcustomResourcesUploader{ctrl: ctrl} - mock.recorder = &MockcustomResourcesUploaderMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockcustomResourcesUploader) EXPECT() *MockcustomResourcesUploaderMockRecorder { - return m.recorder -} - -// UploadEnvironmentCustomResources mocks base method. -func (m *MockcustomResourcesUploader) UploadEnvironmentCustomResources(upload s3.CompressAndUploadFunc) (map[string]string, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "UploadEnvironmentCustomResources", upload) - ret0, _ := ret[0].(map[string]string) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// UploadEnvironmentCustomResources indicates an expected call of UploadEnvironmentCustomResources. -func (mr *MockcustomResourcesUploaderMockRecorder) UploadEnvironmentCustomResources(upload interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UploadEnvironmentCustomResources", reflect.TypeOf((*MockcustomResourcesUploader)(nil).UploadEnvironmentCustomResources), upload) -} - // MockappResourcesGetter is a mock of appResourcesGetter interface. type MockappResourcesGetter struct { ctrl *gomock.Controller @@ -133,3 +94,41 @@ func (mr *MockenvironmentDeployerMockRecorder) UpdateAndRenderEnvironment(out, e varargs := append([]interface{}{out, env}, opts...) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateAndRenderEnvironment", reflect.TypeOf((*MockenvironmentDeployer)(nil).UpdateAndRenderEnvironment), varargs...) } + +// MockprefixListGetter is a mock of prefixListGetter interface. +type MockprefixListGetter struct { + ctrl *gomock.Controller + recorder *MockprefixListGetterMockRecorder +} + +// MockprefixListGetterMockRecorder is the mock recorder for MockprefixListGetter. +type MockprefixListGetterMockRecorder struct { + mock *MockprefixListGetter +} + +// NewMockprefixListGetter creates a new mock instance. +func NewMockprefixListGetter(ctrl *gomock.Controller) *MockprefixListGetter { + mock := &MockprefixListGetter{ctrl: ctrl} + mock.recorder = &MockprefixListGetterMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockprefixListGetter) EXPECT() *MockprefixListGetterMockRecorder { + return m.recorder +} + +// CloudFrontManagedPrefixListID mocks base method. +func (m *MockprefixListGetter) CloudFrontManagedPrefixListID() (*string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CloudFrontManagedPrefixListID") + ret0, _ := ret[0].(*string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// CloudFrontManagedPrefixListID indicates an expected call of CloudFrontManagedPrefixListID. +func (mr *MockprefixListGetterMockRecorder) CloudFrontManagedPrefixListID() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CloudFrontManagedPrefixListID", reflect.TypeOf((*MockprefixListGetter)(nil).CloudFrontManagedPrefixListID)) +} From 45bdcc3549aa7a85ec3f08170ee44e5006ebe323 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Fri, 8 Jul 2022 16:47:34 -0700 Subject: [PATCH 17/27] fix env integ test ymls --- .../template-with-basic-manifest.yml | 34 +++++++++++++++---- ...late-with-imported-certs-observability.yml | 34 +++++++++++++++---- 2 files changed, 54 insertions(+), 14 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-basic-manifest.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-basic-manifest.yml index d8b0d497368..8cfbcfcc30a 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-basic-manifest.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-basic-manifest.yml @@ -570,19 +570,31 @@ Resources: ClusterSettings: - Name: containerInsights Value: disabled - PublicLoadBalancerSecurityGroup: + PublicHTTPLoadBalancerSecurityGroup: Metadata: - 'aws:copilot:description': 'A security group for your load balancer allowing HTTP and HTTPS traffic' + 'aws:copilot:description': 'A security group for your load balancer allowing HTTP traffic' Condition: CreateALB Type: AWS::EC2::SecurityGroup Properties: - GroupDescription: Access to the public facing load balancer + GroupDescription: HTTP access to the public facing load balancer SecurityGroupIngress: - CidrIp: 0.0.0.0/0 Description: Allow from anyone on port 80 FromPort: 80 IpProtocol: tcp ToPort: 80 + VpcId: !Ref VPC + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-lb' + PublicHTTPSLoadBalancerSecurityGroup: + Metadata: + 'aws:copilot:description': 'A security group for your load balancer allowing HTTPS traffic' + Condition: CreateALB + Type: AWS::EC2::SecurityGroup + Properties: + GroupDescription: HTTPS access to the public facing load balancer + SecurityGroupIngress: - CidrIp: 0.0.0.0/0 Description: Allow from anyone on port 443 FromPort: 443 @@ -614,14 +626,22 @@ Resources: Tags: - Key: Name Value: !Sub 'copilot-${AppName}-${EnvironmentName}-env' - EnvironmentSecurityGroupIngressFromPublicALB: + EnvironmentHTTPSecurityGroupIngressFromPublicALB: + Type: AWS::EC2::SecurityGroupIngress + Condition: CreateALB + Properties: + Description: HTTP ingress from the public ALB + GroupId: !Ref EnvironmentSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref PublicHTTPLoadBalancerSecurityGroup + EnvironmentHTTPSSecurityGroupIngressFromPublicALB: Type: AWS::EC2::SecurityGroupIngress Condition: CreateALB Properties: - Description: Ingress from the public ALB + Description: HTTPS ingress from the public ALB GroupId: !Ref EnvironmentSecurityGroup IpProtocol: -1 - SourceSecurityGroupId: !Ref PublicLoadBalancerSecurityGroup + SourceSecurityGroupId: !Ref PublicHTTPSLoadBalancerSecurityGroup EnvironmentSecurityGroupIngressFromInternalALB: Type: AWS::EC2::SecurityGroupIngress Condition: CreateInternalALB @@ -676,7 +696,7 @@ Resources: Type: AWS::ElasticLoadBalancingV2::LoadBalancer Properties: Scheme: internet-facing - SecurityGroups: [ !GetAtt PublicLoadBalancerSecurityGroup.GroupId ] + SecurityGroups: [ !GetAtt PublicHTTPLoadBalancerSecurityGroup.GroupId, !GetAtt PublicHTTPSLoadBalancerSecurityGroup.GroupId] Subnets: [ !Ref PublicSubnet1, !Ref PublicSubnet2, ] Type: application # Assign a dummy target group that with no real services as targets, so that we can create diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml index 7d1cd3078f7..0e0e0e0fc96 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml @@ -253,19 +253,31 @@ Resources: ClusterSettings: - Name: containerInsights Value: enabled - PublicLoadBalancerSecurityGroup: + PublicHTTPLoadBalancerSecurityGroup: Metadata: - 'aws:copilot:description': 'A security group for your load balancer allowing HTTP and HTTPS traffic' + 'aws:copilot:description': 'A security group for your load balancer allowing HTTP traffic' Condition: CreateALB Type: AWS::EC2::SecurityGroup Properties: - GroupDescription: Access to the public facing load balancer + GroupDescription: HTTP access to the public facing load balancer SecurityGroupIngress: - CidrIp: 0.0.0.0/0 Description: Allow from anyone on port 80 FromPort: 80 IpProtocol: tcp ToPort: 80 + VpcId: !Ref VPC + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-lb' + PublicHTTPSLoadBalancerSecurityGroup: + Metadata: + 'aws:copilot:description': 'A security group for your load balancer allowing HTTPS traffic' + Condition: CreateALB + Type: AWS::EC2::SecurityGroup + Properties: + GroupDescription: HTTPS access to the public facing load balancer + SecurityGroupIngress: - CidrIp: 0.0.0.0/0 Description: Allow from anyone on port 443 FromPort: 443 @@ -297,14 +309,22 @@ Resources: Tags: - Key: Name Value: !Sub 'copilot-${AppName}-${EnvironmentName}-env' - EnvironmentSecurityGroupIngressFromPublicALB: + EnvironmentHTTPSecurityGroupIngressFromPublicALB: + Type: AWS::EC2::SecurityGroupIngress + Condition: CreateALB + Properties: + Description: HTTP ingress from the public ALB + GroupId: !Ref EnvironmentSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref PublicHTTPLoadBalancerSecurityGroup + EnvironmentHTTPSSecurityGroupIngressFromPublicALB: Type: AWS::EC2::SecurityGroupIngress Condition: CreateALB Properties: - Description: Ingress from the public ALB + Description: HTTPS ingress from the public ALB GroupId: !Ref EnvironmentSecurityGroup IpProtocol: -1 - SourceSecurityGroupId: !Ref PublicLoadBalancerSecurityGroup + SourceSecurityGroupId: !Ref PublicHTTPSLoadBalancerSecurityGroup EnvironmentSecurityGroupIngressFromInternalALB: Type: AWS::EC2::SecurityGroupIngress Condition: CreateInternalALB @@ -359,7 +379,7 @@ Resources: Type: AWS::ElasticLoadBalancingV2::LoadBalancer Properties: Scheme: internet-facing - SecurityGroups: [ !GetAtt PublicLoadBalancerSecurityGroup.GroupId ] + SecurityGroups: [ !GetAtt PublicHTTPLoadBalancerSecurityGroup.GroupId, !GetAtt PublicHTTPSLoadBalancerSecurityGroup.GroupId] Subnets: [ !Ref PublicSubnet1, !Ref PublicSubnet2, ] Type: application # Assign a dummy target group that with no real services as targets, so that we can create From ede37384ff578129d47144991e9e1e0a2ba5d135 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Mon, 11 Jul 2022 14:06:12 -0700 Subject: [PATCH 18/27] implement iamhopaul123's feedback --- internal/pkg/cli/deploy/env.go | 6 +----- internal/pkg/cli/deploy/env_test.go | 14 +++++++++++++- .../stack/env_integration_test.go | 5 ++++- ...late-with-imported-certs-observability.yml | 11 ++++++----- internal/pkg/manifest/env.go | 19 +++++++++++++++---- internal/pkg/manifest/validate_env.go | 9 +++++++-- internal/pkg/manifest/validate_env_test.go | 4 +++- 7 files changed, 49 insertions(+), 19 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 0ca773550e5..524b92753b2 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -129,11 +129,7 @@ func (d *envDeployer) prefixLists(in *DeployEnvironmentInput) ([]string, error) func (d *envDeployer) cfManagedPrefixListId(in *DeployEnvironmentInput) (*string, error) { // Check if ingress is allowed from cloudfront - if in.Manifest != nil { - if !aws.BoolValue(in.Manifest.HTTPConfig.Public.LimitToCFIngress) { - return nil, nil - } - } else { + if in.Manifest == nil || !aws.BoolValue(in.Manifest.HTTPConfig.Public.Ingress.CDNIngress) { return nil, nil } diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 481f527badc..2dfd6f3095e 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -158,13 +158,25 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { EnvironmentConfig: manifest.EnvironmentConfig{ HTTPConfig: manifest.EnvironmentHTTPConfig{ Public: manifest.PublicHTTPConfig{ - LimitToCFIngress: aws.Bool(true), + Ingress: manifest.Ingress{ + CDNIngress: aws.Bool(true), + }, }, }, }, }, 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{ + S3Bucket: "mockS3Bucket", + }, nil) + m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return(aws.String("mockPrefixListID"), nil).Times(0) + m.envDeployer.EXPECT().UpdateAndRenderEnvironment(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + }, + inManifest: nil, + }, "fail to deploy environment": { setUpMocks: func(m *deployEnvironmentMock) { m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ diff --git a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go index f3df5d0ebfb..e61e26aebc5 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go @@ -32,9 +32,11 @@ name: test type: Environment # Create the public ALB with certificates attached. # All these comments should be deleted. +cdn: true http: public: - restrict_alb_ingress_to_cf: false + ingress: + from_cdn: true certificates: - cert-1 - cert-2 @@ -49,6 +51,7 @@ observability: Name: "demo", }, Name: "test", + PrefixListIDs: []string{"pl-mockid"}, ArtifactBucketARN: "arn:aws:s3:::mockbucket", ArtifactBucketKeyARN: "arn:aws:kms:us-west-2:000000000:key/1234abcd-12ab-34cd-56ef-1234567890ab", CustomResourcesURLs: map[string]string{ diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml index 0e0e0e0fc96..3055d692f8b 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml @@ -6,7 +6,8 @@ Metadata: name: test type: Environment observability: {container_insights: true} - http: {public: {restrict_alb_ingress_to_cf: false, certificates: [cert-1, cert-2]}} + http: {public: {ingress: {from_cdn: true}, certificates: [cert-1, cert-2]}} + cdn: {enabled: true, cdnconfig: {}} Parameters: AppName: @@ -261,8 +262,8 @@ Resources: Properties: GroupDescription: HTTP access to the public facing load balancer SecurityGroupIngress: - - CidrIp: 0.0.0.0/0 - Description: Allow from anyone on port 80 + - SourcePrefixListId: pl-mockid + Description: Allow ingress from prefix list pl-mockid on port 80 FromPort: 80 IpProtocol: tcp ToPort: 80 @@ -278,8 +279,8 @@ Resources: Properties: GroupDescription: HTTPS access to the public facing load balancer SecurityGroupIngress: - - CidrIp: 0.0.0.0/0 - Description: Allow from anyone on port 443 + - SourcePrefixListId: pl-mockid + Description: Allow ingress from prefix list pl-mockid on port 443 FromPort: 443 IpProtocol: tcp ToPort: 443 diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 1e57e80e9ce..85b3b8aa73a 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -299,7 +299,7 @@ func (o *environmentObservability) loadObsConfig(tele *config.Telemetry) { o.ContainerInsights = &tele.EnableContainerInsights } -// EnvironmentHTTPConfig defines the configuration settings for an environment group's HTTP connections +// EnvironmentHTTPConfig defines the configuration settings for an environment group's HTTP connections. type EnvironmentHTTPConfig struct { Public PublicHTTPConfig `yaml:"public,omitempty"` Private privateHTTPConfig `yaml:"private,omitempty"` @@ -322,14 +322,25 @@ func (cfg *EnvironmentHTTPConfig) loadLBConfig(env *config.CustomizeEnv) { cfg.Public.Certificates = env.ImportCertARNs } +// PublicHTTPConfig represents the configuration settings for an environment public ALB. type PublicHTTPConfig struct { - LimitToCFIngress *bool `yaml:"restrict_alb_ingress_to_cf"` - Certificates []string `yaml:"certificates,omitempty"` + Ingress Ingress `yaml:"ingress"` + Certificates []string `yaml:"certificates,omitempty"` +} + +// Ingress represents allowed ingress traffic from specified fields. +type Ingress struct { + CDNIngress *bool `yaml:"from_cdn"` +} + +// IsEmpty returns true if there is are no specified fields for ingress. +func (i Ingress) IsEmpty() bool { + return i.CDNIngress == nil } // IsEmpty returns true if there is no customization to the public ALB. func (cfg PublicHTTPConfig) IsEmpty() bool { - return len(cfg.Certificates) == 0 && cfg.LimitToCFIngress == nil + return len(cfg.Certificates) == 0 && cfg.Ingress.IsEmpty() } type privateHTTPConfig struct { diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 8ce52574498..5fcc0e8e6f4 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -37,7 +37,7 @@ func (e EnvironmentConfig) Validate() error { return fmt.Errorf(`validate "http config": %w`, err) } - if aws.BoolValue(e.HTTPConfig.Public.LimitToCFIngress) && !e.CDNConfig.CDNEnabled() { + if aws.BoolValue(e.HTTPConfig.Public.Ingress.CDNIngress) && !e.CDNConfig.CDNEnabled() { return errors.New("CDN must be enabled to limit security group ingress to CloudFront") } @@ -199,7 +199,7 @@ func (cfg PublicHTTPConfig) Validate() error { return fmt.Errorf(`parse "certificates[%d]": %w`, idx, err) } } - return nil + return cfg.Ingress.Validate() } // Validate returns nil if privateHTTPConfig is configured correctly. @@ -220,6 +220,11 @@ func (cfg environmentCDNConfig) Validate() error { return cfg.CDNConfig.Validate() } +// Validate is a no-op for Ingress. +func (i Ingress) Validate() error { + return nil +} + // Validate is a no-op for AdvancedCDNConfig. func (cfg advancedCDNConfig) Validate() error { return nil diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index e87184d2ba1..b4886a7f1a7 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -99,7 +99,9 @@ func TestEnvironmentConfig_Validate(t *testing.T) { }, HTTPConfig: EnvironmentHTTPConfig{ Public: PublicHTTPConfig{ - LimitToCFIngress: aws.Bool(true), + Ingress: Ingress{ + CDNIngress: aws.Bool(true), + }, }, }, }, From e87bd63b9816e3587608e6fcdf78d2123bb18c85 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Fri, 15 Jul 2022 13:18:42 -0700 Subject: [PATCH 19/27] address efekarakus feedback --- internal/pkg/cli/deploy/env.go | 6 ++--- internal/pkg/cli/deploy/env_test.go | 6 +++-- .../pkg/deploy/cloudformation/stack/env.go | 26 +++++++++---------- .../stack/env_integration_test.go | 2 +- .../template-with-basic-manifest.yml | 8 +++--- ...late-with-imported-certs-observability.yml | 8 +++--- internal/pkg/deploy/env.go | 2 +- internal/pkg/manifest/env.go | 17 +++++++++--- internal/pkg/manifest/validate_env.go | 7 ++++- internal/pkg/manifest/validate_env_test.go | 6 +++-- internal/pkg/template/env.go | 14 +++++----- .../pkg/template/templates/environment/cf.yml | 16 +++++++----- 12 files changed, 71 insertions(+), 47 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index a1db07e1a9a..d5bd57813bd 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -132,7 +132,7 @@ func (d *envDeployer) prefixLists(in *DeployEnvironmentInput) ([]string, error) func (d *envDeployer) cfManagedPrefixListId(in *DeployEnvironmentInput) (*string, error) { // Check if ingress is allowed from cloudfront - if in.Manifest == nil || !aws.BoolValue(in.Manifest.HTTPConfig.Public.Ingress.CDNIngress) { + if in.Manifest == nil || !aws.BoolValue(in.Manifest.HTTPConfig.Public.SecurityGroupConfig.Ingress.CDNIngress) { return nil, nil } @@ -208,7 +208,7 @@ func (d *envDeployer) buildStackInput(in *DeployEnvironmentInput) (*deploy.Creat if err != nil { return nil, err } - prefixListIDs, err := d.prefixLists(in) + cidrPrefixListIDs, err := d.prefixLists(in) if err != nil { return nil, err } @@ -223,7 +223,7 @@ func (d *envDeployer) buildStackInput(in *DeployEnvironmentInput) (*deploy.Creat CustomResourcesURLs: in.CustomResourcesURLs, ArtifactBucketARN: s3.FormatARN(partition.ID(), resources.S3Bucket), ArtifactBucketKeyARN: resources.KMSKeyARN, - PrefixListIDs: prefixListIDs, + CIDRPrefixListIDs: cidrPrefixListIDs, Mft: in.Manifest, RawMft: in.RawManifest, Version: deploy.LatestEnvTemplateVersion, diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 06668183051..385ea3a164a 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -261,8 +261,10 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { EnvironmentConfig: manifest.EnvironmentConfig{ HTTPConfig: manifest.EnvironmentHTTPConfig{ Public: manifest.PublicHTTPConfig{ - Ingress: manifest.Ingress{ - CDNIngress: aws.Bool(true), + SecurityGroupConfig: manifest.ALBSecurityGroupsConfig{ + Ingress: manifest.Ingress{ + CDNIngress: aws.Bool(true), + }, }, }, }, diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index ea3d3dab786..2d0736d327f 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -86,19 +86,19 @@ func (e *EnvStackConfig) Template() (string, error) { } content, err := e.parser.ParseEnv(&template.EnvOpts{ - AppName: e.in.App.Name, - EnvName: e.in.Name, - CustomResources: crs, - ArtifactBucketARN: e.in.ArtifactBucketARN, - ArtifactBucketKeyARN: e.in.ArtifactBucketKeyARN, - PublicFacingPrefixListIDs: e.in.PrefixListIDs, - PublicImportedCertARNs: e.importPublicCertARNs(), - PrivateImportedCertARNs: e.importPrivateCertARNs(), - VPCConfig: e.vpcConfig(), - CustomInternalALBSubnets: e.internalALBSubnets(), - AllowVPCIngress: e.in.AllowVPCIngress, // TODO(jwh): fetch AllowVPCIngress from Manifest or SSM. - Telemetry: e.telemetryConfig(), - CDNConfig: e.cdnConfig(), + AppName: e.in.App.Name, + EnvName: e.in.Name, + CustomResources: crs, + ArtifactBucketARN: e.in.ArtifactBucketARN, + ArtifactBucketKeyARN: e.in.ArtifactBucketKeyARN, + PublicFacingCIDRPrefixListIDs: e.in.CIDRPrefixListIDs, + PublicImportedCertARNs: e.importPublicCertARNs(), + PrivateImportedCertARNs: e.importPrivateCertARNs(), + VPCConfig: e.vpcConfig(), + CustomInternalALBSubnets: e.internalALBSubnets(), + AllowVPCIngress: e.in.AllowVPCIngress, // TODO(jwh): fetch AllowVPCIngress from Manifest or SSM. + Telemetry: e.telemetryConfig(), + CDNConfig: e.cdnConfig(), Version: e.in.Version, LatestVersion: deploy.LatestEnvTemplateVersion, diff --git a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go index fb8baa85570..ec2228425f9 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go @@ -51,7 +51,7 @@ observability: Name: "demo", }, Name: "test", - PrefixListIDs: []string{"pl-mockid"}, + CIDRPrefixListIDs: []string{"pl-mockid"}, ArtifactBucketARN: "arn:aws:s3:::mockbucket", ArtifactBucketKeyARN: "arn:aws:kms:us-west-2:000000000:key/1234abcd-12ab-34cd-56ef-1234567890ab", CustomResourcesURLs: map[string]string{ diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-basic-manifest.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-basic-manifest.yml index 710cbd102fc..21faf079cab 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-basic-manifest.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-basic-manifest.yml @@ -589,7 +589,7 @@ Resources: PublicHTTPSLoadBalancerSecurityGroup: Metadata: 'aws:copilot:description': 'A security group for your load balancer allowing HTTPS traffic' - Condition: CreateALB + Condition: ExportHTTPSListener Type: AWS::EC2::SecurityGroup Properties: GroupDescription: HTTPS access to the public facing load balancer @@ -635,7 +635,7 @@ Resources: SourceSecurityGroupId: !Ref PublicHTTPLoadBalancerSecurityGroup EnvironmentHTTPSSecurityGroupIngressFromPublicALB: Type: AWS::EC2::SecurityGroupIngress - Condition: CreateALB + Condition: ExportHTTPSListener Properties: Description: HTTPS ingress from the public ALB GroupId: !Ref EnvironmentSecurityGroup @@ -695,7 +695,9 @@ Resources: Type: AWS::ElasticLoadBalancingV2::LoadBalancer Properties: Scheme: internet-facing - SecurityGroups: [ !GetAtt PublicHTTPLoadBalancerSecurityGroup.GroupId, !GetAtt PublicHTTPSLoadBalancerSecurityGroup.GroupId] + SecurityGroups: + - !GetAtt PublicHTTPLoadBalancerSecurityGroup.GroupId + - !If [ExportHTTPSListener, !GetAtt PublicHTTPSLoadBalancerSecurityGroup.GroupId, !Ref "AWS::NoValue"] Subnets: [ !Ref PublicSubnet1, !Ref PublicSubnet2, ] Type: application # Assign a dummy target group that with no real services as targets, so that we can create diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml index 056d31ff664..ad1353bfd7e 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml @@ -282,7 +282,7 @@ Resources: PublicHTTPSLoadBalancerSecurityGroup: Metadata: 'aws:copilot:description': 'A security group for your load balancer allowing HTTPS traffic' - Condition: CreateALB + Condition: ExportHTTPSListener Type: AWS::EC2::SecurityGroup Properties: GroupDescription: HTTPS access to the public facing load balancer @@ -328,7 +328,7 @@ Resources: SourceSecurityGroupId: !Ref PublicHTTPLoadBalancerSecurityGroup EnvironmentHTTPSSecurityGroupIngressFromPublicALB: Type: AWS::EC2::SecurityGroupIngress - Condition: CreateALB + Condition: ExportHTTPSListener Properties: Description: HTTPS ingress from the public ALB GroupId: !Ref EnvironmentSecurityGroup @@ -388,7 +388,9 @@ Resources: Type: AWS::ElasticLoadBalancingV2::LoadBalancer Properties: Scheme: internet-facing - SecurityGroups: [ !GetAtt PublicHTTPLoadBalancerSecurityGroup.GroupId, !GetAtt PublicHTTPSLoadBalancerSecurityGroup.GroupId] + SecurityGroups: + - !GetAtt PublicHTTPLoadBalancerSecurityGroup.GroupId + - !If [ExportHTTPSListener, !GetAtt PublicHTTPSLoadBalancerSecurityGroup.GroupId, !Ref "AWS::NoValue"] Subnets: [ !Ref PublicSubnet1, !Ref PublicSubnet2, ] Type: application # Assign a dummy target group that with no real services as targets, so that we can create diff --git a/internal/pkg/deploy/env.go b/internal/pkg/deploy/env.go index e61ef28d457..e4284270f5d 100644 --- a/internal/pkg/deploy/env.go +++ b/internal/pkg/deploy/env.go @@ -38,7 +38,7 @@ type CreateEnvironmentInput struct { ImportCertARNs []string // Optional configuration if users want to import certificates. InternalALBSubnets []string // Optional configuration if users want to specify internal ALB placement. AllowVPCIngress bool // Optional configuration to allow access to internal ALB from ports 80/443. - PrefixListIDs []string // Optional configuration to specify public security group ingress based on prefix lists + CIDRPrefixListIDs []string // Optional configuration to specify public security group ingress based on prefix lists Telemetry *config.Telemetry // Optional observability and monitoring configuration. Mft *manifest.Environment // Unmarshaled and interpolated manifest object. RawMft []byte // Content of the environment manifest without any modifications. diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index b7f05ee37c6..92f7781d1f4 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -328,15 +328,24 @@ func (cfg *EnvironmentHTTPConfig) loadLBConfig(env *config.CustomizeEnv) { cfg.Public.Certificates = env.ImportCertARNs } +// ALBSecurityGroupsConfig represents security group configuration settings for an ALB. +type ALBSecurityGroupsConfig struct { + Ingress Ingress `yaml:"ingress"` +} + +func (cfg ALBSecurityGroupsConfig) IsEmpty() bool { + return cfg.Ingress.IsEmpty() +} + // PublicHTTPConfig represents the configuration settings for an environment public ALB. type PublicHTTPConfig struct { - Ingress Ingress `yaml:"ingress"` - Certificates []string `yaml:"certificates,omitempty"` + SecurityGroupConfig ALBSecurityGroupsConfig `yaml:"security_groups,omitempty"` + Certificates []string `yaml:"certificates,omitempty"` } // Ingress represents allowed ingress traffic from specified fields. type Ingress struct { - CDNIngress *bool `yaml:"from_cdn"` + CDNIngress *bool `yaml:"restrict_from_cdn"` } // IsEmpty returns true if there is are no specified fields for ingress. @@ -346,7 +355,7 @@ func (i Ingress) IsEmpty() bool { // IsEmpty returns true if there is no customization to the public ALB. func (cfg PublicHTTPConfig) IsEmpty() bool { - return len(cfg.Certificates) == 0 && cfg.Ingress.IsEmpty() + return len(cfg.Certificates) == 0 && cfg.SecurityGroupConfig.IsEmpty() } type privateHTTPConfig struct { diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 7300ee507fa..d1b1371aa48 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -37,7 +37,7 @@ func (e EnvironmentConfig) Validate() error { return fmt.Errorf(`validate "http config": %w`, err) } - if aws.BoolValue(e.HTTPConfig.Public.Ingress.CDNIngress) && !e.CDNConfig.CDNEnabled() { + if aws.BoolValue(e.HTTPConfig.Public.SecurityGroupConfig.Ingress.CDNIngress) && !e.CDNConfig.CDNEnabled() { return errors.New("CDN must be enabled to limit security group ingress to CloudFront") } @@ -217,6 +217,11 @@ func (cfg PublicHTTPConfig) Validate() error { return fmt.Errorf(`parse "certificates[%d]": %w`, idx, err) } } + return cfg.SecurityGroupConfig.Validate() +} + +// Validate returns nil if ALBSecurityGroupsConfig is configured correctly. +func (cfg ALBSecurityGroupsConfig) Validate() error { return cfg.Ingress.Validate() } diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index 5f4644e2249..c4d629c3b4b 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -99,8 +99,10 @@ func TestEnvironmentConfig_Validate(t *testing.T) { }, HTTPConfig: EnvironmentHTTPConfig{ Public: PublicHTTPConfig{ - Ingress: Ingress{ - CDNIngress: aws.Bool(true), + SecurityGroupConfig: ALBSecurityGroupsConfig{ + Ingress: Ingress{ + CDNIngress: aws.Bool(true), + }, }, }, }, diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index b6c0660c0b5..2c3933f1ddd 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -97,13 +97,13 @@ type EnvOpts struct { ArtifactBucketARN string ArtifactBucketKeyARN string - VPCConfig VPCConfig - PublicFacingPrefixListIDs []string - PublicImportedCertARNs []string - PrivateImportedCertARNs []string - CustomInternalALBSubnets []string - AllowVPCIngress bool - Telemetry *Telemetry + VPCConfig VPCConfig + PublicFacingCIDRPrefixListIDs []string + PublicImportedCertARNs []string + PrivateImportedCertARNs []string + CustomInternalALBSubnets []string + AllowVPCIngress bool + Telemetry *Telemetry CDNConfig *CDNConfig // If nil, no cdn is to be used diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index 005baa3d4b3..e9d63eb67cd 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -104,8 +104,8 @@ Resources: Properties: GroupDescription: HTTP access to the public facing load balancer SecurityGroupIngress: - {{- if .PublicFacingPrefixListIDs}} - {{- range $id := .PublicFacingPrefixListIDs}} + {{- if .PublicFacingCIDRPrefixListIDs}} + {{- range $id := .PublicFacingCIDRPrefixListIDs}} - SourcePrefixListId: {{$id}} Description: Allow ingress from prefix list {{$id}} on port 80 FromPort: 80 @@ -130,13 +130,13 @@ Resources: PublicHTTPSLoadBalancerSecurityGroup: Metadata: 'aws:copilot:description': 'A security group for your load balancer allowing HTTPS traffic' - Condition: CreateALB + Condition: ExportHTTPSListener Type: AWS::EC2::SecurityGroup Properties: GroupDescription: HTTPS access to the public facing load balancer SecurityGroupIngress: - {{- if .PublicFacingPrefixListIDs}} - {{- range $id := .PublicFacingPrefixListIDs}} + {{- if .PublicFacingCIDRPrefixListIDs}} + {{- range $id := .PublicFacingCIDRPrefixListIDs}} - SourcePrefixListId: {{$id}} Description: Allow ingress from prefix list {{$id}} on port 443 FromPort: 443 @@ -198,7 +198,7 @@ Resources: SourceSecurityGroupId: !Ref PublicHTTPLoadBalancerSecurityGroup EnvironmentHTTPSSecurityGroupIngressFromPublicALB: Type: AWS::EC2::SecurityGroupIngress - Condition: CreateALB + Condition: ExportHTTPSListener Properties: Description: HTTPS ingress from the public ALB GroupId: !Ref EnvironmentSecurityGroup @@ -260,7 +260,9 @@ Resources: Type: AWS::ElasticLoadBalancingV2::LoadBalancer Properties: Scheme: internet-facing - SecurityGroups: [ !GetAtt PublicHTTPLoadBalancerSecurityGroup.GroupId, !GetAtt PublicHTTPSLoadBalancerSecurityGroup.GroupId] + SecurityGroups: + - !GetAtt PublicHTTPLoadBalancerSecurityGroup.GroupId + - !If [ExportHTTPSListener, !GetAtt PublicHTTPSLoadBalancerSecurityGroup.GroupId, !Ref "AWS::NoValue"] {{- if .VPCConfig.Imported}} Subnets: [ {{range $id := .VPCConfig.Imported.PublicSubnetIDs}}{{$id}}, {{end}} ] {{- else}} From 972822ec37d7c86f9e23e995dda9861175bbaa27 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Fri, 15 Jul 2022 13:39:58 -0700 Subject: [PATCH 20/27] fix env_test conflict manually --- internal/pkg/cli/deploy/env_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 7572d14aca0..9e3ad92aebf 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -18,7 +18,6 @@ import ( "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/manifest" - "github.com/aws/copilot-cli/internal/pkg/term/progress" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" ) @@ -289,10 +288,13 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) + m.envDeployer.EXPECT().EnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) + m.envDeployer.EXPECT().ForceUpdateOutputID(gomock.Any(), gomock.Any()).Return("", nil) m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return(aws.String("mockPrefixListID"), nil).Times(0) - m.envDeployer.EXPECT().UpdateAndRenderEnvironment(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + m.envDeployer.EXPECT().UpdateAndRenderEnvironment(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) }, inManifest: nil, + }, "fail to get existing parameters": { setUpMocks: func(m *deployEnvironmentMock) { m.appCFN.EXPECT().GetAppResourcesByRegion(gomock.Any(), gomock.Any()).Return(&stack.AppRegionalResources{ From 54d808afd28770f0fd87ca607c2bddbca72c5f7b Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Fri, 22 Jul 2022 09:09:24 -0700 Subject: [PATCH 21/27] fix local integ test --- internal/pkg/deploy/cloudformation/stack/env.go | 2 +- .../cloudformation/stack/env_integration_test.go | 4 ++-- ...template-with-imported-certs-observability.yml | 4 ++-- internal/pkg/manifest/env.go | 15 ++++----------- internal/pkg/manifest/env_test.go | 6 +++--- internal/pkg/manifest/validate_env.go | 5 +++++ 6 files changed, 17 insertions(+), 19 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index a3e9e137d90..43770cd1c8f 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -106,7 +106,7 @@ func (e *EnvStackConfig) Template() (string, error) { PrivateImportedCertARNs: e.importPrivateCertARNs(), VPCConfig: e.vpcConfig(), CustomInternalALBSubnets: e.internalALBSubnets(), - AllowVPCIngress: e.in.AllowVPCIngress, // TODO(jwh): fetch AllowVPCIngress from Manifest or SSM. + AllowVPCIngress: aws.BoolValue(e.in.Mft.HTTPConfig.Private.SecurityGroupsConfig.Ingress.VPCIngress), Telemetry: e.telemetryConfig(), CDNConfig: e.cdnConfig(), diff --git a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go index a13f1c041e8..3b759c36534 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go @@ -35,14 +35,14 @@ http: public: security_group: ingress: - from_cdn: true + restrict_to_cdn: true certificates: - cert-1 - cert-2 private: security_groups: ingress: - from_vpc: true + allow_from_vpc: true observability: container_insights: true # Enable container insights.` var mft manifest.Environment diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml index 07a2eb5d328..e8c99c9fe46 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml @@ -11,14 +11,14 @@ Metadata: public: security_group: ingress: - from_cdn: true + restrict_to_cdn: true certificates: - cert-1 - cert-2 private: security_groups: ingress: - from_vpc: true + allow_from_vpc: true observability: container_insights: true # Enable container insights. Parameters: diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index a6ee6876dd1..79cd4d81f17 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -347,7 +347,8 @@ type PublicHTTPConfig struct { // Ingress represents allowed ingress traffic from specified fields. type Ingress struct { - CDNIngress *bool `yaml:"restrict_from_cdn"` + CDNIngress *bool `yaml:"restrict_to_cdn"` + VPCIngress *bool `yaml:"allow_from_vpc"` } // IsEmpty returns true if there is are no specified fields for ingress. @@ -372,17 +373,9 @@ func (cfg privateHTTPConfig) IsEmpty() bool { } type securityGroupsConfig struct { - Ingress ingress `yaml:"ingress"` + Ingress Ingress `yaml:"ingress"` } func (cfg securityGroupsConfig) isEmpty() bool { - return cfg.Ingress.isEmpty() -} - -type ingress struct { - VPCIngress *bool `yaml:"from_vpc"` -} - -func (i ingress) isEmpty() bool { - return i.VPCIngress == nil + return cfg.Ingress.IsEmpty() } diff --git a/internal/pkg/manifest/env_test.go b/internal/pkg/manifest/env_test.go index 8940156d86e..9677c9c5475 100644 --- a/internal/pkg/manifest/env_test.go +++ b/internal/pkg/manifest/env_test.go @@ -279,7 +279,7 @@ func TestFromEnvConfig(t *testing.T) { InternalALBSubnets: []string{"subnet2"}, Certificates: []string{"arn:aws:acm:region:account:certificate/certificate_ID_1", "arn:aws:acm:region:account:certificate/certificate_ID_2"}, SecurityGroupsConfig: securityGroupsConfig{ - Ingress: ingress{ + Ingress: Ingress{ VPCIngress: aws.Bool(false), }, }, @@ -435,7 +435,7 @@ http: private: security_groups: ingress: - from_vpc: false + allow_from_vpc: false `, wantedStruct: &Environment{ Workload: Workload{ @@ -449,7 +449,7 @@ http: }, Private: privateHTTPConfig{ SecurityGroupsConfig: securityGroupsConfig{ - Ingress: ingress{ + Ingress: Ingress{ VPCIngress: aws.Bool(false), }, }, diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index d1b1371aa48..955b016f657 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -235,6 +235,11 @@ func (cfg privateHTTPConfig) Validate() error { return nil } +// Validate returns nil if securityGroupsConfig is configured correctly. +func (cfg securityGroupsConfig) Validate() error { + return cfg.Ingress.Validate() +} + // Validate returns nil if environmentCDNConfig is configured correctly. func (cfg environmentCDNConfig) Validate() error { if cfg.CDNConfig.IsEmpty() { From bc876601271852c87b0dd3136b3a1747ffd2da4c Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Fri, 22 Jul 2022 14:07:20 -0700 Subject: [PATCH 22/27] refactor ingress --- internal/pkg/cli/deploy/env.go | 3 +- internal/pkg/cli/deploy/env_test.go | 4 ++- .../stack/env_integration_test.go | 5 +-- ...late-with-imported-certs-observability.yml | 5 +-- internal/pkg/manifest/env.go | 19 +++++++--- internal/pkg/manifest/env_test.go | 2 +- internal/pkg/manifest/validate_env.go | 15 ++++++-- internal/pkg/manifest/validate_env_test.go | 35 ++++++++++++++++++- 8 files changed, 74 insertions(+), 14 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 000fe90d1e7..25cb91634f9 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -133,7 +133,7 @@ func (d *envDeployer) prefixLists(in *DeployEnvironmentInput) ([]string, error) func (d *envDeployer) cfManagedPrefixListId(in *DeployEnvironmentInput) (*string, error) { // Check if ingress is allowed from cloudfront - if in.Manifest == nil || !aws.BoolValue(in.Manifest.HTTPConfig.Public.SecurityGroupConfig.Ingress.CDNIngress) { + if in.Manifest == nil || !aws.BoolValue(in.Manifest.HTTPConfig.Public.SecurityGroupConfig.Ingress.RestrictiveIngress.CDNIngress) { return nil, nil } @@ -141,6 +141,7 @@ func (d *envDeployer) cfManagedPrefixListId(in *DeployEnvironmentInput) (*string if err != nil { return nil, fmt.Errorf("retrieve CloudFront managed prefix list id: %s", err) } + return id, nil } diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 9e3ad92aebf..a16a77f99a5 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -274,7 +274,9 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { Public: manifest.PublicHTTPConfig{ SecurityGroupConfig: manifest.ALBSecurityGroupsConfig{ Ingress: manifest.Ingress{ - CDNIngress: aws.Bool(true), + RestrictiveIngress: manifest.RestrictiveIngress{ + CDNIngress: aws.Bool(true), + }, }, }, }, diff --git a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go index 3b759c36534..41082e92b45 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go @@ -35,14 +35,15 @@ http: public: security_group: ingress: - restrict_to_cdn: true + restrict_to: + cdn: true certificates: - cert-1 - cert-2 private: security_groups: ingress: - allow_from_vpc: true + from_vpc: true observability: container_insights: true # Enable container insights.` var mft manifest.Environment diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml index e8c99c9fe46..539c91b40c0 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml @@ -11,14 +11,15 @@ Metadata: public: security_group: ingress: - restrict_to_cdn: true + restrict_to: + cdn: true certificates: - cert-1 - cert-2 private: security_groups: ingress: - allow_from_vpc: true + from_vpc: true observability: container_insights: true # Enable container insights. Parameters: diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 79cd4d81f17..c77ef4087c5 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -347,15 +347,26 @@ type PublicHTTPConfig struct { // Ingress represents allowed ingress traffic from specified fields. type Ingress struct { - CDNIngress *bool `yaml:"restrict_to_cdn"` - VPCIngress *bool `yaml:"allow_from_vpc"` + RestrictiveIngress RestrictiveIngress `yaml:"restrict_to"` + VPCIngress *bool `yaml:"from_vpc"` } -// IsEmpty returns true if there is are no specified fields for ingress. -func (i Ingress) IsEmpty() bool { +// RestrictiveIngress represents ingress fields which restrict +// default behavior of allowing all public ingress. +type RestrictiveIngress struct { + CDNIngress *bool `yaml:"cdn"` +} + +// IsEmpty returns true if there are no specified fields for ingress. +func (i RestrictiveIngress) IsEmpty() bool { return i.CDNIngress == nil } +// IsEmpty returns true if there are no specified fields for ingress. +func (i Ingress) IsEmpty() bool { + return i.VPCIngress == nil && i.RestrictiveIngress.IsEmpty() +} + // IsEmpty returns true if there is no customization to the public ALB. func (cfg PublicHTTPConfig) IsEmpty() bool { return len(cfg.Certificates) == 0 && cfg.SecurityGroupConfig.IsEmpty() diff --git a/internal/pkg/manifest/env_test.go b/internal/pkg/manifest/env_test.go index 9677c9c5475..b4fb5e064fc 100644 --- a/internal/pkg/manifest/env_test.go +++ b/internal/pkg/manifest/env_test.go @@ -435,7 +435,7 @@ http: private: security_groups: ingress: - allow_from_vpc: false + from_vpc: false `, wantedStruct: &Environment{ Workload: Workload{ diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 955b016f657..15d415cacd6 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -37,7 +37,7 @@ func (e EnvironmentConfig) Validate() error { return fmt.Errorf(`validate "http config": %w`, err) } - if aws.BoolValue(e.HTTPConfig.Public.SecurityGroupConfig.Ingress.CDNIngress) && !e.CDNConfig.CDNEnabled() { + if aws.BoolValue(e.HTTPConfig.Public.SecurityGroupConfig.Ingress.RestrictiveIngress.CDNIngress) && !e.CDNConfig.CDNEnabled() { return errors.New("CDN must be enabled to limit security group ingress to CloudFront") } @@ -217,6 +217,9 @@ func (cfg PublicHTTPConfig) Validate() error { return fmt.Errorf(`parse "certificates[%d]": %w`, idx, err) } } + if cfg.SecurityGroupConfig.Ingress.VPCIngress != nil { + return fmt.Errorf("a public load balancer already allows vpc ingress") + } return cfg.SecurityGroupConfig.Validate() } @@ -232,6 +235,9 @@ func (cfg privateHTTPConfig) Validate() error { return fmt.Errorf(`parse "certificates[%d]": %w`, idx, err) } } + if !cfg.SecurityGroupsConfig.Ingress.RestrictiveIngress.IsEmpty() { + return fmt.Errorf("an internal load balancer cannot have restrictive ingress fields") + } return nil } @@ -248,8 +254,13 @@ func (cfg environmentCDNConfig) Validate() error { return cfg.CDNConfig.Validate() } -// Validate is a no-op for Ingress. +// Validate returns nil if Ingress is configured correctly. func (i Ingress) Validate() error { + return i.RestrictiveIngress.Validate() +} + +// Validate is a no-op for RestrictiveIngress. +func (i RestrictiveIngress) Validate() error { return nil } diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index c4d629c3b4b..72ec65e3d8c 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -5,6 +5,7 @@ package manifest import ( "errors" + "fmt" "testing" "github.com/aws/aws-sdk-go/aws" @@ -101,7 +102,9 @@ func TestEnvironmentConfig_Validate(t *testing.T) { Public: PublicHTTPConfig{ SecurityGroupConfig: ALBSecurityGroupsConfig{ Ingress: Ingress{ - CDNIngress: aws.Bool(true), + RestrictiveIngress: RestrictiveIngress{ + CDNIngress: aws.Bool(true), + }, }, }, }, @@ -701,6 +704,7 @@ func TestEnvironmentHTTPConfig_Validate(t *testing.T) { testCases := map[string]struct { in EnvironmentHTTPConfig wantedErrorMsgPrefix string + wantedError error }{ "malformed public certificate": { in: EnvironmentHTTPConfig{ @@ -732,6 +736,32 @@ func TestEnvironmentHTTPConfig_Validate(t *testing.T) { }, }, }, + "public http config with invalid security group ingress": { + in: EnvironmentHTTPConfig{ + Public: PublicHTTPConfig{ + SecurityGroupConfig: ALBSecurityGroupsConfig{ + Ingress: Ingress{ + VPCIngress: aws.Bool(true), + }, + }, + }, + }, + wantedError: fmt.Errorf(`validate "public": a public load balancer already allows vpc ingress`), + }, + "private http config with invalid security group ingress": { + in: EnvironmentHTTPConfig{ + Private: privateHTTPConfig{ + SecurityGroupsConfig: securityGroupsConfig{ + Ingress: Ingress{ + RestrictiveIngress: RestrictiveIngress{ + CDNIngress: aws.Bool(true), + }, + }, + }, + }, + }, + wantedError: fmt.Errorf(`validate "private": an internal load balancer cannot have restrictive ingress fields`), + }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { @@ -739,6 +769,9 @@ func TestEnvironmentHTTPConfig_Validate(t *testing.T) { if tc.wantedErrorMsgPrefix != "" { require.Error(t, gotErr) require.Contains(t, gotErr.Error(), tc.wantedErrorMsgPrefix) + } else if tc.wantedError != nil { + require.Error(t, gotErr) + require.EqualError(t, tc.wantedError, gotErr.Error()) } else { require.NoError(t, gotErr) } From 9a004fdf62f6731da8999ebd7f703cf2dad493a7 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Fri, 22 Jul 2022 14:18:09 -0700 Subject: [PATCH 23/27] nit --- internal/pkg/manifest/env.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index c77ef4087c5..43ea986c934 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -357,7 +357,7 @@ type RestrictiveIngress struct { CDNIngress *bool `yaml:"cdn"` } -// IsEmpty returns true if there are no specified fields for ingress. +// IsEmpty returns true if there are no specified fields for restrictive ingress. func (i RestrictiveIngress) IsEmpty() bool { return i.CDNIngress == nil } From 395bcff795bc70639da9d7711b20b809f9e1f034 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Fri, 22 Jul 2022 14:50:32 -0700 Subject: [PATCH 24/27] finalize prefix list getter structure --- internal/pkg/aws/ec2/ec2.go | 12 ++++++------ internal/pkg/aws/ec2/ec2_test.go | 4 ++-- internal/pkg/cli/deploy/env.go | 24 ++++++++++------------- internal/pkg/cli/deploy/env_test.go | 8 ++++---- internal/pkg/cli/deploy/mocks/mock_env.go | 4 ++-- 5 files changed, 24 insertions(+), 28 deletions(-) diff --git a/internal/pkg/aws/ec2/ec2.go b/internal/pkg/aws/ec2/ec2.go index d6aff0adb06..27e09fa422a 100644 --- a/internal/pkg/aws/ec2/ec2.go +++ b/internal/pkg/aws/ec2/ec2.go @@ -459,24 +459,24 @@ func (c *EC2) managedPrefixList(prefixListName string) (*ec2.DescribeManagedPref } // CloudFrontManagedPrefixListID returns the PrefixListId of the associated cloudfront prefix list as a *string. -func (c *EC2) CloudFrontManagedPrefixListID() (*string, error) { +func (c *EC2) CloudFrontManagedPrefixListID() (string, error) { prefixListsOutput, err := c.managedPrefixList(cloudFrontPrefixListName) if err != nil { - return nil, err + return "", err } - var ids []*string + var ids []string for _, v := range prefixListsOutput.PrefixLists { - ids = append(ids, v.PrefixListId) + ids = append(ids, *v.PrefixListId) } if len(ids) == 0 { - return nil, fmt.Errorf("cannot find any prefix list with name: %s", cloudFrontPrefixListName) + return "", fmt.Errorf("cannot find any prefix list with name: %s", cloudFrontPrefixListName) } if len(ids) > 1 { - return nil, fmt.Errorf("found more than one prefix list with the name %s: %v", cloudFrontPrefixListName, ids) + return "", fmt.Errorf("found more than one prefix list with the name %s: %v", cloudFrontPrefixListName, ids) } return ids[0], nil diff --git a/internal/pkg/aws/ec2/ec2_test.go b/internal/pkg/aws/ec2/ec2_test.go index 4e139d74b3d..0a0967f2a4b 100644 --- a/internal/pkg/aws/ec2/ec2_test.go +++ b/internal/pkg/aws/ec2/ec2_test.go @@ -342,7 +342,7 @@ func TestEC2_CloudFrontManagedPrefixListId(t *testing.T) { wantedError error wantedErrorMsgPrefix string - wantedId *string + wantedId string }{ "query returns error": { mockEC2Client: func(m *mocks.Mockapi) { @@ -392,7 +392,7 @@ func TestEC2_CloudFrontManagedPrefixListId(t *testing.T) { }, }, nil) }, - wantedId: aws.String(mockPrefixListId), + wantedId: mockPrefixListId, }, } diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 25cb91634f9..69d0e46cc63 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -37,7 +37,7 @@ type environmentDeployer interface { } type prefixListGetter interface { - CloudFrontManagedPrefixListID() (*string, error) + CloudFrontManagedPrefixListID() (string, error) } type envDeployer struct { @@ -120,26 +120,22 @@ func (d *envDeployer) uploadCustomResources(bucket string) (map[string]string, e func (d *envDeployer) prefixLists(in *DeployEnvironmentInput) ([]string, error) { var prefixListIDs []string - cfManagedPrefixListId, err := d.cfManagedPrefixListId(in) - if err != nil { - return nil, err - } - if cfManagedPrefixListId != nil { - prefixListIDs = append(prefixListIDs, *cfManagedPrefixListId) + // Check if ingress is allowed from cloudfront + if in.Manifest != nil && aws.BoolValue(in.Manifest.HTTPConfig.Public.SecurityGroupConfig.Ingress.RestrictiveIngress.CDNIngress) { + cfManagedPrefixListId, err := d.cfManagedPrefixListId(in) + if err != nil { + return nil, err + } + prefixListIDs = append(prefixListIDs, cfManagedPrefixListId) } return prefixListIDs, nil } -func (d *envDeployer) cfManagedPrefixListId(in *DeployEnvironmentInput) (*string, error) { - // Check if ingress is allowed from cloudfront - if in.Manifest == nil || !aws.BoolValue(in.Manifest.HTTPConfig.Public.SecurityGroupConfig.Ingress.RestrictiveIngress.CDNIngress) { - return nil, nil - } - +func (d *envDeployer) cfManagedPrefixListId(in *DeployEnvironmentInput) (string, error) { id, err := d.ec2.CloudFrontManagedPrefixListID() if err != nil { - return nil, fmt.Errorf("retrieve CloudFront managed prefix list id: %s", err) + return "", fmt.Errorf("retrieve CloudFront managed prefix list id: %s", err) } return id, nil diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index a16a77f99a5..4462fdbb6c3 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -266,7 +266,7 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) - m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return(nil, errors.New("some error")) + m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return("", errors.New("some error")) }, inManifest: &manifest.Environment{ EnvironmentConfig: manifest.EnvironmentConfig{ @@ -292,7 +292,7 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { }, nil) m.envDeployer.EXPECT().EnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) m.envDeployer.EXPECT().ForceUpdateOutputID(gomock.Any(), gomock.Any()).Return("", nil) - m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return(aws.String("mockPrefixListID"), nil).Times(0) + m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return("mockPrefixListID", nil).Times(0) m.envDeployer.EXPECT().UpdateAndRenderEnvironment(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) }, inManifest: nil, @@ -321,7 +321,7 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) - m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return(aws.String("mockPrefixListID"), nil).Times(0) + m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return("mockPrefixListID", nil).Times(0) m.envDeployer.EXPECT().EnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) m.envDeployer.EXPECT().ForceUpdateOutputID(gomock.Any(), gomock.Any()).Return("", nil) m.envDeployer.EXPECT().UpdateAndRenderEnvironment(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("some error")) @@ -333,7 +333,7 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { m.appCFN.EXPECT().GetAppResourcesByRegion(mockApp, mockEnvRegion).Return(&stack.AppRegionalResources{ S3Bucket: "mockS3Bucket", }, nil) - m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return(aws.String("mockPrefixListID"), nil).Times(0) + m.prefixListGetter.EXPECT().CloudFrontManagedPrefixListID().Return("mockPrefixListID", nil).Times(0) m.envDeployer.EXPECT().EnvironmentParameters(gomock.Any(), gomock.Any()).Return(nil, nil) m.envDeployer.EXPECT().ForceUpdateOutputID(gomock.Any(), gomock.Any()).Return("", nil) m.envDeployer.EXPECT().UpdateAndRenderEnvironment(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) diff --git a/internal/pkg/cli/deploy/mocks/mock_env.go b/internal/pkg/cli/deploy/mocks/mock_env.go index 9dfd3400beb..59771f77b58 100644 --- a/internal/pkg/cli/deploy/mocks/mock_env.go +++ b/internal/pkg/cli/deploy/mocks/mock_env.go @@ -150,10 +150,10 @@ func (m *MockprefixListGetter) EXPECT() *MockprefixListGetterMockRecorder { } // CloudFrontManagedPrefixListID mocks base method. -func (m *MockprefixListGetter) CloudFrontManagedPrefixListID() (*string, error) { +func (m *MockprefixListGetter) CloudFrontManagedPrefixListID() (string, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "CloudFrontManagedPrefixListID") - ret0, _ := ret[0].(*string) + ret0, _ := ret[0].(string) ret1, _ := ret[1].(error) return ret0, ret1 } From 777ab6b8a13f55e10dbadd43c38f0d167d3c9389 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Fri, 22 Jul 2022 16:24:45 -0700 Subject: [PATCH 25/27] revert to before merge, fix nit --- .../pkg/deploy/cloudformation/stack/env_integration_test.go | 2 +- .../environments/template-with-imported-certs-observability.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go index 41082e92b45..53c239229d3 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go @@ -33,7 +33,7 @@ type: Environment cdn: true http: public: - security_group: + security_groups: ingress: restrict_to: cdn: true diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml index 539c91b40c0..be8557d688b 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml @@ -9,7 +9,7 @@ Metadata: cdn: true http: public: - security_group: + security_groups: ingress: restrict_to: cdn: true From 0815e34274ee649a78ac33eea276736e885bbf50 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Mon, 25 Jul 2022 12:40:24 -0700 Subject: [PATCH 26/27] address efekarakus and Lou1415926 feedback --- internal/pkg/cli/deploy/env.go | 40 +++++++++++++-------------- internal/pkg/cli/deploy/env_test.go | 6 ++-- internal/pkg/deploy/env.go | 2 +- internal/pkg/manifest/env.go | 18 ++++++++---- internal/pkg/manifest/validate_env.go | 2 +- 5 files changed, 37 insertions(+), 31 deletions(-) diff --git a/internal/pkg/cli/deploy/env.go b/internal/pkg/cli/deploy/env.go index 69d0e46cc63..05eae7765bb 100644 --- a/internal/pkg/cli/deploy/env.go +++ b/internal/pkg/cli/deploy/env.go @@ -8,7 +8,6 @@ import ( "io" "os" - "github.com/aws/aws-sdk-go/aws" 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" @@ -45,9 +44,9 @@ type envDeployer struct { env *config.Environment // Dependencies to upload artifacts. - templateFS template.Reader - s3 uploader - ec2 prefixListGetter + templateFS template.Reader + s3 uploader + prefixListGetter prefixListGetter // Dependencies to deploy an environment. appCFN appResourcesGetter envDeployer environmentDeployer @@ -82,9 +81,9 @@ func NewEnvDeployer(in *NewEnvDeployerInput) (*envDeployer, error) { app: in.App, env: in.Env, - templateFS: template.New(), - s3: s3.New(envRegionSession), - ec2: ec2.New(envRegionSession), + templateFS: template.New(), + s3: s3.New(envRegionSession), + prefixListGetter: ec2.New(envRegionSession), appCFN: deploycfn.New(defaultSession), envDeployer: deploycfn.New(envManagerSession), @@ -117,25 +116,26 @@ func (d *envDeployer) uploadCustomResources(bucket string) (map[string]string, e return urls, nil } -func (d *envDeployer) prefixLists(in *DeployEnvironmentInput) ([]string, error) { - var prefixListIDs []string +func (d *envDeployer) cidrPrefixLists(in *DeployEnvironmentInput) ([]string, error) { + var cidrPrefixListIDs []string // Check if ingress is allowed from cloudfront - if in.Manifest != nil && aws.BoolValue(in.Manifest.HTTPConfig.Public.SecurityGroupConfig.Ingress.RestrictiveIngress.CDNIngress) { - cfManagedPrefixListId, err := d.cfManagedPrefixListId(in) - if err != nil { - return nil, err - } - prefixListIDs = append(prefixListIDs, cfManagedPrefixListId) + if in.Manifest == nil || !in.Manifest.IsIngressRestrictedToCDN() { + return nil, nil } + cfManagedPrefixListID, err := d.cfManagedPrefixListID() + if err != nil { + return nil, err + } + cidrPrefixListIDs = append(cidrPrefixListIDs, cfManagedPrefixListID) - return prefixListIDs, nil + return cidrPrefixListIDs, nil } -func (d *envDeployer) cfManagedPrefixListId(in *DeployEnvironmentInput) (string, error) { - id, err := d.ec2.CloudFrontManagedPrefixListID() +func (d *envDeployer) cfManagedPrefixListID() (string, error) { + id, err := d.prefixListGetter.CloudFrontManagedPrefixListID() if err != nil { - return "", fmt.Errorf("retrieve CloudFront managed prefix list id: %s", err) + return "", fmt.Errorf("retrieve CloudFront managed prefix list id: %w", err) } return id, nil @@ -220,7 +220,7 @@ func (d *envDeployer) buildStackInput(in *DeployEnvironmentInput) (*deploy.Creat if err != nil { return nil, err } - cidrPrefixListIDs, err := d.prefixLists(in) + cidrPrefixListIDs, err := d.cidrPrefixLists(in) if err != nil { return nil, err } diff --git a/internal/pkg/cli/deploy/env_test.go b/internal/pkg/cli/deploy/env_test.go index 4462fdbb6c3..d8839803a5f 100644 --- a/internal/pkg/cli/deploy/env_test.go +++ b/internal/pkg/cli/deploy/env_test.go @@ -358,9 +358,9 @@ func TestEnvDeployer_DeployEnvironment(t *testing.T) { ManagerRoleARN: mockManagerRoleARN, Region: mockEnvRegion, }, - appCFN: m.appCFN, - envDeployer: m.envDeployer, - ec2: m.prefixListGetter, + appCFN: m.appCFN, + envDeployer: m.envDeployer, + prefixListGetter: m.prefixListGetter, } mockIn := &DeployEnvironmentInput{ RootUserARN: "mockRootUserARN", diff --git a/internal/pkg/deploy/env.go b/internal/pkg/deploy/env.go index 1ed6e636743..bdee243e7ba 100644 --- a/internal/pkg/deploy/env.go +++ b/internal/pkg/deploy/env.go @@ -38,7 +38,7 @@ type CreateEnvironmentInput struct { ImportCertARNs []string // Optional configuration if users want to import certificates. InternalALBSubnets []string // Optional configuration if users want to specify internal ALB placement. AllowVPCIngress bool // Optional configuration to allow access to internal ALB from ports 80/443. - CIDRPrefixListIDs []string // Optional configuration to specify public security group ingress based on prefix lists + CIDRPrefixListIDs []string // Optional configuration to specify public security group ingress based on prefix lists. Telemetry *config.Telemetry // Optional observability and monitoring configuration. Mft *manifest.Environment // Unmarshaled and interpolated manifest object. RawMft []byte // Content of the environment manifest without any modifications. diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 43ea986c934..9cbe7fe5cad 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -91,6 +91,12 @@ type EnvironmentConfig struct { CDNConfig environmentCDNConfig `yaml:"cdn,omitempty,flow"` } +// IsIngressRestrictedToCDN returns whether or not an environment has its +// Public Load Balancer ingress restricted to a Content Delivery Network. +func (mft *EnvironmentConfig) IsIngressRestrictedToCDN() bool { + return aws.BoolValue(mft.HTTPConfig.Public.SecurityGroupConfig.Ingress.RestrictiveIngress.CDNIngress) +} + type environmentNetworkConfig struct { VPC environmentVPCConfig `yaml:"vpc,omitempty"` } @@ -330,6 +336,12 @@ func (cfg *EnvironmentHTTPConfig) loadLBConfig(env *config.CustomizeEnv) { cfg.Public.Certificates = env.ImportCertARNs } +// PublicHTTPConfig represents the configuration settings for an environment public ALB. +type PublicHTTPConfig struct { + SecurityGroupConfig ALBSecurityGroupsConfig `yaml:"security_groups,omitempty"` + Certificates []string `yaml:"certificates,omitempty"` +} + // ALBSecurityGroupsConfig represents security group configuration settings for an ALB. type ALBSecurityGroupsConfig struct { Ingress Ingress `yaml:"ingress"` @@ -339,12 +351,6 @@ func (cfg ALBSecurityGroupsConfig) IsEmpty() bool { return cfg.Ingress.IsEmpty() } -// PublicHTTPConfig represents the configuration settings for an environment public ALB. -type PublicHTTPConfig struct { - SecurityGroupConfig ALBSecurityGroupsConfig `yaml:"security_groups,omitempty"` - Certificates []string `yaml:"certificates,omitempty"` -} - // Ingress represents allowed ingress traffic from specified fields. type Ingress struct { RestrictiveIngress RestrictiveIngress `yaml:"restrict_to"` diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 15d415cacd6..e4bee3c3af0 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -37,7 +37,7 @@ func (e EnvironmentConfig) Validate() error { return fmt.Errorf(`validate "http config": %w`, err) } - if aws.BoolValue(e.HTTPConfig.Public.SecurityGroupConfig.Ingress.RestrictiveIngress.CDNIngress) && !e.CDNConfig.CDNEnabled() { + if e.IsIngressRestrictedToCDN() && !e.CDNConfig.CDNEnabled() { return errors.New("CDN must be enabled to limit security group ingress to CloudFront") } From 19ca126b240ab21c44c9988d0ec670550a6c3200 Mon Sep 17 00:00:00 2001 From: Aiden Carpenter Date: Mon, 25 Jul 2022 13:07:42 -0700 Subject: [PATCH 27/27] merge conflict fixes --- internal/pkg/manifest/validate_env.go | 54 +++++++++---------- .../pkg/manifest/validate_integration_test.go | 2 +- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index a0b54e0655c..b5d2b64fb3b 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -19,15 +19,15 @@ var ( // Validate returns nil if Environment is configured correctly. func (e Environment) Validate() error { - if err := e.EnvironmentConfig.Validate(); err != nil { + if err := e.EnvironmentConfig.validate(); err != nil { return fmt.Errorf(`validate "network": %w`, err) } return nil } -// Validate returns nil if EnvironmentConfig is configured correctly. -func (e EnvironmentConfig) Validate() error { - if err := e.Network.Validate(); err != nil { +// validate returns nil if EnvironmentConfig is configured correctly. +func (e EnvironmentConfig) validate() error { + if err := e.Network.validate(); err != nil { return fmt.Errorf(`validate "network": %w`, err) } if err := e.Observability.validate(); err != nil { @@ -199,9 +199,9 @@ func (o environmentObservability) validate() error { return nil } -// Validate returns nil if EnvironmentHTTPConfig is configured correctly. -func (cfg EnvironmentHTTPConfig) Validate() error { - if err := cfg.Public.Validate(); err != nil { +// validate returns nil if EnvironmentHTTPConfig is configured correctly. +func (cfg EnvironmentHTTPConfig) validate() error { + if err := cfg.Public.validate(); err != nil { return fmt.Errorf(`validate "public": %w`, err) } if err := cfg.Private.validate(); err != nil { @@ -210,8 +210,8 @@ func (cfg EnvironmentHTTPConfig) Validate() error { return nil } -// Validate returns nil if PublicHTTPConfig is configured correctly. -func (cfg PublicHTTPConfig) Validate() error { +// validate returns nil if PublicHTTPConfig is configured correctly. +func (cfg PublicHTTPConfig) validate() error { for idx, certARN := range cfg.Certificates { if _, err := arn.Parse(certARN); err != nil { return fmt.Errorf(`parse "certificates[%d]": %w`, idx, err) @@ -220,12 +220,12 @@ func (cfg PublicHTTPConfig) Validate() error { if cfg.SecurityGroupConfig.Ingress.VPCIngress != nil { return fmt.Errorf("a public load balancer already allows vpc ingress") } - return cfg.SecurityGroupConfig.Validate() + return cfg.SecurityGroupConfig.validate() } -// Validate returns nil if ALBSecurityGroupsConfig is configured correctly. -func (cfg ALBSecurityGroupsConfig) Validate() error { - return cfg.Ingress.Validate() +// validate returns nil if ALBSecurityGroupsConfig is configured correctly. +func (cfg ALBSecurityGroupsConfig) validate() error { + return cfg.Ingress.validate() } // validate returns nil if privateHTTPConfig is configured correctly. @@ -237,41 +237,41 @@ func (cfg privateHTTPConfig) validate() error { } if !cfg.SecurityGroupsConfig.Ingress.RestrictiveIngress.IsEmpty() { return fmt.Errorf("an internal load balancer cannot have restrictive ingress fields") - } + } if err := cfg.SecurityGroupsConfig.validate(); err != nil { return fmt.Errorf(`validate "security_groups: %w`, err) } return nil } -// Validate returns nil if securityGroupsConfig is configured correctly. -func (cfg securityGroupsConfig) Validate() error { - if s.isEmpty() { +// validate returns nil if securityGroupsConfig is configured correctly. +func (cfg securityGroupsConfig) validate() error { + if cfg.isEmpty() { return nil } - return s.Ingress.Validate() + return cfg.Ingress.validate() } -// Validate returns nil if environmentCDNConfig is configured correctly. -func (cfg environmentCDNConfig) Validate() error { +// validate returns nil if environmentCDNConfig is configured correctly. +func (cfg environmentCDNConfig) validate() error { if cfg.CDNConfig.IsEmpty() { return nil } return cfg.CDNConfig.validate() } -// Validate returns nil if Ingress is configured correctly. -func (i Ingress) Validate() error { - return i.RestrictiveIngress.Validate() +// validate returns nil if Ingress is configured correctly. +func (i Ingress) validate() error { + return i.RestrictiveIngress.validate() } -// Validate is a no-op for RestrictiveIngress. -func (i RestrictiveIngress) Validate() error { +// validate is a no-op for RestrictiveIngress. +func (i RestrictiveIngress) validate() error { return nil } -// Validate is a no-op for AdvancedCDNConfig. -func (cfg advancedCDNConfig) Validate() error { +// validate is a no-op for AdvancedCDNConfig. +func (cfg advancedCDNConfig) validate() error { return nil } diff --git a/internal/pkg/manifest/validate_integration_test.go b/internal/pkg/manifest/validate_integration_test.go index fc8a66545bd..964ab1c7f34 100644 --- a/internal/pkg/manifest/validate_integration_test.go +++ b/internal/pkg/manifest/validate_integration_test.go @@ -62,7 +62,7 @@ func Test_ValidateAudit(t *testing.T) { // Audit environment manifest. t.Run("environment manifest", func(t *testing.T) { env := &Environment{} - err := isValid(reflect.ValueOf(env.environmentConfig).Type()) + err := isValid(reflect.ValueOf(env.EnvironmentConfig).Type()) require.NoError(t, err) }) }