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

Conversation

kvanzuijlen
Copy link
Contributor

@kvanzuijlen kvanzuijlen commented Mar 1, 2024

what

This PR specifies the uid the atlantis user should be created with.

why

Together with runatlantis/helm-charts#359 this would fix runatlantis/helm-charts#305.

tests

references

@kvanzuijlen kvanzuijlen requested review from a team as code owners March 1, 2024 13:59
@kvanzuijlen kvanzuijlen requested review from jamengual, nitrocode and X-Guardian and removed request for a team March 1, 2024 13:59
@github-actions github-actions bot added the build Relating to how we build Atlantis label Mar 1, 2024
Dockerfile Outdated Show resolved Hide resolved
@jamengual jamengual added the needs discussion Large change that needs review from community/maintainers label Mar 1, 2024
@@ -143,7 +148,7 @@ HEALTHCHECK --interval=5m --timeout=3s \

# Set up the 'atlantis' user and adjust permissions
RUN addgroup atlantis && \
adduser -S -G atlantis 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

Dockerfile Outdated
Comment on lines 46 to 50
# Set up the 'atlantis' user and adjust permissions. User with uid 1000 is for backwards compatibility
RUN useradd --uid 100 --system --create-home --user-group --shell /bin/bash atlantis && \
useradd --uid 1000 --system --home=/home/atlantis/ --groups atlantis --shell /bin/bash atlantis2 && \
chown atlantis:atlantis /home/atlantis/ && \
chmod ug+rwx /home/atlantis/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole Atlantis group has the same access as the Atlantis user. I added an atlantis2 user with uid 1000 for backward compatibility. The root group is no longer included in the chown, but that shouldn't matter too much as the group had the same permissions as other. Only in cases where end-users configured a user that is part of the root group and changed the permissions on this folder would this be a breaking change, which I think is an acceptable scenario.

@@ -143,7 +148,7 @@ HEALTHCHECK --interval=5m --timeout=3s \

# Set up the 'atlantis' user and adjust permissions
RUN addgroup atlantis && \
adduser -S -G atlantis atlantis && \
adduser -u 100 -S -G atlantis atlantis && \
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

@kvanzuijlen
Copy link
Contributor Author

@GMartinez-Sisti can you take a look?

Copy link
Member

@GMartinez-Sisti GMartinez-Sisti left a comment

Choose a reason for hiding this comment

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

Adding both 100 and 1000 to keep compatibility looks like a good solution. Can I get another maintainer approval so we can merge please?

Dockerfile Outdated Show resolved Hide resolved
Comment on lines +149 to 152
RUN addgroup --gid 1000 atlantis && \
adduser -u 100 -S -G atlantis atlantis && \
chown atlantis:root /home/atlantis/ && \
chmod u+rwx /home/atlantis/
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?

@GenPage
Copy link
Member

GenPage commented Jun 26, 2024

@lukemassa @chenrui333 are either of you able to review this?

@GenPage GenPage added the waiting-on-review Waiting for a review from a maintainer label Jun 26, 2024
@lukemassa
Copy link
Contributor

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change build Relating to how we build Atlantis needs discussion Large change that needs review from community/maintainers waiting-on-review Waiting for a review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default uuid wrong if using debian image
6 participants