WINC-1966: OTE Migration Batch 1 - Node Basics & Validation

[rrasouli]

Summary

Migrate 5 foundational Smokerun tests from openshift-tests-private (OTP) to WMCO's ote/ directory using the Ginkgo-based OTE framework. This replaces the manual-registration skeleton from PR #3874.

Jira: https://issues.redhat.com/browse/WINC-1966
Epic: https://issues.redhat.com/browse/WINC-1536

Why replace manual registration with Ginkgo?

The previous skeleton (PR #3874) used manual ExtensionTestSpec registration -- each test was a bare Go function registered one-by-one in main.go, with a custom CLI wrapper duplicating compat_otp. Adding tests required editing both the test file and main.go.

The Ginkgo approach uses BuildExtensionTestSpecsFromOpenShiftGinkgoSuite() for automatic discovery. Tests are standard g.Describe/g.It blocks (same structure as OTP), so code can be migrated with minimal changes. Adding a new test only requires a new g.It block -- no registration code to maintain. This is the approach recommended by OTE framework maintainers.

Key design decisions

• Removed bastion SSH dependency from OCP-33612: The original OTP test checked Windows services via SSH through a bastion host. This fails on clusterBot clusters where no bastion is provisioned. The service check is redundant -- WMCO's own e2e tests already cover it via testExpectedServicesRunning in test/e2e/validation_test.go. The SSH/bastion functions from OTP were never migrated -- utils.go was written from scratch with only the functions needed by the 5 tests. The remaining OCP-33612 checks (kubelet version, worker labels, version annotations, payload folders) all use oc commands and work on any cluster.

Files to review

New files (2 files with original logic):

• ote/cmd/wmco-tests-ext/main.go -- OTE entry point: Ginkgo auto-discovery, suite registration, LifecycleInforming for all tests
• ote/test/e2e/winc.go -- the 5 migrated tests as g.It blocks

Adapted from OTP (review recommended):

• ote/test/e2e/utils.go -- shared helper functions adapted from OTP: trimmed to only functions used by the 5 tests, SSH/bastion functions removed, added bounds checks, switched strings.Split to strings.Fields for empty-string safety, added strconv.Atoi error handling

Deleted (replaced by above):

• ote/cmd/wmco-tests-ext/main.go -- manual registration entry point
• ote/test/extended/cli/cli.go -- custom CLI wrapper (unnecessary with compat_otp)
• ote/test/extended/cluster.go -- bare Go function for OCP-37362 (now a g.It block)

Auto-generated (skip during review):

• ote/go.mod, ote/go.sum -- dependency resolution

Tests migrated

OCP ID	Test Name	Priority
OCP-33612	Windows node basic check	Critical
OCP-32615	Generate userData secret [Serial]	Critical
OCP-32554	wmco run in pod with HostNetwork	Low
OCP-37362	wmco using correct golang version	Medium
OCP-38188	Get Windows instance/core number and CPU arch	Medium

All are Smokerun tests. All set to LifecycleInforming (non-blocking) until validated in CI.

Testing evidence (clusterBot - AWS)

All 5 tests passed on clusterBot cluster:

Test	Result	Duration
OCP-33612 - Windows node basic check	PASSED	33s
OCP-32615 - Generate userData secret [Serial]	PASSED	46s
OCP-32554 - wmco run in pod with HostNetwork	PASSED	--
OCP-37362 - wmco using correct golang version	PASSED	12s
OCP-38188 - Get Windows instance/core number and CPU arch	PASSED	22s

How to test on clusterBot

make build-tests-ext

# Derive SSH keys from cluster (required for OCP-32615):
oc get secret cloud-private-key -n openshift-windows-machine-config-operator \
  -o jsonpath='{.data.private-key\.pem}' | base64 -d > /tmp/cloud-private-key.pem
chmod 600 /tmp/cloud-private-key.pem
ssh-keygen -y -f /tmp/cloud-private-key.pem > /tmp/cloud-public-key.pub
export SSH_CLOUD_PRIV_KEY=/tmp/cloud-private-key.pem
export SSH_CLOUD_PUB_KEY=/tmp/cloud-public-key.pub

# Run all tests:
KUBECONFIG=/path/to/kubeconfig ./build/_output/bin/wmco-tests-ext run-test \
  -n "<exact test name from list command>"

# List available tests:
KUBECONFIG=/path/to/kubeconfig ./build/_output/bin/wmco-tests-ext list

Verification

• make build-tests-ext -- builds successfully
• Binary lists exactly 5 tests with correct OCP IDs
• info shows 5 suites: conformance/parallel, conformance/serial, disruptive, non-disruptive, all

Summary by CodeRabbit

• New Features
  • Added expanded Windows end-to-end test coverage for node version checks, labels, payload verification, user-data handling, SSH access, version reporting, and telemetry validation.
• Tests
  • Switched test execution to discover and register Windows-related suites automatically, improving coverage across conformance and disruptive scenarios.
  • Added shared helpers for querying cluster and metric data used by Windows-focused tests.
• Chores
  • Added a simple build/clean workflow for the test extension binary and updated the Go toolchain and dependency set.

[openshift-ci-robot]

@rrasouli: This pull request references WINC-1966 which is a valid jira issue.

Details

In response to this:

Summary

Migrate 5 foundational Smokerun tests from openshift-tests-private to WMCO ote/ directory using the OTE framework (Ginkgo-based approach).

Jira: https://issues.redhat.com/browse/WINC-1966
Epic: https://issues.redhat.com/browse/WINC-1536

Tests to Migrate

• OCP-33612 -- Windows node basic check (Critical, Smokerun)
• OCP-32615 -- Generate userData secret (Critical, Smokerun, Serial)
• OCP-32554 -- wmco run in a pod with HostNetwork (Low, Smokerun)
• OCP-38188 -- Get Windows instance/core number and CPU arch (Medium, Smokerun)
• OCP-37362 -- wmco using correct golang version (Medium, Smokerun) -- re-implement as Ginkgo

Notes

• First batch: validates the OTE scaffold works end-to-end
• All pure Smokerun, no Disruptive or Longduration
• OCP-37362 exists in Weinan's manual-registration style, needs conversion to Ginkgo g.It
• Includes OTE bootstrap setup (Ginkgo suite, compat_otp, Makefile, Dockerfile)

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]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 8689feeb-cf75-4bfc-b27a-769a8bb29f14

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR reworks the OTE (openshift-tests-extension) tooling for the Windows Machine Config Operator. It adds a Makefile for building the test-extension binary, upgrades go.mod (Go 1.25, new Ginkgo/Gomega/OpenShift/Kubernetes dependencies, and replace directives). The main entrypoint is rewritten to dynamically build and filter test specs from the OpenShift Ginkgo suite rather than manually registering WINC suites. New files ote/test/e2e/utils.go and ote/test/e2e/winc.go add helper functions and a migrated Ginkgo test suite covering kubelet versions, node labels, payload verification, user-data secrets, HostNetwork SSH checks, Go-version checks, and telemetry metric validation. The previous ote/test/extended/cli/cli.go and ote/test/extended/cluster.go files are deleted.

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 7 warnings)

