Remove backwards-compatible authentication config

[ambient-code]

Summary

• Remove the legacy authentication configmap key and authenticationConfig Helm value that were kept for backwards compatibility, with TODO comments saying "remove in 0.7.0"
• Remove the now-unused oidc.LoadAuthenticationConfiguration function and its helper newJWTAuthenticator (the config package has its own equivalent implementations that remain in use)
• Remove extractOIDCConfigs() entirely by having LoadConfiguration return the parsed *Config, then calling jwtAuthenticatorsToOIDCConfigs directly with the already-loaded JWT authenticators (eliminates a duplicate ConfigMap fetch)
• Update Dex documentation to reference spec.authentication.jwt on the Jumpstarter resource instead of the removed jumpstarter-controller.authenticationConfiguration Helm value
• Add table-driven tests for jwtAuthenticatorsToOIDCConfigs() covering localhost filtering, nil/empty inputs, multiple authenticators, and content verification
• Add comprehensive unit tests for LoadConfiguration() verifying error handling for legacy authentication key, both keys present scenario, missing keys, and invalid YAML

Known follow-up

• The AuthenticationConfiguration type in controller/api/v1alpha1/authenticationconfiguration_types.go is now unreferenced after the removal of oidc.LoadAuthenticationConfiguration. Removing it requires regenerating zz_generated.deepcopy.go and updating scheme registration, so it will be handled in a separate PR to keep this one focused.

Test plan

• go build ./... passes in the controller module with no errors
• go test ./cmd/ -run TestJwt passes with all test cases (8 total)
• go test ./internal/config -run TestLoadConfiguration passes with 6 test cases covering:
  • Valid config key only
  • Legacy authentication key only (should fail)
  • Both config and authentication keys present (uses config only)
  • Missing config/router keys (should fail)
  • Invalid YAML (should fail)
• Rebased onto main
• Verify Helm chart renders correctly without authenticationConfig value
• Verify existing deployments using the new config key continue to work

Closes #530

Generated with Claude Code

[coderabbitai]

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ec53be19-c61e-4cb0-b453-871d3e5fb8c4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch ambient-fix/530-remove-auth-compat

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[netlify]

✅ Deploy Preview for jumpstarter-docs ready!

Name	Link
🔨 Latest commit	892b7bf
🔍 Latest deploy log	https://app.netlify.com/projects/jumpstarter-docs/deploys/69d7ccccceb40500089fcc96
😎 Deploy Preview	https://deploy-preview-537--jumpstarter-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[ambient-code]

CI Status Update

The pytest failure in jumpstarter/exporter/hooks_test.py::TestHookExecutor::test_exec_bash appears to be unrelated to this PR's changes.

This PR only modifies:

• Go controller code (config.go, oidc/config.go)
• Helm templates and values
• Documentation

The failing test is a macOS-specific Python test that validates bash substring syntax in hook execution. This is not touched by any of the authentication config removal changes.

The test expects the output BASH_OK: world from the bash command V="hello_world"; echo "BASH_OK: ${V:6:5}" but it's not appearing in the mock logger calls on macOS-15 with Python 3.11.

Is this a known flaky test on macOS, or should we investigate further?

[raballew]

Thanks for addressing the legacy fallback removal. The code changes look correct and complete: the authentication key fallback is fully removed from both extractOIDCConfigs() and LoadConfiguration(), the unused oidc/config.go is cleanly deleted, and the Helm chart and docs are updated consistently.

One previous concern remains open: there are still no tests for extractOIDCConfigs(). See inline comment for details.

[ambient-code]

Update: Added tests for extractOIDCConfigs()

Addressing @raballew's review feedback, I've added controller/cmd/main_test.go with table-driven tests in 9511fcb:

Test coverage includes:

• Valid config key with JWT authenticators (positive case)
• Multiple JWT authenticators with localhost issuer filtering
• Legacy authentication key only -- regression test confirming it is ignored
• Missing config key returns nil
• Invalid YAML in config key returns nil
• Missing ConfigMap returns nil
• Content verification (issuer URL, clientID, audiences)

All 9 test cases pass. The regression test specifically guards against reintroduction of the legacy authentication key fallback.

