[Threescale 7522] Fix CDN urls for fonts assets #3072

jlledom · 2022-10-06T15:41:33Z

What this PR does / why we need it:

In the STG-SaaS porta instance, Setting the RAILS_ASSET_HOST variable makes all resources to be loaded from the CDN, except the fonts.

Which issue(s) this PR fixes

THREESCALE-7522

Verification steps

Load any screen with the development tools open
In the network tab, check that all resources (Images, Fonts, Js, etc) are loaded from the CDN
Change the value of RAILS_ASSET_HOST and restart the server
In the network tab, check that all resources (Images, Fonts, Js, etc) are loaded from the new CDN

Special notes for your reviewer:

Where the fonts urls come from:

The fonts are originally declared in Patternfly
Instead of simply transpiling the above file to a CSS, Webpack uses javascript to dynamically generate a CSS code equivalent to that, but in runtime. This design allows webpack to replace the CDN url without the need to recompile the assets again, which only happens once during the deploy/image build.
When no __webpack_public_path__ is given in runtime, the CSS stubs are generated using the URL at settings.yml:asset_host during compilation time. Thanks to webpacker, I guess.

The problem:

In the STG-SaaS server, the assets are compiled during the image build, having settings.yml:asset_host == null and thus setting relative urls for fonts by default.
In runtime, the __webpack_public_path__ variable is either empty or not available, then the JS generates a CSS code pointing to fonts using realtive urls, and thus being loaded from the server.
However, the urls are correct in the SaaS server, that's because the CDN url is hardcoded and available during assets compilation time

Implemented solution:

I used a simplification of this workaround, which:

Assumes the assets where compiled without any value at settings.yml:asset_host, thus generating relative paths.
Tricking webpack to prepend the CDN url in runtime.
Assuring the trick is not executed when a CDN is provided during assets compilation, like SaaS deployment does.

Observations:

Probably because of this, I wasn't able to make webpack honor the value of __webpack_public_path__. Then I used the variable window.rails_asset_host instead, which allows us to simplify the solution and set the the value only once at provider.html.slim.
The value of window.rails_asset_host must be set at the entry point of the app, before including any other JS. I placed it at app/views/layouts/provider.html.slim, not sure if it's the right place.
I updated Webpacker to 5.4.3. This doesn't update webpack, only the Rails wrapper gem. I did it because there are no breaking changes from 4.3.0 to 5.4.3 and I found a comment complaining about how webpacker sets WEBPACKER_ASSET_HOST, which I use to determine whether the user is compiling the assets with a default URL. I think it could probably work without the update, so if the update causes problem to any of you, I could try to remove it.
I also updated file-loader to 6.2.0, this is something I need because our old version doesn't support the postTransformPublicPath option, that I need. Again, if this causes problems, I could probably use an older version, as long as it provides the mentioned option.

The CDN url must be hardcoded at settings.yml:asset_host during assets compilation in order to get static assets like CSS files point to the CDN. Otherwise, the assets are generated with relative paths and are not loaded from the CDN, even when settings.yml:asset_host is set during runtime. We don't want to have to provide any CDN url when building porta docker images in order to get the assets correctly precompiled. To avoid that, this trick assumes the assets are generated with relative paths (settings.yml:asset_host = null during compilation time) and prepends the CDN url in runtime. By default, settings.yml:asset_host is set to RAILS_ASSET_HOST, which makes this env variable the only source of truth for CDN urls.

akostadinov

Awesome detective work!

My suggestion is to handle only assets compiled with asset_host == null. We can also edit the precompile task to fail unless it is not blank. Or better alter the value during compilation to prevent mistakes. This will simplify the pull request.

If we cannot alter the setting during compilation, then we may merge this change only once we fully migrate off SaaS legacy. To avoid having to deal with that environment.

Also we need a test that will verify the asset_host is used in all - master, provider and dev portals.

I haven't checked but please make sure that the updated gems are supported in legacy. You can deploy to preview to double check.

