Fixes #30423 - Change the application layout #115

ekohl · 2020-07-21T16:09:28Z

This explicitly sets the all directories and documents the layout. It diverges from the upstream defaults in that MEDIA_ROOT is set to a subdirectory and the directory permissions are stricter than upstream. The assets are served in static since this works around some SELinux issue.

Upstream this layout is proposed as the default. https://pulp.plan.io/issues/7178 has been opened for that.

It's an alternative to #113. It will need a migration in the installer to move any generated content.

ekohl · 2020-07-21T17:13:46Z

The migration can be done by moving /var/lib/pulp/docroot to /var/lib/pulp/media in foreman-installer, unless media already exists.

ekohl · 2020-07-22T11:39:07Z

manifests/config.pp

    owner  => $pulpcore::user,
    group  => $pulpcore::group,
    mode   => '0775',


So I've learned this may give problems when sharing this directory with Pulp 2. We don't add the Apache user to the pulp group. Looking into this now.

So it turns out Pulp 2's spec file does this: https://github.com/pulp/pulp-packaging/blob/7c05ec9c678c01aa9c6a40f8e3a8be4b1aa9c50d/packages/pulp/pulp.spec#L500

It's lovely that /var/lib/pulp is also owned by pulp-server but with different ownership:
https://github.com/pulp/pulp-packaging/blob/7c05ec9c678c01aa9c6a40f8e3a8be4b1aa9c50d/packages/pulp/pulp.spec#L484-L485

This means rpm -qV pulpserver will result in warnings.

If I understand correctly, this means your implementation should work whether it's working alongside pulp2 or a future new install that is pulp3 only? Because the group membership is only required by Pulp2 and is handled by Pulp2 packaging.

Yes, I wasn't aware Pulp 2 packaging did that and thought it was something the installer needed to do.

wbclark · 2020-07-22T14:16:06Z

manifests/init.pp

 #   include pulpcore
+#
+# @example Disable static content


@ekohl can you help me understand this use case? why would an user want to disable management of static content ?

Static content is only the CSS/JS and you don't need that for the API. Right now in our deployment we don't even serve it and we've shipped that to users.

@ekohl My understanding is that it does allow for a nicer presentation of the api docs... Is that something that we don't expect a Katello user to interact with directly?

My thinking is that from a troubleshooting standpoint that would be nice to have in the Katello scenario, particularly given that there isn't a pulp-admin equivalent for Pulp3.

I think so and it is a goal, but right now we don't serve them at all and this allows a lighter deployment.

After considering this and seeing that Pulp reads the static files on application startup, I've decided to drop the option to not generate these. I have moved it to its own class so it doesn't restart all applications if anything changes. Thinking about it, it should probably restart the API service if it reads the files.

ehelms · 2020-07-27T13:01:32Z

README.md

+
+There is also `chunked_upload_dir` to configure the undocumented `CHUNKED_UPLOAD_DIR`. This directory stores the temporary files used for files uploaded as chunks.
+
+When the Apache vhost is managed, `apache_docroot` (default `/var/lib/pulp/docroot`) is also created. This doesn't contain any files, but this makes sure that there aren't any files being served.


I think there is some nuance of Apache embedded in this wording that makes it hard to know what this means. I can sum this up with: why do I care if files are being served or not?

You should care that no files are served. Turns out that serving any files from Apache is a misconfiguration.

Would another way to phrase this be:

Apache requires that a docroot be configured. By configuring this to an empty directory, we prevent Apache from being configured to accidentally server Pulp content outside of the Pulp content app?

Yes, that is a much better way to phrase it.

ekohl · 2020-07-27T13:39:41Z

I had some conversations with upstream. Right now I want to agree on the best layout and ensure we're not going to have to migrate twice. This PR has been very useful for me to get my thoughts straight, but moving back to a draft for now.

wbclark · 2020-08-18T18:22:58Z

@ekohl @jlsherrill how immediate is the need to get this straightened out?

Should I close out theforeman/foreman-installer#543 , theforeman/puppet-foreman_proxy_content#275 , and #113 while we settle on a definitive layout?

Or is it better to move forward with those while this one is still in progress ?

ekohl · 2020-09-01T16:12:45Z

pulp/pulpcore#799 (comment) would be roughly the migration plan.

We can do the same in a pre hook. That should be after our SELinux hook, though it's probably not that relevant. On non-migrated systems the directory will not be present. On upgraded systems it should already be present.

Puppet will take care of the config update + application restarts.

The cleanup of the symlink should happen in a post hook.

ekohl · 2020-10-05T16:06:39Z

Updated. I now believe this is mostly ready. I'm going to work on an installer migration to migrate the content.

manifests/config.pp

ekohl · 2020-10-05T17:56:51Z

Migration in theforeman/foreman-installer#583.

ekohl · 2020-10-05T18:15:44Z

Note to self: check SELinux context on /var/lib/pulp/media

ekohl · 2020-10-09T13:19:23Z

pulp/pulpcore-selinux#23

This explicitly sets the all directories and documents the layout. It diverges from the upstream defaults in that MEDIA_ROOT is set to a subdirectory and the directory permissions are stricter than upstream. Upstream this layout is proposed as the default. https://pulp.plan.io/issues/7178 has been opened for that.

wbclark · 2020-10-09T16:54:30Z

manifests/static.pp

