fixup! fix: Add a predefined secret to store credentials

eclipse-che · Aug 9, 2021 · cd092ce · cd092ce
1 parent fb0fe45
commit cd092ce
Show file tree

Hide file tree

Showing 7 changed files with 86 additions and 49 deletions.
diff --git a/...se/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java b/...se/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java
@@ -11,6 +11,7 @@
  */
 package org.eclipse.che.workspace.infrastructure.kubernetes.namespace;
 
+import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
@@ -26,7 +27,6 @@
 import org.eclipse.che.api.user.server.PreferenceManager;
 import org.eclipse.che.api.user.server.UserManager;
 import org.eclipse.che.api.workspace.server.spi.InfrastructureException;
-import org.eclipse.che.commons.annotation.Nullable;
 import org.eclipse.che.workspace.infrastructure.kubernetes.CheServerKubernetesClientFactory;
 import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory;
 import org.eclipse.che.workspace.infrastructure.kubernetes.util.KubernetesSharedPool;
@@ -113,7 +113,7 @@ private void ensureImplicitRolesWithBindings(Client k8sClient) {
         buildRole(
             EXEC_ROLE_NAME,
             singletonList("pods/exec"),
-            null,
+            emptyList(),
             singletonList(""),
             singletonList("create")),
         serviceAccountName + "-exec");
@@ -124,7 +124,7 @@ private void ensureImplicitRolesWithBindings(Client k8sClient) {
         buildRole(
             VIEW_ROLE_NAME,
             Arrays.asList("pods", "services"),
-            null,
+            emptyList(),
             singletonList(""),
             singletonList("list")),
         serviceAccountName + "-view");
@@ -135,7 +135,7 @@ private void ensureImplicitRolesWithBindings(Client k8sClient) {
         buildRole(
             METRICS_ROLE_NAME,
             Arrays.asList("pods", "nodes"),
-            null,
+            emptyList(),
             singletonList("metrics.k8s.io"),
             Arrays.asList("list", "get", "watch")),
         serviceAccountName + "-metrics");