config/webpack/environment.js

app/helpers/application_helper.rb

mayorova · 2022-10-07T12:21:21Z

app/views/layouts/provider.html.slim

@@ -6,6 +6,9 @@ html[lang="en" class="pf-m-redhat-font"]
      = content_for?(:title) ? yield(:title) : default_title
      |  | Red Hat 3scale API Management
    = csrf_meta_tag
+    // Is this the entrypoint of the app?


I think it's not the only layout that is used in the app... probably need to add this in others as well.

I think I updated it in all layouts

Co-authored-by: Aleksandar N. Kostadinov <akostadinov@gmail.com>

jlledom · 2022-10-13T12:42:27Z

My suggestion is to handle only assets compiled with asset_host == null. We can also edit the precompile task to fail unless it is not blank. Or better alter the value during compilation to prevent mistakes. This will simplify the pull request.

If we do so, SaaS would stop loading assets from the CDN. At the end, it's really only this line that we could remove if we force asset_host == null. I don't think it's a big deal.

/config/webpack/environment.js#L54

If we cannot alter the setting during compilation, then we may merge this change only once we fully migrate off SaaS legacy. To avoid having to deal with that environment.

That's also OK, we could wait until the migration is done and push a version of this PR without that line linked above. I don't have a strong opinion.

Also we need a test that will verify the asset_host is used in all - master, provider and dev portals.

I tested it locally. But yes, I'll try to figure out how to write tests for this.

I haven't checked but please make sure that the updated gems are supported in legacy. You can deploy to preview to double check.

How can I check that? other than deploying to preview?

akostadinov · 2022-10-13T15:27:45Z

How can I check that? other than deploying to preview?

yes, that's how, also you can install ruby 2.4 locally and bundle install without test and dev deps.

That's also OK, we could wait until the migration is done and push a version of this PR without that line linked above. I don't have a strong opinion.

I would say this, but I don't want you to get into conflicts. Especially the whole TS work going on recently. So I would say, add a TODO description what to do after legacy infra is decommissioned and remove the line afterwards.

Or if you prefer, you can wait.

jlledom · 2022-10-18T07:42:18Z

I would say this, but I don't want you to get into conflicts. Especially the whole TS work going on recently. So I would say, add a TODO description what to do after legacy infra is decommissioned and remove the line afterwards.

I'll wait until the Typescript PR is merged, then I'll fix any conflict, remove any code to support legacy SaaS and wait until the migration to OCP is done to merge this.

We're not going to merge until SaaS is moved to OCP

mayorova · 2022-10-21T09:47:21Z

app/helpers/application_helper.rb

+  end
+
+  def rails_asset_host_url
+    asset_host_enabled = Rails.configuration.asset_host.present?


Not sure I get it - there are two asset_host values in different places in config tree? 🤔

UPDATE: Didn't scroll enough 😅

If I understand correctly Rails.configuration.asset_host is coming from config/environments/{env}.rb, and Rails.configuration.three_scale.asset_host comes from settings.yml.

From the change and the comment in webpack config, it seems to me that we always assume that this asset_host should be empty on compilation time?
If so, shouldn't we force it to be empty, so we don't have double-host issues?

If I understand correctly Rails.configuration.asset_host is coming from config/environments/{env}.rb, and Rails.configuration.three_scale.asset_host comes from settings.yml.

Yes.

From the change and the comment in webpack config, it seems to me that we always assume that this asset_host should be empty on compilation time?

Yes.

If so, shouldn't we force it to be empty, so we don't have double-host issues?

Do you mean force it empty during compilation time? It's the same @akostadinov said. Yes, I agree.

I added a callback that returns an error during assets compilation if the asset host is set. I couldn't find an easy way to just unset the asset host transparently. That would need more time but I can try if you guys think it's needed.

In order to be able to load both Sprockets and Webpacker assets from a CDN, the asset host property must be empty during the webpack assets compilation time. This ensures urls are generated as relative paths and webpack can safely prepend the CDN url to them.

