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

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)
 }

go.mod

@@ -4,7 +4,6 @@ go 1.15
 
 require (
 	github.com/blang/semver/v4 v4.0.0
-	github.com/cockroachdb/cmux v0.0.0-20170110192607-30d10be49292
 	github.com/davecgh/go-spew v1.1.1
 	github.com/ghodss/yaml v1.0.0
 	github.com/google/uuid v1.1.2

go.sum

@@ -92,8 +92,6 @@ github.com/chzyer/logex v1.1.10/go.mod h1:+Ywpsq7O8HXn0nuIou7OrIPyXbp3wmkHB+jjWR
 github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e/go.mod h1:nSuG5e5PlCu98SY8svDHJxuZscDgtXS6KTTbou5AhLI=
 github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1/go.mod h1:Q3SI9o4m/ZMnBNeIyt5eFwwo7qiLfzFZmjNmxjkiQlU=
 github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
-github.com/cockroachdb/cmux v0.0.0-20170110192607-30d10be49292 h1:dzj1/xcivGjNPwwifh/dWTczkwcuqsXXFHY1X/TZMtw=
-github.com/cockroachdb/cmux v0.0.0-20170110192607-30d10be49292/go.mod h1:qRiX68mZX1lGBkTWyp3CLcenw9I94W2dLeRvMzcn9N4=
 github.com/cockroachdb/datadriven v0.0.0-20190809214429-80d97fb3cbaa/go.mod h1:zn76sxSg3SzpJ0PPJaLDCu+Bu0Lg3sKTORVIj19EIF8=
 github.com/containerd/continuity v0.0.0-20190827140505-75bee3e2ccb6/go.mod h1:GL3xCUCBDV3CZiTSEKksMWbLE66hEyuu9qyDOOqM47Y=
 github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk=

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)
 		}
 	}