Fixes #16612 - Don't use the label to designate managed namespaces. I…

…nstead rely solely on the heuristics. (#16632) Signed-off-by: Lukas Krejci <[email protected]>
eclipse-che · Apr 20, 2020 · 06a34e3 · 06a34e3
1 parent af4465f
commit 06a34e3
Show file tree

Hide file tree

Showing 8 changed files with 78 additions and 414 deletions.
diff --git a/...va/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java b/...va/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java
@@ -12,7 +12,6 @@
 package org.eclipse.che.workspace.infrastructure.kubernetes.namespace;
 
 import static java.lang.String.format;
-import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesObjectUtil.isLabeled;
 
 import com.google.common.annotations.VisibleForTesting;
 import io.fabric8.kubernetes.api.model.ConfigMap;
@@ -114,14 +113,12 @@ public KubernetesNamespace(
    *
    * <p>Preparing includes creating if needed and waiting for default service account.
    *
-   * @param markManaged mark the namespace as managed by Che. Also applies for already existing
-   *     namespaces.
    * @param canCreate defines what to do when the namespace is not found. The namespace is created
    *     when {@code true}, otherwise an exception is thrown.
    * @throws InfrastructureException if any exception occurs during namespace preparation or if the
    *     namespace doesn't exist and {@code canCreate} is {@code false}.
    */
-  void prepare(boolean markManaged, boolean canCreate) throws InfrastructureException {
+  void prepare(boolean canCreate) throws InfrastructureException {
     KubernetesClient client = clientFactory.create(workspaceId);
     Namespace namespace = get(name, client);
 
@@ -130,34 +127,19 @@ void prepare(boolean markManaged, boolean canCreate) throws InfrastructureExcept
         throw new InfrastructureException(
             format("Creating the namespace '%s' is not allowed, yet it was not found.", name));
       }
-      namespace = create(name, client);
-    }
-
-    if (markManaged && !isLabeled(namespace, MANAGED_NAMESPACE_LABEL, "true")) {
-      // provision managed label is marking is requested but label is missing
-      KubernetesObjectUtil.putLabel(namespace, MANAGED_NAMESPACE_LABEL, "true");
-      update(namespace, client);
+      create(name, client);
     }
   }
 
   /**
    * Deletes the namespace. Deleting a non-existent namespace is not an error as is not an attempt
-   * to delete a namespace that is already being deleted. If the namespace is not marked as managed,
-   * it is silently not deleted.
+   * to delete a namespace that is already being deleted.
    *
    * @throws InfrastructureException if any unexpected exception occurs during namespace deletion
    */
-  void deleteIfManaged() throws InfrastructureException {
+  void delete() throws InfrastructureException {
     KubernetesClient client = clientFactory.create(workspaceId);
 
-    if (!isNamespaceManaged(client)) {
-      LOG.info(
-          "Namespace {} for workspace {} is not marked as managed. Ignoring the delete request.",
-          name,
-          workspaceId);
-      return;
-    }
-
     try {
       delete(name, client);
     } catch (KubernetesClientException e) {
@@ -173,34 +155,6 @@ void deleteIfManaged() throws InfrastructureException {
     }
   }
 
-  private boolean isNamespaceManaged(KubernetesClient client) throws InfrastructureException {
-    try {
-      Namespace namespace = client.namespaces().withName(name).get();
-      return namespace.getMetadata().getLabels() != null
-          && "true".equals(namespace.getMetadata().getLabels().get(MANAGED_NAMESPACE_LABEL));
-    } catch (KubernetesClientException e) {
-      if (e.getCode() == 403) {
-        throw new InfrastructureException(
-            format(
-                "Could not access the namespace %s when trying to determine if it is managed "
-                    + "for workspace %s",
-                name, workspaceId),
-            e);
-      } else if (e.getCode() == 404) {
-        // we don't want to block whatever work the caller is doing on the namespace. The caller
-        // will fail anyway if the namespace doesn't exist.
-        return true;
-      }
-
-      throw new InternalInfrastructureException(
-          format(
-              "Failed to determine whether the namespace"
-                  + " %s is managed. Kubernetes client said: %s",
-              getName(), e.getMessage()),
-          e);
-    }
-  }
-
   /** Returns namespace name */
   public String getName() {
     return name;
@@ -277,20 +231,17 @@ protected void doRemove(RemoveOperation... operations) throws InfrastructureExce
     }
   }
 
-  private Namespace create(String namespaceName, KubernetesClient client)
+  private void create(String namespaceName, KubernetesClient client)
       throws InfrastructureException {
     try {
-      Namespace ns =
-          client
-              .namespaces()
-              .createNew()
-              .withNewMetadata()
-              .withName(namespaceName)
-              .endMetadata()
-              .done();
+      client
+          .namespaces()
+          .createNew()
+          .withNewMetadata()
+          .withName(namespaceName)
+          .endMetadata()
+          .done();
       waitDefaultServiceAccount(namespaceName, client);
-
-      return ns;
     } catch (KubernetesClientException e) {
       if (e.getCode() == 403) {
         LOG.error(
@@ -301,19 +252,6 @@ private Namespace create(String namespaceName, KubernetesClient client)
     }
   }
 
-  private void update(Namespace namespace, KubernetesClient client) throws InfrastructureException {
-    try {
-      client.namespaces().createOrReplace(namespace);
-    } catch (KubernetesClientException e) {
-      if (e.getCode() == 403) {
-        LOG.error(
-            "Unable to update new Kubernetes namespace due to lack of permissions."
-                + "When using workspace namespace placeholders, service account with lenient permissions (cluster-admin) must be used.");
-      }
-      throw new KubernetesInfrastructureException(e);
-    }
-  }
-
   private void delete(String namespaceName, KubernetesClient client)
       throws InfrastructureException {
     try {

diff --git a/...eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java b/...eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java
@@ -317,24 +317,23 @@ protected boolean canCreateNamespace(RuntimeIdentity identity) throws Infrastruc
   }
 
   /**
-   * Tells the caller whether the namespace that is being prepared, should be marked as managed or
-   * not.
+   * A managed namespace of a workspace is a namespace that is fully controlled by Che. Such
+   * namespaces are deleted when the workspace is deleted.
    *
-   * @param identity the runtime identity of the workspace
-   * @return true if the workspace namespace should be marked managed, false otherwise
+   * @param namespaceName the name of the namespace the workspace is stored in
+   * @param workspace the workspace
+   * @throws InfrastructureException in case of legacy workspaces that don't store their namespace
+   *     in the attributes, this method might fail
    */
-  protected boolean shouldMarkNamespaceManaged(RuntimeIdentity identity) {
-    // when infra namespace contains workspaceId that is generated
-    // it mean that Che Server provides unique namespace for each workspace
-    // and nothing else except workspace should be run there
-    // Che Server also removes such namespace after workspace is removed
-    return identity.getInfrastructureNamespace().contains(identity.getWorkspaceId());
+  protected boolean isWorkspaceNamespaceManaged(String namespaceName, Workspace workspace)
+      throws InfrastructureException {
+    return namespaceName != null && namespaceName.contains(workspace.getId());
   }
 
   public KubernetesNamespace getOrCreate(RuntimeIdentity identity) throws InfrastructureException {
     KubernetesNamespace namespace = get(identity);
 
-    namespace.prepare(shouldMarkNamespaceManaged(identity), canCreateNamespace(identity));
+    namespace.prepare(canCreateNamespace(identity));
 
     if (!isNullOrEmpty(serviceAccountName)) {
       KubernetesWorkspaceServiceAccount workspaceServiceAccount =
@@ -454,7 +453,9 @@ public String evaluateNamespaceName(NamespaceResolutionContext resolutionCtx)
 
   public void deleteIfManaged(Workspace workspace) throws InfrastructureException {
     KubernetesNamespace namespace = get(workspace);
-    namespace.deleteIfManaged();
+    if (isWorkspaceNamespaceManaged(namespace.getName(), workspace)) {
+      namespace.delete();
+    }
   }
 
   private String resolveLegacyNamespaceName(NamespaceResolutionContext resolutionCtx) {

diff --git a/...pse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactoryTest.java b/...pse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactoryTest.java
@@ -269,27 +269,6 @@ public void shouldThrowExceptionWhenFailedToGetNamespaces() throws Exception {
     namespaceFactory.list();
   }
 
-  @Test
-  public void shouldMarkNamespaceManagedIfWorkspaceIdIsUsedInItsName() throws Exception {
-    // given
-    namespaceFactory =
-        spy(
-            new KubernetesNamespaceFactory(
-                "", "", "", "<workspaceid>", false, clientFactory, userManager, pool));
-    KubernetesNamespace toReturnNamespace = mock(KubernetesNamespace.class);
-    doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespaceAccess(any(), any());
-
-    // when
-    RuntimeIdentity identity =
-        new RuntimeIdentityImpl("workspace123", null, USER_ID, "workspace123");
-    KubernetesNamespace namespace = namespaceFactory.getOrCreate(identity);
-
-    // then
-    assertEquals(toReturnNamespace, namespace);
-    verify(namespaceFactory, never()).doCreateServiceAccount(any(), any());
-    verify(toReturnNamespace).prepare(eq(true), eq(true));
-  }
-
   @Test
   public void shouldRequireNamespacePriorExistenceIfDifferentFromDefaultAndUserDefinedIsNotAllowed()
       throws Exception {
@@ -315,34 +294,7 @@ public void shouldRequireNamespacePriorExistenceIfDifferentFromDefaultAndUserDef
     // then
     assertEquals(toReturnNamespace, namespace);
     verify(namespaceFactory, never()).doCreateServiceAccount(any(), any());
-    verify(toReturnNamespace).prepare(eq(false), eq(false));
-  }
-
-  @Test
-  public void
-      shouldHandleUpgradeOfManagedFlagAndRequireNamespacePriorExistenceIfDifferentFromDefaultAndUserDefinedIsNotAllowed()
-          throws Exception {
-    // This is a variation of the above test that checks the same scenario, but additionally checks
-    // that we correctly set the managed flag on the namespace if the namespace name contains
-    // the workspace id (and thus will be automatically deleted after workspace stop).
-
-    // given
-    namespaceFactory =
-        spy(
-            new KubernetesNamespaceFactory(
-                "", "", "", "che", false, clientFactory, userManager, pool));
-    KubernetesNamespace toReturnNamespace = mock(KubernetesNamespace.class);
-    doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespaceAccess(any(), any());
-
-    // when
-    RuntimeIdentity identity =
-        new RuntimeIdentityImpl("workspace123", null, USER_ID, "che-ws-workspace123");
-    KubernetesNamespace namespace = namespaceFactory.getOrCreate(identity);
-
-    // then
-    assertEquals(toReturnNamespace, namespace);
-    verify(namespaceFactory, never()).doCreateServiceAccount(any(), any());
-    verify(toReturnNamespace).prepare(eq(true), eq(false));
+    verify(toReturnNamespace).prepare(eq(false));
   }
 
   @Test