[WIP] CNTRLPLANE-3851: Oauth server proxy config e2e - #950
Open
ehearne-redhat wants to merge 21 commits into
Open
[WIP] CNTRLPLANE-3851: Oauth server proxy config e2e#950ehearne-redhat wants to merge 21 commits into
ehearne-redhat wants to merge 21 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@ehearne-redhat: This pull request references CNTRLPLANE-3851 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
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:
WalkthroughComponent-scoped proxy support is added across proxy resolution, transports, OAuth observation, deployment configuration, route and IdP validation, controller wiring, and serial end-to-end tests. Trusted CA ConfigMaps are synchronized into the OAuth Server namespace and support hot reload without rollout-triggering resource hashing. ChangesComponent Proxy Support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AuthenticationCR
participant AuthenticationOperator
participant OAuthServer
participant SquidProxy
participant IdentityProvider
AuthenticationCR->>AuthenticationOperator: Set component proxy and trusted CA
AuthenticationOperator->>OAuthServer: Reconcile proxy environment and CA mount
OAuthServer->>SquidProxy: Send proxied OIDC request
SquidProxy->>IdentityProvider: Forward request
IdentityProvider-->>OAuthServer: Return OIDC response
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controllers/proxyconfig/proxyconfig_controller.go (1)
260-269: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNo
Timeoutset on proxy/IdP-validation HTTP clients.Unlike the comparable clients in
custom_route_conditions.go(Timeout: 5 * time.Second) andendpoint_accessible_controller.go, neitherhttp.Clientreturned here sets aTimeout. If thesynccontext has no deadline,isEndpointReachablecalls against a slow/unreachable proxy or external IdP could block the controller's sync goroutine indefinitely.As per path instructions, Go code should use "context.Context for cancellation and timeouts" for external calls.
🔧 Proposed fix
return &http.Client{ + Timeout: 5 * time.Second, Transport: &http.Transport{ TLSClientConfig: tlsConfig, Proxy: proxyFn, }, }, &http.Client{ + Timeout: 5 * time.Second, Transport: &http.Transport{ TLSClientConfig: tlsConfig, }, }, nil🤖 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/proxyconfig/proxyconfig_controller.go` around lines 260 - 269, Update the HTTP clients returned by the proxy/IdP validation client-construction function to enforce cancellation via context.Context and a bounded timeout, matching the established timeout behavior in custom_route_conditions.go and endpoint_accessible_controller.go. Ensure both the proxy-enabled and direct clients used by isEndpointReachable cannot block indefinitely when the sync context lacks a deadline.Source: Path instructions
pkg/controllers/configobservation/oauth/idp_conversions.go (1)
318-336: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a bounded timeout to outbound OIDC discovery/password-grant requests.
discoverOpenIDURLsandcheckOIDCPasswordGrantFlowhit operator-configured issuer/token endpoints with nohttp.Client.Timeoutor request deadline.buildIDPTransportonly wires CA/proxy settings, so a slow or hung IdP can block the config-observer sync loop indefinitely.🤖 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/configobservation/oauth/idp_conversions.go` around lines 318 - 336, Add a bounded timeout to the outbound HTTP requests in discoverOpenIDURLs and checkOIDCPasswordGrantFlow, using the existing transport configuration while ensuring issuer and token endpoint calls cannot block indefinitely. Apply the timeout via an http.Client or request context/deadline for both discovery and password-grant flows, and preserve their current response and error handling.
🧹 Nitpick comments (2)
test/e2e-component-proxy/component_proxy_oidc_login.go (1)
254-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the no-op
fmt.Sprintf.
fmt.Sprintf("ocp-test-proxy-login-group")has no format args; use a plain string literal (staticcheck S1039).♻️ Proposed fix
- group := fmt.Sprintf("ocp-test-proxy-login-group") + group := "ocp-test-proxy-login-group"🤖 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 `@test/e2e-component-proxy/component_proxy_oidc_login.go` at line 254, Replace the no-op fmt.Sprintf call assigned to group with the plain string literal "ocp-test-proxy-login-group", and remove the fmt import if it is no longer used elsewhere in the file.Source: Linters/SAST tools
test/library/proxy.go (1)
54-93: 🩺 Stability & Availability | 🔵 TrivialUnimplemented helpers will panic when the e2e suite runs.
DeploySquidProxy,DeployProxyNetworkPolicies,GetOAuthServerProxyEnvVars,GetSquidProxyLogs,WaitForSquidProxyTraffic,VerifyOAuthServerDeploymentProxyConfig,VerifyTrustedCAConfigMapSynced, andCheckFeatureGateEnabledOrSkipare allpanic("not implemented"), yet the new[ComponentProxy]specs invoke them — the suite will panic rather than run. Consistent with the WIP status noted in the PR objectives; flagging so it isn't merged into an enabled suite before implementation. Want me to open a tracking issue?🤖 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 `@test/library/proxy.go` around lines 54 - 93, Implement all eight helper functions currently containing panic("not implemented"): DeploySquidProxy, DeployProxyNetworkPolicies, GetOAuthServerProxyEnvVars, GetSquidProxyLogs, WaitForSquidProxyTraffic, VerifyOAuthServerDeploymentProxyConfig, VerifyTrustedCAConfigMapSynced, and CheckFeatureGateEnabledOrSkip. Use the existing Kubernetes clients and each function’s documented contract so the [ComponentProxy] specs execute their deployment, policy, inspection, polling, verification, and feature-gate behavior without panicking.
🤖 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/common/proxy.go`:
- Around line 99-105: Update mergeNoProxy to use the set’s sorted-list operation
before joining entries, replacing the nondeterministic UnsortedList call while
preserving deduplication and comma-separated output.
In `@pkg/controllers/customroute/custom_route_conditions.go`:
- Around line 165-171: Check the boolean result of AppendCertsFromPEM after
loading proxy trusted-CA data and return a descriptive parse error when it is
false. Update rootCAs in pkg/controllers/customroute/custom_route_conditions.go
lines 165-171, returning the error from the surrounding condition flow, and
caPool in pkg/controllers/proxyconfig/proxyconfig_controller.go lines 249-255,
returning it from createHTTPClients.
In `@pkg/controllers/deployment/deployment_controller.go`:
- Around line 382-392: Update the proxy trusted CA copy flow around targetCM to
propagate sourceCM.BinaryData as well as sourceCM.Data. Ensure the ConfigMap
applied to openshift-authentication preserves certificate content regardless of
whether the source stores it in Data or BinaryData.
In `@test/e2e-component-proxy/component_proxy_oidc_login.go`:
- Line 377: Update the deferred temporary-directory cleanup near the existing
os.RemoveAll call to explicitly handle its return value, either by intentionally
discarding it or logging cleanup failures, while preserving the best-effort
cleanup behavior and satisfying errcheck.
In `@test/e2e-component-proxy/component_proxy.go`:
- Around line 50-59: Rename the proxy-config cleanup result from
SaveAndRestoreProxyConfig to proxyConfigCleanup in both affected sections of
test/e2e-component-proxy/component_proxy.go: lines 50-59 and 155-177. Register
proxyConfigCleanup directly with DeferCleanup in the first section, and invoke
proxyConfigCleanup() inside the existing cleanup closure in the second,
preserving proxyCleanup exclusively for Squid teardown.
---
Outside diff comments:
In `@pkg/controllers/configobservation/oauth/idp_conversions.go`:
- Around line 318-336: Add a bounded timeout to the outbound HTTP requests in
discoverOpenIDURLs and checkOIDCPasswordGrantFlow, using the existing transport
configuration while ensuring issuer and token endpoint calls cannot block
indefinitely. Apply the timeout via an http.Client or request context/deadline
for both discovery and password-grant flows, and preserve their current response
and error handling.
In `@pkg/controllers/proxyconfig/proxyconfig_controller.go`:
- Around line 260-269: Update the HTTP clients returned by the proxy/IdP
validation client-construction function to enforce cancellation via
context.Context and a bounded timeout, matching the established timeout behavior
in custom_route_conditions.go and endpoint_accessible_controller.go. Ensure both
the proxy-enabled and direct clients used by isEndpointReachable cannot block
indefinitely when the sync context lacks a deadline.
---
Nitpick comments:
In `@test/e2e-component-proxy/component_proxy_oidc_login.go`:
- Line 254: Replace the no-op fmt.Sprintf call assigned to group with the plain
string literal "ocp-test-proxy-login-group", and remove the fmt import if it is
no longer used elsewhere in the file.
In `@test/library/proxy.go`:
- Around line 54-93: Implement all eight helper functions currently containing
panic("not implemented"): DeploySquidProxy, DeployProxyNetworkPolicies,
GetOAuthServerProxyEnvVars, GetSquidProxyLogs, WaitForSquidProxyTraffic,
VerifyOAuthServerDeploymentProxyConfig, VerifyTrustedCAConfigMapSynced, and
CheckFeatureGateEnabledOrSkip. Use the existing Kubernetes clients and each
function’s documented contract so the [ComponentProxy] specs execute their
deployment, policy, inspection, polling, verification, and feature-gate behavior
without panicking.
🪄 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: cb1ed3ca-4797-402c-8f83-5ce5903c255c
⛔ Files ignored due to path filters (55)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_crio_credential_provider_config.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_network.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.model_name.gois 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/image/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingresscontroller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.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.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.model_name.gois 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/osin/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/project/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/samples/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/template/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/user/.codegen.yamlis excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (33)
cmd/cluster-authentication-operator-tests-ext/main.gogo.modpkg/controllers/common/proxy.gopkg/controllers/common/proxy_test.gopkg/controllers/configobservation/configobservercontroller/observe_config_controller.gopkg/controllers/configobservation/interfaces.gopkg/controllers/configobservation/oauth/idp_conversions.gopkg/controllers/configobservation/oauth/idp_conversions_test.gopkg/controllers/configobservation/oauth/observe_idps.gopkg/controllers/configobservation/oauth/observe_idps_test.gopkg/controllers/configobservation/oauth/observe_proxy_trusted_ca.gopkg/controllers/configobservation/oauth/observe_proxy_trusted_ca_test.gopkg/controllers/customroute/custom_route_conditions.gopkg/controllers/customroute/custom_route_controller.gopkg/controllers/deployment/default_deployment.gopkg/controllers/deployment/deployment_controller.gopkg/controllers/deployment/deployment_controller_test.gopkg/controllers/oauthendpoints/oauth_endpoints_controller.gopkg/controllers/proxyconfig/proxyconfig_controller.gopkg/controllers/proxyconfig/proxyconfig_controller_test.gopkg/internal/transporttest/transporttest.gopkg/libs/endpointaccessible/endpoint_accessible_controller.gopkg/operator/replacement_starter.gopkg/operator/starter.gopkg/transport/transport.gopkg/transport/transport_test.gotest-data/apply-configuration/overall/minimal-cluster/expected-output/UserWorkload/Create/cluster-scoped-resources/certificates.k8s.io/certificatesigningrequests/9806-body-system-COLON-openshift-COLON-openshift-authenticator-.yamltest-data/apply-configuration/overall/minimal-cluster/expected-output/UserWorkload/Create/cluster-scoped-resources/certificates.k8s.io/certificatesigningrequests/9806-metadata-system-COLON-openshift-COLON-openshift-authenticator-.yamltest-data/apply-configuration/overall/oauth-server-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication/apps/deployments/b3b2-body-oauth-openshift.yamltest-data/apply-configuration/overall/oauth-server-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication/apps/deployments/b3b2-metadata-oauth-openshift.yamltest/e2e-component-proxy/component_proxy.gotest/e2e-component-proxy/component_proxy_oidc_login.gotest/library/proxy.go
ehearne-redhat
force-pushed
the
oauth-server-proxy-config-e2e
branch
from
July 21, 2026 11:02
6a54b55 to
de717b2
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/e2e-component-proxy/component_proxy_oidc_login.go (1)
254-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the no-op
fmt.Sprintf.The format string has no verbs/args, so
fmt.Sprintfis unnecessary (gosimple S1039).♻️ Proposed fix
- group := fmt.Sprintf("ocp-test-proxy-login-group") + group := "ocp-test-proxy-login-group"🤖 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 `@test/e2e-component-proxy/component_proxy_oidc_login.go` at line 254, Replace the no-op fmt.Sprintf call when assigning group with the underlying string literal directly. Remove the unnecessary fmt usage if it is no longer referenced elsewhere in the surrounding function or file.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.
Inline comments:
In `@test/e2e-component-proxy/component_proxy_oidc_login.go`:
- Line 242: Replace both calls to the undefined extractNamespaceFromIDPName in
the OIDC login flow with an existing namespace helper, or pass the namespace
returned or used by DeployKeycloak through the relevant functions. Ensure both
call sites use the same valid Keycloak namespace source and compile without
introducing a new undefined symbol.
- Line 129: Fix compile errors in component_proxy_oidc_login.go by binding all
four return values from each test.DeploySquidProxy call at the identified sites,
using the namespace value or discarding it with _. Resolve the
extractNamespaceFromIDPName calls by adding the missing helper or replacing them
with the existing intended namespace-extraction function, preserving the current
behavior.
In `@test/e2e-component-proxy/component_proxy.go`:
- Line 56: Update every DeploySquidProxy call in component_proxy_oidc_login.go
to capture all four returned values, including proxyCleanup, and remove or
replace the undefined extractNamespaceFromIDPName references with an existing
namespace-resolution approach so the package compiles.
In `@test/library/keycloakidp.go`:
- Around line 152-164: Update the client iteration around adminClientId and
passwdClientId to use comma-ok assertions for clientId, id, and redirectUris
before accessing them. Skip entries with missing, null, or incorrectly typed
fields, while preserving the existing admin-cli and redirect-URI selection
behavior for valid clients.
---
Nitpick comments:
In `@test/e2e-component-proxy/component_proxy_oidc_login.go`:
- Line 254: Replace the no-op fmt.Sprintf call when assigning group with the
underlying string literal directly. Remove the unnecessary fmt usage if it is no
longer referenced elsewhere in the surrounding function or file.
🪄 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: 8e1f81f5-ac15-4d11-bf0f-487ebc775a49
📒 Files selected for processing (33)
cmd/cluster-authentication-operator-tests-ext/main.gopkg/controllers/common/proxy.gopkg/controllers/common/proxy_test.gopkg/controllers/configobservation/configobservercontroller/observe_config_controller.gopkg/controllers/configobservation/interfaces.gopkg/controllers/configobservation/oauth/idp_conversions.gopkg/controllers/configobservation/oauth/idp_conversions_test.gopkg/controllers/configobservation/oauth/observe_idps.gopkg/controllers/configobservation/oauth/observe_idps_test.gopkg/controllers/configobservation/oauth/observe_proxy_trusted_ca.gopkg/controllers/configobservation/oauth/observe_proxy_trusted_ca_test.gopkg/controllers/customroute/custom_route_conditions.gopkg/controllers/customroute/custom_route_controller.gopkg/controllers/deployment/default_deployment.gopkg/controllers/deployment/deployment_controller.gopkg/controllers/deployment/deployment_controller_test.gopkg/controllers/oauthendpoints/oauth_endpoints_controller.gopkg/controllers/proxyconfig/proxyconfig_controller.gopkg/controllers/proxyconfig/proxyconfig_controller_test.gopkg/internal/transporttest/transporttest.gopkg/libs/endpointaccessible/endpoint_accessible_controller.gopkg/operator/replacement_starter.gopkg/operator/starter.gopkg/transport/transport.gopkg/transport/transport_test.gotest-data/apply-configuration/overall/minimal-cluster/expected-output/UserWorkload/Create/cluster-scoped-resources/certificates.k8s.io/certificatesigningrequests/9806-body-system-COLON-openshift-COLON-openshift-authenticator-.yamltest-data/apply-configuration/overall/minimal-cluster/expected-output/UserWorkload/Create/cluster-scoped-resources/certificates.k8s.io/certificatesigningrequests/9806-metadata-system-COLON-openshift-COLON-openshift-authenticator-.yamltest-data/apply-configuration/overall/oauth-server-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication/apps/deployments/b3b2-body-oauth-openshift.yamltest-data/apply-configuration/overall/oauth-server-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication/apps/deployments/b3b2-metadata-oauth-openshift.yamltest/e2e-component-proxy/component_proxy.gotest/e2e-component-proxy/component_proxy_oidc_login.gotest/library/keycloakidp.gotest/library/proxy.go
🚧 Files skipped from review as they are similar to previous changes (22)
- pkg/operator/replacement_starter.go
- test-data/apply-configuration/overall/oauth-server-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication/apps/deployments/b3b2-metadata-oauth-openshift.yaml
- test-data/apply-configuration/overall/oauth-server-creation-minimal/expected-output/Management/Create/namespaces/openshift-authentication/apps/deployments/b3b2-body-oauth-openshift.yaml
- cmd/cluster-authentication-operator-tests-ext/main.go
- pkg/controllers/oauthendpoints/oauth_endpoints_controller.go
- pkg/controllers/configobservation/interfaces.go
- pkg/internal/transporttest/transporttest.go
- test-data/apply-configuration/overall/minimal-cluster/expected-output/UserWorkload/Create/cluster-scoped-resources/certificates.k8s.io/certificatesigningrequests/9806-body-system-COLON-openshift-COLON-openshift-authenticator-.yaml
- pkg/libs/endpointaccessible/endpoint_accessible_controller.go
- pkg/controllers/configobservation/configobservercontroller/observe_config_controller.go
- pkg/controllers/configobservation/oauth/observe_proxy_trusted_ca.go
- pkg/controllers/common/proxy.go
- pkg/operator/starter.go
- test-data/apply-configuration/overall/minimal-cluster/expected-output/UserWorkload/Create/cluster-scoped-resources/certificates.k8s.io/certificatesigningrequests/9806-metadata-system-COLON-openshift-COLON-openshift-authenticator-.yaml
- pkg/controllers/configobservation/oauth/idp_conversions_test.go
- test/library/proxy.go
- pkg/controllers/deployment/default_deployment.go
- pkg/controllers/customroute/custom_route_conditions.go
- pkg/controllers/configobservation/oauth/idp_conversions.go
- pkg/controllers/configobservation/oauth/observe_idps_test.go
- pkg/controllers/customroute/custom_route_controller.go
- pkg/controllers/deployment/deployment_controller.go
Comment on lines
+152
to
164
| var adminClientId, passwdClientId string | ||
| for _, c := range clientList { | ||
| if clientID := c["clientId"].(string); clientID == "admin-cli" { | ||
| adminClientId = c["id"].(string) | ||
| } else if len(c["redirectUris"].([]interface{})) > 0 { | ||
| // just reuse one other client that's already there | ||
| passwdClientId = c["id"].(string) | ||
| passwdClientClientId = clientID | ||
| setup.ClientID = clientID | ||
| } | ||
|
|
||
| if len(passwdClientId) > 0 && len(adminClientId) > 0 { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the client field assertions in test/library/keycloakidp.go:152-164
c["redirectUris"].([]interface{}) and the clientId/id assertions can panic if Keycloak returns a client with missing or null fields. Skip malformed entries with comma-ok checks so setup fails cleanly instead of crashing.
🤖 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 `@test/library/keycloakidp.go` around lines 152 - 164, Update the client
iteration around adminClientId and passwdClientId to use comma-ok assertions for
clientId, id, and redirectUris before accessing them. Skip entries with missing,
null, or incorrectly typed fields, while preserving the existing admin-cli and
redirect-URI selection behavior for valid clients.
tchap
reviewed
Jul 22, 2026
ehearne-redhat
force-pushed
the
oauth-server-proxy-config-e2e
branch
6 times, most recently
from
July 28, 2026 09:49
cf65c53 to
213e014
Compare
tchap
reviewed
Jul 28, 2026
| g.By("Deploying Squid forward proxy") | ||
| httpProxyURL, _, _, proxyNamespace, proxyCleanup := test.DeploySquidProxy(t, clients.KubeClient) | ||
| g.DeferCleanup(func() { | ||
| g.GinkgoWriter.Println("cleaning up: removing Squid proxy") |
Contributor
There was a problem hiding this comment.
This is now included in proxyCleanup, so you can just do g.DeferCleanup(proxyCleanup)
| }) | ||
| }) | ||
|
|
||
| func enableDirectAccessGrants(kcClient *test.KeycloakClient, clientID string) { |
Contributor
There was a problem hiding this comment.
nit: I somehow prefer to put the helpers after tests themselves so that I can firstly see the tests.
| assertOIDCLogin(t, kubeConfig, *clients, username, password, group) | ||
|
|
||
| g.By("Verifying traffic went through the Squid proxy") | ||
| err = test.WaitForSquidProxyTraffic(t, clients.KubeClient, proxyNamespace, 5*time.Minute) |
Contributor
There was a problem hiding this comment.
I am not sure this really needs to be checked and that it's actually checking anything, because the operator does all sorts of things that go through the proxy, like health checks, IdP config reload...
Contributor
There was a problem hiding this comment.
I guess we can leave it here, it's an additional check, albeit not very precise...
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| } | ||
|
|
||
| func testDirectIdPFallback() { |
Contributor
There was a problem hiding this comment.
I am wondering whether we can't just append this to testProxyOIDCLoginFlow since there we test the flow with proxy. So we can just remove it after and do the flow again? I am not sure what kind of cleanup we would have to do, perhaps only log out the user?
| networkPolicyCleanup() | ||
| }) | ||
|
|
||
| configMapName := "e2e-proxy-trusted-ca" |
Contributor
There was a problem hiding this comment.
This should be a new g.By block, I think.
| assertOIDCLogin(t, kubeConfig, *clients, username, password, group) | ||
|
|
||
| g.By("Verifying traffic went through the Squid proxy") | ||
| err = test.WaitForSquidProxyTraffic(t, clients.KubeClient, proxyNamespace, 5*time.Minute) |
Contributor
There was a problem hiding this comment.
Again not entirely sure this really checks anything since the networkpolicy is there, but I am asking this question myself repeatedly when reviewing and writing these tests...
ehearne-redhat
force-pushed
the
oauth-server-proxy-config-e2e
branch
3 times, most recently
from
July 29, 2026 08:40
3d5589f to
46daae8
Compare
Verify that setting spec.proxy.httpsProxy to an unreachable host on the operator Authentication CR causes ProxyConfigControllerDegraded to become True, propagating to ClusterOperator Degraded=True. The test is gated behind the AuthenticationComponentProxy feature gate and registered in the OTE serial/operator suite.
Add C2 test: deploy a Squid proxy, configure spec.proxy to point at it, add a fake OpenID IdP with an unresolvable issuer URL, and verify the ProxyConfigController emits an IdPEndpointUnreachable Warning event without going Degraded. Also add stub helper functions in test/library/proxy.go for future proxy e2e infrastructure (DeploySquidProxy, DeployProxyNetworkPolicies, etc.). Use WaitForOperatorToPickUpChanges in both C1 and C2 cleanup to avoid racing with stale operator status.
…dd A1 test Move proxy config save/restore logic into a shared test helper in test/library/proxy.go. Add the A1 test (OIDC IdP validation through component proxy) and clean up C1/C2 tests to use the shared helper and exported CheckFeatureGateEnabledOrSkip. Remove redundant nil check in C1 test.
Add Group A proxy e2e tests: - A1: validate OIDC IdP through component proxy, with and without trustedCA (two g.It variants sharing testOIDCIdPThroughComponentProxy) - A2: verify operator falls back gracefully on spec.proxy removal Split AddKeycloakIDP into DeployKeycloak + AddKeycloakOIDCIdP so tests can control ordering (deploy Keycloak, apply NetworkPolicy, set proxy, then register IdP). AddKeycloakIDP remains as a convenience wrapper. Simplify DeploySquidProxy to always generate TLS internally and return the CA PEM bytes. Update CheckFeatureGateEnabledOrSkip to replace the local checkFeatureGateOrSkip in all tests.
DeploySquidProxy now listens on both HTTP and HTTPS and returns the raw host:port. Callers prepend http:// (no trustedCA) or https:// (with trustedCA) to construct the proxy URL. This lets the A1 test cover both cases with a single helper.
Accept expectedHTTPProxy, expectedHTTPSProxy, expectedNoProxy, and expectTrustedCAVolume so callers specify exactly what to assert. Empty strings mean the env var should be absent.
- Implement DeploySquidProxy, DeployProxyNetworkPolicies, GetSquidProxyLogs, WaitForSquidProxyTraffic, VerifyOAuthServerDeploymentProxyConfig, VerifyTrustedCAConfigMapSynced, and CheckFeatureGateEnabledOrSkip in the test library - Wrap DeploySquidProxy cleanup with sync.OnceFunc to prevent double-cleanup when the internal failure defer and DeferCleanup both fire - Simplify VerifyOAuthServerDeploymentProxyConfig to compare env var values directly (proxy env vars are always set, just empty when unset) - Remove redundant GetOAuthServerProxyEnvVars call in testFallbackOnProxyRemoval
…nfig Return separate httpProxyURL and httpsProxyURL from DeploySquidProxy. Fix Squid 7 TLS syntax (tls-cert=/tls-key= instead of cert=/key=), add pid_filename /tmp/squid.pid for restricted PSA, pin Squid image to 7.2-26.04_edge.
The operator may add extra entries to NO_PROXY beyond the static set (e.g. the kubernetes service IP from KUBERNETES_SERVICE_HOST). Use a superset check so callers only need to assert the entries they care about.
The fallback-on-removal test only needs to verify the operator recovers after spec.proxy is cleared. Using plain HTTP avoids the trustedCA ConfigMap setup and reduces rollout time.
Deploy Squid, set a working proxy, deploy Keycloak+IdP, and verify stability before switching to an unreachable proxy URL. This tests the transition from a healthy proxy config to a broken one.
- SaveAndRestoreProxyConfig: skip WaitForOperatorToPickUpChanges when the proxy config already matches the original (avoids waiting for Progressing=True that never comes when the test failed before setting the proxy). - C2 test: re-fetch the operator CR before setting the proxy to avoid conflict errors from intervening operator reconciliation.
Replace docker.io/ubuntu/squid with registry.redhat.io/rhel10/squid:10.2-1784702318. The RHEL image is unprivileged and defaults pid_filename to /run/squid.pid (unwritable); override it to /tmp/squid.pid. Log directly to stdout/stderr instead of files, removing the need for the log-tailing sidecar container and the squid-logs emptyDir volume. GetSquidProxyLogs now reads from the main squid container.
Add GetSquidProxyLogsSince(kubeClient, namespace, since time.Time) that sets SinceTime on the log request when since is non-zero. GetSquidProxyLogs becomes a thin wrapper passing a zero time.
Add image mapping support so e2e tests can use mirrored images when KUBE_TEST_REPO is set. Introduce test/library/image/image.go with GetMappedImages (adapted from openshift/origin) which rewrites image pull specs to point to a target mirror registry. Initialize keycloakImage and squidImage from this mapping at startup, replacing hardcoded pull spec constants.
This commit adds 4 tests to test auth proxy config functionality from oauth-server perspective. `testPartialFullProxyEnvVars()` tests environment variables are present when partial and full auth proxy configs are set. `testProxyOIDCLoginFlow()` ensures that when login flow is attempted and proxy is configured, that traffic goes through the proxy for IdP login. It also ensures that when IdP is configured, and proxy config is configured, the cluster will fall back to either cluster-wide proxy or direct IdP connectivity to perform OIDC login flow. `testTrustedCAHotReload()` configures trustedCA and ensures that oauth-server pods are not redeployed when CA file is changed and login flow still works as expected. `testNoProxy()` configures NoProxy and ensures that login flow does not go through the proxy. Helper method `GetSquidProxyLogsSince()` uses SinceTime to filter logs from squid proxy only after the time specified. This ensures string size does not overload when using logs to verify traffic in tests that require multiple login flow attempts.
ehearne-redhat
force-pushed
the
oauth-server-proxy-config-e2e
branch
from
July 29, 2026 08:45
46daae8 to
d55db88
Compare
Contributor
|
@ehearne-redhat: 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.
Continues work from and closes openshift/origin#31398 . Kept in separate file for now. Plan is to add tests to
test/e2e-component-proxy/component_proxy.gointo #949 when consensus reached on test status.Summary by CodeRabbit
New Features
Bug Fixes
Tests