More concise port and TLS configuration #1051

trustin · 2018-03-06T01:54:56Z

Motivation:

A user currently has to specify SessionProtocol when adding a port,
which increases the chance of specifying incorrect SessionProtocol.
sslContext() builder methods require SessionProtocol which is
practically unncessary.
It would be confusing if Server serves plaintext HTTP when a user
configured TLS even if he or she did not specify a port. More sensible
behavior would be serving HTTPS when no port is specified in this case.

Modifications:

Add http() and https() which supercede most port() methods.
Add tls() which supercede sslContext() methods.
Serve HTTPS by default when a user called tls() and no port was
specified.
Add log messages that indicates the protocol and local address.
Deprecate the old methods.
Update documentation.
Miscellaneous:
- Remove redundant port(0, HTTP) calls
- Add @Nullable where necessary

Result:

Fixes ServerBuilder.port() raises an exception when H1/H1C/H2/H2C is specified #1050
More user-friendliness

codecov · 2018-03-06T02:31:16Z

Codecov Report

Merging #1051 into master will decrease coverage by 0.03%.
The diff coverage is 79.48%.

@@            Coverage Diff             @@
##           master    #1051      +/-   ##
==========================================
- Coverage   72.31%   72.27%   -0.04%     
==========================================
  Files         507      507              
  Lines       22119    22154      +35     
  Branches     2705     2709       +4     
==========================================
+ Hits        15995    16012      +17     
- Misses       4715     4727      +12     
- Partials     1409     1415       +6

