Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MySQL and other server-speak-protocols require explicit configuration #339

Closed
briansmith opened this issue Feb 12, 2018 · 12 comments
Closed
Assignees

Comments

@briansmith
Copy link
Contributor

Our protocol detection logic currently only works for client-speaks-first protocols--where the client immediately sends data upon the TCP connection completing. MySQL and some other protocols are server-speaks-first; the client won't send anything on the connection until the server sends something. We should change the protocol detection logic so that it is insensitive to which side speaks first, insofar as that is possible.

Currently in Conduit 0.2 we document that the user must explicitly configure the proxy to turn off L7 features for server-speaks-first protocols. This is counter to our goal to "just work" with zero configuration.

A potential strategy:

  • Assume at first that the underlying service being proxied always speaks one protocol.
  • Assume the underlying service speaks TCP (not HTTP) until we get a positive indication otherwise.
  • When we haven't marked the underlying service as speaking HTTP, as soon as we receive an incoming connection or making an outgoing connection, start the connection to the destination (in the inbound case, this would be the connection to the underlying service; in the outbound case, this would be the connection to the external service). Note that this is insensitive to which side speaks first. Mark the protocol for this as "TBD".
  • If/when a client speaks first on a "TBD" connection, run our HTTP (and eventually TLS) detection logic. If/when that logic indicates HTTP and/or gRPC then reconfigure the connection as a HTTP (and/or gRPC) connection and mark the underlying service as HTTP. In particular, add it to the pool of connections for L7 HTTP load balancing.

/cc @seanmonstar @olix0r @wmorgan

@briansmith briansmith added this to the Conduit 0.3 milestone Feb 12, 2018
@wmorgan
Copy link
Member

wmorgan commented Feb 13, 2018

There's a bunch of usability questions that this raises around automaticity vs convention vs configuration. Tentatively reslotting this for 0.4 to give us time to design and scope.

@olix0r
Copy link
Member

olix0r commented Mar 19, 2018

When we haven't marked the underlying service as speaking HTTP, as soon as we receive an incoming connection or making an outgoing connection, start the connection to the destination (in the inbound case, this would be the connection to the underlying service; in the outbound case, this would be the connection to the external service).

IIUC, this determination would need to be made for each original destination address; and therefore connecting to an HTTP endpoint would always incur a wasted connection establishment:

  1. Local app connect with orig_dst_addr=1.2.3.4:80
  2. We immediately establish a connection to 1.2.3.4:80
  3. We then read data an HTTP preamble from the accepted connection. We determine we need to route it through service discovery.
  4. We throw away connection we made.

Either that, or we'd need some way to offer a connection to the load balancer / connection pool.

This cost may not be that high, but it does seem to be worse in the most common case--especially in the case where we fall back to DNS only to produce the original dst address. I think we'd want some way to avoid that unnecessary latency.

If/when a client speaks first on a "TBD" connection, run our HTTP (and eventually TLS) detection logic.

Is it the case that HTTP/TLS services never write data on the server socket before reading data from the client? It seems that there could be situations where injecting latency into the client could bypass protocol detection (and therefore policy). I assume this approach is fine, but want to double check this point.

@briansmith
Copy link
Contributor Author

We throw away connection we made.

I wasn't proposing this method that involves throwing away a connection.

Either that, or we'd need some way to offer a connection to the load balancer / connection pool.

I'm proposing that every connection starts in the TCP load balancer and then migrates to the HTTP load balancer if/when we detect HTTP traffic.

Is it the case that HTTP/TLS services never write data on the server socket before reading data from the client?

TLS and HTTP are both client-speaks-first protocols.

It seems that there could be situations where injecting latency into the client could bypass protocol detection (and therefore policy). I assume this approach is fine, but want to double check this point.

Nothing here is relying on timeout-like logic so I don't think this is a concern.

@briansmith
Copy link
Contributor Author

briansmith commented Mar 21, 2018

Sean and I discussed this today. Sean pointed out that requiring us to peek at incoming traffic from the server requires us to have a connection to the server, but our routing & load balancing for HTTP is totally designed to avoid making new connections to the server, and these two goals are at odds with each other.

Many thoughts were shared regarding moving this forward. Now I'm less confident we can do this in a 100% zero-configuration way. However, absolute 100% zero configuration has never been a goal.

Strawman "simplest thing that could possibly work" proposal:

  1. Maintain a list of well-known (assigned by IANA) port numbers for protocols we know to be incompatible with automatic protocol detection. If the SO_ORIGINAL_DST port number is on that list, assume that the traffic is NOT HTTP and IS pure TCP, bypassing all HTTP-specific stuff.
  2. Eventually add a mechanism to explicitly indicate what the expected protocol is supposed to be. Details TBD. Maybe Istio-like inference from pod/service port names (when available), maybe annotations in pod definitions, maybe conduit inject flags.
  3. If/when we add policy stuff, the policy will need to include, implicitly or implicitly, protocol identification, and we'll need to figure out the details as part of the policy specification work to ensure this simple port-based logic works in a way that doesn't allow policies to be bypassed.

In particular, this proposal is to NOT try to add any logic like described in the initial comment to detect server-speaks-first protocols, and this proposal is to DISABLE protocol detection for the IANA-assigned port numbers for server-speaks-first protocols.

[edited].

@briansmith
Copy link
Contributor Author

