update: move OIDC check function from Auth service to common cluster pkg+no create real Group for OIDC #2769
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughCentralizes cluster authentication-mode detection in Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler as Reconciler / CreateAuth
participant ClusterPkg as pkg/cluster
participant K8s as Kubernetes API
participant BuildAuth as BuildDefaultAuth
Reconciler->>ClusterPkg: IsIntegratedOAuth(ctx, client)
ClusterPkg->>K8s: GET Authentication CRD
K8s-->>ClusterPkg: Auth object or NotFound/Error
ClusterPkg-->>Reconciler: true/false, error
Reconciler->>BuildAuth: BuildDefaultAuth(ctx, client, platform)
alt Integrated OAuth
BuildAuth->>K8s: Read OAuth adminGroups mapping
K8s-->>BuildAuth: admin group (if present)
BuildAuth-->>Reconciler: Auth resource with adminGroups from OAuth
else OIDC / no Integrated OAuth
BuildAuth-->>Reconciler: Auth resource with placeholder adminGroup ("REPLACE-WITH-OIDC-ADMIN-GROUP")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
zdtsw
requested review from
jctanner and
lphiri
and removed request for
MarianMacik and
grdryn
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
internal/controller/services/gateway/gateway_support_test.go (5)
41-87: Fix parallel subtests: avoid parent T reuse and range-var captureCreate a new Gomega per subtest and capture tc to prevent flaky assertions in parallel.
- t.Parallel() - g := NewWithT(t) // Create once outside the loop for better performance + t.Parallel() for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { + tc := tc + t.Run(tc.name, func(t *testing.T) { t.Parallel() - listeners := createListeners(tc.certSecret, tc.domain) + g := NewWithT(t) + listeners := createListeners(tc.certSecret, tc.domain) g.Expect(listeners).To(HaveLen(tc.expectCount), tc.description)
90-149: Repeat subtest safety pattern in TestGetCertificateType- t.Parallel() - g := NewWithT(t) + t.Parallel() for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t)
151-219: Repeat subtest safety + avoid local name shadowing of imported client package- for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { t.Parallel() - ctx := t.Context() + g := NewWithT(t) + ctx := t.Context() // Create test gateway config using helper gatewayConfig := createTestGatewayConfigSupport(tc.specDomain, nil) // Setup client with or without cluster ingress using helpers - var client client.Client + var k8sClient client.Client if tc.useClusterIngress && tc.clusterDomain != "" { - client = setupSupportTestClientWithClusterIngress(tc.clusterDomain) + k8sClient = setupSupportTestClientWithClusterIngress(tc.clusterDomain) } else { - client = setupSupportTestClient() + k8sClient = setupSupportTestClient() } - domain, err := resolveDomain(ctx, client, gatewayConfig) + domain, err := resolveDomain(ctx, k8sClient, gatewayConfig)
221-268: Apply same subtest pattern in TestCreateListenersEdgeCases- t.Parallel() - g := NewWithT(t) // Create once outside the loop for better performance + t.Parallel() for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { + tc := tc + t.Run(tc.name, func(t *testing.T) { t.Parallel() - listeners := createListeners(tc.certSecret, tc.domain) + g := NewWithT(t) + listeners := createListeners(tc.certSecret, tc.domain)
311-325: Apply same subtest pattern in TestResolveDomainEdgeCases- for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { t.Parallel() - ctx := t.Context() + g := NewWithT(t) + ctx := t.Context()internal/controller/dscinitialization/auth_test.go (1)
30-93: Add OIDC-mode test to validate placeholder groupCore PR intent is to avoid creating real groups for OIDC; please add a test asserting AdminGroups[0] == "REPLACE-WITH-OIDC-ADMIN-GROUP" when cluster auth type is OIDC.
@@ func TestBuildDefaultAuth(t *testing.T) { @@ for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) ctx := t.Context() - // Create fake client (will be Oauth mode by default) - cli, err := fakeclient.New() + // Create fake client (OAuth mode by default) + cli, err := fakeclient.New() g.Expect(err).ShouldNot(HaveOccurred()) @@ }) } + + t.Run("OIDC mode uses placeholder group", func(t *testing.T) { + g := NewWithT(t) + ctx := t.Context() + // Build a client where cluster auth type is OIDC + cli, err := fakeclient.New(fakeclient.WithClusterAuthType("OIDC")) + g.Expect(err).ShouldNot(HaveOccurred()) + obj := dscinitialization.BuildDefaultAuth(ctx, cli, cluster.OpenDataHub) + auth, ok := obj.(*serviceApi.Auth) + g.Expect(ok).To(BeTrue()) + g.Expect(auth.Spec.AdminGroups).To(HaveLen(1)) + g.Expect(auth.Spec.AdminGroups[0]).To(Equal("REPLACE-WITH-OIDC-ADMIN-GROUP")) + // AllowedGroups should remain unchanged + g.Expect(auth.Spec.AllowedGroups).To(Equal([]string{"system:authenticated"})) + })internal/controller/dscinitialization/auth.go (1)
27-33: Default to safe placeholder on detection error + introduce a named constant.If cluster auth-mode detection fails, we currently fall back to OAuth and assign a real admin group. That can grant unintended admin access on OIDC clusters with transient API/RBAC errors. Default to the OIDC placeholder on error, and define a constant to avoid magic strings.
Apply this diff:
@@ var ( @@ adminGroups = map[common.Platform]string{ cluster.SelfManagedRhoai: "rhods-admins", cluster.ManagedRhoai: "dedicated-admins", cluster.OpenDataHub: "odh-admins", } ) + +// Placeholder used when the cluster runs in OIDC mode or when the +// authentication mode cannot be determined at reconcile-time. +const PlaceholderOIDCAdminGroup = "REPLACE-WITH-OIDC-ADMIN-GROUP" @@ func BuildDefaultAuth(ctx context.Context, cli client.Client, platform common.Platform) client.Object { var adminGroup string // Detect authentication mode isOAuth, err := cluster.IsIntegratedOAuth(ctx, cli) - if err == nil && !isOAuth { - // OIDC mode: set an non-exist group that cluster admin shoulud replace with actual OIDC group. - adminGroup = "REPLACE-WITH-OIDC-ADMIN-GROUP" - } else { + // Be conservative: on error or non-OAuth, use placeholder to avoid unintended privileges. + if err != nil || !isOAuth { + // OIDC mode (or unknown): set a non-existent group to be replaced by cluster admin. + adminGroup = PlaceholderOIDCAdminGroup + } else { // OAuth mode: Use platform-specific admin group adminGroup = adminGroups[platform] if adminGroup == "" { // fallback to OpenDataHub admin group. adminGroup = adminGroups[cluster.OpenDataHub] } }Also applies to: 71-86
🧹 Nitpick comments (9)
README.md (1)
121-164: Documentation clearly explains OIDC Auth configuration and placeholder mechanism.The new section effectively communicates:
- The need to replace the placeholder
REPLACE-WITH-OIDC-ADMIN-GROUPwith actual OIDC groups- The default Auth CR structure for OIDC mode
- Key differences between OIDC and OpenShift OAuth modes
- RBAC mappings for admin and allowed groups
The documentation aligns well with the PR's goal of using a placeholder to prevent accidental reuse of existing cluster groups.
Optional enhancement (nice-to-have): Consider adding a brief note explaining that the placeholder pattern explicitly prevents mistaken use of pre-existing cluster groups like
odh-admin, or clarify whetherallowedGroupsis required or can be omitted. These additions would help administrators understand the design rationale.internal/controller/services/auth/auth_controller_actions.go (1)
182-189: Switch to cluster.IsIntegratedOAuth looks good; tweak log for clarityThe guard correctly skips group creation outside Integrated OAuth. Please adjust the log message to reflect the new semantic.
- if !ok { - logf.Log.Info("default auth method is not enabled") + if !ok { + logf.Log.Info("Integrated OAuth not detected; skipping default group creation") return nil }internal/controller/services/gateway/gateway_controller.go (1)
90-94: Also guard OAuthClient ownership by CRD existenceAvoid controller startup errors if OAuthClient CRD is absent but Integrated OAuth is reported. Align with EnvoyFilter/DestinationRule pattern.
- // Only watch OAuthClient if cluster uses IntegratedOAuth (not OIDC or None) - // This prevents errors in ROSA environments where OAuthClient CRD doesn't exist - if isIntegratedOAuth, err := cluster.IsIntegratedOAuth(ctx, mgr.GetClient()); err == nil && isIntegratedOAuth { - reconcilerBuilder = reconcilerBuilder.OwnsGVK(gvk.OAuthClient) // OpenShift OAuth integration - } + // Only watch OAuthClient if IntegratedOAuth AND CRD exists (prevents errors on environments without CRD) + if isIntegratedOAuth, err := cluster.IsIntegratedOAuth(ctx, mgr.GetClient()); err == nil && isIntegratedOAuth { + reconcilerBuilder = reconcilerBuilder. + OwnsGVK(gvk.OAuthClient, reconciler.Dynamic(reconciler.CrdExists(gvk.OAuthClient))) + }internal/controller/services/gateway/gateway_auth_actions.go (1)
33-41: Good move to cluster.GetClusterAuthenticationMode; keep error contextLooks correct. Consider treating NotFound (missing Authentication CRD) as non-fatal if you want gateway to run on clusters lacking that CRD.
- authMode, err := cluster.GetClusterAuthenticationMode(ctx, rr.Client) - if err != nil { + authMode, err := cluster.GetClusterAuthenticationMode(ctx, rr.Client) + if err != nil { return fmt.Errorf("failed to detect cluster authentication mode: %w", err) }pkg/cluster/cluster_config_test.go (1)
379-386: Minor: prefer meta.IsNoMatchError in production code; tests can stay as-isNo change required here, but see suggested fix in cluster_config.go for robustness.
internal/controller/dscinitialization/auth.go (2)
61-63: Tighten wording and fix typos in comments.Correct “shoulud” → “should”; clarify placeholder guidance.
@@ -// For OIDC mode, uses a placeholder that cluster admin must replace with actual OIDC group names. +// For OIDC mode, uses a placeholder that cluster admins must replace with actual OIDC group names. @@ - // OIDC mode: set an non-exist group that cluster admin shoulud replace with actual OIDC group. - adminGroup = "REPLACE-WITH-OIDC-ADMIN-GROUP" + // OIDC mode: set a non-existent group that cluster admins should replace with an actual OIDC group. + adminGroup = PlaceholderOIDCAdminGroupAlso applies to: 78-80
71-71: Optional: return concrete type for clarity.BuildDefaultAuth always returns *serviceApi.Auth; consider returning that type instead of client.Object to improve readability and reduce casting at call sites.
Do any callers rely on the broader client.Object type? If not, we can narrow it safely.
internal/controller/services/gateway/gateway_support.go (2)
317-351: Harden OIDC config validation (trim input and enforce https issuer).Catch whitespace-only values and non-HTTPS issuers early; keeps status messages precise.
func validateOIDCConfig(authMode cluster.AuthenticationMode, oidcConfig *serviceApi.OIDCConfig) *common.Condition { if authMode != cluster.AuthModeOIDC { return nil } @@ - if oidcConfig == nil { + if oidcConfig == nil { condition.Message = status.AuthProxyOIDCModeWithoutConfigMessage return condition } - var validationErrors []string + var validationErrors []string - if oidcConfig.ClientID == "" { + clientID := strings.TrimSpace(oidcConfig.ClientID) + issuer := strings.TrimSpace(oidcConfig.IssuerURL) + + if clientID == "" { validationErrors = append(validationErrors, status.AuthProxyOIDCClientIDEmptyMessage) } - if oidcConfig.IssuerURL == "" { + if issuer == "" { validationErrors = append(validationErrors, status.AuthProxyOIDCIssuerURLEmptyMessage) } + if issuer != "" && !strings.HasPrefix(issuer, "https://") { + validationErrors = append(validationErrors, "OIDC issuerURL must use https scheme") + } if oidcConfig.ClientSecretRef.Name == "" { validationErrors = append(validationErrors, status.AuthProxyOIDCSecretRefNameEmptyMessage) }
366-403: Self-heal when secret exists but is missing keys (avoid hard error).If kube-auth-proxy secret exists but lacks keys, we can regenerate values instead of failing reconciliation. This reduces flakiness and auto-recovers partial secrets.
func getOrGenerateSecrets(ctx context.Context, rr *odhtypes.ReconciliationRequest, authMode cluster.AuthenticationMode) (string, string, error) { existingSecret := &corev1.Secret{} secretErr := rr.Client.Get(ctx, types.NamespacedName{ Name: KubeAuthProxySecretsName, Namespace: GatewayNamespace, }, existingSecret) - if secretErr == nil { - clientSecretBytes, hasClientSecret := existingSecret.Data[EnvClientSecret] - cookieSecretBytes, hasCookieSecret := existingSecret.Data[EnvCookieSecret] - - if !hasClientSecret || !hasCookieSecret { - return "", "", errors.New("existing secret missing required keys") - } - - return string(clientSecretBytes), string(cookieSecretBytes), nil - } - - if !k8serr.IsNotFound(secretErr) { + if secretErr == nil { + clientSecretBytes, hasClientSecret := existingSecret.Data[EnvClientSecret] + cookieSecretBytes, hasCookieSecret := existingSecret.Data[EnvCookieSecret] + if hasClientSecret && hasCookieSecret { + return string(clientSecretBytes), string(cookieSecretBytes), nil + } + // Missing keys: fall through to regeneration below. + } else if !k8serr.IsNotFound(secretErr) { return "", "", fmt.Errorf("failed to check for existing secret: %w", secretErr) } var clientSecretValue string if authMode == cluster.AuthModeIntegratedOAuth { clientSecretGen, err := secretgenerator.NewSecret("client-secret", "random", ClientSecretLength) if err != nil { return "", "", fmt.Errorf("failed to generate client secret: %w", err) } clientSecretValue = clientSecretGen.Value } cookieSecretGen, err := secretgenerator.NewSecret("cookie-secret", "random", CookieSecretLength) if err != nil { return "", "", fmt.Errorf("failed to generate cookie secret: %w", err) } return clientSecretValue, cookieSecretGen.Value, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
README.md(2 hunks)internal/controller/dscinitialization/auth.go(1 hunks)internal/controller/dscinitialization/auth_test.go(1 hunks)internal/controller/services/auth/auth.go(0 hunks)internal/controller/services/auth/auth_controller_actions.go(1 hunks)internal/controller/services/auth/auth_controller_test.go(0 hunks)internal/controller/services/gateway/gateway_auth_actions.go(4 hunks)internal/controller/services/gateway/gateway_controller.go(2 hunks)internal/controller/services/gateway/gateway_support.go(4 hunks)internal/controller/services/gateway/gateway_support_test.go(6 hunks)pkg/cluster/cluster_config.go(3 hunks)pkg/cluster/cluster_config_test.go(5 hunks)
💤 Files with no reviewable changes (2)
- internal/controller/services/auth/auth_controller_test.go
- internal/controller/services/auth/auth.go
🧰 Additional context used
🧬 Code graph analysis (9)
internal/controller/services/gateway/gateway_controller.go (1)
pkg/cluster/cluster_config.go (1)
IsIntegratedOAuth(403-409)
internal/controller/services/auth/auth_controller_actions.go (1)
pkg/cluster/cluster_config.go (1)
IsIntegratedOAuth(403-409)
pkg/cluster/cluster_config_test.go (2)
pkg/cluster/cluster_config.go (2)
IsFipsEnabled(336-362)IsIntegratedOAuth(403-409)pkg/cluster/const.go (1)
ClusterAuthenticationObj(24-24)
internal/controller/dscinitialization/auth_test.go (1)
internal/controller/dscinitialization/auth.go (1)
BuildDefaultAuth(71-96)
internal/controller/services/gateway/gateway_support.go (3)
pkg/cluster/cluster_config.go (4)
AuthenticationMode(47-47)AuthModeOIDC(51-51)AuthModeNone(52-52)AuthModeIntegratedOAuth(50-50)api/services/v1alpha1/gateway_types.go (1)
OIDCConfig(58-70)api/common/types.go (1)
Condition(36-94)
internal/controller/services/gateway/gateway_auth_actions.go (1)
pkg/cluster/cluster_config.go (3)
GetClusterAuthenticationMode(379-400)AuthModeOIDC(51-51)AuthModeIntegratedOAuth(50-50)
pkg/cluster/cluster_config.go (2)
pkg/cluster/const.go (1)
ClusterAuthenticationObj(24-24)pkg/cluster/gvk/gvk.go (2)
Group(124-128)Auth(358-362)
internal/controller/dscinitialization/auth.go (2)
pkg/cluster/cluster_config.go (1)
IsIntegratedOAuth(403-409)pkg/cluster/const.go (1)
OpenDataHub(11-11)
internal/controller/services/gateway/gateway_support_test.go (2)
pkg/cluster/cluster_config.go (4)
AuthenticationMode(47-47)AuthModeOIDC(51-51)AuthModeIntegratedOAuth(50-50)AuthModeNone(52-52)api/services/v1alpha1/gateway_types.go (1)
OIDCConfig(58-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build/push catalog image
- GitHub Check: Run tests and collect coverage on internal and pkg
- GitHub Check: golangci-lint
🔇 Additional comments (7)
README.md (1)
14-14: Table of contents entry added appropriately.The new
OIDC Authentication Configurationentry is correctly positioned in the Configuration section.internal/controller/services/gateway/gateway_auth_actions.go (1)
62-66: Conditional OAuthClient creation is correctCreation gated on Integrated OAuth; matches controller-level watch change.
pkg/cluster/cluster_config_test.go (1)
219-339: Solid coverage for IsIntegratedOAuthCovers IntegratedOAuth/empty/OIDC/None/custom/missing. LGTM.
pkg/cluster/cluster_config.go (1)
402-409: IsIntegratedOAuth wrapper looks goodSimple delegate with clear semantics.
internal/controller/dscinitialization/auth.go (2)
54-56: LGTM: CreateAuth now propagates ctx/cli to BuildDefaultAuth.Passing ctx and r.Client aligns with centralized cluster auth detection and reduces duplication. No issues.
34-58: Code review verification complete — no issues detected.The repo-wide sanity checks confirm the implementation is sound:
BuildDefaultAuthcalls are properly located (2 call sites + definition)- No straggler uses of legacy AuthMode patterns
- Placeholder
REPLACE-WITH-OIDC-ADMIN-GROUPis correctly used for OIDC mode inauth.go:79- Admin groups are cleanly mapped by platform (
rhods-admins,dedicated-admins,odh-admins) and consistently applied throughout the codebaseinternal/controller/services/gateway/gateway_support.go (1)
353-363: LGTM: clear “AuthModeNone” handling.Condition correctly blocks proxy deployment when the cluster uses external auth. Message is explicit.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/utils/test/fakeclient/fakeclient.go (2)
84-106: Fix REST scope: OpenShift Authentication is cluster-scoped, not namespaced.The fake RESTMapper currently defaults unknown GVKs to namespaced, which mis-scopes config.openshift.io/v1 Authentication. This can break Get() calls for the cluster-scoped "cluster" object during auth-mode detection.
Apply this patch to explicitly map Authentication as RESTScopeRoot:
for kt := range s.AllKnownTypes() { switch kt { + // OpenShift config (cluster-scoped) + case configv1.SchemeGroupVersion.WithKind("Authentication"): + fakeMapper.Add(kt, meta.RESTScopeRoot) // k8s case gvk.CustomResourceDefinition: fakeMapper.Add(kt, meta.RESTScopeRoot)
68-76: Ensure OpenShift config types are registered in the scheme used by the fake client.If a custom scheme is provided via WithScheme, it may not include configv1. Safely register it here to avoid runtime type/mapper issues.
s := co.scheme if s == nil { newScheme, err := scheme.New() if err != nil { return nil, fmt.Errorf("unable to create default scheme: %w", err) } s = newScheme } + // Ensure OpenShift config types are present (no-op if already registered) + if err := configv1.AddToScheme(s); err != nil { + return nil, fmt.Errorf("unable to add OpenShift config types to scheme: %w", err) + }
🧹 Nitpick comments (8)
pkg/utils/test/fakeclient/fakeclient.go (1)
45-61: Nice helper; consider guarding invalid inputs (optional).Optionally validate authType against the known modes and log/return a no-op if it’s unexpected, to fail fast in tests.
pkg/cluster/cluster_config_test.go (2)
309-336: Enable subtest parallelism for faster runs (optional).Add t.Parallel() inside the IsIntegratedOAuth subtests to mirror the FIPS pattern.
t.Run(tt.name, func(t *testing.T) { + t.Parallel() g := NewWithT(t)
314-327: Optional: reuse shared fake client helper for auth types.You could simplify client setup using pkg/utils/test/fakeclient.WithClusterAuthType, reducing boilerplate in each case. Not required.
internal/controller/dscinitialization/auth_test.go (3)
93-105: Good OIDC coverage; prefer exported const to avoid string drift.Promote the placeholder to an exported const and reference it here to keep tests and code in sync.
Test change:
- g.Expect(auth.Spec.AdminGroups[0]).To(Equal("REPLACE-WITH-OIDC-ADMIN-GROUP")) + g.Expect(auth.Spec.AdminGroups[0]).To(Equal(dscinitialization.OIDCPlaceholderAdminGroup))Pair with the auth.go change below.
63-91: Parallelize and tiny nit.
- Consider t.Parallel() in each subtest for TestBuildDefaultAuth.
- Nit: “Oauth” → “OAuth” in the comment at Line 68.
108-201: Add a CreateAuth test for OIDC mode (optional).Include a case using fakeclient.WithClusterAuthType(cluster.AuthModeOIDC) to assert the placeholder is set when CreateAuth constructs the CR.
internal/controller/dscinitialization/auth.go (2)
60-86: Export a placeholder constant and use it here.Avoid hardcoding the placeholder in multiple places.
package dscinitialization import ( "context" @@ var ( // adminGroups maps each supported platform to its default admin group name. @@ } ) + +// OIDCPlaceholderAdminGroup is used in OIDC mode to avoid creating/assuming a real admin group. +// Cluster admins must replace this with their actual IdP group. +const OIDCPlaceholderAdminGroup = "REPLACE-WITH-OIDC-ADMIN-GROUP" @@ - if err == nil && !isOAuth { - // OIDC mode: set an non-exist group that cluster admin should replace with actual OIDC group. - adminGroup = "REPLACE-WITH-OIDC-ADMIN-GROUP" + if err == nil && !isOAuth { + // OIDC mode: set a non-existent group that cluster admin should replace with actual OIDC group. + adminGroup = OIDCPlaceholderAdminGroup
74-83: Optional: log detection errors before defaulting to OAuth.When auth-mode detection errors, you fall back to OAuth silently. Consider logging at info/debug to aid operators.
Example:
- if err != nil { log.Info("auth mode detection failed; assuming OAuth", "error", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.md(2 hunks)internal/controller/dscinitialization/auth.go(1 hunks)internal/controller/dscinitialization/auth_test.go(2 hunks)internal/controller/services/auth/auth.go(0 hunks)internal/controller/services/auth/auth_controller_actions.go(1 hunks)internal/controller/services/auth/auth_controller_test.go(0 hunks)internal/controller/services/gateway/gateway_auth_actions.go(4 hunks)internal/controller/services/gateway/gateway_controller.go(2 hunks)internal/controller/services/gateway/gateway_support.go(4 hunks)internal/controller/services/gateway/gateway_support_test.go(6 hunks)pkg/cluster/cluster_config.go(3 hunks)pkg/cluster/cluster_config_test.go(5 hunks)pkg/utils/test/fakeclient/fakeclient.go(2 hunks)
💤 Files with no reviewable changes (2)
- internal/controller/services/auth/auth_controller_test.go
- internal/controller/services/auth/auth.go
🚧 Files skipped from review as they are similar to previous changes (6)
- README.md
- internal/controller/services/gateway/gateway_auth_actions.go
- internal/controller/services/gateway/gateway_support_test.go
- internal/controller/services/gateway/gateway_controller.go
- internal/controller/services/auth/auth_controller_actions.go
- pkg/cluster/cluster_config.go
🧰 Additional context used
🧬 Code graph analysis (5)
pkg/utils/test/fakeclient/fakeclient.go (2)
pkg/cluster/cluster_config.go (1)
AuthenticationMode(47-47)pkg/cluster/const.go (1)
ClusterAuthenticationObj(24-24)
pkg/cluster/cluster_config_test.go (2)
pkg/cluster/cluster_config.go (2)
IsFipsEnabled(336-362)IsIntegratedOAuth(406-412)pkg/cluster/const.go (1)
ClusterAuthenticationObj(24-24)
internal/controller/dscinitialization/auth.go (2)
pkg/cluster/cluster_config.go (1)
IsIntegratedOAuth(406-412)pkg/cluster/const.go (1)
OpenDataHub(11-11)
internal/controller/services/gateway/gateway_support.go (4)
pkg/cluster/cluster_config.go (4)
AuthenticationMode(47-47)AuthModeOIDC(51-51)AuthModeNone(52-52)AuthModeIntegratedOAuth(50-50)api/services/v1alpha1/gateway_types.go (1)
OIDCConfig(58-70)api/common/types.go (1)
Condition(36-94)pkg/controller/types/types.go (1)
ReconciliationRequest(75-112)
internal/controller/dscinitialization/auth_test.go (5)
pkg/utils/test/fakeclient/fakeclient.go (2)
New(62-115)WithClusterAuthType(48-60)internal/controller/dscinitialization/auth.go (1)
BuildDefaultAuth(71-96)pkg/cluster/cluster_config.go (1)
AuthModeOIDC(51-51)pkg/cluster/const.go (1)
OpenDataHub(11-11)api/services/v1alpha1/auth_types.go (1)
Auth(57-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build/push catalog image
- GitHub Check: golangci-lint
- GitHub Check: Run tests and collect coverage on internal and pkg
🔇 Additional comments (6)
pkg/cluster/cluster_config_test.go (1)
37-152: FIPS tests look solid.Good coverage: enabled/disabled, key missing, invalid YAML fallbacks, empty config, not found, generic error. Assertions read clearly.
internal/controller/dscinitialization/auth.go (1)
54-56: Create path is correct and handles AlreadyExists.The idempotent create with IsAlreadyExists guard is good for concurrent reconciles.
internal/controller/services/gateway/gateway_support.go (4)
317-403: Migration to cluster.AuthenticationMode successfully completed.The verification confirms that the old local
AuthModetype definition and all old constant definitions have been fully removed from the codebase. All references now properly usecluster.AuthenticationMode. ThedetectClusterAuthModefunction has also been removed as expected. No stale references remain.
317-351: All callers successfully updated to usecluster.AuthenticationMode.Verification confirms:
- Test case passes
cluster.AuthModeNoneandcluster.AuthModeOIDCconstants- Production call in
gateway_auth_actions.goobtainsauthModefromcluster.GetClusterAuthenticationMode()and correctly passes it to bothvalidateOIDCConfigandcheckAuthModeNone- No references to old local
AuthModetype remain; all usecluster.AuthenticationMode
366-403: LGTM! Code correctly handles empty clientSecret for OIDC mode.The function signature has been correctly updated to use
cluster.AuthenticationMode. The logic properly generatesclientSecretonly forIntegratedOAuthmode, returning an empty string for OIDC mode. This empty string is intentionally handled:createKubeAuthProxySecretoverrides it with the actual OIDC client secret retrieved from the OIDC config reference (line 499), andcreateOAuthClientis only invoked forIntegratedOAuthmode, never for OIDC.
353-363: Code changes verified as correct.All callers have been properly updated. The
cluster.GetClusterAuthenticationMode()function returnscluster.AuthenticationMode, which is correctly passed tocheckAuthModeNone(). The test case structure also correctly definesauthModeascluster.AuthenticationMode, and all constant references use thecluster.package prefix.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2769 +/- ##
==========================================
+ Coverage 49.87% 50.01% +0.13%
==========================================
Files 145 144 -1
Lines 10394 10401 +7
==========================================
+ Hits 5184 5202 +18
+ Misses 4656 4645 -11
Partials 554 554 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- remove type AuthMode in Gateway, use the one in cluster - update Gateway - move unit-test and change to use gomega - update: fips unit test move to use gomega - add unit test on the OIDC part Signed-off-by: Wen Zhou <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
pkg/cluster/cluster_config.go (2)
46-53: Public auth-mode API looks good; consider documenting “unknown/custom” behavior.Define what callers should expect for custom/unknown types (e.g., treated as None) in a brief comment to avoid misuses down the line.
378-403: Map unknown/custom types and aid troubleshooting with a debug log.Logic is correct, including empty → IntegratedOAuth and NoMatch → NotFound. Add a low‑verbosity log when encountering an unknown type to help support triage.
default: - // Custom/unknown auth types are not IntegratedOAuth + // Custom/unknown auth types are not IntegratedOAuth + logf.FromContext(ctx).V(1).Info("unknown authentication type; treating as None", + "type", string(auth.Spec.Type)) return AuthModeNone, nil }pkg/utils/test/fakeclient/fakeclient.go (1)
84-106: Register Authentication (config.openshift.io) as cluster‑scoped in fake RESTMapper.Authentication is cluster‑scoped; current default branch marks unknowns namespaced. This can cause subtle test issues (wrong scope routes). Explicitly scope it to root.
for _, o := range co.objects { if err := resources.EnsureGroupVersionKind(s, o); err != nil { return nil, err } } fakeMapper := meta.NewDefaultRESTMapper(s.PreferredVersionAllGroups()) for kt := range s.AllKnownTypes() { + // Ensure OpenShift Authentication is marked cluster-scoped + if kt.Group == configv1.GroupName && kt.Kind == "Authentication" { + fakeMapper.Add(kt, meta.RESTScopeRoot) + continue + } switch kt { // k8s case gvk.CustomResourceDefinition: fakeMapper.Add(kt, meta.RESTScopeRoot)pkg/cluster/cluster_config_test.go (1)
219-339: Good coverage; consider using fakeclient helper and add a CRD‑missing (NoMatch) case.
- Replace hand‑built clients with pkg/utils/test/fakeclient.WithClusterAuthType for brevity/consistency.
- Add a case that triggers meta.NoMatch (missing CRD) to validate NotFound mapping.
- scheme := runtime.NewScheme() - err := configv1.AddToScheme(scheme) - g.Expect(err).ShouldNot(HaveOccurred()) - if tt.authObject != nil { - fakeClient = fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(tt.authObject).Build() - } else { - fakeClient = fake.NewClientBuilder().WithScheme(scheme).Build() - } + fc, err := fakeclient.New( + fakeclient.WithScheme(scheme.NewMust()), // repo’s test scheme + ) + g.Expect(err).ShouldNot(HaveOccurred()) + if tt.authObject != nil { + // or: fakeclient.WithClusterAuthType(cluster.AuthenticationMode(tt.authObject.Spec.Type)) + fc, _ = fakeclient.New(fakeclient.WithClusterAuthType(cluster.AuthenticationMode(tt.authObject.Spec.Type))) + } + fakeClient = fc(Adjust imports accordingly.)
internal/controller/services/gateway/gateway_auth_actions.go (1)
33-41: Centralizing auth‑mode detection is the right move; add a debug log of the selected mode.Helps troubleshooting without altering flow.
- authMode, err := cluster.GetClusterAuthenticationMode(ctx, rr.Client) + authMode, err := cluster.GetClusterAuthenticationMode(ctx, rr.Client) if err != nil { return fmt.Errorf("failed to detect cluster authentication mode: %w", err) } + logf.FromContext(ctx).V(1).Info("detected cluster auth mode", "mode", authMode)internal/controller/services/gateway/gateway_controller.go (1)
92-94: Also gate by CRD existence and log detection errors.Avoid silent skips and double‑guard ownership on environments lacking OAuthClient.
- if isIntegratedOAuth, err := cluster.IsIntegratedOAuth(ctx, mgr.GetClient()); err == nil && isIntegratedOAuth { - reconcilerBuilder = reconcilerBuilder.OwnsGVK(gvk.OAuthClient) // OpenShift OAuth integration - } + if isIntegratedOAuth, err := cluster.IsIntegratedOAuth(ctx, mgr.GetClient()); err == nil && isIntegratedOAuth { + reconcilerBuilder = reconcilerBuilder. + OwnsGVK(gvk.OAuthClient, + reconciler.Dynamic(reconciler.CrdExists(gvk.OAuthClient))) // guard on CRD too + } else if err != nil { + // keep building; just surface signal for operators + ctrl.Log.V(1).Info("skipping OAuthClient ownership; could not determine integrated OAuth", "error", err) + }internal/controller/dscinitialization/auth.go (1)
71-86: Consider improving error handling and extracting the placeholder constant.The implementation correctly detects authentication mode and applies appropriate admin group configuration. However, there are a few concerns:
Line 75-77: If
cluster.IsIntegratedOAuthreturns an error, it's silently ignored and the code defaults to OAuth behavior. Consider logging the error or adding a comment explaining this fallback strategy.Line 78: Grammar issue - "set an non-exist group" should be "set a non-existent group".
Line 79: The placeholder string
"REPLACE-WITH-OIDC-ADMIN-GROUP"is hardcoded. Consider extracting it as a constant (e.g.,const OIDCPlaceholderAdminGroup = "REPLACE-WITH-OIDC-ADMIN-GROUP") to prevent typos and improve maintainability.Apply this diff to address the grammar issue:
- // OIDC mode: set an non-exist group that cluster admin should replace with actual OIDC group. + // OIDC mode: set a non-existent group that cluster admin should replace with actual OIDC group.Consider extracting the placeholder as a constant at the package level:
const ( // OIDCPlaceholderAdminGroup is a placeholder value used in Auth CR when cluster uses OIDC. // Cluster administrators must replace this with their actual OIDC admin group name. OIDCPlaceholderAdminGroup = "REPLACE-WITH-OIDC-ADMIN-GROUP" )Then use it:
- adminGroup = "REPLACE-WITH-OIDC-ADMIN-GROUP" + adminGroup = OIDCPlaceholderAdminGroup
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.md(2 hunks)internal/controller/dscinitialization/auth.go(1 hunks)internal/controller/dscinitialization/auth_test.go(2 hunks)internal/controller/services/auth/auth.go(0 hunks)internal/controller/services/auth/auth_controller_actions.go(1 hunks)internal/controller/services/auth/auth_controller_test.go(0 hunks)internal/controller/services/gateway/gateway_auth_actions.go(4 hunks)internal/controller/services/gateway/gateway_controller.go(2 hunks)internal/controller/services/gateway/gateway_support.go(4 hunks)internal/controller/services/gateway/gateway_support_test.go(6 hunks)pkg/cluster/cluster_config.go(3 hunks)pkg/cluster/cluster_config_test.go(5 hunks)pkg/utils/test/fakeclient/fakeclient.go(2 hunks)
💤 Files with no reviewable changes (2)
- internal/controller/services/auth/auth.go
- internal/controller/services/auth/auth_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- internal/controller/services/auth/auth_controller_actions.go
🧰 Additional context used
🧬 Code graph analysis (9)
internal/controller/services/gateway/gateway_auth_actions.go (1)
pkg/cluster/cluster_config.go (3)
GetClusterAuthenticationMode(379-403)AuthModeOIDC(51-51)AuthModeIntegratedOAuth(50-50)
internal/controller/services/gateway/gateway_controller.go (1)
pkg/cluster/cluster_config.go (1)
IsIntegratedOAuth(406-412)
internal/controller/dscinitialization/auth.go (2)
pkg/cluster/cluster_config.go (1)
IsIntegratedOAuth(406-412)pkg/cluster/const.go (1)
OpenDataHub(11-11)
pkg/cluster/cluster_config_test.go (2)
pkg/cluster/cluster_config.go (2)
IsFipsEnabled(336-362)IsIntegratedOAuth(406-412)pkg/cluster/const.go (1)
ClusterAuthenticationObj(24-24)
pkg/cluster/cluster_config.go (1)
pkg/cluster/const.go (1)
ClusterAuthenticationObj(24-24)
pkg/utils/test/fakeclient/fakeclient.go (2)
pkg/cluster/cluster_config.go (1)
AuthenticationMode(47-47)pkg/cluster/const.go (1)
ClusterAuthenticationObj(24-24)
internal/controller/dscinitialization/auth_test.go (4)
pkg/utils/test/fakeclient/fakeclient.go (2)
New(62-115)WithClusterAuthType(48-60)internal/controller/dscinitialization/auth.go (1)
BuildDefaultAuth(71-96)pkg/cluster/cluster_config.go (1)
AuthModeOIDC(51-51)pkg/cluster/const.go (1)
OpenDataHub(11-11)
internal/controller/services/gateway/gateway_support.go (1)
pkg/cluster/cluster_config.go (4)
AuthenticationMode(47-47)AuthModeOIDC(51-51)AuthModeNone(52-52)AuthModeIntegratedOAuth(50-50)
internal/controller/services/gateway/gateway_support_test.go (2)
pkg/cluster/cluster_config.go (4)
AuthenticationMode(47-47)AuthModeOIDC(51-51)AuthModeIntegratedOAuth(50-50)AuthModeNone(52-52)api/services/v1alpha1/gateway_types.go (1)
OIDCConfig(58-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build/push catalog image
- GitHub Check: golangci-lint
- GitHub Check: Run tests and collect coverage on internal and pkg
🔇 Additional comments (10)
pkg/cluster/cluster_config.go (1)
405-412: Helper is concise and correct.Thin wrapper over GetClusterAuthenticationMode; good reuse.
pkg/cluster/cluster_config_test.go (1)
37-196: FIPS tests are thorough and parallelized correctly.Good coverage including YAML‑fallback branch and error passthrough.
internal/controller/services/gateway/gateway_auth_actions.go (1)
62-66: Correctly gate OAuthClient creation to IntegratedOAuth only.Prevents ROSA/no‑OAuthClient CRD failures.
internal/controller/services/gateway/gateway_support_test.go (1)
18-18: LGTM! Clean migration to centralized authentication types.The test file has been properly updated to use
cluster.AuthenticationModeand its associated constants (cluster.AuthModeOIDC,cluster.AuthModeIntegratedOAuth,cluster.AuthModeNone) from the cluster package. The test logic remains unchanged, confirming that this is a pure refactoring to centralize type definitions.Also applies to: 333-447
internal/controller/dscinitialization/auth_test.go (2)
66-72: LGTM! Tests properly updated for new BuildDefaultAuth signature.The test correctly creates a context and fake client to match the updated
BuildDefaultAuth(ctx, cli, platform)signature. The fake client defaults to OAuth mode when no specific auth type is configured.
93-105: Excellent test coverage for OIDC placeholder behavior.This new test case properly validates that when the cluster uses OIDC authentication mode, the BuildDefaultAuth function returns the placeholder admin group
REPLACE-WITH-OIDC-ADMIN-GROUPinstead of a platform-specific group. The test correctly usesfakeclient.WithClusterAuthType(cluster.AuthModeOIDC)to simulate OIDC mode.internal/controller/dscinitialization/auth.go (1)
54-54: LGTM! Call site properly updated.The CreateAuth method now correctly passes
ctxandr.Clientto match the updated BuildDefaultAuth signature.internal/controller/services/gateway/gateway_support.go (3)
316-350: LGTM! Function signature and logic properly migrated.The
validateOIDCConfigfunction now usescluster.AuthenticationModeand compares againstcluster.AuthModeOIDC. The validation logic remains intact and correct.
352-362: LGTM! Authentication mode check updated correctly.The
checkAuthModeNonefunction properly usescluster.AuthenticationModeand compares againstcluster.AuthModeNone. The condition message appropriately indicates that external authentication is in use.
365-402: LGTM! Secret generation logic properly updated.The
getOrGenerateSecretsfunction correctly usescluster.AuthenticationModeand conditionally generates the client secret only whenauthMode == cluster.AuthModeIntegratedOAuth. This aligns with the OIDC workflow where the client secret comes from an external OIDC provider secret reference.
| // WithClusterAuthType creates a fake OpenShift Authentication object with the specified type. | ||
| // This is useful for testing authentication mode detection logic. | ||
| // authType should be one of cluster.AuthModeOIDC, cluster.AuthModeIntegratedOAuth, or cluster.AuthModeNone constants. | ||
| func WithClusterAuthType(authType cluster.AuthenticationMode) ClientOpts { | ||
| return func(o *clientOptions) { | ||
| auth := &configv1.Authentication{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: cluster.ClusterAuthenticationObj, | ||
| }, | ||
| Spec: configv1.AuthenticationSpec{ | ||
| Type: configv1.AuthenticationType(authType), | ||
| }, | ||
| } | ||
| o.objects = append(o.objects, auth) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Nice test helper; add a quick guardrail in docs and ensure scheme includes configv1.
- The helper is clean. Add a note that the test scheme must register configv1.
- See script below to verify test scheme registration.
🏁 Script executed:
#!/bin/bash
# Verify test scheme registers OpenShift config/v1
set -euo pipefail
echo "Searching for configv1.AddToScheme in test schemes..."
rg -nP --glob 'pkg/**' 'configv1\.AddToScheme' -C2 || true
echo
echo "Locating scheme.New implementations..."
rg -nP --glob 'pkg/**' '^func\s+New\(' -C3 || trueLength of output: 3138
---
🏁 Script executed:
```shell
#!/bin/bash
# Inspect the scheme.go file to see DefaultAddToSchemes definition
cat pkg/utils/test/scheme/scheme.go
Length of output: 2012
---
🏁 Script executed:
```shell
#!/bin/bash
# Check how fakeclient.New() initializes the scheme
sed -n '62,100p' pkg/utils/test/fakeclient/fakeclient.go
Length of output: 996
Add configv1.AddToScheme to the test scheme's DefaultAddToSchemes list.
The WithClusterAuthType helper creates configv1.Authentication objects, but pkg/utils/test/scheme/scheme.go does not register configv1. When the fake client uses the default scheme, these objects won't deserialize correctly. Either:
- Add
configv1.AddToSchemetoDefaultAddToSchemesinpkg/utils/test/scheme/scheme.go, or - Add a doc comment requiring callers to manually register
configv1if using the default scheme.
Option 1 is simpler since other OpenShift APIs (oauth, operator, route) are already registered there.
🤖 Prompt for AI Agents
In pkg/utils/test/fakeclient/fakeclient.go around lines 45 to 61, the helper
creates configv1.Authentication objects but the test scheme doesn't register
configv1; update pkg/utils/test/scheme/scheme.go to add configv1.AddToScheme to
the DefaultAddToSchemes slice (next to existing oauth/operator/route
registrations) so the fake client can serialize/deserialize Authentication
objects correctly; alternatively, if you prefer not to modify the scheme, add a
doc comment to that file instructing callers to register configv1 manually, but
the preferred fix is to register configv1.AddToScheme in DefaultAddToSchemes.
1 participant
REPLACE-WITH-OIDC-ADMIN-GROUPfor groups in Auth CR spec, since it does not accept "" or no .spec.admingroup. this is to prevernt if odh-admin group exists in the cluster will be used as ODH admin mistakenlyAuthenticationModefrom cluster pkgDescription
How Has This Been Tested?
Screenshot or short clip
Merge criteria
E2E test suite update requirement
When bringing new changes to the operator code, such changes are by default required to be accompanied by extending and/or updating the E2E test suite accordingly.
To opt-out of this requirement:
E2E update requirement opt-out justificationsection belowE2E update requirement opt-out justification
in current e2e we do not have ability to test oidc, therefore to test new placeholder will never happen.
Summary by CodeRabbit
Documentation
New Features
Improvements
Tests