From d15373ad31cd8f82d0169484d58a8c8e2667f32c Mon Sep 17 00:00:00 2001
From: Eno Compton <enocom@google.com>
Date: Wed, 26 Oct 2022 09:27:32 -0600
Subject: [PATCH] Allow min-ready to be set to zero

Also, return 503 is min ready exceeds total number of instances.
---
 internal/healthcheck/healthcheck.go      | 35 ++++++++++------
 internal/healthcheck/healthcheck_test.go | 52 +++++++++++++++---------
 2 files changed, 54 insertions(+), 33 deletions(-)

diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go
index 89d535b0f..def59b249 100644
--- a/internal/healthcheck/healthcheck.go
+++ b/internal/healthcheck/healthcheck.go
@@ -89,23 +89,24 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	var minReady int
+	var minReady *int
 	q := req.URL.Query()
 	if v := q.Get("min-ready"); v != "" {
 		n, err := strconv.Atoi(v)
 		if err != nil {
 			c.logger.Errorf("[Health Check] min-ready %q was not a valid integer", v)
-			w.WriteHeader(http.StatusServiceUnavailable)
+			w.WriteHeader(http.StatusBadRequest)
+			fmt.Fprintf(w, "min-query must be a valid integer, got = %q", v)
 			return
 		}
-		minReady = n
+		minReady = &n
 	}
 
 	n, err := c.proxy.CheckConnections(ctx)
-	if !ready(err, minReady, n) {
-		c.logger.Errorf("[Health Check] Readiness failed: %v", err)
+	if rErr := ready(err, minReady, n); rErr != nil {
+		c.logger.Errorf("[Health Check] Readiness failed: %v", rErr)
 		w.WriteHeader(http.StatusServiceUnavailable)
-		w.Write([]byte(err.Error()))
+		w.Write([]byte(rErr.Error()))
 		return
 	}
 
@@ -113,23 +114,31 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, req *http.Request) {
 	w.Write([]byte("ok"))
 }
 
-func ready(err error, minReady, total int) bool {
+func ready(err error, minReady *int, total int) error {
 	// If err is nil, then the proxy is ready.
 	if err == nil {
-		return true
+		if minReady != nil && *minReady > total {
+			return fmt.Errorf(
+				"min-ready (%v) is greater than registered instances (%v)",
+				*minReady, total,
+			)
+		}
+		return nil
 	}
 	// When minReady is not configured, any error means the proxy is not ready.
-	if minReady == 0 {
-		return false
+	if minReady == nil {
+		return err
 	}
 	mErr, ok := err.(proxy.MultiErr)
 	if !ok {
-		return false
+		return err
 	}
 	notReady := len(mErr)
 	areReady := total - notReady
-	return areReady >= minReady
-
+	if areReady < *minReady {
+		return err
+	}
+	return nil
 }
 
 // HandleLiveness indicates the process is up and responding to HTTP requests.
diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go
index 1dd52072a..b088b5180 100644
--- a/internal/healthcheck/healthcheck_test.go
+++ b/internal/healthcheck/healthcheck_test.go
@@ -263,50 +263,62 @@ func TestReadinessWithMinReady(t *testing.T) {
 		desc       string
 		minReady   string
 		wantStatus int
+		dialer     cloudsql.Dialer
 	}{
 		{
-			desc:       "when min ready is not configured",
-			minReady:   "0",
-			wantStatus: http.StatusServiceUnavailable,
+			desc:     "when min ready is zero",
+			minReady: "0",
+			// Required 0 to be ready, so status OK
+			// even if all checks fail.
+			wantStatus: http.StatusOK,
+			dialer:     &errorDialer{},
 		},
 		{
 			desc:       "when only one instance must be ready",
 			minReady:   "1",
 			wantStatus: http.StatusOK,
+			dialer:     &flakeyDialer{}, // fails on first call, succeeds on second
 		},
 		{
 			desc:       "when all instances must be ready",
 			minReady:   "2",
 			wantStatus: http.StatusServiceUnavailable,
+			dialer:     &errorDialer{},
+		},
+		{
+			desc:       "when min ready is greater than the number of instances",
+			minReady:   "3",
+			wantStatus: http.StatusServiceUnavailable,
+			dialer:     &fakeDialer{},
 		},
 		{
 			desc:       "when min ready is bogus",
 			minReady:   "bogus",
-			wantStatus: http.StatusServiceUnavailable,
+			wantStatus: http.StatusBadRequest,
+			dialer:     &fakeDialer{},
 		},
 		{
 			desc:       "when min ready is not set",
 			minReady:   "",
-			wantStatus: http.StatusServiceUnavailable,
+			wantStatus: http.StatusOK,
+			dialer:     &fakeDialer{},
 		},
 	}
-	p := newProxyWithParams(t, 0,
-		// for every two calls, flaky dialer fails for the first, succeeds for
-		// the second
-		&flakeyDialer{},
-		[]proxy.InstanceConnConfig{
-			{Name: "p:r:instance-1"},
-			{Name: "p:r:instance-2"},
-		},
-	)
-	defer func() {
-		if err := p.Close(); err != nil {
-			t.Logf("failed to close proxy client: %v", err)
-		}
-	}()
-
 	for _, tc := range tcs {
 		t.Run(tc.desc, func(t *testing.T) {
+			p := newProxyWithParams(t, 0,
+				tc.dialer,
+				[]proxy.InstanceConnConfig{
+					{Name: "p:r:instance-1"},
+					{Name: "p:r:instance-2"},
+				},
+			)
+			defer func() {
+				if err := p.Close(); err != nil {
+					t.Logf("failed to close proxy client: %v", err)
+				}
+			}()
+
 			check := healthcheck.NewCheck(p, logger)
 			check.NotifyStarted()
 			u, err := url.Parse(fmt.Sprintf("/readiness?min-ready=%s", tc.minReady))