From d5d427956da5240ee59360d08d547296f4f2ca86 Mon Sep 17 00:00:00 2001
From: Eno Compton <enocom@google.com>
Date: Mon, 24 Oct 2022 11:57:34 -0600
Subject: [PATCH] PR comments

---
 internal/healthcheck/healthcheck.go      | 27 ++++++++++++------------
 internal/healthcheck/healthcheck_test.go | 16 +++++++-------
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go
index b3c2925ec..89d535b0f 100644
--- a/internal/healthcheck/healthcheck.go
+++ b/internal/healthcheck/healthcheck.go
@@ -93,20 +93,16 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, req *http.Request) {
 	q := req.URL.Query()
 	if v := q.Get("min-ready"); v != "" {
 		n, err := strconv.Atoi(v)
-		// If the query value parsed correctly, set it to minReady. Otherwise,
-		// ignore it.
-		if err == nil {
-			minReady = n
+		if err != nil {
+			c.logger.Errorf("[Health Check] min-ready %q was not a valid integer", v)
+			w.WriteHeader(http.StatusServiceUnavailable)
+			return
 		}
+		minReady = n
 	}
 
 	n, err := c.proxy.CheckConnections(ctx)
-	if minReady > n {
-		c.logger.Errorf("[Health Check] min-ready was greater than registered instances")
-		w.WriteHeader(http.StatusBadRequest)
-		return
-	}
-	if err != nil && !ready(err, minReady, n) {
+	if !ready(err, minReady, n) {
 		c.logger.Errorf("[Health Check] Readiness failed: %v", err)
 		w.WriteHeader(http.StatusServiceUnavailable)
 		w.Write([]byte(err.Error()))
@@ -118,13 +114,18 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, req *http.Request) {
 }
 
 func ready(err error, minReady, total int) bool {
-	mErr, ok := err.(proxy.MultiErr)
-	if !ok {
-		return false
+	// If err is nil, then the proxy is ready.
+	if err == nil {
+		return true
 	}
+	// When minReady is not configured, any error means the proxy is not ready.
 	if minReady == 0 {
 		return false
 	}
+	mErr, ok := err.(proxy.MultiErr)
+	if !ok {
+		return false
+	}
 	notReady := len(mErr)
 	areReady := total - notReady
 	return areReady >= minReady
diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go
index 58dbc6e14..1dd52072a 100644
--- a/internal/healthcheck/healthcheck_test.go
+++ b/internal/healthcheck/healthcheck_test.go
@@ -264,6 +264,11 @@ func TestReadinessWithMinReady(t *testing.T) {
 		minReady   string
 		wantStatus int
 	}{
+		{
+			desc:       "when min ready is not configured",
+			minReady:   "0",
+			wantStatus: http.StatusServiceUnavailable,
+		},
 		{
 			desc:       "when only one instance must be ready",
 			minReady:   "1",
@@ -274,20 +279,15 @@ func TestReadinessWithMinReady(t *testing.T) {
 			minReady:   "2",
 			wantStatus: http.StatusServiceUnavailable,
 		},
-		{
-			desc:       "when min ready is not configured",
-			minReady:   "0",
-			wantStatus: http.StatusServiceUnavailable,
-		},
 		{
 			desc:       "when min ready is bogus",
 			minReady:   "bogus",
 			wantStatus: http.StatusServiceUnavailable,
 		},
 		{
-			desc:       "when min ready is greater than registered instances",
-			minReady:   "100",
-			wantStatus: http.StatusBadRequest,
+			desc:       "when min ready is not set",
+			minReady:   "",
+			wantStatus: http.StatusServiceUnavailable,
 		},
 	}
 	p := newProxyWithParams(t, 0,