This reverts commit 2c3e4a8

jlledom · 2022-10-24T11:12:50Z

I removed the webpacker update from this PR and moved it to #3083

mayorova · 2022-10-26T12:54:52Z

app/helpers/application_helper.rb

+
+  def rails_asset_host_url
+    asset_host_enabled = Rails.configuration.asset_host.present?
+    asset_host_url = Rails.configuration.three_scale.asset_host.presence


It is still not clear for me why we are using both settings...
Rails.configuration.asset_host and Rails.configuration.three_scale.asset_host
Shouldn't we use just one?...

I guess Rails.configuration.asset_host is the one that should define the actual value, as it's env-specific, and will use the value in Rails.configuration.three_scale.asset_host, if present.

The relevant piece of code is here: /config/environments/production.rb#L109-L121

We could just set Rails.configuration.asset_host to Rails.configuration.three_scale.asset_host and it would work. The first was converted into a lambda function here. I can't get too much info from the commit message: what other assets besides ours are we loading through javascript_include_tag? Maybe we can remove the lambda function.

Hm, I am not sure about the reason for having this function there.
What I mean is: do you think we can we just rely on Rails.configuration.asset_host in this helper?

We need both variables, asset_host_enabled tells us when the CDN is enabled, it can be disabled when DISABLE_DIGEST == 1. In that case we don't care about the value of RAILS_ASSETS: all assets loaded by rails won't include the CDN url, so the ones loaded by webpack shouldn't neither.

It's also possible that asset_host_enabled is true, but there's not provided asset url (asset_host_url = RAILS_ASSET_HOST = <empty>), then the situation is the same, rails won't load any asset and webpack shouldn't neither. So both variables must be true to go ahead and return the CDN url for webpack.

If we rely only on the value of asset_host_enabled, then we need to check for the value of asset_host_url anyway, to decide if we prepend the https:// prefix or not. Now it's done in a way all possibilities are checked in one line:

asset_host_enabled asset_host_url return value

false '' ''

false 'whtaever.cloudfront.net' ''

true '' ''

true 'whtaever.cloudfront.net' 'https://whtaever.cloudfront.net'

In this case, does it make sense to rely only on asset_host_url, if it is set, then prepend. WDYT? Or is asset_host_enabled a rails thing that is tricky to ignore?

If we rely only in asset_host_url, the second row in the table would fail, it would return 'https://whtaever.cloudfront.net/' instead of ''. Since asset_host_enabled == false, it will prepend the URL to webpack assets like fonts and JS files, but it won't use the CDN for all other assets. I think that's not the expected behaviour, if the admin disables the CDN, then it must be disabled for all assets.

If admin wants to disable CDN, admin could empty asset_host_url. Or there is some other issue related to that?

It can be disabled in two ways: by DISABLE_DIGEST == 1 or by RAILS_ASSET_HOST == ''. If the admin disables the CDN by leaving RAILS_ASSET_HOST empty, that will work, but if they want to enable it solely by RAILS_ASSET_HOST it might not work because DISABLE_DIGEST could be 1, and then it would only enable the CDN for webpack but not for Sprockets. We need to enforce that no matter how the assets are disabled or enabled, they must be disabled or enabled for both webpack and sprockets.

github-actions · 2022-11-12T03:49:32Z

This PR is stale because it has not received activity for more than 14 days. Remove stale label or comment or this will be closed in 7 days.

jlledom · 2022-11-14T07:50:22Z

This is not stale but actively waiting for SaaS to be migrated to OCP

github-actions · 2022-12-15T03:36:09Z

This PR is stale because it has not received activity for more than 14 days. Remove stale label or comment or this will be closed in 7 days.

josemigallas

Oh no! My review was Pending all this time!! 🤦🏻 I hope it still makes sense.

josemigallas · 2022-10-24T07:05:44Z

app/helpers/application_helper.rb

