Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Windows: do not use 'uname' for doing OS validation #40

Merged
merged 1 commit into from
Sep 9, 2024
Merged

Windows: do not use 'uname' for doing OS validation #40

merged 1 commit into from
Sep 9, 2024

Conversation

neolit123
Copy link
Member

@neolit123 neolit123 commented Sep 6, 2024 •
edited
Loading

The idea of the OS validator is to check if the host OS is supported. e.g. 'uname' would return 'Linux' on Linux and something else / unsupported on OSX, BSD.

But 'uname' is a Unix tool, and only available on Windows if third party packages are installed like MSYS.

  • Split the os_validator*.go files for Unix and Windows.
  • Align the fetching of OS name and kernel version for Windows with the kubelet which uses the CurrentVersion reg key under HKLM. check the code here
  • In the Windows spec use 'Windows Server' as the required name and match it in 'CurrentVersion.ProductName' as a prefix.
  • Add OS specific unit tests.

fixes #39

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 6, 2024
@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Sep 6, 2024
@k8s-ci-robot k8s-ci-robot requested a review from odinuge September 6, 2024 10:23
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 6, 2024
@neolit123
Copy link
Member Author

/kind bug
/priority important-longterm
/hold for review

/cc @pacoxu @jsturtevant

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Sep 6, 2024
@neolit123 neolit123 mentioned this pull request Sep 6, 2024
Closed
@neolit123
Copy link
Member Author

/remove-sig node
/sig windows

@k8s-ci-robot k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. and removed sig/node Categorizes an issue or PR as relevant to SIG Node. labels Sep 6, 2024
@neolit123 neolit123 force-pushed the 1.32-fix-os-validation-windows branch from 4152641 to 5541cc3 Compare September 6, 2024 10:56
@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Sep 6, 2024
@neolit123
Copy link
Member Author

note: i ran the _windows unit tests locally on WSL
sadly golang still does not support cross-running UT for another OS.

@neolit123 neolit123 force-pushed the 1.32-fix-os-validation-windows branch 2 times, most recently from ea6b5ef to f070f22 Compare September 6, 2024 11:10
@jsturtevant
Copy link

ran the tests on my Windows 10 machine:

cmd /c ver

Microsoft Windows [Version 10.0.22631.4037]

go test .\validators\
ok      k8s.io/system-validators/validators     1.602s

jsturtevant
jsturtevant approved these changes Sep 6, 2024
View reviewed changes
Copy link

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2024
pacoxu
pacoxu reviewed Sep 9, 2024
View reviewed changes
Copy link
Member

@pacoxu pacoxu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/lgtm

https://github.com/kubernetes/kubernetes/blame/1dbf0830c59038822f37eaf5a1d4f7583f1a1081/cluster/gce/windows/k8s-node-setup.psm1#L139-L149

another similar powershell

@neolit123
Windows: do not use 'uname' for doing OS validation
9435f6c 
The idea of the OS validator is to check if the host OS is supported.
e.g. 'uname' would return 'Linux' on Linux and something else on
OSX, BSD.

But 'uname' is a Unix tool, and only available on Windows if third
party packages are installed like MSYS.

- Split the os_validator*.go files for Unix and Windows.
- Align the fetching of OS name and kernel version for Windows
with the kubelet which uses the CurrentVersion reg key under HKLM.
- In the Windows spec use 'Windows Server' as the required name
and match it in 'CurrentVersion.ProductName' as a prefix.
- Add OS specific unit tests.
@neolit123 neolit123 force-pushed the 1.32-fix-os-validation-windows branch from f070f22 to 9435f6c Compare September 9, 2024 07:48
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2024
neolit123
neolit123 commented Sep 9, 2024
View reviewed changes
Copy link
Member Author

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

@pacoxu @jsturtevant
i pushed a small fix for wrong year in license header
https://github.com/kubernetes/system-validators/compare/f070f228e2386f604341c48b5072c1d0a38b4404..9435f6c43ece976ea9deac83a001108b4846b88e

please re-apply lgtm.

@pacoxu
Copy link
Member

pacoxu commented Sep 9, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsturtevant, neolit123, pacoxu

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@neolit123
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2024
@k8s-ci-robot k8s-ci-robot merged commit 1d2f588 into kubernetes:master Sep 9, 2024
2 of 3 checks passed
@neolit123 neolit123 mentioned this pull request Sep 10, 2024
Open
17 tasks
@pacoxu pacoxu mentioned this pull request Sep 12, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pacoxu pacoxu pacoxu left review comments

@jsturtevant jsturtevant jsturtevant approved these changes

Assignees

@jsturtevant jsturtevant

@pacoxu pacoxu

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
[sig-windows] PR tracking
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OSValidator doesn't work on Windows
4 participants
@neolit123 @jsturtevant @pacoxu @k8s-ci-robot