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

Install: setup umask #1222

Merged
merged 1 commit into from
Jan 5, 2022
Merged

Install: setup umask #1222

merged 1 commit into from
Jan 5, 2022

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Dec 25, 2021

Setup default umask, otherwise files created with permissions that containers later can't read.

    !! Configuration error: ConfigurationError("PermissionError: [Errno 13]
    Unable to load configuration file (Permission denied):
    '/etc/sentry/sentry.conf.py'",)

To reproduce, try:

git checkout 21.6.3 
umask 0007
./install.sh
root@docker2 sentry/21.6.3# ls -l sentry/sentry.conf.py
-rw-rw---- 1 root root 7.4K Dec 25 03:18 sentry/sentry.conf.py

also known to be broken with 21.5.0 and 9.1.2, i.e the versions noted in hard stops:

so, perhaps carry the fix to those branches as well

@aminvakil
Copy link
Collaborator

Do you think it's good to show users that their umask is changing temporarily with executing install.sh?

Or at least to the ones who have disabled reading for world and therefore without this fix sentry won't get installed properly.

@glensc
Copy link
Contributor Author

glensc commented Dec 25, 2021

It doesn't change umask temporarily, it changes for the install script. umask outside the script says as it is.

@aminvakil
Copy link
Collaborator

It doesn't change umask temporarily, it changes for the install script. umask outside the script says as it is.

Sorry for not being clear, by temporarily I mean the same, do you think it's good to tell users this is changing created files by install.sh?

@glensc
Copy link
Contributor Author

glensc commented Dec 26, 2021

No I don't think users need to be aware of any of this.

ideally, this should be solved (also) by doing proper chown/chmod after file creation. or even better at each install.sh invocation.

@aminvakil
Copy link
Collaborator

No I don't think users need to be aware of any of this.

ideally, this should be solved (also) by doing proper chown/chmod after file creation. or even better at each install.sh invocation.

That can be taken care of in another PR.

This LGTM for now.

@glensc
Copy link
Contributor Author

glensc commented Dec 27, 2021

An older issue, that have been likely caused by this problem: #418

@@ -5,6 +5,8 @@ if [[ -n "$MSYSTEM" ]]; then
exit 1
fi

umask 002
Copy link
Member

Choose a reason for hiding this comment

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

Refreshing my memory:

new directories will have permission 775 and new files permission of 664

Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uhm, yes...

easy tester:

➜ (umask 002; mkdir dir1; touch file1; ls -ld dir1 file1)
drwxrwxr-x 2 glen staff 64 dets  28 23:35 dir1/
-rw-rw-r-- 1 glen staff  0 dets  28 23:35 file1

@chadwhitacre
Copy link
Member

Rerunning CI to dodge #1171 ... set to auto-merge once green.

Thanks @glensc @aminvakil. :)

so, perhaps carry the fix to those branches as well

Hrm, that would be a first. We don't really ever backport to older versions, especially now that we're on CalVer. Why are you hitting this (why is this the first we're hearing about it?)? Presumably you were able to work around the problem, yes?

@chadwhitacre chadwhitacre enabled auto-merge (squash) January 4, 2022 23:04
@glensc
Copy link
Contributor Author

glensc commented Jan 4, 2022

@chadwhitacre it's not common to change umask. also, I'm guessing people who hit any problem just do chmod -R 777 . and go on instead of taking time figuring out the problem cause and reporting it :D

@glensc
Copy link
Contributor Author

glensc commented Jan 5, 2022

@chadwhitacre as for the problem workaround, I ran chmod a+rX -R . and re-run the install script. and that as many times as many new files the install script creates. rather annoying and time-waster, but was able to finish install/upgrade like this.

and once I knew it's about umask, the workaround is to set umask once prior script run.

@chadwhitacre
Copy link
Member

it's not common to change umask

So a user has to have set a umask in order to hit this issue? I guess that is indicated in your repro steps:

git checkout 21.6.3 
umask 0007
./install.sh

In that case I guess the user could also manually set a more relaxed umask before running 21.5.0 or 9.1.2. With these workarounds available, I don't see a strong enough case for issuing our first x.x.x.1 releases.

@chadwhitacre chadwhitacre merged commit 8cdae78 into getsentry:master Jan 5, 2022
@chadwhitacre
Copy link
Member

Thanks for taking the time to figure out the root cause and report it, @glensc! 😁

@glensc glensc deleted the patch-1 branch January 5, 2022 21:42
@github-actions github-actions bot locked and limited conversation to collaborators Jan 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants