Skip to content

chore: retrofit --allow-vpc-ingress flag to manifest #3763

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
mergify merged 11 commits into from
Jul 18, 2022
Merged

chore: retrofit --allow-vpc-ingress flag to manifest #3763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7d2b039
add sg.allow_vpc_ingress to env manifest
Lou1415926 Jul 14, 2022
3d6ddf9
remove env upgrade test suites
Lou1415926 Jul 14, 2022
c1de9b7
use manifest instead of SSM in env stack tests
Lou1415926 Jul 14, 2022
9a849ce
feed manifest to stack
Lou1415926 Jul 14, 2022
08f973d
fix e2e;
Lou1415926 Jul 14, 2022
b04fe34
addr fb: change manifest design to sg.ingress.from_vpc`
Lou1415926 Jul 15, 2022
d7b9a50
convert from ssm to manifest
Lou1415926 Jul 15, 2022
654d8cf
Merge branch 'mainline' into env/manifest/internal-alb-ingress
Lou1415926 Jul 16, 2022
86e9b66
reolve conflicts
Lou1415926 Jul 16, 2022
079d866
fix some tests`
Lou1415926 Jul 16, 2022
2df8330
Merge branch 'mainline' into env/manifest/internal-alb-ingress
mergify[bot] Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/pkg/deploy/cloudformation/stack/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,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.
Copy link
Copy Markdown
Contributor

@iamhopaul123 iamhopaul123 Jul 14, 2022

Choose a reason for hiding this comment

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

Do we need to remove AllowVPCIngress from e.in?

Copy link
Copy Markdown
Contributor

@paragbhingre paragbhingre Jul 14, 2022

Choose a reason for hiding this comment

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

+1

Copy link
Copy Markdown
Contributor Author

@Lou1415926 Lou1415926 Jul 15, 2022

Choose a reason for hiding this comment

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

we can't because we need to feed from e.in to manifest for env show --manifest

AllowVPCIngress: aws.BoolValue(e.in.Mft.HTTPConfig.Private.SecurityGroupsConfig.Ingress.VPCIngress),
Telemetry: e.telemetryConfig(),
CDNConfig: e.cdnConfig(),

