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

chore: Specify uid for consistent uids over images #4304

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
20 changes: 11 additions & 9 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,14 @@ RUN --mount=type=cache,target=/go/pkg/mod \
--mount=type=cache,target=/root/.cache/go-build \
CGO_ENABLED=0 go build -trimpath -ldflags "-s -w -X 'main.version=${ATLANTIS_VERSION}' -X 'main.commit=${ATLANTIS_COMMIT}' -X 'main.date=${ATLANTIS_DATE}'" -v -o atlantis .

FROM debian:${DEBIAN_TAG} as debian-base
FROM debian:${DEBIAN_TAG} AS debian-base

# Set up the 'atlantis' user and adjust permissions. User with uid 1000 is for backwards compatibility
RUN groupadd --gid 1000 atlantis && \
useradd --uid 100 --system --create-home --gid 1000 --shell /bin/bash atlantis && \
useradd --uid 1000 --system --home=/home/atlantis/ --gid 1000 --shell /bin/bash atlantis2 && \
chown atlantis:atlantis /home/atlantis/ && \
chmod ug+rwx /home/atlantis/

# Install packages needed to run Atlantis.
# We place this last as it will bust less docker layer caches when packages update
Expand All @@ -64,7 +71,7 @@ RUN apt-get update && \
apt-get clean && \
rm -rf /var/lib/apt/lists/*

FROM debian-base as deps
FROM debian-base AS deps

# Get the architecture the image is being built for
ARG TARGETPLATFORM
Expand Down Expand Up @@ -139,8 +146,8 @@ HEALTHCHECK --interval=5m --timeout=3s \
CMD curl -f http://localhost:${ATLANTIS_PORT:-4141}/healthz || exit 1

# Set up the 'atlantis' user and adjust permissions
RUN addgroup atlantis && \
adduser -S -G atlantis atlantis && \
RUN addgroup --gid 1000 atlantis && \
adduser -u 100 -S -G atlantis atlantis && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to discuss this.
We need to keep backward compatibility or make the breaking change.
A lot of people is already using this and if we change it it could bring some deployments down

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@jamengual jamengual Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The atlantis user does not need a shell to start, so a system user was used.
the problem is that in Alpine we have a different UUID

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. But not making it equal makes it harder to make the helm chart work for both. The helm chart defaults to 100 so that was what we were trying to ensure here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying this is wrong but changing it will break backwards compatibility

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A possible option is to add both users (100 and 1000) on debian. This way the helm chart will be correct and existing users won't have issues. Otherwise we will need to do a breaking change release indeed. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that could work

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like two maintainers agreed, so I'd say let's go with this. @kvanzuijlen can you please update it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! I'll have a look

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for alpine and should stay on 100, as it already was the default

chown atlantis:root /home/atlantis/ && \
chmod u+rwx /home/atlantis/
Comment on lines +149 to 152
Copy link

@arohter arohter May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dupe of line no. 50 RUN cmds?


Expand Down Expand Up @@ -179,11 +186,6 @@ EXPOSE ${ATLANTIS_PORT:-4141}
HEALTHCHECK --interval=5m --timeout=3s \
CMD curl -f http://localhost:${ATLANTIS_PORT:-4141}/healthz || exit 1

# Set up the 'atlantis' user and adjust permissions
RUN useradd --create-home --user-group --shell /bin/bash atlantis && \
chown atlantis:root /home/atlantis/ && \
chmod u+rwx /home/atlantis/

# copy atlantis binary
COPY --from=builder /app/atlantis /usr/local/bin/atlantis
# copy terraform binaries
Expand Down