From 122f7cffdb6dc2daafd37542fc29197171e2cc9c Mon Sep 17 00:00:00 2001
From: Ryan Richard <richardry@vmware.com>
Date: Wed, 21 Oct 2020 15:24:48 -0700
Subject: [PATCH] Make the supervisor healthz endpoint public

Based on our experiences today with GKE, it will be easier for our users
to configure Ingress health checks if the healthz endpoint is available
on the same public port as the OIDC endpoints.

Also add an integration test for the healthz endpoint now that it is
public.

Also add the optional `containers[].ports.containerPort` to the
supervisor Deployment because the GKE docs say that GKE will look
at that field while inferring how to invoke the health endpoint. See
https://cloud.google.com/kubernetes-engine/docs/concepts/ingress#def_inf_hc
---
 cmd/pinniped-supervisor/main.go             | 21 ++++----
 deploy/supervisor/deployment.yaml           |  7 ++-
 test/integration/supervisor_healthz_test.go | 53 +++++++++++++++++++++
 3 files changed, 66 insertions(+), 15 deletions(-)
 create mode 100644 test/integration/supervisor_healthz_test.go

diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go
index b8a59949..346f83ca 100644
--- a/cmd/pinniped-supervisor/main.go
+++ b/cmd/pinniped-supervisor/main.go
@@ -159,8 +159,15 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error {
 		pinnipedinformers.WithNamespace(serverInstallationNamespace),
 	)
 
+	// Serve the /healthz endpoint and make all other paths result in 404.
+	healthMux := http.NewServeMux()
+	healthMux.Handle("/healthz", http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
+		_, _ = writer.Write([]byte("ok"))
+	}))
+
 	dynamicJWKSProvider := jwks.NewDynamicJWKSProvider()
-	oidProvidersManager := manager.NewManager(http.NotFoundHandler(), dynamicJWKSProvider)
+	// OIDC endpoints will be served by the oidProvidersManager, and any non-OIDC paths will fallback to the healthMux.
+	oidProvidersManager := manager.NewManager(healthMux, dynamicJWKSProvider)
 	startControllers(ctx, cfg, oidProvidersManager, dynamicJWKSProvider, kubeClient, pinnipedClient, kubeInformers, pinnipedInformers)
 
 	//nolint: gosec // Intentionally binding to all network interfaces.
@@ -171,18 +178,6 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error {
 	defer l.Close()
 	start(ctx, l, oidProvidersManager)
 
-	//nolint: gosec // Intentionally binding to all network interfaces.
-	healthzListener, err := net.Listen("tcp", ":8080")
-	if err != nil {
-		return fmt.Errorf("cannot create healthzListener: %w", err)
-	}
-	defer healthzListener.Close()
-	healthzMux := http.NewServeMux()
-	healthzMux.Handle("/healthz", http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
-		_, _ = writer.Write([]byte("ok"))
-	}))
-	start(ctx, healthzListener, healthzMux)
-
 	klog.InfoS("supervisor is ready", "address", l.Addr().String())
 
 	gotSignal := waitForSignal()
diff --git a/deploy/supervisor/deployment.yaml b/deploy/supervisor/deployment.yaml
index 764c71d7..afa4efd5 100644
--- a/deploy/supervisor/deployment.yaml
+++ b/deploy/supervisor/deployment.yaml
@@ -84,10 +84,13 @@ spec:
               mountPath: /etc/config
             - name: podinfo
               mountPath: /etc/podinfo
+          ports:
+            - containerPort: 80
+              protocol: TCP
           livenessProbe:
             httpGet:
               path: /healthz
-              port: 8080
+              port: 80
               scheme: HTTP
             initialDelaySeconds: 2
             timeoutSeconds: 15
@@ -96,7 +99,7 @@ spec:
           readinessProbe:
             httpGet:
               path: /healthz
-              port: 8080
+              port: 80
               scheme: HTTP
             initialDelaySeconds: 2
             timeoutSeconds: 3
diff --git a/test/integration/supervisor_healthz_test.go b/test/integration/supervisor_healthz_test.go
new file mode 100644
index 00000000..1691be40
--- /dev/null
+++ b/test/integration/supervisor_healthz_test.go
@@ -0,0 +1,53 @@
+// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package integration
+
+import (
+	"context"
+	"fmt"
+	"io/ioutil"
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+
+	"go.pinniped.dev/test/library"
+)
+
+// The Supervisor health endpoint is public because that makes it easier
+// for users to create an Ingress for the supervisor on platforms like
+// GKE where the Ingress wants to perform a health check. It's somewhere
+// between inconvenient and impossible to make that Ingress health check
+// happen on a private container port at this time.
+// This test checks that it is working and that it is public.
+func TestSupervisorHealthz(t *testing.T) {
+	env := library.IntegrationEnv(t)
+
+	if env.SupervisorHTTPAddress == "" {
+		t.Skip("PINNIPED_TEST_SUPERVISOR_HTTP_ADDRESS not defined")
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+	defer cancel()
+
+	requestHealthEndpoint, err := http.NewRequestWithContext(
+		ctx,
+		http.MethodGet,
+		fmt.Sprintf("http://%s/healthz", env.SupervisorHTTPAddress),
+		nil,
+	)
+	require.NoError(t, err)
+
+	httpClient := &http.Client{}
+	response, err := httpClient.Do(requestHealthEndpoint) //nolint:bodyclose
+	require.NoError(t, err)
+	require.Equal(t, http.StatusOK, response.StatusCode)
+
+	responseBody, err := ioutil.ReadAll(response.Body)
+	require.NoError(t, err)
+	err = response.Body.Close()
+	require.NoError(t, err)
+	require.Equal(t, "ok", string(responseBody))
+}