CloudSnorkel / cdk-github-runners

CDK constructs for self-hosted GitHub Actions runners
https://constructs.dev/packages/@cloudsnorkel/cdk-github-runners/
Apache License 2.0
255 stars 37 forks source link

feat: Enable runnergroup option for provider #500

Open hranum opened 4 months ago

hranum commented 4 months ago

First of all, appreciate all the hard work that has been put into this construct.

By default new runners are placed in the default GitHub runner group.

The option to specify a runner group for a provider upon which the construct uses when it registers the runner with GH, would be a nice addition.

A use case for this is workflow restrictions/security. I'd like some self-hosted runners to only be able to run certain workflow files, which you define in a runner group.

A temporary solution is to define these restrictions in the default runner group, and create a new runner group that don't have these restrictions that "unsafe" actions run on. It will eventually be some extra maintenance to whitelist workflows, since all providers are within the default group.

https://github.com/actions/runner/blob/7310ba0a0825d467f958ab027f0fd99918291721/src/Runner.Listener/Runner.cs#L692

kichik commented 4 months ago

For sure. I never played with groups before, but they sound useful based on your description. @pharindoko in #472 could also use this, for example.

pharindoko commented 4 months ago

For sure. I never played with groups before, but they sound useful based on your description. @pharindoko in #472 could also use this, for example.

Yeah it`s not the case yet, but this request could come up for us internally as well 👍 @hranum: I know you described already the use case but have you thought about a solution or how this could look like based on an easy example.

Do you use it for one or multiple organizations ? Github Enterprise or Github.com ? I assume we would have to create the custom runner group for each organization that uses the github app to have a completely automatic way ... what`s your opinion ?

hranum commented 4 months ago

My use case is for a single organization on github.com

An automatic approach where runner group is created sounds smart, but im my mind, after taking an attempt on implementing this, it looks like less work to use an existing group.

Let me explain the use case more in detail:

For my use case I would create a group and configure it manually, and then tell CDK to use that. It should fail to register runners if the group doesn't exist. I would not want it to create a new one with the same name because it would be wide open for all workflows.

Just a side note, when whitelisting workflows on runner groups, the API validates and checks if workflow exists, it fails if it isn't.

pharindoko commented 4 months ago

ok I`ve seen that we could get the group properties from the webhook queued event: https://docs.github.com/en/enterprise-server@3.9/webhooks/webhook-events-and-payloads?actionType=queued#workflow_job

runner_group_id integer or null Required runner_group_name string or null Required

so would it be feasible to add this as an option on provider level ? e.g.

 const myProvider = new CodeBuildRunnerProvider(this, 'codebuild runner', {
   labels: ['my-codebuild'],
   group: 'my-runner-group',
   vpc: vpc,
   securityGroups: [runnerSg],
});

because on this level it`s possible to assign iam permissions as well ... And it`s possible to inject it at runner-registration.

hranum commented 4 months ago

I think that would be great.

The webhook for a queued event for this workflow:

name: learn-github-actions
on: [push]
jobs:
  check-bats-version:
    runs-on:
      labels: [self-hosted, my-codebuild]
      group: my-runner-group

would be picked up by the following runner:

const myProvider = new CodeBuildRunnerProvider(this, 'codebuild runner', {
   labels: ['my-codebuild'],
   group: 'my-runner-group',
   vpc: vpc,
   securityGroups: [runnerSg],
}); 

I think that is smart. I was not aware of the possibility to specify runner group in the workflow template.

pharindoko commented 4 months ago

@kichik what`s your opinion - do you see any drawbacks ?

kichik commented 4 months ago

ok I`ve seen that we could get the group properties from the webhook queued event: https://docs.github.com/en/enterprise-server@3.9/webhooks/webhook-events-and-payloads?actionType=queued#workflow_job

That's a good point. We shouldn't spawn the runner unless it's going to match the job. But I'm not 100% sure we should require the workflow to specify the group. On the one hand, being explicit is good. On the other, it somewhat takes away from the automation of groups. If you want to move workflows in/out of a group, you have to update both the group and the workflow.

Do we need to support multiple groups per runner? Can we?

pharindoko commented 3 months ago

I`m absolutely happy with the default runner group provided on org level and as it is working now.

Proposal: I would add the group property as an optional string array (and support multiple groups per runner would make sense)

  1. Would mean we need to read (if available) and match the runner group name property from the webhook with the providers group array (if group property is available)
  2. Attach the corresponding runner group at startup (when the gh runner agent get`s configured.)
const myProvider = new CodeBuildRunnerProvider(this, 'codebuild runner', {
   labels: ['my-codebuild'],
   group: ['my-runner-group', 'my-second-runner-group']
   vpc: vpc,
   securityGroups: [runnerSg],
}); 
name: learn-github-actions
on: [push]
jobs:
  check-bats-version:
    runs-on:
      labels: [self-hosted, my-codebuild]
      group: my-second-runner-group
kichik commented 3 months ago

I'd accept a PR for that. Sounds like it won't break anything.

And I think that behavior will match our current labels behavior. We will only spin up a runner if it can supply all the requested labels by the job. Similarly with this, we will only spin up a runner if it can supply all the requested groups by the job.

And on top of that, the user can set up rules on the GitHub side based on the group.

Did I miss anything?