-
Notifications
You must be signed in to change notification settings - Fork 283
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.
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
(fix) Header propagation rule passthrough #6690
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: af85560a491445f7fd6c9c07 |
This comment has been minimized.
This comment has been minimized.
346acd8
to
cf2d82f
Compare
Header propagation contains logic to prevent headers from being propagated more than once. This was being applied regardless of if a rule matched, thus preventing a first matching rule wins situation. This PR alters the logic so that only when a header is populated then the header is marked as fixed. Note that defaulting a head WILL populate a header, so make sure to include your defaults last in your propagation rules.
cf2d82f
to
4117539
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subtle... should we also clarify in the docs that you should put items with default
last?
|
Docs updated: c7c0969 |
This required improving the test harness to allow env to be passed into the closure.
Tests needed refactoring: 7b129df. Code is cleaner now. |
@mergify queue |
🛑 The pull request has been removed from the queue
|
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks:
You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. If you want to requeue this pull request, you need to post a comment with the text: |
@shorgi I've made some docs changes so it'd be great if you could take a pass. |
CI performance tests
|
Co-authored-by: bryn <bryn@apollographql.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit ea7f255) # Conflicts: # apollo-router/src/plugins/test.rs
Header propagation contains logic to prevent headers from being propagated more than once. This was broken in #6281 which always considered a header propagated regardless if a rule actually matched.
This PR alters the logic so that only when a header is populated then the header is marked as fixed.
The following will now work again:
Note that defaulting a head WILL populate a header, so make sure to include your defaults last in your propagation rules.
Instead make sure that your headers are defaulted last:
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
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. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