OTA-1546: Skip an accept-risk case on network-restricted environment#1332
OTA-1546: Skip an accept-risk case on network-restricted environment#1332openshift-merge-bot[bot] merged 2 commits intoopenshift:mainfrom
Conversation
|
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:
WalkthroughThis PR adds network-awareness utilities to skip tests when network access is restricted, applies these utilities to an existing test, and adds three indirect dependencies to support the new functionality. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
openshift-ci
bot
added
the
approved
![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)
test/util/util.go (1)
140-151: Consider idiomatic Go pattern for HTTP response handling.The defer with nil checks works but the conventional Go pattern is to check the error first, then defer the close. This improves readability.
♻️ Suggested idiomatic pattern
func accessible(url string) bool { client := &http.Client{ Timeout: 5 * time.Second, } resp, err := client.Get(url) - defer func() { - if resp != nil && resp.Body != nil { - _ = resp.Body.Close() - } - }() - return err == nil + if err != nil { + return false + } + defer resp.Body.Close() + return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/util/util.go` around lines 140 - 151, The accessible function currently defers closing resp.Body before checking the error; change it to call client.Get(url), check if err != nil and return false immediately, then defer closing resp.Body (resp.Body.Close()) and finally return true. Update the function accessible to follow the idiomatic pattern: perform resp, err := client.Get(url); if err != nil { return false }; defer resp.Body.Close(); return true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/cvo/accept_risks.go`:
- Around line 65-66: Fix the typo in the comment ("replies" → "relies") and
replace the generic network check util.SkipIfNetworkRestricted() with a precise
URL accessibility check for the actual endpoint by adding a helper like
util.SkipIfURLNotAccessible(url string) (or util.SkipIfFauxinnatiUnavailable)
that attempts to reach util.FauxinnatiAPIURL and calls g.Skipf when unreachable;
then update the test in accept_risks.go to call
util.SkipIfURLNotAccessible(util.FauxinnatiAPIURL) instead of
util.SkipIfNetworkRestricted() so the skip reflects the real dependency.
In `@test/util/util.go`:
- Around line 153-156: Fix the typo in the comment inside the NetworkRestricted
function: change "mothed" to "method" in the TODO comment above
NetworkRestricted() (the function name and accessible() call are present in the
snippet) so the comment reads "TODO: Use a more precise method". Ensure only the
comment text is updated and no code logic is changed.
---
Nitpick comments:
In `@test/util/util.go`:
- Around line 140-151: The accessible function currently defers closing
resp.Body before checking the error; change it to call client.Get(url), check if
err != nil and return false immediately, then defer closing resp.Body
(resp.Body.Close()) and finally return true. Update the function accessible to
follow the idiomatic pattern: perform resp, err := client.Get(url); if err !=
nil { return false }; defer resp.Body.Close(); return true.
ℹ️ Review info
Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 623c1887-ee86-400b-969c-f47fc8c1f880
📒 Files selected for processing (2)
test/cvo/accept_risks.gotest/util/util.go
test/util/util.go
Outdated
|
|
||
| func NetworkRestricted() bool { | ||
| // TODO: Use a more precise mothed | ||
| return !accessible("http://google.com") |
There was a problem hiding this comment.
oc does a similar thing.
https://github.com/openshift/oc/blob/2bd770b8f00d1f6b6a49afe8c5ee411665f2e308/test/e2e/util.go#L1008
|
/retest-required |
test/util/util.go
Outdated
|
|
||
| func NetworkRestricted() bool { | ||
| // TODO: Use a more precise method | ||
| return !accessible("http://google.com") |
There was a problem hiding this comment.
you could take an argument here, and pass through FauxinnatiAPIURL for the should work with accept risks case. If you get a HTTP response, it's accessible. If you get a network-connection error, it's not accessible. But that would test the exact endpoint you need to reach, and avoid having to hard-code something here.
There was a problem hiding this comment.
I did most of it in a76bd9c
The only exception is "If you get a network-connection error, it's not accessible."
I did not check the error, any (non-nil) error leads to "not accessible". A bit stricter than the suggestion.
There was a problem hiding this comment.
I tested with
package util
import (
"context"
"os"
"strings"
"testing"
)
func Test_NetworkRestricted(t *testing.T) {
if strings.ToLower(os.Getenv("CI")) == "true" {
t.Skip("Skipping test in CI.")
}
c, err := GetRestConfig()
if err != nil {
t.Fatal(err)
}
networkRestricted := NetworkRestricted(context.TODO(), c, FauxinnatiAPIURL, "https://quay.io")
if networkRestricted {
t.Errorf("network is restricted")
}
}And it works.
If I do "https://quay.io" -> "https://quxxxay.io", it will return false.
wking
left a comment
wking
left a comment
There was a problem hiding this comment.
/lgtm
/hold in case you want to refactor the connectivity check
I left a suggestion, but I'm fine if you want to lift this hold and get back to that suggestion in a follow up pull or never, as you see fit.
openshift-ci
bot
added
the
do-not-merge/hold
openshift-ci
bot
added
lgtm
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/util/util.go`:
- Around line 142-153: The accessible function currently probes URLs from the
test runner process (using http.Client and client.Get), which can differ from
the cluster's network plane; replace this logic so the reachability check runs
from inside the target cluster (for example by creating or exec-ing into a pod
and performing an HTTP GET there) instead of using client.Get in accessible;
update the accessible function to accept or obtain the cluster/kube
client/context used elsewhere in tests, run an in-cluster probe (e.g., curl/wget
or Go exec over the Kubernetes API) and return success based on the in-cluster
probe result, ensuring you close any exec streams/pod resources just like the
current resp.Body cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 799e2da8-a72c-44a8-8a53-9c4598d54f38
📒 Files selected for processing (2)
test/cvo/accept_risks.gotest/util/util.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/util/util.go (1)
156-159: Consider including the inaccessible URL(s) in the skip message.The skip message is generic. Including which URL(s) failed would help with debugging test skips in CI logs.
📝 Proposed improvement
-func SkipIfNetworkRestricted(oc api.OC, urls ...string) { - if NetworkRestricted(oc, urls...) { - g.Skip("This test is skipped because the network is restricted") - } -} +func SkipIfNetworkRestricted(oc api.OC, urls ...string) { + if NetworkRestricted(oc, urls...) { + g.Skip(fmt.Sprintf("This test is skipped because the network is restricted (cannot reach: %v)", urls)) + } +}Note: This would require adding
"fmt"to imports if not already present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/util/util.go` around lines 156 - 159, Update SkipIfNetworkRestricted to include the failing URL(s) in the skip message: call NetworkRestricted(oc, urls...) as before but when it returns true, pass a formatted message to g.Skip that contains the list of URLs (e.g., using fmt.Sprintf or strings.Join) so the CI logs show which URL(s) triggered the skip; add "fmt" to imports if not present and reference the existing functions SkipIfNetworkRestricted, NetworkRestricted and g.Skip when making the change.test/oc/api/api.go (1)
25-25: Consider renamingEXECtoExecfor Go naming conventions.Go convention uses PascalCase for exported identifiers (e.g.,
Exec), not all-caps. All-caps is typically reserved for acronyms likeHTTPorURL. This is a minor style nit but worth considering for consistency.📝 Proposed fix
- EXEC(namespace, pod, container string, command string, commandArgs ...string) (string, error) + Exec(namespace, pod, container string, command string, commandArgs ...string) (string, error)Note: This would require updating the implementation in
cli.goand all call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/oc/api/api.go` at line 25, The exported interface method name EXEC violates Go naming conventions; rename it to Exec across the codebase: update the declaration in test/oc/api/api.go (change EXEC to Exec), update the implementation in cli.go to match the new method name, and update all call sites and any interface implementations to use Exec instead of EXEC so the code compiles and follows PascalCase export conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/oc/api/api.go`:
- Line 25: The exported interface method name EXEC violates Go naming
conventions; rename it to Exec across the codebase: update the declaration in
test/oc/api/api.go (change EXEC to Exec), update the implementation in cli.go to
match the new method name, and update all call sites and any interface
implementations to use Exec instead of EXEC so the code compiles and follows
PascalCase export conventions.
In `@test/util/util.go`:
- Around line 156-159: Update SkipIfNetworkRestricted to include the failing
URL(s) in the skip message: call NetworkRestricted(oc, urls...) as before but
when it returns true, pass a formatted message to g.Skip that contains the list
of URLs (e.g., using fmt.Sprintf or strings.Join) so the CI logs show which
URL(s) triggered the skip; add "fmt" to imports if not present and reference the
existing functions SkipIfNetworkRestricted, NetworkRestricted and g.Skip when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2459b05-c70d-43e4-a793-e73ce66a29e3
📒 Files selected for processing (4)
test/cvo/accept_risks.gotest/oc/api/api.gotest/oc/cli/cli.gotest/util/util.go
openshift-merge-robot
added
the
needs-rebase
hongkailiu
force-pushed
the
guard-more
branch
2 times, most recently
from
2b3a911 to
4747228
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/util/util.go`:
- Around line 192-199: NetworkRestricted currently treats any error from
accessible as a network restriction; change it to only return true for a
confirmed "unreachable" result and propagate other errors to the caller. Update
accessible to distinguish unreachable vs setup/exec/RBAC errors (e.g., return
(bool, error) or a typed error/kind), have NetworkRestricted interpret that
result and only return true when the result indicates unreachable, otherwise
return false and surface the error. Then update SkipIfNetworkRestricted to call
accessible (or NetworkRestricted's new behavior) and call g.Expect / fail the
test on unexpected errors instead of treating them as network-restricted skips;
only call g.Skip when a confirmed unreachable result is returned.
- Around line 158-183: The curl probe only limits TCP connect time; update the
probe in accessible() so it also has an overall timeout and respects the passed
context: append a "--max-time" (e.g. "10") to the command slice (alongside
"--connect-timeout") and attach the provided ctx to the REST request by calling
req = req.Context(ctx) before creating the SPDY executor
(remotecommand.NewSPDYExecutor) so the exec respects test-level cancellation and
fails fast instead of hanging in exec.Stream.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7779b5de-7085-4967-a1d8-08970f1044ba
⛔ Files ignored due to path filters (35)
go.sumis excluded by!**/*.sumvendor/github.com/gorilla/websocket/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/compression.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/conn.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/join.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/mask.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/mask_safe.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/prepared.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/proxy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/MAINTAINERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/NOTICEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/connection.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/handlers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/priority.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/dictionary.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/read.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/write.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/stream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/flowrate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/io.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/util.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (3)
go.modtest/cvo/accept_risks.gotest/util/util.go
hongkailiu
force-pushed
the
guard-more
branch
2 times, most recently
from
e7a3380 to
4892fdd
Compare
|
The case passed (not skipped) in the job: $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-version-operator/1332/pull-ci-openshift-cluster-version-operator-main-e2e-agnostic-ovn-techpreview-serial-1of3/2031830392939483136/artifacts/e2e-agnostic-ovn-techpreview-serial/openshift-e2e-test/artifacts/e2e.log | grep risk
started: 0/1/45 "[Jira:\"Cluster Version Operator\"] cluster-version-operator should work with accept risks"
passed: (3m31s) 2026-03-11T22:42:14 "[Jira:\"Cluster Version Operator\"] cluster-version-operator should work with accept risks" |
|
/retest-required |
|
@hongkailiu: 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. |
| return false, fmt.Errorf("could not find CVO pod: %w", err) | ||
| } | ||
| podName := pods.Items[0].Name | ||
| command := []string{"curl", "-sI", "--max-time", "10"} |
There was a problem hiding this comment.
Note to myself, -I's HEAD will result in a Method Not Allowed 405:
$ oc --as system:admin -n openshift-cluster-version exec deployment/cluster-version-operator -- curl -sI --max-time 10 https://fauxinnati-fauxinnati.apps.ota-stage.q2z4.p1.openshiftapps.com/api/upgrades_info/graph
HTTP/2 405
content-type: text/plain; charset=utf-8
x-content-type-options: nosniff
date: Thu, 12 Mar 2026 20:19:16 GMT
content-length: 19
set-cookie: 39f437928e5cb0affa7deaa4b203b6b2=279efadf6fbb7c1d7c35c6792ac611ed; path=/; HttpOnly; Secure; SameSite=None
$ echo $?
0cURL returning 0 for the 405 is because we aren't setting --fail:
$ man curl | grep -A4 ' -f, --fail' | head -n5
-f, --fail
(HTTP) Fail with error code 22 and with no response body output at all for HTTP transfers returning HTTP response codes at 400 or greater.
In normal cases when an HTTP server fails to deliver a document, it returns a body of text stating so (which often also describes why and more) and a 4xx HTTP response code. This command line op‐
tion prevents curl from outputting that data and instead returns error 22 early. By default, curl does not consider HTTP response codes to indicate failure.For comparison, if we request an unreachable URI, we do get an exit code:
$ oc --as system:admin -n openshift-cluster-version exec deployment/cluster-version-operator -- curl -sI --max-time 10 https://unreachable.example.com/api/upgrades_info/graph
command terminated with exit code 6
$ echo $?
6
$ man curl | grep '^ *6 '
6 Could not resolve host. The given remote host could not be resolved.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu, wking 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 |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest-requied |
|
/retest-required |
|
See #1332 (comment) /verified by @hongkailiu The pull skips a case if some condition holds. The risk is low. |
openshift-ci-robot
added
the
verified
|
@hongkailiu: 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. |
hongkailiu
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@hongkailiu: This pull request references OTA-1546 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 "4.22.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. |
4 participants
Summary by CodeRabbit
Tests
Chores