+    return '' unless asset_host_enabled && asset_host_url
+
+    "#{request.protocol}#{asset_host_url}"


I think there's no need to check the string twice. Would this be equivalent?

Suggested change

return '' unless asset_host_enabled && asset_host_url

"#{request.protocol}#{asset_host_url}"

return '' unless asset_host_url = Rails.configuration.three_scale.asset_host.presence

"#{request.protocol}#{asset_host_url}"

It's not checking the string twice because only asset_host_url is a string, asset_host_enabled is a boolean. This comment explains why both are required.

josemigallas · 2022-10-24T07:14:09Z

config/environments/test.rb

@@ -64,6 +64,16 @@

  config.assets.compile = ENV.fetch('SKIP_ASSETS', '0') == '0'

+  config.asset_host = ->(source) do
+    # does it exist in /public/assets ?


Not sure what this comment is for. Is it a question or a clarification? If the latter, I think the code is clear enough. However, I would use a self-explanatory name such as exist_in_public_assets before resorting to comments.

OK, I'll fix this

josemigallas · 2022-10-24T07:30:55Z

config/webpacker.yml

@@ -8,7 +8,7 @@ default: &default

  # Additional paths webpack should lookup modules
  # ['app/assets', 'engine/foo/app/assets']
-  resolved_paths: ['app/javascript/src']
+  additional_paths: ['app/javascript/src']


Is this due to the upgrade?

Yes: https://stackoverflow.com/questions/64363427/webpacker-the-resolved-paths-option-has-been-deprecated-use-additional-paths-i

josemigallas · 2022-10-24T07:36:13Z

config/webpack/environment.js

+
+output.publicPath = '';
+Object.assign(fileLoader.use[0].options, {
+  publicPath,


This is a bit confusing. Is this supposed to be the previous or the current value of publicPath? If it's the same value please use the same reference for both. Also for readability's sake I'd suggest not mixing up config and loaders: first update publicPath in one block then get the loader and mutate it in second block. These config files tend to get messy over time if we're not super tidy.

Done (or at least I tried 😛)

josemigallas · 2022-10-24T07:36:55Z

features/asset_host/asset_host.feature

@@ -0,0 +1,32 @@
+Feature: Asset host


Tag can be added at Feature level once.

Suggested change

Feature: Asset host

@javascript

Feature: Asset host

josemigallas · 2022-10-24T09:44:20Z

lib/tasks/webpacker.rake

+namespace :webpacker do
+
+  task :check_asset_host do
+    if Rails.configuration.three_scale.asset_host.presence


Suggested change

if Rails.configuration.three_scale.asset_host.presence

if Rails.configuration.three_scale.asset_host.present?

No the same, presence returns nil or the original value, present? return a boolean.

https://stackoverflow.com/questions/19637499/what-is-the-point-of-objectpresence-in-rails

Still looks better wit h the ? IMO :)

Well, now I think about it, maybe it should return a boolean...

Honestly noticed that but was too lazy to comment on it :) Thought it was too small of a detail.

josemigallas · 2022-10-24T09:57:08Z

lib/tasks/webpacker.rake

+
+  task :check_asset_host do
+    if Rails.configuration.three_scale.asset_host.presence
+      STDERR.puts "*** Asset host must be null during compilation time. See https://github.com/3scale/porta/pull/3072 ***"


I think the message should be more practical/meaningful. What about we say:

*** Asset host is not supported. Run <command> and try again. ***

With some instruction to remove it? My reasoning is not everyone will be familiar with rake, webpack or with the assets pipeline or they will simply don't what to know about it and will just want to run the app.
And pointing to the PR in a comment, aimed to developers.

But it's not true that Asset host is not supported, it only needs to be empty during the compilation time, but it can have a value in runtime.

josemigallas · 2022-10-24T09:59:57Z

package.json

@@ -44,7 +44,7 @@
    "eslint-plugin-promise": "^4.1.1",
    "eslint-plugin-react": "^7.13.0",
    "eslint-plugin-standard": "^4.0.0",
