Envoy support (part 1) #734
Conversation
| tcpconns, err := getTCPConcurrentConnectionsPerIP() | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I don't see this used in Envoy template. This was added by Jim as a DoS protection.
There was a problem hiding this comment.
So I dont think envoy has specifically a "max concurrent connections per ip" but it does have some similar things but I'm not sure if this is what we want. From https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/cluster/circuit_breaker.proto
-
max_connections The maximum number of connections that Envoy will make to the upstream cluster. If not specified, the default is 1024.
-
max_pending_requests The maximum number of pending requests that Envoy will allow to the upstream cluster. If not specified, the default is 1024.
-
max_requests The maximum number of parallel requests that Envoy will make to the upstream cluster. If not specified, the default is 1024.
-
max_retries The maximum number of parallel retries that Envoy will allow to the upstream cluster. If not specified, the default is 3.
-
max_connection_pools The maximum number of connection pools per cluster that Envoy will concurrently support at once. If not specified, the default is unlimited. Set this for clusters which create a large number of connection pools.
There was a problem hiding this comment.
I think the default max_connections would work, though for L7(http) we'll need max_requests as well. I suggest actually adding these using the same env_var that getTCPConcurrentConnectionsPerIP() uses. I think it's ok as a default, but being explicit is better and cal let us keep the claim that we have DoS protection. Can you check with Jim, if he's fine with this change as well?
There was a problem hiding this comment.
I can add it in. @jlmorris3827 would the max_connections config option be suitable enough for DoS protection?
|
Also, did you miss Dockerfile for envoy with curl in the PR? |
Yes I did, added it now |
| @@ -0,0 +1,193 @@ | |||
| package nginx // lol | |||
There was a problem hiding this comment.
I suggest putting this in it's own "envoy" package.
There was a problem hiding this comment.
Actually instead see my comment below.
There was a problem hiding this comment.
changed it to proxy, to make things less confusing
| func CreateNginxProxy(client pc.PlatformClient, name, originIP string, ports []dme.AppPort, ops ...Op) error { | ||
| // check to see whether nginx or envoy is needed (or both) | ||
| envoyNeeded, nginxNeeded := CheckProtocols(name, ports) |
There was a problem hiding this comment.
Instead of modifying the behavior of CreateNginxProxy and having to remove TCP support from nginx, it would be better to keep nginx as-is and layer a function on top called CreateProxy that calls CheckProtocols, and then creates two sets of ports (if needed), one without TCP ports that it then passes to the unmodified CreateNginxProxy, and another with only TCP ports that it passes to CreateEnvoyProxy. That way the nginx code remains fully functional if we need it.
What might make sense is to rename this package to "proxies" and keep nginx and envoy in here, and have another file "proxy.go" that has the CreateProxy and CheckProtocols functions which are not specific to either nginx or envoy. Or we could have three separate packages, "proxies" with common stuff, "nginx" and "envoy". This would be the way to go if there were going to be a lot of files in either nginx or envoy, but looks like we only need one each so we lumping them all into one package is fine.
There was a problem hiding this comment.
I did it this way because this is basically just a temporary fix until we port over to envoy entirely. In either method if you wanted to get the TCP functionality of nginx back you would probably just end up reverting back to a previous commit.
| switch p.Proto { | ||
| case dme.LProto_L_PROTO_HTTP: | ||
| needNginx = true | ||
| case dme.LProto_L_PROTO_TCP: |
There was a problem hiding this comment.
I think there could be a problem if the App uses the same port for both UDP and TCP, but that will go away once we shift over completely to envoy, so I think it's ok to ignore it for now.
This includes the first stage of switching over to envoy. This change is for the L4 TCP loadbalancing. L7 and UDP are still being run on nginx. Eventually both of those will also switch over (it looks like they literally just added udp support last week: envoyproxy/envoy#492). When creating the nginx container, if there is a need for TCP, then it will start an envoy container instead. Also renamed some of the globals we have since now its possible for apps to be running either nginx or envoy (or both) to be more load balancer generic. Next steps would be actually scraping the access logs to be able to start tracking session time and bytes sent/received.
There is also an infra portion for the stats side of this here: mobiledgex/edge-cloud-infra#520