Repository navigation
[INF-8330] Fqdn service hosts - #411
justinyang-retool wants to merge 6 commits into
Conversation
The backend's HTTP client resolves hosts with cacheable-lookup, which calls dns.resolve4/resolve6 (c-ares) and never applies the resolv.conf search list. A bare service name such as `retool-code-executor` only works today because most cluster DNS servers answer it with NXDOMAIN, which makes cacheable-lookup fall back to dns.lookup. EKS Auto Mode's DNS answers REFUSED and AKS's answers SERVFAIL, so the fallback never runs and the backend crash-loops with "Could not detect healthy Code Executor". Append `.<namespace>.svc.cluster.local` to every in-cluster service URL and host the chart passes to Retool, matching the agent-sandbox proxy URL the chart already builds this way. Fixes #348. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Add a clusterDomain value (default cluster.local) and a retool.serviceDomain helper, and build every in-cluster service address from it, including the existing agent-sandbox proxy URL. Hard-coding svc.cluster.local would break clusters with a different DNS domain, where the bare names used to resolve through the search list. Also qualify WORKFLOW_BACKEND_HOST and JS_EXECUTOR_INGRESS_DOMAIN in the separate app-serving layer, which the previous commit missed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The new serviceDomain helper sat between "Set Temporal frontend host" and the helper that comment describes, with a docstring that only restated what the helper's one line does. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Co-Authored-By: gpt-6.1-sol <codex@openai.com>
# Conflicts: # charts/retool/Chart.yaml
 [Medium impact] The change reaches internal service connections across the Retool chart. The PR appears safe to merge, with a non-blocking request to test custom service domains and namespaces. Findings
Reviews (1) · Last reviewed commit: "Fix breaking test" · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| self.assertEqual(ingress_routes(output), INGRESS_DIRECT) | ||
| self.assertEqual(http_routes(output), HTTP_DIRECT) | ||
| self.assertIn('value: "http://routing-retool-mcp:4010"', manifest(output, "deployment_backend.yaml")) | ||
| self.assertIn('value: "http://routing-retool-mcp.default.svc.cluster.local:4010"', manifest(output, "deployment_backend.yaml")) |
There was a problem hiding this comment.
This assertion checks only MCP with the default namespace and clusterDomain. It would still pass if retool.serviceDomain hardcoded both values, and it does not check the other changed service URLs.
Add render tests with a non-default namespace and clusterDomain, covering backend and worker URLs plus the sandbox and bundled Temporal defaults. Also check that explicit URL overrides still win. This is non-blocking, but it would catch broken addresses that the current test misses.
Knowledge Base Used: Helm validation coverage
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Making the fix mentioned in PR #409, but with tests added (since I can't push to that contributor fork)