CNTRLPLANE-3378: enhancements/authentication: Add proxy support for integrated auth stack by tchap · Pull Request #2015 · openshift/enhancements

tchap · 2026-05-18T19:29:08Z

This enhancement adds an optional, component-scoped proxy configuration
to the Authentication operator (operator.openshift.io/v1) so that
authentication components (CAO, OAuth Server) can reach external identity
providers through a proxy without requiring the cluster-wide egress proxy.

openshift-ci-robot · 2026-05-18T19:29:12Z

@tchap: This pull request references CNTRLPLANE-3378 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.

Details

In response to this:

This enhancement adds an optional, component-scoped proxy configuration
to the Authentication operator (operator.openshift.io/v1) so that
authentication components (CAO, OAuth Server) can reach external identity
providers through a proxy without requiring the cluster-wide egress proxy.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2026-05-18T19:29:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jmguzik for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…roxy Adds an optional proxy configuration to the Authentication operator (operator.openshift.io/v1) so that authentication components can reach external identity providers through a proxy without requiring the cluster-wide egress proxy.

liouk

Looks good overall, I've added some comments.

Apart from these, there are a couple more considerations that I'd suggest we think about and add to the enhancement:

it is worth mentioning LDAP Group Sync; this is usually done as a CronJob, and while this is user-managed rather than operator-managed, it should be at least clear in documentation that in such a disconnected scenario, setting the auth proxy will not be enough for LDAP group sync to work.
the CAO makes endpoint accessibility checks which hit the external oauth route, so that will need proxy awareness as well; since the proxy isn't configured in the operator's env but rather read from the config, we'll need to set the endpoint checks' proxy appropriately.

liouk · 2026-05-19T10:42:21Z

+Resolution follows three states:
+
+1. `spec.proxy` set with non-empty values — use component-scoped proxy, overriding any cluster-wide proxy.
+2. `spec.proxy` set with empty strings (`httpProxy: ""`, `httpsProxy: ""`) — explicitly no proxy for auth components, even if a cluster-wide proxy is configured.


I am skeptical about the value of the "override cluster proxy with no proxy for auth" use-case. This implies auth has a direct route to the IDP while nothing else has direct egress, which is an unusual network topology.

Given this assumption I see why we choose *string over string for these fields, however if indeed there's no value to this use-case, we could simplify the API further by removing this particular flow.

Yeah, I was hesitant regarding possible values for the proxy object. I was thinking that either

When the proxy object is set, httpProxy and httpsProxy are simply required. In that case it can be string.

Or allow for meaningful zero values, which must be set explicitly, though. In that case I would use *string and using an empty string would disable proxying. I am not sure this makes sense with respect to overwriting the cluster-wide proxy, but just considering the component-scoped proxy, does it ever make sense to have one of the values unset? If not, we can simply go with string and require both values.

I share @liouk's skepticism on the use case for having the cluster-wide proxy configured but omitting auth components specifically. Instead, I'd expect any domains that shouldn't be proxied to be defined in the cluster-wide noProxy configuration.

I'm also not aware of any requests for behavior like this.

Do you mean that we should inherit the noProxy field from the cluster-wide proxy when that is available?

@everettraven @tchap unless I'm missing some use-case other than the IDP, I think what we need here is no inheritance from the cluster-wide proxy (CWP) at all when the auth proxy (AP) is set; we should just let the operator append the default noProxy (in-cluster) values and that's all.

Since setting the AP gets priority over the CWP, inheriting what the CWP sets might lead to a conflict. Imagine the CWP sets the IDP's domain in noProxy, and we also set an AP -- the AP takes precedence over the CWP, but noProxy dictates that calls to the IDP shouldn't be proxied.

This is going to happen soon.

It should reuse the same proxy object

tchap · 2026-05-19T13:41:56Z

it is worth mentioning LDAP Group Sync; this is usually done as a CronJob, and while this is user-managed rather than operator-managed, it should be at least clear in documentation that in such a disconnected scenario, setting the auth proxy will not be enough for LDAP group sync to work.

Added in a16c166

everettraven · 2026-05-19T13:46:30Z

+Resolution follows three states:
+
+1. `spec.proxy` set with non-empty values — use component-scoped proxy, overriding any cluster-wide proxy.
+2. `spec.proxy` set with empty strings (`httpProxy: ""`, `httpsProxy: ""`) — explicitly no proxy for auth components, even if a cluster-wide proxy is configured.


