armeria icon indicating copy to clipboard operation
armeria copied to clipboard

Allow `useOpenSsl` to be configured by `ClientFactoryBuilder`

Open seonWKim opened this issue 2 years ago โ€ข 1 comments

Motivation:

Let users configure useOpenSsl client option when using ClientFactoryBuilder or ServerBuilder

Example Client

ClientFactory
  .builder()
  .useOpenSsl(false)
  .build();

Server

Server.builder()
           .virtualHost("*.example1.com")
           .service("/example", (ctx, req) -> HttpResponse.of(HttpStatus.OK))
           .tlsSelfSigned()
           .useOpenSsl(true)
           .and()
           .virtualHost("*.example2.com")
           .service("/example", (ctx, req) -> HttpResponse.of(HttpStatus.OK))
           .tlsSelfSigned()
           .useOpenSsl(false)
           .and()
           .virtualHost("*.example3.com")
           .service("/example", (ctx, req) -> HttpResponse.of(HttpStatus.OK))
           .and()
           .build();

Modifications: image

  • Add useOpenSsl method in ClientFactoryBuilder to allow users choose whether to use openSsl.
  • SslContextUtil's createSslContext(...) method is modified to receive useOpenSsl as an argument to determine what SslProvider it should use(OPENSSL or JDK).
  • Users can set useOpenSsl field in ServerBuilder.
  • Add useOpenSsl method in VirtualHostBuilder.

Result:

  • Closes #<4949>. (If this resolves the issue.)
  • Users will be able to configure useOpenSsl using ClientFactoryBuilder or ServerBuilder

seonWKim avatar Jun 15 '23 16:06 seonWKim

I'll start working on this issue again ๐Ÿƒ

seonWKim avatar Apr 09 '24 00:04 seonWKim

๐Ÿ” Build Scanยฎ (commit: 623c075cdb1ef2a5612f24bec8970b82c0807e71)

Job name Status Build Scanยฎ
build-windows-latest-jdk-21 โœ… https://ge.armeria.dev/s/l7w3hspopvzda
build-self-hosted-unsafe-jdk-8 โœ… https://ge.armeria.dev/s/r7kvjv2l63haq
build-self-hosted-unsafe-jdk-21-snapshot-blockhound โœ… https://ge.armeria.dev/s/mjet5u2uxkhuc
build-self-hosted-unsafe-jdk-17-min-java-17-coverage โœ… https://ge.armeria.dev/s/h5fsfcl2o45hw
build-self-hosted-unsafe-jdk-17-min-java-11 โœ… https://ge.armeria.dev/s/quadixnwo52rw
build-self-hosted-unsafe-jdk-17-leak โœ… https://ge.armeria.dev/s/pzyt6kzqfykp6
build-self-hosted-unsafe-jdk-11 โœ… https://ge.armeria.dev/s/66xujxwacfale
build-macos-12-jdk-21 โœ… https://ge.armeria.dev/s/oakkefmfaf7fu

github-actions[bot] avatar May 19 '24 04:05 github-actions[bot]

I've not added useOpenSsl because tlsEngineType should be a better alternative to useOpenSsl.

Please let me know if we should add support for both tlsEngineType and useOpenSsl in the ClientFactoryBuilder.

seonWKim avatar May 19 '24 06:05 seonWKim

Codecov Report

Attention: Patch coverage is 83.33333% with 5 lines in your changes missing coverage. Please review.

Project coverage is 74.10%. Comparing base (14c5566) to head (6438c82). Report is 47 commits behind head on main.

:exclamation: Current head 6438c82 differs from pull request most recent head 623c075

Please upload reports for the commit 623c075 to get more accurate results.

Files Patch % Lines
.../linecorp/armeria/client/ClientFactoryBuilder.java 0.00% 2 Missing :warning:
.../linecorp/armeria/server/ServerSslContextUtil.java 75.00% 1 Missing and 1 partial :warning:
...p/armeria/internal/common/util/SslContextUtil.java 50.00% 1 Missing :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4962      +/-   ##
============================================
+ Coverage     74.05%   74.10%   +0.04%     
- Complexity    21253    21313      +60     
============================================
  Files          1850     1853       +3     
  Lines         78600    78775     +175     
  Branches      10032    10067      +35     
============================================
+ Hits          58209    58376     +167     
+ Misses        15686    15682       -4     
- Partials       4705     4717      +12     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 19 '24 07:05 codecov[bot]

Should we use TlsEngineType specified in VirtualHost? It would be better to just use the given option rather than try to guess.

@ikhoon , I've added a method to expose tlsEngineType and use that method instead of trying to guess.

seonWKim avatar Jun 07 '24 23:06 seonWKim

@trustin Thank you for thorough review. ๐Ÿ™‡

seonWKim avatar Jun 09 '24 12:06 seonWKim

@seonWKim, ๐Ÿ‘ ๐Ÿ‘ ๐Ÿ‘

minwoox avatar Jun 11 '24 05:06 minwoox