From 220a7e54b11dfa0f4ee5269725ff41dfe07a1f84 Mon Sep 17 00:00:00 2001 From: Michal Vala Date: Fri, 13 Nov 2020 13:06:09 +0100 Subject: [PATCH] make it possible to disable labeling the namespaces (#18340) --- .../webapp/WEB-INF/classes/che/che.properties | 3 ++ .../namespace/KubernetesNamespace.java | 11 ++++++- .../namespace/KubernetesNamespaceFactory.java | 5 ++- .../KubernetesNamespaceFactoryTest.java | 33 +++++++++++++++++++ .../namespace/KubernetesNamespaceTest.java | 30 +++++++++++++++++ .../project/OpenShiftProjectFactory.java | 5 ++- .../project/OpenShiftProjectFactoryTest.java | 19 +++++++++++ 7 files changed, 103 insertions(+), 3 deletions(-) diff --git a/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties b/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties index 1c35954816..6b609f08fd 100644 --- a/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties +++ b/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties @@ -260,6 +260,9 @@ che.infra.kubernetes.namespace.creation_allowed=true # Is used by OpenShift infra as well to specify Project che.infra.kubernetes.namespace.default=-che +# Defines whether che-server should try to label the workspace namespaces. +che.infra.kubernetes.namespace.label=true + # List of labels to find Namespaces/Projects that are used for Che Workspaces. # They are used to: # - find prepared Namespaces/Projects for users in combination with `che.infra.kubernetes.namespace.annotations`. diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java index d6cf048759..9e56d18a3d 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java @@ -163,6 +163,9 @@ public class KubernetesNamespace { */ protected void label(Namespace namespace, Map ensureLabels) throws InfrastructureException { + if (ensureLabels.isEmpty()) { + return; + } Map currentLabels = namespace.getMetadata().getLabels(); Map newLabels = currentLabels != null ? new HashMap<>(currentLabels) : new HashMap<>(); @@ -187,7 +190,13 @@ public class KubernetesNamespace { .build()); } catch (KubernetesClientException kce) { if (kce.getCode() == 403) { - LOG.debug("Can't label the namespace due to lack of permissions ¯\\_(ツ)_/¯"); + LOG.warn( + "Can't label the namespace due to lack of permissions. Grant cluster-wide permissions " + + "to `get` and `update` the `namespaces` to the `che` service account " + + "(Che operator might have already prepared a cluster role called " + + "`che-namespace-editor` for this, depending on its configuration). " + + "Alternatively, consider disabling the feature by setting " + + "`che.infra.kubernetes.namepsace.label` to `false`."); return; } throw new InfrastructureException(kce); diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java index 353dd9c53f..fe25996aee 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java @@ -93,6 +93,7 @@ public class KubernetesNamespaceFactory { private final String defaultNamespaceName; private final boolean allowUserDefinedNamespaces; + protected final boolean labelNamespaces; protected final Map namespaceLabels; protected final Map namespaceAnnotations; @@ -115,6 +116,7 @@ public class KubernetesNamespaceFactory { @Named("che.infra.kubernetes.namespace.allow_user_defined") boolean allowUserDefinedNamespaces, @Named("che.infra.kubernetes.namespace.creation_allowed") boolean namespaceCreationAllowed, + @Named("che.infra.kubernetes.namespace.label") boolean labelNamespaces, @Named("che.infra.kubernetes.namespace.labels") String namespaceLabels, @Named("che.infra.kubernetes.namespace.annotations") String namespaceAnnotations, KubernetesClientFactory clientFactory, @@ -133,6 +135,7 @@ public class KubernetesNamespaceFactory { this.allowUserDefinedNamespaces = allowUserDefinedNamespaces; this.preferenceManager = preferenceManager; this.sharedPool = sharedPool; + this.labelNamespaces = labelNamespaces; //noinspection UnstableApiUsage Splitter.MapSplitter csvMapSplitter = Splitter.on(",").withKeyValueSeparator("="); @@ -391,7 +394,7 @@ public class KubernetesNamespaceFactory { public KubernetesNamespace getOrCreate(RuntimeIdentity identity) throws InfrastructureException { KubernetesNamespace namespace = get(identity); - namespace.prepare(canCreateNamespace(identity), namespaceLabels); + namespace.prepare(canCreateNamespace(identity), labelNamespaces ? namespaceLabels : emptyMap()); if (!isNullOrEmpty(serviceAccountName)) { KubernetesWorkspaceServiceAccount workspaceServiceAccount = diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactoryTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactoryTest.java index c145f76150..40948045af 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactoryTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactoryTest.java @@ -163,6 +163,7 @@ public class KubernetesNamespaceFactoryTest { "defaultNs", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -186,6 +187,7 @@ public class KubernetesNamespaceFactoryTest { "defaultNs", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -214,6 +216,7 @@ public class KubernetesNamespaceFactoryTest { "defaultNs", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -240,6 +243,7 @@ public class KubernetesNamespaceFactoryTest { "defaultNs", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -263,6 +267,7 @@ public class KubernetesNamespaceFactoryTest { null, false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -314,6 +319,7 @@ public class KubernetesNamespaceFactoryTest { "che-default", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -358,6 +364,7 @@ public class KubernetesNamespaceFactoryTest { "che-default", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -389,6 +396,7 @@ public class KubernetesNamespaceFactoryTest { "che-default", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -424,6 +432,7 @@ public class KubernetesNamespaceFactoryTest { "che-default", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -453,6 +462,7 @@ public class KubernetesNamespaceFactoryTest { "che-default", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -485,6 +495,7 @@ public class KubernetesNamespaceFactoryTest { "che", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -512,6 +523,7 @@ public class KubernetesNamespaceFactoryTest { "default", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -548,6 +560,7 @@ public class KubernetesNamespaceFactoryTest { "default", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -585,6 +598,7 @@ public class KubernetesNamespaceFactoryTest { "default_ns", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -616,6 +630,7 @@ public class KubernetesNamespaceFactoryTest { "new-default", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -649,6 +664,7 @@ public class KubernetesNamespaceFactoryTest { "new-default", true, false, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -683,6 +699,7 @@ public class KubernetesNamespaceFactoryTest { "", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -721,6 +738,7 @@ public class KubernetesNamespaceFactoryTest { "", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -790,6 +808,7 @@ public class KubernetesNamespaceFactoryTest { "", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -847,6 +866,7 @@ public class KubernetesNamespaceFactoryTest { "che-", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -867,6 +887,7 @@ public class KubernetesNamespaceFactoryTest { "che-", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -891,6 +912,7 @@ public class KubernetesNamespaceFactoryTest { "che-", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -919,6 +941,7 @@ public class KubernetesNamespaceFactoryTest { "che-", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -950,6 +973,7 @@ public class KubernetesNamespaceFactoryTest { "che--", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -982,6 +1006,7 @@ public class KubernetesNamespaceFactoryTest { "che--", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -1014,6 +1039,7 @@ public class KubernetesNamespaceFactoryTest { "che--", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -1046,6 +1072,7 @@ public class KubernetesNamespaceFactoryTest { "che-", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -1074,6 +1101,7 @@ public class KubernetesNamespaceFactoryTest { "che-", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -1104,6 +1132,7 @@ public class KubernetesNamespaceFactoryTest { "che-", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -1156,6 +1185,7 @@ public class KubernetesNamespaceFactoryTest { "defaultNs", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -1194,6 +1224,7 @@ public class KubernetesNamespaceFactoryTest { "defaultNs", false, true, + true, "try_placeholder_here=", NAMESPACE_ANNOTATIONS, clientFactory, @@ -1217,6 +1248,7 @@ public class KubernetesNamespaceFactoryTest { "che-", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -1237,6 +1269,7 @@ public class KubernetesNamespaceFactoryTest { "che-", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java index 4d52543cb2..e08b388ee4 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java @@ -41,6 +41,7 @@ import io.fabric8.kubernetes.client.Watcher.Action; import io.fabric8.kubernetes.client.dsl.MixedOperation; import io.fabric8.kubernetes.client.dsl.NonNamespaceOperation; import io.fabric8.kubernetes.client.dsl.Resource; +import java.util.Collections; import java.util.Map; import java.util.concurrent.Executor; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; @@ -367,6 +368,35 @@ public class KubernetesNamespaceTest { verify(nonNamespaceOperation, never()).createOrReplace(any()); } + @Test + public void testDontTryToLabelNamespaceIfNoNamespacesProvided() throws InfrastructureException { + // given + Map existingLabels = Map.of("some", "labels"); + Namespace namespace = prepareNamespace(NAMESPACE); + namespace.getMetadata().setLabels(existingLabels); + KubernetesNamespace kubernetesNamespace = + new KubernetesNamespace(clientFactory, cheClientFactory, executor, NAMESPACE, WORKSPACE_ID); + + KubernetesClient cheKubeClient = mock(KubernetesClient.class); + lenient().doReturn(cheKubeClient).when(cheClientFactory).create(); + + NonNamespaceOperation nonNamespaceOperation = mock(NonNamespaceOperation.class); + lenient().doReturn(nonNamespaceOperation).when(cheKubeClient).namespaces(); + + lenient() + .doAnswer(a -> a.getArgument(0)) + .when(nonNamespaceOperation) + .createOrReplace(any(Namespace.class)); + + // when + kubernetesNamespace.prepare(true, Collections.emptyMap()); + + // then + assertTrue( + namespace.getMetadata().getLabels().entrySet().containsAll(existingLabels.entrySet())); + verify(nonNamespaceOperation, never()).createOrReplace(any()); + } + @Test public void testDoNotFailWhenNoPermissionsToUpdateNamespace() throws InfrastructureException { // given diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectFactory.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectFactory.java index 22315d61d4..c0b990b68a 100644 --- a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectFactory.java +++ b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectFactory.java @@ -14,6 +14,7 @@ package org.eclipse.che.workspace.infrastructure.openshift.project; import static com.google.common.base.Strings.isNullOrEmpty; import static java.lang.String.format; import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; import static org.eclipse.che.workspace.infrastructure.kubernetes.api.shared.KubernetesNamespaceMeta.PHASE_ATTRIBUTE; import com.google.common.annotations.VisibleForTesting; @@ -71,6 +72,7 @@ public class OpenShiftProjectFactory extends KubernetesNamespaceFactory { @Named("che.infra.kubernetes.namespace.allow_user_defined") boolean allowUserDefinedNamespaces, @Named("che.infra.kubernetes.namespace.creation_allowed") boolean namespaceCreationAllowed, + @Named("che.infra.kubernetes.namespace.label") boolean labelProjects, @Named("che.infra.kubernetes.namespace.labels") String projectLabels, @Named("che.infra.kubernetes.namespace.annotations") String projectAnnotations, OpenShiftClientFactory clientFactory, @@ -89,6 +91,7 @@ public class OpenShiftProjectFactory extends KubernetesNamespaceFactory { defaultNamespaceName, allowUserDefinedNamespaces, namespaceCreationAllowed, + labelProjects, projectLabels, projectAnnotations, clientFactory, @@ -111,7 +114,7 @@ public class OpenShiftProjectFactory extends KubernetesNamespaceFactory { public OpenShiftProject getOrCreate(RuntimeIdentity identity) throws InfrastructureException { OpenShiftProject osProject = get(identity); - osProject.prepare(canCreateNamespace(identity), namespaceLabels); + osProject.prepare(canCreateNamespace(identity), labelNamespaces ? namespaceLabels : emptyMap()); if (!isNullOrEmpty(getServiceAccountName())) { OpenShiftWorkspaceServiceAccount osWorkspaceServiceAccount = diff --git a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectFactoryTest.java b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectFactoryTest.java index ab6b3a668a..59c59e80d4 100644 --- a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectFactoryTest.java +++ b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectFactoryTest.java @@ -153,6 +153,7 @@ public class OpenShiftProjectFactoryTest { "defaultNs", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -179,6 +180,7 @@ public class OpenShiftProjectFactoryTest { "defaultNs", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -208,6 +210,7 @@ public class OpenShiftProjectFactoryTest { "defaultNs", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -236,6 +239,7 @@ public class OpenShiftProjectFactoryTest { null, false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -273,6 +277,7 @@ public class OpenShiftProjectFactoryTest { "che-default", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -311,6 +316,7 @@ public class OpenShiftProjectFactoryTest { "che-default", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -345,6 +351,7 @@ public class OpenShiftProjectFactoryTest { "che-default", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -389,6 +396,7 @@ public class OpenShiftProjectFactoryTest { "che-default", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -427,6 +435,7 @@ public class OpenShiftProjectFactoryTest { "che-default", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -465,6 +474,7 @@ public class OpenShiftProjectFactoryTest { "che-default", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -495,6 +505,7 @@ public class OpenShiftProjectFactoryTest { "default", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -541,6 +552,7 @@ public class OpenShiftProjectFactoryTest { "default", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -582,6 +594,7 @@ public class OpenShiftProjectFactoryTest { "default-ns", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -615,6 +628,7 @@ public class OpenShiftProjectFactoryTest { "new-default", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -652,6 +666,7 @@ public class OpenShiftProjectFactoryTest { "", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -691,6 +706,7 @@ public class OpenShiftProjectFactoryTest { "", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -732,6 +748,7 @@ public class OpenShiftProjectFactoryTest { "", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -785,6 +802,7 @@ public class OpenShiftProjectFactoryTest { "defaultNs", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -819,6 +837,7 @@ public class OpenShiftProjectFactoryTest { "che-default", false, true, + true, "try_placeholder_here=", NAMESPACE_ANNOTATIONS, clientFactory,