-
Notifications
You must be signed in to change notification settings - Fork 72
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
udpa: add core proto definitions from xDS transport++ proposal. #29
Conversation
These are the new UDPA protos from envoyproxy/envoy#11264 and https://docs.google.com/document/d/1zZav-IYxMO0mP2A7y5XHBa9v0eXyirw1CxrLLSUqWR8/edit. Signed-off-by: Harvey Tuch <[email protected]>
CC @markdroth @louiscryan @howardjohn @costinm This objective of this patch is to land a viable base set of definitions to work on from both the Envoy and Istio side. These are tagged WiP to allow arbitrary edits until we're happy with them. This PR won't be landing the full set of docs and UDPA spec, I'd prefer to wait until we've worked through some of the implementation details Envoy side to clarify that. |
} | ||
} | ||
|
||
repeated Directive directives = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we still have an open thread on how this interacts with RFC3986 URIs with only a single #fragment in the design doc (@markdroth @louiscryan), see https://docs.google.com/document/d/1zZav-IYxMO0mP2A7y5XHBa9v0eXyirw1CxrLLSUqWR8/edit?disco=AAAAGpGgj2A.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. As I mentioned there, I think we should remove the alt
directive and the repeated
here. Instead, we can use a repeated UdpaResourceLocator
in the places in the API where we have forward-pointers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM to ship and iterate based on the doc. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few comments and questions, but I'll go ahead and approve, since we can iterate after this is merged.
} | ||
} | ||
|
||
repeated Directive directives = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. As I mentioned there, I think we should remove the alt
directive and the repeated
here. Instead, we can use a repeated UdpaResourceLocator
in the places in the API where we have forward-pointers.
Signed-off-by: Harvey Tuch <[email protected]>
Re: alt vs repeated, I think let's merge as is and I will do a dedicated PR around this, this probably needs a bit of thought. |
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
@mattklein123 can I get another rubber stamp on this? Thanks. |
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
These are the new UDPA protos from envoyproxy/envoy#11264
and
https://docs.google.com/document/d/1zZav-IYxMO0mP2A7y5XHBa9v0eXyirw1CxrLLSUqWR8/edit.
Signed-off-by: Harvey Tuch [email protected]