Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 5 additions & 19 deletions hack/machineset.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
# machineset.sh
# OPTIONS
# -b Set 'windowsmachineconfig.openshift.io/ignore' label for BYOH use case. Default: false
# -w Windows Server version (optional) 2019 or 2022. Default: 2022
# -w Windows Server version (optional) 2022 or 2025. Default: 2022
# $1/$2 (if -w is used) Action (optional) apply/delete the MachineSet
# PREREQUISITES
# oc to fetch cluster info and apply/delete MachineSets on the cluster(cluster should be logged in)
Expand Down Expand Up @@ -87,9 +87,6 @@ get_aws_ms() {
# check Windows version and set AMI filter
filter=""
case "$winver" in
"2019")
filter="Windows_Server-2019-English-Core-Base-????.??.??"
;;
"2025")
filter="Windows_Server-2025-English-Core-Base-????.??.??"
;;
Expand Down Expand Up @@ -167,11 +164,6 @@ get_azure_ms() {
local sku=""
local release="latest"
case "$winver" in
"2019")
sku="2019-datacenter-smalldisk"
# TODO: remove when VM SSH issue is patched in Azure cloud
release="17763.6293.240905"
;;
"2025")
sku="2025-datacenter-smalldisk"
;;
Expand Down Expand Up @@ -239,9 +231,6 @@ get_gcp_ms() {
# check Windows version and set image family
local image=""
case "$winver" in
"2019")
image="projects/windows-cloud/global/images/family/windows-2019-core"
;;
"2025")
image="projects/windows-cloud/global/images/family/windows-2025-core"
;;
Expand Down Expand Up @@ -306,9 +295,6 @@ get_vsphere_ms() {

# set golden image template name based on Windows Server version
case "$winver" in
"2019")
error-exit "No template available for Windows Server 2019 in DevQE vCenter"
;;
"2025")
template="windows-golden-images/windows-server-2025-template-ipv6-disabled"
;;
Expand Down Expand Up @@ -430,18 +416,18 @@ winver="2022"
byoh=false
while getopts ":w:b" opt; do
case ${opt} in
w ) # Windows Server version to use in the MachineSet. Defaults to 2022. Other option is 2019.
w ) # Windows Server version to use in the MachineSet. Defaults to 2022. Other option is 2025.
winver="$OPTARG"
if [[ ! "$winver" =~ 2019|2022|2025$ ]]; then
echo "Invalid -w option $winver. Valid options are 2019, 2022 or 2025"
if [[ ! "$winver" =~ 2022|2025$ ]]; then
echo "Invalid -w option $winver. Valid options are 2022 or 2025"
exit 1
Comment on lines +421 to 423
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify the current regex in the script
rg -n 'winver.*=~' hack/machineset.sh

# Demonstrate current behavior of the same regex expression
pattern='2022|2025$'
for v in 2022 2025 abc2022 20220 x2025y; do
  if [[ "$v" =~ $pattern ]]; then
    echo "accepted: $v"
  else
    echo "rejected: $v"
  fi
done

# Expected: malformed values should be rejected, but current pattern accepts some of them.

Repository: openshift/windows-machine-config-operator

Length of output: 216


🏁 Script executed:

sed -n '410,435p' hack/machineset.sh

Repository: openshift/windows-machine-config-operator

Length of output: 621


Fix -w regex anchoring to reject malformed Windows versions.

Line 421 uses 2022|2025$, which accepts partial matches like abc2022 and 20220. Fully anchor the alternation to ensure only exact matches for valid Windows Server versions.

Proposed fix
-      if [[ ! "$winver" =~ 2022|2025$ ]]; then
+      if [[ ! "$winver" =~ ^(2022|2025)$ ]]; then
         echo "Invalid -w option $winver. Valid options are 2022 or 2025"
         exit 1
       fi
📝 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.

