Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 17 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2c3e4a8
THREESCALE-7522: Update webpacker to 5.4.3
jlledom Oct 6, 2022
e0010c3
THREESCALE-7522: Update file-loader to 6.2.0
jlledom Oct 6, 2022
295a331
THREESCALE-7522: Decouple CDN url from assets compilation
jlledom Oct 6, 2022
6e9c99d
Folllorw Red Hat style guidelines on environment.js
jlledom Oct 11, 2022
f29bb2d
THREESCALE-7522: Set the CDN URL in all layouts
jlledom Oct 13, 2022
468c0db
THREESCALE-7522: Remove TODOs
jlledom Oct 13, 2022
44c1e54
THREESCALE-7522: Add unit tests for rails_asset_host_url
jlledom Oct 18, 2022
19f3fc0
THREESCALE-7522: Add cucumbers to ensure assets are loaded from the CDN
jlledom Oct 20, 2022
401fb66
THREESCALE-7522: Remove exception for SaaS
jlledom Oct 20, 2022
3f9cdf0
THREESCALE-7522: Prevent compiling assets w/ asset host
jlledom Oct 21, 2022
9d05346
Revert "THREESCALE-7522: Update webpacker to 5.4.3"
jlledom Oct 24, 2022
727a33c
Asset host config: use a self-explanatory variable name
jlledom Dec 19, 2022
f67119c
CDN Url: Webpacker config: Make code clearer
jlledom Dec 19, 2022
13398a2
CDN Cucumbers: Use only one javascript tag
jlledom Dec 19, 2022
a9a771f
Refactor CDN asset host tests
jlledom Dec 19, 2022
90fb76b
Asset host tests: mock rails config
jlledom Dec 19, 2022
8a79e58
Rake task: check_asset_host: use present?
jlledom Dec 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
View file
Open in desktop
Expand Up @@ -248,7 +248,7 @@ group :development, :test do
gem 'unicorn-rails'
end

gem 'webpacker', '~> 4'
gem 'webpacker', '~> 5'

gem 'developer_portal', path: 'lib/developer_portal'
gem 'unicorn', require: false, group: %i[production preview]
Expand Down
12 changes: 7 additions & 5 deletions Gemfile.lock
View file
Open in desktop
Expand Up @@ -551,7 +551,7 @@ GEM
ruby-openid (>= 2.1.8)
rack-protection (2.0.4)
rack
rack-proxy (0.6.5)
rack-proxy (0.7.4)
rack
rack-test (1.1.0)
rack (>= 1.0, < 3)
Expand Down Expand Up @@ -716,6 +716,7 @@ GEM
selenium-webdriver (3.142.7)
childprocess (>= 0.5, < 4.0)
rubyzip (>= 1.2.2)
semantic_range (3.0.0)
shoulda (4.0.0)
shoulda-context (~> 2.0)
shoulda-matchers (~> 4.0)
Expand Down Expand Up @@ -843,10 +844,11 @@ GEM
addressable (>= 2.3.6)
crack (>= 0.3.2)
hashdiff (>= 0.4.0, < 2.0.0)
webpacker (4.3.0)
activesupport (>= 4.2)
webpacker (5.4.3)
activesupport (>= 5.2)
rack-proxy (>= 0.6.1)
railties (>= 4.2)
railties (>= 5.2)
semantic_range (>= 2.3.0)
webrick (1.7.0)
webrobots (0.1.2)
websocket-driver (0.7.5)
Expand Down Expand Up @@ -1039,7 +1041,7 @@ DEPENDENCIES
unicorn
unicorn-rails
webmock (~> 3.8.0)
webpacker (~> 4)
webpacker (~> 5)
will_paginate (~> 3.3)
with_env
xpath (~> 3.2.0)
Expand Down
10 changes: 10 additions & 0 deletions app/helpers/application_helper.rb
View file
Open in desktop
Expand Up @@ -364,4 +364,14 @@ def has_out_of_date_configuration?(service)
return unless service
service.pending_affecting_changes?
end

#TODO: Tests pending for this
def rails_asset_host_url
asset_host_enabled = Rails.configuration.asset_host.present?
jlledom marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@mayorova mayorova Oct 21, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

asset_host_url = Rails.configuration.three_scale.asset_host.presence
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return '' unless asset_host_enabled && asset_host_url

"#{request.protocol}#{asset_host_url}"
Comment on lines +376 to +378
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}"

Copy link
Contributor Author

@jlledom jlledom Dec 19, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end
3 changes: 3 additions & 0 deletions app/views/layouts/provider.html.slim
View file
Open in desktop
Expand Up @@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I updated it in all layouts

javascript:
window.rails_asset_host = "#{rails_asset_host_url}";
= javascript_pack_tag 'PF4Styles/base'
= render 'provider/theme'
= render 'provider/analytics'
Expand Down
27 changes: 27 additions & 0 deletions config/webpack/environment.js
View file
Open in desktop
Expand Up @@ -37,4 +37,31 @@ environment.loaders.append('yaml', {
type: 'json'
})

/* 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
jlledom marked this conversation as resolved.
Show resolved Hide resolved
* the assets are generated with relative paths (settings.yml:asset_host = null during compilation time). The next code
* is executed by webpack when compiling the assets, and sets the variable `postTransformPublicPath` to an arrow
* function which will prepend the CDN url in runtime.
*
* WEBPACKER_ASSET_HOST contains the value of settings.yml:asset_host in compilation time. When not null, the assets
* will refer to fonts or images using an absolute URL, in this case we don't want to implement the trick that prepends
* the CDN url, that would generate incorrect urls with duplicated domain name.
*
* TODO: Link to the PR
* TODO: Link to the github link where this solution comes from
*/
if(!process.env.WEBPACKER_ASSET_HOST) {
const { output } = environment.config;
const { publicPath } = output;
const fileLoader = environment.loaders.get('file');

output.publicPath = '';
Object.assign(fileLoader.use[0].options, {
publicPath,
postTransformPublicPath: (p) => `window.rails_asset_host + ${p}`
});
}

module.exports = environment
2 changes: 1 addition & 1 deletion config/webpacker.yml
View file
Open in desktop
Expand Up @@ -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']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this due to the upgrade?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# Reload manifest.json on all requests so we reload latest compiled packs
cache_manifest: false
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you trust me? 😢

Copy link
Contributor Author

@jlledom jlledom Dec 19, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

"flow-bin": "^0.152.0",
"flow-typed": "^3.3.1",
"hosted-git-info": "^2.7.1",
Expand Down Expand Up @@ -73,7 +73,7 @@
"@patternfly/react-icons": "^3.14.18",
"@patternfly/react-styles": "^4.8.2",
"@patternfly/react-table": "^4.18.14",
"@rails/webpacker": "^4.3.0",
"@rails/webpacker": "^5.4.3",
"@stripe/react-stripe-js": "^1.1.2",
"@stripe/stripe-js": "^1.11.0",
"babel-loader": "^8.0.6",
Expand Down