Skip to content

CNTRLPLANE-2990: Update CAO to no longer write to the Authentication.spec.webhookTokenAuthenticator field #854

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 3 commits into from
Mar 23, 2026
Merged

CNTRLPLANE-2990: Update CAO to no longer write to the Authentication.spec.webhookTokenAuthenticator field #854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1bb682b
dependencies: bump o/api to commit w/ExternalOIDCExternalClaimsSourci…
everettraven Mar 17, 2026
7709931
feat: webhookauthenticator: do not set authentications spec
everettraven Mar 17, 2026
83e1bc6
testing: refactor webhookauthenticator controller and implement tests
everettraven Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ require (
github.com/google/go-cmp v0.7.0
github.com/onsi/ginkgo/v2 v2.27.2
github.com/openshift-eng/openshift-tests-extension v0.0.0-20251205182537-ff5553e56f33
github.com/openshift/api v0.0.0-20260304122341-cf5d8996109f
github.com/openshift/api v0.0.0-20260306002634-d3bbdada155c
github.com/openshift/build-machinery-go v0.0.0-20250530140348-dc5b2804eeee
github.com/openshift/client-go v0.0.0-20260302182750-20813ce71ca6
github.com/openshift/library-go v0.0.0-20260303171201-5d9eb6295ff6
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,8 @@ github.com/onsi/gomega v1.38.2 h1:eZCjf2xjZAqe+LeWvKb5weQ+NcPwX84kqJ0cZNxok2A=
github.com/onsi/gomega v1.38.2/go.mod h1:W2MJcYxRGV63b418Ai34Ud0hEdTVXq9NW9+Sx6uXf3k=
github.com/openshift-eng/openshift-tests-extension v0.0.0-20251205182537-ff5553e56f33 h1:LJf6kWZQ36iako7WXRzdEa5XKrnyrAX8GBhlAcKRaZQ=
github.com/openshift-eng/openshift-tests-extension v0.0.0-20251205182537-ff5553e56f33/go.mod h1:6gkP5f2HL0meusT0Aim8icAspcD1cG055xxBZ9yC68M=
github.com/openshift/api v0.0.0-20260304122341-cf5d8996109f h1:M8y0oBq/KRkuSNFlUMQRAn2MrXJh1mzTCFgbLpPWQbM=
github.com/openshift/api v0.0.0-20260304122341-cf5d8996109f/go.mod h1:d5uzF0YN2nQQFA0jIEWzzOZ+edmo6wzlGLvx5Fhz4uY=
github.com/openshift/api v0.0.0-20260306002634-d3bbdada155c h1:YQYiDzOLJzwQunxCaa5OpyNyMLPz4HJ2CLaKqUQOQjQ=
github.com/openshift/api v0.0.0-20260306002634-d3bbdada155c/go.mod h1:pyVjK0nZ4sRs4fuQVQ4rubsJdahI1PB94LnQ8sGdvxo=
github.com/openshift/build-machinery-go v0.0.0-20250530140348-dc5b2804eeee h1:+Sp5GGnjHDhT/a/nQ1xdp43UscBMr7G5wxsYotyhzJ4=
github.com/openshift/build-machinery-go v0.0.0-20250530140348-dc5b2804eeee/go.mod h1:8jcm8UPtg2mCAsxfqKil1xrmRMI3a+XU2TZ9fF8A7TE=
github.com/openshift/client-go v0.0.0-20260302182750-20813ce71ca6 h1:wJv4Ia+R4OxoaJcTUyvMtBc5rWFvfTiEA8d5f1MBPqI=
Expand Down
179 changes: 110 additions & 69 deletions pkg/controllers/webhookauthenticator/webhookauthenticator_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package webhookauthenticator
import (
"context"
"encoding/base64"
"errors"
"fmt"
"net"
"os"
Expand All @@ -11,26 +12,25 @@ import (
"time"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/informers"
corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
corev1listers "k8s.io/client-go/listers/core/v1"
"k8s.io/client-go/util/flowcontrol"
"k8s.io/klog/v2"
"k8s.io/utils/ptr"

"github.com/openshift/api/annotations"
configv1 "github.com/openshift/api/config/v1"
"github.com/openshift/api/features"
operatorv1 "github.com/openshift/api/operator/v1"
configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1"
"github.com/openshift/library-go/pkg/controller/factory"
"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
"github.com/openshift/library-go/pkg/operator/events"
"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
"github.com/openshift/library-go/pkg/operator/status"
"github.com/openshift/library-go/pkg/operator/v1helpers"

"github.com/openshift/cluster-authentication-operator/bindata"
Expand All @@ -49,54 +49,63 @@ const (
type webhookAuthenticatorController struct {
controllerInstanceName string
authentication configv1client.AuthenticationInterface
serviceAccounts corev1client.ServiceAccountsGetter

saLister corev1listers.ServiceAccountLister
svcLister corev1listers.ServiceLister
secrets corev1client.SecretsGetter
secretsLister corev1listers.SecretLister
configNSSecretsLister corev1listers.SecretLister
authConfigChecker common.AuthConfigChecker
authConfigChecker oidcAvailabler

operatorClient v1helpers.OperatorClient

apiServerVersionWaitEventsLimiter flowcontrol.RateLimiter
versionGetter status.VersionGetter
featureGateAccessor featuregates.FeatureGateAccess
webhookSecretBuilder webhookSecretBuilder
}

type oidcAvailabler interface {
OIDCAvailable() (bool, error)
}

type webhookSecretBuilder interface {
BuildWebhookSecret(context.Context, *corev1.Service, []byte, []byte) (*corev1.Secret, error)
}

func NewWebhookAuthenticatorController(
instanceName string,
kubeInformersForTargetNamespace informers.SharedInformerFactory,
kubeInformersForConfigNamespace informers.SharedInformerFactory,
secrets corev1client.SecretsGetter,
serviceAccounts corev1client.ServiceAccountsGetter,
authentication configv1client.AuthenticationInterface,
operatorClient v1helpers.OperatorClient,
authConfigChecker common.AuthConfigChecker,
versionGetter status.VersionGetter,
authConfigChecker oidcAvailabler,
recorder events.Recorder,
featureGateAccessor featuregates.FeatureGateAccess,
) factory.Controller {
c := &webhookAuthenticatorController{
controllerInstanceName: factory.ControllerInstanceName(instanceName, "WebhookAuthenticator"),
secrets: secrets,
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(),
authentication: authentication,
operatorClient: operatorClient,
apiServerVersionWaitEventsLimiter: flowcontrol.NewTokenBucketRateLimiter(0.0167, 1), // set it so that the event may only occur once per minute
authConfigChecker: authConfigChecker,
versionGetter: versionGetter,
controllerInstanceName: factory.ControllerInstanceName(instanceName, "WebhookAuthenticator"),
secrets: secrets,
configNSSecretsLister: kubeInformersForConfigNamespace.Core().V1().Secrets().Lister(),
secretsLister: kubeInformersForTargetNamespace.Core().V1().Secrets().Lister(),
svcLister: kubeInformersForTargetNamespace.Core().V1().Services().Lister(),
authentication: authentication,
operatorClient: operatorClient,
authConfigChecker: authConfigChecker,
featureGateAccessor: featureGateAccessor,
webhookSecretBuilder: newDefaultWebhookSecretBuilder(),
}

authCfgInformers := []factory.Informer{}
if _, ok := authConfigChecker.(*common.AuthConfigChecker); ok {
authCfgInformers = append(authCfgInformers, common.AuthConfigCheckerInformers[factory.Informer](authConfigChecker.(*common.AuthConfigChecker))...)
}

return factory.New().
WithInformers(
kubeInformersForTargetNamespace.Core().V1().ServiceAccounts().Informer(),
kubeInformersForTargetNamespace.Core().V1().Services().Informer(),
kubeInformersForTargetNamespace.Core().V1().Secrets().Informer(),
).
WithInformers(common.AuthConfigCheckerInformers[factory.Informer](&authConfigChecker)...).
WithInformers(authCfgInformers...).
ResyncEvery(wait.Jitter(time.Minute, 1.0)).
WithSync(c.sync).
WithSyncDegradedOnError(operatorClient).
Expand All @@ -106,6 +115,15 @@ func NewWebhookAuthenticatorController(
}

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)
}
Comment on lines 117 to +125
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


if oidcAvailable, err := c.authConfigChecker.OIDCAvailable(); err != nil {
return err
} else if oidcAvailable {
Expand All @@ -116,20 +134,6 @@ func (c *webhookAuthenticatorController) sync(ctx context.Context, syncCtx facto
return c.operatorClient.ApplyOperatorStatus(ctx, c.controllerInstanceName, applyoperatorv1.OperatorStatus())
}

// 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
}
}
Comment on lines -119 to -131
Copy link
Copy Markdown
Contributor Author

@everettraven everettraven Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


authConfig, err := c.authentication.Get(ctx, "cluster", metav1.GetOptions{})
if err != nil {
return err
Expand All @@ -150,6 +154,24 @@ func (c *webhookAuthenticatorController) sync(ctx context.Context, syncCtx facto
return err
}

// As part of the new approach for configuring the OIDC authentication method, we no longer want to
// explicitly set the `authentications.config.openshift.io/cluster.spec.webhookTokenAuthenticator`
// field as part of this controller's behavior.
// This allows us to disallow setting `.spec.webhookTokenAuthenticator` when `.spec.type`
// is set to one that consumes the single KAS webhook token authenticator slot with the
// oauth-apiserver.
// Combined with a change in the cluster-kube-apiserver-operator to default
// the secret that it attempts to read from the openshift-config namespaces when `.spec.webhookTokenAuthenticator`
// is not set, simply returning here should be backwards compatible.
// Backwards compatibility example:
// - `.spec.webhookTokenAuthenticator` was previously set by this controller
// - CAO + CKASO have been updated to use a shared constant for default behavior
// - 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
Copy link
Copy Markdown
Contributor Author

@everettraven everettraven Mar 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

if authConfig.Spec.WebhookTokenAuthenticator == nil {
authConfig.Spec.WebhookTokenAuthenticator = &configv1.WebhookTokenAuthenticator{}
}
Expand Down Expand Up @@ -177,36 +199,9 @@ func (c *webhookAuthenticatorController) ensureKubeConfigSecret(ctx context.Cont
return nil, err
}

caBundle, err := os.ReadFile("/var/run/configmaps/service-ca-bundle/service-ca.crt")
if err != nil {
return nil, fmt.Errorf("failed to read service-ca crt bundle: %w", err)
}

kubeconfigBytes, err := bindata.Asset("oauth-apiserver/authenticator-kubeconfig.yaml")
requiredSecret, err := c.webhookSecretBuilder.BuildWebhookSecret(ctx, svc, key, cert)
if err != nil {
return nil, fmt.Errorf("failed to read kubeconfig template: %w", err)
}

replacer := strings.NewReplacer(
"${CA_DATA}", base64.StdEncoding.EncodeToString(caBundle),
"${APISERVER_IP}", net.JoinHostPort(svc.Spec.ClusterIP, strconv.Itoa(int(svc.Spec.Ports[0].Port))),
"${CLIENT_CERT}", base64.StdEncoding.EncodeToString(cert),
"${CLIENT_KEY}", base64.StdEncoding.EncodeToString(key),
)

kubeconfigComplete := replacer.Replace(string(kubeconfigBytes))

requiredSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: webhookSecretName,
Namespace: configNamespace,
Annotations: map[string]string{
annotations.OpenShiftComponent: "apiserver-auth",
},
},
Data: map[string][]byte{
"kubeConfig": []byte(kubeconfigComplete),
},
return nil, fmt.Errorf("building webhook secret: %w", err)
Copy link
Copy Markdown

@ehearne-redhat ehearne-redhat Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe should this read error building webhook secret: %w instead?

Copy link
Copy Markdown
Contributor Author

@everettraven everettraven Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

}

secret, _, err := resourceapply.ApplySecret(ctx, c.secrets, recorder, requiredSecret)
Expand Down Expand Up @@ -271,16 +266,62 @@ func (c *webhookAuthenticatorController) getAuthenticatorCertKeyPair(ctx context

func (c *webhookAuthenticatorController) removeOperands(ctx context.Context) error {
_, err := c.configNSSecretsLister.Secrets(configNamespace).Get(webhookSecretName)
if errors.IsNotFound(err) {
if apierrors.IsNotFound(err) {
return nil
} else if err != nil {
return fmt.Errorf("getting secret %s/%s: %v", configNamespace, webhookSecretName, err)
}

err = c.secrets.Secrets(configNamespace).Delete(ctx, webhookSecretName, metav1.DeleteOptions{})
if err != nil && !errors.IsNotFound(err) {
if err != nil && !apierrors.IsNotFound(err) {
return fmt.Errorf("deleting secret %s/%s: %v", configNamespace, webhookSecretName, err)
}

return nil
}

func newDefaultWebhookSecretBuilder() webhookSecretBuilder {
return &defaultWebhookSecretBuilder{}
}

type defaultWebhookSecretBuilder struct{}

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")
}
Comment on lines +289 to +292
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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).


caBundle, err := os.ReadFile("/var/run/configmaps/service-ca-bundle/service-ca.crt")
if err != nil {
return nil, fmt.Errorf("failed to read service-ca crt bundle: %w", err)
}

kubeconfigBytes, err := bindata.Asset("oauth-apiserver/authenticator-kubeconfig.yaml")
if err != nil {
return nil, fmt.Errorf("failed to read kubeconfig template: %w", err)
}

replacer := strings.NewReplacer(
"${CA_DATA}", base64.StdEncoding.EncodeToString(caBundle),
"${APISERVER_IP}", net.JoinHostPort(svc.Spec.ClusterIP, strconv.Itoa(int(svc.Spec.Ports[0].Port))),
"${CLIENT_CERT}", base64.StdEncoding.EncodeToString(cert),
"${CLIENT_KEY}", base64.StdEncoding.EncodeToString(key),
)

kubeconfigComplete := replacer.Replace(string(kubeconfigBytes))

requiredSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: webhookSecretName,
Namespace: configNamespace,
Annotations: map[string]string{
annotations.OpenShiftComponent: "apiserver-auth",
},
},
Data: map[string][]byte{
"kubeConfig": []byte(kubeconfigComplete),
},
}

return requiredSecret, nil
}
Loading