Repository navigation
router authn/z part 3: Convert all e2e tests so that they pass for static-mtls ingress auth mode - #2390
Conversation
|
Taahir Ahmed (@ahmedtd) Can you please make sure the PR is ready for review today or on Monday? Thank you! |
437ea8f to
e7b90fa
Compare
| if e2e.IngressMTLS() { | ||
| t.Skipf("This subtest only checks deprecated-insecure functionality") | ||
| } |
There was a problem hiding this comment.
this check is in ext_proc so it applies on TLS too, and I think this means the presubmit (envoy-only one) skips it so never runs. Maybe we just send it with router.TLSConfig()?
There was a problem hiding this comment.
OK, I've adapted this so it goes through the same router as the other test cases. Let's see if it passes.
| } | ||
|
|
||
| address := fmt.Sprintf("127.0.0.1:%d", localPort) | ||
| transport := http.DefaultTransport.(*http.Transport).Clone() |
There was a problem hiding this comment.
DefaultTransport here means you move the suit to negotiate h1 and h2 when its TLS.
Was this intentional? if yes, why?
There was a problem hiding this comment.
This wasn't intentional, but I'm not sure what that affects. Does the router only speak H1?
There was a problem hiding this comment.
Just that now it would only go to h2 i think
| // serviceDNSTrustPool returns the live trust bundle of the servicedns signer, | ||
| // which issues the serving certificates of substrate's in-cluster Services | ||
| // (ateapi and atenet-router among them). | ||
| func serviceDNSTrustPool(ctx context.Context, clientset kubernetes.Interface) (*x509.CertPool, error) { |
There was a problem hiding this comment.
nit: isnt this the exact same as serverTLSConfig? should we put it somewhere shared?
There was a problem hiding this comment.
I don't think this needs to be shared. Most clients of the Substrate API and router should be getting these roots from a clusterTrustBundle projected volume, not directly interacting with the ClusterTrustBundle k8s API.
| // The router client's identity. The router's base manifest | ||
| // (manifests/ate-install/atenet-router.yaml) allowlists | ||
| // RouterClientServiceAccount's SPIFFE ID, so the two must change together. The | ||
| // namespace is fixed rather than per-test because the allowlist is fixed at | ||
| // install time. |
There was a problem hiding this comment.
The atenet-router manifest that was merged in #2387 shouldn't carry an e2e-only identity, otherwise with static-mtls install, we trust whoever can create this namespace and a PCR in it. Can we ship the manifest with no IDs and the e2e install add this one? I think that means letting the router start with an empty allowlist in deprecated-insecure mode (it changes nothing there, clients without certs already are allowed), and we probably should keep the non-empty check for static-mtls.
I'd rather fix this here than in a follow-up, since this is the PR that makes CI depend on the ID
There was a problem hiding this comment.
I opened #2903 to make it configurable on cmd/ate-setup. It's a little big, but I can merge it into this one if you like.
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
Lgtm with the understanding that there is a follow up to remove deprecated-insecure mode
3867995 to
409fb46
Compare
…ithout the router's alert (agent-substrate#2961) `TestIngressAuthentication` fails about one e2e job in three since static-mtls became the e2e default (5 of the 15 pr-workflow runs since agent-substrate#2390 merged): the refused client's request ends with `http2: client conn could not be established` instead of the router's TLS alert, and the test calls the refusal unproven. The router closes the connection right after the alert, and the port-forward (`portforward.go:549 ... write: broken pipe`) or Go's HTTP/2 client reports that close first. Recent instances, all on the `https` listener: - https://github.com/agent-substrate/substrate/actions/runs/39402878520/job/118561649470 (untrusted CA) - https://github.com/agent-substrate/substrate/actions/runs/39408627974/job/118580615105 (unlisted SPIFFE ID) - https://github.com/agent-substrate/substrate/actions/runs/39410976322/job/118588372580 (no client certificate) - https://github.com/agent-substrate/substrate/actions/runs/39411585756/job/118590529170 (no client certificate) - https://github.com/agent-substrate/substrate/actions/runs/39413905393/job/118598129904 (unlisted SPIFFE ID) **Change.** A refusal that fails without a TLS alert is retried on a fresh port-forward, three times at most. An alert is still required, and a client the router lets through still fails the test. **Tests.** `go vet` and the suite compile; the retry only shows under the flake, which CI's e2e jobs exercise. - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR (none needed)
… data plane (agent-substrate#2686) Adds what it takes to measure ate-api-server's and Postgres's cold resume capacity on the fake data plane from the benchmark automation, plus the first tests that use it. - **boomer: record server-side latency.** `resumepaused` records `locust_server_duration_milliseconds` from the `x-server-elapsed-us` trailer that `ateinterceptors.ServerUnaryInterceptor` sets, next to the client latency. The gap between the two is network and client time. - **Scrape boomer in automation runner Jobs.** The runner container declares port 8001 and `monitoring.yaml`'s `boomer-worker` job keeps `substrate-benchmark-runner` pods, so automated runs get the same latency histograms as manual ones. - **Shape the control plane for capacity runs.** A locust test can carry a `controlPlane:` block (`benchmarking/automation/controlplane.py`): - `apiServer:` sets ate-api-server's replicas, CPU and memory (requests = limits) and its Postgres pool size. - `postgres:` loads `pg_stat_statements` with a `postgres_exporter` sidecar that Prometheus scrapes, and passes extra Postgres settings. Placement is left to the install: capacity tests install with `--cordon-control-plane`, which keeps ate-api-server and Postgres on their own tainted pools, away from the load. The block patches the installed manifests after the deploy; a TODO in `controlplane.py` records the drift guards that shim still lacks. - **Summarize ladder runs per step, on the runner.** For a ladder run, `benchmarking/locust/ladder_steps.py` reads each step's boundaries from the runner's log, queries the in-cluster Prometheus over the step's steady window, and appends a `ladder_step_NNN` row per step to `stats.jsonl`: throughput, client and server latency, CPU and memory of ate-api-server, Postgres and fake-atelet, and Postgres connections, commits and waits. It is additive, like the server telemetry harvest it follows: a failure is logged and the run's other results are kept. - **Cold resume tests.** `tests.yaml` adds two: - `ateapi_cold_resume_pause_fake_2cpu`: a 100–1500/s cold resume ladder against one 2 CPU replica. - `ateapi_cold_resume_pause_fake_smoke`: the same setup at 50 actors and 20 → 50/s, a quick end-to-end check. Each actor books 10m CPU, since a fake Worker reports its node's allocatable as its capacity and placement must never bind. The benchmarking README explains the per-step rows. **Running them.** The target cluster needs the `ate-control-plane` and `ate-postgres` node pools `--cordon-control-plane` expects, and a node with room for the runner. On current `main` the target cluster's env also needs `SNAPSHOT_LOCATION` (agent-substrate#2544) and `ATE_INGRESS_ALLOWED_SPIFFE_IDS` (agent-substrate#2390), which every install now requires. Addresses agent-substrate#2261 - [x] Tests pass (`go test -race ./internal/benchmarking/...`, the `benchmarking/automation` and `benchmarking/locust` Python tests, `hack/verify-all.sh`) - [x] Appropriate changes to documentation are included in the PR
…gent-substrate#2976) PR agent-substrate#2390 removed the hardcoded `--ingress-allowed-spiffe-ids` flag from `manifests/ate-install/atenet-router.yaml` and made `ingressAllowedSPIFFEIDs` (`ATE_INGRESS_ALLOWED_SPIFFE_IDS` / `--ingress-allowed-spiffe-ids`) required in `ate-setup deploy ate-system` when `--atenet-dataplane=envoy`. `deploy_substrate()` in `benchmarking/automation/orchestrator.py` does not set `ATE_INGRESS_ALLOWED_SPIFFE_IDS`, so automated benchmark runs fail at `deploy_ate_system`. Set `ATE_INGRESS_ALLOWED_SPIFFE_IDS` to `BENCHMARK_RUNNER_PRINCIPAL` alongside `ATE_API_AUTHZ_BOOTSTRAP_OWNERS`, and update the benchmark install docs. Fixes agent-substrate#2975 - [x] Tests pass - [x] Docs updated
TestActorArbitraryPortAccess has been flaking in e2e since the router moved to static-mtls in agent-substrate#2390, e.g. https://github.com/agent-substrate/substrate/actions/runs/39549120410/job/119054007125 client-go closes the whole port-forward when any one connection on it errors: https://github.com/kubernetes/client-go/blob/v0.37.0/tools/portforward/portforward.go#L549-L550 RouterClient shared one CONNECT port-forward for the life of the client. So one reset from the router failed the tunnel in flight and every Connect after it, which then got "connection refused" from the dead local port. agent-substrate#2961 fixed the same thing in ingressauth by forwarding per attempt. <!-- The PR title becomes the release note. Write it for users. --> Why is this needed? (Or link the issue. Open one first for features, API, install or CI changes.) Fixes a trivial test flake bug, test only change. Spotted while reviewing PRs like agent-substrate#2502 <!-- e.g. "AI-assisted, I reviewed and tested it." No AI co-author trailers. --> AI use: AI-assisted, I reviewed it. <!-- Breaking change? Describe what users must do on upgrade and ask for the `breaking-change` label. --> - [x] Tests pass - [x] Docs updated
… next steps From Substrate v0.6.0 the default Envoy ingress router admits only clients that present an allowlisted pod certificate, and ate-setup refuses to deploy it without ATE_INGRESS_ALLOWED_SPIFFE_IDS (agent-substrate/substrate#2390). A v0.6.0 install stopped at "Turn on Substrate" with "at least one of ATE_INGRESS_ALLOWED_SPIFFE_IDS ... must be set". The control-plane deploy now passes the counter demos' driver identities (agent-substrate/substrate#3178), spiffe://cluster.local/ns/ate-demo-counter/sa/default and spiffe://cluster.local/ns/ate-demo-counter-microvm/sa/default, with any IDs the caller exported in ATE_INGRESS_ALLOWED_SPIFFE_IDS appended and duplicates dropped. ate-setup reads the variable as one value, so the installer joins the lists. Both demo IDs are listed because the sandbox runtime and demo are chosen after the control plane is deployed. The pod identity signer fixes the trust domain, so the IDs are known in advance. The control-plane screen shows the allowlist, and the upgrade hand-over exports it. Older trees and the agentgateway router ignore it. The Envoy router also serves only TLS, so the next steps' port-forward to port 80 and plain curl no longer reach an actor. Next steps now follow the router: - agentgateway: unchanged (port-forward, kubectl ate, curl). - Envoy, tree with demos/counter/driver: kubectl logs of the driver, which calls the actor through the router. - Envoy, no driver (v0.6.0): install kubectl ate and create the actor, then a note that callers need an allowlisted pod certificate, linking upstream's router README at the install's commit. The README has a section on the allowlist: the ID format, which workloads need an entry, adding your own clients before or after the install, and the agentgateway alternative. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… next steps From Substrate v0.6.0 the default Envoy ingress router admits only clients that present an allowlisted pod certificate, and ate-setup refuses to deploy it without ATE_INGRESS_ALLOWED_SPIFFE_IDS (agent-substrate/substrate#2390). A v0.6.0 install stopped at "Turn on Substrate" with "at least one of ATE_INGRESS_ALLOWED_SPIFFE_IDS ... must be set". The control-plane deploy now passes the counter demos' driver identities (agent-substrate/substrate#3178), spiffe://cluster.local/ns/ate-demo-counter/sa/default and spiffe://cluster.local/ns/ate-demo-counter-microvm/sa/default, with any IDs the caller exported in ATE_INGRESS_ALLOWED_SPIFFE_IDS appended and duplicates dropped. ate-setup reads the variable as one value, so the installer joins the lists. Both demo IDs are listed because the sandbox runtime and demo are chosen after the control plane is deployed. The pod identity signer fixes the trust domain, so the IDs are known in advance. The control-plane screen shows the allowlist, and the upgrade hand-over exports it. Older trees and the agentgateway router ignore it. The Envoy router also serves only TLS, so the next steps' port-forward to port 80 and plain curl no longer reach an actor. Next steps now follow the router: - agentgateway: unchanged (port-forward, kubectl ate, curl). - Envoy, tree with demos/counter/driver: kubectl logs of the driver, which calls the actor through the router. - Envoy, no driver (v0.6.0): install kubectl ate and create the actor, then a note that callers need an allowlisted pod certificate, linking upstream's router README at the install's commit. The README has a section on the allowlist: the ID format, which workloads need an entry, adding your own clients before or after the install, and the agentgateway alternative.
This PR can be broken into the following parts: