WIP: CNTRLPLANE-3762: Add support for component-scoped proxy#946
Conversation
|
@tchap: This pull request references CNTRLPLANE-3762 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
/jira refresh |
|
@tchap: This pull request references CNTRLPLANE-3762 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
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:
WalkthroughAdds component-scoped proxy resolution and threads it through controller wiring, transport creation, deployment generation, route and endpoint checks, and proxy validation. Also updates the module replace directive. ChangesComponent proxy flow
Estimated code review effort: 4 (Complex) | ~75 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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 |
834349a to
82ce053
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/controllers/configobservation/oauth/observe_idps.go (1)
57-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the component-proxy transport branch.
The new component-scoped proxy resolution and CA-loading logic (lines 61-78) that swaps
buildTransporttotransport.TransportForCARefWithProxyisn't exercised by any test in this batch. This is the most complex new logic in the cohort and currently only has a default/no-proxy path tested (viaidp_conversions_test.go).Want me to draft a test for
ObserveIdentityProviderscovering: authProxy present with valid TrustedCA, authProxy present with missing/unreadable TrustedCA configmap, andGetComponentProxyConfigreturning an error?🤖 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_idps.go` around lines 57 - 82, Add test coverage for the component-proxy branch in ObserveIdentityProviders, since the new authProxy path that replaces buildTransport with TransportForCARefWithProxy is not exercised yet. Add cases around observeIdentityProviders/ObserveIdentityProviders that cover authProxy being present with a valid TrustedCA ConfigMap, authProxy present but the TrustedCA ConfigMap missing or unreadable, and GetComponentProxyConfig returning an error. Verify the buildTransport closure uses the proxy values and CA data when authProxy is available, and that the fallback/default transport path is retained when proxy config lookup fails.pkg/controllers/proxyconfig/proxyconfig_controller.go (1)
166-172: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueComponent proxy TrustedCA changes aren't watched.
Unlike the static
caConfigMaps(filtered informers), the component-proxy'sTrustedCA.Nameconfigmap is resolved dynamically and isn't registered for change notifications — updates to it will only be picked up on the 60-minute periodic resync.🤖 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 166 - 172, The component proxy TrustedCA lookup in the proxy config controller is only fetched on resync and is not watched for updates. Update the proxyconfig controller logic around the TrustedCA handling in proxyconfig_controller.go so the configmap referenced by authProxy.TrustedCA.Name is registered for change notifications through the existing informer/filtering path, similar to how caConfigMaps are tracked, and ensure the reconcile flow in the proxyconfig controller reacts when that configmap changes.pkg/controllers/proxyconfig/proxyconfig_controller_test.go (1)
244-353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the hash-caching skip path.
Coverage here validates the reachability/warning-event behavior well, but there's no test exercising repeated calls with identical config to confirm the caching short-circuit (
hash == p.lastIdPValidationHash) behaves as intended — this ties into the caching-design question raised inproxyconfig_controller.go.🤖 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_test.go` around lines 244 - 353, Add a test that exercises the hash-caching short-circuit in validateIdPConnectivity, since Test_validateIdPConnectivity currently only covers reachability and warning events. Reuse proxyConfigChecker and its validateIdPConnectivity method with the same OAuth/IdentityProvider input twice, then assert the second call hits the skip path when hash matches p.lastIdPValidationHash and does not re-emit events or repeat validation work. Use the existing test helpers like workingHTTPRoundTripper, faultyHTTPRoundTripper, and the in-memory recorder to keep the case aligned with the current test structure.
🤖 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 `@go.mod`:
- Around line 145-147: Remove the personal-fork replace override from go.mod:
the current replace for github.com/openshift/api pointing to
github.com/tchap/api should not remain in the merged branch. Update the module
definition so github.com/openshift/api resolves to the upstream dependency
again, and keep any temporary branch-only override out of the final manifest.
Use the replace directive and github.com/openshift/api entry as the identifiers
to locate and delete the temporary fork reference.
---
Nitpick comments:
In `@pkg/controllers/configobservation/oauth/observe_idps.go`:
- Around line 57-82: Add test coverage for the component-proxy branch in
ObserveIdentityProviders, since the new authProxy path that replaces
buildTransport with TransportForCARefWithProxy is not exercised yet. Add cases
around observeIdentityProviders/ObserveIdentityProviders that cover authProxy
being present with a valid TrustedCA ConfigMap, authProxy present but the
TrustedCA ConfigMap missing or unreadable, and GetComponentProxyConfig returning
an error. Verify the buildTransport closure uses the proxy values and CA data
when authProxy is available, and that the fallback/default transport path is
retained when proxy config lookup fails.
In `@pkg/controllers/proxyconfig/proxyconfig_controller_test.go`:
- Around line 244-353: Add a test that exercises the hash-caching short-circuit
in validateIdPConnectivity, since Test_validateIdPConnectivity currently only
covers reachability and warning events. Reuse proxyConfigChecker and its
validateIdPConnectivity method with the same OAuth/IdentityProvider input twice,
then assert the second call hits the skip path when hash matches
p.lastIdPValidationHash and does not re-emit events or repeat validation work.
Use the existing test helpers like workingHTTPRoundTripper,
faultyHTTPRoundTripper, and the in-memory recorder to keep the case aligned with
the current test structure.
In `@pkg/controllers/proxyconfig/proxyconfig_controller.go`:
- Around line 166-172: The component proxy TrustedCA lookup in the proxy config
controller is only fetched on resync and is not watched for updates. Update the
proxyconfig controller logic around the TrustedCA handling in
proxyconfig_controller.go so the configmap referenced by
authProxy.TrustedCA.Name is registered for change notifications through the
existing informer/filtering path, similar to how caConfigMaps are tracked, and
ensure the reconcile flow in the proxyconfig controller reacts when that
configmap changes.
🪄 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: 2b46b993-e90b-4627-906c-fc961b3829e3
⛔ Files ignored due to path filters (32)
go.sumis excluded by!**/*.sumvendor/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/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/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois 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/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (18)
go.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/customroute/custom_route_conditions.gopkg/controllers/customroute/custom_route_controller.gopkg/controllers/deployment/default_deployment.gopkg/controllers/deployment/deployment_controller.gopkg/controllers/oauthendpoints/oauth_endpoints_controller.gopkg/controllers/proxyconfig/proxyconfig_controller.gopkg/controllers/proxyconfig/proxyconfig_controller_test.gopkg/libs/endpointaccessible/endpoint_accessible_controller.gopkg/operator/starter.gopkg/transport/transport.go
|
|
||
| // branch: cao-proxy | ||
| replace github.com/openshift/api => github.com/tchap/api v0.0.0-20260630145754-8d392a59d62d |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove personal-fork replace directive before merge.
This overrides github.com/openshift/api with an individual's fork (github.com/tchap/api) on a feature branch. This is fine for local development against unmerged vendor API changes, but must not ship — it's an unmaintained, force-pushable dependency source and a supply-chain risk. Track removal once the real openshift/api PR adding AuthenticationProxyConfig/AuthenticationConfigMapReference merges upstream.
As per path instructions, "New deps: justify need, check license compatibility" and "No pre-release or yanked versions in production" for files matching go.mod.
🤖 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 `@go.mod` around lines 145 - 147, Remove the personal-fork replace override
from go.mod: the current replace for github.com/openshift/api pointing to
github.com/tchap/api should not remain in the merged branch. Update the module
definition so github.com/openshift/api resolves to the upstream dependency
again, and keep any temporary branch-only override out of the final manifest.
Use the replace directive and github.com/openshift/api entry as the identifiers
to locate and delete the temporary fork reference.
Source: Path instructions
26d4cf7 to
c1080cc
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/controllers/proxyconfig/proxyconfig_controller.go (1)
174-174: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional: set
MinVersionon this outbound TLS config.Same as the customroute check — this client reaches the route
/healthz(and, viaclientWithProxy, external IdP endpoints invalidateIdPConnectivity). The Go client default is TLS 1.2, so no live risk, but pinningMinVersiondocuments intent for these outbound calls.🔒 Suggested change
- tlsConfig := &tls.Config{RootCAs: caPool} + tlsConfig := &tls.Config{MinVersion: tls.VersionTLS12, RootCAs: caPool}🤖 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` at line 174, The outbound TLS config in the proxy config controller should explicitly pin a minimum TLS version for these client connections. Update the tls.Config initialization in the proxy setup path (the tlsConfig used with clientWithProxy and the /healthz / validateIdPConnectivity calls) to set MinVersion consistently with the other outbound TLS checks so the intent is documented and enforced.Source: Linters/SAST tools
pkg/controllers/customroute/custom_route_conditions.go (1)
166-168: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional: set
MinVersionon the TLS config.Static analysis flags a missing
MinVersion. The Go client default is already TLS 1.2, so this is not a live vulnerability, but pinning it makes the intent explicit for this outbound check.🔒 Suggested change
TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, RootCAs: rootCAs, },🤖 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 166 - 168, The TLS config in custom route outbound check currently only sets RootCAs and leaves the minimum protocol version implicit. Update the tls.Config used in this client setup to explicitly set MinVersion in the same TLSClientConfig block, using the existing configuration path around the outbound check so the intent is clear in the code.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/controllers/customroute/custom_route_conditions.go`:
- Around line 166-168: The TLS config in custom route outbound check currently
only sets RootCAs and leaves the minimum protocol version implicit. Update the
tls.Config used in this client setup to explicitly set MinVersion in the same
TLSClientConfig block, using the existing configuration path around the outbound
check so the intent is clear in the code.
In `@pkg/controllers/proxyconfig/proxyconfig_controller.go`:
- Line 174: The outbound TLS config in the proxy config controller should
explicitly pin a minimum TLS version for these client connections. Update the
tls.Config initialization in the proxy setup path (the tlsConfig used with
clientWithProxy and the /healthz / validateIdPConnectivity calls) to set
MinVersion consistently with the other outbound TLS checks so the intent is
documented and enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6648ea73-99c6-4c00-85df-d8165bf94bf5
📒 Files selected for processing (17)
pkg/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/customroute/custom_route_conditions.gopkg/controllers/customroute/custom_route_controller.gopkg/controllers/deployment/default_deployment.gopkg/controllers/deployment/deployment_controller.gopkg/controllers/oauthendpoints/oauth_endpoints_controller.gopkg/controllers/proxyconfig/proxyconfig_controller.gopkg/controllers/proxyconfig/proxyconfig_controller_test.gopkg/libs/endpointaccessible/endpoint_accessible_controller.gopkg/operator/starter.gopkg/transport/transport.go
🚧 Files skipped from review as they are similar to previous changes (15)
- pkg/controllers/configobservation/configobservercontroller/observe_config_controller.go
- pkg/controllers/configobservation/interfaces.go
- pkg/controllers/oauthendpoints/oauth_endpoints_controller.go
- pkg/controllers/customroute/custom_route_controller.go
- pkg/controllers/common/proxy_test.go
- pkg/controllers/configobservation/oauth/observe_idps.go
- pkg/libs/endpointaccessible/endpoint_accessible_controller.go
- pkg/controllers/configobservation/oauth/idp_conversions_test.go
- pkg/operator/starter.go
- pkg/controllers/common/proxy.go
- pkg/controllers/deployment/default_deployment.go
- pkg/transport/transport.go
- pkg/controllers/configobservation/oauth/idp_conversions.go
- pkg/controllers/deployment/deployment_controller.go
- pkg/controllers/proxyconfig/proxyconfig_controller_test.go
66159bf to
5319ce6
Compare
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.
95f8e10 to
951fb5d
Compare
Add shared utilities for component-scoped proxy configuration: - GetComponentProxyConfig: reads spec.proxy from the Authentication operator CR when the AuthenticationComponentProxy feature gate is enabled. - ResolveProxyConfig: implements component > cluster-wide > none precedence. No per-field inheritance from the cluster-wide proxy. - ComponentProxyFunc: returns a closure that re-resolves the proxy on each HTTP request, falling back to http.ProxyFromEnvironment. - mergeNoProxy: appends cluster-internal defaults (.cluster.local, .svc, 127.0.0.1, localhost) to user-provided noProxy entries.
8428319 to
04baf0e
Compare
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; service and endpoint checks remain proxy-free. Custom route controller: uses ComponentProxyFunc for the route availability health check, falling back to http.ProxyFromEnvironment. Operator startup: passes operatorAuthLister, featureGateAccessor, and operatorAuthInformer to all affected controllers.
04baf0e to
65811c0
Compare
This implements the component-scoped proxy enhancement for this operator.
There are no e2e tests as of now.
Summary by CodeRabbit
noProxymerging.