feat(api): add OAuthNoProxyEndpoints field for IBMCloud platform#8580
feat(api): add OAuthNoProxyEndpoints field for IBMCloud platform#8580pamelachristie wants to merge 1 commit into
Conversation
Add configurable OAuthNoProxyEndpoints field to IBMCloudPlatformSpec to allow specifying additional endpoints for OAuth no-proxy configuration. This enables platform-specific customization of OAuth proxy bypass rules for IBM Cloud hosted clusters.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThis pull request extends the IBM Cloud platform specification with a new optional 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
openshift-ci
Bot
added
area/api
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pamelachristie 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/hypershift/v1beta1/hostedcluster_types.go (1)
1366-1374: ⚡ Quick winStrengthen the API contract with endpoint-shape validation.
The new field only enforces count and length; invalid values (e.g., URL-with-scheme, whitespace-only, malformed hosts) can still pass and silently produce ineffective
NO_PROXYentries. Add item-level validation (CEL/pattern) for supported endpoint formats.As per coding guidelines, "Design APIs contract-first" and "Prefer admission-time CEL validation over controller-time validation; use CEL rules, not webhooks, for new validation."
🤖 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 `@api/hypershift/v1beta1/hostedcluster_types.go` around lines 1366 - 1374, The OAuthNoProxyEndpoints slice allows invalid endpoint shapes; add item-level validation to reject schemes, whitespace-only strings, and malformed hosts by adding a kubebuilder items-pattern and a CEL rule that each element of OAuthNoProxyEndpoints matches a host or host:port pattern (no scheme, no surrounding whitespace, valid hostname/IP or hostname:port), e.g. add a +kubebuilder:validation:items:Pattern annotation on OAuthNoProxyEndpoints and a corresponding CRD CEL validation expression referencing OAuthNoProxyEndpoints to enforce the same regex per item so invalid entries are rejected at admission time.
🤖 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 `@api/hypershift/v1beta1/hostedcluster_types.go`:
- Around line 1366-1374: The API change adding the OAuthNoProxyEndpoints field
in hostedcluster_types.go lacks the required API linter output; run make
api-lint-fix and commit the fixes it produces, then include the make
api-lint-fix output in the PR description; specifically ensure the kubebuilder
validation annotations on the OAuthNoProxyEndpoints slice (OAuthNoProxyEndpoints
[]string) conform to the linter suggestions and any generated CRD or deepcopy
changes are staged and pushed with the PR.
In `@control-plane-operator/controllers/hostedcontrolplane/oauth/params.go`:
- Around line 97-103: Add unit tests for NewOAuthServerParams to verify IBMCloud
OAuth NO_PROXY merge semantics: (1) when Platform.Type == hyperv1.IBMCloud and
IBMCloud is nil, OAuthServerParams.OAuthNoProxy contains only the default
entries "iam.cloud.ibm.com" and "iam.test.cloud.ibm.com"; (2) when IBMCloud is
non-nil but OAuthNoProxyEndpoints is empty, OAuthNoProxy still contains only the
defaults; and (3) when IBMCloud.OAuthNoProxyEndpoints contains values,
OAuthNoProxy contains the defaults plus those endpoints in the merged slice.
Construct HostedControlPlane objects with Platform.Type == hyperv1.IBMCloud and
varying IBMCloud/OAuthNoProxyEndpoints values, call NewOAuthServerParams, and
assert OAuthServerParams.OAuthNoProxy equals the expected slice (including order
if relevant).
---
Nitpick comments:
In `@api/hypershift/v1beta1/hostedcluster_types.go`:
- Around line 1366-1374: The OAuthNoProxyEndpoints slice allows invalid endpoint
shapes; add item-level validation to reject schemes, whitespace-only strings,
and malformed hosts by adding a kubebuilder items-pattern and a CEL rule that
each element of OAuthNoProxyEndpoints matches a host or host:port pattern (no
scheme, no surrounding whitespace, valid hostname/IP or hostname:port), e.g. add
a +kubebuilder:validation:items:Pattern annotation on OAuthNoProxyEndpoints and
a corresponding CRD CEL validation expression referencing OAuthNoProxyEndpoints
to enforce the same regex per item so invalid entries are rejected at admission
time.
🪄 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: 9c5a198e-2d55-4ff2-9030-fa3f505ed02f
⛔ Files ignored due to path filters (44)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/ibmcloudplatformspec.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (3)
api/hypershift/v1beta1/hostedcluster_types.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/params.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go
| // oauthNoProxyEndpoints is a list of endpoints that should be added to the OAuth | ||
| // no_proxy configuration. | ||
| // +optional | ||
| // +listType=set | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +kubebuilder:validation:MaxItems=50 | ||
| // +kubebuilder:validation:items:MinLength=1 | ||
| // +kubebuilder:validation:items:MaxLength=253 | ||
| OAuthNoProxyEndpoints []string `json:"oauthNoProxyEndpoints,omitempty"` |
There was a problem hiding this comment.
API linter findings are missing for this API change.
Please run make api-lint-fix and include its output in the PR review context before merge.
As per coding guidelines, "**/api/**: Run make api-lint-fix before writing any review and include its output in the review. The linter is the authoritative source for convention violations. Review must start with linter findings, then add your own analysis on top."
🤖 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 `@api/hypershift/v1beta1/hostedcluster_types.go` around lines 1366 - 1374, The
API change adding the OAuthNoProxyEndpoints field in hostedcluster_types.go
lacks the required API linter output; run make api-lint-fix and commit the fixes
it produces, then include the make api-lint-fix output in the PR description;
specifically ensure the kubebuilder validation annotations on the
OAuthNoProxyEndpoints slice (OAuthNoProxyEndpoints []string) conform to the
linter suggestions and any generated CRD or deepcopy changes are staged and
pushed with the PR.
| if hcp.Spec.Platform.Type == hyperv1.IBMCloudPlatform { | ||
| // Add default IBM Cloud endpoints for backward compatibility | ||
| p.OAuthNoProxy = append(p.OAuthNoProxy, "iam.cloud.ibm.com", "iam.test.cloud.ibm.com") | ||
| // Add any additional endpoints specified in the platform spec | ||
| if hcp.Spec.Platform.IBMCloud != nil && len(hcp.Spec.Platform.IBMCloud.OAuthNoProxyEndpoints) > 0 { | ||
| p.OAuthNoProxy = append(p.OAuthNoProxy, hcp.Spec.Platform.IBMCloud.OAuthNoProxyEndpoints...) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether tests already cover the new behavior.
rg -n --type=go -g '*_test.go' \
'OAuthNoProxyEndpoints|NewOAuthServerParams|NO_PROXY|iam\.cloud\.ibm\.com|iam\.test\.cloud\.ibm\.com|IBMCloudPlatform'Repository: openshift/hypershift
Length of output: 16241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate the function and relevant symbols in the target file
echo "== params.go excerpt for context =="
sed -n '1,200p' control-plane-operator/controllers/hostedcontrolplane/oauth/params.go | nl -ba | sed -n '70,140p'
# 2) Search for tests or usages in the oauth package
echo "== Search for OAuthNoProxy* in oauth package tests =="
rg -n --type=go \
'OAuthNoProxyEndpoints|OAuthNoProxy|NewOAuthServerParams|OAuthNoProxyEndpoints' \
control-plane-operator/controllers/hostedcontrolplane/oauth -S
echo "== Search for NO_PROXY env merge logic specifically tied to OAuth in oauth tests =="
rg -n --type=go \
'NO_PROXY|no_proxy|OAuth' \
control-plane-operator/controllers/hostedcontrolplane/oauth/* -S
# 3) Find any *_test.go in the oauth directory and list them
echo "== *_test.go files in oauth directory =="
fd -e go --changed-within 99999d '*_test.go' control-plane-operator/controllers/hostedcontrolplane/oauth -a | catRepository: openshift/hypershift
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print params.go with line numbers (no nl available)
echo "== params.go excerpt for context =="
awk 'NR>=70 && NR<=140 {printf "%d\t%s\n", NR, $0}' control-plane-operator/controllers/hostedcontrolplane/oauth/params.go
echo
echo "== *_test.go files in oauth directory =="
fd -e go --changed-within 99999d '*_test.go' control-plane-operator/controllers/hostedcontrolplane/oauth -a | sort
echo
echo "== Search for OAuthNoProxy* in oauth package =="
rg -n --type=go \
'OAuthNoProxyEndpoints|OAuthNoProxy|NewOAuthServerParams' \
control-plane-operator/controllers/hostedcontrolplane/oauth -S
echo
echo "== Search for IBMCloud OAuthNoProxy / iam.cloud.ibm.com / OAuthNoProxyEndpoints in oauth package =="
rg -n --type=go \
'IBMCloud|iam\.cloud\.ibm\.com|iam\.test\.cloud\.ibm\.com|OAuthNoProxyEndpoints|OAuthNoProxy' \
control-plane-operator/controllers/hostedcontrolplane/oauth -SRepository: openshift/hypershift
Length of output: 2621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== List *_test.go under oauth directory =="
fd --type f '*_test.go' control-plane-operator/controllers/hostedcontrolplane/oauth | sort || true
echo
echo "== Search for NewOAuthServerParams in repo =="
rg -n --type=go 'func\s+NewOAuthServerParams|NewOAuthServerParams' . || true
echo
echo "== Search for OAuthNoProxy in repo =="
rg -n --type=go 'OAuthNoProxy' . || true
echo
echo "== Search for iam.cloud.ibm.com in repo (go/tests) =="
rg -n --type=go 'iam\.cloud\.ibm\.com|iam\.test\.cloud\.ibm\.com' . || trueRepository: openshift/hypershift
Length of output: 7000
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== *_test.go under control-plane-operator/controllers/hostedcontrolplane/oauth =="
fd --type f --glob '*_test.go' control-plane-operator/controllers/hostedcontrolplane/oauth | sort
echo
echo "== Search in oauth tests for IBMCloud NO_PROXY / iam.cloud.ibm.com =="
rg -n --type=go 'iam\.cloud\.ibm\.com|iam\.test\.cloud\.ibm\.com|NO_PROXY|OAuthNoProxyEndpoints|OAuthNoProxy' \
control-plane-operator/controllers/hostedcontrolplane/oauth -S || true
echo
echo "== Search for NewOAuthServerParams usage in oauth tests =="
rg -n --type=go 'NewOAuthServerParams' \
control-plane-operator/controllers/hostedcontrolplane/oauth -S || true
echo
echo "== If service_test.go exists, show relevant sections (first 220 lines) =="
if [ -f control-plane-operator/controllers/hostedcontrolplane/oauth/service_test.go ]; then
sed -n '1,220p' control-plane-operator/controllers/hostedcontrolplane/oauth/service_test.go | nl -ba 2>/dev/null || sed -n '1,220p' control-plane-operator/controllers/hostedcontrolplane/oauth/service_test.go
fi
echo
echo "== Show more targeted region around any IBMCloud/NO_PROXY assertions (search within file) =="
if [ -f control-plane-operator/controllers/hostedcontrolplane/oauth/service_test.go ]; then
rg -n 'IBMCloud|NO_PROXY|iam\.cloud\.ibm\.com|iam\.test\.cloud\.ibm\.com|OAuthNoProxy' control-plane-operator/controllers/hostedcontrolplane/oauth/service_test.go || true
fiRepository: openshift/hypershift
Length of output: 9071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== *_test.go containing iam.cloud.ibm.com =="
rg -n --type=go -g '*_test.go' 'iam\.cloud\.ibm\.com' . || true
echo
echo "== *_test.go containing iam.test.cloud.ibm.com =="
rg -n --type=go -g '*_test.go' 'iam\.test\.cloud\.ibm\.com' . || true
echo
echo "== *_test.go containing OAuthNoProxyEndpoints =="
rg -n --type=go -g '*_test.go' 'OAuthNoProxyEndpoints' . || true
echo
echo "== *_test.go containing OAuthNoProxy merge mentions =="
rg -n --type=go -g '*_test.go' 'OAuthNoProxy' control-plane-operator . || true
echo
echo "== *_test.go containing 'iam.cloud' within control-plane-operator =="
rg -n --type=go -g '*_test.go' 'iam\.cloud\.ibm\.com|iam\.test\.cloud\.ibm\.com' control-plane-operator . || trueRepository: openshift/hypershift
Length of output: 504
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== *_test.go under control-plane-operator/controllers/hostedcontrolplane/v2/oauth =="
fd --type f --glob '*_test.go' control-plane-operator/controllers/hostedcontrolplane/v2/oauth | sort || true
echo
echo "== Search v2/oauth tests for NO_PROXY / IBMCloudPlatform / OAuthNoProxyEndpoints =="
rg -n --type=go \
'NO_PROXY|IBMCloudPlatform|OAuthNoProxyEndpoints|OAuthNoProxy|iam\.cloud\.ibm\.com|iam\.test\.cloud\.ibm\.com' \
control-plane-operator/controllers/hostedcontrolplane/v2/oauth -S || true
echo
echo "== Search v2/oauth_apiserver tests for IBMCloudPlatform + NO_PROXY assembly =="
fd --type f --glob '*_test.go' control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver | sort || true
rg -n --type=go \
'NO_PROXY|IBMCloudPlatform|iam\.cloud\.ibm\.com|iam\.test\.cloud\.ibm\.com|OAuthNoProxyEndpoints' \
control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver -S || trueRepository: openshift/hypershift
Length of output: 2368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search in v2/oauth_apiserver tests for IBMCloud default endpoints =="
rg -n --type=go \
'iam\.cloud\.ibm\.com|iam\.test\.cloud\.ibm\.com|OAuthNoProxyEndpoints' \
control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/*_test.go -S || true
echo
echo "== Search in v2/oauth_apiserver deployment_test.go for platform IBMCloud ===="
rg -n --type=go 'IBMCloudPlatform' control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go -S || true
echo
echo "== Show relevant NO_PROXY assertion blocks in v2/oauth_apiserver/deployment_test.go (around matches) =="
rg -n --type=go 'NO_PROXY' control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go -n -S
rg -n --type=go 'iam\.cloud\.ibm\.com|iam\.test\.cloud\.ibm\.com' control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go -S || true
# Print a small neighborhood around the first NO_PROXY assertion occurrences
echo
echo "== Neighborhood for first 2 NO_PROXY matches =="
rg -n --type=go 'FindEnvVar\("NO_PROXY"|Check NO_PROXY|NO_PROXY' control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go -S | head -n 2 | cut -d: -f1 | while read -r line; do
echo "--- around line $line ---"
sed -n "$((line-20)),$((line+20))p" control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment_test.go
doneRepository: openshift/hypershift
Length of output: 3640
Add unit tests for IBMCloud OAuth NO_PROXY endpoint merge behavior (defaults + optional OAuthNoProxyEndpoints). (control-plane-operator/controllers/hostedcontrolplane/oauth/params.go:97-103)
The IBMCloud branch always adds defaults (iam.cloud.ibm.com, iam.test.cloud.ibm.com) and then conditionally appends hcp.Spec.Platform.IBMCloud.OAuthNoProxyEndpoints only when IBMCloud != nil. No existing unit tests cover this merge semantics (defaults-only, defaults+custom endpoints, and nil IBMCloud spec). Add/extend tests for NewOAuthServerParams/OAuthServerParams.OAuthNoProxy.
🤖 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 97 - 103, Add unit tests for NewOAuthServerParams to verify IBMCloud OAuth
NO_PROXY merge semantics: (1) when Platform.Type == hyperv1.IBMCloud and
IBMCloud is nil, OAuthServerParams.OAuthNoProxy contains only the default
entries "iam.cloud.ibm.com" and "iam.test.cloud.ibm.com"; (2) when IBMCloud is
non-nil but OAuthNoProxyEndpoints is empty, OAuthNoProxy still contains only the
defaults; and (3) when IBMCloud.OAuthNoProxyEndpoints contains values,
OAuthNoProxy contains the defaults plus those endpoints in the merged slice.
Construct HostedControlPlane objects with Platform.Type == hyperv1.IBMCloud and
varying IBMCloud/OAuthNoProxyEndpoints values, call NewOAuthServerParams, and
assert OAuthServerParams.OAuthNoProxy equals the expected slice (including order
if relevant).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8580 +/- ##
==========================================
- Coverage 40.41% 40.40% -0.01%
==========================================
Files 755 755
Lines 93235 93243 +8
==========================================
Hits 37679 37679
- Misses 52854 52862 +8
Partials 2702 2702
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
I already have all the information I need from the diff, Codecov report, and PR comments. Let me now compile the complete analysis. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe root cause is missing unit tests for new runtime logic. Specifically:
Both files had 0% coverage before this PR (the The Recommendations
Evidence
|
1 participant
What this PR does / why we need it:
This PR adds a new optional field
oauthNoProxyEndpointsto theIBMCloudPlatformSpecAPI to allow users to specify additional endpoints that should be added to the OAuth server'sNO_PROXYconfiguration.The change extends the existing IBM Cloud platform configuration by:
This allows IBM Cloud users to configure additional no-proxy endpoints for their OAuth server without having to override the defaults, which is particularly useful in environments with custom IAM endpoints or private network configurations.
This PR is based on the original work in #8241 and discussion with Caesar.
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
iam.cloud.ibm.com,iam.test.cloud.ibm.com) and only appending user-specified endpointslistType=setto prevent duplicates**out = **in) to deep copy (DeepCopyInto) due to the new slice fieldChecklist:
Summary by CodeRabbit