Remove user privilege checks from preflight tests #128799

srivastav-abhishek · 2024-11-14T15:30:52Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR removes user-privilege checks for some preflight tests to make sure UT behaviour is consistent and shouldn't get skipped for non-root users.

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#3124

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

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

srivastav-abhishek · 2024-11-14T15:36:36Z

/cc @neolit123 @mkumatag @Rajalakshmi-Girish

neolit123 · 2024-11-14T15:41:50Z

cmd/kubeadm/app/preflight/checks.go

 	manifestsDir := filepath.Join(kubeadmconstants.KubernetesDir, kubeadmconstants.ManifestsSubDirName)
 	checks := []Checker{
+		IsPrivilegedUserCheck{},


it should be added only when this condition is true

if !isSecondaryControlPlane {

just create the checks variable earlier:

var checks []Checker

and keep the condition.

later checks must be appended.

neolit123 · 2024-11-14T15:42:57Z

thanks for the PR, i've added one comment.
/triage accepted
/priority backlog

srivastav-abhishek · 2024-11-15T06:03:14Z

/retest

neolit123

LGTM, running e2e on this PR here:
https://github.com/neolit123/kubeadm-test/actions/runs/11853015645

will also test locally as it's a behavior change.

we might still consider the idea to calling RunRootCheckOnly before Init|Join checks are called and exiting early.

neolit123 · 2024-11-15T08:53:49Z

@SataQiu @pacoxu @carlory

before this change kubeadm exits fast on init/join when the user is not root

$ kubeadm init
[init] Using Kubernetes version: v1.31.2
[preflight] Running pre-flight checks
error execution phase preflight: [preflight] Some fatal errors occurred:
        [ERROR IsPrivilegedUser]: user is not running as root
[preflight] If you know what you are doing, you can make a check non-fatal with `--ignore-preflight-errors=...`
To see the stack trace of this error execute with --v=5 or higher

with this change it runs some more checks before exiting with an error:

$ kubeadm init
[init] Using Kubernetes version: v1.31.2
[preflight] Running pre-flight checks
W1115 10:47:13.259101   12639 checks.go:1075] [preflight] WARNING: Couldn't create the interface used for talking to the container runtime: failed to create new CRI runtime service: validate service connection: validate CRI v1 runtime API for endpoint "unix:///var/run/containerd/containerd.sock": rpc error: code = Unavailable desc = connection error: desc = "transport: Error while dialing: dial unix /var/run/containerd/containerd.sock: connect: permission denied"
        [WARNING FileExisting-ethtool]: ethtool not found in system path
        [WARNING SystemVerification]: cgroups v1 support is in maintenance mode, please migrate to cgroups v2
error execution phase preflight: [preflight] Some fatal errors occurred:
        [ERROR IsPrivilegedUser]: user is not running as root
        [ERROR KubeletVersion]: couldn't get kubelet version: cannot execute 'kubelet --version': executable file not found in $PATH
        [ERROR DirAvailable--var-lib-etcd]: unable to check if /var/lib/etcd is empty: open /var/lib/etcd: permission denied
[preflight] If you know what you are doing, you can make a check non-fatal with `--ignore-preflight-errors=...`
To see the stack trace of this error execute with --v=5 or higher

do you think we should preserve exactly the old behavior by calling the root check before any init/join checks?
we are doing that on upgrade node/apply too, so might be better to be consistent.

pacoxu · 2024-11-15T09:06:35Z

+1 for this change. A consistent behavior makes sense.

BTW, if we can show the [ERROR IsPrivilegedUser]: user is not running as root more clear(already the first ERROR) in the list, it would be better and easier for users to notice the root cause. Should the errors be printed before the warnings?

neolit123 · 2024-11-15T09:55:05Z

BTW, if we can show the [ERROR IsPrivilegedUser]: user is not running as root more clear(already the first ERROR) in the list, it would be better and easier for users to notice the root cause.

maybe the best to do that is to run this check with RunRootCheckOnly first, like it was before this PR.

like in upgrade
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/cmd/phases/upgrade/apply/preflight.go#L71-L85

. Should the errors be printed before the warnings?

i'm +0 on this one, we could log a ticket about it.

srivastav-abhishek · 2024-11-15T10:42:54Z

maybe the best to do that is to run this check with RunRootCheckOnly first, like it was before this PR.

I shall revert the changes in checks.go then.

neolit123 · 2024-11-15T10:53:18Z

maybe the best to do that is to run this check with RunRootCheckOnly first, like it was before this PR.

I shall revert the changes in checks.go then.

you can do the following:

remove anything about the priv check in the init|join check functions
call the runrootchecksonly function in all locations before the init|join functions. but also consider the isSecondaryControlPlane flag.

neolit123 · 2024-11-15T12:50:53Z

cmd/kubeadm/app/preflight/checks.go

+		if err := RunRootCheckOnly(ignorePreflightErrors); err != nil {
+			return err
+		}
+	}


