Skip to content

feat: add optional runtimeclassname#410

Open
jsoref wants to merge 1 commit into
oauth2-proxy:mainfrom
jsoref:runtime-class-name
Open

feat: add optional runtimeclassname#410
jsoref wants to merge 1 commit into
oauth2-proxy:mainfrom
jsoref:runtime-class-name

Conversation

@jsoref
Copy link
Copy Markdown

@jsoref jsoref commented Jun 2, 2026

Description

Add runtimeClassName to enable using oauth2-proxy with gvisor (see https://gvisor.dev/docs/tutorials/kubernetes/#wordpress-deployment)

Checklist:

  • I have bumped the version in the Chart.yaml according to Semantic Versioning.
  • I have updated the documentation/CHANGELOG at the bottom of the Chart.yaml
  • I have signed off all my commits.
  • (Optional) I have updated the Chart.lock for dependency updates
  • (Optional) I have implemented helm tests for new feature flags

Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
@jsoref jsoref force-pushed the runtime-class-name branch from 7f9f6cc to 8788bc5 Compare June 2, 2026 18:56
@pierluigilenoci
Copy link
Copy Markdown
Member

Hi @jsoref, thanks for this PR! Adding runtimeClassName is a useful addition for users running gvisor or kata containers.

A few things before we can merge:

1. Version bump should be minor, not patch

runtimeClassName is a new optional field — that's new functionality, not a bug fix. Per our versioning convention, this should be a minor bump (10.7.0 instead of 10.6.1). Note that 10.7.0 is already taken by two in-flight PRs (#404, #405), so once one of those merges this will need a rebase anyway — we'll reconcile the version at merge time.

2. README description needs improvement

The current entry reads:

| `runtimeClassName` | runtimeClassName | `` .

The description is just the field name repeated, and there's a stray . at the end. Something like this would be clearer:

| `runtimeClassName` | Pod runtimeClassName — set to use an alternative container runtime (e.g. `gvisor`, `kata-containers`) | `""` |

3. Add a CI test value file

There's no chart-testing scenario exercising runtimeClassName. A small file in helm/oauth2-proxy/ci/ would prevent regressions:

# ci/runtime-class-name-values.yaml
runtimeClassName: gvisor

Other than these, the implementation looks correct — the field is properly placed inside spec.template.spec, the {{- if }} guard is correct, and values.yaml defaults to "".

Let me know if you have questions!

@pierluigilenoci pierluigilenoci requested a review from tuunit June 5, 2026 07:32
@jsoref
Copy link
Copy Markdown
Author

jsoref commented Jun 5, 2026

A general comment about version bumping, we internally set up a workflow to deal with it instead of requiring each dev to guess the next available version. It lets us merge things much faster. Devs just declare the kind of version bump they want with a file in a certain directory and it is used to generate the commit for the version bump (different directories for different kinds of bumps).

That same thing could also manage the changelog bit which will always conflict here.

Anyway, I'll see if I can find time today or Monday for this. But I can't make guarantees.

@pierluigilenoci
Copy link
Copy Markdown
Member

Thanks for the context! That workflow pattern sounds really interesting — would you mind sharing which tool/action you use for it? We have exactly this problem with version bump conflicts between concurrent PRs (e.g. #404, #405, #410 all targeting 10.7.0), so something that handles it automatically would be a big improvement for us.

No rush on the changes — take your time!

@jsoref
Copy link
Copy Markdown
Author

jsoref commented Jun 5, 2026

The two actions that work together are more or less:
https://github.com/GarnerCorp/build-actions/tree/main/bump-version
https://github.com/GarnerCorp/build-actions/tree/main/next-version

The code more or less works like this:

on:
  push:
    branches:
      - main*
  pull_request:

permissions:
  contents: read

jobs:
  ci:
    if: github.event_name == 'pull_request' || !contains(github.actor, 'something')
    runs-on: ubuntu-latest
    timeout-minutes: 10
    permissions:
      contents: read
      id-token: write
    steps:
      - name: Checkout with push key
        if: github.event_name == 'push'
        uses: actions/checkout@v6
        with:
          ssh-key: "${{ secrets.PUSH_KEY }}"

      - name: Bump Version
        if: github.event_name == 'push'
        uses: GarnerCorp/build-actions/bump-version@main
        id: version
        with:
          version-type: "node"
          version-file-path: "package.json"
          git-name: "something"
          git-email: "something@example.com"
          major: changelogs/major
          minor: changelogs/minor

There's some fancy dancing involved, and we don't use it for public repositories, so we aren't particularly worried about things, but, ...

You could do it in various other forms, e.g. having a release workflow that only runs on dispatch or something.

I've been meaning to do a blog post writing up this workflow (probably on dev.to), but I haven't yet, so this is probably the first time I've publicly described it.

  • The github.actor guard corresponds to the user tied to the PUSH_KEY and is designed to prevent eating its own tail.
  • The main* bit enables some amount of testing for changes to the workflow w/o burning real version numbers.

@pierluigilenoci
Copy link
Copy Markdown
Member

This is great, thank you for the detailed breakdown! The pattern is elegant — devs declare intent (major/minor/patch via directory), the workflow handles the actual version bump on merge, and conflicts disappear entirely.

I'll look into bump-version and next-version and see if we can adapt it for this repo. The Helm chart version in Chart.yaml is slightly different from a package.json setup but the core idea translates well.

If you ever publish that blog post on dev.to, I'd love to read it — please drop the link here when it's up! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants