Repository navigation
Increase Envoy dataplane efficiency by adding egress policy cache - #2200
yanavlasov wants to merge 27 commits into
Conversation
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
thx yan!
| An Envoy dynamic module, written in Rust, that runs as an HTTP filter on the | ||
| egress gateway's outer `CONNECT` listener and caches egress policy SNI rules | ||
| per actor certificate in a thread-local LRU cache so repeat `CONNECT` requests | ||
| from the same actor can bypass the `ext_proc` sidecar. |
There was a problem hiding this comment.
can bypass the
ext_procsidecar
I think ext-proc side car is doing more things today (like simple authn), do we need that? Or we are saying if the egress policy is cached authn already happened? If yes, is that secure?
I am not sure we need that authn but just checking
There was a problem hiding this comment.
I've updated readme with clarifications. Yes policy is cached after actor and destination port were authorized.
| per actor certificate in a thread-local LRU cache so repeat `CONNECT` requests | ||
| from the same actor can bypass the `ext_proc` sidecar. | ||
|
|
||
| ## How it works |
There was a problem hiding this comment.
shuold we store only what we care about for that stage of the module vs caching the whole egress policy? e.g a small list of https_hostnames (meaning you mitm those) and tls_hostnames (meaning you passthrough). We dont need all cred injection here etc,
There was a problem hiding this comment.
Right now policy is only SNIs and hostnames for the destination port. However I will add key injection as I will implement timed caching and local enforcement of inner policies, including key injection, in the followup. This should make the inner part more efficient as well and avoid callout per request.
There was a problem hiding this comment.
not sure I follow, the policy has a bunch more things than sni and hostnames right? or you are saying we already store only sni and hostanems destination per port in the cache?
My comment was about "we should not cache the whole egresspolicy object, rather the just what we care about which is snis and hostnames per port"
There was a problem hiding this comment.
ok, I checked the code cache already returns just the list of hosts for the port. So its good. I was confused by the readme that still say "Each CachedPolicy entry stores the serialized egress policy JSON " which reads like the whole EgressPolicy
| (`connection.sha256_peer_certificate_digest`) and `<destination_port>` is the | ||
| destination port extracted from the `:authority` request header. | ||
|
|
||
| ### Request path (`on_request_headers`) |
There was a problem hiding this comment.
isnt this section too detailed? can we just replace with a visual?
There was a problem hiding this comment.
I've edited it a bit. It seems like details are ok.
| `shared_with_upstream: ONCE`, making it available to the inner listener's | ||
| `egress-policy` listener filter. | ||
|
|
||
| ### Response path (`on_response_headers`) |
There was a problem hiding this comment.
same comment. too detailed and easy to get stale
There was a problem hiding this comment.
Shortened it a bit.
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
generally lgtm, a few more comments. also - do we have any small benchmarks that lets us compare how this performs vs without this change? so we know what we gain right away?
There was a problem hiding this comment.
nit but I still think thats really too detailed for a readme and it will get stale pretty quickly. is it more a developing.md more? or contributing.md?
There was a problem hiding this comment.
What would you like me to do?
| // TestActorEgressPolicyCachePerPort sends 5 requests to http://example.net and | ||
| // then 5 requests to https://example.com, verifying that per-port policy | ||
| // caching records 2 cache misses, 8 cache hits, and 2 outer CONNECT ext_proc | ||
| // callouts. |
There was a problem hiding this comment.
nit: Probably too verbose - should we just say it tests caching logic per port? or some one meaningful sentence? If someone changes it without an agent, this comment easily going to be misleading
There was a problem hiding this comment.
Please suggest how to improve this.
| // TestActorEgressDialedPortOverridesHostHeader sends requests to | ||
| // https://example.com/ (default TCP port 443) with "example.com:1" in the Host | ||
| // header and to http://example.net/ (default TCP port 80) with "example.net:2" | ||
| // in the Host header, verifying that the egress gateway dials the TCP port the | ||
| // actor connected to rather than the port from the Host header. | ||
| // Sending 2 requests per target verifies that cached policy correctly sets the | ||
| // destination port filter state. |
There was a problem hiding this comment.
Same comment as above for all function comments
There was a problem hiding this comment.
Please suggest how to improve this.
| # E2E_ENVOY_STATS_FLUSH_ON_ADMIN is substituted in renderAtenetEgressManifest | ||
| stats_flush_on_admin: ${E2E_ENVOY_STATS_FLUSH_ON_ADMIN} |
There was a problem hiding this comment.
This is for e2e, is there a way we dont pollute actual "production" config files with some e2e vars?
There was a problem hiding this comment.
I'm open to suggestions here. This needs to be in the bootstrap config. The options are either to have a dedicated config for e2e, use format substitution. This can not even be solved by xDS since this is bootstrap option.
There was a problem hiding this comment.
Just dug a bit about this, and I do not think Kustomize can change an specific value in Envoy yaml in the ConfigMap. Unless I duplicate the entire Envoy config in the Kustomize patch. But then I would need to keep them in sync which is error prone.
If I'm wrong, please let me know.
There was a problem hiding this comment.
I think we dont need stats_flush_on_admin. IIUC Flush-on-admin only pushes to stats sinks and this config has no sinks. So we can remove it if thats correct
For concurrency, its a deployment level config so Kustomize can do.
# manifests/ate-install/components/egress-envoy-single-worker/kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1alpha1
kind: Component
patches:
- target: {group: apps, version: v1, kind: Deployment, name: atenet-egress}
patch: |-
- op: add
path: /spec/template/spec/containers/0/args/-
value: --concurrency
- op: add
path: /spec/template/spec/containers/0/args/-
value: "1" // OR take is from the env var| per actor certificate in a thread-local LRU cache so repeat `CONNECT` requests | ||
| from the same actor can bypass the `ext_proc` sidecar. | ||
|
|
||
| ## How it works |
There was a problem hiding this comment.
not sure I follow, the policy has a bunch more things than sni and hostnames right? or you are saying we already store only sni and hostanems destination per port in the cache?
My comment was about "we should not cache the whole egresspolicy object, rather the just what we care about which is snis and hostnames per port"
We have before benchmark results. Yufan will run benchmark after this PR is submitted. |
Envoy does not cache the whole policy at this point. Only what is needed for the listener filter in the inner filter chain to decide if this MITM, passthrough or plain text traffic. |
Signed-off-by: Yan Avlasov <yavlasov@google.com>
| Envoy worker thread). Each `CachedPolicy` entry stores the serialized egress | ||
| policy JSON string alongside the `Instant` (`stored_at`) when it was cached, |
There was a problem hiding this comment.
| Envoy worker thread). Each `CachedPolicy` entry stores the serialized egress | |
| policy JSON string alongside the `Instant` (`stored_at`) when it was cached, | |
| Envoy worker thread). Each `CachedPolicy` entry stores the SNI rules ext_proc | |
| returned for that port (the hostnames of its `https` and `tls_passthrough` | |
| rules, marked `mitm` or `passthrough`) and when they were cached, |
| // TestActorEgressPolicyCache sends 5 requests to example.com and checks that | ||
| // the egress policy cache counters record 4 hits and 1 miss, and that the | ||
| // outer CONNECT ext_proc filter is invoked only once. |
There was a problem hiding this comment.
| // TestActorEgressPolicyCache sends 5 requests to example.com and checks that | |
| // the egress policy cache counters record 4 hits and 1 miss, and that the | |
| // outer CONNECT ext_proc filter is invoked only once. | |
| // TestActorEgressPolicyCache checks that repeat CONNECTs from one actor to the | |
| // same port skip the ext_proc callout. |
| // TestActorEgressPolicyCachePerPort sends 5 requests to http://example.net and | ||
| // then 5 requests to https://example.com, verifying that per-port policy | ||
| // caching records 2 cache misses, 8 cache hits, and 2 outer CONNECT ext_proc | ||
| // callouts. |
There was a problem hiding this comment.
| // TestActorEgressPolicyCachePerPort sends 5 requests to http://example.net and | |
| // then 5 requests to https://example.com, verifying that per-port policy | |
| // caching records 2 cache misses, 8 cache hits, and 2 outer CONNECT ext_proc | |
| // callouts. | |
| // TestActorEgressPolicyCachePerPort checks that the cache is keyed by | |
| // destination port. |
| // TestActorEgressDialedPortOverridesHostHeader sends requests to | ||
| // https://example.com/ (default TCP port 443) with "example.com:1" in the Host | ||
| // header and to http://example.net/ (default TCP port 80) with "example.net:2" | ||
| // in the Host header, verifying that the egress gateway dials the TCP port the | ||
| // actor connected to rather than the port from the Host header. | ||
| // Sending 2 requests per target verifies that cached policy correctly sets the | ||
| // destination port filter state. |
There was a problem hiding this comment.
| // TestActorEgressDialedPortOverridesHostHeader sends requests to | |
| // https://example.com/ (default TCP port 443) with "example.com:1" in the Host | |
| // header and to http://example.net/ (default TCP port 80) with "example.net:2" | |
| // in the Host header, verifying that the egress gateway dials the TCP port the | |
| // actor connected to rather than the port from the Host header. | |
| // Sending 2 requests per target verifies that cached policy correctly sets the | |
| // destination port filter state. | |
| // TestActorEgressDialedPortOverridesHostHeader checks that the gateway dials | |
| // the port the actor connected to, not the Host header's port, on both a cache | |
| // miss and a hit. |
This PR implements egress policy cache in Envoy dataplane at CONNECT termination. It reduces number of callouts for policy retrievals from the same actor to the same destination port.
Cache for egress policy enforcement on the inner connection is in the followup PR.