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.

Sign up for GitHub

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

Jump to bottom

Add support for PROXY protocol #278

Merged
merged 9 commits into from Aug 25, 2021
Merged

Add support for PROXY protocol #278

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e6da211
Add ProxyConnectionHeader
bob-barrett Jun 24, 2021
d097e81
Merge branch 'master' into proxy-protocol
bob-barrett Jun 30, 2021
5b4b68c
Merge branch 'master' into proxy-protocol
bob-barrett Aug 17, 2021
4dc9100
Add config
bob-barrett Aug 18, 2021
04e0bb2
Default to true temporarily for testing
bob-barrett Aug 18, 2021
a6d64f9
PR feedback
bob-barrett Aug 22, 2021
39bfab5
Add tests for creating an ApplicationServer with PROXY protocol enabled
bob-barrett Aug 23, 2021
a661447
Add check that ProxyConnectionFactory was added
bob-barrett Aug 23, 2021
b40cc9e
PR feedback
bob-barrett Aug 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 44 additions & 10 deletions core/src/main/java/io/confluent/rest/ApplicationServer.java
View file
Open in desktop
Expand Up @@ -27,10 +27,12 @@
import org.eclipse.jetty.http2.server.HTTP2ServerConnectionFactory;
import org.eclipse.jetty.http2.server.HTTP2CServerConnectionFactory;
import org.eclipse.jetty.jmx.MBeanContainer;
import org.eclipse.jetty.server.ConnectionFactory;
import org.eclipse.jetty.server.Handler;
import org.eclipse.jetty.server.HttpConfiguration;
import org.eclipse.jetty.server.HttpConnectionFactory;
import org.eclipse.jetty.server.NetworkTrafficServerConnector;
import org.eclipse.jetty.server.ProxyConnectionFactory;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.SslConnectionFactory;
import org.eclipse.jetty.server.handler.ContextHandlerCollection;
Expand Down Expand Up @@ -416,6 +418,9 @@ private void configureConnectors(SslContextFactory sslContextFactory) {
final boolean http2Enabled = isJava11Compatible()
&& config.getBoolean(RestConfig.HTTP2_ENABLED_CONFIG);

final boolean proxyProtocolEnabled =
config.getBoolean(RestConfig.PROXY_CONNECTION_FACTORY_ENABLED_CONFIG);

@SuppressWarnings("deprecation")
List<NamedURI> listeners = parseListeners(
config.getList(RestConfig.LISTENERS_CONFIG),
Expand All @@ -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);
Comment on lines +432 to +433
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing practice in this file at least seems to be mostly multiple parameters per line, so I'm going to stick with that

}
}

