Repository: karmada-io/karmada

Issue:
Karmada does not retain ServiceAccount credentials for the OpenShift Container Registry
<!-- Please use this template while reporting a bug and provide as much info as possible. Not doing so may result in your bug not being addressed in a timely manner. Thanks!
-->


**What happened**:
In _OpenShift_,  registry credentials are automatically injected into _ServiceAccount_, like `token`. These can be seen by retrieving the service account:
```
$ oc --context admin get sa default -oyaml
apiVersion: v1
imagePullSecrets:
- name: default-dockercfg-mc4cg
kind: ServiceAccount
metadata:
  creationTimestamp: "2023-04-06T06:01:22Z"
  name: default
  namespace: default
  resourceVersion: "16218"
  uid: 20c4843c-61a8-482b-8318-ac010972b5ff
secrets:
- name: default-token-9g74c
- name: default-dockercfg-mc4cg
```
_Karmada_ will reconcile _ServiceAccount_ infinitely due to ignoring `imagePullSecrets` fileld. Unfortunately, the `imagePullSecrets` field which ServiceAccount in member cluster has gets erased by _karmada_. It's a disaster for member clusters.
 
**What you expected to happen**:
Karmada retains ServiceAccount's `imagePullSecrets` fileld and ignores `dockercfg` when applied to member clusters.

**How to reproduce it (as minimally and precisely as possible)**:

**Anything else we need to know?**:
Ref:
* https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#add-image-pull-secret-to-service-account
* https://docs.openshift.com/container-platform/3.11/dev_guide/service_accounts.html#dev-managing-service-accounts

**Environment**:
- Karmada version: All
- kubectl-karmada or karmadactl version (the result of `kubectl-karmada version` or `karmadactl version`):
- Others: OpenShift Env


Discussion:
@Poor12: Yeah, the default retain method of serviceaccount handles secrets, but not handle imagePullSecrets. I think you propagate a sa to openshift cluster from karmada control plane and the member cluster auto injects the imagePullSecerts to sa, but karmada control plane orverrides it, is it the problem?

