[DO NOT MERGE] Upgrade Reactor and Reactor Netty dependencies

[alzimmermsft]

Description

This PR is upgrading Reactor dependencies from 3.4.41 to 3.7.1 and Reactor Netty from 1.0.48 to 1.2.1. This is being done as the previous Reactor and Reactor Netty minor versions, 3.4.x and 1.0.x respectively, are end of life and no longer receiving updates.

The Azure SDKs will continue to perform version validation tests using Reactor versions 3.4.15, 3.5.0, and 3.6.0 and Reactor Netty versions 1.0.0 and 1.1.0 to ensure a baseline compatibility with common ecosystems such as Spring which leverage Reactor as well.

Looking forward into the future, eventually validation for Reactor 3.4.15 and Reactor Netty 1.0.0 can be dropped once Spring Framework 5.3 and Spring Boot 2.7 reach end of life for commercial support.

• https://endoflife.date/spring-framework
• https://endoflife.date/spring-boot

And possibly other versions could be dropped as well depending on ecosystem support for them in maintained versions.

All SDK Contribution checklist:

• The pull request does not introduce [breaking changes]
• CHANGELOG is updated for new features, bug fixes or other significant changes.
• I have read the contribution guidelines.

General Guidelines and Best Practices

• Title of the pull request is clear and informative.
• There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

• Pull request includes test coverage for the included changes.

[chrisribe]

please check branch, there seems to be more changes than expected.
I see changes to deid TelemetryHelper.java

Never mind delta view lead me to believe it was deid code.

[weidongxu-microsoft]

LGTM to mgmt libs.
There appears CI fails in core, currently?

[alzimmermsft]

LGTM to mgmt libs. There appears CI fails in core, currently?

Yeah, there was a change to Reactor Netty at some point where it began to add Content-Length: 0 automatically to GET and HEAD requests which is causing issues. I'm still trying to resolve that in the best way possible.

[violetagg]

LGTM to mgmt libs. There appears CI fails in core, currently?

Yeah, there was a change to Reactor Netty at some point where it began to add Content-Length: 0 automatically to GET and HEAD requests which is causing issues. I'm still trying to resolve that in the best way possible.

@alzimmermsft Can you point me to the test that passes with Reactor Netty 1.0.48 and fails with Reactor Netty 1.2.1?

[alzimmermsft]

LGTM to mgmt libs. There appears CI fails in core, currently?

Yeah, there was a change to Reactor Netty at some point where it began to add Content-Length: 0 automatically to GET and HEAD requests which is causing issues. I'm still trying to resolve that in the best way possible.

@alzimmermsft Can you point me to the test that passes with Reactor Netty 1.0.48 and fails with Reactor Netty 1.2.1?

Thanks for reaching out on this @violetagg!

I left out some clarifying bits, given I was replying to Weidong. This is 100% our own problem and not an issue, per se, with how Reactor Netty is adding Content-Length: 0 to those types of requests. Over here, to reduce costs and maintain reasonable PR CI times we use a custom tool called Test Proxy which records what we call "session records" when our tests are running against Azure services and when we're running in CI (or PLAYBACK) mode it replays those network recordings with validation such as ensuring HTTP headers remain consistent.

That last sentence is where we had issues with the upgrade, with the change where Content-Length: 0 is being added the matching validation began failing with things like "request contained header Content-Length: 0 but it was missing in the recording". Given we support pluggable HTTP stacks, offering Reactor Netty and OkHttp as GA offerings and the JDK HttpClient and Vert.x as beta offerings, my attempt to add Content-Length: 0 to all recordings didn't work as each has slightly different behaviors. I have a commit I'm going to merge here soon which is a slight twist on the recommendation you provided in reactor/reactor-netty#2900, where I'm tracking whether our HttpRequest contained Content-Length and passing that through the ContextView associated with the Netty HttpRequest managed by Reactor Netty. If we find Content-Length: 0 and the flag indicating we didn't set Content-Length the header gets stripped out.

[violetagg]

LGTM to mgmt libs. There appears CI fails in core, currently?