instead of calling the root check inside the runinit|joinnodechecks we should call RunRootCheckOnly in these places:

grep -E 'RunJoinNodeChecks|RunInitNodeChecks' * -rnI | grep -v chec ks.go app/cmd/phases/join/preflight.go:94: if err := preflight.RunJoinNodeChecks(utilsexec.New(), j.Cfg(), j.IgnorePreflightErrors()); err != nil { app/cmd/phases/join/preflight.go:123: if err := preflight.RunInitNodeChecks(utilsexec.New(), initCfg, j.IgnorePreflightErrors(), true, hasCertificateKey); err != nil { app/cmd/phases/init/preflight.go:65: if err := preflight.RunInitNodeChecks(utilsexec.New(), data.Cfg(), data.IgnorePreflightErrors(), false, false); err != nil {

before the run init/join checks are called.

the flag isSecondaryControlPlane should be removed from RunInitNodeChecks

actually
app/cmd/phases/join/preflight.go:123 - should not call RunRootCheck
app/cmd/phases/init/preflight.go:65 - should call RunRootCheck

this is confirmed by the flag values which are passed ( isSecondaryControlPlane)

Alright, I called in the sub-functions.

the way you did it is also OK, but i think we should be more explicit about the runned root checks, similarly to how kubeadm upgrade and reset does it.

Updated. It seems it is not required for

app/cmd/phases/join/preflight.go:123: if err := preflight.RunInitNodeChecks(utilsexec.New(), initCfg, j.IgnorePreflightErrors(), true, hasCertificateKey); err != nil {

as isSecondaryControlPlane is true here.

neolit123 · 2024-11-15T14:00:44Z

cmd/kubeadm/app/preflight/checks.go

@@ -933,7 +926,6 @@ func InitNodeChecks(execer utilsexec.Interface, cfg *kubeadmapi.InitConfiguratio
 		FileAvailableCheck{Path: kubeadmconstants.GetStaticPodFilepath(kubeadmconstants.Etcd, manifestsDir)},
 		HTTPProxyCheck{Proto: "https", Host: cfg.LocalAPIEndpoint.AdvertiseAddress},
 	}
-


don't remove line.

neolit123

/approve
just one more minor line nit.

k8s-ci-robot · 2024-11-15T14:03:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, srivastav-abhishek

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:

~~cmd/kubeadm/OWNERS~~ [neolit123]

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

neolit123

CI is passing
https://github.com/neolit123/kubeadm-test/actions/runs/11853015645

/lgtm

we are in code freeze for 1.32 so this will automatically merge after CF is lifted.

k8s-ci-robot · 2024-11-15T15:04:05Z

LGTM label has been added.

Git tree hash: c739bf0fc621486c89ec9ea1176e4d8a76129358

neolit123 · 2024-11-15T15:04:11Z

thanks for the PR @srivastav-abhishek

srivastav-abhishek · 2024-11-15T15:42:35Z

/retest

pacoxu · 2024-11-18T09:51:29Z

/lgtm

k8s-ci-robot requested review from neolit123 and SataQiu November 14, 2024 15:33

k8s-ci-robot added 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 Nov 14, 2024

k8s-ci-robot requested review from mkumatag and Rajalakshmi-Girish November 14, 2024 15:36

neolit123 reviewed Nov 14, 2024

View reviewed changes

srivastav-abhishek force-pushed the fix-preflight-tests branch from 296a92c to c3060ac Compare November 15, 2024 04:31

neolit123 reviewed Nov 15, 2024

View reviewed changes

srivastav-abhishek force-pushed the fix-preflight-tests branch from c3060ac to 93ca85b Compare November 15, 2024 12:40

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 15, 2024

neolit123 reviewed Nov 15, 2024

View reviewed changes

srivastav-abhishek force-pushed the fix-preflight-tests branch from 93ca85b to 9d0361a Compare November 15, 2024 13:09

neolit123 reviewed Nov 15, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2024

Remove user privilege checks from preflight tests

179fa16

srivastav-abhishek force-pushed the fix-preflight-tests branch from 9d0361a to 179fa16 Compare November 15, 2024 14:23

neolit123 reviewed Nov 15, 2024

View reviewed changes

k8s-ci-robot assigned neolit123 Nov 15, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2024

k8s-ci-robot assigned pacoxu Nov 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove user privilege checks from preflight tests #128799

Remove user privilege checks from preflight tests #128799

srivastav-abhishek commented Nov 14, 2024 •

edited

Loading

srivastav-abhishek commented Nov 14, 2024

neolit123 Nov 14, 2024 •

edited

Loading

neolit123 commented Nov 14, 2024

srivastav-abhishek commented Nov 15, 2024

neolit123 left a comment

neolit123 commented Nov 15, 2024 •

edited

Loading

pacoxu commented Nov 15, 2024

neolit123 commented Nov 15, 2024 •

edited

Loading

srivastav-abhishek commented Nov 15, 2024 •

edited

Loading

neolit123 commented Nov 15, 2024

neolit123 Nov 15, 2024

neolit123 Nov 15, 2024

srivastav-abhishek Nov 15, 2024

neolit123 Nov 15, 2024

srivastav-abhishek Nov 15, 2024

neolit123 Nov 15, 2024

neolit123 left a comment

k8s-ci-robot commented Nov 15, 2024

neolit123 left a comment

k8s-ci-robot commented Nov 15, 2024

neolit123 commented Nov 15, 2024

srivastav-abhishek commented Nov 15, 2024

pacoxu commented Nov 18, 2024

Remove user privilege checks from preflight tests #128799

Are you sure you want to change the base?

Remove user privilege checks from preflight tests #128799

Conversation

srivastav-abhishek commented Nov 14, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

srivastav-abhishek commented Nov 14, 2024

neolit123 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

neolit123 commented Nov 14, 2024

srivastav-abhishek commented Nov 15, 2024

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Nov 15, 2024 • edited Loading

pacoxu commented Nov 15, 2024

neolit123 commented Nov 15, 2024 • edited Loading

srivastav-abhishek commented Nov 15, 2024 • edited Loading

neolit123 commented Nov 15, 2024

neolit123 Nov 15, 2024

Choose a reason for hiding this comment

neolit123 Nov 15, 2024

Choose a reason for hiding this comment

srivastav-abhishek Nov 15, 2024

Choose a reason for hiding this comment

neolit123 Nov 15, 2024

Choose a reason for hiding this comment

srivastav-abhishek Nov 15, 2024

Choose a reason for hiding this comment

neolit123 Nov 15, 2024

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 15, 2024

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 15, 2024

neolit123 commented Nov 15, 2024

srivastav-abhishek commented Nov 15, 2024

pacoxu commented Nov 18, 2024

srivastav-abhishek commented Nov 14, 2024 •

edited

Loading

neolit123 Nov 14, 2024 •

edited

Loading

neolit123 commented Nov 15, 2024 •

edited

Loading

neolit123 commented Nov 15, 2024 •

edited

Loading

srivastav-abhishek commented Nov 15, 2024 •

edited

Loading