chmod temp files using umask from ServerOptions

[ericatkin]

See issue #123

[mnaberez]

No description provided.

Please give us more to work with here. What was the motivation for this change, did you find a problem that this is intended to fix, etc.?

[JonathanGrant]

lgtm

[handlerbot]

Hi @mnaberez I'm not the author, but I am also interested in this getting merged. :-)

My use case is that I have separate processes, running as a user other than the one I run supervisor as, which reviews the supervisor per-job log files for specific errors, etc. Without this patch, by default supervisor creates the files via tempfile.mkstemp() which says "The file is readable and writable only by the creating user ID," regardless of the setting of supervisor's umask or the settings in the supervisor config file.

Is there anywhere I could provide a description for this patch that would let it get reviewed for merge? Thanks! cc @ericatkin

--Michael

[handlerbot]

Sorry to bump, but: anything I can do to help facilitate this getting reviewed and merged? :-)

--Michael

[ericatkin]

So, this isn't really a feature change, it's a bugfix. What I mean is, a use case doesn't seem needful as the use case is to create log files as documented. This change fixes the bug described in issue #123 that caused the behavior to not match the documentation.
Thanks.

[mnaberez]

This change fixes the bug described in issue #123 that caused the behavior to not match the documentation.

Thanks for adding some background on this change. Could you please point to the specific page(s) in the documentation that are inconsistent?

[handlerbot]

So, looking at http://supervisord.org/logging.html#child-process-logs, it says:

supervisord will capture the child process’ stdout and stderr output into temporary files. Each stream is captured to a separate file. This is known as AUTO log mode. AUTO log files are named automatically and placed in the directory configured as childlogdir of the [supervisord] section of the config file.

There's nothing in there explicitly about permissions the files will be created with, but it seems reasonable to assume that the umask for the process would be used here, as it is elsewhere for other files that are created? This was suggested in the original issue at #123 (comment), if nothing else. :-)

And, as we can see from a running system, that isn't what happens, because

$ find . -ls
401556    4 drwxrwxr-x   3 ec2-user ec2-user     4096 Sep 21 21:56 .
399886    8 -rw-r--r--   1 root     root         5208 Sep 22 00:56 ./log
399897    4 -rw-r--r--   1 root     root            6 Sep 21 21:56 ./pid
401557    4 drwxrwsr-x   2 ec2-user ec2-user     4096 Sep 21 21:56 ./programlogs
399888    4 -rw-------   1 root     ec2-user       41 Sep 21 21:56 ./programlogs/myprogram.0-stdout---supervisor-40_uvr.log

$ pgrep supervisord
16976

$ grep Umask /proc/16976/status
Umask:	0022

This is because Python's tempfile.mkstemp explicitly sets the permissions on the file to be read/write only by the user creating the file:

• https://docs.python.org/3/library/tempfile.html#tempfile.mkstemp says "The file is readable and writable only by the creating user ID."
• The permission restriction is done explicitly here: https://github.com/python/cpython/blob/44989bc2696320cf55ae6f329aaf58edd49d792a/Lib/tempfile.py#L258

[mnaberez]

@ericatkin We will probably have another point release soon. Independent of this PR or #123, we can immediately fix the incorrect documentation you mentioned. I had a look and didn't find it, but there's a lot of docs so I probably just missed it. It would be great if you could point out the pages that are inconsistent and we can correct them as a first step. Thanks.

[ericatkin]

@mnaberez I didn't review this issue when I created the comment referring to incorrect documentation. It appears I was wrong about that. I also can't find any documentation that explicitly states log files will be created with permissions derived from the process umask. That said, it seems to me like a reasonable assumption from the language on the umask documentation as well as the behavior of many other CLI utilities. This PR make it behave according to that assumption. I think not creating files with the process umask is the surprising bit. Either this PR (or a similar change) should be merged, or the documentation updated to describe the exceptional behavior. I of course am in favor of the first option :)

[handlerbot]

Doing our regular AMI rebuild, so checking in on if this can be merged and released? 🙏🏻

[flamableconcrete]

As a random person on the internet, I can attest that I tried out this change locally and fixed #123 for me. Any reason to wait further on merging it in? Is it a documentation issue at this point?