Fix gateway readiness always being reported as true#111
Fix gateway readiness always being reported as true#111Agent-Hellboy merged 3 commits intoAgent-Hellboy:mainfrom
Conversation
![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 updates the MCPServer controller to correctly handle resource readiness when the gateway feature is disabled. It modifies the phase determination logic to conditionally include gateway and policy resources and adjusts default readiness states. Review feedback suggests further improving consistency by explicitly setting the canary readiness state when disabled and refactoring the phase determination logic for better maintainability and clarity.
| gatewayReady := false | ||
| if gatewayEnabled(mcpServer) { | ||
| gatewayReady = deploymentReady | ||
| } |
There was a problem hiding this comment.
For consistency with gatewayReady and policyReady, canaryReady should also be set to false when the canary feature is disabled. This ensures that the status correctly reflects that the feature is not in use and prevents a logic bug in determinePhase where a disabled canary (which currently defaults to true in checkCanaryDeploymentReady) could cause the server to report PartiallyReady instead of Pending when no other resources are ready.
gatewayReady := false
if gatewayEnabled(mcpServer) {
gatewayReady = deploymentReady
}
if !canaryEnabled(mcpServer) {
canaryReady = false
}| var allReady bool | ||
| if gatewayEnabled(mcpServer) { | ||
| allReady = readiness.Deployment && readiness.Service && readiness.Ingress && readiness.Gateway && readiness.Policy && readiness.Canary | ||
| } else { | ||
| allReady = readiness.Deployment && readiness.Service && readiness.Ingress && readiness.Canary | ||
|
|
||
| } |
There was a problem hiding this comment.
The logic for determining allReady can be simplified and made more consistent by handling each optional component (Gateway and Canary) separately. This avoids duplicating the core readiness requirements and makes the function easier to maintain as more optional features are added. This refactoring also removes the unnecessary empty line at line 323.
| var allReady bool | |
| if gatewayEnabled(mcpServer) { | |
| allReady = readiness.Deployment && readiness.Service && readiness.Ingress && readiness.Gateway && readiness.Policy && readiness.Canary | |
| } else { | |
| allReady = readiness.Deployment && readiness.Service && readiness.Ingress && readiness.Canary | |
| } | |
| allReady := readiness.Deployment && readiness.Service && readiness.Ingress | |
| if gatewayEnabled(mcpServer) { | |
| allReady = allReady && readiness.Gateway && readiness.Policy | |
| } | |
| if canaryEnabled(mcpServer) { | |
| allReady = allReady && readiness.Canary | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
==========================================
- Coverage 58.11% 58.11% -0.01%
==========================================
Files 59 59
Lines 10678 10684 +6
==========================================
+ Hits 6206 6209 +3
- Misses 3877 3878 +1
- Partials 595 597 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Hi @aparimeet follow https://docs.mcpruntime.org/getting-started/#3-contributor-test-mode-cluster to test it out. |
2 participants
Summary
Fixes misleading MCPServer status when the gateway sidecar is not in use: gatewayReady and policyReady no longer read as “ready” when spec.gateway is absent or disabled, and phase (Ready vs not) is computed in line with whether the gateway is required.
Problem
With no spec.gateway (or gateway disabled), the operator could still report gatewayReady: true and policyReady: true, and overall readiness logic treated gateway/policy as satisfied. That did not match the live Deployment (no mcp-gateway container) and was confusing in kubectl, as reported in issue #94.
Solution
gatewayReady: default false; only derived from deployment readiness when spec.gateway.enabled is true.
policyReady: false when the gateway is not enabled; unchanged behavior when enabled (ConfigMap with policy data / policy.json).
determinePhase: takes the MCPServer in addition to resourceReadiness. When the gateway is enabled, Ready requires deployment, service, ingress, gateway, policy, and canary readiness. When the gateway is disabled, Ready requires deployment, service, ingress, and canary only (gateway/policy are not part of the gate).
Testing
go test ./internal/operator/...
Notes / follow-ups
When gateway is enabled, gatewayReady still follows deployment replica readiness only; a future improvement could assert the mcp-gateway container exists in the pod template so status cannot show gateway ready if the sidecar is missing (remaining gap from the issue’s strict acceptance).