Feature: Add multi-selector support to flagd#1898
Feature: Add multi-selector support to flagd#1898Nilushiya wants to merge 1 commit intoopen-feature:mainfrom
Conversation
✅ Deploy Preview for polite-licorice-3db33c ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
dosubot
bot
added
the
size:L
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. |
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances flagd's capability to manage feature flags in multi-tenant or hierarchical environments. By introducing multi-selector support, clients can now specify multiple flag sets to be evaluated in a defined order, enabling more granular control and dynamic configuration overrides. This change provides greater flexibility for developers to implement sophisticated flag management strategies, improving the adaptability of applications. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces multi-selector support for flag evaluation and synchronization. However, the current implementation has critical security concerns, notably a vulnerability to Denial of Service (DoS) attacks due to the lack of limits on the number of selectors processed in a single request, affecting both evaluation and synchronization services. There's also a logic error in the OFREP single evaluation endpoint that could lead to broken access control by incorrectly parsing multi-selector headers. Additionally, inconsistencies in the flag merging logic for the sync service and redundant code in the gRPC header parsing require attention to ensure overall correctness and consistency.
| func (s syncHandler) fetchMergedFlags(ctx context.Context, selectors []store.Selector) ([]model.Flag, error) { | ||
| switch len(selectors) { | ||
| case 0: | ||
| flags, _, err := s.store.GetAll(ctx, nil) | ||
| return flags, err | ||
| case 1: | ||
| flags, _, err := s.store.GetAll(ctx, &selectors[0]) | ||
| return flags, err | ||
| default: | ||
| type flagIdentifier struct { | ||
| flagSetID string | ||
| key string | ||
| } | ||
|
|
||
| merged := map[flagIdentifier]model.Flag{} | ||
| for _, selector := range selectors { | ||
| flags, _, err := s.store.GetAll(ctx, &selector) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, flag := range flags { | ||
| merged[flagIdentifier{flagSetID: flag.FlagSetId, key: flag.Key}] = flag | ||
| } | ||
| } | ||
|
|
||
| out := make([]model.Flag, 0, len(merged)) | ||
| for _, flag := range merged { | ||
| out = append(out, flag) | ||
| } | ||
| sort.Slice(out, func(i, j int) bool { | ||
| if out[i].FlagSetId != out[j].FlagSetId { | ||
| return out[i].FlagSetId < out[j].FlagSetId | ||
| } | ||
| return out[i].Key < out[j].Key | ||
| }) | ||
| return out, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The fetchMergedFlags function, used by both SyncFlags and FetchAllFlags, is vulnerable to a Denial of Service (DoS) attack. It iterates over an unbounded list of selectors and performs a store query for each, which is an expensive operation triggered on every flag update when multiple selectors are used. This loop lacks a limit on the number of selectors and does not check for context cancellation. Additionally, the merging logic uses a composite key of (flagSetID, key), which is inconsistent with the requirement to override flags based on the FlagKey alone. This will not perform an override if two flags have the same key but different FlagSetIds. To ensure correct override behavior and mitigate the DoS vulnerability, implement a limit on the number of selectors in parseSelectorList, ensure the merge loop respects context cancellation, and use only the flag key for merging.
type flagIdentifier struct {
key string
}
merged := map[string]model.Flag{}
for _, selector := range selectors {
flags, _, err := s.store.GetAll(ctx, &selector)
if err != nil {
return nil, err
}
for _, flag := range flags {
merged[flag.Key] = flag
}| func ResolveAllWithSelectorMerge( | ||
| ctx context.Context, | ||
| reqID string, | ||
| eval evaluator.IEvaluator, | ||
| evaluationContext map[string]any, | ||
| selectorExpression string, | ||
| ) ([]evaluator.AnyValue, model.Metadata, error) { | ||
| selectors := splitSelectorExpression(selectorExpression) | ||
|
|
||
| switch len(selectors) { | ||
| case 0: | ||
| return eval.ResolveAllValues(ctx, reqID, evaluationContext) | ||
| case 1: | ||
| selector := store.NewSelector(selectors[0]) | ||
| selectorCtx := context.WithValue(ctx, store.SelectorContextKey{}, selector) | ||
| return eval.ResolveAllValues(selectorCtx, reqID, evaluationContext) | ||
| default: | ||
| mergedValues := map[string]evaluator.AnyValue{} | ||
| mergedMetadata := model.Metadata{} | ||
|
|
||
| for _, selectorExpression := range selectors { | ||
| selector := store.NewSelector(selectorExpression) | ||
| selectorCtx := context.WithValue(ctx, store.SelectorContextKey{}, selector) | ||
| values, metadata, err := eval.ResolveAllValues(selectorCtx, reqID, evaluationContext) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| for key, value := range metadata { | ||
| mergedMetadata[key] = value | ||
| } | ||
| for _, value := range values { | ||
| mergedValues[value.FlagKey] = value | ||
| } | ||
| } | ||
|
|
||
| keys := make([]string, 0, len(mergedValues)) | ||
| for key := range mergedValues { | ||
| keys = append(keys, key) | ||
| } | ||
| sort.Strings(keys) | ||
|
|
||
| resolutions := make([]evaluator.AnyValue, 0, len(keys)) | ||
| for _, key := range keys { | ||
| resolutions = append(resolutions, mergedValues[key]) | ||
| } | ||
|
|
||
| return resolutions, mergedMetadata, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The ResolveAllWithSelectorMerge function is vulnerable to a Denial of Service (DoS) attack. It splits the selectorExpression by commas and iterates over the resulting slice without any limit on the number of selectors. An attacker can provide a large number of selectors in the request headers, causing the server to perform a full flag evaluation for each one. This process consumes significant CPU and memory, and the loop does not check for context cancellation (ctx.Done()), meaning it will run to completion even if the client disconnects.
Remediation: Implement a limit on the maximum number of selectors allowed (e.g., 10) and check the context status at the beginning of each iteration.
| selectorExpression := service.SelectorExpressionFromHTTPHeaders(r.Header) | ||
| selector := store.NewSelector(selectorExpression) |
There was a problem hiding this comment.
In HandleFlagEvaluation, the selectorExpression (which can now be a comma-separated list of multiple selectors) is passed directly to store.NewSelector. This is a critical logic error as store.NewSelector is designed for a single selector, leading to incorrect parsing or an empty selector. This can cause the evaluator to fall back to the highest priority flag across all sources, potentially bypassing intended isolation between flag sets (e.g., tenant isolation), which is a broken access control vulnerability. This behavior is inconsistent with HandleBulkEvaluation. Update this endpoint to correctly handle multi-selectors, for example by splitting the expression and using the first valid selector if multi-merge is not yet supported for single evaluations.
| func SelectorExpressionFromGRPCMetadata(md metadata.MD) string { | ||
| if selectors := strings.TrimSpace(strings.Join(md.Get(strings.ToLower(FLAG_SELECTOR_HEADER)), ",")); selectors != "" { | ||
| return selectors | ||
| } | ||
| if selectors := strings.TrimSpace(strings.Join(md.Get(strings.ToLower(FLAGD_SELECTOR_HEADER)), ",")); selectors != "" { | ||
| return selectors | ||
| } | ||
| if selectors := strings.TrimSpace(strings.Join(md.Get(FLAG_SELECTOR_HEADER), ",")); selectors != "" { | ||
| return selectors | ||
| } | ||
| return strings.TrimSpace(strings.Join(md.Get(FLAGD_SELECTOR_HEADER), ",")) | ||
| } |
There was a problem hiding this comment.
The logic for reading gRPC metadata is a bit complex and contains redundancy. Specifically, FLAG_SELECTOR_HEADER is checked twice. Also, gRPC metadata keys are typically normalized to lowercase, so checking for the original case of headers is often unnecessary and can be confusing.
Simplifying this function would improve readability and maintainability while retaining the correct behavior of prioritizing the new flag-selector header.
func SelectorExpressionFromGRPCMetadata(md metadata.MD) string {
// gRPC metadata keys are conventionally lowercase.
if selectors := strings.TrimSpace(strings.Join(md.Get(strings.ToLower(FLAG_SELECTOR_HEADER)), ",")); selectors != "" {
return selectors
}
// Fallback to the old header.
return strings.TrimSpace(strings.Join(md.Get(strings.ToLower(FLAGD_SELECTOR_HEADER)), ","))
}1 participant
This PR
Adds multi-selector support for flag evaluation and sync by allowing clients to pass an ordered list of selectors and merging results so later selectors override earlier ones.
Adds HTTP header support for multi-selectors via the new flag-selector header (comma-separated), with fallback to the existing Flagd-Selector header.
Implements ordered multi-merge for ResolveAll by evaluating per selector in sequence and merging by flag key.
Related Issues
Fixes #1234523
Notes
New header: flag-selector (preferred). Example: flag-selector: flagSetId=base,flagSetId=tenantA,flagSetId=tenantB
Backward compatible: existing Flagd-Selector still works (single selector), and also works with comma-separated selector lists after this change.
Merge semantics: selectors are applied left-to-right; later selector results override earlier ones for the same FlagKey.
Key code changes:
flagd/pkg/service/constants.go: adds FLAG_SELECTOR_HEADER = "flag-selector"
flagd/pkg/service/selector.go: helper functions to read selector expression from HTTP headers and gRPC metadata (prefers flag-selector)
flagd/pkg/service/flag-evaluation/selector_merge.go: adds ResolveAllWithSelectorMerge to evaluate selectors in order and merge results
flagd/pkg/service/flag-evaluation/flag_evaluator_v1.go and flagd/pkg/service/flag-evaluation/flag_evaluator.go: ResolveAll now uses ResolveAllWithSelectorMerge
flagd/pkg/service/flag-sync/handler.go: adds ordered selector parsing + merge for sync fetch/stream
Tests added/updated:
flagd/pkg/service/selector_test.go
flagd/pkg/service/flag-sync/handler_test.go
flagd/pkg/service/flag-evaluation/ofrep/handler_test.go
Follow-up Tasks
Extend multi-selector behavior consistently to other evaluation endpoints (non-bulk Resolve* calls) if required.
Align streaming/watch behavior for multi-selectors across all services (if stricter per-selector watch semantics are needed).
How to test
Build and run:
docker build -f flagd/build.Dockerfile -t flagd:multi-selector .
docker compose up -d
Postman (ResolveAll):
POST http://localhost:8013/flagd.evaluation.v1.Service/ResolveAll
Headers:
Content-Type: application/json
flag-selector: flagSetId=base,flagSetId=tenantA (order matters)
Body: {} or {"context":{"Hospital":"A1"}}
Expect: flags from tenantA override flags from base for overlapping keys.
refer this repo : https://github.com/Nilushiya/Multi-tenat-with-multi-selector.git