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.

Sign up for GitHub

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: Add support to provide GHA role per stage #307

Closed
wants to merge 2 commits into from
Closed

feat: Add support to provide GHA role per stage #307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69f19cf
feat: Add support to provide GHA role per stage
danieljamesscott Aug 19, 2022
5f33ae5
Improve parameter name and documentation
danieljamesscott Aug 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/pipeline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ export class GitHubWorkflow extends PipelineBase {
this.addStackProps(stacks, 'environment', options?.gitHubEnvironment);
this.addStackProps(stacks, 'capabilities', options?.stackCapabilities);
this.addStackProps(stacks, 'settings', options?.jobSettings);
this.addStackProps(stacks, 'role', options?.gitHubActionsRoleArn);

return stageDeployment;
}
Expand Down Expand Up @@ -370,6 +371,7 @@ export class GitHubWorkflow extends PipelineBase {
const cdkoutDir = options.assemblyDir;
const jobId = node.uniqueId;
const assetId = assets[0].assetId;
const stageRoleArn = this.ghaStageRoleArn();

// check if asset is docker asset and if we have docker credentials
const dockerLoginSteps: github.JobStep[] = [];
Expand Down Expand Up @@ -419,7 +421,7 @@ export class GitHubWorkflow extends PipelineBase {
name: 'Install',
run: `npm install --no-save cdk-assets${installSuffix}`,
},
...this.stepsToConfigureAws(this.useGitHubActionRole, { region: this.publishAssetsAuthRegion }),
...this.stepsToConfigureAws(this.useGitHubActionRole, { region: this.publishAssetsAuthRegion, stageRoleArn: stageRoleArn }),
...dockerLoginSteps,
publishStep,
],
Expand Down Expand Up @@ -470,6 +472,8 @@ export class GitHubWorkflow extends PipelineBase {
}
const assumeRoleArn = stack.assumeRoleArn ? resolve(stack.assumeRoleArn) : undefined;

const stageRoleArn = this.ghaStageRoleArn();