You can use [custom resource interpreter](https://karmada.io/docs/userguide/globalview/customizing-resource-interpreter) to override the default retain func to retain the imagePullSecrets. Both declarative configuration or webhook is OK.@whitewindmills: I know how to fix it, I mean _Karmada_ should adapt to _OpenShift_ by retaining `imagePullSecrets`. @Poor12: > I know how to fix it, I mean _Karmada_ should adapt to _OpenShift_ by retaining `imagePullSecrets`.

I’m not sure what openshift do(auto injected `imagePullSecrets` to `serviceaccount`) is a standard and routine operation. If it‘s a custom behavior of cloud vendors, we may need to consider whether to add it to the build-in retain process.
@whitewindmills: Yeah, I think we should do it.
@RainbowMango @XiShanYongYe-Chang Do you have any different opinion?@XiShanYongYe-Chang: The member cluster automatically registers `imagePullSecrets` for ServiceAccount, however, this field does not exist on the Karmada control plane. As a result, a conflict occurs. It's a situation like this, is it?

This seems a common situation, how about discussing it at the community meeting tomorrow? Hi @whitewindmills, can you add a topic:
https://docs.google.com/document/d/1y6YLVC-v7cmVAdbjedoyR5WL0-q45DBRXTvz5_I7bkA/edit@whitewindmills: @XiShanYongYe-Chang 
You are right, let's do it.@RainbowMango: > In OpenShift, registry credentials are automatically injected into ServiceAccount, like token.

I want to know more detail about why OpenShift would inject `imagePullSecrets` to SA.  Can you share a link? (I checked the link above, no introduction to this mechanism).

> Karmada retains ServiceAccount's imagePullSecrets fileld and ignores dockercfg when applied to member clusters.

I can get you are asking to retain `imagePullSecrets` just like `secrets`, but why ignore `dockercfg`?@whitewindmills: > I want to know more detail about why OpenShift would inject `imagePullSecrets` to SA. Can you share a link? (I checked the link above, no introduction to this mechanism).

Fine, but it's actually mentioned, maybe it's not obvious. You can still prove this via [_OpenShift_'s code](https://github.com/openshift/openshift-controller-manager/blob/master/pkg/serviceaccounts/controllers/create_dockercfg_secrets.go#L333)
![image](https://user-images.githubusercontent.com/89241565/231420042-34c570ad-ffcd-4881-90b7-c12474ccc16d.png)

> I can get you are asking to retain imagePullSecrets just like secrets, but why ignore dockercfg?

Because _OpenShift_ usually cannot change the _ServiceAccount_ behavior of the _Karmada_ control plane. I think no one is stupid enough to deploy _Karmada_ control plane with _OpenShift_, that would be very bad.



@RainbowMango: Thanks for reminding me, I get it. 
It sounds reasonable for me to retain `imagePullSecrets`, since we will `merge` the secrets instead of replacing them.

But I still don't understand what you mean by `ignores dockercfg when applied to member clusters.`. Can you explain it in more detail or give an example?@whitewindmills: > But I still don't understand what you mean by ignores dockercfg when applied to member clusters.. Can you explain it in more detail or give an example?

Sorry, I have to admit that what I described is flawed. In fact, what I wanted to explain is that we can ignore `dockercfg` secret before being first applied to member clusters by `Prune`@ahorine: > It sounds reasonable for me to retain imagePullSecrets, since we will merge the secrets instead of replacing them.

@RainbowMango has there been any work done already toward realizing this change? In 1.13 I am still seeing the described behavior of Karmada and Openshift going back and forth adding and removing the `imagePullSecrets`, leading to inconsistent deployment behavior.@RainbowMango: @ahorine This is not a blocker, as people can customize the resource interpreter to let Karmada retain the imagePullSecrets when updating the resource. 

@whitewindmills Are you using the customized interpreter? Can you share the interpreter configuration here? Let's see if we need to make it the default?@ahorine: > @ahorine This is not a blocker, as people can customize the resource interpreter to let Karmada retain the imagePullSecrets when updating the resource. 

Yeah I was looking at custom interpreter adjustments for this, but I figured I'd at least make sure there wasn't already anything built in here.

I think there could be a reasonable middle ground achieved here for defaults, something along the lines of "if the incoming resource has the `imagePullSecrets` set, make sure the downstream resource matches, otherwise don't touch the field". More advanced use cases would still be achieved via custom interpreters.

Either way, I agree that it isn't a blocker as it stands, it's just somewhat inconvenient.@RainbowMango: Yeah, thanks for bringing this up. Just tagged this to the v1.14 milestone, so that we won't miss it again.

> Either way, I agree that it isn't a blocker as it stands, it's just somewhat inconvenient.

100% agree! Two possible approaches:
1. Provide guidelines on how to configure the resource interpreter
2. Provide built-in support for this case.@whitewindmills: > @whitewindmills Are you using the customized interpreter? Can you share the interpreter configuration here? Let's see if we need to make it the default?

no, I make it the default, and use the built-in interpreter.@whitewindmills: If you guys agree, I'd be happy to contribute to the community.@RainbowMango: Can you share more details about how you do? A code example would be helpful.@RainbowMango: Kindly ping @whitewindmills 
Can you share more details about your customized interpreter?@whitewindmills: Instead of using a custom interpreter, I modified the built-in interpreter to support this case.
```go
func RetainServiceAccountFields(desired, observed *unstructured.Unstructured) (*unstructured.Unstructured, error) {
        ... ...
	imagePullSecrets, ok, err := unstructured.NestedSlice(observed.Object, "imagePullSecrets")
	if err != nil {
		return nil, fmt.Errorf("error retrieving imagePullSecrets from service account: %w", err)
	}
	if ok && len(imagePullSecrets) > 0 {
		err = unstructured.SetNestedField(desired.Object, imagePullSecrets, "imagePullSecrets")
		if err != nil {
			return nil, fmt.Errorf("error setting imagePullSecrets for service account: %w", err)
		}
	}
}
```@RainbowMango: Currently the default built-in interpreter merges the `secrets` of the ServiceAccount. Do you think it's a good idea to merge the `ImagePullSecrets` as well? I mean, provide built-in support for this case. Like what I said at https://github.com/karmada-io/karmada/issues/3370#issuecomment-2808181158.
@whitewindmills: > Provide built-in support for this case.

100% agree. this change is also compatible with pure k8s clusters.@RainbowMango: So, would you like to send a PR?
@whitewindmills: okay, I'll fix this ASAP.
/assign@RainbowMango: Great!
/remove-kind bug
/kind feature

I don't think this is a bug and should not be backported to previous releases as it involves a behavior change.@RainbowMango: @ahorine This issue has been addressed by #6532, which will be included in v1.15. @ahorine: > @ahorine This issue has been addressed by #6532, which will be included in v1.15. 

Fantastic, I look forward to testing it out. Thanks!@RainbowMango: Yeah, looking forward to your feedback~ 
Now you can test it against the master branch.

Solution patch:
diff --git a/pkg/util/lifted/retain.go b/pkg/util/lifted/retain.go
index e338b233c..afc0dbd2c 100644
--- a/pkg/util/lifted/retain.go
+++ b/pkg/util/lifted/retain.go
@@ -29,6 +29,8 @@ import (
 const (
 	// SecretsField indicates the 'secrets' field of a service account
 	SecretsField = "secrets"
+	// ImagePullSecretsField indicates the 'imagePullSecrets' field of a service account
+	ImagePullSecretsField = "imagePullSecrets"
 )
 
 // +lifted:source=https://github.com/kubernetes-sigs/kubefed/blob/master/pkg/controller/sync/dispatch/retain.go
@@ -84,19 +86,36 @@ 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 merges the 'secrets' field in the service account
+// RetainServiceAccountFields merges the 'secrets' field and the 'imagePullSecrets' 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
+// Related issue:
+// https://github.com/karmada-io/karmada/issues/2573
+// https://github.com/karmada-io/karmada/issues/3370
 func RetainServiceAccountFields(desired, observed *unstructured.Unstructured) (*unstructured.Unstructured, error) {
+	secretsFields := []string{
+		SecretsField,
+		ImagePullSecretsField,
+	}
+
+	for _, field := range secretsFields {
+		if err := retainSecretsField(desired, observed, field); err != nil {
+			return nil, err
+		}
+	}
+	return desired, nil
+}
+
+// retainSecretsField merges a given secrets field (e.g. 'secrets' or 'imagePullSecrets') in the service account.
+func retainSecretsField(desired, observed *unstructured.Unstructured, fieldName string) error {
 	var mergedSecrets []interface{}
 	isSecretExistMap := make(map[string]struct{})
 
-	desiredSecrets, ok, err := unstructured.NestedSlice(desired.Object, SecretsField)
+	desiredSecrets, ok, err := unstructured.NestedSlice(desired.Object, fieldName)
 	if err != nil {
-		return nil, fmt.Errorf("error retrieving secrets from desired service account: %w", err)
+		return fmt.Errorf("error retrieving %s from desired service account: %w", fieldName, err)
 	}
 
 	if ok && len(desiredSecrets) > 0 {
@@ -107,9 +126,9 @@ func RetainServiceAccountFields(desired, observed *unstructured.Unstructured) (*
 		}
 	}
 
-	secrets, ok, err := unstructured.NestedSlice(observed.Object, SecretsField)
+	secrets, ok, err := unstructured.NestedSlice(observed.Object, fieldName)
 	if err != nil {
-		return nil, fmt.Errorf("error retrieving secrets from service account: %w", err)
+		return fmt.Errorf("error retrieving %s from service account: %w", fieldName, err)
 	}
 
 	if ok && len(secrets) > 0 {
@@ -123,10 +142,9 @@ func RetainServiceAccountFields(desired, observed *unstructured.Unstructured) (*
 		}
 	}
 
-	err = unstructured.SetNestedField(desired.Object, mergedSecrets, SecretsField)
+	err = unstructured.SetNestedField(desired.Object, mergedSecrets, fieldName)
 	if err != nil {
-		return nil, fmt.Errorf("error setting secrets for service account: %w", err)
+		return fmt.Errorf("error setting %s for service account: %w", fieldName, err)
 	}
-
-	return desired, nil
+	return nil
 }



Test patch:
diff --git a/pkg/util/lifted/retain_test.go b/pkg/util/lifted/retain_test.go
index cafabdc2f..519a85305 100644
--- a/pkg/util/lifted/retain_test.go
+++ b/pkg/util/lifted/retain_test.go
@@ -191,11 +191,12 @@ 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                          string
+		desiredObj                    *unstructured.Unstructured
+		observedObj                   *unstructured.Unstructured
+		expectedErr                   bool
+		expectedSecretsValue          []interface{}
+		expectedImagePullSecretsValue []interface{}
 	}{
 		{
 			name: "neither desired or observed service account has the secrets field",
@@ -234,6 +235,7 @@ func TestRetainServiceAccountFields(t *testing.T) {
 					"name": "test",
 				},
 			},
+			expectedImagePullSecretsValue: nil,
 		},
 		{
 			name: "desired and observed service account have the different secrets field",
@@ -267,6 +269,44 @@ func TestRetainServiceAccountFields(t *testing.T) {
 					"name": "test-token",
 				},
 			},
+			expectedImagePullSecretsValue: nil,
+		},
+		{
+			name: "observed service account has the imagePullSecrets field but the desired not",
+			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",
+						},
+					},
+					"imagePullSecrets": []interface{}{
+						map[string]interface{}{
+							"name": "foo",
+						},
+					},
+				},
+			},
+			expectedErr: false,
+			expectedSecretsValue: []interface{}{
+				map[string]interface{}{
+					"name": "test",
+				},
+			},
+			expectedImagePullSecretsValue: []interface{}{
+				map[string]interface{}{
+					"name": "foo",
+				},
+			},
 		},
 	}
 	for _, test := range tests {
@@ -277,15 +317,26 @@ func TestRetainServiceAccountFields(t *testing.T) {
 			}
 
 			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)
+				table := []struct {
+					name          string
+					field         string
+					expectedValue []interface{}
+				}{
+					{name: "secrets", field: SecretsField, expectedValue: test.expectedSecretsValue},
+					{name: "imagePullSecrets", field: ImagePullSecretsField, expectedValue: test.expectedImagePullSecretsValue},
 				}
-				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)
+
+				for _, entry := range table {
+					currentValue, ok, err := unstructured.NestedSlice(desiredObj.Object, entry.field)
+					if err != nil {
+						t.Fatalf("failed to get %q field from serviceaccount: %v", entry.name, err)
+					}
+					if !ok && entry.expectedValue != nil {
+						t.Fatalf("expected %q %v but not found", entry.name, entry.expectedValue)
+					}
+					if ok && !reflect.DeepEqual(entry.expectedValue, currentValue) {
+						t.Fatalf("unexpected %q: got %v, want %v", entry.name, currentValue, entry.expectedValue)
+					}
 				}
 			}
 		})