feat(appconfig): add grantReadConfig method to Environment Construct by mazyu36 · Pull Request #30180 · aws/aws-cdk

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

 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


 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>

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

aws-cdk-automation · 2024-05-29T01:44:36Z

AWS CodeBuild CI Report

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

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

mergify · 2024-05-29T01:44:54Z

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

…ws#30180) ### Issue # (if applicable) Closes aws#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 - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation · 2024-07-25T21:08:51Z

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.

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

mergify bot merged commit c8f2646 into aws:main May 29, 2024

mazyu36 deleted the appcpnfig-grant-read-28585 branch May 29, 2024 01:46

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

Monthly PR metrics report #30414

Closed

NOUIY mentioned this pull request Jun 22, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#105

Open

kazimanzurrashid mentioned this pull request Jun 29, 2024

[Snyk] Upgrade aws-cdk-lib from 2.144.0 to 2.145.0 kazimanzurrashid/aws-scheduler-go#821

Open

This was referenced Jun 29, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#106

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#107

Open

This was referenced Jul 12, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#108

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#109

Open

This was referenced Jul 19, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#110

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#111

Open

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


		### Permissions

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


		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:

Conversation

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

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!

daschaa commented May 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GavinZZ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazyu36 commented May 28, 2024

Uh oh!

GavinZZ left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented May 29, 2024

Uh oh!

aws-cdk-automation commented May 29, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented May 29, 2024

Uh oh!

aws-cdk-automation commented Jul 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants