WIP: E2E tests for the Component Proxy#949
Open
tchap wants to merge 9 commits into
Open
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.
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThe operator resolves feature-gated component proxy settings and trusted CA references, applies them to transports, controller checks, OAuth deployments, and configuration observation, and adds serial component-proxy end-to-end coverage. ChangesComponent proxy integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AuthenticationCR
participant ProxyConfigController
participant OAuthDeploymentController
participant OAuthServer
AuthenticationCR->>ProxyConfigController: update component proxy
ProxyConfigController->>ProxyConfigController: resolve proxy and validate IdP connectivity
AuthenticationCR->>OAuthDeploymentController: provide proxy and TrustedCAName
OAuthDeploymentController->>OAuthServer: update proxy environment and mounted CA
OAuthServer-->>ProxyConfigController: report route and IdP reachability
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 4 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controllers/configobservation/oauth/idp_conversions.go (1)
318-332: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound both OIDC probes with request deadlines.
A proxy that accepts the connection but never responds can indefinitely pin the configuration-observer worker.
pkg/controllers/configobservation/oauth/idp_conversions.go#L318-L332: create the discovery request with a timeout context before callingRoundTrip.pkg/controllers/configobservation/oauth/idp_conversions.go#L376-L424: apply a timeout context to the token request, optionally backed by anhttp.Client.Timeout.As per path instructions, use “context.Context for cancellation and timeouts.”
🤖 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 - 332, Bound both OIDC HTTP probes using context.Context cancellation and timeouts: in discoverOpenIDURLs, create the discovery request with a timeout context before rt.RoundTrip; in the token-request flow covering lines 376-424, apply an equivalent timeout context and optionally configure http.Client.Timeout. Update both affected sites in pkg/controllers/configobservation/oauth/idp_conversions.go (318-332 and 376-424), preserving existing request behavior while ensuring unresponsive proxies cannot block indefinitely.Source: Path instructions
pkg/controllers/proxyconfig/proxyconfig_controller.go (1)
59-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWatch every lister-backed input used by validation.
OAuth changes and updates to the component proxy’s dynamic
openshift-config/<trustedCAName>ConfigMap do not enqueue this controller, leaving validation stale until the 60-minute resync. Accept the OAuth informer and theopenshift-configConfigMap informer, then register both withWithInformers; update thestarter.gowiring accordingly.🤖 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 59 - 90, The proxyConfigChecker controller currently watches only routes, auth configuration, operator authentication, and selected CA ConfigMaps, so validation inputs from OAuth and the dynamic openshift-config trusted-CA ConfigMap can remain stale. Update the controller factory and its starter.go wiring to accept the OAuth informer and the openshift-config ConfigMap informer, register both with WithInformers, and preserve the existing filtered ConfigMap watches.
🤖 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 `@cmd/cluster-authentication-operator-tests-ext/main.go`:
- Around line 89-97: The existing operator serial suite selector also matches
component-proxy tests, causing duplicate execution. Update its qualifier in the
existing serial suite registration to exclude names containing
“[ComponentProxy]”, while leaving the dedicated component-proxy suite selector
unchanged.
In `@pkg/controllers/common/proxy.go`:
- Around line 101-104: Update mergeNoProxy to sort the combined entries
deterministically before joining them, replacing the unordered UnsortedList flow
with an ordered list while preserving all existing entries and comma-separated
output.
In `@pkg/controllers/configobservation/oauth/observe_proxy_trusted_ca_test.go`:
- Around line 188-190: Update the test assertion around the observed
configuration to compare tt.expected directly with observed, removing the
NestedString extraction and ignored error returns. Preserve the test’s existing
assertion framework while validating the complete configuration, including
fallback fields.
In `@pkg/controllers/customroute/custom_route_conditions.go`:
- Around line 165-171: Update the trusted CA handling around
transport.LoadCAData in the custom route conditions flow to check the boolean
result from rootCAs.AppendCertsFromPEM(caData). When no certificate is parsed,
return a configuration error for the invalid trusted proxy CA bundle instead of
continuing; preserve the existing load error propagation and successful append
behavior.
In `@pkg/controllers/deployment/deployment_controller.go`:
- Around line 382-392: Update the ConfigMap construction in the trusted CA copy
flow to preserve both sourceCM.Data and sourceCM.BinaryData when creating
targetCM. Keep the existing namespace, name, and apply behavior in the
surrounding deployment controller logic unchanged.
In `@pkg/controllers/proxyconfig/proxyconfig_controller.go`:
- Around line 143-150: Update the no-IdP branch in the validation flow around
extractIdPURLs to clear p.lastIdPValidationHash before returning. Keep the
existing early return, and preserve hash comparison behavior when at least one
IdP remains.
In `@pkg/transport/transport_test.go`:
- Around line 303-308: Do not ignore test fixture insertion errors: in
pkg/transport/transport_test.go:303-308, update newConfigMapLister to accept
*testing.T and assert each indexer.Add result; in
pkg/controllers/common/proxy_test.go:352-357, replace the discarded
Authentication fixture insertion error with an assertion using the test handle.
Preserve the existing fixture setup and lister behavior.
In `@pkg/transport/transport.go`:
- Around line 51-58: Update the CA aggregation loop around LoadCAData so each
ConfigMap value is separated from the next PEM bundle by an explicit newline,
including when the loaded data lacks a trailing newline. Preserve the existing
error propagation and append all certificate data into caData.
- Around line 60-62: Update the no-component-proxy branch in TransportFor to
return a transport with proxy resolution disabled, rather than delegating to the
default environment-aware transport. Preserve the existing behavior for
configurations that provide CA data or HTTP/HTTPS proxy settings.
In `@test/e2e-component-proxy/component_proxy.go`:
- Around line 63-65: Update the error branch in the authentication GET flow
within the component proxy test to propagate the returned API error instead of
returning a nil error. Preserve the existing false result while passing err
through to the caller so cleanup failures are reported immediately.
---
Outside diff comments:
In `@pkg/controllers/configobservation/oauth/idp_conversions.go`:
- Around line 318-332: Bound both OIDC HTTP probes using context.Context
cancellation and timeouts: in discoverOpenIDURLs, create the discovery request
with a timeout context before rt.RoundTrip; in the token-request flow covering
lines 376-424, apply an equivalent timeout context and optionally configure
http.Client.Timeout. Update both affected sites in
pkg/controllers/configobservation/oauth/idp_conversions.go (318-332 and
376-424), preserving existing request behavior while ensuring unresponsive
proxies cannot block indefinitely.
In `@pkg/controllers/proxyconfig/proxyconfig_controller.go`:
- Around line 59-90: The proxyConfigChecker controller currently watches only
routes, auth configuration, operator authentication, and selected CA ConfigMaps,
so validation inputs from OAuth and the dynamic openshift-config trusted-CA
ConfigMap can remain stale. Update the controller factory and its starter.go
wiring to accept the OAuth informer and the openshift-config ConfigMap informer,
register both with WithInformers, and preserve the existing filtered ConfigMap
watches.
🪄 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: 35b76f36-3286-4f09-8779-6c1a053e7f71
⛔ 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 (31)
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.go
Comment on lines
+89
to
+97
| // The following suite runs component-proxy tests that require the | ||
| // AuthenticationComponentProxy feature gate (TechPreviewNoUpgrade). | ||
| extension.AddSuite(oteextension.Suite{ | ||
| Name: "openshift/cluster-authentication-operator/component-proxy/serial", | ||
| Parallelism: 1, | ||
| Qualifiers: []string{ | ||
| `name.contains("[ComponentProxy]")`, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Exclude component-proxy tests from the existing operator serial suite.
These tests carry [Serial][Operator][ComponentProxy], so they match both the existing operator serial selector and this new selector. This can execute the cluster-wide proxy test twice. Add !name.contains("[ComponentProxy]") to the existing serial-suite qualifier.
🤖 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 `@cmd/cluster-authentication-operator-tests-ext/main.go` around lines 89 - 97,
The existing operator serial suite selector also matches component-proxy tests,
causing duplicate execution. Update its qualifier in the existing serial suite
registration to exclude names containing “[ComponentProxy]”, while leaving the
dedicated component-proxy suite selector unchanged.
Comment on lines
+101
to
+104
| 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 | 🟠 Major | ⚡ Quick win
Sort noProxy entries before joining them.
UnsortedList() can produce a different string on each reconciliation. Because this value is included in the deployment rollout hash at pkg/controllers/deployment/deployment_controller.go Line 239, unchanged configuration can trigger repeated OAuth Server rollouts.
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), ",")
}🤖 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 101 - 104, Update mergeNoProxy
to sort the combined entries deterministically before joining them, replacing
the unordered UnsortedList flow with an ordered list while preserving all
existing entries and comma-separated output.
Comment on lines
+188
to
+190
| observedValue, _, _ := unstructured.NestedString(observed, "oauthConfig", "proxyTrustedCA") | ||
| expectedValue, _, _ := unstructured.NestedString(tt.expected, "oauthConfig", "proxyTrustedCA") | ||
| require.Equal(t, expectedValue, observedValue) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Compare the complete observed configuration.
Ignoring both NestedString errors lets malformed output or a lost fallback configuration pass as an empty value. Assert tt.expected against observed directly.
As per path instructions, “Never ignore error returns.”
Proposed fix
- observedValue, _, _ := unstructured.NestedString(observed, "oauthConfig", "proxyTrustedCA")
- expectedValue, _, _ := unstructured.NestedString(tt.expected, "oauthConfig", "proxyTrustedCA")
- require.Equal(t, expectedValue, observedValue)
+ require.Equal(t, tt.expected, observed)📝 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
| observedValue, _, _ := unstructured.NestedString(observed, "oauthConfig", "proxyTrustedCA") | |
| expectedValue, _, _ := unstructured.NestedString(tt.expected, "oauthConfig", "proxyTrustedCA") | |
| require.Equal(t, expectedValue, observedValue) | |
| require.Equal(t, tt.expected, observed) |
🤖 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/observe_proxy_trusted_ca_test.go`
around lines 188 - 190, Update the test assertion around the observed
configuration to compare tt.expected directly with observed, removing the
NestedString extraction and ignored error returns. Preserve the test’s existing
assertion framework while validating the complete configuration, including
fallback fields.
Source: Path instructions
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.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject an invalid trusted proxy CA bundle.
AppendCertsFromPEM reports whether any certificate was parsed, but its result is ignored. A malformed configured CA therefore becomes a later, misleading TLS connectivity failure.
Proposed fix
- rootCAs.AppendCertsFromPEM(caData)
+ if ok := rootCAs.AppendCertsFromPEM(caData); !ok {
+ return fmt.Errorf("proxy CA configmap %q contains no valid certificates", proxy.TrustedCAName)
+ }📝 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
| 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) | |
| } | |
| 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) | |
| } | |
| if ok := rootCAs.AppendCertsFromPEM(caData); !ok { | |
| return fmt.Errorf("proxy CA configmap %q contains no valid certificates", proxy.TrustedCAName) | |
| } | |
| } |
🤖 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, Update the trusted CA handling around transport.LoadCAData in the custom
route conditions flow to check the boolean result from
rootCAs.AppendCertsFromPEM(caData). When no certificate is parsed, return a
configuration error for the invalid trusted proxy CA bundle instead of
continuing; preserve the existing load error propagation and successful append
behavior.
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
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 '\bBinaryData\b|\bWithBinaryData\b|\bAuthenticationProxyConfig\b' \
--type=go pkg vendor 2>/dev/nullRepository: openshift/cluster-authentication-operator
Length of output: 38606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the controller context around the copied ConfigMap
sed -n '340,420p' pkg/controllers/deployment/deployment_controller.go
# Find where the trusted CA ConfigMap is produced/read from
rg -n -C3 'trustedCA|LoadCAData|sourceConfigMapLister|componentProxyCAConfigMapName|ca-bundle\.crt|BinaryData' \
pkg/controllers pkg/transport vendor/github.com/openshift/api/operator/v1 vendor/github.com/openshift/library-go/pkg/operator/resource/resourceapply \
--type=goRepository: openshift/cluster-authentication-operator
Length of output: 50397
Preserve BinaryData when copying the trusted CA ConfigMap.
Copying only sourceCM.Data drops a valid CA bundle stored in BinaryData, leaving the mounted proxy CA ConfigMap empty and breaking HTTPS proxy connections.
Proposed fix
- 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 ConfigMap construction in the trusted CA copy flow to preserve both
sourceCM.Data and sourceCM.BinaryData when creating targetCM. Keep the existing
namespace, name, and apply behavior in the surrounding deployment controller
logic unchanged.
Comment on lines
+143
to
+150
| idpURLs := extractIdPURLs(oauthConfig) | ||
| if len(idpURLs) == 0 { | ||
| return | ||
| } | ||
|
|
||
| hash := computeIdPValidationHash(httpProxy, httpsProxy, noProxy, idpURLs) | ||
| if hash == p.lastIdPValidationHash { | ||
| return |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset the validation hash when no IdPs remain.
After successful validation, removing every IdP leaves the old hash cached. Restoring the same IdP configuration then matches that hash and skips validation.
Proposed fix
idpURLs := extractIdPURLs(oauthConfig)
if len(idpURLs) == 0 {
+ p.lastIdPValidationHash = ""
return
}📝 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
| idpURLs := extractIdPURLs(oauthConfig) | |
| if len(idpURLs) == 0 { | |
| return | |
| } | |
| hash := computeIdPValidationHash(httpProxy, httpsProxy, noProxy, idpURLs) | |
| if hash == p.lastIdPValidationHash { | |
| return | |
| idpURLs := extractIdPURLs(oauthConfig) | |
| if len(idpURLs) == 0 { | |
| p.lastIdPValidationHash = "" | |
| return | |
| } | |
| hash := computeIdPValidationHash(httpProxy, httpsProxy, noProxy, idpURLs) | |
| if hash == p.lastIdPValidationHash { | |
| return |
🤖 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 143 -
150, Update the no-IdP branch in the validation flow around extractIdPURLs to
clear p.lastIdPValidationHash before returning. Keep the existing early return,
and preserve hash comparison behavior when at least one IdP remains.
Comment on lines
+303
to
+308
| func newConfigMapLister(cms ...*corev1.ConfigMap) corelistersv1.ConfigMapLister { | ||
| indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) | ||
| for _, cm := range cms { | ||
| _ = indexer.Add(cm) | ||
| } | ||
| return corelistersv1.NewConfigMapLister(indexer) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not discard test indexer insertion errors. Both lister helpers can silently omit fixtures and report misleading downstream failures.
pkg/transport/transport_test.go#L303-L308: accept*testing.Tand assert everyindexer.Addsucceeds.pkg/controllers/common/proxy_test.go#L352-L357: check the Authentication fixture insertion error instead of assigning it to_.
As per path instructions, “Never ignore error returns.”
📍 Affects 2 files
pkg/transport/transport_test.go#L303-L308(this comment)pkg/controllers/common/proxy_test.go#L352-L357
🤖 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/transport/transport_test.go` around lines 303 - 308, Do not ignore test
fixture insertion errors: in pkg/transport/transport_test.go:303-308, update
newConfigMapLister to accept *testing.T and assert each indexer.Add result; in
pkg/controllers/common/proxy_test.go:352-357, replace the discarded
Authentication fixture insertion error with an assertion using the test handle.
Preserve the existing fixture setup and lister behavior.
Source: Path instructions
Comment on lines
+51
to
+58
| var caData []byte | ||
| for _, ref := range caRefs { | ||
| data, err := LoadCAData(cmLister, ref.ConfigMapName, ref.ConfigMapKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| caData = append(caData, data...) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Separate aggregated PEM bundles explicitly.
If one ConfigMap value lacks a trailing newline, the next PEM block is appended directly after its END CERTIFICATE marker. AppendCertsFromPEM may then load the first bundle while silently omitting subsequent trusted CAs.
Proposed fix
for _, ref := range caRefs {
data, err := LoadCAData(cmLister, ref.ConfigMapName, ref.ConfigMapKey)
if err != nil {
return nil, err
}
caData = append(caData, data...)
+ if len(caData) > 0 && caData[len(caData)-1] != '\n' {
+ caData = append(caData, '\n')
+ }
}📝 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
| var caData []byte | |
| for _, ref := range caRefs { | |
| data, err := LoadCAData(cmLister, ref.ConfigMapName, ref.ConfigMapKey) | |
| if err != nil { | |
| return nil, err | |
| } | |
| caData = append(caData, data...) | |
| } | |
| var caData []byte | |
| for _, ref := range caRefs { | |
| data, err := LoadCAData(cmLister, ref.ConfigMapName, ref.ConfigMapKey) | |
| if err != nil { | |
| return nil, err | |
| } | |
| caData = append(caData, data...) | |
| if len(caData) > 0 && caData[len(caData)-1] != '\n' { | |
| caData = append(caData, '\n') | |
| } | |
| } |
🤖 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/transport/transport.go` around lines 51 - 58, Update the CA aggregation
loop around LoadCAData so each ConfigMap value is separated from the next PEM
bundle by an explicit newline, including when the loaded data lacks a trailing
newline. Preserve the existing error propagation and append all certificate data
into caData.
Comment on lines
+60
to
62
| if len(caData) == 0 && len(httpProxy) == 0 && len(httpsProxy) == 0 { | ||
| return TransportFor("", nil, nil, nil) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Disable environment proxy fallback when no component proxy is configured.
TransportFor returns http.DefaultTransport, whose proxy is derived from the process environment. This contradicts Lines 44-45 and can route component checks through an unrelated cluster-wide environment proxy.
Proposed fix
- if len(caData) == 0 && len(httpProxy) == 0 && len(httpsProxy) == 0 {
- return TransportFor("", nil, nil, nil)
- }
-
transport, err := newTransport("", caData, nil, nil)📝 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
| if len(caData) == 0 && len(httpProxy) == 0 && len(httpsProxy) == 0 { | |
| return TransportFor("", nil, nil, 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/transport/transport.go` around lines 60 - 62, Update the
no-component-proxy branch in TransportFor to return a transport with proxy
resolution disabled, rather than delegating to the default environment-aware
transport. Preserve the existing behavior for configurations that provide CA
data or HTTP/HTTPS proxy settings.
Comment on lines
+63
to
+65
| config, err := clients.OperatorClient.OperatorV1().Authentications().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| return false, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Propagate the cleanup GET error.
Returning nil hides the actual API failure and makes this serial test wait ten minutes before reporting only a timeout.
As per path instructions, “Never ignore error returns.”
Proposed fix
config, err := clients.OperatorClient.OperatorV1().Authentications().Get(ctx, "cluster", metav1.GetOptions{})
if err != nil {
- return false, nil
+ return false, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| config, err := clients.OperatorClient.OperatorV1().Authentications().Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return false, nil | |
| config, err := clients.OperatorClient.OperatorV1().Authentications().Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return false, err |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 65-65: error is not nil (line 63) but it returns nil
(nilerr)
🤖 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 63 - 65, Update the
error branch in the authentication GET flow within the component proxy test to
propagate the returned API error instead of returning a nil error. Preserve the
existing false result while passing err through to the caller so cleanup
failures are reported immediately.
Sources: Path instructions, Linters/SAST tools
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.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.go`:
- Line 119: Remove the g.GinkgoWriter.Printf call that logs proxyURL, or replace
its value with a non-sensitive redacted indicator so the in-cluster proxy
hostname is never written to test artifacts.
- Around line 135-136: Handle and propagate or explicitly fail on the error
returned by the Secrets(...).Delete call in the cleanup flow, rather than
discarding it with “_”. Ensure cleanup reports the deletion failure so leftover
cluster state cannot contaminate subsequent tests.
In `@test/library/proxy.go`:
- Around line 14-16: The proxy helper functions in test/library/proxy.go are
still placeholders and cause the e2e suite to panic. Implement DeploySquidProxy
and every other exported helper in the file according to their declared
contracts, including proxy setup, returned namespace/URL values, and cleanup
behavior, so the tests can execute without panics.
🪄 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: 33c9dd03-651b-4433-a7b0-db01e6b8c2bc
📒 Files selected for processing (2)
test/e2e-component-proxy/component_proxy.gotest/library/proxy.go
| g.GinkgoWriter.Println("cleaning up: removing Squid proxy") | ||
| proxyCleanup() | ||
| }) | ||
| g.GinkgoWriter.Printf("Squid proxy URL: %s\n", proxyURL) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Do not log the in-cluster proxy URL.
proxyURL contains an internal service hostname. Remove or redact it before writing test artifacts.
As per coding guidelines, “Flag logging that may expose ... internal hostnames.”
🤖 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` at line 119, Remove the
g.GinkgoWriter.Printf call that logs proxyURL, or replace its value with a
non-sensitive redacted indicator so the in-cluster proxy hostname is never
written to test artifacts.
Source: Coding guidelines
Comment on lines
+135
to
+136
| g.GinkgoWriter.Println("cleaning up: deleting fake IdP secret") | ||
| _ = clients.KubeClient.CoreV1().Secrets("openshift-config").Delete(ctx, fakeIDPSecretName, metav1.DeleteOptions{}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle the secret deletion error.
A failed cleanup leaves cluster state behind and can contaminate later serial tests.
Proposed fix
- _ = clients.KubeClient.CoreV1().Secrets("openshift-config").Delete(ctx, fakeIDPSecretName, metav1.DeleteOptions{})
+ err := clients.KubeClient.CoreV1().Secrets("openshift-config").Delete(ctx, fakeIDPSecretName, metav1.DeleteOptions{})
+ o.Expect(err).NotTo(o.HaveOccurred())As per path instructions, “Never ignore error returns.”
📝 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
| g.GinkgoWriter.Println("cleaning up: deleting fake IdP secret") | |
| _ = clients.KubeClient.CoreV1().Secrets("openshift-config").Delete(ctx, fakeIDPSecretName, metav1.DeleteOptions{}) | |
| g.GinkgoWriter.Println("cleaning up: deleting fake IdP secret") | |
| err := clients.KubeClient.CoreV1().Secrets("openshift-config").Delete(ctx, fakeIDPSecretName, metav1.DeleteOptions{}) | |
| o.Expect(err).NotTo(o.HaveOccurred()) |
🤖 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 135 - 136, Handle
and propagate or explicitly fail on the error returned by the
Secrets(...).Delete call in the cleanup flow, rather than discarding it with
“_”. Ensure cleanup reports the deletion failure so leftover cluster state
cannot contaminate subsequent tests.
Source: Path instructions
Comment on lines
+14
to
+16
| func DeploySquidProxy(t testing.TB, kubeClient kubernetes.Interface) (proxyURL string, namespace string, cleanup func()) { | ||
| panic("not implemented") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Implement the proxy helpers before registering these tests.
DeploySquidProxy is called by the new e2e test, so the suite currently terminates immediately with panic: not implemented. All other exported helpers have the same incomplete contract.
Would you like me to generate the helper implementations?
Also applies to: 20-22, 26-28, 31-33, 36-38, 41-43, 46-48, 51-53
🤖 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 14 - 16, The proxy helper functions in
test/library/proxy.go are still placeholders and cause the e2e suite to panic.
Implement DeploySquidProxy and every other exported helper in the file according
to their declared contracts, including proxy setup, returned namespace/URL
values, and cleanup behavior, so the tests can execute without panics.
…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.
openshift-ci
Bot
added
the
needs-rebase
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. |
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
|
@tchap: 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. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e-component-proxy/component_proxy.go (1)
229-231: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReturn event-list failures from the polling callback.
Line 231 converts an API failure into another retry, hiding the cause for up to ten minutes. Return
errso the test fails immediately.Proposed fix
if err != nil { g.GinkgoWriter.Printf("failed to list events: %v\n", err) - return false, nil + return false, err }As per path instructions, “Never ignore error returns.”
🤖 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 229 - 231, Update the event-list error branch in the polling callback to return the encountered err instead of returning false, nil. Preserve the existing failure log and successful polling behavior so API failures propagate immediately and stop retries.Source: Path instructions
♻️ Duplicate comments (1)
test/library/proxy.go (1)
52-93: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftImplement the proxy helpers before these specs run.
The helpers still panic.
CheckFeatureGateEnabledOrSkipruns before setup in every new spec, so all registered Component Proxy tests abort immediately; the Squid, NetworkPolicy, deployment-verification, and traffic helpers block the remaining assertions too.🤖 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 52 - 93, Implement all proxy helper functions in test/library/proxy.go instead of leaving them as panics, including CheckFeatureGateEnabledOrSkip before test setup and the Squid deployment, NetworkPolicy, OAuth environment, logs, traffic-wait, deployment-verification, and trusted-CA sync helpers. Use the provided Kubernetes clients to perform the documented operations, assertions, cleanup, and feature-gate skip behavior so all Component Proxy specs can execute.
🤖 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.go`:
- Line 55: Remove or redact the internal endpoint values logged by the component
proxy test, including proxyURL in the GinkgoWriter output and
kcClient.IssuerURL() at the additionally affected log site. Preserve useful
diagnostic context without exposing in-cluster hostnames.
In `@test/library/proxy.go`:
- Around line 20-21: Update SaveAndRestoreProxyConfig to replace context.TODO()
with separate deadline-bound contexts for the initial configuration snapshot and
the cleanup restore path. Apply each timeout context to its corresponding
GET/UPDATE calls, ensure cancellation is handled, and keep cleanup bounded even
if the API stalls.
- Around line 31-47: Update the cleanup logic around the Authentication proxy
restoration to call t.Errorf instead of t.Logf when fetching the resource,
updating its proxy, or waiting via WaitForOperatorToPickUpChanges fails.
Preserve the existing early returns, but ensure each failure marks the spec as
failed so proxy restoration or reconciliation errors cannot leave later tests
poisoned.
---
Outside diff comments:
In `@test/e2e-component-proxy/component_proxy.go`:
- Around line 229-231: Update the event-list error branch in the polling
callback to return the encountered err instead of returning false, nil. Preserve
the existing failure log and successful polling behavior so API failures
propagate immediately and stop retries.
---
Duplicate comments:
In `@test/library/proxy.go`:
- Around line 52-93: Implement all proxy helper functions in
test/library/proxy.go instead of leaving them as panics, including
CheckFeatureGateEnabledOrSkip before test setup and the Squid deployment,
NetworkPolicy, OAuth environment, logs, traffic-wait, deployment-verification,
and trusted-CA sync helpers. Use the provided Kubernetes clients to perform the
documented operations, assertions, cleanup, and feature-gate skip behavior so
all Component Proxy specs can execute.
🪄 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: 4288bd75-d622-46e5-a4a2-38bd1f58545a
📒 Files selected for processing (2)
test/e2e-component-proxy/component_proxy.gotest/library/proxy.go
| g.GinkgoWriter.Println("cleaning up: removing Squid proxy") | ||
| proxyCleanup() | ||
| }) | ||
| g.GinkgoWriter.Printf("Squid proxy URL: %s\n", proxyURL) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Redact in-cluster endpoint URLs from test artifacts.
proxyURL and kcClient.IssuerURL() can expose internal hostnames. Remove or redact both values. This extends the existing proxy-URL finding.
As per coding guidelines, “Flag logging that may expose ... internal hostnames.”
Also applies to: 75-75
🤖 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` at line 55, Remove or redact the
internal endpoint values logged by the component proxy test, including proxyURL
in the GinkgoWriter output and kcClient.IssuerURL() at the additionally affected
log site. Preserve useful diagnostic context without exposing in-cluster
hostnames.
Source: Coding guidelines
Comment on lines
+20
to
+21
| func SaveAndRestoreProxyConfig(t testing.TB, operatorClient *operatorclient.Clientset, configClient *configclient.Clientset) (operatorAuth *operatorv1.Authentication, cleanup func()) { | ||
| ctx := context.TODO() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first
git ls-files 'test/library/proxy.go' 'test/**/*.go' | sed -n '1,120p'
echo '--- outline ---'
ast-grep outline test/library/proxy.go --view expanded || true
echo '--- file with line numbers ---'
cat -n test/library/proxy.go | sed -n '1,220p'Repository: openshift/cluster-authentication-operator
Length of output: 7204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' test/library/proxy.go | cat -nRepository: openshift/cluster-authentication-operator
Length of output: 4813
Use deadline-bound contexts here context.TODO() leaves the GET/UPDATE calls and cleanup able to block indefinitely if the API stalls. Use separate context.WithTimeout values for the snapshot and restore paths so cleanup can’t hang forever.
🤖 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 20 - 21, Update SaveAndRestoreProxyConfig
to replace context.TODO() with separate deadline-bound contexts for the initial
configuration snapshot and the cleanup restore path. Apply each timeout context
to its corresponding GET/UPDATE calls, ensure cancellation is handled, and keep
cleanup bounded even if the API stalls.
Source: Path instructions
Comment on lines
+31
to
+47
| fresh, err := operatorClient.OperatorV1().Authentications().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| t.Logf("cleanup: failed to get operator auth: %v", err) | ||
| return | ||
| } | ||
| if originalProxy != nil { | ||
| fresh.Spec.Proxy = *originalProxy | ||
| } else { | ||
| fresh.Spec.Proxy = operatorv1.AuthenticationProxyConfig{} | ||
| } | ||
| if _, err := operatorClient.OperatorV1().Authentications().Update(ctx, fresh, metav1.UpdateOptions{}); err != nil { | ||
| t.Logf("cleanup: failed to restore proxy: %v", err) | ||
| return | ||
| } | ||
| t.Log("cleanup: waiting for operator to pick up changes and stabilize") | ||
| if err := WaitForOperatorToPickUpChanges(t, configClient.ConfigV1(), "authentication"); err != nil { | ||
| t.Logf("cleanup: operator did not recover: %v", err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l test/library/proxy.go
sed -n '1,220p' test/library/proxy.goRepository: openshift/cluster-authentication-operator
Length of output: 4187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "SaveAndRestoreProxyConfig\(" -S .
rg -n "cleanup :=|defer .*cleanup|t\.Cleanup\(" test . -SRepository: openshift/cluster-authentication-operator
Length of output: 3911
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' test/e2e-component-proxy/component_proxy.goRepository: openshift/cluster-authentication-operator
Length of output: 9086
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '220,320p' test/e2e-component-proxy/component_proxy.goRepository: openshift/cluster-authentication-operator
Length of output: 1975
Fail the spec if proxy restore or reconciliation fails.
These cleanup paths should call t.Errorf (including the final WaitForOperatorToPickUpChanges branch) so a passing test can’t leave Authentication.spec.proxy modified and poison later serial tests.
🤖 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 31 - 47, Update the cleanup logic around
the Authentication proxy restoration to call t.Errorf instead of t.Logf when
fetching the resource, updating its proxy, or waiting via
WaitForOperatorToPickUpChanges fails. Preserve the existing early returns, but
ensure each failure marks the spec as failed so proxy restoration or
reconciliation errors cannot leave later tests poisoned.
Source: Path instructions
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
1 participant
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.
This is currently a testing PR for running e2e tests in progress.
This will be eventually added to #946
Summary by CodeRabbit
NO_PROXY.