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

api/config: use resource type annotations to provide type-to-endpoint. #9566

Merged
merged 7 commits into from
Jan 6, 2020

Conversation

htuch
Copy link
Member

@htuch htuch commented Jan 6, 2020

Previously, type_to_endpoint.cc had a lot of hardcoding, which doesn't scale well with multiple API
versions. See #9526 for an example of the issues
encountered.

This patch switches to using explicit resource type annotations on service descriptors, which is
great for documentation (previously this was sometimes given in comments, sometimes not), and allows
for a reflection driven reverse map from resource type URL to endpoints to be built at runtime.

Risk level: Low
Testing: New unit tests for type_to_endpoint.cc and golden protoxform tests for the new annotations.

Fixes #9454.

Signed-off-by: Harvey Tuch htuch@google.com

Previously, type_to_endpoint.cc had a lot of hardcoding, which doesn't scale well with multiple API
versions. See envoyproxy#9526 for an example of the issues
encountered.

This patch switches to using explicit resource type annotations on service descriptors, which is
great for documentation (previously this was sometimes given in comments, sometimes not), and allows
for a reflection driven reverse map from resource type URL to endpoints to be built at runtime.

Risk level: Low
Testing: New unit tests for type_to_endpoint.cc and golden protoxform tests for the new annotations.

Fixes envoyproxy#9454.

Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch htuch requested a review from lizan January 6, 2020 02:58
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9566 was opened by htuch.

see: more, trace.

@htuch htuch requested review from fredlas and mattklein123 January 6, 2020 02:58
@htuch htuch added the api/v3 Major version release @ end of Q3 2019 label Jan 6, 2020
htuch added 4 commits January 6, 2020 11:05
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

This is really fantastic, thanks. A few comments. Also, can we open a doc issue for 1.14 to use these annotations in doc output instead of all the manual insertions we have everywhere now?

/wait-any

source/common/config/type_to_endpoint.cc Show resolved Hide resolved
source/common/config/type_to_endpoint.cc Show resolved Hide resolved
htuch added 2 commits January 6, 2020 14:29
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Copy link
Member Author

htuch commented Jan 6, 2020

@mattklein123 #3091 tracks the service method doc generation, I left a comment to make use of the resource type annotations.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@repokitteh-read-only repokitteh-read-only bot removed the api label Jan 6, 2020
@htuch htuch merged commit cceab39 into envoyproxy:master Jan 6, 2020
@htuch htuch deleted the resource-annotations branch January 6, 2020 22:58
htuch pushed a commit that referenced this pull request Jan 7, 2020
This typo breaks spell check triggered by git push, thus preventing me
from cleanly uploading unrelated work.

Signed-off-by: Bence Béky <bnc@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/v3 Major version release @ end of Q3 2019
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Annotation based mapping between xDS message types and endpoints
3 participants