fix(ec2): NAT provider's default outbound rules cannot be disabled

[hedrall]

allowAllTraffic only applies to inbound traffic, but it should also apply to outbound traffic.

Deprecate it and add a new enum-based property, defaultAllowedTraffic, which also allows controlling outbound traffic rules. There is no option to allow inbound but not outbound because there is no use case for that.

Fix #12673

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[rix0rrr]

Needs a test.

Pull request has been modified.

[hedrall]

@rix0rrr
Sorry for the delay.
I have added the test and hope you will check it out.

[rix0rrr]

You know what, I'm sorry.

I did not parse this PR properly the last time I looked at it because the PR body was very sparse. I'd like a PR body to explain the problem and the solution properly so that it can be read and understood in isolation. For example, I'd like the PR body to have read:

allowAllTraffic only applies to inbound traffic, but it should also apply to outbound traffic. Change it so allowAllTraffic: false disables all outbound traffic as well

This concisely describes the proposed change, and also writing it all out makes it immediately clear that this change will be BREAKING for everyone currently using allowAllTraffic: false but not having created outbound rules (because with the current code they wouldn't have had to). It is therefore also clear that it is not safe to change the meaning of this flag.

I will agree with you that the flag is ill-named, and SHOULD do what you propose it does based on its name... but it does not currently do that and changing it is not safe.

I would therefore propose a better change is the following:

• Add new (optional) flags allowAllInbound/allowAllOutbound, defaults to true. Make these control the inbound/outbound behavior.
• Deprecate allowAllTraffic, and say it has been replaced with allowAllInbound. If it has been supplied it should be treated as if allowAllInbound has been specified.

Whether or not there should be a new flag which controls both allowAllInbound and allowAllOutbound is a different question. That's one of ergonomics, which we can tackle later.

---

Another alternative deprecation-plus-replacement I could see is something like:

defaultAllowAll: Traffic.OUTBOUND_ONLY,
defaultAllowAll: Traffic.INBOUND_AND_OUTBOUND,
defaultAllowAll: Traffic.NONE,

Or similar, so we don't have to have 2 boolean flags.

Whatever we choose, it's going to have to involve a new parameter as the change cannot be breaking existing infrastructure.

Pull request has been modified.

[hedrall]

@rix0rrr
Thank you for your review.
I'm sorry that I didn't take many things into consideration.

I tried to fix it using enum like class, but is this the right way to do the part you mentioned?

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: c168e1c
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).