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

kubeadm: update the IsPriviligedUser preflight check on Windows #124665

Merged
merged 1 commit into from
May 2, 2024
Merged

kubeadm: update the IsPriviligedUser preflight check on Windows #124665

merged 1 commit into from
May 2, 2024

Conversation

neolit123
Copy link
Member

@neolit123 neolit123 commented May 2, 2024 •
edited

What type of PR is this?

What this PR does / why we need it:

Use GetCurrentProcessToken() instead of checking the groups of a user.

The Go stdlib way of fetching the groups of an user appears
to be failing on some Windows setups. Which could be a regression
in later Go versions, or simply the code does not work on certain
setups.

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#3053

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: improve the "IsPriviledgedUser" preflight check to not fail on certain Windows setups.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 2, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 2, 2024
@neolit123
Copy link
Member Author

/triage accepted
/priority backlog
/hold

/cc @jsturtevant @marosset
PTAL for LGTM

@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. triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 2, 2024
@neolit123 neolit123 mentioned this pull request May 2, 2024
Closed
@neolit123
Copy link
Member Author

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels May 2, 2024
@neolit123
Copy link
Member Author

pull-kubernetes-e2e-capz-windows-master — Job failed.   

but i don't see related errors to the preflight check IsPriviligedUser or this PR

https://gcsweb.k8s.io/gcs/kubernetes-jenkins/pr-logs/pull/124665/pull-kubernetes-e2e-capz-windows-master/1785959872680955904/artifacts/clusters/capz-conf-m8r2ay/machines/

@neolit123
Copy link
Member Author

/retest

jsturtevant
jsturtevant reviewed May 2, 2024
View reviewed changes
cmd/kubeadm/app/preflight/checks_windows.go Outdated
if err != nil {
return nil, []error{errors.Wrap(err, "cannot get group IDs for current user")}
var hProcessToken windows.Token
if err := windows.OpenProcessToken(windows.CurrentProcess(), windows.TOKEN_QUERY, &hProcessToken); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good idea to call token.close on this token or use https://pkg.go.dev/golang.org/x/sys/windows#GetCurrentProcessToken which states it doesn't need to be closed.

Copy link
Member Author

Choose a reason for hiding this comment

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

great, i didn't notice that function earlier.
updated.

@jsturtevant
Copy link
Contributor

Test passed the second time, I also saw that the Nodes came online the first time which means kubeadm worked.

@neolit123
kubeadm: update the IsPriviligedUser preflight check on Windows
d105ddd 
Use GetCurrentProcessToken() instead of checking the groups of a user.

The Go stdlib way of fetching the groups of an user appears
to be failing on some Windows setups. Which could be a regression
in later Go versions, or simply the code does not work on certain
setups.
@neolit123 neolit123 force-pushed the 1.31-fix-windows-priv-preflight-check branch from 66f157b to d105ddd Compare May 2, 2024 17:23
@jsturtevant
Copy link
Contributor

/lgtm

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

LGTM label has been added.

Git tree hash: 167d5d8028c8ccd4123f83e9f34b3fdebe6fa99d

@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 May 2, 2024
@k8s-ci-robot k8s-ci-robot merged commit 201e626 into kubernetes:master May 2, 2024
15 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.31 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsturtevant jsturtevant jsturtevant left review comments

@chendave chendave Awaiting requested review from chendave

@SataQiu SataQiu Awaiting requested review from SataQiu

@marosset marosset Awaiting requested review from marosset

Assignees

@jsturtevant jsturtevant

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm 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/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.31
Development

Successfully merging this pull request may close these issues.

kubeadm.exe fails on Windows Server 2022
3 participants
@neolit123 @k8s-ci-robot @jsturtevant