[WIP] CNTRLPLANE-3851: Oauth server proxy config e2e#950
Open
ehearne-redhat wants to merge 10 commits into
Open
[WIP] CNTRLPLANE-3851: Oauth server proxy config e2e#950ehearne-redhat wants to merge 10 commits into
ehearne-redhat wants to merge 10 commits into
Conversation
Vendor openshift/api with the new AuthenticationProxyConfig and AuthenticationConfigMapReference types on the Authentication operator spec, gated behind FeatureGateAuthenticationComponentProxy.
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).
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.
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
Contributor
|
PR needs rebase. 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. |
WalkthroughAdds feature-gated component proxy resolution across authentication controllers, proxy-aware HTTP transports, trusted CA synchronization for OAuth Server, IdP connectivity validation, controller wiring, and serial end-to-end tests. ChangesComponent proxy foundation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Authentication
participant ResolveProxy
participant OAuthDeployment
participant OAuthServer
participant IdP
Authentication->>ResolveProxy: component proxy and trusted CA
ResolveProxy->>OAuthDeployment: proxy environment and CA mount
OAuthDeployment->>OAuthServer: configured deployment
OAuthServer->>IdP: proxied OIDC request
IdP-->>OAuthServer: discovery or token response
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 4 warnings)
✅ Passed checks (10 passed)
✨ 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 |
Contributor
|
@ehearne-redhat: The following tests failed, say
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. |
![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
+99
to
+105
| // mergeNoProxy combines user-provided noProxy entries with static cluster-internal | ||
| // defaults. It performs deduplication, but the items are not sorted. | ||
| func mergeNoProxy(userNoProxy []string) string { | ||
| entries := sets.New[string](staticNoProxyEntries...) | ||
| entries.Insert(userNoProxy...) | ||
| return strings.Join(entries.UnsortedList(), ",") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Non-deterministic NoProxy ordering will thrash the oauth-server redeploy hash.
UnsortedList() returns entries in random map-iteration order, so mergeNoProxy produces a different comma-joined string on virtually every call even when the underlying set hasn't changed. deployment_controller.go's Sync() feeds this directly into its redeploy-trigger string (fmt.Sprintf("proxy:%s:%s:%s", proxy.HTTPProxy, proxy.HTTPSProxy, proxy.NoProxy)), so this nondeterminism will make that hash change on nearly every reconcile, potentially forcing continual/needless rolling redeployment of the OAuth server. Use a sorted list instead.
🐛 Proposed fix
func mergeNoProxy(userNoProxy []string) string {
entries := sets.New[string](staticNoProxyEntries...)
entries.Insert(userNoProxy...)
- return strings.Join(entries.UnsortedList(), ",")
+ return strings.Join(sets.List(entries), ",")
}📝 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
| // mergeNoProxy combines user-provided noProxy entries with static cluster-internal | |
| // defaults. It performs deduplication, but the items are not sorted. | |
| func mergeNoProxy(userNoProxy []string) string { | |
| entries := sets.New[string](staticNoProxyEntries...) | |
| entries.Insert(userNoProxy...) | |
| return strings.Join(entries.UnsortedList(), ",") | |
| } | |
| // mergeNoProxy combines user-provided noProxy entries with static cluster-internal | |
| // defaults. It performs deduplication, but the items are not sorted. | |
| func mergeNoProxy(userNoProxy []string) string { | |
| entries := sets.New[string](staticNoProxyEntries...) | |
| entries.Insert(userNoProxy...) | |
| return strings.Join(sets.List(entries), ",") | |
| } |
🤖 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/common/proxy.go` around lines 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.
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.
| g.By("Generating self-signed CA and creating trustedCA ConfigMap") | ||
| tempDir, err := os.MkdirTemp("", "testca") | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| defer os.RemoveAll(tempDir) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Handle the os.RemoveAll error (errcheck).
golangci-lint's errcheck flags the unchecked return. Since it's a best-effort temp-dir cleanup, either explicitly discard or log on failure to satisfy the linter.
🧹 Proposed fix
- defer os.RemoveAll(tempDir)
+ defer func() {
+ if err := os.RemoveAll(tempDir); err != nil {
+ g.GinkgoWriter.Printf("failed to remove temp dir %s: %v\n", tempDir, err)
+ }
+ }()🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 377-377: Error return value of os.RemoveAll is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e-component-proxy/component_proxy_oidc_login.go` at 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.
Source: Linters/SAST tools
Comment on lines
+50
to
+59
| proxyURL, proxyNamespace, proxyCleanup := test.DeploySquidProxy(t, clients.KubeClient) | ||
| g.DeferCleanup(func() { | ||
| g.GinkgoWriter.Println("cleaning up: removing Squid proxy") | ||
| proxyCleanup() | ||
| }) | ||
| g.GinkgoWriter.Printf("Squid proxy URL: %s\n", proxyURL) | ||
|
|
||
| g.By("Saving original proxy config and setting component-scoped proxy") | ||
| operatorAuth, proxyCleanup := test.SaveAndRestoreProxyConfig(t, clients.OperatorClient, clients.ConfigClient) | ||
| g.DeferCleanup(proxyCleanup) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Reused proxyCleanup variable breaks Squid teardown in two tests. In both functions the Squid teardown is captured in a DeferCleanup closure by variable reference, then proxyCleanup is reassigned by SaveAndRestoreProxyConfig (via := where only operatorAuth is new). At cleanup time every closure invokes the proxy-config restore, leaking the Squid proxy namespace and restoring config twice. Fix by giving the config cleanup its own variable (as in component_proxy_oidc_login.go).
test/e2e-component-proxy/component_proxy.go#L50-L59: rename tooperatorAuth, proxyConfigCleanup := test.SaveAndRestoreProxyConfig(...)andg.DeferCleanup(proxyConfigCleanup).test/e2e-component-proxy/component_proxy.go#L155-L177: rename the L163 result toproxyConfigCleanupand callproxyConfigCleanup()in the L170-178 cleanup closure instead of reassigningproxyCleanup.
📍 Affects 1 file
test/e2e-component-proxy/component_proxy.go#L50-L59(this comment)test/e2e-component-proxy/component_proxy.go#L155-L177
🤖 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.go` around lines 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.
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
noProxysettings.Bug Fixes