CNTRLPLANE-2991: Do not configure KAS with Structured Authentication when authentication type is OIDC#2075
Conversation
…ng gate Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
|
@everettraven: This pull request references CNTRLPLANE-2991 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
WalkthroughConvert several auth observers into feature-gate-aware constructors, add gate-driven control flow for ExternalOIDCExternalClaimsSourcing (affecting auth metadata, external OIDC, and webhook authenticator logic), update tests for gate permutations, and bump Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
Built on top of #2074 - blocking until 2074 is merged. /hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
[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 |
when the ExternalOIDCExternalClaimsSourcing feature gate is enabled so that the cluster-authentication-operator does not have to set the Authentication resource's spec field to communicate the default oauth-apiserver webhook secret to the cluster-kube-apiserver-operator to communicate the secret that should be used to configure the kube-apiserver to talk to the oauth-apiserver as a webhook token authenticator. Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
everettraven
force-pushed
the
feature/eoidc-ec-nokascfg
branch
from
18eb25c to
dc1767a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/configobservation/auth/webhook_authenticator.go`:
- Around line 117-119: The block assigning webhookSecretName unconditionally
from auth.Spec.WebhookTokenAuthenticator.KubeConfig.Name can clear a
previously-defaulted value (used when ExternalOIDCExternalClaimsSourcing is
enabled); change the logic in the webhook secret resolution so you only override
webhookSecretName when auth.Spec.WebhookTokenAuthenticator is non-nil AND
auth.Spec.WebhookTokenAuthenticator.KubeConfig.Name is non-empty (i.e., check
kubeConfig name != "" before assigning), thereby preserving the default
webhookSecretName when the override name is empty.
- Around line 147-150: The SyncSecret calls (resourceSyncer.SyncSecret) are
currently ignoring returned errors; update each call site in the webhook
authenticator observation logic to capture and propagate the error instead of
discarding it so failures abort/return the observer result. Specifically, change
the three branches that call resourceSyncer.SyncSecret with
ResourceLocation{Namespace: operatorclient.TargetNamespace, Name:
"webhook-authenticator"} / ResourceLocation{Namespace:
operatorclient.GlobalUserSpecifiedConfigNamespace, Name: webhookSecretName} (and
the other two symmetric calls) to check the returned error and return it (or
wrap it) before returning the observed webhook config, ensuring stale or failed
syncs are surfaced. Ensure error handling uses the existing function’s error
return path so callers see sync failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a140ba6a-91c7-4f05-b7e2-5b8e69133f39
⛔ Files ignored due to path filters (13)
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/config/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_pki.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.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 (8)
go.modpkg/operator/configobservation/auth/auth_metadata.gopkg/operator/configobservation/auth/auth_metadata_test.gopkg/operator/configobservation/auth/external_oidc.gopkg/operator/configobservation/auth/external_oidc_test.gopkg/operator/configobservation/auth/webhook_authenticator.gopkg/operator/configobservation/auth/webhook_authenticator_test.gopkg/operator/configobservation/configobservercontroller/observe_config_controller.go
| resourceSyncer.SyncSecret( | ||
| resourcesynccontroller.ResourceLocation{Namespace: operatorclient.TargetNamespace, Name: "webhook-authenticator"}, | ||
| resourcesynccontroller.ResourceLocation{Namespace: operatorclient.GlobalUserSpecifiedConfigNamespace, Name: webhookSecretName}, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Unchecked SyncSecret call sites:"
rg -n -C2 '\bSyncSecret\s*\(' pkg/operator/configobservation/auth/webhook_authenticator.go
echo
echo "Local test-double signature:"
rg -n -C2 'func \(rs \*mockResourceSyncer\) SyncSecret\(.*\) error' pkg/operator/configobservation/auth/webhook_authenticator_test.goRepository: openshift/cluster-kube-apiserver-operator
Length of output: 1236
Propagate SyncSecret failures before returning observed webhook config.
The SyncSecret calls at lines 147-150, 173-176, and 188-191 ignore the error return value. This can cause the observer to publish authentication-token-webhook-* configs pointing to secrets that never synced to openshift-kube-apiserver, or silently fail to remove stale synced state.
🔧 Return the sync error from each branch
- 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)
+ }
@@
- 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)
+ }
@@
- resourceSyncer.SyncSecret(
+ if err := resourceSyncer.SyncSecret(
resourcesynccontroller.ResourceLocation{Namespace: operatorclient.TargetNamespace, Name: "webhook-authenticator"},
resourcesynccontroller.ResourceLocation{Namespace: "", Name: ""},
- )
+ ); err != nil {
+ return existingConfig, append(errs, err)
+ }🤖 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
147 - 150, The SyncSecret calls (resourceSyncer.SyncSecret) are currently
ignoring returned errors; update each call site in the webhook authenticator
observation logic to capture and propagate the error instead of discarding it so
failures abort/return the observer result. Specifically, change the three
branches that call resourceSyncer.SyncSecret with ResourceLocation{Namespace:
operatorclient.TargetNamespace, Name: "webhook-authenticator"} /
ResourceLocation{Namespace: operatorclient.GlobalUserSpecifiedConfigNamespace,
Name: webhookSecretName} (and the other two symmetric calls) to check the
returned error and return it (or wrap it) before returning the observed webhook
config, ensuring stale or failed syncs are surfaced. Ensure error handling uses
the existing function’s error return path so callers see sync failures.
There was a problem hiding this comment.
This seems like a reasonable thing to do to me. I wonder why this wasn't done originally 🤔
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the SyncSecret interface definition
rg -n 'SyncSecret' --type=go -C2
echo "---"
# Check git log for when SyncSecret was first introduced in this file
git log --oneline --follow pkg/operator/configobservation/auth/webhook_authenticator.go | head -20
Length of output: 5259
🏁 Script executed:
#!/bin/bash
# Check what errors SyncSecret can actually return in library-go
rg -n 'func.*SyncSecret' --type=go -C5
echo "---"
# Check the original commit that added SyncSecret without error handling
git show 5f863263 -- pkg/operator/configobservation/auth/webhook_authenticator.go 2>/dev/null | grep -A5 -B5 'SyncSecret' | head -40
Length of output: 3389
@everettraven It looks like it was simply a historical oversight from the very first commit (5f863263 add observer for webhook token authenticator) that introduced the observer — SyncSecret was called without capturing the return value right from the start, and that pattern carried forward through all subsequent changes.
Notably, SyncSecret primarily registers a sync rule rather than performing immediate I/O (the actual syncing happens asynchronously), which may have led the original author to treat it as infallible. However, the function signature does return an error (e.g. for duplicate registration conflicts or internal issues), and the mock in the test file even supports injecting errors via rs.error — so the capability to surface failures was always there.
Since resourcesynccontroller.go in this repo already has a precedent of checking the error (if err := resourceSyncController.SyncSecret(...)), propagating it in the webhook observer is the right thing to do. Would you like me to plan those changes?
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/operator/configobservation/auth/webhook_authenticator.go (2)
147-150:⚠️ Potential issue | 🟠 MajorPropagate
SyncSecretfailures.These calls ignore the returned error, so the observer can publish webhook args or report cleanup success even when the target secret was never synced or deleted.
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) + } @@ - 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) + } @@ - resourceSyncer.SyncSecret( + if err := resourceSyncer.SyncSecret( resourcesynccontroller.ResourceLocation{Namespace: operatorclient.TargetNamespace, Name: "webhook-authenticator"}, resourcesynccontroller.ResourceLocation{Namespace: "", Name: ""}, - ) + ); err != nil { + return existingConfig, append(errs, err) + }Also applies to: 173-176, 188-191
🤖 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 147 - 150, The SyncSecret calls to resourceSyncer.SyncSecret in webhook_authenticator.go are currently ignoring returned errors; update each invocation (the ones that call resourceSyncer.SyncSecret for ResourceLocation{Namespace: operatorclient.TargetNamespace, Name: "webhook-authenticator"} and the calls for operatorclient.GlobalUserSpecifiedConfigNamespace with webhookSecretName at the locations around the existing calls) to capture the returned error, handle or propagate it (e.g., return the error from the surrounding function or wrap it with context), and ensure any caller (such as the observer/publisher/cleanup path) does not proceed on success when SyncSecret failed; make the same change for the other occurrences referenced (around the blocks at ~173-176 and ~188-191) so failures are not silently ignored.
106-119:⚠️ Potential issue | 🟠 MajorDon't let an empty override clear the default webhook secret.
When
ExternalOIDCExternalClaimsSourcingis enabled, this block can replace the defaultedwebhook-authentication-integrated-oauthsecret with""ifspec.webhookTokenAuthenticatoris present butKubeConfig.Nameis empty. That disables the webhook and breaks the new always-configure path.Suggested fix
- if auth.Spec.WebhookTokenAuthenticator != nil { + if auth.Spec.WebhookTokenAuthenticator != nil && len(auth.Spec.WebhookTokenAuthenticator.KubeConfig.Name) > 0 { webhookSecretName = auth.Spec.WebhookTokenAuthenticator.KubeConfig.Name }A regression case with a non-nil but empty
KubeConfig.Namewould lock this in.🤖 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 106 - 119, The current logic can overwrite the defaultWebhookSecretName with an empty string when auth.Spec.WebhookTokenAuthenticator is non-nil but its KubeConfig.Name is empty; update the assignment so webhookSecretName is only set from auth.Spec.WebhookTokenAuthenticator.KubeConfig.Name when KubeConfig is non-nil and KubeConfig.Name is a non-empty string (i.e., check auth.Spec.WebhookTokenAuthenticator != nil && auth.Spec.WebhookTokenAuthenticator.KubeConfig != nil && auth.Spec.WebhookTokenAuthenticator.KubeConfig.Name != ""), keeping the existing behavior around featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) and auth.Spec.Type and leaving defaultWebhookSecretName unchanged if the override is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/operator/configobservation/auth/webhook_authenticator.go`:
- Around line 147-150: The SyncSecret calls to resourceSyncer.SyncSecret in
webhook_authenticator.go are currently ignoring returned errors; update each
invocation (the ones that call resourceSyncer.SyncSecret for
ResourceLocation{Namespace: operatorclient.TargetNamespace, Name:
"webhook-authenticator"} and the calls for
operatorclient.GlobalUserSpecifiedConfigNamespace with webhookSecretName at the
locations around the existing calls) to capture the returned error, handle or
propagate it (e.g., return the error from the surrounding function or wrap it
with context), and ensure any caller (such as the observer/publisher/cleanup
path) does not proceed on success when SyncSecret failed; make the same change
for the other occurrences referenced (around the blocks at ~173-176 and
~188-191) so failures are not silently ignored.
- Around line 106-119: The current logic can overwrite the
defaultWebhookSecretName with an empty string when
auth.Spec.WebhookTokenAuthenticator is non-nil but its KubeConfig.Name is empty;
update the assignment so webhookSecretName is only set from
auth.Spec.WebhookTokenAuthenticator.KubeConfig.Name when KubeConfig is non-nil
and KubeConfig.Name is a non-empty string (i.e., check
auth.Spec.WebhookTokenAuthenticator != nil &&
auth.Spec.WebhookTokenAuthenticator.KubeConfig != nil &&
auth.Spec.WebhookTokenAuthenticator.KubeConfig.Name != ""), keeping the existing
behavior around
featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) and
auth.Spec.Type and leaving defaultWebhookSecretName unchanged if the override is
empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5af73b05-5948-40f4-be57-78d4c5f0f9f5
📒 Files selected for processing (7)
pkg/operator/configobservation/auth/auth_metadata.gopkg/operator/configobservation/auth/auth_metadata_test.gopkg/operator/configobservation/auth/external_oidc.gopkg/operator/configobservation/auth/external_oidc_test.gopkg/operator/configobservation/auth/webhook_authenticator.gopkg/operator/configobservation/auth/webhook_authenticator_test.gopkg/operator/configobservation/configobservercontroller/observe_config_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/configobservation/configobservercontroller/observe_config_controller.go
… when ExternalOIDCExternalClaimsSourcing gate is enabled because the oauth-apiserver will be the thing that needs to be configured under the new architecture associated with this feature. Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
everettraven
force-pushed
the
feature/eoidc-ec-nokascfg
branch
from
dc1767a to
dceb4c3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/configobservation/auth/webhook_authenticator.go (1)
194-197: UncheckedSyncSecreterror in cleanup path.The
SyncSecretcall for cleanup (deleting the webhook-authenticator secret) still ignores the returned error. While this is in the "cleanup" path where the deletion is less critical than the configuration path, for consistency and to surface potential issues, the error should be handled.♻️ Suggested fix
// don't sync anything and remove whatever we synced - resourceSyncer.SyncSecret( + if err := resourceSyncer.SyncSecret( resourcesynccontroller.ResourceLocation{Namespace: operatorclient.TargetNamespace, Name: "webhook-authenticator"}, resourcesynccontroller.ResourceLocation{Namespace: "", Name: ""}, - ) + ); err != nil { + errs = append(errs, fmt.Errorf("clearing webhook-authenticator secret sync: %w", err)) + }🤖 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 194 - 197, The cleanup call to resourceSyncer.SyncSecret for the "webhook-authenticator" secret currently ignores its returned error; capture the returned error (err := resourceSyncer.SyncSecret(...)) and handle it by logging the error with context (including operatorclient.TargetNamespace and the secret name "webhook-authenticator") at an appropriate level (warn/info) so failures are visible; use the existing logger in scope (or klog) and do not change the cleanup control flow (do not return on error), just surface the error details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/configobservation/auth/webhook_authenticator.go`:
- Around line 194-197: The cleanup call to resourceSyncer.SyncSecret for the
"webhook-authenticator" secret currently ignores its returned error; capture the
returned error (err := resourceSyncer.SyncSecret(...)) and handle it by logging
the error with context (including operatorclient.TargetNamespace and the secret
name "webhook-authenticator") at an appropriate level (warn/info) so failures
are visible; use the existing logger in scope (or klog) and do not change the
cleanup control flow (do not return on error), just surface the error details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df5b21b0-9615-4f2a-b0ee-b9fdd75798fa
📒 Files selected for processing (6)
pkg/operator/configobservation/auth/auth_metadata.gopkg/operator/configobservation/auth/auth_metadata_test.gopkg/operator/configobservation/auth/external_oidc.gopkg/operator/configobservation/auth/external_oidc_test.gopkg/operator/configobservation/auth/webhook_authenticator.gopkg/operator/configobservation/configobservercontroller/observe_config_controller.go
|
@everettraven: The following test failed, say
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. |
openshift-ci
bot
added
the
needs-rebase
|
PR needs rebase. 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. |
2 participants
Description
This PR updates various config observers to ensure that the kube-apiserver is not configured to use the Structured Authentication Configuration feature when the cluster authentication type is set to
OIDC. Instead, the oauth-apiserver should always be configured as a webhook authenticator.All behavioral changes are gated by the
ExternalOIDCExternalClaimsSourcingfeature gate as this is the feature that needs the architectural change to how theOIDCauthentication mode operates under the hood.More details as to this architecture change are outlined in the OpenShift Enhancement Proposal for the feature: openshift/enhancements#1907