Suggested change
if [[ ! "$winver" =~ 2022|2025$ ]]; then
echo "Invalid -w option $winver. Valid options are 2022 or 2025"
exit 1
if [[ ! "$winver" =~ ^(2022|2025)$ ]]; then
echo "Invalid -w option $winver. Valid options are 2022 or 2025"
exit 1
fi
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/machineset.sh` around lines 421 - 423, The regex in the if check that
validates the winver variable currently uses unanchored alternation and allows
partial matches; update the conditional that tests "$winver" (the if [[ !
"$winver" =~ 2022|2025$ ]] block) to use a fully anchored alternation such as
^(2022|2025)$ so only exact values "2022" or "2025" pass; keep the same quoting
and exit behavior but replace the regex with the anchored pattern.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not related

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrvaldes, understood — the regex anchoring concern is pre-existing and not related to the 2019 removal in scope here. Would you like me to open a follow-up GitHub issue to track the fix (^(2022|2025)$) so it doesn't get lost?

fi
;;
b )
byoh=true
;;
\? )
echo "Usage: $0 -w <2019/2022> -b apply/delete"
echo "Usage: $0 -w <2022/2025> -b apply/delete"
exit 0
Comment on lines 429 to 431
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Static verification of the invalid-option branch behavior.
rg -n -C2 '\\\? \)|Usage: .*2022/2025|exit 0|exit 1' hack/machineset.sh

Repository: openshift/windows-machine-config-operator

Length of output: 405


Return non-zero exit code for invalid CLI option usage.

Line 431 exits with 0 (success) when invalid options are provided, which masks failures in CI/scripts. Invalid CLI usage should signal an error. Note the inconsistency: line 423 correctly exits with 1 for invalid argument values, but the catchall \? ) pattern exits with 0.

Proposed fix
    \? )
      echo "Usage: $0 -w <2022/2025> -b apply/delete"
-     exit 0
+     exit 1
       ;;
📝 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.

Suggested change
\? )
echo "Usage: $0 -w <2019/2022> -b apply/delete"
echo "Usage: $0 -w <2022/2025> -b apply/delete"
exit 0
\? )
echo "Usage: $0 -w <2022/2025> -b apply/delete"
exit 1
;;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hack/machineset.sh` around lines 429 - 431, The catch-all case handling
invalid CLI options in the case pattern '\? )' currently exits with 0; change it
to exit with non-zero to indicate error (use exit 1) and print the usage message
to stderr (redirect echo to >&2) so scripts/CI detect failure; update the '\? )'
block in hack/machineset.sh that emits "Usage: $0 -w <2022/2025> -b
apply/delete" to echo to stderr and call exit 1.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not related

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrvaldes, understood — this is a pre-existing issue unrelated to the WS2019 removal. Would you like me to open a follow-up GitHub issue to track fixing the exit 0exit 1 behavior in the \? case of hack/machineset.sh?

