fix(router)!: prevent set from being forwarded to client#2686
Open
fix(router)!: prevent set from being forwarded to client#2686
Conversation
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors response header rule execution so Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2686 +/- ##
==========================================
+ Coverage 59.41% 65.88% +6.46%
==========================================
Files 238 254 +16
Lines 25867 26449 +582
==========================================
+ Hits 15370 17427 +2057
+ Misses 8994 7636 -1358
+ Partials 1503 1386 -117
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@docs-website/router/proxy-capabilities/subgraph-response-header-operations.mdx`:
- Line 67: The sentence describing the `set` operation overpromises reuse —
update the wording so it clarifies that only `Cache-Control` values stored via
the `set` operation are reused by the router, while other headers set this way
are neither forwarded to the client nor reused later; reference the operations
`set` and `propagate` and the header `Cache-Control` when adjusting the sentence
on that line.
In `@router/core/header_rule_engine.go`:
- Around line 460-464: The code checks rule.Name == cacheControlKey
case-sensitively, so canonicalize or perform a case-insensitive compare before
the Cache-Control special-case branch: replace the equality check with a
case-insensitive comparison (e.g., strings.EqualFold(rule.Name, cacheControlKey)
or compare strings.ToLower(rule.Name) to a lowercased cacheControlKey) in the
block that currently references rule.Name and cacheControlKey so
propagation.setCacheControl is correctly set when header names differ only by
case; keep the existing propagation.header.Set(rule.Name, rule.Value) behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e257883-8e4d-4cbe-a0f7-e7a56d4e36b2
📒 Files selected for processing (6)
docs-website/router/proxy-capabilities/adjusting-cache-control.mdxdocs-website/router/proxy-capabilities/subgraph-response-header-operations.mdxrouter-tests/protocol/header_propagation_race_test.gorouter-tests/protocol/header_set_test.gorouter/core/header_rule_engine.gorouter/core/header_rule_engine_test.go
SkArchon
force-pushed
the
milinda/eng-8718-weird-response-header-forwarding-logic
branch
from
fcce914 to
874bce1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs-website/router/proxy-capabilities/adjusting-cache-control.mdx`:
- Around line 157-165: The YAML example under cache_control_policy is invalid:
the list item is incorrectly indented under value and the subgraphs key is
missing; update the block for the cache_control_policy config (keys:
cache_control_policy, enabled, value) by adding a top-level subgraphs: key and
dedenting the list so the - name: "specific-subgraph" and its value:
"max-age=5400, public" are entries under subgraphs, producing valid YAML that
readers can copy/paste.
In `@router/core/header_rule_engine_test.go`:
- Around line 137-161: The test incorrectly expects subgraph-specific cache
rules to be appended to HeaderRules.AfterSubgraphResponse; update the assertions
in the AddCacheControlPolicyToRules tests so the global policy is asserted only
against result.AfterSubgraphResponse (e.g., check length 1 and Default ==
"max-age=300") and assert the subgraph-specific rule is checked via
result.Subgraphs["sg1"].Response (Default == "max-age=60"), referencing
AddCacheControlPolicyToRules, config.CacheControlPolicy and
HeaderRules.AfterSubgraphResponse/Subgraphs to locate the code to change.
In `@router/core/header_rule_engine.go`:
- Around line 470-475: The current HeaderRuleOperationSet branch writes
synthetic values directly into res.Header (res.Header.Set(rule.Name,
rule.Value)), which allows later propagate rules to see and export them;
instead, stop mutating res.Header for synthetic “set” operations and store these
values in a separate internal map on the response (e.g., res.syntheticSetHeaders
or similar) so they are available to downstream rule logic that needs them but
are not considered real response headers for propagation (also update
propagate/propagation handling to read only real res.Header for
propagation.header and consult the new synthetic map only where appropriate).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23accb33-dfce-4949-be4d-fed78b77036b
📒 Files selected for processing (8)
docs-website/router/proxy-capabilities/adjusting-cache-control.mdxdocs-website/router/proxy-capabilities/subgraph-response-header-operations.mdxrouter-tests/operations/singleflight_test.gorouter-tests/protocol/header_propagation_test.gorouter-tests/protocol/header_set_test.gorouter/core/header_rule_engine.gorouter/core/header_rule_engine_test.gorouter/pkg/config/config.go
✅ Files skipped from review due to trivial changes (1)
- docs-website/router/proxy-capabilities/subgraph-response-header-operations.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- router-tests/operations/singleflight_test.go
- router-tests/protocol/header_set_test.go
- router-tests/protocol/header_propagation_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/core/header_rule_engine.go (1)
207-207: Simplify redundanthasResponseRulescheck.
len(rhrrs) > 0already includespostRulesviagetAllRules(), so the extra|| postRules.hasRules()is unnecessary.♻️ Suggested simplification
- hf.hasResponseRules = len(rhrrs) > 0 || postRules.hasRules() + hf.hasResponseRules = len(rhrrs) > 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/header_rule_engine.go` at line 207, The assignment to hf.hasResponseRules is redundant because rhrrs (from getAllRules()) already includes postRules; replace the current condition "len(rhrrs) > 0 || postRules.hasRules()" with a single check "len(rhrrs) > 0" so hf.hasResponseRules is true only when the aggregated rule slice rhrrs is non-empty; update the code where hf.hasResponseRules is set (the line referencing hf.hasResponseRules, rhrrs, and postRules) to use only len(rhrrs) > 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router/core/header_rule_engine.go`:
- Line 207: The assignment to hf.hasResponseRules is redundant because rhrrs
(from getAllRules()) already includes postRules; replace the current condition
"len(rhrrs) > 0 || postRules.hasRules()" with a single check "len(rhrrs) > 0" so
hf.hasResponseRules is true only when the aggregated rule slice rhrrs is
non-empty; update the code where hf.hasResponseRules is set (the line
referencing hf.hasResponseRules, rhrrs, and postRules) to use only len(rhrrs) >
0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2df74a91-36d6-4264-870b-fd02a63e951a
📒 Files selected for processing (5)
docs-website/router/proxy-capabilities/adjusting-cache-control.mdxrouter/core/header_rule_engine.gorouter/core/header_rule_engine_buildheader_test.gorouter/core/header_rule_engine_test.gorouter/core/router.go
✅ Files skipped from review due to trivial changes (1)
- docs-website/router/proxy-capabilities/adjusting-cache-control.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- router/core/header_rule_engine_test.go
SkArchon
requested review from
a team,
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
endigma
requested changes
Mar 30, 2026
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
SkArchon
changed the title
SkArchon
changed the title
endigma
approved these changes
Apr 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR refactors the response side of header forwarding
Warning
THIS IS A BREAKING CHANGE
Which includes
"set" is now not propagated to the client, instead the goal of set is to add a header to the response from the subgraph, so it looks like the set header "came from the subgraph directly".
Since "set" is now acts like a header that came directly from the subgraph, if users want to they "technically" can add a propagate op to send this header to the client. Though this is still not recommended (mentioned in the docs), as the same limitations of how set was before applies. (Limitations such as if no subgraph runs nothing will get propagated).
Previously if a user uses "set" to set a Cache-Control header, this would hard override any values set from the cache control configuration without looking at what is the most restrictive configration. This has been removed. Since "set" now acts like a header sent directly from the subgraph, it would behave the same way as it would for an actual Cache Control header that was sent from a subgraph.
Why these changes?
For example
The header would be sent to the client ONLY IF the products subgraph was called. Likewise when using
headers.allfor the same, in case of errors the client header is not set. This PR's goal is to realign set to mean "set (or add) a header to the subgraph response headers".If user's still want to do this they can use the following alternative (currently only supports expressions).
Previously a user was allowed to use the "set" header to do a hard override on the Cache-Control header. Usually we would only pick the most restrictive cache control header, however upon usage of set, whatever was provided would be overridden. After discussing internally we decided to not allow users a hard override for Cache-Control headers, as it could be a potential security issue (i.e. :- When a subgraph wants to expire its data, it's not expired because the "set" override set a higher ttl, and we don't use the lower ttl because of the "set" override for the header)
Summary by CodeRabbit
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.