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

Added support for http/2 to the Monitor web server #4982

Closed
wants to merge 2 commits into from

Conversation

dlmarion
Copy link
Contributor

Added conditional support for http/2 to the Monitor web server. I
investigated adding http/3 support as it's documented, but it appears
that according to the code, it's still experimental and not ready for production. Note that http/2
can be used without TLS, but it appears that when http/2 is used
from a browser that the browser enforces TLS. I fixed MontorSslIT
to test with and without ssl, and with and without http/2.

Closes #4974

@dlmarion dlmarion added this to the 3.1.0 milestone Oct 15, 2024
@dlmarion dlmarion self-assigned this Oct 15, 2024
@dlmarion dlmarion linked an issue Oct 15, 2024 that may be closed by this pull request
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

I'm still looking at the details of the implementation in this PR, but the more I read about HTTP/2, the more I'm thinking this is probably more trouble than it's worth. Browsers (or perhaps most clients) don't typically support HTTP/2 without TLS, so, it adds a lot of complexity to our code (and Jetty code is notoriously frustrating to upgrade across major versions, so complexity is best avoided).

If we are going to do HTTP/2, I suggest these steps first (in separate, incremental PRs):

  1. Upgrade to newest web stack (latest Jetty, Jersey, jakartaee BOM, etc.), then
  2. Force use of TLS instead of making it configurable (use a generated self-signed cert by default, unless user configures one), to eliminate the complexity of supporting both TLS and non-TLS, then
  3. Force use of HTTP/2 (no fallback to HTTP/1.1), so there's no need to configure the extra factory, and no need for the configuration option or extra factory setup (all major browsers and CLI tools, like wget2 and cURL, support HTTP/2, so there's no reason to support HTTP/1.1)

Also, based on a conversation with @dlmarion , I understand that the main motivation for supporting HTTP/2 was to support adding a was to push micrometer metrics to the monitor. We discussed other means of accomplishing that, so I don't think this is really needed anymore, so unless I'm wrong about this, I don't think this alone adds any value to Accumulo.

Comment on lines +871 to +872
MONITOR_SUPPORT_HTTP2("monitor.http2.support", "false", PropertyType.BOOLEAN,
"If true will configure the Monitor web server to support http2 connections.", "3.1.0"),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs to be configurable. All modern browsers support it. If it improves our monitor's performance in any way, it should just be used automatically when we use TLS.

For the technical reasons listed on https://stackoverflow.com/a/46789195/196405 , I suggest not bothering to support it when TLS is off.

Comment on lines +72 to +73
final HttpConfiguration httpConfig = new HttpConfiguration();
httpConfig.setSendServerVersion(false);
Copy link
Member

Choose a reason for hiding this comment

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

This seems unrelated, and only hides the Jetty version. Is that something we want to keep independently of this PR? What's the intention here?

@dlmarion
Copy link
Contributor Author

Also, based on a conversation with @dlmarion , I understand that the main motivation for supporting HTTP/2 was to support adding a was to push micrometer metrics to the monitor. We discussed other means of accomplishing that, so I don't think this is really needed anymore, so unless I'm wrong about this, I don't think this alone adds any value to Accumulo.

If we are able to use the Thrift RPC for getting metrics and logs to the Monitor, then I agree that this is not needed. However, I think that #4879 may have introduced a potential issue, which is highlighted at #4879 (comment).

I'm still looking at the details of the implementation in this PR, but the more I read about HTTP/2, the more I'm thinking this is probably more trouble than it's worth. Browsers (or perhaps most clients) don't typically support HTTP/2 without TLS, so, it adds a lot of complexity to our code (and Jetty code is notoriously frustrating to upgrade across major versions, so complexity is best avoided).

If we are going to do HTTP/2, I suggest these steps first (in separate, incremental PRs):

  1. Upgrade to newest web stack (latest Jetty, Jersey, jakartaee BOM, etc.), then
  2. Force use of TLS instead of making it configurable (use a generated self-signed cert by default, unless user configures one), to eliminate the complexity of supporting both TLS and non-TLS, then
  3. Force use of HTTP/2 (no fallback to HTTP/1.1), so there's no need to configure the extra factory, and no need for the configuration option or extra factory setup (all major browsers and CLI tools, like wget2 and cURL, support HTTP/2, so there's no reason to support HTTP/1.1)

I think there is still a case for HTTP/2 without TLS, even when used with a browser. That case is when SSL termination is done at some proxy, with only http being used behind the proxy. If we still end up needing to do this, then we might need/want to account for this case.

@ctubbsii
Copy link
Member

ctubbsii commented Oct 16, 2024

I think there is still a case for HTTP/2 without TLS, even when used with a browser. That case is when SSL termination is done at some proxy, with only http being used behind the proxy. If we still end up needing to do this, then we might need/want to account for this case.

My understanding is, based on everything I've read, that going through a proxy is specifically the case where HTTP/2 without TLS (h2c) tends to break, and why it's necessary to use only TLS with HTTP/2 for a passthrough proxy (using HTTP CONNECT). My understanding is further that a TLS terminating proxy would necessarily request HTTP/1.1 plaintext, or HTTP/1.1 or HTTP/2 over TLS, depending if it supports TLS termination on both sides, but never HTTP/2 without TLS (h2c).

So, if we want to consider the TLS passthrough proxy case, then we don't need to support non-TLS, and can stick to just HTTP/2 over TLS. If we want to consider the TLS terminating proxy case, then it might support HTTP/2 over TLS if it's new enough (and if it supports TLS termination on both ends), but it definitely would support HTTP/1.1 without TLS. So even in that case, we don't need to support HTTP/2 without TLS (h2c).

I'm not sure the exact situation you're thinking about, but I think it's worth noting that we don't do anything special to support either kinds of proxies today. If we want to continue to support the same kinds of use cases, we just need to continue to support HTTP/1.1 without TLS for the TLS terminating proxy case, and HTTP/2 with TLS would suffice for the TLS passthrough case, since the TCP connection is what is proxied via HTTP CONNECT, at a lower layer than the TLS and HTTP protocols.

@ctubbsii
Copy link
Member

It looks like the the latest HTTP/2 RFC obsoletes the "h2c" upgrade mechanism to upgrade over plaintext HTTP. It also doesn't work over POST. So, if we do HTTP/2 it probably really should be TLS only. So that would be another reason to avoid supporting HTTP/2 with plaintext, and either continue doing HTTP/1.1 for plaintext, or no plain text at all.

@dlmarion
Copy link
Contributor Author

#5012 has implemented a mechanism to pull metrics and other information for the Monitor from the servers. Propogating logs to the Monitor is likely going to be removed, but if not could use the same mechanism being used in #5012. Closing this in favor of #5012.

@dlmarion dlmarion closed this Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuring Monitor for SSL does not disable non-SSL connections
2 participants