CI note: The pytest failures (hooks_test.py and pytest-matrix jobs) are unrelated to this PR -- they affect the Python test suite and this PR only modifies Go controller code, Helm chart, and documentation.

[ambient-code]

Fix plan

Addressing @raballew's latest review:

1. [LOW] Remove duplicate test case -- The table-driven case "legacy authentication key only should NOT produce OIDC configs" duplicates TestExtractOIDCConfigsIgnoresLegacyKey. Will remove the table-driven case and keep the dedicated function.

2. [MEDIUM] Dead AuthenticationConfiguration type -- Acknowledged but deferring to a follow-up PR since removing it requires regenerating zz_generated.deepcopy.go and updating scheme registration. Will note this in the PR description.

CI: The e2e-tests failures (hooks_test.go:355 -- asyncio.exceptions.InvalidStateError in lease timeout test) are unrelated to this PR's authentication config changes.

[ambient-code]

Rebase onto main

Rebased the branch onto current main to resolve a merge conflict in controller/internal/config/config.go.

Conflict resolution: The conflict arose because main added a *LeasePolicy return value to LoadConfiguration() (from PR #622), while this PR removes the legacy authentication fallback block. The resolution:

• Keeps the legacy block removal (this PR's intent)
• Incorporates the new *LeasePolicy return value from main
• All error returns now correctly include the extra nil for the *LeasePolicy position

Verification:

• go build ./... passes in the controller module
• go test ./cmd/ -run TestExtractOIDC passes (all 7 test cases)
• No other files had conflicts

[ambient-code]

CI Status Update

All CI checks are passing except for the typos check, which failed due to a transient network error:

ERROR 502: Bad Gateway or Proxy Error.

This is an infrastructure issue when downloading the typos tool from GitHub releases, not related to the PR changes. The check should pass on the next retry or re-run.

All other CI checks are green:

• Controller unit/functional tests: PASSING
• End-to-end tests: PASSING
• Python tests: PASSING
• Linters (go, helm, python): PASSING
• Kind-based CI: PASSING
• Documentation build: PASSING

No code changes are needed for this PR.

[ambient-code]

Ready for Re-Review

All requested changes from the last review have been addressed:

1. [LOW] Duplicate test case removed - Commit 2c64294 removed the duplicate table-driven test case for legacy key regression, keeping only the dedicated TestExtractOIDCConfigsIgnoresLegacyKey function for clarity.

2. [MEDIUM] Dead AuthenticationConfiguration type documented - Added to PR description under "Known follow-up" section. This will be handled in a separate PR to keep the review surface contained.

3. All CI checks passing - All 30 CI checks are green, including controller tests, e2e tests, linters, and deployments.

@raballew Ready for re-review when you have a moment.

[ambient-code]

Rebase onto main

Rebased the branch onto current main to resolve merge conflicts.

Conflict resolution: Three Helm chart files (model.py, controller-cm.yaml, values.schema.json) that this PR modified were deleted on main by PR #448 ("remove Helm charts and standardize on operator-based deployment"). Since these files no longer exist, the changes to them were dropped during rebase. The remaining PR changes (controller Go code, tests, docs) applied cleanly.

Verification:

• go build ./... passes in the controller module
• go test ./cmd/ -run TestExtractOIDC passes (all 7 test cases)
• No other files had conflicts

[ambient-code]

Status Summary

All requested changes from the April 15 review have been addressed:

Completed

• [LOW] Duplicate test case removed (commit c61a1bd) - Removed the duplicate table-driven test case for legacy key regression, keeping only the dedicated TestExtractOIDCConfigsIgnoresLegacyKey function
• [MEDIUM] Dead AuthenticationConfiguration type - Documented in PR description under "Known follow-up" section. This will be handled in a separate PR to keep the scope focused

Current State

• All 30 CI checks passing
• Test coverage in place for extractOIDCConfigs() with regression tests
• No outstanding issues or failing tests related to this PR's changes

@raballew Ready for re-review when you have a moment.

[raballew]

@ambient-code rebase onto main

[ambient-code]

Rebase onto main

Rebased the branch onto current main (33ad352). The rebase completed cleanly with no conflicts.

All 4 PR commits are now on top of the latest main:

• c91c83be Remove backwards-compatible authentication config (deprecated since 0.7.0)
• 723d9143 Remove legacy authentication fallback from extractOIDCConfigs()
• be235048 Add tests for extractOIDCConfigs() to prevent legacy key regression
• 958945af Remove duplicate legacy key test case from table-driven tests

[ambient-code]

Addressing review feedback (May 28)

Pushed 668d735 with the following changes:

Done in this commit

• [MEDIUM] Dual-key test case -- Added table-driven test "both config and legacy authentication keys present reads only config" with wantIssuer assertion confirming results come from config key only (comment 1)
• [LOW] Verbose comment block removed -- Removed the 5-line comment on TestExtractOIDCConfigsIgnoresLegacyKey per project convention (comment 4)

Acknowledged as follow-ups

• [MEDIUM] LoadConfiguration() tests -- Deferred to a follow-up PR due to deeper dependencies (oidc.Signer, scheme registration, authenticator chain) (comment 2)
• [LOW] Duplicate ConfigMap fetch -- Refactoring to share parsed Config between LoadConfiguration and extractOIDCConfigs deferred to a dedicated PR (comment 3)
• [LOW] Tautological ClientID assertion -- Left as-is; serves as a canary for future changes (comment 5)

All 8 test cases pass locally (go test ./cmd/ -run TestExtractOIDC).

[ambient-code]

Addressing review feedback (May 28, round 2)

Pushed c336f48 with the following changes:

[LOW] Tautological ClientID assertion (comment by @raballew)

Removed the cfg.ClientID != "jumpstarter-cli" assertion from TestJwtAuthenticatorsToOIDCConfigsContent. The test now only verifies dynamic values (issuer URL, audience count).

[LOW] Duplicate ConfigMap fetch (comment by @raballew)

Refactored to eliminate the second API server round-trip:

• LoadConfiguration now returns the parsed *Config as an additional return value
• main() passes loadedConfig.Authentication.JWT directly to jwtAuthenticatorsToOIDCConfigs
• extractOIDCConfigs is removed entirely
• Tests rewritten to test jwtAuthenticatorsToOIDCConfigs directly with []JWTAuthenticator inputs (no more fake client/ConfigMap setup)

CI

• linkcheck failure is a transient network error (Response ended prematurely on kubernetes.io URL) -- not related to PR changes
• e2e-tests (ubuntu-24.04, amd64) is still pending
• All other 28 checks pass

All 8 test cases pass locally and go build ./... succeeds.

[ambient-code]

The branch appears to be up-to-date with main. The only failing CI check is linkcheck which failed due to a network timeout connecting to kubernetes.io (not related to the authentication config changes in this PR). This is a flaky external dependency issue.

All other CI checks are passing. The PR is ready for review from a technical standpoint.

[ambient-code]

I'll address the following requested changes:

1. Eliminate duplicate ConfigMap fetching (from comment on main.go): Refactor to have extractOIDCConfigs reuse the Config already loaded by LoadConfiguration instead of fetching the ConfigMap a second time.

2. Add test for both keys present (from comment on main_test.go): Add a table-driven test case verifying that when both config and authentication keys are present, only the config key is used.

3. Rebase onto main: The branch is currently 1 commit behind.

Working on these fixes now.

[ambient-code]

Addressed all review feedback:

✅ Added comprehensive tests for LoadConfiguration - Created controller/internal/config/config_test.go with 6 test cases:

• Valid config key only
• Legacy authentication key only (correctly returns error)
• Both config and authentication keys present (correctly uses config only)
• Missing config key (returns error)
• Missing router key (returns error)
• Invalid YAML (returns error)

✅ Rebased onto main - Branch is now up-to-date with the latest main branch

Note: The duplicate ConfigMap fetching issue mentioned in earlier review comments has already been addressed in the current code. LoadConfiguration now returns the parsed *Config, and main.go uses it directly (line 310) without a second fetch.

All tests pass:

• go test ./cmd ✓
• go test ./internal/config ✓

[ambient-code]

Fixed the lint-go CI failure: gofmt reported a formatting error in controller/internal/config/config_test.go (line 17). The k8s.io/apiserver import was placed before k8s.io/apimachinery, but gofmt requires imports sorted alphabetically by full path within the group. Reordered accordingly in commit f9cbc27.