feat(router): subgraph header groups (headers.groups) — alternative to #2868#2869
Open
mwisner wants to merge 2 commits into
Open
feat(router): subgraph header groups (headers.groups) — alternative to #2868#2869mwisner wants to merge 2 commits into
mwisner wants to merge 2 commits into
Conversation
Adds an ordered `headers.groups` list whose entries bundle a set of request/response header rules with a selector. The selector can be an explicit list of subgraph names (`subgraphs:`), a Go regex against the subgraph name (`matching:`), or both (a subgraph qualifies if it matches either). Groups apply between `headers.all` and exact `headers.subgraphs.<name>` rules, so an exact-name rule can still override a group rule. Each group requires a unique `id`, at least one of `subgraphs`/ `matching`, and at least one of `request`/`response` rules. Selector regexes are compiled once at startup; an explicit subgraph -> group index inverse map makes list-based matching O(1) at request time. Existing configs without `headers.groups` hit a fast-path and pay no additional cost. This is an alternative API surface to the regex-only `subgraph_patterns` proposal; both target the same use case (rules-once-applied-to-many). Reviewers can pick either approach.
Contributor
WalkthroughThis PR adds support for header rule groups in the router's header propagation engine. Groups enable applying header rules to multiple subgraphs by explicit list and/or regex pattern, evaluated in config order between global and per-subgraph rules. The implementation includes precomputed group indices, validation, matching logic, and comprehensive test coverage. ChangesHeader Rule Groups Implementation
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (8)
docs-website/router/proxy-capabilities/subgraph-request-header-operations.mdx (4)
72-73: ⚡ Quick winRemove LLM-like justification.
The phrase "This is useful for" sounds promotional rather than factual. State what groups do, not why they are useful. As per coding guidelines, avoid phrases like "This powerful feature allows you to..." and describe what things do, not how impressive they are.
✍️ Suggested revision
-The `groups` section applies a bundle of rules to every subgraph that matches the group's selector. A group can list subgraphs explicitly (`subgraphs:`), match by regex (`matching:`), or both. This is useful for applying the same rules to a base subgraph and its feature variants without duplicating the same block under every entry. +The `groups` section applies a bundle of rules to every subgraph that matches the group's selector. A group can list subgraphs explicitly (`subgraphs:`), match by regex (`matching:`), or both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-website/router/proxy-capabilities/subgraph-request-header-operations.mdx` around lines 72 - 73, Remove the promotional phrasing in the paragraph about groups — specifically delete the sentence starting with "This is useful for applying..." and replace it with a factual statement describing behavior only: that a group applies the listed bundle of rules to every subgraph matching the group's selector and can target subgraphs either via explicit subgraphs: entries, via matching: regex patterns, or both (referencing "groups", "subgraphs:", and "matching:"). Keep the explanation terse and factual without value-laden words.
97-97: ⚡ Quick winRemove em dash.
As per coding guidelines, avoid em dashes. Use periods or restructure the sentence instead.
✍️ Suggested revision
-A group with both `subgraphs` and `matching` matches a subgraph if it appears in either (the union — list-match OR regex-match). The group's rules apply at most once per subgraph regardless of how many parts of the selector match. +A group with both `subgraphs` and `matching` matches a subgraph if it appears in either the explicit list or matches the regex. The group's rules apply at most once per subgraph regardless of how many parts of the selector match.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-website/router/proxy-capabilities/subgraph-request-header-operations.mdx` at line 97, The sentence starting "A group with both `subgraphs` and `matching` matches a subgraph if it appears in either (the union — list-match OR regex-match)." uses an em dash; replace the em dash with a period or restructure the clause to avoid punctuation rules violation. For example, split into two sentences or use parentheses/commas so the line describing the union uses plain punctuation: keep the original identifiers `subgraphs` and `matching` and ensure the following sentence still communicates "list-match OR regex-match" and that "The group's rules apply at most once per subgraph..." remains unchanged.
82-82: ⚡ Quick winRemove hedging connector "so".
Replace the hedging connector with a direct statement. As per coding guidelines, avoid filler and hedging words.
✍️ Suggested revision
-Exact-name rules apply last, so a `headers.subgraphs.products` rule can still override a broader group rule when both target the same header. +Exact-name rules apply last. A `headers.subgraphs.products` rule can override a broader group rule when both target the same header.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-website/router/proxy-capabilities/subgraph-request-header-operations.mdx` at line 82, The sentence containing "headers.subgraphs.products" uses a hedging connector "so"—replace it with a direct statement by removing "so" and rephrasing to: "Exact-name rules apply last. A `headers.subgraphs.products` rule can still override a broader group rule when both target the same header." Update the line that currently reads "Exact-name rules apply last, so a `headers.subgraphs.products` rule can still override..." to the two concise sentences above.
91-91: ⚡ Quick winSplit long sentence.
The sentence has multiple clauses separated by a semicolon. As per coding guidelines, prefer short, declarative sentences and consider splitting sentences with more than one comma-separated clause.
✍️ Suggested revision
-* `negate_match` — If `true`, the regex result is inverted. Subgraphs in the explicit `subgraphs` list are still included positively; `negate_match` only affects the regex. +* `negate_match` — If `true`, the regex result is inverted. Subgraphs in the explicit `subgraphs` list are still included positively. The negation only affects the regex.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-website/router/proxy-capabilities/subgraph-request-header-operations.mdx` at line 91, The sentence describing `negate_match` is too long and should be split into two short declarative sentences: one stating that `negate_match` inverts the regex result when set to `true`, and a second clarifying that subgraphs listed explicitly in `subgraphs` are always included and are not affected by `negate_match`. Update the text around the `negate_match` description to reflect these two separate sentences.docs-website/router/proxy-capabilities/subgraph-response-header-operations.mdx (1)
84-85: ⚡ Quick winSplit long sentence and remove hedging connector.
The paragraph contains a long sentence with multiple clauses and uses "so" as a hedging connector. As per coding guidelines, prefer short, declarative sentences and avoid filler and hedging words.
✍️ Suggested revision
-The `groups` section applies a bundle of rules to every subgraph that matches the group's selector. A group can list subgraphs explicitly (`subgraphs:`), match by regex (`matching:`), or both. Groups apply after `all` and before exact `subgraphs` rules, so an exact subgraph rule can still override a group rule. See [Subgraph Request Headers Operations](/router/proxy-capabilities/subgraph-request-header-operations#subgraph-header-groups) for the full set of group fields and validation rules. +The `groups` section applies a bundle of rules to every subgraph that matches the group's selector. A group can list subgraphs explicitly (`subgraphs:`), match by regex (`matching:`), or both. Groups apply after `all` and before exact `subgraphs` rules. An exact subgraph rule can override a group rule. See [Subgraph Request Headers Operations](/router/proxy-capabilities/subgraph-request-header-operations#subgraph-header-groups) for the full set of group fields and validation rules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-website/router/proxy-capabilities/subgraph-response-header-operations.mdx` around lines 84 - 85, The paragraph describing the groups behavior is one long sentence and uses a hedging connector ("so"); split it into shorter declarative sentences and remove the hedging word. Rephrase the sentence that currently reads about application order and overrides into two clear sentences (e.g., state that "Groups apply after all and before exact subgraphs rules." then state that "An exact subgraph rule can still override a group rule."), and preserve the references to the group fields `subgraphs:`, `matching:`, and the sections `all` and `subgraphs` so the meaning and validation guidance remain intact.router/core/header_rule_engine_buildheader_test.go (1)
595-619: ⚡ Quick winStrengthen the “fires only once” assertion.
This case currently uses a constant
op: set, so it can still pass if the same group is applied twice. Add a direct dedup assertion on rule collection for the overlap case.Proposed test hardening
t.Run("hybrid match fires only once when both list and regex would match", func(t *testing.T) { - ht, err := NewHeaderPropagation(&config.HeaderRules{ + rules := &config.HeaderRules{ Groups: []*config.SubgraphHeaderGroup{ { ID: "double-match", Subgraphs: []string{"products"}, Matching: "^products$", // overlaps with list intentionally Request: []*config.RequestHeaderRule{ {Operation: "set", Name: "X-Tag", Value: "once"}, }, }, }, - }) + } + + // Overlapping selectors must contribute the group exactly once. + gotRules := SubgraphRules(rules, "products") + require.Len(t, gotRules, 1) + + ht, err := NewHeaderPropagation(rules) require.NoError(t, err) ctx := newGroupTestRequestContext(t, nil) hdr, _ := ht.BuildRequestHeaderForSubgraph("products", ctx) require.NotNil(t, hdr) assert.Equal(t, []string{"once"}, hdr.Values("X-Tag")) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/core/header_rule_engine_buildheader_test.go` around lines 595 - 619, The test must assert not only that the resulting header value isn't duplicated but that the propagation rule is present only once in the compiled rule collection; after creating ht via NewHeaderPropagation(&config.HeaderRules{...}) inspect ht's compiled request-rule collection for the "products" subgraph and count rules originating from the SubgraphHeaderGroup with ID "double-match" (or inspect ht.Groups/ht.requestRulesForSubgraph("products") if that accessor exists) and assert the count == 1, then proceed to call BuildRequestHeaderForSubgraph("products", ctx) and keep the existing header assertion to ensure deduplication both at rule-registration and runtime.router/pkg/config/config.schema.json (1)
1905-1963: ⚡ Quick winEnforce group selector/rule presence at schema level.
groupscurrently requires onlyid, so structurally invalid groups can pass schema validation and fail later at startup. AddanyOfconstraints for selector and rule presence to fail fast.Proposed schema tightening
"groups": { "type": "array", @@ "items": { "type": "object", "additionalProperties": false, "required": ["id"], + "allOf": [ + { + "anyOf": [ + { "required": ["subgraphs"] }, + { "required": ["matching"] } + ] + }, + { + "anyOf": [ + { "required": ["request"] }, + { "required": ["response"] } + ] + } + ], "properties": {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/pkg/config/config.schema.json` around lines 1905 - 1963, The group object under "groups" currently only requires "id" which lets invalid group entries pass validation; update the group's schema (the object whose properties include "id", "subgraphs", "matching", "negate_match", "request", "response") to add two anyOf constraints: one anyOf requiring at least one selector present ({"required":["subgraphs"]} or {"required":["matching"]}) and a second anyOf requiring at least one rule array present ({"required":["request"]} or {"required":["response"]}), keeping existing types/defaults intact so a group must specify a selector and at least one of request/response.router/core/header_rule_engine.go (1)
456-466: ⚡ Quick winSkip group matching when no relevant group rules exist.
Line 460 still walks groups for response-only configs, and Line 642 does the same for request-only configs. On these hot paths that adds an avoidable allocation/scan even though no group can contribute headers. Track a
hasAnyGroupResponseRulesflag alongsidehasAnyGroupRequestRules, then guard both calls before enteringforEachMatchingGroup.♻️ Proposed change
type HeaderPropagation struct { ... hasAnyGroupRequestRules bool + hasAnyGroupResponseRules bool hasAnyGroupRegex bool } func (h *HeaderPropagation) indexGroups() error { ... if len(g.Request) > 0 { h.hasAnyGroupRequestRules = true } + if len(g.Response) > 0 { + h.hasAnyGroupResponseRules = true + } } - if subgraphName != "" { + if subgraphName != "" && h.hasAnyGroupRequestRules { h.forEachMatchingGroup(subgraphName, func(g *config.SubgraphHeaderGroup) { for _, rule := range g.Request { h.applyRequestRuleToHeader(ctx, outHeader, rule) } }) } - if subgraphName != "" { + if subgraphName != "" && h.hasAnyGroupResponseRules { h.forEachMatchingGroup(subgraphName, func(g *config.SubgraphHeaderGroup) { for _, rule := range g.Response { h.applyResponseRule(propagation, resp, rule) } }) }Also applies to: 639-649
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/core/header_rule_engine.go` around lines 456 - 466, The group-matching loop currently always calls h.forEachMatchingGroup even when no group can contribute request or response headers; add two boolean flags (hasAnyGroupRequestRules and hasAnyGroupResponseRules) that track whether any SubgraphHeaderGroup contains Request or Response rules, then guard the calls that iterate groups: only call h.forEachMatchingGroup for request rules when hasAnyGroupRequestRules is true (the block that invokes h.applyRequestRuleToHeader and references g.Request) and only call it for response rules when hasAnyGroupResponseRules is true (the block that invokes h.applyResponseRuleToHeader and references g.Response); compute/set these flags where groups are loaded/configured so the hot path skips the forEachMatchingGroup allocation/scan when irrelevant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@docs-website/router/proxy-capabilities/subgraph-request-header-operations.mdx`:
- Around line 72-73: Remove the promotional phrasing in the paragraph about
groups — specifically delete the sentence starting with "This is useful for
applying..." and replace it with a factual statement describing behavior only:
that a group applies the listed bundle of rules to every subgraph matching the
group's selector and can target subgraphs either via explicit subgraphs:
entries, via matching: regex patterns, or both (referencing "groups",
"subgraphs:", and "matching:"). Keep the explanation terse and factual without
value-laden words.
- Line 97: The sentence starting "A group with both `subgraphs` and `matching`
matches a subgraph if it appears in either (the union — list-match OR
regex-match)." uses an em dash; replace the em dash with a period or restructure
the clause to avoid punctuation rules violation. For example, split into two
sentences or use parentheses/commas so the line describing the union uses plain
punctuation: keep the original identifiers `subgraphs` and `matching` and ensure
the following sentence still communicates "list-match OR regex-match" and that
"The group's rules apply at most once per subgraph..." remains unchanged.
- Line 82: The sentence containing "headers.subgraphs.products" uses a hedging
connector "so"—replace it with a direct statement by removing "so" and
rephrasing to: "Exact-name rules apply last. A `headers.subgraphs.products` rule
can still override a broader group rule when both target the same header."
Update the line that currently reads "Exact-name rules apply last, so a
`headers.subgraphs.products` rule can still override..." to the two concise
sentences above.
- Line 91: The sentence describing `negate_match` is too long and should be
split into two short declarative sentences: one stating that `negate_match`
inverts the regex result when set to `true`, and a second clarifying that
subgraphs listed explicitly in `subgraphs` are always included and are not
affected by `negate_match`. Update the text around the `negate_match`
description to reflect these two separate sentences.
In
`@docs-website/router/proxy-capabilities/subgraph-response-header-operations.mdx`:
- Around line 84-85: The paragraph describing the groups behavior is one long
sentence and uses a hedging connector ("so"); split it into shorter declarative
sentences and remove the hedging word. Rephrase the sentence that currently
reads about application order and overrides into two clear sentences (e.g.,
state that "Groups apply after all and before exact subgraphs rules." then state
that "An exact subgraph rule can still override a group rule."), and preserve
the references to the group fields `subgraphs:`, `matching:`, and the sections
`all` and `subgraphs` so the meaning and validation guidance remain intact.
In `@router/core/header_rule_engine_buildheader_test.go`:
- Around line 595-619: The test must assert not only that the resulting header
value isn't duplicated but that the propagation rule is present only once in the
compiled rule collection; after creating ht via
NewHeaderPropagation(&config.HeaderRules{...}) inspect ht's compiled
request-rule collection for the "products" subgraph and count rules originating
from the SubgraphHeaderGroup with ID "double-match" (or inspect
ht.Groups/ht.requestRulesForSubgraph("products") if that accessor exists) and
assert the count == 1, then proceed to call
BuildRequestHeaderForSubgraph("products", ctx) and keep the existing header
assertion to ensure deduplication both at rule-registration and runtime.
In `@router/core/header_rule_engine.go`:
- Around line 456-466: The group-matching loop currently always calls
h.forEachMatchingGroup even when no group can contribute request or response
headers; add two boolean flags (hasAnyGroupRequestRules and
hasAnyGroupResponseRules) that track whether any SubgraphHeaderGroup contains
Request or Response rules, then guard the calls that iterate groups: only call
h.forEachMatchingGroup for request rules when hasAnyGroupRequestRules is true
(the block that invokes h.applyRequestRuleToHeader and references g.Request) and
only call it for response rules when hasAnyGroupResponseRules is true (the block
that invokes h.applyResponseRuleToHeader and references g.Response); compute/set
these flags where groups are loaded/configured so the hot path skips the
forEachMatchingGroup allocation/scan when irrelevant.
In `@router/pkg/config/config.schema.json`:
- Around line 1905-1963: The group object under "groups" currently only requires
"id" which lets invalid group entries pass validation; update the group's schema
(the object whose properties include "id", "subgraphs", "matching",
"negate_match", "request", "response") to add two anyOf constraints: one anyOf
requiring at least one selector present ({"required":["subgraphs"]} or
{"required":["matching"]}) and a second anyOf requiring at least one rule array
present ({"required":["request"]} or {"required":["response"]}), keeping
existing types/defaults intact so a group must specify a selector and at least
one of request/response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 135b0afa-29af-4064-a911-0215d11d7f3e
📒 Files selected for processing (10)
docs-website/router/proxy-capabilities/subgraph-request-header-operations.mdxdocs-website/router/proxy-capabilities/subgraph-response-header-operations.mdxrouter/core/header_rule_engine.gorouter/core/header_rule_engine_buildheader_test.gorouter/core/header_rule_engine_test.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
Adds explicit unit tests for the precedence behavior between rules attached to the same subgraph through different layers and across multiple groups. The router applies rules in a strict deterministic order (headers.all -> headers.groups in config order -> exact subgraphs.<name>) and the last writer wins per header name. Tests cover: - Two groups setting the same header — config order is the tiebreaker - set vs propagate conflicts in both orders, with and without the client supplying the header - propagate-without-default no-op behavior when client header missing - propagate-with-default does overwrite an earlier set - Last-writer-wins also applies within a single group's rule list - Exact subgraph rules override every matching group regardless of group config order - Rules touching different header names coexist regardless of order - Swapping group order swaps the winner (the assertion in pure form) Also adds a 'Conflict resolution' subsection to the request-header docs explaining the precedence rules and the propagate/set asymmetry.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs-website/router/proxy-capabilities/subgraph-request-header-operations.mdx (2)
97-97: ⚡ Quick winRemove em dash.
The guideline states: "Avoid em dashes. Use periods or restructure the sentence instead."
Suggested rewrite
-A group with both `subgraphs` and `matching` matches a subgraph if it appears in either (the union — list-match OR regex-match). The group's rules apply at most once per subgraph regardless of how many parts of the selector match. +A group with both `subgraphs` and `matching` matches a subgraph if it appears in either (union semantics: list-match OR regex-match). The group's rules apply at most once per subgraph regardless of how many parts of the selector match.As per coding guidelines: Avoid em dashes. Use periods or restructure the sentence instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-website/router/proxy-capabilities/subgraph-request-header-operations.mdx` at line 97, Replace the em dash in the sentence containing "`subgraphs` and `matching`" so it follows the guideline (avoid em dashes); e.g., change "(the union — list-match OR regex-match)" to "(the union: list-match OR regex-match)" or "(the union (list-match OR regex-match))" and ensure the rest of the sentence ("The group's rules apply at most once per subgraph regardless of how many parts of the selector match.") remains unchanged; update the text in the file where the phrase '`subgraphs` and `matching`' appears.
109-109: ⚡ Quick winRemove em dash.
The guideline states: "Avoid em dashes. Use periods or restructure the sentence instead."
Suggested rewrite
-Concretely, **within the groups layer, group order in the YAML is the tiebreaker** — entries are merged first-to-last, and a later group's rule overrides an earlier group's rule for the same header name. The `headers.subgraphs.<name>` layer always runs last and overrides every group rule for that exact subgraph. +Concretely, **within the groups layer, group order in the YAML is the tiebreaker**. Entries are merged first-to-last, and a later group's rule overrides an earlier group's rule for the same header name. The `headers.subgraphs.<name>` layer always runs last and overrides every group rule for that exact subgraph.As per coding guidelines: Avoid em dashes. Use periods or restructure the sentence instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-website/router/proxy-capabilities/subgraph-request-header-operations.mdx` at line 109, The sentence "Concretely, **within the groups layer, group order in the YAML is the tiebreaker** — entries are merged first-to-last, and a later group's rule overrides an earlier group's rule for the same header name." uses an em dash; remove it and split/restructure into two sentences (or use a period) so it reads e.g. "Concretely, within the groups layer, group order in the YAML is the tiebreaker. Entries are merged first-to-last, and a later group's rule overrides an earlier group's rule for the same header name. The `headers.subgraphs.<name>` layer always runs last and overrides every group rule for that exact subgraph." Apply this change to the paragraph referencing the groups layer and the `headers.subgraphs.<name>` layer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@docs-website/router/proxy-capabilities/subgraph-request-header-operations.mdx`:
- Line 97: Replace the em dash in the sentence containing "`subgraphs` and
`matching`" so it follows the guideline (avoid em dashes); e.g., change "(the
union — list-match OR regex-match)" to "(the union: list-match OR regex-match)"
or "(the union (list-match OR regex-match))" and ensure the rest of the sentence
("The group's rules apply at most once per subgraph regardless of how many parts
of the selector match.") remains unchanged; update the text in the file where
the phrase '`subgraphs` and `matching`' appears.
- Line 109: The sentence "Concretely, **within the groups layer, group order in
the YAML is the tiebreaker** — entries are merged first-to-last, and a later
group's rule overrides an earlier group's rule for the same header name." uses
an em dash; remove it and split/restructure into two sentences (or use a period)
so it reads e.g. "Concretely, within the groups layer, group order in the YAML
is the tiebreaker. Entries are merged first-to-last, and a later group's rule
overrides an earlier group's rule for the same header name. The
`headers.subgraphs.<name>` layer always runs last and overrides every group rule
for that exact subgraph." Apply this change to the paragraph referencing the
groups layer and the `headers.subgraphs.<name>` layer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ac316f6-b6d0-4dcf-844e-6c1ea5132f64
📒 Files selected for processing (2)
docs-website/router/proxy-capabilities/subgraph-request-header-operations.mdxrouter/core/header_rule_engine_buildheader_test.go
5 tasks
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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
1 participant
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.
Why
Subgraph-specific header rules in the router are matched by exact subgraph name today. That works well when each subgraph's rules are unique, but it doesn't scale cleanly to two common cases:
A cohort of subgraphs sharing the same rules. Many graphs have a large set of subgraphs that need the same forwarding rules (e.g.
propagate Authorization,propagate X-User-Id, etc.). Today you either:headers.all, which applies to every subgraph regardless of need, orheaders.subgraphs. This is verbose, drifts over time, and motivates external preprocessors that compile a compact authoring format into the verbose router config.Feature subgraphs replacing a base subgraph. A feature subgraph is composed under its own name (e.g.
products-feature-v2), not the base name (products), so per-subgraph rules defined forproductsdo not apply to its feature variants. This forces config edits and a router redeploy each time a feature subgraph is created or renamed.This PR adds a single, opt-in primitive that handles both cases: named header rule groups with a selector that can be an explicit subgraph list, a regex, or both.
What
A new optional
headers.groupslist. Each entry bundles request/response header rules with a selector that picks the subgraphs it applies to.Schema example
A subgraph matches a group if it appears in the group's
subgraphslist or its name matches the group'smatchingregex (the union of the two).negate_match: trueonly inverts the regex result; subgraphs in the explicit list are always included positively.Rule evaluation order
For each subgraph the router builds a header set in this order:
headers.allrules.headers.groupsentries whose selector matches the subgraph (in config order).headers.subgraphs.<name>rules for the exact subgraph name.Exact-name rules apply last so they can still override a broader group rule (e.g. via
op: set). This is the same precedence rule that exists betweenallandsubgraphs.<name>today.A group fires at most once per subgraph, even when both the explicit list and the regex would match it.
Conflict resolution
When more than one rule targets the same header name for the same subgraph, the router applies rules in a strict deterministic order and the last writer wins per header name. There is no warning, no error, and no validator that detects overlapping rules.
The full ordering across layers:
headers.allrules (in declared order).headers.groupsentries whose selector matches the subgraph, in config order. Within each group, rules apply in declared order.headers.subgraphs.<name>rules (in declared order).Concretely, within the groups layer, group order in the YAML is the tiebreaker — entries are merged first-to-last, and a later group's rule overrides an earlier group's rule for the same header name. The
headers.subgraphs.<name>layer always runs last and overrides every group rule for that exact subgraph.There is one subtle asymmetry between operations: an
op: propagaterule with no client-supplied value and nodefault:is a no-op and does not clear an earlier value. Anop: setrule, and anop: propagaterule with adefault:set, always write a value. This behavior matchesheaders.subgraphs.<name>today and is locked in by the newTestBuildRequestHeaderForSubgraph_GroupsPrecedencetest cases.Validation (at startup, not request time)
idis required and must be unique across groups.subgraphs/matchingmust be set (otherwise the group would never apply).request/responserules must be set (otherwise the group has no effect).matching, when present, must be a valid Go regex.Misconfiguration fails router init with a clear error message rather than producing wrong header sets at request time.
Backward compatibility
No breaking changes.
headers.groupsis a new optional field. Configs without it behave identically to today.headers.allandheaders.subgraphs.<name>continue to work with no semantic change.headers.groupsis empty (len(rules.Groups) == 0), so existing users see zero added work per request.Performance
The hot path is
BuildRequestHeaderForSubgraph, called per data source in the execution plan viaSubgraphHeadersBuilder.headers.groupsggroups, list-onlysubgraphToGroupIdxmap lookupggroups, regex-onlyregex.MatchStringper regex groupggroups, hybridConcrete properties:
NewHeaderPropagation, stored ingroupRegex []*regexp.Regexpindex-aligned withrules.Groups. Invalid regexes fail router init.subgraphToGroupIdx map[string][]intbuilt once at startup turns "which groups list this subgraph?" into a single map read.regexpuses RE2, which is linear-time and rejects backtracking constructs.[]booloflen(rules.Groups)(small fixed allocation).For realistic configs (
g ≤ 10, plan touchingk ≤ 20subgraphs, names ~30 chars), the added cost is dominated by map lookups and is sub-microsecond per built header set.Comparison with #2868 (
subgraph_patterns)subgraph_patterns)headers.groups)id) for traceabilityheaders.groupsis a strict superset ofsubgraph_patterns(a regex-only group{ matching: ... }is equivalent to a pattern). Reviewers may prefer the smaller, more focused API in #2868, or the richer, more expressive API in this PR. This PR's branch is independent of #2868 — it does not stack on it and does not assume it lands.Implementation summary
router/pkg/config/config.goSubgraphHeaderGrouptype andHeaderRules.Groupsfieldrouter/core/header_rule_engine.goforEachMatchingGrouphelper that combines list lookup and regex evaluation; wire group rules into request and response paths betweenalland exact-name; extendhasRequestRulesForSubgraphandSubgraphRulesrouter/pkg/config/config.schema.jsonheaders.groupsrouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_*.jsonrouter/core/header_rule_engine_test.gorouter/core/header_rule_engine_buildheader_test.gonegate_matchregex-only inversion, evaluation order acrossall→ multiple groups → exact, all six startup validation paths, no-groups fast-path,SubgraphRuleshelper coverage. PlusTestBuildRequestHeaderForSubgraph_GroupsPrecedence(9 subtests) covering conflict resolution between groups, mixedset/propagatecollisions, propagate-without-default no-op behavior, intra-group rule order, and exact-name override.docs-website/router/proxy-capabilities/subgraph-request-header-operations.mdxdocs-website/router/proxy-capabilities/subgraph-response-header-operations.mdxValidation
Locally on darwin/arm64 with Go 1.26.3:
Notes for reviewers
groups(underheaders.) to keep scoping clean. Open tosubgraph_groupsif reviewers prefer.headers.subgraphs.<name>behavior. If reviewers want a "warn on overlapping rules at startup" pass, that's an easy follow-up; left out so the diff stays focused.idas an attribute on subgraph spans / metrics. Easy to add later; left out so the diff stays focused on the header-propagation behavior.Summary by CodeRabbit
New Features
Documentation
headers.groupsconfiguration, precedence rules, selector options, and practical examples.