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

fix directive conflicts in cache control header #6543

Merged
merged 8 commits into from
Jan 29, 2025
Merged

fix directive conflicts in cache control header #6543

merged 8 commits into from
Jan 29, 2025

Conversation

bnjjj
Copy link
Contributor

@bnjjj bnjjj commented Jan 14, 2025 •
edited
Loading

Unnnecessary cache-control directives are created in cache-control header. The router will now filter out unnecessary values from the cache-control header when the request resolves. So if there's max-age=10, no-cache, must-revalidate, no-store, the expected value for the cache-control header would simply be no-store. Please see the MDN docs for justification of this reasoning: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#preventing_storing

Fixes #6441

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

  • Changes are compatible1
  • Documentation2 completed
  • Performance impact assessed and acceptable
  • Tests added and passing3
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Note any exceptions here

Notes

Footnotes

  1. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩

  2. Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩

  3. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Jan 14, 2025 •
edited
Loading

✅ Docs preview ready

The preview is ready to be viewed. View the preview

File Changes

0 new, 1 changed, 0 removed 
* graphos/reference/migration/from-router-v1.mdx

Build ID: bcd42b0fd1c0883e3a8b9768

URL: https://www.apollographql.com/docs/deploy-preview/bcd42b0fd1c0883e3a8b9768

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@router-perf
Copy link

router-perf bot commented Jan 17, 2025

CI performance tests

  • connectors-const - Connectors stress test that runs with a constant number of users
  • const - Basic stress test that runs with a constant number of users
  • demand-control-instrumented - A copy of the step test, but with demand control monitoring and metrics enabled
  • demand-control-uninstrumented - A copy of the step test, but with demand control monitoring enabled
  • enhanced-signature - Enhanced signature enabled
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
  • events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
  • events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
  • extended-reference-mode - Extended reference mode enabled
  • large-request - Stress test with a 1 MB request payload
  • no-tracing - Basic stress test, no tracing
  • reload - Reload test over a long period of time at a constant rate of users
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • step-local-metrics - Field stats that are generated from the router rather than FTV1
  • step-with-prometheus - A copy of the step test with the Prometheus metrics exporter enabled
  • step - Basic stress test that steps up the number of users over time
  • xlarge-request - Stress test with 10 MB request payload
  • xxlarge-request - Stress test with 100 MB request payload

@bnjjj bnjjj marked this pull request as ready for review January 29, 2025 12:47
@bnjjj bnjjj requested review from a team as code owners January 29, 2025 12:47
@bnjjj bnjjj changed the title failing test when there is conflicts in cache control fix directive conflicts in cache control header Jan 29, 2025
@bnjjj bnjjj requested a review from IvanGoncharov January 29, 2025 13:29
IvanGoncharov
IvanGoncharov requested changes Jan 29, 2025
View reviewed changes
Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

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

I think we should consider moving https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#preventing_storing
logic here:

router/apollo-router/src/plugins/cache/cache_control.rs

Line 268 in f923298

fn merge_inner(&self, other: &CacheControl, now: u64) -> CacheControl {

Because:

In theory, if directives are conflicted, the most restrictive directive should be honored.

So we should handle other conflicting directives like: max-age=0, max-age=60
And already have this code here:

router/apollo-router/src/plugins/cache/cache_control.rs

Lines 275 to 278 in f923298

(Some(ttl1), Some(ttl2)) => Some(std::cmp::min(
self.update_ttl(ttl1, now),
other.update_ttl(ttl2, now),
)),

So I think the same function should also have logic that no-cache and no-store wipe out other directives.
I think it would be better solution since we have some logic around CacheControl and also store it in context that means we apply no-cache and no-store more consistently across the plugin.

Because in current version we apply no-cache and no-store only before we send response back to a client.

@bnjjj bnjjj requested a review from IvanGoncharov January 29, 2025 14:33
@bnjjj
add comment
2facc3d 
Signed-off-by: Benjamin <[email protected]>
@bnjjj bnjjj added the backport-1.x Backport this PR to 1.x label Jan 29, 2025
@IvanGoncharov
Copy link
Member

@bnjjj I think we should have changeset entry for it.

@bnjjj
add changelog
b6a6d26 
Signed-off-by: Benjamin <[email protected]>
@bnjjj bnjjj requested a review from a team as a code owner January 29, 2025 15:09
@bnjjj bnjjj enabled auto-merge (squash) January 29, 2025 15:11
@bnjjj bnjjj merged commit 620acac into dev Jan 29, 2025
15 checks passed
@bnjjj bnjjj deleted the bnjjj/fix_6441 branch January 29, 2025 15:26
@IvanGoncharov
Copy link
Member

@Mergifyio backport 1.x

@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 29, 2025 •
edited
Loading

backport 1.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jan 29, 2025
@bnjjj @mergify
fix directive conflicts in cache control header (#6543)
243f7c2 
Signed-off-by: Benjamin <[email protected]>
Co-authored-by: Ivan Goncharov <[email protected]>
(cherry picked from commit 620acac)
@mergify mergify bot mentioned this pull request Jan 29, 2025
Merged
6 tasks
bnjjj pushed a commit that referenced this pull request Jan 29, 2025
@mergify
fix directive conflicts in cache control header (backport #6543) (#6691)
ff5fef1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IvanGoncharov IvanGoncharov IvanGoncharov approved these changes

Assignees
No one assigned
Labels
backport-1.x Backport this PR to 1.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Entity Caching - Router can propagate unnecessary cache-control directives to client
3 participants
@bnjjj @svc-apollo-docs @IvanGoncharov