Merge pull request #640 from opokornyy/fix-tls-bug

fix: CrashLoopBackOff once tlsProfile changed

controllers/ssp_controller.go

@@ -19,7 +19,6 @@ package controllers
 import (
 	"context"
 	"fmt"
-	"os"
 	"reflect"
 	"strconv"
 	"strings"
@@ -148,7 +147,6 @@ func (r *sspReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ct
 		// Error reading the object - requeue the request.
 		return ctrl.Result{}, err
 	}
-	restartNeeded := r.isRestartNeeded(instance)
 	r.clearCacheIfNeeded(instance)
 
 	sspRequest := &common.Request{
@@ -163,10 +161,6 @@ func (r *sspReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ct
 		CrdList:        r.crdList,
 	}
 
-	if restartNeeded {
-		r.restart(sspRequest)
-	}
-
 	if !isInitialized(sspRequest.Instance) {
 		err := initialize(sspRequest)
 		// No need to requeue here, because
@@ -235,25 +229,6 @@ func (r *sspReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ct
 	return ctrl.Result{}, nil
 }
 
-func (r *sspReconciler) isRestartNeeded(sspObj *ssp.SSP) bool {
-	if reflect.DeepEqual(r.lastSspSpec, ssp.SSPSpec{}) {
-		return false
-	}
-	if !reflect.DeepEqual(r.lastSspSpec.TLSSecurityProfile, sspObj.Spec.TLSSecurityProfile) {
-		return true
-	}
-	return false
-}
-
-func (r *sspReconciler) restart(request *common.Request) {
-	r.log.Info("TLSSecurityProfile changed, restarting")
-	err := setSspResourceDeploying(request)
-	if err != nil {
-		r.log.Info("Error at setSspResourceDeploying", "Error: ", err)
-	}
-	os.Exit(0)
-}
-
 func (r *sspReconciler) clearCacheIfNeeded(sspObj *ssp.SSP) {
 	if !reflect.DeepEqual(r.lastSspSpec, sspObj.Spec) {
 		r.subresourceCache = common.VersionCache{}

internal/common/crypto_policy.go

@@ -1,16 +1,12 @@
 package common
 
 import (
-	"context"
 	"crypto/tls"
 	"fmt"
 
 	"github.com/go-logr/logr"
 	ocpv1 "github.com/openshift/api/config/v1"
 	"github.com/openshift/library-go/pkg/crypto"
-	ssp "kubevirt.io/ssp-operator/api/v1beta2"
-	ctrl "sigs.k8s.io/controller-runtime"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 type SSPTLSOptions struct {
@@ -65,8 +61,8 @@ func GetKnownCipherId(IANACipherName string) (uint16, bool) {
 	return 0, false
 }
 
-func (s *SSPTLSOptions) CipherIDs(logger *logr.Logger) (cipherSuites []uint16) {
-	for _, cipherName := range crypto.OpenSSLToIANACipherSuites(s.OpenSSLCipherNames) {
+func CipherIDs(names []string, logger *logr.Logger) (cipherSuites []uint16) {
+	for _, cipherName := range crypto.OpenSSLToIANACipherSuites(names) {
 		if id, ok := GetKnownCipherId(cipherName); ok {
 			cipherSuites = append(cipherSuites, id)
 		} else {
@@ -78,33 +74,6 @@ func (s *SSPTLSOptions) CipherIDs(logger *logr.Logger) (cipherSuites []uint16) {
 	return
 }
 
-func GetSspTlsOptions(ctx context.Context) (*SSPTLSOptions, error) {
-	setupLog := ctrl.Log.WithName("setup tls options")
-	restConfig := ctrl.GetConfigOrDie()
-	apiReader, err := client.New(restConfig, client.Options{Scheme: Scheme})
-	if err != nil {
-		return nil, err
-	}
-
-	var sspList ssp.SSPList
-	if err := apiReader.List(ctx, &sspList, &client.ListOptions{}); err != nil {
-		return nil, err
-	}
-
-	if len(sspList.Items) == 0 {
-		setupLog.Info("SSP CR not found, will use default tlsProfile")
-		return &SSPTLSOptions{}, nil
-	}
-
-	ssp := sspList.Items[0]
-
-	sspTLSOptions, err := NewSSPTLSOptions(ssp.Spec.TLSSecurityProfile, &setupLog)
-	if err != nil {
-		return nil, err
-	}
-	return sspTLSOptions, nil
-}
-
 func selectCipherSuitesAndMinTLSVersion(profile *ocpv1.TLSSecurityProfile) ([]string, ocpv1.TLSProtocolVersion) {
 	if profile == nil {
 		return nil, ""

internal/template-validator/tlsinfo/tlsinfo.go

@@ -192,7 +192,7 @@ func (ti *TLSInfo) CreateTlsConfig() *tls.Config {
 	}
 
 	if !ti.sspTLSOptions.IsEmpty() {
-		tlsConfig.CipherSuites = ti.sspTLSOptions.CipherIDs(nil)
+		tlsConfig.CipherSuites = common.CipherIDs(ti.sspTLSOptions.OpenSSLCipherNames, nil)
 		minVersion, err := ti.sspTLSOptions.MinTLSVersionId()
 		if err != nil {
 			panic(fmt.Sprintf("TLS Configuration broken, min version misconfigured %v", err))

main.go

@@ -17,6 +17,7 @@ limitations under the License.
 package main
 
 import (
+	"context"
 	"crypto/tls"
 	"flag"
 	"fmt"
@@ -28,13 +29,18 @@ import (
 	// to ensure that exec-entrypoint and run can make use of them.
 	_ "k8s.io/client-go/plugin/pkg/client/auth"
 
+	ocpconfigv1 "github.com/openshift/api/config/v1"
+	"github.com/openshift/library-go/pkg/crypto"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/cache"
+	"sigs.k8s.io/controller-runtime/pkg/certwatcher"
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	"sigs.k8s.io/controller-runtime/pkg/metrics"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 
+	ssp "kubevirt.io/ssp-operator/api/v1beta2"
 	"kubevirt.io/ssp-operator/controllers"
 	"kubevirt.io/ssp-operator/internal/common"
 	common_templates "kubevirt.io/ssp-operator/internal/operands/common-templates"
@@ -66,53 +72,121 @@ const (
 	webhookPort = 9443
 )
 
-func runPrometheusServer(metricsAddr string, tlsOptions common.SSPTLSOptions) error {
+// This callback executes on each client call returning a new config to be used
+// please be aware that the APIServer is using http keepalive so this is going to
+// be executed only after a while for fresh connections and not on existing ones
+func getConfigForClient(ctx context.Context, cfg *tls.Config, cache cache.Cache) (*tls.Config, error) {
+	var sspList ssp.SSPList
+	err := cache.List(ctx, &sspList)
+	if err != nil {
+		return nil, err
+	}
+
+	if len(sspList.Items) == 0 || sspList.Items[0].Spec.TLSSecurityProfile == nil {
+		cfg.MinVersion = crypto.DefaultTLSVersion()
+		cfg.CipherSuites = nil
+		return cfg, nil
+	}
+
+	tlsProfile := sspList.Items[0].Spec.TLSSecurityProfile
+	if tlsProfile.Type == ocpconfigv1.TLSProfileCustomType {
+		minVersion, err := crypto.TLSVersion(string(tlsProfile.Custom.MinTLSVersion))
+		if err != nil {
+			return nil, err
+		}
+		cfg.MinVersion = minVersion
+		cfg.CipherSuites = common.CipherIDs(tlsProfile.Custom.Ciphers, &ctrl.Log)
+		return cfg, nil
+	}
+
+	minVersion, err := crypto.TLSVersion(string(ocpconfigv1.TLSProfiles[tlsProfile.Type].MinTLSVersion))
+	if err != nil {
+		return nil, err
+	}
+	cfg.MinVersion = minVersion
+	cfg.CipherSuites = common.CipherIDs(ocpconfigv1.TLSProfiles[tlsProfile.Type].Ciphers, &ctrl.Log)
+
+	return cfg, nil
+}
+
+type prometheusServer struct {
+	cache         cache.Cache
+	certPath      string
+	keyPath       string
+	serverAddress string
+}
+
+// NeedLeaderElection implements the LeaderElectionRunnable interface, which indicates
+// the prometheus server doesn't need leader election.
+func (s *prometheusServer) NeedLeaderElection() bool {
+	return false
+}
+
+func (s *prometheusServer) Start(ctx context.Context) error {
 	setupLog.Info("Starting Prometheus metrics endpoint server with TLS")
-	metrics.Registry.MustRegister(common_templates.CommonTemplatesRestored)
-	metrics.Registry.MustRegister(common.SSPOperatorReconcileSucceeded)
 	handler := promhttp.HandlerFor(metrics.Registry, promhttp.HandlerOpts{})
 	mux := http.NewServeMux()
 	mux.Handle("/metrics", handler)
 
-	minTlsVersion, err := tlsOptions.MinTLSVersionId()
-	if err != nil {
-		return err
+	server := &http.Server{
+		Addr:    s.serverAddress,
+		Handler: mux,
 	}
 
-	tlsConfig := tls.Config{
-		CipherSuites: tlsOptions.CipherIDs(&setupLog),
-		MinVersion:   minTlsVersion,
+	certWatcher, err := certwatcher.New(s.certPath, s.keyPath)
+	if err != nil {
+		return err
 	}
 
-	server := http.Server{
-		Addr:      metricsAddr,
-		Handler:   mux,
-		TLSConfig: &tlsConfig,
-	}
+	go func() {
+		// TODO: change context, so it can be closed when
+		// this function returns an error
+		if err := certWatcher.Start(ctx); err != nil {
+			setupLog.Error(err, "certificate watcher error")
+		}
+	}()
 
+	idleConnsClosed := make(chan struct{})
 	go func() {
-		err := server.ListenAndServeTLS(path.Join(sdkTLSDir, sdkTLSCrt), path.Join(sdkTLSDir, sdkTLSKey))
-		if err != nil {
-			setupLog.Error(err, "Failed to start Prometheus metrics endpoint server")
+		// TODO: make sure that the goroutine finishes when
+		// this function returns an error
+		<-ctx.Done()
+		setupLog.Info("shutting down Prometheus metrics server")
+
+		if err := server.Shutdown(context.Background()); err != nil {
+			setupLog.Error(err, "error shutting down the HTTP server")
 		}
+		close(idleConnsClosed)
 	}()
+
+	server.TLSConfig = s.getPrometheusTLSConfig(ctx, certWatcher)
+
+	if err := server.ListenAndServeTLS(s.certPath, s.keyPath); err != nil && err != http.ErrServerClosed {
+		setupLog.Error(err, "Failed to start Prometheus metrics endpoint server")
+		return err
+	}
+
+	<-idleConnsClosed
 	return nil
 }
 
-func getWebhookServer(sspTLSOptions common.SSPTLSOptions) *webhook.Server {
-	// If TLSSecurityProfile is empty, we want to return nil so that the default
-	// webhook server configuration is used.
-	if sspTLSOptions.IsEmpty() {
-		return nil
+func (s *prometheusServer) getPrometheusTLSConfig(ctx context.Context, certWatcher *certwatcher.CertWatcher) *tls.Config {
+	return &tls.Config{
+		GetConfigForClient: func(_ *tls.ClientHelloInfo) (*tls.Config, error) {
+			cfg := &tls.Config{}
+			cfg.GetCertificate = certWatcher.GetCertificate
+			return getConfigForClient(ctx, cfg, s.cache)
+		},
 	}
+}
 
-	tlsCfgFunc := func(cfg *tls.Config) {
-		cfg.CipherSuites = sspTLSOptions.CipherIDs(&setupLog)
-		setupLog.Info("Configured ciphers", "ciphers", cfg.CipherSuites)
+func newPrometheusServer(metricsAddr string, cache cache.Cache) *prometheusServer {
+	return &prometheusServer{
+		certPath:      path.Join(sdkTLSDir, sdkTLSCrt),
+		keyPath:       path.Join(sdkTLSDir, sdkTLSKey),
+		cache:         cache,
+		serverAddress: metricsAddr,
 	}
-
-	funcs := []func(*tls.Config){tlsCfgFunc}
-	return &webhook.Server{Port: webhookPort, TLSMinVersion: sspTLSOptions.MinTLSVersion, TLSOpts: funcs}
 }
 
 func main() {
@@ -127,6 +201,8 @@ func main() {
 	opts := zap.Options{}
 	opts.BindFlags(flag.CommandLine)
 	flag.Parse()
+	metrics.Registry.MustRegister(common_templates.CommonTemplatesRestored)
+	metrics.Registry.MustRegister(common.SSPOperatorReconcileSucceeded)
 
 	ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts)))
 
@@ -138,26 +214,24 @@ func main() {
 
 	ctx := ctrl.SetupSignalHandler()
 
-	tlsOptions, err := common.GetSspTlsOptions(ctx)
-	if err != nil {
-		setupLog.Error(err, "Error while getting tls profile")
-		os.Exit(1)
-	}
+	var mgr ctrl.Manager
 
-	err = runPrometheusServer(metricsAddr, *tlsOptions)
-	if err != nil {
-		setupLog.Error(err, "unable to start prometheus server")
-		os.Exit(1)
+	getTLSOptsFunc := func(cfg *tls.Config) {
+		cfg.GetConfigForClient = func(_ *tls.ClientHelloInfo) (*tls.Config, error) {
+			return getConfigForClient(ctx, cfg, mgr.GetCache())
+		}
 	}
 
-	mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
+	mgr, err = ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
 		Scheme:                 common.Scheme,
 		MetricsBindAddress:     "0",
 		HealthProbeBindAddress: probeAddr,
 		LeaderElection:         enableLeaderElection,
 		LeaderElectionID:       leaderElectionID,
-		// If WebhookServer is set to nil, a default one will be created.
-		WebhookServer: getWebhookServer(*tlsOptions),
+		WebhookServer: &webhook.Server{
+			Port:    webhookPort,
+			TLSOpts: []func(*tls.Config){getTLSOptsFunc},
+		},
 	})
 
 	if err != nil {
@@ -171,10 +245,23 @@ func main() {
 			os.Exit(1)
 		}
 	}
+
+	metricsServer := newPrometheusServer(metricsAddr, mgr.GetCache())
+	if err != nil {
+		setupLog.Error(err, "unable create Prometheus server")
+		os.Exit(1)
+	}
+
+	if err := mgr.Add(metricsServer); err != nil {
+		setupLog.Error(err, "unable to set up metrics")
+		os.Exit(1)
+	}
+
 	if err := mgr.AddReadyzCheck("check", healthz.Ping); err != nil {
 		setupLog.Error(err, "unable to set up ready check")
 		os.Exit(1)
 	}
+
 	if err := mgr.AddHealthzCheck("health", healthz.Ping); err != nil {
 		setupLog.Error(err, "unable to set up health check")
 		os.Exit(1)