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

fix(cognito-identitypool-alpha): remove RoleAttachment construct #33305

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Leo10Gama
Copy link
Member

@Leo10Gama Leo10Gama commented Feb 5, 2025

Issue # (if applicable)

Closes #23449

Reason for this change

What we had assumed was a bug from the service team had been investigated and revealed to be expected behaviour: an IdentityPool can only have a single IdentityPoolRoleAttachment attached to it. This went against our initial assumptions that were supported by previously-written code, which was that multiple of these attachments could be created. As such, to ensure clarity and prevent ambiguity, this library will be updated to abstract away the role attachment, as one is already created by default.

Description of changes

  • Deleted identitypool-role-attachment.ts, moving or deleting its contents:
    • Removed IdentityPoolRoleAttachment class
    • IdentityPoolRoleAttachment.configureRoleMappings() function has been moved inside of IdentityPool
    • Removed IIdentityPoolRoleAttachment and IdentityPoolRoleAttachmentProps interfaces
    • Moved IdentityPoolRoleMapping, RoleMatchingMatchType, and RoleMappingRule to lib/identitypool.ts
  • IdentityPool's private roleAttachmentCount attribute has been removed, as it never should have been there to begin with
  • IdentityPool.addRoleMappings() method has been removed
  • The logic for creating a default role attachment has been altered to create the L1 instead of the L2. This will trigger redeployments for all users of the IdentityPool construct.
  • Unit tests have been tweaked to reflect this new behaviour

Describe any new or updated permissions being added

N/A

Description of how you validated changes

yarn test runs and the integ test snapshot was updated via yarn integ.

Checklist

BREAKING CHANGE: The IdentityPoolRoleAttachment construct and IdentityPool.addRoleMappings() function will no longer exist. This is to disambiguate that only one role attachment can exist per Identity Pool. If you need to add role mappings, please do so when the IdentityPool is created.


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

@aws-cdk-automation aws-cdk-automation requested a review from a team February 5, 2025 21:45
@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Feb 5, 2025
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Feb 5, 2025
@Leo10Gama Leo10Gama requested a review from a team as a code owner February 5, 2025 22:39
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.92%. Comparing base (c400e0e) to head (01fb1fc).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #33305   +/-   ##
=======================================
  Coverage   80.92%   80.92%           
=======================================
  Files         236      236           
  Lines       14256    14256           
  Branches     2491     2491           
=======================================
  Hits        11537    11537           
  Misses       2434     2434           
  Partials      285      285           
Flag Coverage Δ
suite.unit 80.92% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
packages/aws-cdk 79.74% <ø> (ø)
packages/aws-cdk-lib/core 82.20% <ø> (ø)

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 6, 2025
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

It feels like there's a bunch of breaking changes here. They need to be called out in the PR description so that they show up in the changelog.

didn't see em, sorry. i think the comment should have a remediation step for users tho

@Leo10Gama Leo10Gama changed the title fix(cognito-identitypools): remove RoleAttachment construct fix(cognito-identitypools-alpha): remove RoleAttachment construct Feb 11, 2025
@Leo10Gama Leo10Gama changed the title fix(cognito-identitypools-alpha): remove RoleAttachment construct fix(cognito-identitypool-alpha): remove RoleAttachment construct Feb 11, 2025
Copy link
Contributor

@paulhcsun paulhcsun left a comment

Choose a reason for hiding this comment

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

The logic for creating a default role attachment has been altered to create the L1 instead of the L2. This will trigger redeployments for all users of the IdentityPool construct.

Can you elaborate on this decision to replace the L2 with the L1?

Overall looks good, just a few clarifying questions.

/**
* Configures role mappings for the Identity Pool Role Attachment
*/
private configureRoleMappings(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method the exact same implementation as the old one or were there any modifications made?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's directly copy-pasted from the old IdentityPoolRoleAttachment class, so all the logic is preserved.

@Leo10Gama
Copy link
Member Author

The logic for creating a default role attachment has been altered to create the L1 instead of the L2. This will trigger redeployments for all users of the IdentityPool construct.

Can you elaborate on this decision to replace the L2 with the L1?

Previously, the "role attachment" that links authenticated and unauthenticated roles to the identity pool were their own construct. The L1s exist separately, but since only one role attachment can exist for a given identity pool, the L2 is effectively useless, since we already create one by default. The IdentityPool L2 was previously creating an IdentityPoolRoleAttachment L2, but since we're removing that construct entirely, we can just create the L1 instead.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 01fb1fc
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cognito-identity-pool: Can't attach IdentityPoolRoleAttachment even if not present
4 participants