Project

Profile

Help

Issue #7445

closed

Pulp2 overrides permissions set by pulp3

Added by ipanova@redhat.com over 3 years ago. Updated over 3 years ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
High
Assignee:
Category:
-
Sprint/Milestone:
-
Start date:
Due date:
Estimated time:
Severity:
2. Medium
Version:
Platform Release:
2.21.4
OS:
Triaged:
Yes
Groomed:
No
Sprint Candidate:
No
Tags:
Pulp 2
Sprint:
Sprint 83
Quarter:

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

Is duplicate of Migration Plugin - Issue #7244: Hard links not being used during RPM content type migrationCLOSED - DUPLICATEipanova@redhat.comActions
Copied to RPM Support - Issue #7653: Pulp2 overrides permissions set by pulp3CLOSED - CURRENTRELEASEggaineyActions
Copied to Docker Support - Issue #7654: Pulp2 overrides permissions set by pulp3CLOSED - CURRENTRELEASEggaineyActions
Actions #1

Updated by ipanova@redhat.com over 3 years ago

  • Description updated (diff)
Actions #2

Updated by ipanova@redhat.com over 3 years ago

  • Description updated (diff)
Actions #3

Updated by ipanova@redhat.com over 3 years ago

  • Description updated (diff)
Actions #4

Updated by ipanova@redhat.com over 3 years ago

  • Description updated (diff)
Actions #5

Updated by ipanova@redhat.com over 3 years ago

  • Subject changed from Pulp2 ovverides permissions set by pulp3 to Pulp2 overrides permissions set by pulp3
Actions #6

Updated by fao89 over 3 years ago

  • Triaged changed from No to Yes
  • Sprint set to Sprint 80
Actions #7

Updated by ttereshc over 3 years ago

  • Priority changed from Normal to High
  • Tags Katello added
Actions #8

Updated by ipanova@redhat.com over 3 years ago

  • Is duplicate of Issue #7244: Hard links not being used during RPM content type migration added
Actions #9

Updated by rchan over 3 years ago

  • Sprint changed from Sprint 80 to Sprint 81
Actions #10

Updated by rchan over 3 years ago

  • Sprint changed from Sprint 81 to Sprint 82
Actions #11

Updated by fao89 over 3 years ago

  • Category set to Installer - Moved to GitHub issues
Actions #12

Updated by ggainey over 3 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to ggainey
Actions #13

Updated by ggainey over 3 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.

Actions #14

Updated by ggainey over 3 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?

Actions #15

Updated by mdepaulo@redhat.com over 3 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.

Actions #16

Updated by ggainey over 3 years ago

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 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.

Yeah, concur, ownership should be fine, it's just the default-directory-perms that need to be addressed.

Actions #17

Updated by ggainey over 3 years ago

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.

Actions #18

Updated by ggainey over 3 years ago

Commentary/discussion with jsherrill:

On Wed, Sep 30, 2020 at 9:04 AM Justin Sherrill 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 :)

Actions #19

Updated by ggainey over 3 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")

Actions #20

Updated by ggainey over 3 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....

Actions #21

Updated by mdepaulo@redhat.com over 3 years ago

ggainey wrote:

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:

  1. it would be quicker/easier to implement.
  2. 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.

Actions #22

Updated by rchan over 3 years ago

  • Sprint changed from Sprint 82 to Sprint 83
Actions #23

Updated by ggainey over 3 years ago

  • Copied to Issue #7653: Pulp2 overrides permissions set by pulp3 added
Actions #24

Updated by ggainey over 3 years ago

  • Copied to Issue #7654: Pulp2 overrides permissions set by pulp3 added
Actions #25

Updated by ggainey over 3 years ago

  • Status changed from ASSIGNED to POST

Added by ggainey over 3 years ago

Revision e067d28a | 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

Actions #26

Updated by ggainey over 3 years ago

  • Status changed from POST to MODIFIED
Actions #27

Updated by ggainey over 3 years ago

  • Platform Release set to 2.21.4

Added by ggainey over 3 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)

Actions #28

Updated by ggainey over 3 years ago

  • Status changed from MODIFIED to CLOSED - CURRENTRELEASE

Also available in: Atom PDF