Check name	Status	Explanation	Resolution
No-Sensitive-Data-In-Logs	❌ Error	Failure logs print Windows hostnames and decoded windows-user-data secret contents, which can expose internal hostnames and secret data.	Remove node/secret values from Failf messages; log generic identifiers or redact values, and avoid dumping decoded secret contents.
Go Best Practices & Build Tags	⚠️ Warning	Touched Go files still ignore multiple errors (_ on oc/base64/Atoi/exec) and use runtime panics in main.go; that violates the error-handling rules.	Check every ignored result and return wrapped errors with context. Replace panic paths with fatal error handling, and guard slice accesses before indexing.
Security: Secrets, Ssh & Csr	⚠️ Warning	winc.go logs decoded windows-user-data secret contents in multiple Failf paths, violating the no-secret-in-errors rule; no CSR/SSH hardening is added.	Redact decoded secret values from all error messages, log only names/IDs, and surface decode errors separately; add SSH/CSR guards only where those flows exist.
Platform-Specific Requirements	⚠️ Warning	No vSphere/AWS/Azure/GCP-specific checks or limitation docs were added; the new ote tests are generic Windows flows plus payload presence checks.	Add explicit platform gates/tests or docs for vSphere name limits, AWS EC2LaunchV2, Azure cloud-node-manager, GCP hostname script usage, and platform limitations.
Test Structure And Quality	⚠️ Warning	winc.go’s first It mixes 4 unrelated checks, and many Expect(err).NotTo(HaveOccurred()) calls lack failure context.	Split the broad It into focused tests, and add descriptive messages to bare assertions; keep any secret mutations paired with explicit cleanup.
Microshift Test Compatibility	⚠️ Warning	The new Ginkgo suite uses config.openshift.io, openshift-monitoring, machine-api/capi namespaces, and Windows/Linux node assumptions, with no MicroShift skip/tag.	Tag or guard the suite with [Skipped:MicroShift] (or an apigroup tag where applicable) and avoid MicroShift-unsupported OpenShift APIs/namespaces when running there.
Single Node Openshift (Sno) Test Compatibility	⚠️ Warning	New Ginkgo tests assume separate Linux/Windows nodes (kubelet compare, Windows IP curl) and have no SNO skip/guard.	Add a [Skipped:SingleReplicaTopology] label or an IsSingleNode()/topology guard to the Windows-node/version and HostNetwork tests, or confirm they never run on SNO.
Ipv6 And Disconnected Network Test Compatibility	⚠️ Warning	HostNetwork test builds winInternalIP + ":22", which is IPv6-unsafe; no external internet calls were found.	Replace the host:port concat with net.JoinHostPort(winInternalIP, "22") and guard the empty internal-IP slice so IPv6 CI passes.

