OCPBUGS-88312: use in-cluster service for oauth-server - #8772
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@mihivagyok: This pull request references Jira Issue OCPBUGS-88312, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR changes OAuth in-cluster DNS handling for config, certificate SANs, and deployment proxy settings. It adds a helper to derive the OAuth service DNS name from the hosted control plane namespace, uses that DNS name for Sequence Diagram(s)sequenceDiagram
participant HostedControlPlane
participant adaptOAuthConfig
participant ReconcileOAuthServerCert
participant adaptDeployment
HostedControlPlane->>adaptOAuthConfig: namespace, OAuth host/port
adaptOAuthConfig->>adaptOAuthConfig: build in-cluster MasterURL
HostedControlPlane->>ReconcileOAuthServerCert: externalOAuthAddress
ReconcileOAuthServerCert->>ReconcileOAuthServerCert: add in-cluster DNS SAN
HostedControlPlane->>adaptDeployment: platform, namespace
adaptDeployment->>adaptDeployment: upsert NO_PROXY entries
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@mihivagyok: This pull request references Jira Issue OCPBUGS-88312, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
area/control-plane-operator
d31dfac to
36f1992
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.go (1)
73-73: ⚡ Quick winCentralize OAuth service DNS construction to avoid cross-file drift.
Line 73 hardcodes the same namespace-scoped DNS contract that is also repeated in
params.goanddeployment.go. A shared helper/constant foroauth-openshift.<ns>.svc.cluster.local(and port where needed) will keep MasterURL/NO_PROXY paths consistent over time.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.go` at line 73, Extract the hardcoded OAuth service DNS construction pattern (oauth-openshift.<namespace>.svc.cluster.local) from line 73 in config.go where it's used to set cfg.OAuthConfig.MasterURL, and create a shared helper function or constant in a common location that constructs this DNS name given a namespace parameter. Then replace the inline string concatenation in config.go with a call to this helper, and apply the same helper function to params.go and deployment.go where this same pattern is repeated to ensure consistency across all three files.
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go`:
- Around line 55-63: Add unit tests to verify the NO_PROXY environment variable
configuration logic in the deployment setup. The tests should validate that the
noProxy list includes the namespace-scoped oauth-openshift service name, verify
that IBM Cloud entries (iam.cloud.ibm.com and iam.test.cloud.ibm.com) are only
appended when the platform type is hyperv1.IBMCloudPlatform (not for other
platform types), and confirm that the NO_PROXY environment variable is correctly
set via UpsertEnvVar with the comma-joined noProxy values. Include both positive
cases (when IBM Cloud platform is used) and negative cases (when other platforms
are used) to ensure the conditional logic works as intended.
---
Nitpick comments:
In `@control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.go`:
- Line 73: Extract the hardcoded OAuth service DNS construction pattern
(oauth-openshift.<namespace>.svc.cluster.local) from line 73 in config.go where
it's used to set cfg.OAuthConfig.MasterURL, and create a shared helper function
or constant in a common location that constructs this DNS name given a namespace
parameter. Then replace the inline string concatenation in config.go with a call
to this helper, and apply the same helper function to params.go and
deployment.go where this same pattern is repeated to ensure consistency across
all three files.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 24758d2f-26ec-471a-983d-a60a8fa10823
📒 Files selected for processing (3)
control-plane-operator/controllers/hostedcontrolplane/oauth/params.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8772 +/- ##
==========================================
+ Coverage 43.12% 43.13% +0.01%
==========================================
Files 766 766
Lines 94872 94873 +1
==========================================
+ Hits 40909 40927 +18
+ Misses 51115 51093 -22
- Partials 2848 2853 +5
... and 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
control-plane-operator/controllers/hostedcontrolplane/oauth/params.go (1)
41-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the comment to reflect broader usage.
The comment states the
OAuthNoProxyfield is "Currently only used for IBM Cloud specific addresses," but line 68 now seeds it with kube-apiserver, audit webhook service, and the OAuth service for all platforms. IBM Cloud-specific domains are conditionally appended later at line 98.📝 Proposed fix
// OAuthNoProxy is a list of hosts or IPs that should not be routed through -// konnectivity. Currently only used for IBM Cloud specific addresses. +// konnectivity. Includes in-cluster service endpoints and platform-specific addresses. OAuthNoProxy []string🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/oauth/params.go` around lines 41 - 43, The comment for the `OAuthNoProxy` field incorrectly states it is "Currently only used for IBM Cloud specific addresses" when in fact the field is seeded with addresses for kube-apiserver, audit webhook service, and OAuth service for all platforms (as referenced in line 68), with IBM Cloud-specific domains only conditionally appended later. Update the comment to accurately describe that OAuthNoProxy contains hosts or IPs across multiple platforms that should not be routed through konnectivity, noting that platform-specific addresses like IBM Cloud domains may be conditionally included as needed.
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/oauth/params.go`:
- Line 68: In the OAuthNoProxy slice assignment in params.go, replace the
hardcoded service name "oauth-openshift" with the helper function
manifests.OauthServerService("").Name. This should be done in the FQDN
construction where the service name is concatenated with the namespace,
following the same pattern already established on the same line where
manifests.KubeAPIServerService("").Name is used. This change improves
consistency and maintainability by using the provided service name helper
function instead of hardcoding the string.
---
Outside diff comments:
In `@control-plane-operator/controllers/hostedcontrolplane/oauth/params.go`:
- Around line 41-43: The comment for the `OAuthNoProxy` field incorrectly states
it is "Currently only used for IBM Cloud specific addresses" when in fact the
field is seeded with addresses for kube-apiserver, audit webhook service, and
OAuth service for all platforms (as referenced in line 68), with IBM
Cloud-specific domains only conditionally appended later. Update the comment to
accurately describe that OAuthNoProxy contains hosts or IPs across multiple
platforms that should not be routed through konnectivity, noting that
platform-specific addresses like IBM Cloud domains may be conditionally included
as needed.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9043bd14-706b-40f3-9ae8-545c17df2010
📒 Files selected for processing (3)
control-plane-operator/controllers/hostedcontrolplane/oauth/params.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go
🚧 Files skipped from review as they are similar to previous changes (2)
- control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.go
- control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go
51ef123 to
fa6e463
Compare
muraee
left a comment
muraee
left a comment
There was a problem hiding this comment.
Review Findings
Blockers
1. TLS SAN mismatch — oauth serving cert doesn't include the in-cluster FQDN
ReconcileOAuthServerCert (pki/oauth.go) generates the oauth serving cert with only the external OAuth hostname as a SAN. The new MasterURL (https://oauth-openshift.<ns>.svc.cluster.local:6443) is not in the cert's SANs. When the oauth process connects to that URL (bypassing konnectivity via NO_PROXY), TLS hostname verification should fail.
Either:
- Add
oauth-openshift.<ns>.svc.cluster.localto the SANs inReconcileOAuthServerCert, or - Confirm that TLS hostname verification is skipped/disabled for
MasterURLconnections
2. params.go — OAuthNoProxy is dead code
The OAuthNoProxy field is set in params.go but never consumed outside that file. NewOAuthServerParams is only called from reconcileValidIDPConfigurationCondition which checks IDP validity — it doesn't drive deployment reconciliation. This change has no runtime effect and should be removed or wired up.
Required
3. Use OAuthServerPort constant instead of hardcoded 6443
The port is hardcoded in v2/oauth/config.go. There's already oauth.OAuthServerPort = 6443 in oauth/service.go — use it instead of the magic number.
4. Tests for adaptOAuthConfig and adaptDeployment
No unit tests added (checklist item unchecked). Given the connectivity fix and the behavioral change to NO_PROXY for all platforms, table-driven tests verifying MasterURL, MasterPublicURL, and NO_PROXY contents per platform would make this safer to maintain.
236271e to
0760e98
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mihivagyok, muraee, rtheis The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/test e2e-aks |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest |
|
Now I have all the evidence needed. Let me synthesize the analysis. The key findings are:
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryTwo independent test failures occurred, neither caused by PR #8772's changes. The PR modifies only the oauth-server's Root CauseFailure 1 — TestCreateCluster/EnsureGlobalPullSecret (cascade: 5 test failures) The This is a transient data-plane issue — one node's Failure 2 — TestKarpenterUpgradeControlPlane (1 test failure) The test flow was:
Karpenter detected the drift but failed to complete the node replacement cycle within 45 minutes. The old node was healthy (Ready=True, no resource pressure) but running the pre-upgrade OS version. This is a Karpenter node lifecycle issue — either the replacement node failed to launch, or Karpenter did not cordon/drain/terminate the old node. PR #8772 does not modify anything related to node lifecycle, machine management, or Karpenter integration. CI Infrastructure Pattern:
This pattern of different failure modes across retests, combined with AWS quota exhaustion in prior runs, strongly indicates CI environment instability rather than a PR-induced regression. Recommendations
Evidence
|
|
/retest-required |
1 similar comment
|
/retest-required |
|
/uncc @bryan-cox |
|
/retest |
|
@mihivagyok: The 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. |
|
/verified by @mihivagyok Environment: IBM Cloud, Openshift 5 With the change: -> masterURL is using in-cluster service (before it was equal with masterPublicURL) -> oauth-openshift deployment's NO_PROXY variable is extended with the internal service URL Copy login command and request token is successful without any external connectivity requirement: 
Thanks! |
|
@mihivagyok: Jira verification commands are restricted to collaborators for this repo. 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. |
|
/verified by @mihivagyok |
openshift-ci-robot
added
the
verified
|
@TwoDCube: This PR has been marked as verified by 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. |
|
/retest-required |
|
@mihivagyok: all tests passed! 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. |
|
@mihivagyok: Jira Issue Verification Checks: Jira Issue OCPBUGS-88312 Jira Issue OCPBUGS-88312 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
Fix included in release 5.0.0-0.nightly-2026-07-21-023721 |
ModernTLS fixtures from openshift#8871 landed stale due to Tide merge skew. GHA tested against a base that predated openshift#8772, openshift#8940, openshift#8971, openshift#8705 which changed oauth masterURL, wait-for-etcd init container, router ordering, and etcd job label regex respectively. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>
ModernTLS fixtures from openshift#8871 landed stale due to Tide merge skew. GHA tested against a base that predated openshift#8772, openshift#8940, openshift#8971, openshift#8705 which changed oauth masterURL, wait-for-etcd init container, router ordering, and etcd job label regex respectively. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>
ModernTLS fixtures from openshift#8871 landed stale due to Tide merge skew. GHA tested against a base that predated openshift#8772, openshift#8940, openshift#8971, openshift#8705 which changed oauth masterURL, wait-for-etcd init container, router ordering, and etcd job label regex respectively. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>
8 participants
What this PR does / why we need it:
When a user clicks “Display Token”, the oauth-openshift service attempts to call itself using the cluster’s API endpoint.
This self-call fails depending on whether the oauth-server is configured a public or private service URL as MasterURL.
In case of a public URL and control plane outbound connecitivty is restricted, the call fails.
In case a private URL, it can happen that the DNS is also a private one, and DNS resolution fails.
Which issue(s) this PR fixes:
Fixes OCPBUGS-88312
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
oauth-openshiftservice when the external endpoint is DNS-based.NO_PROXYhandling to ensure required internal endpoints are reliably bypassed, including IBM Cloud-specific exceptions.