I share @liouk's skepticism on the use case for having the cluster-wide proxy configured but omitting auth components specifically. Instead, I'd expect any domains that shouldn't be proxied to be defined in the cluster-wide noProxy configuration.

I'm also not aware of any requests for behavior like this.

There were 2 separate sections. Now when adding endpoint accessiblity details, this became too fragmented. So there is now one Operator Process section with multiple bullet points for each component affected.

tchap · 2026-05-20T11:01:51Z

the CAO makes endpoint accessibility checks which hit the external oauth route, so that will need proxy awareness as well; since the proxy isn't configured in the operator's env but rather read from the config, we'll need to set the endpoint checks' proxy appropriately.

Added in c878755

Add a table listing which controllers make outbound calls to external IdP endpoints, covering both Integrated OAuth and External OIDC modes with links to the relevant code locations.

Add code links to affected controllers with two-column format (component + outbound call). Add custom route controller which was missing from the list of controllers needing proxy awareness. Add deployment controller to the affected list.

openshift-ci · 2026-05-21T15:20:31Z

@tchap: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

everettraven

Only a handful of comments. Overall this looks pretty solid to me. Great work!

everettraven · 2026-05-21T18:34:58Z

+  - "@everettraven"
+creation-date: 2026-05-18
+last-updated: 2026-05-21
+status: provisional|implementable|implemented|deferred|rejected|withdrawn|replaced|informational


implementable is probably a good choice at this point.

everettraven · 2026-05-21T18:41:05Z

+	// When omitted, the cluster-wide proxy is used, preserving
+	// existing behavior.


We generally try to keep the GoDoc comments for APIs end-user focused and avoid leaking underlying component logic/behaviors that they won't care about. In this case, "preserving existing behavior" is likely ambiguous to an end user reading this so we should just be explicitly clear as to what the behavior here is:

Suggested change

// When omitted, the cluster-wide proxy is used, preserving

// existing behavior.

// When omitted, the cluster-wide proxy configuration is used.

// If the cluster-wide proxy is not configured, no proxy is used.

everettraven · 2026-05-21T18:42:06Z

+	OperatorSpec `json:",inline"`
+
+	// proxy configures proxy settings for authentication components
+	// (the OAuth server and the cluster authentication operator).


In the same vein of avoiding leaking of underlying architectural details, we can just remove the text within the parenthesis here:

Suggested change

// (the OAuth server and the cluster authentication operator).

everettraven · 2026-05-21T18:50:52Z

+	// httpProxy is the URL of the proxy for HTTP requests.
+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:XValidation:rule="isURL(self)",message="httpProxy must be a valid URL"
+	// +optional
+	HTTPProxy string `json:"httpProxy,omitempty"`
+
+	// httpsProxy is the URL of the proxy for HTTPS requests.
+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:XValidation:rule="isURL(self)",message="httpsProxy must be a valid URL"
+	// +optional
+	HTTPSProxy string `json:"httpsProxy,omitempty"`


Both of these fields need a maximum length constraint to prevent the CEL rule from always assuming a worst-case string size for cost estimations.

For URLs I typically see 2048 characters being the limit here.

Also, isURL(self) parses this according to Golang's net/url.Parse() function that is pretty lenient in what a "valid URL" entails. For example, https:// parses as a valid URL.

We probably want some additional constraints enforced here for what would be considered a valid input. At the very least we probably want to ensure a scheme of http:// or https:// is specified and that there is a hostname.

Beyond that, I'd think about whether or not we should allow specifying the path, query parameters, fragments, and user info (I suspect we may want to allow user info since the cluster-wide proxy today does proxy authentication via the proxy url).

everettraven · 2026-05-21T19:13:31Z

