@david-yu could you possibly help get a review on this please?

bumpety bump in case anyone's watching

@jukie taking a look at this now

Hey @jukie I was about to merge yesterday but then I was thinking more about the use cases you listed in the issue, and since Consul supports health checks, should we instead use Consul healthchecks to reflect the readiness of an endpoint, rather than ignoring it entirely if its non-ready? That way the user can configure Consul as they wish based on the health checks. I talked to the team and the consensus was that it makes more sense to use the health checks to reflect the readiness status.

If you're willing to make that change, I'll keep an eye on this PR (or a new PR if you make one, feel free to tag me) for those changes, or I can bring up the changes for prioritization within the team and work on it myself (but I'm not sure when I'll be able to commit to having this done). Thanks so much for your effort on this.

@ndhanushkodi could you explain how that would work? I think that may be in addition to or an extension of this change vs instead of.
Since consul-k8s is already watching for updates to the EndpointSlice objects that seems like the most efficient place to react to changes in readiness in real time. If a user wants to always publish endpoints they can also set spec.publishNotReadyAddresses and that would already be supported.

I'd be willing to make a new PR to support consul health checks as well but I'd like to understand the opinion that it wouldn't be a separate feature contribution. The existing behavior reacts to Service Endpoint changes and this extends that to make use of the additional state provided by EndpointSlices.

I'd like to understand the opinion that it wouldn't be a separate feature contribution. The existing behavior reacts to Service Endpoint changes and this extends that to make use of the additional state provided by EndpointSlices.

Thanks for your willingness to contribute and so sorry for the late response!

The existing behavior seems to register all endpoints in the endpoint slice, regardless of the condition of that endpoint, so if I created a service where one of the endpoints went into a non-ready state, that would still get registered as a service instance in Consul. This PR would change the behavior to no longer register service instances in Consul for endpoints that are in a non-ready state. My thought with the suggestion above was we should continue to register non-ready endpoints into Consul as we do today, but instead mark the health check unhealthy. Rather than changing the behavior to not register the non-ready endpoints, I'm proposing the health check change as the way to deal with non-ready endpoints.

For the cases that use the function registerServiceInstance, I'm imagining making a change here where we change the health based on the endpoint condition rather than always marking it as passing. For Nodeport services which doesn't use registerServiceInstance, I'm thinking we'd have to manually add a check here and line 679.

Let me know if I'm misunderstanding anything you're saying, or if this makes sense.

If a user wants to always publish endpoints they can also set spec.publishNotReadyAddresses and that would already be supported.

This is for K8s Services right? Whether the endpoint addresses should exist in the K8s Endpoint itself? This change is about whether the K8s Endpoint addresses that exist should get synced to Consul if they are non-ready so I think that feature doesn't quite match the behavior I mentioned in my suggestion.

Again, let me know if I'm misunderstanding what you mean!