private void addConnectorForListener(HttpConfiguration httpConfiguration,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

HttpConnectionFactory httpConnectionFactory,
NamedURI listener,
boolean http2Enabled) {
boolean http2Enabled,
boolean proxyProtocolEnabled) {
ArrayList<ConnectionFactory> connectionFactories = new ArrayList<>();
NetworkTrafficServerConnector connector;

if (http2Enabled) {
Expand All @@ -441,9 +449,13 @@ private void addConnectorForListener(HttpConfiguration httpConfiguration,
final HTTP2CServerConnectionFactory h2cConnectionFactory =
new HTTP2CServerConnectionFactory(httpConfiguration);

if (proxyProtocolEnabled) {
connectionFactories.add(new ProxyConnectionFactory(httpConnectionFactory.getProtocol()));
}

// The order of HTTP and HTTP/2 is significant here but it's not clear why :)
connector = new NetworkTrafficServerConnector(this, null, null, null, 0, 0,
httpConnectionFactory, h2cConnectionFactory);
connectionFactories.add(httpConnectionFactory);
connectionFactories.add(h2cConnectionFactory);
} else {
final HTTP2ServerConnectionFactory h2ConnectionFactory =
new HTTP2ServerConnectionFactory(httpConfiguration);
Expand All @@ -454,11 +466,19 @@ private void addConnectorForListener(HttpConfiguration httpConfiguration,
SslConnectionFactory sslConnectionFactory = new SslConnectionFactory(sslContextFactory,
alpnConnectionFactory.getProtocol());

connector = new NetworkTrafficServerConnector(this, null, null, null, 0, 0,
sslConnectionFactory, alpnConnectionFactory, h2ConnectionFactory,
httpConnectionFactory);
if (proxyProtocolEnabled) {
connectionFactories.add(new ProxyConnectionFactory(sslConnectionFactory.getProtocol()));
}

connectionFactories.add(sslConnectionFactory);
connectionFactories.add(alpnConnectionFactory);
connectionFactories.add(h2ConnectionFactory);
connectionFactories.add(httpConnectionFactory);
}

connector = new NetworkTrafficServerConnector(this, null, null, null, 0, 0,
connectionFactories.toArray(new ConnectionFactory[0]));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// In Jetty 9.4.37, there was a change in behaviour to implement RFC 7230 more
// rigorously and remove support for ambiguous URIs, such as escaping
// . and / characters. While this is a good idea because it prevents clever
Expand All @@ -471,11 +491,25 @@ private void addConnectorForListener(HttpConfiguration httpConfiguration,
} else {
log.info("Adding listener: " + listener);
if (listener.uri.getScheme().equals("http")) {
connector = new NetworkTrafficServerConnector(this, httpConnectionFactory);
if (proxyProtocolEnabled) {
connectionFactories.add(new ProxyConnectionFactory(httpConnectionFactory.getProtocol()));
}

connectionFactories.add(httpConnectionFactory);
} else {
connector = new NetworkTrafficServerConnector(this, httpConnectionFactory,
sslContextFactory);
SslConnectionFactory sslConnectionFactory = new SslConnectionFactory(sslContextFactory,
httpConnectionFactory.getProtocol());

if (proxyProtocolEnabled) {
connectionFactories.add(new ProxyConnectionFactory(sslConnectionFactory.getProtocol()));
}

connectionFactories.add(sslConnectionFactory);
connectionFactories.add(httpConnectionFactory);
}

connector = new NetworkTrafficServerConnector(this, null, null, null, 0, 0,
connectionFactories.toArray(new ConnectionFactory[0]));
}

connector.setPort(listener.getUri().getPort());
Expand Down
13 changes: 13 additions & 0 deletions core/src/main/java/io/confluent/rest/RestConfig.java
View file
Open in desktop
Expand Up @@ -446,6 +446,13 @@ public class RestConfig extends AbstractConfig {
+ "Java 11 JVM or later. Default is true.";
protected static final boolean HTTP2_ENABLED_DEFAULT = true;

public static final String PROXY_CONNECTION_FACTORY_ENABLED_CONFIG =
"proxy.connection.factory.enabled";
bob-barrett marked this conversation as resolved.
Show resolved Hide resolved
protected static final String PROXY_CONNECTION_FACTORY_ENABLED_DOC =
"If true, support PROXY protocol requests using "
+ "org.eclipse.jetty.server.ProxyConnectionFactory";
bob-barrett marked this conversation as resolved.
Show resolved Hide resolved
protected static final boolean PROXY_CONNECTION_FACTORY_ENALBED_DEFAULT = true;
bob-barrett marked this conversation as resolved.
Show resolved Hide resolved

public static ConfigDef baseConfigDef() {
return baseConfigDef(
PORT_CONFIG_DEFAULT,
Expand Down Expand Up @@ -921,6 +928,12 @@ private static ConfigDef incompleteBaseConfigDef() {
LISTENER_PROTOCOL_MAP_DEFAULT,
Importance.LOW,
LISTENER_PROTOCOL_MAP_DOC
).define(
PROXY_CONNECTION_FACTORY_ENABLED_CONFIG,
Type.BOOLEAN,
PROXY_CONNECTION_FACTORY_ENALBED_DEFAULT,
Importance.LOW,
PROXY_CONNECTION_FACTORY_ENABLED_DOC
);
}

Expand Down