Add support for PROXY protocol #278
Conversation
|
Hey @bob-barrett, @confluentinc It looks like this brave person signed our Contributor License Agreement. 👍 Always at your service, clabot |
| addConnectorForListener(httpConfiguration, httpConnectionFactory, listener, | ||
| http2Enabled, proxyProtocolEnabled); |
There was a problem hiding this comment.
nit: Personally I prefer formatting where either all method parameters are on the same line or each method parameter is on its own line, but unlike kafka-rest, rest-utils doesn't have a well established formatting rule about that, so you don't necessarily need to change this.
There was a problem hiding this comment.
The existing practice in this file at least seems to be mostly multiple parameters per line, so I'm going to stick with that
| connector = new NetworkTrafficServerConnector(this, null, null, null, 0, 0, | ||
| connectionFactories.toArray(new ConnectionFactory[0])); | ||
|
|
There was a problem hiding this comment.
nit: I think this and the similar expression at the end of the non-HTTP2 path can be combined by being moved out of the HTTP2 if-else.
There was a problem hiding this comment.
I originally didn't do this because there's one additional thing that has to happen in the HTTP2 case: https://github.com/confluentinc/rest-utils/blob/master/core/src/main/java/io/confluent/rest/ApplicationServer.java#L470. I've made this change now, and I think it's fine, but I could go either way.
| private static Properties props; | ||
| private static TestRestConfig testConfig; | ||
| private static ApplicationServer<TestRestConfig> server; |
There was a problem hiding this comment.
nit: It seems a bit weird to me to use static fields for per-test data (as opposed to per-class data).
| private File trustStore; | ||
| private File clientKeystore; | ||
| private File serverKeystore; |
There was a problem hiding this comment.
nit: These seem to be redundant, as they are only local to testConnectionFactories and its helper methods.
| @@ -424,14 +429,17 @@ private void configureConnectors(SslContextFactory sslContextFactory) { | |||
| SUPPORTED_URI_SCHEMES, "http"); | |||
|
|
|||
| for (NamedURI listener : listeners) { | |||
| addConnectorForListener(httpConfiguration, httpConnectionFactory, listener, http2Enabled); | |||
| addConnectorForListener(httpConfiguration, httpConnectionFactory, listener, | |||
| http2Enabled, proxyProtocolEnabled); | |||
| } | |||
| } | |||
|
|
|||
| private void addConnectorForListener(HttpConfiguration httpConfiguration, | |||
There was a problem hiding this comment.
Not really a comment to your change, but addConnectorForListener has become really hard to read. We should probably have an early return method that does the connection factories juggling and returns the set of relevant factories, and another one which creates a connector out of these.
This change adds support for the PROXY protocol, using the
ProxyConnectionFactory. Unit tests check that, when this feature is enabled, we can create and communicate with a server using any of the possible combinations of connection factories.