Skip to content

SSL configuration overwrites other WebClient customization#35914

Closed
fcappi wants to merge 2 commits intospring-projects:mainfrom
fcappi:patch-1
Closed

SSL configuration overwrites other WebClient customization#35914
fcappi wants to merge 2 commits intospring-projects:mainfrom
fcappi:patch-1

Conversation

@fcappi
Copy link
Contributor

@fcappi fcappi commented Jun 15, 2023 •
edited by mhalbritter
Loading

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

pivotal-cla commented Jun 15, 2023

@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

pivotal-cla commented Jun 15, 2023

@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

scottfrederick commented Jun 15, 2023

@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
@fcappi @philwebb
Apply SslConfigurer in addition to configured mappers
e6b5322 
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 added a commit that referenced this pull request Jun 21, 2023
@philwebb
Polish 'Apply SslConfigurer in addition to configured mappers'
3c7fbf3 
See gh-35914
@philwebb philwebb closed this in eb91a92 Jun 21, 2023
philwebb added a commit that referenced this pull request Jun 21, 2023
@philwebb
Fix checkstyle violation
7a2fc9e 
See gh-35914
@philwebb
Copy link
Member

philwebb commented Jun 21, 2023

Thanks very much for contributing @fcappi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@philwebb philwebb

Labels

for: merge-with-amendments Needs some changes when we merge type: bug A general bug

Projects

None yet

Milestone

3.1.1

Development

Successfully merging this pull request may close these issues.

5 participants

@fcappi @pivotal-cla @scottfrederick @philwebb @spring-projects-issues