feat: centralized ambient egress #2194

chance-coleman · 2025-12-04T19:39:31Z

Description

Centralizes Ambient egress control per host. For each external host:
- Creates a shared ServiceEntry in istio-egress-ambient.
- Creates a centralized AuthorizationPolicy in istio-egress-ambient targeting that ServiceEntry with from-only rules.
Identity resolution: Unions owners (host-specific) and participants (remoteGenerated: Anywhere) with SA-first principals and namespace fallback.
Safety: Skips SE/AP creation when no identities are resolved to avoid transient allow; uses generation-based purge to remove prior resources (Gateway, ServiceEntry, AuthorizationPolicy).
Quality: Deterministic resource names via sanitizeWithLimit(). Ambient constants centralized in istio-resources.ts.

New AP flow (Ambient)

Build per-host desired state from in-memory package map (union of ports/protocols + contributing packages).
Resolve identities (owners + Anywhere participants). If non-empty:
- Apply shared per-host ServiceEntry (bound to egress waypoint).
- Apply centralized AuthorizationPolicy with from-only rules and targetRef to the host’s ServiceEntry.
Increment generation and purge prior-gen ambient resources.

Related Issue

Fixes #2064

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Other (security config, docs update, etc)

Steps to Validate

uds run test:uds-core-e2e
npm run test:unit
npm run test-uds-core-upgrade

Manual steps

apply these manifests
verify central resources
- kubectl -n istio-egress-ambient get se,ap | grep httpbin # expect ambient-se-httpbin-org, ambient-ap-httpbin-org
get pods
- R=$(kubectl -n ambient-egress-restricted get pod -l app=restricted-curl -o name)
- A=$(kubectl -n ambient-egress-anywhere get pod -l app=anywhere-curl -o name)
curl checks
- Restricted: kubectl -n ambient-egress-restricted exec -it $R -- sh -c 'curl -s -w " HTTP_CODE:%{http_code}\n" https://httpbin.org' # ALLOW (retry if 503)
- Restricted: kubectl -n ambient-egress-restricted exec -it $R -- sh -c "curl -s -w ' HTTP_CODE:%{http_code}\n' http://httpbin.org" # DENY
- Restricted: kubectl -n ambient-egress-restricted exec -it $R -- sh -c 'curl -s -w " HTTP_CODE:%{http_code}\n" https://api.github.com' # DENY
- Anywhere: kubectl -n ambient-egress-anywhere exec -it $A -- sh -c 'curl -s -w " HTTP_CODE:%{http_code}\n" https://httpbin.org' # ALLOW
- Anywhere: kubectl -n ambient-egress-anywhere exec -it $A -- sh -c "curl -s -w ' HTTP_CODE:%{http_code}\n' http://httpbin.org" # DENY
- Anywhere: kubectl -n ambient-egress-anywhere exec -it $A -- sh -c 'curl -s -w " HTTP_CODE:%{http_code}\n" https://api.github.com' # ALLOW (L4 Anywhere)

Checklist before merging

Test, docs, adr added or updated as needed
Contributor Guide followed

chance-coleman · 2025-12-08T14:43:08Z

src/pepr/operator/controllers/istio/egress-orchestrator.ts

This file came out of a refactor to avoid circular import dependency between egress.ts and istio-resources.ts due to ingress and egress orchestration having been intertwined. Previously resulted in unit-test failures due to the dependency issues.

Copilot

Pull request overview

This PR centralizes Ambient egress control by creating shared, per-host resources in the istio-egress-ambient namespace. Previously, each package created its own egress resources; now a single ServiceEntry and AuthorizationPolicy per external host is created across all Ambient packages. Identity resolution uses a SA-first principals approach with namespace fallback, combining host owners and "Anywhere" participants. Safety checks prevent transient allow windows by skipping resource creation when no identities are resolved, and generation-based purging removes stale resources.

Key Changes:

