DO-NOT-MERGE: POC: decouple oauthapiserver auth config generation for shared library use#942
Draft
liouk wants to merge 1 commit into
Draft
DO-NOT-MERGE: POC: decouple oauthapiserver auth config generation for shared library use#942liouk wants to merge 1 commit into
liouk wants to merge 1 commit into
Conversation
…y use Refactor the AuthenticationConfigurationGenerator so it can be extracted into a shared library (oauth-apiserver) usable by both the standalone auth operator and HyperShift. Key changes: - Replace featuregates.FeatureGate with With*() methods that set bools, removing the dependency on OpenShift feature gate packages - Replace corev1listers with ResolverFunc callbacks, removing the dependency on k8s client-go listers - Convert standalone functions to methods, eliminating parameter threading of struct fields - Remove hardcoded "openshift-config" namespace; callers bind namespace into their resolver closures
openshift-ci
Bot
added
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis PR refactors ExternalOIDC authentication configuration generators to accept ChangesExternalOIDC generator refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant Generator as AuthenticationConfigurationGenerator
participant CAResolver
participant SecretResolver
Controller->>Generator: NewAuthenticationConfigurationGenerator(caResolver, clientSecretResolver)
Controller->>Generator: WithUpstreamParity()/WithAdditionalClaimMappings()/WithExternalClaimsSourcing()
Controller->>Generator: GenerateAuthenticationConfiguration(&auth.Spec)
Generator->>Generator: generateJWTForProvider(provider)
Generator->>CAResolver: getCertificateAuthority(name)
Generator->>SecretResolver: getClientSecret(name)
Generator-->>Controller: AuthenticationConfiguration
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
pkg/controllers/externaloidc/generation/kubeapiserver/generate.go (1)
65-65: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a nil-check on
authSpec.Since this generator is being extracted for reuse as a shared library (per PR objective) consumed by both this operator and HyperShift, a future caller could pass a nil
*AuthenticationSpecand cause a panic atauthSpec.OIDCProviders(Line 74). Current in-repo call sites are safe since they pass&auth.Spec, but a public/reusable API benefits from guarding against nil input.🛡️ Optional defensive guard
func (acg *AuthenticationConfigurationGenerator) GenerateAuthenticationConfiguration(authSpec *configv1.AuthenticationSpec) (runtime.Object, error) { + if authSpec == nil { + return nil, errors.New("authSpec must not be nil") + } authConfig := &apiserverv1beta1.AuthenticationConfiguration{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controllers/externaloidc/generation/kubeapiserver/generate.go` at line 65, The GenerateAuthenticationConfiguration method should defensively handle a nil authSpec before dereferencing it. Add an early guard in AuthenticationConfigurationGenerator.GenerateAuthenticationConfiguration so a nil *configv1.AuthenticationSpec returns a clear error instead of panicking when accessing authSpec.OIDCProviders. Keep the fix localized to this reusable API and use the existing method name to place the check at the start of the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 60-63: Add the missing openshift-config Secret informer to the
external OIDC controller so auth config regenerates when client secrets rotate.
Update the informer setup around NewAuthenticationConfigurationGenerator in
externaloidc_controller.go to include
kubeInformersForNamespaces.InformersFor(configNamespace).Core().V1().Secrets().Informer()
alongside the existing ConfigMap and Secret listers, ensuring the controller
requeues on secret updates.
---
Nitpick comments:
In `@pkg/controllers/externaloidc/generation/kubeapiserver/generate.go`:
- Line 65: The GenerateAuthenticationConfiguration method should defensively
handle a nil authSpec before dereferencing it. Add an early guard in
AuthenticationConfigurationGenerator.GenerateAuthenticationConfiguration so a
nil *configv1.AuthenticationSpec returns a clear error instead of panicking when
accessing authSpec.OIDCProviders. Keep the fix localized to this reusable API
and use the existing method name to place the check at the start of the
function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 84771049-e0d5-47cd-bff1-defed4a0519d
📒 Files selected for processing (6)
pkg/controllers/externaloidc/externaloidc_controller.gopkg/controllers/externaloidc/externaloidc_controller_test.gopkg/controllers/externaloidc/generation/kubeapiserver/generate.gopkg/controllers/externaloidc/generation/kubeapiserver/generate_test.gopkg/controllers/externaloidc/generation/oauthapiserver/generate.gopkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go
Comment on lines
+60
to
+63
| gen := oauthapiserver.NewAuthenticationConfigurationGenerator( | ||
| newCABundleResolver(kubeInformersForNamespaces.ConfigMapLister(), configNamespace), | ||
| newClientSecretResolver(kubeInformersForNamespaces.SecretLister(), configNamespace), | ||
| ).WithExternalClaimsSourcing() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the controller and related factory wiring
echo "== externaloidc_controller.go =="
cat -n pkg/controllers/externaloidc/externaloidc_controller.go | sed -n '1,180p'
echo
echo "== search for informer wiring and secret resolver usage =="
rg -n "newClientSecretResolver|WithInformers|Secrets\(\)\.Informer\(\)|SecretLister\(\)" pkg/controllers pkg/operator manifests -S
echo
echo "== search for RBAC references to openshift-config secrets =="
rg -n "openshift-config.*secrets|resources:.*secrets|verbs:.*(get|list|watch)" manifests pkg -SRepository: openshift/cluster-authentication-operator
Length of output: 15186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the controller factory and resolver helpers in smaller slices if needed
echo "== externaloidc_controller.go (selected range) =="
sed -n '1,180p' pkg/controllers/externaloidc/externaloidc_controller.go | cat -n
echo
echo "== client secret resolver helper =="
rg -n "func newClientSecretResolver|type .*clientSecretResolver|SecretLister" pkg/controllers/externaloidc -SRepository: openshift/cluster-authentication-operator
Length of output: 9431
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== externaloidc_controller.go =="
cat -n pkg/controllers/externaloidc/externaloidc_controller.go | sed -n '1,180p'
echo
echo "== search for informer wiring and secret resolver usage =="
rg -n "newClientSecretResolver|WithInformers|Secrets\(\)\.Informer\(\)|SecretLister\(\)" pkg/controllers pkg/operator manifests -S
echo
echo "== search for RBAC references to openshift-config secrets =="
rg -n "openshift-config.*secrets|resources:.*secrets|verbs:.*(get|list|watch)" manifests pkg -SRepository: openshift/cluster-authentication-operator
Length of output: 15186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== KubeInformersForNamespaces definition =="
rg -n "type KubeInformersForNamespaces|func .*SecretLister|InformersFor\\(" $(git ls-files '*.go') -S
echo
echo "== starter wiring for openshift-config informers =="
cat -n pkg/operator/starter.go | sed -n '150,260p'
echo
echo "== inspect namespace informer implementation =="
rg -n "openshift-config.*Secrets\\(\\)|Core\\(\\)\\.V1\\(\\)\\.Secrets\\(\\)" pkg/operator pkg/controllers -SRepository: openshift/cluster-authentication-operator
Length of output: 24801
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the factory/informer helper implementation that backs SecretLister.
fd -a ".*informers.*\.go$" pkg vendor | sed -n '1,120p'Repository: openshift/cluster-authentication-operator
Length of output: 372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate KubeInformersForNamespaces implementation in repo/vendor =="
rg -n "type KubeInformersForNamespaces|SecretLister\\(" . -S --glob '!**/.git/**'
echo
echo "== show starter informer factory setup =="
cat -n pkg/operator/starter.go | sed -n '1,260p'Repository: openshift/cluster-authentication-operator
Length of output: 16992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== controller registration and informer factories =="
rg -n "NewExternalOIDCController|kubeInformersForNamespaces|newInformerFactories|InformersFor\\(\"openshift-config\"" pkg/operator pkg/controllers -S
echo
echo "== snippet around external OIDC starter registration if present =="
cat -n pkg/operator/starter.go | sed -n '180,240p'Repository: openshift/cluster-authentication-operator
Length of output: 15180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== v1helpers informer implementation =="
cat -n vendor/github.com/openshift/library-go/pkg/operator/v1helpers/informers.go | sed -n '1,180p'
echo
echo "== replacement_starter.go informer factory setup =="
cat -n pkg/operator/replacement_starter.go | sed -n '230,330p'
echo
echo "== starter.go around externaloidc registration and openshift-config informers =="
cat -n pkg/operator/starter.go | sed -n '740,880p'Repository: openshift/cluster-authentication-operator
Length of output: 20211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== externaloidc resolver helpers =="
cat -n pkg/controllers/externaloidc/externaloidc_controller.go | sed -n '200,280p'
echo
echo "== RBAC for openshift-config secrets =="
rg -n "openshift-config|resources:.*secrets|verbs:.*(get|list|watch)" manifests pkg -S
echo
echo "== secret-related config/observer wiring =="
rg -n "SecretLister\\(|Secrets\\(\\)\\.Informer\\(\\)|newClientSecretResolver|ExternalClaimsSourcing" pkg/controllers/externaloidc pkg/operator -SRepository: openshift/cluster-authentication-operator
Length of output: 19201
Add the openshift-config Secret informer
pkg/controllers/externaloidc/externaloidc_controller.go:85-94 — the shared SecretLister() cache is already available, but this controller never requeues on openshift-config secret updates. Client-secret rotations can therefore leave the generated auth config stale until some other watched resource changes. Add kubeInformersForNamespaces.InformersFor(configNamespace).Core().V1().Secrets().Informer() here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 60 -
63, Add the missing openshift-config Secret informer to the external OIDC
controller so auth config regenerates when client secrets rotate. Update the
informer setup around NewAuthenticationConfigurationGenerator in
externaloidc_controller.go to include
kubeInformersForNamespaces.InformersFor(configNamespace).Core().V1().Secrets().Informer()
alongside the existing ConfigMap and Secret listers, ensuring the controller
requeues on secret updates.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: This is a POC PR and shouldn't be merged.
/hold
Refactor the AuthenticationConfigurationGenerator so it can be extracted into a shared library (oauth-apiserver) usable by both the standalone auth operator and HyperShift. Key changes:
Summary by CodeRabbit
New Features
Bug Fixes