CNTRLPLANE-3210: Refactor externaloidc controller to support different configuration generation logic#893
Open
everettraven wants to merge 4 commits into
Open
Conversation
…ion generation Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@everettraven: This pull request references CNTRLPLANE-3210 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 "5.0.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. |
|
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:
WalkthroughController generation logic for External OIDC was extracted into a pluggable, feature-gated authConfigGenerator and two concrete generators (kube-apiserver and oauth-apiserver). Generators build AuthenticationConfiguration objects (with issuer, CA loading, CEL claim mappings, and validation). Tests and go.mod dependency bumps were added. ChangesExternal OIDC Generator Abstraction and Implementation
Sequence DiagramsequenceDiagram
participant Controller as Controller
participant Generator as authConfigGenerator
participant ConfigMapAPI as ConfigMap API
participant Validator as Well-known endpoint
Controller->>Generator: GenerateAuthenticationConfiguration(auth)
Generator->>ConfigMapAPI: (optional) Read CA bundle ConfigMap / Secret
Generator->>Validator: HTTPS GET /.well-known/openid-configuration (per issuer)
Validator-->>Generator: 200 / TLS OK or error
Generator-->>Controller: AuthenticationConfiguration (or error)
Controller->>ConfigMapAPI: Apply ConfigMap `auth-config` (with JSON payload)
ConfigMapAPI-->>Controller: Apply result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
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 |
everettraven
force-pushed
the
feature/ext-claims-refactor
branch
from
9b5e2a1 to
652e070
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
🧹 Nitpick comments (5)
pkg/controllers/externaloidc/generation/common.go (1)
126-138: ⚡ Quick winReplace deprecated ast.Expr with recommended API.
Lines 130 and 137 use the deprecated
ast.Expr()method. The deprecation notice recommends usingcel.AstToCheckedExpr()orcel.AstToParsedExpr()followed byGetExpr().♻️ Proposed fix using recommended API
func usesEmailClaim(ast *celgo.Ast) bool { if ast == nil { return false } - return hasSelectExp(ast.Expr(), "claims", "email") + return hasSelectExp(celgo.AstToCheckedExpr(ast).GetExpr(), "claims", "email") } func usesEmailVerifiedClaim(ast *celgo.Ast) bool { if ast == nil { return false } - return hasSelectExp(ast.Expr(), "claims", "email_verified") + return hasSelectExp(celgo.AstToCheckedExpr(ast).GetExpr(), "claims", "email_verified") }🤖 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/common.go` around lines 126 - 138, The functions usesEmailClaim and usesEmailVerifiedClaim call the deprecated ast.Expr(); replace those calls by converting the celgo.Ast to a checked (or parsed) proto and then calling GetExpr() — e.g., use cel.AstToCheckedExpr(ast.Ast) or cel.AstToParsedExpr(ast.Ast) and retrieve the expression via GetExpr(); handle conversion errors and nil results the same way you currently handle a nil ast (return false) and then pass the obtained expr into hasSelectExp instead of ast.Expr().pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.go (1)
171-174: 💤 Low valueConsider setting TLS MinVersion in test server configuration.
The test TLS configuration is missing
MinVersion. While this is test code and less critical, explicitly settingMinVersion: tls.VersionTLS12ensures tests validate production-like TLS behavior.🔒 Optional fix for test TLS config
testServer := httptest.NewUnstartedServer(handlerFunc) testServer.TLS = &tls.Config{ Certificates: []tls.Certificate{*servingCertPair}, + MinVersion: tls.VersionTLS12, }🤖 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/validation/validator_test.go` around lines 171 - 174, The test TLS config for testServer currently sets Certificates but not a minimum TLS version; update the tls.Config used with testServer (the struct instance passed to testServer.TLS where servingCertPair is applied) to include MinVersion: tls.VersionTLS12 so the test server enforces at least TLS 1.2 (refer to tls.Config and the testServer/handlerFunc setup).pkg/controllers/externaloidc/externaloidc_controller.go (1)
55-61: 💤 Low valueConsider using if/else for generator selection to avoid unnecessary initialization.
The code always creates the
kubeapiservergenerator on line 57, then conditionally replaces it with theoauthapiservergenerator on lines 59-61. While harmless, this wastes a small amount of initialization work.♻️ Optional refactor using if/else
var authCfgGenerator authConfigGenerator - -authCfgGenerator = kubeapiserver.NewAuthenticationConfigurationGenerator(kubeInformersForNamespaces.ConfigMapLister(), featureGates) - -if featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) { - authCfgGenerator = oauthapiserver.NewAuthenticationConfigurationGenerator(kubeInformersForNamespaces.ConfigMapLister(), kubeInformersForNamespaces.SecretLister(), featureGates) + +if featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) { + authCfgGenerator = oauthapiserver.NewAuthenticationConfigurationGenerator(kubeInformersForNamespaces.ConfigMapLister(), kubeInformersForNamespaces.SecretLister(), featureGates) +} else { + authCfgGenerator = kubeapiserver.NewAuthenticationConfigurationGenerator(kubeInformersForNamespaces.ConfigMapLister(), featureGates) }🤖 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 55 - 61, The code currently always initializes authCfgGenerator with kubeapiserver.NewAuthenticationConfigurationGenerator and then overwrites it if featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) is true; change this to an if/else so you only construct the chosen generator. Concretely, use if featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) { authCfgGenerator = oauthapiserver.NewAuthenticationConfigurationGenerator(kubeInformersForNamespaces.ConfigMapLister(), kubeInformersForNamespaces.SecretLister(), featureGates) } else { authCfgGenerator = kubeapiserver.NewAuthenticationConfigurationGenerator(kubeInformersForNamespaces.ConfigMapLister(), featureGates) } to avoid the unnecessary initial creation while preserving the same behavior.pkg/controllers/externaloidc/generation/common_test.go (1)
151-154: 💤 Low valueConsider setting TLS MinVersion in test server configuration.
The test TLS configuration is missing
MinVersion. While this is test code and less critical, explicitly settingMinVersion: tls.VersionTLS12ensures tests validate production-like TLS behavior.🔒 Optional fix for test TLS config
testServer := httptest.NewUnstartedServer(handlerFunc) testServer.TLS = &tls.Config{ Certificates: []tls.Certificate{*servingCertPair}, + MinVersion: tls.VersionTLS12, }🤖 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/common_test.go` around lines 151 - 154, The test TLS config for the httptest server omits MinVersion; update the TLS config used when creating testServer (the tls.Config assigned to testServer.TLS where Certificates is set from servingCertPair) to include MinVersion: tls.VersionTLS12 so the test server enforces TLS1.2+ (keep using handlerFunc and the existing Certificates setup).pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go (1)
222-222: 💤 Low valueConsider checking the pem.Encode return value.
While this is test code and the impact is low, checking the error return from
pem.Encodewould make the test helper more robust and align with the static analysis suggestion.♻️ Optional fix
certPEM := new(bytes.Buffer) -pem.Encode(certPEM, &pem.Block{ +if err := pem.Encode(certPEM, &pem.Block{ Type: "CERTIFICATE", Bytes: certBytes, -}) +}); err != nil { + return nil, err +}🤖 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/oauthapiserver/validation/validator_test.go` at line 222, The pem.Encode call that writes into certPEM should check its returned error; change the plain call to capture the error from pem.Encode(certPEM, &pem.Block{...}) and fail the test on error (e.g., use t.Fatalf or require.NoError) so the test helper in validator_test.go reports encoding failures instead of silently ignoring them.
🤖 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/generation/common.go`:
- Around line 39-50: The TLS config for the HTTP client (the tls.Config used in
the http.Transport when constructing client) does not set a MinVersion; update
the tls.Config in the http.Transport used to build the http.Client (the client
variable) to include MinVersion: tls.VersionTLS12 (or tls.VersionTLS13 if you
prefer) so weak TLS protocols are not allowed; ensure the change is made inside
the tls.Config literal passed to Transport in the code that constructs the
http.Client.
- Around line 62-65: The call to retry.RetryOnConnectionErrors currently ignores
its returned error, so failures like retry exhaustion or context cancellation
are lost; update the code around retry.RetryOnConnectionErrors(retryCtx,
func(ctx context.Context) (done bool, err error) { resp, connErr =
client.Do(req); return connErr == nil, connErr }) to capture its error (e.g.
retryErr := retry.RetryOnConnectionErrors(...)), check retryErr after the call,
and propagate or wrap that error (rather than only relying on connErr) where the
function using resp/connErr expects an error; ensure you still preserve connErr
for the last attempt details but return or log retryErr when non-nil.
---
Nitpick comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 55-61: The code currently always initializes authCfgGenerator with
kubeapiserver.NewAuthenticationConfigurationGenerator and then overwrites it if
featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) is
true; change this to an if/else so you only construct the chosen generator.
Concretely, use if
featureGates.Enabled(features.FeatureGateExternalOIDCExternalClaimsSourcing) {
authCfgGenerator =
oauthapiserver.NewAuthenticationConfigurationGenerator(kubeInformersForNamespaces.ConfigMapLister(),
kubeInformersForNamespaces.SecretLister(), featureGates) } else {
authCfgGenerator =
kubeapiserver.NewAuthenticationConfigurationGenerator(kubeInformersForNamespaces.ConfigMapLister(),
featureGates) } to avoid the unnecessary initial creation while preserving the
same behavior.
In `@pkg/controllers/externaloidc/generation/common_test.go`:
- Around line 151-154: The test TLS config for the httptest server omits
MinVersion; update the TLS config used when creating testServer (the tls.Config
assigned to testServer.TLS where Certificates is set from servingCertPair) to
include MinVersion: tls.VersionTLS12 so the test server enforces TLS1.2+ (keep
using handlerFunc and the existing Certificates setup).
In `@pkg/controllers/externaloidc/generation/common.go`:
- Around line 126-138: The functions usesEmailClaim and usesEmailVerifiedClaim
call the deprecated ast.Expr(); replace those calls by converting the celgo.Ast
to a checked (or parsed) proto and then calling GetExpr() — e.g., use
cel.AstToCheckedExpr(ast.Ast) or cel.AstToParsedExpr(ast.Ast) and retrieve the
expression via GetExpr(); handle conversion errors and nil results the same way
you currently handle a nil ast (return false) and then pass the obtained expr
into hasSelectExp instead of ast.Expr().
In
`@pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.go`:
- Around line 171-174: The test TLS config for testServer currently sets
Certificates but not a minimum TLS version; update the tls.Config used with
testServer (the struct instance passed to testServer.TLS where servingCertPair
is applied) to include MinVersion: tls.VersionTLS12 so the test server enforces
at least TLS 1.2 (refer to tls.Config and the testServer/handlerFunc setup).
In
`@pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go`:
- Line 222: The pem.Encode call that writes into certPEM should check its
returned error; change the plain call to capture the error from
pem.Encode(certPEM, &pem.Block{...}) and fail the test on error (e.g., use
t.Fatalf or require.NoError) so the test helper in validator_test.go reports
encoding failures instead of silently ignoring them.
🪄 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: 099c170d-9c77-4573-92c7-b2233cfe05a4
⛔ Files ignored due to path filters (25)
go.sumis excluded by!**/*.sumvendor/github.com/google/go-cmp/cmp/cmpopts/equate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/ignore.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/sort.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/struct_filter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/xform.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runsuite.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result_writer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/viewer.htmlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/logging.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/build-machinery-go/make/targets/openshift/yq.mkis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/zz_generated.conversion.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (14)
go.modpkg/controllers/externaloidc/externaloidc_controller.gopkg/controllers/externaloidc/externaloidc_controller_test.gopkg/controllers/externaloidc/generation/common.gopkg/controllers/externaloidc/generation/common_test.gopkg/controllers/externaloidc/generation/kubeapiserver/doc.gopkg/controllers/externaloidc/generation/kubeapiserver/generate.gopkg/controllers/externaloidc/generation/kubeapiserver/generate_test.gopkg/controllers/externaloidc/generation/kubeapiserver/validation/validator.gopkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.gopkg/controllers/externaloidc/generation/oauthapiserver/generate.gopkg/controllers/externaloidc/generation/oauthapiserver/generate_test.gopkg/controllers/externaloidc/generation/oauthapiserver/validation/validator.gopkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/generation/common_test.go`:
- Around line 201-211: The two calls to pem.Encode that write into certPEM and
certPrivKeyPEM currently ignore returned errors; update both calls to capture
their error returns (from pem.Encode) and handle them appropriately in this test
(e.g., check err and call t.Fatalf/t.Fatal or propagate the error) so the test
no longer ignores encoding errors; specifically modify the pem.Encode
invocations that target variables certPEM and certPrivKeyPEM to validate the
error and fail/return on non-nil.
In `@pkg/controllers/externaloidc/generation/common.go`:
- Around line 62-64: The retry callback ignores the per-attempt context and
calls client.Do(req) with the original request context; update the callback used
in retry.RetryOnConnectionErrors to create a per-attempt request bound to the
provided ctx (e.g., call req.WithContext(ctx) into a newReq) and then call
client.Do(newReq), assigning resp and connErr from that call so
cancellation/deadline from the retryCtx is enforced for each attempt.
- Around line 130-137: The usesEmailClaim and usesEmailVerifiedClaim functions
currently call the deprecated ast.Expr(); replace that with converting the
celgo.Ast to a checked/parsed proto and then calling .GetExpr() (e.g. use
cel.AstToCheckedExpr(ast) or cel.AstToParsedExpr(ast) followed by .GetExpr()).
Extract this conversion into a small nil-safe helper (e.g. astToExpr or
getCheckedExpr) that returns the expression proto and handles the
nil/convert-error case, then have usesEmailClaim and usesEmailVerifiedClaim call
that helper and pass the resulting expr into hasSelectExp; ensure errors or nil
returns cause a safe false result.
In
`@pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.go`:
- Around line 221-231: The test fixture generation currently ignores pem.Encode
errors for the certificate and private key buffers; update the code around
certPEM and certPrivKeyPEM to check the returned error from pem.Encode (when
encoding certBytes into certPEM and x509.MarshalPKCS1PrivateKey(certPrivKey)
into certPrivKeyPEM) and return early from the test (or call t.Fatalf/t.Fatal)
on error to avoid silent fixture corruption; locate the two pem.Encode calls and
add proper error handling using the test helper or t to fail immediately when
pem.Encode returns a non-nil error.
🪄 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: ef97a611-3869-47cc-8498-11fb371b6b7a
⛔ Files ignored due to path filters (25)
go.sumis excluded by!**/*.sumvendor/github.com/google/go-cmp/cmp/cmpopts/equate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/ignore.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/sort.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/struct_filter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/xform.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runsuite.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result_writer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/viewer.htmlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/logging.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/build-machinery-go/make/targets/openshift/yq.mkis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/zz_generated.conversion.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (13)
go.modpkg/controllers/externaloidc/externaloidc_controller.gopkg/controllers/externaloidc/generation/common.gopkg/controllers/externaloidc/generation/common_test.gopkg/controllers/externaloidc/generation/kubeapiserver/doc.gopkg/controllers/externaloidc/generation/kubeapiserver/generate.gopkg/controllers/externaloidc/generation/kubeapiserver/generate_test.gopkg/controllers/externaloidc/generation/kubeapiserver/validation/validator.gopkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.gopkg/controllers/externaloidc/generation/oauthapiserver/generate.gopkg/controllers/externaloidc/generation/oauthapiserver/generate_test.gopkg/controllers/externaloidc/generation/oauthapiserver/validation/validator.gopkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/controllers/externaloidc/generation/kubeapiserver/doc.go
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator.go
- go.mod
- pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator.go
- pkg/controllers/externaloidc/generation/kubeapiserver/generate_test.go
- pkg/controllers/externaloidc/generation/kubeapiserver/generate.go
- pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go
- pkg/controllers/externaloidc/externaloidc_controller.go
- pkg/controllers/externaloidc/generation/oauthapiserver/generate.go
Comment on lines
+221
to
+231
| certPEM := new(bytes.Buffer) | ||
| pem.Encode(certPEM, &pem.Block{ | ||
| Type: "CERTIFICATE", | ||
| Bytes: certBytes, | ||
| }) | ||
|
|
||
| certPrivKeyPEM := new(bytes.Buffer) | ||
| pem.Encode(certPrivKeyPEM, &pem.Block{ | ||
| Type: "RSA PRIVATE KEY", | ||
| Bytes: x509.MarshalPKCS1PrivateKey(certPrivKey), | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file in the repository
fd -t f "validator_test.go" | grep -E "externaloidc|validation"Repository: openshift/cluster-authentication-operator
Length of output: 252
🏁 Script executed:
# Read the file around lines 221-231 to verify the issue
if [ -f "pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.go" ]; then
wc -l "pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.go"
fiRepository: openshift/cluster-authentication-operator
Length of output: 172
🏁 Script executed:
# Get broader context: examine the function containing these lines
if [ -f "pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.go" ]; then
cat -n "pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.go" | sed -n '200,250p'
fiRepository: openshift/cluster-authentication-operator
Length of output: 1824
🏁 Script executed:
# Check pem.Encode() signature in Go documentation
# It returns (n int, err error)
# Verify by checking if there are other correct usages in the codebase
rg "pem\.Encode\(" --type go -A 2 -B 1 | head -50Repository: openshift/cluster-authentication-operator
Length of output: 3847
Check pem.Encode return values in cert fixture generation.
Lines 222 and 228 ignore encoding errors; add error handling with early returns to prevent silent fixture corruption.
Suggested fix
certPEM := new(bytes.Buffer)
- pem.Encode(certPEM, &pem.Block{
+ if err := pem.Encode(certPEM, &pem.Block{
Type: "CERTIFICATE",
Bytes: certBytes,
- })
+ }); err != nil {
+ return nil, err
+ }
certPrivKeyPEM := new(bytes.Buffer)
- pem.Encode(certPrivKeyPEM, &pem.Block{
+ if err := pem.Encode(certPrivKeyPEM, &pem.Block{
Type: "RSA PRIVATE KEY",
Bytes: x509.MarshalPKCS1PrivateKey(certPrivKey),
- })
+ }); err != nil {
+ return nil, err
+ }📝 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
| certPEM := new(bytes.Buffer) | |
| pem.Encode(certPEM, &pem.Block{ | |
| Type: "CERTIFICATE", | |
| Bytes: certBytes, | |
| }) | |
| certPrivKeyPEM := new(bytes.Buffer) | |
| pem.Encode(certPrivKeyPEM, &pem.Block{ | |
| Type: "RSA PRIVATE KEY", | |
| Bytes: x509.MarshalPKCS1PrivateKey(certPrivKey), | |
| }) | |
| certPEM := new(bytes.Buffer) | |
| if err := pem.Encode(certPEM, &pem.Block{ | |
| Type: "CERTIFICATE", | |
| Bytes: certBytes, | |
| }); err != nil { | |
| return nil, err | |
| } | |
| certPrivKeyPEM := new(bytes.Buffer) | |
| if err := pem.Encode(certPrivKeyPEM, &pem.Block{ | |
| Type: "RSA PRIVATE KEY", | |
| Bytes: x509.MarshalPKCS1PrivateKey(certPrivKey), | |
| }); err != nil { | |
| return nil, err | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 222-222: Error return value of pem.Encode is not checked
(errcheck)
🤖 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/validation/validator_test.go`
around lines 221 - 231, The test fixture generation currently ignores pem.Encode
errors for the certificate and private key buffers; update the code around
certPEM and certPrivKeyPEM to check the returned error from pem.Encode (when
encoding certBytes into certPEM and x509.MarshalPKCS1PrivateKey(certPrivKey)
into certPrivKeyPEM) and return early from the test (or call t.Fatalf/t.Fatal)
on error to avoid silent fixture corruption; locate the two pem.Encode calls and
add proper error handling using the test helper or t to fail immediately when
pem.Encode returns a non-nil error.
652e070 to
41b5395
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.go (1)
221-231:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCheck
pem.Encodereturn values in test fixture generation.Lines 222 and 228 ignore
pem.Encodeerrors, which could lead to silent test fixture corruption if encoding fails.🛠️ Proposed fix
certPEM := new(bytes.Buffer) -pem.Encode(certPEM, &pem.Block{ +if err := pem.Encode(certPEM, &pem.Block{ Type: "CERTIFICATE", Bytes: certBytes, -}) +}); err != nil { + return nil, err +} certPrivKeyPEM := new(bytes.Buffer) -pem.Encode(certPrivKeyPEM, &pem.Block{ +if err := pem.Encode(certPrivKeyPEM, &pem.Block{ Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(certPrivKey), -}) +}); err != nil { + return nil, err +}🤖 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/validation/validator_test.go` around lines 221 - 231, The pem.Encode calls that build certPEM and certPrivKeyPEM in validator_test.go ignore returned errors; update the test fixture generation around the pem.Encode(...) invocations for certPEM and certPrivKeyPEM to check the error return and fail the test on error (e.g., using t.Fatal/t.Fatalf or require.NoError) so encoding failures cannot silently corrupt fixtures; locate the two pem.Encode calls that wrap certBytes and x509.MarshalPKCS1PrivateKey(certPrivKey) and add error handling for their returned error values.
🧹 Nitpick comments (3)
pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go (1)
171-174: 💤 Low valueConsider setting TLS MinVersion for test server.
Setting
MinVersion: tls.VersionTLS12would ensure test TLS configuration aligns with production standards.🤖 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/oauthapiserver/validation/validator_test.go` around lines 171 - 174, The test TLS config for the httptest server currently builds tls.Config with Certificates only; update the tls.Config used for testServer (created with testServer := httptest.NewUnstartedServer(handlerFunc) and servingCertPair) to include MinVersion: tls.VersionTLS12 (i.e., set tls.Config{Certificates: []tls.Certificate{*servingCertPair}, MinVersion: tls.VersionTLS12}) so the test server enforces TLS 1.2 minimum.pkg/controllers/externaloidc/generation/common_test.go (1)
152-155: 💤 Low valueConsider setting TLS MinVersion for test server.
While this is test-only code, setting
MinVersion: tls.VersionTLS12would better mirror production TLS configuration and ensure tests validate realistic scenarios.🔒 Optional improvement
testServer.TLS = &tls.Config{ Certificates: []tls.Certificate{*servingCertPair}, + MinVersion: tls.VersionTLS12, }🤖 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/common_test.go` around lines 152 - 155, The test TLS server configuration for testServer (created via httptest.NewUnstartedServer) currently sets Certificates only; update the tls.Config used there (the struct initialized with Certificates: []tls.Certificate{*servingCertPair}) to include MinVersion: tls.VersionTLS12 so the test server enforces TLS 1.2+ like production; locate the tls.Config initialization next to testServer.TLS and add the MinVersion field to that struct.pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.go (1)
171-174: 💤 Low valueConsider setting TLS MinVersion for test server.
Setting
MinVersion: tls.VersionTLS12would ensure test TLS configuration matches production expectations.🤖 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/validation/validator_test.go` around lines 171 - 174, The test TLS config for the httptest server (testServer) currently sets Certificates using servingCertPair but lacks a minimum TLS version; update the tls.Config passed to testServer.TLS (where handlerFunc and servingCertPair are used) to include MinVersion: tls.VersionTLS12 so the test server enforces TLS1.2+ to match production expectations.
🤖 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/generation/oauthapiserver/validation/validator_test.go`:
- Around line 221-231: The test fixture creation ignores errors from pem.Encode
when writing certificate and private key into certPEM and certPrivKeyPEM; update
the code around calls to pem.Encode (for certPEM and certPrivKeyPEM using
certBytes and x509.MarshalPKCS1PrivateKey(certPrivKey)) to capture its returned
error and fail the test on error (e.g., t.Fatalf or require.NoError) so any
encoding failure is surfaced immediately.
---
Duplicate comments:
In
`@pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.go`:
- Around line 221-231: The pem.Encode calls that build certPEM and
certPrivKeyPEM in validator_test.go ignore returned errors; update the test
fixture generation around the pem.Encode(...) invocations for certPEM and
certPrivKeyPEM to check the error return and fail the test on error (e.g., using
t.Fatal/t.Fatalf or require.NoError) so encoding failures cannot silently
corrupt fixtures; locate the two pem.Encode calls that wrap certBytes and
x509.MarshalPKCS1PrivateKey(certPrivKey) and add error handling for their
returned error values.
---
Nitpick comments:
In `@pkg/controllers/externaloidc/generation/common_test.go`:
- Around line 152-155: The test TLS server configuration for testServer (created
via httptest.NewUnstartedServer) currently sets Certificates only; update the
tls.Config used there (the struct initialized with Certificates:
[]tls.Certificate{*servingCertPair}) to include MinVersion: tls.VersionTLS12 so
the test server enforces TLS 1.2+ like production; locate the tls.Config
initialization next to testServer.TLS and add the MinVersion field to that
struct.
In
`@pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.go`:
- Around line 171-174: The test TLS config for the httptest server (testServer)
currently sets Certificates using servingCertPair but lacks a minimum TLS
version; update the tls.Config passed to testServer.TLS (where handlerFunc and
servingCertPair are used) to include MinVersion: tls.VersionTLS12 so the test
server enforces TLS1.2+ to match production expectations.
In
`@pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go`:
- Around line 171-174: The test TLS config for the httptest server currently
builds tls.Config with Certificates only; update the tls.Config used for
testServer (created with testServer := httptest.NewUnstartedServer(handlerFunc)
and servingCertPair) to include MinVersion: tls.VersionTLS12 (i.e., set
tls.Config{Certificates: []tls.Certificate{*servingCertPair}, MinVersion:
tls.VersionTLS12}) so the test server enforces TLS 1.2 minimum.
🪄 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: 9f2017d2-0a14-44d2-998c-38155f6b7a7f
⛔ Files ignored due to path filters (25)
go.sumis excluded by!**/*.sumvendor/github.com/google/go-cmp/cmp/cmpopts/equate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/ignore.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/sort.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/struct_filter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/xform.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runsuite.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result_writer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/viewer.htmlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/logging.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/build-machinery-go/make/targets/openshift/yq.mkis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/zz_generated.conversion.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (13)
go.modpkg/controllers/externaloidc/externaloidc_controller.gopkg/controllers/externaloidc/generation/common.gopkg/controllers/externaloidc/generation/common_test.gopkg/controllers/externaloidc/generation/kubeapiserver/doc.gopkg/controllers/externaloidc/generation/kubeapiserver/generate.gopkg/controllers/externaloidc/generation/kubeapiserver/generate_test.gopkg/controllers/externaloidc/generation/kubeapiserver/validation/validator.gopkg/controllers/externaloidc/generation/kubeapiserver/validation/validator_test.gopkg/controllers/externaloidc/generation/oauthapiserver/generate.gopkg/controllers/externaloidc/generation/oauthapiserver/generate_test.gopkg/controllers/externaloidc/generation/oauthapiserver/validation/validator.gopkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/controllers/externaloidc/generation/kubeapiserver/doc.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator.go
- go.mod
- pkg/controllers/externaloidc/generation/kubeapiserver/generate_test.go
- pkg/controllers/externaloidc/generation/kubeapiserver/generate.go
- pkg/controllers/externaloidc/externaloidc_controller.go
- pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go
- pkg/controllers/externaloidc/generation/oauthapiserver/generate.go
Comment on lines
+221
to
+231
| certPEM := new(bytes.Buffer) | ||
| pem.Encode(certPEM, &pem.Block{ | ||
| Type: "CERTIFICATE", | ||
| Bytes: certBytes, | ||
| }) | ||
|
|
||
| certPrivKeyPEM := new(bytes.Buffer) | ||
| pem.Encode(certPrivKeyPEM, &pem.Block{ | ||
| Type: "RSA PRIVATE KEY", | ||
| Bytes: x509.MarshalPKCS1PrivateKey(certPrivKey), | ||
| }) |
There was a problem hiding this comment.
Check pem.Encode return values in test fixture generation.
Lines 222 and 228 ignore pem.Encode errors, creating potential for silent test fixture corruption.
🛠️ Proposed fix
certPEM := new(bytes.Buffer)
-pem.Encode(certPEM, &pem.Block{
+if err := pem.Encode(certPEM, &pem.Block{
Type: "CERTIFICATE",
Bytes: certBytes,
-})
+}); err != nil {
+ return nil, err
+}
certPrivKeyPEM := new(bytes.Buffer)
-pem.Encode(certPrivKeyPEM, &pem.Block{
+if err := pem.Encode(certPrivKeyPEM, &pem.Block{
Type: "RSA PRIVATE KEY",
Bytes: x509.MarshalPKCS1PrivateKey(certPrivKey),
-})
+}); err != nil {
+ return nil, err
+}📝 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
| certPEM := new(bytes.Buffer) | |
| pem.Encode(certPEM, &pem.Block{ | |
| Type: "CERTIFICATE", | |
| Bytes: certBytes, | |
| }) | |
| certPrivKeyPEM := new(bytes.Buffer) | |
| pem.Encode(certPrivKeyPEM, &pem.Block{ | |
| Type: "RSA PRIVATE KEY", | |
| Bytes: x509.MarshalPKCS1PrivateKey(certPrivKey), | |
| }) | |
| certPEM := new(bytes.Buffer) | |
| if err := pem.Encode(certPEM, &pem.Block{ | |
| Type: "CERTIFICATE", | |
| Bytes: certBytes, | |
| }); err != nil { | |
| return nil, err | |
| } | |
| certPrivKeyPEM := new(bytes.Buffer) | |
| if err := pem.Encode(certPrivKeyPEM, &pem.Block{ | |
| Type: "RSA PRIVATE KEY", | |
| Bytes: x509.MarshalPKCS1PrivateKey(certPrivKey), | |
| }); err != nil { | |
| return nil, err | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 222-222: Error return value of pem.Encode is not checked
(errcheck)
🤖 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/oauthapiserver/validation/validator_test.go`
around lines 221 - 231, The test fixture creation ignores errors from pem.Encode
when writing certificate and private key into certPEM and certPrivKeyPEM; update
the code around calls to pem.Encode (for certPEM and certPrivKeyPEM using
certBytes and x509.MarshalPKCS1PrivateKey(certPrivKey)) to capture its returned
error and fail the test on error (e.g., t.Fatalf or require.NoError) so any
encoding failure is surfaced immediately.
liouk
suggested changes
May 13, 2026
Member
liouk
left a comment
liouk
left a comment
There was a problem hiding this comment.
One thing that stands out in this PR is that the two generators massively duplicate code for most of the generation logic (and the respective tests). As a note to self and other reviewers, as far as I understand this will change with #880 and is meant as a placeholder with the current PR. Notably, tests could be de-duplicated further even with #880 in place.
The rest of the comments I've added here seem to be applicable regardless of the changes from #880.
| recorder events.Recorder, | ||
| featureGates featuregates.FeatureGate, | ||
| ) factory.Controller { | ||
| var authCfgGenerator authConfigGenerator |
Member
There was a problem hiding this comment.
nit: we don't really need to pre-declare the variable, how about direct assignment?
Contributor
Author
There was a problem hiding this comment.
We need to pre-declare the variable so it is of the local interface type rather than the package-specific generator type
|
|
||
| type AuthenticationConfigurationValidator struct{} | ||
|
|
||
| func (acv *AuthenticationConfigurationValidator) ValidateAuthenticationConfiguration(auth *apiserverv1beta1.AuthenticationConfiguration) error { |
Member
There was a problem hiding this comment.
As far as I can see, ValidateAuthenticationConfiguration() methods are almost identical in the two generator packages (only difference is really the type of DiscoveryURL, string vs *string), which makes this whole structure overcomplicated: generator -> validator -> common helpers from generation pkg (which is where most of the validation happens).
Can we structure this in a simpler way? For example, I don't see the need for the validator packages; we each generator could call a private validation method that uses ValidateCACert() directly.
Contributor
Author
There was a problem hiding this comment.
The purpose of implementing the distinct validator packages was two-fold:
- Abstract the validation behavior out into a mockable interface so we can test the generator logic without having to also inherently test the validation behavior that requires standing up a local http server.
- The separation of the
kube-apiserverandoauth-apiservervalidator packages is so that we can evolve the validation logic of theoauth-apiserverpackage over time as needed. WIP: CNTRLPLANE-3210: Update external OIDC config generation to support external claims sourcing #880 currently doesn't add any changes to the validation logic because of the fact that there is a kube version skew between CAO and oauth-apiserver that makes it not yet possible to import the validations the oauth-apiserver runs to be used as our validation step.
I do agree that the current pathing of generator -> validator -> common is a bit cumbersome and is probably a bit over-abstracted. I'm happy to simplify this to generator -> validator by having each validation package carry a duplicate of the ValidateCACert() function that it calls instead of using common.ValidateCACert().
I don't see the need for the validator packages; we each generator could call a private validation method
I'm not strongly opinionated here, and I'm OK with dropping the validator packages altogether if you feel they aren't providing any value.
My intention with separating it out into a package is so that we could, over time, introduce new feature validation logic as part of a new validator that is configured based on gate state instead of making the validators aware of gate state. That being said, there is a trade off there of likely duplicating existing validation logic.
Reasoning about this a bit further, I've convinced myself that just making it an overridable private field on the generator is probably OK and making it gate aware is probably the path with less churn overall. I'll make the updates to adjust this accordingly.
Contributor
Author
There was a problem hiding this comment.
In the latest push, I've collapsed this to be such that everything is self-contained for each generator.
This increases code duplication, but should simplify the logical overhead of how a config is produced and validated.
See #893 (comment) for my thoughts on the code duplication aspect.
| authConfigDataKey = "auth-config.json" | ||
| oidcDiscoveryEndpointPath = "/.well-known/openid-configuration" | ||
| kindAuthenticationConfiguration = "AuthenticationConfiguration" | ||
| configNamespace = "openshift-config" |
Member
There was a problem hiding this comment.
Looks like we can drop this, as we're now using generation.ConfigNamespace everywhere.
Contributor
Author
There was a problem hiding this comment.
I removed the exported generation.ConfigNamespace in favor of everything just explicitly declaring the namespaces it cares about.
This constant is used to influence the namespace that the ConfigMap informers this controller uses is created against.
This follows the existing pattern of all the packages defining their own constants. I would prefer that we move to a shared constants model where possible but that would be a significantly larger change that I'd consider out of scope for this work.
everettraven
force-pushed
the
feature/ext-claims-refactor
branch
from
41b5395 to
6679413
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/controllers/externaloidc/generation/kubeapiserver/generate_test.go (1)
1438-1441: ⚡ Quick winAssert the well-known path in the test server.
This handler accepts every path, so these tests would still pass if
validateApiserverAuthenticationConfigurationstopped calling/.well-known/openid-configuration. Making the helper fail on other paths would protect the contract being tested.💡 Proposed fix
if handlerFunc == nil { handlerFunc = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != oidcDiscoveryEndpointPath { + http.Error(w, "unexpected path", http.StatusNotFound) + return + } w.WriteHeader(http.StatusOK) }) }🤖 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_test.go` around lines 1438 - 1441, The test HTTP handler (handlerFunc) currently accepts any path which masks whether validateApiserverAuthenticationConfiguration actually requests "/.well-known/openid-configuration"; modify the test helper so the handler returns 404 for any request path other than "/.well-known/openid-configuration" and only return http.StatusOK for that exact path, updating the handlerFunc in the test to check r.URL.Path and fail or write NotFound when it differs, so the test enforces the well-known path contract.pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go (1)
1506-1509: ⚡ Quick winAssert the well-known path in the test server.
This handler returns 200 for any path, so the test would still pass if
validateOAuthApiserverAuthenticationConfigurationstopped appending/.well-known/openid-configuration. Checkingr.URL.Pathhere would keep the test aligned with the behavior it's supposed to cover.💡 Proposed fix
if handlerFunc == nil { handlerFunc = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != oidcDiscoveryEndpointPath { + http.Error(w, "unexpected path", http.StatusNotFound) + return + } w.WriteHeader(http.StatusOK) }) }🤖 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/oauthapiserver/generate_test.go` around lines 1506 - 1509, The test's default handlerFunc returns 200 for every path which lets validateOAuthApiserverAuthenticationConfiguration be wrong without failing; update the test handlerFunc used in generate_test.go to assert that r.URL.Path equals "/.well-known/openid-configuration" (or the expected well-known path used by validateOAuthApiserverAuthenticationConfiguration) and only return 200 for that path (otherwise return a non-200 or call t.Fatalf/t.Errorf), so the test fails if the code stops appending the well-known suffix.
🤖 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/generation/kubeapiserver/generate.go`:
- Around line 556-561: The Proxy callback currently calls
httpproxy.FromEnvironment() and returns HTTPSProxy without applying NO_PROXY
rules; replace the body of the Proxy field in generate.go so it obtains
proxyConfig := httpproxy.FromEnvironment(), then uses proxyFunc :=
proxyConfig.ProxyFunc() and, if proxyFunc is non-nil, returns proxyFunc(req)
(adapting its single-return signature to the Transport Proxy signature by
returning (url, nil)); otherwise return (nil, nil). Also remove the now-unused
golang.org/x/net/http/httpproxy import. Ensure you update the anonymous Proxy
function where it's defined so NO_PROXY rules are honored.
In `@pkg/controllers/externaloidc/generation/oauthapiserver/generate.go`:
- Around line 559-564: The custom Proxy callback in the HTTP client (the
anonymous function assigned to Proxy) forces HTTPS requests through HTTPS_PROXY
and ignores NO_PROXY, causing issuer validation to go through the proxy even for
exempt hosts; replace that custom callback with http.ProxyFromEnvironment to
honor NO_PROXY rules (i.e., use http.ProxyFromEnvironment in the Proxy field of
the HTTP client's Transport used during issuer validation, e.g. where the Proxy:
func(*http.Request)... is defined) and remove the now-unused
golang.org/x/net/http/httpproxy import.
---
Nitpick comments:
In `@pkg/controllers/externaloidc/generation/kubeapiserver/generate_test.go`:
- Around line 1438-1441: The test HTTP handler (handlerFunc) currently accepts
any path which masks whether validateApiserverAuthenticationConfiguration
actually requests "/.well-known/openid-configuration"; modify the test helper so
the handler returns 404 for any request path other than
"/.well-known/openid-configuration" and only return http.StatusOK for that exact
path, updating the handlerFunc in the test to check r.URL.Path and fail or write
NotFound when it differs, so the test enforces the well-known path contract.
In `@pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go`:
- Around line 1506-1509: The test's default handlerFunc returns 200 for every
path which lets validateOAuthApiserverAuthenticationConfiguration be wrong
without failing; update the test handlerFunc used in generate_test.go to assert
that r.URL.Path equals "/.well-known/openid-configuration" (or the expected
well-known path used by validateOAuthApiserverAuthenticationConfiguration) and
only return 200 for that path (otherwise return a non-200 or call
t.Fatalf/t.Errorf), so the test fails if the code stops appending the well-known
suffix.
🪄 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: d60c8524-62e5-4fc2-9908-d3e3ce996e2a
⛔ Files ignored due to path filters (25)
go.sumis excluded by!**/*.sumvendor/github.com/google/go-cmp/cmp/cmpopts/equate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/ignore.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/sort.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/struct_filter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/xform.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runsuite.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result_writer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/viewer.htmlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/logging.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/build-machinery-go/make/targets/openshift/yq.mkis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/zz_generated.conversion.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (7)
go.modpkg/controllers/externaloidc/externaloidc_controller.gopkg/controllers/externaloidc/generation/kubeapiserver/doc.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
✅ Files skipped from review due to trivial changes (1)
- pkg/controllers/externaloidc/generation/kubeapiserver/doc.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- pkg/controllers/externaloidc/externaloidc_controller.go
Comment on lines
+559
to
+564
| Proxy: func(*http.Request) (*url.URL, error) { | ||
| if proxyConfig := httpproxy.FromEnvironment(); len(proxyConfig.HTTPSProxy) > 0 { | ||
| return url.Parse(proxyConfig.HTTPSProxy) | ||
| } | ||
| return nil, nil | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Go's http.Transport, does http.ProxyFromEnvironmentrespectNO_PROXYfor HTTPS requests? How does that compare to manually returninghttpproxy.FromEnvironment().HTTPSProxy from a custom proxy function?
💡 Result:
Yes, http.ProxyFromEnvironment respects NO_PROXY for HTTPS requests. It uses the golang.org/x/net/http/httpproxy package internally, which loads NO_PROXY via httpproxy.FromEnvironment() and applies it in proxyForURL: for HTTPS, it selects the proxy from HTTPSProxy (or falls back to HTTPProxy), but only if cfg.useProxy(addr) returns true, where useProxy checks the target host against NO_PROXY rules (IP prefixes, CIDR, domains, etc.) [1][2][3]. A nil proxy URL is returned (bypassing proxy) if the target matches NO_PROXY [4]. This is identical to manually returning httpproxy.FromEnvironment().ProxyFunc()(req.URL) (or equivalent) from a custom proxy function, as ProxyFromEnvironment is implemented as a once-initialized call to httpproxy.FromEnvironment().ProxyFunc() [2][5]. Both fully respect NO_PROXY for HTTPS, using the same logic: scheme-specific proxy selection followed by NO_PROXY exclusion check [1][6]. No differences in behavior; the manual approach allows recapturing environment changes (though ProxyFromEnvironment caches once), but respects NO_PROXY equivalently [5].
Citations:
- 1: https://github.com/golang/net/blob/master/http/httpproxy/proxy.go
- 2: https://github.com/golang/go/blob/master/src/net/http/transport.go
- 3: https://go.dev/src/vendor/golang.org/x/net/http/httpproxy/proxy.go
- 4: https://smartbft-go.github.io/godoc/pkg/golang.org/x/net/http/httpproxy/index.html
- 5: net/http: provide ProxyFromEnvironment functionality without requiring environment golang/go#22079
- 6: https://go.dev/issue/32649
🏁 Script executed:
# Check if httpproxy is imported and used elsewhere in the file
cd /tmp && cat -n pkg/controllers/externaloidc/generation/oauthapiserver/generate.go | grep -E "(import|httpproxy)" | head -30Repository: openshift/cluster-authentication-operator
Length of output: 184
🏁 Script executed:
# Get the exact context around lines 559-564
sed -n '550,575p' pkg/controllers/externaloidc/generation/oauthapiserver/generate.go | cat -nRepository: openshift/cluster-authentication-operator
Length of output: 1104
🏁 Script executed:
# Search for other uses of httpproxy in the file
rg "httpproxy" pkg/controllers/externaloidc/generation/oauthapiserver/generate.goRepository: openshift/cluster-authentication-operator
Length of output: 205
🏁 Script executed:
# Search the entire file for httpproxy usage to confirm it's only used in the proxy callback
git ls-files | grep -E "generate\.go$" | grep externaloidcRepository: openshift/cluster-authentication-operator
Length of output: 218
🏁 Script executed:
# Get the full file to check all httpproxy references
GENFILE=$(git ls-files | grep -E "generate\.go$" | grep externaloidc | head -1)
if [ -n "$GENFILE" ]; then
echo "File: $GENFILE"
rg "httpproxy" "$GENFILE" -n
fiRepository: openshift/cluster-authentication-operator
Length of output: 284
🏁 Script executed:
# Check the oauthapiserver/generate.go file specifically
rg "httpproxy" "pkg/controllers/externaloidc/generation/oauthapiserver/generate.go" -nRepository: openshift/cluster-authentication-operator
Length of output: 212
Honor NO_PROXY during issuer validation.
The proxy callback ignores host-level NO_PROXY exemptions and forces every HTTPS validation request through HTTPS_PROXY whenever it is set. In proxied clusters, this breaks valid internal issuers and routes exempted endpoints to the proxy instead of connecting directly.
Replace the custom proxy callback with http.ProxyFromEnvironment, which properly respects NO_PROXY rules:
Proposed fix
- Proxy: func(*http.Request) (*url.URL, error) {
- if proxyConfig := httpproxy.FromEnvironment(); len(proxyConfig.HTTPSProxy) > 0 {
- return url.Parse(proxyConfig.HTTPSProxy)
- }
- return nil, nil
- },
+ Proxy: http.ProxyFromEnvironment,Also remove the now-unused golang.org/x/net/http/httpproxy import.
📝 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
| Proxy: func(*http.Request) (*url.URL, error) { | |
| if proxyConfig := httpproxy.FromEnvironment(); len(proxyConfig.HTTPSProxy) > 0 { | |
| return url.Parse(proxyConfig.HTTPSProxy) | |
| } | |
| return nil, nil | |
| }, | |
| Proxy: http.ProxyFromEnvironment, |
🤖 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/oauthapiserver/generate.go` around
lines 559 - 564, The custom Proxy callback in the HTTP client (the anonymous
function assigned to Proxy) forces HTTPS requests through HTTPS_PROXY and
ignores NO_PROXY, causing issuer validation to go through the proxy even for
exempt hosts; replace that custom callback with http.ProxyFromEnvironment to
honor NO_PROXY rules (i.e., use http.ProxyFromEnvironment in the Proxy field of
the HTTP client's Transport used during issuer validation, e.g. where the Proxy:
func(*http.Request)... is defined) and remove the now-unused
golang.org/x/net/http/httpproxy import.
Contributor
Author
@liouk Thanks for the review! I made some pretty significant changes in response to your comments. Notably, there is no longer a separate validation package for each generator and there is no more Instead, all the generation logic is mostly the exact same between the I agree that, in theory, we could wire things up such that there is less duplication in place even with the staged changes in #880 . In practice, I don't think the juice is worth the squeeze there. We already have it planned to remove the If you feel strongly that we should limit duplication, I'm happy to have continued discussions here. |
everettraven
force-pushed
the
feature/ext-claims-refactor
branch
from
6679413 to
ca0e8b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/generation/kubeapiserver/generate.go`:
- Around line 549-580: The code currently mutates http.DefaultClient and
type-asserts its nil Transport causing a panic; instead create a new http.Client
(set Timeout), derive a transport from http.DefaultTransport by cloning it
(guard nil), set TLSClientConfig.RootCAs on that transport, assign it to the new
client's Transport (do not touch http.DefaultClient), and use that client for
client.Do(req.WithContext(ctx)); also ensure any non-nil *http.Response returned
during retries has its Body closed before retrying to avoid leaks. Reference:
http.DefaultClient, http.DefaultTransport, transport.Clone(), client.Transport,
client.Do, req.WithContext.
In `@pkg/controllers/externaloidc/generation/oauthapiserver/generate.go`:
- Around line 552-583: The validateCACert logic wrongly mutates the global
http.DefaultClient and can panic on Transport type assertion; instead create a
new local *http.Client using a cloned transport derived safely from
http.DefaultTransport (type-assert http.DefaultTransport to *http.Transport only
after a nil check, clone or create a new &http.Transport when nil), set its
Timeout and TLSClientConfig.RootCAs = caCertPool, and assign that transport to
the new client; also inside the retry.RetryOnConnectionErrors callback (where
client.Do and resp are used) ensure any non-nil resp.Body from failed attempts
is closed to avoid leaks and return the connection error so retry logic
continues.
🪄 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: 1eccd65b-9294-4bb4-b813-0e7b3cb9ed41
⛔ Files ignored due to path filters (25)
go.sumis excluded by!**/*.sumvendor/github.com/google/go-cmp/cmp/cmpopts/equate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/ignore.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/sort.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/struct_filter.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/google/go-cmp/cmp/cmpopts/xform.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runsuite.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result_writer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/viewer.htmlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/logging.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/build-machinery-go/make/targets/openshift/yq.mkis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/zz_generated.conversion.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (7)
go.modpkg/controllers/externaloidc/externaloidc_controller.gopkg/controllers/externaloidc/generation/kubeapiserver/doc.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
✅ Files skipped from review due to trivial changes (1)
- pkg/controllers/externaloidc/generation/kubeapiserver/doc.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controllers/externaloidc/externaloidc_controller.go
…rate package 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
force-pushed
the
feature/ext-claims-refactor
branch
from
ca0e8b0 to
9f49d4e
Compare
Contributor
|
@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. |
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
3 participants
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.
Description
This PR refactors the existing
externaloidccontroller to better facilitate creating unique generation logic depending on feature gate state. As part of this:/generation/kube-apiserverfor kube-apiserver specific generation/validation logic/generation/oauth-apiserverfor oauth-apiserver specific generation/validation logicNote
The key difference between the
generation/kube-apiserverandgeneration/oauth-apiserverpackages are the types they use for conversion and config generation.This PR sets the foundation for future changes to the oauth-apiserver specific configuration generation process as the external claims sourcing feature continues to progress.
This PR was split out from #880 which adds new oauth-apiserver configuration generation logic - in case reviewers would like to see how this will be updated in the future.
Summary by CodeRabbit
Chores
Refactor
New
Tests
Documentation