armeria icon indicating copy to clipboard operation
armeria copied to clipboard

Add support for self config source

Open jrhee17 opened this issue 1 year ago • 1 comments

Motivation:

This pull request adds support for SELF config source types. This feature allows the control plane to delegate the decision of which protocol to use to the client. Reference: https://github.com/envoyproxy/envoy/blob/bd18d0fa7790a7e5fe1a95f6ae271ca02614e36c/api/envoy/config/core/v3/config_source.proto#L239

Note that this implementation may not have been completed from envoy side, so we should use this option with caution. ref: https://github.com/envoyproxy/envoy/issues/13951

Modifications:

  • The dependency relationship between ResourceNode and ConfigSource. This can be a problem because the configSource specified in ResourceNode#ConfigSource can be different from the actual ConfigSource used for subscribing. Modified so that ConfigSource is computed before creating a ResourceNode.
  • Renamed BootstrapApiConfigs to ConfigSourceMapper since it better represents the functionality of the class.
  • Added parentConfigSource to the logic of computing a new configSource.
  • Modified so that subscribed ResourceNode#configSource is always non-null. The only case where this is null is for static clusters (bootstrap clusters)

Result:

  • SELF type configSource is now supported

jrhee17 avatar Feb 13 '24 08:02 jrhee17

🔍 Build Scan® (commit: 5c7c0760aae97b7c21e776ba16b60de9972e330f)

Job name Status Build Scan®
build-windows-latest-jdk-19 https://ge.armeria.dev/s/nr5wajdfyhr3g
build-self-hosted-unsafe-jdk-8 https://ge.armeria.dev/s/avoh4hayvffse
build-self-hosted-unsafe-jdk-19-snapshot-blockhound https://ge.armeria.dev/s/qegj7venqyrc4
build-self-hosted-unsafe-jdk-17-min-java-17-coverage https://ge.armeria.dev/s/jiu7c3ssdfiju
build-self-hosted-unsafe-jdk-17-min-java-11 https://ge.armeria.dev/s/kodynqf53zbiq
build-self-hosted-unsafe-jdk-17-leak https://ge.armeria.dev/s/edo7eqdyhpzds
build-self-hosted-unsafe-jdk-11 https://ge.armeria.dev/s/oxr5qbxm6z4ik
build-macos-12-jdk-19 https://ge.armeria.dev/s/puxr42xbrhsoe

github-actions[bot] avatar Feb 13 '24 09:02 github-actions[bot]

Let's remove Nullable in the ListenerResourceNode constructor.

Coincidentally, due to the recent changes now ListenerRoot can pass a null config source https://github.com/line/armeria/blob/42edbb2940a0447f8df9dd189fe24acfc842335a/xds/src/main/java/com/linecorp/armeria/xds/ListenerRoot.java#L37

jrhee17 avatar Apr 02 '24 09:04 jrhee17

Codecov Report

Attention: Patch coverage is 62.50000% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 74.10%. Comparing base (42edbb2) to head (5c7c076). Report is 7 commits behind head on main.

Files Patch % Lines
...a/com/linecorp/armeria/xds/ConfigSourceMapper.java 40.62% 13 Missing and 6 partials :warning:
...ava/com/linecorp/armeria/xds/XdsBootstrapImpl.java 88.88% 0 Missing and 1 partial :warning:
...ava/com/linecorp/armeria/xds/XdsConverterUtil.java 0.00% 0 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #5459   +/-   ##
=========================================
  Coverage     74.09%   74.10%           
- Complexity    20968    20993   +25     
=========================================
  Files          1817     1818    +1     
  Lines         77187    77273   +86     
  Branches       9854     9867   +13     
=========================================
+ Hits          57194    57262   +68     
- Misses        15323    15326    +3     
- Partials       4670     4685   +15     

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

codecov[bot] avatar Apr 02 '24 10:04 codecov[bot]

Still LGTM.

ikhoon avatar Apr 05 '24 06:04 ikhoon