Merge pull request #2578 from Poor12/fix-sa

Fix serviceaccount continual regeneration by service account controller

pkg/util/lifted/retain.go

@@ -84,33 +84,49 @@ func retainServiceClusterIP(desired, observed *unstructured.Unstructured) error
 // +lifted:source=https://github.com/kubernetes-sigs/kubefed/blob/master/pkg/controller/sync/dispatch/retain.go
 // +lifted:changed
 
-// RetainServiceAccountFields retains the 'secrets' field of a service account
-// if the desired representation does not include a value for the field.  This
-// ensures that the sync controller doesn't continually clear a generated
+// RetainServiceAccountFields merges the 'secrets' field in the service account
+// of the control plane and the member clusters and retains the merged service account. This
+// ensures that the karmada-controller-manager doesn't continually clear a generated
 // secret from a service account, prompting continual regeneration by the
 // service account controller in the member cluster.
+// Related issue: https://github.com/karmada-io/karmada/issues/2573
 func RetainServiceAccountFields(desired, observed *unstructured.Unstructured) (*unstructured.Unstructured, error) {
-	// Check whether the secrets field is populated in the desired object.
+	var mergedSecrets []interface{}
+	isSecretExistMap := make(map[string]struct{})
+
 	desiredSecrets, ok, err := unstructured.NestedSlice(desired.Object, SecretsField)
 	if err != nil {
 		return nil, fmt.Errorf("error retrieving secrets from desired service account: %w", err)
 	}
+
 	if ok && len(desiredSecrets) > 0 {
-		// Field is populated, so an update to the target resource does not
-		// risk triggering a race with the service account controller.
-		return desired, nil
+		for _, desiredSecret := range desiredSecrets {
+			secretName := desiredSecret.(map[string]interface{})["name"].(string)
+			mergedSecrets = append(mergedSecrets, desiredSecret)
+			isSecretExistMap[secretName] = struct{}{}
+		}
 	}
 
-	// Retrieve the secrets from the cluster object and retain them.
 	secrets, ok, err := unstructured.NestedSlice(observed.Object, SecretsField)
 	if err != nil {
 		return nil, fmt.Errorf("error retrieving secrets from service account: %w", err)
 	}
+
 	if ok && len(secrets) > 0 {
-		err := unstructured.SetNestedField(desired.Object, secrets, SecretsField)
-		if err != nil {
-			return nil, fmt.Errorf("error setting secrets for service account: %w", err)
+		for _, secret := range secrets {
+			secretName := secret.(map[string]interface{})["name"].(string)
+			if _, exist := isSecretExistMap[secretName]; exist {
+				continue
+			}
+			mergedSecrets = append(mergedSecrets, secret)
+			isSecretExistMap[secretName] = struct{}{}
 		}
 	}
+
+	err = unstructured.SetNestedField(desired.Object, mergedSecrets, SecretsField)
+	if err != nil {
+		return nil, fmt.Errorf("error setting secrets for service account: %w", err)
+	}
+
 	return desired, nil
 }

pkg/util/lifted/retain_test.go

@@ -21,6 +21,7 @@ limitations under the License.
 package lifted
 
 import (
+	"reflect"
 	"testing"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -187,3 +188,106 @@ func TestRetainClusterIPInServiceFields(t *testing.T) {
 		})
 	}
 }
+
+func TestRetainServiceAccountFields(t *testing.T) {
+	tests := []struct {
+		name                 string
+		desiredObj           *unstructured.Unstructured
+		observedObj          *unstructured.Unstructured
+		expectedErr          bool
+		expectedSecretsValue []interface{}
+	}{
+		{
+			name: "neither desired or observed service account has the secrets field",
+			desiredObj: &unstructured.Unstructured{
+				Object: map[string]interface{}{},
+			},
+			observedObj: &unstructured.Unstructured{
+				Object: map[string]interface{}{},
+			},
+			expectedErr:          false,
+			expectedSecretsValue: nil,
+		},
+		{
+			name: "both desired and observed service account have the same secrets field",
+			desiredObj: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"secrets": []interface{}{
+						map[string]interface{}{
+							"name": "test",
+						},
+					},
+				},
+			},
+			observedObj: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"secrets": []interface{}{
+						map[string]interface{}{
+							"name": "test",
+						},
+					},
+				},
+			},
+			expectedErr: false,
+			expectedSecretsValue: []interface{}{
+				map[string]interface{}{
+					"name": "test",
+				},
+			},
+		},
+		{
+			name: "desired and observed service account have the different secrets field",
+			desiredObj: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"secrets": []interface{}{
+						map[string]interface{}{
+							"name": "test",
+						},
+					},
+				},
+			},
+			observedObj: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"secrets": []interface{}{
+						map[string]interface{}{
+							"name": "test",
+						},
+						map[string]interface{}{
+							"name": "test-token",
+						},
+					},
+				},
+			},
+			expectedErr: false,
+			expectedSecretsValue: []interface{}{
+				map[string]interface{}{
+					"name": "test",
+				},
+				map[string]interface{}{
+					"name": "test-token",
+				},
+			},
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			desiredObj, err := RetainServiceAccountFields(test.desiredObj, test.observedObj)
+			if (err != nil) != test.expectedErr {
+				t.Fatalf("unexpected returned error %v", err)
+			}
+
+			if err == nil {
+				currentSecretValue, ok, err := unstructured.NestedSlice(desiredObj.Object, SecretsField)
+				if err != nil {
+					t.Fatalf("failed to get the secrets field from the serviceaccount, err is: %v", err)
+				}
+				if !ok && test.expectedSecretsValue != nil {
+					t.Fatalf("expect specified secrets %s but not found", test.expectedSecretsValue)
+				}
+				if ok && !reflect.DeepEqual(test.expectedSecretsValue, currentSecretValue) {
+					t.Fatalf("expect specified secrets %s but get %s", test.expectedSecretsValue, currentSecretValue)
+				}
+			}
+		})
+	}
+}