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

danieljamesscott · 2022-08-19T15:32:45Z

Fixes #302.

kaizencc

Hi @danieljamesscott! Thanks for opening this PR! I'm not opposed to adding this feature but I think there are some things to think through before we can merge. I'm most worried about the behavior when gitHubActionRoleArn is not supplied. Today that means we will search for GitHub Secrets to authenticate. I don't see that behavior changed in the PR so I can only assume it will still do that. How will we be able to guess whether the user means to send their roles per stage instead of using github secrets?

kaizencc · 2022-08-22T16:52:06Z

test/stage-options.test.ts

@@ -197,3 +197,64 @@ describe('job settings', () => {
    });
  });
 });
+
+describe('role settings', () => {
+  test('can specify gha role', () => {


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

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

kaizencc · 2022-08-22T16:53:05Z

test/stage-options.test.ts

+
+      app.synth();
+
+      expect(readFileSync(pipeline.workflowPath, 'utf-8')).toMatchSnapshot();


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.

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.

kaizencc · 2022-08-22T16:54:30Z

test/stage-options.test.ts

+    });
+  });
+
+  test('can specify role override settings at stage level', () => {


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.

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

kaizencc · 2022-08-22T16:54:52Z

src/stage-options.ts

@@ -37,6 +37,13 @@ export interface AddGitHubStageOptions extends AddStageOpts {
   * Currently the only valid setting is 'if'.
   */
  readonly jobSettings?: JobSettings;
+
+  /**


Can we be a tad more descriptive here.

kaizencc · 2022-08-22T16:55:07Z

src/stage-options.ts

+   *
+   * @default - The pipeline role
+   */
+  readonly role?: string;


This should be roleArn.

kaizencc · 2022-08-22T16:58:35Z

src/pipeline.ts

@@ -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;


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

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?

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?

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.

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.

danieljamesscott · 2022-08-23T08:36:12Z

Hi @danieljamesscott! Thanks for opening this PR! I'm not opposed to adding this feature but I think there are some things to think through before we can merge.

Great. Thanks for the reply.

I've addressed a few issues, but have a few questions around others. If you can let me know which way you'd prefer things to work, I'll make the changes. I may be missing something, but the only testing strategy I could see was to ensure that the entire snapshot matches, or that a specific substring is present in the generated file.

I'm most worried about the behavior when gitHubActionRoleArn is not supplied. Today that means we will search for GitHub Secrets to authenticate. I don't see that behavior changed in the PR so I can only assume it will still do that. How will we be able to guess whether the user means to send their roles per stage instead of using github secrets?

I was following the same logic, but with an additional "stage role arn". So it should "fall back" if not supplied. e.g.

Use stage role if supplied, if not:
Use pipeline role if supplied, if not:
Use secrets

kaizencc · 2024-03-22T14:25:24Z

I think this looks stale :). Given that its from 1.5 years ago. Would basically have to start from scratch for this, and I'm not sure we need it.

feat: Add support to provide GHA role per stage

69f19cf

danieljamesscott force-pushed the main branch from cd8efbd to 69f19cf Compare August 19, 2022 15:34

danieljamesscott changed the title ~~Add support to provide GHA role per stage~~ feat: Add support to provide GHA role per stage Aug 19, 2022

danieljamesscott force-pushed the main branch from 38a3649 to 69f19cf Compare August 19, 2022 15:53

kaizencc requested changes Aug 22, 2022

View reviewed changes

Improve parameter name and documentation

5f33ae5

kaizencc mentioned this pull request Aug 29, 2022

Use separate GHA roles per stage #302

Open

kaizencc closed this Mar 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

danieljamesscott commented Aug 19, 2022

kaizencc left a comment

kaizencc Aug 22, 2022

danieljamesscott Aug 23, 2022

kaizencc Aug 22, 2022

danieljamesscott Aug 23, 2022

kaizencc Aug 22, 2022

danieljamesscott Aug 23, 2022

kaizencc Aug 22, 2022

kaizencc Aug 22, 2022

kaizencc Aug 22, 2022

danieljamesscott Aug 23, 2022

danieljamesscott Aug 24, 2022

danieljamesscott Aug 26, 2022

kaizencc Aug 29, 2022

danieljamesscott commented Aug 23, 2022

kaizencc commented Mar 22, 2024


		app.synth();

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

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

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

Conversation

danieljamesscott commented Aug 19, 2022

kaizencc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieljamesscott commented Aug 23, 2022

kaizencc commented Mar 22, 2024