design-proposal: application endpoints and EndpointAttachment - #45

Actionable comments posted: 3

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@design-proposals/endpoint-attachments/README.md`:
- Around line 78-80: Update the application identity used by authorization to
include the API group alongside kind and name, and ensure the resolver compares
all three fields. Apply this consistently to the applicationRef examples and
related authorization flow, or explicitly define and validate a single allowed
group; prefer binding to the application UID to prevent delete/recreate reuse.
- Around line 86-94: Resolve the unattainable Attached phase value in the status
definition by either documenting the lifecycle transition and finalizer behavior
that emits Detached, or removing Detached from the phase enum and its Pending |
Attached | Detached comment. Keep endpoint-loss behavior represented through
Resolved=False and resource deletion semantics consistent.
- Around line 104-105: Update the Render design to make the attachment UID in
the controller owner reference the authoritative identity for the generated
LoadBalancer Service. Use the ownership label only for indexing or diagnostics,
and require reconciliation to verify the Service’s controller owner reference
points to the attachment before adopting or updating it; otherwise ignore it and
create or rediscover the correctly owned Service.

Hi Timofei Larkin (@lllamnyp)!
I've read your design and find some issues,

Main blocker for now - this design not mention accepted database exposure design that may contradicts with yours at some places, take a look please.

Non-blocking follow-ups

1. Mutation semantics of spec.loadBalancer are undefined (line 78-83). spec.applicationRef is immutable via CEL and className/claimName are mutually exclusive, but nothing states what happens on className → a different class, claimName → a different claim, or a switch between the two members. Each of those silently changes the address a client may have allow-listed — the exact failure named in What exists today ("Turning external off and on may change the address a customer has allow-listed"). Either declare spec.loadBalancer immutable alongside applicationRef, or specify the re-mint and release semantics.

2. The API group choice is what buys the quota claim; say so. count/endpointattachments.cozystack.io (line 153) works because cozystack.io is CRD-served by the kube-apiserver (packages/system/cozystack-controller/definitions/cozystack.io_workloadmonitors.yaml), matching the existing count/certificates.cert-manager.io pattern documented at packages/core/platform/values.yaml:327. SecurityGroup — the resource this one is modelled beside — instead lives in sdn.cozystack.io, served by the aggregated cozystack-api (pkg/registry/sdn/securitygroup/rest.go, no CRD under definitions/), where the kube-apiserver's ResourceQuota admission never runs. A later move to sdn.cozystack.io for group consistency would silently drop stock quota support, so the coupling deserves a sentence.

3. Does any platform path delete and recreate an application's HelmRelease under the same name? Lifecycle and garbage collection hangs attachment GC on an ownerReference to the application's HelmRelease. Kubernetes GC matches owners by UID, so a same-name recreate orphans and collects every dependent — here, tearing down tenant exposure and releasing minted addresses. The platform already uses delete-and-let-the-chart-recreate for system releases (packages/core/platform/images/migrations/migrations/20, .../25:15-17); I could not find that pattern applied to a tenant application HelmRelease, so this is a question rather than a finding. If it is reachable, the design needs a finalizer or a name-based fallback.

4. spec.applicationRef should carry the group, per existing precedent. The precedent: SecurityGroup's ApplicationReference already resolves on all three of group, kind and name (pkg/registry/sdn/securitygroup/rest.go:67-69), and the proposal's own resolver text names only .kind and .name while the labels it reads include .group.

5. status.phase and status.conditions is duplicating info? how conditions list managed if it not have timestamps or some identification for list items?

Forgot to request changes in previous review message. Doing now

The document is thorough, and the two structural choices are right: mirroring the endpoint Service's selector and ports is what makes the mechanism engine-agnostic without per-chart render logic, and the lineage-label check is a real authorization seam rather than a stated intention. Alternatives considered is honest about why this is not ServiceExposure repeated, which was the obvious objection.

myasnikovdaniil's blocker is correct, and I want to sharpen it, because it is more than a missing cross-reference. Then a concrete requirement from our side that I think changes the scope.

1. The accepted design this collides with is community #20, and it is merged

design-proposals/external-database-exposure opens with the problem statement: "Today every managed database a tenant exposes externally gets its own LoadBalancer Service, and therefore its own public IP." Its goal is to collapse a tenant's databases — including several instances of the same engine — onto the single LoadBalancer IP of the tenant Gateway, routed by SNI.

EndpointAttachment renders one additive LoadBalancer Service per attachment, each drawing its own address. That is the pattern #20 was accepted to replace. Scope and related proposals lists #35, #29, #3218, #2922, unified-TLS and Gateway API — and not #20.

I do not think these conflict as designs: SNI consolidation for the common case, a dedicated address for the tenant whose client allow-lists an IP or whose driver cannot do SNI. But the document has to say so, because as written it reads as the general answer to external exposure, and the general answer was decided differently.

2. This proposal inherited an obligation from #29 and does not know it

#20's User-facing changes says its trigger is "an external-adjacent toggle to select passthrough/SNI mode (expected to become an expose entry once the structured-exposure model lands)". #20 was written expecting #29 to be its user-facing surface. This proposal declares itself #29's successor — so it is now that surface.

The practical consequence today: a tenant cannot request SNI exposure at all. #29 is closed, and the Gateway union member here is "future work, not designed here". The merged design has no trigger. That reframes the union member: it is not speculative, it is specified elsewhere and waiting.

3. The hostname non-goal points past the accepted answer

Non-goals sends hostnames, DNS and certificates to the unified-TLS effort. For databases, #20 already settled the SNI hostname scheme and the end-to-end TLS story. Deferring to unified-TLS is right for the PKI half and misses that the naming half for this exact case is decided.

4. A question about the mirroring contract

Failure and edge cases says a changed endpoint selector is "mirrored on the next reconcile". That is safe where an operator expresses instance role as a pod label and keeps the Service selector static — CloudNativePG's -rw works this way, so the flagship case is fine. It is not safe for an operator that rewrites the Service selector on failover: for the reconcile window, external write traffic would land on a demoted primary. Worth stating the assumption and which engines were checked against it.

5. VMs are a second first-class consumer, not future work

Non-goals defers vm-instance ("the existing externalMethod contract stays as-is; folding it into the union is future work"). We have a live requirement that does not fit that deferral: attaching one or more public IPs to a VM.

That requirement is exactly this proposal's N-per-application shape, and more of the machinery already fits than the document assumes. On a live cluster, a vm-instance Service already carries everything the resolve step needs:

labels:
  apps.cozystack.io/application.group: apps.cozystack.io
  apps.cozystack.io/application.kind: VMInstance
  apps.cozystack.io/application.name: whmcs
  apps.cozystack.io/user-service: "true"
  service.kubernetes.io/service-proxy-name: cozy-proxy

The lineage labels are stamped, and the selector is a normal pod selector that mirrors fine. Three things block it:

• Ports must be in the attachment spec. A VM's non-external Service is headless with a single sentinel port 65535, so there is nothing meaningful to mirror, and externalPorts has no counterpart. This answers your open question 1 — a ports filter is required, not optional.
• A whole-IP mode. The rendered Service needs the service.kubernetes.io/service-proxy-name: cozy-proxy label and the networking.cozystack.io/wholeIP / allowICMP annotations. The datapath already exists; only the render is missing.
• Egress with several addresses is a real design question. WholeIP today means both "all ports in" and "the VM egresses as this address". With two attachments that is ambiguous. Either one attachment is marked primary for egress, or egress stays on the application's own Service, or wholeIP is permitted on at most one attachment. This is the one part that is not a field.

Related, the deprecation sequence has an internal tension: step 3 removes external from chart schemas "with a validation error pointing at attachments", while Non-goals says vm-instance's contract stays as-is. For VMs there is currently nothing to point at.

6. A granularity mismatch worth settling before implementation

We are mapping a portal-side model onto this: a standalone PublicIP resource (a clean fit for #35's IPAddressClaim) attached through an instance's NIC list, where each NIC carries its own security groups. So one VM can have a management address with SSH open and a public address with only HTTPS.

Cozystack's SecurityGroup.spec.attachments is a list of ApplicationReference, and the controller stamps a membership label onto the pods of the referenced application (pkg/apis/sdn/v1alpha1/securitygroup_types.go). The granularity is per-application, so two addresses on the same VM necessarily share one rule set.

Per-attachment ports covers part of that in practice, but it is port filtering rather than source-IP ACL. Since Non-goals hands ACL to #2922 wholesale, it is worth saying explicitly whether per-address ACL is intended to become expressible, or whether consumers should expect application-level granularity permanently.

Points 1–3 are a few lines each. Point 5 is the one that changes scope, and I would rather raise it now than after the MVP ships database-only.

Separately: your five non-blocking points from 2026-08-05 have had no response and the PR has not moved in three and a half weeks. Points 1 (mutation semantics of spec.loadBalancer) and 3 (same-name HelmRelease recreate orphaning attachments by UID) look load-bearing to me, not nits.

Not adding a second CHANGES_REQUESTED on top of myasnikovdaniil's — one block is enough.

Requesting changes, four blockers. Five non-blocking points from my first review are addressed and the section about #20 is in, blocker 4 is what is left of my first blocker.

Blockers

1. A second delegated Service on a VM breaks the first one, in PortList mode too (README.md:122, also :12, :53, :216, :218)

Doc limits only whole-IP to one per application, and promises that a second PortList address works ("both addresses reachable on their ports", :216) and that legacy external: true Service stays untouched (:218). Datapath doesn't work like that. Cozystack ships cozy-proxy v0.4.0 (packages/system/cozy-proxy/charts/cozy-proxy/values.yaml:3), and EnsureRules in pkg/proxy/nft.go:394-455 deletes the existing pod_svc and svc_pod entries of the pod before adding the new pair. It is called from pkg/controllers/services_controller.go:237,335,365,425 for every delegated Service, wholeIP only affects port filter. Main has the same limit ("a pod IP can only stand for one service IP", nft.go:406-441), and your own implementation says it in docs/endpoint-attachments.md:69 of cozystack/cozystack#4324. I only read the code, did not run it on live datapath.

So second attachment reports Provisioned=True and the earlier address (attachment or chart Service) stops working. IPv6 and Dual claims with method are same story, cozy-proxy uses ipv4 table and To4() (nft.go:57,397), nothing will serve that address.

Please restate the limit as one cozy-proxy delegated Service per backend pod in any method, chart Service included, and refuse second one and non ipv4 family with Provisioned=False. Rewrite :12, :53, :216, :218 to match. Several addresses per VM need datapath change, :230 puts it to future CNI but nothing tracks it in cozystack/cozy-proxy, please open issue there and gate VM modes on it wherever more than one address is promised.

2. Claims are minted under controller identity, so nobody checks the class, and minted address doesn't die with attachment (README.md:189, also :56, :118, :206, :214)

:189 says class selection and admission are #35 surface and this proposal "adds no new path to an address that the substrate does not already govern". #35 says opposite in Security, "Where this component stops": substrate doesn't decide which class a namespace may draw from, that belongs to a ValidatingAdmissionPolicy restricting classes or to "platform machinery that mints claims on a tenant's behalf". The controller from step 2 (:118) is that machinery. In cozystack/cozystack#4324 it is a reconciler inside cozystack-controller and the claim is created by its ServiceAccount, so any policy keyed on tenant never sees it. Also there tenant roles have endpointattachments but not ipaddressclaims (clusterroles.yaml:40,141,304, only controller has it, rbac.yaml:39), so attachment is the only way to mint, with any className.

Second part, :56 says minted address "dies with its attachment" and e2e at :214 expects "address released". #35 section 2 has reclaimPolicy default Retain, and after claim delete with Retain the IPAddress stays Released until admin clears claimRef. So with default class every create/delete cycle strands one address.

Please name the controller and identity that creates the claim, and say who authorizes className for the tenant (SubjectAccessReview on the class, or allow-list in controller with Provisioned=False otherwise). Then either mint only from Delete classes, or change :56, :206, :214 to class dependent reclaim and say how stranded address is recovered.

3. Public address on a database weakens the no-plaintext guarantee and doc doesn't say it (README.md:149, also :43, :186-192)

:43 says external doubles as TLS default and :149 says loadBalancer member is additive "TLS or not", Security has nothing about plaintext on public address. Today external: true makes platform refuse plaintext, attachment never writes external so those guards don't run. Redis: cozystack-database-external-tls-policy (packages/system/cozystack-basics/templates/database-external-tls-policy.yaml:4,39-42,73) denies external Redis without TLS, but matches only redises, and TLS is fixed at creation (:15-19), so existing plaintext Redis can't be made safe after attach. Postgres: tls.required unset follows external (packages/apps/postgres/templates/_tls.tpl:19-23), only then pg_hba gets hostssl plus reject (db.yaml:280-310), otherwise cnpg default rule accepts plaintext (db.yaml:281-283).

So attach to -ro of Postgres with external: false, main use case from :48, gives plaintext credentials and data to anyone on the path, weaker than external: true. You know about it, docs/endpoint-attachments.md:63 in cozystack/cozystack#4324 says "Attaching a database does not turn its TLS on" and "expect a follow-up that enforces 'no plaintext database on a public address'". That follow-up is not in this proposal.

Please write the hole in Security and make that follow-up a mandatory Rollout item with owner, landing before database endpoints can be attached, because shipping attachment first removes the guarantee external: true gives today. Where enforcement is not practical for some engine docs may be enough, but decide it per engine and write it down. Add per engine TLS check to Rollout 1 as release gate next to the mirroring contract check.

4. SNI default and trigger must be agreed with #20 (README.md:14, :144-148, also :65, :173, :226)

These two proposals are two halves of how tenant exposes a database and they disagree. :14, :144 and :146 call SNI consolidation the general answer and "the default", #20 keeps per-database LoadBalancer as default and makes SNI opt-in ("Upgrade and rollback compatibility"). :65 and :148 say no tenant-facing trigger exists, #20 section 5 defines interim external adjacent toggle and the expose model as long term trigger. And cozystack/cozystack#4324 says the trigger decision moved to TLS work, which contradicts :65, :148, :173, :226. Three documents, three answers for what turns SNI exposure on, so two teams can build two switches for the same thing.

Please agree one answer with the owner of #20, fix whichever text is wrong and record it in both proposals. Until then drop "the default" and "no trigger exists".

Follow-ups, not blocking

1. README.md:137: "no path that delete-and-recreates a tenant application's HelmRelease" is not true today. Restore of VMInstance under new name creates new HelmRelease and deletes the old one (internal/backupcontroller/velerostrategy_controller.go:1063-1130), migration 28 did the same for mysql to mariadb. I don't ask to change those flows and UID anchored GC can stay, but since they exist please say what happens with attachments and minted addresses on restore or rename (lost or carried over).

2. README.md:117 and :188 call the tenantresource marker the declaration of tenant-facing Services, but openbao-{{ .name }}-internal (packages/system/openbao-rd/cozyrds/openbao.yaml:37) and qdrant-{{ .name }}-headless (packages/system/qdrant-rd/cozyrds/qdrant.yaml:46) are in spec.services. Tenant can only publish own application, so risk is low, but Rollout 3 (:224) should check over-declaration too, not only under-declaration.

3. WholeIP with more than one backend: :122 and :204 refuse with Provisioned=False, cozystack/cozystack#4324 keeps Service and claim and selects nothing (MultipleBackends) so rolling update and live migration don't churn the address. Please align both. I could not verify from vm chart if both pods are selected during migration.

4. Doc builds on #35 and on address-controller packaging in cozystack/cozystack#3382, both open with changes requested (Gateway as association target, holder based reservation and confused deputy in #35, unpinned sources and images in cozystack/cozystack#3382). Group local.sdn.cozystack.io, claim annotation and loadBalancerClass contract come from #35, so please add Dependencies line to Status and say what changes here if #35 changes, gateway member first.