Issue #7445
closedPulp2 overrides permissions set by pulp3
Description
Imagine you have pulp2 and some data( in this case i had only ISO content), then pulp3 gets installed. Write permission for the pulp group is granted and setgid is set [0] Issue comes when new content gets synced into pulp2( in this case I synced an rpm repo) .Pulp2 overrides permissions, this makes is impossible to create a hardlink
$ ll /var/lib/pulp/content/units/
total 12
drwxrwsr-x. 173 apache pulp 4096 Sep 2 08:34 iso <------------------ existing content by the time pulp3 installed
drwxr-sr-x. 12 apache pulp 106 Sep 2 08:35 modulemd <---------------------- new content after pulp3 installed
drwxr-sr-x. 5 apache pulp 36 Sep 2 08:35 modulemd_defaults
drwxr-sr-x. 35 apache pulp 4096 Sep 2 08:35 rpm
(pulp) [vagrant@pulp2-nightly-pulp3-source-centos7 ~
$ ll /var/lib/pulp/content/units/modulemd
total 0
drwxr-sr-x. 2 apache pulp 76 Sep 2 08:35 00
drwxr-sr-x. 2 apache pulp 76 Sep 2 08:35 04
drwxr-sr-x. 2 apache pulp 76 Sep 2 08:35 1b
drwxr-sr-x. 2 apache pulp 76 Sep 2 08:35 41
drwxr-sr-x. 2 apache pulp 76 Sep 2 08:35 66
drwxr-sr-x. 2 apache pulp 76 Sep 2 08:35 78
drwxr-sr-x. 2 apache pulp 76 Sep 2 08:35 8a
drwxr-sr-x. 2 apache pulp 76 Sep 2 08:35 90
drwxr-sr-x. 2 apache pulp 76 Sep 2 08:35 a8
drwxr-sr-x. 2 apache pulp 76 Sep 2 08:35 ea
Then i synced a new ISO repo.
$ ll /var//lib//pulp/content/units/iso/12
total 0
drwxr-sr-x. 2 apache pulp 19 Sep 2 08:33 3f7c65dc3598a59bbb867425c4e52cc54ecb66ff0f6db4656d349799b96594
(pulp) [vagrant@pulp2-nightly-pulp3-source-centos7 ~]$ ll /var//lib//pulp/content/units/iso/12/3f7c65dc3598a59bbb867425c4e52cc54ecb66ff0f6db4656d349799b96594/
All the content that appears after pulp3 is installed, does not have write permission for the pulp group. This makes it impossible to create hard link during the migration https://pulp.plan.io/issues/7244
[0] https://github.com/pulp/pulp_installer/blob/master/roles/pulp_common/tasks/install.yml#L107-L133
Related issues
Updated by ipanova@redhat.com over 4 years ago
- Subject changed from Pulp2 ovverides permissions set by pulp3 to Pulp2 overrides permissions set by pulp3
Updated by fao89 over 4 years ago
- Triaged changed from No to Yes
- Sprint set to Sprint 80
Updated by ttereshc over 4 years ago
- Priority changed from Normal to High
- Tags Katello added
Updated by ipanova@redhat.com over 4 years ago
- Is duplicate of Issue #7244: Hard links not being used during RPM content type migration added
Updated by fao89 about 4 years ago
- Category set to Installer - Moved to GitHub issues
Updated by ggainey about 4 years ago
- Status changed from NEW to ASSIGNED
- Assignee set to ggainey
Updated by ggainey about 4 years ago
- Category deleted (
Installer - Moved to GitHub issues) - Tags Pulp 2 added
- Tags deleted (
Katello)
This isn't an installer or migration issue. What's happening here is that Pulp2 uses os.makedirs() to create new paths when syncing. makedirs(), even when mode= is specified, uses current-umask to set permissions (see the os.makedirs documentation). The umask for pulp is 022. As a result, directories created post-pulp3-install end up 755, and Things Break.
os.makedir() is used in a number of places in pulp2, any of which will result in directories not ending up with the permissions the code is expecting (especially the places where pulp2 calls makedirs() with mode=770, which clearly expect the result to be what was specified at least for the leaf-node).
Pulp2 has a utility method in base-pulp, pulp.plugins.util.misc.mkdir(). We probably should a) fix this method to Do The Right Thing in terms of insuring the leaf-node ends up with permissions specified by mode=, if any, and b) replace existing explicit os.makedirs() calls with calls to this utility routine.
Updated by ggainey about 4 years ago
Is the problem here just the leaf-node? If it's just the leaf-node, then we can 'fix' this just by insuring we directly chmod() the end-result to the perms we want/need/expect. If any/all internediate dirs need the permissions as well, then there is some more-interesting work that has to happen.
ipanova@redhat.com , ttereshc - thoughts?
Updated by mdepaulo@redhat.com about 4 years ago
How about we use the systemd unit files to set the umask to 002?
Note about setgid:
Those perms drwxr-sr-x
are 2755
, where the leading 2 is setgid.
The setgid is getting inherited from the parent dir, so pulp_installer did its job of setting it. setgid cannot be controlled by umask, so it will not complicate this approach.
Updated by ggainey about 4 years ago
mdepaulo@redhat.com wrote:
How about we use the systemd unit files to set the umask to 002?
Interesting idea. We're explicitly setting umask for pulp-resource-manager in Pulp2 now:
system/pulp_resource_manager.service:
ExecStart=/usr/bin/celery worker -A pulp.server.async.app -n resource_manager@%%h
-Q resource_manager -c 1 --events --umask 18 --pidfile=/var/run/pulp/resource_manager.pid
Note about setgid:
Those perms
drwxr-sr-x
are2755
, where the leading 2 is setgid.The setgid is getting inherited from the parent dir, so pulp_installer did its job of setting it. setgid cannot be controlled by umask, so it will not complicate this approach.
Yeah, concur, ownership should be fine, it's just the default-directory-perms that need to be addressed.
Updated by ggainey about 4 years ago
mdepaulo@redhat.com wrote:
How about we use the systemd unit files to set the umask to 002?
The problem with setting it outside code is, that umask can be/is set:
- at the service level in /etc/systemd/system/.service, which can be overridden by
- at the user-level, in /etc/systemd/user.conf and users/, which can be overridden by
- the system-level shell/env files /etc/bashrc /etc/profile and /etc/cshrc
Setting it explicitly in code, of course, trumps all of the above.
I'm experimenting on a pulp2 box. So far, I have not figured out which umask-default controls this particular scenario, and it makes me sad. I do have a fix in code that works. Experimentation continues.
Updated by ggainey about 4 years ago
Commentary/discussion with jsherrill:
On Wed, Sep 30, 2020 at 9:04 AM Justin Sherrill jsherril@redhat.com wrote:
Wouldn't this first approach also require a script to fix existing files/directories?
The pulp3 installer does exactly tyhat at install-time :
https://github.com/pulp/pulp_installer/blob/master/roles/pulp_common/tasks/install.yml#L107-L133
The difference I think is that this first approach would only need the script run once prior to migration. While the 2nd approach would require it to be run before every migration. I could see this taking a long time on a filesystem with 100s of thousands of files, or millions even. More so on a high latency file system. Since subsequent migrations are aimed to be as 'quick' as possible, i think we have to go with the first option?
The 'what if this is a HYOOGE filesystem" argument is persuasive :)
Updated by ggainey about 4 years ago
(Addendum to #c18 - "first approach" is "explicitly set/reset umask prior to makedirs() calls", "second approach" is "run script after any pulp2-activity post-pulp3-install")
Updated by ggainey about 4 years ago
One final(?) complicating factor to be aware of: in python2, os.makedirs(dir, mode=) will try to set the permissions on all directories it creates on the way to the leaf. In python3, that is no longer true - intermediate directories'permissions are driven just by umask.
This means that a sequence like
import os
mask = os.umask(0)
os.makedirs("/tmp/foo/bar/blech", mode=0770)
results in each of foo, bar, and blech ending up with permissions 770.
The same code in python3 results in foo and bar being 777, and only blech being 770.
Especially in testing, it's important to remember the environment the code will be running under. Pulp2 runs under Python2 and can therefore 'assume' the "mode= is for perms for any/all created dirs" behavior, where Pulp3 cannot.
Hopefully this note will save someone else from the 10 minutes of confusion I just suffered....
Updated by mdepaulo@redhat.com about 4 years ago
ggainey wrote:
mdepaulo@redhat.com wrote:
How about we use the systemd unit files to set the umask to 002?
The problem with setting it outside code is, that umask can be/is set:
- at the service level in /etc/systemd/system/.service, which can be overridden by
- at the user-level, in /etc/systemd/user.conf and users/, which can be overridden by
No, this affects systemd user mode, not system mode, which Pulp runs in.
- the system-level shell/env files /etc/bashrc /etc/profile and /etc/cshrc
No, one of the designs behind systemd is to prevent these from bleeding in. And for umask, that's actually a different issue entirely than environment variables.
Setting it explicitly in code, of course, trumps all of the above.
You're right, I just thought I'd propose the systemd unit files because:
- it would be quicker/easier to implement.
- If there was ever a need to configure it at runtime/install-time, it would be easier.
I'm experimenting on a pulp2 box. So far, I have not figured out which umask-default controls this particular scenario, and it makes me sad. I do have a fix in code that works. Experimentation continues.
To clarify, this is a variant of the 1st approach.
Updated by ggainey about 4 years ago
- Copied to Issue #7653: Pulp2 overrides permissions set by pulp3 added
Updated by ggainey about 4 years ago
- Copied to Issue #7654: Pulp2 overrides permissions set by pulp3 added
Updated by ggainey about 4 years ago
- Status changed from ASSIGNED to POST
Added by ggainey about 4 years ago
Updated by ggainey about 4 years ago
- Status changed from POST to MODIFIED
Applied in changeset pulp|e067d28a00bdec989dd03170d599e58cde4fba13.
Added by ggainey about 4 years ago
Revision 034c6baf | View on GitHub
Taught util.misc.mkdir() to set umask to allow group-w for makedirs calls.
Hunted down a LOT of makedirs calls and replaced with call to misc.mkdir()
fixes #7445
(cherry picked from commit e067d28a00bdec989dd03170d599e58cde4fba13)
Updated by ggainey about 4 years ago
- Status changed from MODIFIED to CLOSED - CURRENTRELEASE
Taught util.misc.mkdir() to set umask to allow group-w for makedirs calls.
Hunted down a LOT of makedirs calls and replaced with call to misc.mkdir()
fixes #7445