TLS setup as part of channel acquire #3056

dagnir · 2022-02-24T00:36:21Z

Motivation and Context

Fix confusing error behavior where acquisition completes, but then channel appears to be closed as request is made because TLS setup did not complete successfully.

Modifications

This commit correctly factors in the TLS negotiation into the channel pooling
acquire logic; this ensures that when the Netty client successfully acquires a
connection from the channel pool, TLS negotiation is guaranteed to have
completed successfully.

Testing

New unit tests
Running integ tests

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

This commit correctly factors in the TLS negotiation into the channel pooling acquire logic; this ensures that when the Netty client successfully acquires a connection from the channel pool, TLS negotiation is guaranteed to have completed successfully.

...y-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/NettyClientTlsAuthTest.java

...ent/src/main/java/software/amazon/awssdk/http/nio/netty/TlsHandshakeEnsuringChannelPool.java

...y-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/NettyClientTlsAuthTest.java

sonarqubecloud · 2022-02-25T20:51:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
8 Code Smells

96.2% Coverage
0.0% Duplication

Bennett-Lynch · 2022-02-25T21:48:29Z

...ent/src/main/java/software/amazon/awssdk/http/nio/netty/TlsHandshakeEnsuringChannelPool.java

+
+/**
+ * This {@code ChannelPool} ensures that the channel has completed the TLS negotiation before giving it back to the caller. Note
+ * that it's possible for a channel to have multiple {@link SslHandler} instances its pipeline, for example if it's using a


nit/typo: "in its pipeline"

Bennett-Lynch · 2022-02-25T21:51:43Z

...ent/src/main/java/software/amazon/awssdk/http/nio/netty/TlsHandshakeEnsuringChannelPool.java

+ * that it's possible for a channel to have multiple {@link SslHandler} instances its pipeline, for example if it's using a
+ * proxy over HTTPS. This pool explicitly looks for the handler of the given name.
+ * <p>
+ * If TLS setup fails, the channel will be closed and released back to the wrapped pool and the future will be failed.


Should we mention anything about how this will interact w/ retries?

Bennett-Lynch · 2022-02-25T21:53:05Z

...ent/src/main/java/software/amazon/awssdk/http/nio/netty/TlsHandshakeEnsuringChannelPool.java

+ * If TLS setup fails, the channel will be closed and released back to the wrapped pool and the future will be failed.
+ */
+@SdkInternalApi
+public class TlsHandshakeEnsuringChannelPool implements ChannelPool {


nit: "ensuring" vs "awaiting"? (Not deeply opinionated since it's internal API.)

Bennett-Lynch · 2022-02-25T22:02:31Z

...ent/src/main/java/software/amazon/awssdk/http/nio/netty/TlsHandshakeEnsuringChannelPool.java

+        if (!f.isSuccess()) {
+            promise.tryFailure(f.cause());
+            return;
+        }


Should we more explicitly propagate cancellation here? E.g., via NettyUtils#consumeOrPropagate.

Bennett-Lynch · 2022-02-25T22:03:33Z

...ent/src/main/java/software/amazon/awssdk/http/nio/netty/TlsHandshakeEnsuringChannelPool.java

+    }
+
+    private Optional<Future<Channel>> sslHandshakeFuture(Channel ch) {
+        ChannelHandler handlerByName = ch.pipeline().get(sslHandlerName);


Rather than looking by name, would it be simpler to just look for, e.g., the first handler of type SslHandler? Would that be guaranteed to be the outer-most layer? Or can we not make any assumptions about the order?

Bennett-Lynch · 2022-02-25T22:11:23Z

...ent/src/main/java/software/amazon/awssdk/http/nio/netty/TlsHandshakeEnsuringChannelPool.java

+    private Optional<Future<Channel>> sslHandshakeFuture(Channel ch) {
+        ChannelHandler handlerByName = ch.pipeline().get(sslHandlerName);
+        if (!(handlerByName instanceof SslHandler)) {
+            return Optional.empty();


Perhaps could simplify this slightly by also just returning an already-successful future.

Bennett-Lynch · 2022-02-25T22:14:43Z

...y-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/NettyClientTlsAuthTest.java

@@ -205,4 +226,16 @@ private static SdkHttpFullRequest testSdkRequest() {
                .build();
    }

+    private static int jdkVersion() {


Shared test utility, or too premature?

Bennett-Lynch · 2022-02-25T22:15:53Z

...est/java/software/amazon/awssdk/http/nio/netty/fault/ServerConnectivityErrorMessageTest.java

+        return Arrays.asList(new TestCase(CloseTime.DURING_INIT, "Failed TLS connection setup"),
+                             new TestCase(CloseTime.BEFORE_SSL_HANDSHAKE, "Failed TLS connection setup"),
+                             new TestCase(CloseTime.DURING_SSL_HANDSHAKE, "Failed TLS connection setup"),


Nice! This is a big improvement on wording/clarity.

Bennett-Lynch · 2022-02-25T22:24:20Z

...ent/src/main/java/software/amazon/awssdk/http/nio/netty/TlsHandshakeEnsuringChannelPool.java

+                LOGGER.debug(channel, () -> "Failed TLS connection setup. Channel will be closed.", handshake.cause());
+                channel.close();
+                delegate.release(channel);
+                IOException error = new IOException("Failed TLS connection setup: " + channel, handshake.cause());


Should we consider more prescriptive wording if the error was, e.g., a timeout?

zoewangg · 2022-03-10T23:56:29Z

Can we run benchmarks to see how much performance impact this change would cause?

dagnir · 2022-03-25T22:58:25Z

Can we run benchmarks to see how much performance impact this change would cause?

Did a test with the following code

Test code

    public static void main(String[] args) throws InterruptedException {
        CloudWatchMetricPublisher metricPublisher = CloudWatchMetricPublisher.builder()
                .namespace("DongieTlsTesting")
                .cloudWatchClient(CloudWatchAsyncClient.builder()
                        .region(Region.US_WEST_2)
                        .credentialsProvider(EnvironmentVariableCredentialsProvider.create())
                        .build())
                .build();

        int nThreads = Runtime.getRuntime().availableProcessors() * 2;

        ExecutorService exec = Executors.newFixedThreadPool(nThreads);

        System.out.printf("Spinning up %d threads%n", nThreads);
        for (int i = 0; i < nThreads; ++i) {
            exec.submit(() -> {
                while (true) {
                    S3AsyncClient s3 = null;
                    try {
                        // Always create a new client to defeat pooling
                         s3 = S3AsyncClient.builder()
                                .region(Region.US_WEST_2)
                                .credentialsProvider(EnvironmentVariableCredentialsProvider.create())
                                .overrideConfiguration(o -> o.addMetricPublisher(metricPublisher))
                                .build();

                        s3.getObject(r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes()).join();
                    } catch (Exception e) {
                        System.out.printf("ERROR [%s]: %s%n", Thread.currentThread().getName(), e.getMessage());
                    } finally {
                        if (s3 != null) {
                            s3.close();
                        }
                    }
                }
            });
        }

        Thread.sleep(Duration.ofMinutes(15).toMillis());

        System.out.println("Done.");
        exec.shutdown();

Predictably, the ConcurrencyAcquireDuration is higher, 13-14ms vs 7ms, since the complete handshake is being factored in. Interestingly though, both the ServiceCallDuration and ApiCallDuration are lower overall, probably since previously they were including at least part of the TLS handshake:

I'm also pulling this change in internally to test in the canaries.

dagnir · 2022-09-29T20:25:55Z

Closing this for now because of performance regression concerns.

…f4b406b40 Pull request: release <- staging/78627166-ab50-482c-9c30-34bf4b406b40

dagnir force-pushed the ssl-handshake-lease branch 3 times, most recently from 478f176 to c2c0096 Compare February 24, 2022 21:04

dagnir force-pushed the ssl-handshake-lease branch from c2c0096 to 103e9a5 Compare February 24, 2022 22:01

dagnir marked this pull request as ready for review February 24, 2022 22:04

dagnir requested a review from a team as a code owner February 24, 2022 22:04

zoewangg reviewed Feb 25, 2022

View reviewed changes

dagnir added 3 commits February 25, 2022 10:42

Review comments

5db9782

Fix test to check either exception

5c44333

fixup! Fix test to check either exception

9933841

Bennett-Lynch approved these changes Feb 25, 2022

View reviewed changes

Bennett-Lynch reviewed Feb 25, 2022

View reviewed changes

dagnir closed this Sep 29, 2022

aws-sdk-java-automation added a commit that referenced this pull request Jun 4, 2024

Merge pull request #3056 from aws/staging/78627166-ab50-482c-9c30-34b…

5333a5b

…f4b406b40 Pull request: release <- staging/78627166-ab50-482c-9c30-34bf4b406b40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

TLS setup as part of channel acquire #3056

TLS setup as part of channel acquire #3056

Uh oh!

dagnir commented Feb 24, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Feb 25, 2022

Uh oh!

Bennett-Lynch Feb 25, 2022

Uh oh!

Bennett-Lynch Feb 25, 2022

Uh oh!

Bennett-Lynch Feb 25, 2022

Uh oh!

Bennett-Lynch Feb 25, 2022

Uh oh!

Bennett-Lynch Feb 25, 2022

Uh oh!

Bennett-Lynch Feb 25, 2022

Uh oh!

Bennett-Lynch Feb 25, 2022

Uh oh!

Bennett-Lynch Feb 25, 2022

Uh oh!

Bennett-Lynch Feb 25, 2022

Uh oh!

zoewangg commented Mar 10, 2022

Uh oh!

dagnir commented Mar 25, 2022

Uh oh!

dagnir commented Sep 29, 2022

Uh oh!

Uh oh!

TLS setup as part of channel acquire #3056

TLS setup as part of channel acquire #3056

Uh oh!

Conversation

dagnir commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Feb 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoewangg commented Mar 10, 2022

Uh oh!

dagnir commented Mar 25, 2022

Uh oh!

dagnir commented Sep 29, 2022

Uh oh!

Uh oh!

dagnir commented Feb 24, 2022 •

edited

Loading