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

Add proxy protocol config api message. #10845

Merged
merged 6 commits into from
Apr 28, 2020
Merged

Add proxy protocol config api message. #10845

merged 6 commits into from
Apr 28, 2020

Conversation

wez470
Copy link
Contributor

@wez470 wez470 commented Apr 20, 2020

Description: This PR creates a common PROXY protocol config API message. Currently it will be used for @alyssawilk's CONNECT work as well as in the transport socket for my upstream proxy proto work. This message could be extended to include TLVs in the future.
Risk Level: Low
Testing: None
Docs Changes: None
Release Notes: None
Discussed in: #10682 (my draft PR to discuss the upstream implementation)

Wasn't 100% sure where the best place to stick this would be but I put it in types for now.

@repokitteh-read-only
Copy link

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

🐱

Caused by: #10845 was opened by wez470.

see: more, trace.

Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Mind adding the build rules too? We want to make sure this will catch typos. Also I think you need to run
./tools/proto_format/proto_format.sh fix
to get all the other proto changes :-)

@alyssawilk alyssawilk self-assigned this Apr 20, 2020
@wez470
Copy link
Contributor Author

wez470 commented Apr 20, 2020

I think this build rule covers it https://github.com/envoyproxy/envoy/blob/master/api/BUILD#L251?

I think I ran proto_format, but will double check after work today

edit - updated link to proper location

@wez470
Copy link
Contributor Author

wez470 commented Apr 21, 2020

Definitely forgot to add the shadow file when I committed earlier 😅

alyssawilk
alyssawilk previously approved these changes Apr 21, 2020
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

I'm still confused at the lack of v4 but then I don't grok the vagaries of the new config system.
@htuch or @mattklein123 for API review.


// [#protodoc-title: Proxy Protocol]

message ProxyProtocolConfig {
Copy link
Member

Choose a reason for hiding this comment

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

Is this a core type or does it belong somewhere like envoy.config.core? I prefer to keep the type hierarchy for fundamental types.

The reason that there is no v4 is that this does not reference any deprecated fields or other types that have transitively deprecated fields.

Copy link
Contributor Author

@wez470 wez470 Apr 22, 2020

Choose a reason for hiding this comment

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

Yeah, I wasn't sure where to put it. Taking another look, the types already in this package probably would be more widely used than the proxy proto config. I can move to envoy.config.core

Copy link
Member

Choose a reason for hiding this comment

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

+1, agreed on moving it. Thank you.

/wait

Signed-off-by: Weston Carlson <[email protected]>
@wez470
Copy link
Contributor Author

wez470 commented Apr 25, 2020

/azp run envoy-presubmit

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 10845 in repo envoyproxy/envoy

Signed-off-by: Weston Carlson <[email protected]>
@wez470
Copy link
Contributor Author

wez470 commented Apr 27, 2020

@htuch @mattklein123 @alyssawilk ready for another look

@mattklein123 mattklein123 self-assigned this Apr 27, 2020
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!

@alyssawilk alyssawilk merged commit 1c28302 into envoyproxy:master Apr 28, 2020
@wez470 wez470 deleted the proxy-proto-api branch June 11, 2020 01:48
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.

4 participants