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

(aws-cdk/pipelines): allow use of custom role for CodePipeline #18167

Closed
ekeyser opened this issue Dec 24, 2021 · 6 comments · Fixed by #21299
Closed

(aws-cdk/pipelines): allow use of custom role for CodePipeline #18167

ekeyser opened this issue Dec 24, 2021 · 6 comments · Fixed by #21299
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management @aws-cdk/pipelines CDK Pipelines library effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1

Comments

@ekeyser
Copy link

ekeyser commented Dec 24, 2021

What is the problem?

When creating pipelines.CodePipeline with a synth pipelines.CodeBuild and even passing in role=role.without_policy_updates() parameter to the CodeBuildStep, the resulting Cfn template is generated with a PipelineRoleDefaultPolicy that exceeds the allowable size. According to other issues and to the docs regarding opting out of policy updates the policy is still being updated.

Reproduction Steps

install_commands = ['npm install -g aws-cdk', 'pip install -r requirements.txt', 'cdk synth']
        role = iam.Role(
            self,
            "Role",
            assumed_by=iam.CompositePrincipal(
                iam.ServicePrincipal("codepipeline.amazonaws.com"),
                iam.ServicePrincipal("codebuild.amazonaws.com"),
            ),
            description="My Custom Role"
        )

        policy_statement = iam.PolicyStatement(
            actions=[
                "s3:GetObject*",
                "s3:GetBucket*",
                "s3:List*",
                "s3:DeleteObject*",
                "s3:PutObject",
                "s3:Abort*",
            ],
            effect=iam.Effect.ALLOW,
            resources=[
                "*"
            ],
        )

        role.add_to_policy(
            policy_statement,
        )

        synth = pipelines.CodeBuildStep(
            "Synth",
            role=role.without_policy_updates(),
            commands=install_commands,
            input=pipelines.CodePipelineSource.connection(
                "blah/blahblah",
                "master",
                connection_arn=connection_arn,
            ),
        )

        pipeline = pipelines.CodePipeline(
            self,
            "Pipeline",
            synth=synth,
        )

What did you expect to happen?

the passed in role should not be updated

What actually happened?

policy for passed in role is updated to include individual assets for resources. It's also possible that the roles between the CodeBuildStep and the pipelines.CodePipeline are completely separate in which case I would expect that the pipelines.CodePipelines to allow for a role parameter but this does not appear to be the case.

CDK CLI Version

1.137.0

Framework Version

No response

Node.js Version

v14.18.1

OS

Linux/Ubuntu 20.04

Language

Python

Language Version

3.8.10

Other information

No response

@ekeyser ekeyser added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 24, 2021
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Dec 24, 2021
@ryparker ryparker added effort/small Small work item – less than a day of effort p1 needs-reproduction This issue needs reproduction. needs-triage This issue or PR still needs to be triaged. and removed needs-triage This issue or PR still needs to be triaged. needs-reproduction This issue needs reproduction. effort/small Small work item – less than a day of effort labels Dec 28, 2021
@github-actions github-actions bot added the @aws-cdk/pipelines CDK Pipelines library label Jan 3, 2022
@rix0rrr rix0rrr added effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Jan 3, 2022
@rix0rrr rix0rrr removed their assignment Jan 3, 2022
@ekeyser
Copy link
Author

ekeyser commented Jan 25, 2022

I can't tell if there is any traction on this issue. Just to be clear - I can create the policy and role myself if I need to but it doesn't appear that the role that I'm applying is being used and what seems to be broken or not implemented is the method to not use the role without_policy_updates.

@saltman424
Copy link
Contributor

This is the PR for this issue: #18293

@rix0rrr rix0rrr removed their assignment Feb 9, 2022
@rix0rrr rix0rrr changed the title (aws-cdk/pipelines): Maximum policy size of 10240 bytes exceeded (aws-cdk/pipelines): mutates role even if withoutPolicyUpdates() is passed Feb 23, 2022
@peterwoodworth
Copy link
Contributor

I'm not finding that this role is being mutated at all. The role and policy being created are being created because the pipelines.CodePipeline construct creates a codepipeline.Pipeline without passing a role to the codepipeline.Pipeline. This causes the codepipeline.Pipeline to create a role and policy to use.

Where you're passing the role here, it's ending up being used as the CodeBuild::Project service role, and isn't being modified.

We offer the option to pass in your own codepipeline.Pipeline to the pipelines.CodePipeline so that the CDK won't generate a new codepipeline.Pipeline with its own defaults. I don't think it's out of the question to allow users to pass in a role to pipelines.CodePipeline to then pass on to the generated codepipeline.Pipeline - will convert this to a feature request

@peterwoodworth peterwoodworth added feature-request A feature should be added or improved. effort/small Small work item – less than a day of effort and removed bug This issue is a bug. effort/medium Medium work item – several days of effort labels Jul 21, 2022
@peterwoodworth peterwoodworth changed the title (aws-cdk/pipelines): mutates role even if withoutPolicyUpdates() is passed (aws-cdk/pipelines): allow use of custom role for CodePipeline Jul 21, 2022
@ekeyser
Copy link
Author