@@ -194,15 +194,14 @@ private void ensureExplicitClusterRoleBindings(Client k8sClient) {
    *
    * @param name the name of the role
    * @param resources the resources the role grants access to
-   * @param resourceNames specific resource names witch the role grants access to. {@code null} if
-   *     empty.
+   * @param resourceNames specific resource names witch the role grants access to.
    * @param verbs the verbs the role allows
    * @return the role object for the given type of Client
    */
   protected abstract R buildRole(
       String name,
       List<String> resources,
-      @Nullable List<String> resourceNames,
+      List<String> resourceNames,
       List<String> apiGroups,
       List<String> verbs);
 

diff --git a/...va/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java b/...va/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java
@@ -13,7 +13,6 @@
 
 import static io.fabric8.kubernetes.api.model.DeletionPropagation.BACKGROUND;
 import static java.lang.String.format;
-import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.AbstractWorkspaceServiceAccount.CREDENTIALS_SECRET_NAME;
 
 import com.google.common.annotations.VisibleForTesting;
 import io.fabric8.kubernetes.api.model.ConfigMap;
@@ -22,7 +21,6 @@
 import io.fabric8.kubernetes.api.model.PersistentVolumeClaim;
 import io.fabric8.kubernetes.api.model.Pod;
 import io.fabric8.kubernetes.api.model.Secret;
-import io.fabric8.kubernetes.api.model.SecretBuilder;
 import io.fabric8.kubernetes.api.model.Service;
 import io.fabric8.kubernetes.api.model.ServiceAccount;
 import io.fabric8.kubernetes.api.model.extensions.Ingress;
@@ -128,8 +126,7 @@ public KubernetesNamespace(
   /**
    * Prepare namespace for using.
    *
-   * <p>Preparing includes creating if needed and waiting for default service account, then it
-   * creates a secret for storing credentials.
+   * <p>Preparing includes creating if needed and waiting for default service account.
    *
    * <p>The method will try to label the namespace with provided `labels`. It does not matter if the
    * namespace already exists or we create new one. If update labels operation fail due to lack of
@@ -145,29 +142,26 @@ public KubernetesNamespace(
    * @param annotations annotations that should be set to the namespace
    * @throws InfrastructureException if any exception occurs during namespace preparation or if the
    *     namespace doesn't exist and {@code canCreate} is {@code false}.
+   * @return {@code true} if the namespace didn't exist and namespace creation was invoked, {@code
+   *     false} if the namespace was already created in the previous calls.
    */
-  void prepare(boolean canCreate, Map<String, String> labels, Map<String, String> annotations)
+  boolean prepare(boolean canCreate, Map<String, String> labels, Map<String, String> annotations)
       throws InfrastructureException {
     KubernetesClient client = clientFactory.create(workspaceId);
     Namespace namespace = get(name, client);
+    boolean needToCreateNewNamespace = false;
 
     if (namespace == null) {
+      needToCreateNewNamespace = true;
       if (!canCreate) {
         throw new InfrastructureException(
             format("Creating the namespace '%s' is not allowed, yet it was not found.", name));
       }
       namespace = create(name, client);
-      Secret secret =
-          new SecretBuilder()
-              .withType("opaque")
-              .withNewMetadata()
-              .withName(CREDENTIALS_SECRET_NAME)
-              .endMetadata()
-              .build();
-      client.secrets().inNamespace(name).create(secret);
     }
     label(namespace, labels);
     annotate(namespace, annotations);
+    return needToCreateNewNamespace;
   }
 
   /**

diff --git a/...eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java b/...eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java
@@ -20,6 +20,7 @@
 import static org.eclipse.che.api.workspace.shared.Constants.WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE;
 import static org.eclipse.che.workspace.infrastructure.kubernetes.api.shared.KubernetesNamespaceMeta.DEFAULT_ATTRIBUTE;
 import static org.eclipse.che.workspace.infrastructure.kubernetes.api.shared.KubernetesNamespaceMeta.PHASE_ATTRIBUTE;
+import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.AbstractWorkspaceServiceAccount.CREDENTIALS_SECRET_NAME;
 import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.NamespaceNameValidator.METADATA_NAME_MAX_LENGTH;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -30,6 +31,8 @@
 import com.google.inject.Singleton;
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.api.model.Namespace;
+import io.fabric8.kubernetes.api.model.Secret;
+import io.fabric8.kubernetes.api.model.SecretBuilder;
 import io.fabric8.kubernetes.client.KubernetesClientException;
 import java.util.Collections;
 import java.util.HashMap;
@@ -336,10 +339,25 @@ public KubernetesNamespace getOrCreate(RuntimeIdentity identity) throws Infrastr
     Map<String, String> namespaceAnnotationsEvaluated =
         evaluateAnnotationPlaceholders(resolutionCtx);
 
-    namespace.prepare(
-        canCreateNamespace(identity),
-        labelNamespaces ? namespaceLabels : emptyMap(),
-        annotateNamespaces ? namespaceAnnotationsEvaluated : emptyMap());
+    boolean newNamespace =
+        namespace.prepare(
+            canCreateNamespace(identity),
+            labelNamespaces ? namespaceLabels : emptyMap(),
+            annotateNamespaces ? namespaceAnnotationsEvaluated : emptyMap());
+    if (newNamespace) {
+      Secret secret =
+          new SecretBuilder()
+              .withType("opaque")
+              .withNewMetadata()
+              .withName(CREDENTIALS_SECRET_NAME)
+              .endMetadata()
+              .build();
+      clientFactory
+          .create()
+          .secrets()
+          .inNamespace(identity.getInfrastructureNamespace())
+          .create(secret);
+    }
 
     if (!isNullOrEmpty(serviceAccountName)) {
       KubernetesWorkspaceServiceAccount workspaceServiceAccount =

diff --git a/.../che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccount.java b/.../che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccount.java
@@ -11,8 +11,6 @@
  */
 package org.eclipse.che.workspace.infrastructure.kubernetes.namespace;
 
-import static java.util.Collections.emptyList;
-
 import io.fabric8.kubernetes.api.model.rbac.PolicyRuleBuilder;
 import io.fabric8.kubernetes.api.model.rbac.Role;
 import io.fabric8.kubernetes.api.model.rbac.RoleBinding;
@@ -22,7 +20,6 @@
 import io.fabric8.kubernetes.client.KubernetesClient;
 import java.util.List;
 import java.util.Set;
-import org.eclipse.che.commons.annotation.Nullable;
 import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory;
 
 /**
@@ -55,7 +52,7 @@ public KubernetesWorkspaceServiceAccount(
   protected Role buildRole(
       String name,
       List<String> resources,
-      @Nullable List<String> resourceNames,
+      List<String> resourceNames,
       List<String> apiGroups,
       List<String> verbs) {
     return new RoleBuilder()
@@ -65,7 +62,7 @@ protected Role buildRole(
         .withRules(
             new PolicyRuleBuilder()
                 .withResources(resources)
-                .withResourceNames(resourceNames != null ? resourceNames : emptyList())
+                .withResourceNames(resourceNames)
                 .withApiGroups(apiGroups)
                 .withVerbs(verbs)
                 .build())

diff --git a/...pse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactoryTest.java b/...pse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactoryTest.java
@@ -20,6 +20,7 @@
 import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.AbstractWorkspaceServiceAccount.SECRETS_ROLE_NAME;
 import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory.NAMESPACE_TEMPLATE_ATTRIBUTE;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
 import static org.mockito.ArgumentMatchers.anyMap;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
@@ -43,6 +44,7 @@
 import io.fabric8.kubernetes.api.model.Namespace;
 import io.fabric8.kubernetes.api.model.NamespaceBuilder;
 import io.fabric8.kubernetes.api.model.NamespaceList;
+import io.fabric8.kubernetes.api.model.Secret;
 import io.fabric8.kubernetes.api.model.ServiceAccountList;
 import io.fabric8.kubernetes.api.model.Status;
 import io.fabric8.kubernetes.api.model.rbac.ClusterRoleBuilder;
@@ -53,6 +55,7 @@
 import io.fabric8.kubernetes.client.KubernetesClient;
 import io.fabric8.kubernetes.client.KubernetesClientException;
 import io.fabric8.kubernetes.client.dsl.FilterWatchListDeletable;
+import io.fabric8.kubernetes.client.dsl.MixedOperation;
 import io.fabric8.kubernetes.client.dsl.NonNamespaceOperation;
 import io.fabric8.kubernetes.client.dsl.Resource;
 import io.fabric8.kubernetes.client.server.mock.KubernetesServer;
@@ -121,8 +124,7 @@ public class KubernetesNamespaceFactoryTest {
   @Mock private PreferenceManager preferenceManager;
   @Mock Appender mockedAppender;
 
-  @Mock
-  private NonNamespaceOperation<Namespace, NamespaceList, Resource<Namespace>> namespaceOperation;
+  @Mock private NonNamespaceOperation namespaceOperation;
 
   @Mock private Resource<Namespace> namespaceResource;
 
@@ -457,6 +459,45 @@ public void shouldReturnDefaultNamespaceWhenItDoesNotExistAndUserDefinedIsNotAll
             .get(PHASE_ATTRIBUTE)); // no phase - means such namespace does not exist
   }
 
+  @Test
+  public void shouldCreateCredentialsSecretWhenNamespaceDoesNotExist() throws Exception {
+    // given
+    namespaceFactory =
+        spy(
+            new KubernetesNamespaceFactory(
+                "",
+                "",
+                "<username>-che",
+                true,
+                true,
+                true,
+                NAMESPACE_LABELS,
+                NAMESPACE_ANNOTATIONS,
+                clientFactory,
+                cheClientFactory,
+                userManager,
+                preferenceManager,
+                pool));
+    KubernetesNamespace toReturnNamespace = mock(KubernetesNamespace.class);
+    when(toReturnNamespace.prepare(anyBoolean(), anyMap(), anyMap())).thenReturn(true);
+    doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespaceAccess(any(), any());
+    MixedOperation mixedOperation = mock(MixedOperation.class);
+    lenient().when(k8sClient.secrets()).thenReturn(mixedOperation);
+    lenient().when(mixedOperation.inNamespace(anyString())).thenReturn(namespaceOperation);
+
+    // when
+    RuntimeIdentity identity =
+        new RuntimeIdentityImpl("workspace123", null, USER_ID, "workspace123");
+    namespaceFactory.getOrCreate(identity);
+
+    // then
+    ArgumentCaptor<Secret> secretsCaptor = ArgumentCaptor.forClass(Secret.class);
+    verify(namespaceOperation).create(secretsCaptor.capture());
+    Secret secret = secretsCaptor.getValue();
+    Assert.assertEquals(secret.getMetadata().getName(), CREDENTIALS_SECRET_NAME);
+    Assert.assertEquals(secret.getType(), "opaque");
+  }
+
   @Test(
       expectedExceptions = InfrastructureException.class,
       expectedExceptionsMessageRegExp =

diff --git a/...rg/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java b/...rg/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java
@@ -13,7 +13,6 @@
 
 import static io.fabric8.kubernetes.api.model.DeletionPropagation.BACKGROUND;
 import static java.util.Collections.emptyMap;
-import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.AbstractWorkspaceServiceAccount.CREDENTIALS_SECRET_NAME;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
@@ -23,7 +22,6 @@
 import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static org.testng.Assert.assertEquals;
@@ -32,7 +30,6 @@
 
 import io.fabric8.kubernetes.api.model.Namespace;
 import io.fabric8.kubernetes.api.model.NamespaceBuilder;
-import io.fabric8.kubernetes.api.model.Secret;
 import io.fabric8.kubernetes.api.model.ServiceAccount;
 import io.fabric8.kubernetes.api.model.Status;
 import io.fabric8.kubernetes.client.KubernetesClient;
@@ -91,8 +88,8 @@ public void setUp() throws Exception {
     lenient().doReturn(namespaceOperation).when(kubernetesClient).namespaces();
 
     final MixedOperation mixedOperation = mock(MixedOperation.class);
+    final NonNamespaceOperation namespaceOperation = mock(NonNamespaceOperation.class);
     lenient().doReturn(mixedOperation).when(kubernetesClient).serviceAccounts();
-    lenient().doReturn(mixedOperation).when(kubernetesClient).secrets();
     lenient().when(mixedOperation.inNamespace(anyString())).thenReturn(namespaceOperation);
     lenient().when(namespaceOperation.withName(anyString())).thenReturn(serviceAccountResource);
     lenient().when(serviceAccountResource.get()).thenReturn(mock(ServiceAccount.class));
@@ -123,7 +120,6 @@ public void testKubernetesNamespacePreparingWhenNamespaceExists() throws Excepti
 
     // then
     verify(namespaceOperation, never()).create(any(Namespace.class));
-    verify(kubernetesClient, never()).secrets();
   }
 
   @Test
@@ -139,14 +135,9 @@ public void testKubernetesNamespacePreparingCreationWhenNamespaceDoesNotExist()
     namespace.prepare(true, Map.of(), Map.of());
 
     // then
-    ArgumentCaptor<Namespace> namespaceCaptor = ArgumentCaptor.forClass(Namespace.class);
-    ArgumentCaptor<Secret> secretsCaptor = ArgumentCaptor.forClass(Secret.class);
-    verify(namespaceOperation, times(2)).create(namespaceCaptor.capture());
-    verify(namespaceOperation, times(2)).create(secretsCaptor.capture());
-    Assert.assertEquals(namespaceCaptor.getAllValues().get(0).getMetadata().getName(), NAMESPACE);
-    Secret secret = secretsCaptor.getAllValues().get(1);
-    Assert.assertEquals(secret.getMetadata().getName(), CREDENTIALS_SECRET_NAME);
-    Assert.assertEquals(secret.getType(), "opaque");
+    ArgumentCaptor<Namespace> captor = ArgumentCaptor.forClass(Namespace.class);
+    verify(namespaceOperation).create(captor.capture());
+    Assert.assertEquals(captor.getValue().getMetadata().getName(), NAMESPACE);
   }
 
   @Test(expectedExceptions = InfrastructureException.class)

diff --git a/...ipse/che/workspace/infrastructure/openshift/project/OpenShiftWorkspaceServiceAccount.java b/...ipse/che/workspace/infrastructure/openshift/project/OpenShiftWorkspaceServiceAccount.java
@@ -11,8 +11,6 @@
  */
 package org.eclipse.che.workspace.infrastructure.openshift.project;
 
-import static java.util.Collections.emptyList;
-
 import io.fabric8.kubernetes.api.model.ObjectReferenceBuilder;
 import io.fabric8.openshift.api.model.PolicyRuleBuilder;
 import io.fabric8.openshift.api.model.Role;
@@ -23,7 +21,6 @@
 import io.fabric8.openshift.client.OpenShiftClient;
 import java.util.List;
 import java.util.Set;
-import org.eclipse.che.commons.annotation.Nullable;
 import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.AbstractWorkspaceServiceAccount;
 import org.eclipse.che.workspace.infrastructure.openshift.OpenShiftClientFactory;
 
@@ -60,7 +57,7 @@ class OpenShiftWorkspaceServiceAccount
   protected Role buildRole(
       String name,
       List<String> resources,
-      @Nullable List<String> resourceNames,
+      List<String> resourceNames,
       List<String> apiGroups,
       List<String> verbs) {
     return new RoleBuilder()
@@ -70,7 +67,7 @@ protected Role buildRole(
         .withRules(
             new PolicyRuleBuilder()
                 .withResources(resources)
-                .withResourceNames(resourceNames != null ? resourceNames : emptyList())
+                .withResourceNames(resourceNames)
                 .withApiGroups(apiGroups)
                 .withVerbs(verbs)
                 .build())