Prevent Service Account annotation conflicts on OpenShift 4.16 (strim…

…zi#10314)

Signed-off-by: Jakub Scholz <[email protected]>

cluster-operator/src/main/java/io/strimzi/operator/cluster/operator/resource/kubernetes/ServiceAccountOperator.java

@@ -14,10 +14,14 @@
 import io.vertx.core.Future;
 import io.vertx.core.Vertx;
 
+import java.util.Map;
+
 /**
  * Operator for managing Service Accounts
  */
 public class ServiceAccountOperator extends AbstractNamespacedResourceOperator<KubernetesClient, ServiceAccount, ServiceAccountList, ServiceAccountResource> {
+    /* test */ static final String OPENSHIFT_IO_INTERNAL_REGISTRY_PULL_SECRET_REF = "openshift.io/internal-registry-pull-secret-ref";
+
     /**
      * Constructor
      * @param vertx The Vertx instance
@@ -42,6 +46,27 @@ protected Future<ReconcileResult<ServiceAccount>> internalUpdate(Reconciliation
             desired.setImagePullSecrets(current.getImagePullSecrets());
         }
 
+        // OpenShift 4.16 will create an image pull secret, add it to the image pull secrets list, and attach an
+        // annotation openshift.io/internal-registry-pull-secret-ref to the Service Account with the name of the image
+        // pull secret. If Strimzi removes this annotation by patching the resource in the next reconciliation,
+        // OpenShift will create another Secret, add it to the image pull secret list and add back the annotation.
+        // This way, every reconciliation will for every Service Account make OCP generate new Secret and in a little
+        // while, the namespace is full of these Secrets.
+        //
+        // The code below recovers the annotation from the original resource when patching it and reads it to the
+        // desired Service Account to avoid removing the annotation and triggering OCP to create a new Secret. This is
+        // not a great solution, but until we have Server Side Apply support, this is the best we can do.
+        if (current.getMetadata() != null
+                && current.getMetadata().getAnnotations() != null
+                && current.getMetadata().getAnnotations().containsKey(OPENSHIFT_IO_INTERNAL_REGISTRY_PULL_SECRET_REF))  {
+            if (desired.getMetadata().getAnnotations() != null
+                    && !desired.getMetadata().getAnnotations().containsKey(OPENSHIFT_IO_INTERNAL_REGISTRY_PULL_SECRET_REF)) {
+                desired.getMetadata().getAnnotations().put(OPENSHIFT_IO_INTERNAL_REGISTRY_PULL_SECRET_REF, current.getMetadata().getAnnotations().get(OPENSHIFT_IO_INTERNAL_REGISTRY_PULL_SECRET_REF));
+            } else if (desired.getMetadata().getAnnotations() == null)    {
+                desired.getMetadata().setAnnotations(Map.of(OPENSHIFT_IO_INTERNAL_REGISTRY_PULL_SECRET_REF, current.getMetadata().getAnnotations().get(OPENSHIFT_IO_INTERNAL_REGISTRY_PULL_SECRET_REF)));
+            }
+        }
+
         return super.internalUpdate(reconciliation, namespace, name, current, desired);
     }
 }

cluster-operator/src/test/java/io/strimzi/operator/cluster/operator/resource/kubernetes/ServiceAccountOperatorTest.java

@@ -137,6 +137,7 @@ public void testSecretsPatching(VertxTestContext context)   {
 
         ServiceAccount current = new ServiceAccountBuilder()
                 .withNewMetadata()
+                    .withAnnotations(Map.of(ServiceAccountOperator.OPENSHIFT_IO_INTERNAL_REGISTRY_PULL_SECRET_REF, "pullSecretName1"))
                     .withNamespace(NAMESPACE)
                     .withName(RESOURCE_NAME)
                 .endMetadata()
@@ -182,6 +183,61 @@ public void testSecretsPatching(VertxTestContext context)   {
                     assertThat(saCaptor.getValue().getImagePullSecrets(), is(imagePullSecrets));
                     assertThat(saCaptor.getValue().getMetadata().getLabels().get("lKey"), is("lValue"));
                     assertThat(saCaptor.getValue().getMetadata().getAnnotations().get("aKey"), is("aValue"));
+                    assertThat(saCaptor.getValue().getMetadata().getAnnotations().get(ServiceAccountOperator.OPENSHIFT_IO_INTERNAL_REGISTRY_PULL_SECRET_REF), is("pullSecretName1"));
+
+                    async.flag();
+                }));
+    }
+
+    @Test
+    public void testSecretsPatchingNoChange(VertxTestContext context)   {
+        List<ObjectReference> secrets = List.of(
+                new ObjectReferenceBuilder().withName("secretName1").build(),
+                new ObjectReferenceBuilder().withName("secretName2").build()
+        );
+
+        List<LocalObjectReference> imagePullSecrets = List.of(
+                new LocalObjectReferenceBuilder().withName("pullSecretName1").build(),
+                new LocalObjectReferenceBuilder().withName("pullSecretName2").build()
+        );
+
+        ServiceAccount current = new ServiceAccountBuilder()
+                .withNewMetadata()
+                    .withAnnotations(Map.of(ServiceAccountOperator.OPENSHIFT_IO_INTERNAL_REGISTRY_PULL_SECRET_REF, "pullSecretName1"))
+                    .withNamespace(NAMESPACE)
+                    .withName(RESOURCE_NAME)
+                .endMetadata()
+                .withSecrets(secrets)
+                .withImagePullSecrets(imagePullSecrets)
+                .build();
+
+        ServiceAccount desired = new ServiceAccountBuilder()
+                .withNewMetadata()
+                    .withNamespace(NAMESPACE)
+                    .withName(RESOURCE_NAME)
+                .endMetadata()
+                .build();
+
+        Resource mockResource = mock(resourceType());
+        when(mockResource.get()).thenReturn(current);
+        when(mockResource.patch(any(), any())).thenReturn(desired);
+        when(mockResource.withPropagationPolicy(DeletionPropagation.FOREGROUND)).thenReturn(mockResource);
+
+        NonNamespaceOperation mockNameable = mock(NonNamespaceOperation.class);
+        when(mockNameable.withName(matches(RESOURCE_NAME))).thenReturn(mockResource);
+
+        MixedOperation mockCms = mock(MixedOperation.class);
+        when(mockCms.inNamespace(matches(NAMESPACE))).thenReturn(mockNameable);
+
+        KubernetesClient mockClient = mock(clientType());
+        mocker(mockClient, mockCms);
+
+        ServiceAccountOperator op = new ServiceAccountOperator(vertx, mockClient);
+
+        Checkpoint async = context.checkpoint();
+        op.reconcile(Reconciliation.DUMMY_RECONCILIATION, NAMESPACE, RESOURCE_NAME, desired)
+                .onComplete(context.succeeding(rr -> {
+                    verify(mockResource, never()).patch(any(), any(ServiceAccount.class));
 
                     async.flag();
                 }));