@@ -11,6 +11,6 @@
  pulpcore::admin { 'collectstatic --noinput':
    refreshonly => true,
    subscribe   => Concat['pulpcore settings'],
-    require     => File[$pulpcore::pulp_static_root],
+    require     => File[$pulpcore::static_root],


Suggested change

require => File[$pulpcore::static_root],

subscribe => File[$pulpcore::static_root],

As discussed on IRC, there is a very small possibility that owner/group could be changed. In that case we would want to re-run collectstatic so that the collected static content will also be changed to match.

In most regular use cases that is unlikely to happen, but that also means the cost of making this change (in terms of execution time) is negligible since the refresh would only get triggered in that edge case.

wbclark

This worked well in my testing

theforeman-bot added Needs testing Not yet reviewed labels Jul 21, 2020

ekohl force-pushed the 30423-app-layout branch from 16900a3 to a31e6fd Compare July 21, 2020 17:11

ekohl marked this pull request as ready for review July 21, 2020 17:24

ekohl mentioned this pull request Jul 21, 2020

Add full Apache management #105

Merged

ekohl force-pushed the 30423-app-layout branch from a31e6fd to ba29168 Compare July 21, 2020 17:25

ekohl mentioned this pull request Jul 21, 2020

Fixes #30423 - match default directories in pulp_installer #113

Closed

ekohl commented Jul 22, 2020

View reviewed changes

wbclark reviewed Jul 22, 2020

View reviewed changes

ekohl mentioned this pull request Jul 22, 2020

Fixes #30465 - Use libexec wrappers for SELinux #116

Merged

ekohl force-pushed the 30423-app-layout branch from 522a7e2 to 7c07797 Compare July 23, 2020 15:32

ehelms reviewed Jul 27, 2020

View reviewed changes

ekohl marked this pull request as draft July 27, 2020 13:26

wbclark mentioned this pull request Aug 28, 2020

Fixes #29799 - Move --clear-pulp-content to hooks/ theforeman/foreman-installer#540

Merged

ekohl force-pushed the 30423-app-layout branch from 7c07797 to f627859 Compare September 3, 2020 13:21

ekohl mentioned this pull request Sep 14, 2020

Manage the static assets via a class #121

Merged

ekohl force-pushed the 30423-app-layout branch 2 times, most recently from 4e12059 to 6c1f3cd Compare September 14, 2020 17:50

ekohl force-pushed the 30423-app-layout branch 2 times, most recently from 1667892 to 1fa9ca3 Compare October 5, 2020 16:04

ekohl added the Backwards incompatible label Oct 5, 2020

wbclark reviewed Oct 5, 2020

View reviewed changes

manifests/config.pp Outdated Show resolved Hide resolved

ekohl force-pushed the 30423-app-layout branch from 1fa9ca3 to dd7141c Compare October 5, 2020 16:37

ekohl mentioned this pull request Oct 5, 2020

Refs #30423 - Migrate Pulp media directory theforeman/foreman-installer#583

Merged

ekohl marked this pull request as ready for review October 5, 2020 17:56

ekohl mentioned this pull request Oct 5, 2020

Change the default deployment layout pulp/pulpcore#799

Merged

ekohl force-pushed the 30423-app-layout branch from dd7141c to b225183 Compare October 6, 2020 19:22

ekohl force-pushed the 30423-app-layout branch from b225183 to 4096989 Compare October 9, 2020 16:14

wbclark reviewed Oct 9, 2020

View reviewed changes

wbclark approved these changes Oct 9, 2020

View reviewed changes

theforeman-bot removed the Not yet reviewed label Oct 9, 2020

ehelms merged commit 6a88107 into theforeman:master Oct 9, 2020

ekohl deleted the 30423-app-layout branch October 9, 2020 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #30423 - Change the application layout #115

Fixes #30423 - Change the application layout #115

ekohl commented Jul 21, 2020

ekohl commented Jul 21, 2020

ekohl Jul 22, 2020

ekohl Jul 22, 2020

wbclark Jul 22, 2020

ekohl Jul 22, 2020

wbclark Jul 22, 2020

ekohl Jul 22, 2020

wbclark Jul 22, 2020

wbclark Jul 22, 2020

ekohl Jul 22, 2020

ekohl Jul 27, 2020

ehelms Jul 27, 2020

ekohl Jul 27, 2020

ehelms Jul 27, 2020

ekohl Jul 27, 2020

ekohl commented Jul 27, 2020

wbclark commented Aug 18, 2020

ekohl commented Sep 1, 2020

ekohl commented Oct 5, 2020

ekohl commented Oct 5, 2020

ekohl commented Oct 5, 2020

ekohl commented Oct 9, 2020

wbclark Oct 9, 2020

wbclark left a comment


		There is also `chunked_upload_dir` to configure the undocumented `CHUNKED_UPLOAD_DIR`. This directory stores the temporary files used for files uploaded as chunks.

		When the Apache vhost is managed, `apache_docroot` (default `/var/lib/pulp/docroot`) is also created. This doesn't contain any files, but this makes sure that there aren't any files being served.

	require => File[$pulpcore::static_root],
	subscribe => File[$pulpcore::static_root],

Fixes #30423 - Change the application layout #115

Fixes #30423 - Change the application layout #115

Conversation

ekohl commented Jul 21, 2020

ekohl commented Jul 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Jul 27, 2020

wbclark commented Aug 18, 2020

ekohl commented Sep 1, 2020

ekohl commented Oct 5, 2020

ekohl commented Oct 5, 2020

ekohl commented Oct 5, 2020

ekohl commented Oct 9, 2020

Choose a reason for hiding this comment

wbclark left a comment

Choose a reason for hiding this comment