From 68a994764a8be978a5a5ace2aebcd7e0a05fe9ec Mon Sep 17 00:00:00 2001 From: Subbarao Meduri Date: Thu, 19 Oct 2023 11:35:54 -0400 Subject: [PATCH] fix defer unsafe Close vulnerability Signed-off-by: Subbarao Meduri --- .../metricfamily/hypershift_transformer.go | 2 +- .../pkg/metricsclient/metricsclient.go | 3 +- loaders/dashboards/pkg/util/grafana_util.go | 18 +++++++----- operators/endpointmetrics/pkg/util/client.go | 2 +- .../multiclusterobservability_controller.go | 28 +++++++++++------- .../observatorium.go | 2 +- .../observatorium_test.go | 29 ++++++++++--------- proxy/pkg/util/util.go | 12 ++++++-- tests/pkg/utils/mco_pods.go | 6 +++- 9 files changed, 62 insertions(+), 40 deletions(-) diff --git a/collectors/metrics/pkg/metricfamily/hypershift_transformer.go b/collectors/metrics/pkg/metricfamily/hypershift_transformer.go index d3c9f73ea4..9bb5f4f531 100644 --- a/collectors/metrics/pkg/metricfamily/hypershift_transformer.go +++ b/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") diff --git a/collectors/metrics/pkg/metricsclient/metricsclient.go b/collectors/metrics/pkg/metricsclient/metricsclient.go index 24f1b76bca..6a72c5be41 100644 --- a/collectors/metrics/pkg/metricsclient/metricsclient.go +++ b/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" diff --git a/loaders/dashboards/pkg/util/grafana_util.go b/loaders/dashboards/pkg/util/grafana_util.go index 6b81a80b89..0d94cc216c 100644 --- a/loaders/dashboards/pkg/util/grafana_util.go +++ b/loaders/dashboards/pkg/util/grafana_util.go @@ -61,14 +61,16 @@ 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 err != nil { - klog.Info("failed to parse response body ", "error ", err) - } - return respBody, resp.StatusCode - } else { + if resp == nil { return nil, http.StatusNotFound } + + respBody, err := io.ReadAll(resp.Body) + if err != nil { + klog.Info("failed to parse response body ", "error ", err) + resp.Body.Close() + return nil, resp.StatusCode + } + resp.Body.Close() + return respBody, resp.StatusCode } diff --git a/operators/endpointmetrics/pkg/util/client.go b/operators/endpointmetrics/pkg/util/client.go index 42eb84a234..b2ca4b625b 100644 --- a/operators/endpointmetrics/pkg/util/client.go +++ b/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 } diff --git a/operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go b/operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go index 7400c6b6a1..4503e3a096 100644 --- a/operators/multiclusterobservability/controllers/multiclusterobservability/multiclusterobservability_controller.go +++ b/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 } diff --git a/operators/multiclusterobservability/controllers/multiclusterobservability/observatorium.go b/operators/multiclusterobservability/controllers/multiclusterobservability/observatorium.go index 7ffdad336d..29b1188e6d 100644 --- a/operators/multiclusterobservability/controllers/multiclusterobservability/observatorium.go +++ b/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) diff --git a/operators/multiclusterobservability/controllers/multiclusterobservability/observatorium_test.go b/operators/multiclusterobservability/controllers/multiclusterobservability/observatorium_test.go index d7a49acd43..cd995b95cc 100644 --- a/operators/multiclusterobservability/controllers/multiclusterobservability/observatorium_test.go +++ b/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) } diff --git a/proxy/pkg/util/util.go b/proxy/pkg/util/util.go index 757832b489..fae3e33680 100644 --- a/proxy/pkg/util/util.go +++ b/proxy/pkg/util/util.go @@ -427,14 +427,18 @@ func FetchUserProjectList(token string, url string) []string { writeError(fmt.Sprintf("failed to send http request: %v", err)) return []string{} } - defer resp.Body.Close() var projects projectv1.ProjectList err = json.NewDecoder(resp.Body).Decode(&projects) if err != nil { klog.Errorf("failed to decode response json body: %v", err) + resp.Body.Close() return []string{} } + err = resp.Body.Close() + if err != nil { + klog.Errorf("failed to close response body: %v", err) + } projectList := make([]string, len(projects.Items)) for idx, p := range projects.Items { @@ -451,14 +455,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 } diff --git a/tests/pkg/utils/mco_pods.go b/tests/pkg/utils/mco_pods.go index 4a3d567f7c..caa22bfe1b 100644 --- a/tests/pkg/utils/mco_pods.go +++ b/tests/pkg/utils/mco_pods.go @@ -65,12 +65,16 @@ 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) _, err = io.Copy(buf, podLogs) if err != nil { klog.Errorf("Failed to copy pod logs to buffer due to %v", err) + podLogs.Close() return "", err } + err = podLogs.Close() + if err != nil { + klog.Errorf("Failed to close pod logs due to %v", err) + } return buf.String(), nil }