Fixes #28739: Fix static asset caching when using Puma

[ehelms]

No description provided.

[ekohl]

I don't think this works because I don't see expire headers. Compare:

$ curl -I https://foreman.example.com/webpack/bundle-c61d31ff17f4bee60391.css
HTTP/1.1 200 OK
Date: Thu, 09 Jan 2020 12:25:13 GMT
Server: Apache
Last-Modified: Tue, 10 Dec 2019 18:31:56 GMT
Content-Type: text/css
Vary: Accept-Encoding
Strict-Transport-Security: max-age=631139040; includeSubdomains
X-Frame-Options: sameorigin
X-Content-Type-Options: nosniff
X-XSS-Protection: 1; mode=block
X-Download-Options: noopen
X-Permitted-Cross-Domain-Policies: none
Content-Security-Policy: default-src 'self'; child-src 'self'; connect-src 'self' ws: wss:; img-src 'self' data: *.gravatar.com; script-src 'unsafe-eval' 'unsafe-inline' 'self'; style-src 'unsafe-inline' 'self'
Content-Length: 35810
Via: 1.1 foreman.example.com
Cache-Control: max-age=31536000
Expires: Fri, 08 Jan 2021 12:25:13 GMT

When I add ProxyPass /webpack !:

$  curl -I https://foreman.example.com/webpack/bundle-c61d31ff17f4bee60391.css
HTTP/1.1 200 OK
Date: Thu, 09 Jan 2020 12:24:52 GMT
Server: Apache
Last-Modified: Tue, 10 Dec 2019 18:31:56 GMT
Accept-Ranges: bytes
Content-Length: 35810
Vary: Accept-Encoding
Cache-Control: max-age=31536000
Expires: Fri, 08 Jan 2021 12:24:52 GMT
Content-Type: text/css

[ehelms]

A bit confused here, both of your examples show Expires and Cache-Control headers.

[ekohl]

So your change is an improvement, but you still hit the application server. The hint is that the security headers are added in the Rails code. IMHO we want to reduce the load on the actual application server if we can.

[ehelms]

Passenger does the same thing today as far as I can tell. Apache then caches after the first time it's fetched. In the Passenger case, Passenger is the application server.

Why do I think it works this way? If you don't proxy requests to /assets or /webpack and try to let Apache serve them statically, all plugins will fail to load their serve assets because their assets don't exist in /usr/share/foreman/public. In order to adopt Apache serving it statically we will need to change our installations to identify and put plugin public folders as symlinks to the main /usr/share/foreman/public directory.

[ekohl]

Interesting, I learned something new today. I guess that's why we have this PassengerEnabled off for pulp.

[ehelms]

I suppose you would want this model as well if you ever wanted to deploy multiple application servers exposed directly with a load balancer separately. We generally don't have a ton of UI users hammering our application so requesting once and then having that cached from there on out I think should be fine. Most of our app load comes from the API.

[ekohl]

So on a Foreman with passenger installation (1.23.1):

$ curl -Ik https://foreman.example.com/assets/Redhat-ba7bc908b35f4073bc0565a27f635b706b71288581b26a94c8a3a390ee68c98e.png
HTTP/1.1 200 OK
Date: Fri, 10 Jan 2020 15:20:22 GMT
Server: Apache
Last-Modified: Mon, 04 Nov 2019 14:43:11 GMT
Accept-Ranges: bytes
Content-Length: 1914
Cache-Control: max-age=31536000
Expires: Sat, 09 Jan 2021 15:20:22 GMT
Content-Type: image/png

So this doesn't appear to hit Passenger and just straight up serve the file via Apache.

[ehelms]

I believe this is only true for Foreman assets and not plugins, this is what I get when using passenger:

[root@centos7-katello-nightly vagrant]# curl -I https://192.168.121.244/assets/bastion_katello/bastion_katello-2f98ac7d4342ebfb94ad6f0dd73f2ea270b67d5c62bfd6825c7ae9838eba24bc.css -k
HTTP/1.1 200 OK
Date: Mon, 13 Jan 2020 13:08:46 GMT
Server: Apache
Vary: Accept-Encoding
Cache-Control: no-cache
X-Request-Id: 774494ee-a89e-40b0-9a9d-512db7321f07
X-Runtime: 0.000449
Strict-Transport-Security: max-age=631139040; includeSubdomains
X-Frame-Options: sameorigin
X-Content-Type-Options: nosniff
X-XSS-Protection: 1; mode=block
X-Download-Options: noopen
X-Permitted-Cross-Domain-Policies: none
Content-Security-Policy: default-src 'self'; child-src 'self'; connect-src 'self' ws: wss:; img-src 'self' data:; script-src 'unsafe-eval' 'unsafe-inline' 'self'; style-src 'unsafe-inline' 'self'
X-Powered-By: Phusion Passenger 4.0.53
Last-Modified: Mon, 30 Dec 2019 12:36:06 GMT
Content-Length: 1598
Status: 200 OK
Cache-Control: max-age=31536000
Expires: Tue, 12 Jan 2021 13:08:46 GMT
Content-Type: text/css

For me, this implies that with this change we are still caching after the initial request of an asset, and this is only changing for core Foreman assets as plugins are already treated this way. That is, plugin assets are not served statically from Apache and this levels the playing field.

From testing, I saw no change in page load times especially after the first request given all of the cache headers are being set.

If we want all assets to be served statically from Apache we need to make a bigger change overall that affects both deployment models. Given, if we support serving static assets from Apache with Puma we break plugins.

To support serving static assets with Apache, we'd have to for all plugins likely add to their RPM and Deb builds a symlink from /usr/share/foreman/public/assets to their install directory's public asset and webpack folders.

[ekohl]

It appears you might be correct. With webpack we do symlink but this isn't the case with the Rails assets pipeline. What kind of rabbit hole did we just uncover.

With all of the above in mind, I think that we should merge this now. It's still a slight regression for Foreman and webpack resources compared to the Passenger setup, but by correctly adding headers it's at least mitigated on subsequent page loads.

[ekohl]

It appears you might be correct. With webpack we do symlink but this isn't the case with the Rails assets pipeline. What kind of rabbit hole did we just uncover.

With all of the above in mind, I think that we should merge this now. It's still a slight regression for Foreman and webpack resources compared to the Passenger setup, but by correctly adding headers it's at least mitigated on subsequent page loads.

[ekohl]

Before I hit the green merge button, can we make a Redmine issue for this as well?

[ehelms]

Will do, I think the indenting is also a bit off so I will fix that

[ekohl]

After our discussion I think that your new implementation should actually work for passenger as well. My only concern is that you have a LocationMatch which matches anything in the path. That means that it can also apply to a resource which has the name assets in it (/hosts/assets01.example.com). It should take $suburi into account and match the beginning of the path (^) to avoid this.

After that, I think there's no need to make a distinction between Passenger and non-Passenger.

[ehelms]

Updated to include start of line

[ehelms]

This now handles the assets one way for both Passenger and Puma

[ekohl]

This is now a static file. Should we change it in apache.pp to call file('foreman/_assets.conf') and move it to files?

[ehelms]

Moved to files

[ekohl]

Looks like the unit tests expect the old <Directory> statement, other than that I think this can be merged.

[ehelms]

The same Debian failure as is on master. Merging.