From 52ac3a397ae5abf5d384dfbd7abf07f3368f9576 Mon Sep 17 00:00:00 2001
From: Eno Compton <enocom@google.com>
Date: Wed, 19 Oct 2022 13:53:23 -0600
Subject: [PATCH 01/13] feat: add support for min ready instances

Fixes #1375.
---
 cmd/root.go                              |  5 +-
 internal/healthcheck/healthcheck.go      | 33 ++++++---
 internal/healthcheck/healthcheck_test.go | 92 ++++++++++++++++++++----
 internal/proxy/proxy.go                  |  5 +-
 tests/connection_test.go                 |  2 +-
 5 files changed, 110 insertions(+), 27 deletions(-)

diff --git a/cmd/root.go b/cmd/root.go
index a088e2e76..2df70ef46 100644
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -80,6 +80,7 @@ type Command struct {
 	prometheus                 bool
 	prometheusNamespace        string
 	healthCheck                bool
+	minReady                   uint64
 	httpAddress                string
 	httpPort                   string
 
@@ -288,6 +289,8 @@ func NewCommand(opts ...Option) *Command {
 		"Port for Prometheus and health check server")
 	cmd.PersistentFlags().BoolVar(&c.healthCheck, "health-check", false,
 		"Enables health check endpoints /startup, /liveness, and /readiness on localhost.")
+	cmd.PersistentFlags().Uint64Var(&c.minReady, "min-ready-instances", 0,
+		"Configures the minimum ready number of instances for the readiness check to pass")
 	cmd.PersistentFlags().StringVar(&c.conf.APIEndpointURL, "sqladmin-api-endpoint", "",
 		"API endpoint for all Cloud SQL Admin API requests. (default: https://sqladmin.googleapis.com)")
 	cmd.PersistentFlags().StringVar(&c.conf.QuotaProject, "quota-project", "",
@@ -620,7 +623,7 @@ func runSignalWrapper(cmd *Command) error {
 	notify := func() {}
 	if cmd.healthCheck {
 		needsHTTPServer = true
-		hc := healthcheck.NewCheck(p, cmd.logger)
+		hc := healthcheck.NewCheck(p, cmd.logger, cmd.minReady)
 		mux.HandleFunc("/startup", hc.HandleStartup)
 		mux.HandleFunc("/readiness", hc.HandleReadiness)
 		mux.HandleFunc("/liveness", hc.HandleLiveness)
diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go
index c2c083560..fb57c4ac2 100644
--- a/internal/healthcheck/healthcheck.go
+++ b/internal/healthcheck/healthcheck.go
@@ -29,19 +29,21 @@ import (
 // Check provides HTTP handlers for use as healthchecks typically in a
 // Kubernetes context.
 type Check struct {
-	once    *sync.Once
-	started chan struct{}
-	proxy   *proxy.Client
-	logger  cloudsql.Logger
+	once     *sync.Once
+	started  chan struct{}
+	proxy    *proxy.Client
+	logger   cloudsql.Logger
+	minReady uint64
 }
 
 // NewCheck is the initializer for Check.
-func NewCheck(p *proxy.Client, l cloudsql.Logger) *Check {
+func NewCheck(p *proxy.Client, l cloudsql.Logger, minReady uint64) *Check {
 	return &Check{
-		once:    &sync.Once{},
-		started: make(chan struct{}),
-		proxy:   p,
-		logger:  l,
+		once:     &sync.Once{},
+		started:  make(chan struct{}),
+		proxy:    p,
+		logger:   l,
+		minReady: minReady,
 	}
 }
 
@@ -89,7 +91,7 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, _ *http.Request) {
 	}
 
 	err := c.proxy.CheckConnections(ctx)
-	if err != nil {
+	if err != nil && !ready(err, c.minReady) {
 		c.logger.Errorf("[Health Check] Readiness failed: %v", err)
 		w.WriteHeader(http.StatusServiceUnavailable)
 		w.Write([]byte(err.Error()))
@@ -100,6 +102,17 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, _ *http.Request) {
 	w.Write([]byte("ok"))
 }
 
+func ready(err error, minReady uint64) bool {
+	mErr, ok := err.(proxy.MultiErr)
+	if !ok {
+		return false
+	}
+	if uint64(len(mErr)) > minReady {
+		return false
+	}
+	return true
+}
+
 // HandleLiveness indicates the process is up and responding to HTTP requests.
 // If this check fails (because it's not reachable), the process is in a bad
 // state and should be restarted.
diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go
index 00c03495d..e1913f7ac 100644
--- a/internal/healthcheck/healthcheck_test.go
+++ b/internal/healthcheck/healthcheck_test.go
@@ -24,6 +24,7 @@ import (
 	"net/http/httptest"
 	"os"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -71,6 +72,21 @@ func (*fakeDialer) Close() error {
 	return nil
 }
 
+type flakyDialer struct {
+	dialCount uint64
+	fakeDialer
+}
+
+// Dial fails on odd calls and succeeds on even calls.
+func (f *flakyDialer) Dial(_ context.Context, _ string, _ ...cloudsqlconn.DialOption) (net.Conn, error) {
+	c := atomic.AddUint64(&f.dialCount, 1)
+	if c%2 == 0 {
+		conn, _ := net.Pipe()
+		return conn, nil
+	}
+	return nil, errors.New("flakey dialer fails on odd calls")
+}
+
 type errorDialer struct {
 	fakeDialer
 }
@@ -79,13 +95,11 @@ func (*errorDialer) Dial(_ context.Context, _ string, _ ...cloudsqlconn.DialOpti
 	return nil, errors.New("errorDialer always errors")
 }
 
-func newProxyWithParams(t *testing.T, maxConns uint64, dialer cloudsql.Dialer) *proxy.Client {
+func newProxyWithParams(t *testing.T, maxConns uint64, dialer cloudsql.Dialer, instances []proxy.InstanceConnConfig) *proxy.Client {
 	c := &proxy.Config{
-		Addr: proxyHost,
-		Port: proxyPort,
-		Instances: []proxy.InstanceConnConfig{
-			{Name: "proj:region:pg"},
-		},
+		Addr:           proxyHost,
+		Port:           proxyPort,
+		Instances:      instances,
 		MaxConnections: maxConns,
 	}
 	p, err := proxy.NewClient(context.Background(), dialer, logger, c)
@@ -96,15 +110,17 @@ func newProxyWithParams(t *testing.T, maxConns uint64, dialer cloudsql.Dialer) *
 }
 
 func newTestProxyWithMaxConns(t *testing.T, maxConns uint64) *proxy.Client {
-	return newProxyWithParams(t, maxConns, &fakeDialer{})
+	return newProxyWithParams(t, maxConns, &fakeDialer{}, []proxy.InstanceConnConfig{
+		{Name: "proj:region:pg"},
+	})
 }
 
 func newTestProxyWithDialer(t *testing.T, d cloudsql.Dialer) *proxy.Client {
-	return newProxyWithParams(t, 0, d)
+	return newProxyWithParams(t, 0, d, []proxy.InstanceConnConfig{{Name: "proj:region:pg"}})
 }
 
 func newTestProxy(t *testing.T) *proxy.Client {
-	return newProxyWithParams(t, 0, &fakeDialer{})
+	return newProxyWithParams(t, 0, &fakeDialer{}, []proxy.InstanceConnConfig{{Name: "proj:region:pg"}})
 }
 
 func TestHandleStartupWhenNotNotified(t *testing.T) {
@@ -114,7 +130,7 @@ func TestHandleStartupWhenNotNotified(t *testing.T) {
 			t.Logf("failed to close proxy client: %v", err)
 		}
 	}()
-	check := healthcheck.NewCheck(p, logger)
+	check := healthcheck.NewCheck(p, logger, 0)
 
 	rec := httptest.NewRecorder()
 	check.HandleStartup(rec, &http.Request{})
@@ -134,7 +150,7 @@ func TestHandleStartupWhenNotified(t *testing.T) {
 			t.Logf("failed to close proxy client: %v", err)
 		}
 	}()
-	check := healthcheck.NewCheck(p, logger)
+	check := healthcheck.NewCheck(p, logger, 0)
 
 	check.NotifyStarted()
 
@@ -154,7 +170,7 @@ func TestHandleReadinessWhenNotNotified(t *testing.T) {
 			t.Logf("failed to close proxy client: %v", err)
 		}
 	}()
-	check := healthcheck.NewCheck(p, logger)
+	check := healthcheck.NewCheck(p, logger, 0)
 
 	rec := httptest.NewRecorder()
 	check.HandleReadiness(rec, &http.Request{})
@@ -173,7 +189,7 @@ func TestHandleReadinessForMaxConns(t *testing.T) {
 		}
 	}()
 	started := make(chan struct{})
-	check := healthcheck.NewCheck(p, logger)
+	check := healthcheck.NewCheck(p, logger, 0)
 	go p.Serve(context.Background(), func() {
 		check.NotifyStarted()
 		close(started)
@@ -220,7 +236,7 @@ func TestHandleReadinessWithConnectionProblems(t *testing.T) {
 			t.Logf("failed to close proxy client: %v", err)
 		}
 	}()
-	check := healthcheck.NewCheck(p, logger)
+	check := healthcheck.NewCheck(p, logger, 0)
 	check.NotifyStarted()
 
 	rec := httptest.NewRecorder()
@@ -239,3 +255,51 @@ func TestHandleReadinessWithConnectionProblems(t *testing.T) {
 		t.Fatalf("want substring with = %q, got = %v", want, string(body))
 	}
 }
+
+func TestReadinessWithMinReady(t *testing.T) {
+	tcs := []struct {
+		desc       string
+		minReady   uint64
+		wantStatus int
+	}{
+		{
+			desc:       "when all instances must be ready",
+			minReady:   0,
+			wantStatus: http.StatusServiceUnavailable,
+		},
+		{
+			desc:       "when only one instance must be ready",
+			minReady:   1,
+			wantStatus: http.StatusOK,
+		},
+	}
+	p := newProxyWithParams(t, 0,
+		// for every two calls, flaky dialer fails for the first, succeeds for
+		// the second
+		&flakyDialer{},
+		[]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) {
+			check := healthcheck.NewCheck(p, logger, tc.minReady)
+			check.NotifyStarted()
+
+			rec := httptest.NewRecorder()
+			check.HandleReadiness(rec, &http.Request{})
+
+			resp := rec.Result()
+			if got, want := resp.StatusCode, tc.wantStatus; got != want {
+				t.Fatalf("want = %v, got = %v", want, got)
+			}
+		})
+	}
+}
diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go
index 4546e7fef..79404d6d8 100644
--- a/internal/proxy/proxy.go
+++ b/internal/proxy/proxy.go
@@ -443,7 +443,10 @@ func (c *Client) CheckConnections(ctx context.Context) error {
 			}
 			cErr := conn.Close()
 			if cErr != nil {
-				errCh <- fmt.Errorf("%v: %v", m.inst, cErr)
+				c.logger.Errorf(
+					"connection check failed to close connection for %v: %v",
+					m.inst, cErr,
+				)
 			}
 		}(mnt)
 	}
diff --git a/tests/connection_test.go b/tests/connection_test.go
index de38de4ab..b61a74fbd 100644
--- a/tests/connection_test.go
+++ b/tests/connection_test.go
@@ -100,7 +100,7 @@ func testHealthCheck(t *testing.T, connName string) {
 	ctx, cancel := context.WithTimeout(context.Background(), connTestTimeout)
 	defer cancel()
 
-	args := []string{connName, "--health-check"}
+	args := []string{connName, "--min-ready-instances", "1", "--health-check"}
 	// Start the proxy.
 	p, err := StartProxy(ctx, args...)
 	if err != nil {

From 3b9367a279e2e665f4fa91e58eab7c5477e12ea7 Mon Sep 17 00:00:00 2001
From: Eno Compton <enocom@google.com>
Date: Wed, 19 Oct 2022 14:21:44 -0600
Subject: [PATCH 02/13] Correct behavior with 0

---
 internal/healthcheck/healthcheck.go      | 5 +----
 internal/healthcheck/healthcheck_test.go | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go
index fb57c4ac2..bf1278769 100644
--- a/internal/healthcheck/healthcheck.go
+++ b/internal/healthcheck/healthcheck.go
@@ -107,10 +107,7 @@ func ready(err error, minReady uint64) bool {
 	if !ok {
 		return false
 	}
-	if uint64(len(mErr)) > minReady {
-		return false
-	}
-	return true
+	return minReady != 0 && minReady <= uint64(len(mErr))
 }
 
 // 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 e1913f7ac..c8d3a31ad 100644
--- a/internal/healthcheck/healthcheck_test.go
+++ b/internal/healthcheck/healthcheck_test.go
@@ -264,7 +264,7 @@ func TestReadinessWithMinReady(t *testing.T) {
 	}{
 		{
 			desc:       "when all instances must be ready",
-			minReady:   0,
+			minReady:   2,
 			wantStatus: http.StatusServiceUnavailable,
 		},
 		{

From 8ea844564f1ec8983c1ce6ba3187554100a68c50 Mon Sep 17 00:00:00 2001
From: Eno Compton <enocom@google.com>
Date: Wed, 19 Oct 2022 14:31:23 -0600
Subject: [PATCH 03/13] Correct typo

---
 internal/healthcheck/healthcheck_test.go | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go
index c8d3a31ad..72d400fff 100644
--- a/internal/healthcheck/healthcheck_test.go
+++ b/internal/healthcheck/healthcheck_test.go
@@ -72,13 +72,13 @@ func (*fakeDialer) Close() error {
 	return nil
 }
 
-type flakyDialer struct {
+type flakeyDialer struct {
 	dialCount uint64
 	fakeDialer
 }
 
 // Dial fails on odd calls and succeeds on even calls.
-func (f *flakyDialer) Dial(_ context.Context, _ string, _ ...cloudsqlconn.DialOption) (net.Conn, error) {
+func (f *flakeyDialer) Dial(_ context.Context, _ string, _ ...cloudsqlconn.DialOption) (net.Conn, error) {
 	c := atomic.AddUint64(&f.dialCount, 1)
 	if c%2 == 0 {
 		conn, _ := net.Pipe()
@@ -276,7 +276,7 @@ func TestReadinessWithMinReady(t *testing.T) {
 	p := newProxyWithParams(t, 0,
 		// for every two calls, flaky dialer fails for the first, succeeds for
 		// the second
-		&flakyDialer{},
+		&flakeyDialer{},
 		[]proxy.InstanceConnConfig{
 			{Name: "p:r:instance-1"},
 			{Name: "p:r:instance-2"},

From 78dbd921c03269398ebf0c00b15f2d4906c40014 Mon Sep 17 00:00:00 2001
From: Eno Compton <enocom@google.com>
Date: Fri, 21 Oct 2022 09:27:59 -0600
Subject: [PATCH 04/13] Determine ready from total

---
 internal/healthcheck/healthcheck.go | 14 ++++++++++----
 internal/proxy/fuse_test.go         |  6 +++++-
 internal/proxy/proxy.go             | 13 +++++++------
 internal/proxy/proxy_test.go        | 11 +++++++++--
 4 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go
index bf1278769..b0da1fc18 100644
--- a/internal/healthcheck/healthcheck.go
+++ b/internal/healthcheck/healthcheck.go
@@ -90,8 +90,8 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, _ *http.Request) {
 		return
 	}
 
-	err := c.proxy.CheckConnections(ctx)
-	if err != nil && !ready(err, c.minReady) {
+	n, err := c.proxy.CheckConnections(ctx)
+	if err != nil && !ready(err, c.minReady, n) {
 		c.logger.Errorf("[Health Check] Readiness failed: %v", err)
 		w.WriteHeader(http.StatusServiceUnavailable)
 		w.Write([]byte(err.Error()))
@@ -102,12 +102,18 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, _ *http.Request) {
 	w.Write([]byte("ok"))
 }
 
-func ready(err error, minReady uint64) bool {
+func ready(err error, minReady uint64, total int) bool {
 	mErr, ok := err.(proxy.MultiErr)
 	if !ok {
 		return false
 	}
-	return minReady != 0 && minReady <= uint64(len(mErr))
+	if minReady == 0 {
+		return false
+	}
+	notReady := len(mErr)
+	areReady := total - notReady
+	return areReady >= int(minReady)
+
 }
 
 // HandleLiveness indicates the process is up and responding to HTTP requests.
diff --git a/internal/proxy/fuse_test.go b/internal/proxy/fuse_test.go
index 823570aba..2068e4618 100644
--- a/internal/proxy/fuse_test.go
+++ b/internal/proxy/fuse_test.go
@@ -287,9 +287,13 @@ func TestFUSECheckConnections(t *testing.T) {
 	conn := tryDialUnix(t, filepath.Join(fuseDir, "proj:reg:mysql"))
 	defer conn.Close()
 
-	if err := c.CheckConnections(context.Background()); err != nil {
+	n, err := c.CheckConnections(context.Background())
+	if err != nil {
 		t.Fatalf("c.CheckConnections(): %v", err)
 	}
+	if want, got := 1, n; want != got {
+		t.Fatalf("CheckConnections number of connections: want = %v, got = %v", want, got)
+	}
 
 	// verify the dialer was invoked twice, once for connect, once for check
 	// connection
diff --git a/internal/proxy/proxy.go b/internal/proxy/proxy.go
index 79404d6d8..4106b5c53 100644
--- a/internal/proxy/proxy.go
+++ b/internal/proxy/proxy.go
@@ -421,9 +421,9 @@ func NewClient(ctx context.Context, d cloudsql.Dialer, l cloudsql.Logger, conf *
 	return c, nil
 }
 
-// CheckConnections dials each registered instance and reports any errors that
-// may have occurred.
-func (c *Client) CheckConnections(ctx context.Context) error {
+// CheckConnections dials each registered instance and reports the number of
+// connections checked and any errors that may have occurred.
+func (c *Client) CheckConnections(ctx context.Context) (int, error) {
 	var (
 		wg    sync.WaitGroup
 		errCh = make(chan error, len(c.mnts))
@@ -453,7 +453,7 @@ func (c *Client) CheckConnections(ctx context.Context) error {
 	wg.Wait()
 
 	var mErr MultiErr
-	for i := 0; i < len(c.mnts); i++ {
+	for i := 0; i < len(mnts); i++ {
 		select {
 		case err := <-errCh:
 			mErr = append(mErr, err)
@@ -461,10 +461,11 @@ func (c *Client) CheckConnections(ctx context.Context) error {
 			continue
 		}
 	}
+	mLen := len(mnts)
 	if len(mErr) > 0 {
-		return mErr
+		return mLen, mErr
 	}
-	return nil
+	return mLen, nil
 }
 
 // ConnCount returns the number of open connections and the maximum allowed
diff --git a/internal/proxy/proxy_test.go b/internal/proxy/proxy_test.go
index 1f04cb6b1..86ceca99e 100644
--- a/internal/proxy/proxy_test.go
+++ b/internal/proxy/proxy_test.go
@@ -586,9 +586,13 @@ func TestCheckConnections(t *testing.T) {
 	defer c.Close()
 	go c.Serve(context.Background(), func() {})
 
-	if err = c.CheckConnections(context.Background()); err != nil {
+	n, err := c.CheckConnections(context.Background())
+	if err != nil {
 		t.Fatalf("CheckConnections failed: %v", err)
 	}
+	if want, got := len(in.Instances), n; want != got {
+		t.Fatalf("CheckConnections number of connections: want = %v, got = %v", want, got)
+	}
 
 	if want, got := 1, d.dialAttempts(); want != got {
 		t.Fatalf("dial attempts: want = %v, got = %v", want, got)
@@ -610,8 +614,11 @@ func TestCheckConnections(t *testing.T) {
 	defer c.Close()
 	go c.Serve(context.Background(), func() {})
 
-	err = c.CheckConnections(context.Background())
+	n, err = c.CheckConnections(context.Background())
 	if err == nil {
 		t.Fatal("CheckConnections should have failed, but did not")
 	}
+	if want, got := len(in.Instances), n; want != got {
+		t.Fatalf("CheckConnections number of connections: want = %v, got = %v", want, got)
+	}
 }

From c7374d3b32d0f9086a2e9479ca6dfab7389c0794 Mon Sep 17 00:00:00 2001
From: Eno Compton <enocom@google.com>
Date: Fri, 21 Oct 2022 11:29:50 -0600
Subject: [PATCH 05/13] Convert min ready into query param

---
 cmd/root.go                              |  5 +--
 internal/healthcheck/healthcheck.go      | 40 +++++++++++++++---------
 internal/healthcheck/healthcheck_test.go | 37 +++++++++++++++-------
 tests/connection_test.go                 |  2 +-
 4 files changed, 53 insertions(+), 31 deletions(-)

diff --git a/cmd/root.go b/cmd/root.go
index 2df70ef46..a088e2e76 100644
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -80,7 +80,6 @@ type Command struct {
 	prometheus                 bool
 	prometheusNamespace        string
 	healthCheck                bool
-	minReady                   uint64
 	httpAddress                string
 	httpPort                   string
 
@@ -289,8 +288,6 @@ func NewCommand(opts ...Option) *Command {
 		"Port for Prometheus and health check server")
 	cmd.PersistentFlags().BoolVar(&c.healthCheck, "health-check", false,
 		"Enables health check endpoints /startup, /liveness, and /readiness on localhost.")
-	cmd.PersistentFlags().Uint64Var(&c.minReady, "min-ready-instances", 0,
-		"Configures the minimum ready number of instances for the readiness check to pass")
 	cmd.PersistentFlags().StringVar(&c.conf.APIEndpointURL, "sqladmin-api-endpoint", "",
 		"API endpoint for all Cloud SQL Admin API requests. (default: https://sqladmin.googleapis.com)")
 	cmd.PersistentFlags().StringVar(&c.conf.QuotaProject, "quota-project", "",
@@ -623,7 +620,7 @@ func runSignalWrapper(cmd *Command) error {
 	notify := func() {}
 	if cmd.healthCheck {
 		needsHTTPServer = true
-		hc := healthcheck.NewCheck(p, cmd.logger, cmd.minReady)
+		hc := healthcheck.NewCheck(p, cmd.logger)
 		mux.HandleFunc("/startup", hc.HandleStartup)
 		mux.HandleFunc("/readiness", hc.HandleReadiness)
 		mux.HandleFunc("/liveness", hc.HandleLiveness)
diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go
index b0da1fc18..3268a714a 100644
--- a/internal/healthcheck/healthcheck.go
+++ b/internal/healthcheck/healthcheck.go
@@ -20,6 +20,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"strconv"
 	"sync"
 
 	"github.com/GoogleCloudPlatform/cloud-sql-proxy/v2/cloudsql"
@@ -29,21 +30,19 @@ import (
 // Check provides HTTP handlers for use as healthchecks typically in a
 // Kubernetes context.
 type Check struct {
-	once     *sync.Once
-	started  chan struct{}
-	proxy    *proxy.Client
-	logger   cloudsql.Logger
-	minReady uint64
+	once    *sync.Once
+	started chan struct{}
+	proxy   *proxy.Client
+	logger  cloudsql.Logger
 }
 
 // NewCheck is the initializer for Check.
-func NewCheck(p *proxy.Client, l cloudsql.Logger, minReady uint64) *Check {
+func NewCheck(p *proxy.Client, l cloudsql.Logger) *Check {
 	return &Check{
-		once:     &sync.Once{},
-		started:  make(chan struct{}),
-		proxy:    p,
-		logger:   l,
-		minReady: minReady,
+		once:    &sync.Once{},
+		started: make(chan struct{}),
+		proxy:   p,
+		logger:  l,
 	}
 }
 
@@ -69,7 +68,7 @@ var errNotStarted = errors.New("proxy is not started")
 // HandleReadiness ensures the Check has been notified of successful startup,
 // that the proxy has not reached maximum connections, and that all connections
 // are healthy.
-func (c *Check) HandleReadiness(w http.ResponseWriter, _ *http.Request) {
+func (c *Check) HandleReadiness(w http.ResponseWriter, req *http.Request) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
@@ -90,8 +89,19 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, _ *http.Request) {
 		return
 	}
 
+	var minReady int
+	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
+		}
+	}
+
 	n, err := c.proxy.CheckConnections(ctx)
-	if err != nil && !ready(err, c.minReady, n) {
+	if err != nil && !ready(err, minReady, n) {
 		c.logger.Errorf("[Health Check] Readiness failed: %v", err)
 		w.WriteHeader(http.StatusServiceUnavailable)
 		w.Write([]byte(err.Error()))
@@ -102,7 +112,7 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, _ *http.Request) {
 	w.Write([]byte("ok"))
 }
 
-func ready(err error, minReady uint64, total int) bool {
+func ready(err error, minReady, total int) bool {
 	mErr, ok := err.(proxy.MultiErr)
 	if !ok {
 		return false
@@ -112,7 +122,7 @@ func ready(err error, minReady uint64, total int) bool {
 	}
 	notReady := len(mErr)
 	areReady := total - notReady
-	return areReady >= int(minReady)
+	return areReady >= minReady
 
 }
 
diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go
index 72d400fff..c88a9ebb2 100644
--- a/internal/healthcheck/healthcheck_test.go
+++ b/internal/healthcheck/healthcheck_test.go
@@ -22,6 +22,7 @@ import (
 	"net"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"os"
 	"strings"
 	"sync/atomic"
@@ -130,7 +131,7 @@ func TestHandleStartupWhenNotNotified(t *testing.T) {
 			t.Logf("failed to close proxy client: %v", err)
 		}
 	}()
-	check := healthcheck.NewCheck(p, logger, 0)
+	check := healthcheck.NewCheck(p, logger)
 
 	rec := httptest.NewRecorder()
 	check.HandleStartup(rec, &http.Request{})
@@ -150,7 +151,7 @@ func TestHandleStartupWhenNotified(t *testing.T) {
 			t.Logf("failed to close proxy client: %v", err)
 		}
 	}()
-	check := healthcheck.NewCheck(p, logger, 0)
+	check := healthcheck.NewCheck(p, logger)
 
 	check.NotifyStarted()
 
@@ -170,7 +171,7 @@ func TestHandleReadinessWhenNotNotified(t *testing.T) {
 			t.Logf("failed to close proxy client: %v", err)
 		}
 	}()
-	check := healthcheck.NewCheck(p, logger, 0)
+	check := healthcheck.NewCheck(p, logger)
 
 	rec := httptest.NewRecorder()
 	check.HandleReadiness(rec, &http.Request{})
@@ -189,7 +190,7 @@ func TestHandleReadinessForMaxConns(t *testing.T) {
 		}
 	}()
 	started := make(chan struct{})
-	check := healthcheck.NewCheck(p, logger, 0)
+	check := healthcheck.NewCheck(p, logger)
 	go p.Serve(context.Background(), func() {
 		check.NotifyStarted()
 		close(started)
@@ -216,6 +217,7 @@ func TestHandleReadinessForMaxConns(t *testing.T) {
 			}
 			time.Sleep(time.Second)
 		}
+		t.Fatalf("failed to receive status code = %v", wantCode)
 		return nil
 	}
 	resp := waitForConnect(t, http.StatusServiceUnavailable)
@@ -236,7 +238,7 @@ func TestHandleReadinessWithConnectionProblems(t *testing.T) {
 			t.Logf("failed to close proxy client: %v", err)
 		}
 	}()
-	check := healthcheck.NewCheck(p, logger, 0)
+	check := healthcheck.NewCheck(p, logger)
 	check.NotifyStarted()
 
 	rec := httptest.NewRecorder()
@@ -259,19 +261,29 @@ func TestHandleReadinessWithConnectionProblems(t *testing.T) {
 func TestReadinessWithMinReady(t *testing.T) {
 	tcs := []struct {
 		desc       string
-		minReady   uint64
+		minReady   string
 		wantStatus int
 	}{
 		{
 			desc:       "when all instances must be ready",
-			minReady:   2,
+			minReady:   "2",
 			wantStatus: http.StatusServiceUnavailable,
 		},
 		{
 			desc:       "when only one instance must be ready",
-			minReady:   1,
+			minReady:   "1",
 			wantStatus: http.StatusOK,
 		},
+		{
+			desc:       "when min ready is not configured",
+			minReady:   "0",
+			wantStatus: http.StatusServiceUnavailable,
+		},
+		{
+			desc:       "when min ready is bogus",
+			minReady:   "bogus",
+			wantStatus: http.StatusServiceUnavailable,
+		},
 	}
 	p := newProxyWithParams(t, 0,
 		// for every two calls, flaky dialer fails for the first, succeeds for
@@ -290,11 +302,14 @@ func TestReadinessWithMinReady(t *testing.T) {
 
 	for _, tc := range tcs {
 		t.Run(tc.desc, func(t *testing.T) {
-			check := healthcheck.NewCheck(p, logger, tc.minReady)
+			check := healthcheck.NewCheck(p, logger)
 			check.NotifyStarted()
-
+			u, err := url.Parse(fmt.Sprintf("/readiness?min-ready=%s", tc.minReady))
+			if err != nil {
+				t.Fatal(err)
+			}
 			rec := httptest.NewRecorder()
-			check.HandleReadiness(rec, &http.Request{})
+			check.HandleReadiness(rec, &http.Request{URL: u})
 
 			resp := rec.Result()
 			if got, want := resp.StatusCode, tc.wantStatus; got != want {
diff --git a/tests/connection_test.go b/tests/connection_test.go
index b61a74fbd..de38de4ab 100644
--- a/tests/connection_test.go
+++ b/tests/connection_test.go
@@ -100,7 +100,7 @@ func testHealthCheck(t *testing.T, connName string) {
 	ctx, cancel := context.WithTimeout(context.Background(), connTestTimeout)
 	defer cancel()
 
-	args := []string{connName, "--min-ready-instances", "1", "--health-check"}
+	args := []string{connName, "--health-check"}
 	// Start the proxy.
 	p, err := StartProxy(ctx, args...)
 	if err != nil {

From e11d22653e280bbf8aba99e0d58eb6cffa83bbdc Mon Sep 17 00:00:00 2001
From: Eno Compton <enocom@google.com>
Date: Fri, 21 Oct 2022 12:24:21 -0600
Subject: [PATCH 06/13] Expand docs with test fixes

---
 cmd/root.go                              | 25 +++++++++++++++++++++++-
 internal/healthcheck/healthcheck.go      |  5 +++++
 internal/healthcheck/healthcheck_test.go | 25 ++++++++++++++----------
 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/cmd/root.go b/cmd/root.go
index a088e2e76..fb436b7ad 100644
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -174,6 +174,27 @@ Instance Level Configuration
     	    my-project:us-central1:my-db-server \
     	    'my-project:us-central1:my-other-server?address=0.0.0.0&port=7000'
 
+Health checks
+
+    When enabling the --health-checks flag, the proxy will start an HTTP server
+    on localhost with three endpoints:
+
+    - /startup: Returns 200 status when the proxy has finished starting up.
+    Otherwise returns 503 status.
+
+    - /readiness: Returns 200 status when the proxy has started, has available
+    connections if max connections have been set with the --max-connections
+    flag, and when the proxy can connect to all registered instances. Otherwise,
+    returns a 503 status. Optionally supports a min-ready query param (e.g.,
+    /readiness?min-ready=3) where the proxy will return a 200 status if the
+    proxy can connect successfully to at least min-ready number of instances. If
+    min-ready exceeds the number of registered instances, returns a 400.
+
+    - /liveness: Always returns 200 status. If this endpoint is not responding,
+    the proxy is in a bad state and should be restarted.
+
+    To configure the address, use --http-server.
+
 Service Account Impersonation
 
     The proxy supports service account impersonation with the
@@ -620,6 +641,8 @@ func runSignalWrapper(cmd *Command) error {
 	notify := func() {}
 	if cmd.healthCheck {
 		needsHTTPServer = true
+		cmd.logger.Infof("Starting health check server at %s",
+			net.JoinHostPort(cmd.httpAddress, cmd.httpPort))
 		hc := healthcheck.NewCheck(p, cmd.logger)
 		mux.HandleFunc("/startup", hc.HandleStartup)
 		mux.HandleFunc("/readiness", hc.HandleReadiness)
@@ -630,7 +653,7 @@ func runSignalWrapper(cmd *Command) error {
 	// Start the HTTP server if anything requiring HTTP is specified.
 	if needsHTTPServer {
 		server := &http.Server{
-			Addr:    fmt.Sprintf("%s:%s", cmd.httpAddress, cmd.httpPort),
+			Addr:    net.JoinHostPort(cmd.httpAddress, cmd.httpPort),
 			Handler: mux,
 		}
 		// Start the HTTP server.
diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go
index 3268a714a..b3c2925ec 100644
--- a/internal/healthcheck/healthcheck.go
+++ b/internal/healthcheck/healthcheck.go
@@ -101,6 +101,11 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, req *http.Request) {
 	}
 
 	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) {
 		c.logger.Errorf("[Health Check] Readiness failed: %v", err)
 		w.WriteHeader(http.StatusServiceUnavailable)
diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go
index c88a9ebb2..58dbc6e14 100644
--- a/internal/healthcheck/healthcheck_test.go
+++ b/internal/healthcheck/healthcheck_test.go
@@ -134,7 +134,7 @@ func TestHandleStartupWhenNotNotified(t *testing.T) {
 	check := healthcheck.NewCheck(p, logger)
 
 	rec := httptest.NewRecorder()
-	check.HandleStartup(rec, &http.Request{})
+	check.HandleStartup(rec, &http.Request{URL: &url.URL{}})
 
 	// Startup is not complete because the Check has not been notified of the
 	// proxy's startup.
@@ -156,7 +156,7 @@ func TestHandleStartupWhenNotified(t *testing.T) {
 	check.NotifyStarted()
 
 	rec := httptest.NewRecorder()
-	check.HandleStartup(rec, &http.Request{})
+	check.HandleStartup(rec, &http.Request{URL: &url.URL{}})
 
 	resp := rec.Result()
 	if got, want := resp.StatusCode, http.StatusOK; got != want {
@@ -174,7 +174,7 @@ func TestHandleReadinessWhenNotNotified(t *testing.T) {
 	check := healthcheck.NewCheck(p, logger)
 
 	rec := httptest.NewRecorder()
-	check.HandleReadiness(rec, &http.Request{})
+	check.HandleReadiness(rec, &http.Request{URL: &url.URL{}})
 
 	resp := rec.Result()
 	if got, want := resp.StatusCode, http.StatusServiceUnavailable; got != want {
@@ -210,7 +210,7 @@ func TestHandleReadinessForMaxConns(t *testing.T) {
 	waitForConnect := func(t *testing.T, wantCode int) *http.Response {
 		for i := 0; i < 10; i++ {
 			rec := httptest.NewRecorder()
-			check.HandleReadiness(rec, &http.Request{})
+			check.HandleReadiness(rec, &http.Request{URL: &url.URL{}})
 			resp := rec.Result()
 			if resp.StatusCode == wantCode {
 				return resp
@@ -242,7 +242,7 @@ func TestHandleReadinessWithConnectionProblems(t *testing.T) {
 	check.NotifyStarted()
 
 	rec := httptest.NewRecorder()
-	check.HandleReadiness(rec, &http.Request{})
+	check.HandleReadiness(rec, &http.Request{URL: &url.URL{}})
 
 	resp := rec.Result()
 	if got, want := resp.StatusCode, http.StatusServiceUnavailable; got != want {
@@ -264,16 +264,16 @@ func TestReadinessWithMinReady(t *testing.T) {
 		minReady   string
 		wantStatus int
 	}{
-		{
-			desc:       "when all instances must be ready",
-			minReady:   "2",
-			wantStatus: http.StatusServiceUnavailable,
-		},
 		{
 			desc:       "when only one instance must be ready",
 			minReady:   "1",
 			wantStatus: http.StatusOK,
 		},
+		{
+			desc:       "when all instances must be ready",
+			minReady:   "2",
+			wantStatus: http.StatusServiceUnavailable,
+		},
 		{
 			desc:       "when min ready is not configured",
 			minReady:   "0",
@@ -284,6 +284,11 @@ func TestReadinessWithMinReady(t *testing.T) {
 			minReady:   "bogus",
 			wantStatus: http.StatusServiceUnavailable,
 		},
+		{
+			desc:       "when min ready is greater than registered instances",
+			minReady:   "100",
+			wantStatus: http.StatusBadRequest,
+		},
 	}
 	p := newProxyWithParams(t, 0,
 		// for every two calls, flaky dialer fails for the first, succeeds for

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 07/13] 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,

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 08/13] 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))

From d44c38a45d91dda106126985f7d136ced31bd082 Mon Sep 17 00:00:00 2001
From: Eno Compton <enocom@google.com>
Date: Wed, 26 Oct 2022 09:35:24 -0600
Subject: [PATCH 09/13] 400 when min-ready exceeds total

---
 internal/healthcheck/healthcheck.go      | 18 +++++++++---------
 internal/healthcheck/healthcheck_test.go |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go
index def59b249..838cea655 100644
--- a/internal/healthcheck/healthcheck.go
+++ b/internal/healthcheck/healthcheck.go
@@ -103,9 +103,9 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, req *http.Request) {
 	}
 
 	n, err := c.proxy.CheckConnections(ctx)
-	if rErr := ready(err, minReady, n); rErr != nil {
+	if status, rErr := ready(err, minReady, n); rErr != nil {
 		c.logger.Errorf("[Health Check] Readiness failed: %v", rErr)
-		w.WriteHeader(http.StatusServiceUnavailable)
+		w.WriteHeader(status)
 		w.Write([]byte(rErr.Error()))
 		return
 	}
@@ -114,31 +114,31 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, req *http.Request) {
 	w.Write([]byte("ok"))
 }
 
-func ready(err error, minReady *int, total int) error {
+func ready(err error, minReady *int, total int) (int, error) {
 	// If err is nil, then the proxy is ready.
 	if err == nil {
 		if minReady != nil && *minReady > total {
-			return fmt.Errorf(
+			return http.StatusBadRequest, fmt.Errorf(
 				"min-ready (%v) is greater than registered instances (%v)",
 				*minReady, total,
 			)
 		}
-		return nil
+		return http.StatusOK, nil
 	}
 	// When minReady is not configured, any error means the proxy is not ready.
 	if minReady == nil {
-		return err
+		return http.StatusServiceUnavailable, err
 	}
 	mErr, ok := err.(proxy.MultiErr)
 	if !ok {
-		return err
+		return http.StatusServiceUnavailable, err
 	}
 	notReady := len(mErr)
 	areReady := total - notReady
 	if areReady < *minReady {
-		return err
+		return http.StatusServiceUnavailable, err
 	}
-	return nil
+	return http.StatusOK, 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 b088b5180..2d15de43d 100644
--- a/internal/healthcheck/healthcheck_test.go
+++ b/internal/healthcheck/healthcheck_test.go
@@ -288,7 +288,7 @@ func TestReadinessWithMinReady(t *testing.T) {
 		{
 			desc:       "when min ready is greater than the number of instances",
 			minReady:   "3",
-			wantStatus: http.StatusServiceUnavailable,
+			wantStatus: http.StatusBadRequest,
 			dialer:     &fakeDialer{},
 		},
 		{

From 3fa20f33b3056f18a66c5dd2459fd58b40326f41 Mon Sep 17 00:00:00 2001
From: Eno Compton <enocom@google.com>
Date: Wed, 26 Oct 2022 09:38:11 -0600
Subject: [PATCH 10/13] Min-ready must be greater than zero

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

diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go
index 838cea655..f154776bf 100644
--- a/internal/healthcheck/healthcheck.go
+++ b/internal/healthcheck/healthcheck.go
@@ -99,6 +99,12 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, req *http.Request) {
 			fmt.Fprintf(w, "min-query must be a valid integer, got = %q", v)
 			return
 		}
+		if n <= 0 {
+			c.logger.Errorf("[Health Check] min-ready %q must be greater than zero", v)
+			w.WriteHeader(http.StatusBadRequest)
+			fmt.Fprint(w, "min-query must be greater than zero", v)
+			return
+		}
 		minReady = &n
 	}
 
diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go
index 2d15de43d..e0ebdd50e 100644
--- a/internal/healthcheck/healthcheck_test.go
+++ b/internal/healthcheck/healthcheck_test.go
@@ -266,12 +266,16 @@ func TestReadinessWithMinReady(t *testing.T) {
 		dialer     cloudsql.Dialer
 	}{
 		{
-			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 min ready is zero",
+			minReady:   "0",
+			wantStatus: http.StatusBadRequest,
+			dialer:     &fakeDialer{},
+		},
+		{
+			desc:       "when min ready is less than zero",
+			minReady:   "-1",
+			wantStatus: http.StatusBadRequest,
+			dialer:     &fakeDialer{},
 		},
 		{
 			desc:       "when only one instance must be ready",

From cc1e2cce9e29e821418eddc925267cc2790b95b5 Mon Sep 17 00:00:00 2001
From: Eno Compton <enocom@google.com>
Date: Wed, 26 Oct 2022 09:41:17 -0600
Subject: [PATCH 11/13] Improve error messages

---
 internal/healthcheck/healthcheck.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go
index f154776bf..5846c645e 100644
--- a/internal/healthcheck/healthcheck.go
+++ b/internal/healthcheck/healthcheck.go
@@ -94,7 +94,7 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, req *http.Request) {
 	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)
+			c.logger.Errorf("[Health Check] min-ready must be a valid integer, got = %q", v)
 			w.WriteHeader(http.StatusBadRequest)
 			fmt.Fprintf(w, "min-query must be a valid integer, got = %q", v)
 			return
@@ -125,7 +125,7 @@ func ready(err error, minReady *int, total int) (int, error) {
 	if err == nil {
 		if minReady != nil && *minReady > total {
 			return http.StatusBadRequest, fmt.Errorf(
-				"min-ready (%v) is greater than registered instances (%v)",
+				"min-ready (%v) must be less than or equal to the number of registered instances (%v)",
 				*minReady, total,
 			)
 		}

From 5cf7c937681e06047f180b375eab24f8eed827fc Mon Sep 17 00:00:00 2001
From: Eno Compton <enocom@google.com>
Date: Tue, 1 Nov 2022 10:58:07 -0600
Subject: [PATCH 12/13] Consolidate error handling

---
 internal/healthcheck/healthcheck.go | 70 ++++++++++++++++-------------
 1 file changed, 38 insertions(+), 32 deletions(-)

diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go
index 5846c645e..8409ba7ed 100644
--- a/internal/healthcheck/healthcheck.go
+++ b/internal/healthcheck/healthcheck.go
@@ -108,45 +108,51 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, req *http.Request) {
 		minReady = &n
 	}
 
-	n, err := c.proxy.CheckConnections(ctx)
-	if status, rErr := ready(err, minReady, n); rErr != nil {
-		c.logger.Errorf("[Health Check] Readiness failed: %v", rErr)
-		w.WriteHeader(status)
-		w.Write([]byte(rErr.Error()))
+	total, err := c.proxy.CheckConnections(ctx)
+
+	switch {
+	case minReady != nil && *minReady > total:
+		// When min ready is set and exceeds total instances, 400 status.
+		mErr := fmt.Errorf(
+			"min-ready (%v) must be less than or equal to the number of registered instances (%v)",
+			*minReady, total,
+		)
+		c.logger.Errorf("[Health Check] Readiness failed: %v", mErr)
+
+		w.WriteHeader(http.StatusBadRequest)
+		w.Write([]byte(mErr.Error()))
+	case err != nil && minReady != nil:
+		// When there's an error and min ready is set, AND min ready instances
+		// are not ready, 503 status.
+		c.logger.Errorf("[Health Check] Readiness failed: %v", err)
+
+		mErr, ok := err.(proxy.MultiErr)
+		if !ok {
+			// If the err is not a MultiErr, just return it as is.
+			w.WriteHeader(http.StatusServiceUnavailable)
+			w.Write([]byte(err.Error()))
+			return
+		}
+
+		areReady := total - len(mErr)
+		if areReady < *minReady {
+			w.WriteHeader(http.StatusServiceUnavailable)
+			w.Write([]byte(err.Error()))
+			return
+		}
+	case err != nil:
+		// When there's just an error without min-ready: 503 status.
+		c.logger.Errorf("[Health Check] Readiness failed: %v", err)
+		w.WriteHeader(http.StatusServiceUnavailable)
+		w.Write([]byte(err.Error()))
 		return
 	}
 
+	// No error cases apply, 200 status.
 	w.WriteHeader(http.StatusOK)
 	w.Write([]byte("ok"))
 }
 
-func ready(err error, minReady *int, total int) (int, error) {
-	// If err is nil, then the proxy is ready.
-	if err == nil {
-		if minReady != nil && *minReady > total {
-			return http.StatusBadRequest, fmt.Errorf(
-				"min-ready (%v) must be less than or equal to the number of registered instances (%v)",
-				*minReady, total,
-			)
-		}
-		return http.StatusOK, nil
-	}
-	// When minReady is not configured, any error means the proxy is not ready.
-	if minReady == nil {
-		return http.StatusServiceUnavailable, err
-	}
-	mErr, ok := err.(proxy.MultiErr)
-	if !ok {
-		return http.StatusServiceUnavailable, err
-	}
-	notReady := len(mErr)
-	areReady := total - notReady
-	if areReady < *minReady {
-		return http.StatusServiceUnavailable, err
-	}
-	return http.StatusOK, nil
-}
-
 // HandleLiveness indicates the process is up and responding to HTTP requests.
 // If this check fails (because it's not reachable), the process is in a bad
 // state and should be restarted.

From c6fee01b9a52ad2fea59d426dfbdbe6b7be9bb5b Mon Sep 17 00:00:00 2001
From: Eno Compton <enocom@google.com>
Date: Tue, 1 Nov 2022 10:58:58 -0600
Subject: [PATCH 13/13] Add missing return

---
 internal/healthcheck/healthcheck.go | 1 +
 1 file changed, 1 insertion(+)

diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go
index 8409ba7ed..1491d150c 100644
--- a/internal/healthcheck/healthcheck.go
+++ b/internal/healthcheck/healthcheck.go
@@ -121,6 +121,7 @@ func (c *Check) HandleReadiness(w http.ResponseWriter, req *http.Request) {
 
 		w.WriteHeader(http.StatusBadRequest)
 		w.Write([]byte(mErr.Error()))
+		return
 	case err != nil && minReady != nil:
 		// When there's an error and min ready is set, AND min ready instances
 		// are not ready, 503 status.