Allow healthcheck on different port for non-EDS cluster types

[bobzilladev]

The http healthcheck will allow a path but not a port, which is problematic for existing Dropwizard-based services which by default to have the healthcheck endpoint on a separate admin port. There are also instances of proxies where a tcp check is being done to check the health of the proxy but the traffic is going over different ports.

Envoy would need a healthcheck_port in the static configuration and the SDS API to support being a drop-in replacement for HAProxy under these conditions.

http://www.dropwizard.io/1.0.6/docs/manual/core.html#health-checks

[mattklein123]

Please see linked PR for a first attempt at this. Needs cleanup.

[mpuncel]

I'd like to +1 this feature, we have a few cases where we'd like to be able to health check a port that's different from the "traffic port"

[mpuncel]

I looked through the previous PR. It sounds like the idea to add a "health check address" to each endpoint in EDS was uncontroversial but figuring out a clean design for specifying an alternate health check port when using strict or logical DNS (and probably static hosts) was difficult.

Would it be reasonable to add the "health check address" config option to EDS first, and then follow up with the other ways of specifying cluster membership later on?

[mattklein123]

@mpuncel yes, at this point I think it's reasonable to just make this happen for EDS only which I think would not be too hard. I would add alternate HC port information to https://github.com/envoyproxy/data-plane-api/blob/master/envoy/api/v2/endpoint/endpoint.proto#L19 most likely. If you want to tackle I would do a data-plane-api PR and we can discuss the details there.

[dio]

@mpuncel, if you're not on this, may I try to propose a fix for this case?

[mpuncel]

@dio go for it!

[mattklein123]

@dio is there any reason this is not fixed by #3007? I noticed you said "partially fixed." It seems fixed to me?

[dio]

@mattklein123 I thought we want to have alternate health check port for all setup, including when using strict or logical DNS (and probably static hosts) as well.

[mattklein123]

@dio OK fair enough we can keep it open to track.

[dio]

Currently, for STATIC, STRICT_DNS and LOGICAL_DNS, a cluster member (a host of hosts) is defined as a core.Address. In order to accommodate alternative health check port for each host, could we have an alternative type (oneof?) of declaring host? Probably just core.Address + health_check_port just like what we have with endpoint (to allow defining host members as a collection of endpoints instead of core.Address-es). Thoughts?

[mattklein123]

@dio yes I think we can add oneof and not break wire compatibility. I think what you propose makes sense to me. @htuch @wdyt?

[dio]

If we change https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/cds.proto#L158 to the following:

  message Host {
    oneof host_specifier {
      option (validate.required) = true;

      core.Address address = 1;
      endpoint.Endpoint endpoint = 2;
    }
  }

  repeated Host hosts = 7;

will it break wire compatibility?

Since I guess repeated oneof is not a thing: protocolbuffers/protobuf#2592 (comment)

[mattklein123]

Yes I think it will break wire compatibility. You may need to add a new repeated hosts field and deprecate the old hosts addresses field or something like that. That is probably a better long term solution.

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[bobzilladev]

This is still an important one for our organization, fwiw.

[mattklein123]

@bobzilladev AFAIK this is implemented already for EDS, just not for non-EDS. Right @dio? Maybe we should change the title.

[htuch]

@mattklein123 do we even need this for non-EDS, given that we can embed ClusterLoadAssignment in Cluster now?

[mattklein123]

@htuch AFAIK implementation of that was never finished, right? I think just the config was added. @dio ?

[dio]

@mattklein123 yes for EDS it is there.

While for the ClusterLoadAssignment inside Cluster, the config is added #3504.

Sorry for the delay in the implementation. I'll start again next week.

[dio]

Update: #3783 is part of this effort.

[dio]

Update. I have submitted: #3864 to fix this.

[amaximus]

Does this work for non-xDS clusters (e.g. static and/or strict_dns)?
If so, from which envoy version?