Expand Down
14 changes: 8 additions & 6 deletions internal/pkg/deploy/cloudformation/stack/env_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ http:
certificates:
- cert-1
- cert-2
private:
security_groups:
ingress:
from_vpc: true
observability:
container_insights: true # Enable container insights.`
var mft manifest.Environment
Expand All @@ -54,9 +58,8 @@ observability:
"DNSDelegationFunction": "https://mockbucket.s3-us-west-2.amazonaws.com/dns-delegation",
"CustomDomainFunction": "https://mockbucket.s3-us-west-2.amazonaws.com/custom-domain",
},
AllowVPCIngress: true,
Mft: &mft,
RawMft: []byte(rawMft),
Mft: &mft,
RawMft: []byte(rawMft),
}
}(),
wantedFileName: "template-with-imported-certs-observability.yml",
Expand All @@ -82,9 +85,8 @@ type: Environment`
"DNSDelegationFunction": "https://mockbucket.s3-us-west-2.amazonaws.com/dns-delegation",
"CustomDomainFunction": "https://mockbucket.s3-us-west-2.amazonaws.com/custom-domain",
},
AllowVPCIngress: true,
Mft: &mft,
RawMft: []byte(rawMft),
Mft: &mft,
RawMft: []byte(rawMft),
}
}(),
wantedFileName: "template-with-basic-manifest.yml",
Expand Down
21 changes: 17 additions & 4 deletions internal/pkg/deploy/cloudformation/stack/env_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/aws/copilot-cli/internal/pkg/config"
"github.com/aws/copilot-cli/internal/pkg/deploy"
"github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation/stack/mocks"
"github.com/aws/copilot-cli/internal/pkg/manifest"
"github.com/aws/copilot-cli/internal/pkg/template"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/require"
Expand All @@ -33,7 +34,7 @@ func TestEnv_Template(t *testing.T) {
AppName: "project",
EnvName: "env",
VPCConfig: template.VPCConfig{
Imported: &template.ImportVPC{},
Imported: nil,
Managed: template.ManagedVPC{
CIDR: DefaultVPCCIDR,
PrivateSubnetCIDRs: DefaultPrivateSubnetCIDRs,
Expand All @@ -55,7 +56,11 @@ func TestEnv_Template(t *testing.T) {
Key: "mockkey4",
},
},
ForceUpdateID: "mockPreviousForceUpdateID",
Telemetry: &template.Telemetry{
EnableContainerInsights: false,
},
SerializedManifest: "name: env\ntype: Environment\n",
ForceUpdateID: "mockPreviousForceUpdateID",
}, data)
return &template.Content{Buffer: bytes.NewBufferString("mockTemplate")}, nil
})
Expand Down Expand Up @@ -95,7 +100,7 @@ func TestEnv_Parameters(t *testing.T) {
deploymentInputWithDNS := mockDeployEnvironmentInput()
deploymentInputWithDNS.App.Domain = "ecs.aws"
deploymentInputWithPrivateDNS := mockDeployEnvironmentInput()
deploymentInputWithPrivateDNS.ImportCertARNs = []string{"arn:aws:acm:us-east-1:123456789012:certificate/12345678-1234-1234-1234-123456789012"}
deploymentInputWithPrivateDNS.Mft.HTTPConfig.Private.Certificates = []string{"arn:aws:acm:us-east-1:123456789012:certificate/12345678-1234-1234-1234-123456789012"}
testCases := map[string]struct {
input *deploy.CreateEnvironmentInput
oldParams []*cloudformation.Parameter
Expand Down Expand Up @@ -671,6 +676,14 @@ func mockDeployEnvironmentInput() *deploy.CreateEnvironmentInput {
"DNSDelegationFunction": "https://mockbucket.s3-us-west-2.amazonaws.com/mockkey2",
"CustomDomainFunction": "https://mockbucket.s3-us-west-2.amazonaws.com/mockkey4",
},
ImportVPCConfig: &config.ImportVPC{},
Mft: &manifest.Environment{
Workload: manifest.Workload{
Name: aws.String("env"),
Type: aws.String("Environment"),
},
},
RawMft: []byte(`name: env
type: Environment
`),
}
}
24 changes: 0 additions & 24 deletions ...al/pkg/deploy/cloudformation/stack/testdata/environments/template-with-basic-manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,30 +645,6 @@ Resources:
GroupId: !Ref InternalLoadBalancerSecurityGroup
IpProtocol: -1
SourceSecurityGroupId: !Ref EnvironmentSecurityGroup
InternalLoadBalancerSecurityGroupIngressFromHttp:
Metadata:
'aws:copilot:description': 'An inbound rule to the internal load balancer security group for port 80 within the VPC'
Type: AWS::EC2::SecurityGroupIngress
Condition: CreateInternalALB
Properties:
Description: Allow from within the VPC on port 80
CidrIp: 0.0.0.0/0
FromPort: 80
ToPort: 80
IpProtocol: tcp
GroupId: !Ref InternalLoadBalancerSecurityGroup
InternalLoadBalancerSecurityGroupIngressFromHttps:
Metadata:
'aws:copilot:description': 'An inbound rule to the internal load balancer security group for port 443 within the VPC'
Type: AWS::EC2::SecurityGroupIngress
Condition: ExportInternalHTTPSListener
Properties:
Description: Allow from within the VPC on port 443
CidrIp: 0.0.0.0/0
FromPort: 443
ToPort: 443
IpProtocol: tcp
GroupId: !Ref InternalLoadBalancerSecurityGroup
PublicLoadBalancer:
Metadata:
'aws:copilot:description': 'An Application Load Balancer to distribute public traffic to your services'
Expand Down
4 changes: 4 additions & 0 deletions ...cloudformation/stack/testdata/environments/template-with-imported-certs-observability.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ Metadata:
certificates:
- cert-1
- cert-2
private:
security_groups:
ingress:
from_vpc: true
observability:
container_insights: true # Enable container insights.
Parameters:
Expand Down
25 changes: 22 additions & 3 deletions internal/pkg/manifest/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,9 +318,11 @@ func (cfg *environmentHTTPConfig) loadLBConfig(env *config.CustomizeEnv) {
if env.IsEmpty() {
return
}

if env.ImportVPC != nil && len(env.ImportVPC.PublicSubnetIDs) == 0 {
cfg.Private.InternalALBSubnets = env.InternalALBSubnets
cfg.Private.Certificates = env.ImportCertARNs
cfg.Private.SecurityGroupsConfig.Ingress.VPCIngress = aws.Bool(env.EnableInternalALBVPCIngress)
return
}
cfg.Public.Certificates = env.ImportCertARNs
Expand All @@ -336,11 +338,28 @@ func (cfg publicHTTPConfig) IsEmpty() bool {
}

type privateHTTPConfig struct {
InternalALBSubnets []string `yaml:"subnets,omitempty"`
Certificates []string `yaml:"certificates,omitempty"`
InternalALBSubnets []string `yaml:"subnets,omitempty"`
Certificates []string `yaml:"certificates,omitempty"`
SecurityGroupsConfig securityGroupsConfig `yaml:"security_groups,omitempty"`
Copy link
Copy Markdown
Contributor

@paragbhingre paragbhingre Jul 14, 2022

Choose a reason for hiding this comment

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

Just wanted to bring to your notice that my PR to add security_group to env manifest also have similar secuirty_group field in the vpc.
I was just concerned about having the same naming with just extra s at the end of security_groups will be little confusing for the customers. What do you think about this?

Copy link
Copy Markdown
Contributor Author

@Lou1415926 Lou1415926 Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is going to read

http:
   private:
      security_groups: # configs for the sg used on internal ALB

Whereas yours iirc would be

security_group:
   # environmentsg configs

so I guess we're probably fine!!

Copy link
Copy Markdown
Contributor

@paragbhingre paragbhingre Jul 14, 2022

Choose a reason for hiding this comment

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

Yes you are correct. I was just making sure that 2 almost similar parameters shouldn't be confusing to the customers. But as you said they read differently in different sections should be okay.

Copy link
Copy Markdown
Contributor

@iamhopaul123 iamhopaul123 Jul 14, 2022

Choose a reason for hiding this comment

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

This is related to https://github.com/aws/copilot-cli/pull/3737/files#diff-4c63a9ce04347fa71ffd19988ac84f3eca7aacf36c414bb9e8f95c011b0f622dR327 . it is just from_cdn won't be valid for private.

http:
  public:
    security_groups:
      ingress:
        from_cdn: true

}

// IsEmpty returns true if there is no customization to the internal ALB.
func (cfg privateHTTPConfig) IsEmpty() bool {
return len(cfg.InternalALBSubnets) == 0 && len(cfg.Certificates) == 0
return len(cfg.InternalALBSubnets) == 0 && len(cfg.Certificates) == 0 && cfg.SecurityGroupsConfig.isEmpty()
}

type securityGroupsConfig struct {
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
}
16 changes: 16 additions & 0 deletions internal/pkg/manifest/env_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,11 @@ func TestFromEnvConfig(t *testing.T) {
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"},
SecurityGroupsConfig: securityGroupsConfig{
Ingress: ingress{
VPCIngress: aws.Bool(false),
},
},
},
},
},
Expand Down Expand Up @@ -427,6 +432,10 @@ http:
certificates:
- cert-1
- cert-2
private:
security_groups:
ingress:
from_vpc: false
`,
wantedStruct: &Environment{
Workload: Workload{
Expand All @@ -438,6 +447,13 @@ http:
Public: publicHTTPConfig{
Certificates: []string{"cert-1", "cert-2"},
},
Private: privateHTTPConfig{
SecurityGroupsConfig: securityGroupsConfig{
Ingress: ingress{
VPCIngress: aws.Bool(false),
},
},
},
},
},
},
Expand Down