fix: Env manifest UX fix for http config

[paragbhingre]

This PR fix multiple problems -

1. This PR fix the env manifest fields that were organized in incorrect manner.
2. Also resolves Using "--internal-alb-allow-vpc-ingress" with "copilot env deploy" #4032
3. copilot env init with private subnets create env manifest with following field

http:
  private:

This should only be created when there is at least one parameter inside private is enabled. This PR fix this nit as well.

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

[Lou1415926]

Just making sure I'm understanding correctly (also for other reviewers' reference!)
We currently have:

http:
  public:
    security_groups:
      ingress: # This doesn't make sense because `ingress` is for one sg, not sgs
        restrict_to: # This is redundant because `ingress` implies "restrict_to".
          cdn: true
  private:
    security_groups:
      ingress: # This doesn't make sense because `ingress` is for one sg, not sgs
        from_vpc: true

After the fix, we want to have:

http:
  public:
    ingress:
      cdn: true
  private:
    ingress:
      from_vpc: true

while still accepting the old manifest.

[paragbhingre]

Just making sure I'm understanding correctly (also for other reviewers' reference!) We currently have:

http:
  public:
    security_groups:
      ingress: # This doesn't make sense because `ingress` is for one sg, not sgs
        restrict_to: # This is redundant because `ingress` implies "restrict_to".
          cdn: true
  private:
    security_groups:
      ingress: # This doesn't make sense because `ingress` is for one sg, not sgs
        from_vpc: true

After the fix, we want to have:

http:
  public:
    ingress:
      cdn: true
  private:
    ingress:
      from_vpc: true

while still accepting the old manifest.

Yes you are right @Lou1415926

[Lou1415926]

Same question here. Why do we need a pointer?

[efekarakus]

Is it because you want to differentiate between nil and &false? I feel like if this field is false then it still means CustomizeEnv is empty

[paragbhingre]

The reason why made this change because if we do not keep EnableInternalALBVPCIngress as pointer then this will always be false and in that case if customer imports vpc config with private subnet without --internal-alb-allow-vpc-ingress flag then also manifest has following config specified.

# Configure the load balancers in your environment, once created.
http:
  private:
    ingress:
      from_vpc: false

In order to get rid of this part I made changes related to pointer. Does that make sense?

But right now I have removed pointer logic and kept the existing logic as it is.

[efekarakus]

Could we just not write to the manifest if the field is false? 🤔

[paragbhingre]

I was not sure how right it is to not write that field to the manifest when it is false. But now I have made changes in the new commit, let me know if that works.

[efekarakus]

Is it because you want to differentiate between nil and &false? I feel like if this field is false then it still means CustomizeEnv is empty

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 55.69620% with 35 lines in your changes missing coverage. Please review.

Project coverage is 69.14%. Comparing base (3c4fd93) to head (99510a0).
Report is 943 commits behind head on mainline.

Files with missing lines	Patch %	Lines
internal/pkg/manifest/errors.go	7.40%	25 Missing ⚠️
internal/pkg/manifest/env.go	68.75%	4 Missing and 1 partial ⚠️
internal/pkg/manifest/validate_env.go	85.71%	2 Missing and 2 partials ⚠️
internal/pkg/config/env.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           mainline    #4068      +/-   ##
============================================
- Coverage     69.17%   69.14%   -0.03%     
============================================
  Files           248      249       +1     
  Lines         35608    35656      +48     
  Branches        264      264              
============================================
+ Hits          24631    24654      +23     
- Misses         9782     9805      +23     
- Partials       1195     1197       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[Lou1415926]

Do you think this sort of manifest UX fix is worth testing in a regression test? Like we can test a manifest that looks like:

http:
   public: 
        ingress:
             from_cdn: true

works the same before & after?

[Lou1415926]

lgtm!

[efekarakus]

Looks good, just tiny nits. Feel free to remove the label afterwards!

[Lou1415926]

!!