chore: additional changes from code review

Signed-off-by: Mikhail Fedosin <mfedosin@redhat.com>

Author: Fedosin

pkg/deployment/config.go

@@ -236,6 +236,17 @@ func NewConfigFromMap(configMap map[string]string) (*Config, error) {
 		return nil, fmt.Errorf("digest-resolution-timeout cannot be a non-positive duration, was %v", nc.DigestResolutionTimeout)
 	}
 
+	for _, pair := range []struct {
+		key, value string
+	}{
+		{queueSidecarTLSMinVersionKey, nc.QueueSidecarTLSMinVersion},
+		{queueSidecarTLSMaxVersionKey, nc.QueueSidecarTLSMaxVersion},
+	} {
+		if pair.value != "" && pair.value != "1.2" && pair.value != "1.3" {
+			return nil, fmt.Errorf("%s must be \"1.2\", \"1.3\", or empty, got %q", pair.key, pair.value)
+		}
+	}
+
 	if affinity, ok := configMap[defaultAffinityTypeKey]; ok {
 		switch opt := AffinityType(affinity); opt {
 		case None, PreferSpreadRevisionOverNodes:

pkg/deployment/config_test.go

@@ -494,6 +494,20 @@ kata:
 			queueSidecarTLSCipherSuitesKey:     "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
 			queueSidecarTLSCurvePreferencesKey: "X25519,CurveP256",
 		},
+	}, {
+		name:    "invalid queue sidecar TLS min version",
+		wantErr: true,
+		data: map[string]string{
+			QueueSidecarImageKey:         defaultSidecarImage,
+			queueSidecarTLSMinVersionKey: "1.1",
+		},
+	}, {
+		name:    "invalid queue sidecar TLS max version",
+		wantErr: true,
+		data: map[string]string{
+			QueueSidecarImageKey:         defaultSidecarImage,
+			queueSidecarTLSMaxVersionKey: "invalid",
+		},
 	}}
 
 	for _, tt := range configTests {

pkg/queue/sharedmain/main.go

@@ -18,7 +18,6 @@ package sharedmain
 
 import (
 	"context"
-	"crypto/tls"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -293,20 +292,22 @@ func Main(opts ...Option) error {
 			}
 		}(name, server)
 	}
