chore: remove use of deprecated ServicePrincipal Mapping by TheRealAmazonKendra · Pull Request #30832 · aws/aws-cdk

TheRealAmazonKendra · 2024-07-11T22:15:55Z

They have now been standardized for a few years. We did not initially remove the old mappings out of caution and because we were unsure that the changes has made it to all regions yet. It is long past that happening at this point.

Because we never removed this or marked it as deprecated, we still have a not insignificant amount of customers who believe the individual mapping is necessary and cut tickets because it is not up-to-date.

Issue # (if applicable)

Closes #.

Reason for this change

Description of changes

Description of how you validated changes

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

They have now been standardized for a few years. We did not initially remove the old mappings out of caution and because we were unsure that the changes has made it to all regions yet. It is long past that happening at this point.

comcalvi · 2024-07-12T18:42:46Z

packages/aws-cdk-lib/region-info/lib/fact.ts

  public static servicePrincipal(service: string): string {
-    return `service-principal:${service.replace(/\.amazonaws\.com(\.cn)?$/, '')}`;
+    return `${service.replace(/\.amazonaws\.com(\.cn)?$/, '')}.amazonaws.com`;


isn't this breaking? I know you've deprecated it, but you're changing the behavior of this method

This formatting was to do a lookup in the database, which resulted in returning what I have now changed it to.

comcalvi · 2024-07-12T18:44:41Z

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

  public servicePrincipal(service: string): string | undefined {
-    return Fact.find(this.name, FactName.servicePrincipal(service));
+    return `${service.replace(/\.amazonaws\.com(\.cn)?$/, '')}.amazonaws.com`;


breaking change, no?

Same as above.

scanlonp

How do the principals work for services that conditionally have the region in the principal? Are those still consistent after this change?

scanlonp · 2024-07-12T19:11:52Z

...pfunctions-tasks/test/glue/integ.glue-task.js.snapshot/aws-stepfunctions-integ.template.json

+              "Action": "sts:AssumeRole",
+              "Effect": "Allow",
+              "Principal": {
+                "Service": "glue.amazonaws.com"


Were these glue service principals simply wrong before?

Correct, this policy wasn't actually in use in the test so it wasn't caught but it was invalid altogether.

scanlonp · 2024-07-12T19:15:57Z

packages/@aws-cdk/aws-pipes-enrichments-alpha/test/__snapshots__/stepfunctions.test.ts.snap

-                  "states",
-                ],
-              },
+              "Service": "states.amazonaws.com",


I believe this service principal should have the region in it, i.e. states.us-west-1.amazonaws.com. This could be right since the principal is sans region some of the time, but looking through the regional maps we had, a majority of the regions are included in the principal.

Or at least it should not be static if it can be either. The test should be environment agnostic and be a mapping, correct?

The only context in which any region should be included is in opt in regions when the permissions are cross region. This is not that case.

comcalvi

This is great work, nothing but some minor comments.

comcalvi · 2024-07-12T19:22:21Z

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

+   * @deprecated - Use VpceEndpointService.VPC_ENDPOINT_SERVICE_NAME_PREFIX instead
   */
  public static readonly VPC_ENDPOINT_SERVICE_NAME_PREFIX = 'com.amazonaws.vpce';


You gave it a different name above, it's just DEFAULT_PREFIX now.

OOOOOPS. Missed updating this.

comcalvi · 2024-07-12T19:23:56Z

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

- *
- * Not in the list ==> default service principal mappings.
- */
-export const AWS_SERVICES: readonly string[] = [


yes! Deleting this is a win.

DELETE ALL THE THINGS

comcalvi · 2024-07-12T19:27:01Z

scripts/check-region-info-compatibility.ts

    .filter((oldFact) => !newFacts.some((newFact) => factEq(oldFact, newFact)))
    .map((fact) => ({ fact, key: `${fact[0]}:${fact[1]}` }))
+    // These aren't accessed directly and we've just updated our handling of them,
+    // not removed this functionality. The mapping is unnecessary.


Can you clarify which mapping this is referring to?

We generate it at build time. I'll clarify.

comcalvi · 2024-07-12T19:29:34Z

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

@@ -1,12 +1,15 @@
 /**
 * Provides default values for certain regional information points.
+ * @deprecated - Service principals are now globally `<SERVICE>.amazonaws.com`


What's the direct replacement class / object / functionality that users should use instead of Default?

I forgot to update this. Will change.

scanlonp

Good by me

mergify · 2024-07-12T20:22:43Z

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

aws-cdk-automation · 2024-07-12T20:51:15Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 8c08272
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2024-07-12T20:51:32Z

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

aws-cdk-automation · 2024-07-25T17:42:08Z

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

github-actions bot added the p2 label Jul 11, 2024

aws-cdk-automation requested a review from a team July 11, 2024 22:16

mergify bot added the contribution/core This is a PR that came from AWS. label Jul 11, 2024

TheRealAmazonKendra added 6 commits July 11, 2024 16:46

fix test

eb427c5

fix another snapshot

2fe3501

additional handling to avoid breaking changes

c8a6486

spaaaaaaace

797305e

snapshots

f9c05c4

false advertising, not breaking

e73cd3a

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jul 12, 2024

comcalvi suggested changes Jul 12, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jul 12, 2024

scanlonp suggested changes Jul 12, 2024

View reviewed changes

comcalvi suggested changes Jul 12, 2024

View reviewed changes

update documentation a bit for better clarity

dc0183a

scanlonp approved these changes Jul 12, 2024

View reviewed changes

comcalvi approved these changes Jul 12, 2024

View reviewed changes

Merge branch 'main' into TheRealAmazonKendra/service-principal

8c08272

mergify bot merged commit 4af3685 into main Jul 12, 2024

mergify bot deleted the TheRealAmazonKendra/service-principal branch July 12, 2024 20:51

github-actions bot mentioned this pull request Jul 15, 2024

Weekly PR metrics report #30858

Closed

aws locked as resolved and limited conversation to collaborators Jul 25, 2024

Conversation

TheRealAmazonKendra commented Jul 11, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scanlonp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comcalvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scanlonp left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jul 12, 2024

Uh oh!

aws-cdk-automation commented Jul 12, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 12, 2024

Uh oh!

aws-cdk-automation commented Jul 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants