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

SSL configuration overwrites other WebClient customization #35914

Closed
wants to merge 2 commits into from

Conversation

fcappi
Copy link
Contributor

@fcappi fcappi commented Jun 15, 2023

Issue: Whenever you are configuring TLS on a webclient, any other custom configuration of HttpClient is lost and not applied to the final instance.

Example:

  @Bean
  public WebClient webClient(
      ...
    return WebClient.builder().apply(clientSsl.fromBundle(sslBundle)).build();
  }

  @Bean
  public ReactorNettyHttpClientMapper httpClient() {
   // This is not applied to the WebClient instance generated by the builder on webclient()
    return client ->
        client.compress(true)

Check the code being changed, where we can see that mapper is being reassigned to only consider configurations from SslConfigurer. It should be just added to the map stream, so everything is applied

@pivotal-cla
Copy link

@fcappi Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@fcappi Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 15, 2023
@scottfrederick
Copy link
Contributor

@fcappi Thanks for the suggestion. Can you describe the problem that this change is intended to solve? Ideally we'd have tests to verify that the existing behavior is incorrect, and change the tests to match the new behavior.

@scottfrederick scottfrederick added the status: waiting-for-feedback We need additional information before we can continue label Jun 15, 2023
@fcappi fcappi marked this pull request as ready for review June 16, 2023 16:44
@fcappi
Copy link
Contributor Author

fcappi commented Jun 16, 2023

Hey @scottfrederick , sorry for that. I created the PR before it was ready. Details added to the PR description. Tests are covering the expected behaviour.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 16, 2023
@philwebb philwebb added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Jun 16, 2023
@philwebb philwebb added this to the 3.1.x milestone Jun 16, 2023
@philwebb philwebb added the for: merge-with-amendments Needs some changes when we merge label Jun 16, 2023
@scottfrederick scottfrederick changed the title Apply SslConfigurer on previously configured HttpClient SSL configuration overwrites other WebClient customization Jun 16, 2023
@philwebb philwebb modified the milestones: 3.1.x, 3.1.1 Jun 21, 2023
philwebb pushed a commit that referenced this pull request Jun 21, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Update `ReactorClientHttpConnectorFactory` to that SSL configuration
is applied in addition to any configured mappers.

Prior to this commit, SSL configuration would prevent configured
mappers from being applied.

See gh-35914
@philwebb philwebb closed this in eb91a92 Jun 21, 2023
philwebb added a commit that referenced this pull request Jun 21, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
See gh-35914
@philwebb
Copy link
Member

Thanks very much for contributing @fcappi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants