-
Notifications
You must be signed in to change notification settings - Fork 2k
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
whoami: cannot find name for user ID 101 after ssh-ing in pod container #4503
Comments
Hi @vepatel thanks for reporting! Be sure to check out the docs and the Contributing Guidelines while you wait for a human to take a look at this 🙂 Cheers! |
hey @sigv, any opinion or thoughts on this one? |
Taking a quick look, there do not appear to be relevant changes in this repository comparing
OSS
Base images did change:
But the base images are not responsible for the I have a suspicion about the Additionally, does this affect Debian only, or Alpine only, or other Plus variants? |
Alpine has BusyBox's
Debian has
@vepatel, I think the Nginx Plus Based on manpage: adduser will choose the first available UID from the range specified by FIRST_SYSTEM_UID and LAST_SYSTEM_UID in the configuration file. This can be overridden with the --uid option. This value defaults to Debian 11 image had UID 100 already taken by One dirty way how to mitigate this is running Therefore, I would propose to handle this by setting |
I opened #4540 with my proposed change to However, please check how the Nginx Plus Debian package creates the user, as you want I would suggest to change packaging for the Debian package, switching back to |
thanks @sigv, I'll have a look at the changes. |
Just debugged this because it was breaking app-protect after 3.2.1 which came down to a difference in R29 and R30. The main issue is that on 3.2.0 the nginx user is 101 while on 3.2.1 the nginx user is 999. For reference the debian package adds the nginx user if it does not exist. It looks like @sigv was right and it switched from adduser to useradd. R29
R30
|
Why do we not want to just add the user ourselves like we do for the UBI image? |
Generally there is a benefit in having the upstream package configure its expected environment -- you avoid repeating the same logic. But here it could make sense as to ensure the account is created with a specific UID. If creating the account in this chart's Dockerfile is preferred, then #4540 (configuring account ranges) is redundant and can be skipped. |
I think that we should explicity set the UID for all images otherwise we are relying on the package to do what we expect still. What if Alpine changes in the future? Again another breakage will happen. Explicit is better than implicit. |
Explicit is not always better. It's more about alignment, compromise and accepting risks. In the Debian Plus scenario, there are no notable drawbacks to creating the user early (before installing the package) -- it's just a bit longer command for the builder. At the same time, for Debian OSS and Alpine OSS images (built from |
This is not about the open source images. Those are not relevant here. Sure there are trade offs as always. The build time is negligible in this case. Broken releases and debug time aren't. If the user was explicitly set to begin with then we would not have upgraded to a broken version of nginx-ic-appprotect. Maybe other things broke because of this as well. |
Oh of course. I am just highlighting that reverting the packaging change and applying #4540 has its own benefits of re-using upstream an implementation. But doing explicit user/group creation in Dockerfile here is a way forward too! (The user creation doesn't slow anything down. I fully agree it's negligible from a size and speed point of view.) On a different topic though, it is interesting to me how this was not picked up in test scenarios, if app-protect is broken. |
I am an old systems person and not a fan of hard coding anything. It always returns to bite you in the end. |
I think we should stick to manually setting the kubernetes-ingress/build/Dockerfile Lines 156 to 157 in b9afaa7
|
Sounds like a plan. |
Should the same change be done for alpine as well? |
Describe the bug
To Reproduce
The text was updated successfully, but these errors were encountered: