Skip to content

Support mixed v2/v3 xDS request/responses #12913

Merged
htuch merged 38 commits intoenvoyproxy:masterfrom
chaoqin-li1123:mixed_type
Sep 22, 2020
Merged

Support mixed v2/v3 xDS request/responses #12913
htuch merged 38 commits intoenvoyproxy:masterfrom
chaoqin-li1123:mixed_type

Conversation

@chaoqin-li1123
Copy link
Copy Markdown
Contributor

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
Signed-off-by: chaoqinli <chaoqinli@google.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
Copy link
Copy Markdown
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.

Signed-off-by: chaoqinli <chaoqinli@google.com>
@htuch htuch self-assigned this Sep 1, 2020
@htuch
Copy link
Copy Markdown
Member

htuch commented Sep 1, 2020

@adisuissa could you take a first pass? Thanks.

Copy link
Copy Markdown
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?

chaoqinli added 5 commits September 2, 2020 02:37
Signed-off-by: chaoqinli <chaoqinli@google.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
Copy link
Copy Markdown
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

Copy link
Copy Markdown
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

Copy link
Copy Markdown
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.

Signed-off-by: chaoqinli <chaoqinli@google.com>
@chaoqin-li1123
Copy link
Copy Markdown
Contributor Author

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

Signed-off-by: chaoqinli <chaoqinli@google.com>
chaoqinli added 3 commits September 17, 2020 19:53
Signed-off-by: chaoqinli <chaoqinli@google.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
Copy link
Copy Markdown
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

Signed-off-by: chaoqinli <chaoqinli@google.com>
chaoqinli added 5 commits September 20, 2020 01:02
Signed-off-by: chaoqinli <chaoqinli@google.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
Copy link
Copy Markdown
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

Signed-off-by: chaoqinli <chaoqinli@google.com>
Copy link
Copy Markdown
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
Copy Markdown
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
htuch pushed a commit that referenced this pull request May 10, 2021
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants