Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(region-info): Model region-specific information #1839

Merged
merged 19 commits into from
Mar 13, 2019

Conversation

RomainMuller
Copy link
Contributor

@RomainMuller RomainMuller commented Feb 22, 2019

Information such as:

  • is the AWS::CDK::Metadata resource supported in region?
  • what is the S3 static website endpoint?
  • how are service principals named?

Fixes #1282


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

@RomainMuller RomainMuller self-assigned this Feb 22, 2019
@RomainMuller RomainMuller added the pr/breaking-change This PR is a breaking change. It needs to be modified to be allowed in the current major version. label Feb 22, 2019
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the configuration strategy here is going to make it hard to configure in a way that's not going to make us tear our hairs out, or get it wrong in subtle ways.

I'd propose we switch to a more generic rules engine in which we can do things like (hypothetical syntax):

built_after_2014(_) = true
built_after_2014(us-east-1) = false
built_after_2014(eu-west-1) = false
// ...

s3_format(region) = if built_after_2014(region) then 's3-website.${region}.${domain}' else 's3-website-${region}.${domain}';

I would also like us to think about escape hatching -- how will users override the values we define (if we get them wrong) or missing values (if it's a region for which we don't have information yet). I'd prefer for users to supply some data files rather than them having to write code.

packages/@aws-cdk/aws-iam/lib/policy-document.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iam/lib/policy-document.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iam/lib/policy-document.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/region-settings/lib/settings/aws.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/region-settings/lib/service-principal.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/region-settings/lib/settings/aws.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/region-settings/lib/region.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/region-settings/lib/region.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iam/lib/policy-document.ts Outdated Show resolved Hide resolved
@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 22, 2019

I'm suggesting rules engine because I imagine it'll be easier to extend with data points and rules after the fact, by users.

Some well-crafted JS should be able to fulfil the same role. Not sure which one I prefer

@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 22, 2019

Some well-crafted JS should be able to fulfil the same role. Not sure which one I prefer

Here are reasons I can come up with against JS:

  • We we need the "fact database" to extensible from "outside". I.e., someone should be able to come by and say built_after_2014(some_region_youve_never_heard_of) = false. JavaScript functions will not suffice for this.

  • Having the rules in a format that we can do more with than just evaluate will give us the ability to do some static analysis on it (for example, no contradictions in the rules).

I'm trying to find an article that I've read that says something along the lines of "use TABLES not RULES", but I can't find it.

@RomainMuller
Copy link
Contributor Author

The system I implemented allows a user to register their own region (can write code to source this from files somewhere - I don't need this right now so I don't code this right now). You can also replace an existing region with your own if we messed it up (and it's simple enough to "override" the existing region if you wanted - so you only change what you mean to...

But I guess fine about the rule engine (or a table?) assuming I find it legible and not too complicated...

packages/@aws-cdk/region-settings/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/region-settings/package.json Outdated Show resolved Hide resolved
packages/@aws-cdk/region-settings/test/test.region.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iam/lib/policy-document.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iam/lib/policy-document.ts Outdated Show resolved Hide resolved
@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 25, 2019

Here are some reservations I still have:

  • We can't require the service principal to come from our data lookup table. Sometimes people should be able to specify a literal principal, and have that be used. Cases: you explicitly want to indicate a certain region's principal, and not an "arbitrary" principal or the principal for "your" region. Other common case: non-public service principals. We want internal Amazon builders to be able to use our library as well.

  • For some regions we need to be able to supply "point values" for the facts. As mentioned, in some regions the service principals don't follow a templated string at all, and I bet the same will be true for many of the other facts (s3_website_url and what else we might have).

  • I care about users being able to add regions with minimal effort, as a sort of "official workaround" so they don't have to wait around for us to do work. I want to take ourselves out of the critical path for anything as much as possible. I think I'd prefer dropping a file somewhere and setting and envvar to putting a require('./my-magic-new-region') at the top of the entrypoint file because it wouldn't require changes to the CDK app itself to support the new region, but if you are both okay with the code-based approach I will yield.

@eladb
Copy link
Contributor

eladb commented Mar 4, 2019

@RomainMuller any updates on this?

@RomainMuller
Copy link
Contributor Author

@eladb Was on-call last week so made no progress, but I will be making a brand new version of this accounting for all the conversations that were had on the subject. Expect a force-push there... in some not-so-distant future.

@eladb
Copy link
Contributor

eladb commented Mar 4, 2019

No worries. Just checking in :-)

Allows accessing region-specific information such as the name of the
partition the region is in, service principals used in-region, and other
static information that is useful when building infrastructure.
@RomainMuller RomainMuller changed the title feat(region-settings): Model region-specific information feat(region-info): Model region-specific information Mar 5, 2019
@RomainMuller
Copy link
Contributor Author

Force-pushed a new approach on there, early stages (not using the data yet). Looking to receive early feedback.

eladb
eladb previously requested changes Mar 5, 2019
packages/@aws-cdk/region-info/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/region-info/README.md Show resolved Hide resolved
packages/@aws-cdk/region-info/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/region-info/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/region-info/lib/iregion-info.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/region-info/lib/iregion-info.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/region-info/lib/region-info.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/region-info/lib/static.generated.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/region-info/lib/user-data.ts Outdated Show resolved Hide resolved
If a package's `package.json` file contains a `jest` entry (used for
configuring the behavior of `jest`), assert the package uses `jest`
instead of `nodeunit` as it's test harness (and fail if there are any
`nodeunit` tests - based on file name).

This is going to make it easier to migrate away from `nodeunit`.
@RomainMuller RomainMuller marked this pull request as ready for review March 6, 2019 13:44
@RomainMuller RomainMuller requested a review from a team as a code owner March 6, 2019 13:44
@RomainMuller RomainMuller requested a review from eladb March 7, 2019 08:43
@RomainMuller RomainMuller dismissed eladb’s stale review March 7, 2019 10:07

Code was 100% replaced.

@RomainMuller RomainMuller added @aws-cdk/aws-iam Related to AWS Identity and Access Management and removed pr/breaking-change This PR is a breaking change. It needs to be modified to be allowed in the current major version. labels Mar 11, 2019
packages/@aws-cdk/aws-iam/lib/policy-document.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/region-info/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/region-info/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/region-info/build-tools/aws-entities.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/region-info/lib/region-info-token.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/region-info/lib/region-info-token.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-iam/lib/policy-document.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support different service principals in China, GovCloud et. al.
5 participants