Yeah, there was a change to Reactor Netty at some point where it began to add Content-Length: 0 automatically to GET and HEAD requests which is causing issues. I'm still trying to resolve that in the best way possible.

@alzimmermsft Can you point me to the test that passes with Reactor Netty 1.0.48 and fails with Reactor Netty 1.2.1?

Thanks for reaching out on this @violetagg!

I left out some clarifying bits, given I was replying to Weidong. This is 100% our own problem and not an issue, per se, with how Reactor Netty is adding Content-Length: 0 to those types of requests. Over here, to reduce costs and maintain reasonable PR CI times we use a custom tool called Test Proxy which records what we call "session records" when our tests are running against Azure services and when we're running in CI (or PLAYBACK) mode it replays those network recordings with validation such as ensuring HTTP headers remain consistent.

That last sentence is where we had issues with the upgrade, with the change where Content-Length: 0 is being added the matching validation began failing with things like "request contained header Content-Length: 0 but it was missing in the recording". Given we support pluggable HTTP stacks, offering Reactor Netty and OkHttp as GA offerings and the JDK HttpClient and Vert.x as beta offerings, my attempt to add Content-Length: 0 to all recordings didn't work as each has slightly different behaviors. I have a commit I'm going to merge here soon which is a slight twist on the recommendation you provided in reactor/reactor-netty#2900, where I'm tracking whether our HttpRequest contained Content-Length and passing that through the ContextView associated with the Netty HttpRequest managed by Reactor Netty. If we find Content-Length: 0 and the flag indicating we didn't set Content-Length the header gets stripped out.

I asked because I don't remember any change in this functionality (we always were adding Content-Length) so I want to be sure that this is not some incompatibility that we missed.

[alzimmermsft]

@violetagg I did some additional investigation and there does seem to be a slight runtime behavior change.

I broke down our usage of Reactor Netty to this very simple reproducer:

public class ReactorNettyContentLength {
    public static void main(String[] args) {
        DisposableServer server = HttpServer.create().host("localhost").handle((request, response) -> {
            request.requestHeaders().forEach(header -> System.out.println(header.getKey() + ": " + header.getValue()));
            return response.status(200).send();
        }).bindNow();

        HttpClient httpClient = HttpClient.create();
        httpClient.request(HttpMethod.GET)
            .uri(server.host() + ":" + server.port())
            .send((request, outbound) -> {
                request.requestHeaders().set("Accept", "application/json");
                return outbound;
            })
            .response()
            .block();
    }
}

When running using our current dependency of Reactor Netty 1.0.48 the following headers are printed by the server handler:

user-agent: ReactorNetty/1.0.48
host: 127.0.0.1:58149
Accept: application/json

When running using Reactor Netty 1.2.1 the following headers are printed:

user-agent: ReactorNetty/1.2.1
host: 127.0.0.1:58062
Accept: application/json
content-length: 0

So, it does appear there is a slight runtime behavioral change with regards to how Content-Length: 0 is added for us as we almost always send Accept with our requests.

I've filed this corresponding issue: reactor/reactor-netty#3603

[kushagraThapar]

@alzimmermsft since this is a big change and will affect cosmos db java sdks and its connectors, can we please not merge this PR until we have also done verification on cosmos SDKs (by running our full test suite)?
We can run them and look into the results, please let us know once this is ready to be merged so that we don't overstep on your current investigation.

[alzimmermsft]

@alzimmermsft since this is a big change and will affect cosmos db java sdks and its connectors, can we please not merge this PR until we have also done verification on cosmos SDKs (by running our full test suite)? We can run them and look into the results, please let us know once this is ready to be merged so that we don't overstep on your current investigation.

Will let you know when this PR gets closer to being merged in @kushagraThapar.

We've identified a few regressions we're working through that we're blocking this PR on at this time. I'd expect this to be ready for merge around the late March / early April timeframe when we're going to begin gearing up for our May releases.

[kushagraThapar]

thanks @alzimmermsft for the update, this inlines with our expectation on the timelines. Looking forward to this upgrade in the May timeline.

This change is meant for May timeline and we will review it closely once this PR is closer to be merged in.