We looked at Istio BookInfo's use of MySQL and it's pod/service definitions are using the IANA-assigned port for MySQL, so the proposal in the previous comment would get the BookInfo example, and probably many other use cases, working without any explicit configuration and with a simple and minimal code change. AFAICT the practical side effects are minimal.

@seanmonstar
Copy link
Contributor

Maintain a list of well-known

A possible expansion of this is to allow our list to be configurable, such that by default, we assume some certain list, and an environment variable could be used to override. This would allow 1) to add a port if not using a well-known one, 2) to disable some ports if using the well-known for HTTP.

@briansmith
Copy link
Contributor Author

allow our list to be configurable

How would the user configure it?

If they would configure it on a per-pod basis (in every conduit inject), I it might be best to require them to instead just provide a protocol name for the given port, instead of just having a "disable HTTP L7 semantics and protocol detection" flag for the port. That way, if/when we get smarter about handling that particular protocol, we can start doing the smart thing using their explicit information given to us. That is, instead of saying "disable the problematic stuff for port 1234" the user would say "port 1234 is protocol foo" and since we don't know what protocol redis is, we'd treat it as some arbitrary tcp protocol. Then in the future when we can do, say, L7 load balancing for redis, we could automatically do that w/o any further configuration changes. Not sure if this is a great idea but it seems good to me.

@seanmonstar
Copy link
Contributor

How would the user configure it?

From the proxy's point of view, it's just an environment variable, so like CONDUIT_PROXY_OUTBOUND_TCP_PORTS=80. Perhaps the user could configure it with inject, like they currently do to disable port forwarding.

it might be best to require them to instead just provide a protocol name for the given port

I do think that's for the best, but we don't yet know how we would communicate that for outbound. Especially in cases where, as you mentioned, they may have their service outside of Kubernetes. For example, putting their MySQL server at mysql.local:80 (because of course they would).

@briansmith
Copy link
Contributor Author

briansmith commented Mar 21, 2018

How would the user configure it?

From the proxy's point of view, it's just an environment variable, so like CONDUIT_PROXY_OUTBOUND_TCP_PORTS=80. Perhaps the user could configure it with inject, like they currently do to disable port forwarding.

it might be best to require them to instead just provide a protocol name for the given port

I do think that's for the best, but we don't yet know how we would communicate that for outbound. Especially in cases where, as you mentioned, they may have their service outside of Kubernetes. For example, putting their MySQL server at mysql.local:80 (because of course they would).

What I meant was something like CONDUIT_PROXY_OUTBOUND_TCP_PORTS=80:mysql,3306:http would mean "My outgoing port 80 is using the MySQL protocol and my outgoing port 3306 is using the HTTP protocol." That is, don't ask for only the port number in CONDUIT_PROXY_OUTBOUND_TCP_PORTS, but also insist on a protocol name be included too.

@olix0r
Copy link
Member

olix0r commented Mar 21, 2018

Maintain a list of well-known (assigned by IANA) port numbers for protocols we know to be incompatible with automatic protocol detection. If the SO_ORIGINAL_DST port number is on that list, assume that the traffic is NOT HTTP and IS pure TCP, bypassing all HTTP-specific stuff.

This sounds like a good short-term fix to me.

I want to note one additional wrinkle: we currently need to skip ports for things like websockets (since the proxy currently fails to upgrade these streams), meaning that if an application communicates via websockets on port 80, we'll have to treat all http traffic as TCP. This is probably okay in the short-term, but just want to note that there are non-server-speaks-first use cases here that may impact usability more broadly.

@briansmith
Copy link
Contributor Author

we currently need to skip ports for things like websockets (since the proxy currently fails to upgrade these streams), meaning that if an application communicates via websockets on port 80, we'll have to treat all http traffic as TCP. This is probably okay in the short-term, but just want to note that there are non-server-speaks-first use cases here that may impact usability more broadly.

Agreed. This issue is about server-speaks-first protocols. #195 is about websockets. I don't think we have one about CONNECT but I think it's similar enough to websockets that we can consider #195 to be about it too.

@seanmonstar
Copy link
Contributor

I've got a branch with this working, minus just one part: naming the ports in the environment variable. I'm not sure what that gets us, besides some additional complexity parsing.

@seanmonstar seanmonstar self-assigned this Mar 29, 2018
seanmonstar added a commit that referenced this issue Mar 29, 2018
- Adds environment variables to configure a set of ports that, when an
  incoming connection has an SO_ORIGINAL_DST with a port matching, will
  disable protocol detection for that connection and immediately start a
  TCP proxy.
- Adds a default list of well known ports: SMTP and MySQL.

Closes #339

Signed-off-by: Sean McArthur <sean@seanmonstar.com>
@olix0r olix0r added the review label Mar 29, 2018
seanmonstar added a commit that referenced this issue Apr 2, 2018
- Adds environment variables to configure a set of ports that, when an
  incoming connection has an SO_ORIGINAL_DST with a port matching, will
  disable protocol detection for that connection and immediately start a
  TCP proxy.
- Adds a default list of well known ports: SMTP and MySQL.

Closes #339
@olix0r olix0r removed the review label Apr 2, 2018
khappucino pushed a commit to Nordstrom/linkerd2 that referenced this issue Mar 5, 2019
- Adds environment variables to configure a set of ports that, when an
  incoming connection has an SO_ORIGINAL_DST with a port matching, will
  disable protocol detection for that connection and immediately start a
  TCP proxy.
- Adds a default list of well known ports: SMTP and MySQL.

Closes linkerd#339
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants