Repository navigation
egress: add an example credential provider which reads from k8s secret - #1335
Conversation
|
I think this PR is honestly a bit noisy. There's both the credential provider, and the implementation of yet another Doc since it's not linked anywhere: https://docs.google.com/document/d/1-zQKvu-oYJlva_kwL3xbAOaFoJJaqO0qEqmqRlt1r0c/edit?resourcekey=0-TDrWohfRuYolcXYO-QPVTQ&tab=t.0#heading=h.rx0okwgtp5yu |
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
We discussed offline but for posterity, Eitan Yarmush (@EItanya) 's idea to split the pr sounds good.
02a01c3 to
d2731aa
Compare
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
Thanks Yufan Su (@yufan-su) !
d4669d5 to
9587727
Compare
9587727 to
81e77c4
Compare
There was a problem hiding this comment.
One of the comments Tim mentioned is multi cluster. E.g I am storing a secret in a different cluster.
Not a requirement for this PR but the (code) design is important to support that. E.g
"I install the kubernetes.io provider (and have it in as a registration flag), and together with this I either (a) provide a config map that has my cluster-name/dns/control-plane ip to a kubeconfig entry (b) store that mapping somewhere else not in a config map?"
how is the plugin authorized to read secrets? is it a service account only? can we support what I mentioned above to support fetching secrets from a different cluster?
Should that be in scope for this initial round of this plugin. I still don't think we've decided what the support level is for this component, if it's official or just an example etc. Maybe let's start with this as the default local secret impl and then we can expand or let others bring their own multicluster vairant? |
|
I think we should not get too into the weeds on the multicluster aspect of it right now. We could leave a space in the path for later expansion if we really want to: We have: Maybe Later we can add |
81e77c4 to
55072e6
Compare
|
Updated the substrate uri format to be replaced |
|
Minor comments -- generally looks good. Should clarify URI escaping is not supported in the string. |
Wait why not starting with clusters now? What are we loosing? /clusters/self |
We should start with clusters/cluster-name now. Adding another element later just complicates that. Why not just go with clusters/'self' and support only local now or something (Or self instead or underscore self) |
|
Errr _self_ |
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
🤖 AI-generated review.
Three findings below. The four focused test packages passed, and local TLS and request-load probes reproduced the first two findings on 55072e64. I checked the changes through 6fba1c3; the client trust loading, Kubernetes client limits, and image build lists are unchanged. No live Kubernetes deployment was tested.
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
LGTM. I really prefer having the same element list in the uri and start with /clusters/_self_ or some other preserved thing but... not going to block on that, so good with whatever you decide
… config via flags
|
What are the requested changes that are still pending? Eitan Yarmush (@EItanya) I can't find any pending items from your comments. |
…nfigurable (agent-substrate#350) # Description **Substrate assumes the canonical install layout, and every deviation fails closed.** The namespace `ate-system`, the Services `api` / `atenet-router`, and the ServiceAccounts `atelet` / `atenet-router` are compiled-in constants. An install in a per-developer namespace, or under a deployment that prefixes resource names, breaks — and no failure points at the naming. This series makes the namespace, the Service names, and the ServiceAccount names configurable. Every option defaults to the canonical value. A canonical install is byte-for-byte unaffected. | Hardcoded assumption | Failure | What it looks like instead | |---|---|---| | atelet's namespace in ateapi's SPIFFE check | ateapi rejects every atelet | an mTLS handshake failure, not a naming error | | atelet's namespace in the worker's broker check | no actor obtains a certificate | `credential broker is not atelet` | | NetworkPolicy's ingress namespace | the CNI drops every request to the pool | a silent network fault | | ateapi Service name and namespace in the client | the client cannot authenticate | `services "api" not found`, then `invalid bearer token` | | Resource names in `hack/install-ate.sh` and the e2e harness | authorities land in the wrong namespace | suites die in preflight | **A SPIFFE ID breaks on two axes.** It names a namespace and a ServiceAccount. Both were constants, and the failure surfaces as a rejected peer, not a missing object. ## Design notes - **ate-controller passes the worker-side identities.** ateom already took `--atunnel-client-identity` as a flag but relied on its default. A new `--atunnel-broker-identity` flag alone would be inert: correct only where the constant was already correct. The controller knows the control plane's namespace, so it supplies both. - **ateapi takes the whole expected identity, not a namespace.** The dialer and `ateletauth` used the namespace only to build one string. One place decides how atelet's identity is spelled. - **`ateletauth` deduplicates without taking the dependency it was avoiding.** Its constants are duplicated rather than imported so the package does not depend on `controlapi` for three strings. `ateletdial` would make a third copy, so the strings move to `internal/installdefaults` instead — a leaf package of constants that imports only the standard library, so consuming it does not reintroduce the coupling the duplication was there to prevent. - **The client reads environment variables, not flags.** It runs outside the cluster. It has no downward API and nothing to discover from. - **Namespaces come from the downward API, not flags.** Every supported topology co-locates these components. Only Service and ServiceAccount names, which a deployment may legitimately rename, get flags. - **`hack/install-ate.sh` and the e2e harness are in scope.** A relocated install cannot be bootstrapped or exercised without them. The script refuses `ATE_NAMESPACE` or `ATE_API_SERVICE_NAME` overrides on the manifest-applying subcommands, which would half-install: `manifests/ate-install/` names `ate-system` and `api` literally. ## Why this is one PR The series is stacked, not parallel. Commit 1 creates `internal/installdefaults` and seeds seven imports; commit 2 adds fourteen more; every later commit builds on those constants. Split into separate PRs, each blocks on the previous merging and none reviews independently. The guard test cannot land first. Applied to `main` it fails on eleven hardcoded literals across `ateletauth`, `controlapi/informer.go`, `networkpolicy_controller.go`, both `ateom` mains, `ateclient`, `ateletdial` and three e2e files. It is green only because the preceding commits removed them. A partial series still fails closed. Each commit's audit turned up more places assuming `ate-system`, so landing commits 1-3 without 4-5 leaves a relocated install broken later and less legibly than before. `CONTRIBUTING.md` covers this case: when the intermediate steps are not useful on their own, keep the change as one PR split into commits at logical break points. Review commit by commit, and preserve the commits on merge. If you would still rather split, the one defensible cut is by axis — relocation (commits 1-3) then renaming (commits 4-5) with the guard test rebased on top. ## Rebase notes (2026-09-15) The series is rebased onto `main` at `d0d85c39`, ninety-two commits on from the base the PR first carried: - `main` moved actor JWT and certificate minting out of `cmd/ateapi/internal/actoridentity` into `controlapi`, deleting the package and dropping its inline atelet authorization check in favor of the unified authorizer. This series previously threaded the expected atelet identity into that check; the check no longer exists, so that adaptation is gone. `ateletauth` survives with one caller, `workerservice.SetWorkerCapacity`, which still takes the configured identity. - The same refactor replaced `BrokerConfig.ExpectedActorUID` with an `ActorAtespace` / `ActorName` / `ActorUID` triple. `AteletSPIFFEID` is additive and still required by `ateletdial.TLSConfig`. - `ateletauth` (ateapi's side) and `ateletdial` (the worker's side) each re-declared the hardcoded SPIFFE ID. Both take the expected identity as a parameter. - `main` deleted the atenet DNS subsystem. The series no longer touches it, and `installdefaults` carries no DNS Service name. - The controller passes `--atunnel-broker-identity` only when it differs from the canonical default. An ateom old enough to predate the flag exits on it, and `docs/upgrade.md` keeps such a pool serving during a rolling upgrade. A relocated install gets the flag and necessarily runs an image that accepts it. - `main` added `internal/e2e/collector_metrics.go` (agentgateway CI support), which addresses the router through the package-level `routerNamespace` and `routerService` that this series replaces. Its two call sites become `SystemNamespace()` and `ResourceName("atenet-router")`, matching `router_client.go` and `statusz.go`. A reviewer diffing against the older base sees those call sites move; nothing else in that file changes. - `main` added `cmd/credential-provider/kubernetes-secrets` (`agent-substrate#1335`), whose `injectorSPIFFEID` constant is the mTLS peer check on the only caller permitted to read Secrets. The comparison is an exact string match, so a renamed install rejects every fetch at TLS. It becomes `--injector-identity`, defaulting to `installdefaults.EgressSPIFFEID(SystemNamespace)`, alongside a new `EgressServiceAccount` constant and helper. The flag name follows `--atunnel-client-identity`; the manifest beside it still names `ate-system`, so a renaming deployment configures both. - `main` replaced the dialer's worker indexer with `DialForAteletOnNode`, dropped `WorkerPodInformer` from `controlapi`, and removed `kataConfig` from ateom-microvm's `NewService`. The series adapts to each narrower signature and keeps only its own added parameter. - The guard test's allowlist entry for the two inert `ate-system` literals follows the code from `actoridentity.go` to `controlapi/actor.go`. The refactor re-introduced exactly the class of constant this series removes, so the guard earns its place. # Testing - `go test -race ./...` passes. `make verify` passes boilerplate, codegen, go-modules, gofmt, golangci-lint, kube-api-linter, licenses, metrics and postgresql-migrations. `proto-fmt` needs `clang-format`, which is absent locally; this series touches no `.proto` files. - Every commit builds and vets individually, via `git rebase -x 'go build ./... && go vet ./...'`. - On a fresh kind cluster, all twelve e2e suites pass: `capabilities`, `combinedvolumes`, `demo`, `egressauthz`, `egressmitm`, `example`, `identity`, `metrics`, `networking`, `networkpolicy`, `parking`, `sizing`. The `demo`, `metrics`, `networkpolicy`, `parking` and `networking` suites need the `--deploy-demo-counter` and `--deploy-demo-egress` fixtures, and the MITM path needs `--deploy-demo-egress-mitm`; without them they fail on `actor template not found`, which reads as a control-plane fault rather than a missing fixture. - With the sdsmint egress gateway (`--deploy-atenet --experimental-use-sdsmint`, `E2E_EGRESS_MITM=1`), `TestActorEgressMITMTrust` and `TestActorEgressHTTPSByHostnameMITM` both pass, and the `networking` suite is green at 25 passed. The two modes are mutually exclusive by design: `TestActorEgressHTTPSByHostnamePassthrough` covers the plain gateway and skips under MITM, and the two MITM tests skip without it. Both modes were run, so every case executed in one of them. - The new unit tests use a relocated namespace and renamed ServiceAccounts. Reintroducing each hardcoding makes them fail. One e2e caveat that predates this series and misleads: the `identity` suite calls `ReplaceEgressTrustPool`, which takes over the shared `egress-mitm-ca-pool` Secret, overwrites it and registers no cleanup. Any MITM test running afterwards fails with `certificate signed by unknown authority`, which reads as a broken interception path rather than a mutated fixture. Reinstall the gateway before running `egressmitm`. # Additional Notes **Credential contents stay canonical.** `controlapi/actor.go` still mints credentials naming `api.ate-system.svc`: the JWT issuer and the certificate's Issuer CN. Relying parties validate these, so changing them is a compatibility decision, not a lookup fix. The code carries a TODO to make the issuer a globally unique, OIDC-compliant name.
) Follow-up change on a comment in agent-substrate#1335 to add a statusz page in the credential provider service. - [x] Tests pass - [ ] Appropriate changes to documentation are included in the PR
Add an example gRPC credential-provider plugin which reads from k8s secrets for the egress
credential-injection path.
ate-secret://k8s.io/default/<namespace>/<secret>/<key>URIs to Kubernetes Secret values, so Substrate never stores secrets — it only
brokers a read that the provider is authorized to perform.
component in the injection path with Kubernetes secerts access; the egress gateway and
its injector never read Secrets directly.
What's included
cmd/credential-provider/kubernetes-secrets— the gRPC service:ate-secret://URIs, resolving the requested Secret(with single-key fallback when the URI omits a key).
derived from the caller's attested actor SPIFFE ID.
the trust bundle and carry the egress injector's SPIFFE SAN.
manifests/egress-credential-injection/) — Deployment,Service, ServiceAccount + RBAC, a sample namespace-policy ConfigMap, and a
sample Secret.
credprovidertok8s-credential-provideracrossate-setup, install scripts, and the egressinjection overlay.
atespace → namespace authorization
Beyond the mTLS check that only the egress injector may call the provider, each
request is authorized against a default-deny atespace→namespace policy. The
provider derives the requesting actor's atespace from its attested SPIFFE ID and
resolves a Secret only if that atespace is explicitly granted access to the URI's
namespace.