fix(region-info): ssm service principal is wrong in majority of regions #17984

rix0rrr · 2021-12-13T10:41:15Z

The SSM service principal format depends on the region. Older regions have a "global" service principal (ssm.amazonaws.com), while newer regions have only regional service principals (ssm.ap-east-1.amazonaws.com).

A number of things have been changed to address this:

Add the notion of a "region order" into the region-info library. This allows us to express things like "does this region predate or postdate the change of some convention", and allows us to express that certain regions are after SSM introduced this change.
For region-agnostic stacks, it is no longer possible to supply a single value for the template that will suffice in all regions, as the format itself will have changed (neither "ssm.amazonaws.com" nor "ssm.$REGION.amazonaws.com" will work in all regions). That means we must always introduce a lookup map for region-agnostic stacks. Add stack.regionalFact() to generate lookup maps from facts in case it is necessary.
- Detect if all map values are just an instantiation of a token pattern, and return the simplification if possible (e.g.: if the lookup values are service.us-east-1.amazonaws.com, service.us-east-2.amazonaws.com, etc, then simplify to service.$REGION.$URL_SUFFIX and avoid emitting a lookup).
- Simplify existing usage sites of RegionInfo.regionMap() in Lambda and CodeBuild to use the new stack.regionalFact().
Because lookup maps would always include information for all regions, including GovCloud regions, and those are only rarely necessary: add the infrastructure for users to restrict what partitions they want to include information for, by means of a context flag. Defaults to all regions if not specified (so we don't break old templates), but for new projects restricts itself to ['aws', 'aws-cn']. Set to just ['aws'] for integration tests so we don't break all of our snapshot tests.

Fixes #16188, fixes #17646.

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

gitpod-io · 2021-12-13T10:41:17Z

njlynch

Looks good, except it's not 100% clear the Lambda insights stuff is 1:1 compatible.

njlynch · 2021-12-13T11:34:18Z

packages/@aws-cdk/aws-lambda/lib/lambda-insights.ts

-   * To get around this limitation, this is the mapping structure:
-   * LambdaInsightsVersions10980 // for version 1.0.98.0
-   * - us-east-1
-   * -- {'arn': 'arn1'},
-   * - us-east-2
-   * -- {'arn': 'arn2'}
-   * LambdaInsightsVersions10890 // a separate mapping version 1.0.89.0
-   * - us-east-1
-   * -- {'arn': 'arn3'},
-   * - us-east-2
-   * -- {'arn': 'arn4'}
-   */
-
-  const mapName = DEFAULT_MAPPING_PREFIX + insightsVersion.split('.').join('');


I'm not seeing where this was replicated/replaced. If I'm reading the above comment correctly, that means the new mappings won't actually deploy.

They will deploy, the mapping name will just be different.

Ah, ok. FWIW, I checked out this branch locally and ran the aws-lambda tests, and got several failures (looks related to the rename).

packages/@aws-cdk/core/lib/stack.ts

packages/@aws-cdk/cx-api/lib/features.ts

packages/@aws-cdk/region-info/lib/aws-entities.ts

packages/@aws-cdk/region-info/lib/default.ts

Co-authored-by: Nick Lynch <[email protected]>

njlynch

One comment about a potential breaking change; everything else is peachy.

njlynch · 2021-12-14T15:14:19Z

packages/@aws-cdk/aws-codebuild/lib/linux-gpu-build-image.ts

-    this.accountExpression = account ?? core.Fn.findInMap(mappingName, core.Aws.REGION, 'repositoryAccount');
-    this.imageId = `${this.accountExpression}.dkr.ecr.${core.Aws.REGION}.${core.Aws.URL_SUFFIX}/${repositoryName}:${tag}`;
+  private constructor(private readonly repositoryName: string, private readonly tag: string, private readonly account: string | undefined) {
+    this.imageId = core.Lazy.string({


This is a potentially breaking change, no?

const myImage = codebuild.LinuxGpuBuildImage.DLC_TENSORFLOW_1_14_0; createCBJob(myImage); // ... function createCBJob(myImage: codebuild.IBindableBuildImage) { const jobId = myImage.imageId.includes('pytorch-training') ? 'pytorchjob' : 'tensorflowjob'; new codebuild.Project(this, jobId, { ... }); }

Obviously, a fabricated example, but can we really convert a string from a concrete to Lazy, tokenized value in a stable module and guarantee we won't break customers?

Sigh. I suppose not, and it's a good catch. But based on those kinds of arguments I don't think we can't make any changes at all because anybody anywhere might have taken a dependency on anything.

I want to say how big is the chance people have actually done that... and I think not too big?

mergify · 2021-12-15T11:01:54Z

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 · 2021-12-15T11:49:39Z

AWS CodeBuild CI Report

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

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

mergify · 2021-12-15T11:50:08Z

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).

@rix0rrr

The #17984 (big kudos to @rix0rrr for that) introduced a fix for the SSM service principal format which depends on the region. However, due to a typo in that PR some of regions still don't have correct SSM service principal. Currently the SSM service principal for the following regions incorrectly include region, while according to the [issue #16188](#16188) it should be only added to all regions since `ap-east-1`. ``` cn-north-1 us-iso-east-1 eu-central-1 ap-northeast-2 ap-south-1 us-east-2 ca-central-1 eu-west-2 us-isob-east-1 cn-northwest-1 eu-west-3 ap-northeast-3 us-gov-east-1 eu-north-1 ``` It works like that because by accident `RULE_SSM_PRINCIPALS_ARE_REGIONAL` has the same value as `RULE_S3_WEBSITE_REGIONAL_SUBDOMAIN`. This causes incorrect results returned by the `aws-entities/before` function. This PR fixes that issue. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

This brings in two fixes which is reflected in the snapshot changes. See: - aws/aws-cdk#17984 - aws/aws-cdk#18011 BREAKING CHANGE: Update aws-cdk libraries to 1.137.0

In #17984, mappings were altered so that non-alphanumeric values were replaced with `_`. However, the names in the name-value pairs must be fully alphanumeric according to the [docs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/mappings-section-structure.html). The result was potentially invalid mappings generated at `cdk synth` that would fail at `cdk deploy`. One such example is the mappings generated for `lambda-insights` in #18789. In this PR, the replacement value is updated from `_` to `x`. The mapping is not surfaced anywhere other than the template, so we just need a value that satisfies cloudformation. Thus we're okay with the slight loss of readability. In addition, `CfnMapping` is updated to validate the names in the name-value pair and ensure that it is alphanumeric. Fixes #18789. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…ns (aws#17984) The SSM service principal format depends on the region. Older regions have a "global" service principal (`ssm.amazonaws.com`), while newer regions have only regional service principals (`ssm.ap-east-1.amazonaws.com`). A number of things have been changed to address this: * Add the notion of a "region order" into the `region-info` library. This allows us to express things like "does this region predate or postdate the change of some convention", and allows us to express that certain regions are *after* SSM introduced this change. * For region-agnostic stacks, it is no longer possible to supply a single value for the template that will suffice in all regions, as the *format itself* will have changed (neither `"ssm.amazonaws.com"` nor `"ssm.$REGION.amazonaws.com"` will work in all regions). That means we must always introduce a lookup map for region-agnostic stacks. Add `stack.regionalFact()` to generate lookup maps from facts in case it is necessary. * Detect if all map values are just an instantiation of a token pattern, and return the simplification if possible (e.g.: if the lookup values are `service.us-east-1.amazonaws.com`, `service.us-east-2.amazonaws.com`, etc, then simplify to `service.$REGION.$URL_SUFFIX` and avoid emitting a lookup). * Simplify existing usage sites of `RegionInfo.regionMap()` in Lambda and CodeBuild to use the new `stack.regionalFact()`. * Because lookup maps would always include information for all regions, including GovCloud regions, and those are only rarely necessary: add the infrastructure for users to restrict what partitions they want to include information for, by means of a context flag. Defaults to all regions if not specified (so we don't break old templates), but for new projects restricts itself to `['aws', 'aws-cn']`. Set to just `['aws']` for integration tests so we don't break all of our snapshot tests. Fixes aws#16188, fixes aws#17646. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

@rix0rrr

The aws#17984 (big kudos to @rix0rrr for that) introduced a fix for the SSM service principal format which depends on the region. However, due to a typo in that PR some of regions still don't have correct SSM service principal. Currently the SSM service principal for the following regions incorrectly include region, while according to the [issue aws#16188](aws#16188) it should be only added to all regions since `ap-east-1`. ``` cn-north-1 us-iso-east-1 eu-central-1 ap-northeast-2 ap-south-1 us-east-2 ca-central-1 eu-west-2 us-isob-east-1 cn-northwest-1 eu-west-3 ap-northeast-3 us-gov-east-1 eu-north-1 ``` It works like that because by accident `RULE_SSM_PRINCIPALS_ARE_REGIONAL` has the same value as `RULE_S3_WEBSITE_REGIONAL_SUBDOMAIN`. This causes incorrect results returned by the `aws-entities/before` function. This PR fixes that issue. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

In aws#17984, mappings were altered so that non-alphanumeric values were replaced with `_`. However, the names in the name-value pairs must be fully alphanumeric according to the [docs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/mappings-section-structure.html). The result was potentially invalid mappings generated at `cdk synth` that would fail at `cdk deploy`. One such example is the mappings generated for `lambda-insights` in aws#18789. In this PR, the replacement value is updated from `_` to `x`. The mapping is not surfaced anywhere other than the template, so we just need a value that satisfies cloudformation. Thus we're okay with the slight loss of readability. In addition, `CfnMapping` is updated to validate the names in the name-value pair and ensure that it is alphanumeric. Fixes aws#18789. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

rix0rrr added 2 commits December 13, 2021 10:38

fix(region-info): ssm service principal format depends on region

dd6ad07

Add flag to control regions that are rendered

2d67d2e

rix0rrr requested review from njlynch and a team December 13, 2021 10:41

rix0rrr self-assigned this Dec 13, 2021

rix0rrr changed the title ~~fix(region-info): ssm service principal is wrong~~ fix(region-info): ssm service principal is wrong in some regions Dec 13, 2021

github-actions bot assigned skinny85 Dec 13, 2021

github-actions bot added the @aws-cdk/region-info Related to AWS Region information label Dec 13, 2021

rix0rrr changed the title ~~fix(region-info): ssm service principal is wrong in some regions~~ fix(region-info): ssm service principal is wrong in old regions Dec 13, 2021

mergify bot added the contribution/core This is a PR that came from AWS. label Dec 13, 2021

rix0rrr changed the title ~~fix(region-info): ssm service principal is wrong in old regions~~ fix(region-info): ssm service principal is wrong in majority of regions Dec 13, 2021

Fix some tests

d755327

njlynch suggested changes Dec 13, 2021

View reviewed changes

rix0rrr and others added 9 commits December 13, 2021 13:43

Update packages/@aws-cdk/region-info/lib/default.ts

f572278

Co-authored-by: Nick Lynch <[email protected]>

Update packages/@aws-cdk/region-info/lib/aws-entities.ts

7549390

Co-authored-by: Nick Lynch <[email protected]>

Update packages/@aws-cdk/cx-api/lib/features.ts

45fba99

Co-authored-by: Nick Lynch <[email protected]>

Rename regions -> partitions

8042efe

Try to match patterns if we can

69f2ed4

Add defaultValue for principals we don't have exceptions for

05438e6

Fix Lambda Insights tests

ebb6470

Do the same in CodeBuild

0273b0b

Fix tests for CLI

2d3e5c2

rix0rrr requested a review from njlynch December 13, 2021 14:19

rix0rrr added 4 commits December 13, 2021 16:25

add tests for regionalFact

2d6b3e6

Mark test as testing a deprecated API

962cb22

Update snapshot

d2f1cfb

Merge branch 'master' into huijbers/ssm-service-principal

ca77a56

skinny85 removed their assignment Dec 13, 2021

Merge branch 'master' into huijbers/ssm-service-principal

5b5b68f

rix0rrr added 3 commits December 14, 2021 14:17

Forgot snapshot

9a6be58

Fix CodeBuild

7927e49

Fix test

3653490

rix0rrr requested a review from a team December 14, 2021 15:19

njlynch suggested changes Dec 14, 2021

View reviewed changes

rix0rrr requested review from njlynch and a team December 14, 2021 16:09

Put silly old behavior back

25803e4

njlynch approved these changes Dec 15, 2021

View reviewed changes

If account was concrete use that immediately in the URI

d918687

Merge branch 'master' into huijbers/ssm-service-principal

18c1b60

mergify bot merged commit 77144f5 into master Dec 15, 2021

mergify bot deleted the huijbers/ssm-service-principal branch December 15, 2021 11:50

ocher mentioned this pull request Dec 22, 2021

fix(region-info): ssm service principal - fix more regions #18135

Merged

akash1810 mentioned this pull request Dec 22, 2021

chore(deps)!: Update aws-cdk libraries to 1.137.0 guardian/cdk#997

Merged

2 tasks

kaizencc mentioned this pull request Jan 4, 2022

(aws-backup): Resolution error: stack.regionalFact is not a function #18252

Closed

kaizencc mentioned this pull request Feb 10, 2022

fix(core): undeployable due to invalid mapping #18922

Merged

Chriscbr mentioned this pull request Mar 7, 2022

(aws-iam): cdk deployment adds region to some service trust relationships #19267

Closed

joekiller mentioned this pull request Nov 18, 2022

Deployment of custom CDK resource containing AWS Step Function fails aws-amplify/amplify-cli#10306

Open

5 tasks

mrgrain mentioned this pull request Oct 23, 2023

fix(region-info): facts only returned from constant region list #27506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(region-info): ssm service principal is wrong in majority of regions #17984

fix(region-info): ssm service principal is wrong in majority of regions #17984

rix0rrr commented Dec 13, 2021 •

edited

Loading

gitpod-io bot commented Dec 13, 2021

njlynch left a comment

njlynch Dec 13, 2021

rix0rrr Dec 13, 2021

njlynch Dec 13, 2021

njlynch left a comment

njlynch Dec 14, 2021

rix0rrr Dec 14, 2021

mergify bot commented Dec 15, 2021

aws-cdk-automation commented Dec 15, 2021

mergify bot commented Dec 15, 2021

fix(region-info): ssm service principal is wrong in majority of regions #17984

fix(region-info): ssm service principal is wrong in majority of regions #17984

Conversation

rix0rrr commented Dec 13, 2021 • edited Loading

gitpod-io bot commented Dec 13, 2021

njlynch left a comment

Choose a reason for hiding this comment

njlynch Dec 13, 2021

Choose a reason for hiding this comment

rix0rrr Dec 13, 2021

Choose a reason for hiding this comment

njlynch Dec 13, 2021

Choose a reason for hiding this comment

njlynch left a comment

Choose a reason for hiding this comment

njlynch Dec 14, 2021

Choose a reason for hiding this comment

rix0rrr Dec 14, 2021

Choose a reason for hiding this comment

mergify bot commented Dec 15, 2021

aws-cdk-automation commented Dec 15, 2021

AWS CodeBuild CI Report

mergify bot commented Dec 15, 2021

rix0rrr commented Dec 13, 2021 •

edited

Loading