-
Notifications
You must be signed in to change notification settings - Fork 188
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
Make image be rootless #86
Conversation
c67b951
to
249d2fb
Compare
fc59037
to
9b2bb27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is removing the whole vendor/
directory, could you also remove references to said directory from Makefile?
#87 is merged now, could you rebase? Thank you for this enhancement! 💯 |
55aea8f
to
4171951
Compare
It seems that an e2e test scenario using deployment files from https://github.com/brancz/kube-rbac-proxy/tree/master/test/e2e/allowpaths is failing in CI. Pod is failing to start due to:
|
8c843ca
to
feca0a7
Compare
@camilamacedo86 Is there something left for this PR? |
hi @paulfantom, Thank you for your support and patient. I want to look better on how we could update the manifests as well. However, I need to stop to work on it. It is fine now and can get merged. Could you also to do. new release after this merge? |
@camilamacedo86 I tried this branch and I get this error - |
hi @harpratap, I am unable to reproduce the error. could you please let us know how are you doing this test? also, these changes have been passing in these tests and against OCP as well. See: openshift#29. However, would be nice to know more about how are you testing it. |
@harpratap ping |
@camilamacedo86 I'm trying out with this role & psp, forcing to user 65532 fixes it. Can check the error message in events when doing |
Hi @harpratap, Shows that the problem is because you are not using the changes made in the manifests. See: https://github.com/brancz/kube-rbac-proxy/pull/86/files#diff-c669d94f3867a7c8d5a54532c08e029bR61-R77. You came with this USER ID because of you check it out in https://github.com/GoogleContainerTools/distroless/blob/master/base/base.bzl#L7. But it is only a value for the const. We do not need to use this value to be able to use the distroless image. |
@camilamacedo86 I did try again after adding |
This seems to be a known issue in distroless:nonroot with containerd. It isn't related to this PR so this should be good to go, I'll watch for fixes in upstream. Thanks everyone! |
Hi @harpratap, could we get it merged and do a release with it? Also, just use the ID 65532 solved the problem for containerd/cri#1397 I have no objections to change it here as well. I will do that . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM too, thank you! |
Description
Motivation