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

[Kubelet] Check if kubelet is running as uid 0 #30466

Merged
merged 1 commit into from
Aug 15, 2016
Merged

[Kubelet] Check if kubelet is running as uid 0 #30466

merged 1 commit into from
Aug 15, 2016
+12 −10

Conversation

vishh
Copy link
Contributor

@vishh vishh commented Aug 11, 2016 •
edited
Loading

Related to #30176

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Aug 11, 2016
@vishh vishh added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 11, 2016
@vishh vishh changed the title Check if kubelet is running as uid 0. Avoid checking capabilities since access to filesystem is typically not managed via Caps. Check if kubelet is running as uid 0. Aug 11, 2016
@vishh vishh changed the title Check if kubelet is running as uid 0. [Kubelet] Check if kubelet is running as uid 0 Aug 11, 2016
@derekwaynecarr
Copy link
Member

@vishh - is this a breaking change? should this be an OR check?

@smarterclayton @kubernetes/rh-cluster-infra - any concerns here?

@vishh
Copy link
Contributor Author

vishh commented Aug 11, 2016

@derekwaynecarr
This could be potentially breaking based on the deployment configuration. AFAIK, we have never documented how to run kubelet with uid != 0.

As for an OR check, what is the alternative?

@ncdc
Copy link
Member

ncdc commented Aug 11, 2016

AFAIK we don't have any deployments where the OpenShift node is run with a nonzero UID

@smarterclayton
Copy link
Contributor

smarterclayton commented Aug 12, 2016 via email

@dims
Copy link
Member

dims commented Aug 12, 2016

@vishh as i mentioned on IRC yesterday,

Note that kubelet is started inside test-cmd.sh and hence we need to run test-cmd.sh as sudo to get past the test failure

kubernetes/hack/make-rules/test-cmd.sh

Line 179 in e877a23

kube::log::status "Starting kubelet in masterless mode"

note that the test-cmd.sh is run inside a docker container (where sudo is not available).

kubernetes/hack/jenkins/gotest-dockerized.sh

Line 49 in d4691a7

bash -c "cd kubernetes && ${KUBE_TEST_SCRIPT:-./hack/jenkins/test-dockerized.sh}"

@dims
Copy link
Member

dims commented Aug 12, 2016

Oh, another one, Will this (check for uid==0) work for folks running "hyperkube kubectl" (#4869) or minikube(?) ?

@derekwaynecarr
Copy link
Member

Can you not run as uid=0 with dropped capabilities?

@k8s-github-robot k8s-github-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 Aug 12, 2016
@vishh
Copy link
Contributor Author

vishh commented Aug 12, 2016

Can you not run as uid=0 with dropped capabilities?

I originally had a section in this PR that validated the permitted and effective caps too. But in reality caps do not typically apply to filesystem access. Hence decided to drop it. If you feel strongly that validating caps is useful, I can add it back in.

@vishh
check if kubelet is running as uid 0
c75b61e 
Signed-off-by: Vishnu kannan <vishnuk@google.com>
@vishh
Copy link
Contributor Author

vishh commented Aug 12, 2016

@derekwaynecarr @dims Integration tests seem to run kubelet not as root. I don't have cycles to fix that. For now, I'm throwing an error and reverting @dims changes! If either of you have spare cycles to clean up the integration test infra to allow kubelet to run as root, kindly post a PR.

This PR will now not cause a regression!

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 12, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 12, 2016

GCE e2e build/test passed for commit c75b61e.

@k8s-github-robot k8s-github-robot mentioned this pull request Aug 12, 2016
Closed
@dims
Copy link
Member

dims commented Aug 14, 2016

Looks good to me 👍

@vishh
Copy link
Contributor Author

vishh commented Aug 15, 2016

Thanks for the review! Given that this PR is non-intrusive, I'm marking it as LGTM. I'm happy to address comments in a separate PR. This should at-least fix HEAD.

@vishh vishh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2016
@vishh vishh added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Aug 15, 2016
@derekwaynecarr
Copy link
Member

LGTM as well (logging the error, but proceeding)

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 15, 2016

GCE e2e build/test passed for commit c75b61e.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 921c460 into kubernetes:master Aug 15, 2016
@luxas luxas mentioned this pull request Aug 16, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dims dims

@derekwaynecarr derekwaynecarr

@dchen1107 dchen1107

Labels
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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@vishh @derekwaynecarr @ncdc @smarterclayton @dims @k8s-bot @k8s-github-robot @googlebot @dchen1107