WIP: Feature/external OIDC external sources#2065
WIP: Feature/external OIDC external sources#2065everettraven wants to merge 5 commits intoopenshift:mainfrom
Conversation
…OIDCExternalClaimsSourcing feature gate Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe changes integrate feature-gate awareness into OAuth metadata and webhook token authenticator observers. Standalone functions are converted to methods bound to new observer types with feature-gate accessors. Observers now conditionally suppress ConfigMap existence checks when the ExternalOIDCExternalClaimsSourcing feature gate is enabled. Dependencies are updated and observers are rewired in the controller. Changes
Sequence DiagramsequenceDiagram
participant Controller
participant Observer as Observer<br/>(Feature-Gate Aware)
participant FG as Feature Gate<br/>Accessor
participant Listers
participant Config
Controller->>Observer: Invoke with listers, recorder, existingConfig
Observer->>FG: Check if feature gates observed
alt Feature gates not ready
FG-->>Observer: Not observed
Observer-->>Controller: Return existingConfig, no changes
else Feature gates ready
FG-->>Observer: Return current feature gates
alt ExternalOIDCExternalClaimsSourcing enabled
Observer->>Config: Skip ConfigMap existence check
Observer->>Config: Proceed with conditional logic
else Feature gate disabled
Observer->>Listers: Check for AuthConfigCM
Listers-->>Observer: ConfigMap status
Observer->>Config: Apply original behavior
end
Observer-->>Controller: Return observedConfig, errors
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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 (2)
pkg/operator/configobservation/auth/webhook_authenticator.go (2)
113-116:⚠️ Potential issue | 🟠 MajorPropagate
SyncSecretfailures before advertising the webhook args.Both
SyncSecretcall sites ignore their error return. If the sync fails, this observer still returnsauthentication-token-webhook-*, so kube-apiserver can be reconfigured to use a secret that never made it intoopenshift-kube-apiserver.🚨 Suggested fix
- resourceSyncer.SyncSecret( + if err := resourceSyncer.SyncSecret( resourcesynccontroller.ResourceLocation{Namespace: operatorclient.TargetNamespace, Name: "webhook-authenticator"}, resourcesynccontroller.ResourceLocation{Namespace: operatorclient.GlobalUserSpecifiedConfigNamespace, Name: webhookSecretName}, - ) + ); err != nil { + return existingConfig, append(errs, err) + }Apply the same pattern to the other
SyncSecret(...)call sites in this function.Also applies to: 138-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/configobservation/auth/webhook_authenticator.go` around lines 113 - 116, The SyncSecret calls on resourceSyncer (for ResourceLocation{Namespace: operatorclient.TargetNamespace, Name: "webhook-authenticator"} -> ResourceLocation{Namespace: operatorclient.GlobalUserSpecifiedConfigNamespace, Name: webhookSecretName}) are ignoring returned errors; change both places where resourceSyncer.SyncSecret(...) is invoked to capture the error, return or propagate it (or stop and not advertise the webhook args) when non-nil, and ensure the function does not proceed to set/return authentication-token-webhook-* args if SyncSecret failed; apply the same error-check-and-propagate pattern to the other SyncSecret call site(s) in this function so failures prevent advertising the webhook secret.
90-117:⚠️ Potential issue | 🟠 MajorLimit the new gate-specific branch to OIDC auth.
Unlike
ObserveAuthMetadata, this branch is not scoped toconfigv1.AuthenticationTypeOIDC. As written, turning onExternalOIDCExternalClaimsSourcingforces all auth modes through the webhook-secret path, and non-OIDC clusters with no webhook secret configured will start failing onGet("").🎯 Suggested guard
- if featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) { + if featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) && + auth.Spec.Type == configv1.AuthenticationTypeOIDC {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/configobservation/auth/webhook_authenticator.go` around lines 90 - 117, The new feature-gate branch currently runs unconditionally and must be limited to OIDC clusters: change the condition that checks featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) to also require that existingConfig.Authentication.Type == configv1.AuthenticationTypeOIDC (or equivalent check on the observed/existing config object) so the webhook-secret path (calls to listers.ConfigSecretLister().Secrets(...).Get(webhookSecretName), validateKubeconfigSecret, and resourceSyncer.SyncSecret) only executes for OIDC auth; update logic around featureGates.Enabled, existingConfig, and the webhookSecretName/validateKubeconfigSecret usage so non-OIDC clusters are skipped and you avoid calling Get("").
🧹 Nitpick comments (2)
pkg/operator/configobservation/auth/auth_metadata_test.go (1)
317-317: Add an enabled-gate OIDC case here.This harness now goes through the feature-gated constructor, but the table still only validates the legacy OIDC flow. Please add at least one
AuthenticationTypeOIDCcase withExternalOIDCExternalClaimsSourcingenabled so we cover the new “delete oauth metadata without waiting forauth-config” path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/configobservation/auth/auth_metadata_test.go` at line 317, Add a test table entry that exercises the feature-gated OIDC path by creating an Authentication object with Type set to AuthenticationTypeOIDC and the scenario where ExternalOIDCExternalClaimsSourcing (feature gate) is enabled; call NewOAuthMetadataObserver(featuregates.NewHardcodedFeatureGateAccess(map[string]bool{auth.ExternalOIDCExternalClaimsSourcing: true}, nil))(listers, eventRecorder, tt.existingConfig) (or equivalent gate construction used elsewhere in tests) and assert the behavior that deletes oauth metadata without waiting for auth-config, mirroring the existing legacy OIDC case but with AuthenticationTypeOIDC and the feature gate enabled so the new branch is covered.pkg/operator/configobservation/auth/webhook_authenticator_test.go (1)
178-178: The new OIDC/webhook gate path still has no coverage.These cases never drive
AuthenticationSpec.TypetoOIDCand don't exercise theExternalOIDCExternalClaimsSourcingbranch, so the new behavior can regress silently. Please add one enabled-gate OIDC case and one non-OIDC case to verify the branch is correctly scoped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/configobservation/auth/webhook_authenticator_test.go` at line 178, Add two unit tests in webhook_authenticator_test.go that call NewObserveWebhookTokenAuthenticator(...) to exercise the OIDC gate path: create one test case where the existingConfig.AuthenticationSpec.Type is set to "OIDC" and the feature gate ExternalOIDCExternalClaimsSourcing is enabled via featuregates.NewHardcodedFeatureGateAccess(nil, map[string]bool{"ExternalOIDCExternalClaimsSourcing": true}) and assert the observed config and errors reflect the OIDC-branch behavior; and add a second case with AuthenticationSpec.Type set to a non-OIDC value (e.g., "Webhook") with the gate enabled or disabled to verify the branch is not taken. Ensure both cases use the same call site (NewObserveWebhookTokenAuthenticator(...)(listers, eventRecorder, tt.existingConfig)) so the ExternalOIDCExternalClaimsSourcing branch in NewObserveWebhookTokenAuthenticator is exercised and validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/operator/configobservation/auth/webhook_authenticator.go`:
- Around line 113-116: The SyncSecret calls on resourceSyncer (for
ResourceLocation{Namespace: operatorclient.TargetNamespace, Name:
"webhook-authenticator"} -> ResourceLocation{Namespace:
operatorclient.GlobalUserSpecifiedConfigNamespace, Name: webhookSecretName}) are
ignoring returned errors; change both places where
resourceSyncer.SyncSecret(...) is invoked to capture the error, return or
propagate it (or stop and not advertise the webhook args) when non-nil, and
ensure the function does not proceed to set/return
authentication-token-webhook-* args if SyncSecret failed; apply the same
error-check-and-propagate pattern to the other SyncSecret call site(s) in this
function so failures prevent advertising the webhook secret.
- Around line 90-117: The new feature-gate branch currently runs unconditionally
and must be limited to OIDC clusters: change the condition that checks
featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) to
also require that existingConfig.Authentication.Type ==
configv1.AuthenticationTypeOIDC (or equivalent check on the observed/existing
config object) so the webhook-secret path (calls to
listers.ConfigSecretLister().Secrets(...).Get(webhookSecretName),
validateKubeconfigSecret, and resourceSyncer.SyncSecret) only executes for OIDC
auth; update logic around featureGates.Enabled, existingConfig, and the
webhookSecretName/validateKubeconfigSecret usage so non-OIDC clusters are
skipped and you avoid calling Get("").
---
Nitpick comments:
In `@pkg/operator/configobservation/auth/auth_metadata_test.go`:
- Line 317: Add a test table entry that exercises the feature-gated OIDC path by
creating an Authentication object with Type set to AuthenticationTypeOIDC and
the scenario where ExternalOIDCExternalClaimsSourcing (feature gate) is enabled;
call
NewOAuthMetadataObserver(featuregates.NewHardcodedFeatureGateAccess(map[string]bool{auth.ExternalOIDCExternalClaimsSourcing:
true}, nil))(listers, eventRecorder, tt.existingConfig) (or equivalent gate
construction used elsewhere in tests) and assert the behavior that deletes oauth
metadata without waiting for auth-config, mirroring the existing legacy OIDC
case but with AuthenticationTypeOIDC and the feature gate enabled so the new
branch is covered.
In `@pkg/operator/configobservation/auth/webhook_authenticator_test.go`:
- Line 178: Add two unit tests in webhook_authenticator_test.go that call
NewObserveWebhookTokenAuthenticator(...) to exercise the OIDC gate path: create
one test case where the existingConfig.AuthenticationSpec.Type is set to "OIDC"
and the feature gate ExternalOIDCExternalClaimsSourcing is enabled via
featuregates.NewHardcodedFeatureGateAccess(nil,
map[string]bool{"ExternalOIDCExternalClaimsSourcing": true}) and assert the
observed config and errors reflect the OIDC-branch behavior; and add a second
case with AuthenticationSpec.Type set to a non-OIDC value (e.g., "Webhook") with
the gate enabled or disabled to verify the branch is not taken. Ensure both
cases use the same call site (NewObserveWebhookTokenAuthenticator(...)(listers,
eventRecorder, tt.existingConfig)) so the ExternalOIDCExternalClaimsSourcing
branch in NewObserveWebhookTokenAuthenticator is exercised and validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d7cb59e5-59b3-4443-a7f7-20caa1bf3d5f
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/AGENTS.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (7)
go.modpkg/operator/configobservation/auth/auth_metadata.gopkg/operator/configobservation/auth/auth_metadata_test.gopkg/operator/configobservation/auth/external_oidc.gopkg/operator/configobservation/auth/webhook_authenticator.gopkg/operator/configobservation/auth/webhook_authenticator_test.gopkg/operator/configobservation/configobservercontroller/observe_config_controller.go
| // When the ExternalOIDCExternalClaimsSourcing feature gate is enabled, the kube-apiserver | ||
| // should not have the built-in Structured Authentication Configuration feature configured, | ||
| // which this controller handles. | ||
| // When this feature goes GA, this controller should be removed. | ||
| if featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) { | ||
| return existingConfig, nil |
There was a problem hiding this comment.
Clear the old auth-config when external claims sourcing is enabled.
return existingConfig at Lines 66-71 keeps any previously observed apiServerArguments.authentication-config and also skips the SyncConfigMap(..., empty) cleanup path below. If a cluster enables ExternalOIDCExternalClaimsSourcing after already using external OIDC, the stale auth-config stays mounted even though this branch is supposed to disable the structured auth flow.
🧹 Suggested cleanup
if featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) {
- return existingConfig, nil
+ if err := genericListers.ResourceSyncer().SyncConfigMap(
+ resourcesynccontroller.ResourceLocation{Namespace: operatorclient.TargetNamespace, Name: AuthConfigCMName},
+ resourcesynccontroller.ResourceLocation{Namespace: "", Name: ""},
+ ); err != nil {
+ return existingConfig, []error{err}
+ }
+ return nil, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // When the ExternalOIDCExternalClaimsSourcing feature gate is enabled, the kube-apiserver | |
| // should not have the built-in Structured Authentication Configuration feature configured, | |
| // which this controller handles. | |
| // When this feature goes GA, this controller should be removed. | |
| if featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) { | |
| return existingConfig, nil | |
| // When the ExternalOIDCExternalClaimsSourcing feature gate is enabled, the kube-apiserver | |
| // should not have the built-in Structured Authentication Configuration feature configured, | |
| // which this controller handles. | |
| // When this feature goes GA, this controller should be removed. | |
| if featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) { | |
| if err := genericListers.ResourceSyncer().SyncConfigMap( | |
| resourcesynccontroller.ResourceLocation{Namespace: operatorclient.TargetNamespace, Name: AuthConfigCMName}, | |
| resourcesynccontroller.ResourceLocation{Namespace: "", Name: ""}, | |
| ); err != nil { | |
| return existingConfig, []error{err} | |
| } | |
| return nil, nil | |
| } |
…de gate is enabled and default webhook secret name Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
|
@everettraven: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary by CodeRabbit
New Features
Refactor