Ensure non-empty arrays for CIDR ranges #2093
Conversation
🦋 Changeset detectedLatest commit: 5c92098 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days |
mxdvl
force-pushed
the
mxdvl/non-empty-cidr-ranges
branch
2 times, most recently
from
88063c2 to
a10dbb7
Compare
|
This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days |
list best practices before worse ones, and ensure that we recommend the right thing rather than the wrong one.
type NonEmptyArray<T> = [T, ...T[]] https://www.typescriptlang.org/play?#code/C4TwDgpgBAcg9gOwKIFsygIICcsEMRQC8UA2gM7BYCWCA5gDRQB0LF1dJAupwFA8DGiClFoRgAGQh1gACyJQAFLhz4AXLESp0IbHhABKIgD4oyvUwA2U2rL6iJ12Qq76A3FAD0HqAGU4AVyx+aBlcMigABigIKxQpYAUyQwAjf2AoYGV7KCwIAEd-KlzwgEYmRQAmAGYAFgBWfR57SWkZZwByRAh2zjcmsRabNpJOhG7GduAAdzgevqA
mxdvl
force-pushed
the
mxdvl/non-empty-cidr-ranges
branch
from
a10dbb7 to
5c92098
Compare
|
This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days |
| * | ||
| * You should, for example, use Google Auth for human access, | ||
| * or a suitable machine auth mechanism, rather treating an | ||
| * access limited to the VPN as sufficient. |
There was a problem hiding this comment.
Nice improvement @mxdvl!
This change introduces a type error to src/patterns/ec2-app/framework.ts, do you want to fix that as part of this PR? Perhaps the GuWorkerPlayApp will need to take at least one CIDR range as a parameter (assuming it's up to the caller to specify one?).
Yes, is that correct? I was wondering whether the example was correct or not and whether my assumption about CIDR ranges made sense in practice. |
|
Am I mistaken in thinking this change would force Apps to have atleast one Ingress that uses a IP based filter? I'm not sure thats behaviour that we would want, apps may want to use more sophisticated methods of limiting access such as only allowing traffic from specific security groups to access the app. |
What does this change?
Ensure that Internal and Restricted access provide at least one CIDR range.
This is technically a breaking change, as some consumers may currently provide empty arrays.
How to test
type NonEmptyArray<T> = [T, ...T[]]How can we measure success?
More type safety.
Have we considered potential risks?
Some consumers might start getting type errors.
Checklist
Footnotes
Consider whether this is something that will mean changes to projects that have already been migrated, or to the CDK CLI tool. If changes are required, consider adding a checklist here and/or linking to related PRs. ↩
If you are adding a new construct or pattern, has new documentation been added? If you are amending defaults or changing behaviour, are the existing docs still valid? ↩