api/etcdhttp: change /health type back to string for backwards compatibility

[liggitt]

Fixes #9144
Fixes breaking change in /health API response prior to 3.3.0 release

Needs picking to release-3.3

Example impact of compatibility break is at kubernetes/kubernetes#58240

[liggitt]

cc @gyuho

[xiang90]

if we are going to revert this, we need to revert the newly added reason fields. I do not want to evolve the broken health endpoint if we do not want to fix it.

But before doing anything, I want to understand why it is hard for application to adopt this change.

[liggitt]

if we are going to revert this, we need to revert the newly added reason fields. I do not want to evolve the broken health endpoint if we do not want to fix it.

That doesn't follow. A well-behaved, forwards-compatible json client ignores unknown fields, so adding the error details is far safer than changing the type of an existing field.

But before doing anything, I want to understand why it is hard for application to adopt this change.

I'd actually ask the opposite question... I want to understand why it is hard for etcd to maintain compatibility here. Unless there's a compelling reason, etcd updates shouldn't require clients to rewrite their checks, or add check-bool-then-fallback-to-string logic.

[xiang90]

That doesn't follow.

I do not want to evolve an API with a broken field that can never be fixed. The health API is not versioned from the beginning.

[xiang90]

@liggitt

If you have a way to fix this broken field while keep this exact http path and does not introduce any compatibility issue, I would like to hear.

[liggitt]

The health API is not versioned from the beginning.

Then it seems like the right approach would be to live with the current state of the existing fields and only make changes in an additive way.

[xiang90]

Then it seems like the right approach would be to live with the current state of the existing fields and only make changes in an additive way.

Again,

1. if the change affects a lot of clients, tell me which are affected. there are tradeoffs to be made. If this breaking change only affects one or two applications/clients, i would rather fix the client/application.

2. if you hope we can evolve (meaning that we can add,remove,replace fields and its types) a unversioned API without breaking users, tell me exactly how. if there is no way to do that, and we want to keep the current API. I would rather keep the current one as simple as possible, and start to version this API to make it evolveable in the future. Or if you believe that the health API can be stabilized and evolved without versioning, provide reasons behind it.

If you do not help me to understand the above two questions, we will simply go circles here.

[liggitt]

if the change affects a lot of clients, tell me which are affected.

That is unknowable. It affects any client that wrote a health check following the documented API. Don't assume all of those uses are published and public.

there are tradeoffs to be made

I don't see a strong case for breaking the existing API. The string can express everything the boolean can.

if you hope we can evolve (meaning that we can add,remove,replace fields and its types) a unversioned API without breaking users, tell me exactly how.

You can only add new fields, and as you do, indicate they are optional. For such a simple API as this, that takes you a long way.

[xiang90]

You can only add new fields, and as you do, indicate they are optional.

I fully understand how to keep a json API backward and forward compatible.

For such a simple API as this,

As I said, I would like to keep it as simple as possible if the API is unversioned and we 100% do not want to break it. So I told you to revert the other newly added fields, which make this API complicated.

If we want a complicated healthy API, let us do it correctly if nothing can be changed.

[liggitt]

I told you to revert the other newly added fields, which make this API complicated.

My only concern was fixing the compatibility break before 3.3.0 is released, which this PR does. I'll let @gyuho handle a complete revert of #8312 if that is what is desired instead.

I will point out that @heyitsanthony had the same opinion on changing this field when you asked

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@7a8c192). Click here to learn what that means.
The diff coverage is 50%.

@@            Coverage Diff            @@
##             master    #9143   +/-   ##
=========================================
  Coverage          ?   76.06%           
=========================================
  Files             ?      359           
  Lines             ?    30006           
  Branches          ?        0           
=========================================
  Hits              ?    22825           
  Misses            ?     5599           
  Partials          ?     1582

Impacted Files	Coverage Δ
proxy/grpcproxy/health.go	8.33% <0%> (ø)
etcdserver/api/etcdhttp/metrics.go	76.31% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a8c192...f77e54e. Read the comment docs.

[gyuho]

Talked with @xiang90. We think it's best to not break backward compatibility in this case.
Next time, we will clearly document whether it's stable or experimental.
/health endpoint wasn't clearly documented.