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

internal/xds: xDS v3 server implementation #3017

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

stevesloka
Copy link
Member

@stevesloka stevesloka commented Oct 9, 2020

Follows #3016 by adding an xDS v3 server. Follow-up PR will introduce a config option to specify the xDS version wanting to serve.

Updates #1898

@stevesloka stevesloka added this to the 1.10.0 milestone Oct 9, 2020
@stevesloka stevesloka changed the title internal/xds: Xds v3 server impl internal/xds: xDS v3 server implementation Oct 9, 2020
@stevesloka stevesloka marked this pull request as draft October 9, 2020 18:59
@stevesloka stevesloka requested a review from skriss October 9, 2020 19:00
@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #3017 into main will decrease coverage by 0.29%.
The diff coverage is 53.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3017      +/-   ##
==========================================
- Coverage   74.30%   74.00%   -0.30%     
==========================================
  Files          92       95       +3     
  Lines        6008     6081      +73     
==========================================
+ Hits         4464     4500      +36     
- Misses       1446     1479      +33     
- Partials       98      102       +4     
Impacted Files Coverage Δ
internal/xds/v3/server.go 0.00% <0.00%> (ø)
internal/xds/v3/contour.go 69.09% <69.09%> (ø)
internal/xds/resource.go 100.00% <100.00%> (ø)
internal/xds/v2/contour.go 89.09% <100.00%> (-0.20%) ⬇️
internal/dag/cache.go 95.36% <0.00%> (-0.78%) ⬇️

@stevesloka stevesloka requested a review from jpeach October 9, 2020 19:10
@stevesloka stevesloka marked this pull request as ready for review October 9, 2020 19:45
Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

Do we really need to duplicate all the code here? The only think that is really different is the types of the discovery request and response. Can we take an approach where we only have to duplicate or abstract the guts of (*contourServer)stream()?

@stevesloka stevesloka force-pushed the xdsV3ServerImpl branch 2 times, most recently from 56ab109 to 9fa871e Compare October 14, 2020 21:16
@stevesloka stevesloka requested a review from jpeach October 14, 2020 21:17
@stevesloka
Copy link
Member Author

I pulled out some more bits that were duplicated. Please take a look @jpeach, @skriss.

@skriss
Copy link
Member

skriss commented Oct 14, 2020

It seems like in order to reuse much more code here, we'd need to define some interfaces and wrapper structs to adapt the v2 & v3 discovery request and response structs to common interfaces that the stream method could then interact with.

Given that we're looking to replace this all with the go-control-plane xds server though, I'm not sure it's really worth spending that effort.

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

Couple nits but overall I'm fine with this as-is, given that (1) we're going to drop v2 support pretty soon since it will be removed from Envoy early next year; (2) we're in the process of moving exclusively to the go-control-plane impl; and (3) upstream Envoy will not be revving a major version of the API every year going fwd.

internal/xds/resource.go Outdated Show resolved Hide resolved
internal/xds/v2/contour.go Outdated Show resolved Hide resolved
Adds a v3 xDS server which can be used to serve Envoy xDS v3 apis.

Updates projectcontour#1898

Signed-off-by: Steve Sloka <slokas@vmware.com>
@stevesloka stevesloka merged commit d2bb321 into projectcontour:main Oct 15, 2020
@stevesloka stevesloka deleted the xdsV3ServerImpl branch October 15, 2020 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants