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

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

Merged
merged 8 commits into from
May 29, 2024

Conversation

mazyu36
Copy link
Contributor

@mazyu36 mazyu36 commented May 13, 2024

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


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 May 13, 2024 13:11
@github-actions 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
@aws-cdk-automation 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
*/
grant(grantee: iam.IGrantable, ...actions: string[]): iam.Grant;

/** TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this TODO can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

*for more information

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


### Permissions

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

* 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.)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

@daschaa
Copy link
Contributor

daschaa commented May 15, 2024

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

@aws-cdk-automation 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
return iam.Grant.addToPrincipal({
grantee,
actions,
resourceArns: [`${this.environmentArn}/*`],
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

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

LGTM in general, just one question regarding permission.

packages/aws-cdk-lib/aws-appconfig/lib/environment.ts Outdated Show resolved Hide resolved
@@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks for removing the whitespaces.

@mazyu36
Copy link
Contributor Author

mazyu36 commented May 28, 2024

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

Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing!

Copy link
Contributor

mergify bot commented May 29, 2024

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 aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label May 29, 2024
@aws-cdk-automation
Copy link
Collaborator

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 mergify bot merged commit c8f2646 into aws:main May 29, 2024
12 checks passed
Copy link
Contributor

mergify bot commented May 29, 2024

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

@mazyu36 mazyu36 deleted the appcpnfig-grant-read-28585 branch May 29, 2024 01:46
atanaspam pushed a commit to atanaspam/aws-cdk that referenced this pull request Jun 3, 2024
…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*
vdahlberg pushed a commit to vdahlberg/aws-cdk that referenced this pull request Jun 10, 2024
…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
Copy link
Collaborator

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.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(appconfig-alpha): grant read Configuration
4 participants