Centralized per-host ServiceEntry and AuthorizationPolicy in istio-egress-ambient namespace with unified identity resolution (owners + Anywhere participants)
New egress-orchestrator.ts separates egress reconciliation logic; remapAmbientEgressResources() merges package maps into per-host resources
sanitizeWithLimit() utility ensures deterministic resource naming within Kubernetes limits

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
test/vitest/network.spec.ts	Adds tests for per-host egress isolation in Ambient mode
src/pepr/operator/reconcilers/package-reconciler.ts	Calls `istioEgressResources()` separately; counts unique hosts for AP status
src/pepr/operator/crd/validators/package-validator.ts	Allows `serviceAccount` with `remoteGenerated: Anywhere` on Egress in Ambient
src/pepr/operator/controllers/utils.ts	Adds `sanitizeWithLimit()` for deterministic name truncation with hash suffix
src/pepr/operator/controllers/network/generators/egress.ts	Updates import for `ambientEgressNamespace` from centralized location
src/pepr/operator/controllers/istio/service-entry.ts	Implements `generateSharedAmbientServiceEntry()` for centralized per-host SE
src/pepr/operator/controllers/istio/service-entry.spec.ts	Tests shared Ambient SE generation with waypoint binding and annotations
src/pepr/operator/controllers/istio/istio-resources.ts	Centralizes Ambient egress constants; removes legacy `istioEgressResources()`
src/pepr/operator/controllers/istio/istio-resources.spec.ts	Updates mocks and expectations for orchestrator-based egress flow
src/pepr/operator/controllers/istio/egress.ts	Adds `remapAmbientEgressResources()` to merge package maps; defensive null checks
src/pepr/operator/controllers/istio/egress.spec.ts	Tests remapping logic for merging ports/protocols and collecting packages
src/pepr/operator/controllers/istio/egress-orchestrator.ts	New orchestrator for egress reconciliation; replaces removed function
src/pepr/operator/controllers/istio/egress-ambient.ts	Rewrites to create centralized SE/AP with pre-indexed identity resolution
src/pepr/operator/controllers/istio/egress-ambient.spec.ts	Comprehensive tests for identity resolution, merging, and fallback scenarios
src/pepr/operator/controllers/istio/defaultTestMocks.ts	Adds `getPkgListMock` for UDSPackage.Get() in tests
src/pepr/operator/controllers/istio/auth-policy.ts	Implements `generateCentralAmbientEgressAuthorizationPolicy()` with targetRef
src/pepr/operator/controllers/istio/auth-policy.spec.ts	Tests central AP generation with from-only rules targeting ServiceEntry
src/pepr/operator/controllers/istio/ambient-waypoint.ts	Updates import path for centralized ambient constants
docs/reference/configuration/service-mesh/egress.md	Updates documentation to reflect centralized per-host SE/AP architecture

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

joelmccoy

Generally LGTM! Tested some of the functionality locally with no issues. A couple comments/questions for consideration

docs/reference/configuration/service-mesh/egress.md

src/pepr/operator/controllers/istio/egress-ambient.ts

src/pepr/operator/controllers/istio/egress.ts

joelmccoy · 2025-12-10T19:09:44Z

src/pepr/operator/reconcilers/package-reconciler.ts

+    const uniqueHosts = new Set(
+      egressRequestedFromNetwork(pkg.spec.network.allow)
+        .map(a => a.remoteHost)
+        .filter((h): h is string => typeof h === "string" && h.length > 0),
+    );
+    numEgressAuthPols = uniqueHosts.size;


nit: I don't have a strong opinion, but wanted to ask the question in case somebody does.... Do we want to count the central auth policy as a count for this package?

I almost lean towards not counting it as it's extra logic and doesn't reflect the namespaced scope # of auth policies we would see in the package namespace

Chatted with @mjnagel about this and i think we're in agreement that this count doesn't really provide that much so we also lean towards ont counting it towards the policy count.

Just to sanity check, we are in agreement that the remote host shared authpolicy entries should NOT be counted towards the package auth policy count? If so, I think we can still remove some of this logic here?

this should be addressed in the lates commit 👍🏼

test/vitest/network.spec.ts

src/pepr/operator/controllers/utils.ts

src/pepr/operator/controllers/istio/istio-resources.spec.ts

src/pepr/operator/controllers/istio/egress-ambient.ts

src/pepr/operator/controllers/istio/egress-ambient.spec.ts

mjnagel

Initial comments - need to go through some of the shared logic a bit more to comprehend the behavior and do testing.

src/pepr/operator/controllers/utils.ts

src/pepr/operator/controllers/istio/auth-policy.ts

src/pepr/operator/controllers/istio/egress-ambient.ts

src/pepr/operator/controllers/istio/egress.ts

test/vitest/network.spec.ts

…s using central resources

mjnagel · 2025-12-18T15:39:22Z

src/pepr/operator/controllers/istio/egress-ambient.ts

