feat: dynamic config registry for extensible package-level configs#117
feat: dynamic config registry for extensible package-level configs#117gaarutyunov wants to merge 2 commits into
Conversation
…configs Adds a DynamicConfig wrapper over ProxyConfig that allows packages to register their own config sections under specific keys via init(). This enables SDK consumers to extend the proxy without modifying the core config package — eliminating conflicts when multiple features edit the same file and enabling true SDK-first extensibility. Key changes: - pkg/config/registry.go: New file with RegisterProxySection and RegisterUpstreamSection for proxy and upstream extension points. DynamicConfig wraps ProxyConfig and holds dynamically populated extension sections. GetProxySection[T] and GetUpstreamSection[T] provide typed access. mapProvider implements koanf.Provider for building scoped koanf instances from raw upstream map data. - pkg/config/loader.go: Load() now returns *DynamicConfig. Loader stores *DynamicConfig and its callback accepts *DynamicConfig. - pkg/mcp/manager.go: Rebuild() accepts *config.DynamicConfig. - pkg/mcpanything/proxy.go: Proxy.cfg and LoadConfig/New updated to DynamicConfig; functions expecting *ProxyConfig receive &cfg.ProxyConfig. - pkg/caddy/caddy.go: Updated for new Load() return type. - pkg/crd/v1alpha1/types.go: Added Extensions map[string]apiextensionsv1.JSON to MCPProxySpec and MCPUpstreamSpec for CRD-level extension. - pkg/crd/v1alpha1/zz_generated.deepcopy.go: Regenerated via controller-gen with deepcopy for new Extensions fields. - pkg/operator/configgen/generator.go: Extensions from CRD specs are decoded from JSON and inlined into the generated YAML using yaml:",inline". - charts/mcp-anything/crds/: Regenerated CRD manifests. Closes #113 Co-authored-by: German Arutyunov <gaarutyunov@users.noreply.github.com>
📝 WalkthroughWalkthroughThe PR introduces a dynamic configuration registry pattern across the system. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Pkg as Extension Package
participant Reg as Config Registry
participant Loader as Config Loader
participant Factory as Factory Function
participant Store as DynamicConfig Storage
Note over Pkg,Store: Extension Registration Phase (init-time)
Pkg->>Reg: RegisterProxySection(key, factory)
activate Reg
Reg->>Reg: Lock registry
Reg->>Reg: Add factory to registry[key]
Reg->>Reg: Unlock registry
deactivate Reg
Note over Pkg,Store: Config Loading Phase (runtime)
Loader->>Loader: Load(path) → parse YAML
Loader->>Reg: loadExtensions(d, koanf, rawUpstreams)
activate Reg
Reg->>Reg: Read lock & snapshot registries
loop For each registered proxy section key
Reg->>Factory: factory(koanf.Cut(key))
activate Factory
Factory-->>Reg: extension value
deactivate Factory
Reg->>Store: proxySections[key] = value
end
loop For each upstream
loop For each registered upstream section key
Reg->>Factory: factory(scoped koanf)
activate Factory
Factory-->>Reg: extension value
deactivate Factory
Reg->>Store: upstreamSections[i][key] = value
end
end
Reg->>Reg: Unlock registry
deactivate Reg
Loader->>Store: Return DynamicConfig with populated sections
sequenceDiagram
participant Op as Operator ConfigGen
participant Reg as ConfigGen Registry
participant ProxyFac as Proxy Factory
participant UpFac as Upstream Factory
participant Out as Generated Config
Note over Op,Out: Operator-driven Extension Generation
Op->>Op: Generate(ctx, proxy, upstreams)
Op->>Reg: applyProxyExtensions(ctx, proxy, upstreams, exts)
activate Reg
Reg->>Reg: Read lock & snapshot factories
loop For each registered proxy factory
Reg->>ProxyFac: factory(ctx, proxy, upstreams)
activate ProxyFac
ProxyFac-->>Reg: extension value
deactivate ProxyFac
Reg->>Out: exts[key] = value
end
Reg->>Reg: Unlock registry
deactivate Reg
loop For each upstream
Op->>Reg: applyUpstreamExtensions(ctx, upstream, exts)
activate Reg
Reg->>Reg: Read lock & snapshot factories
loop For each registered upstream factory
Reg->>UpFac: factory(ctx, upstream)
activate UpFac
UpFac-->>Reg: extension value
deactivate UpFac
Reg->>Out: exts[key] = value
end
Reg->>Reg: Unlock registry
deactivate Reg
end
Op->>Out: Return config with Extensions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
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 `@pkg/config/registry.go`:
- Around line 23-27: The register helpers currently overwrite existing entries
in proxyRegistry and upstreamRegistry under sectionMu; change
RegisterProxySection and RegisterUpstreamSection to acquire the write lock,
check if the key already exists, and fail loudly instead of replacing it (either
return an error from the registration function or panic/log.Fatal with a
descriptive message that includes the duplicate key and registry type) so
duplicate init-time registrations cannot silently clobber each other; ensure the
check-and-set is done atomically while holding sectionMu to avoid races.
- Around line 117-126: The loop over proxyCopy always calls each proxy factory
and unconditionally sets d.proxySections[key], which can make absent sections
appear present; change the logic in the proxy-level section construction to
first check whether the section key exists in the config before invoking the
factory (use the same presence check semantics as GetProxySection), and only
call f(sub) and assign d.proxySections[key] when the section is present; keep
using k.Cut(sub) for the factory input and preserve the existing error wrapping
when f(sub) returns an error.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d399b8c-7206-45e2-9d49-3f7f2edf4267
📒 Files selected for processing (10)
charts/mcp-anything/crds/mcpproxy.yamlcharts/mcp-anything/crds/mcpupstream.yamlpkg/caddy/caddy.gopkg/config/loader.gopkg/config/registry.gopkg/crd/v1alpha1/types.gopkg/crd/v1alpha1/zz_generated.deepcopy.gopkg/mcp/manager.gopkg/mcpanything/proxy.gopkg/operator/configgen/generator.go
|
@claude no extensions with additional properties. The typing will be regenerated with strict types for each proxy instance. The operator for this must be modular to in the same way proxy is. |
|
Claude finished @gaarutyunov's task in 13m 19s —— View job Changes pushed
What changed
CRD types —
|
|
@claude you also haven’t implemented anything. Just the registry. This is not what I meant. I meant migrating existing configs too, because all our code is modular. Let’s not implement it, but decompose the tasks into refactoring tasks: runtime configs, inbound and outbound configs, and others. Propose the decomposition first. |
|
Claude finished @gaarutyunov's task in 3m 46s —— View job Task decomposition: migrate existing configs to the registry pattern
The current PR only built the registry infrastructure ( Current state
Proposed refactoring tasksTask 1 — Runtime config (
|
|
@claude decomposition is good. You must add operator as a subtask to each task with integration tests. |
|
Claude finished @gaarutyunov's task in 43s —— View job Updated task decompositionEach task below covers:
Task 1 — Runtime config (
|
…erator configgen registry Remove Extensions map[string]apiextensionsv1.JSON from MCPProxySpec and MCPUpstreamSpec (and their CRD YAML schemas), eliminating the additionalProperties/x-kubernetes-preserve-unknown-fields approach. Add pkg/operator/configgen/registry.go with ProxySectionFactory and UpstreamSectionFactory types following the same init()-based registry pattern as pkg/config.RegisterProxySection. Packages register their own configgen factories that receive strongly-typed CRD values and return YAML sections to merge into the generated config. Also fix two CodeRabbit issues in pkg/config/registry.go: - Duplicate section registrations now panic instead of silently overwriting, preventing import-order-dependent collisions. - Proxy section factories are skipped when their key is absent from the config file, so absent sections cannot appear present via a zero-value factory return. Co-authored-by: German Arutyunov <gaarutyunov@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
pkg/config/registry.go (1)
23-27:⚠️ Potential issue | 🟠 MajorMove this registry out of package-global state and stop panicking from a library package.
This makes extension availability process-global and import-order dependent, and a duplicate registration crashes any embedding binary instead of returning a normal error path. Please hide the registry behind an explicit dependency passed into the loader and make registration/reporting non-panicking.
Based on learnings, "Applies to **/*.go : Avoid global mutable state; pass dependencies explicitly". As per coding guidelines, "Do not use
panic()in library code; only inmain()for unrecoverable startup failures".Also applies to: 34-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/registry.go` around lines 23 - 27, The package-global registries (proxyRegistry, upstreamRegistry) and sectionMu must be removed and replaced with an explicit registry type (e.g., type Registry struct { mu sync.RWMutex; proxy map[string]SectionFactory; upstream map[string]SectionFactory }) that is constructed and passed into the loader instead of relying on import-time globals; change any registration helpers (the code that currently writes to proxyRegistry/upstreamRegistry) to be methods on that Registry (e.g., Registry.RegisterProxy(name string, f SectionFactory) error, RegisterUpstream(...)) that return an error on duplicate registration instead of calling panic, and update loader/consumer code to accept a *Registry parameter so availability is not process-global or import-order dependent. Ensure all accesses use the Registry's mu and maps and remove the package-level vars sectionMu/proxyRegistry/upstreamRegistry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/config/registry.go`:
- Around line 29-55: Prevent extension name collisions by rejecting reserved
core keys during registration: add a set/list of reserved keys (including
"server", "naming", "upstreams", "name", "type") and in both
RegisterProxySection and RegisterUpstreamSection check if the given key exists
in that reserved set before taking the sectionMu lock (or immediately after) and
panic with a clear message like "config: key %q is reserved by core config —
registration conflict" if it is reserved; otherwise proceed with the existing
duplicate-registration check and assignment to proxyRegistry/upstreamRegistry.
- Around line 152-158: The code currently treats a present but non-object
upstream entry as "absent" by doing `rawMap, _ :=
rawVal.(map[string]interface{})` and `if rawMap == nil { continue }`; instead,
in the function handling upstream parsing (the block that reads `rawUp`,
`rawVal`, `rawMap`), return a typed error when the value exists but is not a
map—e.g. construct and return an error like `fmt.Errorf("upstream %q: expected
object, got %T", key, rawVal)` or use a package-level sentinel error
(ErrMalformedUpstreamSection) so the caller can fail fast; replace the
`continue` with this error return and update callers/signatures as needed to
propagate the error.
In `@pkg/operator/configgen/registry.go`:
- Around line 36-59: The RegisterProxySectionFactory and
RegisterUpstreamSectionFactory functions should reject reserved YAML keys before
inserting into proxyGenRegistry or upstreamGenRegistry; add a small reserved-key
set (e.g., {"server","upstreams","name","transport"} or similar core fields)
and, while holding genMu, check if the provided key is in that set and panic (or
return an explicit error) with a clear message about reserved core keys to
prevent collisions; apply this same pre-check logic to both
RegisterProxySectionFactory and RegisterUpstreamSectionFactory so reserved names
are never registered.
- Around line 72-78: The loop over snapshot currently inserts entries even when
a factory returns nil; update the loop that iterates "for key, f := range
snapshot" (and the analogous loop at lines 92-98) to skip adding to exts when
the factory result v is nil by checking "if v == nil { continue }" before
"exts[key] = v", so that Generate omits sections instead of emitting key: null.
- Around line 30-34: The package currently exposes package-level globals genMu,
proxyGenRegistry and upstreamGenRegistry which mirror runtime global mutable
state and use panic on duplicate registration; instead create a Registry struct
(holding the maps and mutex) and change functions that register or look up
ProxySectionFactory and UpstreamSectionFactory to be methods on that Registry so
callers inject an instance into config generation, and replace any panic() on
duplicate registration with a returned error (or a boolean) from the
RegisterProxy/ RegisterUpstream methods so callers can handle/report duplicates
rather than crashing the process.
---
Duplicate comments:
In `@pkg/config/registry.go`:
- Around line 23-27: The package-global registries (proxyRegistry,
upstreamRegistry) and sectionMu must be removed and replaced with an explicit
registry type (e.g., type Registry struct { mu sync.RWMutex; proxy
map[string]SectionFactory; upstream map[string]SectionFactory }) that is
constructed and passed into the loader instead of relying on import-time
globals; change any registration helpers (the code that currently writes to
proxyRegistry/upstreamRegistry) to be methods on that Registry (e.g.,
Registry.RegisterProxy(name string, f SectionFactory) error,
RegisterUpstream(...)) that return an error on duplicate registration instead of
calling panic, and update loader/consumer code to accept a *Registry parameter
so availability is not process-global or import-order dependent. Ensure all
accesses use the Registry's mu and maps and remove the package-level vars
sectionMu/proxyRegistry/upstreamRegistry.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c4687ed-1056-42d4-a51f-fcc630834d07
📒 Files selected for processing (4)
pkg/config/registry.gopkg/crd/v1alpha1/zz_generated.deepcopy.gopkg/operator/configgen/generator.gopkg/operator/configgen/registry.go
✅ Files skipped from review due to trivial changes (1)
- pkg/crd/v1alpha1/zz_generated.deepcopy.go
| // RegisterProxySection registers a factory for a top-level proxy config section. | ||
| // key is the koanf path in the proxy config (e.g. "tool_search", "session_store"). | ||
| // The factory is called with a koanf subtree scoped to that key on every Load. | ||
| // Typically called from init() in packages that define their own proxy-level config. | ||
| // Panics if a factory is already registered under the same key. | ||
| func RegisterProxySection(key string, f SectionFactory) { | ||
| sectionMu.Lock() | ||
| defer sectionMu.Unlock() | ||
| if _, exists := proxyRegistry[key]; exists { | ||
| panic(fmt.Sprintf("config: proxy section %q is already registered — import conflict", key)) | ||
| } | ||
| proxyRegistry[key] = f | ||
| } | ||
|
|
||
| // RegisterUpstreamSection registers a factory for a per-upstream config section. | ||
| // key is the sub-key within each upstream config object in the YAML. | ||
| // The factory is called once per upstream per Load for upstreams that contain the key. | ||
| // Typically called from init() in packages that define upstream-level config. | ||
| // Panics if a factory is already registered under the same key. | ||
| func RegisterUpstreamSection(key string, f SectionFactory) { | ||
| sectionMu.Lock() | ||
| defer sectionMu.Unlock() | ||
| if _, exists := upstreamRegistry[key]; exists { | ||
| panic(fmt.Sprintf("config: upstream section %q is already registered — import conflict", key)) | ||
| } | ||
| upstreamRegistry[key] = f | ||
| } |
There was a problem hiding this comment.
Reject reserved core keys at registration time.
Right now an extension can register a key like server, naming, upstreams, name, or type, and the runtime loader will start treating built-in config as an extension section. Add a reserved-key check here so extension namespaces cannot collide with the core config schema.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/config/registry.go` around lines 29 - 55, Prevent extension name
collisions by rejecting reserved core keys during registration: add a set/list
of reserved keys (including "server", "naming", "upstreams", "name", "type") and
in both RegisterProxySection and RegisterUpstreamSection check if the given key
exists in that reserved set before taking the sectionMu lock (or immediately
after) and panic with a clear message like "config: key %q is reserved by core
config — registration conflict" if it is reserved; otherwise proceed with the
existing duplicate-registration check and assignment to
proxyRegistry/upstreamRegistry.
| rawVal, ok := rawUp[key] | ||
| if !ok { | ||
| continue | ||
| } | ||
| rawMap, _ := rawVal.(map[string]interface{}) | ||
| if rawMap == nil { | ||
| continue |
There was a problem hiding this comment.
Don't silently drop malformed upstream sections.
Once the key exists, a non-object value currently gets treated as “section absent”, which hides bad config and disables the extension without any load error. Return a typed error here instead of continue.
Suggested fix
for key, f := range upstreamCopy {
rawVal, ok := rawUp[key]
if !ok {
continue
}
rawMap, _ := rawVal.(map[string]interface{})
if rawMap == nil {
- continue
+ return fmt.Errorf("upstream[%d] section %q: expected object, got %T", i, key, rawVal)
}
// Build a scoped koanf instance from the raw map so the factory can
// use the standard k.Unmarshal("", &target) pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/config/registry.go` around lines 152 - 158, The code currently treats a
present but non-object upstream entry as "absent" by doing `rawMap, _ :=
rawVal.(map[string]interface{})` and `if rawMap == nil { continue }`; instead,
in the function handling upstream parsing (the block that reads `rawUp`,
`rawVal`, `rawMap`), return a typed error when the value exists but is not a
map—e.g. construct and return an error like `fmt.Errorf("upstream %q: expected
object, got %T", key, rawVal)` or use a package-level sentinel error
(ErrMalformedUpstreamSection) so the caller can fail fast; replace the
`continue` with this error return and update callers/signatures as needed to
propagate the error.
| var ( | ||
| genMu sync.RWMutex | ||
| proxyGenRegistry = map[string]ProxySectionFactory{} | ||
| upstreamGenRegistry = map[string]UpstreamSectionFactory{} | ||
| ) |
There was a problem hiding this comment.
This repeats the runtime registry's process-global mutable state and panic behavior in another library package.
The operator side now has the same import-order coupling and hard crash path as the runtime side. Please inject a registry instance into config generation instead of relying on package globals, and report duplicate registrations without panic().
As per coding guidelines, "Do not use panic() in library code; only in main() for unrecoverable startup failures". Based on learnings, "Applies to **/*.go : Avoid global mutable state; pass dependencies explicitly".
Also applies to: 40-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/configgen/registry.go` around lines 30 - 34, The package
currently exposes package-level globals genMu, proxyGenRegistry and
upstreamGenRegistry which mirror runtime global mutable state and use panic on
duplicate registration; instead create a Registry struct (holding the maps and
mutex) and change functions that register or look up ProxySectionFactory and
UpstreamSectionFactory to be methods on that Registry so callers inject an
instance into config generation, and replace any panic() on duplicate
registration with a returned error (or a boolean) from the RegisterProxy/
RegisterUpstream methods so callers can handle/report duplicates rather than
crashing the process.
| // RegisterProxySectionFactory registers a factory for a top-level proxy config | ||
| // section that is injected by the operator when generating proxy config YAML. | ||
| // key is the top-level YAML key the returned value will be set at. | ||
| // Panics if a factory is already registered under the same key. | ||
| func RegisterProxySectionFactory(key string, f ProxySectionFactory) { | ||
| genMu.Lock() | ||
| defer genMu.Unlock() | ||
| if _, exists := proxyGenRegistry[key]; exists { | ||
| panic(fmt.Sprintf("configgen: proxy section factory %q is already registered — import conflict", key)) | ||
| } | ||
| proxyGenRegistry[key] = f | ||
| } | ||
|
|
||
| // RegisterUpstreamSectionFactory registers a factory for a per-upstream config | ||
| // section that is injected by the operator when generating upstream config YAML. | ||
| // key is the YAML key within the upstream object the returned value will be set at. | ||
| // Panics if a factory is already registered under the same key. | ||
| func RegisterUpstreamSectionFactory(key string, f UpstreamSectionFactory) { | ||
| genMu.Lock() | ||
| defer genMu.Unlock() | ||
| if _, exists := upstreamGenRegistry[key]; exists { | ||
| panic(fmt.Sprintf("configgen: upstream section factory %q is already registered — import conflict", key)) | ||
| } | ||
| upstreamGenRegistry[key] = f |
There was a problem hiding this comment.
Block extension keys that collide with generated core YAML fields.
Because these values are later inlined into the generated YAML, registering keys like server, upstreams, name, or transport can collide with the base schema and produce ambiguous output. Reject reserved keys here before they ever enter the registry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/configgen/registry.go` around lines 36 - 59, The
RegisterProxySectionFactory and RegisterUpstreamSectionFactory functions should
reject reserved YAML keys before inserting into proxyGenRegistry or
upstreamGenRegistry; add a small reserved-key set (e.g.,
{"server","upstreams","name","transport"} or similar core fields) and, while
holding genMu, check if the provided key is in that set and panic (or return an
explicit error) with a clear message about reserved core keys to prevent
collisions; apply this same pre-check logic to both RegisterProxySectionFactory
and RegisterUpstreamSectionFactory so reserved names are never registered.
| for key, f := range snapshot { | ||
| v, err := f(ctx, proxy, upstreams) | ||
| if err != nil { | ||
| return fmt.Errorf("proxy section factory %q: %w", key, err) | ||
| } | ||
| exts[key] = v | ||
| } |
There was a problem hiding this comment.
Treat nil factory results as “omit this section”.
Right now a factory that returns nil, nil still adds an entry to exts, so Generate emits inline key: null blocks instead of leaving the section absent. Skip insertion when the factory result is nil.
Suggested fix
for key, f := range snapshot {
v, err := f(ctx, proxy, upstreams)
if err != nil {
return fmt.Errorf("proxy section factory %q: %w", key, err)
}
+ if v == nil {
+ continue
+ }
exts[key] = v
}
@@
for key, f := range snapshot {
v, err := f(ctx, upstream)
if err != nil {
return fmt.Errorf("upstream section factory %q: %w", key, err)
}
+ if v == nil {
+ continue
+ }
exts[key] = v
}Also applies to: 92-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/configgen/registry.go` around lines 72 - 78, The loop over
snapshot currently inserts entries even when a factory returns nil; update the
loop that iterates "for key, f := range snapshot" (and the analogous loop at
lines 92-98) to skip adding to exts when the factory result v is nil by checking
"if v == nil { continue }" before "exts[key] = v", so that Generate omits
sections instead of emitting key: null.
1 participant
Implements the dynamic config registry described in #113.
Adds a
DynamicConfigwrapper overProxyConfigthat allows packages to register their own config sections viainit(), eliminating conflicts when multiple features edit the monolithic config file. Operator CRDs gainExtensionsfields for passing arbitrary package config through Kubernetes.Closes #113
Generated with Claude Code
Summary by CodeRabbit
Release Notes