From 7bb2641d151a759e1797c4fe706af808b1d751a8 Mon Sep 17 00:00:00 2001 From: disaster37 Date: Tue, 3 Aug 2021 11:31:36 +0200 Subject: [PATCH] feat: Allow to add annotations when che server create namspace like labels (#57) * Allow to add annotations when che server create namspace like labels Signed-off-by: disaster37 * Rename che.infra.kubernetes.namespace.annotate to che.infra.kubernetes.namespace.annotation Signed-off-by: disaster37 * Remove quick fix on devfile (need dedicated PR) Signed-off-by: disaster37 * Replace placeHolders on namespace annotations Signed-off-by: disaster37 * Add tests to valide placeholders on annotation. Rename properties to che.infra.kubernetes.namespace.annotate Signed-off-by: disaster37 * Fix annotations placeholder on openshift project Signed-off-by: disaster37 * Try to fix test with add cleanup context on KeycloakProviderConfigFactoryTest Signed-off-by: disaster37 * Add lenient method on test to avoid unnecessary stubbing error Signed-off-by: disaster37 * clean test by remove unused stubs Signed-off-by: disaster37 --- .../webapp/WEB-INF/classes/che/che.properties | 6 ++ .../namespace/KubernetesNamespace.java | 61 ++++++++++- .../namespace/KubernetesNamespaceFactory.java | 13 ++- .../KubernetesNamespaceFactoryTest.java | 66 +++++++++++- .../namespace/KubernetesNamespaceTest.java | 100 +++++++++++++++--- .../openshift/project/OpenShiftProject.java | 7 +- .../project/OpenShiftProjectFactory.java | 11 +- .../KeycloakProviderConfigFactoryTest.java | 6 ++ .../project/OpenShiftProjectFactoryTest.java | 56 +++++++++- .../project/OpenShiftProjectTest.java | 86 +++++++++++++-- 10 files changed, 383 insertions(+), 29 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 e602f03989..b4cb69a43e 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 @@ -312,6 +312,9 @@ che.infra.kubernetes.namespace.default=-che # Defines whether che-server should try to label the workspace namespaces. che.infra.kubernetes.namespace.label=true +# Defines whether che-server should try to annotate the workspace namespaces. +che.infra.kubernetes.namespace.annotate=true + # List of labels to find {orch-namespace} that are used for {prod-short} Workspaces. # They are used to: # - find prepared {orch-namespace} for users in combination with `che.infra.kubernetes.namespace.annotations`. @@ -323,6 +326,9 @@ che.infra.kubernetes.namespace.labels=app.kubernetes.io/part-of=che.eclipse.org, # {orch-namespace} that matches both `che.infra.kubernetes.namespace.labels` and `che.infra.kubernetes.namespace.annotations` # will be preferentially used for User's workspaces. # It's possible to use `` placeholder to specify the {orch-namespace} to concrete user. +# They are used to: +# - find prepared {orch-namespace} for users in combination with `che.infra.kubernetes.namespace.labels`. +# - actively annotate {orch-namespace} with any workspace. che.infra.kubernetes.namespace.annotations=che.eclipse.org/username= # Defines Kubernetes Service Account name which should be specified to be bound to all workspaces Pods. 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 2d5cff710e..295ca57673 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 @@ -132,13 +132,19 @@ public class KubernetesNamespace { * namespace already exists or we create new one. If update labels operation fail due to lack of * permission, we do not fail completely. * + *

The method will try to annotate the namespace with provided `annotations`. It does not + * matter if the namespace already exists or we create new one. If update annotations operation + * fail due to lack of permission, we do not fail completely. + * * @param canCreate defines what to do when the namespace is not found. The namespace is created * when {@code true}, otherwise an exception is thrown. * @param labels labels that should be set to the namespace + * @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}. */ - void prepare(boolean canCreate, Map labels) throws InfrastructureException { + void prepare(boolean canCreate, Map labels, Map annotations) + throws InfrastructureException { KubernetesClient client = clientFactory.create(workspaceId); Namespace namespace = get(name, client); @@ -150,6 +156,7 @@ public class KubernetesNamespace { namespace = create(name, client); } label(namespace, labels); + annotate(namespace, annotations); } /** @@ -204,6 +211,58 @@ public class KubernetesNamespace { } } + /** + * Applies given `ensureAnnotations` into given `namespace` and update the `namespace` in the + * Kubernetes. + * + *

If we do not have permissions to do so (code=403), this method does not throw any exception. + * + * @param namespace namespace to annotate + * @param ensureAnnotations these annotations should be applied on given `namespace` + * @throws InfrastructureException if something goes wrong with update, except lack of permissions + */ + protected void annotate(Namespace namespace, Map ensureAnnotations) + throws InfrastructureException { + if (ensureAnnotations.isEmpty()) { + return; + } + Map currentAnnotations = namespace.getMetadata().getAnnotations(); + Map newAnnotations = + currentAnnotations != null ? new HashMap<>(currentAnnotations) : new HashMap<>(); + + if (newAnnotations.entrySet().containsAll(ensureAnnotations.entrySet())) { + LOG.debug( + "Nothing to do, namespace [{}] already has all required annotations.", + namespace.getMetadata().getName()); + return; + } + + try { + // update the namespace with new annotations + cheSAClientFactory + .create() + .namespaces() + .createOrReplace( + new NamespaceBuilder(namespace) + .editMetadata() + .addToAnnotations(ensureAnnotations) + .endMetadata() + .build()); + } catch (KubernetesClientException kce) { + if (kce.getCode() == 403) { + LOG.warn( + "Can't annotate 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.annotate` to `false`."); + return; + } + throw new InfrastructureException(kce); + } + } + /** * 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. 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 5ac78d01d9..f787b5c082 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; protected final boolean labelNamespaces; + protected final boolean annotateNamespaces; protected final Map namespaceLabels; protected final Map namespaceAnnotations; @@ -112,6 +113,7 @@ public class KubernetesNamespaceFactory { @Nullable @Named("che.infra.kubernetes.namespace.default") String defaultNamespaceName, @Named("che.infra.kubernetes.namespace.creation_allowed") boolean namespaceCreationAllowed, @Named("che.infra.kubernetes.namespace.label") boolean labelNamespaces, + @Named("che.infra.kubernetes.namespace.annotate") boolean annotateNamespaces, @Named("che.infra.kubernetes.namespace.labels") String namespaceLabels, @Named("che.infra.kubernetes.namespace.annotations") String namespaceAnnotations, KubernetesClientFactory clientFactory, @@ -129,6 +131,7 @@ public class KubernetesNamespaceFactory { this.preferenceManager = preferenceManager; this.sharedPool = sharedPool; this.labelNamespaces = labelNamespaces; + this.annotateNamespaces = annotateNamespaces; //noinspection UnstableApiUsage Splitter.MapSplitter csvMapSplitter = Splitter.on(",").withKeyValueSeparator("="); @@ -327,7 +330,15 @@ public class KubernetesNamespaceFactory { public KubernetesNamespace getOrCreate(RuntimeIdentity identity) throws InfrastructureException { KubernetesNamespace namespace = get(identity); - namespace.prepare(canCreateNamespace(identity), labelNamespaces ? namespaceLabels : emptyMap()); + NamespaceResolutionContext resolutionCtx = + new NamespaceResolutionContext(EnvironmentContext.getCurrent().getSubject()); + Map namespaceAnnotationsEvaluated = + evaluateAnnotationPlaceholders(resolutionCtx); + + namespace.prepare( + canCreateNamespace(identity), + labelNamespaces ? namespaceLabels : emptyMap(), + annotateNamespaces ? namespaceAnnotationsEvaluated : 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 0cd0e49cef..384a70417b 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 @@ -165,6 +165,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -192,6 +193,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -217,6 +219,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -239,6 +242,7 @@ public class KubernetesNamespaceFactoryTest { null, true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -289,6 +293,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -332,6 +337,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -362,6 +368,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -395,6 +402,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -423,6 +431,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -454,6 +463,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -479,6 +489,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -509,6 +520,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -526,7 +538,7 @@ public class KubernetesNamespaceFactoryTest { // then assertEquals(toReturnNamespace, namespace); verify(namespaceFactory, never()).doCreateServiceAccount(any(), any()); - verify(toReturnNamespace).prepare(eq(false), any()); + verify(toReturnNamespace).prepare(eq(false), any(), any()); } @Test @@ -540,6 +552,7 @@ public class KubernetesNamespaceFactoryTest { "-che", false, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -558,7 +571,7 @@ public class KubernetesNamespaceFactoryTest { // then assertEquals(toReturnNamespace, namespace); verify(namespaceFactory, never()).doCreateServiceAccount(any(), any()); - verify(toReturnNamespace).prepare(eq(false), any()); + verify(toReturnNamespace).prepare(eq(false), any(), any()); } @Test @@ -573,6 +586,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -610,6 +624,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -679,6 +694,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -735,6 +751,7 @@ public class KubernetesNamespaceFactoryTest { "che-", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -754,6 +771,7 @@ public class KubernetesNamespaceFactoryTest { "che-", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -777,6 +795,7 @@ public class KubernetesNamespaceFactoryTest { "che-", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -802,6 +821,7 @@ public class KubernetesNamespaceFactoryTest { "che-", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -832,6 +852,7 @@ public class KubernetesNamespaceFactoryTest { "che--", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -863,6 +884,7 @@ public class KubernetesNamespaceFactoryTest { "che--", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -894,6 +916,7 @@ public class KubernetesNamespaceFactoryTest { "che-", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -923,6 +946,7 @@ public class KubernetesNamespaceFactoryTest { "che-", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -973,6 +997,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -1010,6 +1035,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, "try_placeholder_here=", NAMESPACE_ANNOTATIONS, clientFactory, @@ -1023,6 +1049,40 @@ public class KubernetesNamespaceFactoryTest { verify(namespaceOperation).withLabels(Map.of("try_placeholder_here", "")); } + @Test + public void testUsernamePlaceholderInAnnotationsIsEvaluated() throws InfrastructureException { + + // given + namespaceFactory = + spy( + new KubernetesNamespaceFactory( + "", + "", + "-che", + true, + true, + true, + NAMESPACE_LABELS, + "try_placeholder_here=", + clientFactory, + cheClientFactory, + userManager, + preferenceManager, + pool)); + EnvironmentContext.getCurrent().setSubject(new SubjectImpl("jondoe", "123", null, false)); + KubernetesNamespace toReturnNamespace = mock(KubernetesNamespace.class); + doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespaceAccess(any(), any()); + + // when + RuntimeIdentity identity = new RuntimeIdentityImpl("workspace123", null, USER_ID, "old-che"); + KubernetesNamespace namespace = namespaceFactory.getOrCreate(identity); + + // then + assertEquals(toReturnNamespace, namespace); + verify(toReturnNamespace) + .prepare(eq(false), any(), eq(Map.of("try_placeholder_here", "jondoe"))); + } + @Test(dataProvider = "invalidUsernames") public void normalizeTest(String raw, String expected) { namespaceFactory = @@ -1032,6 +1092,7 @@ public class KubernetesNamespaceFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, clientFactory, @@ -1051,6 +1112,7 @@ public class KubernetesNamespaceFactoryTest { "che-", true, 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 fab2d6c85e..964bf2ce3b 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 @@ -12,6 +12,7 @@ package org.eclipse.che.workspace.infrastructure.kubernetes.namespace; import static io.fabric8.kubernetes.api.model.DeletionPropagation.BACKGROUND; +import static java.util.Collections.emptyMap; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; @@ -40,7 +41,6 @@ import io.fabric8.kubernetes.client.WatcherException; 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; @@ -116,7 +116,7 @@ public class KubernetesNamespaceTest { new KubernetesNamespace(clientFactory, cheClientFactory, executor, NAMESPACE, WORKSPACE_ID); // when - namespace.prepare(true, Map.of()); + namespace.prepare(true, Map.of(), Map.of()); // then verify(namespaceOperation, never()).create(any(Namespace.class)); @@ -132,7 +132,7 @@ public class KubernetesNamespaceTest { new KubernetesNamespace(clientFactory, cheClientFactory, executor, NAMESPACE, WORKSPACE_ID); // when - namespace.prepare(true, Map.of()); + namespace.prepare(true, Map.of(), Map.of()); // then ArgumentCaptor captor = ArgumentCaptor.forClass(Namespace.class); @@ -149,7 +149,7 @@ public class KubernetesNamespaceTest { new KubernetesNamespace(clientFactory, cheClientFactory, executor, NAMESPACE, WORKSPACE_ID); // when - namespace.prepare(false, Map.of()); + namespace.prepare(false, Map.of(), Map.of()); // then // exception is thrown @@ -196,7 +196,7 @@ public class KubernetesNamespaceTest { doThrow(KubernetesClientException.class).when(kubernetesClient).serviceAccounts(); new KubernetesNamespace(clientFactory, cheClientFactory, executor, NAMESPACE, WORKSPACE_ID) - .prepare(false, Map.of()); + .prepare(false, Map.of(), Map.of()); } @Test(expectedExceptions = InfrastructureException.class) @@ -207,7 +207,7 @@ public class KubernetesNamespaceTest { when(serviceAccountResource.get()).thenReturn(null); new KubernetesNamespace(clientFactory, cheClientFactory, executor, NAMESPACE, WORKSPACE_ID) - .prepare(false, Map.of()); + .prepare(false, Map.of(), Map.of()); } @Test(expectedExceptions = InfrastructureException.class) @@ -226,7 +226,7 @@ public class KubernetesNamespaceTest { .watch(any()); new KubernetesNamespace(clientFactory, cheClientFactory, executor, NAMESPACE, WORKSPACE_ID) - .prepare(false, Map.of()); + .prepare(false, Map.of(), Map.of()); } @Test @@ -245,7 +245,7 @@ public class KubernetesNamespaceTest { .watch(any()); new KubernetesNamespace(clientFactory, cheClientFactory, executor, NAMESPACE, WORKSPACE_ID) - .prepare(true, Map.of()); + .prepare(true, Map.of(), Map.of()); verify(serviceAccountResource).get(); verify(serviceAccountResource).watch(any()); @@ -318,7 +318,7 @@ public class KubernetesNamespaceTest { Map labels = Map.of("label.with.this", "yes", "and.this", "of courese"); // when - kubernetesNamespace.prepare(true, labels); + kubernetesNamespace.prepare(true, labels, emptyMap()); // then Namespace updatedNamespace = namespaceArgumentCaptor.getValue(); @@ -349,7 +349,7 @@ public class KubernetesNamespaceTest { .createOrReplace(any(Namespace.class)); // when - kubernetesNamespace.prepare(true, labels); + kubernetesNamespace.prepare(true, labels, emptyMap()); // then assertTrue(namespace.getMetadata().getLabels().entrySet().containsAll(labels.entrySet())); @@ -377,7 +377,7 @@ public class KubernetesNamespaceTest { .createOrReplace(any(Namespace.class)); // when - kubernetesNamespace.prepare(true, Collections.emptyMap()); + kubernetesNamespace.prepare(true, emptyMap(), emptyMap()); // then assertTrue( @@ -385,6 +385,80 @@ public class KubernetesNamespaceTest { verify(nonNamespaceOperation, never()).createOrReplace(any()); } + public void testAnnotateNamespace() throws InfrastructureException { + // given + prepareNamespace(NAMESPACE); + KubernetesNamespace kubernetesNamespace = + new KubernetesNamespace(clientFactory, cheClientFactory, executor, NAMESPACE, WORKSPACE_ID); + + KubernetesClient cheKubeClient = mock(KubernetesClient.class); + doReturn(cheKubeClient).when(cheClientFactory).create(); + + NonNamespaceOperation nonNamespaceOperation = mock(NonNamespaceOperation.class); + doReturn(nonNamespaceOperation).when(cheKubeClient).namespaces(); + + ArgumentCaptor namespaceArgumentCaptor = ArgumentCaptor.forClass(Namespace.class); + doAnswer(a -> a.getArgument(0)) + .when(nonNamespaceOperation) + .createOrReplace(namespaceArgumentCaptor.capture()); + + Map annotations = Map.of("annotate.with.this", "yes", "and.this", "of courese"); + + // when + kubernetesNamespace.prepare(true, emptyMap(), annotations); + + // then + Namespace updatedNamespace = namespaceArgumentCaptor.getValue(); + assertTrue( + updatedNamespace + .getMetadata() + .getAnnotations() + .entrySet() + .containsAll(annotations.entrySet())); + assertEquals(updatedNamespace.getMetadata().getName(), NAMESPACE); + } + + @Test + public void testDontTryToAnnotateNamespaceIfAlreadyAnnoted() throws InfrastructureException { + // given + Map annotations = + Map.of("annotation.with.this", "yes", "and.this", "of courese"); + + Namespace namespace = prepareNamespace(NAMESPACE); + namespace.getMetadata().setAnnotations(annotations); + KubernetesNamespace kubernetesNamespace = + new KubernetesNamespace(clientFactory, cheClientFactory, executor, NAMESPACE, WORKSPACE_ID); + + // when + kubernetesNamespace.prepare(true, emptyMap(), annotations); + + // then + assertTrue( + namespace.getMetadata().getAnnotations().entrySet().containsAll(annotations.entrySet())); + } + + @Test + public void testDontTryToAnnotateNamespaceIfNoNamespacesProvided() + throws InfrastructureException { + // given + Map existingAnnotations = Map.of("some", "annotations"); + Namespace namespace = prepareNamespace(NAMESPACE); + namespace.getMetadata().setAnnotations(existingAnnotations); + KubernetesNamespace kubernetesNamespace = + new KubernetesNamespace(clientFactory, cheClientFactory, executor, NAMESPACE, WORKSPACE_ID); + + // when + kubernetesNamespace.prepare(true, emptyMap(), emptyMap()); + + // then + assertTrue( + namespace + .getMetadata() + .getAnnotations() + .entrySet() + .containsAll(existingAnnotations.entrySet())); + } + @Test public void testDoNotFailWhenNoPermissionsToUpdateNamespace() throws InfrastructureException { // given @@ -407,7 +481,7 @@ public class KubernetesNamespaceTest { .createOrReplace(namespaceArgumentCaptor.capture()); // when - kubernetesNamespace.prepare(true, labels); + kubernetesNamespace.prepare(true, labels, emptyMap()); // then Namespace updatedNamespace = namespaceArgumentCaptor.getValue(); @@ -437,7 +511,7 @@ public class KubernetesNamespaceTest { .createOrReplace(any(Namespace.class)); // when - kubernetesNamespace.prepare(true, labels); + kubernetesNamespace.prepare(true, labels, emptyMap()); // then verify(nonNamespaceOperation).createOrReplace(namespace); diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProject.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProject.java index 1859ab0a1f..9d202ade4d 100644 --- a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProject.java +++ b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProject.java @@ -110,7 +110,11 @@ public class OpenShiftProject extends KubernetesNamespace { * @throws InfrastructureException if any exception occurs during project preparation or if the * project doesn't exist and {@code canCreate} is {@code false}. */ - void prepare(boolean canCreate, boolean initWithCheServerSa, Map labels) + void prepare( + boolean canCreate, + boolean initWithCheServerSa, + Map labels, + Map annotations) throws InfrastructureException { String workspaceId = getWorkspaceId(); String projectName = getName(); @@ -153,6 +157,7 @@ public class OpenShiftProject extends KubernetesNamespace { } } label(osClient.namespaces().withName(projectName).get(), labels); + annotate(osClient.namespaces().withName(projectName).get(), annotations); } /** 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 d5b980bd2e..b193ddec63 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 @@ -36,6 +36,7 @@ import org.eclipse.che.api.user.server.UserManager; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.api.workspace.server.spi.NamespaceResolutionContext; import org.eclipse.che.commons.annotation.Nullable; +import org.eclipse.che.commons.env.EnvironmentContext; import org.eclipse.che.workspace.infrastructure.kubernetes.CheServerKubernetesClientFactory; import org.eclipse.che.workspace.infrastructure.kubernetes.api.server.impls.KubernetesNamespaceMetaImpl; import org.eclipse.che.workspace.infrastructure.kubernetes.api.shared.KubernetesNamespaceMeta; @@ -71,6 +72,7 @@ public class OpenShiftProjectFactory extends KubernetesNamespaceFactory { @Nullable @Named("che.infra.kubernetes.namespace.default") String defaultNamespaceName, @Named("che.infra.kubernetes.namespace.creation_allowed") boolean namespaceCreationAllowed, @Named("che.infra.kubernetes.namespace.label") boolean labelProjects, + @Named("che.infra.kubernetes.namespace.annotate") boolean annotateProjects, @Named("che.infra.kubernetes.namespace.labels") String projectLabels, @Named("che.infra.kubernetes.namespace.annotations") String projectAnnotations, @Named("che.infra.openshift.project.init_with_server_sa") boolean initWithCheServerSa, @@ -89,6 +91,7 @@ public class OpenShiftProjectFactory extends KubernetesNamespaceFactory { defaultNamespaceName, namespaceCreationAllowed, labelProjects, + annotateProjects, projectLabels, projectAnnotations, clientFactory, @@ -106,10 +109,16 @@ public class OpenShiftProjectFactory extends KubernetesNamespaceFactory { public OpenShiftProject getOrCreate(RuntimeIdentity identity) throws InfrastructureException { OpenShiftProject osProject = get(identity); + NamespaceResolutionContext resolutionCtx = + new NamespaceResolutionContext(EnvironmentContext.getCurrent().getSubject()); + Map namespaceAnnotationsEvaluated = + evaluateAnnotationPlaceholders(resolutionCtx); + osProject.prepare( canCreateNamespace(identity), initWithCheServerSa && !isNullOrEmpty(oAuthIdentityProvider), - labelNamespaces ? namespaceLabels : emptyMap()); + labelNamespaces ? namespaceLabels : emptyMap(), + annotateNamespaces ? namespaceAnnotationsEvaluated : emptyMap()); if (!isNullOrEmpty(getServiceAccountName())) { OpenShiftWorkspaceServiceAccount osWorkspaceServiceAccount = diff --git a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/multiuser/oauth/KeycloakProviderConfigFactoryTest.java b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/multiuser/oauth/KeycloakProviderConfigFactoryTest.java index 6eea1e6d24..501c6bcf2c 100644 --- a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/multiuser/oauth/KeycloakProviderConfigFactoryTest.java +++ b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/multiuser/oauth/KeycloakProviderConfigFactoryTest.java @@ -44,6 +44,7 @@ import org.eclipse.che.multiuser.keycloak.server.KeycloakSettings; import org.eclipse.che.multiuser.keycloak.shared.dto.KeycloakTokenResponse; import org.mockito.Mock; import org.mockito.testng.MockitoTestNGListener; +import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Listeners; import org.testng.annotations.Test; @@ -134,6 +135,11 @@ public class KeycloakProviderConfigFactoryTest { defaultConfig = new io.fabric8.kubernetes.client.ConfigBuilder().build(); } + @AfterMethod + public void cleanup() { + EnvironmentContext.reset(); + } + @Test public void testFallbackToDefaultConfigWhenProvideIsNull() throws Exception { configBuilder = 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 3a781b1c75..3d5e47dc06 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 @@ -152,6 +152,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -184,6 +185,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -216,6 +218,7 @@ public class OpenShiftProjectFactoryTest { null, true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -253,6 +256,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -291,6 +295,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -325,6 +330,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -369,6 +375,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -407,6 +414,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -445,6 +453,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -473,6 +482,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -506,6 +516,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -528,7 +539,7 @@ public class OpenShiftProjectFactoryTest { // then assertEquals(toReturnProject, project); verify(projectFactory, never()).doCreateServiceAccount(any(), any()); - verify(toReturnProject).prepare(eq(false), eq(false), any()); + verify(toReturnProject).prepare(eq(false), eq(false), any(), any()); } @Test @@ -543,6 +554,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -582,6 +594,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -623,6 +636,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -676,6 +690,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, NAMESPACE_LABELS, NAMESPACE_ANNOTATIONS, true, @@ -710,6 +725,7 @@ public class OpenShiftProjectFactoryTest { "-che", true, true, + true, "try_placeholder_here=", NAMESPACE_ANNOTATIONS, true, @@ -727,6 +743,44 @@ public class OpenShiftProjectFactoryTest { verify(projectOperation).withLabels(Map.of("try_placeholder_here", "")); } + @Test + public void testUsernamePlaceholderInAnnotationsIsEvaluated() throws InfrastructureException { + + // given + projectFactory = + spy( + new OpenShiftProjectFactory( + "", + null, + "-che", + true, + true, + true, + NAMESPACE_LABELS, + "try_placeholder_here=", + true, + clientFactory, + cheClientFactory, + cheServerOpenshiftClientFactory, + stopWorkspaceRoleProvisioner, + userManager, + preferenceManager, + pool, + NO_OAUTH_IDENTITY_PROVIDER)); + EnvironmentContext.getCurrent().setSubject(new SubjectImpl("jondoe", "123", null, false)); + OpenShiftProject toReturnProject = mock(OpenShiftProject.class); + doReturn(toReturnProject).when(projectFactory).doCreateProjectAccess(any(), any()); + + // when + RuntimeIdentity identity = new RuntimeIdentityImpl("workspace123", null, USER_ID, "old-che"); + OpenShiftProject project = projectFactory.getOrCreate(identity); + + // then + assertEquals(toReturnProject, project); + verify(toReturnProject) + .prepare(eq(false), eq(false), any(), eq(Map.of("try_placeholder_here", "jondoe"))); + } + private void prepareNamespaceToBeFoundByName(String name, Project project) throws Exception { @SuppressWarnings("unchecked") Resource getProjectByNameOperation = mock(Resource.class); diff --git a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectTest.java b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectTest.java index 09772c4986..1e8ad7820b 100644 --- a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectTest.java +++ b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectTest.java @@ -156,7 +156,7 @@ public class OpenShiftProjectTest { WORKSPACE_ID); // when - project.prepare(true, true, Map.of()); + project.prepare(true, true, Map.of(), Map.of()); // then verify(metadataNested, never()).withName(PROJECT_NAME); @@ -179,7 +179,7 @@ public class OpenShiftProjectTest { WORKSPACE_ID); // when - openShiftProject.prepare(true, false, Map.of()); + openShiftProject.prepare(true, false, Map.of(), Map.of()); // then ArgumentCaptor captor = ArgumentCaptor.forClass(ProjectRequest.class); @@ -210,7 +210,7 @@ public class OpenShiftProjectTest { when(openShiftClient.currentUser()) .thenReturn(new UserBuilder().withNewMetadata().withName("user").endMetadata().build()); // when - openShiftProject.prepare(true, true, Map.of()); + openShiftProject.prepare(true, true, Map.of(), Map.of()); // then ArgumentCaptor captor = ArgumentCaptor.forClass(ProjectRequest.class); @@ -247,7 +247,7 @@ public class OpenShiftProjectTest { when(openShiftClient.currentUser()) .thenReturn(new UserBuilder().withNewMetadata().withName("jdoe").endMetadata().build()); // when - openShiftProject.prepare(true, true, Map.of()); + openShiftProject.prepare(true, true, Map.of(), Map.of()); // then ArgumentCaptor roleBindingArgumentCaptor = @@ -275,7 +275,7 @@ public class OpenShiftProjectTest { WORKSPACE_ID); // when - project.prepare(false, true, Map.of()); + project.prepare(false, true, Map.of(), Map.of()); // then // exception is thrown @@ -406,7 +406,7 @@ public class OpenShiftProjectTest { Map labels = Map.of("label.with.this", "yes", "and.this", "of courese"); // when - openShiftProject.prepare(true, true, labels); + openShiftProject.prepare(true, true, labels, Map.of()); // then Namespace updatedNamespace = namespaceArgumentCaptor.getValue(); @@ -444,13 +444,81 @@ public class OpenShiftProjectTest { .createOrReplace(any(Namespace.class)); // when - openShiftProject.prepare(true, true, labels); + openShiftProject.prepare(true, true, labels, Map.of()); // then assertTrue(namespace.getMetadata().getLabels().entrySet().containsAll(labels.entrySet())); verify(nonNamespaceOperation, never()).createOrReplace(any()); } + @Test + public void testAnnotateNamespace() throws InfrastructureException { + // given + prepareProject(PROJECT_NAME); + prepareNamespaceGet(PROJECT_NAME); + OpenShiftProject openShiftProject = + new OpenShiftProject( + clientFactory, + cheClientFactory, + cheServerOpenshiftClientFactory, + executor, + PROJECT_NAME, + WORKSPACE_ID); + + KubernetesClient cheKubeClient = mock(KubernetesClient.class); + doReturn(cheKubeClient).when(cheClientFactory).create(); + + NonNamespaceOperation nonNamespaceOperation = mock(NonNamespaceOperation.class); + doReturn(nonNamespaceOperation).when(cheKubeClient).namespaces(); + + ArgumentCaptor namespaceArgumentCaptor = ArgumentCaptor.forClass(Namespace.class); + doAnswer(a -> a.getArgument(0)) + .when(nonNamespaceOperation) + .createOrReplace(namespaceArgumentCaptor.capture()); + + Map annotations = + Map.of("annotation.with.this", "yes", "and.this", "of courese"); + + // when + openShiftProject.prepare(true, true, Map.of(), annotations); + + // then + Namespace updatedNamespace = namespaceArgumentCaptor.getValue(); + assertTrue( + updatedNamespace + .getMetadata() + .getAnnotations() + .entrySet() + .containsAll(annotations.entrySet())); + assertEquals(updatedNamespace.getMetadata().getName(), PROJECT_NAME); + } + + @Test + public void testDontTryToAnnotateNamespaceIfAlreadyAnnotated() throws InfrastructureException { + // given + Map annotations = + Map.of("annotation.with.this", "yes", "and.this", "of courese"); + + prepareProject(PROJECT_NAME); + Namespace namespace = prepareNamespaceGet(PROJECT_NAME); + namespace.getMetadata().setAnnotations(annotations); + OpenShiftProject openShiftProject = + new OpenShiftProject( + clientFactory, + cheClientFactory, + cheServerOpenshiftClientFactory, + executor, + PROJECT_NAME, + WORKSPACE_ID); + + // when + openShiftProject.prepare(true, true, Map.of(), annotations); + + // then + assertTrue( + namespace.getMetadata().getAnnotations().entrySet().containsAll(annotations.entrySet())); + } + @Test public void testDoNotFailWhenNoPermissionsToUpdateNamespace() throws InfrastructureException { // given @@ -480,7 +548,7 @@ public class OpenShiftProjectTest { .createOrReplace(namespaceArgumentCaptor.capture()); // when - openShiftProject.prepare(true, true, labels); + openShiftProject.prepare(true, true, labels, Map.of()); // then Namespace updatedNamespace = namespaceArgumentCaptor.getValue(); @@ -517,7 +585,7 @@ public class OpenShiftProjectTest { .createOrReplace(any(Namespace.class)); // when - openShiftProject.prepare(true, true, labels); + openShiftProject.prepare(true, true, labels, Map.of()); // then verify(nonNamespaceOperation).createOrReplace(namespace);