✅ Passed checks (12 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Kubernetes Controller Patterns	✅ Passed	Not applicable: this PR only adds OTE test entrypoints and Ginkgo E2E specs; no controller/reconcile/predicate/finalizer/owner-ref code is present.
Windows Service Management	✅ Passed	The PR only migrates Windows e2e tests; the changed ote files contain no SCM/service-control code, dependencies, cleanup, or reboot handling.
Stable And Deterministic Test Names	✅ Passed	All Ginkgo titles in ote are static literals; none include generated names, dates, UUIDs, node/namespace/IP data, or other runtime values.
Topology-Aware Scheduling Compatibility	✅ Passed	No deployment/controller scheduling logic was added; the changed OTE files are test/build code only, with no affinity, topology spread, or nodeSelector constraints.
Ote Binary Stdout Contract	✅ Passed	No process-level stdout writes found: main.go has no fmt.Print* calls, no init/TestMain/BeforeSuite hooks, and only initializes logs plus Cobra commands.
No-Weak-Crypto	✅ Passed	Scanned the changed OTE files; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret/token comparisons were introduced.
Container-Privileges	✅ Passed	No privileged/hostNetwork/hostPID/hostIPC/SYS_ADMIN/allowPrivilegeEscalation settings appear in the added OTE files or manifests.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the OTE migration batch for node basics and validation tests.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rrasouli

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• ote/OWNERS [rrasouli]

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

[rrasouli]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 10

🤖 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/go.mod`:
- Line 243: The dependency set still pulls in google.golang.org/grpc v1.75.1,
which needs to be bumped to a patched release. Update the grpc module version in
go.mod (and any related indirect/replace entries if present) to v1.79.3 or
newer, then ensure the module graph resolves to the patched grpc version through
the existing dependency chain.

In `@ote/test/e2e/utils.go`:
- Around line 62-67: The truncatedVersion helper can panic when given a version
string with fewer than two dot-separated parts. Update truncatedVersion to guard
the slice operation before using strings.Split and str[:2], and return a safe
fallback for empty or short inputs so callers like the winc.go version parsing
path do not crash the test. Use the truncatedVersion function and its
removeOuterQuotes/string splitting logic as the fix point.
- Around line 69-89: getMetricsFromCluster currently miscounts empty Windows
node results and ignores parsing failures. In the getMetricsFromCluster helper,
replace the string splitting used for node and CPU lists with logic that treats
empty output as zero (for example, use strings.Fields) so an empty node list
returns 0 instead of 1, and update the capacity_cpu_cores branch to handle the
strconv.Atoi error explicitly instead of discarding it. Keep the behavior
localized to getMetricsFromCluster and preserve the existing metric dispatch
logic.
- Around line 27-33: The version-annotation lookup in
checkVersionAnnotationReady is passing shell-style single quotes through Args(),
which makes oc treat them as part of the JSONPath template and breaks the read.
Remove the surrounding quotes from the jsonpath argument so oc receives the raw
template, and keep the rest of the node lookup logic in
checkVersionAnnotationReady unchanged. This should restore the annotation check
used by the e2e flow in winc.go.

In `@ote/test/e2e/winc.go`:
- Line 117: The `base64.StdEncoding.DecodeString` calls in `winc.go` are
discarding decode errors, which can leave `decodedUserData` empty or partial and
hide the real failure. Update all affected call sites in the test flow to
capture and check the returned error, and fail the test immediately with a
message that includes the decode error before using `decodedUserData`. Keep the
fix consistent across the decode logic in the helper/test functions that
currently ignore the error.
- Around line 22-24: The infrastructure platform lookup in BeforeEach is
ignoring the command error, which can leave iaasPlatform empty and hide
failures; update the
oc.AsAdmin().WithoutNamespace().Run("get").Args(...).Output() call to handle the
returned error explicitly, and fail the test or otherwise stop setup when the
query does not succeed. Keep the iaasPlatform assignment in the same setup
block, but only after confirming the output was retrieved successfully.
- Around line 197-199: The winc E2E test ignores the error from exec.Command,
which can leave goVersion empty and cause truncatedVersion to panic. Update the
command execution block in the winc test to capture and check the error before
calling truncatedVersion, and fail the test with a clear message if the command
cannot run or returns no output. Use the existing exec.Command,
truncatedVersion, and tVersion flow to locate the fix.
- Around line 230-234: The metric extraction in winc.go is happening before the
SimpleQuery error check and is being called twice, which can mask the real
failure and duplicate work. In the query handling flow around
extractMetricValue(queryResult), move the err assertion ahead of any use of
queryResult so failures are surfaced first, then call extractMetricValue only
once after confirming the query succeeded. Remove the redundant second
invocation and keep the existing expectation message aligned with the
metricQuery/metricValue context.
- Around line 184-185: The Windows internal IP handling in the e2e test can
panic on an empty result and builds an IPv4-only SSH target string. In the test
setup around getWindowsInternalIPs(oc), first check that the returned slice is
non-empty and fail the test cleanly if no Windows IPs are available, then
construct curlDest using net.JoinHostPort with winInternalIP and "22" so IPv6
addresses are formatted correctly.
- Around line 221-225: The metric filter string in the telemetry exposure check
is using a raw string in the fmt.Sprintf call, so the backslashes are preserved
and the assertion matches the wrong text. Update the construction of
expectedExposedMetric in winc.go to use a double-quoted string in the same test
block so it produces {__name__="..."} exactly, and keep the rest of the
telemetry-config assertion unchanged.

🪄 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: bb64c2dc-3348-49aa-bc96-425996923327

📥 Commits

Reviewing files that changed from the base of the PR and between a8f9eb8 and 9c3994d.

⛔ Files ignored due to path filters (1)

• ote/go.sum is excluded by !**/*.sum

📒 Files selected for processing (8)

• ote/Makefile
• ote/cmd/main.go
• ote/cmd/wmco-tests-ext/main.go
• ote/go.mod
• ote/test/e2e/utils.go
• ote/test/e2e/winc.go
• ote/test/extended/cli/cli.go
• ote/test/extended/cluster.go

💤 Files with no reviewable changes (3)

• ote/cmd/wmco-tests-ext/main.go
• ote/test/extended/cluster.go
• ote/test/extended/cli/cli.go

[rrasouli]

All CodeRabbit review comments have been addressed:

• utils.go:33 (jsonpath quotes): Removed shell-style single quotes from the jsonpath argument in checkVersionAnnotationReady. Args() passes the value directly to oc, so quotes are not needed.
• utils.go:69 (truncatedVersion panic): Added bounds check -- returns input string unchanged if fewer than 2 dot-separated segments.
• utils.go:95 (getMetricsFromCluster): Replaced strings.Split with strings.Fields (returns empty slice for empty string) and added strconv.Atoi error check with o.Expect.
• winc.go:26 (BeforeEach error): Infrastructure platform query error is now checked with o.Expect(err).NotTo(o.HaveOccurred()).
• winc.go:235 (extractMetricValue): Moved error check before extractMetricValue call and removed duplicate invocation.
• go.mod:243 (grpc CVE): Out of scope -- indirect dependency inherited from openshift/origin (OTP). Cannot be independently bumped without upstream changes.

[rrasouli]

@CodeRabbit review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[openshift-ci]

@rrasouli: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.