Skip to content

Kubelet broken uids #78178

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
Closed
wants to merge 1 commit into from
Closed

Kubelet broken uids #78178

wants to merge 1 commit into from

Conversation

@aholler
Copy link

@aholler aholler commented May 21, 2019 •
edited by tallclair
Loading

Revert #76665

Fix detection of container UID

@aholler
Kubelet: fix uid detection by reverting "Use consistent imageRef duri…
26127cd 
…ng container startup"

This reverts commit 5769863.

The reverted commit breaks uid detection and images which are using a
different uid than 0 will run as root (0).

E.g. in my case many things just were broken because I like to use pod
security policies and all the non-root-images did not start after the
update to v1.14.2 with the error message

    Error: container has runAsNonRoot and image will run as root

Workaround is to use a Security Context which defines the uid to use so
that pods don't depend on the USER defined in the images.
@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 21, 2019
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 21, 2019

Keywords which can automatically close issues and at(@) mentions are not allowed in commit messages.

The list of commits with invalid commit messages:

  • 3acb81c Fix Windows to read VM UUIDs from serial numbers
  • fdd0a45 vSphere: add token auth support for tags client
  • c6bb01d Finish saving test results on failure
  • d399072 Bump coreos/go-semver
  • 9141812 Fix concurrent map access in Portworx create volume call

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels May 21, 2019
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 21, 2019

Hi @aholler. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 21, 2019
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 21, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aholler
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: smarterclayton

If they are not already assigned, you can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

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 area/apiserver area/cloudprovider area/conformance Issues or PRs related to kubernetes conformance tests area/kubeadm area/kubectl area/kubelet area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/gcp sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels May 21, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 22, 2019
@aholler
Copy link
Author

aholler commented May 22, 2019

This is not a cherry-pick.

This is just a revert of the commit which caused the breakage in 1,14.2.

As said before, just by looking at the little commit which caused the breakage in 1.14.2, I can't say if it also causes the same breakage in the master. And I don't have a cluster > 1.14.2 to test.

I've tested and using a kubelet 1.14.2 with the reverted commit and it works, but besides that I can't comment on what happens in the master.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 22, 2019
@mattjmcnaughton
Copy link
Contributor

mattjmcnaughton commented May 23, 2019

This is not a cherry-pick.

This is just a revert of the commit which caused the breakage in 1,14.2.

As said before, just by looking at the little commit which caused the breakage in 1.14.2, I can't say if it also causes the same breakage in the master. And I don't have a cluster > 1.14.2 to test.

I've tested and using a kubelet 1.14.2 with the reverted commit and it works, but besides that I can't comment on what happens in the master.

Thanks for the additional info @aholler! My understanding is that if we decide to move forward with this change, we'd want to first verify whether its currently broken on master, and if yes, start by merging a fix into master and then cherry-picking it.

I'm happy to do this investigation re the current state of master if you'd like :) I could create a pr for master and then contact you when its time for the cherry-pick to 1.14?

@aholler
Copy link
Author

aholler commented May 23, 2019

@mattjmcnaughton
Thanks for offering help and feel free do whatever you think makes sense to move things forward.
Actually I don't really care if the PR will be accepted, I just did it to give other people an easy to use patch and because I believe some code with a patch description perfectly describes the problem.

Besides that I thought the author (tallclair) might see right out of the box what the real problem is and if it might be already fixed in the master.

Just to add some note, kubelet 1.14.2 will currently break almost any (1.14) cluster where Pod Security Policies are used.

@tallclair
Copy link
Member

tallclair commented May 23, 2019

Opened #78261 for master

@roycaihw
Copy link
Member

roycaihw commented May 23, 2019

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 23, 2019
@liggitt liggitt removed area/apiserver area/cloudprovider area/conformance Issues or PRs related to kubernetes conformance tests area/kubeadm area/kubectl area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/gcp sig/network Categorizes an issue or PR as relevant to SIG Network. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels May 23, 2019
@verwilst
Copy link

verwilst commented May 24, 2019

#78261 has been merged. Could this be backported to 1.14 pretty please?

@tallclair
Copy link
Member

tallclair commented May 24, 2019

Closing in favor of #78316, which follows the cherrypick process.

@tallclair tallclair closed this May 24, 2019
@jmccormick2001 jmccormick2001 mentioned this pull request Jun 4, 2019
Closed
@aholler
Copy link
Author

aholler commented Jun 7, 2019

Thanks a lot for fixing the bug and writing a test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BenTheElder BenTheElder Awaiting requested review from BenTheElder

@cheftako cheftako Awaiting requested review from cheftako

@tallclair tallclair Awaiting requested review from tallclair

Assignees

No one assigned

Labels

area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. kind/bug Categorizes issue or PR as related to a bug. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@aholler @k8s-ci-robot @mattjmcnaughton @tallclair @roycaihw @verwilst @liggitt