-
Notifications
You must be signed in to change notification settings - Fork 51
Added alpine3.14-nr (non root user) #71
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
Conversation
Based on @tunnelpr0 PR #43 Signed-off-by: Ivan Kozlovic <[email protected]>
@variadico I added a directory for alpine3.14 with the "no root" addition proposed by @tunnelpr0 in PR #43. I will have a PR for docker's official-images pointing to this branch to see if that would work ok. |
echo "${sha256} *nats-server.tar.gz" | sha256sum -c -; \ | ||
\ | ||
apk add --no-cache ca-certificates; \ | ||
apk add --no-cache --virtual buildtmp; \ |
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.
This --virtual
isn't doing anything; its apk add --virtual foo bar baz
to create foo
as an alternative to world
, and add bar
and baz
to foo
, so that you can later delete foo
to delete both bar
and baz
. A virtual with no packages in it is ... doing nothing.
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.
I am not familiar with that and is there in existing alpine docker file, so will have to look into it more.
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.
I think this is a bug.
This was probably the intention.
Oh, wait. Actually, I think at some point this line was something like this.
apk add --no-cache --virtual buildtmp unzip;
But then we removed a dependency on zip and just used tar. So during the refactor, only unzip was removed, instead of the whole line.
Ah, here's the change. Yep, I should have removed the whole line during this change.
1eb49a4?diff=unified#diff-f5513fbb12ca460ce02949e86e280ed0597a9c35dfe0599a43f09f10fb93dc23L20
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.
So if we remove this line apk add --no-cache --virtual buildtmp;
, then we remove buildtmp
from that line:
apk del --no-cache --no-network buildtmp
right?
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.
We have to remove 2 lines:
wget -O nats-server.tar.gz "https://github.com/nats-io/nats-server/releases/download/v${NATS_SERVER}/nats-server-v${NATS_SERVER}-linux-${natsArch}.tar.gz"; \
echo "${sha256} *nats-server.tar.gz" | sha256sum -c -; \
\
apk add --no-cache ca-certificates; \
- apk add --no-cache --virtual buildtmp; \
\
tar -xf nats-server.tar.gz; \
rm nats-server.tar.gz; \
mv "nats-server-v${NATS_SERVER}-linux-${natsArch}/nats-server" /usr/local/bin; \
+ rm -rf "nats-server-v${NATS_SERVER}-linux-${natsArch}"
- rm -rf "nats-server-v${NATS_SERVER}-linux-${natsArch}"; \
- \
- apk del --no-cache --no-network buildtmp
Remove these two lines:
apk add --no-cache --virtual buildtmp
apk del --no-cache --no-network buildtmp
(this will fail if buildtmp doesn't exist)
|
||
COPY nats-server.conf /etc/nats/nats-server.conf | ||
COPY docker-entrypoint.sh /usr/local/bin | ||
EXPOSE 4222 8222 6222 |
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.
Are we expecting to add 443 to this, for the websockets support?
If we add websockets, how would that interact with rootless? Would we use something like 1433 instead?
(Sorry for asking about "not in the PR" as part of the PR review, but it's a natural consequence of an interaction between a feature and the proposed change)
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.
EXPOSE is not really necessary anyway. For instance, I can start the image with -p 443:443
and have the image use a local config file that has websocket enabled and then I can connect my app.
Of course, we could expose all ports that we think users may configure with their own config file (since the one we provide simply has client/http/route), but then what if they want their own port numbers anyway?
COPY nats-server.conf /etc/nats/nats-server.conf | ||
COPY docker-entrypoint.sh /usr/local/bin | ||
EXPOSE 4222 8222 6222 | ||
RUN adduser -g '' -h / -s /bin/ash -H -D nats |
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.
The built-in shell being ash
is an implementation detail waiting to bite us if that changes, this should just be /bin/sh
.
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.
Note, but this may be moot point anyway since from docker's official image PR I posted there, this current approach does not seem to be the good one.
Closing as stale. @kozlovic or @philpennock feel free to re-open if there is value to this beyond the |
Based on @tunnelpr0 PR #43
Signed-off-by: Ivan Kozlovic [email protected]