[WIP] CNTRLPLANE-3851: Oauth server proxy config e2e#950
Open
ehearne-redhat wants to merge 23 commits into
Open
[WIP] CNTRLPLANE-3851: Oauth server proxy config e2e#950ehearne-redhat wants to merge 23 commits into
ehearne-redhat wants to merge 23 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
Comment on lines
+165
to
+171
| if len(proxy.TrustedCAName) > 0 { | ||
| caData, err := transport.LoadCAData(configMapLister, proxy.TrustedCAName, "ca-bundle.crt") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load proxy CA: %w", err) | ||
| } | ||
| rootCAs.AppendCertsFromPEM(caData) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Ignored AppendCertsFromPEM result when loading proxy trusted CA in two files. Both sites load the proxy's trusted-CA PEM bytes via transport.LoadCAData and then call AppendCertsFromPEM without checking its ok bool return, unlike the existing sibling checks in the same files (e.g., custom_route_conditions.go line 161, proxyconfig_controller.go line 287). Malformed or truncated CA data is silently ignored instead of surfacing a clear error.
pkg/controllers/customroute/custom_route_conditions.go#L165-L171: check theokreturn ofrootCAs.AppendCertsFromPEM(caData)and return an error (e.g.fmt.Errorf("failed to parse proxy CA %q", proxy.TrustedCAName)) whenfalse.pkg/controllers/proxyconfig/proxyconfig_controller.go#L249-L255: check theokreturn ofcaPool.AppendCertsFromPEM(caData)and return an error fromcreateHTTPClientswhenfalse.
📍 Affects 2 files
pkg/controllers/customroute/custom_route_conditions.go#L165-L171(this comment)pkg/controllers/proxyconfig/proxyconfig_controller.go#L249-L255
🤖 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/customroute/custom_route_conditions.go` around lines 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.
Source: Path instructions
Comment on lines
+382
to
+392
| sourceCM, err := c.sourceConfigMapLister.ConfigMaps("openshift-config").Get(trustedCAName) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get component proxy CA configmap \"%s/%s\": %w", "openshift-config", trustedCAName, err) | ||
| } | ||
|
|
||
| targetCM := corev1ac.ConfigMap(componentProxyCAConfigMapName, "openshift-authentication").WithData(sourceCM.Data) | ||
| if _, err := c.configMaps.ConfigMaps("openshift-authentication").Apply( | ||
| ctx, targetCM, metav1.ApplyOptions{FieldManager: c.name, Force: true}, | ||
| ); err != nil { | ||
| return fmt.Errorf("failed to apply component proxy CA configmap \"%s/%s\": %w", "openshift-authentication", componentProxyCAConfigMapName, err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Proxy trusted CA copy drops BinaryData, potentially loses cert content.
targetCM := corev1ac.ConfigMap(...).WithData(sourceCM.Data) only copies sourceCM.Data. transport.LoadCAData (used elsewhere to read this exact kind of CA configmap) explicitly falls back to cm.BinaryData[key] when Data[key] is empty, indicating this configmap's PEM content can legitimately live in BinaryData. If the source openshift-config configmap referenced by TrustedCA.Name stores the bundle there, the copied target configmap mounted into the OAuth server pod will be empty, silently breaking proxy TLS trust for the running server.
🐛 Proposed fix to also propagate BinaryData
- targetCM := corev1ac.ConfigMap(componentProxyCAConfigMapName, "openshift-authentication").WithData(sourceCM.Data)
+ targetCM := corev1ac.ConfigMap(componentProxyCAConfigMapName, "openshift-authentication").
+ WithData(sourceCM.Data).
+ WithBinaryData(sourceCM.BinaryData)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sourceCM, err := c.sourceConfigMapLister.ConfigMaps("openshift-config").Get(trustedCAName) | |
| if err != nil { | |
| return fmt.Errorf("failed to get component proxy CA configmap \"%s/%s\": %w", "openshift-config", trustedCAName, err) | |
| } | |
| targetCM := corev1ac.ConfigMap(componentProxyCAConfigMapName, "openshift-authentication").WithData(sourceCM.Data) | |
| if _, err := c.configMaps.ConfigMaps("openshift-authentication").Apply( | |
| ctx, targetCM, metav1.ApplyOptions{FieldManager: c.name, Force: true}, | |
| ); err != nil { | |
| return fmt.Errorf("failed to apply component proxy CA configmap \"%s/%s\": %w", "openshift-authentication", componentProxyCAConfigMapName, err) | |
| } | |
| sourceCM, err := c.sourceConfigMapLister.ConfigMaps("openshift-config").Get(trustedCAName) | |
| if err != nil { | |
| return fmt.Errorf("failed to get component proxy CA configmap \"%s/%s\": %w", "openshift-config", trustedCAName, err) | |
| } | |
| targetCM := corev1ac.ConfigMap(componentProxyCAConfigMapName, "openshift-authentication"). | |
| WithData(sourceCM.Data). | |
| WithBinaryData(sourceCM.BinaryData) | |
| if _, err := c.configMaps.ConfigMaps("openshift-authentication").Apply( | |
| ctx, targetCM, metav1.ApplyOptions{FieldManager: c.name, Force: true}, | |
| ); err != nil { | |
| return fmt.Errorf("failed to apply component proxy CA configmap \"%s/%s\": %w", "openshift-authentication", componentProxyCAConfigMapName, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controllers/deployment/deployment_controller.go` around lines 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.
Add TransportForCARefWithProxy for building proxy-aware transports with optional extra CA data.
Add shared utilities for component-scoped proxy configuration: - ResolveProxy: reads spec.proxy from the Authentication operator CR when the AuthenticationComponentProxy feature gate is enabled and returns a ResolvedProxy with the effective proxy strings, falling back to the process environment when no component proxy is configured. - ResolvedProxy.ProxyFunc: returns a func(*http.Request) (*url.URL, error) suitable for http.Transport.Proxy.
Wire the component-scoped proxy into every affected controller: Config observer: builds a proxy-aware HTTP transport for OIDC discovery and password grant checks. The Listers struct gains OperatorAuthLister and FeatureGateAccessor. The convertIdentityProviders signature changes from a ConfigMap lister to a transportForCABuilderFunc, decoupling transport construction from the IDP conversion logic. Adds an ObserveProxyTrustedCA observer that injects the proxy CA bundle path into the observed config when a component proxy with a trustedCA is active. Deployment controller: resolves the component proxy, injects HTTP_PROXY/HTTPS_PROXY/NO_PROXY env vars into the OAuth Server pod, syncs the trustedCA ConfigMap from openshift-config to openshift-authentication, and adds a volume/mount with Optional=true for CA hot-reload without redeployment. Proxy validation controller: validates route reachability through the component proxy. Tests IdP endpoint connectivity and emits IdPEndpointUnreachable Warning events for transient failures (does not set Degraded). Uses hash-based change detection to avoid redundant validation. Endpoint accessible controller: gains an optional proxy function for route health checks. Only the route check controller gets the proxy because it connects to the external route hostname; service and endpoint checks connect to cluster-internal addresses and remain proxy-free. Custom route controller: resolves the component proxy via ResolveProxy for the route availability health check. Loads the proxy trusted CA via LoadCAData and appends it to the root CA pool. Fixes the routeAvailablity typo (now routeAvailability). Operator startup: passes operatorAuthLister, featureGateAccessor, and operatorAuthInformer to all affected controllers.
Extract shared test helpers into pkg/internal/transporttest: UnwrapTransport, MakeSelfSignedCA, MustParseURL. Replace local copies in transport, proxy, and observe_idps test files. Add TestBuildIDPTransport to verify that with the feature gate enabled and a component proxy with trusted CA configured, the IDP transport builder wires the proxy function and loads the CA. Add Test_validateIdPConnectivity_hashDedup to verify hash-based dedup: successful validation saves the hash and skips redundant checks; failed validation does not save the hash, allowing retry. Enable the AuthenticationComponentProxy feature gate in TestObserveIdentityProviders to exercise the gate-enabled path with no proxy configured (no-op fallback to environment).
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
Use sets.List() instead of sets.UnsortedList() to produce sorted, deterministic noProxy values and prevent oauth-server deployment churn. Remove the now-unnecessary sortedCSV test helper and the standalone TestResolveProxy_FeatureGateError that duplicated a table-driven case.
ehearne-redhat
force-pushed
the
oauth-server-proxy-config-e2e
branch
2 times, most recently
from
July 23, 2026 10:55
fe4cd31 to
31c90b2
Compare
The OAuthServerRouteEndpointAccessibleController checks the OAuth route health through the proxy. When the proxy uses TLS (https://), the controller needs the proxy's trustedCA in its root CA pool. Use ResolveProxy to get the TrustedCAName, then load the CA from the source ConfigMap in openshift-config via transport.LoadCAData — consistent with how ProxyConfigController loads it.
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 change includes tests moving to `WaitForOperatorToPickUpChanges()` when authentication CR is updated, VerifyOAuthServerDeploymentProxyConfig() being used in now merged env var tests, ensuring no connect entries for squid proxy in relevant tests, and CA file reconfigure for squid proxy to verify hot reload behavior.
ehearne-redhat
force-pushed
the
oauth-server-proxy-config-e2e
branch
from
July 23, 2026 15:46
31c90b2 to
73b2ff3
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