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

docs: Explicit restrictions of atlantis user apply to the --data-dir flag #2915

Merged
merged 6 commits into from
Jan 4, 2023
Merged

docs: Explicit restrictions of atlantis user apply to the --data-dir flag #2915

merged 6 commits into from
Jan 4, 2023

Conversation

bschaatsbergen
Copy link
Member

@bschaatsbergen bschaatsbergen commented Jan 3, 2023

what

As the atlantis user is restricted to the home directory of atlantis (/home/atlantis), setting the --data-dir flag to another path will result in a permission denied error.

I've updated the documentation to be more explicit on this common pitfall.

why

This is a common made mistake as we nowhere document how restricted the atlantis user is, I had to look at the docker-base to understand why, what and where the atlantis user permissions were applied.

tests

references

@bschaatsbergen bschaatsbergen requested a review from a team as a code owner January 3, 2023 21:09
@bschaatsbergen
Copy link
Member Author

Previous branch was branched from master, I suppose that's why the circle-ci job failed..

@nitrocode
Copy link
Member

Please do not close the old PR and open a new one just to resolve a test issue. This adds to the notifications.

We usually rerun an inconsistent test if it fails.

Now that this is open and the other is closed, please keep this one open until it's ready to merge.

@bschaatsbergen
Copy link
Member Author

Previous branch was branched from master, I suppose that's why the circle-ci job failed..

Seems to work now :)

@nitrocode
Copy link
Member

Thank you. We deleted the master branch to avoid the issue in the future.

Copy link
Member

@GenPage GenPage left a comment

Choose a reason for hiding this comment

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

This is technically not true as the Default Date Dir is set to ~/.atlantis

DefaultDataDir = "~/.atlantis"

What should probably be addressed is the fact that most shells use tilde as an expression to expand the user's home directory. It's a shortcut and we really should be using an absolute path instead of a relative one here as it causes confusion with functionality based on the shell the user is using.

Changing default is a big deal and could have unintended consequences as people could be relying on unintended behavior of their shell, but for consistency I think it's worth a breaking change.

@bschaatsbergen
Copy link
Member Author

bschaatsbergen commented Jan 4, 2023

This is technically not true as the Default Date Dir is set to ~/.atlantis

DefaultDataDir = "~/.atlantis"

What should probably be addressed is the fact that most shells use tilde as an expression to expand the user's home directory. It's a shortcut and we really should be using an absolute path instead of a relative one here as it causes confusion with functionality based on the shell the user is using.

Changing default is a big deal and could have unintended consequences as people could be relying on unintended behavior of their shell, but for consistency I think it's worth a breaking change.

Hmm that's right indeed, this also seems to conflict with the hardcoded absolute path in the Docker base. What do you suggest? I don't plan on changing the DefaultDataDir through this PR though.

@GenPage
Copy link
Member

GenPage commented Jan 4, 2023

I don't expect you to, maybe instead of changing the wording from ~/.atlantis to /home/<user>/.atlantis we add a note about the default directory expanding to the current users home directory based on the shell they use.

@bschaatsbergen
Copy link
Member Author

bschaatsbergen commented Jan 4, 2023

I don't expect you to, maybe instead of changing the wording from ~/.atlantis to /home/<user>/.atlantis we add a note about the default directory expanding to the current users home directory based on the shell they use.

I've reverted it back to ~/atlantis. To avoid further confusion I won't add another note.
What you had mentioned regarding to changing the DefaultDataDir is probably something we should do at some point.

@GenPage
Copy link
Member

GenPage commented Jan 4, 2023

I don't expect you to, maybe instead of changing the wording from ~/.atlantis to /home/<user>/.atlantis we add a note about the default directory expanding to the current users home directory based on the shell they use.

I've reverted it back to ~/atlantis. To avoid further confusion I won't add another note. What you had mentioned regarding to changing the DefaultDataDir is probably something we should do at some point.

Thank you, just to note it is ~/.atlantis not ~/atlantis if you could fix that. After that, we can squash merge the doc update.

@bschaatsbergen
Copy link
Member Author

My bad, was a bit too quick @GenPage. Thanks for being sharp 👍

@GenPage
Copy link
Member

GenPage commented Jan 4, 2023

No worries that's what PRs are for!

@jamengual jamengual merged commit 673c4f1 into runatlantis:main Jan 4, 2023
@bschaatsbergen bschaatsbergen deleted the atlantis-user-filesystem-restriction branch January 4, 2023 22:48
@nitrocode nitrocode added this to the 0.22.2 milestone Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If --data-dir is not /home/atlantis results in permission denied
4 participants