Repository navigation
design-proposal: application endpoints and EndpointAttachment - #45
Timofei Larkin (lllamnyp) wants to merge 7 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
📝 Walkthrough
🚥 Pre-merge checks | ✅ 5
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dd691aa7-3010-4ff4-b440-29580c3c0eca
📒 Files selected for processing (1)
design-proposals/endpoint-attachments/README.md
| applicationRef: # immutable; the application, in this namespace | ||
| kind: Postgres | ||
| name: mydb |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Include the API group in the application identity used for authorization.
applicationRef contains only kind and name, and the resolver compares only those two fields, while the proposal says lineage labels also include .group. Unless a single application group is hard-coded and validated, same-kind/name resources from another group could satisfy the authorization check. Add the group to the reference—or explicitly constrain it—and compare all identity fields; binding to the application UID would also prevent reuse after delete/recreate.
Also applies to: 102-103, 159-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/endpoint-attachments/README.md` around lines 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.
| status: | ||
| phase: Attached # Pending | Attached | Detached | ||
| serviceName: mydb-replicas-public-x7ktq # the rendered LoadBalancer Service (generateName) | ||
| addresses: | ||
| - "203.0.113.7" | ||
| conditions: | ||
| - type: Resolved # applicationRef + endpoint resolve to a live, lineage-labeled Service | ||
| - type: Provisioned # claim bound, LB Service has its address | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Define or remove the Detached phase.
No lifecycle path emits Detached: deletion removes the resource, while endpoint loss leaves it present with Resolved=False. Specify the transition/finalizer semantics or remove this unreachable status value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/endpoint-attachments/README.md` around lines 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.
| 3. **Render.** Create one additive `type: LoadBalancer` Service via `generateName` (prefixed with the attachment's name), owned by the attachment and found again through an ownership label — never by name, so an attachment name can never collide with an existing Service. The Service **copies the selector and ports of the resolved endpoint Service** and stays in sync with them; this is what makes the mechanism engine-agnostic, and it is the structural answer to why #29's chart-side mechanism could not work: the controller does not need to know how CloudNativePG or mariadb-operator label their pods, it only needs to mirror the Service they already maintain. The Service carries the substrate's consumption annotation `local.sdn.cozystack.io/ip-address-claim: <claim>`; the per-class driver does the rest (pin, announce, associate). `externalTrafficPolicy: Local` and node-port allocation follow the platform's existing conventions. | ||
| 4. **Report.** `status.addresses` mirrors the claim's bound addresses; `status.serviceName` names the rendered Service; conditions say why anything is missing. Nothing is fabricated client-side. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the owner reference as the rendered-Service identity.
The design says the Service is owned by the attachment but rediscovered through an ownership label. Labels are mutable and can collide, so a stale or tenant-created Service could be selected, while label removal could cause duplicate LoadBalancer Services. Reconcile only a Service whose controller owner reference points to the attachment UID; use the label only as an index or diagnostic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/endpoint-attachments/README.md` around lines 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.
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
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
-
Mutation semantics of
spec.loadBalancerare undefined (line 78-83).spec.applicationRefis immutable via CEL andclassName/claimNameare mutually exclusive, but nothing states what happens onclassName→ 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 ("Turningexternaloff and on may change the address a customer has allow-listed"). Either declarespec.loadBalancerimmutable alongsideapplicationRef, or specify the re-mint and release semantics. -
The API group choice is what buys the quota claim; say so.
count/endpointattachments.cozystack.io(line 153) works becausecozystack.iois CRD-served by the kube-apiserver (packages/system/cozystack-controller/definitions/cozystack.io_workloadmonitors.yaml), matching the existingcount/certificates.cert-manager.iopattern documented atpackages/core/platform/values.yaml:327.SecurityGroup— the resource this one is modelled beside — instead lives insdn.cozystack.io, served by the aggregatedcozystack-api(pkg/registry/sdn/securitygroup/rest.go, no CRD underdefinitions/), where the kube-apiserver's ResourceQuota admission never runs. A later move tosdn.cozystack.iofor group consistency would silently drop stock quota support, so the coupling deserves a sentence. -
Does any platform path delete and recreate an application's HelmRelease under the same name? Lifecycle and garbage collection hangs attachment GC on an
ownerReferenceto 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. -
spec.applicationRefshould carry the group, per existing precedent. The precedent:SecurityGroup'sApplicationReferencealready 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.kindand.namewhile the labels it reads include.group. -
status.phaseandstatus.conditionsis duplicating info? how conditions list managed if it not have timestamps or some identification for list items?
| - type: Provisioned # claim bound, LB Service has its address | ||
| ``` | ||
|
|
||
| `spec.applicationRef` is immutable (CEL); retargeting is delete-and-recreate. `spec.endpoint.serviceName` is required — a struct rather than a bare string so that a future named-endpoint vocabulary (`endpoint: {name: ro}`) can join it as an alternative member. Within `loadBalancer`, `className` and `claimName` are mutually exclusive (CEL); both absent means "mint from the default class". An optional `family` (`IPv4|IPv6|Dual`, defaulting per the substrate) passes through to a minted claim. |
There was a problem hiding this comment.
You are identifying spec.endpoint.serviceName here in comment as struct but example shows plain string.
|
|
||
| ## Open questions | ||
|
|
||
| 1. **Port subsetting.** An attachment currently mirrors all ports of the endpoint Service. Is a `ports` filter (publish 5432 but not metrics) worth having in v1, or does it invite divergence from the endpoint definition? |
There was a problem hiding this comment.
I think it is charts part to address (do not expose mixed services that serve data+ui or data+metrics), not this endpoint attachments responsibility.
|
|
||
| 1. **Port subsetting.** An attachment currently mirrors all ports of the endpoint Service. Is a `ports` filter (publish 5432 but not metrics) worth having in v1, or does it invite divergence from the endpoint definition? | ||
| 2. **Family/dual-stack surface.** Pass `family` through to minted claims only, or also validate it against a referenced claim's family at resolve time? | ||
| 3. **Attachment identity in the dashboard.** Attachments are the user-visible record of "how is my app reachable" — does the dashboard list them per application via a label, and should the controller stamp `apps.cozystack.io/application.*` lineage labels on attachments themselves for symmetry? |
There was a problem hiding this comment.
Looks like this endpoint attachment resources is not application - it can't be created by itself without other app to attach. So it is first option or maybe define new category of resources, something like "applicationaddons"
|
|
||
| `external: true` keeps working unchanged, and its sunset is explicitly gated on an address-preserving migration path. | ||
|
|
||
| ## Scope and related proposals |
There was a problem hiding this comment.
You did not mentioned already accepted proposal for external exposure of databases. How do you think both designs will coexist? Accepted design address #29 as owner of exposure mechanics, and you superseded it, so now you need to resolve claims made to it. Also I see problem in idea: you making app-agnostic additive exposure, accepted design making app based exposure - certificates for tls sni based routing and encryption must be emitted by app itself, so it is not additive.
|
|
||
| One controller, engine-agnostic, reconciling four things per attachment: | ||
|
|
||
| 1. **Resolve.** Look up `spec.endpoint.serviceName` in the attachment's namespace and require that the Service carries lineage labels matching `spec.applicationRef` (`apps.cozystack.io/application.kind` and `.name`, stamped today by the lineage webhook from `ApplicationDefinition.spec.services`). This is the authorization seam: a tenant can attach only to Services the platform has already marked as tenant-facing endpoints of that application, never to arbitrary or foreign Services. Failure → `Resolved=False` with a reason; the attachment waits. Validation is by condition, not admission, because the Service set is dynamic (a mariadb `-secondary` Service exists only while `replicas > 1`) — eventual consistency, not admission-time races. |
There was a problem hiding this comment.
internal/lineagecontrollerwebhook/webhook.go:159-174 stamps apps.cozystack.io/application.{group,kind,name} on every object resolving to a managed HelmRelease. ApplicationDefinition.spec.services feeds a different label at lines 180-189 — internal.cozystack.io/tenantresource — and that is the tenant-facing marker (default-deny, matcher.go:65-73). Since the allowlists are strict subsets (kafka declares exactly one Service name, clickhouse one), the specified check would admit operator-internal and headless Services to a public address. Fix is a one-word change: gate on internal.cozystack.io/tenantresource: "true".
|
|
||
| ## Security | ||
|
|
||
| - **The lineage-label check is the authorization boundary for what may be exposed.** A tenant can attach only to Services in their own namespace that the platform's lineage webhook has labeled as tenant-facing endpoints of the named application. Attaching to unlabeled, system, or foreign Services fails `Resolved`. All references (`applicationRef`, `serviceName`, `claimName`) are same-namespace by construction. |
There was a problem hiding this comment.
Here is the correct wording for the L102 labels problem
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
Forgot to request changes in previous review message. Doing now
Align the document with what is built and argue the group choice on the record rather than leaving it in a commit message of another repository. - API group: ipam.cozystack.io -> local.sdn.cozystack.io throughout, including the association annotation, the default-class annotation and the example provisioner name, which now match what the controllers actually ship. Positioning carries the argument: Cozystack networking is already sdn.cozystack.io (SecurityGroup, served by the Cozystack API), cozyplane serves its own SecurityGroup there plus local.sdn.cozystack.io for its CRD-served kinds, and addresses are the third networking feature to want a home. One sdn family, with the local. prefix marking CRD-served kinds and the bare group the aggregated ones; ipam.cozystack.io would add a fourth name to a subject area that should have one. Stated plainly as a proposal, not a precedent: nothing serves local.sdn.cozystack.io today. - New Implementation status section: the core controller and the MetalLB reference driver exist, and cozyplane consumes reservations through the contract with no module import, no CRD dependency and no informer on the claim kinds -- the association seam is exercised by a second consumer, while Rollout phase 2, a second provisioner, remains outstanding. The ValidatingAdmissionPolicy is recorded as implemented nowhere and still part of phase 1. - The stub banner no longer says the proposal predates any code, and Rollout phases 1 and 2 say what exists. - Scope records that community #45 depends on this proposal, which does not wait on it. - Directory renamed public-ip-claims -> ip-address-claims; the kinds lost the "public" prefix two revisions ago. Assisted-By: Claude Opus 5 Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
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-proxyThe 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, andexternalPortshas no counterpart. This answers your open question 1 — aportsfilter is required, not optional. - A whole-IP mode. The rendered Service needs the
service.kubernetes.io/service-proxy-name: cozy-proxylabel and thenetworking.cozystack.io/wholeIP/allowICMPannotations. The datapath already exists; only the render is missing. - Egress with several addresses is a real design question.
WholeIPtoday 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, orwholeIPis 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.
Review approved the shape and left four specifics. Three change the document. - spec.addressName appeared once, commented out in the claim example, with no prose anywhere. Two reviewers read it opposite ways -- one as absent entirely, one as an ungoverned way for a tenant to name a cluster-scoped object. It is now specified: it narrows the candidate set and never widens it, honoured only for an address that is Available, unclaimed, not being deleted, and of the claim's own resolved class and family, with a name that matches nothing leaving the claim Pending rather than failing. Failure cases cover the absent, Bound, Released and wrong-class names. Security now states where this component stops. It secures the surface a reservation can be stolen through; it does not decide which class a namespace may draw from, and neither addressName nor className can be gated by namespace RBAC. That is deliberate: the capability is reserve/hold/attach/release, and encoding a tenancy model this controller cannot see belongs above it -- an admission policy over those two fields, or machinery that mints claims on a tenant's behalf. Both fields are named so a deployment hardening this does not have to find them. - ExposureClass was described as a kind that exists in network.cozystack.io. It was proposed alongside community #29 and never shipped, and that group does not exist; the direction went to loadBalancerClass. Open question 3 was anchored to its fate and so could not be answered. It now asks the live question -- whether a class should declare the loadBalancerClass it corresponds to, so an association can check the pair -- and records that loadBalancerClass is immutable on an existing Service, so a mismatch has to be a refused association rather than a rewrite. - The non-goal said a workload holds one claim, which forbids a case that is not ours to forbid and that community #45 depends on: a managed service reachable on a public address and an internal routable one is two Services with two claims. The 1:1 rule that is real -- one claim, one consumer at a time -- is kept, and what stays deferred is N owned addresses on a single consumer. Also records, in Positioning, why the group is not ipam.cozystack.io: this is not all of IPAM and cannot become it, since allocation stays with the allocator. Assisted-By: Claude Opus 5 Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Add the exposure half to the IP-addresses-as-a-resource substrate (#35): a tenant-created, namespaced EndpointAttachment that attaches an external address to one endpoint (tenant-facing Service) of a managed application. Attachments are additive and engine-agnostic (the controller mirrors the endpoint Service's selector and ports), mint or reference IPAddressClaims, and are garbage-collected with the application. Succeeds the closed structured-external-exposure proposal (#29); external: true coexists until an address-preserving migration exists. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
…#20; add VM modes Resolve the review feedback on community#45 and finalize the proposal: - applicationRef gains group (default apps.cozystack.io); the resolver compares all three lineage labels AND requires the tenant-facing marker internal.cozystack.io/tenantresource=true — lineage labels alone cover operator-internal Services too. - Address-source fields (className/claimName/family) are immutable via CEL; render knobs stay mutable. Phase enum defined (Detached is the endpoint-absent state); conditions are standard metav1.Condition, phase a derived summary. - The rendered Service's identity is the controller ownerReference; the ownership label is index/diagnostic only. The controller sets loadBalancerClass from the claim's class at creation, per #35 §5. - GC identity is UID-anchored by design: attachments belong to one application incarnation; same-name recreate collects them. - New section positioning attachments beside the accepted SNI consolidation design (#20): dedicated addresses are the exception, the gateway union member is #20's inherited tenant-facing trigger. - VMs are a second first-class consumer: ports filter, method WholeIP|PortList rendering the existing cozy-proxy contract, with an at-most-one-whole-IP rule; the multi-address egress model is the one open question. - Quota/group coupling, per-address ACL granularity, and the mirroring contract (static selector, pod-label roles) stated explicitly. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Whole-IP is defined as 1:1 NAT between one external address and one workload interface: it presumes a single-backend endpoint (refused otherwise), and the default is at most one whole-IP Service per application — which is the current datapath's actual shape, since cozy-proxy translates through strictly one-to-one stateless address maps where a second mapping for a workload replaces the first. Widening beyond one whole-IP is left open for a datapath with stateful NAT and symmetric flows (cozyplane the expected first candidate): with conntrack, replies leave through the arrival address for free and only unsolicited egress needs a designated primary mapping, so the remaining open question narrows to how that primary is designated. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
6ae684d to
ab8ea4c
Compare
…al.sdn.cozystack.io Move the kind from cozystack.io into #35's networking family. The local. prefix marks CRD-served kinds served by the kube-apiserver, where ResourceQuota admission runs, so count/endpointattachments quota keeps working; the bare aggregated sdn.cozystack.io (SecurityGroup's group) would silently drop it. Encoding the CRD-served requirement in the group name protects it from a later group tidy-up, and the same real storage is what incarnation-anchored ownerReference GC depends on. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
… local.sdn family Community #45 now homes its EndpointAttachment kind in local.sdn.cozystack.io rather than consuming the substrate from another group. Record it as another CRD-served resident of the family, which exercises the local.-prefix quota guarantee the Positioning section argues for. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
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
- 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.
- 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.
- 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.
- 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
-
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). -
README.md:117and :188 call thetenantresourcemarker the declaration of tenant-facing Services, butopenbao-{{ .name }}-internal(packages/system/openbao-rd/cozyrds/openbao.yaml:37) andqdrant-{{ .name }}-headless(packages/system/qdrant-rd/cozyrds/qdrant.yaml:46) are inspec.services. Tenant can only publish own application, so risk is low, but Rollout 3 (:224) should check over-declaration too, not only under-declaration. -
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. -
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 andloadBalancerClasscontract come from #35, so please add Dependencies line to Status and say what changes here if #35 changes,gatewaymember first.
| 3. **Render.** Create one additive `type: LoadBalancer` Service via `generateName` (prefixed with the attachment's name) — so an attachment name can never collide with an existing Service. **The controller owner reference is the Service's identity:** the controller sets itself as controller owner and rediscovers the Service by that reference on every reconcile; an ownership label is stamped for indexing and diagnostics but is never trusted for adoption, because labels are mutable and can collide — a Service whose controller owner reference does not point at the attachment's UID is ignored, never adopted or updated. The Service **mirrors the resolved endpoint Service**: the selector is always the endpoint's, kept in sync by watching the endpoint Service (event-driven, not polled); ports come from the member's `ports` when set (a subset filter, and the only source when the endpoint has nothing meaningful to mirror), otherwise from the endpoint. Mirroring is what makes the mechanism engine-agnostic, and it is the structural answer to why #29's chart-side mechanism could not work: the controller does not need to know how CloudNativePG or mariadb-operator label their pods, it only needs to mirror the Service they already maintain. **The mirroring contract assumes the operator expresses instance role as pod labels under a static Service selector** — CloudNativePG does (the `-rw` selector is static; the operator relabels pods on switchover), so the flagship case holds. An operator that instead rewrites the *Service selector* on failover leaves the mirror stale for one watch-driven reconcile, during which external write traffic can land on a demoted primary; *Rollout* therefore gates each engine's documentation on verifying its selector model. The Service carries the substrate's consumption annotation `local.sdn.cozystack.io/ip-address-claim: <claim>`, and the controller sets `spec.loadBalancerClass` at creation to the value the claim's resolved `IPAddressClass` fixes — per #35 §5 that check is refusal-only and the field is immutable, so the creator must be right at creation, and this controller can be, because it reads the class first. The per-class driver does the rest (pin, announce, associate). `externalTrafficPolicy: Local` and node-port allocation follow the platform's existing conventions. | ||
| 4. **Report.** `status.addresses` mirrors the claim's bound addresses; `status.serviceName` names the rendered Service; conditions say why anything is missing. Nothing is fabricated client-side. The controller also stamps the application's lineage labels (`apps.cozystack.io/application.*`) onto the attachment itself, so dashboards and tooling list an application's attachments with the same selector that finds its other objects — an attachment is an application add-on, not an application, and its presentation is the dashboard's call. | ||
|
|
||
| **VM datapath modes.** A vm-instance's tenant-facing Service is headless with a single sentinel port (`65535`) until exposed, so there is nothing to mirror: a VM attachment must set `ports` or `method` in the `loadBalancer` member (neither → `Provisioned=False` with a reason). `method: WholeIP` renders the existing cozy-proxy contract — the `service.kubernetes.io/service-proxy-name: cozy-proxy` label, the `networking.cozystack.io/wholeIP: "true"` annotation, and the sentinel port when `ports` is absent. `method: PortList` renders the same label with `wholeIP: "false"`, `networking.cozystack.io/allowICMP` from `allowICMP`, and requires `ports`. These are byte-for-byte the objects vm-instance's chart renders under `externalMethod` today — the datapath already exists; only this render was missing, and the enum deliberately reuses the chart's `externalMethod` vocabulary. **Whole-IP is 1:1 NAT, so it presumes a single backend**: the mapping is between one external address and one workload interface address, so an endpoint resolving to more than one backend has no interface to map — its external address is simply a load-balancer address, and `WholeIP` on such an endpoint is refused (`Provisioned=False`), not half-applied. **Default: at most one whole-IP Service per application**, counting a legacy `externalMethod: WholeIP` chart Service. This states the datapath's actual shape, not a caution: cozy-proxy translates statelessly — per-packet source/destination rewrites through strictly one-to-one address maps — and a second mapping for the same workload replaces the first in both directions, so a second whole-IP address today would not be ambiguous but dead. The controller refuses it with `Provisioned=False` (cross-object state, so a condition, not admission). Widening the default is left open for a more capable datapath — see *Open questions*. |
There was a problem hiding this comment.
Same as blocker 1, cozy-proxy keeps one mapping per backend pod in any method (v0.4.0 nft.go:394-455), so second PortList address from :216 can't work.
|
|
||
| The accepted general answer to external *database* exposure is #20: any number of a tenant's fitting databases — same-engine instances included — collapse onto the tenant Gateway's single LoadBalancer address, SNI-routed on native ports, with end-to-end TLS from the operator-issued certificate. This proposal is not that answer's replacement, and must not be read as the general path for databases. The division of labor: | ||
|
|
||
| - **SNI consolidation is the default for the engines it fits** (Redis, direct-TLS PostgreSQL 17+, sharded `mongos`): one address per tenant, a stable hostname per database, zero per-database address cost. |
There was a problem hiding this comment.
#20 keeps per-database LoadBalancer as default and SNI opt-in, so "the default" here contradicts it. Blocker 4.
| - **SNI consolidation is the default for the engines it fits** (Redis, direct-TLS PostgreSQL 17+, sharded `mongos`): one address per tenant, a stable hostname per database, zero per-database address cost. | ||
| - **A dedicated address — an attachment — is the deliberate exception**: the client allow-lists an IP; the client cannot emit SNI (pre-17 Postgres clients, `redis-cli` without `--sni`); the engine's wire protocol precludes SNI entirely (MariaDB, server-speaks-first — permanently outside #20); the endpoint is not a database at all (a VM, the managed-Kubernetes API); or the tenant needs an address they can hold and move (#35's reservation semantics, which a shared Gateway address by definition cannot give one database). | ||
| - **The two meet at the trigger, and the obligation is inherited.** #20 left its tenant-facing trigger to the structured-exposure model it expected from #29; this proposal succeeded #29, so the trigger surface lands here: the `gateway` union member *is* it — an attachment whose member selects the tenant Gateway renders a per-release `TLSRoute` onto #20's shared per-engine listener (mechanics in #20 §5, not re-designed here) instead of a LoadBalancer Service. Wiring it is a phase-2 *Rollout* item. Until it ships, a tenant cannot request SNI exposure at all — this sequencing accepts that gap knowingly rather than leaving the merged design's trigger unowned. | ||
| - **The `gateway` member cannot be purely additive, and this is expected.** SNI exposure requires the operator-issued certificate to carry the external hostname SANs (#20 §6) — chart/operator cooperation, gated per engine on the unified-TLS convergence #20 already depends on. The `loadBalancer` member carries no such dependency: it is additive against any passthrough-clean engine, TLS or not. One union, two members, two honesty levels — the additive one ships first. |
There was a problem hiding this comment.
Attachment never writes external, so Redis admission policy and Postgres tls.required default don't fire. Blocker 3.
| ## Security | ||
|
|
||
| - **The tenant-facing-endpoint check is the authorization boundary for what may be exposed.** A tenant can attach only to Services in their own namespace whose lineage identity labels match `spec.applicationRef` on group, kind, *and* name, and which carry `internal.cozystack.io/tenantresource: "true"`. The marker is the load-bearing half: lineage labels alone also cover operator-internal and headless Services, and admitting those to a public address would leak surfaces the platform never declared tenant-facing. Attaching to unlabeled, internal, system, or foreign Services fails `Resolved`. All references (`applicationRef`, `serviceName`, `claimName`) are same-namespace by construction. | ||
| - **Who may expose to which pool** is the substrate's question: `IPAddressClass` selection, claim quota, and the admission posture against raw pin annotations are #35's surface, consumed here unchanged. This proposal adds no new path to an address that the substrate does not already govern. |
There was a problem hiding this comment.
#35 leaves class authorization to machinery that mints claims on tenant's behalf, that is this controller, so "no new path" is not true. Blocker 2.
Generalize EndpointAttachment from an address-only kind to a two-member mechanism union: loadBalancer (a dedicated address, as before) and a new gateway member that binds an endpoint to a hostname on a shared Gateway, rendering HTTPRoute or passthrough TLSRoute. - The gateway member is the tenant-facing trigger community #20 (accepted SNI consolidation) left to #29; #45 succeeds #29, so #45 now owns the route resource, authorization and lifecycle while #20 keeps the database passthrough data plane and the TLS/SAN story. Generalizes past databases to any route, the managed-Kubernetes API included. Largely resolves #20's ownerless-trigger gap. - Hostnames are NOT a first-class resource: allocation asymmetry — the platform allocates addresses, registrars allocate names, so there is nothing to reserve, quota, hold or move. Conflicts resolve via Gateway API (oldest wins) within a per-tenant Gateway, and DNS across tenants. - Legacy support, bounded: render the gateway member as a passthrough Ingress on the tenant-root controller where that is the live data plane, and report chart-rendered exposures read-only beside attachments. - Per-tenant Gateway is the default and does the cross-tenant isolation; the platform-Gateway-with-per-tenant-listeners variant is left open (private clouds), its hostname-under-apex admission guard sketched only. - Passthrough first; TLS termination and cert issuance stay with the unified-TLS effort. Route-vs-address fit needs a protocol-aware endpoint vocabulary, flagged as future work. Adds a Routes section, route render path, route security and edge cases, a gateway e2e, a dedicated rollout phase, and open questions; reframes the SNI-consolidation relationship and the rejected alternatives accordingly. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
The problems with the external toggle are real, but I don't think they need a new API and a controller. Service, Gateway, ListenerSet and *Route are standard building blocks that anyone who knows Kubernetes can debug. A Cozystack layer on top of them has to justify itself, and I don't see that in the proposal.
This proposal goes against a design we already accepted. #20 gives the route and its trigger to the application chart: "a release's route is rendered by that release's own chart". The trigger is "a chart value on the database beside external". So there is no ownerless trigger left for this proposal to fill. #20 also records decision 0001: external exposure is the native primitive "with no Cozystack object in front of it". EndpointAttachment is exactly such an object.
Gateway API already splits the work the way we need. The platform owns Gateways, listener sets, the LB, the CNI and the addresses from #35 claims. The application owns its Routes. On Cilium 1.20.2, which we ship, that means HTTPRoute for HTTP, TLSRoute for SNI and TCPRoute for raw TCP (MariaDB, Postgres before 17). When a client needs a dedicated IP, the platform puts it on the Gateway or listener. VM WholeIP is the only exception. 1:1 NAT is not routing, so it stays in the CNI layer.
I don't see why attachments need their own lifecycle. A held address already survives detach through its #35 claim. Several addresses per application is a list in values. The proposal counts "every attach/detach is a Helm upgrade" as a cost but doesn't name any harm. Five app charts have upgrade hooks. The vm-instance and vm-disk hooks render only when the instance type, Service type or disk size changes. The postgres, kubernetes and kafka hooks already run on every values change today. Each checksum/* annotation is computed from one template, not from .Values, so changing a Route rolls no pods. Which chart breaks, and how?
The route member needs chart-side knowledge too, which breaks the "no per-chart render logic" goal. It needs a per-port protocol vocabulary, and Future work has the chart template it. A chart that declares its protocols can render the Route itself. For comparison, packages/apps/harbor/templates/httproute.yaml is 30 lines. I've upstreamed Route templates for several charts and it's routine work. That is much cheaper than an API and a controller we have to maintain forever.
The legacy rendering ties the new API to a retired ingress controller. ingress-nginx stopped getting updates in March 2026 and its repository is archived. Some deployments also must run without it. That path should move to Gateway API, not into a new API.
The authorization check relies on a label that was made for visibility. tenantresource means "the tenant can see this". Here it would also mean "the tenant may publish this to the internet". The Kubernetes application selects services by cluster.x-k8s.io/cluster-name, so anything with that label becomes publishable. Is that intended?
What if the default were one platform Gateway with a listener set per tenant? Cilium 1.20.2 supports ListenerSet. Then hostname isolation is admission on Routes, and the route member is just a Route plus a ValidatingAdmissionPolicy.
Every layer that wraps Routes has to learn each new data plane. I maintain a Gateway API controller that publishes routes through an outbound tunnel instead of an LB address, and it's used outside Cozystack. A plain Route works with it unchanged, as with any conformant Gateway. A wrapper would need a new mode for it.
I'd drop the new kind. Gateways, listeners and addresses stay with the platform and #35. Routes go in the charts, as #20 already says.
…ing; drop path The gateway member binds an entry point — a whole hostname, SNI, or port — to one backend, and expresses no paths, methods, or header matches. A path is L7 routing topology, not a binding: handling it means prefix matching and usually stripping, and a stripped prefix leaks the moment the app emits a redirect or absolute URL from what it believes is its root — unfixable from the binding layer. Paths are therefore application topology, owned by the app's own HTTPRoute. Governing principle now stated explicitly: the platform owns the external binding at every layer; the application owns its intrinsic routing topology, which only L7 has. L4 (TCP/TLS) has no topology and collapses to binding; L7 keeps its path/method/header topology in the app's HTTPRoute while the hostname and listener under it stay binding. - Drop the path field; protocol is TLSPassthrough | TCP | HTTP (the binding kind, not a route table). - Render modes: L4 passthrough TLSRoute/TCPRoute; HTTP a catch-all HTTPRoute binding the whole hostname, or a provisioned listener an app/tenant HTTPRoute attaches to (base-path-aware). Host-based exposure is the default. - Add the non-goal and the rejected-alternative entries; ripple through overview, goals, render, conflict and edge cases. The gatewayRef->className (granted exposure-class vocabulary) change is held pending the class-kind name decision and is not in this pass. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
Timofei Larkin (@lllamnyp) Aleksei Sviridkin (@lexfrei) I read the last commit (gateway member does no L7 routing) and I like the principle there, platform owns the binding and app owns the topology. I think the text can go one step further: if binding is platform job on every layer, tenant should not choose the mechanism either. Today the union asks tenant to pick spec:
applicationRef: {kind: Postgres, name: mydb}
endpoint: {serviceName: postgres-mydb-ro}
protocol: TLSPassthrough # TLSPassthrough | TCP | HTTP
hostname: mydb-ro.tenant-a.example.org
port: 5432
address: # omit to use the shared tenant Gateway address
claimName: held-ip # or classNameRendering by case:
One more change: every attachment provisions own Tenants have no RBAC on Aleksei Sviridkin (@lexfrei) this does not add second data plane, controller writes upstream objects and only You asked if this just brings Ingress back. From tenant side it looks similar, one small object, name to service. But there are no rules in it, no paths, no annotations, no knobs of one implementation, and the last commit already says a path field would reintroduce Ingress. The mechanics are different, listener, route, certificate and address are rendered by controller and tenant never sees them, and status names the rendered objects ( On decision 0001 from #20, what this object does beyond picking a class: tenant can publish without RBAC on Gateway API objects, endpoint and hostname are checked before anything is rendered, it has own lifecycle and status ( Timofei Larkin (@lllamnyp) you give up Timofei Larkin (@lllamnyp) the last commit holds Before we pick this needs a test with shipped Cilium image (it carries operator patch): |
…; gateway synthesizes TLSRoute/TCPRoute Reframe the gateway member around what the app ships. The attachment binds one of two app-shipped artifacts — a Service (the L4, sufficient contract, first-class now) or the app's own HTTPRoute (the L7 case, carrying path topology). TLSRoute and TCPRoute are never bind targets: the controller synthesizes them from a Service, because an app gains nothing by shipping a route whose only inputs are the backend and an SNI hostname. The synthesized passthrough TLSRoute is #20's per-release data plane. - L7 multi-route consistency: an app that must know its hostname declares it in its spec, templating a consistent HTTPRoute-set; the attachment binds the set, not individual routes, constrained to the hostnames they declare, so it can never split frontend and backend across hosts. - Discovery by lineage label extends to route objects (webhook labels them; it never creates or places routes). Listeners/Gateways/GatewayClasses stay admin-owned outside the app. Binding leans on the experimental Gateway-API listener-selects-route capability. Flux/Argo SSA per-field ownership makes controller-written labels safe under GitOps. - endpoint gains routeName beside serviceName (gateway L7 case). - Non-goal: gateway-routing VMs and tenant-ordered LoadBalancers (VMs stay on the loadBalancer / attach-public-IP path; kubevirt territory). - ClickHouse cited as the worked case for discovery-by-label over a chart exposure field (operator-owned ClusterIP Service, no external key, no route). Open questions added, flagged not resolved: dedicated controller vs chart helpers (lexfrei's objection recorded, no alternative on the table yet); the experimental route->listener reference direction; routes-in-charts up front vs gradually; and the gatewayRef->exposure-class rename, still held on the class name. Carries the 10-18 drop-path edits (no L7 routing, no path). Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
Hi! We removed Sorry for the extra work, I wanted to mention this now so it doesn't come up late in review. |
|
I went through the current text (2d3e481) again against the code it relies on, and I'm keeping my request for changes. Items 1-5 are what I'd want settled before approval. Items 6-9 are text fixes that can go in the same push. I haven't run any of this: where I say "reading", I read the code, and the cozy-proxy points come from v0.4.0 (the version pinned in the chart), not from a live datapath. 1. Who may draw from which address class, and what happens to a minted addressREADME L239 leaves "who may expose to which pool" to the substrate. #35 in turn says nothing in it decides which class a namespace may draw from, and leaves that to "a In the implementation branch (cozystack#4324), Reclaim has a similar gap. L60 says a minted address "dies with its attachment" and L137 says it "follows its class's reclaim policy". #35 defaults to 2. Plaintext on a public addressToday both guards against plaintext on a public address hang on
An attachment is purely additive and doesn't set Decision 0002 on #20 refuses plaintext external Redis, and notes that a generic TCP publisher such as 3. "No per-chart work" and the L7 design say opposite thingsGoals say "One mechanism for every engine, with no per-chart render logic and no chart schema changes" (L63), Rollout says "no chart schema changes" (L234), Alternatives rejects a chart-values The L7 member needs exactly that. The app ships its own HTTPRoute-set (L174), "an app that must know its own hostname declares that hostname in its spec" (L178), "HTTP exposure waits on the app declaring its I understand why app-shipped routes are in the design, and I'm fine with it. But then the text should say so: Goals, Rollout and Alternatives need to state that HTTP exposure is per-chart work, who does it, and which charts go first. Right now a reader gets two different proposals depending on the section. 4. #20 and #45 disagree on who renders the routeL191 says #20 "declared its tenant-facing trigger out of scope" and "that trigger lands here". #20 does define it: the database gains "a value on the app beside #20 may well be wrong about this. But it's accepted, so either #45 follows it, or an amending decision record goes in first (#20 has 5. "Tenants never share a Gateway" doesn't match the moduleL187 and L193 say tenants never share a Gateway or a listener, and the isolation argument rests on that. But L193 also says the route attaches to the tenant's Gateway, "inherited like the tenant's ingress", and the Gateway module README says "every other tenant in the tree attaches its routes to the Gateway of the nearest ancestor that does own one". So the It matters for databases. Native-port passthrough listeners admit routes only from the publishing tenant's own namespace, deliberately, because the subtree label "would put a native database port within reach of the whole subtree" (reconciler.go L1596-L1618). My reading is that a TLSRoute synthesized in a child tenant's namespace wouldn't attach to such a listener; I haven't tested it. For TCP there are no hostnames, so two tenants on a shared port can't be told apart at all. Could the proposal say what should happen for child tenants, for hostnames and for ports? 6. A second address on a VM (text fix)L141 limits whole-IP to one per application because "a second mapping for the same workload replaces the first". In cozy-proxy v0.4.0 that holds for every delegated Service, not just whole-IP. The e2e case at L271 attaches a second 7. "The listener selects the route"L180 and open question 5 rely on a route that doesn't name its listener, and call it experimental. I couldn't find a source for it. The Gateway API spec needs both sides: the listener's 8. A false claim about HelmRelease recreationL158 says the platform has no path that deletes and recreates a tenant application's HelmRelease. 9. The tenant-facing markerAuthorization relies on |
Align the document with what is built and argue the group choice on the record rather than leaving it in a commit message of another repository. - API group: ipam.cozystack.io -> local.sdn.cozystack.io throughout, including the association annotation, the default-class annotation and the example provisioner name, which now match what the controllers actually ship. Positioning carries the argument: Cozystack networking is already sdn.cozystack.io (SecurityGroup, served by the Cozystack API), cozyplane serves its own SecurityGroup there plus local.sdn.cozystack.io for its CRD-served kinds, and addresses are the third networking feature to want a home. One sdn family, with the local. prefix marking CRD-served kinds and the bare group the aggregated ones; ipam.cozystack.io would add a fourth name to a subject area that should have one. Stated plainly as a proposal, not a precedent: nothing serves local.sdn.cozystack.io today. - New Implementation status section: the core controller and the MetalLB reference driver exist, and cozyplane consumes reservations through the contract with no module import, no CRD dependency and no informer on the claim kinds -- the association seam is exercised by a second consumer, while Rollout phase 2, a second provisioner, remains outstanding. The ValidatingAdmissionPolicy is recorded as implemented nowhere and still part of phase 1. - The stub banner no longer says the proposal predates any code, and Rollout phases 1 and 2 say what exists. - Scope records that community #45 depends on this proposal, which does not wait on it. - Directory renamed public-ip-claims -> ip-address-claims; the kinds lost the "public" prefix two revisions ago. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Review approved the shape and left four specifics. Three change the document. - spec.addressName appeared once, commented out in the claim example, with no prose anywhere. Two reviewers read it opposite ways -- one as absent entirely, one as an ungoverned way for a tenant to name a cluster-scoped object. It is now specified: it narrows the candidate set and never widens it, honoured only for an address that is Available, unclaimed, not being deleted, and of the claim's own resolved class and family, with a name that matches nothing leaving the claim Pending rather than failing. Failure cases cover the absent, Bound, Released and wrong-class names. Security now states where this component stops. It secures the surface a reservation can be stolen through; it does not decide which class a namespace may draw from, and neither addressName nor className can be gated by namespace RBAC. That is deliberate: the capability is reserve/hold/attach/release, and encoding a tenancy model this controller cannot see belongs above it -- an admission policy over those two fields, or machinery that mints claims on a tenant's behalf. Both fields are named so a deployment hardening this does not have to find them. - ExposureClass was described as a kind that exists in network.cozystack.io. It was proposed alongside community #29 and never shipped, and that group does not exist; the direction went to loadBalancerClass. Open question 3 was anchored to its fate and so could not be answered. It now asks the live question -- whether a class should declare the loadBalancerClass it corresponds to, so an association can check the pair -- and records that loadBalancerClass is immutable on an existing Service, so a mismatch has to be a refused association rather than a rewrite. - The non-goal said a workload holds one claim, which forbids a case that is not ours to forbid and that community #45 depends on: a managed service reachable on a public address and an internal routable one is two Services with two claims. The 1:1 rule that is real -- one claim, one consumer at a time -- is kept, and what stays deferred is N owned addresses on a single consumer. Also records, in Positioning, why the group is not ipam.cozystack.io: this is not all of IPAM and cannot become it, since allocation stays with the allocator. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
… local.sdn family Community #45 now homes its EndpointAttachment kind in local.sdn.cozystack.io rather than consuming the substrate from another group. Record it as another CRD-served resident of the family, which exercises the local.-prefix quota guarantee the Positioning section argues for. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Align the document with what is built and argue the group choice on the record rather than leaving it in a commit message of another repository. - API group: ipam.cozystack.io -> local.sdn.cozystack.io throughout, including the association annotation, the default-class annotation and the example provisioner name, which now match what the controllers actually ship. Positioning carries the argument: Cozystack networking is already sdn.cozystack.io (SecurityGroup, served by the Cozystack API), cozyplane serves its own SecurityGroup there plus local.sdn.cozystack.io for its CRD-served kinds, and addresses are the third networking feature to want a home. One sdn family, with the local. prefix marking CRD-served kinds and the bare group the aggregated ones; ipam.cozystack.io would add a fourth name to a subject area that should have one. Stated plainly as a proposal, not a precedent: nothing serves local.sdn.cozystack.io today. - New Implementation status section: the core controller and the MetalLB reference driver exist, and cozyplane consumes reservations through the contract with no module import, no CRD dependency and no informer on the claim kinds -- the association seam is exercised by a second consumer, while Rollout phase 2, a second provisioner, remains outstanding. The ValidatingAdmissionPolicy is recorded as implemented nowhere and still part of phase 1. - The stub banner no longer says the proposal predates any code, and Rollout phases 1 and 2 say what exists. - Scope records that community #45 depends on this proposal, which does not wait on it. - Directory renamed public-ip-claims -> ip-address-claims; the kinds lost the "public" prefix two revisions ago. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Review approved the shape and left four specifics. Three change the document. - spec.addressName appeared once, commented out in the claim example, with no prose anywhere. Two reviewers read it opposite ways -- one as absent entirely, one as an ungoverned way for a tenant to name a cluster-scoped object. It is now specified: it narrows the candidate set and never widens it, honoured only for an address that is Available, unclaimed, not being deleted, and of the claim's own resolved class and family, with a name that matches nothing leaving the claim Pending rather than failing. Failure cases cover the absent, Bound, Released and wrong-class names. Security now states where this component stops. It secures the surface a reservation can be stolen through; it does not decide which class a namespace may draw from, and neither addressName nor className can be gated by namespace RBAC. That is deliberate: the capability is reserve/hold/attach/release, and encoding a tenancy model this controller cannot see belongs above it -- an admission policy over those two fields, or machinery that mints claims on a tenant's behalf. Both fields are named so a deployment hardening this does not have to find them. - ExposureClass was described as a kind that exists in network.cozystack.io. It was proposed alongside community #29 and never shipped, and that group does not exist; the direction went to loadBalancerClass. Open question 3 was anchored to its fate and so could not be answered. It now asks the live question -- whether a class should declare the loadBalancerClass it corresponds to, so an association can check the pair -- and records that loadBalancerClass is immutable on an existing Service, so a mismatch has to be a refused association rather than a rewrite. - The non-goal said a workload holds one claim, which forbids a case that is not ours to forbid and that community #45 depends on: a managed service reachable on a public address and an internal routable one is two Services with two claims. The 1:1 rule that is real -- one claim, one consumer at a time -- is kept, and what stays deferred is N owned addresses on a single consumer. Also records, in Positioning, why the group is not ipam.cozystack.io: this is not all of IPAM and cannot become it, since allocation stays with the allocator. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
… local.sdn family Community #45 now homes its EndpointAttachment kind in local.sdn.cozystack.io rather than consuming the substrate from another group. Record it as another CRD-served resident of the family, which exercises the local.-prefix quota guarantee the Positioning section argues for. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
What this proposal is
Managed applications already have endpoints — the Services their operators create (Postgres
-rw/-ro, a tenant cluster's API Service, Redis master) — and, with #35, addresses are becoming first-class resources that can be held, moved, and quota'd. Nothing connects the two: the only exposure mechanism is the chart-levelexternal: true|false, which publishes one chart-chosen endpoint on one unchooseable address through five structurally different render paths.This proposal adds
EndpointAttachment(cozystack.io/v1alpha1, namespaced, tenant-created): attach an external address to one endpoint of one application — as many attachments per application as needed, each with an independent lifecycle, none of them ever mutating the application's own Services or values. An attachment either mints anIPAddressClaimfrom a class (ephemeral, dies with the attachment) or binds a pre-reserved claim (the elastic-IP hold/move experience — the address survives detachment). The controller is engine-agnostic: it renders one additivetype: LoadBalancerService mirroring the endpoint Service's selector and ports, and consumes the address through the substrate's claim annotation, so no per-chart render logic is needed. Attachments are garbage-collected with their application.Relationship to prior proposals
ServiceExposure): the Alternatives considered section addresses why this is not that mistake repeated — N-per-app attachment lifecycle, claim hold/move semantics, and an authorization seam over which Services may be published do not reduce to aloadBalancerClassfield on an existing Service.external: truekeeps its exact behavior; its sunset is explicitly gated on an address-preserving migration via the substrate's adoption path.Out of scope
Hostnames/DNS/TLS/SANs (unified-TLS effort, #19), source-IP ACL (
SecurityGroup, cozystack#2922), the discovery-engine write-back loop (Kafka / Mongo replica sets / NATS — deferred, stated plainly), Gateway API (a future union member the API shape leaves room for), and vm-instance whole-IP semantics.Summary by CodeRabbit