fix: make KService naming injective (#7622) - #7746
Draft
paullongtan wants to merge 2 commits into
Draft
Conversation
Signed-off-by: paullongtan <paullongtan@gmail.com>
Signed-off-by: paullongtan <paullongtan@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
Related to #7622
KServiceName()is non-injective, so distinct(project, domain, name)triples can map to the same Knative Service name. This would allow collisions to silently overwrite another app's spec instead of creating a separate kService.Collision cause pointed out in the issue:
-, but project and domain may themselves contain-.{name: svc, project: team, domain: prod-x}and{name: svc, project: team-prod, domain: x}both producedsvc-team-prod-x.A third cause not pointed out:
The old implementation lowercased the whole joined string (project/domain/appName) during hash and returned it directly when it fit in 63 chars, with no digest at all. So apps named
Fooandfooin one project/domain will always collide — the name field permits both capitalized and non-capitalized lettersA-Z(flyteidl2/app/app_definition.proto:33).Summary
Injective name derivation
KServiceNamenow returns:k-prefix versions the format and guarantees the name starts with a letter, as Knative requires (DNS-1035).App Identity
Identity is now read from labels, not a joined annotation.
flyte.org/app-idstored{project}/{domain}/{name}as a/-joined string and was parsed back withSplitN— the same ambiguity as the name, for the same reason.identifierFromKServicefunction readsflyte.org/project,flyte.org/domainandflyte.org/app-name, which already existed as separate labels, so no value can be mistaken for a delimiter.flyte.org/app-idwill be deleted afterwards.kServiceToStatustook the app ID by re-parsing the annotation; it now receives the*Identifierits caller already resolved, so the KService name and the URL reported for it are computed from one identity rather than two independently parsed ones.The public URL shape changes as a side effect, from
{name}-{project}-{domain}.{base_domain}tok-{name}-{digest}.{base_domain}. This stays correct without further change because Knative'sdomain-templateis{{.Name}}.{{.Domain}}, so the route follows the KService name by construction.Open question —
INTERNAL_APP_ENDPOINT_PATTERNThis is why the PR is a draft.
INTERNAL_APP_ENDPOINT_PATTERN(added in #7323) is injected into every app pod as a template with one substitution slot:The SDK's
AppEndpoint("some-app")string-replaces{app_fqdn}with the target app's name. That worked because everything except the name was constant across a project/domain and could be baked in at deploy time.Any injective scheme puts a digest of the full identity in the name, and that digest varies with the target's name — so there is no constant suffix left to bake in, and the SDK cannot compute one. Every URL built from the pattern now resolves to nothing.
The injection site is marked with a
TODOin this PR rather than silently left broken; it must be resolved before merge.Three options, discussed in more detail on #7622:
(org, project, domain), name stays a literal segment. The digest is then constant per project/domain and the existing template keeps working with no SDK or proto change. Still injective: name is capped at 30 chars (app_definition.proto:30) against a 34-char budget so it never truncates, and the fixed-width digest makes right-parsing unambiguous. Requires app names to be unique case-insensitively.internal_urltoIngress, haveAppEndpointread it viaGetApp, retire the env var. Structurally cleanest, puts no constraint on names, and enables cross-project discovery — but needs a proto change plus a coordinated SDK rollout.Guidance on which direction to take is welcome.
Related PRs
INTERNAL_APP_ENDPOINT_PATTERN, which this change breaks.