CNTRLPLANE-3210: Update external OIDC config generation to support external claims sourcing#880
Open
everettraven wants to merge 2 commits into
Open
CNTRLPLANE-3210: Update external OIDC config generation to support external claims sourcing#880everettraven wants to merge 2 commits into
everettraven wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
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:
WalkthroughBumps four OpenShift Go module dependencies in ChangesExternal OIDC Claims Sourcing Generator
Sequence Diagram(s)sequenceDiagram
participant Caller
participant generateJWTForProvider
participant FeatureGate
participant generateExternalClaimsSources
participant ConfigMapLister
participant SecretLister
Caller->>generateJWTForProvider: auth *configv1.Authentication
generateJWTForProvider->>FeatureGate: Enabled(ExternalOIDCExternalClaimsSourcing)?
alt feature enabled
generateJWTForProvider->>generateExternalClaimsSources: []configv1.ExternalClaimsSource
loop per source
generateExternalClaimsSources->>ConfigMapLister: Get TLS CA ConfigMap (ca-bundle.crt)
alt client-credential auth
generateExternalClaimsSources->>SecretLister: Get Secret (client-secret key)
generateExternalClaimsSources->>ConfigMapLister: Get credential TLS CA ConfigMap
end
generateExternalClaimsSources-->>generateExternalClaimsSources: build ExternalClaimsSource (URL, TLS, mappings, predicates)
end
generateExternalClaimsSources-->>generateJWTForProvider: []ExternalClaimsSource, error
generateJWTForProvider->>generateJWTForProvider: out.ExternalClaimsSources = result
end
generateJWTForProvider-->>Caller: JWTAuthenticator, error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/controllers/externaloidc/externaloidc_controller_test.go (1)
438-443:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPopulate
authConfigGeneratorin these sync tests.
sync()now always dereferencesc.authConfigGenerator; leaving it unset here makes every OIDC testcase panic before the assertions run. A small stub generator per testcase is enough to keep this suite testing controller behavior instead of crashing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/externaloidc/externaloidc_controller_test.go` around lines 438 - 443, Tests instantiate externalOIDCController without setting authConfigGenerator, but sync() now dereferences c.authConfigGenerator causing panics; update the test setup that builds externalOIDCController to set c.authConfigGenerator to a simple stub generator (e.g., a small function or struct implementing the generator interface that returns a static/empty auth config and no error) so each testcase provides a non-nil authConfigGenerator; reference the externalOIDCController struct instantiation and the sync() call to locate where to add the stub.pkg/controllers/externaloidc/externaloidc_controller.go (2)
103-118:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDon't persist secret-bearing oauth config into a ConfigMap.
On the oauth-apiserver path, the generator resolves
client-secretvalues from Secrets into the returned auth object, and this code marshals that object straight intoopenshift-config-managed/auth-config. That downgrades credential material from Secret storage to plain ConfigMap data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 103 - 118, The code is persisting secret-bearing oauth client secrets into the managed ConfigMap (openshift-config-managed/auth-config) by applying the full object returned from getExpectedApplyConfig; modify the flow so secret material is not written into a ConfigMap: either strip/omit secret fields (e.g., remove client-secret entries) from the returned object in getExpectedApplyConfig (or call a new sanitizeAuthConfig function) before calling c.configMaps.ConfigMaps(...).Apply, or persist the sensitive parts into a Secret (use c.secrets.Secrets(...).Apply) and leave only non-secret references in the ConfigMap; update references to targetAuthConfigCMName/managedNamespace and ensure c.getExistingApplyConfig comparison uses the sanitized form to avoid reapplying secrets.
73-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWatch Secrets on the external-claims-sourcing path.
When
FeatureGateExternalOIDCExternalClaimsSourcingis on, the selected generator reads referenced Secrets viaSecretLister(), but this controller only resyncs on Authentication and ConfigMap events. Secret rotation or client-credential updates will leave the rendered auth config stale until some unrelated event happens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 73 - 84, The controller currently only watches ConfigMaps and Authentications but must also watch Secrets used by the external-claims-sourcing generator; update the informer registration in the factory chain (the block starting with factory.New().WithInformers(...) and WithFilteredEventsInformers(...)) to add a Secrets informer—e.g., include kubeInformersForNamespaces.InformersFor(managedNamespace).Core().V1().Secrets().Informer() (or a NamesFilter-wrapped Secrets informer for the specific secret names) so that SecretLister() updates trigger c.sync; only add this Secrets informer when FeatureGateExternalOIDCExternalClaimsSourcing is enabled so the controller resyncs on secret rotation/client-credential changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 137-139: In go.mod remove the two local replace directives that
point to ../api/ and ../oauth-apiserver/ so module resolution uses the pinned
upstream pseudo-versions instead; specifically delete the replace lines
referencing github.com/openshift/api => ../api/ and
github.com/openshift/oauth-apiserver => ../oauth-apiserver/ to avoid breaking CI
or downstream consumers that don’t have the sibling paths.
In `@pkg/controllers/externaloidc/generation/kubeapiserver/generate_test.go`:
- Around line 3-1551: The entire test suite is commented out with a block
comment starting at the top of generate_test.go, so none of the tests (e.g.
TestExternalOIDCController_generateAuthConfig,
TestExternalOIDCController_validateAuthConfig,
TestExternalOIDCController_validateCACert and helpers like everFailingIndexer,
generateCAKeyPair, generateServingCert) run; remove the surrounding /* ... */
block (or otherwise restore the file to valid Go test source) so the imports and
test functions are active again, ensure the package declaration and imports
compile, then run go test to verify the suite executes.
In `@pkg/controllers/externaloidc/generation/kubeapiserver/generate.go`:
- Around line 695-721: The custom Proxy lambda on the http.Transport only checks
HTTPS_PROXY and ignores standard proxy rules; also the request passed to
client.Do is not bound to the retry context so the 10s retryCtx timeout is
ineffective. Replace the Transport.Proxy assignment with
http.ProxyFromEnvironment and, inside the retry closure passed to
retry.RetryOnConnectionErrors, call req.Clone(ctx) and use client.Do(clonedReq)
so each attempt inherits the retry attempt context (retryCtx). Apply the same
change to the equivalent code in oauthapiserver generate logic.
In `@pkg/controllers/externaloidc/generation/oauthapiserver/generate.go`:
- Around line 884-910: The custom proxy function in the http.Client Transport
(where Proxy is currently using httpproxy.FromEnvironment and parsing
HTTPSProxy) should be replaced with the standard http.ProxyFromEnvironment to
respect NO_PROXY, and the retry loop that calls client.Do(req) inside
retry.RetryOnConnectionErrors must pass the per-attempt context so the 10s
deadline cancels in-flight requests; update the retry invocation to call
client.Do(req.Clone(ctx)) (or recreate the request with
http.NewRequestWithContext) instead of reusing req, and remove the custom Proxy
lambda in favor of http.ProxyFromEnvironment in the http.Client construction.
---
Outside diff comments:
In `@pkg/controllers/externaloidc/externaloidc_controller_test.go`:
- Around line 438-443: Tests instantiate externalOIDCController without setting
authConfigGenerator, but sync() now dereferences c.authConfigGenerator causing
panics; update the test setup that builds externalOIDCController to set
c.authConfigGenerator to a simple stub generator (e.g., a small function or
struct implementing the generator interface that returns a static/empty auth
config and no error) so each testcase provides a non-nil authConfigGenerator;
reference the externalOIDCController struct instantiation and the sync() call to
locate where to add the stub.
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 103-118: The code is persisting secret-bearing oauth client
secrets into the managed ConfigMap (openshift-config-managed/auth-config) by
applying the full object returned from getExpectedApplyConfig; modify the flow
so secret material is not written into a ConfigMap: either strip/omit secret
fields (e.g., remove client-secret entries) from the returned object in
getExpectedApplyConfig (or call a new sanitizeAuthConfig function) before
calling c.configMaps.ConfigMaps(...).Apply, or persist the sensitive parts into
a Secret (use c.secrets.Secrets(...).Apply) and leave only non-secret references
in the ConfigMap; update references to targetAuthConfigCMName/managedNamespace
and ensure c.getExistingApplyConfig comparison uses the sanitized form to avoid
reapplying secrets.
- Around line 73-84: The controller currently only watches ConfigMaps and
Authentications but must also watch Secrets used by the external-claims-sourcing
generator; update the informer registration in the factory chain (the block
starting with factory.New().WithInformers(...) and
WithFilteredEventsInformers(...)) to add a Secrets informer—e.g., include
kubeInformersForNamespaces.InformersFor(managedNamespace).Core().V1().Secrets().Informer()
(or a NamesFilter-wrapped Secrets informer for the specific secret names) so
that SecretLister() updates trigger c.sync; only add this Secrets informer when
FeatureGateExternalOIDCExternalClaimsSourcing is enabled so the controller
resyncs on secret rotation/client-credential changes.
🪄 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: 61c219d4-b89b-4932-a2b5-f6d28261ac00
⛔ Files ignored due to path filters (84)
go.sumis excluded by!**/*.sumvendor/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/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_dns.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/envtest-releases.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1alpha1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/quota/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/security/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/security/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*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 (6)
go.modpkg/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.go
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 |
openshift-ci
Bot
removed
the
approved
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controllers/externaloidc/externaloidc_controller_test.go (1)
3-44:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused imports and dead helper functions.
The crypto-related imports (
crypto,crypto/ecdsa,crypto/elliptic,crypto/rand,crypto/rsa,crypto/tls,crypto/x509,crypto/x509/pkix,net, andcertutil) are only used within the helper functionsgenerateCAKeyPair()andgenerateServingCert()(lines 472–526), which are never called in this test file. These functions and their associated imports should be removed.🤖 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_test.go` around lines 3 - 44, Remove the unused crypto-related imports (crypto, crypto/ecdsa, crypto/elliptic, crypto/rand, crypto/rsa, crypto/tls, crypto/x509, crypto/x509/pkix, net, and certutil) and delete the dead helper functions generateCAKeyPair() and generateServingCert() which are not referenced anywhere in the test file; ensure you only remove those specific imports and the two functions so no other tests or helpers are affected, and run `go vet`/`go test` to confirm there are no remaining unused-import or undefined-symbol errors.
♻️ Duplicate comments (1)
pkg/controllers/externaloidc/generation/common.go (1)
38-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse standard proxy resolver and bind requests to retry context.
The custom
Proxyfunction only checksHTTPS_PROXYand ignores standard proxy environment variables likeNO_PROXY,HTTP_PROXY(lowercase variants), etc. Additionally,client.Do(req)uses the original request without binding to the retry context, so the 10-secondretryCtxtimeout doesn't actually limit in-flight requests.Proposed fix
+ "net/http" - "golang.org/x/net/http/httpproxy" ... client := &http.Client{ Transport: &http.Transport{ TLSClientConfig: &tls.Config{RootCAs: caCertPool}, - 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, }, Timeout: 5 * time.Second, } ... retry.RetryOnConnectionErrors(retryCtx, func(ctx context.Context) (done bool, err error) { - resp, connErr = client.Do(req) + resp, connErr = client.Do(req.Clone(ctx)) return connErr == nil, connErr })🤖 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 38 - 68, In ValidateCACert replace the custom Transport.Proxy implementation with the standard http.ProxyFromEnvironment so all proxy env vars (HTTP_PROXY/HTTPS_PROXY/no_proxy etc.) and their canonicalization are respected, and ensure requests executed in retry.RetryOnConnectionErrors are bound to the retry context by using req.WithContext(ctx) (or creating a new request inside the retry closure) so the 10s retryCtx deadline actually cancels in-flight client.Do calls; reference ValidateCACert, the Transport.Proxy field, client.Do(req), retryCtx and req.WithContext(ctx) when making the changes.
🧹 Nitpick comments (4)
pkg/controllers/externaloidc/generation/common.go (1)
40-41: 💤 Low valueConsider setting TLS MinVersion for production security.
The
tls.Configlacks aMinVersionsetting. While Go defaults to TLS 1.2 for clients, explicitly settingMinVersion: tls.VersionTLS12(ortls.VersionTLS13if legacy OIDC providers aren't a concern) documents the security posture and prevents accidental downgrades if defaults change.🤖 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 40 - 41, The TLS configuration for the HTTP client (the http.Transport with TLSClientConfig currently set to &tls.Config{RootCAs: caCertPool}) lacks an explicit MinVersion; update the TLSConfig used by Transport to include MinVersion: tls.VersionTLS12 (or tls.VersionTLS13 if you can drop legacy OIDC support) so the client enforces a minimum TLS protocol version and prevents downgrades.pkg/controllers/externaloidc/generation/kubeapiserver/generate.go (1)
336-352: 💤 Low valueUID mapping default case may be unreachable.
The switch statement handles
uid == nil, claim-only, expression-only, and both-set cases. Thedefaultcase at line 350 would only be reached if bothClaimandExpressionare empty strings (not nil), which seems like it should either be handled explicitly or fall through to theuid == nilcase to default to "sub".Consider simplifying
switch { case uid == nil: out.Claim = "sub" +case len(uid.Claim) == 0 && len(uid.Expression) == 0: + out.Claim = "sub" case len(uid.Claim) > 0 && len(uid.Expression) == 0: out.Claim = uid.Claim case len(uid.Expression) > 0 && len(uid.Claim) == 0: // ... CEL validation case len(uid.Claim) > 0 && len(uid.Expression) > 0: return apiserverv1beta1.ClaimOrExpression{}, fmt.Errorf("uid mapping must set either claim or expression, not both: %v", uid) -default: - return apiserverv1beta1.ClaimOrExpression{}, fmt.Errorf("unable to handle uid mapping: %v", uid) }🤖 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` around lines 336 - 352, The default branch in the switch handling uid mapping is effectively unreachable for the case where uid is non-nil but both Claim and Expression are empty; update the logic in generate.go (the switch operating on uid that sets out.Claim/out.Expression and calls generation.ValidateClaimsCELExpression) to explicitly treat a non-nil uid with empty Claim and Expression the same as uid == nil (i.e., set out.Claim = "sub") or otherwise return a clear error; remove or replace the current default branch that returns "unable to handle uid mapping" so the function consistently handles the empty-strings case (either by defaulting to "sub" or by returning a validation error) and keep references to generation.ValidateClaimsCELExpression and authenticationcel.ClaimMappingExpression as needed.pkg/controllers/externaloidc/generation/common_test.go (1)
91-108: 💤 Low valueTest relies on fragile timing assumption.
The "well-known request error" test uses a 6-second sleep to trigger the 5-second client timeout. This could be flaky if CI environments are slow or if timeout values change. Consider using a handler that immediately closes the connection or returns after the context is cancelled.
That said, the current approach works and is straightforward to understand.
🤖 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 91 - 108, The test "well-known request error" should avoid relying on a 6s sleep to trigger the client timeout; replace the handlerFunc used with createTestServer so it immediately simulates a broken connection (for example by using http.Hijacker on the ResponseWriter and closing the underlying net.Conn) or otherwise immediately close the connection/return an error, then assert ValidateCACert(testServer.URL, certPool) returns an error; update the handlerFunc in the test to perform the immediate-close hijack (referencing handlerFunc, createTestServer, and ValidateCACert) instead of time.Sleep to make the test deterministic and CI-stable.pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go (1)
149-1221: ⚡ Quick winAdd focused cases for
ExternalClaimsSourcesgeneration path.This suite is strong on core claim-mapping/issuer behavior, but it currently doesn’t exercise provider
ExternalClaimsSourcespayloads (URL/path expression, auth mode, TLS CA, mappings, predicates). Given this PR’s scope, adding at least one happy-path and one invalid-path case would materially reduce regression risk.🤖 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 149 - 1221, Add focused test cases covering ExternalClaimsSources in TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration: create one "happy-path" case where auth.Spec.OIDCProviders[].ExternalClaimsSources has a valid source (URL, PathExpression, AuthMode, TLS CA referencing baseCABundleConfigMap, mappings and predicates) and assert expectedAuthConfig via authConfigWithUpdates to include the generated external claims mapping; and one "invalid-path" case (e.g. bad URL, missing PathExpression, unsupported AuthMode, or missing TLS CA) that sets expectError=true. Ensure both cases enable the features.FeatureGateExternalOIDCExternalClaimsSourcing in featureGates and, where TLS CA is used, supply caBundleConfigMap/baseCABundleConfigMap; use authWithUpdates to modify baseAuthResource and reference authConfigWithUpdates to build expectedAuthConfig.
🤖 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/validation/validator_test.go`:
- Around line 126-135: The test case in validator_test.go incorrectly expects
CEL compilation errors even though AuthenticationConfigurationValidator only
validates CA certificates; update the failing case that mutates
authConfig.JWT[0].ClaimMappings.UID (the ClaimOrExpression with Expression
"#@!$&*(^)") to not expect an error (set expectError to false) or remove the
case entirely, or alternatively extend AuthenticationConfigurationValidator to
perform CEL syntax validation if you intend to keep the expectError=true
behavior; reference AuthenticationConfigurationValidator, authConfigWithUpdates,
and ClaimOrExpression to locate the change.
In
`@pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator.go`:
- Around line 36-42: The error message in the validation block misplaces
certMessage where the validated URL should appear; update the fmt.Errorf call in
the validator (the block calling generation.ValidateCACert) to include the
actual url variable and also mention certMessage for context—e.g. format the
message to contain url first and then certMessage (or both) before the error, so
the output shows the IDP URL being validated and whether system or specified CAs
were used.
In `@pkg/controllers/externaloidc/generation/oauthapiserver/generate.go`:
- Around line 709-724: The loop currently accumulates validation errors into
errs but still appends every predicate to out and always returns nil; change the
logic in the loop so that when validation.ValidateExternalSourceCondition(...)
returns an error you append a formatted error to errs and do NOT append that
invalid predicate to out, and after the loop if errs is non-empty return an
aggregated error (e.g. utilerrors.NewAggregate or fmt.Errorf combined) instead
of nil; update references in this code path (externalSourcePredicates,
seenConditions, validation.ValidateExternalSourceCondition,
kubeErrorListToGoError, out) so callers receive a non-nil error for validation
failures.
- Around line 672-673: The call to ValidateExternalClaimsSourceURLPathExpression
is wired to the wrong field: it currently passes &sourceURL.Hostname so path
expressions are never validated; change the argument to
&sourceURL.PathExpression (keeping externaloidccel.NewCompiler(),
field.NewPath(""), and the kubeErrorListToGoError wrapping intact) so the
validator runs against the actual PathExpression field.
In
`@pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go`:
- Around line 126-135: The test fails because
ValidateAuthenticationConfiguration() does not validate CEL expressions; either
update the validator to run CEL checks or remove the test case. To fix, add CEL
validation calls inside
AuthenticationConfigurationValidator.ValidateAuthenticationConfiguration(): for
JWT/claim mappings call generation.ValidateClaimsCELExpression() (and
generation.ValidateUserCELExpression() for user-related expressions) on fields
like JWT[0].ClaimMappings.UID.Expression and other ClaimOrExpression entries,
return an error if those validations fail; alternatively, if you prefer not to
change runtime validation, remove or change the "cel expression can not compile"
test case so it no longer expects an error.
In
`@pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator.go`:
- Around line 36-42: The error message in validator.go inside the
generation.ValidateCACert error path is missing the actual URL and misplaces the
cert description; update the fmt.Errorf call in that block (where
generation.ValidateCACert(url, caCertPool) is checked) to include the URL and
then the certMessage (e.g., "could not validate IDP URL %s using %s: %v", url,
certMessage, err) so the returned error includes both the IDP URL and which CA
set was used.
---
Outside diff comments:
In `@pkg/controllers/externaloidc/externaloidc_controller_test.go`:
- Around line 3-44: Remove the unused crypto-related imports (crypto,
crypto/ecdsa, crypto/elliptic, crypto/rand, crypto/rsa, crypto/tls, crypto/x509,
crypto/x509/pkix, net, and certutil) and delete the dead helper functions
generateCAKeyPair() and generateServingCert() which are not referenced anywhere
in the test file; ensure you only remove those specific imports and the two
functions so no other tests or helpers are affected, and run `go vet`/`go test`
to confirm there are no remaining unused-import or undefined-symbol errors.
---
Duplicate comments:
In `@pkg/controllers/externaloidc/generation/common.go`:
- Around line 38-68: In ValidateCACert replace the custom Transport.Proxy
implementation with the standard http.ProxyFromEnvironment so all proxy env vars
(HTTP_PROXY/HTTPS_PROXY/no_proxy etc.) and their canonicalization are respected,
and ensure requests executed in retry.RetryOnConnectionErrors are bound to the
retry context by using req.WithContext(ctx) (or creating a new request inside
the retry closure) so the 10s retryCtx deadline actually cancels in-flight
client.Do calls; reference ValidateCACert, the Transport.Proxy field,
client.Do(req), retryCtx and req.WithContext(ctx) when making the changes.
---
Nitpick comments:
In `@pkg/controllers/externaloidc/generation/common_test.go`:
- Around line 91-108: The test "well-known request error" should avoid relying
on a 6s sleep to trigger the client timeout; replace the handlerFunc used with
createTestServer so it immediately simulates a broken connection (for example by
using http.Hijacker on the ResponseWriter and closing the underlying net.Conn)
or otherwise immediately close the connection/return an error, then assert
ValidateCACert(testServer.URL, certPool) returns an error; update the
handlerFunc in the test to perform the immediate-close hijack (referencing
handlerFunc, createTestServer, and ValidateCACert) instead of time.Sleep to make
the test deterministic and CI-stable.
In `@pkg/controllers/externaloidc/generation/common.go`:
- Around line 40-41: The TLS configuration for the HTTP client (the
http.Transport with TLSClientConfig currently set to &tls.Config{RootCAs:
caCertPool}) lacks an explicit MinVersion; update the TLSConfig used by
Transport to include MinVersion: tls.VersionTLS12 (or tls.VersionTLS13 if you
can drop legacy OIDC support) so the client enforces a minimum TLS protocol
version and prevents downgrades.
In `@pkg/controllers/externaloidc/generation/kubeapiserver/generate.go`:
- Around line 336-352: The default branch in the switch handling uid mapping is
effectively unreachable for the case where uid is non-nil but both Claim and
Expression are empty; update the logic in generate.go (the switch operating on
uid that sets out.Claim/out.Expression and calls
generation.ValidateClaimsCELExpression) to explicitly treat a non-nil uid with
empty Claim and Expression the same as uid == nil (i.e., set out.Claim = "sub")
or otherwise return a clear error; remove or replace the current default branch
that returns "unable to handle uid mapping" so the function consistently handles
the empty-strings case (either by defaulting to "sub" or by returning a
validation error) and keep references to generation.ValidateClaimsCELExpression
and authenticationcel.ClaimMappingExpression as needed.
In `@pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go`:
- Around line 149-1221: Add focused test cases covering ExternalClaimsSources in
TestAuthenticationConfigurationGeneratorGenerateAuthenticationConfiguration:
create one "happy-path" case where
auth.Spec.OIDCProviders[].ExternalClaimsSources has a valid source (URL,
PathExpression, AuthMode, TLS CA referencing baseCABundleConfigMap, mappings and
predicates) and assert expectedAuthConfig via authConfigWithUpdates to include
the generated external claims mapping; and one "invalid-path" case (e.g. bad
URL, missing PathExpression, unsupported AuthMode, or missing TLS CA) that sets
expectError=true. Ensure both cases enable the
features.FeatureGateExternalOIDCExternalClaimsSourcing in featureGates and,
where TLS CA is used, supply caBundleConfigMap/baseCABundleConfigMap; use
authWithUpdates to modify baseAuthResource and reference authConfigWithUpdates
to build expectedAuthConfig.
🪄 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: ba40a147-7651-4efa-860b-c5de82336230
⛔ Files ignored due to path filters (78)
go.sumis excluded by!**/*.sumvendor/github.com/coreos/go-oidc/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-oidc/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-oidc/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-oidc/DCOis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-oidc/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-oidc/MAINTAINERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-oidc/NOTICEis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-oidc/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-oidc/code-of-conduct.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-oidc/jose.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-oidc/jwks.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-oidc/oidc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-oidc/testis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/go-oidc/verify.gois excluded by!**/vendor/**,!vendor/**vendor/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/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/conversion/conversion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/apis/authentication/validation/validation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/cel/accessors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/cel/compiler.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/cel/mappers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/oidc/externalclaims/resolver/resolver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/oidc/externalclaims/tokengetters/anonymous.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/oidc/externalclaims/tokengetters/clientcredential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/oidc/externalclaims/tokengetters/requestprovided.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/oauth-apiserver/pkg/externaloidc/oidc/oidc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pquerna/cachecontrol/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/pquerna/cachecontrol/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/pquerna/cachecontrol/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/pquerna/cachecontrol/api.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pquerna/cachecontrol/cacheobject/directive.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pquerna/cachecontrol/cacheobject/lex.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pquerna/cachecontrol/cacheobject/object.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pquerna/cachecontrol/cacheobject/reasons.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pquerna/cachecontrol/cacheobject/warning.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pquerna/cachecontrol/doc.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/ed25519/ed25519.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/pbkdf2/pbkdf2.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/oauth2/clientcredentials/clientcredentials.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/.gitcookies.sh.encis excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/README.mdis excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/asymmetric.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/cipher/cbc_hmac.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/cipher/concat_kdf.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/cipher/ecdh_es.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/cipher/key_wrap.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/crypter.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/doc.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/encoding.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/json/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/json/README.mdis excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/json/decode.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/json/encode.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/json/indent.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/json/scanner.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/json/stream.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/json/tags.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/jwe.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/jwk.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/jws.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/opaque.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/shared.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/signing.gois excluded by!**/vendor/**,!vendor/**vendor/gopkg.in/go-jose/go-jose.v2/symmetric.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apiserver/pkg/authentication/token/jwt/jwt.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/metrics.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (13)
go.modpkg/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
✅ 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)
- go.mod
openshift-ci
Bot
added
the
needs-rebase
everettraven
force-pushed
the
feature/ext-claims-impl
branch
from
834e097 to
7438b6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
pkg/controllers/externaloidc/generation/oauthapiserver/generate.go (1)
578-578: ⚡ Quick winUnused variable
printableASCIIRegexp.This regexp is defined but never referenced in the code. It appears to be leftover from commented-out validation code.
Proposed fix
-var printableASCIIRegexp = regexp.MustCompile(`^[[:print:]]+$`)Or add a comment indicating this will be used when validation is enabled:
+// printableASCIIRegexp is used for client secret validation when enabled. +// TODO: Remove this comment when validation is uncommented. var printableASCIIRegexp = regexp.MustCompile(`^[[:print:]]+$`)🤖 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` at line 578, The declared variable printableASCIIRegexp in generate.go is unused; either remove the declaration or document why it remains (e.g., add a comment stating it is reserved for future validation) so linters won't flag it. Locate the regexp variable named printableASCIIRegexp and either delete that line or replace it with a short explanatory comment referencing its intended use in future validation logic (e.g., for username/label printable-ASCII checks).pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go (1)
1789-1790: 💤 Low valueGenerator validator field is set directly instead of through constructor.
The test sets
c.validatordirectly after construction. Consider adding aWithValidatoroption or setter method to the generator for cleaner API design.🤖 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 1789 - 1790, The test directly assigns the private validator field (c.validator) on the object returned by NewAuthenticationConfigurationGenerator; instead, add a public option or setter to inject the validator (e.g., implement a WithValidator functional option or a SetValidator method on AuthenticationConfigurationGenerator) and update NewAuthenticationConfigurationGenerator to accept options or expose the setter, then change tests to use NewAuthenticationConfigurationGenerator(..., WithValidator(tt.configValidator)) or c.SetValidator(tt.configValidator) and remove direct field assignment.pkg/controllers/externaloidc/generation/common_test.go (2)
43-48: 💤 Low valueDuplicate test cases testing the same scenario.
Both "nil CA cert to use system CAs" (line 43) and "nil cert pool" (line 77) test the same condition - passing
niltoValidateCACert. Consider removing one to avoid redundancy.Also applies to: 77-82
🤖 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 43 - 48, Two tests duplicate the same scenario by calling ValidateCACert with nil (test names "nil CA cert to use system CAs" and "nil cert pool"); remove one of the duplicate cases (either the t.Run block using "nil CA cert to use system CAs" or the "nil cert pool" block) and keep a single clearly named test for the nil-CA behavior (e.g., "nil CA cert uses system CAs") to avoid redundancy; ensure the remaining test continues to call ValidateCACert(testServer.URL, nil) and asserts an error as before.
91-108: ⚡ Quick winTest with 6-second sleep may slow CI.
The "well-known request error" test waits 6 seconds for a timeout. Since
ValidateCACerthas a 5-second client timeout and 10-second retry context, consider reducing the sleep or using a handler that closes the connection immediately to speed up tests.Alternative: close connection immediately
t.Run("well-known request error", func(t *testing.T) { - handlerFunc := func(w http.ResponseWriter, r *http.Request) { - time.Sleep(6 * time.Second) - w.WriteHeader(http.StatusOK) - } + handlerFunc := func(w http.ResponseWriter, r *http.Request) { + // Hijack and close connection to simulate network error + hj, ok := w.(http.Hijacker) + if ok { + conn, _, _ := hj.Hijack() + conn.Close() + } + }🤖 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 91 - 108, The "well-known request error" test uses a 6s sleep in handlerFunc which slows CI; update the handler in this test (the closure used to createTestServer) to either close the connection immediately (e.g., by calling Hijack/Close on the ResponseWriter) or use a much shorter sleep well below the client timeout (e.g., 500–1000ms) so ValidateCACert still triggers its error path without waiting 6 seconds; change the handlerFunc in this test accordingly and keep the test name and call to ValidateCACert unchanged.pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go (1)
149-239: ⚖️ Poor tradeoffConsider extracting shared test helpers to reduce duplication.
The
createTestServer,generateCAKeyPair,generateServingCert, andauthConfigWithUpdatesfunctions are duplicated across multiple test files (common_test.go,kubeapiserver/validation/validator_test.go, and this file). Consider extracting to a sharedtestutilpackage.🤖 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 149 - 239, Extract the duplicated test helpers createTestServer, generateCAKeyPair, generateServingCert, and authConfigWithUpdates into a shared testutil package: create a new pkg/.../testutil package (or internal testutil) containing these functions, update their package names/visibility if needed, replace the local copies in common_test.go, kubeapiserver/validation/validator_test.go and this file to import testutil and call testutil.createTestServer / testutil.generateCAKeyPair / testutil.generateServingCert / testutil.authConfigWithUpdates, and run/update imports and any references to types (e.g., change unexported identifiers to exported if cross-package usage requires it) so all tests compile and use the single shared helpers.
🤖 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 42-47: The custom Proxy func currently calls
httpproxy.FromEnvironment().HTTPSProxy which ignores NO_PROXY; replace that
lambda with the standard http.ProxyFromEnvironment function so proxy resolution
respects NO_PROXY and other env rules. Locate the Proxy: func(*http.Request)
(*url.URL, error) block in common.go and assign http.ProxyFromEnvironment (or
return its result) instead of manually calling httpproxy.FromEnvironment and
url.Parse, ensuring the Proxy field uses the http.ProxyFromEnvironment handler.
- Around line 60-65: The retry loop creates retryCtx but still calls
client.Do(req) with the original request context, so in-flight requests won't be
cancelled when the 10s timeout expires; update the call inside the
retry.RetryOnConnectionErrors closure to use a request bound to retryCtx (e.g.,
clone or call req.WithContext/retryCtx to produce reqWithCtx) and call
client.Do(reqWithCtx) instead of client.Do(req), keeping the existing cancel()
defer as-is.
In `@pkg/controllers/externaloidc/generation/oauthapiserver/generate.go`:
- Around line 667-676: In generateExternalClaimsSourceTLS, validate that
externalSourceTLS.CertificateAuthority is non-nil and that
externalSourceTLS.CertificateAuthority.Name is not empty before calling
getCertificateAuthorityFromConfigMap; if missing, return a clear error (e.g.,
"missing certificate authority name for external source") instead of attempting
a ConfigMap lookup with an empty name so callers get a descriptive failure;
reference the function generateExternalClaimsSourceTLS and the helper
getCertificateAuthorityFromConfigMap and use cmLister only after the name check.
---
Nitpick comments:
In `@pkg/controllers/externaloidc/generation/common_test.go`:
- Around line 43-48: Two tests duplicate the same scenario by calling
ValidateCACert with nil (test names "nil CA cert to use system CAs" and "nil
cert pool"); remove one of the duplicate cases (either the t.Run block using
"nil CA cert to use system CAs" or the "nil cert pool" block) and keep a single
clearly named test for the nil-CA behavior (e.g., "nil CA cert uses system CAs")
to avoid redundancy; ensure the remaining test continues to call
ValidateCACert(testServer.URL, nil) and asserts an error as before.
- Around line 91-108: The "well-known request error" test uses a 6s sleep in
handlerFunc which slows CI; update the handler in this test (the closure used to
createTestServer) to either close the connection immediately (e.g., by calling
Hijack/Close on the ResponseWriter) or use a much shorter sleep well below the
client timeout (e.g., 500–1000ms) so ValidateCACert still triggers its error
path without waiting 6 seconds; change the handlerFunc in this test accordingly
and keep the test name and call to ValidateCACert unchanged.
In `@pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go`:
- Around line 1789-1790: The test directly assigns the private validator field
(c.validator) on the object returned by NewAuthenticationConfigurationGenerator;
instead, add a public option or setter to inject the validator (e.g., implement
a WithValidator functional option or a SetValidator method on
AuthenticationConfigurationGenerator) and update
NewAuthenticationConfigurationGenerator to accept options or expose the setter,
then change tests to use NewAuthenticationConfigurationGenerator(...,
WithValidator(tt.configValidator)) or c.SetValidator(tt.configValidator) and
remove direct field assignment.
In `@pkg/controllers/externaloidc/generation/oauthapiserver/generate.go`:
- Line 578: The declared variable printableASCIIRegexp in generate.go is unused;
either remove the declaration or document why it remains (e.g., add a comment
stating it is reserved for future validation) so linters won't flag it. Locate
the regexp variable named printableASCIIRegexp and either delete that line or
replace it with a short explanatory comment referencing its intended use in
future validation logic (e.g., for username/label printable-ASCII checks).
In
`@pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go`:
- Around line 149-239: Extract the duplicated test helpers createTestServer,
generateCAKeyPair, generateServingCert, and authConfigWithUpdates into a shared
testutil package: create a new pkg/.../testutil package (or internal testutil)
containing these functions, update their package names/visibility if needed,
replace the local copies in common_test.go,
kubeapiserver/validation/validator_test.go and this file to import testutil and
call testutil.createTestServer / testutil.generateCAKeyPair /
testutil.generateServingCert / testutil.authConfigWithUpdates, and run/update
imports and any references to types (e.g., change unexported identifiers to
exported if cross-package usage requires it) so all tests compile and use the
single shared helpers.
🪄 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: 282d5019-9d77-476a-9a3d-5b7916ce231a
⛔ Files ignored due to path filters (89)
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/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_dns.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_kmsencryption.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/envtest-releases.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1alpha1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/quota/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/security/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/security/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*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
✅ 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 (5)
- pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator.go
- pkg/controllers/externaloidc/generation/kubeapiserver/generate_test.go
- pkg/controllers/externaloidc/externaloidc_controller_test.go
- pkg/controllers/externaloidc/generation/kubeapiserver/generate.go
- pkg/controllers/externaloidc/externaloidc_controller.go
everettraven
force-pushed
the
feature/ext-claims-impl
branch
from
7438b6f to
76f1602
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/controllers/externaloidc/generation/common_test.go (1)
43-48: 💤 Low valueDuplicate test cases for nil cert pool.
The test cases "nil CA cert to use system CAs" (line 43) and "nil cert pool" (line 77) both pass
niltoValidateCACertand expect an error. Consider consolidating them into a single test case or differentiating their purpose.Also applies to: 77-82
🤖 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 43 - 48, The two duplicate tests both pass nil to ValidateCACert ("nil CA cert to use system CAs" and "nil cert pool")—consolidate them into a single test or make their intent distinct; update the test suite by removing one of the duplicated t.Run blocks and keeping a single case that calls ValidateCACert(testServer.URL, nil) and asserts an error, or rename and change one test to exercise a different scenario (e.g., an empty x509.CertPool vs. nil) so both ValidateCACert and the test names clearly reflect unique behaviors; ensure the unique identifiers ValidateCACert and the t.Run names are adjusted accordingly.pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go (1)
149-239: ⚖️ Poor tradeoffConsider extracting shared test helpers to reduce duplication.
The helper functions
createTestServer,generateCAKeyPair, andgenerateServingCertare duplicated acrosspkg/controllers/externaloidc/generation/common_test.go, this file, andkubeapiserver/validation/validator_test.go. Consider extracting them into a sharedtestutilpackage under thegenerationdirectory.🤖 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 149 - 239, Extract the duplicated helpers createTestServer, generateCAKeyPair, and generateServingCert into a shared test helper package (e.g., generation/testutil); move the implementations from this file and the other locations into that package, export them if needed, update callers in pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go, pkg/controllers/externaloidc/generation/common_test.go and kubeapiserver/validation/validator_test.go to import the new testutil package and call testutil.CreateTestServer, testutil.GenerateCAKeyPair, and testutil.GenerateServingCert, and remove the now-duplicate local definitions so all tests use the single shared implementation.
🤖 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 `@go.mod`:
- Line 19: Replace the all-zero sentinel pseudo-version in go.mod for module
github.com/openshift/oauth-apiserver with the valid pseudo-version provided;
locate the line containing "github.com/openshift/oauth-apiserver
v0.0.0-00010101000000-000000000000" and change that version to
"v0.0.0-20260430140618-160ac7fb4ea6c0d5" so the module resolves without the
local replace directive.
In `@pkg/controllers/externaloidc/generation/common.go`:
- Around line 39-50: The TLS config used to create the HTTP client in the
http.Client/http.Transport setup (see tls.Config initialized with RootCAs:
caCertPool) needs a minimum TLS version; update the tls.Config in the Transport
to include MinVersion: tls.VersionTLS12 so the OIDC certificate validation
enforces TLS 1.2+.
---
Nitpick comments:
In `@pkg/controllers/externaloidc/generation/common_test.go`:
- Around line 43-48: The two duplicate tests both pass nil to ValidateCACert
("nil CA cert to use system CAs" and "nil cert pool")—consolidate them into a
single test or make their intent distinct; update the test suite by removing one
of the duplicated t.Run blocks and keeping a single case that calls
ValidateCACert(testServer.URL, nil) and asserts an error, or rename and change
one test to exercise a different scenario (e.g., an empty x509.CertPool vs. nil)
so both ValidateCACert and the test names clearly reflect unique behaviors;
ensure the unique identifiers ValidateCACert and the t.Run names are adjusted
accordingly.
In
`@pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go`:
- Around line 149-239: Extract the duplicated helpers createTestServer,
generateCAKeyPair, and generateServingCert into a shared test helper package
(e.g., generation/testutil); move the implementations from this file and the
other locations into that package, export them if needed, update callers in
pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go,
pkg/controllers/externaloidc/generation/common_test.go and
kubeapiserver/validation/validator_test.go to import the new testutil package
and call testutil.CreateTestServer, testutil.GenerateCAKeyPair, and
testutil.GenerateServingCert, and remove the now-duplicate local definitions so
all tests use the single shared implementation.
🪄 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: 45c6e8a9-3a74-47f1-8efe-8cb145dafac5
⛔ Files ignored due to path filters (29)
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/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*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
✅ 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/kubeapiserver/validation/validator.go
- pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator.go
- pkg/controllers/externaloidc/generation/oauthapiserver/generate.go
- pkg/controllers/externaloidc/externaloidc_controller_test.go
- pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go
- pkg/controllers/externaloidc/externaloidc_controller.go
- pkg/controllers/externaloidc/generation/kubeapiserver/generate_test.go
- pkg/controllers/externaloidc/generation/kubeapiserver/generate.go
everettraven
force-pushed
the
feature/ext-claims-impl
branch
from
76f1602 to
f786628
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/controllers/externaloidc/generation/common.go (2)
39-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
MinVersion: tls.VersionTLS12to the TLS configuration and usehttp.ProxyFromEnvironment.The TLS configuration lacks a minimum version setting, and the custom proxy function bypasses
NO_PROXYenvironment variable handling. These issues were flagged in previous reviews and by static analysis.🔒 Proposed fix
client := &http.Client{ Transport: &http.Transport{ - TLSClientConfig: &tls.Config{RootCAs: caCertPool}, - Proxy: func(*http.Request) (*url.URL, error) { - if proxyConfig := httpproxy.FromEnvironment(); len(proxyConfig.HTTPSProxy) > 0 { - return url.Parse(proxyConfig.HTTPSProxy) - } - return nil, nil - }, + TLSClientConfig: &tls.Config{ + RootCAs: caCertPool, + MinVersion: tls.VersionTLS12, + }, + Proxy: http.ProxyFromEnvironment, }, Timeout: 5 * time.Second, }🤖 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 39 - 50, The TLS config for the http client (constructed as client := &http.Client{ Transport: &http.Transport{ TLSClientConfig: &tls.Config{RootCAs: caCertPool}, Proxy: func... }}) should enforce a minimum TLS version and delegate proxy resolution to the stdlib; update the tls.Config used in the Transport to include MinVersion: tls.VersionTLS12 and replace the custom Proxy func with Proxy: http.ProxyFromEnvironment so NO_PROXY/NOHTTP/NOHTTPS semantics are respected.
58-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAttach retry context to the request so in-flight requests are cancelled on timeout.
The
retryCtxcontrols the retry loop butclient.Do(req)uses the original request context. In-flight requests won't be cancelled when the 10-second deadline expires.🐛 Proposed fix
var resp *http.Response var connErr error retryCtx, cancel := context.WithTimeout(req.Context(), 10*time.Second) defer cancel() retry.RetryOnConnectionErrors(retryCtx, func(ctx context.Context) (done bool, err error) { - resp, connErr = client.Do(req) + resp, connErr = client.Do(req.Clone(ctx)) return connErr == nil, connErr })🤖 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 58 - 68, The retry loop uses retryCtx but client.Do(req) still uses the original request context; update the request to carry retryCtx so in-flight HTTP calls are cancelled on timeout. Create a contextualized request (e.g., reqWithCtx := req.WithContext(retryCtx) or req.Clone(retryCtx)) and call client.Do(reqWithCtx) inside the RetryOnConnectionErrors callback (or recreate per-attempt if needed) so RetryOnConnectionErrors, client.Do, retryCtx, resp and connErr continue to work with the 10s deadline.
🧹 Nitpick comments (3)
pkg/controllers/externaloidc/generation/common_test.go (1)
70-75: 💤 Low valueVariable shadowing:
erris reassigned but the original outer-scope error is not rechecked.At line 71,
erris reassigned fromValidateCACert, but the outer-scopeerrfromgenerateCAKeyPairat line 58 is not re-verified before the check at line 64. While this works because thegenerateCAKeyPairerror path returns early at line 60-61, the shadowing could cause confusion.Consider using a different variable name or restructuring:
♻️ Proposed fix
t.Run("mismatched CA cert", func(t *testing.T) { anotherCACert, _, err := generateCAKeyPair() if err != nil { t.Errorf("could not generate CA keypair: %v", err) } certPool := x509.NewCertPool() certPool.AddCert(anotherCACert) - err = ValidateCACert(testServer.URL, certPool) + validateErr := ValidateCACert(testServer.URL, certPool) - if err == nil { + if validateErr == nil { t.Errorf("did not get an error while expecting one") } })🤖 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 70 - 75, The test shadows the outer err from generateCAKeyPair by reassigning err inside the t.Run closure when calling ValidateCACert, which is confusing; update the test so the ValidateCACert call uses a new local variable name (e.g., vErr or validateErr) or explicitly reuse the outer err in a clear way to avoid shadowing, ensuring generateCAKeyPair's returned error remains distinct and still handled; update references in the closure accordingly (look for generateCAKeyPair, ValidateCACert, and the err variable in the t.Run "unknown URL" block).pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go (1)
149-239: ⚖️ Poor tradeoffConsider extracting shared test helpers to reduce duplication.
The
createTestServer,generateCAKeyPair, andgenerateServingCertfunctions are duplicated across multiple test files. Consider extracting these to a shared test utilities package (e.g.,pkg/controllers/externaloidc/testutil/) to improve maintainability.🤖 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 149 - 239, Extract the duplicated helpers createTestServer, generateCAKeyPair, and generateServingCert into a shared test utility package (e.g., pkg/controllers/externaloidc/testutil) and export them (rename to CreateTestServer, GenerateCAKeyPair, GenerateServingCert) so other test packages can import and reuse them; move their implementations into a new .go file under that package, update all test files that currently define those functions to import the new testutil package and call the exported names, and run `go test ./...` to ensure imports and signatures match.pkg/controllers/externaloidc/externaloidc_controller_test.go (1)
472-532: 💤 Low valueRemove unused helper functions to reduce code duplication.
The
generateCAKeyPairandgenerateServingCerthelper functions are not called in this test file. These functions are already defined and actively used incommon_test.goandvalidator_test.go, making the copies here redundant. Removing them will reduce maintenance burden.🤖 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_test.go` around lines 472 - 532, Delete the unused helper functions generateCAKeyPair and generateServingCert from this test file (they're duplicates of implementations in common_test.go / validator_test.go); remove any now-unused imports that only supported these functions and run tests/build to verify nothing else references 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.
Duplicate comments:
In `@pkg/controllers/externaloidc/generation/common.go`:
- Around line 39-50: The TLS config for the http client (constructed as client
:= &http.Client{ Transport: &http.Transport{ TLSClientConfig:
&tls.Config{RootCAs: caCertPool}, Proxy: func... }}) should enforce a minimum
TLS version and delegate proxy resolution to the stdlib; update the tls.Config
used in the Transport to include MinVersion: tls.VersionTLS12 and replace the
custom Proxy func with Proxy: http.ProxyFromEnvironment so
NO_PROXY/NOHTTP/NOHTTPS semantics are respected.
- Around line 58-68: The retry loop uses retryCtx but client.Do(req) still uses
the original request context; update the request to carry retryCtx so in-flight
HTTP calls are cancelled on timeout. Create a contextualized request (e.g.,
reqWithCtx := req.WithContext(retryCtx) or req.Clone(retryCtx)) and call
client.Do(reqWithCtx) inside the RetryOnConnectionErrors callback (or recreate
per-attempt if needed) so RetryOnConnectionErrors, client.Do, retryCtx, resp and
connErr continue to work with the 10s deadline.
---
Nitpick comments:
In `@pkg/controllers/externaloidc/externaloidc_controller_test.go`:
- Around line 472-532: Delete the unused helper functions generateCAKeyPair and
generateServingCert from this test file (they're duplicates of implementations
in common_test.go / validator_test.go); remove any now-unused imports that only
supported these functions and run tests/build to verify nothing else references
them.
In `@pkg/controllers/externaloidc/generation/common_test.go`:
- Around line 70-75: The test shadows the outer err from generateCAKeyPair by
reassigning err inside the t.Run closure when calling ValidateCACert, which is
confusing; update the test so the ValidateCACert call uses a new local variable
name (e.g., vErr or validateErr) or explicitly reuse the outer err in a clear
way to avoid shadowing, ensuring generateCAKeyPair's returned error remains
distinct and still handled; update references in the closure accordingly (look
for generateCAKeyPair, ValidateCACert, and the err variable in the t.Run
"unknown URL" block).
In
`@pkg/controllers/externaloidc/generation/oauthapiserver/validation/validator_test.go`:
- Around line 149-239: Extract the duplicated helpers createTestServer,
generateCAKeyPair, and generateServingCert into a shared test utility package
(e.g., pkg/controllers/externaloidc/testutil) and export them (rename to
CreateTestServer, GenerateCAKeyPair, GenerateServingCert) so other test packages
can import and reuse them; move their implementations into a new .go file under
that package, update all test files that currently define those functions to
import the new testutil package and call the exported names, and run `go test
./...` to ensure imports and signatures match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ddf5b43b-3cca-4043-aeb6-3bc9656989fa
⛔ Files ignored due to path filters (29)
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/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*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
✅ 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
- pkg/controllers/externaloidc/generation/kubeapiserver/validation/validator.go
- pkg/controllers/externaloidc/externaloidc_controller.go
- pkg/controllers/externaloidc/generation/oauthapiserver/generate.go
- pkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go
- pkg/controllers/externaloidc/generation/kubeapiserver/generate_test.go
- pkg/controllers/externaloidc/generation/kubeapiserver/generate.go
everettraven
force-pushed
the
feature/ext-claims-impl
branch
3 times, most recently
from
d44cdc3 to
d7ec6d4
Compare
Signed-off-by: Bryce Palmer <[email protected]>
everettraven
force-pushed
the
feature/ext-claims-impl
branch
from
d7ec6d4 to
1b9e58c
Compare
everettraven
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controllers/externaloidc/generation/oauthapiserver/generate.go (1)
842-842: 💤 Low valueRemove unused variable.
printableASCIIRegexpis declared but never used. If this is intended for future validation (per the TODO comments), consider adding it when that validation is enabled, or document its intended purpose.🤖 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` at line 842, The `printableASCIIRegexp` variable declared in the file is not used anywhere in the code and should be removed. If the regex pattern is intended for future validation based on TODO comments in the codebase, either implement the validation when that functionality is enabled, or add a clear comment at the declaration documenting its intended purpose instead of leaving it unused in the code.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@pkg/controllers/externaloidc/generation/oauthapiserver/generate.go`:
- Line 842: The `printableASCIIRegexp` variable declared in the file is not used
anywhere in the code and should be removed. If the regex pattern is intended for
future validation based on TODO comments in the codebase, either implement the
validation when that functionality is enabled, or add a clear comment at the
declaration documenting its intended purpose instead of leaving it unused in the
code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ca9d6496-a75d-4704-ac36-7d56abe0e18b
⛔ Files ignored due to path filters (35)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.ci-operator.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Dockerfile.ocpis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/legacyfeaturegates.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_etcd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/route/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*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/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 (3)
go.modpkg/controllers/externaloidc/generation/oauthapiserver/generate.gopkg/controllers/externaloidc/generation/oauthapiserver/generate_test.go
Signed-off-by: Bryce Palmer <[email protected]>
everettraven
force-pushed
the
feature/ext-claims-impl
branch
from
1b9e58c to
ed229c4
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.
Summary by CodeRabbit
Release Notes
New Features
Tests