Merge pull request noobaa#1767 from liranmauda/liran-backport-into-5.19

[Backport into 5.19] Backport fixes into 5.19

Author: liranmauda

.github/workflows/run_hac_test.yml

@@ -1,5 +1,5 @@
 name: HAC on KIND cluster Test
-on: [push, pull_request, workflow_dispatch]
+on: [workflow_dispatch]
 
 jobs:
   run-hac-test:

pkg/controller/add_hac.go

@@ -1,10 +1,15 @@
 package controller
 
 import (
+	"os"
+
 	hac "github.com/noobaa/noobaa-operator/v5/pkg/controller/ha"
 )
 
 func init() {
-	// AddToManagerFuncs is a list of functions to create controllers and add them to a manager.
-	AddToManagerFuncs = append(AddToManagerFuncs, hac.Add)
+	hacEnabled := os.Getenv("HAC_ENABLED")
+	if hacEnabled == "true" {
+		// AddToManagerFuncs is a list of functions to create controllers and add them to a manager.
+		AddToManagerFuncs = append(AddToManagerFuncs, hac.Add)
+	}
 }

pkg/controller/ha/ha_controller.go

@@ -71,6 +71,7 @@ func nodeNotReadyPredicate() predicate.Predicate {
 
 // Add creates a nodewatcher Controller and adds it to the Manager.
 func Add(mgr manager.Manager) error {
+	util.Logger().Info("Adding high availability controller (HAC)")
 
 	opts := controller.Options{
 		MaxConcurrentReconciles: 1,

pkg/hac/hac.go

@@ -5,6 +5,7 @@ package hac
 
 import (
 	"context"
+	"os"
 
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -43,8 +44,12 @@ func (pd *PodDeleter) DeletePodsOnNode() error {
 		return errors.Errorf("failed to list noobaa pods on the node %v in namespace %v", pd.NodeName, options.Namespace)
 	}
 
-	// delete the found pods
-	var gracePeriod int64 = 0
+	// delete the found pods. by default using a 1 second grace period.
+	var gracePeriod int64 = 1
+	if os.Getenv("HAC_FORCE_DELETE") == "true" {
+		// if HAC_FORCE_DELETE is set to true, use a 0 second grace period.
+		gracePeriod = 0
+	}
 	deleteOpts := client.DeleteOptions{GracePeriodSeconds: &gracePeriod}
 	for _, pod := range podList.Items {
 		if err := pd.Client.Delete(context.Background(), &pod, &deleteOpts); err != nil {

pkg/options/options.go

@@ -48,9 +48,6 @@ const (
 
 	// SystemName is a constant as we want just a single system per namespace
 	SystemName = "noobaa"
-
-	// ServiceServingCertCAFile points to OCP root CA to be added to the default root CA list
-	ServiceServingCertCAFile = "/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt"
 )
 
 // Namespace is the target namespace for locating the noobaa system

pkg/system/phase2_creating.go

@@ -392,6 +392,21 @@ func (r *Reconciler) SetDesiredNooBaaDB() error {
 }
 
 func (r *Reconciler) setDesiredCoreEnv(c *corev1.Container) {
+	// Filter out NOOBAA_ROOT_SECRET from env vars to avoid exposing it in pod spec
+	// it is set via mounting the secret as files
+	// this will remove the leftover env var in case of an upgrade from older operator version (older than 4.21)
+	// as we preserve env vars on updates by merging the arrays and not replacing them.
+
+    if len(c.Env) > 0 {
+        filtered := c.Env[:0]
+        for _, env := range c.Env {
+            if env.Name != "NOOBAA_ROOT_SECRET" {
+                filtered = append(filtered, env)
+            }
+        }
+        c.Env = filtered
+    }
+
 	for j := range c.Env {
 		switch c.Env[j].Name {
 		case "AGENT_PROFILE":
@@ -521,6 +536,10 @@ func (r *Reconciler) SetDesiredCoreApp() error {
 	r.CoreApp.Spec.ServiceName = r.ServiceMgmt.Name
 
 	podSpec := &r.CoreApp.Spec.Template.Spec
+	// set the termination grace period for noobaa-core pod.
+	// For now we set it to 1 second. A better approach should be to implement a graceful shutdown for the noobaa-core pod when SIGTERM is received.
+	terminationGracePeriodSeconds := int64(1)
+	podSpec.TerminationGracePeriodSeconds = &terminationGracePeriodSeconds
 	podSpec.ServiceAccountName = "noobaa-core"
 	coreImageChanged := false
 
@@ -575,10 +594,11 @@ func (r *Reconciler) SetDesiredCoreApp() error {
 				util.MergeVolumeMountList(&c.VolumeMounts, &dbSecretVolumeMounts)
 			}
 
-			if util.KubeCheckQuiet(r.CaBundleConf) {
+			// we want to check that the cm exists and also that it has data in it
+			if util.KubeCheckQuiet(r.CaBundleConf) && len(r.CaBundleConf.Data) > 0 {
 				configMapVolumeMounts := []corev1.VolumeMount{{
 					Name:      r.CaBundleConf.Name,
-					MountPath: "/etc/ocp-injected-ca-bundle.crt",
+					MountPath: "/etc/ocp-injected-ca-bundle",
 					ReadOnly:  true,
 				}}
 				util.MergeVolumeMountList(&c.VolumeMounts, &configMapVolumeMounts)
@@ -658,10 +678,11 @@ func (r *Reconciler) SetDesiredCoreApp() error {
 					Limits:   logResourceList,
 				}
 			}
-			if util.KubeCheckQuiet(r.CaBundleConf) {
+			// we want to check that the cm exists and also that it has data in it
+			if util.KubeCheckQuiet(r.CaBundleConf) && len(r.CaBundleConf.Data) > 0 {
 				configMapVolumeMounts := []corev1.VolumeMount{{
 					Name:      r.CaBundleConf.Name,
-					MountPath: "/etc/ocp-injected-ca-bundle.crt",
+					MountPath: "/etc/ocp-injected-ca-bundle",
 					ReadOnly:  true,
 				}}
 				util.MergeVolumeMountList(&c.VolumeMounts, &configMapVolumeMounts)
@@ -701,7 +722,8 @@ func (r *Reconciler) SetDesiredCoreApp() error {
 
 	r.CoreApp.Spec.Template.Annotations["noobaa.io/configmap-hash"] = r.CoreAppConfig.Annotations["noobaa.io/configmap-hash"]
 
-	if util.KubeCheckQuiet(r.CaBundleConf) {
+	// we want to check that the cm exists and also that it has data in it
+	if util.KubeCheckQuiet(r.CaBundleConf) && len(r.CaBundleConf.Data) > 0 {
 		configMapVolumes := []corev1.Volume{{
 			Name: r.CaBundleConf.Name,
 			VolumeSource: corev1.VolumeSource{

pkg/system/phase4_configuring.go

@@ -507,7 +507,8 @@ func (r *Reconciler) setDesiredEndpointMounts(podSpec *corev1.PodSpec, container
 		util.MergeVolumeMountList(&container.VolumeMounts, &dbSecretVolumeMounts)
 	}
 
-	if util.KubeCheckQuiet(r.CaBundleConf) {
+	// we want to check that the cm exists and also that it has data in it
+	if util.KubeCheckQuiet(r.CaBundleConf) && len(r.CaBundleConf.Data) > 0 {
 		configMapVolumes := []corev1.Volume{{
 			Name: r.CaBundleConf.Name,
 			VolumeSource: corev1.VolumeSource{
@@ -525,7 +526,7 @@ func (r *Reconciler) setDesiredEndpointMounts(podSpec *corev1.PodSpec, container
 		util.MergeVolumeList(&podSpec.Volumes, &configMapVolumes)
 		configMapVolumeMounts := []corev1.VolumeMount{{
 			Name:      r.CaBundleConf.Name,
-			MountPath: "/etc/ocp-injected-ca-bundle.crt",
+			MountPath: "/etc/ocp-injected-ca-bundle",
 			ReadOnly:  true,
 		}}
 		util.MergeVolumeMountList(&container.VolumeMounts, &configMapVolumeMounts)
@@ -1069,6 +1070,10 @@ func (r *Reconciler) prepareAWSBackingStore() error {
 				*result.Credentials.SecretAccessKey,
 				*result.Credentials.SessionToken,
 			),
+			HTTPClient: &http.Client{
+				Transport: util.GlobalCARefreshingTransport,
+				Timeout:   10 * time.Second,
+			},
 			Region: &region,
 		}
 	} else { // handle AWS long-lived credentials (not STS)
@@ -1078,6 +1083,10 @@ func (r *Reconciler) prepareAWSBackingStore() error {
 				cloudCredsSecret.StringData["aws_secret_access_key"],
 				"",
 			),
+			HTTPClient: &http.Client{
+				Transport: util.GlobalCARefreshingTransport,
+				Timeout:   10 * time.Second,
+			},
 			Region: &region,
 		}
 	}

pkg/system/reconciler.go

@@ -68,7 +68,7 @@ type Reconciler struct {
 	OperatorVersion          string
 	OAuthEndpoints           *util.OAuth2Endpoints
 	PostgresConnectionString string
-	ApplyCAsToPods           string
+	ApplyCAsToPods           string // the path that will be applied to the core and endpoint pods in NODE_EXTRA_CA_CERTS
 
 	NooBaa                    *nbv1.NooBaa
 	ServiceAccount            *corev1.ServiceAccount
@@ -282,7 +282,7 @@ func NewReconciler(
 	r.RouteS3.Name = r.ServiceS3.Name
 	r.RouteSts.Name = r.ServiceSts.Name
 	r.DeploymentEndpoint.Name = r.Request.Name + "-endpoint"
-	r.CaBundleConf.Name = r.Request.Name + "-ca-inject"
+	r.CaBundleConf.Name = "ocp-injected-ca-bundle"
 	r.KedaScaled.Name = r.Request.Name
 	r.AdapterHPA.Name = r.Request.Name + "-hpav2"
 	r.BucketLoggingPVC.Name = r.Request.Name + "-bucket-logging-pvc"
@@ -404,9 +404,30 @@ func (r *Reconciler) Reconcile() (reconcile.Result, error) {
 		}
 	}
 
-	err = util.AddToRootCAs(options.ServiceServingCertCAFile)
+	/*
+	This code is problematic due to the way other parts of the product work.
+	On the core side, get_unsecured_agent() relies on the presence of the NODE_EXTRA_CA_CERTS
+	environment variable to determine whether an HTTP or HTTPS client should be used.
+
+	At the time of writing this comment, if the environment variable is not set, an HTTP agent
+	will be used for *all* S3-compatible domains that aren't under amazonaws.com - including
+	domains that are already present by default in the system's certificate store.
+
+	Forcing the environment variable to always be set leads to a different problem where
+	some things might fail - e.g. the admission tests that rely on creating a namespacestore
+	that points towards NooBaa's (self-signed) S3 service. In that case, the HTTPS agent fails
+	due to the self-signed certificate.	
+
+	Also, note that the code that combines certificates only applies to the operator.
+	Based on whether the certificate bundling was successful, the operator will set the value of
+	NODE_EXTRA_CA_CERTS in endpoints and core pods to point to *the system generated service-serving certs*.
+
+	At the time of writing, user certs are not included at any point.
+	*/
+	
+	err = util.CombineCaBundle(util.ServiceServingCertCAFile)
 	if err == nil {
-		r.ApplyCAsToPods = options.ServiceServingCertCAFile
+		r.ApplyCAsToPods = util.ServiceServingCertCAFile
 	} else if !os.IsNotExist(err) {
 		log.Errorf("❌ NooBaa %q failed to add root CAs to system default", r.NooBaa.Name)
 		res.RequeueAfter = 3 * time.Second

pkg/util/util.go

@@ -84,6 +84,12 @@ const (
 
 	topologyConstraintsEnabledKubeVersion = "1.26.0"
 	trueStr                               = "true"
+
+	// ServiceServingCertCAFile points to OCP default root CA list
+	ServiceServingCertCAFile = "/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt"
+
+	// InjectedBundleCertCAFile points to OCP root CA to be added to the default root CA list
+	InjectedBundleCertCAFile = "/etc/ocp-injected-ca-bundle/ca-bundle.crt"
 )
 
 // OAuth2Endpoints holds OAuth2 endpoints information.
@@ -143,15 +149,15 @@ var (
 	}
 )
 
-// AddToRootCAs adds a local cert file to Our GlobalCARefreshingTransport
-func AddToRootCAs(localCertFile string) error {
+// CombineCaBundle combines a local cert file to Our GlobalCARefreshingTransport
+func CombineCaBundle(localCertFile string) error {
 	rootCAs, _ := x509.SystemCertPool()
 	if rootCAs == nil {
 		rootCAs = x509.NewCertPool()
 	}
 
 	var certFiles = []string{
-		"/etc/ocp-injected-ca-bundle.crt",
+		InjectedBundleCertCAFile,
 		localCertFile,
 	}
 
@@ -169,7 +175,7 @@ func AddToRootCAs(localCertFile string) error {
 		}
 
 		// Trust the augmented cert pool in our client
-		log.Infof("Successfuly appended %q to RootCAs", certFile)
+		log.Infof("Successfully appended %q to RootCAs", certFile)
 	}
 	GlobalCARefreshingTransport.TLSClientConfig.RootCAs = rootCAs
 	return nil