-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Envoy config cluster #5308
Envoy config cluster #5308
Conversation
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. Thanks Nick!
// }, | ||
// }, | ||
|
||
if c == nil { |
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.
nit: I'd probably just return c, nil
in the If above rather than have the rest conditional on the first part - slightly easier to read. That said you probably copied the pattern from my code else where 😄
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.
Ah yeah the upstream below shows why - in other cases we need to always inject TLS stuff even if we got the config from the user so we continue processing. Leaving this consistent is fine.
nit: the code is using |
func makeUpstreamCluster(name string, cfgSnap *proxycfg.ConfigSnapshot) *envoy.Cluster { | ||
return &envoy.Cluster{ | ||
Name: name, | ||
// TODO(banks): make this configurable from the upstream config |
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.
it looks like all the //TODO(banks)
comments have been lost, is there a pending issue out there as a reminder to make them configurable?
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.
Hey sorry about this, this is work in progress, with the code in master you should be able to configure the public listener cluster JSON and the upstream listener JSON. I plan to do a PR on the docs next week to update to show this configuration.
Upstream example (see API upstream):
https://github.com/hashicorp/service-mesh-training/blob/master/exercises/lab-04/04-outlier-detection/files/app/ingress.yml
Public listener example:
https://github.com/hashicorp/service-mesh-training/blob/master/exercises/lab-04/04-outlier-detection/files/app/cache.yml
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.
Thanks, @nicholasjackson will you be adding any circuit_breakers
examples to those docs?
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 I'll be enabling default Envoy circuit breaker (i.e. outlier detection) in #5499.
Full support for configuring them without using escape hatch will be in a future release too along with a lot more control of Envoy!
Could someone confirm if this escape hatch functionality is in the very recent 1.4.4 release please? Accepting that the docs aren't there yet but more interested in the feature. |
@andye2004 yep the escape hatch should work in 1.4.4. We intentionally choose to leave it out of the changelog partly because wasn't documented yet (1.4.4 was a quick security patch). |
@banks Thanks Paul. |
@banks, hi Paul, just wanted to point something out, maybe this is be design maybe not. Using a config very similar to the example at the top of this PR, when the envoy sidecar is configured with the endpoints from Consul it only receives a single endpoint even when multiple instances of a service are registered. E.g. I get this in the logs
EDIT: Further testing carried out. As a further test I decided to kill the docker container one of the services was running in and issued a further request which failed then a second request which succeeded, When looking in the logs again after this, I see that Envoy now has both services. E.g.
2nd EDIT: I actually believe that second response to be wrong also. I think both endpoints should be under a single lb_endpoints instance. Apologies for spamming! |
@andye2004 if you can reproduce an issue here can you post a new issue with a full description/repro? There are a few bugs in this area fixed in master already that you could be hitting, if you have a chance to try master that would be great extra info in the issue. |
This PR adds the capability to add an escape hatch for cluster config giving the user the ability to configure connection_timeout and L7 features like
outlier_detection
.It adds two new parameters
envoy_public_cluster_json
for the main listener andenvoy_cluster_json
for upstream listeners. An example can be shown below:I have not added the documentation for this yet, would appreciate a quick pass on the code and I will add the docs once all is ok.