WIP: E2E tests for the Component Proxy#949
Open
tchap wants to merge 21 commits into
Open
Conversation
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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
Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 4 warnings, 1 inconclusive)
✅ 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
+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
+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.
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
openshift-ci
Bot
added
the
needs-rebase
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 |
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
There was a problem hiding this comment.
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)
265-288: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
proxyCleanupis reassigned, so the Squid proxy is never cleaned up.Line 265 binds
proxyCleanupto the Squid cleanup, and theDeferCleanupat Lines 266-269 closes over that variable. Line 273 uses:=withoperatorAuth(the only new name) in the same block scope, so it reassignsproxyCleanupto theSaveAndRestoreProxyConfigcleanup rather than declaring a new variable. As a result the "removing Squid proxy" deferred closure invokes the restore function instead, the Squid deployment is never torn down (resource leak that can contaminate subsequent serial tests), and the restore cleanup runs twice (Lines 269 and 287).🐛 Proposed fix: use a distinct name for the restore cleanup
g.By("Saving original proxy config for cleanup") - operatorAuth, proxyCleanup := test.SaveAndRestoreProxyConfig(t, clients.OperatorClient, clients.ConfigClient) + operatorAuth, proxyRestore := test.SaveAndRestoreProxyConfig(t, clients.OperatorClient, clients.ConfigClient) @@ g.GinkgoWriter.Println("cleaning up: deleting fake IdP secret") _ = clients.KubeClient.CoreV1().Secrets("openshift-config").Delete(ctx, fakeIDPSecretName, metav1.DeleteOptions{}) - proxyCleanup() + proxyRestore() })🤖 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 265 - 288, Rename the cleanup returned by SaveAndRestoreProxyConfig in the operatorAuth setup to a distinct symbol, such as a proxy-config restore cleanup, so it does not reassign the Squid proxy cleanup captured by the first DeferCleanup. Update the later deferred cleanup to invoke the renamed restore function, while leaving the original proxyCleanup closure responsible only for deleting the Squid proxy.
🤖 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.
Outside diff comments:
In `@test/e2e-component-proxy/component_proxy.go`:
- Around line 265-288: Rename the cleanup returned by SaveAndRestoreProxyConfig
in the operatorAuth setup to a distinct symbol, such as a proxy-config restore
cleanup, so it does not reassign the Squid proxy cleanup captured by the first
DeferCleanup. Update the later deferred cleanup to invoke the renamed restore
function, while leaving the original proxyCleanup closure responsible only for
deleting the Squid proxy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 44fdc303-375b-4320-a04d-9ddb59cb0e85
📒 Files selected for processing (3)
test/e2e-component-proxy/component_proxy.gotest/library/keycloakidp.gotest/library/proxy.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/library/proxy.go
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).
tchap
force-pushed
the
disconnected-env-e2e
branch
from
July 21, 2026 10:31
1575d36 to
ce6e2da
Compare
openshift-ci
Bot
removed
the
needs-rebase
Use sets.List() instead of sets.UnsortedList() to produce sorted, deterministic noProxy values and prevent oauth-server deployment churn. Remove the now-unnecessary sortedCSV test helper and the standalone TestResolveProxy_FeatureGateError that duplicated a table-driven case.
tchap
force-pushed
the
disconnected-env-e2e
branch
from
July 23, 2026 12:09
39b47b3 to
9596fb9
Compare
The OAuthServerRouteEndpointAccessibleController checks the OAuth route health through the proxy. When the proxy uses TLS (https://), the controller needs the proxy's trustedCA in its root CA pool. Use ResolveProxy to get the TrustedCAName, then load the CA from the source ConfigMap in openshift-config via transport.LoadCAData — consistent with how ProxyConfigController loads it.
tchap
force-pushed
the
disconnected-env-e2e
branch
from
July 23, 2026 12:48
9596fb9 to
3a1d8fd
Compare
Go's in-cluster client connects to the kube API by raw IP (from KUBERNETES_SERVICE_HOST env var), not by hostname, so the existing .svc and .cluster.local entries do not exclude it from proxy routing. Add the IP explicitly so oauth-openshift pods don't route kube API calls through the component proxy.
Verify that setting spec.proxy.httpsProxy to an unreachable host on the operator Authentication CR causes ProxyConfigControllerDegraded to become True, propagating to ClusterOperator Degraded=True. The test is gated behind the AuthenticationComponentProxy feature gate and registered in the OTE serial/operator suite.
Add C2 test: deploy a Squid proxy, configure spec.proxy to point at it, add a fake OpenID IdP with an unresolvable issuer URL, and verify the ProxyConfigController emits an IdPEndpointUnreachable Warning event without going Degraded. Also add stub helper functions in test/library/proxy.go for future proxy e2e infrastructure (DeploySquidProxy, DeployProxyNetworkPolicies, etc.). Use WaitForOperatorToPickUpChanges in both C1 and C2 cleanup to avoid racing with stale operator status.
…dd A1 test Move proxy config save/restore logic into a shared test helper in test/library/proxy.go. Add the A1 test (OIDC IdP validation through component proxy) and clean up C1/C2 tests to use the shared helper and exported CheckFeatureGateEnabledOrSkip. Remove redundant nil check in C1 test.
Add Group A proxy e2e tests: - A1: validate OIDC IdP through component proxy, with and without trustedCA (two g.It variants sharing testOIDCIdPThroughComponentProxy) - A2: verify operator falls back gracefully on spec.proxy removal Split AddKeycloakIDP into DeployKeycloak + AddKeycloakOIDCIdP so tests can control ordering (deploy Keycloak, apply NetworkPolicy, set proxy, then register IdP). AddKeycloakIDP remains as a convenience wrapper. Simplify DeploySquidProxy to always generate TLS internally and return the CA PEM bytes. Update CheckFeatureGateEnabledOrSkip to replace the local checkFeatureGateOrSkip in all tests.
DeploySquidProxy now listens on both HTTP and HTTPS and returns the raw host:port. Callers prepend http:// (no trustedCA) or https:// (with trustedCA) to construct the proxy URL. This lets the A1 test cover both cases with a single helper.
Accept expectedHTTPProxy, expectedHTTPSProxy, expectedNoProxy, and expectTrustedCAVolume so callers specify exactly what to assert. Empty strings mean the env var should be absent.
- Implement DeploySquidProxy, DeployProxyNetworkPolicies, GetSquidProxyLogs, WaitForSquidProxyTraffic, VerifyOAuthServerDeploymentProxyConfig, VerifyTrustedCAConfigMapSynced, and CheckFeatureGateEnabledOrSkip in the test library - Wrap DeploySquidProxy cleanup with sync.OnceFunc to prevent double-cleanup when the internal failure defer and DeferCleanup both fire - Simplify VerifyOAuthServerDeploymentProxyConfig to compare env var values directly (proxy env vars are always set, just empty when unset) - Remove redundant GetOAuthServerProxyEnvVars call in testFallbackOnProxyRemoval
…nfig Return separate httpProxyURL and httpsProxyURL from DeploySquidProxy. Fix Squid 7 TLS syntax (tls-cert=/tls-key= instead of cert=/key=), add pid_filename /tmp/squid.pid for restricted PSA, pin Squid image to 7.2-26.04_edge.
tchap
force-pushed
the
disconnected-env-e2e
branch
from
July 23, 2026 17:50
3a1d8fd to
ee6d9be
Compare
Contributor
|
@tchap: The following test 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. |
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_PROXYhandling.