enhance: deploy one webhook container for system#6334
enhance: deploy one webhook container for system#6334thedadams merged 3 commits intoobot-platform:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Deploy webhook validation as shared SystemMCPServer-backed MCP endpoints (instead of per-MCP-server/per-user webhook sidecars), reducing runtime resource usage while keeping webhook filtering behavior.
Changes:
- Introduces a controller that ensures a managed
SystemMCPServerexists for eachMCPWebhookValidationand wires it to the validation resource. - Refactors MCP webhook discovery/consumption to use internal
/mcp-connect/URLs for these system servers and removes per-webhook containers from Docker/Kubernetes backends. - Updates OAuth token-exchange handling for internal connect URLs and adds cleanup linkage between
SystemMCPServerandMCPWebhookValidation.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/storage/openapi/generated/openapi_generated.go | Exposes webhookValidationName on SystemMCPServerSpec in generated OpenAPI schema. |
| pkg/storage/apis/obot.obot.ai/v1/systemmcpserver.go | Adds WebhookValidationName and implements DeleteRefs() so system servers can be cleaned up when the validation disappears. |
| pkg/services/config.go | Builds an informer indexer for MCPWebhookValidation, constructs the new WebhookHelper, and passes it into SessionManager. |
| pkg/mcp/webhookhelper.go | Changes webhook resolution to return internal connect URLs to system webhook servers (and removes per-webhook secret/image fields). |
| pkg/mcp/loader.go | Makes WebhookHelper a constructor dependency and removes the old Init flow / GPT client coupling. |
| pkg/mcp/kubernetes_test.go | Updates test backend construction to reflect removal of per-webhook container image fields. |
| pkg/mcp/kubernetes.go | Removes per-webhook sidecar containers and rewrites webhook URLs for in-cluster access. |
| pkg/mcp/docker.go | Removes per-webhook containers, adds deprecated webhook container cleanup, and rewrites webhook URLs for host reachability. |
| pkg/mcp/details.go | Updates call sites to the new webhook helper signature. |
| pkg/mcp/backend.go | Removes webhookToServerConfig (no more webhook sidecar deployments). |
| pkg/controller/routes.go | Wires new MCPWebhookValidation handler and adds cleanup handler for SystemMCPServer. |
| pkg/controller/mcpwebhookvalidation/mcpwebhookvalidation.go | New controller logic: cleans up stale resources, ensures managed SystemMCPServer, and manages webhook secret credential propagation. |
| pkg/controller/mcpwebhookvalidation/cleanupresources.go | Removes old handler file (functionality merged into the new handler). |
| pkg/controller/handlers/systemmcpserver/systemmcpserver.go | Credential/tool-name refactor and cleanup improvements; adds deletion of secret-info credential on finalization. |
| pkg/api/handlers/mcpgateway/oauth/token.go | Extends token-exchange logic for internal connect URLs (including system MCP servers) and adjusts system-server token-exchange behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c1b15f9 to
1e31444
Compare
1e31444 to
cf0e9ce
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This change will deploy one MCP webhook container for the system rather thn one per MCP server. For users that take advantage of webhooks, this will dramatically reduce their resource requirements. Signed-off-by: Donnie Adams <donnie@obot.ai>
cf0e9ce to
d10f2b8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Donnie Adams <donnie@obot.ai>
29b8b13 to
9f36846
Compare
Signed-off-by: Donnie Adams <donnie@obot.ai>
|
I simplified the token exchange changes. I did it in a separate commit, but it may be easier to review as part of the whole change. Re-requesting reviews for these changes. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
g-linville
left a comment
There was a problem hiding this comment.
The token exchange logic is clear to me now 👍
Not sure I understand it all, but at least what I added is clearer. |
This change will deploy one MCP webhook container for the system rather than one per MCP server. For users that take advantage of webhooks, this will dramatically reduce their resource requirements.
Issue: #6260