feat(router): allow split config polling#2814
Merged
dkorittki merged 16 commits intofeat/split-configsfrom May 5, 2026
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR adds split-config loading: a feature flag–aware mechanism where the control plane emits a ChangesSplit-Config Loading Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
connect-go - uncommitted changes detectedSeems like you forgot to commit some code. Possible causes:
Dirty files
|
controlplane - uncommitted changes detectedSeems like you forgot to commit some code. Possible causes:
Dirty files
|
Router image scan passed✅ No security vulnerabilities found in image: |
![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: 3
🧹 Nitpick comments (3)
controlplane/src/types/index.ts (1)
499-503: ⚡ Quick winNarrow
featurestoFeatureIds[]for stronger token-claim safety.Line 502 currently allows arbitrary strings; using
FeatureIds[]catches claim typos at compile time.♻️ Proposed change
export interface GraphApiKeyJwtPayload extends JWTPayload { federated_graph_id: string; organization_id: string; - features?: string[]; + features?: FeatureIds[]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/types/index.ts` around lines 499 - 503, The JWT payload interface GraphApiKeyJwtPayload currently types features as string[], which permits arbitrary values; change its features property's type to FeatureIds[] (i.e., replace features?: string[] with features?: FeatureIds[]) so TypeScript will enforce allowed feature identifiers—ensure FeatureIds is imported or defined in the same module and update any usages or tests that construct GraphApiKeyJwtPayload claims to use the FeatureIds enum/type.router/core/init_config_poller.go (1)
136-145: ⚡ Quick winMake split-mode fallback behavior explicit when fallback storage is configured.
When split mode is enabled, this early return bypasses fallback storage config. Add an explicit log (or validation error) so operators don’t assume fallback is active.
♻️ Proposed change
if hasSplitCfgFeature { + if r.routerConfigPollerConfig.FallbackStorage.Enabled { + r.logger.Info("split-config-loading currently ignores fallback storage configuration") + } providerID := r.routerConfigPollerConfig.Storage.ProviderID if providerID != "" { r.logger.Info("split-config-loading feature is enabled but a custom storage provider is configured; falling back to regular config polling", zap.String("provider_id", providerID), ) } else { return newSplitConfigPoller(r) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/init_config_poller.go` around lines 136 - 145, The code path in init_config_poller.go around hasSplitCfgFeature currently returns newSplitConfigPoller only when r.routerConfigPollerConfig.Storage.ProviderID is empty but logs an info that fallback will happen when ProviderID is set; make this explicit and obvious by changing that log to a warning or validation error and add a clear message stating that split-mode is disabled due to a configured fallback storage provider (include provider_id), e.g. use r.logger.Warn or r.logger.Error with zap.String("provider_id", providerID) and keep the existing control flow so newSplitConfigPoller is only returned when ProviderID == ""; ensure the message clearly states that regular config polling will be used instead of split-config-loading.router/pkg/controlplane/configpoller/split_config_poller_test.go (1)
128-136: ⚡ Quick winAdd regression coverage for “mapper missing base key” (non-empty mapper).
Current tests cover empty mapper but not the non-empty-without-
""case. Add this to lock in expected failure/skip behavior and prevent stale base-config regressions.Also applies to: 371-392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/controlplane/configpoller/split_config_poller_test.go` around lines 128 - 136, Add a new unit test mirroring TestSplitGetRouterConfig_EmptyMapper to cover the case where mapper is non-empty but missing the base key ""; instantiate mockSplitFetcher with mapperResult set to a non-empty map (e.g., makeActiveGraphs(map[string]string{"foo":"..."})), call newTestPoller(mock).GetRouterConfig(context.Background()), and assert it returns an error and that the error message indicates the missing base key (consistent with the poller's expected failure/skip behavior). Use the same helpers (mockSplitFetcher, makeActiveGraphs, newTestPoller, GetRouterConfig) to locate and implement the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/pkg/controlplane/configpoller/split_config_poller.go`:
- Around line 145-147: The assignment p.knownHashes = graphConfigs aliases an
external map into the poller's state and risks corruption from external
mutation; instead perform a defensive copy of graphConfigs into a new map and
assign that to p.knownHashes (do the same for the other assignment site around
the code that sets p.currentConfig/p.latestVersion), then compute
p.latestVersion using computeCompositeVersion on the copied map; locate usages
by the symbols p.knownHashes, graphConfigs, p.currentConfig, and
computeCompositeVersion and replace direct map assignment with a shallow clone
loop that copies keys/values before storing in p.knownHashes.
- Around line 136-138: The code currently only rejects an entirely empty mapper
(len(graphConfigs) == 0) but doesn't ensure the mapper contains the base graph
key "" so base config changes can be detected; update both places that validate
graphConfigs/mapper (the blocks using graphConfigs and mapper variables in
split_config_poller.go) to additionally check for the presence of the
empty-string key (""), and return an error (fmt.Errorf) if that base entry is
missing so polling won't proceed with a mapper that can't detect base config
updates.
In `@router/pkg/routerconfig/cdn/split_fetcher.go`:
- Around line 51-53: The constructor currently dereferences opts without
checking for nil, causing a panic; update the constructor (the function that
accepts opts and sets defaults, e.g., NewSplitFetcher or its options
initializer) to first guard if opts == nil and if so allocate a new options
struct (opts = &Options{} or the concrete options type used), then apply
defaults like setting opts.Logger = zap.NewNop() when nil; ensure you reference
and set Logger via the same opts variable so no fields are accessed on a nil
pointer.
---
Nitpick comments:
In `@controlplane/src/types/index.ts`:
- Around line 499-503: The JWT payload interface GraphApiKeyJwtPayload currently
types features as string[], which permits arbitrary values; change its features
property's type to FeatureIds[] (i.e., replace features?: string[] with
features?: FeatureIds[]) so TypeScript will enforce allowed feature
identifiers—ensure FeatureIds is imported or defined in the same module and
update any usages or tests that construct GraphApiKeyJwtPayload claims to use
the FeatureIds enum/type.
In `@router/core/init_config_poller.go`:
- Around line 136-145: The code path in init_config_poller.go around
hasSplitCfgFeature currently returns newSplitConfigPoller only when
r.routerConfigPollerConfig.Storage.ProviderID is empty but logs an info that
fallback will happen when ProviderID is set; make this explicit and obvious by
changing that log to a warning or validation error and add a clear message
stating that split-mode is disabled due to a configured fallback storage
provider (include provider_id), e.g. use r.logger.Warn or r.logger.Error with
zap.String("provider_id", providerID) and keep the existing control flow so
newSplitConfigPoller is only returned when ProviderID == ""; ensure the message
clearly states that regular config polling will be used instead of
split-config-loading.
In `@router/pkg/controlplane/configpoller/split_config_poller_test.go`:
- Around line 128-136: Add a new unit test mirroring
TestSplitGetRouterConfig_EmptyMapper to cover the case where mapper is non-empty
but missing the base key ""; instantiate mockSplitFetcher with mapperResult set
to a non-empty map (e.g., makeActiveGraphs(map[string]string{"foo":"..."})),
call newTestPoller(mock).GetRouterConfig(context.Background()), and assert it
returns an error and that the error message indicates the missing base key
(consistent with the poller's expected failure/skip behavior). Use the same
helpers (mockSplitFetcher, makeActiveGraphs, newTestPoller, GetRouterConfig) to
locate and implement the test.
🪄 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: fca262c8-cb67-466f-a6ce-0af950f1de30
⛔ Files ignored due to path filters (1)
router/gen/proto/wg/cosmo/node/v1/node.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (10)
controlplane/src/core/bufservices/federated-graph/createFederatedGraphToken.tscontrolplane/src/core/repositories/OrganizationRepository.tscontrolplane/src/types/index.tsproto/wg/cosmo/node/v1/node.protorouter/core/graph_server.gorouter/core/init_config_poller.gorouter/internal/jwt/claims.gorouter/pkg/controlplane/configpoller/split_config_poller.gorouter/pkg/controlplane/configpoller/split_config_poller_test.gorouter/pkg/routerconfig/cdn/split_fetcher.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/split-configs #2814 +/- ##
=====================================================
Coverage ? 65.62%
=====================================================
Files ? 256
Lines ? 26806
Branches ? 0
=====================================================
Hits ? 17591
Misses ? 7758
Partials ? 1457
🚀 New features to boost your workflow:
|
…t-loading-deployment-configs-in-the-router
dkorittki
requested review from
Noroth,
SkArchon,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Noroth
requested changes
May 4, 2026
Comment on lines
+138
to
+144
| if providerID != "" { | ||
| r.logger.Info("split-config-loading feature is enabled but a custom storage provider is configured; falling back to regular config polling", | ||
| zap.String("provider_id", providerID), | ||
| ) | ||
| } else { | ||
| return newSplitConfigPoller(r) | ||
| } |
Contributor
There was a problem hiding this comment.
You can swap the if then you can omit else
Suggested change
| if providerID != "" { | |
| r.logger.Info("split-config-loading feature is enabled but a custom storage provider is configured; falling back to regular config polling", | |
| zap.String("provider_id", providerID), | |
| ) | |
| } else { | |
| return newSplitConfigPoller(r) | |
| } | |
| if providerID = "" { | |
| return newSplitConfigPoller(r) | |
| } | |
| r.logger.Info("split-config-loading feature is enabled but a custom storage provider is configured; falling back to regular config polling", | |
| zap.String("provider_id", providerID)) |
| type FederatedGraphTokenClaims struct { | ||
| FederatedGraphID string | ||
| OrganizationID string | ||
| Features []string |
Contributor
There was a problem hiding this comment.
nit: You could also use a hash set here to not use slices.Contains() but have O(1) lookups but I don't think it really matters in this case. Up to you
Contributor
Author
There was a problem hiding this comment.
I don't think we will ever run into the amount of features where we can feel this
Comment on lines
-187
to
-225
| /** | ||
| * If a signature key is set, we need to validate the signature of the received config | ||
| */ | ||
|
|
||
| if cdn.hash != nil { | ||
| configSignature := resp.Header.Get(sigResponseHeaderName) | ||
| if configSignature == "" { | ||
| cdn.logger.Error( | ||
| "Signature header not found in CDN response. Ensure that your Admission Controller was able to sign the config. Open the compositions page in the Studio to check the status of the last deployment", | ||
| zap.Error(ErrMissingSignatureHeader), | ||
| ) | ||
| return nil, ErrMissingSignatureHeader | ||
| } | ||
|
|
||
| // create a signature of the received config body | ||
| if _, err := cdn.hash.Write(body); err != nil { | ||
| return nil, fmt.Errorf("could not write config body to hmac: %w", err) | ||
| } | ||
| dataHmac := cdn.hash.Sum(nil) | ||
| cdn.hash.Reset() | ||
|
|
||
| // compare received signature with the one we calculated with the private signature key | ||
| rawSignature, err := base64.StdEncoding.DecodeString(configSignature) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not hex decode signature key: %w", err) | ||
| if err := validateHMACSignature(cdn.hash, body, resp.Header.Get(sigResponseHeaderName), cdn.logger, cdn.federatedGraphID); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if subtle.ConstantTimeCompare(rawSignature, dataHmac) != 1 { | ||
| cdn.logger.Error( | ||
| "Invalid config signature, potential tampering detected. Ensure that your Admission Controller has signed the config correctly. Open the compositions page in the Studio to check the status of the last deployment", | ||
| zap.Error(ErrInvalidSignature), | ||
| ) | ||
| return nil, ErrInvalidSignature | ||
| } | ||
|
|
||
| cdn.logger.Info("Config signature validation successful", | ||
| zap.String("federatedGraphID", cdn.federatedGraphID), | ||
| zap.String("signature", configSignature), | ||
| ) |
Contributor
There was a problem hiding this comment.
All of this was put in utils.go? A bit hard to see what actually changed here
Contributor
Author
There was a problem hiding this comment.
I'll revert this so the diff is easier to understand
| func (f *SplitFetcher) FetchConfig(ctx context.Context, featureFlagName string) (*nodev1.RouterConfig, error) { | ||
| var path string | ||
| if featureFlagName == "" { | ||
| path = fmt.Sprintf("/%s/%s/manifest/latest.json", f.organizationID, f.federatedGraphID) |
Contributor
There was a problem hiding this comment.
I vote for using https://pkg.go.dev/net/url#JoinPath. Less error prone than fmt
This reverts commit d0675dd.
Noroth
approved these changes
May 5, 2026
| func (f *SplitFetcher) FetchMapper(ctx context.Context) (*nodev1.ActiveGraphs, error) { | ||
| path := fmt.Sprintf("/%s/%s/manifest/mapper.json", f.organizationID, f.federatedGraphID) | ||
| body, err := f.get(ctx, path) | ||
| path, err := url.JoinPath("/", f.organizationID, f.federatedGraphID, "manifest/mapper.json") |
Contributor
There was a problem hiding this comment.
nit: could leave the / logic to JoinPath but fine as it is
Suggested change
| path, err := url.JoinPath("/", f.organizationID, f.federatedGraphID, "manifest/mapper.json") | |
| path, err := url.JoinPath("/", f.organizationID, f.federatedGraphID, "manifest", "mapper.json") |
dkorittki
deleted the
dominik/eng-9431-router-support-loading-deployment-configs-in-the-router
branch
5 tasks
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.
Adds a new config poller implementation satisfying the
configpoller.ConfigPollerinterface. It fetches from the CDN first amapper.jsonfile to see what parts of the engine config have changed, fetches changed configs (feature flag graphs or base graph) and updates the engine config. The end result is always a completenodev1.RouterConfig, just like the other config poller produces as well.To use the new config poller the routers JWT token needs to have the
split-config-loadingfeature in thefeaturesclaim. If the token does not have this feature, the router uses the old config poller.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.