Skip to content

Conversation

@JAORMX
Copy link
Collaborator

@JAORMX JAORMX commented Nov 26, 2025

Summary

This proposal adds support for selecting ConfigMaps via label selectors in MCPRegistry, providing a more Kubernetes-native approach to discovering registry data sources dynamically.

  • Adds new configMapSelector field for label-based ConfigMap discovery
  • Enables dynamic discovery without modifying MCPRegistry spec
  • Handles server name conflicts by prefixing both conflicting servers
  • Follows Kubernetes patterns (same namespace only, label selectors)

Key Design Decisions

  • New field: configMapSelector with matchLabels (no matchExpressions initially)
  • Conflict resolution: Both conflicting servers get prefixed (configmap-a/github-mcp, configmap-b/github-mcp)
  • Namespace scope: Same namespace only for security
  • Filters: Applied post-merge (see final names including prefixes)
  • Partial failures: Log warnings, continue with valid ConfigMaps, emit Event

Test plan

  • Review proposal for completeness
  • Discuss any open concerns
  • Rename file with PR number once merged

🤖 Generated with Claude Code

JAORMX and others added 2 commits November 26, 2025 16:38
This proposal adds support for selecting ConfigMaps via label selectors
in MCPRegistry, providing a more Kubernetes-native approach to discovering
registry data sources dynamically.

Key design decisions:
- New configMapSelector field (mutually exclusive with other sources)
- Conflict resolution: both conflicting servers get prefixed
- Same namespace only for security
- Post-merge filtering
- Partial failure with status update and Event emission

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@github-actions github-actions bot added the size/M Medium PR: 300-599 lines changed label Nov 26, 2025
@codecov
Copy link

codecov bot commented Nov 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.28%. Comparing base (5093c91) to head (58a7c58).
⚠️ Report is 39 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2755      +/-   ##
==========================================
+ Coverage   56.07%   56.28%   +0.21%     
==========================================
  Files         319      319              
  Lines       30697    30753      +56     
==========================================
+ Hits        17214    17310      +96     
+ Misses      12002    11955      -47     
- Partials     1481     1488       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@dmartinol dmartinol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BH, I'm not fully convinced by the proposal: we worked until now for a lighter controller and now it's back to fetch and merge registries.
Take into account that some previous decisions may affect the proposal:

  • Sync flow was moved to the server
  • Server configuration updates do not trigger new syncs
  • On-demand syncs are not managed on the controller

Comment on lines 143 to 144
- Fetch and parse registry data from each ConfigMap
- Implement conflict detection and prefixing logic
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This registry handling was moved to the registry server: do we really want to bring it back to the controller to generate the ConfigMap with the merged registry? 🤔

3. **Controller Updates**
- Add ConfigMap watch with label predicates
- Trigger reconciliation on matching ConfigMap changes
- Integrate selector handler with existing sync flow
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above: sync flow is in the registry server now


### Conflict Resolution

**Question:** What happens when multiple ConfigMaps define a server with the same name?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we instead map each ConfigMap to its own "registry" and create a file-backed configuration in the registry server?

BTW: I assume the server already handles the server name duplication, because it can already happen today

Copy link
Collaborator

@ChrisJBurns ChrisJBurns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't have thought that this change would have warranted a proposal tbh. The change itself is probably small enough to just do it - however, that being said, two things from me:

  • The wider proposal sounds like it's trying to do more than just the configmap change i.e. prefix logic and sync flow
  • I don't actually think we'll need the ConfigMapRef stuff anymore when we offer podTemplateSpec overrides. Because the user themselves will be able to specify the configMaps they want to mount to the registry server container (they can use selectors for this if they wanted), and then just configure them as a file based registry. This removes the need for us to have configMapRef logic anywhere in the code.

Address PR feedback by making the separation of concerns explicit:

- Add Architecture section with diagram showing operator/server boundary
- Clarify operator handles K8s primitives only (no sync logic)
- Registry server remains Kubernetes-agnostic (reads files)
- Document that syncPolicy.interval applies to server, not operator
- Add "Alternative Considered" section for multiple file sources
- Add directory source support as future registry server enhancement

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@github-actions github-actions bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Nov 27, 2025
@dmartinol
Copy link
Collaborator

  • I don't actually think we'll need the ConfigMapRef stuff anymore when we offer podTemplateSpec overrides. Because the user themselves will be able to specify the configMaps they want to mount to the registry server container (they can use selectors for this if they wanted), and then just configure them as a file based registry. This removes the need for us to have configMapRef logic anywhere in the code.

Do you want to add podTemplateSpec option for the registry through the controller? 🤔

@JAORMX
Copy link
Collaborator Author

JAORMX commented Nov 27, 2025

I think the controller reconciling multiple configmaps into volume mounts is less heavyweight than it modifying the pod template spec.

@dmartinol
Copy link
Collaborator

I think the controller reconciling multiple configmaps into volume mounts is less heavyweight than it modifying the pod template spec.

@rdimitrov we need some clarity on the directions.

@JAORMX
Copy link
Collaborator Author

JAORMX commented Nov 28, 2025

@dmartinol the template spec is meant for the user to add complex enhancements to the deployment. Letting the user configure a label selector on a dedicated path is a lot less disruptive and easier to reconcile.

@JAORMX JAORMX closed this Dec 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants