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(pipelines): specifying the Action Role for CodeBuild steps #18293

Merged
merged 6 commits into from May 19, 2022

Conversation

tobytipton
Copy link
Contributor

This fix should address the issue #18291

fixes #18291

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

@gitpod-io
Copy link

gitpod-io bot commented Jan 6, 2022

@github-actions github-actions bot added the @aws-cdk/pipelines CDK Pipelines library label Jan 6, 2022
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

These should be two different roles. Please add a separate configuration field.

But this actually doesn't seem like the right fix. Can we look at the Maximum policy size of 10240 bytes exceeded error a little closer ?

How many roles do we have in there? Why is the policy so large?

@tobytipton
Copy link
Contributor Author

I can change it to a buildActionRole property, if that makes better sense.

The Maximum policy size of 10240 bytes exceeded happens on the policy associated with the role which the Pipeline itself runs as because for each build role which is created the pipeline role needs to have the ability to assume that role, and because the CodeBuildAction will create a new role which means if you have to a lot of build actions to multiple stages that will cause those multiple roles to be added to the policy.

Scenario would be having a pipeline which deploys to multiple stages and each stage a code build action to do a test or change.

@saltman424
Copy link
Contributor

saltman424 commented Jan 24, 2022

@tobytipton if you are going to introduce a new property, you will need to either make that property default to using this.props.role, or else specify that property here (and possibly other places):

const script = new CodeBuildStep(node.id, {
commands,
installCommands: [
`npm install -g cdk-assets${installSuffix}`,
],
input: this._cloudAssemblyFileSet,
buildEnvironment: {
privileged: assets.some(asset => asset.assetType === AssetType.DOCKER_IMAGE),
},
role,
});

@rix0rrr As a note, it does seem that the intention, in at least the following case, is for them to be the same (note that this comment expresses the desired behavior, but is not actually what is currently happening):

/**
* This role is used by both the CodePipeline build action and related CodeBuild project. Consolidating these two
* roles into one, and re-using across all assets, saves significant size of the final synthesized output.
* Modeled after the CodePipeline role and 'CodePipelineActionRole' roles.
* Generates one role per asset type to separate file and Docker/image-based permissions.
*/

Regardless, a fix to this is desperately needed as, currently, the pipelines module can quickly become unusable (e.g. if you have a lot of Lambda functions with associated Code assets) since there does not seem to be a way to override this behavior.

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 31, 2022

Is this change still necessary after #19114 ?

@saltman424
Copy link
Contributor

@rix0rrr yes, but less often. If you have enough assets, you can still get too big of a policy document from having so many roles that the pipeline role needs to be able to assume. However, with #19114 those roles are all listed in a single policy statement rather than many, so the number of assets needed to hit the limit is now a lot larger, but I believe I hit the limit when I tried something similar to #19114 with about 150 or so assets.

@ekeyser
Copy link

ekeyser commented Mar 31, 2022

Thanks for checking. I'm planning on implementing the feature flag for #19114 today or tomorrow and then attempting to test this weekend. I'll update this ticket with my findings. I wanted to wait a few days.

@ekeyser
Copy link

ekeyser commented Apr 2, 2022

Yes, this change is still required. I deployed a stack using 1.150.0 with the iam minimize feature flag and still get errors regarding policy size exceeding 10240 bytes. Additionally, the role.without_policy_updates method appears to have in influence on what role is used for the pipeline.

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

I am actively working on #20189 #20396, which will remove the need for manual control of the action role to stay within policy sizes.

We can still decide to merge this for environments where customers aren't allowed to create IAM roles, and they'll need a mechanism to pass in their custom roles.

*
* @default - A role is automatically created
*/
readonly buildActionRole?: iam.IRole;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename this to actionRole.

@mergify mergify bot dismissed rix0rrr’s stale review May 18, 2022 18:12

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented May 19, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@rix0rrr rix0rrr changed the title fix(pipelines): use CodeBuildStep role on the CodeBuildAction fix(pipelines): specifying the Action Role for CodeBuild steps May 19, 2022
@github-actions github-actions bot added the effort/small Small work item – less than a day of effort label May 19, 2022
@mergify
Copy link
Contributor

mergify bot commented May 19, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: a9f937c
  • 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 719edfc into aws:master May 19, 2022
@mergify
Copy link
Contributor

mergify bot commented May 19, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

wphilipw pushed a commit to wphilipw/aws-cdk that referenced this pull request May 23, 2022
…8293)

This fix should address the issue aws#18291 

fixes aws#18291 
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(pipelines): role on CodeBuildStep doesn't get used on CodeBuildAction
5 participants