fix(operator): block duplicate network expose endpoints across packages#2783
fix(operator): block duplicate network expose endpoints across packages#2783chance-coleman wants to merge 3 commits into
Conversation
|
@greptileai review |
Greptile SummaryThis PR adds admission-time collision detection to the Pepr operator to prevent two
Confidence Score: 5/5Safe to merge — the change is additive and contained to the in-memory index and admission webhook. The collision detection follows the identical pattern as the existing SSO clientId check, the fqdnIndex cleanup is symmetric across add/update/remove, and the test suite covers the full matrix of scenarios including self-update, gateway distinction, and cross-namespace denial. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant K8s as Kubernetes API
participant PW as Pepr Webhook (validator)
participant PS as PackageStore (in-memory)
participant FI as fqdnIndex
Note over PS,FI: Package A (ns-a) already admitted
PS->>FI: "fqdnIndex[tenant:doom.uds.dev] = ns-a"
K8s->>PW: Admit UDSPackage (ns-b, expose: doom.uds.dev)
PW->>PS: "findNamespaceForExpose({host:doom})"
PS->>FI: get(tenant:doom.uds.dev)
FI-->>PS: ns-a
PS-->>PW: "ownerNs = ns-a"
PW->>PW: "ownerNs(ns-a) != ns(ns-b)"
PW-->>K8s: Deny - endpoint already owned by ns-a
K8s->>PW: Admit UDSPackage (ns-a update, expose: doom.uds.dev)
PW->>PS: "findNamespaceForExpose({host:doom})"
PS->>FI: get(tenant:doom.uds.dev)
FI-->>PS: ns-a
PS-->>PW: "ownerNs = ns-a"
PW->>PW: "ownerNs(ns-a) == ns(ns-a)"
PW-->>K8s: Approve (self-update allowed)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant K8s as Kubernetes API
participant PW as Pepr Webhook (validator)
participant PS as PackageStore (in-memory)
participant FI as fqdnIndex
Note over PS,FI: Package A (ns-a) already admitted
PS->>FI: "fqdnIndex[tenant:doom.uds.dev] = ns-a"
K8s->>PW: Admit UDSPackage (ns-b, expose: doom.uds.dev)
PW->>PS: "findNamespaceForExpose({host:doom})"
PS->>FI: get(tenant:doom.uds.dev)
FI-->>PS: ns-a
PS-->>PW: "ownerNs = ns-a"
PW->>PW: "ownerNs(ns-a) != ns(ns-b)"
PW-->>K8s: Deny - endpoint already owned by ns-a
K8s->>PW: Admit UDSPackage (ns-a update, expose: doom.uds.dev)
PW->>PS: "findNamespaceForExpose({host:doom})"
PS->>FI: get(tenant:doom.uds.dev)
FI-->>PS: ns-a
PS-->>PW: "ownerNs = ns-a"
PW->>PW: "ownerNs(ns-a) == ns(ns-a)"
PW-->>K8s: Approve (self-update allowed)
Reviews (2): Last reviewed commit: "Merge branch 'main' into chance/core-600" | Re-trigger Greptile |
|
LGTM 🚀 |
mjnagel
left a comment
There was a problem hiding this comment.
Haven't done any code review but do want to call out that this is a breaking change and should be reflected as such in release notes at a minimum. I believe there is also technically a use-case for this when using advanced http match in an expose entry - you could have certain paths route to services in different namespaces (for the same endpoint). Whether that is something that is currently being used and/or should be supported I'm not sure.
Yeah this is a good callout. I think it wouldn't be a breaking change if we prevent expose entries that technically cause a conflict? If they have the same spec one of the expose entries will just not work (and I would consider that as a bug fix?). Maybe we take into account more than just the expose fqdn to make sure there aren't conflicts. |
|
On the breaking change: i agree that any existing overlap is already silently broken at the Istio level (last VirtualService wins, undefined behavior). The admission deny just makes it visible. That said, i think it makes sense to call this change in behavior out. so I'll add a release note calling it out. On advancedHTTP.match: The current check uses gateway:fqdn only and would incorrectly deny two packages routing different paths on the same host. I'm thinking i will just implement a fix for this now, if |
I need to think through this... But would it instead be worth it to just take a hash or string combination of all the fields that make an expose route unique/work? i.e. FQDN + advancedHTTP + any other fields? |
the hash approach is better than skip for catching exact duplicates, but neither solves actual path overlap detection. Whether that extra coverage is worth the added implementation complexity is the real question. |
Description
Two
UDSPackageCRs in different namespaces can expose the same ingress endpoint with no warning. Whichever VirtualService Istio resolves last wins and the other silently drops traffic. This was discovered while working on UDS Connect where deploying the same app into two namespaces created two packages both claimingdoom.uds.dev.Adds admission-time collision detection to the Pepr operator. If a package tries to expose an endpoint already owned by a package in another namespace, Pepr denies the request immediately with a clear error naming the owning namespace.
The index key is
gateway:fqdnso tenant and passthrough entries with the same hostname are correctly treated as distinct routes and don't false-collide. Follows the same in-memory index pattern used by the existing SSOclientIdcollision check.Related Issue
Fixes #CORE-600
Type of change
Steps to Validate
UDSPackageCR at admission with an error message containing the FQDN and the owning namespaceChecklist before merging