-
Notifications
You must be signed in to change notification settings - Fork 684
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
Prepare for Envoy 1.13 upgrade; migrate from now-disabled deprecated fields #2328
Conversation
"operation_name": "egress" | ||
} | ||
self.base_http_config["tracing"] = {} | ||
self.traffic_direction = "OUTBOUND" |
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.
This seems wrong-ish to me, but I think it's what preserves existing behavior.
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 think you're correct. @alexgervais might have some thoughts as to whether we should support configuring this.
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
At some point, "operation_name": "egress"
was required for tracing to work on upstream calls. Having a quick look at the docs, and it would seem traffic_direction
could be omitted entirely. Anyways, I'd be happy to test tracing manually before shipping.
This is driven by "which of these cause test failures". It is NOT an audit of the codebase cross-referenced with <https://www.envoyproxy.io/docs/envoy/latest/intro/deprecated>. It would be a good idea to do that audit at some point.
310bb7b
to
b0b7bd8
Compare
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.
This looks good. Let's land it and get some experience with it.
"operation_name": "egress" | ||
} | ||
self.base_http_config["tracing"] = {} | ||
self.traffic_direction = "OUTBOUND" |
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 think you're correct. @alexgervais might have some thoughts as to whether we should support configuring this.
Description
This is everything (so far?) that is a part of upgrading to Envoy 1.13, short of doing the actual upgrade and things that depend on the actual upgrade. The biggest part of that is dealing with Envoy disallowing many previously deprecated fields (datawire/apro#713).
It probably makes sense to review this commit-by-commit.
Related Issues
PR #2318
Issue datawire/apro#713
Issue datawire/apro#998