-    log.debug(serviceEntry, `Applying Service Entry ${serviceEntry.metadata?.name}`);
+    for (const allow of egressRules) {
+      // Process Anywhere participants
+      if (allow.remoteGenerated === RemoteGenerated.Anywhere) {


I think we also need to check the port here to make sure that something allowed to "anywhere" on port 80 isn't allowed to hit a remote host on 443.

In testing I think this will mean we need to split the authpols by port (to.port on the rules) in order to handle remoteGenerated allowed to one port but not another, where a remoteHost is set for multiple ports.

mjnagel · 2025-12-18T16:11:16Z

src/pepr/operator/controllers/istio/auth-policy.ts

-      namespace,
+      name,
+      namespace: ambientEgressNamespace,
      labels: {


Functionality it doesn't seem like we need it, but might be good to add the uds.dev/user- annotations to link this back to the "owners" (the packages explicitly requesting it)?

mjnagel

Did more e2e testing today and encountered some timing problems I believe as well as the port issue.

mjnagel · 2025-12-18T20:09:11Z

src/pepr/operator/controllers/istio/egress-ambient.ts

+      await purgeOrphans(String(generation), ns, name, IstioServiceEntry, log, {
+        "istio.io/use-waypoint": "egress-waypoint",
+      });
+      await purgeOrphans(String(generation), ns, name, IstioAuthorizationPolicy, log, {
+        "uds/for": "egress",
+      });


I think there's a slim edge case here where this fails to delete things? Since generation is the new "shared" generation, if the Package CR happened to have been on that generation (for its "local" generation) then nothing would get purged. I think we could probably just directly get, then delete all of them (probably separate from purgeOrphans since that requires a generation for filtering).

mjnagel · 2025-12-18T20:24:19Z

src/pepr/operator/controllers/istio/egress-ambient.ts

+      sharedEgressPkgId,
+      IstioAuthorizationPolicy,
+      log,
+    );


I'm having trouble tracing down the "why" but this purge seems to have a race condition or other bug where it can result in most resources getting purged. I've been testing with a large set of packages and when applying them all together I end up with no egress service entries or authpols in my cluster. Restarting the watcher typically works to create them, but there's also some strange behavior on delete at times where deleting all the packages together sometimes creates/leaves them.

Trying to do more testing and debug, but definitely seems like the applies are ending up running out of order so I end up with an outdated generation "winning" for the resulting serviceentry/authpol, and then the purge functions remove them.

mjnagel · 2025-12-18T21:34:31Z

src/pepr/operator/controllers/istio/egress-ambient.ts

-    log.debug(serviceEntry, `Applying Service Entry ${serviceEntry.metadata?.name}`);
+    for (const allow of egressRules) {
+      // Process Anywhere participants
+      if (allow.remoteGenerated === RemoteGenerated.Anywhere) {


In testing I think this will mean we need to split the authpols by port (to.port on the rules) in order to handle remoteGenerated allowed to one port but not another, where a remoteHost is set for multiple ports.

feat: Centralized Ambient Egress

5023e14

chance-coleman self-assigned this Dec 4, 2025

Merge branch 'main' into ambient-egress

d7bd102

chance-coleman changed the title ~~feat: Centralized Ambient Egress~~ feat: centralized ambient egress Dec 4, 2025

chance-coleman added 2 commits December 5, 2025 08:59

refactor circular dependency issues

0b86f4c

remove duplication and fix unique counter

05e2d3f

chance-coleman commented Dec 8, 2025

View reviewed changes

chance-coleman marked this pull request as ready for review December 8, 2025 14:44

chance-coleman requested a review from a team as a code owner December 8, 2025 14:44

Merge branch 'main' into ambient-egress

bdfa13d

joelmccoy requested a review from Copilot December 9, 2025 21:26

Copilot started reviewing on behalf of joelmccoy December 9, 2025 21:26 View session

Copilot AI reviewed Dec 9, 2025

View reviewed changes

joelmccoy reviewed Dec 10, 2025

View reviewed changes

mjnagel reviewed Dec 10, 2025

View reviewed changes

chance-coleman added 3 commits December 11, 2025 13:32

address PR feedback

50583f0

add operator removal of local ap and se resources for ambient package…

a2e5291

…s using central resources

remove central policy from count

ddf788a

mjnagel reviewed Dec 18, 2025

View reviewed changes

chance-coleman added 3 commits December 19, 2025 10:42

refactor for pr feedback

6fb9f10

fix: bulk apply and deletion

2d04977

fix: tests linting errors

f0552c6

feat: centralized ambient egress #2194

Are you sure you want to change the base?

feat: centralized ambient egress #2194

Uh oh!

Conversation

chance-coleman commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

New AP flow (Ambient)

Related Issue

Type of change

Steps to Validate

Manual steps

Checklist before merging

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

joelmccoy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjnagel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjnagel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chance-coleman commented Dec 4, 2025 •

edited

Loading