Problem: Pulp 3 can't sync when installed along side Pulp 2

[dkliban]

Solution: make /var/lib/pulp writable by group 'pulp'

This patch also removes the recursive chown for /var/lib/pulp.

fixes: #5152
https://pulp.plan.io/issues/5152

[dkliban]

I just filed https://pulp.plan.io/issues/5154 related to this also. That is going to require a code change in Pulp.

[dkliban]

@pulp/release-engineering Can I get a review?

[evgeni]

@dkliban sorry for the slow review!

Does this change ensure that these permissions are also present on upgraded (2.x to 2.20) installs?
My gut says it will not :(

Btw, we also manage a few files/folders under /var/lib/pulp in the Katello installer (https://github.com/theforeman/puppet-pulp/blob/7729fe8db004f93d3af226ccadaa8075be0fa53d/manifests/config.pp#L4-L9) -- would that also need adjustments?

[evgeni]

A brief test with a different RPM proves otherwise, the permissions are updated.

[evgeni]

@ekohl any objections?

[dkliban]

@evgeni, the /var/lib/pulp/packages directory should probably be owned by group 'pulp' also. However, Pulp 3 is not going to access that directory.

[ekohl]

My biggest worry about this is newly created files. Usually they're not created with the correct ownership and permissions. It's a much bigger task, but should we instead focus on running the actual application as pulp:pulp? mod_wsgi can do that but we just don't configure it.

[dkliban]

@ekohl We are addressing that concern as part of this ticket: https://pulp.plan.io/issues/5154

[ekohl]

No objection, but I can't say I have enough insight to properly know what could go wrong and if it's sufficient. Have you done scratch builds to verify this anything?

[evgeni]

One thing that I just realized, this will make all files executable. See http://ftp.rpm.org/max-rpm/s1-rpm-inside-files-list-directives.html#S3-RPM-INSIDE-FLIST-DEFATTR-DIRECTIVE

Do you maybe need %defattr(-, apache, pulp, 755) instead?

[dkliban]

One thing that I just realized, this will make all files executable. See http://ftp.rpm.org/max-rpm/s1-rpm-inside-files-list-directives.html#S3-RPM-INSIDE-FLIST-DEFATTR-DIRECTIVE

Do you maybe need %defattr(-, apache, pulp, 755) instead?

Good catch. I want all the directories to be 775. So I will update the PR to %defattr(-, apache, pulp, 775)

[dkliban]

I installed pulp-server RPM with these changes. Then I synced and published a repository. I see the following permissions set:

$ ls -la /var/lib/pulp
total 8
drwxrwxr-x.  7 apache  pulp    147 Jul 30 18:02 .
drwxr-xr-x. 38 root    root   4096 Jul 30 16:43 ..
-rw-r--r--.  1 apache  apache    2 Jul 30 16:40 0005_puppet_module_name_change.txt
drwxr-xr-x.  3 apache  apache   19 Jul 30 18:02 content
-rw-r--r--.  1 root    root      0 Jul 30 16:40 db_initialized.flag
drwxrwxr-x.  7 apache  pulp     73 Jul 30 13:08 published
drwxrwxr-x.  2 apache  pulp     25 Jul 30 13:08 static
drwxrwxr-x.  5 vagrant pulp    269 Jul 30 16:43 tmp
drwxrwxr-x.  2 apache  pulp      6 Jul 30 13:08 uploads

And the following

$ ls -la /var/lib/pulp/content/units/rpm/
total 4
drwxr-xr-x. 31 apache apache 4096 Jul 30 18:02 .
drwxr-xr-x.  3 apache apache   17 Jul 30 18:02 ..
drwxr-xr-x.  3 apache apache   76 Jul 30 18:02 02
drwxr-xr-x.  3 apache apache   76 Jul 30 18:02 13
drwxr-xr-x.  3 apache apache   76 Jul 30 18:02 17
drwxr-xr-x.  4 apache apache  146 Jul 30 18:02 1a
drwxr-xr-x.  3 apache apache   76 Jul 30 18:02 26
drwxr-xr-x.  3 apache apache   76 Jul 30 18:02 31

And the following:

$ ls -la /var/lib/pulp/published/yum/master/yum_distributor/zoo/1564509752.26/
total 4
drwxr-xr-x.  4 apache apache   38 Jul 30 18:02 .
drwxr-xr-x.  3 apache apache   27 Jul 30 18:02 ..
drwxr-x---. 17 apache apache  141 Jul 30 18:02 Packages
drwxr-x---.  2 apache apache 4096 Jul 30 18:02 repodata

We are also going to introduce a change to pulp workers that will make them create new files with user apache and group pulp. That change is part of https://pulp.plan.io/issues/5154. Because of 5154, I still need to add a change to the spec file that will run chmod on /var/lib/pulp for any installation that is upgrading from 2.20.0 and earlier. Right now we are limiting it to anything that doesn't have 'apache:pulp' ownership of /var/lib/pulp.

[evgeni]

Thanks @dkliban for testing the RPM. I gather from your output that the change produces the result you wanted and everything else will be handled in Pulp itself in 5154?

I do agree with @ekohl that enforcing apache:pulp on the "how we run things" level (WSGI, systemd, etc) might be easier than adjusting the application for that. But this is more of a talk in 5154.

Also, because you mention chmod/chown that is already in

pulp-packaging/packages/pulp/pulp.spec

Lines 512 to 514 in f6f127c

	if [ $(stat -c '%U:%G' /var/lib/pulp) != 'apache:pulp' ] ; then
	/usr/bin/chown -R apache:pulp /var/lib/pulp
	fi

This will take quite some time if /var/lib/pulp is big (our install is ~1TB) and I wonder if that could be limited somehow. But again, probably not a topic for this PR.

So ACK on this particular change, but "let's talk" on the overall topic :)

[dkliban]

@evgeni The files that are created in /var/lib/pulp/content are created by celery workers. The solution to 5154 is to adjust the systemd unit files for workers to start with these same permissions.

I am also concerned about the amount of time it will take users to upgrade. Let's hold off on merging this PR. I may even want to revert the other change to the spec file that relabels the filesystem.

[evgeni]

OK, holding until you yell again :)

[bmbouter]

@dkliban This looks right to me, but I think security wise this needs to come w/ a release note. What if it's a shared environment? Making the data world readable needs to be understood by the admin.

[ekohl]

@bmbouter good point, but isn't the default 755?

[evgeni]

It always was world readable

[dkliban]

@dkliban This looks right to me, but I think security wise this needs to come w/ a release note. What if it's a shared environment? Making the data world readable needs to be understood by the admin.

@bmbouter It was already world readable. So there is nothing new here.

[bmbouter]

@dkliban oh so this reverts to what it was before we made any changes?

[dkliban]

@bmbouter - that's right. but it also adds a new group called pulp and makes /var/lib/pulp/ writable by group pulp. I will break these two things up into separate commits. That way the revert can be part of 2.20.1 and the new group can be part of 2.21.0.

[bmbouter]

@dkliban cool

[dkliban]

@pulp/release-engineering This is ready for final review and merge. I've split this up over 2 commits.

[evgeni]

Thanks @dkliban!