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

permissions for repo runner #442

Closed pharindoko closed 7 months ago

pharindoko commented 7 months ago

I figured out that the current solution needs to have administration permissions on repository level when I configure the github app for a new repository.

This permissions is way too heavy just for registering runners in a repo and I found multiple github discussions, which propose to have the same permissions settings on repository level available as on organization level available (separate permission to register self-hosted runners).

I don`t see any progress from github side - so my question... Can we have a feature to register ephemeral runners on organization level ?

kichik commented 7 months ago

Agreed. I dislike overly permissive permissions that are not technically required. I don't want that responsibility of having them.

I think mainly I chose repository level because:

  1. It will work whether or not the user is using an organization. I don't think organization runners are supported for user repositories.
  2. Repository specific runners are far more likely to be assigned to the actual job that needed them.
  3. Simpler and easier to maintain code since we don't have to handle two types of runners (like when deleting runners on error).
  4. It might have required additional permissions on the organization level. Maybe it required something like all repos access? Not sure.

There may have been more reasons, but I can't recall them right now or find where I wrote them down.

Bottom line, I'm open to it.

pharindoko commented 7 months ago

@kichik Sounds good. So I assume I have your support for this. 🙂

I checked the code and will add some suggestions here and make a draft PR within the next few days / next 2 weeks.

pharindoko commented 7 months ago

Ok - I checked the code. First thing that needs to be changed is the manifest in the svelte file... https://github.com/CloudSnorkel/cdk-github-runners/blob/3c7ac100e47cc996f79a66b7e8a083775c554f28/setup/src/App.svelte#L15

@kichik I need your feedback to make a design decision.

2 Options how to deal with it.

Option1: Radiobutton selection

User can select the RunnerRegistrationLevel on Repository or OrganizationLevel directly in the svelte webapp. It`s obvious for the user and in the right context. Maybe later hard to distinguish which level has been chosen on github app creation event. I checked it - its possible to distinguish this when the app is created (nested property permissions will be transmitted) Default:Repository. Optional:Organization`

Sample:

const repositoryPermissions = {
    actions: "write",
    administration: "write",
    deployments: "read",
  };

  const organizationPermissions = {
    actions: "write",
    organization_self_hosted_runners: "write",
    deployments: "read",
  };

  const manifest = {
    url: "https://github.com/CloudSnorkel/cdk-github-runners",
    hook_attributes: {
      url: "INSERT_WEBHOOK_URL_HERE",
    },
    redirect_url: "INSERT_BASE_URL_HERE/complete-new-app",
    public: false,
    default_permissions: repositoryPermissions,
    default_events: ["workflow_job"],
  };

...
...

 <h3>Choose Registration Level</h3>
        <div class="px-3 py-3">
          <p>
            You can configure the github app to register the runners at the
            repository or organization level. The main difference are the
            permissions that will be assigned to the github app. If you choose
            to register the runners at the repository level, the github app will
            have full administrative access (create, delete) to the selected
            repositories. If you choose to register the runners at the
            organization level, the github app will have only "self-hosted
            github runner" to register runners in the organization.
          </p>
          <div class="form-check">
            <input
              class="form-check-input"
              type="radio"
              bind:group={manifest.default_permissions}
              value={repositoryPermissions}
              id="repository"
            />
            <label class="form-check-label" for="repo">Repository</label>
          </div>
          <div class="form-check">
            <input
              class="form-check-input"
              type="radio"
              bind:group={manifest.default_permissions}
              value={organizationPermissions}
              id="organization"
            />
            <label class="form-check-label" for="org">Organization</label>
          </div>
        </div>

Option2: GitHubRunners Property

GitHubRunners properties interface gets a new property (based on enum) to select between Repository | Organization. This will be injected into the svelte site the same kinda way as the secret arn or token. (INSERT_REGISTER_RUNNER_LEVEL_HERE) Technically the quite simpler approach. Flag will be set in SSM Parameterstore and can be injected later to the lambda functions to set on which level the runner has to be registered. Less code changes in the svelte code, but user has to make decision in advance when cdk stack is created / updated. The question is if this is the right context to decide this.

Default: Repository Optional: Organization

Sample:

<script lang="ts">
  const secret = "INSERT_SECRET_ARN_HERE";
  const token = "INSERT_TOKEN_HERE";
  const registerRunnerLevel = "INSERT_REGISTER_RUNNER_LEVEL_HERE";
 ...
 ...

  const repositoryPermissions = {
    actions: "write",
    administration: "write",
    deployments: "read",
  };

  const organizationPermissions = {
    actions: "write",
    organization_self_hosted_runners: "write",
    deployments: "read",
  };

  const manifest = {
    url: "https://github.com/CloudSnorkel/cdk-github-runners",
  ...
  ...
  ...
    default_permissions:
      // @ts-ignore
      registerRunnerLevel === "repository"
        ? repositoryPermissions
        : organizationPermissions,
    default_events: ["workflow_job"],
  };

To keep maintainability and have no breaking changes the default value should be Repository

kichik commented 7 months ago

I would prefer it in setup as that's where all GitHub configuration is currently done. Would be weird to split it up.

We can also consider just always use organization level if user is installing the app on the organization level. But maybe we should wait a bit with that one until issues come up or don't come up with this.

pharindoko commented 7 months ago

I would prefer it in setup as that's where all GitHub configuration is currently done. Would be weird to split it up.

fine

We can also consider just always use organization level if user is installing the app on the organization level. But maybe we should wait a bit with that one until issues come up or don't come up with this.

yeah, guess that`s quite easy to change if the rest works as expected. As you mention this could be a later change...

pharindoko commented 7 months ago

hmm I think I waste some time trying to debug the svelte webapp.

How do you test this in an easy manner ? Is it possible to test the cdk construct lambda functions locally using sam-cli ?

I used now the integration test for testing ... npm run integ:default:deploy but I think there must be an easier way ...

kichik commented 7 months ago

To locally test the setup page, I use npx vite in the setup folder. It won't be able to make any HTTP requests, but you can see the UI in action. If you comment out the domain post, you can also see it sending the manifest and see if it's correct. Or you can just console.log() the manifest.

It's not perfect, but for such a small "app", it was good enough for me.

pharindoko commented 7 months ago

To locally test the setup page, I use npx vite in the setup folder. It won't be able to make any HTTP requests, but you can see the UI in action. If you comment out the domain post, you can also see it sending the manifest and see if it's correct. Or you can just console.log() the manifest.

It's not perfect, but for such a small "app", it was good enough for me.

Owright, fine for me too

pharindoko commented 7 months ago

These are the tasks that I assume are required:

Secrets.ts

Setup.lambda.ts

App.svelte

Tokenretriever.lambda.ts

Determine runner registration config command for:

kichik commented 7 months ago

Org level runners will also help future "warm pool" runners. It's cheaper to maintain a warm pool of runners when it's for an entire organization rather than repo by repo.