Remove backwards-compatible authentication config by ambient-code[bot] · Pull Request #537 · jumpstarter-dev/jumpstarter

ambient-code · 2026-04-09T15:59:03Z

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 the legacy authentication fallback from extractOIDCConfigs() in controller/cmd/main.go so it only reads from the config key
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 extractOIDCConfigs() covering valid parsing, legacy key regression, and error cases
Remove duplicate test case for legacy key regression (keep dedicated TestExtractOIDCConfigsIgnoresLegacyKey)

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 TestExtractOIDC passes with all test cases
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 · 2026-04-09T15:59:11Z

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: 7df29687-ef74-493c-9f8a-5a25fd8758c8

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

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

netlify · 2026-04-09T16:02:16Z

✅ 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 · 2026-04-09T16:32:44Z

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?

….7.0) Remove the legacy `authentication` configmap key and `authenticationConfig` Helm value that were kept for backwards compatibility with a TODO to remove in 0.7.0. The project is now past v0.8.1, so this cleanup is overdue. Also removes the now-unused `oidc.LoadAuthenticationConfiguration` function and updates documentation to reference the current config approach. Closes #530 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The extractOIDCConfigs() function in controller/cmd/main.go still had a fallback block reading configmap.Data["authentication"], which was missed during the initial cleanup. This removes that legacy path so the function only reads from the "config" key, consistent with the rest of this PR. Also improves error handling by logging parse errors instead of silently swallowing them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

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.

Table-driven tests verify that extractOIDCConfigs() only reads from the "config" key in the ConfigMap, and that the legacy "authentication" key is no longer used. Covers valid parsing, localhost skipping, missing keys, invalid YAML, and missing ConfigMap scenarios. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ambient-code · 2026-04-15T10:38:49Z

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.

raballew · 2026-04-15T20:05:31Z

+		{
+			name: "legacy authentication key only should NOT produce OIDC configs",
+			configmap: &corev1.ConfigMap{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "jumpstarter-controller",
+					Namespace: "default",
+				},
+				Data: map[string]string{
+					"authentication": `
+jwt:
+  - issuer:
+      url: "https://dex.example.com"
+      audiences:
+        - "jumpstarter"
+    claimMappings:
+      username:
+        claim: "email"
+        prefix: ""
+`,
+				},
+			},
+			wantNil: true,
+		},


[LOW] The table-driven case "legacy authentication key only should NOT produce OIDC configs" (lines 103-125) tests the exact same scenario as the dedicated TestExtractOIDCConfigsIgnoresLegacyKey function (lines 246-283): both create a ConfigMap with only the legacy authentication key, call extractOIDCConfigs, and assert nil. This adds maintenance cost without extra coverage.

Consider removing one of the two. If you want the regression test to stand out, keep the dedicated function and drop the table-driven case (or vice versa).

AI-generated, human reviewed

Good catch -- the table-driven case and the dedicated TestExtractOIDCConfigsIgnoresLegacyKey function are indeed testing the same scenario. I will remove the table-driven case and keep the dedicated function since its name and doc comment make the regression intent clearer.

raballew · 2026-04-15T20:05:31Z

 	}

-	return nil
+	return jwtAuthenticatorsToOIDCConfigs(cfg.Authentication.JWT)


[MEDIUM] Now that oidc.LoadAuthenticationConfiguration has been removed, the AuthenticationConfiguration API type in controller/api/v1alpha1/authenticationconfiguration_types.go appears to be unreferenced by any Go code. It is still registered in the scheme via init() and has generated deep copy functions in zz_generated.deepcopy.go, making it dead code.

AI-generated, human reviewed

Correct observation -- with controller/internal/oidc/config.go deleted in this PR, the AuthenticationConfiguration type in controller/api/v1alpha1/authenticationconfiguration_types.go is no longer referenced by any Go code. However, removing it also requires regenerating zz_generated.deepcopy.go (via controller-gen) and updating the scheme registration, which expands the scope of this cleanup PR. I think this is better handled as a follow-up to keep the review surface contained. I will note it in the PR description and open a tracking issue.

ambient-code · 2026-04-15T20:33:46Z

Fix plan

Addressing @raballew's latest review:

[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.
[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.

The table-driven case "legacy authentication key only should NOT produce OIDC configs" duplicated the dedicated TestExtractOIDCConfigsIgnoresLegacyKey function. Keep the dedicated function for clearer regression intent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ambient-code Bot mentioned this pull request Apr 9, 2026

Remove backwards-compatible authentication config in 0.7.0 #530

Open

raballew self-requested a review April 13, 2026 17:09

raballew reviewed Apr 13, 2026

View reviewed changes

Comment thread python/docs/source/getting-started/configuration/authentication.md

raballew requested changes Apr 13, 2026

View reviewed changes

Ambient Code Bot and others added 2 commits April 15, 2026 07:47

ambient-code Bot force-pushed the ambient-fix/530-remove-auth-compat branch from 7adea6f to d26f745 Compare April 15, 2026 07:47

raballew reviewed Apr 15, 2026

View reviewed changes

Comment thread controller/cmd/main.go

raballew requested changes Apr 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove backwards-compatible authentication config#537

Remove backwards-compatible authentication config#537
ambient-code[bot] wants to merge 4 commits intomainfrom
ambient-fix/530-remove-auth-compat

ambient-code Bot commented Apr 9, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Apr 9, 2026 •

edited

Loading

Review skipped

Uh oh!

netlify Bot commented Apr 9, 2026 •

edited

Loading

Uh oh!

ambient-code Bot commented Apr 9, 2026

Uh oh!

Uh oh!

raballew left a comment

Uh oh!

Uh oh!

ambient-code Bot commented Apr 15, 2026

Uh oh!

raballew Apr 15, 2026

Uh oh!

ambient-code Bot Apr 15, 2026

Uh oh!

raballew Apr 15, 2026 •

edited

Loading

Uh oh!

ambient-code Bot Apr 15, 2026

Uh oh!

ambient-code Bot commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ambient-code Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Known follow-up

Test plan

Uh oh!

coderabbitai Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

netlify Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for jumpstarter-docs ready!

Uh oh!

ambient-code Bot commented Apr 9, 2026

CI Status Update

Uh oh!

Uh oh!

raballew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ambient-code Bot commented Apr 15, 2026

Update: Added tests for extractOIDCConfigs()

Uh oh!

raballew Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

ambient-code Bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

raballew Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ambient-code Bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

ambient-code Bot commented Apr 15, 2026

Fix plan

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ambient-code Bot commented Apr 9, 2026 •

edited

Loading

coderabbitai Bot commented Apr 9, 2026 •

edited

Loading

netlify Bot commented Apr 9, 2026 •

edited

Loading

Update: Added tests for `extractOIDCConfigs()`

raballew Apr 15, 2026 •

edited

Loading