From ba654d1790fdd688b232cc9bf946559debec4172 Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Thu, 15 Sep 2022 17:52:53 +0200 Subject: [PATCH] Improve caching on raw endpoints - Serves strong ETags allowing CF to cache those resources - Set appropriate headers on responses to allow optimizing the caching --- app/controllers/concerns/sends_blob.rb | 22 ++++++++++++---- .../improve_blobs_cache_headers.yml | 8 ++++++ .../designs/raw_images_controller_spec.rb | 2 +- .../projects/raw_controller_spec.rb | 25 ++++++++++++++++--- .../wiki_actions_shared_examples.rb | 2 +- 5 files changed, 49 insertions(+), 10 deletions(-) create mode 100644 config/feature_flags/development/improve_blobs_cache_headers.yml diff --git a/app/controllers/concerns/sends_blob.rb b/app/controllers/concerns/sends_blob.rb index 381f2eba3523e4..3cf260c9f1bff8 100644 --- a/app/controllers/concerns/sends_blob.rb +++ b/app/controllers/concerns/sends_blob.rb @@ -27,12 +27,14 @@ def send_blob(repository, blob, inline: true, allow_caching: false) private def cached_blob?(blob, allow_caching: false) - stale = stale?(etag: blob.id) # The #stale? method sets cache headers. - - # Because we are opinionated we set the cache headers ourselves. - response.cache_control[:public] = allow_caching + stale = + if Feature.enabled?(:improve_blobs_cache_headers) + stale?(strong_etag: blob.id) + else + stale?(etag: blob.id) + end - response.cache_control[:max_age] = + max_age = if @ref && @commit && @ref == @commit.id # rubocop:disable Gitlab/ModuleWithInstanceVariables # This is a link to a commit by its commit SHA. That means that the blob # is immutable. The only reason to invalidate the cache is if the commit @@ -44,6 +46,16 @@ def cached_blob?(blob, allow_caching: false) Blob::CACHE_TIME end + # Because we are opinionated we set the cache headers ourselves. + if Feature.enabled?(:improve_blobs_cache_headers) + expires_in(max_age, + public: allow_caching, must_revalidate: true, stale_if_error: 5.minutes, + stale_while_revalidate: 1.minute, 's-maxage': 1.minute) + else + response.cache_control[:public] = allow_caching + response.cache_control[:max_age] = max_age + end + !stale end diff --git a/config/feature_flags/development/improve_blobs_cache_headers.yml b/config/feature_flags/development/improve_blobs_cache_headers.yml new file mode 100644 index 00000000000000..33fb9669106bdf --- /dev/null +++ b/config/feature_flags/development/improve_blobs_cache_headers.yml @@ -0,0 +1,8 @@ +--- +name: improve_blobs_cache_headers +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98110 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/374126 +milestone: '15.5' +type: development +group: group::source code +default_enabled: false diff --git a/spec/controllers/projects/design_management/designs/raw_images_controller_spec.rb b/spec/controllers/projects/design_management/designs/raw_images_controller_spec.rb index 55ab0f0eefaa68..2d39e0e5317eab 100644 --- a/spec/controllers/projects/design_management/designs/raw_images_controller_spec.rb +++ b/spec/controllers/projects/design_management/designs/raw_images_controller_spec.rb @@ -132,7 +132,7 @@ subject expect(response.header['ETag']).to be_present - expect(response.header['Cache-Control']).to eq("max-age=60, private") + expect(response.header['Cache-Control']).to eq("max-age=60, private, must-revalidate, stale-while-revalidate=60, stale-if-error=300, s-maxage=60") end end diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb index e0d88fa799f663..1c9aafacbd9ab5 100644 --- a/spec/controllers/projects/raw_controller_spec.rb +++ b/spec/controllers/projects/raw_controller_spec.rb @@ -247,9 +247,11 @@ def request_file sign_in create(:user) request_file - expect(response.cache_control[:public]).to eq(true) - expect(response.cache_control[:max_age]).to eq(60) + expect(response.headers['ETag']).to eq("\"bdd5aa537c1e1f6d1b66de4bac8a6132\"") expect(response.cache_control[:no_store]).to be_nil + expect(response.header['Cache-Control']).to eq( + 'max-age=60, public, must-revalidate, stale-while-revalidate=60, stale-if-error=300, s-maxage=60' + ) end context 'when a public project has private repo' do @@ -260,7 +262,9 @@ def request_file sign_in user request_file - expect(response.header['Cache-Control']).to include('max-age=60, private') + expect(response.header['Cache-Control']).to eq( + 'max-age=60, private, must-revalidate, stale-while-revalidate=60, stale-if-error=300, s-maxage=60' + ) end end @@ -274,6 +278,21 @@ def request_file expect(response).to have_gitlab_http_status(:not_modified) end end + + context 'when improve_blobs_cache_headers disabled' do + before do + stub_feature_flags(improve_blobs_cache_headers: false) + end + + it 'uses weak etags with a restricted set of headers' do + sign_in create(:user) + request_file + + expect(response.headers['ETag']).to eq("W/\"bdd5aa537c1e1f6d1b66de4bac8a6132\"") + expect(response.cache_control[:no_store]).to be_nil + expect(response.header['Cache-Control']).to eq('max-age=60, public') + end + end end end end diff --git a/spec/support/shared_examples/controllers/wiki_actions_shared_examples.rb b/spec/support/shared_examples/controllers/wiki_actions_shared_examples.rb index 885c0229038a87..a595680ad02a45 100644 --- a/spec/support/shared_examples/controllers/wiki_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/wiki_actions_shared_examples.rb @@ -300,7 +300,7 @@ expect(response.headers['Content-Disposition']).to match(/^inline/) expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq('true') expect(response.cache_control[:public]).to be(false) - expect(response.headers['Cache-Control']).to eq('max-age=60, private') + expect(response.headers['Cache-Control']).to eq('max-age=60, private, must-revalidate, stale-while-revalidate=60, stale-if-error=300, s-maxage=60') end end end -- GitLab