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

Add support for self config source #5459

Merged
merged 6 commits into from Apr 3, 2024

Conversation

jrhee17
Copy link
Contributor

@jrhee17 jrhee17 commented Feb 13, 2024

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: envoyproxy/envoy#13951

Modifications:

  • The dependency relationship between ResourceNode and ConfigSource has changed. 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 jrhee17 added the defect label Feb 13, 2024
@jrhee17 jrhee17 added this to the 1.28.0 milestone Feb 13, 2024
@jrhee17 jrhee17 marked this pull request as ready for review February 13, 2024 10:06
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified so that subscribed ResourceNode#configSource is always non-null.

Let's remove Nullable in the ListenerResourceNode constructor.

@jrhee17
Copy link
Contributor Author

jrhee17 commented Apr 2, 2024

Let's remove Nullable in the ListenerResourceNode constructor.

Coincidentally, due to the recent changes now ListenerRoot can pass a null config source

node = new ListenerResourceNode(null, resourceName, xdsBootstrap, this, ResourceNodeType.STATIC);

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very useful. Thanks! 👍 👍 👍

Copy link

codecov bot commented Apr 2, 2024

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 ⚠️
...ava/com/linecorp/armeria/xds/XdsBootstrapImpl.java 88.88% 0 Missing and 1 partial ⚠️
...ava/com/linecorp/armeria/xds/XdsConverterUtil.java 0.00% 0 Missing and 1 partial ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

�Though it is not a standard feature, it could be useful. 🙇‍♂️🙇‍♂️

@jrhee17 jrhee17 merged commit 5e79a26 into line:main Apr 3, 2024
17 of 18 checks passed
@ikhoon
Copy link
Contributor

ikhoon commented Apr 5, 2024

Still LGTM.

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

Successfully merging this pull request may close these issues.

None yet

3 participants