pkg/cvo/metrics: Drop HTTP, require HTTPS for metrics access

We began serving metrics over HTTPS with 6132bc3 (Bug 1809195: Send
CVO metrics over https, 2020-05-07, #358), which also requested
monitoring to scrape us over HTTPS.  Now that that is all in place in
4.6, we no longer need to serve over HTTP in 4.7 and later.  This
commit pivots us to always serving over HTTPS.

Because we are no longer serving HTTP, move to requiring
--serving-cert-file and --serving-key-file when --listen is non-empty.
I'd like to drop the --listen default, to make it an explicit opt-in,
but I don't want to lose metrics when folks update from 4.6 -> 4.7.
With this commit we start setting --listen explicitly when we launch
child CVOs, and in 4.8 we can drop:

  ListenAddr: "0.0.0.0:9099",

from pkg/start.  It's possible that the manifest for the incoming CVO
is constructed from the incoming release image, in which case we may
be able to drop the --listen default now.

I'm not setting --listen in the bootstrap manifest, because we don't
need to serve metrics then (it's long before we have Prometheus around
to scrape us).

bootstrap/bootstrap-pod.yaml

@@ -15,6 +15,7 @@ spec:
       - "--release-image={{.ReleaseImage}}"
       - "--enable-auto-update=false"
       - "--enable-default-cluster-version=false"
+      - "--listen="
       - "--v=5"
       - "--kubeconfig=/etc/kubernetes/kubeconfig"
     securityContext:

cmd/start.go

@@ -33,7 +33,7 @@ func init() {
 	cmd.PersistentFlags().BoolVar(&opts.EnableAutoUpdate, "enable-auto-update", opts.EnableAutoUpdate, "Enables the autoupdate controller.")
 	cmd.PersistentFlags().BoolVar(&opts.EnableDefaultClusterVersion, "enable-default-cluster-version", opts.EnableDefaultClusterVersion, "Allows the operator to create a ClusterVersion object if one does not already exist.")
 	cmd.PersistentFlags().StringVar(&opts.ReleaseImage, "release-image", opts.ReleaseImage, "The Openshift release image url.")
-	cmd.PersistentFlags().StringVar(&opts.ServingCertFile, "serving-cert-file", opts.ServingCertFile, "The X.509 certificate file for serving metrics over HTTPS.  You must set both --serving-cert-file and --serving-key-file, or neither.")
-	cmd.PersistentFlags().StringVar(&opts.ServingKeyFile, "serving-key-file", opts.ServingKeyFile, "The X.509 key file for serving metrics over HTTPS.  You must set both --serving-cert-file and --serving-key-file, or neither.")
+	cmd.PersistentFlags().StringVar(&opts.ServingCertFile, "serving-cert-file", opts.ServingCertFile, "The X.509 certificate file for serving metrics over HTTPS.  You must set both --serving-cert-file and --serving-key-file unless you set --listen empty.")
+	cmd.PersistentFlags().StringVar(&opts.ServingKeyFile, "serving-key-file", opts.ServingKeyFile, "The X.509 key file for serving metrics over HTTPS.  You must set both --serving-cert-file and --serving-key-file unless you set --listen empty.")
 	rootCmd.AddCommand(cmd)
 }

install/0000_00_cluster-version-operator_03_deployment.yaml

@@ -29,6 +29,7 @@ spec:
           - "--release-image={{.ReleaseImage}}"
           - "--enable-auto-update=false"
           - "--enable-default-cluster-version=true"
+          - "--listen=0.0.0.0:9099"
           - "--serving-cert-file=/etc/tls/serving-cert/tls.crt"
           - "--serving-key-file=/etc/tls/serving-cert/tls.key"
           - "--v=5"

pkg/cvo/metrics.go

@@ -3,11 +3,11 @@ package cvo
 import (
 	"context"
 	"crypto/tls"
+	"errors"
 	"net"
 	"net/http"
 	"time"
 
-	"github.com/cockroachdb/cmux"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	corev1 "k8s.io/api/core/v1"
@@ -106,12 +106,15 @@ type asyncResult struct {
 }
 
 // RunMetrics launches an server bound to listenAddress serving
-// Prometheus metrics at /metrics over HTTP, and, if tlsConfig is
-// non-nil, also over HTTPS.  Continues serving until runContext.Done()
-// and then attempts a clean shutdown limited by shutdownContext.Done().
-// Assumes runContext.Done() occurs before or simultaneously with
-// shutdownContext.Done().
+// Prometheus metrics at /metrics over HTTPS.  Continues serving
+// until runContext.Done() and then attempts a clean shutdown
+// limited by shutdownContext.Done().  Assumes runContext.Done()
+// occurs before or simultaneously with shutdownContext.Done().
 func RunMetrics(runContext context.Context, shutdownContext context.Context, listenAddress string, tlsConfig *tls.Config) error {
+	if tlsConfig == nil {
+		return errors.New("TLS configuration is required to serve metrics")
+	}
+
 	handler := http.NewServeMux()
 	handler.Handle("/metrics", promhttp.Handler())
 	server := &http.Server{
@@ -123,36 +126,14 @@ func RunMetrics(runContext context.Context, shutdownContext context.Context, lis
 		return err
 	}
 
-	// if a TLS connection was requested, set up a connection mux that will send TLS requests to
-	// the TLS server but send HTTP requests to the HTTP server. Preserves the ability for legacy
-	// HTTP, needed during upgrade, while still allowing TLS certs and end to end metrics protection.
-	mux := cmux.New(tcpListener)
-
 	resultChannel := make(chan asyncResult, 1)
 	resultChannelCount := 1
 
 	go func() {
-		// match HTTP first
-		httpListener := mux.Match(cmux.HTTP1())
-		klog.Infof("Metrics port listening for HTTP on %v", listenAddress)
-		err := server.Serve(httpListener)
-		resultChannel <- asyncResult{name: "HTTP server", error: err}
-	}()
-
-	if tlsConfig != nil {
-		resultChannelCount++
-		go func() {
-			tlsListener := tls.NewListener(mux.Match(cmux.Any()), tlsConfig)
-			klog.Infof("Metrics port listening for HTTPS on %v", listenAddress)
-			err := server.Serve(tlsListener)
-			resultChannel <- asyncResult{name: "HTTPS server", error: err}
-		}()
-	}
-
-	resultChannelCount++
-	go func() {
-		err := mux.Serve()
-		resultChannel <- asyncResult{name: "TCP muxer", error: err}
+		tlsListener := tls.NewListener(tcpListener), tlsConfig)
+		klog.Infof("Metrics port listening for HTTPS on %v", listenAddress)
+		err := server.Serve(tlsListener)
+		resultChannel <- asyncResult{name: "HTTPS server", error: err}
 	}()
 
 	shutdown := false

pkg/start/start.go

@@ -117,11 +117,11 @@ func (o *Options) Run(ctx context.Context) error {
 	if o.ReleaseImage == "" {
 		return fmt.Errorf("missing --release-image flag, it is required")
 	}
-	if o.ServingCertFile == "" && o.ServingKeyFile != "" {
-		return fmt.Errorf("--serving-key-file was set, so --serving-cert-file must also be set")
+	if o.ListenAddr != "" && o.ServingCertFile == "" {
+		return fmt.Errorf("--listen was not set empty, so --serving-cert-file must be set")
 	}
-	if o.ServingKeyFile == "" && o.ServingCertFile != "" {
-		return fmt.Errorf("--serving-cert-file was set, so --serving-key-file must also be set")
+	if o.ListenAddr != "" && o.ServingKeyFile == "" {
+		return fmt.Errorf("--listen was not set empty, so --serving-key-file must be set")
 	}
 	if len(o.PayloadOverride) > 0 {
 		klog.Warningf("Using an override payload directory for testing only: %s", o.PayloadOverride)
@@ -202,12 +202,10 @@ func (o *Options) run(ctx context.Context, controllerCtx *Context, lock *resourc
 	resultChannelCount := 0
 	var tlsConfig *tls.Config
 	if o.ListenAddr != "" {
-		if o.ServingCertFile != "" || o.ServingKeyFile != "" {
-			var err error
-			tlsConfig, err = o.makeTLSConfig()
-			if err != nil {
-				klog.Fatalf("Failed to create TLS config: %v", err)
-			}
+		var err error
+		tlsConfig, err = o.makeTLSConfig()
+		if err != nil {
+			klog.Fatalf("Failed to create TLS config: %v", err)
 		}
 	}