-	tlsCfg, err := queueProxyTLSConfig()
-	if err != nil {
-		logger.Fatalw("Failed to read TLS configuration from environment", zap.Error(err))
-	}
-	for name, server := range tlsServers {
-		go func(name string, s *http.Server) {
-			logger.Info("Starting tls server ", name, s.Addr)
-			s.TLSConfig = tlsCfg.TLSConfig()
-			s.TLSConfig.GetCertificate = certWatcher.GetCertificate
-			// Don't forward ErrServerClosed as that indicates we're already shutting down.
-			if err := s.ListenAndServeTLS("", ""); err != nil && !errors.Is(err, http.ErrServerClosed) {
-				errCh <- fmt.Errorf("%s server failed to serve: %w", name, err)
-			}
-		}(name, server)
+	if tlsEnabled {
+		tlsCfg, err := knativetls.DefaultConfigFromEnv("QUEUE_PROXY_")
+		if err != nil {
+			logger.Fatalw("Failed to read TLS configuration from environment", zap.Error(err))
+		}
+		for name, server := range tlsServers {
+			go func(name string, s *http.Server) {
+				logger.Info("Starting tls server ", name, s.Addr)
+				s.TLSConfig = tlsCfg
+				s.TLSConfig.GetCertificate = certWatcher.GetCertificate
+				// Don't forward ErrServerClosed as that indicates we're already shutting down.
+				if err := s.ListenAndServeTLS("", ""); err != nil && !errors.Is(err, http.ErrServerClosed) {
+					errCh <- fmt.Errorf("%s server failed to serve: %w", name, err)
+				}
+			}(name, server)
+		}
 	}
 
 	// Blocks until we actually receive a TERM signal or one of the servers
@@ -460,17 +461,6 @@ func requestAppMetricsHandler(
 	return h
 }
 
-func queueProxyTLSConfig() (*knativetls.Config, error) {
-	cfg, err := knativetls.NewConfigFromEnv("QUEUE_PROXY_")
-	if err != nil {
-		return nil, err
-	}
-	if cfg.MinVersion == 0 {
-		cfg.MinVersion = tls.VersionTLS13
-	}
-	return cfg, nil
-}
-
 func flush(logger *zap.SugaredLogger) {
 	logger.Sync()
 	os.Stdout.Sync()

pkg/queue/sharedmain/tls_config_test.go

@@ -19,6 +19,8 @@ package sharedmain
 import (
 	"crypto/tls"
 	"testing"
+
+	knativetls "knative.dev/pkg/tls"
 )
 
 func TestQueueProxyTLSConfig(t *testing.T) {
@@ -32,34 +34,14 @@ func TestQueueProxyTLSConfig(t *testing.T) {
 		name:    "defaults to TLS 1.3 when no env vars set",
 		wantMin: tls.VersionTLS13,
 	}, {
-		name:    "respects QUEUE_PROXY_TLS_MIN_VERSION=1.2",
-		envVars: map[string]string{"QUEUE_PROXY_TLS_MIN_VERSION": "1.2"},
+		name:    "env vars are passed through",
+		envVars: map[string]string{"QUEUE_PROXY_TLS_MIN_VERSION": "1.2", "QUEUE_PROXY_TLS_MAX_VERSION": "1.3"},
 		wantMin: tls.VersionTLS12,
-	}, {
-		name:    "respects QUEUE_PROXY_TLS_MIN_VERSION=1.3",
-		envVars: map[string]string{"QUEUE_PROXY_TLS_MIN_VERSION": "1.3"},
-		wantMin: tls.VersionTLS13,
-	}, {
-		name:    "respects QUEUE_PROXY_TLS_MAX_VERSION",
-		envVars: map[string]string{"QUEUE_PROXY_TLS_MAX_VERSION": "1.3"},
-		wantMin: tls.VersionTLS13,
 		wantMax: tls.VersionTLS13,
 	}, {
-		name: "min and max version together",
-		envVars: map[string]string{
-			"QUEUE_PROXY_TLS_MIN_VERSION": "1.2",
-			"QUEUE_PROXY_TLS_MAX_VERSION": "1.3",
-		},
-		wantMin: tls.VersionTLS12,
-		wantMax: tls.VersionTLS13,
-	}, {
-		name:    "invalid min version returns error",
+		name:    "invalid env var forwards error",
 		envVars: map[string]string{"QUEUE_PROXY_TLS_MIN_VERSION": "1.1"},
 		wantErr: true,
-	}, {
-		name:    "invalid max version returns error",
-		envVars: map[string]string{"QUEUE_PROXY_TLS_MAX_VERSION": "invalid"},
-		wantErr: true,
 	}}
 
 	for _, tt := range tests {
@@ -68,9 +50,9 @@ func TestQueueProxyTLSConfig(t *testing.T) {
 				t.Setenv(k, v)
 			}
 
-			cfg, err := queueProxyTLSConfig()
+			cfg, err := knativetls.DefaultConfigFromEnv("QUEUE_PROXY_")
 			if (err != nil) != tt.wantErr {
-				t.Fatalf("queueProxyTLSConfig() error = %v, wantErr %v", err, tt.wantErr)
+				t.Fatalf("DefaultConfigFromEnv() error = %v, wantErr %v", err, tt.wantErr)
 			}
 			if tt.wantErr {
 				return
@@ -82,14 +64,6 @@ func TestQueueProxyTLSConfig(t *testing.T) {
 			if cfg.MaxVersion != tt.wantMax {
 				t.Errorf("MaxVersion = %#x, want %#x", cfg.MaxVersion, tt.wantMax)
 			}
-
-			tlsCfg := cfg.TLSConfig()
-			if tlsCfg.MinVersion != tt.wantMin {
-				t.Errorf("TLSConfig().MinVersion = %#x, want %#x", tlsCfg.MinVersion, tt.wantMin)
-			}
-			if tlsCfg.MaxVersion != tt.wantMax {
-				t.Errorf("TLSConfig().MaxVersion = %#x, want %#x", tlsCfg.MaxVersion, tt.wantMax)
-			}
 		})
 	}
 }