fix(rds): fixed the IAM policy that grantConnect() generates for DatabaseInstanceReadReplica #31068

ashishdhingra · 2024-08-09T00:58:23Z

Issue # (if applicable)

Reason for this change

Calling grantConnect() on an instance of DatabaseInstanceReadReplica generates an incorrect policy that uses the full ARN of the instance instead of the instanceResourceId value. It should have created policy with correct resource format arn:aws:rds-db:region:account-id:dbuser:DbiResourceId/db-user-name per Creating and using an IAM policy for IAM database access.

Description of changes

Fixed the IAM policy that grantConnect() generates for DatabaseInstanceReadReplica. The change correctly sets the value of instanceResourceId to replica instance attrDbiResourceId. The value of instanceResourceId is used to generate IAM policy.

Description of how you validated changes

Added new unit test.
Updated existing integration test.

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

…baseInstanceReadReplica

moelasmar · 2024-08-09T19:16:31Z

packages/aws-cdk-lib/aws-rds/lib/instance.ts

@@ -1366,7 +1366,7 @@ export class DatabaseInstanceReadReplica extends DatabaseInstanceNew implements
    this.instanceIdentifier = instance.ref;
    this.dbInstanceEndpointAddress = instance.attrEndpointAddress;
    this.dbInstanceEndpointPort = instance.attrEndpointPort;
-    this.instanceResourceId = instance.attrDbInstanceArn;


I am afraid that this can be a breaking change to customers. I know that we assigned a wrong value, but may be there are some customers that use this property to get the DB Instance Arn and use it some how. So, my recommendation is to deprecate this property, and to add a new property for the instance resource id, and use it in grant method, and also check where else we use the current property.

