feat(appconfig): add grantReadConfig method to Environment Construct #30180

mazyu36 · 2024-05-13T13:10:56Z

Issue # (if applicable)

Closes #28585

Reason for this change

To simplify policy configuration for AppConfig Environments.

Description of changes

add grantReadConfig method to Environment Construct

Description of how you validated changes

Add unit test and integ tests

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

daschaa · 2024-05-14T21:06:10Z

packages/aws-cdk-lib/aws-appconfig/lib/environment.ts

+   */
+  grant(grantee: iam.IGrantable, ...actions: string[]): iam.Grant;
+
+  /** TODO


I guess this TODO can be removed?

@daschaa
Oops...sorry, I removed it.

daschaa · 2024-05-15T05:11:54Z

packages/aws-cdk-lib/aws-appconfig/README.md

@@ -78,6 +78,24 @@ new appconfig.Environment(this, 'MyEnvironment', {
 Environment monitors also support L1 `CfnEnvironment.MonitorsProperty` constructs through the `fromCfnMonitorsProperty` method.
 However, this is not the recommended approach for CloudWatch alarms because a role will not be auto-generated if not provided.

+See [About the AWS AppConfig data plane service](https://docs.aws.amazon.com/appconfig/latest/userguide/about-data-plane.html) more information.


*for more information

@daschaa
Thank you for your review.
I added for.

daschaa · 2024-05-15T05:14:42Z

packages/aws-cdk-lib/aws-appconfig/README.md

+
+### Permissions
+
+You can grant read configuration permission on an Environment to any Principal as follows:


This sounds a bit raw to me. Maybe: You can grant permissions to read a configuration to an Environment with the grantReadConfig method as follows:

What do you think?

@daschaa
I think your suggestion is better.
I edited it.

daschaa · 2024-05-15T05:18:35Z

packages/aws-cdk-lib/aws-appconfig/lib/environment.ts

+
+  /**
+   * Grants read configuration permissions for this environment
+   * to an IAM principal (Role/Group/User).


Is the Grantable interface === IAM principal? Or is it even broader?
Are other docstrings for grant methods also describing this in this way?

@daschaa
From my understanding, the Grantable interface === IAM principal.
I have revised the explanation referring to the documentation of the grant method of the DynamoDB Table v2 Construct.

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_dynamodb.TableV2.html#grantwbrreadwbrwritewbrdatagrantee

@mazyu36 Awesome! Thank you very much for the explanation!

daschaa · 2024-05-15T06:36:48Z

packages/aws-cdk-lib/aws-appconfig/lib/environment.ts

+   * Adds an IAM policy statement associated with this environment to an IAM principal's policy.
+   *
+   * @param grantee the principal (no-op if undefined)
+   * @param actions the set of actions to allow (i.e., 'appconfig:GetLatestConfiguration', 'appconfig:StartConfigurationSession', etc.)


I think the comma after i.e. can be removed, right? 🤔

@daschaa
I'm adding a comma (,) to align with the descriptions for DynamoDB.

aws-cdk/packages/aws-cdk-lib/aws-dynamodb/lib/table-v2-base.ts

Line 69 in 4b6dc8c

* @param actions the set of actions to allow (i.e., 'dynamodb:PutItem', 'dynamodb:GetItem', etc.)

daschaa · 2024-05-15T06:45:44Z

@mazyu36 Thank you very much for the pull requests and sorry for the back and forth with the docstrings.

GavinZZ · 2024-05-27T21:24:31Z

packages/aws-cdk-lib/aws-appconfig/lib/environment.ts

+    return iam.Grant.addToPrincipal({
+      grantee,
+      actions,
+      resourceArns: [`${this.environmentArn}/*`],


I'm not too familiar with appconfig usage. Any document supporting that this is the correct arns and least privilege?

Thank you for your comment.

Upon reviewing it again, I found that the scope of the resourcesArn for grantReadConfig was too broad, so I made a modification (added /configuration/).

In AppConfig, a Configuration Profile represents a parameter (e.g., a Feature Flag), and an Environment represents a logical group of deployed Configuration Profiles. (docs)

The ARN of a Configuration belonging to an Environment takes the following format. (docs)

arn:${Partition}:appconfig:${Region}:${Account}:application/${ApplicationId}/environment/${EnvironmentId}/configuration/${ConfigurationProfileId}

For this issue, I wanted to grant read permission for the parameters (Configurations) belonging to a logical group (Environment), so I needed to use a wildcard for /configuration/ and beyond, as I realized.

Previously, the wildcard was applied from /${EnvironmentId}/ onward, which was too broad in scope.

Regarding the grant method, I changed it to limit the resources to the Environment only.

GavinZZ

LGTM in general, just one question regarding permission.

packages/aws-cdk-lib/aws-appconfig/lib/environment.ts

GavinZZ · 2024-05-27T21:27:45Z

packages/aws-cdk-lib/aws-appconfig/README.md

@@ -3,13 +3,13 @@

 This module is part of the [AWS Cloud Development Kit](https://github.com/aws/aws-cdk) project.

-For a high level overview of what AWS AppConfig is and how it works, please take a look here: 


Nice, thanks for removing the whitespaces.

Co-authored-by: GZ <hz351086153@gmail.com>

aws-cdk-automation · 2024-05-28T08:01:19Z

AWS CodeBuild CI Report

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

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

mazyu36 · 2024-05-28T08:21:37Z

@GavinZZ
Thank you for your review!
I've addressed all your comments.

GavinZZ

LGTM, thanks for contributing!

mergify · 2024-05-29T01:15:13Z

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

feat: add grantReadConfig method

5a0fdd0

aws-cdk-automation requested a review from a team May 13, 2024 13:11

github-actions bot added repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels May 13, 2024

mazyu36 added 2 commits May 13, 2024 22:19

Merge branch 'main' into appcpnfig-grant-read-28585

2584627

fix: update README

12e900a

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

daschaa reviewed May 14, 2024

View reviewed changes

fix: remove TODO

990adb8

daschaa reviewed May 15, 2024

View reviewed changes

fix: document

490b730

daschaa reviewed May 15, 2024

View reviewed changes

daschaa approved these changes May 15, 2024

View reviewed changes

aws-cdk-automation 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 May 15, 2024

github-actions bot mentioned this pull request May 20, 2024

Weekly PR metrics report #30274

Closed

GavinZZ reviewed May 27, 2024

View reviewed changes

mazyu36 and others added 2 commits May 28, 2024 12:45

Update packages/aws-cdk-lib/aws-appconfig/lib/environment.ts

4e9322a

Co-authored-by: GZ <hz351086153@gmail.com>

fix: permission changed

1e2ae69

GavinZZ approved these changes May 29, 2024

View reviewed changes

Merge branch 'main' into appcpnfig-grant-read-28585

0d2dbfc

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appconfig): add grantReadConfig method to Environment Construct #30180

feat(appconfig): add grantReadConfig method to Environment Construct #30180

mazyu36 commented May 13, 2024

daschaa May 14, 2024

mazyu36 May 15, 2024

daschaa May 15, 2024

mazyu36 May 15, 2024

daschaa May 15, 2024

mazyu36 May 15, 2024

daschaa May 15, 2024

mazyu36 May 15, 2024

daschaa May 15, 2024

daschaa May 15, 2024

mazyu36 May 15, 2024

daschaa commented May 15, 2024

GavinZZ May 27, 2024

mazyu36 May 28, 2024

GavinZZ left a comment

GavinZZ May 27, 2024

aws-cdk-automation commented May 28, 2024

mazyu36 commented May 28, 2024

GavinZZ left a comment

mergify bot commented May 29, 2024


		### Permissions

		You can grant read configuration permission on an Environment to any Principal as follows:

		@@ -3,13 +3,13 @@

		This module is part of the [AWS Cloud Development Kit](https://github.com/aws/aws-cdk) project.

		For a high level overview of what AWS AppConfig is and how it works, please take a look here:

feat(appconfig): add grantReadConfig method to Environment Construct #30180

Are you sure you want to change the base?

feat(appconfig): add grantReadConfig method to Environment Construct #30180

Conversation

mazyu36 commented May 13, 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

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

daschaa commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GavinZZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented May 28, 2024

AWS CodeBuild CI Report

mazyu36 commented May 28, 2024

GavinZZ left a comment

Choose a reason for hiding this comment

mergify bot commented May 29, 2024