+| Mode | Controller | Component | Outbound call |
+|------|------------|-----------|---------------|
+| Integrated OAuth | Config observation | [`ObserveIdentityProviders()`](https://github.com/openshift/cluster-authentication-operator/blob/9d5f19f8ad67/pkg/controllers/configobservation/oauth/observe_idps.go#L19) | [`discoverOpenIDURLs()`](https://github.com/openshift/cluster-authentication-operator/blob/9d5f19f8ad67/pkg/controllers/configobservation/oauth/idp_conversions.go#L315) |
+| Integrated OAuth | Config observation | [`ObserveIdentityProviders()`](https://github.com/openshift/cluster-authentication-operator/blob/9d5f19f8ad67/pkg/controllers/configobservation/oauth/observe_idps.go#L19) | [`checkOIDCPasswordGrantFlow()`](https://github.com/openshift/cluster-authentication-operator/blob/9d5f19f8ad67/pkg/controllers/configobservation/oauth/idp_conversions.go#L373) |


A bit of an aside, this is a weird way to determine if the provider supports the password grant flow.

If we are doing discovery, we could probably drop that in favor of basing off of the spec outlined in https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata but 🤷 .

I'm debating whether or not it would be worth it to change this behavior as part of this to reduce the areas we need to actually take the custom proxy into account. At the very least we should probably make some kind of tech debt item for us to track to take a look at this at some time in the future.

everettraven · 2026-05-21T19:24:55Z

+## Test Plan
+
+TBD
+
+## Graduation Criteria
+
+TBD
+
+### Dev Preview -> Tech Preview
+
+### Tech Preview -> GA
+


We should fill these out with at least a high-level plan of how we are going to test the feature and determine whether it is ready to be promoted or not (outside of the standard requirements for all new features)

everettraven · 2026-05-21T19:25:21Z

+
+### Tech Preview -> GA
+
+### Removing a deprecated feature


Can add n/a for the text in this section since it doesn't actually apply to us here.

everettraven · 2026-05-21T19:28:16Z

+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=253
+	// +required
+	Name string `json:"name"`


I missed this in my last review, but this needs an omitempty tag.

We also want to add this CEL validation to ensure the correct input format for a ConfigMap name:

+kubebuilder:validation:XValidation:rule="!format.dns1123Subdomain().validate(self).hasValue()",message="name must be a valid DNS subdomain name: contain no more than 253 characters, contain only lowercase alphanumeric characters, '-' or '.', and start and end with an alphanumeric character"

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 18, 2026

openshift-ci Bot requested review from flavianmissi and tremes May 18, 2026 19:29

tchap force-pushed the auth-proxy branch 3 times, most recently from 2704243 to dff70c3 Compare May 18, 2026 19:51

tchap force-pushed the auth-proxy branch from dff70c3 to 89b3e00 Compare May 19, 2026 09:33

liouk suggested changes May 19, 2026

View reviewed changes

tchap added 4 commits May 19, 2026 14:24

Mention that trustedCA must be watched for changes

e58ad87

Mention External OIDC gated as TechPreview

697f861

This is going to happen soon.

Clarify future External OIDC config

bf7ea2a

It should reuse the same proxy object

Document that LDAP sync must be handled manually

a16c166

everettraven reviewed May 19, 2026

View reviewed changes

Reconcile Operator Process details into a block

c878755

There were 2 separate sections. Now when adding endpoint accessiblity details, this became too fragmented. So there is now one Operator Process section with multiple bullet points for each component affected.

tchap added 9 commits May 20, 2026 13:08

Fill in reviewers and approvers

9ba642c

Remove noProxy from spec.proxy

ff174c0

Remove open question: Secret for credentials

5c707f7

Rework API types and validation

087a402

Document outbound IdP calls by authentication mode

941cfd6

Add a table listing which controllers make outbound calls to external IdP endpoints, covering both Integrated OAuth and External OIDC modes with links to the relevant code locations.

Add discoveryURL reverse proxy as rejected alternative

6c77199

Apply Claude review suggestions

37efae7

Clarify endpoint accessible and custom route controller descriptions

d0c2ee0

everettraven reviewed May 21, 2026

View reviewed changes

		// When omitted, the cluster-wide proxy is used, preserving
		// existing behavior.

Conversation

tchap commented May 18, 2026

Uh oh!

openshift-ci-robot commented May 18, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented May 18, 2026

Uh oh!

liouk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchap May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liouk May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchap commented May 19, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tchap commented May 20, 2026

Uh oh!

openshift-ci Bot commented May 21, 2026

Uh oh!

everettraven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

openshift-ci-robot commented May 18, 2026 •

edited by openshift-ci Bot

Loading

tchap May 19, 2026 •

edited

Loading

liouk May 28, 2026 •

edited

Loading