Optimize asset secret update logic. (prometheus-operator#3933)

* asset secret update optimize * Update pkg/prometheus/operator.go Co-authored-by: Simon Pasquier <spasquie@redhat.com> * optimize code * optimize updateSecret to CreateOrUpdateSecret * optimize updateSecret to CreateOrUpdateSecret * optimize updateSecret to CreateOrUpdateSecret * optimize updateSecret to CreateOrUpdateSecret * optimize code * optimize code * Update pkg/k8sutil/k8sutil.go Co-authored-by: Simon Pasquier <spasquie@redhat.com> * Update pkg/k8sutil/k8sutil.go Co-authored-by: Simon Pasquier <spasquie@redhat.com> * Update pkg/prometheus/operator.go Co-authored-by: Simon Pasquier <spasquie@redhat.com> * optimize code * remove deep copy * format code * rollback deepcopy code * rollback deepcopy code * optimize code * optimize code * optimize code * Update pkg/k8sutil/k8sutil.go Co-authored-by: Simon Pasquier <spasquie@redhat.com> * Update pkg/alertmanager/operator.go Co-authored-by: Simon Pasquier <spasquie@redhat.com> * Update pkg/prometheus/operator.go Co-authored-by: Simon Pasquier <spasquie@redhat.com> * Update pkg/k8sutil/k8sutil.go Co-authored-by: Simon Pasquier <spasquie@redhat.com> * optimize code style * optimize code style * optimize code style Co-authored-by: Simon Pasquier <spasquie@redhat.com>
pasancario · Apr 7, 2021 · b18d542 · b18d542
1 parent 89ee666
commit b18d542
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 91 deletions.
diff --git a/pkg/alertmanager/operator.go b/pkg/alertmanager/operator.go
@@ -834,25 +834,9 @@ func (c *Operator) createOrUpdateGeneratedConfigSecret(ctx context.Context, am *
 	}
 	generatedConfigSecret.Data[alertmanagerConfigFile] = conf
 
-	_, err := sClient.Get(ctx, generatedConfigSecret.Name, metav1.GetOptions{})
+	err := k8sutil.CreateOrUpdateSecret(ctx, sClient, generatedConfigSecret)
 	if err != nil {
-		if !apierrors.IsNotFound(err) {
-			return errors.Wrapf(
-				err,
-				"failed to check whether generated config secret already exists for Alertmanager %v in namespace %v",
-				am.Name,
-				am.Namespace,
-			)
-		}
-		_, err = sClient.Create(ctx, generatedConfigSecret, metav1.CreateOptions{})
-		level.Debug(c.logger).Log("msg", "created generated config secret", "secretname", generatedConfigSecret.Name)
-	} else {
-		err = k8sutil.UpdateSecret(ctx, sClient, generatedConfigSecret)
-		level.Debug(c.logger).Log("msg", "updated generated config secret", "secretname", generatedConfigSecret.Name)
-	}
-
-	if err != nil {
-		return errors.Wrapf(err, "failed to update generated config secret for Alertmanager %v in namespace %v", am.Name, am.Namespace)
+		return errors.Wrap(err, "failed to update generated config secret")
 	}
 
 	return nil
@@ -1320,26 +1304,9 @@ func (c *Operator) createOrUpdateTLSAssetSecret(ctx context.Context, am *monitor
 		tlsAssetsSecret.Data[key.String()] = []byte(asset)
 	}
 
-	_, err := sClient.Get(ctx, tlsAssetsSecret.Name, metav1.GetOptions{})
-	if err != nil {
-		if !apierrors.IsNotFound(err) {
-			return errors.Wrapf(
-				err,
-				"failed to check whether tls assets secret already exists for Alertmanager %v in namespace %v",
-				am.Name,
-				am.Namespace,
-			)
-		}
-		_, err = sClient.Create(ctx, tlsAssetsSecret, metav1.CreateOptions{})
-		level.Debug(c.logger).Log("msg", "created tlsAssetsSecret", "secretname", tlsAssetsSecret.Name)
-
-	} else {
-		err = k8sutil.UpdateSecret(ctx, sClient, tlsAssetsSecret)
-		level.Debug(c.logger).Log("msg", "updated tlsAssetsSecret", "secretname", tlsAssetsSecret.Name)
-	}
-
+	err := k8sutil.CreateOrUpdateSecret(ctx, sClient, tlsAssetsSecret)
 	if err != nil {
-		return errors.Wrapf(err, "failed to create TLS assets secret for Alertmanager %v in namespace %v", am.Name, am.Namespace)
+		return errors.Wrap(err, "failed to create TLS assets secret for Alertmanager")
 	}
 
 	return nil

diff --git a/pkg/alertmanager/operator_test.go b/pkg/alertmanager/operator_test.go
@@ -1041,6 +1041,7 @@ func TestProvisionAlertmanagerConfiguration(t *testing.T) {
 			}
 
 			store := assets.NewStore(c.CoreV1(), c.CoreV1())
+
 			err = o.provisionAlertmanagerConfiguration(context.Background(), tc.am, store)
 
 			if !tc.ok {

diff --git a/pkg/k8sutil/k8sutil.go b/pkg/k8sutil/k8sutil.go
@@ -28,6 +28,7 @@ import (
 	"github.com/hashicorp/go-version"
 	"github.com/pkg/errors"
 	v1 "k8s.io/api/core/v1"
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation"
@@ -175,20 +176,34 @@ func UpdateStatefulSet(ctx context.Context, sstClient clientappsv1.StatefulSetIn
 	return nil
 }
 
-// UpdateSecret merges metadata of existing Secret with new one and updates it.
-func UpdateSecret(ctx context.Context, secretClient clientv1.SecretInterface, secret *v1.Secret) error {
-	existingSecret, err := secretClient.Get(ctx, secret.Name, metav1.GetOptions{})
+// CreateOrUpdateSecret merges metadata of existing Secret with new one and updates it.
+func CreateOrUpdateSecret(ctx context.Context, secretClient clientv1.SecretInterface, desired *v1.Secret) error {
+	existingSecret, err := secretClient.Get(ctx, desired.Name, metav1.GetOptions{})
 	if err != nil {
-		return errors.Wrap(err, "getting secret object failed")
+		if !apierrors.IsNotFound(err) {
+			return errors.Wrapf(
+				err,
+				"failed to check whether secret %q in namespace %q already exists",
+				desired.Name,
+				desired.Namespace,
+			)
+		}
+		_, err = secretClient.Create(ctx, desired, metav1.CreateOptions{})
+		return errors.Wrapf(err, "failed to create secret %q in namespace %q", desired.Name, desired.Namespace)
 	}
-
-	mergeMetadata(&secret.ObjectMeta, existingSecret.ObjectMeta)
-
-	_, err = secretClient.Update(ctx, secret, metav1.UpdateOptions{})
-	if err != nil {
-		return err
+	mutated := existingSecret.DeepCopyObject().(*v1.Secret)
+	mergeMetadata(&desired.ObjectMeta, mutated.ObjectMeta)
+	if apiequality.Semantic.DeepEqual(existingSecret, desired) {
+		return nil
+	}
+	if _, err = secretClient.Update(ctx, desired, metav1.UpdateOptions{}); err != nil {
+		return errors.Wrapf(
+			err,
+			"failed to update secret %q in namespace %q",
+			desired.Name,
+			desired.Namespace,
+		)
 	}
-
 	return nil
 }
 

diff --git a/pkg/k8sutil/k8sutil_test.go b/pkg/k8sutil/k8sutil_test.go
@@ -261,7 +261,7 @@ func TestMergeMetadata(t *testing.T) {
 		}
 	})
 
-	t.Run("UpdateSecret", func(t *testing.T) {
+	t.Run("CreateOrUpdateSecret", func(t *testing.T) {
 		for _, tc := range testCases {
 			t.Run(tc.name, func(t *testing.T) {
 				secret := &corev1.Secret{
@@ -287,7 +287,7 @@ func TestMergeMetadata(t *testing.T) {
 					t.Fatal(err)
 				}
 
-				err = UpdateSecret(context.TODO(), sClient, secret)
+				err = CreateOrUpdateSecret(context.TODO(), sClient, secret)
 				if err != nil {
 					t.Fatal(err)
 				}

diff --git a/pkg/prometheus/operator.go b/pkg/prometheus/operator.go
@@ -1529,29 +1529,8 @@ func (c *Operator) createOrUpdateConfigurationSecret(ctx context.Context, p *mon
 	}
 	s.Data[configFilename] = buf.Bytes()
 
-	curSecret, err := sClient.Get(ctx, s.Name, metav1.GetOptions{})
-	if apierrors.IsNotFound(err) {
-		level.Debug(c.logger).Log("msg", "creating configuration")
-		_, err = sClient.Create(ctx, s, metav1.CreateOptions{})
-		return err
-	}
-
-	var (
-		generatedConf             = s.Data[configFilename]
-		curConfig, curConfigFound = curSecret.Data[configFilename]
-	)
-	if curConfigFound {
-		if bytes.Equal(curConfig, generatedConf) {
-			level.Debug(c.logger).Log("msg", "updating Prometheus configuration secret skipped, no configuration change")
-			return nil
-		}
-		level.Debug(c.logger).Log("msg", "current Prometheus configuration has changed")
-	} else {
-		level.Debug(c.logger).Log("msg", "no current Prometheus configuration secret found", "currentConfigFound", curConfigFound)
-	}
-
 	level.Debug(c.logger).Log("msg", "updating Prometheus configuration secret")
-	return k8sutil.UpdateSecret(ctx, sClient, s)
+	return k8sutil.CreateOrUpdateSecret(ctx, sClient, s)
 }
 
 func (c *Operator) createOrUpdateTLSAssetSecret(ctx context.Context, p *monitoringv1.Prometheus, store *assets.Store) error {
@@ -1580,26 +1559,9 @@ func (c *Operator) createOrUpdateTLSAssetSecret(ctx context.Context, p *monitori
 		tlsAssetsSecret.Data[key.String()] = []byte(asset)
 	}
 
-	_, err := sClient.Get(ctx, tlsAssetsSecret.Name, metav1.GetOptions{})
-	if err != nil {
-		if !apierrors.IsNotFound(err) {
-			return errors.Wrapf(
-				err,
-				"failed to check whether tls assets secret already exists for Prometheus %v in namespace %v",
-				p.Name,
-				p.Namespace,
-			)
-		}
-		_, err = sClient.Create(ctx, tlsAssetsSecret, metav1.CreateOptions{})
-		level.Debug(c.logger).Log("msg", "created tlsAssetsSecret", "secretname", tlsAssetsSecret.Name)
-
-	} else {
-		err = k8sutil.UpdateSecret(ctx, sClient, tlsAssetsSecret)
-		level.Debug(c.logger).Log("msg", "updated tlsAssetsSecret", "secretname", tlsAssetsSecret.Name)
-	}
-
+	err := k8sutil.CreateOrUpdateSecret(ctx, sClient, tlsAssetsSecret)
 	if err != nil {
-		return errors.Wrapf(err, "failed to create TLS assets secret for Prometheus %v in namespace %v", p.Name, p.Namespace)
+		return errors.Wrap(err, "failed to create TLS assets secret for Prometheus")
 	}
 
 	return nil
-Original file line number
+Diff line change
@@ Expand Up @@
     			}
     			store := assets.NewStore(c.CoreV1(), c.CoreV1())
     			err = o.provisionAlertmanagerConfiguration(context.Background(), tc.am, store)
     			if !tc.ok {
@@ Expand Down @@