WINC-1967: OTE Migration Batch 2 - Monitoring & Observability#4319
WINC-1967: OTE Migration Batch 2 - Monitoring & Observability#4319rrasouli wants to merge 2 commits into
Conversation
Replace manual-registration skeleton with Ginkgo-based OTE approach that uses BuildExtensionTestSpecsFromOpenShiftGinkgoSuite() for automatic test discovery. Migrate 5 Smokerun tests from OTP: OCP-33612, OCP-32615, OCP-32554, OCP-37362, OCP-38188.
openshift-ci-robot
added
the
jira/valid-reference
|
@rrasouli: This pull request references WINC-1967 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe OTE command is replaced with an automatically discovered OpenShift Ginkgo extension launcher and updated suite registration. Module dependencies and replacements are expanded. New Windows e2e utilities provide cluster introspection, parsing, polling, metrics, and provider-ID helpers. A new Windows test suite validates nodes, secrets, networking, versions, metrics, alert rules, containerd reporting, HTTPS enforcement, and provider IDs. Legacy CLI and cluster helpers are removed, and OTE ownership assignments are updated. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 6 warnings)
✅ Passed checks (13 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 |
openshift-ci
Bot
added
the
approved
|
/approve cancel |
|
[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 |
openshift-ci
Bot
removed
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: 8
🧹 Nitpick comments (1)
ote/test/e2e/utils.go (1)
268-305: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
wait.Pollis deprecated.
wait.Poll(line 274) is deprecated in favor of context-aware polling (wait.PollUntilContextTimeout) and is documented as "Will be removed in a future release" ink8s.io/apimachinery. Sinceapimachineryis already pinned at v0.35.1 ingo.mod, consider migrating now to avoid a forced rewrite later.🤖 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 `@ote/test/e2e/utils.go` around lines 268 - 305, The waitUntilWMCOStatusChanged function still uses deprecated wait.Poll; migrate this polling call to wait.PollUntilContextTimeout using an appropriate context and the existing poll interval and timeout, while preserving the current log retrieval, retry behavior, and final assertion.
🤖 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 `@ote/cmd/wmco-tests-ext/main.go`:
- Around line 34-35: Ensure the error paths in main, including the additional
occurrence around the referenced later lines, flush logs before calling
os.Exit(1). Update the control flow around logs.FlushLogs and os.Exit so
deferred cleanup is not relied upon when terminating the process.
In `@ote/test/e2e/utils.go`:
- Around line 219-226: Update the error construction in the jq execution flow
around cmd.Output() to wrap err with %w instead of %v, preserving
errors.Is/errors.As unwrapping while retaining the existing resourceType context
and logging.
- Around line 173-184: Update getValueFromText to handle the case where no line
contains searchVal before indexing the split result. Return a safe empty value
(or otherwise preserve the caller’s expected clean-failure behavior) when
searchVal is absent, while retaining the existing trimmed substring behavior
when it is found.
In `@ote/test/e2e/winc.go`:
- Around line 311-317: Replace the assertion on len(winInternalIPs) in the
“Verifying HTTP is not allowed on metrics endpoint” test with a conditional skip
when fewer than two Windows internal IPs are available. Keep the cross-node curl
and response validation unchanged for clusters with at least two nodes.
- Around line 311-317: Update the metrics endpoint URL construction in the HTTP
rejection test to use IPv6-safe host/port joining via the existing
net.JoinHostPort pattern, preserving the winInternalIPs[1] address, port 9182,
and /metrics path.
- Line 193: The call sites invoking execInPod in the relevant test cases discard
execution errors; capture and log the returned error before evaluating the
command output. Update both the curl check and the other execInPod call noted by
the review, preserving their existing stdout assertions while ensuring transport
or execution failures are visible.
- Around line 262-285: Remove the public GitHub request from the
“Smokerun-Author:rrasouli-Medium-60814-Check containerd version is properly
reported” test and source the expected containerd version from an already
available in-cluster WMCO artifact via the existing oc-based helpers. Preserve
the comparison against getContainerdVersion for each Windows host, and ensure
any retained HTTP request uses a bounded timeout and response cleanup.
- Around line 113-117: Replace the shell-based file read in the smokerun test
with direct os.ReadFile(publicKey), adding the os import as needed. Preserve the
existing error assertion and public-key parsing behavior after reading the file.
---
Nitpick comments:
In `@ote/test/e2e/utils.go`:
- Around line 268-305: The waitUntilWMCOStatusChanged function still uses
deprecated wait.Poll; migrate this polling call to wait.PollUntilContextTimeout
using an appropriate context and the existing poll interval and timeout, while
preserving the current log retrieval, retry behavior, and final assertion.
🪄 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: cfd8a973-dd13-441c-acd4-2059efa5ed34
⛔ Files ignored due to path filters (1)
ote/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
ote/OWNERSote/cmd/wmco-tests-ext/main.goote/go.modote/test/e2e/utils.goote/test/e2e/winc.goote/test/extended/cli/cli.goote/test/extended/cluster.go
💤 Files with no reviewable changes (2)
- ote/test/extended/cli/cli.go
- ote/test/extended/cluster.go
| logs.InitLogs() | ||
| defer logs.FlushLogs() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
os.Exit(1) skips the deferred logs.FlushLogs().
defer logs.FlushLogs() at line 35 never runs because os.Exit() bypasses all deferred calls — precisely on the error path where flushed logs are most needed for diagnosing a failed run.
🔧 Proposed fix
if err := func() error {
return root.Execute()
}(); err != nil {
+ logs.FlushLogs()
os.Exit(1)
}Also applies to: 89-93
🤖 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 `@ote/cmd/wmco-tests-ext/main.go` around lines 34 - 35, Ensure the error paths
in main, including the additional occurrence around the referenced later lines,
flush logs before calling os.Exit(1). Update the control flow around
logs.FlushLogs and os.Exit so deferred cleanup is not relied upon when
terminating the process.
| // getValueFromText finds a line containing searchVal and returns the text after the delimiter. | ||
| func getValueFromText(body []byte, searchVal string) string { | ||
| s := "" | ||
| lines := strings.Split(string(body), "\n") | ||
| for _, field := range lines { | ||
| if strings.Contains(field, searchVal) { | ||
| s = field | ||
| break | ||
| } | ||
| } | ||
| return strings.TrimSpace(strings.Split(s, searchVal)[1]) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Index-out-of-range panic when searchVal isn't found.
If no line contains searchVal, s stays "", and strings.Split(s, searchVal) returns [""] (length 1) — indexing [1] panics. This is reachable from winc.go line 279 (getValueFromText(body, "CONTAINERD_GIT_VERSION=")) whenever the fetched Makefile content doesn't contain that string, crashing the test instead of failing it cleanly.
🐛 Proposed fix
func getValueFromText(body []byte, searchVal string) string {
- s := ""
+ s := ""
lines := strings.Split(string(body), "\n")
for _, field := range lines {
if strings.Contains(field, searchVal) {
s = field
break
}
}
- return strings.TrimSpace(strings.Split(s, searchVal)[1])
+ if s == "" {
+ e2e.Logf("value for %q not found in text", searchVal)
+ return ""
+ }
+ return strings.TrimSpace(strings.Split(s, searchVal)[1])
}🤖 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 `@ote/test/e2e/utils.go` around lines 173 - 184, Update getValueFromText to
handle the case where no line contains searchVal before indexing the split
result. Return a safe empty value (or otherwise preserve the caller’s expected
clean-failure behavior) when searchVal is absent, while retaining the existing
trimmed substring behavior when it is found.
| cmd := exec.Command("jq", "-r", `.items[] | "\(.metadata.name)|\(.spec.providerID)"`) | ||
| cmd.Stdin = bytes.NewBufferString(jsonData) | ||
| jqOutput, err := cmd.Output() | ||
| if err != nil { | ||
| e2e.Logf("Error parsing jq output for %s: %v", resourceType, err) | ||
| return nil, fmt.Errorf("failed to parse jq output for %s: %v", resourceType, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Error wrapped with %v instead of %w.
Line 224 wraps err using %v, losing the ability to errors.Is/errors.As unwrap it, unlike the correct %w usage at line 99 in this same file.
🔧 Proposed fix
- return nil, fmt.Errorf("failed to parse jq output for %s: %v", resourceType, err)
+ return nil, fmt.Errorf("failed to parse jq output for %s: %w", resourceType, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cmd := exec.Command("jq", "-r", `.items[] | "\(.metadata.name)|\(.spec.providerID)"`) | |
| cmd.Stdin = bytes.NewBufferString(jsonData) | |
| jqOutput, err := cmd.Output() | |
| if err != nil { | |
| e2e.Logf("Error parsing jq output for %s: %v", resourceType, err) | |
| return nil, fmt.Errorf("failed to parse jq output for %s: %v", resourceType, err) | |
| } | |
| cmd := exec.Command("jq", "-r", `.items[] | "\(.metadata.name)|\(.spec.providerID)"`) | |
| cmd.Stdin = bytes.NewBufferString(jsonData) | |
| jqOutput, err := cmd.Output() | |
| if err != nil { | |
| e2e.Logf("Error parsing jq output for %s: %v", resourceType, err) | |
| return nil, fmt.Errorf("failed to parse jq output for %s: %w", resourceType, err) | |
| } |
🤖 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 `@ote/test/e2e/utils.go` around lines 219 - 226, Update the error construction
in the jq execution flow around cmd.Output() to wrap err with %w instead of %v,
preserving errors.Is/errors.As unwrapping while retaining the existing
resourceType context and logging.
Source: Coding guidelines
| g.It("Smokerun-Author:sgao-Critical-32615-Generate userData secret [Serial]", func() { | ||
| g.By("Check secret windows-user-data generated and contain correct public key") | ||
| output, err := exec.Command("bash", "-c", "cat "+publicKey+"").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| publicKeyContent := strings.Split(string(output), " ")[1] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Shell-out to read a local file — prefer os.ReadFile.
exec.Command("bash", "-c", "cat "+publicKey+"") shells out with a string built from a variable to do what os.ReadFile(publicKey) accomplishes directly, safely, and without a shell. publicKey currently comes from a trusted test-harness helper (compat_otp.GetPublicKey()), so exploitability is low today, but this pattern is exactly what the repo's injection-prevention guideline (no shell=True/backtick exec with user input) warns against, and it's trivial to avoid entirely.
🔧 Proposed fix
- output, err := exec.Command("bash", "-c", "cat "+publicKey+"").Output()
- o.Expect(err).NotTo(o.HaveOccurred())
- publicKeyContent := strings.Split(string(output), " ")[1]
+ output, err := os.ReadFile(publicKey)
+ o.Expect(err).NotTo(o.HaveOccurred())
+ publicKeyContent := strings.Split(string(output), " ")[1](requires adding "os" to imports)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| g.It("Smokerun-Author:sgao-Critical-32615-Generate userData secret [Serial]", func() { | |
| g.By("Check secret windows-user-data generated and contain correct public key") | |
| output, err := exec.Command("bash", "-c", "cat "+publicKey+"").Output() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| publicKeyContent := strings.Split(string(output), " ")[1] | |
| g.It("Smokerun-Author:sgao-Critical-32615-Generate userData secret [Serial]", func() { | |
| g.By("Check secret windows-user-data generated and contain correct public key") | |
| output, err := os.ReadFile(publicKey) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| publicKeyContent := strings.Split(string(output), " ")[1] |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 114-114: An argument passed to exec.Command/exec.CommandContext is built by concatenating a string literal with dynamic input. If that input is attacker-controlled (and especially when the command is a shell such as sh -c/bash -c), this enables OS command injection. Pass untrusted data as separate, fixed arguments instead of interpolating it into a command string, avoid invoking a shell, and validate/escape the input where a shell is unavoidable.
Context: exec.Command("bash", "-c", "cat "+publicKey+"")
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-concat-arg-go)
[error] 114-114: A shell (sh/bash) is invoked with -c and a dynamically built command string (string concatenation, fmt.Sprintf, or a variable holding the command) passed to exec.Command / exec.CommandContext. Untrusted input embedded in the command lets an attacker inject arbitrary shell commands. Avoid the shell: call the target binary directly with exec.Command(name, arg1, arg2, ...) so each argument is passed as a separate, non-interpreted token, and never build a shell command string from external input.
Context: exec.Command("bash", "-c", "cat "+publicKey+"")
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-sh-c-go)
🪛 OpenGrep (1.25.0)
[ERROR] 115-115: Dynamic command passed to exec.Command with a shell invocation. Pass arguments directly to exec.Command without a shell wrapper.
(coderabbit.command-injection.go-exec-command)
🤖 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 `@ote/test/e2e/winc.go` around lines 113 - 117, Replace the shell-based file
read in the smokerun test with direct os.ReadFile(publicKey), adding the os
import as needed. Preserve the existing error assertion and public-key parsing
behavior after reading the file.
Sources: Coding guidelines, Linters/SAST tools
| e2e.Failf("No Windows nodes with InternalIP found") | ||
| } | ||
| curlDest := net.JoinHostPort(winInternalIPs[0], "22") | ||
| msg, _ := execInPod(oc, wmcoNamespace, wmcoDeployment, "curl", "--http0.9", curlDest) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Errors ignored with _ from execInPod.
Both call sites discard the error return of execInPod, relying only on stdout content matching. Per Go guidelines, error returns should not be silently discarded without justification — at minimum, log the error so a transport/exec failure isn't misread as a genuine "not rejected" / "no SSH banner" test failure.
Also applies to: 315-315
🤖 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 `@ote/test/e2e/winc.go` at line 193, The call sites invoking execInPod in the
relevant test cases discard execution errors; capture and log the returned error
before evaluating the command output. Update both the curl check and the other
execInPod call noted by the review, preserving their existing stdout assertions
while ensuring transport or execution failures are visible.
Source: Coding guidelines
| g.It("Smokerun-Author:rrasouli-Medium-60814-Check containerd version is properly reported", func() { | ||
| wmcoVersion, err := getWMCOVersionFromLogs(oc) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| parts := strings.Split(wmcoVersion, "-") | ||
| o.Expect(len(parts)).Should(o.BeNumerically(">", 1), "unexpected WMCO version format") | ||
| versionHash := parts[1] | ||
| resp, err := http.Get("https://raw.githubusercontent.com/openshift/windows-machine-config-operator/" + versionHash + "/Makefile") | ||
| if err != nil { | ||
| e2e.Failf("unable to get http with error: %v", err) | ||
| } | ||
| body, err := io.ReadAll(resp.Body) | ||
| defer resp.Body.Close() | ||
| if err != nil { | ||
| e2e.Failf("unable to parse the http result with error: %v", err) | ||
| } | ||
|
|
||
| submoduleContainerdVersion := getValueFromText(body, "CONTAINERD_GIT_VERSION=") | ||
| for _, winhost := range getWindowsHostNames(oc) { | ||
| if strings.Compare(string(submoduleContainerdVersion), getContainerdVersion(oc, winhost)) != 0 { | ||
| e2e.Failf("Containerd version mismatch expected %s actual %s", submoduleContainerdVersion, getContainerdVersion(oc, winhost)) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Test requires public internet access to raw.githubusercontent.com — breaks disconnected/airgapped clusters.
http.Get("https://raw.githubusercontent.com/openshift/windows-machine-config-operator/" + versionHash + "/Makefile") requires outbound access to a public host with no timeout/context. WMCO explicitly documents support for disconnected/airgapped environments, and the e2e guideline for this repo states tests must not require external connectivity to public hosts/URLs. In a disconnected CI lane this test will hang (no timeout) or fail for reasons unrelated to the actual containerd-version check it's meant to validate. Consider sourcing the expected containerd version from an in-cluster artifact (e.g., the WMCO image/payload itself, already fetched via oc exec elsewhere in this file) instead of a public GitHub fetch, and add a request timeout regardless.
🤖 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 `@ote/test/e2e/winc.go` around lines 262 - 285, Remove the public GitHub
request from the “Smokerun-Author:rrasouli-Medium-60814-Check containerd version
is properly reported” test and source the expected containerd version from an
already available in-cluster WMCO artifact via the existing oc-based helpers.
Preserve the comparison against getContainerdVersion for each Windows host, and
ensure any retained HTTP request uses a bounded timeout and response cleanup.
Source: Coding guidelines
| g.By("Verifying HTTP is not allowed on metrics endpoint") | ||
| winInternalIPs := getWindowsInternalIPs(oc) | ||
| o.Expect(len(winInternalIPs)).To(o.BeNumerically(">=", 2), | ||
| "Need at least 2 Windows nodes to test cross-node HTTP rejection") | ||
| msg, _ := execInPod(oc, wmcoNamespace, wmcoDeployment, "curl", "-k", "http://"+winInternalIPs[1]+":9182/metrics") | ||
| o.Expect(msg).To(o.ContainSubstring("Client sent an HTTP request to an HTTPS server"), | ||
| "Expected HTTP request to be rejected with HTTPS requirement message") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fails instead of skipping on insufficient Windows-node count.
Unlike other tests in this file (isNone(oc) skip, validatedCount == 0 skip), this cross-node HTTP-rejection check uses o.Expect(...).To(BeNumerically(">=", 2), ...), which fails the test on single-Windows-node clusters instead of gracefully skipping. Consider if len(winInternalIPs) < 2 { g.Skip(...) } to match the established pattern and avoid false failures on valid single-node Windows topologies.
🤖 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 `@ote/test/e2e/winc.go` around lines 311 - 317, Replace the assertion on
len(winInternalIPs) in the “Verifying HTTP is not allowed on metrics endpoint”
test with a conditional skip when fewer than two Windows internal IPs are
available. Keep the cross-node curl and response validation unchanged for
clusters with at least two nodes.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unbracketed IPv6-unsafe URL construction.
"http://"+winInternalIPs[1]+":9182/metrics" breaks for IPv6 addresses (ambiguous colon parsing), unlike the correct net.JoinHostPort usage two tests earlier (line 192). Per this repo's e2e guidelines, unbracketed host/IP URL construction is disallowed.
🔧 Proposed fix
- msg, _ := execInPod(oc, wmcoNamespace, wmcoDeployment, "curl", "-k", "http://"+winInternalIPs[1]+":9182/metrics")
+ msg, _ := execInPod(oc, wmcoNamespace, wmcoDeployment, "curl", "-k", "http://"+net.JoinHostPort(winInternalIPs[1], "9182")+"/metrics")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| g.By("Verifying HTTP is not allowed on metrics endpoint") | |
| winInternalIPs := getWindowsInternalIPs(oc) | |
| o.Expect(len(winInternalIPs)).To(o.BeNumerically(">=", 2), | |
| "Need at least 2 Windows nodes to test cross-node HTTP rejection") | |
| msg, _ := execInPod(oc, wmcoNamespace, wmcoDeployment, "curl", "-k", "http://"+winInternalIPs[1]+":9182/metrics") | |
| o.Expect(msg).To(o.ContainSubstring("Client sent an HTTP request to an HTTPS server"), | |
| "Expected HTTP request to be rejected with HTTPS requirement message") | |
| g.By("Verifying HTTP is not allowed on metrics endpoint") | |
| winInternalIPs := getWindowsInternalIPs(oc) | |
| o.Expect(len(winInternalIPs)).To(o.BeNumerically(">=", 2), | |
| "Need at least 2 Windows nodes to test cross-node HTTP rejection") | |
| msg, _ := execInPod(oc, wmcoNamespace, wmcoDeployment, "curl", "-k", "http://"+net.JoinHostPort(winInternalIPs[1], "9182")+"/metrics") | |
| o.Expect(msg).To(o.ContainSubstring("Client sent an HTTP request to an HTTPS server"), | |
| "Expected HTTP request to be rejected with HTTPS requirement message") |
🤖 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 `@ote/test/e2e/winc.go` around lines 311 - 317, Update the metrics endpoint URL
construction in the HTTP rejection test to use IPv6-safe host/port joining via
the existing net.JoinHostPort pattern, preserving the winInternalIPs[1] address,
port 9182, and /metrics path.
Source: Coding guidelines
rrasouli
force-pushed
the
winc-1966-ote-batch2-monitoring
branch
from
76a75f1 to
cbe92fd
Compare
Migrate 4 monitoring/observability tests from OTP to OTE framework: - OCP-33768: Prometheus alert rules validation - OCP-60814: Containerd version verification against upstream - OCP-77777: ServiceMonitor, HTTPS metrics, HTTP rejection - OCP-79251: Provider ID matching between nodes and machines Removed OCP-33779 (node logs) - redundant with WMCO e2e testNodeLogs coverage in test/e2e/logs_test.go. Tracked in WINC-1978 for OTP disable. Key changes: - Add generic execInPod() helper replacing SSH/bastion (WINC-1931) - Extract wmcoDeployment variable for reuse - Simplify truncatedVersion() with regex, remove removeOuterQuotes() - Add helpers: getContainerdVersion, getValueFromText, isNone, extractInstanceID, waitUntilWMCOStatusChanged, isBYOH - Add one-line comments to all utils.go functions Stacked on: WINC-1966 Batch 1 (openshift#4279)
rrasouli
force-pushed
the
winc-1966-ote-batch2-monitoring
branch
from
cbe92fd to
df1f5ee
Compare
2 participants
Summary
Migrates 4 monitoring/observability tests from OTP to the OTE Ginkgo framework, continuing the batch migration tracked in WINC-1966. This PR stacks on Batch 1 (#4279).
New tests migrated:
Test removed (not migrated):
test/e2e/logs_test.go:testNodeLogs, which already validates the same 6 log paths viaoc adm node-logs. Disabling in OTP tracked in WINC-1978.Key Changes
New generic
execInPod()helper (WINC-1931)Replaces SSH/bastion access pattern with
oc execinto any pod. Used by OCP-33768 (prometheus pod), OCP-32554 and OCP-77777 (WMCO pod). Aligns with WINC-1931 refactoring initiative.Local Makefile read instead of GitHub fetch (OCP-60814)
Reads
CONTAINERD_GIT_VERSIONfrom the local repo Makefile viaos.ReadFileinstead of fetching fromraw.githubusercontent.com. Works on disconnected/airgapped clusters without public internet access.Extracted
wmcoDeploymentvariable"deployment.apps/windows-machine-config-operator"extracted to a reusable variable, eliminating 5+ hardcoded occurrences.Simplified
truncatedVersion()with regexReplaced
truncatedVersion+removeOuterQuotes(17 lines, 2 functions) with a single 7-line regex-basedtruncatedVersion.removeOuterQuotesdeleted as unused.New utility functions (all with comments)
getContainerdVersion- Node containerRuntimeVersion field parsinggetValueFromText- Line search and delimiter-based value extraction (with safe empty return)isNone- Platform None / missing MachineSet detectionextractInstanceID- JSON provider ID parsing for nodes and machineswaitUntilWMCOStatusChanged- WMCO log polling with timeoutisBYOH- BYOH label detection on nodesAdditional improvements
net.JoinHostPortfor metrics endpointg.Skipinstead of hard fail when < 2 Windows nodes for cross-node HTTP rejection testexecInPoderrors logged instead of silently discarded%winstead of%vinextractInstanceIDDocumentation
All 17 functions in utils.go now have one-line comments.
Test list (9 total: 5 batch 1 + 4 batch 2)
Jira
Dependencies
Stacked on Batch 1 PR #4279 - must merge first.