Conversation
da8729d to
d877580
Compare
ci/Dockerfile-envoy-alpine-debug
Outdated
There was a problem hiding this comment.
I don't think you need update if you do add --no-cache below.
|
can you merge master? that will fix the CI |
|
done |
a26c616 to
b62d97b
Compare
ci/docker-entrypoint.sh
Outdated
There was a problem hiding this comment.
What if a user want to run Envoy as root? Non-root user cannot listen on well-known ports. Setting ENVOY_UID to 0 will fail at usermod.
There was a problem hiding this comment.
this has been updated, so if ENVOY_UID=0 it follows the existing exec codepath and avoids usermod, su-exec etc
337fa99 to
018d2bc
Compare
|
@lizan i have updated the PR and rebased to master. lmk if there are any further changes that you would like |
lizan
left a comment
There was a problem hiding this comment.
Thanks, this is in right direction. Can you add a release note as well? It should be noted that users have to set ENVOY_UID=0 to get old behavior.
|
release note added |
There was a problem hiding this comment.
alphabetical order, that's why format CI fails
mattklein123
left a comment
There was a problem hiding this comment.
Small question, thank you!
/wait
ci/docker-entrypoint.sh
Outdated
There was a problem hiding this comment.
What is the default here? If not set somehow by default should this default to "0"? I think by default we will enter this statement but the string will be empty?
There was a problem hiding this comment.
thats correct. The default is to use the envoy user with the uid/gid that is set at build time
There was a problem hiding this comment.
Where are these env variables set?
There was a problem hiding this comment.
they are runtime env vars for the container - so they can be set eg on the docker command line or in a compose file etc
There was a problem hiding this comment.
It's not clear that this now has to be set. Can we please change this so that the default stays wat it was and we document how to set the right variables? By default I don't think this makes sense.
There was a problem hiding this comment.
in the default case nothing should need to be set and all should work as it does now - save for the fact that process inside the container doesnt run as root.
The cases where you would want to set these vars are:
- opening well-known ports inside the container and running as root - containers kind of negate the need to ever do this
- setting a specific
uid/gidto get the correct permissions on in or out sockets
my own opinion is that it is better to not run as root by default. Im happy to make whatever changes others feel necessary though
There was a problem hiding this comment.
If both of these env vars are empty strings which would be the defualt, do the commands even work?
There was a problem hiding this comment.
yep for sure.
the default case is that ENVOY_UID is not set - ie also not set to 0
in this case it goes into this code block, but as ENVOY_UID is not set (same with gid) it goes straight to
su-exec envoy "${@}"
There was a problem hiding this comment.
Ah OK, gotcha. Alright thanks for the explanation. LGTM.
|
Oops sorry can you merge master? /wait |
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
|
done |
Signed-off-by: Ryan Northey <ryan@synca.io>
|
its not obvious to me why im wondering if i need to do anything to get tests passing |
|
@phlax no your fault, don't worry :) |
because we cannot bind to 80 port for envoy since envoyproxy/envoy#11323 envoyproxy/envoy#11506
Allow envoy to run as non-root user in Docker container. Signed-off-by: Ryan Northey <ryan@synca.io> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
Commit Message:
Allow envoy to run as non-root user in Docker container, Fixes #11311
Additional description:
su-execand requirements forusermodandgroupmodentrypointto usesu-execUnfortunately su-exec is not currently packaged in
debian, so i had to compile the binaryAn alternative would be to use
gosubut its signififcantly largerRisk Level: low/medium
The main risk is from compiling the
su-execbinary for thedebianbuildIts not clear to me if these recipes are intended for
ciorproduction- if the latter then introducingsu-exechas more significant security implicationsTesting:
Docs Changes: I could not find any docs relating to running with Docker
Release Notes: n/a