egress: add egress gateway support - atenet-egress - #693
Conversation
305b232 to
598a548
Compare
| # Envoy calls it over localhost to authenticate actor identity against the | ||
| # ate API on every CONNECT. This mirrors the ingress gateway topology | ||
| # (Envoy + ext_proc in one pod); a shared/standalone ext_proc is a future step. | ||
| - name: ext-proc |
There was a problem hiding this comment.
Let us make ext-proc a native sidecar (initContainers + restartPolicy: Always).
Regular containers get SIGTERM together, ext_proc has no drain and will exit first,
and with failure_mode_allow: false every CONNECT arriving during Envoy's drain
window fails closed with a 503.
There was a problem hiding this comment.
good point. created an issue for that - #760
There was a problem hiding this comment.
I've actually opened a PR to handle the graceful termination of the atenet-router (#774, which should help mitigate the Envoy drain window issue mentioned here.
There was a problem hiding this comment.
You can also just have ext-proc register the SIGTERM but not quit (alternatively)
| # Co-located ext_proc server (the atenet router, ext_proc-only). The egress | ||
| # Envoy calls it over localhost to authenticate actor identity against the | ||
| # ate API on every CONNECT. This mirrors the ingress gateway topology | ||
| # (Envoy + ext_proc in one pod); a shared/standalone ext_proc is a future step. |
There was a problem hiding this comment.
a shared/standalone ext_proc is a future step: can we first move the exe_proc logic into a separate binary?
There was a problem hiding this comment.
ext_proc is already separate
There was a problem hiding this comment.
I don't think so. See Line 268 in this file:
image: ko://github.com/agent-substrate/substrate/cmd/atenet
There was a problem hiding this comment.
with what we have now - atenet == extproc server
atenet-egress deployment == envoy + atenet (handling egress releated extproc).
atenet-ingress deployment == envoy + atenet(handling ingress releated extproc).
|
implementation itself looks ok, but would like to fix a few organization things:
probably something like: |
Bowei Du (bowei)
left a comment
There was a problem hiding this comment.
You need to scrub through all of the comments -- there seems to some internal notes to yourself and your friend Claude.
|
Needs rebase |
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
Offline we discussed cleaning up the locations in which the egressgateway address needs to be set, right now it's set on quite a few pods, but we can definitely simplify.
| // assigned, and atunnel documents it as a lower bound on trustworthy actor | ||
| // metadata. If our authoritative view is older than what the worker asserts, | ||
| // we cannot yet vouch for the identity, so reject rather than allow blindly. | ||
| if assertedVersion != "" { |
There was a problem hiding this comment.
Will not be necessary once my PR goes in, this version is removed
|
Here is the list of specific PEP todos as a result of #708 |
c133167 to
b4bc40a
Compare
b4bc40a to
acc96e7
Compare
| # SEE(lior): this pair is how the actor's certificate reaches the | ||
| # ext_proc handler. ext_proc can request Envoy attributes, but none | ||
| # of them carry a custom X.509 extension, so the only way to check | ||
| # ActorIdentity in Go is to have Envoy hand over the raw chain. | ||
| # SANITIZE_SET drops whatever x-forwarded-client-cert the client | ||
| # sent and writes Envoy's own view of the verified peer, and | ||
| # chain: true puts the full URL-encoded PEM chain in it. |
There was a problem hiding this comment.
verify this is what we want
There was a problem hiding this comment.
It would be ideal to have a cleaner solution ... and ideally not in the config itself, but we should not block on getting it working.
There was a problem hiding this comment.
verified with Boteng. This is ~fine. Added a TODO. Dont think an issue is needed since "cleaner" is undefined yet.
9bdfc39 to
ae9b6e9
Compare
|
Probably closes #338 |
Bowei Du (bowei)
left a comment
There was a problem hiding this comment.
Mostly looks ok.
| # SEE(lior): this pair is how the actor's certificate reaches the | ||
| # ext_proc handler. ext_proc can request Envoy attributes, but none | ||
| # of them carry a custom X.509 extension, so the only way to check | ||
| # ActorIdentity in Go is to have Envoy hand over the raw chain. | ||
| # SANITIZE_SET drops whatever x-forwarded-client-cert the client | ||
| # sent and writes Envoy's own view of the verified peer, and | ||
| # chain: true puts the full URL-encoded PEM chain in it. |
There was a problem hiding this comment.
It would be ideal to have a cleaner solution ... and ideally not in the config itself, but we should not block on getting it working.
| # hack/install-ate-kind.sh --deploy-demo-egress | ||
| # | ||
| # The egress demo Actor accepts {"url":"..."} and performs an HTTP GET. With | ||
| # egress turned on (ate-api-server --egress-gateway-address, which ateapi stamps |
There was a problem hiding this comment.
probably detail is no needed and could change in the future, can be confusing for humans and agents.
There was a problem hiding this comment.
I believe you meant for the thing in the "()". Removed.
|
|
||
| // Confirm the certified actor still exists. The name is only a lookup key | ||
| // here; the UID below is what actually authorizes. | ||
| actor, err := h.apiClient.GetActor(ctx, &ateapipb.GetActorRequest{ |
There was a problem hiding this comment.
we should be careful here as this results in load to the ATEAPI server
file a TODO as a sub issue of the actor state caching issue
There was a problem hiding this comment.
added a ref to this issue here
| // is ever loosened, and costs one signature check per CONNECT rather than per | ||
| // request. The IsCA, ClientAuth-EKU, and purpose checks below have no Envoy-side | ||
| // equivalent at all. | ||
| func (h *Handler) verifyActorCertificate(chain []*x509.Certificate) (*substratex509.ActorIdentity, error) { |
There was a problem hiding this comment.
put this and the functions it calls in its own package
egress/cert
cert.VerifyActor(...)
If a func doesn't really need to be a method, you should make it a free func for easier testing.
There was a problem hiding this comment.
this PR has grew very big (partly because I addressed a similar comment from you to split ingress + egress under router). Lets take this as a follow up
fbddff3 to
6ec3e03
Compare
The gateway terminates actor's CONNECT request. It requires downstream mTLS, so only a worker's atunnel can reach it. The gateway consists of an Envoy and an `atenet router --standalone` ext_proc sidecar.
demos/egress is a small Actor that fetches a URL it is given and echoes the upstream status and body back, which makes the egress path observable from outside the sandbox. hack/install-demo-egress.sh registers it as a --deploy-demo-egress fixture and hack/verify-egress-demo.sh drives it and checks the atenet-egress logs for the corresponding authorized CONNECT. TestActorEgress in the networking suite covers the same path automatically: it creates an Actor from the demo template, POSTs a fetch request through atenet-router, and asserts 200. The suite's actor helper is parameterised by template so the ingress test keeps using the counter fixture.
The gateway access-log assertion read the log once, immediately after curl
returned, and failed if it saw nothing new. That is racy twice over:
* Envoy emits the CONNECT access-log entry asynchronously, so for an
external destination it can land seconds after the actor's response.
* The Actor's HTTP client keeps the tunnel alive. A repeat fetch to a host
it already reached rides the open tunnel and produces no new entry at
all, so a run against a warm actor failed even though egress was working.
Poll for the new entry, and fall back to any tunnel already open for this
actor's SAN before declaring failure. Also read -c envoy explicitly (the pod
also runs the ext-proc sidecar) and mark the script executable, as every other
directly-invoked script in hack/ is.
6ec3e03 to
da323ec
Compare
| // egress CONNECTs, and so the one that selects the egress handler. It must | ||
| // stay in sync with the filter chain name in | ||
| // manifests/ate-install/atenet-egress.yaml. | ||
| EgressFilterChainName = "egress" |
There was a problem hiding this comment.
Take this as a follow-up: it should be named something more unique to give a clue that there is something special about the name. Ideally, git grep <name> should just yield this and the configuration location.
|
There are a number of follow ups. Let's make sure we get them in as well. |
|
Bowei Du (@bowei) if we don't have clear logical commits, let's squash merge? |
Added pluggable egress PEP support. (Feedback on atenet-egress name is welcomed - will open a separate PR to rename atenet-router to atenet-ingress )
#559 shipped the actor egress data path without any egress gateway. This adds an Envoy deployment that terminates actor CONNECTs. It requires downstream mTLS, so only a worker's atunnel can reach it.
The gateway consists of an Envoy and an
atenet router --standaloneext_proc sidecar.The same ext_proc binary now can serve both directions. It has a mode=ingress|egress|all.
We should remove this when egress API pr is merged
One more FYI
The egress listener's is pinned to the actor-identity CA root alone, so the trust anchor is a property of the filter chain rather than the request, any client certificate not signed by the actor-identity CA fails the TLS handshake, and only traffic with actor-identities can open a tunnel through the egress gateway.