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.

Sign up for GitHub

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

Jump to bottom

Support mixed v2/v3 xDS request/responses #12913

Merged
merged 38 commits into from
Sep 22, 2020
Merged

Support mixed v2/v3 xDS request/responses #12913

merged 38 commits into from
Sep 22, 2020

Conversation

chaoqin-li1123
Copy link
Member

Commit Message: To better support migration from v2 type url to v3 type url, we should be able to support a DiscoveryResponse with a v3 resource to a DiscoveryRequest with v2 resource type. This can be achieved by downgrading the type url when subscriber can't be found.
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue] Support mixed v2/v3 xDS request/responses
[Optional Deprecated:]

chaoqinli added 2 commits September 1, 2020 01:21
first version
28dcba3 
Signed-off-by: chaoqinli <chaoqinli@google.com>
add comment
0b27712 
Signed-off-by: chaoqinli <chaoqinli@google.com>
Shikugawa
Shikugawa reviewed Sep 1, 2020
View reviewed changes
Copy link
Member

@Shikugawa Shikugawa left a comment

Choose a reason for hiding this comment

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

Thank you! Generally LGTM. I'd commented on some of the sections.

source/common/config/api_type_oracle.cc Show resolved Hide resolved
fix no watcher case
e3f3485 
Signed-off-by: chaoqinli <chaoqinli@google.com>
@htuch htuch self-assigned this Sep 1, 2020
@htuch
Copy link
Member

htuch commented Sep 1, 2020

@adisuissa could you take a first pass? Thanks.

@htuch htuch requested a review from adisuissa September 1, 2020 17:28
adisuissa
adisuissa reviewed Sep 1, 2020
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, looks good.
Can you make sure the tests cover cases where getEarlierTypeUrl returns absl::nullopt?

source/common/config/api_type_oracle.cc Outdated Show resolved Hide resolved
source/common/config/api_type_oracle.cc Outdated Show resolved Hide resolved
source/common/config/grpc_mux_impl.cc Outdated Show resolved Hide resolved
source/common/config/grpc_mux_impl.cc Outdated Show resolved Hide resolved
chaoqinli added 5 commits September 2, 2020 02:37
clean up
17a1002 
Signed-off-by: chaoqinli <chaoqinli@google.com>
clean up
333d68f 
Signed-off-by: chaoqinli <chaoqinli@google.com>
clean up
ca8e67b 
Signed-off-by: chaoqinli <chaoqinli@google.com>
Merge branch 'master' of https://github.com/envoyproxy/envoy into mix…
a4deb6b 
…ed_type
clean up
53887ac 
Signed-off-by: chaoqinli <chaoqinli@google.com>
dmitri-d
dmitri-d reviewed Sep 2, 2020
View reviewed changes
Copy link
Contributor

@dmitri-d dmitri-d left a comment

Choose a reason for hiding this comment

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

lgtm other than nits

source/common/config/api_type_oracle.cc Outdated Show resolved Hide resolved
source/common/config/grpc_mux_impl.cc Outdated Show resolved Hide resolved
htuch
htuch suggested changes Sep 2, 2020
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Looks great, thanks, flushing a few comments.
/wait

source/common/config/api_type_oracle.cc Show resolved Hide resolved
source/common/config/api_type_oracle.cc Outdated Show resolved Hide resolved
source/common/config/grpc_mux_impl.cc Outdated Show resolved Hide resolved
source/common/config/grpc_mux_impl.cc Outdated Show resolved Hide resolved
source/common/config/grpc_mux_impl.cc Outdated Show resolved Hide resolved
test/integration/ads_integration_test.cc Outdated Show resolved Hide resolved
source/common/config/grpc_mux_impl.cc Outdated Show resolved Hide resolved
adisuissa
adisuissa reviewed Sep 3, 2020
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!
A couple of nits.

source/common/config/api_type_oracle.cc Outdated Show resolved Hide resolved
source/common/config/new_grpc_mux_impl.cc Outdated Show resolved Hide resolved
support v2 response to v3 request
de87fd6 
Signed-off-by: chaoqinli <chaoqinli@google.com>
@chaoqin-li1123
Copy link
Member Author

I have added support for case where grpc_mux watch for v3 type_url but receive v2 type_url resource.

fix spelling
8153e6c 
Signed-off-by: chaoqinli <chaoqinli@google.com>
chaoqinli added 3 commits September 17, 2020 19:53
format
1278fda 
Signed-off-by: chaoqinli <chaoqinli@google.com>
clean up
ca3ece4 
Signed-off-by: chaoqinli <chaoqinli@google.com>
test coverage
bfd7e87 
Signed-off-by: chaoqinli <chaoqinli@google.com>
htuch
htuch suggested changes Sep 18, 2020
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments on version history, docs and cleaning up the type URL util location.
/wait

docs/root/faq/api/control_plane_version_support.rst Outdated Show resolved Hide resolved
docs/root/faq/api/control_plane_version_support.rst Outdated Show resolved Hide resolved
source/common/config/api_type_oracle.cc Outdated Show resolved Hide resolved
type util test
804721d 
Signed-off-by: chaoqinli <chaoqinli@google.com>
chaoqinli added 5 commits September 20, 2020 01:02
format
ca31bf9 
Signed-off-by: chaoqinli <chaoqinli@google.com>
format
fd37479 
Signed-off-by: chaoqinli <chaoqinli@google.com>
add doc
4617ca2 
Signed-off-by: chaoqinli <chaoqinli@google.com>
format
1d9c95c 
Signed-off-by: chaoqinli <chaoqinli@google.com>
Merge branch 'master' of https://github.com/envoyproxy/envoy into mix…
8c4170a 
…ed_type
change doc
eded175 
Signed-off-by: chaoqinli <chaoqinli@google.com>
htuch
htuch suggested changes Sep 22, 2020
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Ready to ship once the RST docs suggestions is done, thanks!
/wait

test/common/protobuf/utility_test.cc Outdated Show resolved Hide resolved
docs
5d14694 
Signed-off-by: chaoqinli <chaoqinli@google.com>
htuch
htuch approved these changes Sep 22, 2020
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, this is great!

@htuch htuch merged commit c8b894c into envoyproxy:master Sep 22, 2020
@htuch
Copy link
Member

htuch commented Sep 22, 2020

@ramaraochavali will be supported until immediately after Q1 2021 release, planning on removing v2 at that point.

@chaoqin-li1123 chaoqin-li1123 deleted the mixed_type branch February 23, 2021 21:18
@alyssawilk alyssawilk mentioned this pull request Apr 15, 2021
Closed
@chaoqin-li1123 chaoqin-li1123 mentioned this pull request May 6, 2021
Merged
htuch pushed a commit that referenced this pull request May 10, 2021
@chaoqin-li1123
remove support for type url downgrade and upgrade conversion (#16372)
93f9d16 
Remove support for mixed v2/v3 type url introduced by pull request(#12913)

Fixes #16015

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shikugawa Shikugawa Shikugawa left review comments

@htuch htuch htuch approved these changes

@dmitri-d dmitri-d Awaiting requested review from dmitri-d

@adisuissa adisuissa Awaiting requested review from adisuissa

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

@dio dio Awaiting requested review from dio

@jmarantz jmarantz Awaiting requested review from jmarantz

@mattklein123 mattklein123 Awaiting requested review from mattklein123

Assignees

@adisuissa adisuissa

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@chaoqin-li1123 @htuch @ramaraochavali @dmitri-d @adisuissa @Shikugawa