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.

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

Kubelet broken uids #78178

Closed
wants to merge 1 commit into from
Closed

Conversation

@aholler
Copy link

aholler commented May 21, 2019

Revert #76665

Fix detection of container UID
…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
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
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
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

@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.

@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

@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.

@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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.