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(iam): incorrect input for AccountPrincipal is building successfully #30559

Closed
wants to merge 32 commits into from

Conversation

duranbe
Copy link
Contributor

@duranbe duranbe commented Jun 15, 2024

Reason for this change

There is no validation and test that the AWS Account Id when creating AccountPrincipal Object. In my case I missed a digit when copy pasting an account id and the build still passed, the typo has been caught only during the code review process

Description of changes

Adding simple regex to check that AWS Id is 12 digits long & update error message

Description of how you validated changes

Existing tests are passing and added 2 of them

Checklist


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

@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Jun 15, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team June 15, 2024 21:19
@github-actions github-actions bot added the p2 label Jun 15, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Jun 15, 2024
@duranbe duranbe changed the title fix(iam) add 12-digits validation for AccountPrincipal fix(iam): add 12-digits validation for AccountPrincipal Jun 15, 2024
@duranbe
Copy link
Contributor Author

duranbe commented Jun 15, 2024

Logs are quite odd

[4/4] Building fresh packages...
error /codebuild/output/src3225815236/src/github.com/aws/aws-cdk/node_modules/@lerna/create/node_modules/nx, /codebuild/output/src3225815236/src/github.com/aws/aws-cdk/node_modules/@nx/devkit/node_modules/nx, /codebuild/output/src3225815236/src/github.com/aws/aws-cdk/node_modules/lerna/node_modules/nx: Command failed.
Exit code: 135
Command: node ./bin/post-install
Arguments: 
Directory: /codebuild/output/src3225815236/src/github.com/aws/aws-cdk/node_modules/@lerna/create/node_modules/nx
Output:
Bus error (core dumped)
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

Seems issue with yarn/nx

/**
*
* @param accountId AWS account ID (i.e. '123456789012')
*/
constructor(public readonly accountId: any) {
super(new StackDependentToken(stack => `arn:${stack.partition}:iam::${accountId}:root`).toString());
if (!cdk.Token.isUnresolved(accountId) && typeof accountId !== 'string') {
throw new Error('accountId should be of type string');
if (!cdk.Token.isUnresolved(accountId) && typeof accountId !== 'string' && !this.accountIdRegExp.test(accountId)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're at it, maybe we should check to see that all characters in the string are digits.

Copy link
Contributor Author

@duranbe duranbe Jun 24, 2024

Choose a reason for hiding this comment

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

It's covered with the regex new RegExp('^[0-9]{12}$'); as restricting only digits to 0-9 (could also be \d, but 0-9 is easier to read imo)

@TheRealAmazonKendra
Copy link
Contributor

This build failure can't possibly have anything to do with this change... I'll retrigger it.

@TheRealAmazonKendra
Copy link
Contributor

@Mergifyio update

Copy link
Contributor

mergify bot commented Jun 21, 2024

update

✅ Branch has been successfully updated

@TheRealAmazonKendra TheRealAmazonKendra added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Jun 21, 2024
@TheRealAmazonKendra
Copy link
Contributor

For fixes, the title should describe the bug, not the solution. Could you please update it?

@aws-cdk-automation aws-cdk-automation dismissed their stale review June 21, 2024 21:20

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@duranbe duranbe changed the title fix(iam): add 12-digits validation for AccountPrincipal fix(iam): Incorrect input for AccountPrincipal is not failing build/validation Jun 24, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@duranbe
Copy link
Contributor Author

duranbe commented Jun 24, 2024

Hey Kendra, thanks for the review and restarting the build

For fixes, the title should describe the bug, not the solution. Could you please update it?

Done ! Is it better with this ?

fix(iam): incorrect input for AccountPrincipal is building successfully

@duranbe duranbe changed the title fix(iam): Incorrect input for AccountPrincipal is not failing build/validation fix(iam): incorrect input for AccountPrincipal is not failing build/validation Jun 24, 2024
@duranbe duranbe changed the title fix(iam): incorrect input for AccountPrincipal is not failing build/validation fix(iam): incorrect input for AccountPrincipal is building successfully Jun 24, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review June 24, 2024 15:33

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review June 24, 2024 15:34

Pull request has been modified.

@duranbe
Copy link
Contributor Author

duranbe commented Jun 29, 2024

Seems this PR is breaking lots of existing test ... I'll look into it, if it's too complex or there is actual use case where the an AWS ID is not 12 digits (which should not be the case ?) I'll close it.

@duranbe
Copy link
Contributor Author

duranbe commented Jul 17, 2024

@TheRealAmazonKendra can you take a quick look ? PR seems big (🥲) but 99% is just updating existing test

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

1 similar comment
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

xazhao and others added 5 commits August 25, 2024 20:01
…rror (#30726)

Closes #30717.

esbuild introduced a breaking change in v0.22 which caused the build error in `aws-lambda-nodejs` module.

Pin the esbuild version to 0.21 in Dockerfile

- [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*
They have now been standardized for a few years. We did not initially remove the old mappings out of caution and because we were unsure that the changes has made it to all regions yet. It is long past that happening at this point.

Because we never removed this or marked it as deprecated, we still have a not insignificant amount of customers who believe the individual mapping is necessary and cut tickets because it is not up-to-date.

Closes #<issue number here>.

- [ ] 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*
Closes #30331.

This will improve the security group lookup functionality for importing existing security groups into a CDK stack.

I added the ability to lookup existing security groups via more filters. Filters are supported by the [DescribeSecurityGroups API](https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_DescribeSecurityGroups.html), and using these filters can be immensely useful for looking up existing security groups, especially if your account or organization follows predictable rules regarding things like security group tags.

I added unit tests similar to the ones that test the normal lookup by ID or name.

- [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

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: eb7276b
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

1 similar comment
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@duranbe duranbe closed this Sep 4, 2024
Copy link

github-actions bot commented Sep 4, 2024

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2024
@duranbe duranbe deleted the fix-account-principal branch September 4, 2024 21:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants