chore(manifest): add security_group to env manifest

[paragbhingre]

This PR fix the issue #3719

Now customers can define security group rules for EnvironmentSecurityGroup (e.g. ingress/egress) in their env manifest

Sample manifest template with security group rules is given below

network:
  vpc:
    security_group:
      ingress:
        IpProtocol: tcp
        FromPort: 80
        ToPort: 80
        CidrIp: 0.0.0.0/0
      egress:
        IpProtocol: tcp
        FromPort: 80
        ToPort: 80
        CidrIp: 0.0.0.0/0

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

[bvtujo]

couple of dumb questions but this looks great!

[bvtujo]

🚀

[huanjani]

So if the Ingress/Egress fields are empty, this will return a *template.SecurityGroupConfig object with nil values for ingress and egress?

Would it be more efficient to return earlier if IsZero, as that's the more common use case?

[huanjani]

Looks good!
Are there docs showing users how to specify their rules correctly?
Would you mind changing the example in your PR description to match the changes from your latest commit?
Feel free to remove the label when you're ready!

[huanjani]

Just a couple small comments!

[efekarakus]

Is it possible to hold on to merging this PR until after the release please 🥺

If we can we'd like to halt on env template CFN changes unless it's about the env manifest. Once released, we can merge the PR!

[iamhopaul123]

Ideally we don't put yaml.node for them because then it wholly depends on customers to write CFN template which is very similar to what addons should do. As a manifest field, it is usually with some level of abstraction (integrated logic). For example, allow_vpc_ingress and from_cdn

[bvtujo]

So does the marshaling logic depend on the fact that customers have to specify the exact same keys in the manifest vs in CF?

For example, the manifest config

network:
  vpc:
    security_group:
      ingress:
        - IpProtocol: tcp
          FromPort: 0
          ToPort: 65535
        - IpProtocol: tcp
          FromPort: 1
          ToPort: 6
      egress:
        - IpProtocol: tcp
          FromPort: 0
          ToPort: 65535`

maps to the CloudFormation:

      SecurityGroupIngress:
        - IpProtocol: tcp
          FromPort: 0
          ToPort: 65535
        - IpProtocol: tcp
          FromPort: 1
          ToPort: 6
      SecurityGroupEgress:
        - IpProtocol: tcp
          FromPort: 0
          ToPort: 65535

but this means that the customer has to write manifest keys in two separate capitalization styles, which may be confusing. Are there any other places in the manifest that we directly use CF keys versus remapping them to their snake case counterparts (protocol, from_port, and to_port)?

[paragbhingre]

To address everyone's concern (including @bvtujo , @iamhopaul123 ) about accepting security_group as yaml.Node or other concern as if we should be allowing customers to specify CFN template inside manifest, I would like to mention that as per my discussion with @efekarakus we are going to implement security_group rules with restricted fields only. That means we will be not using yaml.Node neither we allow customers to define CFN fields inside manifest. The expected input from the customer would be,

security_groups:
 ingress:
    - cidr:
      from_port:
      to_port:
      ip_protocol: 

We will target just CIDR for now which will solve some use cases such as mentioned here

[rclinard-amzn]

It looks like this doesn't pass CI, likely due to a broken merge?