chore: revert "force NLB security group" (#5551)

This reverts commit [ab6e1da](#5471)





By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

internal/pkg/cli/deploy/lbws.go

@@ -5,7 +5,6 @@ package deploy
 
 import (
 	"fmt"
-
 	"github.com/aws/copilot-cli/internal/pkg/aws/elbv2"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -15,6 +14,7 @@ import (
 	awsecs "github.com/aws/copilot-cli/internal/pkg/aws/ecs"
 	"github.com/aws/copilot-cli/internal/pkg/aws/partitions"
 	"github.com/aws/copilot-cli/internal/pkg/config"
+	"github.com/aws/copilot-cli/internal/pkg/deploy"
 	"github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation"
 	"github.com/aws/copilot-cli/internal/pkg/deploy/cloudformation/stack"
 	"github.com/aws/copilot-cli/internal/pkg/deploy/upload/customresource"
@@ -43,15 +43,21 @@ var (
 		color.HighlightCode("copilot app init --domain example.com"))
 )
 
+// TODO(Aiden): remove when NetworkLoadBalancer is forcibly updated
+type publicCIDRBlocksGetter interface {
+	PublicCIDRBlocks() ([]string, error)
+}
+
 type elbGetter interface {
 	LoadBalancer(nameOrARN string) (*elbv2.LoadBalancer, error)
 }
 
 type lbWebSvcDeployer struct {
 	*svcDeployer
-	appVersionGetter versionGetter
-	elbGetter        elbGetter
-	lbMft            *manifest.LoadBalancedWebService
+	appVersionGetter       versionGetter
+	publicCIDRBlocksGetter publicCIDRBlocksGetter
+	elbGetter              elbGetter
+	lbMft                  *manifest.LoadBalancedWebService
 
 	// Overriden in tests.
 	newAliasCertValidator func(optionalRegion *string) aliasCertValidator
@@ -70,6 +76,18 @@ func NewLBWSDeployer(in *WorkloadDeployerInput) (*lbWebSvcDeployer, error) {
 		return nil, fmt.Errorf("new app describer for application %s: %w", in.App.Name, err)
 	}
 
+	// TODO(Aiden): remove when NetworkLoadBalancer is forcibly updated
+	deployStore, err := deploy.NewStore(in.SessionProvider, svcDeployer.store)
+	if err != nil {
+		return nil, fmt.Errorf("new deploy store: %w", err)
+	}
+	envDescriber, err := describe.NewEnvDescriber(describe.NewEnvDescriberConfig{
+		App:         in.App.Name,
+		Env:         in.Env.Name,
+		ConfigStore: svcDeployer.store,
+		DeployStore: deployStore,
+	})
+
 	if err != nil {
 		return nil, fmt.Errorf("create describer for environment %s in application %s: %w", in.Env.Name, in.App.Name, err)
 	}
@@ -78,10 +96,11 @@ func NewLBWSDeployer(in *WorkloadDeployerInput) (*lbWebSvcDeployer, error) {
 		return nil, fmt.Errorf("manifest is not of type %s", manifestinfo.LoadBalancedWebServiceType)
 	}
 	return &lbWebSvcDeployer{
-		svcDeployer:      svcDeployer,
-		appVersionGetter: versionGetter,
-		elbGetter:        elbv2.New(svcDeployer.envSess),
-		lbMft:            lbMft,
+		svcDeployer:            svcDeployer,
+		appVersionGetter:       versionGetter,
+		publicCIDRBlocksGetter: envDescriber,
+		elbGetter:              elbv2.New(svcDeployer.envSess),
+		lbMft:                  lbMft,
 		newAliasCertValidator: func(optionalRegion *string) aliasCertValidator {
 			sess := svcDeployer.envSess.Copy(&aws.Config{
 				Region: optionalRegion,
@@ -143,6 +162,13 @@ func (d *lbWebSvcDeployer) stackConfiguration(in *StackRuntimeConfiguration) (*s
 		return nil, err
 	}
 	var opts []stack.LoadBalancedWebServiceOption
+	if !d.lbMft.NLBConfig.IsEmpty() {
+		cidrBlocks, err := d.publicCIDRBlocksGetter.PublicCIDRBlocks()
+		if err != nil {
+			return nil, fmt.Errorf("get public CIDR blocks information from the VPC of environment %s: %w", d.env.Name, err)
+		}
+		opts = append(opts, stack.WithNLB(cidrBlocks))
+	}
 	if d.lbMft.HTTPOrBool.ImportedALB != nil {
 		lb, err := d.elbGetter.LoadBalancer(aws.StringValue(d.lbMft.HTTPOrBool.ImportedALB))
 		if err != nil {

internal/pkg/cli/deploy/workload_test.go

@@ -50,6 +50,7 @@ type deployMocks struct {
 	mockRepositoryService      *mocks.MockrepositoryService
 	mockEndpointGetter         *mocks.MockendpointGetter
 	mockSpinner                *mocks.Mockspinner
+	mockPublicCIDRBlocksGetter *mocks.MockpublicCIDRBlocksGetter
 	mockSNSTopicsLister        *mocks.MocksnsTopicsLister
 	mockServiceDeployer        *mocks.MockserviceDeployer
 	mockServiceForceUpdater    *mocks.MockserviceForceUpdater
@@ -921,6 +922,26 @@ func TestWorkloadDeployer_DeployWorkload(t *testing.T) {
 			},
 			wantErr: fmt.Errorf("validate ALB runtime configuration for \"http\": validate aliases against the imported CDN certificate for env mockEnv: some error"),
 		},
+		"fail to get public CIDR blocks": {
+			inNLB: manifest.NetworkLoadBalancerConfiguration{
+				Listener: manifest.NetworkLoadBalancerListener{
+					Port: aws.String("443/tcp"),
+				},
+			},
+			inEnvironment: &config.Environment{
+				Name:   mockEnvName,
+				Region: "us-west-2",
+			},
+			inApp: &config.Application{
+				Name: mockAppName,
+			},
+			mock: func(m *deployMocks) {
+				m.mockEndpointGetter.EXPECT().ServiceDiscoveryEndpoint().Return("mockApp.local", nil)
+				m.mockEnvVersionGetter.EXPECT().Version().Return("v1.42.0", nil)
+				m.mockPublicCIDRBlocksGetter.EXPECT().PublicCIDRBlocks().Return(nil, errors.New("some error"))
+			},
+			wantErr: fmt.Errorf("get public CIDR blocks information from the VPC of environment mockEnv: some error"),
+		},
 		"alias used while app is not associated with a domain": {
 			inAliases: manifest.Alias{AdvancedAliases: mockAlias},
 			inEnvironment: &config.Environment{
@@ -1340,13 +1361,14 @@ func TestWorkloadDeployer_DeployWorkload(t *testing.T) {
 			defer ctrl.Finish()
 
 			m := &deployMocks{
-				mockAppVersionGetter:    mocks.NewMockversionGetter(ctrl),
-				mockEnvVersionGetter:    mocks.NewMockversionGetter(ctrl),
-				mockEndpointGetter:      mocks.NewMockendpointGetter(ctrl),
-				mockServiceDeployer:     mocks.NewMockserviceDeployer(ctrl),
-				mockServiceForceUpdater: mocks.NewMockserviceForceUpdater(ctrl),
-				mockSpinner:             mocks.NewMockspinner(ctrl),
-				mockValidator:           mocks.NewMockaliasCertValidator(ctrl),
+				mockAppVersionGetter:       mocks.NewMockversionGetter(ctrl),
+				mockEnvVersionGetter:       mocks.NewMockversionGetter(ctrl),
+				mockEndpointGetter:         mocks.NewMockendpointGetter(ctrl),
+				mockServiceDeployer:        mocks.NewMockserviceDeployer(ctrl),
+				mockServiceForceUpdater:    mocks.NewMockserviceForceUpdater(ctrl),
+				mockPublicCIDRBlocksGetter: mocks.NewMockpublicCIDRBlocksGetter(ctrl),
+				mockSpinner:                mocks.NewMockspinner(ctrl),
+				mockValidator:              mocks.NewMockaliasCertValidator(ctrl),
 			}
 			tc.mock(m)
 
@@ -1376,7 +1398,8 @@ func TestWorkloadDeployer_DeployWorkload(t *testing.T) {
 						return mockNowTime
 					},
 				},
-				appVersionGetter: m.mockAppVersionGetter,
+				appVersionGetter:       m.mockAppVersionGetter,
+				publicCIDRBlocksGetter: m.mockPublicCIDRBlocksGetter,
 				newAliasCertValidator: func(region *string) aliasCertValidator {
 					return m.mockValidator
 				},

internal/pkg/cli/interfaces.go

@@ -472,6 +472,7 @@ type statusDescriber interface {
 
 type envDescriber interface {
 	Describe() (*describe.EnvDescription, error)
+	PublicCIDRBlocks() ([]string, error)
 	Manifest() ([]byte, error)
 	ValidateCFServiceDomainAliases() error
 }

internal/pkg/cli/svc_deploy.go

@@ -350,14 +350,14 @@ After fixing the deployment, you can:
 func (o *deploySvcOpts) RecommendActions() error {
 	if lbMft, ok := o.appliedDynamicMft.Manifest().(*manifest.LoadBalancedWebService); ok {
 		if !lbMft.NLBConfig.IsEmpty() {
-			log.Warning("With v1.33.0, Copilot applies a security group to your network load balancer. ",
+			log.Warning("Starting with v1.33.0, Copilot will start applying a security group to your network load balancer. ",
 				"This allows more fine-grained intra-VPC access control: ",
 				"your service won't need to allow-list the CIDR blocks of the public subnets where the NLB is deployed; ",
 				"it only needs to allow-list the NLB, specifically.\n",
 				"\n",
 				"NLB security group onboarding implies resource recreation, ",
 				"because a security group can't be added to an existing NLB that does not already have one. ",
-				"Therefore, you may see some resource recreation related to your NLB. ",
+				"Therefore, after v1.33.0, you might see some resource recreation related to your NLB. ",
 				"This means:\n",
 				"1. If you don't use DNS aliases, then the NLB's domain name will change.\n",
 				"2. If you use DNS aliases, then the aliases will start pointing to the new NLB that is enhanced with a security group.\n",

internal/pkg/deploy/cloudformation/stack/lb_network_web_service_integration_test.go

@@ -115,7 +115,7 @@ func TestNetworkLoadBalancedWebService_Template(t *testing.T) {
 				Version:                  "v1.29.0",
 			},
 			RootUserARN: "arn:aws:iam::123456789123:root",
-		})
+		}, stack.WithNLB([]string{"10.0.0.0/24", "10.1.0.0/24"}))
 		tpl, err := serializer.Template()
 		require.NoError(t, err, "template should render")
 		regExpGUID := regexp.MustCompile(`([a-f\d]{8}-)([a-f\d]{4}-){3}([a-f\d]{12})`) // Matches random guids

internal/pkg/deploy/cloudformation/stack/lb_web_svc.go

@@ -5,11 +5,10 @@ package stack
 
 import (
 	"fmt"
+	"github.com/aws/copilot-cli/internal/pkg/aws/elbv2"
 	"strconv"
 	"strings"
 
-	"github.com/aws/copilot-cli/internal/pkg/aws/elbv2"
-
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/cloudformation"
 	"github.com/aws/copilot-cli/internal/pkg/config"
@@ -31,18 +30,27 @@ const (
 // LoadBalancedWebService represents the configuration needed to create a CloudFormation stack from a load balanced web service manifest.
 type LoadBalancedWebService struct {
 	*ecsWkld
-	manifest             *manifest.LoadBalancedWebService
-	httpsEnabled         bool
-	dnsDelegationEnabled bool
-	importedALB          *elbv2.LoadBalancer
-	appInfo              deploy.AppInformation
+	manifest               *manifest.LoadBalancedWebService
+	httpsEnabled           bool
+	dnsDelegationEnabled   bool
+	publicSubnetCIDRBlocks []string
+	importedALB            *elbv2.LoadBalancer
+	appInfo                deploy.AppInformation
 
 	parser loadBalancedWebSvcReadParser
 }
 
 // LoadBalancedWebServiceOption is used to configuring an optional field for LoadBalancedWebService.
 type LoadBalancedWebServiceOption func(s *LoadBalancedWebService)
 
+// WithNLB enables Network Load Balancer in a LoadBalancedWebService.
+// TODO(Aiden): remove when NetworkLoadBalancer is forcibly updated
+func WithNLB(cidrBlocks []string) func(s *LoadBalancedWebService) {
+	return func(s *LoadBalancedWebService) {
+		s.publicSubnetCIDRBlocks = cidrBlocks
+	}
+}
+
 // WithImportedALB specifies an imported load balancer.
 func WithImportedALB(alb *elbv2.LoadBalancer) func(s *LoadBalancedWebService) {
 	return func(s *LoadBalancedWebService) {