fix defer unsafe Close vulnerability

Signed-off-by: Subbarao Meduri <[email protected]>

collectors/metrics/pkg/metricfamily/hypershift_transformer.go

@@ -44,7 +44,7 @@ func NewHypershiftTransformer(l log.Logger, c client.Client, labels map[string]s
 	//clusters := map[string]string{}
 	hClient := c
 	if hClient == nil {
-		if os.Getenv("UNIT_TEST") != "true" {
+		if _, ok := os.LookupEnv("UNIT_TEST"); !ok {
 			config, err := clientcmd.BuildConfigFromFlags("", "")
 			if err != nil {
 				return nil, errors.New("failed to create the kube config")

collectors/metrics/pkg/metricsclient/metricsclient.go

@@ -366,8 +366,7 @@ func withCancel(ctx context.Context, client *http.Client, req *http.Request, fn
 }
 
 func MTLSTransport(logger log.Logger, caCertFile, tlsCrtFile, tlsKeyFile string) (*http.Transport, error) {
-	testMode := os.Getenv("UNIT_TEST") != ""
-	if testMode {
+	if _, ok := os.LookupEnv("UNIT_TEST"); !ok {
 		caCertFile = "../../testdata/tls/ca.crt"
 		tlsKeyFile = "../../testdata/tls/tls.key"
 		tlsCrtFile = "../../testdata/tls/tls.crt"

loaders/dashboards/pkg/util/grafana_util.go

@@ -61,14 +61,21 @@ func SetRequest(method string, url string, body io.Reader, retry int) ([]byte, i
 		resp, err = getHTTPClient().Do(req)
 	}
 
-	if resp != nil {
-		defer resp.Body.Close()
-		respBody, err := io.ReadAll(resp.Body)
+	if resp == nil {
+		return nil, http.StatusNotFound
+	}
+
+	defer func() {
+		err := resp.Body.Close()
 		if err != nil {
-			klog.Info("failed to parse response body ", "error ", err)
+			klog.Info("failed to close response body ", "error ", err)
 		}
-		return respBody, resp.StatusCode
-	} else {
-		return nil, http.StatusNotFound
+	}()
+
+	respBody, err := io.ReadAll(resp.Body)
+	if err != nil {
+		klog.Info("failed to parse response body ", "error ", err)
+		return nil, resp.StatusCode
 	}
+	return respBody, resp.StatusCode
 }

operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go

@@ -105,7 +105,7 @@ func (r *ObservabilityAddonReconciler) Reconcile(ctx context.Context, req ctrl.R
 		deleteFlag = true
 	}
 	isHypershift := true
-	if os.Getenv("UNIT_TEST") != "true" {
+	if _, ok := os.LookupEnv("UNIT_TEST"); !ok {
 		crdClient, err := operatorutil.GetOrCreateCRDClient()
 		if err != nil {
 			return ctrl.Result{}, err

operators/endpointmetrics/pkg/util/client.go

@@ -37,7 +37,7 @@ const (
 
 // GetOrCreateOCPClient get an existing hub client or create new one if it doesn't exist.
 func GetOrCreateHubClient(renew bool) (client.Client, error) {
-	if os.Getenv("UNIT_TEST") == "true" {
+	if _, ok := os.LookupEnv("UNIT_TEST"); !ok {
 		return hubClient, nil
 	}
 

operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go

@@ -20,7 +20,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	storev1 "k8s.io/api/storage/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -72,6 +71,7 @@ var (
 	isReceiveStorageSizeChanged      = false
 	isStoreStorageSizeChanged        = false
 	isLegacyResourceRemoved          = false
+	lastLogTime                      = time.Now()
 )
 
 // MultiClusterObservabilityReconciler reconciles a MultiClusterObservability object
@@ -170,6 +170,14 @@ func (r *MultiClusterObservabilityReconciler) Reconcile(ctx context.Context, req
 	mchCrdExists := r.CRDMap[config.MCHCrdName]
 	// requeue after 10 seconds if the mch crd exists and image image manifests map is empty
 	if mchCrdExists && len(config.GetImageManifests()) == 0 {
+		currentTime := time.Now()
+
+		// Log the message if it has been longer than 5 minutes since the last log
+		if currentTime.Sub(lastLogTime) > 5*time.Minute {
+			reqLogger.Info("Waiting for the mch CR to be ready", "mchCrdExists", mchCrdExists, "imageManifests", len(config.GetImageManifests()))
+			lastLogTime = currentTime
+		}
+
 		// if the mch CR is not ready, then requeue the request after 10s
 		return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
 	}
@@ -321,8 +329,7 @@ func (r *MultiClusterObservabilityReconciler) Reconcile(ctx context.Context, req
 		}
 	}
 
-	if os.Getenv("UNIT_TEST") != "true" && !isLegacyResourceRemoved {
-		isLegacyResourceRemoved = true
+	if _, ok := os.LookupEnv("UNIT_TEST"); !ok && !isLegacyResourceRemoved {
 		// Delete PrometheusRule from openshift-monitoring namespace
 		if err := r.deleteSpecificPrometheusRule(ctx); err != nil {
 			reqLogger.Error(err, "Failed to delete the specific PrometheusRule in the openshift-monitoring namespace")
@@ -333,6 +340,7 @@ func (r *MultiClusterObservabilityReconciler) Reconcile(ctx context.Context, req
 			reqLogger.Error(err, "Failed to delete service monitor in the openshift-monitoring namespace")
 			return ctrl.Result{}, err
 		}
+		isLegacyResourceRemoved = true
 	}
 
 	//update status
@@ -626,19 +634,19 @@ func GenerateAlertmanagerRoute(
 		log.Info("BYO CA/Certificate found for the Route of Alertmanager, will using BYO CA/certificate for the Route of Alertmanager")
 		amRouteCA, ok := amRouteBYOCaSrt.Data["tls.crt"]
 		if !ok {
-			return &ctrl.Result{}, cerr.New("Invalid BYO CA for the Route of Alertmanager")
+			return &ctrl.Result{}, cerr.New("invalid BYO CA for the Route of Alertmanager")
 		}
 		amGateway.Spec.TLS.CACertificate = string(amRouteCA)
 
 		amRouteCert, ok := amRouteBYOCertSrt.Data["tls.crt"]
 		if !ok {
-			return &ctrl.Result{}, cerr.New("Invalid BYO Certificate for the Route of Alertmanager")
+			return &ctrl.Result{}, cerr.New("invalid BYO Certificate for the Route of Alertmanager")
 		}
 		amGateway.Spec.TLS.Certificate = string(amRouteCert)
 
 		amRouteCertKey, ok := amRouteBYOCertSrt.Data["tls.key"]
 		if !ok {
-			return &ctrl.Result{}, cerr.New("Invalid BYO Certificate Key for the Route of Alertmanager")
+			return &ctrl.Result{}, cerr.New("invalid BYO Certificate Key for the Route of Alertmanager")
 		}
 		amGateway.Spec.TLS.Key = string(amRouteCertKey)
 	}
@@ -720,19 +728,19 @@ func GenerateProxyRoute(
 		log.Info("BYO CA/Certificate found for the Route of Proxy, will using BYO CA/certificate for the Route of Proxy")
 		proxyRouteCA, ok := proxyRouteBYOCaSrt.Data["tls.crt"]
 		if !ok {
-			return &ctrl.Result{}, cerr.New("Invalid BYO CA for the Route of Proxy")
+			return &ctrl.Result{}, cerr.New("invalid BYO CA for the Route of Proxy")
 		}
 		proxyGateway.Spec.TLS.CACertificate = string(proxyRouteCA)
 
 		proxyRouteCert, ok := proxyRouteBYOCertSrt.Data["tls.crt"]
 		if !ok {
-			return &ctrl.Result{}, cerr.New("Invalid BYO Certificate for the Route of Proxy")
+			return &ctrl.Result{}, cerr.New("invalid BYO Certificate for the Route of Proxy")
 		}
 		proxyGateway.Spec.TLS.Certificate = string(proxyRouteCert)
 
 		proxyRouteCertKey, ok := proxyRouteBYOCertSrt.Data["tls.key"]
 		if !ok {
-			return &ctrl.Result{}, cerr.New("Invalid BYO Certificate Key for the Route of Proxy")
+			return &ctrl.Result{}, cerr.New("invalid BYO Certificate Key for the Route of Proxy")
 		}
 		proxyGateway.Spec.TLS.Key = string(proxyRouteCertKey)
 	}
@@ -826,7 +834,7 @@ func (r *MultiClusterObservabilityReconciler) ensureOpenShiftNamespaceLabel(ctx
 	}
 
 	err := r.Client.Get(ctx, types.NamespacedName{Name: resNS}, existingNs)
-	if err != nil || errors.IsNotFound(err) {
+	if err != nil || apierrors.IsNotFound(err) {
 		log.Error(err, fmt.Sprintf("Failed to find namespace for Multicluster Operator: %s", resNS))
 		return reconcile.Result{Requeue: true}, err
 	}

operators/multiclusterobservability/controllers/multiclusterobservability/observatorium.go

@@ -468,7 +468,7 @@ func newAPISpec(c client.Client, mco *mcov1beta2.MultiClusterObservability) (obs
 				data, ok := storageSecret.Data[storageConfig.Key]
 				if !ok {
 					log.Error(err, "Invalid key in secret", "name", storageConfig.Name, "key", storageConfig.Key)
-					return apiSpec, fmt.Errorf("Invalid key %s in secret %s", storageConfig.Key, storageConfig.Name)
+					return apiSpec, fmt.Errorf("invalid key %s in secret %s", storageConfig.Key, storageConfig.Name)
 				}
 				ep := &mcoutil.RemoteWriteEndpointWithSecret{}
 				err = yaml.Unmarshal(data, ep)

operators/multiclusterobservability/controllers/multiclusterobservability/observatorium_test.go

@@ -11,7 +11,6 @@ import (
 
 	"gopkg.in/yaml.v2"
 	corev1 "k8s.io/api/core/v1"
-	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -20,9 +19,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	mcoshared "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/shared"
-	oashared "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/shared"
 	mcov1beta2 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta2"
-	"github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/pkg/config"
+
 	mcoconfig "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/pkg/config"
 	mcoutil "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/pkg/util"
 	observatoriumv1alpha1 "github.com/stolostron/observatorium-operator/api/v1alpha1"
@@ -34,8 +32,8 @@ var (
 
 func TestNewVolumeClaimTemplate(t *testing.T) {
 	vct := newVolumeClaimTemplate("10Gi", "test")
-	if vct.Spec.AccessModes[0] != v1.ReadWriteOnce ||
-		vct.Spec.Resources.Requests[v1.ResourceStorage] != resource.MustParse("10Gi") {
+	if vct.Spec.AccessModes[0] != corev1.ReadWriteOnce ||
+		vct.Spec.Resources.Requests[corev1.ResourceStorage] != resource.MustParse("10Gi") {
 		t.Errorf("Failed to newVolumeClaimTemplate")
 	}
 }
@@ -81,7 +79,7 @@ func TestNewDefaultObservatoriumSpec(t *testing.T) {
 	writeStorageS := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "write_name",
-			Namespace: config.GetDefaultNamespace(),
+			Namespace: mcoconfig.GetDefaultNamespace(),
 		},
 		Type: "Opaque",
 		Data: map[string][]byte{
@@ -120,7 +118,7 @@ func TestNewDefaultObservatoriumSpec(t *testing.T) {
 	endpointS := &corev1.Secret{}
 	err := cl.Get(context.TODO(), types.NamespacedName{
 		Name:      endpointsConfigName,
-		Namespace: config.GetDefaultNamespace(),
+		Namespace: mcoconfig.GetDefaultNamespace(),
 	}, endpointS)
 	if err != nil {
 		t.Errorf("Failed to get endpoint config secret due to %v", err)
@@ -214,7 +212,7 @@ func TestGetTLSSecretMountPath(t *testing.T) {
 	testCaseList := []struct {
 		name        string
 		secret      *corev1.Secret
-		storeConfig *oashared.PreConfiguredStorage
+		storeConfig *mcoshared.PreConfiguredStorage
 		expected    string
 	}{
 
@@ -223,7 +221,7 @@ func TestGetTLSSecretMountPath(t *testing.T) {
 			&corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "test",
-					Namespace: config.GetDefaultNamespace(),
+					Namespace: mcoconfig.GetDefaultNamespace(),
 				},
 				Type: "Opaque",
 				Data: map[string][]byte{
@@ -234,7 +232,7 @@ config:
 `),
 				},
 			},
-			&oashared.PreConfiguredStorage{
+			&mcoshared.PreConfiguredStorage{
 				Key:  "thanos.yaml",
 				Name: "test",
 			},
@@ -245,7 +243,7 @@ config:
 			&corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "test-1",
-					Namespace: config.GetDefaultNamespace(),
+					Namespace: mcoconfig.GetDefaultNamespace(),
 				},
 				Type: "Opaque",
 				Data: map[string][]byte{
@@ -263,7 +261,7 @@ config:
 `),
 				},
 			},
-			&oashared.PreConfiguredStorage{
+			&mcoshared.PreConfiguredStorage{
 				Key:  "thanos.yaml",
 				Name: "test-1",
 			},
@@ -274,7 +272,7 @@ config:
 			&corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "test-2",
-					Namespace: config.GetDefaultNamespace(),
+					Namespace: mcoconfig.GetDefaultNamespace(),
 				},
 				Type: "Opaque",
 				Data: map[string][]byte{
@@ -292,7 +290,7 @@ config:
 `),
 				},
 			},
-			&oashared.PreConfiguredStorage{
+			&mcoshared.PreConfiguredStorage{
 				Key:  "thanos.yaml",
 				Name: "test-2",
 			},
@@ -307,6 +305,9 @@ config:
 			t.Errorf("failed to create object storage secret, due to %v", err)
 		}
 		path, err := getTLSSecretMountPath(client, c.storeConfig)
+		if err != nil {
+			t.Errorf("failed to get tls secret mount path, due to %v", err)
+		}
 		if path != c.expected {
 			t.Errorf("case (%v) output: (%v) is not the expected: (%v)", c.name, path, c.expected)
 		}

proxy/pkg/util/util.go

@@ -427,7 +427,13 @@ func FetchUserProjectList(token string, url string) []string {
 		writeError(fmt.Sprintf("failed to send http request: %v", err))
 		return []string{}
 	}
-	defer resp.Body.Close()
+
+	defer func() {
+		err := resp.Body.Close()
+		if err != nil {
+			klog.Errorf("failed to close response body: %v", err)
+		}
+	}()
 
 	var projects projectv1.ProjectList
 	err = json.NewDecoder(resp.Body).Decode(&projects)
@@ -451,14 +457,18 @@ func GetUserName(token string, url string) string {
 		writeError(fmt.Sprintf("failed to send http request: %v", err))
 		return ""
 	}
-	defer resp.Body.Close()
 
 	user := userv1.User{}
 	err = json.NewDecoder(resp.Body).Decode(&user)
 	if err != nil {
 		klog.Errorf("failed to decode response json body: %v", err)
+		resp.Body.Close()
 		return ""
 	}
+	err = resp.Body.Close()
+	if err != nil {
+		klog.Errorf("failed to close response body: %v", err)
+	}
 
 	return user.Name
 }

tests/pkg/utils/mco_pods.go

@@ -65,8 +65,14 @@ func GetPodLogs(
 		klog.Errorf("Failed to get logs for %s/%s in namespace %s due to %v", podName, containerName, namespace, err)
 		return "", err
 	}
-	defer podLogs.Close()
 	buf := new(bytes.Buffer)
+	defer func() {
+		err := podLogs.Close()
+		if err != nil {
+			klog.Errorf("Failed to close pod logs due to %v", err)
+		}
+	}()
+
 	_, err = io.Copy(buf, podLogs)
 	if err != nil {
 		klog.Errorf("Failed to copy pod logs to buffer due to %v", err)

tests/pkg/utils/utils.go

@@ -191,7 +191,7 @@ func FetchBearerToken(opt TestOptions) (string, error) {
 	}
 	for _, secret := range secretList.Items {
 		// nolint:staticcheck
-		if secret.GetObjectMeta() != nil && len(secret.GetObjectMeta().GetAnnotations()) > 0 {
+		if len(secret.GetObjectMeta().GetAnnotations()) > 0 {
 			annos := secret.GetObjectMeta().GetAnnotations()
 			sa, saExists := annos["kubernetes.io/service-account.name"]
 			//_, createByExists := annos["kubernetes.io/created-by"]