From 9f5a0ebeb5baeaaf28b11bf0d82ca79af05f34a7 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Thu, 9 Apr 2026 15:58:49 +0000 Subject: [PATCH 1/8] Remove backwards-compatible authentication config (deprecated since 0.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 --- controller/internal/config/config.go | 25 ----- controller/internal/oidc/config.go | 106 ------------------ .../configuration/authentication.md | 4 +- 3 files changed, 2 insertions(+), 133 deletions(-) delete mode 100644 controller/internal/oidc/config.go diff --git a/controller/internal/config/config.go b/controller/internal/config/config.go index d65a5ce0a..969bed335 100644 --- a/controller/internal/config/config.go +++ b/controller/internal/config/config.go @@ -3,11 +3,9 @@ package config import ( "context" "fmt" - "time" "github.com/jumpstarter-dev/jumpstarter-controller/internal/oidc" "google.golang.org/grpc" - "google.golang.org/grpc/keepalive" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/yaml" @@ -67,29 +65,6 @@ func LoadConfiguration( return nil, "", nil, nil, nil, nil, err } - rawAuthenticationConfiguration, ok := configmap.Data["authentication"] - if ok { - // backwards compatibility - // TODO: remove in 0.7.0 - authenticator, prefix, err := oidc.LoadAuthenticationConfiguration( - ctx, - scheme, - []byte(rawAuthenticationConfiguration), - signer, - certificateAuthority, - ) - if err != nil { - return nil, "", nil, nil, nil, nil, err - } - - return authenticator, prefix, router, []grpc.ServerOption{ - grpc.KeepaliveEnforcementPolicy(keepalive.EnforcementPolicy{ - MinTime: 1 * time.Second, - PermitWithoutStream: true, - }), - }, &Provisioning{Enabled: false}, &LeasePolicy{MaxTags: 10}, nil - } - rawConfig, ok := configmap.Data["config"] if !ok { return nil, "", nil, nil, nil, nil, fmt.Errorf("LoadConfiguration: missing config section") diff --git a/controller/internal/oidc/config.go b/controller/internal/oidc/config.go deleted file mode 100644 index 67550125b..000000000 --- a/controller/internal/oidc/config.go +++ /dev/null @@ -1,106 +0,0 @@ -package oidc - -import ( - "context" - "os" - - jumpstarterdevv1alpha1 "github.com/jumpstarter-dev/jumpstarter-controller/api/v1alpha1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/serializer" - "k8s.io/apiserver/pkg/apis/apiserver" - apiserverv1beta1 "k8s.io/apiserver/pkg/apis/apiserver/v1beta1" - "k8s.io/apiserver/pkg/authentication/authenticator" - tokenunion "k8s.io/apiserver/pkg/authentication/token/union" - "k8s.io/apiserver/pkg/server/dynamiccertificates" - "k8s.io/apiserver/plugin/pkg/authenticator/token/oidc" -) - -func LoadAuthenticationConfiguration( - ctx context.Context, - scheme *runtime.Scheme, - configuration []byte, - signer *Signer, - certificateAuthority string, -) (authenticator.Token, string, error) { - var authenticationConfiguration jumpstarterdevv1alpha1.AuthenticationConfiguration - if err := runtime.DecodeInto( - serializer.NewCodecFactory(scheme, serializer.EnableStrict). - UniversalDecoder(jumpstarterdevv1alpha1.GroupVersion), - configuration, - &authenticationConfiguration, - ); err != nil { - return nil, "", err - } - - if authenticationConfiguration.Internal.Prefix == "" { - authenticationConfiguration.Internal.Prefix = "internal:" - } - - authenticationConfiguration.JWT = append(authenticationConfiguration.JWT, apiserverv1beta1.JWTAuthenticator{ - Issuer: apiserverv1beta1.Issuer{ - URL: signer.Issuer(), - CertificateAuthority: certificateAuthority, - Audiences: []string{signer.Audience()}, - }, - ClaimMappings: apiserverv1beta1.ClaimMappings{ - Username: apiserverv1beta1.PrefixedClaimOrExpression{ - Claim: "sub", - Prefix: &authenticationConfiguration.Internal.Prefix, - }, - }, - }) - - authn, err := newJWTAuthenticator( - ctx, - scheme, - authenticationConfiguration, - ) - if err != nil { - return nil, "", err - } - return authn, authenticationConfiguration.Internal.Prefix, nil -} - -// Reference: https://github.com/kubernetes/kubernetes/blob/v1.32.1/pkg/kubeapiserver/authenticator/config.go#L244 -func newJWTAuthenticator( - ctx context.Context, - scheme *runtime.Scheme, - config jumpstarterdevv1alpha1.AuthenticationConfiguration, -) (authenticator.Token, error) { - var jwtAuthenticators []authenticator.Token - for _, jwtAuthenticator := range config.JWT { - var oidcCAContent oidc.CAContentProvider - if len(jwtAuthenticator.Issuer.CertificateAuthority) > 0 { - var oidcCAError error - if _, err := os.Stat(jwtAuthenticator.Issuer.CertificateAuthority); err == nil { - oidcCAContent, oidcCAError = dynamiccertificates.NewDynamicCAContentFromFile( - "oidc-authenticator", - jwtAuthenticator.Issuer.CertificateAuthority, - ) - jwtAuthenticator.Issuer.CertificateAuthority = "" - } else { - oidcCAContent, oidcCAError = dynamiccertificates.NewStaticCAContent( - "oidc-authenticator", - []byte(jwtAuthenticator.Issuer.CertificateAuthority), - ) - } - if oidcCAError != nil { - return nil, oidcCAError - } - } - var jwtAuthenticatorUnversioned apiserver.JWTAuthenticator - if err := scheme.Convert(&jwtAuthenticator, &jwtAuthenticatorUnversioned, nil); err != nil { - return nil, err - } - oidcAuth, err := oidc.New(ctx, oidc.Options{ - JWTAuthenticator: jwtAuthenticatorUnversioned, - CAContentProvider: oidcCAContent, - SupportedSigningAlgs: oidc.AllValidSigningAlgorithms(), - }) - if err != nil { - return nil, err - } - jwtAuthenticators = append(jwtAuthenticators, oidcAuth) - } - return tokenunion.NewFailOnError(jwtAuthenticators...), nil -} diff --git a/python/docs/source/getting-started/configuration/authentication.md b/python/docs/source/getting-started/configuration/authentication.md index 182e20bb1..773a712a6 100644 --- a/python/docs/source/getting-started/configuration/authentication.md +++ b/python/docs/source/getting-started/configuration/authentication.md @@ -194,8 +194,8 @@ $ helm repo add dex https://charts.dexidp.io $ helm install --namespace dex --wait -f values.yaml dex dex/dex ``` -3. Configure Jumpstarter to trust Dex. Use this configuration for - `jumpstarter-controller.authenticationConfiguration` during installation: +3. Configure Jumpstarter to trust Dex. Set `spec.authentication.jwt` on your + `Jumpstarter` resource: ```yaml spec: From ce7739615fd1b59b3c78fd2d549f6f654eaeb817 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Mon, 13 Apr 2026 18:36:46 +0000 Subject: [PATCH 2/8] Remove legacy authentication fallback from extractOIDCConfigs() 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 --- controller/cmd/main.go | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/controller/cmd/main.go b/controller/cmd/main.go index 94645da1f..38ed95b97 100644 --- a/controller/cmd/main.go +++ b/controller/cmd/main.go @@ -343,25 +343,18 @@ func extractOIDCConfigs(reader client.Reader, namespace string) []login.OIDCConf return nil } - // Try new config format first rawConfig, ok := configmap.Data["config"] - if ok { - var cfg config.Config - if err := yaml.UnmarshalStrict([]byte(rawConfig), &cfg); err == nil { - return jwtAuthenticatorsToOIDCConfigs(cfg.Authentication.JWT) - } + if !ok { + return nil } - // Fall back to legacy authentication format - rawAuth, ok := configmap.Data["authentication"] - if ok { - var auth config.Authentication - if err := yaml.Unmarshal([]byte(rawAuth), &auth); err == nil { - return jwtAuthenticatorsToOIDCConfigs(auth.JWT) - } + var cfg config.Config + if err := yaml.UnmarshalStrict([]byte(rawConfig), &cfg); err != nil { + setupLog.Error(err, "unable to parse config for OIDC config extraction") + return nil } - return nil + return jwtAuthenticatorsToOIDCConfigs(cfg.Authentication.JWT) } // jwtAuthenticatorsToOIDCConfigs converts JWT authenticators to login OIDC configs From 6a9db26c63b9ac77969e040b808f15471e6c5da8 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Wed, 15 Apr 2026 10:38:36 +0000 Subject: [PATCH 3/8] Add tests for extractOIDCConfigs() to prevent legacy key regression 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 --- controller/cmd/main_test.go | 283 ++++++++++++++++++++++++++++++++++++ 1 file changed, 283 insertions(+) create mode 100644 controller/cmd/main_test.go diff --git a/controller/cmd/main_test.go b/controller/cmd/main_test.go new file mode 100644 index 000000000..95e8fa5a4 --- /dev/null +++ b/controller/cmd/main_test.go @@ -0,0 +1,283 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestExtractOIDCConfigs(t *testing.T) { + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("failed to add corev1 to scheme: %v", err) + } + + tests := []struct { + name string + configmap *corev1.ConfigMap + wantCount int + wantNil bool + }{ + { + name: "valid config key with JWT authenticators", + configmap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "jumpstarter-controller", + Namespace: "default", + }, + Data: map[string]string{ + "config": ` +authentication: + internal: + prefix: "internal:" + tokenLifetime: "43800h" + jwt: + - issuer: + url: "https://dex.example.com" + audiences: + - "jumpstarter" + claimMappings: + username: + claim: "email" + prefix: "" +`, + }, + }, + wantCount: 1, + }, + { + name: "valid config key with multiple JWT authenticators including localhost", + configmap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "jumpstarter-controller", + Namespace: "default", + }, + Data: map[string]string{ + "config": ` +authentication: + internal: + prefix: "internal:" + tokenLifetime: "43800h" + jwt: + - issuer: + url: "https://localhost:8085" + audiences: + - "jumpstarter" + claimMappings: + username: + claim: "sub" + prefix: "" + - issuer: + url: "https://dex.example.com" + audiences: + - "jumpstarter" + claimMappings: + username: + claim: "email" + prefix: "" +`, + }, + }, + wantCount: 1, // localhost issuer should be skipped + }, + { + 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, + }, + { + name: "missing config key returns nil", + configmap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "jumpstarter-controller", + Namespace: "default", + }, + Data: map[string]string{ + "router": `default: {endpoint: "router.example.com:443"}`, + }, + }, + wantNil: true, + }, + { + name: "invalid YAML in config key returns nil", + configmap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "jumpstarter-controller", + Namespace: "default", + }, + Data: map[string]string{ + "config": `{{{invalid yaml`, + }, + }, + wantNil: true, + }, + { + name: "missing configmap returns nil", + configmap: nil, + wantNil: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + builder := fake.NewClientBuilder().WithScheme(scheme) + if tt.configmap != nil { + builder = builder.WithObjects(tt.configmap) + } + fakeClient := builder.Build() + + configs := extractOIDCConfigs(fakeClient.(client.Reader), "default") + + if tt.wantNil { + if configs != nil { + t.Errorf("expected nil, got %v", configs) + } + return + } + + if configs == nil { + t.Fatal("expected non-nil configs, got nil") + } + + if len(configs) != tt.wantCount { + t.Errorf("expected %d configs, got %d", tt.wantCount, len(configs)) + } + }) + } +} + +func TestExtractOIDCConfigsContent(t *testing.T) { + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("failed to add corev1 to scheme: %v", err) + } + + configmap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "jumpstarter-controller", + Namespace: "default", + }, + Data: map[string]string{ + "config": ` +authentication: + internal: + prefix: "internal:" + tokenLifetime: "43800h" + jwt: + - issuer: + url: "https://dex.example.com" + audiences: + - "jumpstarter" + - "jumpstarter-cli" + claimMappings: + username: + claim: "email" + prefix: "" +`, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(configmap). + Build() + + configs := extractOIDCConfigs(fakeClient.(client.Reader), "default") + + if len(configs) != 1 { + t.Fatalf("expected 1 config, got %d", len(configs)) + } + + cfg := configs[0] + if cfg.Issuer != "https://dex.example.com" { + t.Errorf("expected issuer 'https://dex.example.com', got %q", cfg.Issuer) + } + if cfg.ClientID != "jumpstarter-cli" { + t.Errorf("expected clientID 'jumpstarter-cli', got %q", cfg.ClientID) + } + if len(cfg.Audiences) != 2 { + t.Errorf("expected 2 audiences, got %d", len(cfg.Audiences)) + } +} + +// TestExtractOIDCConfigsIgnoresLegacyKey is a dedicated regression test +// ensuring that a ConfigMap containing only the legacy "authentication" key +// (without the "config" key) does NOT produce any OIDC configurations. +// This prevents accidental reintroduction of the legacy fallback removed in +// this PR. +func TestExtractOIDCConfigsIgnoresLegacyKey(t *testing.T) { + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("failed to add corev1 to scheme: %v", err) + } + + configmap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "jumpstarter-controller", + Namespace: "default", + }, + Data: map[string]string{ + // Only the legacy key -- this must NOT be read + "authentication": ` +jwt: + - issuer: + url: "https://dex.example.com" + audiences: + - "jumpstarter" + claimMappings: + username: + claim: "email" + prefix: "" +`, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(configmap). + Build() + + configs := extractOIDCConfigs(fakeClient.(client.Reader), "default") + + if configs != nil { + t.Errorf("expected nil when only legacy 'authentication' key is present, got %v", configs) + } +} From 4b2a794385899adcda8b50aed9408d52dc542776 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Wed, 15 Apr 2026 20:34:43 +0000 Subject: [PATCH 4/8] Remove duplicate legacy key test case from table-driven tests 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 --- controller/cmd/main_test.go | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/controller/cmd/main_test.go b/controller/cmd/main_test.go index 95e8fa5a4..cbe9abdd4 100644 --- a/controller/cmd/main_test.go +++ b/controller/cmd/main_test.go @@ -100,29 +100,6 @@ authentication: }, wantCount: 1, // localhost issuer should be skipped }, - { - 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, - }, { name: "missing config key returns nil", configmap: &corev1.ConfigMap{ From 8270ff3c654bfa3fbd6fbe52e5ed331cd95ed50c Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Thu, 28 May 2026 11:40:52 +0000 Subject: [PATCH 5/8] Address review feedback: add dual-key test case and remove verbose comment - Add table-driven test for ConfigMap with both 'config' and legacy 'authentication' keys present, asserting results come from 'config' only - Remove redundant 5-line comment block on TestExtractOIDCConfigsIgnoresLegacyKey per project convention that code should be self-explanatory Co-Authored-By: Claude Opus 4.6 --- controller/cmd/main_test.go | 59 +++++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 9 deletions(-) diff --git a/controller/cmd/main_test.go b/controller/cmd/main_test.go index cbe9abdd4..014d7ba0f 100644 --- a/controller/cmd/main_test.go +++ b/controller/cmd/main_test.go @@ -33,10 +33,11 @@ func TestExtractOIDCConfigs(t *testing.T) { } tests := []struct { - name string - configmap *corev1.ConfigMap - wantCount int - wantNil bool + name string + configmap *corev1.ConfigMap + wantCount int + wantNil bool + wantIssuer string }{ { name: "valid config key with JWT authenticators", @@ -126,6 +127,45 @@ authentication: }, wantNil: true, }, + { + name: "both config and legacy authentication keys present reads only config", + configmap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "jumpstarter-controller", + Namespace: "default", + }, + Data: map[string]string{ + "config": ` +authentication: + internal: + prefix: "internal:" + tokenLifetime: "43800h" + jwt: + - issuer: + url: "https://dex.example.com" + audiences: + - "jumpstarter" + claimMappings: + username: + claim: "email" + prefix: "" +`, + "authentication": ` +jwt: + - issuer: + url: "https://legacy.example.com" + audiences: + - "jumpstarter" + claimMappings: + username: + claim: "email" + prefix: "" +`, + }, + }, + wantCount: 1, + wantIssuer: "https://dex.example.com", + }, { name: "missing configmap returns nil", configmap: nil, @@ -157,6 +197,12 @@ authentication: if len(configs) != tt.wantCount { t.Errorf("expected %d configs, got %d", tt.wantCount, len(configs)) } + + if tt.wantIssuer != "" && len(configs) > 0 { + if configs[0].Issuer != tt.wantIssuer { + t.Errorf("expected issuer %q, got %q", tt.wantIssuer, configs[0].Issuer) + } + } }) } } @@ -215,11 +261,6 @@ authentication: } } -// TestExtractOIDCConfigsIgnoresLegacyKey is a dedicated regression test -// ensuring that a ConfigMap containing only the legacy "authentication" key -// (without the "config" key) does NOT produce any OIDC configurations. -// This prevents accidental reintroduction of the legacy fallback removed in -// this PR. func TestExtractOIDCConfigsIgnoresLegacyKey(t *testing.T) { scheme := runtime.NewScheme() if err := corev1.AddToScheme(scheme); err != nil { From e5e70af3ab95bcdcd14b314c530799c5eb2e8ca1 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Thu, 28 May 2026 12:51:16 +0000 Subject: [PATCH 6/8] Refactor extractOIDCConfigs to use parsed config from LoadConfiguration Eliminate the duplicate ConfigMap fetch by having LoadConfiguration return the parsed Config, then calling jwtAuthenticatorsToOIDCConfigs directly with the already-loaded JWT authenticators. Also removes the tautological ClientID assertion from tests. Co-Authored-By: Claude Opus 4.6 --- controller/cmd/main.go | 34 +--- controller/cmd/main_test.go | 270 +++++++-------------------- controller/internal/config/config.go | 18 +- 3 files changed, 75 insertions(+), 247 deletions(-) diff --git a/controller/cmd/main.go b/controller/cmd/main.go index 38ed95b97..a6f63764b 100644 --- a/controller/cmd/main.go +++ b/controller/cmd/main.go @@ -30,10 +30,8 @@ import ( apiserverv1beta1 "k8s.io/apiserver/pkg/apis/apiserver/v1beta1" _ "k8s.io/client-go/plugin/pkg/client/auth" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" - "k8s.io/apimachinery/pkg/util/yaml" clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/cache" @@ -219,7 +217,7 @@ func main() { os.Exit(1) } - authenticator, prefix, router, option, provisioning, leasePolicy, err := config.LoadConfiguration( + authenticator, prefix, router, option, provisioning, leasePolicy, loadedConfig, err := config.LoadConfiguration( context.Background(), mgr.GetAPIReader(), mgr.GetScheme(), @@ -308,9 +306,8 @@ func main() { // Setup Login Service for simplified CLI login loginService := login.NewServiceFromEnv() - // Extract OIDC configuration from the loaded config for the login service - oidcConfigs := extractOIDCConfigs(mgr.GetAPIReader(), watchNamespace) - loginService.SetOIDCConfig(oidcConfigs) + // Use the already-parsed config to extract OIDC configuration for the login service + loginService.SetOIDCConfig(jwtAuthenticatorsToOIDCConfigs(loadedConfig.Authentication.JWT)) if err = loginService.SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create service", "service", "Login") os.Exit(1) @@ -332,31 +329,6 @@ func main() { } } -// extractOIDCConfigs reads the OIDC configuration from the ConfigMap -func extractOIDCConfigs(reader client.Reader, namespace string) []login.OIDCConfig { - var configmap corev1.ConfigMap - if err := reader.Get(context.Background(), client.ObjectKey{ - Namespace: namespace, - Name: "jumpstarter-controller", - }, &configmap); err != nil { - setupLog.Error(err, "unable to read configmap for OIDC config") - return nil - } - - rawConfig, ok := configmap.Data["config"] - if !ok { - return nil - } - - var cfg config.Config - if err := yaml.UnmarshalStrict([]byte(rawConfig), &cfg); err != nil { - setupLog.Error(err, "unable to parse config for OIDC config extraction") - return nil - } - - return jwtAuthenticatorsToOIDCConfigs(cfg.Authentication.JWT) -} - // jwtAuthenticatorsToOIDCConfigs converts JWT authenticators to login OIDC configs func jwtAuthenticatorsToOIDCConfigs(authenticators []apiserverv1beta1.JWTAuthenticator) []login.OIDCConfig { var configs []login.OIDCConfig diff --git a/controller/cmd/main_test.go b/controller/cmd/main_test.go index 014d7ba0f..ccd18ac91 100644 --- a/controller/cmd/main_test.go +++ b/controller/cmd/main_test.go @@ -19,169 +19,93 @@ package main import ( "testing" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" + apiserverv1beta1 "k8s.io/apiserver/pkg/apis/apiserver/v1beta1" ) -func TestExtractOIDCConfigs(t *testing.T) { - scheme := runtime.NewScheme() - if err := corev1.AddToScheme(scheme); err != nil { - t.Fatalf("failed to add corev1 to scheme: %v", err) - } - +func TestJwtAuthenticatorsToOIDCConfigs(t *testing.T) { tests := []struct { - name string - configmap *corev1.ConfigMap - wantCount int - wantNil bool - wantIssuer string + name string + authenticators []apiserverv1beta1.JWTAuthenticator + wantCount int + wantNil bool + wantIssuer string }{ { - name: "valid config key with JWT authenticators", - configmap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "jumpstarter-controller", - Namespace: "default", - }, - Data: map[string]string{ - "config": ` -authentication: - internal: - prefix: "internal:" - tokenLifetime: "43800h" - jwt: - - issuer: - url: "https://dex.example.com" - audiences: - - "jumpstarter" - claimMappings: - username: - claim: "email" - prefix: "" -`, + name: "single JWT authenticator", + authenticators: []apiserverv1beta1.JWTAuthenticator{ + { + Issuer: apiserverv1beta1.Issuer{ + URL: "https://dex.example.com", + Audiences: []string{"jumpstarter"}, + }, }, }, wantCount: 1, }, { - name: "valid config key with multiple JWT authenticators including localhost", - configmap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "jumpstarter-controller", - Namespace: "default", + name: "multiple JWT authenticators including localhost", + authenticators: []apiserverv1beta1.JWTAuthenticator{ + { + Issuer: apiserverv1beta1.Issuer{ + URL: "https://localhost:8085", + Audiences: []string{"jumpstarter"}, + }, }, - Data: map[string]string{ - "config": ` -authentication: - internal: - prefix: "internal:" - tokenLifetime: "43800h" - jwt: - - issuer: - url: "https://localhost:8085" - audiences: - - "jumpstarter" - claimMappings: - username: - claim: "sub" - prefix: "" - - issuer: - url: "https://dex.example.com" - audiences: - - "jumpstarter" - claimMappings: - username: - claim: "email" - prefix: "" -`, + { + Issuer: apiserverv1beta1.Issuer{ + URL: "https://dex.example.com", + Audiences: []string{"jumpstarter"}, + }, }, }, wantCount: 1, // localhost issuer should be skipped }, { - name: "missing config key returns nil", - configmap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "jumpstarter-controller", - Namespace: "default", - }, - Data: map[string]string{ - "router": `default: {endpoint: "router.example.com:443"}`, - }, - }, - wantNil: true, + name: "nil authenticators returns nil", + authenticators: nil, + wantNil: true, }, { - name: "invalid YAML in config key returns nil", - configmap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "jumpstarter-controller", - Namespace: "default", - }, - Data: map[string]string{ - "config": `{{{invalid yaml`, + name: "empty authenticators returns nil", + authenticators: []apiserverv1beta1.JWTAuthenticator{}, + wantNil: true, + }, + { + name: "only localhost authenticator returns nil", + authenticators: []apiserverv1beta1.JWTAuthenticator{ + { + Issuer: apiserverv1beta1.Issuer{ + URL: "https://localhost:8085", + Audiences: []string{"jumpstarter"}, + }, }, }, wantNil: true, }, { - name: "both config and legacy authentication keys present reads only config", - configmap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "jumpstarter-controller", - Namespace: "default", + name: "multiple external authenticators", + authenticators: []apiserverv1beta1.JWTAuthenticator{ + { + Issuer: apiserverv1beta1.Issuer{ + URL: "https://dex.example.com", + Audiences: []string{"jumpstarter"}, + }, }, - Data: map[string]string{ - "config": ` -authentication: - internal: - prefix: "internal:" - tokenLifetime: "43800h" - jwt: - - issuer: - url: "https://dex.example.com" - audiences: - - "jumpstarter" - claimMappings: - username: - claim: "email" - prefix: "" -`, - "authentication": ` -jwt: - - issuer: - url: "https://legacy.example.com" - audiences: - - "jumpstarter" - claimMappings: - username: - claim: "email" - prefix: "" -`, + { + Issuer: apiserverv1beta1.Issuer{ + URL: "https://keycloak.example.com", + Audiences: []string{"jumpstarter", "jumpstarter-cli"}, + }, }, }, - wantCount: 1, + wantCount: 2, wantIssuer: "https://dex.example.com", }, - { - name: "missing configmap returns nil", - configmap: nil, - wantNil: true, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - builder := fake.NewClientBuilder().WithScheme(scheme) - if tt.configmap != nil { - builder = builder.WithObjects(tt.configmap) - } - fakeClient := builder.Build() - - configs := extractOIDCConfigs(fakeClient.(client.Reader), "default") + configs := jwtAuthenticatorsToOIDCConfigs(tt.authenticators) if tt.wantNil { if configs != nil { @@ -207,43 +131,17 @@ jwt: } } -func TestExtractOIDCConfigsContent(t *testing.T) { - scheme := runtime.NewScheme() - if err := corev1.AddToScheme(scheme); err != nil { - t.Fatalf("failed to add corev1 to scheme: %v", err) - } - - configmap := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "jumpstarter-controller", - Namespace: "default", - }, - Data: map[string]string{ - "config": ` -authentication: - internal: - prefix: "internal:" - tokenLifetime: "43800h" - jwt: - - issuer: - url: "https://dex.example.com" - audiences: - - "jumpstarter" - - "jumpstarter-cli" - claimMappings: - username: - claim: "email" - prefix: "" -`, +func TestJwtAuthenticatorsToOIDCConfigsContent(t *testing.T) { + authenticators := []apiserverv1beta1.JWTAuthenticator{ + { + Issuer: apiserverv1beta1.Issuer{ + URL: "https://dex.example.com", + Audiences: []string{"jumpstarter", "jumpstarter-cli"}, + }, }, } - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(configmap). - Build() - - configs := extractOIDCConfigs(fakeClient.(client.Reader), "default") + configs := jwtAuthenticatorsToOIDCConfigs(authenticators) if len(configs) != 1 { t.Fatalf("expected 1 config, got %d", len(configs)) @@ -253,49 +151,7 @@ authentication: if cfg.Issuer != "https://dex.example.com" { t.Errorf("expected issuer 'https://dex.example.com', got %q", cfg.Issuer) } - if cfg.ClientID != "jumpstarter-cli" { - t.Errorf("expected clientID 'jumpstarter-cli', got %q", cfg.ClientID) - } if len(cfg.Audiences) != 2 { t.Errorf("expected 2 audiences, got %d", len(cfg.Audiences)) } } - -func TestExtractOIDCConfigsIgnoresLegacyKey(t *testing.T) { - scheme := runtime.NewScheme() - if err := corev1.AddToScheme(scheme); err != nil { - t.Fatalf("failed to add corev1 to scheme: %v", err) - } - - configmap := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "jumpstarter-controller", - Namespace: "default", - }, - Data: map[string]string{ - // Only the legacy key -- this must NOT be read - "authentication": ` -jwt: - - issuer: - url: "https://dex.example.com" - audiences: - - "jumpstarter" - claimMappings: - username: - claim: "email" - prefix: "" -`, - }, - } - - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(configmap). - Build() - - configs := extractOIDCConfigs(fakeClient.(client.Reader), "default") - - if configs != nil { - t.Errorf("expected nil when only legacy 'authentication' key is present, got %v", configs) - } -} diff --git a/controller/internal/config/config.go b/controller/internal/config/config.go index 969bed335..bb4be3386 100644 --- a/controller/internal/config/config.go +++ b/controller/internal/config/config.go @@ -49,30 +49,30 @@ func LoadConfiguration( key client.ObjectKey, signer *oidc.Signer, certificateAuthority string, -) (authenticator.Token, string, Router, []grpc.ServerOption, *Provisioning, *LeasePolicy, error) { +) (authenticator.Token, string, Router, []grpc.ServerOption, *Provisioning, *LeasePolicy, *Config, error) { var configmap corev1.ConfigMap if err := client.Get(ctx, key, &configmap); err != nil { - return nil, "", nil, nil, nil, nil, err + return nil, "", nil, nil, nil, nil, nil, err } rawRouter, ok := configmap.Data["router"] if !ok { - return nil, "", nil, nil, nil, nil, fmt.Errorf("LoadConfiguration: missing router section") + return nil, "", nil, nil, nil, nil, nil, fmt.Errorf("LoadConfiguration: missing router section") } var router Router if err := yaml.Unmarshal([]byte(rawRouter), &router); err != nil { - return nil, "", nil, nil, nil, nil, err + return nil, "", nil, nil, nil, nil, nil, err } rawConfig, ok := configmap.Data["config"] if !ok { - return nil, "", nil, nil, nil, nil, fmt.Errorf("LoadConfiguration: missing config section") + return nil, "", nil, nil, nil, nil, nil, fmt.Errorf("LoadConfiguration: missing config section") } var config Config if err := yaml.UnmarshalStrict([]byte(rawConfig), &config); err != nil { - return nil, "", nil, nil, nil, nil, err + return nil, "", nil, nil, nil, nil, nil, err } authenticator, prefix, err := LoadAuthenticationConfiguration( @@ -83,13 +83,13 @@ func LoadConfiguration( certificateAuthority, ) if err != nil { - return nil, "", nil, nil, nil, nil, err + return nil, "", nil, nil, nil, nil, nil, err } serverOptions, err := LoadGrpcConfiguration(config.Grpc) if err != nil { - return nil, "", nil, nil, nil, nil, err + return nil, "", nil, nil, nil, nil, nil, err } - return authenticator, prefix, router, serverOptions, &config.Provisioning, &config.LeasePolicy, nil + return authenticator, prefix, router, serverOptions, &config.Provisioning, &config.LeasePolicy, &config, nil } From 3a6e9f909a45c598ad1a40cdc4d4a5dc3a871ea9 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Fri, 29 May 2026 02:39:27 +0000 Subject: [PATCH 7/8] Add comprehensive tests for LoadConfiguration - Add unit tests for LoadConfiguration to verify it returns an error when only the legacy 'authentication' key is present - Add test verifying that when both 'config' and 'authentication' keys are present, only the 'config' key is used - Add tests for missing config/router keys and invalid YAML - Generate self-signed test certificate for OIDC authentication testing - All tests pass, ensuring the removal of legacy authentication config is properly validated Addresses code review feedback from raballew. Co-Authored-By: Claude Opus 4.6 --- controller/internal/config/config_test.go | 289 ++++++++++++++++++++++ 1 file changed, 289 insertions(+) create mode 100644 controller/internal/config/config_test.go diff --git a/controller/internal/config/config_test.go b/controller/internal/config/config_test.go new file mode 100644 index 000000000..9d55daf1b --- /dev/null +++ b/controller/internal/config/config_test.go @@ -0,0 +1,289 @@ +package config + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "testing" + "time" + + "github.com/jumpstarter-dev/jumpstarter-controller/internal/oidc" + corev1 "k8s.io/api/core/v1" + apiserverinstall "k8s.io/apiserver/pkg/apis/apiserver/install" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// generateTestCertificate creates a minimal self-signed certificate for testing +func generateTestCertificate(t *testing.T) string { + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("failed to generate private key: %v", err) + } + + template := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{ + Organization: []string{"Test"}, + }, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + } + + certDER, err := x509.CreateCertificate(rand.Reader, &template, &template, &priv.PublicKey, priv) + if err != nil { + t.Fatalf("failed to create certificate: %v", err) + } + + return string(pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: certDER, + })) +} + +func TestLoadConfiguration(t *testing.T) { + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("failed to add corev1 to scheme: %v", err) + } + apiserverinstall.Install(scheme) + + // Create a mock OIDC signer for testing + signer, err := oidc.NewSignerFromSeed([]byte{}, "https://example.com", "test") + if err != nil { + t.Fatalf("failed to create OIDC signer: %v", err) + } + + // Generate a test certificate + testCert := generateTestCertificate(t) + + tests := []struct { + name string + configMap *corev1.ConfigMap + wantErr bool + errMsg string + }{ + { + name: "valid config key only", + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-config", + Namespace: "default", + }, + Data: map[string]string{ + "router": ` +default: + endpoint: "router.example.com:443" +`, + "config": ` +authentication: + internal: + prefix: "internal:" + jwt: + - issuer: + url: "https://dex.example.com" + audiences: ["jumpstarter"] + claimMappings: + username: + claim: "name" + prefix: "dex:" +grpc: + keepalive: {} +provisioning: + enabled: false +leasePolicy: {} +`, + }, + }, + wantErr: false, + }, + { + name: "legacy authentication key only should fail", + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-config", + Namespace: "default", + }, + Data: map[string]string{ + "router": ` +default: + endpoint: "router.example.com:443" +`, + "authentication": ` +jwt: + - issuer: + url: "https://dex.example.com" + audiences: ["jumpstarter"] +`, + }, + }, + wantErr: true, + errMsg: "missing config section", + }, + { + name: "both config and authentication keys present should use config only", + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-config", + Namespace: "default", + }, + Data: map[string]string{ + "router": ` +default: + endpoint: "router.example.com:443" +`, + "config": ` +authentication: + internal: + prefix: "internal:" + jwt: + - issuer: + url: "https://dex.example.com" + audiences: ["jumpstarter"] + claimMappings: + username: + claim: "name" + prefix: "dex:" +grpc: + keepalive: {} +provisioning: + enabled: false +leasePolicy: {} +`, + "authentication": ` +jwt: + - issuer: + url: "https://legacy.example.com" + audiences: ["legacy"] +`, + }, + }, + wantErr: false, + }, + { + name: "missing config key should fail", + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-config", + Namespace: "default", + }, + Data: map[string]string{ + "router": ` +default: + endpoint: "router.example.com:443" +`, + }, + }, + wantErr: true, + errMsg: "missing config section", + }, + { + name: "missing router key should fail", + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-config", + Namespace: "default", + }, + Data: map[string]string{ + "config": ` +authentication: + internal: + prefix: "internal:" + jwt: + - issuer: + url: "https://dex.example.com" + audiences: ["jumpstarter"] + claimMappings: + username: + claim: "name" + prefix: "dex:" +grpc: + keepalive: {} +provisioning: + enabled: false +leasePolicy: {} +`, + }, + }, + wantErr: true, + errMsg: "missing router section", + }, + { + name: "invalid YAML in config should fail", + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-config", + Namespace: "default", + }, + Data: map[string]string{ + "router": ` +default: + endpoint: "router.example.com:443" +`, + "config": "invalid: yaml: content:", + }, + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create fake client with the test ConfigMap + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(tt.configMap). + Build() + + key := client.ObjectKey{ + Namespace: tt.configMap.Namespace, + Name: tt.configMap.Name, + } + + _, _, _, _, _, _, loadedConfig, err := LoadConfiguration( + context.Background(), + fakeClient, + scheme, + key, + signer, + testCert, + ) + + if tt.wantErr { + if err == nil { + t.Errorf("LoadConfiguration() expected error but got nil") + } else if tt.errMsg != "" && !contains(err.Error(), tt.errMsg) { + t.Errorf("LoadConfiguration() error = %v, want error containing %q", err, tt.errMsg) + } + } else { + if err != nil { + t.Errorf("LoadConfiguration() unexpected error = %v", err) + } + if loadedConfig == nil { + t.Error("LoadConfiguration() returned nil config") + } + } + }) + } +} + +func contains(s, substr string) bool { + return len(s) >= len(substr) && (s == substr || len(s) > len(substr) && stringContains(s, substr)) +} + +func stringContains(s, substr string) bool { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} From df6f799368917bb8e11021468262d6e5ead5e5f9 Mon Sep 17 00:00:00 2001 From: Ambient Code Bot Date: Fri, 29 May 2026 03:32:27 +0000 Subject: [PATCH 8/8] Fix gofmt import ordering in config_test.go Reorder k8s.io imports alphabetically by full path to satisfy gofmt: k8s.io/apimachinery before k8s.io/apiserver. Co-Authored-By: Claude Opus 4.6 --- controller/internal/config/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/internal/config/config_test.go b/controller/internal/config/config_test.go index 9d55daf1b..28c74ec36 100644 --- a/controller/internal/config/config_test.go +++ b/controller/internal/config/config_test.go @@ -14,9 +14,9 @@ import ( "github.com/jumpstarter-dev/jumpstarter-controller/internal/oidc" corev1 "k8s.io/api/core/v1" - apiserverinstall "k8s.io/apiserver/pkg/apis/apiserver/install" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + apiserverinstall "k8s.io/apiserver/pkg/apis/apiserver/install" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" )