ekeyser commented Jul 22, 2022

I think I follow. I'll admit I'm a little rusty on the details of what's going on and it's been a while since I opened this. From what you're saying I take it that there are two roles involved: one for creating the pipeline and one for execution of the pipeline. Is that correct? And it's the execution role that is exceeding the 10K policy size limit not the role that is a property of the pipelines.CodePipeline?

When you say that you can pass in your own codepipeline.Pipeline you mean an existing fully formed codepipeline.Pipeline, correct? If so, what would be the point of using codepipelines at that point? Am I missing something? I suppose I could pass in our existing codepipeline.Pipeline using the static method fromPipelineArn which was created from pipelines.CdkPipeline but I think at that point I'm creating a potential mess of a situation not to mention I'm not sure that's entirely possibly without impacting our existing production infrastructure.

One last bit, which is this stemmed from attempting to workaround a policy size exception. I understand that inline policies, even if there are multiple per role, are all coalesced into a single inline policy and that combined policy size is limited to 10K. My question is why is there a hard requirement for relying on inline policies? Wouldn't it be possible to create custom/customer managed policies and even multiples of these policies to then assign to whatever role is using them? That should at least allow up to 60K (120K if limit of policies / role is bumped up to 20) worth of policy size minus some policy crease loss. Again, maybe I'm missing something, in fact I'm sure I am.

@peterwoodworth
Copy link
Contributor

The role you're passing into codebuildstep is the ServiceRole which enables CodeBuild to interact with AWS services. This is just limited to CodeBuild.

The other role is the role the pipeline itself uses to either perform actions that don't have a role specified for that action, or will assume the role specified for the action

Yes, I do mean a fully existing codepipeline.Pipeline. Under the hood, the CodePipeline construct will create one itself. This could be used to migrate an existing codepipeline.Pipeline to be used with our modern api, or it could be used if you wish to create a codepipeline.Pipeline construct with passing in different props than our modern API would otherwise allow. Here's an example:

    const pipeline = new Pipeline(this, 'Pipeline', {
      crossAccountKeys: false,
      restartExecutionOnUpdate: true,
      role: new Role(this, "pipeline", {
        roleName: "pipeline",
        assumedBy: new ServicePrincipal("codepipeline.amazonaws.com")
      }).withoutPolicyUpdates()
    });
    const cdkpipeline = new CodePipeline(this, 'CodePipeline', {
      codePipeline: pipeline,

This will create a CodePipeline with a role that will not be mutated, with no additional policies. All else should be identical I believe. You will then be able to use the CodePipeline construct as normal.

For reference, here's where we generate the underlying pipeline

if (this.props.codePipeline) {
if (this.props.pipelineName) {
throw new Error('Cannot set \'pipelineName\' if an existing CodePipeline is given using \'codePipeline\'');
}
if (this.props.crossAccountKeys !== undefined) {
throw new Error('Cannot set \'crossAccountKeys\' if an existing CodePipeline is given using \'codePipeline\'');
}
this._pipeline = this.props.codePipeline;
} else {
this._pipeline = new cp.Pipeline(this, 'Pipeline', {
pipelineName: this.props.pipelineName,
crossAccountKeys: this.props.crossAccountKeys ?? false,
reuseCrossRegionSupportStacks: this.props.reuseCrossRegionSupportStacks,
// This is necessary to make self-mutation work (deployments are guaranteed
// to happen only after the builds of the latest pipeline definition).
restartExecutionOnUpdate: true,
});
}

Of course, it would be easier if you could just directly pass in this role to CodePipeline.

As for the policy question - I'm not sure personally 😅 I've seen this discussed a while ago on why we don't do managed policies for this problem, but I don't remember the details.

@mergify mergify bot closed this as completed in #21299 Aug 1, 2022
mergify bot pushed a commit that referenced this issue Aug 1, 2022
fixes: #18167, fixes #21412

- adds a new role prop for `pipelines.CodePipeline` to pass on to the generated `codepipeline.Pipeline`
- This role will be assumed by the pipeline

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Aug 1, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

imanolympic pushed a commit to imanolympic/aws-cdk that referenced this issue Aug 8, 2022
fixes: aws#18167, fixes aws#21412

- adds a new role prop for `pipelines.CodePipeline` to pass on to the generated `codepipeline.Pipeline`
- This role will be assumed by the pipeline

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

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

cr: https://code.amazon.com/reviews/CR-73598842
josephedward pushed a commit to josephedward/aws-cdk that referenced this issue Aug 30, 2022
fixes: aws#18167, fixes aws#21412

- adds a new role prop for `pipelines.CodePipeline` to pass on to the generated `codepipeline.Pipeline`
- This role will be assumed by the pipeline

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*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
@aws-cdk/aws-iam Related to AWS Identity and Access Management @aws-cdk/pipelines CDK Pipelines library effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants