Merge pull request #1734 from JoaoBraveCoding/2115527

Bug 2115527: reverts #1704
openshift · Aug 5, 2022 · c288090 · c288090
2 parents f43c87e + b339add
commit c288090
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 127 deletions.
diff --git a/jsonnet/components/cluster-monitoring-operator.libsonnet b/jsonnet/components/cluster-monitoring-operator.libsonnet
@@ -240,7 +240,7 @@ function(params) {
       {
         apiGroups: [''],
         resources: ['services', 'serviceaccounts', 'configmaps'],
-        verbs: ['create', 'get', 'list', 'watch', 'update', 'patch', 'delete'],
+        verbs: ['create', 'get', 'list', 'watch', 'update', 'delete'],
       },
       {
         apiGroups: ['apps'],

diff --git a/manifests/0000_50_cluster-monitoring-operator_02-namespaced-cluster-role.yaml b/manifests/0000_50_cluster-monitoring-operator_02-namespaced-cluster-role.yaml
@@ -19,7 +19,6 @@ rules:
   - list
   - watch
   - update
-  - patch
   - delete
 - apiGroups:
   - apps

diff --git a/pkg/client/client.go b/pkg/client/client.go
@@ -16,7 +16,6 @@ package client
 
 import (
 	"context"
-	"encoding/json"
 	"net/url"
 	"reflect"
 	"strings"
@@ -50,8 +49,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apimachinery/pkg/util/strategicpatch"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/client-go/kubernetes"
@@ -1529,38 +1526,42 @@ func (c *Client) CreateOrUpdateClusterRoleBinding(ctx context.Context, crb *rbac
 
 func (c *Client) CreateOrUpdateServiceAccount(ctx context.Context, sa *v1.ServiceAccount) error {
 	sClient := c.kclient.CoreV1().ServiceAccounts(sa.GetNamespace())
-	existing, err := sClient.Get(ctx, sa.GetName(), metav1.GetOptions{})
+	_, err := sClient.Get(ctx, sa.GetName(), metav1.GetOptions{})
 	if apierrors.IsNotFound(err) {
 		_, err := sClient.Create(ctx, sa, metav1.CreateOptions{})
 		return errors.Wrap(err, "creating ServiceAccount object failed")
 	}
-	if err != nil {
-		return errors.Wrap(err, "retrieving ServiceAccount object failed")
-	}
-
-	required := sa.DeepCopy()
-	mergeMetadata(&required.ObjectMeta, existing.ObjectMeta)
-
-	// Why we use Patch here? ServiceAccounts get a new secret generated whenever
-	// they are updated, even if nothing has changed. This is likely due to "Update"
-	// performing a PUT call signifying, that this may be a new ServiceAccount,
-	// therefore a new token is needed.
-	oldData, err := json.Marshal(existing)
-	if err != nil {
-		return errors.Wrap(err, "marshaling existing ServiceAccount object failed")
-	}
-
-	newData, err := json.Marshal(required)
-	if err != nil {
-		return errors.Wrap(err, "marshaling updated ServiceAccount object failed")
-	}
-
-	patchBytes, patchErr := strategicpatch.CreateTwoWayMergePatch(oldData, newData, required)
-	if patchErr != nil {
-		return errors.Wrap(patchErr, "creating ServiceAccount two way merge patch failed")
-	}
-	_, err = sClient.Patch(ctx, required.Name, types.StrategicMergePatchType, patchBytes, metav1.PatchOptions{})
-	return errors.Wrap(err, "patching ServiceAccount object failed")
+	return errors.Wrap(err, "retrieving ServiceAccount object failed")
+	// TODO(JoaoBraveCoding) The code below was reverted due to
+	// https://bugzilla.redhat.com/show_bug.cgi?id=2115527 see also
+	// https://bugzilla.redhat.com/show_bug.cgi?id=2095719 for more context
+	// if err != nil {
+	// 	return errors.Wrap(err, "retrieving ServiceAccount object failed")
+	// }
+
+	// required := sa.DeepCopy()
+	// mergeMetadata(&required.ObjectMeta, existing.ObjectMeta)
+
+	// // Why we use Patch here? ServiceAccounts get a new secret generated whenever
+	// // they are updated, even if nothing has changed. This is likely due to "Update"
+	// // performing a PUT call signifying, that this may be a new ServiceAccount,
+	// // therefore a new token is needed.
+	// oldData, err := json.Marshal(existing)
+	// if err != nil {
+	// 	return errors.Wrap(err, "marshaling existing ServiceAccount object failed")
+	// }
+
+	// newData, err := json.Marshal(required)
+	// if err != nil {
+	// 	return errors.Wrap(err, "marshaling updated ServiceAccount object failed")
+	// }
+
+	// patchBytes, patchErr := strategicpatch.CreateTwoWayMergePatch(oldData, newData, required)
+	// if patchErr != nil {
+	// 	return errors.Wrap(patchErr, "creating ServiceAccount two way merge patch failed")
+	// }
+	// _, err = sClient.Patch(ctx, required.Name, types.StrategicMergePatchType, patchBytes, metav1.PatchOptions{})
+	// return errors.Wrap(err, "patching ServiceAccount object failed")
 }
 
 func (c *Client) CreateOrUpdateServiceMonitor(ctx context.Context, sm *monv1.ServiceMonitor) error {

diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go
@@ -952,100 +952,6 @@ func TestCreateOrUpdateService(t *testing.T) {
 	}
 }
 
-func TestCreateOrUpdateServiceAccount(t *testing.T) {
-	ctx := context.Background()
-	testCases := []struct {
-		name                string
-		initialLabels       map[string]string
-		initialAnnotations  map[string]string
-		updatedLabels       map[string]string
-		updatedAnnotations  map[string]string
-		expectedLabels      map[string]string
-		expectedAnnotations map[string]string
-	}{
-		{
-			name: "inital labels/annotations are empty",
-			updatedLabels: map[string]string{
-				"app.kubernetes.io/name": "app",
-			},
-			updatedAnnotations: map[string]string{
-				"monitoring.openshift.io/foo": "bar",
-			},
-			expectedLabels: map[string]string{
-				"app.kubernetes.io/name": "app",
-			},
-			expectedAnnotations: map[string]string{
-				"monitoring.openshift.io/foo": "bar",
-			},
-		},
-		{
-			name: "label/annotation merge and spec change",
-			initialLabels: map[string]string{
-				"app.kubernetes.io/name": "",
-				"label":                  "value",
-			},
-			initialAnnotations: map[string]string{
-				"monitoring.openshift.io/foo": "",
-				"annotation":                  "value",
-			},
-			updatedLabels: map[string]string{
-				"app.kubernetes.io/name": "app",
-			},
-			updatedAnnotations: map[string]string{
-				"monitoring.openshift.io/foo": "bar",
-			},
-			expectedLabels: map[string]string{
-				"app.kubernetes.io/name": "app",
-				"label":                  "value",
-			},
-			expectedAnnotations: map[string]string{
-				"monitoring.openshift.io/foo": "bar",
-				"annotation":                  "value",
-			},
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(st *testing.T) {
-			sa := &v1.ServiceAccount{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:        "prometheus-k8s",
-					Namespace:   ns,
-					Labels:      tc.initialLabels,
-					Annotations: tc.initialAnnotations,
-				},
-			}
-
-			c := Client{
-				kclient: fake.NewSimpleClientset(sa.DeepCopy()),
-			}
-
-			_, err := c.kclient.CoreV1().ServiceAccounts(ns).Get(ctx, sa.Name, metav1.GetOptions{})
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			sa.SetLabels(tc.updatedLabels)
-			sa.SetAnnotations(tc.updatedAnnotations)
-			if err := c.CreateOrUpdateServiceAccount(ctx, sa); err != nil {
-				t.Fatal(err)
-			}
-
-			after, err := c.kclient.CoreV1().ServiceAccounts(ns).Get(ctx, sa.Name, metav1.GetOptions{})
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			if !reflect.DeepEqual(tc.expectedAnnotations, after.Annotations) {
-				t.Errorf("expected annotations %q, got %q", tc.expectedAnnotations, after.Annotations)
-			}
-			if !reflect.DeepEqual(tc.expectedLabels, after.Labels) {
-				t.Errorf("expected labels %q, got %q", tc.expectedLabels, after.Labels)
-			}
-		})
-	}
-}
-
 func TestCreateOrUpdateRole(t *testing.T) {
 	ctx := context.Background()
 	testCases := []struct {