return {
id: node.uniqueId,
definition: {
Expand All @@ -486,7 +490,7 @@ export class GitHubWorkflow extends PipelineBase {
needs: this.renderDependencies(node),
runsOn: this.runner.runsOn,
steps: [
...this.stepsToConfigureAws(this.useGitHubActionRole, { region, assumeRoleArn }),
...this.stepsToConfigureAws(this.useGitHubActionRole, { region, assumeRoleArn, stageRoleArn }),
{
id: 'Deploy',
uses: 'aws-actions/aws-cloudformation-github-deploy@v1',
Expand Down Expand Up @@ -639,7 +643,16 @@ export class GitHubWorkflow extends PipelineBase {
};
}

private stepsToConfigureAws(openId: boolean, { region, assumeRoleArn }: { region: string; assumeRoleArn?: string }): github.JobStep[] {
private ghaStageRoleArn(): string {
const stageRoleArn = Object.entries(this.stackProperties)[0][1].role;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is excessively brittle... stackProperties should be accessed by stackArtifactId.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry. I really struggled with this. I was doing as you suggested in jobForDeploy():

const gitHubActionsStageRoleArn = this.stackProperties[stack.stackArtifactId]?.role

But in jobForAssetPublish(), I didn't have access to stack, because it didn't seem available to pass in here: https://github.com/cdklabs/cdk-pipelines-github/blob/main/src/pipeline.ts#L345

Any ideas for how I can solve this?

Copy link
Author

Choose a reason for hiding this comment

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

I've looked into this quite a bit more, and I can't figure out a nice way to work out which Stage corresponds to which "asset publish" step. Do you have any ideas here?

Copy link
Author

Choose a reason for hiding this comment

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

I've tried lots of different things here, but still can't figure out how to link the publish step to the correct stage in jobForAssetPublish. So I can't figure out which role should be used for the publish step.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @danieljamesscott. This could be a problem. You're right, jobForAssetPublish doesn't have a concept of Stage, because it is stage-agnostic. What's happening is that the assets for all stages are combined into the asset publishing step, which is really supposed to be abstracted from the user. That's the benefit of having a single "pipeline" role that has the correct permissions just like in aws-cdk-lib/pipelines.

Having thought this through a bit, I'm a little less groovy of the idea of natively supporting a role per stage. This looks like it will introduce more problems than it solves. If we did support this, we'd need to introduce a new property like assetPublishingRoleArn and then we're not really solving the problem you put forth -- somewhere one of your AWS accounts is going to have to own the role for asset publishing.

Ideally, we introduce some sort of escape hatching mechanism that allows users to go off the beaten path and do what they want. Let's take this conversation back to the issue and see if we can come up with a design that makes sense.

return stageRoleArn || this.gitHubActionRoleArn;
}

private stepsToConfigureAws(openId: boolean, {
region,
assumeRoleArn,
stageRoleArn,
}: { region: string; assumeRoleArn?: string; stageRoleArn?: string }): github.JobStep[] {
function getDeployRole(arn: string) {
return arn.replace('cfn-exec', 'deploy');
}
Expand All @@ -649,7 +662,7 @@ export class GitHubWorkflow extends PipelineBase {
if (openId) {
steps.push(awsCredentialStep('Authenticate Via OIDC Role', {
region,
gitHubActionRoleArn: this.gitHubActionRoleArn,
gitHubActionRoleArn: stageRoleArn,
}));

if (assumeRoleArn) {
Expand Down
12 changes: 12 additions & 0 deletions src/stage-options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,18 @@ export interface AddGitHubStageOptions extends AddStageOpts {
* Currently the only valid setting is 'if'.
*/
readonly jobSettings?: JobSettings;

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be a tad more descriptive here.

* A role that utilizes the GitHub OIDC Identity Provider in your AWS account.
* If supplied, this will be used instead of `awsCredentials`.
*
* You can create your own role in the console with the necessary trust policy
* to allow gitHub actions from your gitHub repository to assume the role, or
* you can utilize the `GitHubActionRole` construct to create a role for you.
*
* @default - GitHub repository secrets are used instead of OpenId Connect role.
*/
readonly gitHubActionsRoleArn?: string;
}

/**
Expand Down
97 changes: 97 additions & 0 deletions test/__snapshots__/stage-options.test.ts.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

61 changes: 61 additions & 0 deletions test/stage-options.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,64 @@ describe('job settings', () => {
});
});
});

describe('role settings', () => {
test('can specify gha role', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this test isn't actually testing any features implemented in this PR, is it?

Copy link
Author

Choose a reason for hiding this comment

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

No. Sorry. This was meant to be a snapshot test to confirm the GHA pipeline role. I'll improve the tests

withTemporaryDirectory((dir) => {
const pipeline = new GitHubWorkflow(app, 'Pipeline', {
workflowPath: `${dir}/.github/workflows/deploy.yml`,
synth: new ShellStep('Build', {
installCommands: ['yarn'],
commands: ['yarn build'],
}),
gitHubActionRoleArn: 'my-pipeline-role',
});

const stage = new Stage(app, 'MyStack', {
env: { account: '111111111111', region: 'us-east-1' },
});

new Stack(stage, 'MyStack');

pipeline.addStageWithGitHubOptions(stage, {
jobSettings: {
if: 'github.repository == \'github/repo\'',
},
});

app.synth();

expect(readFileSync(pipeline.workflowPath, 'utf-8')).toContain('if: github.repository == \'github/repo\'\n');
});
});

test('can specify role override settings at stage level', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need an additional test that asserts the behavior when the addStageWitHGitHubOptions is given a role, but the pipeline is not given a gitHubActionRoleArn.

The current default when no gitHubActionRoleArn is supplied is that GitHub secrets will be used instead. I suspect the behavior of this case is not fully thought through and would run into problems the way this PR is currently configured.

Copy link
Author

Choose a reason for hiding this comment

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

Yep. I will write a test for this. I'm struggling to figure out how best to do so though.

withTemporaryDirectory((dir) => {
const pipeline = new GitHubWorkflow(app, 'Pipeline', {
workflowPath: `${dir}/.github/workflows/deploy.yml`,
synth: new ShellStep('Build', {
installCommands: ['yarn'],
commands: ['yarn build'],
}),
gitHubActionRoleArn: 'my-pipeline-role',
});

const stage = new Stage(app, 'MyStack', {
env: { account: '111111111111', region: 'us-east-1' },
});

new Stack(stage, 'MyStack');

pipeline.addStageWithGitHubOptions(stage, {
jobSettings: {
if: 'github.repository == \'github/repo\'',
},
gitHubActionsRoleArn: 'my-stage-role',
});

app.synth();

expect(readFileSync(pipeline.workflowPath, 'utf-8')).toMatchSnapshot();
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer not to snapshot test this. Lets actually assert that the role in the stage is my-stage-role. Let's also add a second stage without the role property and assert that it gets my-pipeline-role.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. I couldn't see a great way to so this. Should I parse the yaml from the generated file and then run some asserts? I could do some string comparisons, but that seems brittle, especially if I'm going to test stages with and without roles.

It would be great if we could get access to the generated data structure, before it's written to a file. That would help with escape hatches, as well as allowing me to write better tests, without using the filesystem at all.

});
});
});