-    "file-loader": "^1.1.6",
+    "file-loader": "^6.2.0",


That is a bold move 😄 No breaking changes or caveats?

Don't you trust me? 😢

I didn't try this thoroughly, I only used it in my development environment and my dev cluster and noticed no incidences, also all tests pass. So how could I be sure I'm not breaking anything?

You can probably read through the changelog for any breaking changes I guess.

This is the list of breaking changes:

use md4 by default for hashing (chore(deps): update webpack-contrib/file-loader#369) (ad39022)

minimum required nodejs version is 10.13.0

rename the esModules option to esModule

switch to ES modules by default (the option esModule is true by default)

removed the useRelativePath option. It is dangerously and break url when you use multiple entry points.

drop support for webpack < 4

Only the nodejs version seems relevant for me. Which one is installed in SaaS?

josemigallas · 2022-10-24T10:55:45Z

test/unit/helpers/application_helper_test.rb

+      @asset_host = 'cdn.3scale.test.localhost'
+    end
+
+    attr_reader :request


Suggested change

attr_reader :request

attr_reader :request, :asset_host

josemigallas · 2022-10-24T10:58:22Z

test/unit/helpers/application_helper_test.rb

+      Rails.configuration.asset_host = nil
+      Rails.configuration.three_scale.asset_host = @asset_host


Suggested change

Rails.configuration.asset_host = nil

Rails.configuration.three_scale.asset_host = @asset_host

Rails.configuration.expects(:asset_host).returns(nil)

Rails.configuration.three_scale.expects(:asset_host).returns(@asset_host)

I applied this, also for other tests

github-actions · 2023-01-04T03:34:19Z

This PR is stale because it has not received activity for more than 14 days. Remove stale label or comment or this will be closed in 7 days.

github-actions · 2023-01-19T03:34:44Z

This PR is stale because it has not received activity for more than 14 days. Remove stale label or comment or this will be closed in 7 days.

github-actions · 2023-02-03T03:35:26Z

This PR is stale because it has not received activity for more than 14 days. Remove stale label or comment or this will be closed in 7 days.

jlledom · 2023-02-15T13:50:27Z

Closing in favor of #3199

Detailed info about this commit in it's original PR: #3072

jlledom added 3 commits October 6, 2022 11:25

THREESCALE-7522: Update webpacker to 5.4.3

2c3e4a8

THREESCALE-7522: Update file-loader to 6.2.0

e0010c3

akostadinov reviewed Oct 6, 2022

View reviewed changes

config/webpack/environment.js Outdated Show resolved Hide resolved

mayorova reviewed Oct 7, 2022

View reviewed changes

app/helpers/application_helper.rb Show resolved Hide resolved

mayorova reviewed Oct 7, 2022

View reviewed changes

jlledom and others added 2 commits October 11, 2022 10:19

Folllorw Red Hat style guidelines on environment.js

6e9c99d

Co-authored-by: Aleksandar N. Kostadinov <akostadinov@gmail.com>

THREESCALE-7522: Set the CDN URL in all layouts

f29bb2d

jlledom force-pushed the THREESCALE-7522 branch from 1e00e85 to f29bb2d Compare October 13, 2022 12:30

THREESCALE-7522: Remove TODOs

468c0db

jlledom added 3 commits October 18, 2022 15:40

THREESCALE-7522: Add unit tests for rails_asset_host_url

44c1e54

THREESCALE-7522: Add cucumbers to ensure assets are loaded from the CDN

19f3fc0

THREESCALE-7522: Remove exception for SaaS

401fb66

We're not going to merge until SaaS is moved to OCP

jlledom marked this pull request as ready for review October 20, 2022 14:10

mayorova reviewed Oct 21, 2022

View reviewed changes

josemigallas assigned jlledom Oct 24, 2022

Revert "THREESCALE-7522: Update webpacker to 5.4.3"

9d05346

This reverts commit 2c3e4a8

jlledom mentioned this pull request Oct 24, 2022

Update webpacker to 5.4.3 #3083

Merged

jlledom mentioned this pull request Oct 24, 2022

Update webpacker to 5.4.3 #3085

Merged

mayorova reviewed Oct 26, 2022

View reviewed changes

jlledom marked this pull request as draft October 27, 2022 09:04

github-actions bot added the Stale label Nov 12, 2022

github-actions bot removed the Stale label Nov 15, 2022

github-actions bot added the Stale label Nov 30, 2022

jlledom removed the Stale label Nov 30, 2022

3scale deleted a comment from github-actions bot Nov 30, 2022

github-actions bot added the Stale label Dec 15, 2022

jlledom removed the Stale label Dec 15, 2022

josemigallas reviewed Dec 19, 2022

View reviewed changes

jlledom added 5 commits December 19, 2022 14:56

Asset host config: use a self-explanatory variable name

727a33c

CDN Url: Webpacker config: Make code clearer

f67119c

CDN Cucumbers: Use only one javascript tag

13398a2

Refactor CDN asset host tests

a9a771f

Asset host tests: mock rails config

90fb76b

jlledom force-pushed the THREESCALE-7522 branch from f13e51b to 90fb76b Compare December 19, 2022 16:49

Rake task: check_asset_host: use present?

8a79e58

github-actions bot added the Stale label Jan 4, 2023

jlledom removed the Stale label Jan 4, 2023

github-actions bot added the Stale label Jan 19, 2023

jlledom removed the Stale label Jan 19, 2023

github-actions bot added the Stale label Feb 3, 2023

jlledom removed the Stale label Feb 3, 2023

jlledom mentioned this pull request Feb 7, 2023

Fix CDN assets for SaaS in OCP #3199

Merged

jlledom closed this Feb 15, 2023

jlledom added a commit that referenced this pull request Feb 27, 2023

Fix CDN assets for SaaS in OCP (#3199)

d025228

Detailed info about this commit in it's original PR: #3072

jlledom deleted the THREESCALE-7522 branch March 13, 2023 08:18

asset_host_enabled	asset_host_url	return value
false	''	''
false	'whtaever.cloudfront.net'	''
true	''	''
true	'whtaever.cloudfront.net'	'https://whtaever.cloudfront.net'

		return '' unless asset_host_enabled && asset_host_url

		"#{request.protocol}#{asset_host_url}"

	if Rails.configuration.three_scale.asset_host.presence
	if Rails.configuration.three_scale.asset_host.present?

		Rails.configuration.asset_host = nil
		Rails.configuration.three_scale.asset_host = @asset_host

[Threescale 7522] Fix CDN urls for fonts assets #3072

[Threescale 7522] Fix CDN urls for fonts assets #3072

Conversation

jlledom commented Oct 6, 2022 • edited Loading

Where the fonts urls come from:

The problem:

Implemented solution:

akostadinov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlledom commented Oct 13, 2022

akostadinov commented Oct 13, 2022

jlledom commented Oct 18, 2022

mayorova Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlledom commented Oct 24, 2022

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

github-actions bot commented Nov 12, 2022

jlledom commented Nov 14, 2022

github-actions bot commented Dec 15, 2022

josemigallas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlledom Dec 19, 2022 • edited Loading

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

jlledom Dec 19, 2022 • edited Loading

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

jlledom Dec 19, 2022 • edited Loading

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

github-actions bot commented Jan 4, 2023

github-actions bot commented Jan 19, 2023

github-actions bot commented Feb 3, 2023

jlledom commented Feb 15, 2023

jlledom commented Oct 6, 2022 •

edited

Loading

mayorova Oct 21, 2022 •

edited

Loading

jlledom Dec 19, 2022 •

edited

Loading

jlledom Dec 19, 2022 •

edited

Loading

jlledom Dec 19, 2022 •

edited

Loading