Inject INTERNAL_APP_ENDPOINT_PATTERN into KService pods for in-cluster app discovery#7323
Inject INTERNAL_APP_ENDPOINT_PATTERN into KService pods for in-cluster app discovery#7323AdilFayyaz wants to merge 1 commit intomainfrom
Conversation
Signed-off-by: M. Adil Fayyaz <62440954+AdilFayyaz@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to enable in-cluster Flyte app-to-app discovery by injecting an INTERNAL_APP_ENDPOINT_PATTERN environment variable into Knative Service (KService) pods at deploy time, so SDK code can resolve internal service URLs without hardcoding full cluster DNS names.
Changes:
- Inject
INTERNAL_APP_ENDPOINT_PATTERNinto the first container of each deployed KService pod. - Add
NamespacedNamePrefixTemplatetoInternalAppConfig(intended to support templated internal endpoint patterns).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
app/internal/k8s/app_client.go |
Injects INTERNAL_APP_ENDPOINT_PATTERN into KService pod env during manifest construction. |
app/config/config.go |
Adds a new InternalAppConfig field intended for templating the internal endpoint pattern. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Name: "INTERNAL_APP_ENDPOINT_PATTERN", | ||
| Value: fmt.Sprintf("http://{app_fqdn}-%s-%s.%s.svc.cluster.local", | ||
| strings.ToLower(appID.GetProject()), | ||
| strings.ToLower(appID.GetDomain()), | ||
| ns), | ||
| }) |
There was a problem hiding this comment.
The generated INTERNAL_APP_ENDPOINT_PATTERN does not match the actual KService naming logic for apps with mixed-case or long names. KServiceName() lowercases the full name and, when >63 chars, truncates and appends a SHA256 suffix; this pattern assumes the service name is always "{app_fqdn}-{project}-{domain}". For long app names, AppEndpoint resolution will produce a hostname that can never exist. Consider aligning the pattern/placeholder with KServiceName() semantics (e.g., have the SDK compute the KServiceName and substitute that), or enforce/reject names that would trigger hashing so the pattern remains valid.
| if len(podSpec.Containers) > 0 { | ||
| podSpec.Containers[0].Env = append(podSpec.Containers[0].Env, corev1.EnvVar{ | ||
| Name: "INTERNAL_APP_ENDPOINT_PATTERN", | ||
| Value: fmt.Sprintf("http://{app_fqdn}-%s-%s.%s.svc.cluster.local", | ||
| strings.ToLower(appID.GetProject()), | ||
| strings.ToLower(appID.GetDomain()), | ||
| ns), | ||
| }) |
There was a problem hiding this comment.
This unconditionally appends INTERNAL_APP_ENDPOINT_PATTERN to container env. If users previously worked around this by setting the same env var in their app spec (or via DefaultEnvVars), the resulting duplicate env var name will cause KService validation to fail. Suggest checking for an existing env var with this name and updating/overwriting it (or skipping injection if already set) instead of always appending a new entry.
| // Inject INTERNAL_APP_ENDPOINT_PATTERN so app code can construct internal cluster URLs | ||
| // for other apps by substituting {app_fqdn} with the target app name. | ||
| // The pattern includes project and domain as a suffix to match the KService name format | ||
| // {name}-{project}-{domain} used by KServiceName(). | ||
| if len(podSpec.Containers) > 0 { | ||
| podSpec.Containers[0].Env = append(podSpec.Containers[0].Env, corev1.EnvVar{ | ||
| Name: "INTERNAL_APP_ENDPOINT_PATTERN", | ||
| Value: fmt.Sprintf("http://{app_fqdn}-%s-%s.%s.svc.cluster.local", | ||
| strings.ToLower(appID.GetProject()), | ||
| strings.ToLower(appID.GetDomain()), | ||
| ns), | ||
| }) | ||
| } |
There was a problem hiding this comment.
There are existing unit tests in this package covering Deploy/Stop/KServiceName, but no test asserting INTERNAL_APP_ENDPOINT_PATTERN injection/value. Add a test that deploys an app and verifies the env var is present and matches the expected in-cluster URL template (and ideally cover the long-name hashing edge case if that behavior is supported).
| // NamespacedNamePrefixTemplate is the template for generating the INTERNAL_APP_ENDPOINT_PATTERN env var. | ||
| // Supported variables: {{ project }}, {{ domain }}, {{ org }}. | ||
| // Example: "{{ project }}-{{ domain }}-" | ||
| NamespacedNamePrefixTemplate string `json:"namespacedNamePrefixTemplate" pflag:",Template for internal app endpoint pattern (e.g., {{ project }}-{{ domain }}-)"` | ||
|
|
There was a problem hiding this comment.
NamespacedNamePrefixTemplate is introduced with comments implying it is used to generate INTERNAL_APP_ENDPOINT_PATTERN, but there is no code path that reads this field. This makes the config misleading and risks bitrot. Either wire it into buildKService (and document the actual templating implementation), or remove/adjust the field and comment until it's supported.
| // NamespacedNamePrefixTemplate is the template for generating the INTERNAL_APP_ENDPOINT_PATTERN env var. | |
| // Supported variables: {{ project }}, {{ domain }}, {{ org }}. | |
| // Example: "{{ project }}-{{ domain }}-" | |
| NamespacedNamePrefixTemplate string `json:"namespacedNamePrefixTemplate" pflag:",Template for internal app endpoint pattern (e.g., {{ project }}-{{ domain }}-)"` |
| // NamespacedNamePrefixTemplate is the template for generating the INTERNAL_APP_ENDPOINT_PATTERN env var. | ||
| // Supported variables: {{ project }}, {{ domain }}, {{ org }}. | ||
| // Example: "{{ project }}-{{ domain }}-" | ||
| NamespacedNamePrefixTemplate string `json:"namespacedNamePrefixTemplate" pflag:",Template for internal app endpoint pattern (e.g., {{ project }}-{{ domain }}-)"` |
There was a problem hiding this comment.
It's for dev mode (running manager locally) right? Could we add a default here https://github.com/flyteorg/flyte/pull/7323/files#diff-2a2301646ea40f7638775b9a0cfbd0cb8c9f081b5f8732a73f44767aeae95e9dL23-L26
There was a problem hiding this comment.
this isnt only for the dev mode, it should work in both
There was a problem hiding this comment.
Got it. Is it supposed to be used in app_client.go? I didn’t see any files using it
Why are the changes needed?
When multiple Flyte apps run in the same cluster (e.g. a Gradio app calling a vLLM model-serving app), apps need to call each other via the internal Kubernetes service URL (*.flyte.svc.cluster.local). Previouly the
INTERNAL_APP_ENDPOINT_PATTERNenv var — which the Flyte SDK'sAppEndpointuses to resolve internal URLs — was never injected into KService pods. This meantAppEndpoint(app_name="...")had no pattern to resolve against, and developers had to manually construct and hardcode the full internal cluster URL.What changes were proposed in this pull request?
INTERNAL_APP_ENDPOINT_PATTERNinto every KService container at deploy time in buildKService. The value is a URL template with an {app_fqdn} placeholder that the SDK substitutes at runtime with the target app's name:http://{app_fqdn}-{project}-{domain}.flyte.svc.cluster.localhttp://gemma4-26b-flytesnacks-development.flyte.svc.cluster.localCode Changes
app/config/config.go— addsNamespacedNamePrefixTemplatefield toInternalAppConfigapp/internal/k8s/app_client.go— injectsINTERNAL_APP_ENDPOINT_PATTERNinbuildKServiceHow was this patch tested?
Verified using the
app_calling_appSDK example (examples/apps/app_calling_app/app.py). After this change, hittingapp2-calls-app1.../greeting/worldcorrectly proxies through to app1 via the internal cluster URLThis is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Stack
If you do use
git townto manage PR Stacks, the stack relevant to this PRwill show below. Otherwise, you can ignore this section.
Docs link