;;
esac
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ func TestMain(m *testing.M) {
flag.BoolVar(&skipWorkloadDeletion, "skip-workload-deletion", false,
"skips deletion of workloads at the end of tests")
flag.Func("windows-server-version", "Windows Server version to test. "+
"Supported versions: 2019, 2022 or 2025 (default 2022)",
"Supported versions: 2022 or 2025 (default 2022)",
func(value string) error {
if len(value) == 0 {
windowsServerVersion = windows.Server2022
Expand Down
2 changes: 0 additions & 2 deletions test/e2e/network_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -864,8 +864,6 @@ func (tc *testContext) getPodIP(selector metav1.LabelSelector) (string, error) {
// getWindowsServerContainerImage gets the appropriate WindowsServer image based on the OS version
func (tc *testContext) getWindowsServerContainerImage() string {
switch tc.windowsServerVersion {
case windows.Server2019:
return "mcr.microsoft.com/powershell:lts-nanoserver-1809"
case windows.Server2022:
case windows.Server2025:
// 2022-based container images are compatible with Windows Server 2025
Expand Down
2 changes: 0 additions & 2 deletions test/e2e/providers/aws/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ func newEC2Client(region string) (*ec2.EC2, error) {
// pattern Windows_Server-$version-variant-YYYY.MM.DD and the filter will grab all AMIs that match the filter.
func getWindowsAMIFilter(windowsServerVersion windows.ServerVersion) string {
switch windowsServerVersion {
case windows.Server2019:
return "Windows_Server-2019-English-Core-Base-????.??.??"
case windows.Server2025:
return "Windows_Server-2025-English-Core-Base-????.??.??"
case windows.Server2022:
Expand Down
6 changes: 0 additions & 6 deletions test/e2e/providers/azure/azure.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,6 @@ func New(clientset *clusterinfo.OpenShift, infraStatus *config.InfrastructureSta
// newAzureMachineProviderSpec returns an AzureMachineProviderSpec generated from the inputs, or an error
func (p *Provider) newAzureMachineProviderSpec(location, zone, vmSize, publicLoadBalancer string, windowsServerVersion windows.ServerVersion) (*mapi.AzureMachineProviderSpec, error) {
imageVersion := defaultImageVersion
if windowsServerVersion == windows.Server2019 {
// TODO: remove when VM SSH issue is patched in Azure cloud
imageVersion = "17763.6293.240905"
}
return &mapi.AzureMachineProviderSpec{
TypeMeta: meta.TypeMeta{
APIVersion: "azureproviderconfig.openshift.io/v1beta1",
Expand Down Expand Up @@ -345,8 +341,6 @@ func (p *Provider) ensureWindowsCSIDaemonSet(client client.Interface) error {
// getImageSKU returns the SKU based on the Windows Server version
func getImageSKU(windowsServerVersion windows.ServerVersion) string {
switch windowsServerVersion {
case windows.Server2019:
return "2019-datacenter-smalldisk"
case windows.Server2025:
return "2025-datacenter-smalldisk"
case windows.Server2022:
Expand Down
2 changes: 0 additions & 2 deletions test/e2e/providers/gcp/gcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,6 @@ func (p *Provider) CreatePVC(_ client.Interface, _ string, _ *core.PersistentVol
// getImage returns the image based on the Windows Server version
func getImage(windowsServerVersion windows.ServerVersion) string {
switch windowsServerVersion {
case windows.Server2019:
return "projects/windows-cloud/global/images/family/windows-2019-core"
case windows.Server2025:
return "projects/windows-cloud/global/images/family/windows-2025-core"
case windows.Server2022:
Expand Down
6 changes: 1 addition & 5 deletions test/e2e/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import (
"github.com/openshift/windows-machine-config-operator/pkg/secrets"
"github.com/openshift/windows-machine-config-operator/pkg/servicescm"
"github.com/openshift/windows-machine-config-operator/pkg/windows"
e2e_windows "github.com/openshift/windows-machine-config-operator/test/e2e/windows"
)

const (
Expand Down Expand Up @@ -354,10 +353,7 @@ func (tc *testContext) ensureTestRunnerRBAC() error {
func (tc *testContext) runPowerShellSSHJob(name, command, ip string) (string, error) {
// Modify command to work when default shell is the newer Powershell version present on Windows Server 2022 and
// later
powershellDefaultCommand := command
if tc.windowsServerVersion != e2e_windows.Server2019 {
powershellDefaultCommand = strings.ReplaceAll(command, "\\\"", "\"")
}
powershellDefaultCommand := strings.ReplaceAll(command, "\\\"", "\"")

keyMountDir := "/private-key"
sshOptions := "-o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null"
Expand Down
5 changes: 1 addition & 4 deletions test/e2e/windows/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package windows
type ServerVersion string

const (
// Server2019 represent Windows Server 2019
Server2019 ServerVersion = "2019"
// Server2022 represent Windows Server 2022
Server2022 ServerVersion = "2022"
// Server2025 represent Windows Server 2025
Expand All @@ -13,11 +11,10 @@ const (

// SupportedVersions are the Windows Server versions supported by the e2e test.
// "" implies the default which is Server2022
var SupportedVersions = []ServerVersion{Server2019, Server2022, Server2025, ""}
var SupportedVersions = []ServerVersion{Server2022, Server2025, ""}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// BuildNumber returns the build for a given server version as defined by Microsoft
var BuildNumber = map[ServerVersion]string{
Server2019: "10.0.17763",
Server2022: "10.0.20348",
Server2025: "10.0.26100",
}
Expand Down