From 5f382aeaa4046b31209abca587a80e9924160c11 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Mon, 25 Jul 2022 09:40:19 -0700 Subject: [PATCH 01/19] chore(manifest): add security_group to env manifest --- .../pkg/deploy/cloudformation/stack/env.go | 37 +- .../stack/env_integration_test.go | 56 + .../deploy/cloudformation/stack/env_test.go | 5 +- .../template-with-custom-security-group.yml | 1015 +++++++++++++++++ internal/pkg/manifest/env.go | 44 +- internal/pkg/template/env.go | 15 + .../pkg/template/templates/environment/cf.yml | 14 + 7 files changed, 1180 insertions(+), 6 deletions(-) create mode 100644 internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index 43770cd1c8f..d413c106090 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -5,12 +5,12 @@ package stack import ( "fmt" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/arn" "github.com/aws/aws-sdk-go/service/cloudformation" "github.com/aws/copilot-cli/internal/pkg/config" "github.com/aws/copilot-cli/internal/pkg/deploy" + "github.com/aws/copilot-cli/internal/pkg/manifest" "github.com/aws/copilot-cli/internal/pkg/template" "github.com/google/uuid" ) @@ -95,6 +95,10 @@ func (e *EnvStackConfig) Template() (string, error) { } forceUpdateID = id.String() } + securityGroupConfig, err := getSecurityGroupConfig(e.in.Mft) + if err != nil { + return "", err + } content, err := e.parser.ParseEnv(&template.EnvOpts{ AppName: e.in.App.Name, EnvName: e.in.Name, @@ -108,6 +112,7 @@ func (e *EnvStackConfig) Template() (string, error) { CustomInternalALBSubnets: e.internalALBSubnets(), AllowVPCIngress: aws.BoolValue(e.in.Mft.HTTPConfig.Private.SecurityGroupsConfig.Ingress.VPCIngress), Telemetry: e.telemetryConfig(), + SecurityGroupConfig: securityGroupConfig, CDNConfig: e.cdnConfig(), Version: e.in.Version, @@ -193,6 +198,36 @@ func (e *EnvStackConfig) Parameters() ([]*cloudformation.Parameter, error) { return e.transformParameters(currParams, e.prevParams, transformEnvControllerParameters) } +func getSecurityGroupConfig(mft *manifest.Environment) (*template.SecurityGroupConfig, error) { + if mft != nil && !mft.Network.VPC.SecurityGroupConfig.IsEmpty() { + if !mft.Network.VPC.SecurityGroupConfig.IsValid() { + var ingress = make([]template.SecurityGroupRules, len(mft.Network.VPC.SecurityGroupConfig.Ingress)) + var egress = make([]template.SecurityGroupRules, len(mft.Network.VPC.SecurityGroupConfig.Egress)) + for idx, ingressValue := range mft.Network.VPC.SecurityGroupConfig.Ingress { + ingress[idx].IpProtocol = ingressValue.IpProtocol + ingress[idx].CidrIp = ingressValue.CidrIp + ingress[idx].ToPort = ingressValue.ToPort + ingress[idx].FromPort = ingressValue.FromPort + } + for idx, egressValue := range mft.Network.VPC.SecurityGroupConfig.Egress { + egress[idx].IpProtocol = egressValue.IpProtocol + egress[idx].CidrIp = egressValue.CidrIp + egress[idx].ToPort = egressValue.ToPort + egress[idx].FromPort = egressValue.FromPort + } + + return &template.SecurityGroupConfig{ + Ingress: ingress, + Egress: egress, + }, nil + } else { + return &template.SecurityGroupConfig{}, fmt.Errorf("security group config is invalid") + } + } else { + return &template.SecurityGroupConfig{}, nil + } +} + // SerializedParameters returns the CloudFormation stack's parameters serialized to a JSON document. func (e *EnvStackConfig) SerializedParameters() (string, error) { return serializeTemplateConfig(e.parser, e) diff --git a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go index 53c239229d3..54499fa7383 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go @@ -70,6 +70,62 @@ observability: }(), wantedFileName: "template-with-imported-certs-observability.yml", }, + + "generate template with embedded manifest file with custom security groups rules added by the customer": { + input: func() *deploy.CreateEnvironmentInput { + rawMft := `name: test +type: Environment +# Create the public ALB with certificates attached. +http: + public: + certificates: + - cert-1 + - cert-2 +observability: + container_insights: true # Enable container insights. +network: + vpc: + security_group: + ingress: + - ip_protocol: tcp + from_port: 0 + to_port: 65535 + cidr: 0.0.0.0 + - ip_protocol: tcp + from_port: 1 + to_port: 6 + cidr: 0.0.0.0 + egress: + - ip_protocol: tcp + from_port: 0 + to_port: 65535 + cidr: 0.0.0.0` + var mft manifest.Environment + err := yaml.Unmarshal([]byte(rawMft), &mft) + require.NoError(t, err) + return &deploy.CreateEnvironmentInput{ + Version: "1.x", + App: deploy.AppInformation{ + AccountPrincipalARN: "arn:aws:iam::000000000:root", + Name: "demo", + }, + Name: "test", + ArtifactBucketARN: "arn:aws:s3:::mockbucket", + ArtifactBucketKeyARN: "arn:aws:kms:us-west-2:000000000:key/1234abcd-12ab-34cd-56ef-1234567890ab", + CustomResourcesURLs: map[string]string{ + "CertificateValidationFunction": "https://mockbucket.s3-us-west-2.amazonaws.com/dns-cert-validator", + "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), + } + }(), + + wantedFileName: "template-with-custom-security-group.yml", + }, + "generate template with custom resources": { input: func() *deploy.CreateEnvironmentInput { rawMft := `name: test diff --git a/internal/pkg/deploy/cloudformation/stack/env_test.go b/internal/pkg/deploy/cloudformation/stack/env_test.go index 195e33a5924..3e5698ca7bd 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_test.go @@ -59,8 +59,9 @@ func TestEnv_Template(t *testing.T) { Telemetry: &template.Telemetry{ EnableContainerInsights: false, }, - SerializedManifest: "name: env\ntype: Environment\n", - ForceUpdateID: "mockPreviousForceUpdateID", + SecurityGroupConfig: &template.SecurityGroupConfig{}, + SerializedManifest: "name: env\ntype: Environment\n", + ForceUpdateID: "mockPreviousForceUpdateID", }, data) return &template.Content{Buffer: bytes.NewBufferString("mockTemplate")}, nil }) diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml new file mode 100644 index 00000000000..13384d5f2d8 --- /dev/null +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml @@ -0,0 +1,1015 @@ +# Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: MIT-0 +Description: CloudFormation environment template for infrastructure shared among Copilot workloads. +Metadata: + Manifest: | + name: test + type: Environment + # Create the public ALB with certificates attached. + http: + public: + certificates: + - cert-1 + - cert-2 + observability: + container_insights: true # Enable container insights. + network: + vpc: + security_group: + ingress: + - ip_protocol: tcp + from_port: 0 + to_port: 65535 + cidr: 0.0.0.0 + - ip_protocol: tcp + from_port: 1 + to_port: 6 + cidr: 0.0.0.0 + egress: + - ip_protocol: tcp + from_port: 0 + to_port: 65535 + cidr: 0.0.0.0 +Parameters: + AppName: + Type: String + EnvironmentName: + Type: String + ALBWorkloads: + Type: String + InternalALBWorkloads: + Type: String + EFSWorkloads: + Type: String + NATWorkloads: + Type: String + ToolsAccountPrincipalARN: + Type: String + AppDNSName: + Type: String + AppDNSDelegationRole: + Type: String + Aliases: + Type: String + CreateHTTPSListener: + Type: String + AllowedValues: [true, false] + CreateInternalHTTPSListener: + Type: String + AllowedValues: [true, false] + ServiceDiscoveryEndpoint: + Type: String +Conditions: + CreateALB: + !Not [!Equals [ !Ref ALBWorkloads, "" ]] + DelegateDNS: + !Not [!Equals [ !Ref AppDNSName, "" ]] + ExportHTTPSListener: !And + - !Condition CreateALB + - !Equals [!Ref CreateHTTPSListener, true] + ExportInternalHTTPSListener: !And + - !Condition CreateInternalALB + - !Equals [ !Ref CreateInternalHTTPSListener, true] + CreateEFS: + !Not [!Equals [ !Ref EFSWorkloads, ""]] + CreateInternalALB: + !Not [!Equals [ !Ref InternalALBWorkloads, ""]] + CreateNATGateways: + !Not [!Equals [ !Ref NATWorkloads, ""]] + ManagedAliases: !And + - !Condition DelegateDNS + - !Not [!Equals [ !Ref Aliases, "" ]] +Resources: + VPC: + Metadata: + 'aws:copilot:description': 'A Virtual Private Cloud to control networking of your AWS resources' + Type: AWS::EC2::VPC + Properties: + CidrBlock: 10.0.0.0/16 + EnableDnsHostnames: true + EnableDnsSupport: true + InstanceTenancy: default + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}' + + PublicRouteTable: + Metadata: + 'aws:copilot:description': "A custom route table that directs network traffic for the public subnets" + Type: AWS::EC2::RouteTable + Properties: + VpcId: !Ref VPC + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}' + + DefaultPublicRoute: + Type: AWS::EC2::Route + DependsOn: InternetGatewayAttachment + Properties: + RouteTableId: !Ref PublicRouteTable + DestinationCidrBlock: 0.0.0.0/0 + GatewayId: !Ref InternetGateway + + InternetGateway: + Metadata: + 'aws:copilot:description': 'An Internet Gateway to connect to the public internet' + Type: AWS::EC2::InternetGateway + Properties: + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}' + + InternetGatewayAttachment: + Type: AWS::EC2::VPCGatewayAttachment + Properties: + InternetGatewayId: !Ref InternetGateway + VpcId: !Ref VPC + PublicSubnet1: + Metadata: + 'aws:copilot:description': 'Public subnet 1 for resources that can access the internet' + Type: AWS::EC2::Subnet + Properties: + CidrBlock: 10.0.0.0/24 + VpcId: !Ref VPC + AvailabilityZone: !Select [ 0, !GetAZs '' ] + MapPublicIpOnLaunch: true + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-pub0' + PublicSubnet2: + Metadata: + 'aws:copilot:description': 'Public subnet 2 for resources that can access the internet' + Type: AWS::EC2::Subnet + Properties: + CidrBlock: 10.0.1.0/24 + VpcId: !Ref VPC + AvailabilityZone: !Select [ 1, !GetAZs '' ] + MapPublicIpOnLaunch: true + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-pub1' + PrivateSubnet1: + Metadata: + 'aws:copilot:description': 'Private subnet 1 for resources with no internet access' + Type: AWS::EC2::Subnet + Properties: + CidrBlock: 10.0.2.0/24 + VpcId: !Ref VPC + AvailabilityZone: !Select [ 0, !GetAZs '' ] + MapPublicIpOnLaunch: false + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-priv0' + PrivateSubnet2: + Metadata: + 'aws:copilot:description': 'Private subnet 2 for resources with no internet access' + Type: AWS::EC2::Subnet + Properties: + CidrBlock: 10.0.3.0/24 + VpcId: !Ref VPC + AvailabilityZone: !Select [ 1, !GetAZs '' ] + MapPublicIpOnLaunch: false + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-priv1' + PublicSubnet1RouteTableAssociation: + Type: AWS::EC2::SubnetRouteTableAssociation + Properties: + RouteTableId: !Ref PublicRouteTable + SubnetId: !Ref PublicSubnet1 + PublicSubnet2RouteTableAssociation: + Type: AWS::EC2::SubnetRouteTableAssociation + Properties: + RouteTableId: !Ref PublicRouteTable + SubnetId: !Ref PublicSubnet2 + + NatGateway1Attachment: + Type: AWS::EC2::EIP + Condition: CreateNATGateways + DependsOn: InternetGatewayAttachment + Properties: + Domain: vpc + NatGateway1: + Metadata: + 'aws:copilot:description': 'NAT Gateway 1 enabling workloads placed in private subnet 1 to reach the internet' + Type: AWS::EC2::NatGateway + Condition: CreateNATGateways + Properties: + AllocationId: !GetAtt NatGateway1Attachment.AllocationId + SubnetId: !Ref PublicSubnet1 + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-0' + PrivateRouteTable1: + Type: AWS::EC2::RouteTable + Condition: CreateNATGateways + Properties: + VpcId: !Ref 'VPC' + PrivateRoute1: + Type: AWS::EC2::Route + Condition: CreateNATGateways + Properties: + RouteTableId: !Ref PrivateRouteTable1 + DestinationCidrBlock: 0.0.0.0/0 + NatGatewayId: !Ref NatGateway1 + PrivateRouteTable1Association: + Type: AWS::EC2::SubnetRouteTableAssociation + Condition: CreateNATGateways + Properties: + RouteTableId: !Ref PrivateRouteTable1 + SubnetId: !Ref PrivateSubnet1 + NatGateway2Attachment: + Type: AWS::EC2::EIP + Condition: CreateNATGateways + DependsOn: InternetGatewayAttachment + Properties: + Domain: vpc + NatGateway2: + Metadata: + 'aws:copilot:description': 'NAT Gateway 2 enabling workloads placed in private subnet 2 to reach the internet' + Type: AWS::EC2::NatGateway + Condition: CreateNATGateways + Properties: + AllocationId: !GetAtt NatGateway2Attachment.AllocationId + SubnetId: !Ref PublicSubnet2 + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-1' + PrivateRouteTable2: + Type: AWS::EC2::RouteTable + Condition: CreateNATGateways + Properties: + VpcId: !Ref 'VPC' + PrivateRoute2: + Type: AWS::EC2::Route + Condition: CreateNATGateways + Properties: + RouteTableId: !Ref PrivateRouteTable2 + DestinationCidrBlock: 0.0.0.0/0 + NatGatewayId: !Ref NatGateway2 + PrivateRouteTable2Association: + Type: AWS::EC2::SubnetRouteTableAssociation + Condition: CreateNATGateways + Properties: + RouteTableId: !Ref PrivateRouteTable2 + SubnetId: !Ref PrivateSubnet2 + # Creates a service discovery namespace with the form provided in the parameter. + # For new environments after 1.5.0, this is "env.app.local". For upgraded environments from + # before 1.5.0, this is app.local. + ServiceDiscoveryNamespace: + Metadata: + 'aws:copilot:description': 'A private DNS namespace for discovering services within the environment' + Type: AWS::ServiceDiscovery::PrivateDnsNamespace + Properties: + Name: !Ref ServiceDiscoveryEndpoint + Vpc: !Ref VPC + Cluster: + Metadata: + 'aws:copilot:description': 'An ECS cluster to group your services' + Type: AWS::ECS::Cluster + Properties: + CapacityProviders: ['FARGATE', 'FARGATE_SPOT'] + Configuration: + ExecuteCommandConfiguration: + Logging: DEFAULT + ClusterSettings: + - Name: containerInsights + Value: enabled + PublicLoadBalancerSecurityGroup: + Metadata: + 'aws:copilot:description': 'A security group for your load balancer allowing HTTP and HTTPS traffic' + Condition: CreateALB + Type: AWS::EC2::SecurityGroup + Properties: + GroupDescription: 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 + - CidrIp: 0.0.0.0/0 + Description: Allow from anyone on port 443 + FromPort: 443 + IpProtocol: tcp + ToPort: 443 + VpcId: !Ref VPC + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-lb' + InternalLoadBalancerSecurityGroup: + Metadata: + 'aws:copilot:description': 'A security group for your internal load balancer allowing HTTP traffic from within the VPC' + Condition: CreateInternalALB + Type: AWS::EC2::SecurityGroup + Properties: + GroupDescription: Access to the internal load balancer + VpcId: !Ref VPC + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-internal-lb' + # Only accept requests coming from the public ALB or other containers in the same security group. + EnvironmentSecurityGroup: + Metadata: + 'aws:copilot:description': 'A security group to allow your containers to talk to each other' + Type: AWS::EC2::SecurityGroup + Properties: + GroupDescription: !Join ['', [!Ref AppName, '-', !Ref EnvironmentName, EnvironmentSecurityGroup]] + VpcId: !Ref VPC + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-env' + SecurityGroupIngress: + - CidrIp: "0.0.0.0" + FromPort: 0 + IpProtocol: tcp + ToPort: 65535 + - CidrIp: "0.0.0.0" + FromPort: 1 + IpProtocol: tcp + ToPort: 6 + SecurityGroupEgress: + - CidrIp: "0.0.0.0" + FromPort: 0 + IpProtocol: tcp + ToPort: 65535 + EnvironmentSecurityGroupIngressFromPublicALB: + Type: AWS::EC2::SecurityGroupIngress + Condition: CreateALB + Properties: + Description: Ingress from the public ALB + GroupId: !Ref EnvironmentSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref PublicLoadBalancerSecurityGroup + EnvironmentSecurityGroupIngressFromInternalALB: + Type: AWS::EC2::SecurityGroupIngress + Condition: CreateInternalALB + Properties: + Description: Ingress from the internal ALB + GroupId: !Ref EnvironmentSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref InternalLoadBalancerSecurityGroup + EnvironmentSecurityGroupIngressFromSelf: + Type: AWS::EC2::SecurityGroupIngress + Properties: + Description: Ingress from other containers in the same security group + GroupId: !Ref EnvironmentSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref EnvironmentSecurityGroup + InternalALBIngressFromEnvironmentSecurityGroup: + Type: AWS::EC2::SecurityGroupIngress + Condition: CreateInternalALB + Properties: + Description: Ingress from the env security group + GroupId: !Ref InternalLoadBalancerSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref EnvironmentSecurityGroup + PublicLoadBalancer: + Metadata: + 'aws:copilot:description': 'An Application Load Balancer to distribute public traffic to your services' + Condition: CreateALB + Type: AWS::ElasticLoadBalancingV2::LoadBalancer + Properties: + Scheme: internet-facing + SecurityGroups: [ !GetAtt PublicLoadBalancerSecurityGroup.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 + # the listeners for the services. + DefaultHTTPTargetGroup: + Type: AWS::ElasticLoadBalancingV2::TargetGroup + Condition: CreateALB + Properties: + # Check if your application is healthy within 20 = 10*2 seconds, compared to 2.5 mins = 30*5 seconds. + HealthCheckIntervalSeconds: 10 # Default is 30. + HealthyThresholdCount: 2 # Default is 5. + HealthCheckTimeoutSeconds: 5 + Port: 80 + Protocol: HTTP + TargetGroupAttributes: + - Key: deregistration_delay.timeout_seconds + Value: 60 # Default is 300. + TargetType: ip + VpcId: !Ref VPC + HTTPListener: + Metadata: + 'aws:copilot:description': 'A load balancer listener to route HTTP traffic' + Type: AWS::ElasticLoadBalancingV2::Listener + Condition: CreateALB + Properties: + DefaultActions: + - TargetGroupArn: !Ref DefaultHTTPTargetGroup + Type: forward + LoadBalancerArn: !Ref PublicLoadBalancer + Port: 80 + Protocol: HTTP + HTTPSListener: + Metadata: + 'aws:copilot:description': 'A load balancer listener to route HTTPS traffic' + Type: AWS::ElasticLoadBalancingV2::Listener + Condition: ExportHTTPSListener + Properties: + Certificates: + - CertificateArn: cert-1 + DefaultActions: + - TargetGroupArn: !Ref DefaultHTTPTargetGroup + Type: forward + LoadBalancerArn: !Ref PublicLoadBalancer + Port: 443 + Protocol: HTTPS + HTTPSImportCertificate2: + Type: AWS::ElasticLoadBalancingV2::ListenerCertificate + Condition: ExportHTTPSListener + Properties: + ListenerArn: !Ref HTTPSListener + Certificates: + - CertificateArn: cert-2 + InternalLoadBalancer: + Metadata: + 'aws:copilot:description': 'An internal Application Load Balancer to distribute private traffic from within the VPC to your services' + Condition: CreateInternalALB + Type: AWS::ElasticLoadBalancingV2::LoadBalancer + Properties: + Scheme: internal + SecurityGroups: [ !GetAtt InternalLoadBalancerSecurityGroup.GroupId ] + Subnets: [ !Ref PrivateSubnet1, !Ref PrivateSubnet2, ] + Type: application + DefaultInternalHTTPTargetGroup: + Type: AWS::ElasticLoadBalancingV2::TargetGroup + Condition: CreateInternalALB + Properties: + # Check if your application is healthy within 20 = 10*2 seconds, compared to 2.5 mins = 30*5 seconds. + HealthCheckIntervalSeconds: 10 # Default is 30. + HealthyThresholdCount: 2 # Default is 5. + HealthCheckTimeoutSeconds: 5 + Port: 80 + Protocol: HTTP + TargetGroupAttributes: + - Key: deregistration_delay.timeout_seconds + Value: 60 # Default is 300. + TargetType: ip + VpcId: !Ref VPC + InternalHTTPListener: + Metadata: + 'aws:copilot:description': 'An internal load balancer listener to route HTTP traffic' + Type: AWS::ElasticLoadBalancingV2::Listener + Condition: CreateInternalALB + Properties: + DefaultActions: + - TargetGroupArn: !Ref DefaultInternalHTTPTargetGroup + Type: forward + LoadBalancerArn: !Ref InternalLoadBalancer + Port: 80 + Protocol: HTTP + InternalHTTPSListener: + Metadata: + 'aws:copilot:description': 'An internal load balancer listener to route HTTPS traffic' + Type: AWS::ElasticLoadBalancingV2::Listener + Condition: ExportInternalHTTPSListener + Properties: + DefaultActions: + - TargetGroupArn: !Ref DefaultInternalHTTPTargetGroup + Type: forward + LoadBalancerArn: !Ref InternalLoadBalancer + Port: 443 + Protocol: HTTPS + InternalWorkloadsHostedZone: + Metadata: + 'aws:copilot:description': 'A hosted zone named test.demo.internal for backends behind a private load balancer' + Condition: CreateInternalALB + Type: AWS::Route53::HostedZone + Properties: + Name: !Sub ${EnvironmentName}.${AppName}.internal + VPCs: + - VPCId: !Ref VPC + VPCRegion: !Ref AWS::Region + FileSystem: + Condition: CreateEFS + Type: AWS::EFS::FileSystem + Metadata: + 'aws:copilot:description': 'An EFS filesystem for persistent task storage' + Properties: + BackupPolicy: + Status: ENABLED + Encrypted: true + FileSystemPolicy: + Version: '2012-10-17' + Id: CopilotEFSPolicy + Statement: + - Sid: AllowIAMFromTaggedRoles + Effect: Allow + Principal: + AWS: '*' + Action: + - elasticfilesystem:ClientWrite + - elasticfilesystem:ClientMount + Condition: + Bool: + 'elasticfilesystem:AccessedViaMountTarget': true + StringEquals: + 'iam:ResourceTag/copilot-application': !Sub '${AppName}' + 'iam:ResourceTag/copilot-environment': !Sub '${EnvironmentName}' + - Sid: DenyUnencryptedAccess + Effect: Deny + Principal: '*' + Action: 'elasticfilesystem:*' + Condition: + Bool: + 'aws:SecureTransport': false + LifecyclePolicies: + - TransitionToIA: AFTER_30_DAYS + PerformanceMode: generalPurpose + ThroughputMode: bursting + EFSSecurityGroup: + Metadata: + 'aws:copilot:description': 'A security group to allow your containers to talk to EFS storage' + Type: AWS::EC2::SecurityGroup + Condition: CreateEFS + Properties: + GroupDescription: !Join ['', [!Ref AppName, '-', !Ref EnvironmentName, EFSSecurityGroup]] + VpcId: !Ref VPC + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-efs' + EFSSecurityGroupIngressFromEnvironment: + Type: AWS::EC2::SecurityGroupIngress + Condition: CreateEFS + Properties: + Description: Ingress from containers in the Environment Security Group. + GroupId: !Ref EFSSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref EnvironmentSecurityGroup + MountTarget1: + Type: AWS::EFS::MountTarget + Condition: CreateEFS + Properties: + FileSystemId: !Ref FileSystem + SubnetId: !Ref PrivateSubnet1 + SecurityGroups: + - !Ref EFSSecurityGroup + MountTarget2: + Type: AWS::EFS::MountTarget + Condition: CreateEFS + Properties: + FileSystemId: !Ref FileSystem + SubnetId: !Ref PrivateSubnet2 + SecurityGroups: + - !Ref EFSSecurityGroup + # The CloudformationExecutionRole definition must be immediately followed with DeletionPolicy: Retain. + # See #1533. + CloudformationExecutionRole: + Metadata: + 'aws:copilot:description': 'An IAM Role for AWS CloudFormation to manage resources' + DeletionPolicy: Retain + Type: AWS::IAM::Role + Properties: + RoleName: !Sub ${AWS::StackName}-CFNExecutionRole + AssumeRolePolicyDocument: + Version: '2012-10-17' + Statement: + - Effect: Allow + Principal: + Service: + - 'cloudformation.amazonaws.com' + - 'lambda.amazonaws.com' + Action: sts:AssumeRole + Path: / + Policies: + - PolicyName: executeCfn + # This policy is more permissive than the managed PowerUserAccess + # since it allows arbitrary role creation, which is needed for the + # ECS task role specified by the customers. + PolicyDocument: + Version: '2012-10-17' + Statement: + - + Effect: Allow + NotAction: + - 'organizations:*' + - 'account:*' + Resource: '*' + - + Effect: Allow + Action: + - 'organizations:DescribeOrganization' + - 'account:ListRegions' + Resource: '*' + + EnvironmentManagerRole: + Metadata: + 'aws:copilot:description': 'An IAM Role to describe resources in your environment' + DeletionPolicy: Retain + Type: AWS::IAM::Role + DependsOn: CloudformationExecutionRole + Properties: + RoleName: !Sub ${AWS::StackName}-EnvManagerRole + AssumeRolePolicyDocument: + Version: '2012-10-17' + Statement: + - Effect: Allow + Principal: + AWS: !Sub ${ToolsAccountPrincipalARN} + Action: sts:AssumeRole + Path: / + Policies: + - PolicyName: root + PolicyDocument: + Version: '2012-10-17' + Statement: + - Sid: ImportedCertificates + Effect: Allow + Action: [ + acm:DescribeCertificate + ] + Resource: + - "cert-1" + - "cert-2" + - Sid: CloudwatchLogs + Effect: Allow + Action: [ + "logs:GetLogRecord", + "logs:GetQueryResults", + "logs:StartQuery", + "logs:GetLogEvents", + "logs:DescribeLogStreams", + "logs:StopQuery", + "logs:TestMetricFilter", + "logs:FilterLogEvents", + "logs:GetLogGroupFields", + "logs:GetLogDelivery" + ] + Resource: "*" + - Sid: Cloudwatch + Effect: Allow + Action: [ + "cloudwatch:DescribeAlarms" + ] + Resource: "*" + - Sid: ECS + Effect: Allow + Action: [ + "ecs:ListAttributes", + "ecs:ListTasks", + "ecs:DescribeServices", + "ecs:DescribeTaskSets", + "ecs:ListContainerInstances", + "ecs:DescribeContainerInstances", + "ecs:DescribeTasks", + "ecs:DescribeClusters", + "ecs:UpdateService", + "ecs:PutAttributes", + "ecs:StartTelemetrySession", + "ecs:StartTask", + "ecs:StopTask", + "ecs:ListServices", + "ecs:ListTaskDefinitionFamilies", + "ecs:DescribeTaskDefinition", + "ecs:ListTaskDefinitions", + "ecs:ListClusters", + "ecs:RunTask" + ] + Resource: "*" + - Sid: ExecuteCommand + Effect: Allow + Action: [ + "ecs:ExecuteCommand" + ] + Resource: "*" + Condition: + StringEquals: + 'aws:ResourceTag/copilot-application': !Sub '${AppName}' + 'aws:ResourceTag/copilot-environment': !Sub '${EnvironmentName}' + - Sid: StartStateMachine + Effect: Allow + Action: + - "states:StartExecution" + Resource: + - !Sub "arn:aws:states:${AWS::Region}:${AWS::AccountId}:stateMachine:${AppName}-${EnvironmentName}-*" + - Sid: CloudFormation + Effect: Allow + Action: [ + "cloudformation:CancelUpdateStack", + "cloudformation:CreateChangeSet", + "cloudformation:CreateStack", + "cloudformation:DeleteChangeSet", + "cloudformation:DeleteStack", + "cloudformation:Describe*", + "cloudformation:DetectStackDrift", + "cloudformation:DetectStackResourceDrift", + "cloudformation:ExecuteChangeSet", + "cloudformation:GetTemplate", + "cloudformation:GetTemplateSummary", + "cloudformation:UpdateStack", + "cloudformation:UpdateTerminationProtection" + ] + Resource: "*" + - Sid: GetAndPassCopilotRoles + Effect: Allow + Action: [ + "iam:GetRole", + "iam:PassRole" + ] + Resource: "*" + Condition: + StringEquals: + 'iam:ResourceTag/copilot-application': !Sub '${AppName}' + 'iam:ResourceTag/copilot-environment': !Sub '${EnvironmentName}' + - Sid: ECR + Effect: Allow + Action: [ + "ecr:BatchGetImage", + "ecr:BatchCheckLayerAvailability", + "ecr:CompleteLayerUpload", + "ecr:DescribeImages", + "ecr:DescribeRepositories", + "ecr:GetDownloadUrlForLayer", + "ecr:InitiateLayerUpload", + "ecr:ListImages", + "ecr:ListTagsForResource", + "ecr:PutImage", + "ecr:UploadLayerPart", + "ecr:GetAuthorizationToken" + ] + Resource: "*" + - Sid: ResourceGroups + Effect: Allow + Action: [ + "resource-groups:GetGroup", + "resource-groups:GetGroupQuery", + "resource-groups:GetTags", + "resource-groups:ListGroupResources", + "resource-groups:ListGroups", + "resource-groups:SearchResources" + ] + Resource: "*" + - Sid: SSM + Effect: Allow + Action: [ + "ssm:DeleteParameter", + "ssm:DeleteParameters", + "ssm:GetParameter", + "ssm:GetParameters", + "ssm:GetParametersByPath" + ] + Resource: "*" + - Sid: SSMSecret + Effect: Allow + Action: [ + "ssm:PutParameter", + "ssm:AddTagsToResource" + ] + Resource: + - !Sub 'arn:${AWS::Partition}:ssm:${AWS::Region}:${AWS::AccountId}:parameter/copilot/${AppName}/${EnvironmentName}/secrets/*' + - Sid: ELBv2 + Effect: Allow + Action: [ + "elasticloadbalancing:DescribeLoadBalancerAttributes", + "elasticloadbalancing:DescribeSSLPolicies", + "elasticloadbalancing:DescribeLoadBalancers", + "elasticloadbalancing:DescribeTargetGroupAttributes", + "elasticloadbalancing:DescribeListeners", + "elasticloadbalancing:DescribeTags", + "elasticloadbalancing:DescribeTargetHealth", + "elasticloadbalancing:DescribeTargetGroups", + "elasticloadbalancing:DescribeRules" + ] + Resource: "*" + - Sid: BuiltArtifactAccess + Effect: Allow + Action: [ + "s3:ListBucketByTags", + "s3:GetLifecycleConfiguration", + "s3:GetBucketTagging", + "s3:GetInventoryConfiguration", + "s3:GetObjectVersionTagging", + "s3:ListBucketVersions", + "s3:GetBucketLogging", + "s3:ListBucket", + "s3:GetAccelerateConfiguration", + "s3:GetBucketPolicy", + "s3:GetObjectVersionTorrent", + "s3:GetObjectAcl", + "s3:GetEncryptionConfiguration", + "s3:GetBucketRequestPayment", + "s3:GetObjectVersionAcl", + "s3:GetObjectTagging", + "s3:GetMetricsConfiguration", + "s3:HeadBucket", + "s3:GetBucketPublicAccessBlock", + "s3:GetBucketPolicyStatus", + "s3:ListBucketMultipartUploads", + "s3:GetBucketWebsite", + "s3:ListJobs", + "s3:GetBucketVersioning", + "s3:GetBucketAcl", + "s3:GetBucketNotification", + "s3:GetReplicationConfiguration", + "s3:ListMultipartUploadParts", + "s3:GetObject", + "s3:GetObjectTorrent", + "s3:GetAccountPublicAccessBlock", + "s3:ListAllMyBuckets", + "s3:DescribeJob", + "s3:GetBucketCORS", + "s3:GetAnalyticsConfiguration", + "s3:GetObjectVersionForReplication", + "s3:GetBucketLocation", + "s3:GetObjectVersion", + "kms:Decrypt" + ] + Resource: "*" + - Sid: PutObjectsToArtifactBucket + Effect: Allow + Action: + - s3:PutObject + - s3:PutObjectAcl + Resource: + - arn:aws:s3:::mockbucket + - arn:aws:s3:::mockbucket/* + - Sid: EncryptObjectsInArtifactBucket + Effect: Allow + Action: + - kms:GenerateDataKey + Resource: arn:aws:kms:us-west-2:000000000:key/1234abcd-12ab-34cd-56ef-1234567890ab + - Sid: EC2 + Effect: Allow + Action: [ + "ec2:DescribeSubnets", + "ec2:DescribeSecurityGroups", + "ec2:DescribeNetworkInterfaces", + "ec2:DescribeRouteTables" + ] + Resource: "*" + - Sid: AppRunner + Effect: Allow + Action: [ + "apprunner:DescribeService", + "apprunner:ListOperations", + "apprunner:ListServices", + "apprunner:PauseService", + "apprunner:ResumeService", + "apprunner:StartDeployment", + "apprunner:DescribeObservabilityConfiguration" + ] + Resource: "*" + - Sid: Tags + Effect: Allow + Action: [ + "tag:GetResources" + ] + Resource: "*" + - Sid: ApplicationAutoscaling + Effect: Allow + Action: [ + "application-autoscaling:DescribeScalingPolicies" + ] + Resource: "*" + - Sid: DeleteRoles + Effect: Allow + Action: [ + "iam:DeleteRole", + "iam:ListRolePolicies", + "iam:DeleteRolePolicy" + ] + Resource: + - !GetAtt CloudformationExecutionRole.Arn + - !Sub "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/${AWS::StackName}-EnvManagerRole" + - Sid: DeleteEnvStack + Effect: Allow + Action: + - 'cloudformation:DescribeStacks' + - 'cloudformation:DeleteStack' + Resource: + - !Sub 'arn:${AWS::Partition}:cloudformation:${AWS::Region}:${AWS::AccountId}:stack/${AWS::StackName}/*' + +Outputs: + VpcId: + Value: !Ref VPC + Export: + Name: !Sub ${AWS::StackName}-VpcId + PublicSubnets: + Value: !Join [ ',', [ !Ref PublicSubnet1, !Ref PublicSubnet2, ] ] + Export: + Name: !Sub ${AWS::StackName}-PublicSubnets + PrivateSubnets: + Value: !Join [ ',', [ !Ref PrivateSubnet1, !Ref PrivateSubnet2, ] ] + Export: + Name: !Sub ${AWS::StackName}-PrivateSubnets + InternetGatewayID: + Value: !Ref InternetGateway + Export: + Name: !Sub ${AWS::StackName}-InternetGatewayID + PublicRouteTableID: + Value: !Ref PublicRouteTable + Export: + Name: !Sub ${AWS::StackName}-PublicRouteTableID + PrivateRouteTableIDs: + Condition: CreateNATGateways + Value: !Join [ ',', [ !Ref PrivateRouteTable1, !Ref PrivateRouteTable2, ] ] + Export: + Name: !Sub ${AWS::StackName}-PrivateRouteTableIDs + ServiceDiscoveryNamespaceID: + Value: !GetAtt ServiceDiscoveryNamespace.Id + Export: + Name: !Sub ${AWS::StackName}-ServiceDiscoveryNamespaceID + EnvironmentSecurityGroup: + Value: !Ref EnvironmentSecurityGroup + Export: + Name: !Sub ${AWS::StackName}-EnvironmentSecurityGroup + PublicLoadBalancerDNSName: + Condition: CreateALB + Value: !GetAtt PublicLoadBalancer.DNSName + Export: + Name: !Sub ${AWS::StackName}-PublicLoadBalancerDNS + PublicLoadBalancerFullName: + Condition: CreateALB + Value: !GetAtt PublicLoadBalancer.LoadBalancerFullName + Export: + Name: !Sub ${AWS::StackName}-PublicLoadBalancerFullName + PublicLoadBalancerHostedZone: + Condition: CreateALB + Value: !GetAtt PublicLoadBalancer.CanonicalHostedZoneID + Export: + Name: !Sub ${AWS::StackName}-CanonicalHostedZoneID + HTTPListenerArn: + Condition: CreateALB + Value: !Ref HTTPListener + Export: + Name: !Sub ${AWS::StackName}-HTTPListenerArn + HTTPSListenerArn: + Condition: ExportHTTPSListener + Value: !Ref HTTPSListener + Export: + Name: !Sub ${AWS::StackName}-HTTPSListenerArn + DefaultHTTPTargetGroupArn: + Condition: CreateALB + Value: !Ref DefaultHTTPTargetGroup + Export: + Name: !Sub ${AWS::StackName}-DefaultHTTPTargetGroup + InternalLoadBalancerDNSName: + Condition: CreateInternalALB + Value: !GetAtt InternalLoadBalancer.DNSName + Export: + Name: !Sub ${AWS::StackName}-InternalLoadBalancerDNS + InternalLoadBalancerFullName: + Condition: CreateInternalALB + Value: !GetAtt InternalLoadBalancer.LoadBalancerFullName + Export: + Name: !Sub ${AWS::StackName}-InternalLoadBalancerFullName + InternalLoadBalancerHostedZone: + Condition: CreateInternalALB + Value: !GetAtt InternalLoadBalancer.CanonicalHostedZoneID + Export: + Name: !Sub ${AWS::StackName}-InternalLoadBalancerCanonicalHostedZoneID + InternalWorkloadsHostedZone: + Condition: CreateInternalALB + Value: !GetAtt InternalWorkloadsHostedZone.Id + Export: + Name: !Sub ${AWS::StackName}-InternalWorkloadsHostedZoneID + InternalWorkloadsHostedZoneName: + Condition: CreateInternalALB + Value: !Sub ${EnvironmentName}.${AppName}.internal + Export: + Name: !Sub ${AWS::StackName}-InternalWorkloadsHostedZoneName + InternalHTTPListenerArn: + Condition: CreateInternalALB + Value: !Ref InternalHTTPListener + Export: + Name: !Sub ${AWS::StackName}-InternalHTTPListenerArn + InternalHTTPSListenerArn: + Condition: ExportInternalHTTPSListener + Value: !Ref InternalHTTPSListener + Export: + Name: !Sub ${AWS::StackName}-InternalHTTPSListenerArn + InternalLoadBalancerSecurityGroup: + Condition: CreateInternalALB + Value: !Ref InternalLoadBalancerSecurityGroup + Export: + Name: !Sub ${AWS::StackName}-InternalLoadBalancerSecurityGroup + ClusterId: + Value: !Ref Cluster + Export: + Name: !Sub ${AWS::StackName}-ClusterId + EnvironmentManagerRoleARN: + Value: !GetAtt EnvironmentManagerRole.Arn + Description: The role to be assumed by the ecs-cli to manage environments. + Export: + Name: !Sub ${AWS::StackName}-EnvironmentManagerRoleARN + CFNExecutionRoleARN: + Value: !GetAtt CloudformationExecutionRole.Arn + Description: The role to be assumed by the Cloudformation service when it deploys application infrastructure. + Export: + Name: !Sub ${AWS::StackName}-CFNExecutionRoleARN + EnabledFeatures: + Value: !Sub '${ALBWorkloads},${InternalALBWorkloads},${EFSWorkloads},${NATWorkloads},${Aliases}' + Description: Required output to force the stack to update if mutating feature params, like ALBWorkloads, does not change the template. + ManagedFileSystemID: + Condition: CreateEFS + Value: !Ref FileSystem + Description: The ID of the Copilot-managed EFS filesystem. + Export: + Name: !Sub ${AWS::StackName}-FilesystemID + LastForceDeployID: + Value: "" + Description: Optionally force the template to update when no immediate resource change is present. \ No newline at end of file diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 9cbe7fe5cad..2438e15e428 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -102,9 +102,47 @@ type environmentNetworkConfig struct { } type environmentVPCConfig struct { - ID *string `yaml:"id,omitempty"` - CIDR *IPNet `yaml:"cidr,omitempty"` - Subnets subnetsConfiguration `yaml:"subnets,omitempty"` + ID *string `yaml:"id,omitempty"` + CIDR *IPNet `yaml:"cidr,omitempty"` + Subnets subnetsConfiguration `yaml:"subnets,omitempty"` + SecurityGroupConfig securityGroupConfig `yaml:"security_group,omitempty"` +} + +type securityGroupConfig struct { + Ingress []securityGroupRules `yaml:"ingress,omitempty"` + Egress []securityGroupRules `yaml:"egress,omitempty"` +} + +// IsEmpty returns true if there is no security group rule defined. +func (sgc securityGroupConfig) IsEmpty() bool { + return len(sgc.Ingress) == 0 && len(sgc.Egress) == 0 +} + +type securityGroupRules struct { + CidrIp string `yaml:"cidr"` + FromPort int `yaml:"from_port"` + IpProtocol string `yaml:"ip_protocol"` + ToPort int `yaml:"to_port"` +} + +//IsValid checks for valid security group config. +func (sgc securityGroupConfig) IsValid() bool { + for _, ingress := range sgc.Ingress { + if ingress.IsEmpty() { + return true + } + } + for _, egress := range sgc.Egress { + if egress.IsEmpty() { + return true + } + } + return false +} + +//IsEmpty checks for required parameters in the security group rules. +func (sgr securityGroupRules) IsEmpty() bool { + return sgr.CidrIp == "" || sgr.IpProtocol == "" } type environmentCDNConfig struct { diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index c7562cc7d62..06ba8767fd9 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -113,6 +113,8 @@ type EnvOpts struct { AllowVPCIngress bool Telemetry *Telemetry + SecurityGroupConfig *SecurityGroupConfig + CDNConfig *CDNConfig // If nil, no cdn is to be used LatestVersion string @@ -148,6 +150,19 @@ type Telemetry struct { EnableContainerInsights bool } +// SecurityGroupConfig holds the fields to import security group config +type SecurityGroupConfig struct { + Ingress []SecurityGroupRules + Egress []SecurityGroupRules +} + +type SecurityGroupRules struct { + CidrIp string + FromPort int + IpProtocol string + ToPort int +} + // ParseEnv parses an environment's CloudFormation template with the specified data object and returns its content. func (t *Template) ParseEnv(data *EnvOpts) (*Content, error) { tpl, err := t.parse("base", envCFTemplatePath, withEnvParsingFuncs()) diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index e49c201b8d4..36db0fef2fa 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -189,6 +189,20 @@ Resources: Tags: - Key: Name Value: !Sub 'copilot-${AppName}-${EnvironmentName}-env' +{{- if and .SecurityGroupConfig .SecurityGroupConfig.Ingress }} + SecurityGroupIngress: {{range $ind, $securityRule := .SecurityGroupConfig.Ingress}} + - IpProtocol: {{$securityRule.IpProtocol}} + FromPort: {{$securityRule.FromPort}} + ToPort: {{$securityRule.ToPort}} + CidrIp: {{$securityRule.CidrIp}} +{{end }}{{- end}} +{{- if and .SecurityGroupConfig .SecurityGroupConfig.Egress }} + SecurityGroupEgress: {{range $ind, $securityRule := .SecurityGroupConfig.Egress}} + - IpProtocol: {{$securityRule.IpProtocol}} + FromPort: {{$securityRule.FromPort}} + ToPort: {{$securityRule.ToPort}} + CidrIp: {{$securityRule.CidrIp}} +{{end }}{{- end}} EnvironmentHTTPSecurityGroupIngressFromPublicALB: Type: AWS::EC2::SecurityGroupIngress Condition: CreateALB From 909b0934e05cd56dae09a63fcef1785cf68ee4b1 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Mon, 25 Jul 2022 20:10:48 -0700 Subject: [PATCH 02/19] fix integ test --- .../template-with-custom-security-group.yml | 49 +++++++++++++------ internal/pkg/manifest/validate_env.go | 12 +++++ 2 files changed, 47 insertions(+), 14 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml index 13384d5f2d8..12439528f9a 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml @@ -276,19 +276,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: ExportHTTPSListener + 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 @@ -334,14 +346,6 @@ Resources: FromPort: 0 IpProtocol: tcp ToPort: 65535 - EnvironmentSecurityGroupIngressFromPublicALB: - Type: AWS::EC2::SecurityGroupIngress - Condition: CreateALB - Properties: - Description: Ingress from the public ALB - GroupId: !Ref EnvironmentSecurityGroup - IpProtocol: -1 - SourceSecurityGroupId: !Ref PublicLoadBalancerSecurityGroup EnvironmentSecurityGroupIngressFromInternalALB: Type: AWS::EC2::SecurityGroupIngress Condition: CreateInternalALB @@ -372,8 +376,10 @@ Resources: Type: AWS::ElasticLoadBalancingV2::LoadBalancer Properties: Scheme: internet-facing - SecurityGroups: [ !GetAtt PublicLoadBalancerSecurityGroup.GroupId ] - Subnets: [ !Ref PublicSubnet1, !Ref PublicSubnet2, ] + 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 # the listeners for the services. @@ -595,7 +601,22 @@ Resources: - 'organizations:DescribeOrganization' - 'account:ListRegions' Resource: '*' - + 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: ExportHTTPSListener + Properties: + Description: HTTPS ingress from the public ALB + GroupId: !Ref EnvironmentSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref PublicHTTPSLoadBalancerSecurityGroup EnvironmentManagerRole: Metadata: 'aws:copilot:description': 'An IAM Role to describe resources in your environment' diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index b5d2b64fb3b..bf8fed3dd03 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -81,6 +81,18 @@ func (cfg environmentVPCConfig) validate() error { return nil } +// validate returns true if there is no security group rule defined. +func (cfg securityGroupRules) validate() error { + //no-op + return nil +} + +// validate returns true if there is no security group rule defined. +func (cfg securityGroupConfig) validate() error { + //no-op + return nil +} + func (cfg environmentVPCConfig) validateImportedVPC() error { for idx, subnet := range cfg.Subnets.Public { if aws.StringValue(subnet.SubnetID) == "" { From 89e2d691d29121ca884cd3b850b7e63d1649ca50 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Tue, 26 Jul 2022 00:38:56 -0700 Subject: [PATCH 03/19] Address feedback --- .../pkg/deploy/cloudformation/stack/env.go | 39 +- .../stack/env_integration_test.go | 43 + .../deploy/cloudformation/stack/env_test.go | 6 +- ...plate-with-custom-empty-security-group.yml | 1010 +++++++++++++++++ .../cloudformation/stack/transformers.go | 25 + internal/pkg/manifest/env.go | 37 +- internal/pkg/manifest/validate_env.go | 13 +- internal/pkg/template/env.go | 16 +- .../pkg/template/templates/environment/cf.yml | 20 +- 9 files changed, 1137 insertions(+), 72 deletions(-) create mode 100644 internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-empty-security-group.yml diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index d413c106090..8e44ce53b50 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -10,7 +10,6 @@ import ( "github.com/aws/aws-sdk-go/service/cloudformation" "github.com/aws/copilot-cli/internal/pkg/config" "github.com/aws/copilot-cli/internal/pkg/deploy" - "github.com/aws/copilot-cli/internal/pkg/manifest" "github.com/aws/copilot-cli/internal/pkg/template" "github.com/google/uuid" ) @@ -95,7 +94,6 @@ func (e *EnvStackConfig) Template() (string, error) { } forceUpdateID = id.String() } - securityGroupConfig, err := getSecurityGroupConfig(e.in.Mft) if err != nil { return "", err } @@ -112,7 +110,6 @@ func (e *EnvStackConfig) Template() (string, error) { CustomInternalALBSubnets: e.internalALBSubnets(), AllowVPCIngress: aws.BoolValue(e.in.Mft.HTTPConfig.Private.SecurityGroupsConfig.Ingress.VPCIngress), Telemetry: e.telemetryConfig(), - SecurityGroupConfig: securityGroupConfig, CDNConfig: e.cdnConfig(), Version: e.in.Version, @@ -123,6 +120,7 @@ func (e *EnvStackConfig) Template() (string, error) { if err != nil { return "", err } + fmt.Println("printing cfn", content.String()) return content.String(), nil } @@ -198,36 +196,6 @@ func (e *EnvStackConfig) Parameters() ([]*cloudformation.Parameter, error) { return e.transformParameters(currParams, e.prevParams, transformEnvControllerParameters) } -func getSecurityGroupConfig(mft *manifest.Environment) (*template.SecurityGroupConfig, error) { - if mft != nil && !mft.Network.VPC.SecurityGroupConfig.IsEmpty() { - if !mft.Network.VPC.SecurityGroupConfig.IsValid() { - var ingress = make([]template.SecurityGroupRules, len(mft.Network.VPC.SecurityGroupConfig.Ingress)) - var egress = make([]template.SecurityGroupRules, len(mft.Network.VPC.SecurityGroupConfig.Egress)) - for idx, ingressValue := range mft.Network.VPC.SecurityGroupConfig.Ingress { - ingress[idx].IpProtocol = ingressValue.IpProtocol - ingress[idx].CidrIp = ingressValue.CidrIp - ingress[idx].ToPort = ingressValue.ToPort - ingress[idx].FromPort = ingressValue.FromPort - } - for idx, egressValue := range mft.Network.VPC.SecurityGroupConfig.Egress { - egress[idx].IpProtocol = egressValue.IpProtocol - egress[idx].CidrIp = egressValue.CidrIp - egress[idx].ToPort = egressValue.ToPort - egress[idx].FromPort = egressValue.FromPort - } - - return &template.SecurityGroupConfig{ - Ingress: ingress, - Egress: egress, - }, nil - } else { - return &template.SecurityGroupConfig{}, fmt.Errorf("security group config is invalid") - } - } else { - return &template.SecurityGroupConfig{}, nil - } -} - // SerializedParameters returns the CloudFormation stack's parameters serialized to a JSON document. func (e *EnvStackConfig) SerializedParameters() (string, error) { return serializeTemplateConfig(e.parser, e) @@ -391,8 +359,9 @@ func (e *EnvStackConfig) cdnConfig() *template.CDNConfig { func (e *EnvStackConfig) vpcConfig() template.VPCConfig { return template.VPCConfig{ - Imported: e.importVPC(), - Managed: e.managedVPC(), + Imported: e.importVPC(), + Managed: e.managedVPC(), + SecurityGroupConfig: convertEnvSecurityGroupCfg(e.in.Mft), } } diff --git a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go index 54499fa7383..e3def4ee1d0 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go @@ -126,6 +126,49 @@ network: wantedFileName: "template-with-custom-security-group.yml", }, + "generate template with embedded manifest file with empty security groups rules added by the customer": { + input: func() *deploy.CreateEnvironmentInput { + rawMft := `name: test +type: Environment +# Create the public ALB with certificates attached. +http: + public: + certificates: + - cert-1 + - cert-2 +observability: + container_insights: true # Enable container insights. +network: + vpc: + security_group: + ingress: + egress:` + var mft manifest.Environment + err := yaml.Unmarshal([]byte(rawMft), &mft) + require.NoError(t, err) + return &deploy.CreateEnvironmentInput{ + Version: "1.x", + App: deploy.AppInformation{ + AccountPrincipalARN: "arn:aws:iam::000000000:root", + Name: "demo", + }, + Name: "test", + ArtifactBucketARN: "arn:aws:s3:::mockbucket", + ArtifactBucketKeyARN: "arn:aws:kms:us-west-2:000000000:key/1234abcd-12ab-34cd-56ef-1234567890ab", + CustomResourcesURLs: map[string]string{ + "CertificateValidationFunction": "https://mockbucket.s3-us-west-2.amazonaws.com/dns-cert-validator", + "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), + } + }(), + + wantedFileName: "template-with-custom-empty-security-group.yml", + }, + "generate template with custom resources": { input: func() *deploy.CreateEnvironmentInput { rawMft := `name: test diff --git a/internal/pkg/deploy/cloudformation/stack/env_test.go b/internal/pkg/deploy/cloudformation/stack/env_test.go index 3e5698ca7bd..edb8e1e1fd4 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_test.go @@ -40,6 +40,7 @@ func TestEnv_Template(t *testing.T) { PrivateSubnetCIDRs: DefaultPrivateSubnetCIDRs, PublicSubnetCIDRs: DefaultPublicSubnetCIDRs, }, + SecurityGroupConfig: template.SecurityGroupConfig{}, }, LatestVersion: deploy.LatestEnvTemplateVersion, CustomResources: map[string]template.S3ObjectLocation{ @@ -59,9 +60,8 @@ func TestEnv_Template(t *testing.T) { Telemetry: &template.Telemetry{ EnableContainerInsights: false, }, - SecurityGroupConfig: &template.SecurityGroupConfig{}, - SerializedManifest: "name: env\ntype: Environment\n", - ForceUpdateID: "mockPreviousForceUpdateID", + SerializedManifest: "name: env\ntype: Environment\n", + ForceUpdateID: "mockPreviousForceUpdateID", }, data) return &template.Content{Buffer: bytes.NewBufferString("mockTemplate")}, nil }) diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-empty-security-group.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-empty-security-group.yml new file mode 100644 index 00000000000..83efd713e1c --- /dev/null +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-empty-security-group.yml @@ -0,0 +1,1010 @@ +# Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: MIT-0 +Description: CloudFormation environment template for infrastructure shared among Copilot workloads. +Metadata: + Manifest: | + name: test + type: Environment + # Create the public ALB with certificates attached. + http: + public: + certificates: + - cert-1 + - cert-2 + observability: + container_insights: true # Enable container insights. + network: + vpc: + security_group: + ingress: + egress: +Parameters: + AppName: + Type: String + EnvironmentName: + Type: String + ALBWorkloads: + Type: String + InternalALBWorkloads: + Type: String + EFSWorkloads: + Type: String + NATWorkloads: + Type: String + ToolsAccountPrincipalARN: + Type: String + AppDNSName: + Type: String + AppDNSDelegationRole: + Type: String + Aliases: + Type: String + CreateHTTPSListener: + Type: String + AllowedValues: [true, false] + CreateInternalHTTPSListener: + Type: String + AllowedValues: [true, false] + ServiceDiscoveryEndpoint: + Type: String +Conditions: + CreateALB: + !Not [!Equals [ !Ref ALBWorkloads, "" ]] + DelegateDNS: + !Not [!Equals [ !Ref AppDNSName, "" ]] + ExportHTTPSListener: !And + - !Condition CreateALB + - !Equals [!Ref CreateHTTPSListener, true] + ExportInternalHTTPSListener: !And + - !Condition CreateInternalALB + - !Equals [ !Ref CreateInternalHTTPSListener, true] + CreateEFS: + !Not [!Equals [ !Ref EFSWorkloads, ""]] + CreateInternalALB: + !Not [!Equals [ !Ref InternalALBWorkloads, ""]] + CreateNATGateways: + !Not [!Equals [ !Ref NATWorkloads, ""]] + ManagedAliases: !And + - !Condition DelegateDNS + - !Not [!Equals [ !Ref Aliases, "" ]] +Resources: + VPC: + Metadata: + 'aws:copilot:description': 'A Virtual Private Cloud to control networking of your AWS resources' + Type: AWS::EC2::VPC + Properties: + CidrBlock: 10.0.0.0/16 + EnableDnsHostnames: true + EnableDnsSupport: true + InstanceTenancy: default + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}' + + PublicRouteTable: + Metadata: + 'aws:copilot:description': "A custom route table that directs network traffic for the public subnets" + Type: AWS::EC2::RouteTable + Properties: + VpcId: !Ref VPC + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}' + + DefaultPublicRoute: + Type: AWS::EC2::Route + DependsOn: InternetGatewayAttachment + Properties: + RouteTableId: !Ref PublicRouteTable + DestinationCidrBlock: 0.0.0.0/0 + GatewayId: !Ref InternetGateway + + InternetGateway: + Metadata: + 'aws:copilot:description': 'An Internet Gateway to connect to the public internet' + Type: AWS::EC2::InternetGateway + Properties: + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}' + + InternetGatewayAttachment: + Type: AWS::EC2::VPCGatewayAttachment + Properties: + InternetGatewayId: !Ref InternetGateway + VpcId: !Ref VPC + PublicSubnet1: + Metadata: + 'aws:copilot:description': 'Public subnet 1 for resources that can access the internet' + Type: AWS::EC2::Subnet + Properties: + CidrBlock: 10.0.0.0/24 + VpcId: !Ref VPC + AvailabilityZone: !Select [ 0, !GetAZs '' ] + MapPublicIpOnLaunch: true + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-pub0' + PublicSubnet2: + Metadata: + 'aws:copilot:description': 'Public subnet 2 for resources that can access the internet' + Type: AWS::EC2::Subnet + Properties: + CidrBlock: 10.0.1.0/24 + VpcId: !Ref VPC + AvailabilityZone: !Select [ 1, !GetAZs '' ] + MapPublicIpOnLaunch: true + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-pub1' + PrivateSubnet1: + Metadata: + 'aws:copilot:description': 'Private subnet 1 for resources with no internet access' + Type: AWS::EC2::Subnet + Properties: + CidrBlock: 10.0.2.0/24 + VpcId: !Ref VPC + AvailabilityZone: !Select [ 0, !GetAZs '' ] + MapPublicIpOnLaunch: false + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-priv0' + PrivateSubnet2: + Metadata: + 'aws:copilot:description': 'Private subnet 2 for resources with no internet access' + Type: AWS::EC2::Subnet + Properties: + CidrBlock: 10.0.3.0/24 + VpcId: !Ref VPC + AvailabilityZone: !Select [ 1, !GetAZs '' ] + MapPublicIpOnLaunch: false + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-priv1' + PublicSubnet1RouteTableAssociation: + Type: AWS::EC2::SubnetRouteTableAssociation + Properties: + RouteTableId: !Ref PublicRouteTable + SubnetId: !Ref PublicSubnet1 + PublicSubnet2RouteTableAssociation: + Type: AWS::EC2::SubnetRouteTableAssociation + Properties: + RouteTableId: !Ref PublicRouteTable + SubnetId: !Ref PublicSubnet2 + + NatGateway1Attachment: + Type: AWS::EC2::EIP + Condition: CreateNATGateways + DependsOn: InternetGatewayAttachment + Properties: + Domain: vpc + NatGateway1: + Metadata: + 'aws:copilot:description': 'NAT Gateway 1 enabling workloads placed in private subnet 1 to reach the internet' + Type: AWS::EC2::NatGateway + Condition: CreateNATGateways + Properties: + AllocationId: !GetAtt NatGateway1Attachment.AllocationId + SubnetId: !Ref PublicSubnet1 + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-0' + PrivateRouteTable1: + Type: AWS::EC2::RouteTable + Condition: CreateNATGateways + Properties: + VpcId: !Ref 'VPC' + PrivateRoute1: + Type: AWS::EC2::Route + Condition: CreateNATGateways + Properties: + RouteTableId: !Ref PrivateRouteTable1 + DestinationCidrBlock: 0.0.0.0/0 + NatGatewayId: !Ref NatGateway1 + PrivateRouteTable1Association: + Type: AWS::EC2::SubnetRouteTableAssociation + Condition: CreateNATGateways + Properties: + RouteTableId: !Ref PrivateRouteTable1 + SubnetId: !Ref PrivateSubnet1 + NatGateway2Attachment: + Type: AWS::EC2::EIP + Condition: CreateNATGateways + DependsOn: InternetGatewayAttachment + Properties: + Domain: vpc + NatGateway2: + Metadata: + 'aws:copilot:description': 'NAT Gateway 2 enabling workloads placed in private subnet 2 to reach the internet' + Type: AWS::EC2::NatGateway + Condition: CreateNATGateways + Properties: + AllocationId: !GetAtt NatGateway2Attachment.AllocationId + SubnetId: !Ref PublicSubnet2 + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-1' + PrivateRouteTable2: + Type: AWS::EC2::RouteTable + Condition: CreateNATGateways + Properties: + VpcId: !Ref 'VPC' + PrivateRoute2: + Type: AWS::EC2::Route + Condition: CreateNATGateways + Properties: + RouteTableId: !Ref PrivateRouteTable2 + DestinationCidrBlock: 0.0.0.0/0 + NatGatewayId: !Ref NatGateway2 + PrivateRouteTable2Association: + Type: AWS::EC2::SubnetRouteTableAssociation + Condition: CreateNATGateways + Properties: + RouteTableId: !Ref PrivateRouteTable2 + SubnetId: !Ref PrivateSubnet2 + # Creates a service discovery namespace with the form provided in the parameter. + # For new environments after 1.5.0, this is "env.app.local". For upgraded environments from + # before 1.5.0, this is app.local. + ServiceDiscoveryNamespace: + Metadata: + 'aws:copilot:description': 'A private DNS namespace for discovering services within the environment' + Type: AWS::ServiceDiscovery::PrivateDnsNamespace + Properties: + Name: !Ref ServiceDiscoveryEndpoint + Vpc: !Ref VPC + Cluster: + Metadata: + 'aws:copilot:description': 'An ECS cluster to group your services' + Type: AWS::ECS::Cluster + Properties: + CapacityProviders: ['FARGATE', 'FARGATE_SPOT'] + Configuration: + ExecuteCommandConfiguration: + Logging: DEFAULT + ClusterSettings: + - Name: containerInsights + Value: enabled + PublicHTTPLoadBalancerSecurityGroup: + Metadata: + 'aws:copilot:description': 'A security group for your load balancer allowing HTTP traffic' + Condition: CreateALB + Type: AWS::EC2::SecurityGroup + Properties: + 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: ExportHTTPSListener + 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 + IpProtocol: tcp + ToPort: 443 + VpcId: !Ref VPC + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-lb' + InternalLoadBalancerSecurityGroup: + Metadata: + 'aws:copilot:description': 'A security group for your internal load balancer allowing HTTP traffic from within the VPC' + Condition: CreateInternalALB + Type: AWS::EC2::SecurityGroup + Properties: + GroupDescription: Access to the internal load balancer + VpcId: !Ref VPC + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-internal-lb' + # Only accept requests coming from the public ALB or other containers in the same security group. + EnvironmentSecurityGroup: + Metadata: + 'aws:copilot:description': 'A security group to allow your containers to talk to each other' + Type: AWS::EC2::SecurityGroup + Properties: + GroupDescription: !Join ['', [!Ref AppName, '-', !Ref EnvironmentName, EnvironmentSecurityGroup]] + VpcId: !Ref VPC + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-env' + EnvironmentSecurityGroupIngressFromInternalALB: + Type: AWS::EC2::SecurityGroupIngress + Condition: CreateInternalALB + Properties: + Description: Ingress from the internal ALB + GroupId: !Ref EnvironmentSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref InternalLoadBalancerSecurityGroup + EnvironmentSecurityGroupIngressFromSelf: + Type: AWS::EC2::SecurityGroupIngress + Properties: + Description: Ingress from other containers in the same security group + GroupId: !Ref EnvironmentSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref EnvironmentSecurityGroup + InternalALBIngressFromEnvironmentSecurityGroup: + Type: AWS::EC2::SecurityGroupIngress + Condition: CreateInternalALB + Properties: + Description: Ingress from the env security group + GroupId: !Ref InternalLoadBalancerSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref EnvironmentSecurityGroup + PublicLoadBalancer: + Metadata: + 'aws:copilot:description': 'An Application Load Balancer to distribute public traffic to your services' + Condition: CreateALB + Type: AWS::ElasticLoadBalancingV2::LoadBalancer + Properties: + Scheme: internet-facing + 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 + # the listeners for the services. + DefaultHTTPTargetGroup: + Type: AWS::ElasticLoadBalancingV2::TargetGroup + Condition: CreateALB + Properties: + # Check if your application is healthy within 20 = 10*2 seconds, compared to 2.5 mins = 30*5 seconds. + HealthCheckIntervalSeconds: 10 # Default is 30. + HealthyThresholdCount: 2 # Default is 5. + HealthCheckTimeoutSeconds: 5 + Port: 80 + Protocol: HTTP + TargetGroupAttributes: + - Key: deregistration_delay.timeout_seconds + Value: 60 # Default is 300. + TargetType: ip + VpcId: !Ref VPC + HTTPListener: + Metadata: + 'aws:copilot:description': 'A load balancer listener to route HTTP traffic' + Type: AWS::ElasticLoadBalancingV2::Listener + Condition: CreateALB + Properties: + DefaultActions: + - TargetGroupArn: !Ref DefaultHTTPTargetGroup + Type: forward + LoadBalancerArn: !Ref PublicLoadBalancer + Port: 80 + Protocol: HTTP + HTTPSListener: + Metadata: + 'aws:copilot:description': 'A load balancer listener to route HTTPS traffic' + Type: AWS::ElasticLoadBalancingV2::Listener + Condition: ExportHTTPSListener + Properties: + Certificates: + - CertificateArn: cert-1 + DefaultActions: + - TargetGroupArn: !Ref DefaultHTTPTargetGroup + Type: forward + LoadBalancerArn: !Ref PublicLoadBalancer + Port: 443 + Protocol: HTTPS + HTTPSImportCertificate2: + Type: AWS::ElasticLoadBalancingV2::ListenerCertificate + Condition: ExportHTTPSListener + Properties: + ListenerArn: !Ref HTTPSListener + Certificates: + - CertificateArn: cert-2 + InternalLoadBalancer: + Metadata: + 'aws:copilot:description': 'An internal Application Load Balancer to distribute private traffic from within the VPC to your services' + Condition: CreateInternalALB + Type: AWS::ElasticLoadBalancingV2::LoadBalancer + Properties: + Scheme: internal + SecurityGroups: [ !GetAtt InternalLoadBalancerSecurityGroup.GroupId ] + Subnets: [ !Ref PrivateSubnet1, !Ref PrivateSubnet2, ] + Type: application + DefaultInternalHTTPTargetGroup: + Type: AWS::ElasticLoadBalancingV2::TargetGroup + Condition: CreateInternalALB + Properties: + # Check if your application is healthy within 20 = 10*2 seconds, compared to 2.5 mins = 30*5 seconds. + HealthCheckIntervalSeconds: 10 # Default is 30. + HealthyThresholdCount: 2 # Default is 5. + HealthCheckTimeoutSeconds: 5 + Port: 80 + Protocol: HTTP + TargetGroupAttributes: + - Key: deregistration_delay.timeout_seconds + Value: 60 # Default is 300. + TargetType: ip + VpcId: !Ref VPC + InternalHTTPListener: + Metadata: + 'aws:copilot:description': 'An internal load balancer listener to route HTTP traffic' + Type: AWS::ElasticLoadBalancingV2::Listener + Condition: CreateInternalALB + Properties: + DefaultActions: + - TargetGroupArn: !Ref DefaultInternalHTTPTargetGroup + Type: forward + LoadBalancerArn: !Ref InternalLoadBalancer + Port: 80 + Protocol: HTTP + InternalHTTPSListener: + Metadata: + 'aws:copilot:description': 'An internal load balancer listener to route HTTPS traffic' + Type: AWS::ElasticLoadBalancingV2::Listener + Condition: ExportInternalHTTPSListener + Properties: + DefaultActions: + - TargetGroupArn: !Ref DefaultInternalHTTPTargetGroup + Type: forward + LoadBalancerArn: !Ref InternalLoadBalancer + Port: 443 + Protocol: HTTPS + InternalWorkloadsHostedZone: + Metadata: + 'aws:copilot:description': 'A hosted zone named test.demo.internal for backends behind a private load balancer' + Condition: CreateInternalALB + Type: AWS::Route53::HostedZone + Properties: + Name: !Sub ${EnvironmentName}.${AppName}.internal + VPCs: + - VPCId: !Ref VPC + VPCRegion: !Ref AWS::Region + FileSystem: + Condition: CreateEFS + Type: AWS::EFS::FileSystem + Metadata: + 'aws:copilot:description': 'An EFS filesystem for persistent task storage' + Properties: + BackupPolicy: + Status: ENABLED + Encrypted: true + FileSystemPolicy: + Version: '2012-10-17' + Id: CopilotEFSPolicy + Statement: + - Sid: AllowIAMFromTaggedRoles + Effect: Allow + Principal: + AWS: '*' + Action: + - elasticfilesystem:ClientWrite + - elasticfilesystem:ClientMount + Condition: + Bool: + 'elasticfilesystem:AccessedViaMountTarget': true + StringEquals: + 'iam:ResourceTag/copilot-application': !Sub '${AppName}' + 'iam:ResourceTag/copilot-environment': !Sub '${EnvironmentName}' + - Sid: DenyUnencryptedAccess + Effect: Deny + Principal: '*' + Action: 'elasticfilesystem:*' + Condition: + Bool: + 'aws:SecureTransport': false + LifecyclePolicies: + - TransitionToIA: AFTER_30_DAYS + PerformanceMode: generalPurpose + ThroughputMode: bursting + EFSSecurityGroup: + Metadata: + 'aws:copilot:description': 'A security group to allow your containers to talk to EFS storage' + Type: AWS::EC2::SecurityGroup + Condition: CreateEFS + Properties: + GroupDescription: !Join ['', [!Ref AppName, '-', !Ref EnvironmentName, EFSSecurityGroup]] + VpcId: !Ref VPC + Tags: + - Key: Name + Value: !Sub 'copilot-${AppName}-${EnvironmentName}-efs' + EFSSecurityGroupIngressFromEnvironment: + Type: AWS::EC2::SecurityGroupIngress + Condition: CreateEFS + Properties: + Description: Ingress from containers in the Environment Security Group. + GroupId: !Ref EFSSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref EnvironmentSecurityGroup + MountTarget1: + Type: AWS::EFS::MountTarget + Condition: CreateEFS + Properties: + FileSystemId: !Ref FileSystem + SubnetId: !Ref PrivateSubnet1 + SecurityGroups: + - !Ref EFSSecurityGroup + MountTarget2: + Type: AWS::EFS::MountTarget + Condition: CreateEFS + Properties: + FileSystemId: !Ref FileSystem + SubnetId: !Ref PrivateSubnet2 + SecurityGroups: + - !Ref EFSSecurityGroup + # The CloudformationExecutionRole definition must be immediately followed with DeletionPolicy: Retain. + # See #1533. + CloudformationExecutionRole: + Metadata: + 'aws:copilot:description': 'An IAM Role for AWS CloudFormation to manage resources' + DeletionPolicy: Retain + Type: AWS::IAM::Role + Properties: + RoleName: !Sub ${AWS::StackName}-CFNExecutionRole + AssumeRolePolicyDocument: + Version: '2012-10-17' + Statement: + - Effect: Allow + Principal: + Service: + - 'cloudformation.amazonaws.com' + - 'lambda.amazonaws.com' + Action: sts:AssumeRole + Path: / + Policies: + - PolicyName: executeCfn + # This policy is more permissive than the managed PowerUserAccess + # since it allows arbitrary role creation, which is needed for the + # ECS task role specified by the customers. + PolicyDocument: + Version: '2012-10-17' + Statement: + - + Effect: Allow + NotAction: + - 'organizations:*' + - 'account:*' + Resource: '*' + - + Effect: Allow + Action: + - 'organizations:DescribeOrganization' + - 'account:ListRegions' + Resource: '*' + 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: ExportHTTPSListener + Properties: + Description: HTTPS ingress from the public ALB + GroupId: !Ref EnvironmentSecurityGroup + IpProtocol: -1 + SourceSecurityGroupId: !Ref PublicHTTPSLoadBalancerSecurityGroup + EnvironmentManagerRole: + Metadata: + 'aws:copilot:description': 'An IAM Role to describe resources in your environment' + DeletionPolicy: Retain + Type: AWS::IAM::Role + DependsOn: CloudformationExecutionRole + Properties: + RoleName: !Sub ${AWS::StackName}-EnvManagerRole + AssumeRolePolicyDocument: + Version: '2012-10-17' + Statement: + - Effect: Allow + Principal: + AWS: !Sub ${ToolsAccountPrincipalARN} + Action: sts:AssumeRole + Path: / + Policies: + - PolicyName: root + PolicyDocument: + Version: '2012-10-17' + Statement: + - Sid: ImportedCertificates + Effect: Allow + Action: [ + acm:DescribeCertificate + ] + Resource: + - "cert-1" + - "cert-2" + - Sid: CloudwatchLogs + Effect: Allow + Action: [ + "logs:GetLogRecord", + "logs:GetQueryResults", + "logs:StartQuery", + "logs:GetLogEvents", + "logs:DescribeLogStreams", + "logs:StopQuery", + "logs:TestMetricFilter", + "logs:FilterLogEvents", + "logs:GetLogGroupFields", + "logs:GetLogDelivery" + ] + Resource: "*" + - Sid: Cloudwatch + Effect: Allow + Action: [ + "cloudwatch:DescribeAlarms" + ] + Resource: "*" + - Sid: ECS + Effect: Allow + Action: [ + "ecs:ListAttributes", + "ecs:ListTasks", + "ecs:DescribeServices", + "ecs:DescribeTaskSets", + "ecs:ListContainerInstances", + "ecs:DescribeContainerInstances", + "ecs:DescribeTasks", + "ecs:DescribeClusters", + "ecs:UpdateService", + "ecs:PutAttributes", + "ecs:StartTelemetrySession", + "ecs:StartTask", + "ecs:StopTask", + "ecs:ListServices", + "ecs:ListTaskDefinitionFamilies", + "ecs:DescribeTaskDefinition", + "ecs:ListTaskDefinitions", + "ecs:ListClusters", + "ecs:RunTask" + ] + Resource: "*" + - Sid: ExecuteCommand + Effect: Allow + Action: [ + "ecs:ExecuteCommand" + ] + Resource: "*" + Condition: + StringEquals: + 'aws:ResourceTag/copilot-application': !Sub '${AppName}' + 'aws:ResourceTag/copilot-environment': !Sub '${EnvironmentName}' + - Sid: StartStateMachine + Effect: Allow + Action: + - "states:StartExecution" + Resource: + - !Sub "arn:aws:states:${AWS::Region}:${AWS::AccountId}:stateMachine:${AppName}-${EnvironmentName}-*" + - Sid: CloudFormation + Effect: Allow + Action: [ + "cloudformation:CancelUpdateStack", + "cloudformation:CreateChangeSet", + "cloudformation:CreateStack", + "cloudformation:DeleteChangeSet", + "cloudformation:DeleteStack", + "cloudformation:Describe*", + "cloudformation:DetectStackDrift", + "cloudformation:DetectStackResourceDrift", + "cloudformation:ExecuteChangeSet", + "cloudformation:GetTemplate", + "cloudformation:GetTemplateSummary", + "cloudformation:UpdateStack", + "cloudformation:UpdateTerminationProtection" + ] + Resource: "*" + - Sid: GetAndPassCopilotRoles + Effect: Allow + Action: [ + "iam:GetRole", + "iam:PassRole" + ] + Resource: "*" + Condition: + StringEquals: + 'iam:ResourceTag/copilot-application': !Sub '${AppName}' + 'iam:ResourceTag/copilot-environment': !Sub '${EnvironmentName}' + - Sid: ECR + Effect: Allow + Action: [ + "ecr:BatchGetImage", + "ecr:BatchCheckLayerAvailability", + "ecr:CompleteLayerUpload", + "ecr:DescribeImages", + "ecr:DescribeRepositories", + "ecr:GetDownloadUrlForLayer", + "ecr:InitiateLayerUpload", + "ecr:ListImages", + "ecr:ListTagsForResource", + "ecr:PutImage", + "ecr:UploadLayerPart", + "ecr:GetAuthorizationToken" + ] + Resource: "*" + - Sid: ResourceGroups + Effect: Allow + Action: [ + "resource-groups:GetGroup", + "resource-groups:GetGroupQuery", + "resource-groups:GetTags", + "resource-groups:ListGroupResources", + "resource-groups:ListGroups", + "resource-groups:SearchResources" + ] + Resource: "*" + - Sid: SSM + Effect: Allow + Action: [ + "ssm:DeleteParameter", + "ssm:DeleteParameters", + "ssm:GetParameter", + "ssm:GetParameters", + "ssm:GetParametersByPath" + ] + Resource: "*" + - Sid: SSMSecret + Effect: Allow + Action: [ + "ssm:PutParameter", + "ssm:AddTagsToResource" + ] + Resource: + - !Sub 'arn:${AWS::Partition}:ssm:${AWS::Region}:${AWS::AccountId}:parameter/copilot/${AppName}/${EnvironmentName}/secrets/*' + - Sid: ELBv2 + Effect: Allow + Action: [ + "elasticloadbalancing:DescribeLoadBalancerAttributes", + "elasticloadbalancing:DescribeSSLPolicies", + "elasticloadbalancing:DescribeLoadBalancers", + "elasticloadbalancing:DescribeTargetGroupAttributes", + "elasticloadbalancing:DescribeListeners", + "elasticloadbalancing:DescribeTags", + "elasticloadbalancing:DescribeTargetHealth", + "elasticloadbalancing:DescribeTargetGroups", + "elasticloadbalancing:DescribeRules" + ] + Resource: "*" + - Sid: BuiltArtifactAccess + Effect: Allow + Action: [ + "s3:ListBucketByTags", + "s3:GetLifecycleConfiguration", + "s3:GetBucketTagging", + "s3:GetInventoryConfiguration", + "s3:GetObjectVersionTagging", + "s3:ListBucketVersions", + "s3:GetBucketLogging", + "s3:ListBucket", + "s3:GetAccelerateConfiguration", + "s3:GetBucketPolicy", + "s3:GetObjectVersionTorrent", + "s3:GetObjectAcl", + "s3:GetEncryptionConfiguration", + "s3:GetBucketRequestPayment", + "s3:GetObjectVersionAcl", + "s3:GetObjectTagging", + "s3:GetMetricsConfiguration", + "s3:HeadBucket", + "s3:GetBucketPublicAccessBlock", + "s3:GetBucketPolicyStatus", + "s3:ListBucketMultipartUploads", + "s3:GetBucketWebsite", + "s3:ListJobs", + "s3:GetBucketVersioning", + "s3:GetBucketAcl", + "s3:GetBucketNotification", + "s3:GetReplicationConfiguration", + "s3:ListMultipartUploadParts", + "s3:GetObject", + "s3:GetObjectTorrent", + "s3:GetAccountPublicAccessBlock", + "s3:ListAllMyBuckets", + "s3:DescribeJob", + "s3:GetBucketCORS", + "s3:GetAnalyticsConfiguration", + "s3:GetObjectVersionForReplication", + "s3:GetBucketLocation", + "s3:GetObjectVersion", + "kms:Decrypt" + ] + Resource: "*" + - Sid: PutObjectsToArtifactBucket + Effect: Allow + Action: + - s3:PutObject + - s3:PutObjectAcl + Resource: + - arn:aws:s3:::mockbucket + - arn:aws:s3:::mockbucket/* + - Sid: EncryptObjectsInArtifactBucket + Effect: Allow + Action: + - kms:GenerateDataKey + Resource: arn:aws:kms:us-west-2:000000000:key/1234abcd-12ab-34cd-56ef-1234567890ab + - Sid: EC2 + Effect: Allow + Action: [ + "ec2:DescribeSubnets", + "ec2:DescribeSecurityGroups", + "ec2:DescribeNetworkInterfaces", + "ec2:DescribeRouteTables" + ] + Resource: "*" + - Sid: AppRunner + Effect: Allow + Action: [ + "apprunner:DescribeService", + "apprunner:ListOperations", + "apprunner:ListServices", + "apprunner:PauseService", + "apprunner:ResumeService", + "apprunner:StartDeployment", + "apprunner:DescribeObservabilityConfiguration" + ] + Resource: "*" + - Sid: Tags + Effect: Allow + Action: [ + "tag:GetResources" + ] + Resource: "*" + - Sid: ApplicationAutoscaling + Effect: Allow + Action: [ + "application-autoscaling:DescribeScalingPolicies" + ] + Resource: "*" + - Sid: DeleteRoles + Effect: Allow + Action: [ + "iam:DeleteRole", + "iam:ListRolePolicies", + "iam:DeleteRolePolicy" + ] + Resource: + - !GetAtt CloudformationExecutionRole.Arn + - !Sub "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/${AWS::StackName}-EnvManagerRole" + - Sid: DeleteEnvStack + Effect: Allow + Action: + - 'cloudformation:DescribeStacks' + - 'cloudformation:DeleteStack' + Resource: + - !Sub 'arn:${AWS::Partition}:cloudformation:${AWS::Region}:${AWS::AccountId}:stack/${AWS::StackName}/*' + +Outputs: + VpcId: + Value: !Ref VPC + Export: + Name: !Sub ${AWS::StackName}-VpcId + PublicSubnets: + Value: !Join [ ',', [ !Ref PublicSubnet1, !Ref PublicSubnet2, ] ] + Export: + Name: !Sub ${AWS::StackName}-PublicSubnets + PrivateSubnets: + Value: !Join [ ',', [ !Ref PrivateSubnet1, !Ref PrivateSubnet2, ] ] + Export: + Name: !Sub ${AWS::StackName}-PrivateSubnets + InternetGatewayID: + Value: !Ref InternetGateway + Export: + Name: !Sub ${AWS::StackName}-InternetGatewayID + PublicRouteTableID: + Value: !Ref PublicRouteTable + Export: + Name: !Sub ${AWS::StackName}-PublicRouteTableID + PrivateRouteTableIDs: + Condition: CreateNATGateways + Value: !Join [ ',', [ !Ref PrivateRouteTable1, !Ref PrivateRouteTable2, ] ] + Export: + Name: !Sub ${AWS::StackName}-PrivateRouteTableIDs + ServiceDiscoveryNamespaceID: + Value: !GetAtt ServiceDiscoveryNamespace.Id + Export: + Name: !Sub ${AWS::StackName}-ServiceDiscoveryNamespaceID + EnvironmentSecurityGroup: + Value: !Ref EnvironmentSecurityGroup + Export: + Name: !Sub ${AWS::StackName}-EnvironmentSecurityGroup + PublicLoadBalancerDNSName: + Condition: CreateALB + Value: !GetAtt PublicLoadBalancer.DNSName + Export: + Name: !Sub ${AWS::StackName}-PublicLoadBalancerDNS + PublicLoadBalancerFullName: + Condition: CreateALB + Value: !GetAtt PublicLoadBalancer.LoadBalancerFullName + Export: + Name: !Sub ${AWS::StackName}-PublicLoadBalancerFullName + PublicLoadBalancerHostedZone: + Condition: CreateALB + Value: !GetAtt PublicLoadBalancer.CanonicalHostedZoneID + Export: + Name: !Sub ${AWS::StackName}-CanonicalHostedZoneID + HTTPListenerArn: + Condition: CreateALB + Value: !Ref HTTPListener + Export: + Name: !Sub ${AWS::StackName}-HTTPListenerArn + HTTPSListenerArn: + Condition: ExportHTTPSListener + Value: !Ref HTTPSListener + Export: + Name: !Sub ${AWS::StackName}-HTTPSListenerArn + DefaultHTTPTargetGroupArn: + Condition: CreateALB + Value: !Ref DefaultHTTPTargetGroup + Export: + Name: !Sub ${AWS::StackName}-DefaultHTTPTargetGroup + InternalLoadBalancerDNSName: + Condition: CreateInternalALB + Value: !GetAtt InternalLoadBalancer.DNSName + Export: + Name: !Sub ${AWS::StackName}-InternalLoadBalancerDNS + InternalLoadBalancerFullName: + Condition: CreateInternalALB + Value: !GetAtt InternalLoadBalancer.LoadBalancerFullName + Export: + Name: !Sub ${AWS::StackName}-InternalLoadBalancerFullName + InternalLoadBalancerHostedZone: + Condition: CreateInternalALB + Value: !GetAtt InternalLoadBalancer.CanonicalHostedZoneID + Export: + Name: !Sub ${AWS::StackName}-InternalLoadBalancerCanonicalHostedZoneID + InternalWorkloadsHostedZone: + Condition: CreateInternalALB + Value: !GetAtt InternalWorkloadsHostedZone.Id + Export: + Name: !Sub ${AWS::StackName}-InternalWorkloadsHostedZoneID + InternalWorkloadsHostedZoneName: + Condition: CreateInternalALB + Value: !Sub ${EnvironmentName}.${AppName}.internal + Export: + Name: !Sub ${AWS::StackName}-InternalWorkloadsHostedZoneName + InternalHTTPListenerArn: + Condition: CreateInternalALB + Value: !Ref InternalHTTPListener + Export: + Name: !Sub ${AWS::StackName}-InternalHTTPListenerArn + InternalHTTPSListenerArn: + Condition: ExportInternalHTTPSListener + Value: !Ref InternalHTTPSListener + Export: + Name: !Sub ${AWS::StackName}-InternalHTTPSListenerArn + InternalLoadBalancerSecurityGroup: + Condition: CreateInternalALB + Value: !Ref InternalLoadBalancerSecurityGroup + Export: + Name: !Sub ${AWS::StackName}-InternalLoadBalancerSecurityGroup + ClusterId: + Value: !Ref Cluster + Export: + Name: !Sub ${AWS::StackName}-ClusterId + EnvironmentManagerRoleARN: + Value: !GetAtt EnvironmentManagerRole.Arn + Description: The role to be assumed by the ecs-cli to manage environments. + Export: + Name: !Sub ${AWS::StackName}-EnvironmentManagerRoleARN + CFNExecutionRoleARN: + Value: !GetAtt CloudformationExecutionRole.Arn + Description: The role to be assumed by the Cloudformation service when it deploys application infrastructure. + Export: + Name: !Sub ${AWS::StackName}-CFNExecutionRoleARN + EnabledFeatures: + Value: !Sub '${ALBWorkloads},${InternalALBWorkloads},${EFSWorkloads},${NATWorkloads},${Aliases}' + Description: Required output to force the stack to update if mutating feature params, like ALBWorkloads, does not change the template. + ManagedFileSystemID: + Condition: CreateEFS + Value: !Ref FileSystem + Description: The ID of the Copilot-managed EFS filesystem. + Export: + Name: !Sub ${AWS::StackName}-FilesystemID + LastForceDeployID: + Value: "" + Description: Optionally force the template to update when no immediate resource change is present. \ No newline at end of file diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index fbaf35c339f..06ae2d16151 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -354,6 +354,31 @@ type networkLoadBalancerConfig struct { appDNSName *string } +func convertEnvSecurityGroupCfg(mft *manifest.Environment) template.SecurityGroupConfig { + securityGroupConfig, isSecurityConfigSet := mft.EnvSecurityGroup() + if isSecurityConfigSet { + var ingress = make([]template.SecurityGroupRule, len(securityGroupConfig.Ingress)) + var egress = make([]template.SecurityGroupRule, len(securityGroupConfig.Egress)) + for idx, ingressValue := range securityGroupConfig.Ingress { + ingress[idx].IpProtocol = ingressValue.IpProtocol + ingress[idx].CidrIP = ingressValue.CidrIP + ingress[idx].ToPort = ingressValue.ToPort + ingress[idx].FromPort = ingressValue.FromPort + } + for idx, egressValue := range securityGroupConfig.Egress { + egress[idx].IpProtocol = egressValue.IpProtocol + egress[idx].CidrIP = egressValue.CidrIP + egress[idx].ToPort = egressValue.ToPort + egress[idx].FromPort = egressValue.FromPort + } + return template.SecurityGroupConfig{ + Ingress: ingress, + Egress: egress, + } + } + return template.SecurityGroupConfig{} +} + func (s *LoadBalancedWebService) convertNetworkLoadBalancer() (networkLoadBalancerConfig, error) { nlbConfig := s.manifest.NLBConfig if nlbConfig.IsEmpty() { diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 2438e15e428..0ba425e7013 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -109,40 +109,49 @@ type environmentVPCConfig struct { } type securityGroupConfig struct { - Ingress []securityGroupRules `yaml:"ingress,omitempty"` - Egress []securityGroupRules `yaml:"egress,omitempty"` + Ingress []securityGroupRule `yaml:"ingress,omitempty"` + Egress []securityGroupRule `yaml:"egress,omitempty"` } // IsEmpty returns true if there is no security group rule defined. -func (sgc securityGroupConfig) IsEmpty() bool { - return len(sgc.Ingress) == 0 && len(sgc.Egress) == 0 +func (cfg securityGroupConfig) IsEmpty() bool { + return len(cfg.Ingress) == 0 && len(cfg.Egress) == 0 } -type securityGroupRules struct { - CidrIp string `yaml:"cidr"` +type securityGroupRule struct { + CidrIP string `yaml:"cidr"` FromPort int `yaml:"from_port"` IpProtocol string `yaml:"ip_protocol"` ToPort int `yaml:"to_port"` } //IsValid checks for valid security group config. -func (sgc securityGroupConfig) IsValid() bool { - for _, ingress := range sgc.Ingress { +func (cfg securityGroupConfig) IsValid() bool { + for _, ingress := range cfg.Ingress { if ingress.IsEmpty() { - return true + return false } } - for _, egress := range sgc.Egress { + for _, egress := range cfg.Egress { if egress.IsEmpty() { - return true + return false } } - return false + return true +} + +// EnvSecurityGroup returns the security group config if the user has set any values. +// If there is no env security group settings, then returns nil and false. +func (cfg *EnvironmentConfig) EnvSecurityGroup() (*securityGroupConfig, bool) { + if isEmpty := cfg.Network.VPC.SecurityGroupConfig.IsEmpty(); !isEmpty { + return &cfg.Network.VPC.SecurityGroupConfig, true + } + return nil, false } //IsEmpty checks for required parameters in the security group rules. -func (sgr securityGroupRules) IsEmpty() bool { - return sgr.CidrIp == "" || sgr.IpProtocol == "" +func (cfg securityGroupRule) IsEmpty() bool { + return cfg.CidrIP == "" || cfg.IpProtocol == "" } type environmentCDNConfig struct { diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index bf8fed3dd03..042d9cf542c 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -68,6 +68,9 @@ func (cfg environmentVPCConfig) validate() error { if err := cfg.Subnets.validate(); err != nil { return fmt.Errorf(`validate "subnets": %w`, err) } + if err := cfg.SecurityGroupConfig.validate(); err != nil { + return fmt.Errorf(`validate "security group rules": %w`, err) + } if cfg.imported() { if err := cfg.validateImportedVPC(); err != nil { return fmt.Errorf(`validate "subnets" for an imported VPC: %w`, err) @@ -81,15 +84,17 @@ func (cfg environmentVPCConfig) validate() error { return nil } -// validate returns true if there is no security group rule defined. -func (cfg securityGroupRules) validate() error { +// validate is a no-op for securityGroupRules. +func (cfg securityGroupRule) validate() error { //no-op return nil } -// validate returns true if there is no security group rule defined. +// Validate returns nil if securityGroupConfig is configured correctly. func (cfg securityGroupConfig) validate() error { - //no-op + if !cfg.IsValid() { + return fmt.Errorf(`security group config is invalid`) + } return nil } diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index 06ba8767fd9..9f2628f9788 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -113,8 +113,6 @@ type EnvOpts struct { AllowVPCIngress bool Telemetry *Telemetry - SecurityGroupConfig *SecurityGroupConfig - CDNConfig *CDNConfig // If nil, no cdn is to be used LatestVersion string @@ -126,8 +124,9 @@ type EnvOpts struct { type CDNConfig struct{} type VPCConfig struct { - Imported *ImportVPC // If not-nil, use the imported VPC resources instead of the Managed VPC. - Managed ManagedVPC + Imported *ImportVPC // If not-nil, use the imported VPC resources instead of the Managed VPC. + Managed ManagedVPC + SecurityGroupConfig SecurityGroupConfig } // ImportVPC holds the fields to import VPC resources. @@ -152,12 +151,13 @@ type Telemetry struct { // SecurityGroupConfig holds the fields to import security group config type SecurityGroupConfig struct { - Ingress []SecurityGroupRules - Egress []SecurityGroupRules + Ingress []SecurityGroupRule + Egress []SecurityGroupRule } -type SecurityGroupRules struct { - CidrIp string +// SecurityGroupRules holds the fields to import security group rules +type SecurityGroupRule struct { + CidrIP string FromPort int IpProtocol string ToPort int diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index 36db0fef2fa..3ddd6601669 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -189,20 +189,24 @@ Resources: Tags: - Key: Name Value: !Sub 'copilot-${AppName}-${EnvironmentName}-env' -{{- if and .SecurityGroupConfig .SecurityGroupConfig.Ingress }} - SecurityGroupIngress: {{range $ind, $securityRule := .SecurityGroupConfig.Ingress}} +{{- if (gt (len .VPCConfig.SecurityGroupConfig.Ingress) 0)}} + SecurityGroupIngress: + {{range $ind, $securityRule := .VPCConfig.SecurityGroupConfig.Ingress}} - IpProtocol: {{$securityRule.IpProtocol}} FromPort: {{$securityRule.FromPort}} ToPort: {{$securityRule.ToPort}} - CidrIp: {{$securityRule.CidrIp}} -{{end }}{{- end}} -{{- if and .SecurityGroupConfig .SecurityGroupConfig.Egress }} - SecurityGroupEgress: {{range $ind, $securityRule := .SecurityGroupConfig.Egress}} + CidrIp: {{$securityRule.CidrIP}} + {{end }} +{{- end}} + {{- if (gt (len .VPCConfig.SecurityGroupConfig.Egress) 0)}} + SecurityGroupEgress: + {{range $ind, $securityRule := .VPCConfig.SecurityGroupConfig.Egress}} - IpProtocol: {{$securityRule.IpProtocol}} FromPort: {{$securityRule.FromPort}} ToPort: {{$securityRule.ToPort}} - CidrIp: {{$securityRule.CidrIp}} -{{end }}{{- end}} + CidrIp: {{$securityRule.CidrIP}} + {{end }} +{{- end}} EnvironmentHTTPSecurityGroupIngressFromPublicALB: Type: AWS::EC2::SecurityGroupIngress Condition: CreateALB From e8dd035763118122d55213746ef70adf8d7eb507 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Tue, 26 Jul 2022 09:39:11 -0700 Subject: [PATCH 04/19] minor nits --- internal/pkg/manifest/validate_env.go | 2 +- internal/pkg/template/env.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 042d9cf542c..4d79a6907c2 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -84,7 +84,7 @@ func (cfg environmentVPCConfig) validate() error { return nil } -// validate is a no-op for securityGroupRules. +// validate is a no-op for securityGroupRule. func (cfg securityGroupRule) validate() error { //no-op return nil diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index 9f2628f9788..af7fde7dfc5 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -155,7 +155,7 @@ type SecurityGroupConfig struct { Egress []SecurityGroupRule } -// SecurityGroupRules holds the fields to import security group rules +// SecurityGroupRule holds the fields to import security group rule type SecurityGroupRule struct { CidrIP string FromPort int From b680761446cb80e463e323f5f89913eabe7e1a6f Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Tue, 26 Jul 2022 10:19:32 -0700 Subject: [PATCH 05/19] fix static check --- internal/pkg/manifest/validate_env.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 4d79a6907c2..94e833ecb28 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -71,6 +71,16 @@ func (cfg environmentVPCConfig) validate() error { if err := cfg.SecurityGroupConfig.validate(); err != nil { return fmt.Errorf(`validate "security group rules": %w`, err) } + for _, ingress := range cfg.SecurityGroupConfig.Ingress { + if err := ingress.validate(); err != nil { + return err + } + } + for _, egress := range cfg.SecurityGroupConfig.Egress { + if err := egress.validate(); err != nil { + return err + } + } if cfg.imported() { if err := cfg.validateImportedVPC(); err != nil { return fmt.Errorf(`validate "subnets" for an imported VPC: %w`, err) @@ -86,7 +96,6 @@ func (cfg environmentVPCConfig) validate() error { // validate is a no-op for securityGroupRule. func (cfg securityGroupRule) validate() error { - //no-op return nil } From e5dabc3d8a4b757ff10d22058668838112c77176 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Tue, 26 Jul 2022 13:56:30 -0700 Subject: [PATCH 06/19] Address feedback by Wanxian --- .../pkg/deploy/cloudformation/stack/env.go | 16 ++++---- .../stack/env_integration_test.go | 40 +++++++++---------- .../deploy/cloudformation/stack/env_test.go | 6 +-- ...plate-with-custom-empty-security-group.yml | 8 ++-- .../template-with-custom-security-group.yml | 32 +++++++-------- internal/pkg/manifest/env.go | 35 +++++----------- internal/pkg/manifest/validate_env.go | 37 ++++++++++------- internal/pkg/manifest/validate_env_test.go | 28 +++++++++++++ internal/pkg/template/env.go | 8 ++-- .../pkg/template/templates/environment/cf.yml | 8 ++-- 10 files changed, 114 insertions(+), 104 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index 8e44ce53b50..179eb97fd2e 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -111,16 +111,15 @@ func (e *EnvStackConfig) Template() (string, error) { AllowVPCIngress: aws.BoolValue(e.in.Mft.HTTPConfig.Private.SecurityGroupsConfig.Ingress.VPCIngress), Telemetry: e.telemetryConfig(), CDNConfig: e.cdnConfig(), - - Version: e.in.Version, - LatestVersion: deploy.LatestEnvTemplateVersion, - SerializedManifest: string(e.in.RawMft), - ForceUpdateID: forceUpdateID, + SecurityGroupConfig: convertEnvSecurityGroupCfg(e.in.Mft), + Version: e.in.Version, + LatestVersion: deploy.LatestEnvTemplateVersion, + SerializedManifest: string(e.in.RawMft), + ForceUpdateID: forceUpdateID, }) if err != nil { return "", err } - fmt.Println("printing cfn", content.String()) return content.String(), nil } @@ -359,9 +358,8 @@ func (e *EnvStackConfig) cdnConfig() *template.CDNConfig { func (e *EnvStackConfig) vpcConfig() template.VPCConfig { return template.VPCConfig{ - Imported: e.importVPC(), - Managed: e.managedVPC(), - SecurityGroupConfig: convertEnvSecurityGroupCfg(e.in.Mft), + Imported: e.importVPC(), + Managed: e.managedVPC(), } } diff --git a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go index e3def4ee1d0..34bd08c2368 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go @@ -83,23 +83,21 @@ http: - cert-2 observability: container_insights: true # Enable container insights. -network: - vpc: - security_group: - ingress: - - ip_protocol: tcp - from_port: 0 - to_port: 65535 - cidr: 0.0.0.0 - - ip_protocol: tcp - from_port: 1 - to_port: 6 - cidr: 0.0.0.0 - egress: - - ip_protocol: tcp - from_port: 0 - to_port: 65535 - cidr: 0.0.0.0` +security_group: + ingress: + - ip_protocol: tcp + from_port: 0 + to_port: 65535 + cidr: 0.0.0.0 + - ip_protocol: tcp + from_port: 1 + to_port: 6 + cidr: 0.0.0.0 + egress: + - ip_protocol: tcp + from_port: 0 + to_port: 65535 + cidr: 0.0.0.0` var mft manifest.Environment err := yaml.Unmarshal([]byte(rawMft), &mft) require.NoError(t, err) @@ -138,11 +136,9 @@ http: - cert-2 observability: container_insights: true # Enable container insights. -network: - vpc: - security_group: - ingress: - egress:` +security_group: + ingress: + egress:` var mft manifest.Environment err := yaml.Unmarshal([]byte(rawMft), &mft) require.NoError(t, err) diff --git a/internal/pkg/deploy/cloudformation/stack/env_test.go b/internal/pkg/deploy/cloudformation/stack/env_test.go index edb8e1e1fd4..24c3280ae5e 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_test.go @@ -40,7 +40,6 @@ func TestEnv_Template(t *testing.T) { PrivateSubnetCIDRs: DefaultPrivateSubnetCIDRs, PublicSubnetCIDRs: DefaultPublicSubnetCIDRs, }, - SecurityGroupConfig: template.SecurityGroupConfig{}, }, LatestVersion: deploy.LatestEnvTemplateVersion, CustomResources: map[string]template.S3ObjectLocation{ @@ -60,8 +59,9 @@ func TestEnv_Template(t *testing.T) { Telemetry: &template.Telemetry{ EnableContainerInsights: false, }, - SerializedManifest: "name: env\ntype: Environment\n", - ForceUpdateID: "mockPreviousForceUpdateID", + SecurityGroupConfig: template.SecurityGroupConfig{}, + SerializedManifest: "name: env\ntype: Environment\n", + ForceUpdateID: "mockPreviousForceUpdateID", }, data) return &template.Content{Buffer: bytes.NewBufferString("mockTemplate")}, nil }) diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-empty-security-group.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-empty-security-group.yml index 83efd713e1c..d962b62ace2 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-empty-security-group.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-empty-security-group.yml @@ -13,11 +13,9 @@ Metadata: - cert-2 observability: container_insights: true # Enable container insights. - network: - vpc: - security_group: - ingress: - egress: + security_group: + ingress: + egress: Parameters: AppName: Type: String diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml index 12439528f9a..876b4766aa5 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml @@ -13,23 +13,21 @@ Metadata: - cert-2 observability: container_insights: true # Enable container insights. - network: - vpc: - security_group: - ingress: - - ip_protocol: tcp - from_port: 0 - to_port: 65535 - cidr: 0.0.0.0 - - ip_protocol: tcp - from_port: 1 - to_port: 6 - cidr: 0.0.0.0 - egress: - - ip_protocol: tcp - from_port: 0 - to_port: 65535 - cidr: 0.0.0.0 + security_group: + ingress: + - ip_protocol: tcp + from_port: 0 + to_port: 65535 + cidr: 0.0.0.0 + - ip_protocol: tcp + from_port: 1 + to_port: 6 + cidr: 0.0.0.0 + egress: + - ip_protocol: tcp + from_port: 0 + to_port: 65535 + cidr: 0.0.0.0 Parameters: AppName: Type: String diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 0ba425e7013..40e5f081db6 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -85,10 +85,11 @@ func (e *Environment) MarshalBinary() ([]byte, error) { // 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"` - CDNConfig environmentCDNConfig `yaml:"cdn,omitempty,flow"` + Network environmentNetworkConfig `yaml:"network,omitempty,flow"` + Observability environmentObservability `yaml:"observability,omitempty,flow"` + HTTPConfig EnvironmentHTTPConfig `yaml:"http,omitempty,flow"` + CDNConfig environmentCDNConfig `yaml:"cdn,omitempty,flow"` + SecurityGroupConfig securityGroupConfig `yaml:"security_group,omitempty"` } // IsIngressRestrictedToCDN returns whether or not an environment has its @@ -102,10 +103,9 @@ type environmentNetworkConfig struct { } type environmentVPCConfig struct { - ID *string `yaml:"id,omitempty"` - CIDR *IPNet `yaml:"cidr,omitempty"` - Subnets subnetsConfiguration `yaml:"subnets,omitempty"` - SecurityGroupConfig securityGroupConfig `yaml:"security_group,omitempty"` + ID *string `yaml:"id,omitempty"` + CIDR *IPNet `yaml:"cidr,omitempty"` + Subnets subnetsConfiguration `yaml:"subnets,omitempty"` } type securityGroupConfig struct { @@ -125,26 +125,11 @@ type securityGroupRule struct { ToPort int `yaml:"to_port"` } -//IsValid checks for valid security group config. -func (cfg securityGroupConfig) IsValid() bool { - for _, ingress := range cfg.Ingress { - if ingress.IsEmpty() { - return false - } - } - for _, egress := range cfg.Egress { - if egress.IsEmpty() { - return false - } - } - return true -} - // EnvSecurityGroup returns the security group config if the user has set any values. // If there is no env security group settings, then returns nil and false. func (cfg *EnvironmentConfig) EnvSecurityGroup() (*securityGroupConfig, bool) { - if isEmpty := cfg.Network.VPC.SecurityGroupConfig.IsEmpty(); !isEmpty { - return &cfg.Network.VPC.SecurityGroupConfig, true + if isEmpty := cfg.SecurityGroupConfig.IsEmpty(); !isEmpty { + return &cfg.SecurityGroupConfig, true } return nil, false } diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 94e833ecb28..b77511bb26a 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -36,7 +36,20 @@ func (e EnvironmentConfig) validate() error { if err := e.HTTPConfig.validate(); err != nil { return fmt.Errorf(`validate "http config": %w`, err) } + if err := e.SecurityGroupConfig.validate(); err != nil { + return fmt.Errorf(`validate "security_group": %w`, err) + } + for _, ingress := range e.SecurityGroupConfig.Ingress { + if err := ingress.validate(); err != nil { + return err + } + } + for _, egress := range e.SecurityGroupConfig.Egress { + if err := egress.validate(); err != nil { + return err + } + } if e.IsIngressRestrictedToCDN() && !e.CDNConfig.CDNEnabled() { return errors.New("CDN must be enabled to limit security group ingress to CloudFront") } @@ -68,19 +81,6 @@ func (cfg environmentVPCConfig) validate() error { if err := cfg.Subnets.validate(); err != nil { return fmt.Errorf(`validate "subnets": %w`, err) } - if err := cfg.SecurityGroupConfig.validate(); err != nil { - return fmt.Errorf(`validate "security group rules": %w`, err) - } - for _, ingress := range cfg.SecurityGroupConfig.Ingress { - if err := ingress.validate(); err != nil { - return err - } - } - for _, egress := range cfg.SecurityGroupConfig.Egress { - if err := egress.validate(); err != nil { - return err - } - } if cfg.imported() { if err := cfg.validateImportedVPC(); err != nil { return fmt.Errorf(`validate "subnets" for an imported VPC: %w`, err) @@ -101,8 +101,15 @@ func (cfg securityGroupRule) validate() error { // Validate returns nil if securityGroupConfig is configured correctly. func (cfg securityGroupConfig) validate() error { - if !cfg.IsValid() { - return fmt.Errorf(`security group config is invalid`) + for _, ingress := range cfg.Ingress { + if ingress.IsEmpty() { + return fmt.Errorf(`security group config is invalid`) + } + } + for _, egress := range cfg.Egress { + if egress.IsEmpty() { + return fmt.Errorf(`security group config is invalid`) + } } return nil } diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index eb700f03b17..4a6a29ed7df 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -93,6 +93,34 @@ func TestEnvironmentConfig_validate(t *testing.T) { }, wantedError: "in order to specify internal ALB subnet placement, subnets must be imported", }, + "error if invalid security group config": { + in: EnvironmentConfig{ + SecurityGroupConfig: securityGroupConfig{ + Ingress: []securityGroupRule{ + { + IpProtocol: "tcp", + ToPort: 80, + FromPort: 80, + }, + }, + }, + }, + wantedError: "validate \"security_group\": security group config is invalid", + }, + "valid security group config": { + in: EnvironmentConfig{ + SecurityGroupConfig: securityGroupConfig{ + Ingress: []securityGroupRule{ + { + CidrIP: "0.0.0.0", + IpProtocol: "tcp", + ToPort: 80, + FromPort: 80, + }, + }, + }, + }, + }, "error if security group ingress is limited to a cdn distribution not enabled": { in: EnvironmentConfig{ CDNConfig: environmentCDNConfig{ diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index af7fde7dfc5..a8290d8e8b1 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -113,7 +113,8 @@ type EnvOpts struct { AllowVPCIngress bool Telemetry *Telemetry - CDNConfig *CDNConfig // If nil, no cdn is to be used + CDNConfig *CDNConfig // If nil, no cdn is to be used + SecurityGroupConfig SecurityGroupConfig LatestVersion string SerializedManifest string // Serialized manifest used to render the environment template. @@ -124,9 +125,8 @@ type EnvOpts struct { type CDNConfig struct{} type VPCConfig struct { - Imported *ImportVPC // If not-nil, use the imported VPC resources instead of the Managed VPC. - Managed ManagedVPC - SecurityGroupConfig SecurityGroupConfig + Imported *ImportVPC // If not-nil, use the imported VPC resources instead of the Managed VPC. + Managed ManagedVPC } // ImportVPC holds the fields to import VPC resources. diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index 3ddd6601669..114791da709 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -189,18 +189,18 @@ Resources: Tags: - Key: Name Value: !Sub 'copilot-${AppName}-${EnvironmentName}-env' -{{- if (gt (len .VPCConfig.SecurityGroupConfig.Ingress) 0)}} +{{- if (gt (len .SecurityGroupConfig.Ingress) 0)}} SecurityGroupIngress: - {{range $ind, $securityRule := .VPCConfig.SecurityGroupConfig.Ingress}} + {{range $securityRule := .SecurityGroupConfig.Ingress}} - IpProtocol: {{$securityRule.IpProtocol}} FromPort: {{$securityRule.FromPort}} ToPort: {{$securityRule.ToPort}} CidrIp: {{$securityRule.CidrIP}} {{end }} {{- end}} - {{- if (gt (len .VPCConfig.SecurityGroupConfig.Egress) 0)}} + {{- if (gt (len .SecurityGroupConfig.Egress) 0)}} SecurityGroupEgress: - {{range $ind, $securityRule := .VPCConfig.SecurityGroupConfig.Egress}} + {{range $securityRule := .SecurityGroupConfig.Egress}} - IpProtocol: {{$securityRule.IpProtocol}} FromPort: {{$securityRule.FromPort}} ToPort: {{$securityRule.ToPort}} From f4b8875e23ff81ffa7d1cf5c652ee5d64420bc73 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Tue, 26 Jul 2022 14:49:43 -0700 Subject: [PATCH 07/19] resolve merge cpnflict and rename method --- internal/pkg/deploy/cloudformation/stack/env.go | 2 +- internal/pkg/manifest/env.go | 12 +++++++++--- internal/pkg/manifest/validate_env.go | 8 ++++---- internal/pkg/manifest/validate_env_test.go | 2 +- internal/pkg/template/env.go | 14 ++++++++------ 5 files changed, 23 insertions(+), 15 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index 6d3e0fd45a8..b15b920d814 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -108,7 +108,7 @@ func (e *EnvStackConfig) Template() (string, error) { PrivateHTTPConfig: e.privateHTTPConfig(), Telemetry: e.telemetryConfig(), CDNConfig: e.cdnConfig(), - SecurityGroupConfig: convertEnvSecurityGroupCfg(e.in.Mft), + SecurityGroupConfig: convertEnvSecurityGroupCfg(e.in.Mft), Version: e.in.Version, LatestVersion: deploy.LatestEnvTemplateVersion, diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 40e5f081db6..45c3fbc1ead 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -134,9 +134,15 @@ func (cfg *EnvironmentConfig) EnvSecurityGroup() (*securityGroupConfig, bool) { return nil, false } -//IsEmpty checks for required parameters in the security group rules. -func (cfg securityGroupRule) IsEmpty() bool { - return cfg.CidrIP == "" || cfg.IpProtocol == "" +//Validate checks for required parameters in the security group rules. +func (cfg securityGroupRule) Validate() error { + if cfg.CidrIP == "" { + return fmt.Errorf(`cidr field is required`) + } + if cfg.IpProtocol == "" { + return fmt.Errorf(`ip_protocol field is required`) + } + return nil } type environmentCDNConfig struct { diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index b77511bb26a..ff76f8db866 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -102,13 +102,13 @@ func (cfg securityGroupRule) validate() error { // Validate returns nil if securityGroupConfig is configured correctly. func (cfg securityGroupConfig) validate() error { for _, ingress := range cfg.Ingress { - if ingress.IsEmpty() { - return fmt.Errorf(`security group config is invalid`) + if err := ingress.Validate(); err != nil { + return err } } for _, egress := range cfg.Egress { - if egress.IsEmpty() { - return fmt.Errorf(`security group config is invalid`) + if err := egress.Validate(); err != nil { + return err } } return nil diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index 4a6a29ed7df..8fe59909047 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -105,7 +105,7 @@ func TestEnvironmentConfig_validate(t *testing.T) { }, }, }, - wantedError: "validate \"security_group\": security group config is invalid", + wantedError: "validate \"security_group\": cidr field is required", }, "valid security group config": { in: EnvironmentConfig{ diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index 89da2483a3f..e26b6d9922a 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -105,12 +105,12 @@ type EnvOpts struct { ArtifactBucketARN string ArtifactBucketKeyARN string - VPCConfig VPCConfig - PublicHTTPConfig HTTPConfig - PrivateHTTPConfig HTTPConfig - Telemetry *Telemetry - CDNConfig *CDNConfig - SecurityGroupConfig SecurityGroupConfig + VPCConfig VPCConfig + PublicHTTPConfig HTTPConfig + PrivateHTTPConfig HTTPConfig + Telemetry *Telemetry + CDNConfig *CDNConfig + SecurityGroupConfig SecurityGroupConfig LatestVersion string SerializedManifest string // Serialized manifest used to render the environment template. @@ -146,6 +146,8 @@ type ManagedVPC struct { AZs []string PublicSubnetCIDRs []string PrivateSubnetCIDRs []string +} + // Telemetry represents optional observability and monitoring configuration. type Telemetry struct { EnableContainerInsights bool From 7337412f22151a3350f5057de8bad6517f8a3fc6 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Tue, 26 Jul 2022 15:04:21 -0700 Subject: [PATCH 08/19] nits --- internal/pkg/deploy/cloudformation/stack/env.go | 3 --- internal/pkg/manifest/env.go | 11 ----------- internal/pkg/manifest/validate_env.go | 14 ++++++++++---- 3 files changed, 10 insertions(+), 18 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index b15b920d814..39862b341b1 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -94,9 +94,6 @@ func (e *EnvStackConfig) Template() (string, error) { } forceUpdateID = id.String() } - if err != nil { - return "", err - } content, err := e.parser.ParseEnv(&template.EnvOpts{ AppName: e.in.App.Name, EnvName: e.in.Name, diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 45c3fbc1ead..b462a4cd50b 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -134,17 +134,6 @@ func (cfg *EnvironmentConfig) EnvSecurityGroup() (*securityGroupConfig, bool) { return nil, false } -//Validate checks for required parameters in the security group rules. -func (cfg securityGroupRule) Validate() error { - if cfg.CidrIP == "" { - return fmt.Errorf(`cidr field is required`) - } - if cfg.IpProtocol == "" { - return fmt.Errorf(`ip_protocol field is required`) - } - return nil -} - type environmentCDNConfig struct { Enabled *bool CDNConfig advancedCDNConfig // mutually exclusive with Enabled diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index ff76f8db866..640f835443e 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -94,20 +94,26 @@ func (cfg environmentVPCConfig) validate() error { return nil } -// validate is a no-op for securityGroupRule. +// validate returns nil if securityGroupRule has all the required parameters set. func (cfg securityGroupRule) validate() error { + if cfg.CidrIP == "" { + return fmt.Errorf(`cidr field is required`) + } + if cfg.IpProtocol == "" { + return fmt.Errorf(`ip_protocol field is required`) + } return nil } -// Validate returns nil if securityGroupConfig is configured correctly. +// validate returns nil if securityGroupConfig is configured correctly. func (cfg securityGroupConfig) validate() error { for _, ingress := range cfg.Ingress { - if err := ingress.Validate(); err != nil { + if err := ingress.validate(); err != nil { return err } } for _, egress := range cfg.Egress { - if err := egress.Validate(); err != nil { + if err := egress.validate(); err != nil { return err } } From 677786ef7e00ef665a968c2743e8d1cec9449ed9 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Tue, 26 Jul 2022 15:10:50 -0700 Subject: [PATCH 09/19] remove extra code --- internal/pkg/manifest/validate_env.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 640f835443e..dd9ec3e6f38 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -39,17 +39,6 @@ func (e EnvironmentConfig) validate() error { if err := e.SecurityGroupConfig.validate(); err != nil { return fmt.Errorf(`validate "security_group": %w`, err) } - - for _, ingress := range e.SecurityGroupConfig.Ingress { - if err := ingress.validate(); err != nil { - return err - } - } - for _, egress := range e.SecurityGroupConfig.Egress { - if err := egress.validate(); err != nil { - return err - } - } if e.IsIngressRestrictedToCDN() && !e.CDNConfig.CDNEnabled() { return errors.New("CDN must be enabled to limit security group ingress to CloudFront") } From fe6d70fa9552e941516ef00beee91ec4f1abb81c Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Wed, 27 Jul 2022 23:09:09 -0700 Subject: [PATCH 10/19] address feedback:1 --- .../stack/env_integration_test.go | 1 - .../template-with-custom-security-group.yml | 3 +- .../cloudformation/stack/transformers.go | 55 +++++++++++++------ internal/pkg/manifest/env.go | 19 +++++-- internal/pkg/manifest/validate_env.go | 16 ++++-- internal/pkg/manifest/validate_env_test.go | 23 ++++++-- 6 files changed, 81 insertions(+), 36 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go index 34bd08c2368..2d24174e79d 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go @@ -95,7 +95,6 @@ security_group: cidr: 0.0.0.0 egress: - ip_protocol: tcp - from_port: 0 to_port: 65535 cidr: 0.0.0.0` var mft manifest.Environment diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml index 876b4766aa5..2244393bd8a 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml @@ -25,7 +25,6 @@ Metadata: cidr: 0.0.0.0 egress: - ip_protocol: tcp - from_port: 0 to_port: 65535 cidr: 0.0.0.0 Parameters: @@ -341,7 +340,7 @@ Resources: ToPort: 6 SecurityGroupEgress: - CidrIp: "0.0.0.0" - FromPort: 0 + FromPort: 65535 IpProtocol: tcp ToPort: 65535 EnvironmentSecurityGroupIngressFromInternalALB: diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 06ae2d16151..e3a8b4ab43d 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -52,6 +52,11 @@ const ( maxPercentDefault = 200 ) +const ( + toPortValue = 0 + fromPortValue = 65535 +) + var ( taskDefOverrideRulePrefixes = []string{"Resources", "TaskDefinition", "Properties"} subnetPlacementForTemplate = map[manifest.PlacementString]string{ @@ -356,27 +361,43 @@ type networkLoadBalancerConfig struct { func convertEnvSecurityGroupCfg(mft *manifest.Environment) template.SecurityGroupConfig { securityGroupConfig, isSecurityConfigSet := mft.EnvSecurityGroup() - if isSecurityConfigSet { - var ingress = make([]template.SecurityGroupRule, len(securityGroupConfig.Ingress)) - var egress = make([]template.SecurityGroupRule, len(securityGroupConfig.Egress)) - for idx, ingressValue := range securityGroupConfig.Ingress { - ingress[idx].IpProtocol = ingressValue.IpProtocol - ingress[idx].CidrIP = ingressValue.CidrIP - ingress[idx].ToPort = ingressValue.ToPort - ingress[idx].FromPort = ingressValue.FromPort + if !isSecurityConfigSet { + return template.SecurityGroupConfig{} + } + var ingress = make([]template.SecurityGroupRule, len(securityGroupConfig.Ingress)) + var egress = make([]template.SecurityGroupRule, len(securityGroupConfig.Egress)) + for idx, ingressValue := range securityGroupConfig.Ingress { + ingress[idx].IpProtocol = ingressValue.IpProtocol + ingress[idx].CidrIP = ingressValue.CidrIP + if ingressValue.IsToPortEmpty() { + ingress[idx].ToPort = toPortValue + } else { + ingress[idx].ToPort = *ingressValue.ToPort } - for idx, egressValue := range securityGroupConfig.Egress { - egress[idx].IpProtocol = egressValue.IpProtocol - egress[idx].CidrIP = egressValue.CidrIP - egress[idx].ToPort = egressValue.ToPort - egress[idx].FromPort = egressValue.FromPort + if ingressValue.IsFromPortEmpty() { + ingress[idx].FromPort = fromPortValue + } else { + ingress[idx].FromPort = *ingressValue.FromPort } - return template.SecurityGroupConfig{ - Ingress: ingress, - Egress: egress, + } + for idx, egressValue := range securityGroupConfig.Egress { + egress[idx].IpProtocol = egressValue.IpProtocol + egress[idx].CidrIP = egressValue.CidrIP + if egressValue.IsToPortEmpty() { + egress[idx].ToPort = toPortValue + } else { + egress[idx].ToPort = *egressValue.ToPort + } + if egressValue.IsFromPortEmpty() { + egress[idx].FromPort = fromPortValue + } else { + egress[idx].FromPort = *egressValue.FromPort } } - return template.SecurityGroupConfig{} + return template.SecurityGroupConfig{ + Ingress: ingress, + Egress: egress, + } } func (s *LoadBalancedWebService) convertNetworkLoadBalancer() (networkLoadBalancerConfig, error) { diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index b462a4cd50b..dc7e38fa5bd 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -113,22 +113,31 @@ type securityGroupConfig struct { Egress []securityGroupRule `yaml:"egress,omitempty"` } -// IsEmpty returns true if there is no security group rule defined. -func (cfg securityGroupConfig) IsEmpty() bool { +func (cfg securityGroupConfig) isEmpty() bool { return len(cfg.Ingress) == 0 && len(cfg.Egress) == 0 } type securityGroupRule struct { CidrIP string `yaml:"cidr"` - FromPort int `yaml:"from_port"` + FromPort *int `yaml:"from_port"` IpProtocol string `yaml:"ip_protocol"` - ToPort int `yaml:"to_port"` + ToPort *int `yaml:"to_port"` +} + +// IsToPortEmpty checks if ToPort is empty +func (cfg securityGroupRule) IsToPortEmpty() bool { + return cfg.ToPort == nil +} + +// IsFromPortEmpty checks if FromPort is empty +func (cfg securityGroupRule) IsFromPortEmpty() bool { + return cfg.FromPort == nil } // EnvSecurityGroup returns the security group config if the user has set any values. // If there is no env security group settings, then returns nil and false. func (cfg *EnvironmentConfig) EnvSecurityGroup() (*securityGroupConfig, bool) { - if isEmpty := cfg.SecurityGroupConfig.IsEmpty(); !isEmpty { + if isEmpty := cfg.SecurityGroupConfig.isEmpty(); !isEmpty { return &cfg.SecurityGroupConfig, true } return nil, false diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index dd9ec3e6f38..53601fcf013 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -86,24 +86,28 @@ func (cfg environmentVPCConfig) validate() error { // validate returns nil if securityGroupRule has all the required parameters set. func (cfg securityGroupRule) validate() error { if cfg.CidrIP == "" { - return fmt.Errorf(`cidr field is required`) + return fmt.Errorf(`cidr`) } if cfg.IpProtocol == "" { - return fmt.Errorf(`ip_protocol field is required`) + return fmt.Errorf(`ip_protocol`) } return nil } // validate returns nil if securityGroupConfig is configured correctly. func (cfg securityGroupConfig) validate() error { - for _, ingress := range cfg.Ingress { + for idx, ingress := range cfg.Ingress { if err := ingress.validate(); err != nil { - return err + return fmt.Errorf(`validate ingress[%d]: %w`, idx, &errFieldMustBeSpecified{ + missingField: err.Error(), + }) } } - for _, egress := range cfg.Egress { + for idx, egress := range cfg.Egress { if err := egress.validate(); err != nil { - return err + return fmt.Errorf(`validate egress[%d]: %w`, idx, &errFieldMustBeSpecified{ + missingField: err.Error(), + }) } } return nil diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index 8fe59909047..ab8ee6f3b0e 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -99,13 +99,13 @@ func TestEnvironmentConfig_validate(t *testing.T) { Ingress: []securityGroupRule{ { IpProtocol: "tcp", - ToPort: 80, - FromPort: 80, + ToPort: aws.Int(80), + FromPort: aws.Int(80), }, }, }, }, - wantedError: "validate \"security_group\": cidr field is required", + wantedError: "validate \"security_group\": validate ingress[0]: \"cidr\" must be specified", }, "valid security group config": { in: EnvironmentConfig{ @@ -114,8 +114,21 @@ func TestEnvironmentConfig_validate(t *testing.T) { { CidrIP: "0.0.0.0", IpProtocol: "tcp", - ToPort: 80, - FromPort: 80, + ToPort: aws.Int(80), + FromPort: aws.Int(80), + }, + }, + }, + }, + }, + "valid security group config without FromPort": { + in: EnvironmentConfig{ + SecurityGroupConfig: securityGroupConfig{ + Ingress: []securityGroupRule{ + { + CidrIP: "0.0.0.0", + IpProtocol: "tcp", + ToPort: aws.Int(80), }, }, }, From 92b4622137b12a5f039be1510b5a29491f8498cf Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Fri, 29 Jul 2022 15:14:43 -0700 Subject: [PATCH 11/19] Address feedback to change port defaults and bring securitygroup inside network.vpc --- .../pkg/deploy/cloudformation/stack/env.go | 24 ++-- .../stack/env_integration_test.go | 28 ++--- .../deploy/cloudformation/stack/env_test.go | 7 +- .../template-with-custom-security-group.yml | 36 +++--- .../cloudformation/stack/transformers.go | 45 +++---- internal/pkg/manifest/env.go | 112 ++++++++++++++---- internal/pkg/manifest/validate_env.go | 25 +++- internal/pkg/manifest/validate_env_test.go | 74 ++++++++---- internal/pkg/template/env.go | 18 +-- .../pkg/template/templates/environment/cf.yml | 8 +- 10 files changed, 254 insertions(+), 123 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/env.go b/internal/pkg/deploy/cloudformation/stack/env.go index 39862b341b1..debed0b9e9f 100644 --- a/internal/pkg/deploy/cloudformation/stack/env.go +++ b/internal/pkg/deploy/cloudformation/stack/env.go @@ -94,18 +94,23 @@ func (e *EnvStackConfig) Template() (string, error) { } forceUpdateID = id.String() } + + vpcConfig, err := e.vpcConfig() + if err != nil { + return "", err + } + 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, - VPCConfig: e.vpcConfig(), + VPCConfig: vpcConfig, PublicHTTPConfig: e.publicHTTPConfig(), PrivateHTTPConfig: e.privateHTTPConfig(), Telemetry: e.telemetryConfig(), CDNConfig: e.cdnConfig(), - SecurityGroupConfig: convertEnvSecurityGroupCfg(e.in.Mft), Version: e.in.Version, LatestVersion: deploy.LatestEnvTemplateVersion, @@ -365,12 +370,17 @@ func (e *EnvStackConfig) privateHTTPConfig() template.HTTPConfig { } } -func (e *EnvStackConfig) vpcConfig() template.VPCConfig { - return template.VPCConfig{ - Imported: e.importVPC(), - Managed: e.managedVPC(), - AllowVPCIngress: aws.BoolValue(e.in.Mft.HTTPConfig.Private.SecurityGroupsConfig.Ingress.VPCIngress), +func (e *EnvStackConfig) vpcConfig() (template.VPCConfig, error) { + securityGroupConfig, err := convertEnvSecurityGroupCfg(e.in.Mft) + if err != nil { + return template.VPCConfig{}, err } + return template.VPCConfig{ + Imported: e.importVPC(), + Managed: e.managedVPC(), + AllowVPCIngress: aws.BoolValue(e.in.Mft.HTTPConfig.Private.SecurityGroupsConfig.Ingress.VPCIngress), + SecurityGroupConfig: securityGroupConfig, + }, nil } func (e *EnvStackConfig) importVPC() *template.ImportVPC { diff --git a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go index 2d24174e79d..61d7ae067cd 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_integration_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_integration_test.go @@ -83,20 +83,20 @@ http: - cert-2 observability: container_insights: true # Enable container insights. -security_group: - ingress: - - ip_protocol: tcp - from_port: 0 - to_port: 65535 - cidr: 0.0.0.0 - - ip_protocol: tcp - from_port: 1 - to_port: 6 - cidr: 0.0.0.0 - egress: - - ip_protocol: tcp - to_port: 65535 - cidr: 0.0.0.0` +network: + vpc: + security_group: + ingress: + - ip_protocol: tcp + ports: 10 + cidr: 0.0.0.0 + - ip_protocol: tcp + ports: 1-10 + cidr: 0.0.0.0 + egress: + - ip_protocol: tcp + ports: 0-65535 + cidr: 0.0.0.0` var mft manifest.Environment err := yaml.Unmarshal([]byte(rawMft), &mft) require.NoError(t, err) diff --git a/internal/pkg/deploy/cloudformation/stack/env_test.go b/internal/pkg/deploy/cloudformation/stack/env_test.go index 24c3280ae5e..d99a5ffef99 100644 --- a/internal/pkg/deploy/cloudformation/stack/env_test.go +++ b/internal/pkg/deploy/cloudformation/stack/env_test.go @@ -40,6 +40,7 @@ func TestEnv_Template(t *testing.T) { PrivateSubnetCIDRs: DefaultPrivateSubnetCIDRs, PublicSubnetCIDRs: DefaultPublicSubnetCIDRs, }, + SecurityGroupConfig: nil, }, LatestVersion: deploy.LatestEnvTemplateVersion, CustomResources: map[string]template.S3ObjectLocation{ @@ -59,9 +60,9 @@ func TestEnv_Template(t *testing.T) { Telemetry: &template.Telemetry{ EnableContainerInsights: false, }, - SecurityGroupConfig: template.SecurityGroupConfig{}, - SerializedManifest: "name: env\ntype: Environment\n", - ForceUpdateID: "mockPreviousForceUpdateID", + + SerializedManifest: "name: env\ntype: Environment\n", + ForceUpdateID: "mockPreviousForceUpdateID", }, data) return &template.Content{Buffer: bytes.NewBufferString("mockTemplate")}, nil }) diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml index 2244393bd8a..58f1e01d403 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml @@ -13,20 +13,20 @@ Metadata: - cert-2 observability: container_insights: true # Enable container insights. - security_group: - ingress: - - ip_protocol: tcp - from_port: 0 - to_port: 65535 - cidr: 0.0.0.0 - - ip_protocol: tcp - from_port: 1 - to_port: 6 - cidr: 0.0.0.0 - egress: - - ip_protocol: tcp - to_port: 65535 - cidr: 0.0.0.0 + network: + vpc: + security_group: + ingress: + - ip_protocol: tcp + ports: 10 + cidr: 0.0.0.0 + - ip_protocol: tcp + ports: 1-10 + cidr: 0.0.0.0 + egress: + - ip_protocol: tcp + ports: 0-65535 + cidr: 0.0.0.0 Parameters: AppName: Type: String @@ -331,16 +331,16 @@ Resources: Value: !Sub 'copilot-${AppName}-${EnvironmentName}-env' SecurityGroupIngress: - CidrIp: "0.0.0.0" - FromPort: 0 + FromPort: 10 IpProtocol: tcp - ToPort: 65535 + ToPort: 10 - CidrIp: "0.0.0.0" FromPort: 1 IpProtocol: tcp - ToPort: 6 + ToPort: 10 SecurityGroupEgress: - CidrIp: "0.0.0.0" - FromPort: 65535 + FromPort: 0 IpProtocol: tcp ToPort: 65535 EnvironmentSecurityGroupIngressFromInternalALB: diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index e3a8b4ab43d..d9739d532ab 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -359,45 +359,50 @@ type networkLoadBalancerConfig struct { appDNSName *string } -func convertEnvSecurityGroupCfg(mft *manifest.Environment) template.SecurityGroupConfig { +func getPortValues(cfg manifest.SecurityGroupRule) (fromPort int, toPort int, err error) { + if cfg.PortsConfig.Ports.IsEmpty() { + toPort = *cfg.PortsConfig.Port + fromPort = *cfg.PortsConfig.Port + } else { + fromPort, toPort, err = cfg.PortsConfig.Ports.Parse() + if err != nil { + return 0, 0, err + } + } + return fromPort, toPort, nil +} + +func convertEnvSecurityGroupCfg(mft *manifest.Environment) (*template.SecurityGroupConfig, error) { securityGroupConfig, isSecurityConfigSet := mft.EnvSecurityGroup() if !isSecurityConfigSet { - return template.SecurityGroupConfig{} + return nil, nil } var ingress = make([]template.SecurityGroupRule, len(securityGroupConfig.Ingress)) var egress = make([]template.SecurityGroupRule, len(securityGroupConfig.Egress)) for idx, ingressValue := range securityGroupConfig.Ingress { ingress[idx].IpProtocol = ingressValue.IpProtocol ingress[idx].CidrIP = ingressValue.CidrIP - if ingressValue.IsToPortEmpty() { - ingress[idx].ToPort = toPortValue - } else { - ingress[idx].ToPort = *ingressValue.ToPort - } - if ingressValue.IsFromPortEmpty() { - ingress[idx].FromPort = fromPortValue + if fromPort, toPort, err := getPortValues(ingressValue); err != nil { + return nil, err } else { - ingress[idx].FromPort = *ingressValue.FromPort + ingress[idx].ToPort = toPort + ingress[idx].FromPort = fromPort } } for idx, egressValue := range securityGroupConfig.Egress { egress[idx].IpProtocol = egressValue.IpProtocol egress[idx].CidrIP = egressValue.CidrIP - if egressValue.IsToPortEmpty() { - egress[idx].ToPort = toPortValue - } else { - egress[idx].ToPort = *egressValue.ToPort - } - if egressValue.IsFromPortEmpty() { - egress[idx].FromPort = fromPortValue + if fromPort, toPort, err := getPortValues(egressValue); err != nil { + return nil, err } else { - egress[idx].FromPort = *egressValue.FromPort + egress[idx].ToPort = toPort + egress[idx].FromPort = fromPort } } - return template.SecurityGroupConfig{ + return &template.SecurityGroupConfig{ Ingress: ingress, Egress: egress, - } + }, nil } func (s *LoadBalancedWebService) convertNetworkLoadBalancer() (networkLoadBalancerConfig, error) { diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index dc7e38fa5bd..34aab268a5e 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -6,9 +6,10 @@ package manifest import ( "errors" "fmt" - "sort" - "github.com/aws/copilot-cli/internal/pkg/config" + "sort" + "strconv" + "strings" "github.com/aws/aws-sdk-go/aws" "github.com/aws/copilot-cli/internal/pkg/template" @@ -20,6 +21,12 @@ const EnvironmentManifestType = "Environment" var environmentManifestPath = "environment/manifest.yml" +// Error definitions. +var ( + errUnmarshalPortsConfig = errors.New(`unable to unmarshal ports field into int or a range`) + errUnmarshalEnvironmentCDNConfig = errors.New(`unable to unmarshal cdn field into bool or composite-style map`) +) + // Environment is the manifest configuration for an environment. type Environment struct { Workload `yaml:",inline"` @@ -85,11 +92,10 @@ func (e *Environment) MarshalBinary() ([]byte, error) { // 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"` - CDNConfig environmentCDNConfig `yaml:"cdn,omitempty,flow"` - SecurityGroupConfig securityGroupConfig `yaml:"security_group,omitempty"` + Network environmentNetworkConfig `yaml:"network,omitempty,flow"` + Observability environmentObservability `yaml:"observability,omitempty,flow"` + HTTPConfig EnvironmentHTTPConfig `yaml:"http,omitempty,flow"` + CDNConfig environmentCDNConfig `yaml:"cdn,omitempty,flow"` } // IsIngressRestrictedToCDN returns whether or not an environment has its @@ -103,42 +109,96 @@ type environmentNetworkConfig struct { } type environmentVPCConfig struct { - ID *string `yaml:"id,omitempty"` - CIDR *IPNet `yaml:"cidr,omitempty"` - Subnets subnetsConfiguration `yaml:"subnets,omitempty"` + ID *string `yaml:"id,omitempty"` + CIDR *IPNet `yaml:"cidr,omitempty"` + Subnets subnetsConfiguration `yaml:"subnets,omitempty"` + SecurityGroupConfig securityGroupConfig `yaml:"security_group,omitempty"` } type securityGroupConfig struct { - Ingress []securityGroupRule `yaml:"ingress,omitempty"` - Egress []securityGroupRule `yaml:"egress,omitempty"` + Ingress []SecurityGroupRule `yaml:"ingress,omitempty"` + Egress []SecurityGroupRule `yaml:"egress,omitempty"` } func (cfg securityGroupConfig) isEmpty() bool { return len(cfg.Ingress) == 0 && len(cfg.Egress) == 0 } -type securityGroupRule struct { - CidrIP string `yaml:"cidr"` - FromPort *int `yaml:"from_port"` - IpProtocol string `yaml:"ip_protocol"` - ToPort *int `yaml:"to_port"` +// SecurityGroupRule holds the security group ingress and egress configs +type SecurityGroupRule struct { + CidrIP string `yaml:"cidr"` + PortsConfig PortsConfig `yaml:"ports"` + IpProtocol string `yaml:"ip_protocol"` +} + +// Ports is a custom type which supports unmarshaling yaml which +// can either be of type int or type PortsRangeBand. +type PortsConfig struct { + Port *int // 0 is a valid value, so we want the default value to be nil. + Ports *PortsRangeBand // Mutually exclusive with port. +} + +// PortsRangeBand is a number range with to and from port values. +type PortsRangeBand string + +// Parse parses Ports string and returns the from and to port values. +// For example: 1-100 returns 1 and 100. +func (str PortsRangeBand) Parse() (fromPort int, toPort int, err error) { + ports := strings.Split(string(str), "-") + fromPort, err = strconv.Atoi(ports[0]) + if err != nil { + return 0, 0, fmt.Errorf("cannot convert from_port value %s to integer", ports[0]) + } + toPort, err = strconv.Atoi(ports[1]) + if err != nil { + return 0, 0, fmt.Errorf("cannot convert to_port value %s to integer", ports[1]) + } + return fromPort, toPort, nil } -// IsToPortEmpty checks if ToPort is empty -func (cfg securityGroupRule) IsToPortEmpty() bool { - return cfg.ToPort == nil +// IsEmpty returns whether PortsRangeBand is empty. +func (cfg *PortsRangeBand) IsEmpty() bool { + return cfg == nil } -// IsFromPortEmpty checks if FromPort is empty -func (cfg securityGroupRule) IsFromPortEmpty() bool { - return cfg.FromPort == nil +// IsEmpty returns whether PortsConfig is empty. +func (cfg *PortsConfig) IsEmpty() bool { + return cfg.Port == nil && cfg.Ports == nil +} + +// UnmarshalYAML overrides the default YAML unmarshaling logic for the Ports +// struct, allowing it to perform more complex unmarshaling behavior. +// This method implements the yaml.Unmarshaler (v3) interface. +func (cfg *PortsConfig) UnmarshalYAML(value *yaml.Node) error { + _, err := strconv.Atoi(value.Value) // detmines if input is integer? if err then we decode range values + if err != nil { + if err := value.Decode(&cfg.Ports); err != nil { + switch err.(type) { + case *yaml.TypeError: + break + default: + return err + } + } + } + + if !cfg.Ports.IsEmpty() { + // Successfully unmarshalled Ports fields and unset port field, return + cfg.Port = nil + return nil + } + + if err := value.Decode(&cfg.Port); err != nil { + return errUnmarshalPortsConfig + } + return nil } // EnvSecurityGroup returns the security group config if the user has set any values. // If there is no env security group settings, then returns nil and false. func (cfg *EnvironmentConfig) EnvSecurityGroup() (*securityGroupConfig, bool) { - if isEmpty := cfg.SecurityGroupConfig.isEmpty(); !isEmpty { - return &cfg.SecurityGroupConfig, true + if isEmpty := cfg.Network.VPC.SecurityGroupConfig.isEmpty(); !isEmpty { + return &cfg.Network.VPC.SecurityGroupConfig, true } return nil, false } @@ -187,7 +247,7 @@ func (cfg *environmentCDNConfig) UnmarshalYAML(value *yaml.Node) error { } if err := value.Decode(&cfg.Enabled); err != nil { - return errors.New(`unable to unmarshal into bool or composite-style map`) + return errUnmarshalEnvironmentCDNConfig } return nil } diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 53601fcf013..3a23cea90d1 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -6,6 +6,7 @@ package manifest import ( "errors" "fmt" + "strings" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/arn" @@ -36,7 +37,7 @@ func (e EnvironmentConfig) validate() error { if err := e.HTTPConfig.validate(); err != nil { return fmt.Errorf(`validate "http config": %w`, err) } - if err := e.SecurityGroupConfig.validate(); err != nil { + if err := e.Network.VPC.SecurityGroupConfig.validate(); err != nil { return fmt.Errorf(`validate "security_group": %w`, err) } if e.IsIngressRestrictedToCDN() && !e.CDNConfig.CDNEnabled() { @@ -84,13 +85,33 @@ func (cfg environmentVPCConfig) validate() error { } // validate returns nil if securityGroupRule has all the required parameters set. -func (cfg securityGroupRule) validate() error { +func (cfg SecurityGroupRule) validate() error { if cfg.CidrIP == "" { return fmt.Errorf(`cidr`) } if cfg.IpProtocol == "" { return fmt.Errorf(`ip_protocol`) } + return cfg.PortsConfig.validate() +} + +// validate if ports are set. +func (cfg PortsConfig) validate() error { + if cfg.IsEmpty() { + return fmt.Errorf(`ports`) + } + if !cfg.Ports.IsEmpty() { + return cfg.Ports.validate() + } + return nil +} + +// validate checks if PortsRangeBand is set correctly. +func (str PortsRangeBand) validate() error { + ports := strings.Split(string(str), "-") + if len(ports) != 2 { + return fmt.Errorf("invalid ports value %s. Should be in format of ${from_port}-${to_port}", string(str)) + } return nil } diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index ab8ee6f3b0e..f843eec9b9a 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -95,12 +95,17 @@ func TestEnvironmentConfig_validate(t *testing.T) { }, "error if invalid security group config": { in: EnvironmentConfig{ - SecurityGroupConfig: securityGroupConfig{ - Ingress: []securityGroupRule{ - { - IpProtocol: "tcp", - ToPort: aws.Int(80), - FromPort: aws.Int(80), + Network: environmentNetworkConfig{ + VPC: environmentVPCConfig{ + SecurityGroupConfig: securityGroupConfig{ + Ingress: []SecurityGroupRule{ + { + IpProtocol: "tcp", + PortsConfig: PortsConfig{ + Port: aws.Int(80), + }, + }, + }, }, }, }, @@ -109,30 +114,59 @@ func TestEnvironmentConfig_validate(t *testing.T) { }, "valid security group config": { in: EnvironmentConfig{ - SecurityGroupConfig: securityGroupConfig{ - Ingress: []securityGroupRule{ - { - CidrIP: "0.0.0.0", - IpProtocol: "tcp", - ToPort: aws.Int(80), - FromPort: aws.Int(80), + Network: environmentNetworkConfig{ + VPC: environmentVPCConfig{ + SecurityGroupConfig: securityGroupConfig{ + Ingress: []SecurityGroupRule{ + { + CidrIP: "0.0.0.0", + IpProtocol: "tcp", + PortsConfig: PortsConfig{ + Ports: (*PortsRangeBand)(aws.String("1-10")), + }, + }, + }, }, }, }, }, }, - "valid security group config without FromPort": { + "invalid ports value in security group config": { in: EnvironmentConfig{ - SecurityGroupConfig: securityGroupConfig{ - Ingress: []securityGroupRule{ - { - CidrIP: "0.0.0.0", - IpProtocol: "tcp", - ToPort: aws.Int(80), + Network: environmentNetworkConfig{ + VPC: environmentVPCConfig{ + SecurityGroupConfig: securityGroupConfig{ + Ingress: []SecurityGroupRule{ + { + CidrIP: "0.0.0.0", + IpProtocol: "tcp", + PortsConfig: PortsConfig{ + Ports: (*PortsRangeBand)(aws.String("1-10-10")), + }, + }, + }, + }, + }, + }, + }, + wantedError: "validate \"security_group\": validate ingress[0]: \"invalid ports value 1-10-10. Should be in format of ${from_port}-${to_port}\" must be specified", + }, + "valid security group config without ports": { + in: EnvironmentConfig{ + Network: environmentNetworkConfig{ + VPC: environmentVPCConfig{ + SecurityGroupConfig: securityGroupConfig{ + Ingress: []SecurityGroupRule{ + { + CidrIP: "0.0.0.0", + IpProtocol: "tcp", + }, + }, }, }, }, }, + wantedError: "validate \"security_group\": validate ingress[0]: \"ports\" must be specified", }, "error if security group ingress is limited to a cdn distribution not enabled": { in: EnvironmentConfig{ diff --git a/internal/pkg/template/env.go b/internal/pkg/template/env.go index e26b6d9922a..f266592971c 100644 --- a/internal/pkg/template/env.go +++ b/internal/pkg/template/env.go @@ -105,12 +105,11 @@ type EnvOpts struct { ArtifactBucketARN string ArtifactBucketKeyARN string - VPCConfig VPCConfig - PublicHTTPConfig HTTPConfig - PrivateHTTPConfig HTTPConfig - Telemetry *Telemetry - CDNConfig *CDNConfig - SecurityGroupConfig SecurityGroupConfig + VPCConfig VPCConfig + PublicHTTPConfig HTTPConfig + PrivateHTTPConfig HTTPConfig + Telemetry *Telemetry + CDNConfig *CDNConfig LatestVersion string SerializedManifest string // Serialized manifest used to render the environment template. @@ -128,9 +127,10 @@ type HTTPConfig struct { type CDNConfig struct{} type VPCConfig struct { - Imported *ImportVPC // If not-nil, use the imported VPC resources instead of the Managed VPC. - Managed ManagedVPC - AllowVPCIngress bool + Imported *ImportVPC // If not-nil, use the imported VPC resources instead of the Managed VPC. + Managed ManagedVPC + AllowVPCIngress bool + SecurityGroupConfig *SecurityGroupConfig } // ImportVPC holds the fields to import VPC resources. diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index b84f9be0f54..a6d123f3145 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -189,18 +189,18 @@ Resources: Tags: - Key: Name Value: !Sub 'copilot-${AppName}-${EnvironmentName}-env' -{{- if (gt (len .SecurityGroupConfig.Ingress) 0)}} +{{- if and .VPCConfig.SecurityGroupConfig (gt (len .VPCConfig.SecurityGroupConfig.Ingress) 0)}} SecurityGroupIngress: - {{range $securityRule := .SecurityGroupConfig.Ingress}} + {{range $securityRule := .VPCConfig.SecurityGroupConfig.Ingress}} - IpProtocol: {{$securityRule.IpProtocol}} FromPort: {{$securityRule.FromPort}} ToPort: {{$securityRule.ToPort}} CidrIp: {{$securityRule.CidrIP}} {{end }} {{- end}} - {{- if (gt (len .SecurityGroupConfig.Egress) 0)}} + {{- if and .VPCConfig.SecurityGroupConfig (gt (len .VPCConfig.SecurityGroupConfig.Egress) 0)}} SecurityGroupEgress: - {{range $securityRule := .SecurityGroupConfig.Egress}} + {{range $securityRule := .VPCConfig.SecurityGroupConfig.Egress}} - IpProtocol: {{$securityRule.IpProtocol}} FromPort: {{$securityRule.FromPort}} ToPort: {{$securityRule.ToPort}} From 3610a1d2f3fb04a3775344967d8a3585989d9a6c Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Fri, 29 Jul 2022 15:30:36 -0700 Subject: [PATCH 12/19] remove extra code --- internal/pkg/deploy/cloudformation/stack/transformers.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index d9739d532ab..a7df6d8fe45 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -52,11 +52,6 @@ const ( maxPercentDefault = 200 ) -const ( - toPortValue = 0 - fromPortValue = 65535 -) - var ( taskDefOverrideRulePrefixes = []string{"Resources", "TaskDefinition", "Properties"} subnetPlacementForTemplate = map[manifest.PlacementString]string{ From d2fc3995db185cf59070b7e1324de7e3ae96e376 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Mon, 1 Aug 2022 12:01:20 -0700 Subject: [PATCH 13/19] address feedback by Efe --- .../cloudformation/stack/transformers.go | 17 ++----------- internal/pkg/manifest/env.go | 25 +++++++++++++------ internal/pkg/manifest/validate.go | 2 +- internal/pkg/manifest/validate_env.go | 22 ++++++++-------- internal/pkg/manifest/validate_env_test.go | 2 +- internal/pkg/manifest/validate_test.go | 2 +- .../pkg/template/templates/environment/cf.yml | 8 +++--- 7 files changed, 38 insertions(+), 40 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index a7df6d8fe45..351767f876a 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -354,19 +354,6 @@ type networkLoadBalancerConfig struct { appDNSName *string } -func getPortValues(cfg manifest.SecurityGroupRule) (fromPort int, toPort int, err error) { - if cfg.PortsConfig.Ports.IsEmpty() { - toPort = *cfg.PortsConfig.Port - fromPort = *cfg.PortsConfig.Port - } else { - fromPort, toPort, err = cfg.PortsConfig.Ports.Parse() - if err != nil { - return 0, 0, err - } - } - return fromPort, toPort, nil -} - func convertEnvSecurityGroupCfg(mft *manifest.Environment) (*template.SecurityGroupConfig, error) { securityGroupConfig, isSecurityConfigSet := mft.EnvSecurityGroup() if !isSecurityConfigSet { @@ -377,7 +364,7 @@ func convertEnvSecurityGroupCfg(mft *manifest.Environment) (*template.SecurityGr for idx, ingressValue := range securityGroupConfig.Ingress { ingress[idx].IpProtocol = ingressValue.IpProtocol ingress[idx].CidrIP = ingressValue.CidrIP - if fromPort, toPort, err := getPortValues(ingressValue); err != nil { + if fromPort, toPort, err := ingressValue.Ports(); err != nil { return nil, err } else { ingress[idx].ToPort = toPort @@ -387,7 +374,7 @@ func convertEnvSecurityGroupCfg(mft *manifest.Environment) (*template.SecurityGr for idx, egressValue := range securityGroupConfig.Egress { egress[idx].IpProtocol = egressValue.IpProtocol egress[idx].CidrIP = egressValue.CidrIP - if fromPort, toPort, err := getPortValues(egressValue); err != nil { + if fromPort, toPort, err := egressValue.Ports(); err != nil { return nil, err } else { egress[idx].ToPort = toPort diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 34aab268a5e..84241b71608 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -124,26 +124,27 @@ func (cfg securityGroupConfig) isEmpty() bool { return len(cfg.Ingress) == 0 && len(cfg.Egress) == 0 } -// SecurityGroupRule holds the security group ingress and egress configs +// SecurityGroupRule holds the security group ingress and egress configs. type SecurityGroupRule struct { CidrIP string `yaml:"cidr"` PortsConfig PortsConfig `yaml:"ports"` IpProtocol string `yaml:"ip_protocol"` } -// Ports is a custom type which supports unmarshaling yaml which -// can either be of type int or type PortsRangeBand. +// PortsConfig represents a range of ports [from:to] inclusive. +// The simple form allow represent from and to ports as a single value, whereas the advanced form is for different values. type PortsConfig struct { Port *int // 0 is a valid value, so we want the default value to be nil. Ports *PortsRangeBand // Mutually exclusive with port. } -// PortsRangeBand is a number range with to and from port values. +// PortsRangeBand represents a range of from and to port values. +// For example, "0-65536". type PortsRangeBand string -// Parse parses Ports string and returns the from and to port values. +// parse parses Ports string and returns the from and to port values. // For example: 1-100 returns 1 and 100. -func (str PortsRangeBand) Parse() (fromPort int, toPort int, err error) { +func (str PortsRangeBand) parse() (fromPort int, toPort int, err error) { ports := strings.Split(string(str), "-") fromPort, err = strconv.Atoi(ports[0]) if err != nil { @@ -158,12 +159,20 @@ func (str PortsRangeBand) Parse() (fromPort int, toPort int, err error) { // IsEmpty returns whether PortsRangeBand is empty. func (cfg *PortsRangeBand) IsEmpty() bool { - return cfg == nil + return cfg == nil || *cfg == "" } // IsEmpty returns whether PortsConfig is empty. func (cfg *PortsConfig) IsEmpty() bool { - return cfg.Port == nil && cfg.Ports == nil + return cfg.Port == nil && cfg.Ports.IsEmpty() +} + +// Ports returns the from and to ports of a security group rule. +func (r SecurityGroupRule) Ports() (from, to int, err error) { + if r.PortsConfig.Ports.IsEmpty() { + return *r.PortsConfig.Port, *r.PortsConfig.Port, nil // a single value is provided for ports. + } + return r.PortsConfig.Ports.parse() } // UnmarshalYAML overrides the default YAML unmarshaling logic for the Ports diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index 45b8d51e8ed..d6bf4bf2c62 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -1031,7 +1031,7 @@ func (r IntRangeBand) validate() error { minMax := intRangeBandRegexp.FindStringSubmatch(str) // Valid minMax example: ["1-2", "1", "2"] if len(minMax) != 3 { - return fmt.Errorf("invalid range value %s. Should be in format of ${min}-${max}", str) + return fmt.Errorf("invalid range value %s: valid format is ${min}-${max}", str) } // Guaranteed by intRangeBandRegexp. min, err := strconv.Atoi(minMax[1]) diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 3a23cea90d1..e5c6ba17ae2 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -87,10 +87,14 @@ func (cfg environmentVPCConfig) validate() error { // validate returns nil if securityGroupRule has all the required parameters set. func (cfg SecurityGroupRule) validate() error { if cfg.CidrIP == "" { - return fmt.Errorf(`cidr`) + return &errFieldMustBeSpecified{ + missingField: "cidr", + } } if cfg.IpProtocol == "" { - return fmt.Errorf(`ip_protocol`) + return &errFieldMustBeSpecified{ + missingField: "ip_protocol", + } } return cfg.PortsConfig.validate() } @@ -98,7 +102,9 @@ func (cfg SecurityGroupRule) validate() error { // validate if ports are set. func (cfg PortsConfig) validate() error { if cfg.IsEmpty() { - return fmt.Errorf(`ports`) + return &errFieldMustBeSpecified{ + missingField: "ports", + } } if !cfg.Ports.IsEmpty() { return cfg.Ports.validate() @@ -110,7 +116,7 @@ func (cfg PortsConfig) validate() error { func (str PortsRangeBand) validate() error { ports := strings.Split(string(str), "-") if len(ports) != 2 { - return fmt.Errorf("invalid ports value %s. Should be in format of ${from_port}-${to_port}", string(str)) + return fmt.Errorf("invalid ports value %s: valid port format is ${from_port}-${to_port}", string(str)) } return nil } @@ -119,16 +125,12 @@ func (str PortsRangeBand) validate() error { func (cfg securityGroupConfig) validate() error { for idx, ingress := range cfg.Ingress { if err := ingress.validate(); err != nil { - return fmt.Errorf(`validate ingress[%d]: %w`, idx, &errFieldMustBeSpecified{ - missingField: err.Error(), - }) + return fmt.Errorf(`validate ingress[%d]: %w`, idx, err) } } for idx, egress := range cfg.Egress { if err := egress.validate(); err != nil { - return fmt.Errorf(`validate egress[%d]: %w`, idx, &errFieldMustBeSpecified{ - missingField: err.Error(), - }) + return fmt.Errorf(`validate egress[%d]: %w`, idx, err) } } return nil diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index f843eec9b9a..38054dfbb7c 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -149,7 +149,7 @@ func TestEnvironmentConfig_validate(t *testing.T) { }, }, }, - wantedError: "validate \"security_group\": validate ingress[0]: \"invalid ports value 1-10-10. Should be in format of ${from_port}-${to_port}\" must be specified", + wantedError: "validate \"security_group\": validate ingress[0]: invalid ports value 1-10-10: valid port format is ${from_port}-${to_port}", }, "valid security group config without ports": { in: EnvironmentConfig{ diff --git a/internal/pkg/manifest/validate_test.go b/internal/pkg/manifest/validate_test.go index 238305e8d1e..b2faa296420 100644 --- a/internal/pkg/manifest/validate_test.go +++ b/internal/pkg/manifest/validate_test.go @@ -2020,7 +2020,7 @@ func TestIntRangeBand_validate(t *testing.T) { }{ "error if range value is in invalid format": { IntRangeBand: IntRangeBand(*aws.String("")), - wantedError: fmt.Errorf("invalid range value . Should be in format of ${min}-${max}"), + wantedError: fmt.Errorf("invalid range value : valid format is ${min}-${max}"), }, "error if range min is greater than max": { IntRangeBand: IntRangeBand(*aws.String("6-4")), diff --git a/internal/pkg/template/templates/environment/cf.yml b/internal/pkg/template/templates/environment/cf.yml index a6d123f3145..d9ed6ee3f52 100644 --- a/internal/pkg/template/templates/environment/cf.yml +++ b/internal/pkg/template/templates/environment/cf.yml @@ -191,21 +191,21 @@ Resources: Value: !Sub 'copilot-${AppName}-${EnvironmentName}-env' {{- if and .VPCConfig.SecurityGroupConfig (gt (len .VPCConfig.SecurityGroupConfig.Ingress) 0)}} SecurityGroupIngress: - {{range $securityRule := .VPCConfig.SecurityGroupConfig.Ingress}} + {{- range $securityRule := .VPCConfig.SecurityGroupConfig.Ingress}} - IpProtocol: {{$securityRule.IpProtocol}} FromPort: {{$securityRule.FromPort}} ToPort: {{$securityRule.ToPort}} CidrIp: {{$securityRule.CidrIP}} - {{end }} + {{- end }} {{- end}} {{- if and .VPCConfig.SecurityGroupConfig (gt (len .VPCConfig.SecurityGroupConfig.Egress) 0)}} SecurityGroupEgress: - {{range $securityRule := .VPCConfig.SecurityGroupConfig.Egress}} + {{- range $securityRule := .VPCConfig.SecurityGroupConfig.Egress}} - IpProtocol: {{$securityRule.IpProtocol}} FromPort: {{$securityRule.FromPort}} ToPort: {{$securityRule.ToPort}} CidrIp: {{$securityRule.CidrIP}} - {{end }} + {{- end }} {{- end}} EnvironmentHTTPSecurityGroupIngressFromPublicALB: Type: AWS::EC2::SecurityGroupIngress From df845b502b91ce20d36df2f1cac6626a685d89d4 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Tue, 2 Aug 2022 11:18:02 -0700 Subject: [PATCH 14/19] nits --- .../cloudformation/stack/transformers.go | 4 +-- internal/pkg/manifest/env.go | 34 +++++++++---------- internal/pkg/manifest/validate_env.go | 6 ++-- internal/pkg/manifest/validate_env_test.go | 14 ++++---- 4 files changed, 29 insertions(+), 29 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 351767f876a..d114a90c3fd 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -364,7 +364,7 @@ func convertEnvSecurityGroupCfg(mft *manifest.Environment) (*template.SecurityGr for idx, ingressValue := range securityGroupConfig.Ingress { ingress[idx].IpProtocol = ingressValue.IpProtocol ingress[idx].CidrIP = ingressValue.CidrIP - if fromPort, toPort, err := ingressValue.Ports(); err != nil { + if fromPort, toPort, err := ingressValue.GetPorts(); err != nil { return nil, err } else { ingress[idx].ToPort = toPort @@ -374,7 +374,7 @@ func convertEnvSecurityGroupCfg(mft *manifest.Environment) (*template.SecurityGr for idx, egressValue := range securityGroupConfig.Egress { egress[idx].IpProtocol = egressValue.IpProtocol egress[idx].CidrIP = egressValue.CidrIP - if fromPort, toPort, err := egressValue.Ports(); err != nil { + if fromPort, toPort, err := egressValue.GetPorts(); err != nil { return nil, err } else { egress[idx].ToPort = toPort diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 84241b71608..0b05f639802 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -116,24 +116,24 @@ type environmentVPCConfig struct { } type securityGroupConfig struct { - Ingress []SecurityGroupRule `yaml:"ingress,omitempty"` - Egress []SecurityGroupRule `yaml:"egress,omitempty"` + Ingress []securityGroupRule `yaml:"ingress,omitempty"` + Egress []securityGroupRule `yaml:"egress,omitempty"` } func (cfg securityGroupConfig) isEmpty() bool { return len(cfg.Ingress) == 0 && len(cfg.Egress) == 0 } -// SecurityGroupRule holds the security group ingress and egress configs. -type SecurityGroupRule struct { - CidrIP string `yaml:"cidr"` - PortsConfig PortsConfig `yaml:"ports"` - IpProtocol string `yaml:"ip_protocol"` +// securityGroupRule holds the security group ingress and egress configs. +type securityGroupRule struct { + CidrIP string `yaml:"cidr"` + Ports portsConfig `yaml:"ports"` + IpProtocol string `yaml:"ip_protocol"` } -// PortsConfig represents a range of ports [from:to] inclusive. -// The simple form allow represent from and to ports as a single value, whereas the advanced form is for different values. -type PortsConfig struct { +// portsConfig represents a range of ports [from:to] inclusive. +// The simple form allow represents from and to ports as a single value, whereas the advanced form is for different values. +type portsConfig struct { Port *int // 0 is a valid value, so we want the default value to be nil. Ports *PortsRangeBand // Mutually exclusive with port. } @@ -163,22 +163,22 @@ func (cfg *PortsRangeBand) IsEmpty() bool { } // IsEmpty returns whether PortsConfig is empty. -func (cfg *PortsConfig) IsEmpty() bool { +func (cfg *portsConfig) IsEmpty() bool { return cfg.Port == nil && cfg.Ports.IsEmpty() } -// Ports returns the from and to ports of a security group rule. -func (r SecurityGroupRule) Ports() (from, to int, err error) { - if r.PortsConfig.Ports.IsEmpty() { - return *r.PortsConfig.Port, *r.PortsConfig.Port, nil // a single value is provided for ports. +// GetPorts returns the from and to ports of a security group rule. +func (r securityGroupRule) GetPorts() (from, to int, err error) { + if r.Ports.Ports.IsEmpty() { + return aws.IntValue(r.Ports.Port), aws.IntValue(r.Ports.Port), nil // a single value is provided for ports. } - return r.PortsConfig.Ports.parse() + return r.Ports.Ports.parse() } // UnmarshalYAML overrides the default YAML unmarshaling logic for the Ports // struct, allowing it to perform more complex unmarshaling behavior. // This method implements the yaml.Unmarshaler (v3) interface. -func (cfg *PortsConfig) UnmarshalYAML(value *yaml.Node) error { +func (cfg *portsConfig) UnmarshalYAML(value *yaml.Node) error { _, err := strconv.Atoi(value.Value) // detmines if input is integer? if err then we decode range values if err != nil { if err := value.Decode(&cfg.Ports); err != nil { diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index e5c6ba17ae2..6a68c80fd7a 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -85,7 +85,7 @@ func (cfg environmentVPCConfig) validate() error { } // validate returns nil if securityGroupRule has all the required parameters set. -func (cfg SecurityGroupRule) validate() error { +func (cfg securityGroupRule) validate() error { if cfg.CidrIP == "" { return &errFieldMustBeSpecified{ missingField: "cidr", @@ -96,11 +96,11 @@ func (cfg SecurityGroupRule) validate() error { missingField: "ip_protocol", } } - return cfg.PortsConfig.validate() + return cfg.Ports.validate() } // validate if ports are set. -func (cfg PortsConfig) validate() error { +func (cfg portsConfig) validate() error { if cfg.IsEmpty() { return &errFieldMustBeSpecified{ missingField: "ports", diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index 38054dfbb7c..9ee9a03ff6a 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -98,10 +98,10 @@ func TestEnvironmentConfig_validate(t *testing.T) { Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ SecurityGroupConfig: securityGroupConfig{ - Ingress: []SecurityGroupRule{ + Ingress: []securityGroupRule{ { IpProtocol: "tcp", - PortsConfig: PortsConfig{ + Ports: portsConfig{ Port: aws.Int(80), }, }, @@ -117,11 +117,11 @@ func TestEnvironmentConfig_validate(t *testing.T) { Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ SecurityGroupConfig: securityGroupConfig{ - Ingress: []SecurityGroupRule{ + Ingress: []securityGroupRule{ { CidrIP: "0.0.0.0", IpProtocol: "tcp", - PortsConfig: PortsConfig{ + Ports: portsConfig{ Ports: (*PortsRangeBand)(aws.String("1-10")), }, }, @@ -136,11 +136,11 @@ func TestEnvironmentConfig_validate(t *testing.T) { Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ SecurityGroupConfig: securityGroupConfig{ - Ingress: []SecurityGroupRule{ + Ingress: []securityGroupRule{ { CidrIP: "0.0.0.0", IpProtocol: "tcp", - PortsConfig: PortsConfig{ + Ports: portsConfig{ Ports: (*PortsRangeBand)(aws.String("1-10-10")), }, }, @@ -156,7 +156,7 @@ func TestEnvironmentConfig_validate(t *testing.T) { Network: environmentNetworkConfig{ VPC: environmentVPCConfig{ SecurityGroupConfig: securityGroupConfig{ - Ingress: []SecurityGroupRule{ + Ingress: []securityGroupRule{ { CidrIP: "0.0.0.0", IpProtocol: "tcp", From 63fad79048112c3e95988a9567e03975bedd5d9c Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Tue, 2 Aug 2022 13:24:47 -0700 Subject: [PATCH 15/19] remove extra string to int conversion from unmarshalYAML --- internal/pkg/manifest/env.go | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 0b05f639802..f3b8ee9a068 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -179,25 +179,23 @@ func (r securityGroupRule) GetPorts() (from, to int, err error) { // struct, allowing it to perform more complex unmarshaling behavior. // This method implements the yaml.Unmarshaler (v3) interface. func (cfg *portsConfig) UnmarshalYAML(value *yaml.Node) error { - _, err := strconv.Atoi(value.Value) // detmines if input is integer? if err then we decode range values - if err != nil { - if err := value.Decode(&cfg.Ports); err != nil { - switch err.(type) { - case *yaml.TypeError: - break - default: - return err - } + if err := value.Decode(&cfg.Port); err != nil { + switch err.(type) { + case *yaml.TypeError: + cfg.Port = nil + break + default: + return err } } - if !cfg.Ports.IsEmpty() { - // Successfully unmarshalled Ports fields and unset port field, return - cfg.Port = nil + if cfg.Port != nil { + // Successfully unmarshalled Port field and unset Ports field, return + cfg.Ports = nil return nil } - if err := value.Decode(&cfg.Port); err != nil { + if err := value.Decode(&cfg.Ports); err != nil { return errUnmarshalPortsConfig } return nil From 24b4525b057ea64f266ad6242715fc19c7808f52 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Tue, 2 Aug 2022 15:20:29 -0700 Subject: [PATCH 16/19] use IntRangeBand instead of creating new PortRangeBand --- internal/pkg/manifest/env.go | 41 ++++------------------ internal/pkg/manifest/validate_env.go | 21 +++++------ internal/pkg/manifest/validate_env_test.go | 4 +-- 3 files changed, 17 insertions(+), 49 deletions(-) diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index f3b8ee9a068..0724c489f44 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -6,14 +6,11 @@ package manifest import ( "errors" "fmt" - "github.com/aws/copilot-cli/internal/pkg/config" - "sort" - "strconv" - "strings" - "github.com/aws/aws-sdk-go/aws" + "github.com/aws/copilot-cli/internal/pkg/config" "github.com/aws/copilot-cli/internal/pkg/template" "gopkg.in/yaml.v3" + "sort" ) // EnvironmentManifestType identifies that the type of manifest is environment manifest. @@ -134,45 +131,21 @@ type securityGroupRule struct { // portsConfig represents a range of ports [from:to] inclusive. // The simple form allow represents from and to ports as a single value, whereas the advanced form is for different values. type portsConfig struct { - Port *int // 0 is a valid value, so we want the default value to be nil. - Ports *PortsRangeBand // Mutually exclusive with port. -} - -// PortsRangeBand represents a range of from and to port values. -// For example, "0-65536". -type PortsRangeBand string - -// parse parses Ports string and returns the from and to port values. -// For example: 1-100 returns 1 and 100. -func (str PortsRangeBand) parse() (fromPort int, toPort int, err error) { - ports := strings.Split(string(str), "-") - fromPort, err = strconv.Atoi(ports[0]) - if err != nil { - return 0, 0, fmt.Errorf("cannot convert from_port value %s to integer", ports[0]) - } - toPort, err = strconv.Atoi(ports[1]) - if err != nil { - return 0, 0, fmt.Errorf("cannot convert to_port value %s to integer", ports[1]) - } - return fromPort, toPort, nil -} - -// IsEmpty returns whether PortsRangeBand is empty. -func (cfg *PortsRangeBand) IsEmpty() bool { - return cfg == nil || *cfg == "" + Port *int // 0 is a valid value, so we want the default value to be nil. + Ports *IntRangeBand // Mutually exclusive with port. } // IsEmpty returns whether PortsConfig is empty. func (cfg *portsConfig) IsEmpty() bool { - return cfg.Port == nil && cfg.Ports.IsEmpty() + return cfg.Port == nil && cfg.Ports == nil } // GetPorts returns the from and to ports of a security group rule. func (r securityGroupRule) GetPorts() (from, to int, err error) { - if r.Ports.Ports.IsEmpty() { + if r.Ports.Ports == nil { return aws.IntValue(r.Ports.Port), aws.IntValue(r.Ports.Port), nil // a single value is provided for ports. } - return r.Ports.Ports.parse() + return r.Ports.Ports.Parse() } // UnmarshalYAML overrides the default YAML unmarshaling logic for the Ports diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index 6a68c80fd7a..d8ff3a0325e 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -6,10 +6,9 @@ package manifest import ( "errors" "fmt" - "strings" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/arn" + "strings" ) var ( @@ -106,17 +105,13 @@ func (cfg portsConfig) validate() error { missingField: "ports", } } - if !cfg.Ports.IsEmpty() { - return cfg.Ports.validate() - } - return nil -} - -// validate checks if PortsRangeBand is set correctly. -func (str PortsRangeBand) validate() error { - ports := strings.Split(string(str), "-") - if len(ports) != 2 { - return fmt.Errorf("invalid ports value %s: valid port format is ${from_port}-${to_port}", string(str)) + if cfg.Ports != nil { + if err := cfg.Ports.validate(); err != nil { + if strings.Contains(err.Error(), "invalid range value") { + return fmt.Errorf("invalid ports value %s: valid port format is ${from_port}-${to_port}", *cfg.Ports) + } + return err + } } return nil } diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index 9ee9a03ff6a..be53528ac62 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -122,7 +122,7 @@ func TestEnvironmentConfig_validate(t *testing.T) { CidrIP: "0.0.0.0", IpProtocol: "tcp", Ports: portsConfig{ - Ports: (*PortsRangeBand)(aws.String("1-10")), + Ports: (*IntRangeBand)(aws.String("1-10")), }, }, }, @@ -141,7 +141,7 @@ func TestEnvironmentConfig_validate(t *testing.T) { CidrIP: "0.0.0.0", IpProtocol: "tcp", Ports: portsConfig{ - Ports: (*PortsRangeBand)(aws.String("1-10-10")), + Ports: (*IntRangeBand)(aws.String("1-10-10")), }, }, }, From ef75b74e6b971d3e9bb155ca7951b71d3a67df95 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Wed, 3 Aug 2022 12:11:36 -0700 Subject: [PATCH 17/19] minor nits and change error format --- internal/pkg/manifest/env.go | 12 ++++++------ internal/pkg/manifest/validate.go | 14 +++++++++++++- internal/pkg/manifest/validate_env.go | 16 ++++++++++------ internal/pkg/manifest/validate_env_test.go | 6 +++--- 4 files changed, 32 insertions(+), 16 deletions(-) diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 0724c489f44..8efc061d0fd 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -132,20 +132,20 @@ type securityGroupRule struct { // The simple form allow represents from and to ports as a single value, whereas the advanced form is for different values. type portsConfig struct { Port *int // 0 is a valid value, so we want the default value to be nil. - Ports *IntRangeBand // Mutually exclusive with port. + Range *IntRangeBand // Mutually exclusive with port. } // IsEmpty returns whether PortsConfig is empty. func (cfg *portsConfig) IsEmpty() bool { - return cfg.Port == nil && cfg.Ports == nil + return cfg.Port == nil && cfg.Range == nil } // GetPorts returns the from and to ports of a security group rule. func (r securityGroupRule) GetPorts() (from, to int, err error) { - if r.Ports.Ports == nil { + if r.Ports.Range == nil { return aws.IntValue(r.Ports.Port), aws.IntValue(r.Ports.Port), nil // a single value is provided for ports. } - return r.Ports.Ports.Parse() + return r.Ports.Range.Parse() } // UnmarshalYAML overrides the default YAML unmarshaling logic for the Ports @@ -164,11 +164,11 @@ func (cfg *portsConfig) UnmarshalYAML(value *yaml.Node) error { if cfg.Port != nil { // Successfully unmarshalled Port field and unset Ports field, return - cfg.Ports = nil + cfg.Range = nil return nil } - if err := value.Decode(&cfg.Ports); err != nil { + if err := value.Decode(&cfg.Range); err != nil { return errUnmarshalPortsConfig } return nil diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index d6bf4bf2c62..71dbb90b17b 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -1025,13 +1025,25 @@ func (r Range) validate() error { return r.Value.validate() } +type errInvalidRange struct { + value string + validFormat string +} + +func (e *errInvalidRange) Error() string { + return fmt.Sprintf("invalid range value %s: valid format is %s", e.value, e.validFormat) +} + // validate returns nil if IntRangeBand is configured correctly. func (r IntRangeBand) validate() error { str := string(r) minMax := intRangeBandRegexp.FindStringSubmatch(str) // Valid minMax example: ["1-2", "1", "2"] if len(minMax) != 3 { - return fmt.Errorf("invalid range value %s: valid format is ${min}-${max}", str) + return &errInvalidRange{ + value: str, + validFormat: "${min}-${max}", + } } // Guaranteed by intRangeBandRegexp. min, err := strconv.Atoi(minMax[1]) diff --git a/internal/pkg/manifest/validate_env.go b/internal/pkg/manifest/validate_env.go index d8ff3a0325e..1228b8e7673 100644 --- a/internal/pkg/manifest/validate_env.go +++ b/internal/pkg/manifest/validate_env.go @@ -8,7 +8,6 @@ import ( "fmt" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/arn" - "strings" ) var ( @@ -105,13 +104,18 @@ func (cfg portsConfig) validate() error { missingField: "ports", } } - if cfg.Ports != nil { - if err := cfg.Ports.validate(); err != nil { - if strings.Contains(err.Error(), "invalid range value") { - return fmt.Errorf("invalid ports value %s: valid port format is ${from_port}-${to_port}", *cfg.Ports) + if cfg.Range == nil { + return nil + } + if err := cfg.Range.validate(); err != nil { + var targetErr *errInvalidRange + if errors.As(err, &targetErr) { + return &errInvalidRange{ + value: aws.StringValue((*string)(cfg.Range)), + validFormat: "${from_port}-${to_port}", } - return err } + return err } return nil } diff --git a/internal/pkg/manifest/validate_env_test.go b/internal/pkg/manifest/validate_env_test.go index be53528ac62..bb883129c22 100644 --- a/internal/pkg/manifest/validate_env_test.go +++ b/internal/pkg/manifest/validate_env_test.go @@ -122,7 +122,7 @@ func TestEnvironmentConfig_validate(t *testing.T) { CidrIP: "0.0.0.0", IpProtocol: "tcp", Ports: portsConfig{ - Ports: (*IntRangeBand)(aws.String("1-10")), + Range: (*IntRangeBand)(aws.String("1-10")), }, }, }, @@ -141,7 +141,7 @@ func TestEnvironmentConfig_validate(t *testing.T) { CidrIP: "0.0.0.0", IpProtocol: "tcp", Ports: portsConfig{ - Ports: (*IntRangeBand)(aws.String("1-10-10")), + Range: (*IntRangeBand)(aws.String("1-10-10")), }, }, }, @@ -149,7 +149,7 @@ func TestEnvironmentConfig_validate(t *testing.T) { }, }, }, - wantedError: "validate \"security_group\": validate ingress[0]: invalid ports value 1-10-10: valid port format is ${from_port}-${to_port}", + wantedError: "validate \"security_group\": validate ingress[0]: invalid range value 1-10-10: valid format is ${from_port}-${to_port}", }, "valid security group config without ports": { in: EnvironmentConfig{ From 6ac01733635d385e426189e23ea27f4a351db8b3 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Wed, 3 Aug 2022 12:35:20 -0700 Subject: [PATCH 18/19] fix integ test --- .../template-with-custom-empty-security-group.yml | 5 ++++- .../environments/template-with-custom-security-group.yml | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-empty-security-group.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-empty-security-group.yml index d962b62ace2..89ec6090650 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-empty-security-group.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-empty-security-group.yml @@ -678,7 +678,7 @@ Resources: Action: - "states:StartExecution" Resource: - - !Sub "arn:aws:states:${AWS::Region}:${AWS::AccountId}:stateMachine:${AppName}-${EnvironmentName}-*" + - !Sub "arn:${AWS::Partition}:states:${AWS::Region}:${AWS::AccountId}:stateMachine:${AppName}-${EnvironmentName}-*" - Sid: CloudFormation Effect: Allow Action: [ @@ -1003,6 +1003,9 @@ Outputs: Description: The ID of the Copilot-managed EFS filesystem. Export: Name: !Sub ${AWS::StackName}-FilesystemID + PublicALBAccessible: + Condition: CreateALB + Value: true LastForceDeployID: Value: "" Description: Optionally force the template to update when no immediate resource change is present. \ No newline at end of file diff --git a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml index 58f1e01d403..8e37a110f93 100644 --- a/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml +++ b/internal/pkg/deploy/cloudformation/stack/testdata/environments/template-with-custom-security-group.yml @@ -703,7 +703,7 @@ Resources: Action: - "states:StartExecution" Resource: - - !Sub "arn:aws:states:${AWS::Region}:${AWS::AccountId}:stateMachine:${AppName}-${EnvironmentName}-*" + - !Sub "arn:${AWS::Partition}:states:${AWS::Region}:${AWS::AccountId}:stateMachine:${AppName}-${EnvironmentName}-*" - Sid: CloudFormation Effect: Allow Action: [ @@ -1028,6 +1028,9 @@ Outputs: Description: The ID of the Copilot-managed EFS filesystem. Export: Name: !Sub ${AWS::StackName}-FilesystemID + PublicALBAccessible: + Condition: CreateALB + Value: true LastForceDeployID: Value: "" Description: Optionally force the template to update when no immediate resource change is present. \ No newline at end of file From 0937bb5561e7dab836bb1184b695fc7cac080576 Mon Sep 17 00:00:00 2001 From: Parag Bhingre Date: Wed, 3 Aug 2022 12:46:17 -0700 Subject: [PATCH 19/19] remove redundant break --- internal/pkg/manifest/env.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/pkg/manifest/env.go b/internal/pkg/manifest/env.go index 8efc061d0fd..6c189c50d0c 100644 --- a/internal/pkg/manifest/env.go +++ b/internal/pkg/manifest/env.go @@ -156,7 +156,6 @@ func (cfg *portsConfig) UnmarshalYAML(value *yaml.Node) error { switch err.(type) { case *yaml.TypeError: cfg.Port = nil - break default: return err }