chore: allow cloud front prefix list to restrict security group access via environment manifest

[CaptainCarpensir]

Allow users to specify prefix lists, and to restrict ingress traffic to the ALB via the environment manifest. Adds a new field to the PublicHTTPConfig field restrict_alb_ingress_to_cf for this behavior.

Feature for part of #3701

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

[CaptainCarpensir]

I realize that I changed a field name that we just released to make it more clear in the most recent commit to allow_from_vpc rather than just from_vpc. Unfortuantely I don't think we can push this since it's a breaking change, but I also regret not recognizing that I don't think the name from_vpc is clear enough, especially under the context of adding it to a struct which has both restricting and allowing fields.

[rclinard-amzn]

"ingress from vpc" reads naturally to me and does imply the "allow" part, so it might be a fine name by itself. How about instead moving away from the "restrict" terminology? only_from_cdn might be a more clear name and could avoid the ambiguity, and would read similarly naturally as well - "(allow) ingress only from cdn".

[rclinard-amzn]

Per discussion in-person between myself, Austin, and Aiden - we think that modeling this as the following might work better:

http:
  public:
    security_group:
      ingress:
        restrict_to: [cdn]

This would accommodate adding additional prefix lists (or exposing prefix lists to the user) in a way that wouldn't generate ambiguity as would happen if we ended up with two fields along the lines of only_from_..., and without changing the current default behavior of the public ingress field of allowing all inbound Internet traffic by default.

[Lou1415926]

My concern is that it may delete the old security group and create a new one. If we run copilot env deploy on an environment that still has PublicLoadBalancerSecurityGroup and not PublicHTTPLoadBalancerSecurityGroup yet (i.e. before your PR change), we'd probably see the following activities in Event tab:

PublicLoadBalancerSecurityGroup DELETE_COMPLETE
PublicLoadBalancerSecurityGroup DELETE_IN_PROGRESS
...
<stack_name> UPDATE_COMPLETE_CLEANUP_IN_PROGRESS
...
PublicHTTPLoadBalancerSecurityGroup CREATE_COMPLETE
PublicHTTPLoadBalancerSecurityGroup CREATE_IN_PROGRESS
...
<stack_name> UPDATE_IN_PROGRESS

Although it's probably not a problem to recreate the security group 🤔 but we should be careful about it if it introduces unnecessary resource updates just because the logical ID changes.

[Lou1415926]

qq: I see we are trying to add this ingress only if the ALB is listening for HTTPS on 443. This makes sense. Any other reasons why we are doing this now?

[CaptainCarpensir]

Splitting the security group is needed because one security group has a max count of prefix lists of 60, but each cloud front prefix list counts for 55 of those, so in order to still only allow http and https traffic, we need to split them into two different SGs.

[iamhopaul123]

To add on @CaptainCarpensir. One other benefit is that we don't always open HTTPS 443 for users anymore if they don't enable https.

[efekarakus]

Similarly, I'm also still worried because I don't have a clear understanding of the sequence of steps that occur.

After testing, yeah this does succeed at least in a basic circumstance.

@CaptainCarpensir what did you observe on an env existing stack with a LBWS already enabled?
Does it:

1. Create the new PublicHTTPLoadBalancerSecurityGroup
2. Update PublicLoadBalancer
3. Delete PublicLoadBalancerSecurityGroup

Is that the sequence? If it's the other way around (what Wanxian wrote) then the users will experience downtime because the load balancer for a brief period won't accept any incoming traffic.

[efekarakus]

nit:

Suggested change

	var cidrPrefixListIDs []string
	// Check if ingress is allowed from cloudfront
	if in.Manifest == nil || !in.Manifest.IsIngressRestrictedToCDN() {
	return nil, nil
	}
	cfManagedPrefixListID, err := d.cfManagedPrefixListID()
	if err != nil {
	return nil, err
	}
	cidrPrefixListIDs = append(cidrPrefixListIDs, cfManagedPrefixListID)
	return cidrPrefixListIDs, nil
	// Check if ingress is allowed from cloudfront
	if in.Manifest == nil || !in.Manifest.IsIngressRestrictedToCDN() {
	return nil, nil
	}
	id, err := d.cfManagedPrefixListID()
	if err != nil {
	return nil, err
	}
	return []string{id}, nil