Impacted Files	Coverage Δ
...java/com/linecorp/armeria/server/ServerConfig.java	`44.62% <ø> (-1.54%)`	⬇️
...n/java/com/linecorp/armeria/server/ServerPort.java	`43.75% <ø> (ø)`	⬆️
...ecorp/armeria/spring/ArmeriaAutoConfiguration.java	`61.11% <60%> (+0.31%)`	⬆️
...orp/armeria/server/AbstractVirtualHostBuilder.java	`57.4% <70%> (-2.21%)`	⬇️
.../main/java/com/linecorp/armeria/server/Server.java	`69.41% <80%> (-0.78%)`	⬇️
...ava/com/linecorp/armeria/server/ServerBuilder.java	`72.63% <83.01%> (-2.37%)`	⬇️
...necorp/armeria/internal/grpc/HttpStreamReader.java	`82.43% <0%> (-4.06%)`	⬇️
...m/linecorp/armeria/client/HttpResponseDecoder.java	`82.05% <0%> (-2.57%)`	⬇️
...linecorp/armeria/client/HttpRequestSubscriber.java	`62.87% <0%> (-2.28%)`	⬇️
...corp/armeria/common/logging/DefaultRequestLog.java	`78.68% <0%> (-1.02%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f96772f...8df57d6. Read the comment docs.

hyangtack · 2018-03-06T02:18:18Z

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

+     *                               {@link #defaultVirtualHost(VirtualHost)} already
+     */
+    public ServerBuilder tls(
+            File keyCertChainFile, File keyFile, String keyPassword) throws SSLException {


@Nullable on keyPassword?

hyangtack · 2018-03-06T02:31:19Z

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

+
+    /**
+     * Adds an HTTPS port that listens on all available network interfaces. If no HTTPS port is added,
+     * (i.e. no {@code https()} method is called), HTTPS support is disabled.


Just curious. What if a user called tls() method? I'm not sure but what's better behavior? Listening on randomly-assigned HTTPS port or disabling HTTPS or raising an exception? (Actually the current behavior is also OK to me)

That's a good question. Refactored a little bit to default to HTTPS when tls() was called and added documentation about the behavior.

anuraaga

Agree that defaulting https on if tls is called would make sense, but either way LGTM

trustin · 2018-03-06T03:27:34Z

@anuraaga Now defaulting to HTTPS when tls() was called. Please review again.

anuraaga · 2018-03-06T03:29:35Z

core/src/main/java/com/linecorp/armeria/server/Server.java

@@ -653,6 +631,14 @@ public void operationComplete(ChannelFuture f) throws Exception {
                    primaryActivePort = actualPort;
                }

+                if (localAddress.getAddress().isAnyLocalAddress() ||
+                    localAddress.getAddress().isLoopbackAddress()) {
+                    logger.info("Serving {} at {} - {}://127.0.0.1:{}/",


This is a good change :) FWIW, it might be considered a breaking change as it will result in double-logging for production users of armeria who almost certainly are doing the logging themselves.

I'm curious if we should log at DEBUG level.

Let me just leave this as it is for the sake of beginners' convenience. :-)

Yeah I think it's good to use INFO but should probably mention in the release notes.

anuraaga · 2018-03-06T03:30:33Z

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

+ * <h2 id="no_port_specified">What happens if no HTTP(S) port is specified?</h2>
+ *
+ * <p>When no TCP/IP port number or local address is specified, {@link ServerBuilder} will automatically bind
+ * to a random TCP/IP port assigned by O/S. It will serve HTTPS if you configured TLS (or HTTP otherwise), e.g.


by the O/S (or maybe OS, I'm not used to seeing a slash there)

Motivation: - A user currently has to specify SessionProtocol when adding a port, which increases the chance of specifying incorrect SessionProtocol. - sslContext() builder methods require SessionProtocol which is practically unncessary. - It would be confusing if Server serves plaintext HTTP when a user configured TLS even if he or she did not specify a port. More sensible behavior would be serving HTTPS when no port is specified in this case. Modifications: - Add http() and https() which supercede most port() methods. - Add tls() which supercede sslContext() methods. - Serve HTTPS by default when a user called tls() and no port was specified. - Add log messages that indicates the protocol and local address. - Deprecate the old methods. - Update documentation. - Miscellaneous: - Remove redundant `port(0, HTTP)` calls - Add `@Nullable` where necessary Result: - Fixes line#1050 - More user-friendliness

trustin · 2018-03-06T11:35:30Z

Thanks, reviewers!

Motivation: - A user currently has to specify SessionProtocol when adding a port, which increases the chance of specifying incorrect SessionProtocol. - sslContext() builder methods require SessionProtocol which is practically unncessary. - It would be confusing if Server serves plaintext HTTP when a user configured TLS even if he or she did not specify a port. More sensible behavior would be serving HTTPS when no port is specified in this case. Modifications: - Add http() and https() which supercede most port() methods. - Add tls() which supercede sslContext() methods. - Serve HTTPS by default when a user called tls() and no port was specified. - Add log messages that indicates the protocol and local address. - Deprecate the old methods. - Update documentation. - Miscellaneous: - Remove redundant `port(0, HTTP)` calls - Add `@Nullable` where necessary Result: - Fixes line#1050 - More user-friendliness

trustin added new feature deprecation labels Mar 6, 2018

trustin added this to the 0.59.0 milestone Mar 6, 2018

trustin requested review from imasahiro, anuraaga, hyangtack and minwoox March 6, 2018 01:54

trustin force-pushed the ports branch 4 times, most recently from c06046b to f7e74ef Compare March 6, 2018 02:03

hyangtack reviewed Mar 6, 2018

View reviewed changes

trustin force-pushed the ports branch from f7e74ef to f4c4578 Compare March 6, 2018 02:33

anuraaga approved these changes Mar 6, 2018

View reviewed changes

trustin force-pushed the ports branch from f4c4578 to 9f64d7f Compare March 6, 2018 03:26

trustin force-pushed the ports branch from 9f64d7f to 974ed5e Compare March 6, 2018 03:31

anuraaga approved these changes Mar 6, 2018

View reviewed changes

trustin force-pushed the ports branch from 974ed5e to 90c0a81 Compare March 6, 2018 03:35

hyangtack approved these changes Mar 6, 2018

View reviewed changes

trustin force-pushed the ports branch from 90c0a81 to 8df57d6 Compare March 6, 2018 03:39

imasahiro approved these changes Mar 6, 2018

View reviewed changes

minwoox approved these changes Mar 6, 2018

View reviewed changes

trustin merged commit 53aa23d into line:master Mar 6, 2018

trustin deleted the ports branch March 6, 2018 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More concise port and TLS configuration #1051

More concise port and TLS configuration #1051

trustin commented Mar 6, 2018 •

edited

Loading

codecov bot commented Mar 6, 2018 •

edited

Loading

hyangtack Mar 6, 2018

trustin Mar 6, 2018

hyangtack Mar 6, 2018

trustin Mar 6, 2018

anuraaga left a comment

trustin commented Mar 6, 2018

anuraaga Mar 6, 2018

trustin Mar 6, 2018

trustin Mar 6, 2018

anuraaga Mar 6, 2018

anuraaga Mar 6, 2018

trustin Mar 6, 2018

trustin commented Mar 6, 2018

More concise port and TLS configuration #1051

More concise port and TLS configuration #1051

Conversation

trustin commented Mar 6, 2018 • edited Loading

codecov bot commented Mar 6, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

trustin commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin commented Mar 6, 2018

trustin commented Mar 6, 2018 •

edited

Loading

codecov bot commented Mar 6, 2018 •

edited

Loading