CNTRLPLANE-2990: Update CAO to no longer write to the Authentication.spec.webhookTokenAuthenticator field#854
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@everettraven: This pull request references CNTRLPLANE-2990 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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (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:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds feature-gate observation to the webhook authenticator controller, narrows authConfigChecker to an oidcAvailabler interface, introduces a webhook secret builder abstraction, adjusts operator wiring to pass feature-gate accessor and pointer changes, adds table-driven controller tests, and bumps one go.mod dependency. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Depends on #853 /hold |
openshift-ci
bot
added
the
do-not-merge/hold
| // - CAO returns early and does not attempt to set the field (field is still set) | ||
| // - CKASO sees the field is set - it reads from the set field instead of using its hardcoded default | ||
| if featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) { | ||
| return nil |
There was a problem hiding this comment.
Note for reviewers: I'm intentionally not removing any existing configuration so that there is a migration path from a version of CAO that did set this to one that does not.
When we tighten the validation of the API, ratcheting will be taken into consideration.
openshift-ci
bot
added
the
needs-rebase
|
/lgtm |
…ng gate Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
for communicating the webhook token authenticator secret to be used by the cluster-kube-apiserver-operator for configuring the kube-apiserver. Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
everettraven
force-pushed
the
feature/eoidc-ec-nosetspec
branch
from
32ddde8 to
7709931
Compare
openshift-ci
bot
removed
lgtm
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@liouk I've updated this to add tests for the webhookauthenticator controller. I had to do some refactoring to make it a bit easier to implement the tests (e.g to not always attempting to read a CA bundle from As a heads up, I used claude to write the unit tests and did a pretty thorough review. It looked pretty good to me and they all pass, but I'm still trying to find the best way to review the claude output so I may have missed something. |
![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/controllers/webhookauthenticator/webhookauthenticator_controller.go`:
- Around line 121-129: The sync method currently fails early by calling
featureGateAccessor.AreInitialFeatureGatesObserved() and CurrentFeatureGates()
at the top of webhookAuthenticatorController.sync; move the feature-gate lookup
(calls to AreInitialFeatureGatesObserved and CurrentFeatureGates) down into the
specific branch that actually uses the gates (the branch around the
OIDC/auth-type check currently referencing the gate at line ~175). Change the
behavior so that if AreInitialFeatureGatesObserved() is false you return a
requeue/wait (not an error) and if CurrentFeatureGates() returns an error handle
it locally in that branch (wrap/return as appropriate) so other no-op paths are
not blocked by gate warm-up/read failures.
- Around line 293-296: The nil-only guard in
defaultWebhookSecretBuilder.BuildWebhookSecret allows zero-length tls key/cert
to pass; change the check to reject empty slices (treat key or cert with len==0
same as nil) so BuildWebhookSecret returns an error for empty material, and make
the same change in the earlier secret validation path that inspects
tls.key/tls.crt and clears AuthenticatorCertKeyProgressing (ensure that code
also checks len(secret.Data["tls.key"])==0 and len(secret.Data["tls.crt"])==0
and treats them as missing/invalid).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7d15cd5-c549-4a26-bd8b-bee1b4ed7131
⛔ Files ignored due to path filters (42)
vendor/github.com/openshift/client-go/config/applyconfigurations/utils.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/fake/clientset_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/fake/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/fake/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_apiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_build.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_clusterimagepolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_clusteroperator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_clusterversion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_config_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_console.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_dns.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_featuregate.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_image.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_imagecontentpolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_imagedigestmirrorset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_imagepolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_imagetagmirrorset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_infrastructure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_ingress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_insightsdatagather.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_oauth.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_operatorhub.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_project.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_proxy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_scheduler.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/fake/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/fake/fake_backup.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/fake/fake_clusterimagepolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/fake/fake_clustermonitoring.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/fake/fake_config_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/fake/fake_criocredentialproviderconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/fake/fake_imagepolicy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha1/fake/fake_insightsdatagather.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha2/fake/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha2/fake/fake_config_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1alpha2/fake/fake_insightsdatagather.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (3)
pkg/controllers/webhookauthenticator/webhookauthenticator_controller.gopkg/controllers/webhookauthenticator/webhookauthenticator_controller_test.gopkg/operator/starter.go
| func (c *webhookAuthenticatorController) sync(ctx context.Context, syncCtx factory.SyncContext) error { | ||
| if !c.featureGateAccessor.AreInitialFeatureGatesObserved() { | ||
| return errors.New("observing feature gates: initial feature gates have not been observed yet") | ||
| } | ||
|
|
||
| featureGates, err := c.featureGateAccessor.CurrentFeatureGates() | ||
| if err != nil { | ||
| return fmt.Errorf("observing feature gates: %w", err) | ||
| } |
There was a problem hiding this comment.
Move feature-gate lookup down to the only branch that uses it.
Lines 122-129 fail sync before the OIDC and auth-type checks, so a feature-gate warm-up/read problem now blocks paths that would otherwise be a no-op. The gate is only consulted at Line 175; fetch it there instead, and treat “not observed yet” as a wait/requeue path rather than an error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controllers/webhookauthenticator/webhookauthenticator_controller.go`
around lines 121 - 129, The sync method currently fails early by calling
featureGateAccessor.AreInitialFeatureGatesObserved() and CurrentFeatureGates()
at the top of webhookAuthenticatorController.sync; move the feature-gate lookup
(calls to AreInitialFeatureGatesObserved and CurrentFeatureGates) down into the
specific branch that actually uses the gates (the branch around the
OIDC/auth-type check currently referencing the gate at line ~175). Change the
behavior so that if AreInitialFeatureGatesObserved() is false you return a
requeue/wait (not an error) and if CurrentFeatureGates() returns an error handle
it locally in that branch (wrap/return as appropriate) so other no-op paths are
not blocked by gate warm-up/read failures.
| func (d *defaultWebhookSecretBuilder) BuildWebhookSecret(ctx context.Context, svc *corev1.Service, key, cert []byte) (*corev1.Secret, error) { | ||
| if key == nil || cert == nil { | ||
| return nil, fmt.Errorf("nil key or cert") | ||
| } |
There was a problem hiding this comment.
Reject empty TLS material, not just nil.
A zero-length tls.key or tls.crt still passes this guard, so the controller can publish a kubeconfig with empty client-cert data and clear AuthenticatorCertKeyProgressing as if everything were healthy. Please treat len(...) == 0 the same as missing data here and in the earlier secret validation path.
Possible fix
func (d *defaultWebhookSecretBuilder) BuildWebhookSecret(ctx context.Context, svc *corev1.Service, key, cert []byte) (*corev1.Secret, error) {
- if key == nil || cert == nil {
- return nil, fmt.Errorf("nil key or cert")
+ if len(key) == 0 || len(cert) == 0 {
+ return nil, fmt.Errorf("empty key or cert")
}📝 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.
| func (d *defaultWebhookSecretBuilder) BuildWebhookSecret(ctx context.Context, svc *corev1.Service, key, cert []byte) (*corev1.Secret, error) { | |
| if key == nil || cert == nil { | |
| return nil, fmt.Errorf("nil key or cert") | |
| } | |
| func (d *defaultWebhookSecretBuilder) BuildWebhookSecret(ctx context.Context, svc *corev1.Service, key, cert []byte) (*corev1.Secret, error) { | |
| if len(key) == 0 || len(cert) == 0 { | |
| return nil, fmt.Errorf("empty key or cert") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controllers/webhookauthenticator/webhookauthenticator_controller.go`
around lines 293 - 296, The nil-only guard in
defaultWebhookSecretBuilder.BuildWebhookSecret allows zero-length tls key/cert
to pass; change the check to reject empty slices (treat key or cert with len==0
same as nil) so BuildWebhookSecret returns an error for empty material, and make
the same change in the earlier secret validation path that inspects
tls.key/tls.crt and clears AuthenticatorCertKeyProgressing (ensure that code
also checks len(secret.Data["tls.key"])==0 and len(secret.Data["tls.crt"])==0
and treats them as missing/invalid).
| serviceAccounts: serviceAccounts, | ||
| configNSSecretsLister: kubeInformersForConfigNamespace.Core().V1().Secrets().Lister(), | ||
| secretsLister: kubeInformersForTargetNamespace.Core().V1().Secrets().Lister(), | ||
| svcLister: kubeInformersForTargetNamespace.Core().V1().Services().Lister(), | ||
| saLister: kubeInformersForTargetNamespace.Core().V1().ServiceAccounts().Lister(), |
There was a problem hiding this comment.
These were unused.
| // TODO: remove in 4.9; this is to ensure we don't configure webhook authenticators | ||
| // before the oauth-apiserver revision that's capable of handling it is ready | ||
| // during upgrade | ||
| versions := c.versionGetter.GetVersions() | ||
| if apiserverVersion, ok := versions["oauth-apiserver"]; ok { | ||
| // a previous version found means this could be an upgrade, unless the version is already current | ||
| if expectedVersion := os.Getenv("OPERATOR_IMAGE_VERSION"); apiserverVersion != expectedVersion { | ||
| if c.apiServerVersionWaitEventsLimiter.TryAccept() { | ||
| syncCtx.Recorder().Eventf("OAuthAPIServerWaitForLatest", "the oauth-apiserver hasn't reported its version to be %q yet, its current version is %q", expectedVersion, apiserverVersion) | ||
| } | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
I figured that while I was doing a refactor to implement the tests, I might as well clean this up based on the seemingly pretty old TODO.
|
|
||
| operatorClient v1helpers.OperatorClient | ||
|
|
||
| apiServerVersionWaitEventsLimiter flowcontrol.RateLimiter |
There was a problem hiding this comment.
This looks like it can be removed now that we've dropped that 4.9 TODO.
everettraven
force-pushed
the
feature/eoidc-ec-nosetspec
branch
from
a740957 to
3d0b870
Compare
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
everettraven
force-pushed
the
feature/eoidc-ec-nosetspec
branch
from
3d0b870 to
83e1bc6
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven, liouk, ShazaAldawamneh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
| Data: map[string][]byte{ | ||
| "kubeConfig": []byte(kubeconfigComplete), | ||
| }, | ||
| return nil, fmt.Errorf("building webhook secret: %w", err) |
There was a problem hiding this comment.
maybe should this read error building webhook secret: %w instead?
There was a problem hiding this comment.
Generally, I prefer that if we are returning an error that we don't include the word "error". Following that pattern generally ends up in logs as something like:
ERR: error doing thing: error when trying something: error ...: error ......Lots of, IMO, unnecessary repetition of the word "error".
|
I'll come back to this once tests come back. So far, LGTM. |
|
/test e2e-agnostic |
|
@everettraven: The following tests 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. |
|
/verified by @everettraven |
openshift-ci-robot
added
the
verified
|
@everettraven: This PR has been marked as verified by 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-merge-bot
bot
merged commit 9a8c53c
into
openshift:master
5 participants
Description
This PR updates the webhook authenticator controller to return early when the
ExternalOIDCExternalClaimsSourcingfeature gate is enabled instead of attempting to write to theauthentications.config.openshift.io/clusterresource'sspec.webhookTokenAuthenticatorfield.This is part of a two-step change intended to allow us to maintain the existing validation enforced by https://github.com/openshift/kubernetes/blob/2034d92b4a3a51d42e306ba405fc10a89768ac69/openshift-kube-apiserver/admission/customresourcevalidation/authentication/validate_authentication.go#L183-L200 that does not allow setting the
spec.webhookTokenAuthenticatorfield whenspec.typeis set toOIDC. The second step will be to update the cluster-kube-apiserver-operator to use a hardcoded default for the webhook token authenticator secret that is overridden when thespec.webhookTokenAuthenticatorfield is set.This is necessary as part of the work to implement the architectural change we are making to how the
OIDCauthentication mode operates under the hood that is outlined in openshift/enhancements#1907.