@moelasmar Thanks for your review comment. Would creating a feature flag (example #29794) be the recommended approach instead of adding a new property? Please advise.

I think the feature flag here will not fix the problem, as the flag default value will be false (to avoid breaking customers), so the policy will still referring to the wrong replica instance id. So, I do not prefer feature flag, it add a lot of confusion on us when we write code, so we can use this property in similar cases assuming that we are using the correct property, but because the feature flag is disabled, this may cause more issues for customers.

I prefer to go with deprecation, and adding new property.

@moelasmar Thanks for your review comment. I have added a commit to:

Deprecate instanceResourceId property and add new instanceResourceIdV2 property in DatabaseInstanceReadReplica.

Added a copy of grantConnect() in DatabaseInstanceReadReplica that makes use of instanceResourceIdV2.

Please review.

Thanks,
Ashish

Thanks @ashishdhingra for resolving my comment, and sorry for late response.

I actually, did a deeper check, and I found that I missed that the property instanceResourceId is actually exist in the Interface IDatabaseInstance so, adding a new property will actually break the idea of that interface, and also deprecating this property is not a good idea.

So, I think it is better to make this change under feature flag, and to enable the feature flag by default, and the customers who want to keep the current behaviour, they can disable the flag.

I pushed some updates to this change to add the feature flag. Could you please take a look.

Looks good reviewing at high level. Needs review from the maintainer.

moelasmar

left one comment

Pull request has been modified.

…ceResourceIdV2 property in DatabaseInstanceReadReplica to use in grantConnect().

…InstanceReadReplica-GrantRead-Fix # Conflicts: # packages/@aws-cdk/cx-api/FEATURE_FLAGS.md # packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md # packages/aws-cdk-lib/cx-api/README.md # packages/aws-cdk-lib/cx-api/lib/features.ts # packages/aws-cdk-lib/cx-api/test/features.test.ts

aws-cdk-automation · 2024-09-08T06:22:06Z

AWS CodeBuild CI Report

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

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

paulhcsun

Changes look good, just a couple comments about how the feature flag is defined and the default value being true.

paulhcsun · 2024-09-11T21:52:16Z

packages/@aws-cdk/cx-api/FEATURE_FLAGS.md

@@ -73,6 +73,7 @@ Flags come in three types:
 | [@aws-cdk/custom-resources:logApiResponseDataPropertyTrueDefault](#aws-cdkcustom-resourceslogapiresponsedatapropertytruedefault) | When enabled, the custom resource used for `AwsCustomResource` will configure the `logApiResponseData` property as true by default | 2.145.0 | (fix) |
 | [@aws-cdk/aws-s3:keepNotificationInImportedBucket](#aws-cdkaws-s3keepnotificationinimportedbucket) | When enabled, Adding notifications to a bucket in the current stack will not remove notification from imported stack. | 2.155.0 | (fix) |
 | [@aws-cdk/aws-stepfunctions-tasks:useNewS3UriParametersForBedrockInvokeModelTask](#aws-cdkaws-stepfunctions-tasksusenews3uriparametersforbedrockinvokemodeltask) | When enabled, use new props for S3 URI field in task definition of state machine for bedrock invoke model. | V2NEXT | (fix) |
+| [@aws-cdk/aws-rds:setCorrectValueForDatabaseInstanceReadReplicaInstanceResourceId](#aws-rds-setCorrectValueForDatabaseInstanceReadReplicaInstanceResourceId)                 | When enabled, the value of property `instanceResourceId` in construct `DatabaseInstanceReadReplica` will be set to the correct value which is `DbiResourceId` instead of currently `DbInstanceArn` | V2NEXT | (fix) |


Suggested change

| [@aws-cdk/aws-rds:setCorrectValueForDatabaseInstanceReadReplicaInstanceResourceId](#aws-rds-setCorrectValueForDatabaseInstanceReadReplicaInstanceResourceId) | When enabled, the value of property `instanceResourceId` in construct `DatabaseInstanceReadReplica` will be set to the correct value which is `DbiResourceId` instead of currently `DbInstanceArn` | V2NEXT | (fix) |

| [@aws-cdk/aws-rds:setCorrectValueForDatabaseInstanceReadReplicaInstanceResourceId](#aws-rds-setCorrectValueForDatabaseInstanceReadReplicaInstanceResourceId) | When enabled, the value of property `instanceResourceId` in construct `DatabaseInstanceReadReplica` will be set to the correct value which is `DbiResourceId` instead of currently `DbInstanceArn` | V2NEXT | (fix) |

paulhcsun · 2024-09-11T22:41:34Z

packages/@aws-cdk/cx-api/FEATURE_FLAGS.md

+| ----- |---------|-------------|
+| (not in v1) |         |             |
+| V2NEXT | `true`  | `true`      |


The spacing on some of these seem off, was it manually generated or auto generated by running the build?

it seems while I merged the conflicts I messed up these files, let me rerun the build and see if it will fix it

paulhcsun · 2024-09-11T22:45:36Z

packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md

+| Since | Default | Recommended |
+| ----- | ----- | ----- |
+| (not in v1) |  |  |
+| V2NEXT | `true` | `true` |


Do we want this to be true by default? Would that not break customers?

my idea was which behaviour should I keep, shall I keep the wrong behaviour, or should I fix it. I chose to fix the wrong behaviour with an option to let people keep the current one. But, honestly you have a point, it is a breaking change

I will change it to false.

moelasmar · 2024-09-26T19:35:06Z

for some reason I am not able to update this PR, and push changes to @ashishdhingra forked repo, so I created this new PR #31579 that includes @ashishdhingra changes, and close this one.

github-actions · 2024-09-26T19:35:24Z

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.

fix(rds): fixed the IAM policy that grantConnect() generates for Data…

e821569

…baseInstanceReadReplica

aws-cdk-automation requested a review from a team August 9, 2024 00:58

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Aug 9, 2024

ashishdhingra force-pushed the DatabaseInstanceReadReplica-GrantRead-Fix branch from e821569 to 4b8b0d5 Compare August 9, 2024 01:00

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 9, 2024

moelasmar reviewed Aug 9, 2024

View reviewed changes

moelasmar previously requested changes Aug 9, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 9, 2024

github-actions bot mentioned this pull request Aug 12, 2024

Weekly PR metrics report #31084

Closed

amazon-codecatalyst bot force-pushed the DatabaseInstanceReadReplica-GrantRead-Fix branch from 4b8b0d5 to ad7c2de Compare August 15, 2024 23:01

ashishdhingra force-pushed the DatabaseInstanceReadReplica-GrantRead-Fix branch from ad7c2de to 113ab2a Compare August 15, 2024 23:02

ashishdhingra requested a review from moelasmar August 15, 2024 23:06

fix(rds): deprecated instanceResourceId property and added new instan…

4d0239a

…ceResourceIdV2 property in DatabaseInstanceReadReplica to use in grantConnect().

amazon-codecatalyst bot force-pushed the DatabaseInstanceReadReplica-GrantRead-Fix branch from ced2113 to 4d0239a Compare August 18, 2024 02:48

Merge branch 'main' into DatabaseInstanceReadReplica-GrantRead-Fix

4d9c05d

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 18, 2024

ashishdhingra added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Aug 21, 2024

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

github-actions bot mentioned this pull request Sep 1, 2024

Monthly PR metrics report #31279

Closed

moelasmar added 3 commits September 7, 2024 22:25

Add feature flag to fix the value of the instanceResourceId

7d63cb1

do some fixes in th FEATURE_FLAGS.md file.

16b6c21

paulhcsun self-assigned this Sep 11, 2024

paulhcsun reviewed Sep 11, 2024

View reviewed changes

moelasmar closed this Sep 26, 2024

github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rds): fixed the IAM policy that grantConnect() generates for DatabaseInstanceReadReplica #31068

fix(rds): fixed the IAM policy that grantConnect() generates for DatabaseInstanceReadReplica #31068

ashishdhingra commented Aug 9, 2024

moelasmar Aug 9, 2024

ashishdhingra Aug 12, 2024

moelasmar Aug 13, 2024

ashishdhingra Aug 15, 2024

moelasmar Sep 8, 2024

moelasmar Sep 8, 2024

ashishdhingra Sep 11, 2024

moelasmar left a comment

aws-cdk-automation commented Sep 8, 2024

paulhcsun left a comment

paulhcsun Sep 11, 2024

paulhcsun Sep 11, 2024

moelasmar Sep 12, 2024

paulhcsun Sep 11, 2024

moelasmar Sep 12, 2024

moelasmar Sep 12, 2024

moelasmar commented Sep 26, 2024

github-actions bot commented Sep 26, 2024

	\| [@aws-cdk/aws-rds:setCorrectValueForDatabaseInstanceReadReplicaInstanceResourceId](#aws-rds-setCorrectValueForDatabaseInstanceReadReplicaInstanceResourceId) \| When enabled, the value of property `instanceResourceId` in construct `DatabaseInstanceReadReplica` will be set to the correct value which is `DbiResourceId` instead of currently `DbInstanceArn` \| V2NEXT \| (fix) \|
	\| [@aws-cdk/aws-rds:setCorrectValueForDatabaseInstanceReadReplicaInstanceResourceId](#aws-rds-setCorrectValueForDatabaseInstanceReadReplicaInstanceResourceId) \| When enabled, the value of property `instanceResourceId` in construct `DatabaseInstanceReadReplica` will be set to the correct value which is `DbiResourceId` instead of currently `DbInstanceArn` \| V2NEXT \| (fix) \|

fix(rds): fixed the IAM policy that grantConnect() generates for DatabaseInstanceReadReplica #31068

fix(rds): fixed the IAM policy that grantConnect() generates for DatabaseInstanceReadReplica #31068

Conversation

ashishdhingra commented Aug 9, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moelasmar left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 8, 2024

AWS CodeBuild CI Report

paulhcsun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moelasmar commented Sep 26, 2024

github-actions bot commented Sep 26, 2024