#3044 First step for windows worker

[krotz-dieter]

#3044 First step for windows worker

[krotz-dieter]

I'll review this PR systematically by examining each changed file for bugs, logic errors, security issues, and potential regressions.

Now let me verify the struct definitions to ensure the code changes reference valid fields:

The current main branch might not have these structures yet. Let me check the PR branch:

Let me trace through the code logic more carefully based on the diff. I'll analyze the key changes:

Now let me identify specific issues:

Let me verify Issue #8 more carefully and check for the definite bugs:

Now let me verify Bug #1 by looking at the exact line numbers in the diff:

Based on my thorough review of PR #3101, here's my analysis:

Review Summary

This PR implements Windows worker provisioning on native Linux hosts for K2s. It introduces significant new functionality with platform-specific configurations and complex provisioning scripts. Overall structure is sound, but I've identified critical bugs.

Critical Issues

❌ Bug 1: Undefined Variable Reference in NVRAM Path

File: lib/modules/linux/node/windows-worker.sh, line 195
Severity: High

nvram="$(dirname "$disk")/$K2S_WINDOWS_WORKER_NAME_VARS.fd"

This references $K2S_WINDOWS_WORKER_NAME_VARS, but the defined constant is K2S_WINDOWS_WORKER_NAME (line 10). The variable will expand to empty, resulting in a path like /path/to/vms/_VARS.fd instead of /path/to/vms/k2s-win-worker_VARS.fd. This breaks NVRAM file handling.

Fix: Change to:

nvram="$(dirname "$disk")/${K2S_WINDOWS_WORKER_NAME}_VARS.fd"

⚠️ Bug 2: Unescaped sed Delimiter Vulnerability

File: lib/modules/linux/node/windows-worker.sh, line 125
Severity: Medium

sed -i "s|@SSH_PUBLIC_KEY@|$public_key|" "$stage/bootstrap.ps1"

SSH public keys can theoretically contain a pipe character (|), which is the sed delimiter here. This would break the sed command. Use proper escaping or a safer substitution method.

⚠️ Bug 3: Missing findNodeByRole Method

File: cmd/k2s/cmd/install/config/config.go, line 239
Severity: Critical (compilation blocker)

The new GetOrCreateNodeByRole() function calls findNodeByRole(), which is not visible in the diff. If this helper method doesn't already exist in the codebase, the code will not compile. Verify that findNodeByRole() is defined in config.go.

Additional Observations

✅ Good: Platform-specific flag binding via build tags (flags_linux.go/flags_windows.go) cleanly separates concerns and prevents invalid flag combinations.

✅ Good: MAC address fix from invalid 52:54:00:k2:5w:01 to valid 52:54:00:25:57:01 is correct (previous address had invalid hex characters).

✅ Good: Proper error handling and validation in preflight checks with clear error messages.

✅ Good: SSH key permissions set correctly (600/644).

⚠️ Note: The bootstrap PowerShell script complexity is high but appears architecturally sound with proper error handling ($ErrorActionPreference = 'Stop').

---

Files Reviewed

• ✅ cmd/k2s/cmd/install/config/config.go
• ✅ cmd/k2s/cmd/install/config/config_test.go
• ✅ cmd/k2s/cmd/install/config/embed/k2s.config.yaml
• ✅ cmd/k2s/cmd/install/flags_linux.go
• ✅ cmd/k2s/cmd/install/flags_windows.go
• ✅ cmd/k2s/cmd/install/install.go
• ✅ docs/op-manual/installing-k2s.md
• ✅ internal/linux/lifecycle/dispatcher.go
• ✅ internal/provider/cluster.go
• ✅ internal/provider/cluster_linux.go
• ✅ internal/setuporchestration/libvirt_domain.xml.tmpl
• ✅ internal/setuporchestration/libvirt_template_linux_test.go
• ✅ internal/setuporchestration/network_linux.go
• ✅ internal/setuporchestration/vmmanager_linux.go
• ✅ lib/modules/linux/cluster/lifecycle.sh
• ✅ lib/modules/linux/infra/proxy.sh
• ✅ lib/modules/linux/node/windows-worker.sh
• ✅ lib/scripts/linux/debian/linuxonly/Install.sh
• ✅ lib/scripts/linux/debian/linuxonly/Start.sh
• ✅ lib/scripts/linux/debian/linuxonly/Stop.sh
• ✅ lib/scripts/linux/debian/linuxonly/Uninstall.sh
• ✅ test/native-linux-layout.sh