From 7ed6b189a0a8dca9a8af0075df185ea1c37f6e25 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 9 Oct 2019 14:52:22 +0200 Subject: [PATCH 01/25] Load the workspace during then namespace resolution so that we can check for the attribute stored in the workspace config. Signed-off-by: Lukas Krejci --- .../webapp/WEB-INF/classes/che/che.properties | 2 +- .../namespace/KubernetesNamespaceFactory.java | 143 ++++++++++---- .../namespace/pvc/WorkspacePVCCleaner.java | 2 +- .../KubernetesNamespaceFactoryTest.java | 175 +++++++++++++++--- .../pvc/WorkspacePVCCleanerTest.java | 4 +- .../project/OpenShiftProjectFactory.java | 23 ++- .../project/OpenShiftProjectFactoryTest.java | 73 ++++++-- .../che/api/workspace/shared/Constants.java | 7 + 8 files changed, 350 insertions(+), 79 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 10f737cad6b..7ed10fa94b3 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 @@ -394,7 +394,7 @@ che.infra.kubernetes.namespace= # # BETA It's not fully supported by infra. # Use che.infra.kubernetes.namespace to configure workspaces' namespace -che.infra.kubernetes.namespace.default=-che +che.infra.kubernetes.namespace.default= # Defines if a user is able to specify Kubernetes namespace different from default. # It's NOT RECOMMENDED to configured true without OAuth configured. 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 243d4546497..2b553485701 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 @@ -13,6 +13,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Strings.isNullOrEmpty; +import static java.lang.String.format; import static java.util.Collections.singletonList; 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; @@ -30,7 +31,14 @@ import java.util.function.Function; import java.util.stream.Collectors; import javax.inject.Named; +import org.eclipse.che.api.core.ConflictException; +import org.eclipse.che.api.core.NotFoundException; +import org.eclipse.che.api.core.ServerException; +import org.eclipse.che.api.core.ValidationException; +import org.eclipse.che.api.core.model.workspace.Workspace; +import org.eclipse.che.api.workspace.server.WorkspaceManager; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; +import org.eclipse.che.api.workspace.shared.Constants; import org.eclipse.che.commons.annotation.Nullable; import org.eclipse.che.commons.env.EnvironmentContext; import org.eclipse.che.commons.subject.Subject; @@ -38,6 +46,8 @@ import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory; import org.eclipse.che.workspace.infrastructure.kubernetes.api.server.impls.KubernetesNamespaceMetaImpl; import org.eclipse.che.workspace.infrastructure.kubernetes.api.shared.KubernetesNamespaceMeta; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Helps to create {@link KubernetesNamespace} instances. @@ -46,23 +56,26 @@ */ @Singleton public class KubernetesNamespaceFactory { + private static final Logger LOG = LoggerFactory.getLogger(KubernetesNamespaceFactory.class); - private static final Map> NAMESPACE_NAME_PLACEHOLDERS = - new HashMap<>(); + private static final Map> + NAMESPACE_NAME_PLACEHOLDERS = new HashMap<>(); static { - NAMESPACE_NAME_PLACEHOLDERS.put("", Subject::getUserName); - NAMESPACE_NAME_PLACEHOLDERS.put("", Subject::getUserId); + NAMESPACE_NAME_PLACEHOLDERS.put("", ctx -> ctx.user.getUserName()); + NAMESPACE_NAME_PLACEHOLDERS.put("", ctx -> ctx.user.getUserId()); + NAMESPACE_NAME_PLACEHOLDERS.put("", ctx -> ctx.workspaceId); } private final String defaultNamespaceName; private final boolean allowUserDefinedNamespaces; private final String namespaceName; - private final boolean isPredefined; + private final boolean isStatic; private final String serviceAccountName; private final String clusterRoleName; private final KubernetesClientFactory clientFactory; + private final WorkspaceManager workspaceManager; @Inject public KubernetesNamespaceFactory( @@ -72,15 +85,27 @@ public KubernetesNamespaceFactory( @Nullable @Named("che.infra.kubernetes.namespace.default") String defaultNamespaceName, @Named("che.infra.kubernetes.namespace.allow_user_defined") boolean allowUserDefinedNamespaces, - KubernetesClientFactory clientFactory) + KubernetesClientFactory clientFactory, + WorkspaceManager workspaceManager) throws ConfigurationException { this.namespaceName = namespaceName; - this.isPredefined = !isNullOrEmpty(namespaceName) && hasNoPlaceholders(this.namespaceName); + this.isStatic = !isNullOrEmpty(namespaceName) && hasNoPlaceholders(namespaceName); this.serviceAccountName = serviceAccountName; this.clusterRoleName = clusterRoleName; this.clientFactory = clientFactory; this.defaultNamespaceName = defaultNamespaceName; this.allowUserDefinedNamespaces = allowUserDefinedNamespaces; + this.workspaceManager = workspaceManager; + + // This will disappear once we support user selection of workspaces... + if (allowUserDefinedNamespaces) { + LOG.warn( + "'che.infra.kubernetes.namespace.allow_user_defined' is not supported yet. It currently has no" + + " effect."); + } + + // right now allowUserDefinedNamespaces can't be true, but eventually we will implement it. + // noinspection ConstantConditions if (isNullOrEmpty(defaultNamespaceName) && !allowUserDefinedNamespaces) { throw new ConfigurationException( "che.infra.kubernetes.namespace.default or " @@ -96,8 +121,8 @@ private boolean hasNoPlaceholders(String namespaceName) { * True if namespace is predefined for all workspaces. False if each workspace will be provided * with a new namespace or provided for each user when using placeholders. */ - public boolean isPredefined() { - return isPredefined; + public boolean isNamespaceStatic() { + return isStatic; } /** @@ -141,8 +166,7 @@ private KubernetesNamespaceMeta getDefaultNamespace() throws InfrastructureExcep // the default namespace must be configured if user defined are not allowed // so return only it String evaluatedName = - evalDefaultNamespaceName( - defaultNamespaceName, EnvironmentContext.getCurrent().getSubject()); + evalPlaceholders(defaultNamespaceName, EnvironmentContext.getCurrent().getSubject(), null); Optional defaultNamespaceOpt = fetchNamespace(evaluatedName); @@ -164,8 +188,7 @@ private KubernetesNamespaceMeta getDefaultNamespace() throws InfrastructureExcep */ private void provisionDefaultNamespace(List namespaces) { String evaluatedName = - evalDefaultNamespaceName( - defaultNamespaceName, EnvironmentContext.getCurrent().getSubject()); + evalPlaceholders(defaultNamespaceName, EnvironmentContext.getCurrent().getSubject(), null); Optional defaultNamespaceOpt = namespaces.stream().filter(n -> evaluatedName.equals(n.getName())).findAny(); @@ -244,12 +267,18 @@ private KubernetesNamespaceMeta asNamespaceMeta(Namespace namespace) { * @throws InfrastructureException if any exception occurs during namespace preparing */ public KubernetesNamespace create(String workspaceId) throws InfrastructureException { - final String namespaceName = - evalNamespaceName(workspaceId, EnvironmentContext.getCurrent().getSubject()); + final String namespaceName; + try { + namespaceName = evalNamespaceName(workspaceId, EnvironmentContext.getCurrent().getSubject()); + } catch (NotFoundException | ServerException | ConflictException | ValidationException e) { + throw new InfrastructureException( + format("Failed to determine the namespace to put the workspace %s to", workspaceId), e); + } + KubernetesNamespace namespace = doCreateNamespace(workspaceId, namespaceName); namespace.prepare(); - if (!isPredefined() && !isNullOrEmpty(serviceAccountName)) { + if (!isNamespaceStatic() && !isNullOrEmpty(serviceAccountName)) { // prepare service account for workspace only if account name is configured // and project is not predefined // since predefined project should be prepared during Che deployment @@ -261,29 +290,67 @@ public KubernetesNamespace create(String workspaceId) throws InfrastructureExcep return namespace; } - protected String evalNamespaceName(String workspaceId, Subject currentUser) { - if (isPredefined) { - return this.namespaceName; - } else if (isNullOrEmpty(this.namespaceName)) { - return workspaceId; + protected String evalNamespaceName(String workspaceId, Subject currentUser) + throws NotFoundException, ServerException, InfrastructureException, ConflictException, + ValidationException { + // This, my friend, is a hack of magnificent proportions put forth as a result of dire time + // constraints imposed on the tears shedding developer writing these unfortunate lines. + // The effort required to propagate the full workspace, including the attributes (or some + // alternative thereof) from the callers (all of which happen to already possess the + // information) down to this sad place is too effing much to do with confidence in the + // couple of days left until the release. Let's pretend we will have time to fix this later + // in the better times... + Workspace wkspc = workspaceManager.getWorkspace(workspaceId); + String ns = wkspc.getAttributes().get(Constants.WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE); + + if (ns != null) { + return ns; } else { - String tmpNamespaceName = this.namespaceName; - for (String placeholder : NAMESPACE_NAME_PLACEHOLDERS.keySet()) { - tmpNamespaceName = - tmpNamespaceName.replaceAll( - placeholder, NAMESPACE_NAME_PLACEHOLDERS.get(placeholder).apply(currentUser)); + String effectiveOldLogicNamespace = + isNullOrEmpty(namespaceName) ? "" : namespaceName; + String namespace = evalPlaceholders(effectiveOldLogicNamespace, currentUser, workspaceId); + + if (clientFactory.create().namespaces().withName(namespace).get() == null) { + // ok, the namespace pointed to by the legacy config doesn't exist.. that means there can be + // no damage done by storing the workspace in the namespace designated by the new way of + // doing things... + + if (isNullOrEmpty(defaultNamespaceName)) { + throw new InfrastructureException( + format( + "'che.infra.kubernetes.namespace.default' is not" + + " defined and no explicit namespace configured for workspace %s", + workspaceId)); + } + + namespace = evalPlaceholders(defaultNamespaceName, currentUser, workspaceId); } - return tmpNamespaceName; + + // Now, believe it or not, the horror continues - since the callers are as of now unaware + // of the namespaces being stored within the workspace, we need to do it all here. Hopefully, + // one day, when the callers (and workspace manager in particular) support workspace namespace + // selection, things will make more sense again because this logic will have to move up a + // layer or two and become infrastructure independent. + wkspc.getAttributes().put(Constants.WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, namespace); + workspaceManager.updateWorkspace(workspaceId, wkspc); + + return namespace; } } - protected String evalDefaultNamespaceName(String defaultNamespace, Subject currentUser) { - checkArgument(!isNullOrEmpty(defaultNamespace)); - String evaluated = defaultNamespace; - for (Entry> placeHolder : + protected String evalPlaceholders(String namespace, Subject currentUser, String workspaceId) { + checkArgument(!isNullOrEmpty(namespace)); + String evaluated = namespace; + PlaceholderResolutionContext ctx = new PlaceholderResolutionContext(currentUser, workspaceId); + for (Entry> placeHolder : NAMESPACE_NAME_PLACEHOLDERS.entrySet()) { - evaluated = - evaluated.replaceAll(placeHolder.getKey(), placeHolder.getValue().apply(currentUser)); + + String key = placeHolder.getKey(); + String value = placeHolder.getValue().apply(ctx); + + if (value != null) { + evaluated = evaluated.replaceAll(key, value); + } } return evaluated; } @@ -302,4 +369,14 @@ protected String getServiceAccountName() { protected String getClusterRoleName() { return clusterRoleName; } + + private static final class PlaceholderResolutionContext { + final Subject user; + final String workspaceId; + + private PlaceholderResolutionContext(Subject user, String workspaceId) { + this.user = user; + this.workspaceId = workspaceId; + } + } } diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java index cdc365f4280..116850a4dbf 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java @@ -52,7 +52,7 @@ public WorkspacePVCCleaner( @Inject public void subscribe(EventService eventService) { - if (pvcEnabled && namespaceFactory.isPredefined()) + if (pvcEnabled && namespaceFactory.isNamespaceStatic()) eventService.subscribe( event -> { final Workspace workspace = event.getWorkspace(); 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 b1e1b1b46b7..55500898199 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 @@ -11,10 +11,13 @@ */ package org.eclipse.che.workspace.infrastructure.kubernetes.namespace; +import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; 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.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; @@ -37,7 +40,10 @@ import io.fabric8.kubernetes.client.dsl.Resource; import java.util.Arrays; import java.util.List; +import org.eclipse.che.api.workspace.server.WorkspaceManager; +import org.eclipse.che.api.workspace.server.model.impl.WorkspaceImpl; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; +import org.eclipse.che.api.workspace.shared.Constants; import org.eclipse.che.commons.subject.SubjectImpl; import org.eclipse.che.inject.ConfigurationException; import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory; @@ -58,18 +64,26 @@ public class KubernetesNamespaceFactoryTest { @Mock private KubernetesClientFactory clientFactory; @Mock private KubernetesClient k8sClient; + @Mock private WorkspaceManager workspaceManager; @Mock private NonNamespaceOperation< Namespace, NamespaceList, DoneableNamespace, Resource> namespaceOperation; + @Mock private Resource namespaceResource; + private KubernetesNamespaceFactory namespaceFactory; @BeforeMethod public void setUp() throws Exception { lenient().when(clientFactory.create()).thenReturn(k8sClient); lenient().when(k8sClient.namespaces()).thenReturn(namespaceOperation); + when(workspaceManager.getWorkspace(any())) + .thenReturn(WorkspaceImpl.builder().setId("1").setAttributes(emptyMap()).build()); + + when(namespaceOperation.withName(any())).thenReturn(namespaceResource); + when(namespaceResource.get()).thenReturn(mock(Namespace.class)); } @Test( @@ -81,7 +95,8 @@ public void setUp() throws Exception { shouldThrowExceptionIfNoDefaultNamespaceIsConfiguredAndUserDefinedNamespacesAreNotAllowed() throws Exception { namespaceFactory = - new KubernetesNamespaceFactory("predefined", "", "", null, false, clientFactory); + new KubernetesNamespaceFactory( + "predefined", "", "", null, false, clientFactory, workspaceManager); } @Test @@ -96,7 +111,8 @@ public void shouldReturnDefaultNamespaceWhenItExistsAndUserDefinedIsNotAllowed() .withNewStatus("Active") .build()); namespaceFactory = - new KubernetesNamespaceFactory("predefined", "", "", "che-default", false, clientFactory); + new KubernetesNamespaceFactory( + "predefined", "", "", "che-default", false, clientFactory, workspaceManager); List availableNamespaces = namespaceFactory.list(); assertEquals(availableNamespaces.size(), 1); @@ -112,7 +128,8 @@ public void shouldReturnDefaultNamespaceWhenItDoesNotExistAndUserDefinedIsNotAll prepareNamespaceToBeFoundByName("che-default", null); namespaceFactory = - new KubernetesNamespaceFactory("predefined", "", "", "che-default", false, clientFactory); + new KubernetesNamespaceFactory( + "predefined", "", "", "che-default", false, clientFactory, workspaceManager); List availableNamespaces = namespaceFactory.list(); assertEquals(availableNamespaces.size(), 1); @@ -131,7 +148,8 @@ public void shouldReturnDefaultNamespaceWhenItDoesNotExistAndUserDefinedIsNotAll "Error occurred when tried to fetch default namespace. Cause: connection refused") public void shouldThrownExceptionWhenFailedToGetInfoAboutDefaultNamespace() throws Exception { namespaceFactory = - new KubernetesNamespaceFactory("predefined", "", "", "che", false, clientFactory); + new KubernetesNamespaceFactory( + "predefined", "", "", "che", false, clientFactory, workspaceManager); throwOnTryToGetNamespaceByName("che", new KubernetesClientException("connection refused")); namespaceFactory.list(); @@ -145,7 +163,8 @@ public void shouldReturnListOfExistingNamespacesIfUserDefinedIsAllowed() throws createNamespace("experimental", "Terminating"))); namespaceFactory = - new KubernetesNamespaceFactory("predefined", "", "", null, true, clientFactory); + new KubernetesNamespaceFactory( + "predefined", "", "", null, true, clientFactory, workspaceManager); List availableNamespaces = namespaceFactory.list(); assertEquals(availableNamespaces.size(), 2); @@ -167,7 +186,8 @@ public void shouldReturnListOfExistingNamespacesAlongWithDefaultIfUserDefinedIsA createNamespace("my-for-ws", "Active"), createNamespace("default", "Active"))); namespaceFactory = - new KubernetesNamespaceFactory("predefined", "", "", "default", true, clientFactory); + new KubernetesNamespaceFactory( + "predefined", "", "", "default", true, clientFactory, workspaceManager); List availableNamespaces = namespaceFactory.list(); @@ -190,7 +210,8 @@ public void shouldReturnListOfExistingNamespacesAlongWithDefaultIfUserDefinedIsA prepareListedNamespaces(singletonList(createNamespace("my-for-ws", "Active"))); namespaceFactory = - new KubernetesNamespaceFactory("predefined", "", "", "default", true, clientFactory); + new KubernetesNamespaceFactory( + "predefined", "", "", "default", true, clientFactory, workspaceManager); List availableNamespaces = namespaceFactory.list(); assertEquals(availableNamespaces.size(), 2); @@ -214,7 +235,8 @@ public void shouldReturnListOfExistingNamespacesAlongWithDefaultIfUserDefinedIsA "Error occurred when tried to list all available namespaces. Cause: connection refused") public void shouldThrownExceptionWhenFailedToGetNamespaces() throws Exception { namespaceFactory = - new KubernetesNamespaceFactory("predefined", "", "", null, true, clientFactory); + new KubernetesNamespaceFactory( + "predefined", "", "", null, true, clientFactory, workspaceManager); throwOnTryToGetNamespacesList(new KubernetesClientException("connection refused")); namespaceFactory.list(); @@ -224,10 +246,11 @@ public void shouldThrownExceptionWhenFailedToGetNamespaces() throws Exception { public void shouldReturnTrueIfNamespaceIsNotEmptyOnCheckingIfNamespaceIsPredefined() { // given namespaceFactory = - new KubernetesNamespaceFactory("predefined", "", "", "che", false, clientFactory); + new KubernetesNamespaceFactory( + "predefined", "", "", "che", false, clientFactory, workspaceManager); // when - boolean isPredefined = namespaceFactory.isPredefined(); + boolean isPredefined = namespaceFactory.isNamespaceStatic(); // then assertTrue(isPredefined); @@ -236,10 +259,11 @@ public void shouldReturnTrueIfNamespaceIsNotEmptyOnCheckingIfNamespaceIsPredefin @Test public void shouldReturnTrueIfNamespaceIsEmptyOnCheckingIfNamespaceIsPredefined() { // given - namespaceFactory = new KubernetesNamespaceFactory("", "", "", "che", false, clientFactory); + namespaceFactory = + new KubernetesNamespaceFactory("", "", "", "che", false, clientFactory, workspaceManager); // when - boolean isPredefined = namespaceFactory.isPredefined(); + boolean isPredefined = namespaceFactory.isNamespaceStatic(); // then assertFalse(isPredefined); @@ -248,10 +272,11 @@ public void shouldReturnTrueIfNamespaceIsEmptyOnCheckingIfNamespaceIsPredefined( @Test public void shouldReturnTrueIfNamespaceIsNullOnCheckingIfNamespaceIsPredefined() { // given - namespaceFactory = new KubernetesNamespaceFactory(null, "", "", "che", false, clientFactory); + namespaceFactory = + new KubernetesNamespaceFactory(null, "", "", "che", false, clientFactory, workspaceManager); // when - boolean isPredefined = namespaceFactory.isPredefined(); + boolean isPredefined = namespaceFactory.isNamespaceStatic(); // then assertFalse(isPredefined); @@ -261,7 +286,9 @@ public void shouldReturnTrueIfNamespaceIsNullOnCheckingIfNamespaceIsPredefined() public void shouldCreateAndPrepareNamespaceWithPredefinedValueIfItIsNotEmpty() throws Exception { // given namespaceFactory = - spy(new KubernetesNamespaceFactory("predefined", "", "", "che", false, clientFactory)); + spy( + new KubernetesNamespaceFactory( + "predefined", "", "", "che", false, clientFactory, workspaceManager)); KubernetesNamespace toReturnNamespace = mock(KubernetesNamespace.class); doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespace(any(), any()); @@ -278,7 +305,10 @@ public void shouldCreateAndPrepareNamespaceWithPredefinedValueIfItIsNotEmpty() t public void shouldCreateAndPrepareNamespaceWithWorkspaceIdAsNameIfConfiguredNameIsNotPredefined() throws Exception { // given - namespaceFactory = spy(new KubernetesNamespaceFactory("", "", "", "che", false, clientFactory)); + namespaceFactory = + spy( + new KubernetesNamespaceFactory( + "", "", "", "che", false, clientFactory, workspaceManager)); KubernetesNamespace toReturnNamespace = mock(KubernetesNamespace.class); doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespace(any(), any()); @@ -296,7 +326,10 @@ public void shouldCreateAndPrepareNamespaceWithWorkspaceIdAsNameIfConfiguredName shouldCreateNamespaceAndDoNotPrepareNamespaceOnCreatingNamespaceWithWorkspaceIdAndNameSpecified() throws Exception { // given - namespaceFactory = spy(new KubernetesNamespaceFactory("", "", "", "che", false, clientFactory)); + namespaceFactory = + spy( + new KubernetesNamespaceFactory( + "", "", "", "che", false, clientFactory, workspaceManager)); KubernetesNamespace toReturnNamespace = mock(KubernetesNamespace.class); doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespace(any(), any()); @@ -314,7 +347,9 @@ public void shouldPrepareWorkspaceServiceAccountIfItIsConfiguredAndNamespaceIsNo throws Exception { // given namespaceFactory = - spy(new KubernetesNamespaceFactory("", "serviceAccount", "", "che", false, clientFactory)); + spy( + new KubernetesNamespaceFactory( + "", "serviceAccount", "", "che", false, clientFactory, workspaceManager)); KubernetesNamespace toReturnNamespace = mock(KubernetesNamespace.class); doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespace(any(), any()); @@ -337,7 +372,13 @@ public void shouldNotPrepareWorkspaceServiceAccountIfItIsConfiguredAndProjectIsP namespaceFactory = spy( new KubernetesNamespaceFactory( - "namespace", "serviceAccount", "clusterRole", "che", false, clientFactory)); + "namespace", + "serviceAccount", + "clusterRole", + "che", + false, + clientFactory, + workspaceManager)); KubernetesNamespace toReturnNamespace = mock(KubernetesNamespace.class); doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespace(any(), any()); @@ -356,7 +397,10 @@ public void shouldNotPrepareWorkspaceServiceAccountIfItIsConfiguredAndProjectIsP public void shouldNotPrepareWorkspaceServiceAccountIfItIsNotConfiguredAndProjectIsNotPredefined() throws Exception { // given - namespaceFactory = spy(new KubernetesNamespaceFactory("", "", "", "che", false, clientFactory)); + namespaceFactory = + spy( + new KubernetesNamespaceFactory( + "", "", "", "che", false, clientFactory, workspaceManager)); KubernetesNamespace toReturnNamespace = mock(KubernetesNamespace.class); doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespace(any(), any()); @@ -372,21 +416,106 @@ public void shouldNotPrepareWorkspaceServiceAccountIfItIsNotConfiguredAndProject } @Test - public void testPlaceholder() { + public void + testEvalNamespaceUsesNamespaceDefaultIfWorkspaceDoesntRecordNamespaceAndLegacyNamespaceDoesntExist() + throws Exception { namespaceFactory = new KubernetesNamespaceFactory( "blabol------", "", "", - "che", + "che-", false, - clientFactory); + clientFactory, + workspaceManager); + + when(namespaceResource.get()).thenReturn(null); + + String namespace = + namespaceFactory.evalNamespaceName(null, new SubjectImpl("JonDoe", "123", null, false)); + + assertEquals(namespace, "che-123"); + } + + @Test + public void + testEvalNamespaceUsesLegacyNamespaceIfWorkspaceDoesntRecordNamespaceAndLegacyNamespaceExists() + throws Exception { + + namespaceFactory = + new KubernetesNamespaceFactory( + "blabol------", + "", + "", + "che-", + false, + clientFactory, + workspaceManager); + String namespace = namespaceFactory.evalNamespaceName(null, new SubjectImpl("JonDoe", "123", null, false)); assertEquals(namespace, "blabol-123-JonDoe-123-JonDoe--"); } + @Test + public void testEvalNamespaceUsesWorkspaceRecordedNamespaceIfWorkspaceRecordsIt() + throws Exception { + + namespaceFactory = + new KubernetesNamespaceFactory( + "blabol------", + "", + "", + "che-", + false, + clientFactory, + workspaceManager); + + when(workspaceManager.getWorkspace(eq("42"))) + .thenReturn( + WorkspaceImpl.builder() + .setId("42") + .setAttributes( + singletonMap( + Constants.WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, "wkspcnmspc")) + .build()); + + String namespace = + namespaceFactory.evalNamespaceName("42", new SubjectImpl("JonDoe", "123", null, false)); + + assertEquals(namespace, "wkspcnmspc"); + } + + @Test + public void testEvalNamespaceTreatsWorkspaceRecordedNamespaceLiterally() throws Exception { + + namespaceFactory = + new KubernetesNamespaceFactory( + "blabol------", + "", + "", + "che-", + false, + clientFactory, + workspaceManager); + + when(workspaceManager.getWorkspace(eq("42"))) + .thenReturn( + WorkspaceImpl.builder() + .setId("42") + .setAttributes( + singletonMap( + Constants.WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, "")) + .build()); + + String namespace = + namespaceFactory.evalNamespaceName("42", new SubjectImpl("JonDoe", "123", null, false)); + + // this is an invalid name, but that is not a purpose of this test. + assertEquals(namespace, ""); + } + private void prepareNamespaceToBeFoundByName(String name, Namespace namespace) throws Exception { @SuppressWarnings("unchecked") Resource getNamespaceByNameOperation = mock(Resource.class); diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java index c54b9a16c13..277a7f5f9f1 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java @@ -50,7 +50,7 @@ public class WorkspacePVCCleanerTest { @BeforeMethod public void setUp() { - when(namespaceFactory.isPredefined()).thenReturn(true); + when(namespaceFactory.isNamespaceStatic()).thenReturn(true); workspacePVCCleaner = new WorkspacePVCCleaner(true, namespaceFactory, pvcStrategy); } @@ -65,7 +65,7 @@ public void testDoNotSubscribesCleanerWhenPVCDisabled() { @Test public void testDoNotSubscribesCleanerWhenPVCEnabledAndNamespaceIsNotPredefined() { - when(namespaceFactory.isPredefined()).thenReturn(false); + when(namespaceFactory.isNamespaceStatic()).thenReturn(false); workspacePVCCleaner = spy(new WorkspacePVCCleaner(false, namespaceFactory, pvcStrategy)); workspacePVCCleaner.subscribe(eventService); 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 fb4f2bb9e6c..bc55754c6ae 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 @@ -12,6 +12,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 org.eclipse.che.workspace.infrastructure.kubernetes.api.shared.KubernetesNamespaceMeta.PHASE_ATTRIBUTE; import com.google.common.annotations.VisibleForTesting; @@ -26,6 +27,11 @@ import java.util.Optional; import java.util.stream.Collectors; import javax.inject.Named; +import org.eclipse.che.api.core.ConflictException; +import org.eclipse.che.api.core.NotFoundException; +import org.eclipse.che.api.core.ServerException; +import org.eclipse.che.api.core.ValidationException; +import org.eclipse.che.api.workspace.server.WorkspaceManager; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.commons.annotation.Nullable; import org.eclipse.che.commons.env.EnvironmentContext; @@ -58,14 +64,16 @@ public OpenShiftProjectFactory( @Named("che.infra.kubernetes.namespace.allow_user_defined") boolean allowUserDefinedNamespaces, OpenShiftClientFactory clientFactory, - OpenShiftClientConfigFactory clientConfigFactory) { + OpenShiftClientConfigFactory clientConfigFactory, + WorkspaceManager workspaceManager) { super( projectName, serviceAccountName, clusterRoleName, defaultNamespaceName, allowUserDefinedNamespaces, - clientFactory); + clientFactory, + workspaceManager); if (allowUserDefinedNamespaces && !clientConfigFactory.isPersonalized()) { LOG.warn( "Users are allowed to list projects but Che server is configured with a service account. " @@ -86,12 +94,17 @@ public OpenShiftProjectFactory( * @throws InfrastructureException if any exception occurs during project preparing */ public OpenShiftProject create(String workspaceId) throws InfrastructureException { - final String projectName = - evalNamespaceName(workspaceId, EnvironmentContext.getCurrent().getSubject()); + final String projectName; + try { + projectName = evalNamespaceName(workspaceId, EnvironmentContext.getCurrent().getSubject()); + } catch (NotFoundException | ServerException | ConflictException | ValidationException e) { + throw new InfrastructureException( + format("Failed to evaluate the project name to use for workspace %s.", workspaceId), e); + } OpenShiftProject osProject = doCreateProject(workspaceId, projectName); osProject.prepare(); - if (!isPredefined() && !isNullOrEmpty(getServiceAccountName())) { + if (!isNamespaceStatic() && !isNullOrEmpty(getServiceAccountName())) { // prepare service account for workspace only if account name is configured // and project is not predefined // since predefined project should be prepared during Che deployment 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 5963d1fda0d..a73d14d92aa 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 @@ -42,6 +42,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import org.eclipse.che.api.workspace.server.WorkspaceManager; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.inject.ConfigurationException; import org.eclipse.che.workspace.infrastructure.kubernetes.api.shared.KubernetesNamespaceMeta; @@ -64,6 +65,7 @@ public class OpenShiftProjectFactoryTest { @Mock private OpenShiftClientConfigFactory configFactory; @Mock private OpenShiftClientFactory clientFactory; + @Mock private WorkspaceManager workspaceManager; @Mock private NonNamespaceOperation< @@ -90,7 +92,7 @@ public void setUp() throws Exception { throws Exception { projectFactory = new OpenShiftProjectFactory( - "projectName", "", "", null, false, clientFactory, configFactory); + "projectName", "", "", null, false, clientFactory, configFactory, workspaceManager); } @Test @@ -112,7 +114,14 @@ public void shouldReturnDefaultProjectWhenItExistsAndUserDefinedIsNotAllowed() t projectFactory = new OpenShiftProjectFactory( - "predefined", "", "", "che-default", false, clientFactory, configFactory); + "predefined", + "", + "", + "che-default", + false, + clientFactory, + configFactory, + workspaceManager); List availableNamespaces = projectFactory.list(); assertEquals(availableNamespaces.size(), 1); @@ -135,7 +144,14 @@ public void shouldReturnDefaultProjectWhenItDoesNotExistAndUserDefinedIsNotAllow projectFactory = new OpenShiftProjectFactory( - "predefined", "", "", "che-default", false, clientFactory, configFactory); + "predefined", + "", + "", + "che-default", + false, + clientFactory, + configFactory, + workspaceManager); List availableNamespaces = projectFactory.list(); assertEquals(availableNamespaces.size(), 1); @@ -158,7 +174,14 @@ public void shouldThrownExceptionWhenFailedToGetInfoAboutDefaultNamespace() thro projectFactory = new OpenShiftProjectFactory( - "predefined", "", "", "che-default", false, clientFactory, configFactory); + "predefined", + "", + "", + "che-default", + false, + clientFactory, + configFactory, + workspaceManager); projectFactory.list(); } @@ -171,7 +194,8 @@ public void shouldReturnListOfExistingProjectsIfUserDefinedIsAllowed() throws Ex createProject("experimental", null, null, "Terminating"))); projectFactory = - new OpenShiftProjectFactory("predefined", "", "", null, true, clientFactory, configFactory); + new OpenShiftProjectFactory( + "predefined", "", "", null, true, clientFactory, configFactory, workspaceManager); List availableNamespaces = projectFactory.list(); assertEquals(availableNamespaces.size(), 2); @@ -197,7 +221,7 @@ public void shouldReturnListOfExistingProjectsAlongWithDefaultIfUserDefinedIsAll projectFactory = new OpenShiftProjectFactory( - "predefined", "", "", "default", true, clientFactory, configFactory); + "predefined", "", "", "default", true, clientFactory, configFactory, workspaceManager); List availableNamespaces = projectFactory.list(); @@ -228,7 +252,7 @@ public void shouldReturnListOfExistingProjectsAlongWithNonExistingDefaultIfUserD projectFactory = new OpenShiftProjectFactory( - "predefined", "", "", "default", true, clientFactory, configFactory); + "predefined", "", "", "default", true, clientFactory, configFactory, workspaceManager); List availableNamespaces = projectFactory.list(); assertEquals(availableNamespaces.size(), 2); @@ -253,7 +277,8 @@ public void shouldReturnListOfExistingProjectsAlongWithNonExistingDefaultIfUserD public void shouldThrownExceptionWhenFailedToGetNamespaces() throws Exception { throwOnTryToGetProjectsList(new KubernetesClientException("connection refused")); projectFactory = - new OpenShiftProjectFactory("predefined", "", "", "", true, clientFactory, configFactory); + new OpenShiftProjectFactory( + "predefined", "", "", "", true, clientFactory, configFactory, workspaceManager); projectFactory.list(); } @@ -264,7 +289,14 @@ public void shouldCreateAndPrepareProjectWithPredefinedValueIfItIsNotEmpty() thr projectFactory = spy( new OpenShiftProjectFactory( - "projectName", "", "", "che", false, clientFactory, configFactory)); + "projectName", + "", + "", + "che", + false, + clientFactory, + configFactory, + workspaceManager)); OpenShiftProject toReturnProject = mock(OpenShiftProject.class); doReturn(toReturnProject).when(projectFactory).doCreateProject(any(), any()); @@ -282,7 +314,9 @@ public void shouldCreateAndPrepareProjectWithWorkspaceIdAsNameIfConfiguredValueI throws Exception { // given projectFactory = - spy(new OpenShiftProjectFactory("", "", "", "che", false, clientFactory, configFactory)); + spy( + new OpenShiftProjectFactory( + "", "", "", "che", false, clientFactory, configFactory, workspaceManager)); OpenShiftProject toReturnProject = mock(OpenShiftProject.class); doReturn(toReturnProject).when(projectFactory).doCreateProject(any(), any()); @@ -302,7 +336,14 @@ public void shouldPrepareWorkspaceServiceAccountIfItIsConfiguredAndProjectIsNotP projectFactory = spy( new OpenShiftProjectFactory( - "", "serviceAccount", "", "che", false, clientFactory, configFactory)); + "", + "serviceAccount", + "", + "che", + false, + clientFactory, + configFactory, + workspaceManager)); OpenShiftProject toReturnProject = mock(OpenShiftProject.class); doReturn(toReturnProject).when(projectFactory).doCreateProject(any(), any()); @@ -330,7 +371,8 @@ public void shouldNotPrepareWorkspaceServiceAccountIfItIsConfiguredAndProjectIsP "che", false, clientFactory, - configFactory)); + configFactory, + workspaceManager)); OpenShiftProject toReturnProject = mock(OpenShiftProject.class); doReturn(toReturnProject).when(projectFactory).doCreateProject(any(), any()); @@ -346,7 +388,9 @@ public void shouldNotPrepareWorkspaceServiceAccountIfItIsNotConfiguredAndProject throws Exception { // given projectFactory = - spy(new OpenShiftProjectFactory("", "", "", "che", false, clientFactory, configFactory)); + spy( + new OpenShiftProjectFactory( + "", "", "", "che", false, clientFactory, configFactory, workspaceManager)); OpenShiftProject toReturnProject = mock(OpenShiftProject.class); doReturn(toReturnProject).when(projectFactory).doCreateProject(any(), any()); @@ -371,7 +415,8 @@ public void shouldNotPrepareWorkspaceServiceAccountIfItIsNotConfiguredAndProject "che", false, clientFactory, - configFactory)); + configFactory, + workspaceManager)); OpenShiftProject toReturnProject = mock(OpenShiftProject.class); doReturn(toReturnProject).when(projectFactory).doCreateProject(any(), any()); diff --git a/wsmaster/che-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java b/wsmaster/che-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java index a7b34a72d75..1fa2c618a22 100644 --- a/wsmaster/che-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java +++ b/wsmaster/che-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java @@ -185,5 +185,12 @@ public final class Constants { /** When generating workspace name from generateName, append this many characters. */ public static final int WORKSPACE_GENERATE_NAME_CHARS_APPEND = 5; + /** + * The attribute in the workspace config where we store the infrastructure namespace the workspace + * is deployed to + */ + public static final String WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE = + "infrastructureNamespace"; + private Constants() {} } From 87764391ae5d08dc34128a5368939fb72a0e5429 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 10 Oct 2019 10:47:40 +0200 Subject: [PATCH 02/25] Adapt the namespace checking for Openshift infra and fix the tests. Signed-off-by: Lukas Krejci --- .../namespace/KubernetesNamespaceFactory.java | 6 +- .../project/OpenShiftProjectFactory.java | 14 +++ .../project/OpenShiftProjectFactoryTest.java | 119 ++++++++++++++++++ 3 files changed, 138 insertions(+), 1 deletion(-) 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 2b553485701..86c1efa31b5 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 @@ -310,7 +310,7 @@ protected String evalNamespaceName(String workspaceId, Subject currentUser) isNullOrEmpty(namespaceName) ? "" : namespaceName; String namespace = evalPlaceholders(effectiveOldLogicNamespace, currentUser, workspaceId); - if (clientFactory.create().namespaces().withName(namespace).get() == null) { + if (!checkNamespaceExists(namespace)) { // ok, the namespace pointed to by the legacy config doesn't exist.. that means there can be // no damage done by storing the workspace in the namespace designated by the new way of // doing things... @@ -338,6 +338,10 @@ protected String evalNamespaceName(String workspaceId, Subject currentUser) } } + protected boolean checkNamespaceExists(String namespaceName) throws InfrastructureException { + return clientFactory.create().namespaces().withName(namespaceName).get() != null; + } + protected String evalPlaceholders(String namespace, Subject currentUser, String workspaceId) { checkArgument(!isNullOrEmpty(namespace)); String evaluated = namespace; 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 bc55754c6ae..2b86b46d9b1 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 @@ -35,6 +35,7 @@ import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.commons.annotation.Nullable; import org.eclipse.che.commons.env.EnvironmentContext; +import org.eclipse.che.commons.subject.Subject; import org.eclipse.che.workspace.infrastructure.kubernetes.api.server.impls.KubernetesNamespaceMetaImpl; import org.eclipse.che.workspace.infrastructure.kubernetes.api.shared.KubernetesNamespaceMeta; import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory; @@ -116,6 +117,19 @@ public OpenShiftProject create(String workspaceId) throws InfrastructureExceptio return osProject; } + @VisibleForTesting + @Override + protected String evalNamespaceName(String workspaceId, Subject currentUser) + throws NotFoundException, ServerException, InfrastructureException, ConflictException, + ValidationException { + return super.evalNamespaceName(workspaceId, currentUser); + } + + @Override + protected boolean checkNamespaceExists(String namespaceName) throws InfrastructureException { + return clientFactory.createOC().projects().withName(namespaceName).get() != null; + } + /** * Creates a kubernetes namespace for the specified workspace. * 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 a73d14d92aa..34fefc4d1e7 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 @@ -11,7 +11,9 @@ */ package org.eclipse.che.workspace.infrastructure.openshift.project; +import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; 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.openshift.Constants.PROJECT_DESCRIPTION_ANNOTATION; @@ -19,6 +21,7 @@ import static org.eclipse.che.workspace.infrastructure.openshift.Constants.PROJECT_DISPLAY_NAME_ANNOTATION; import static org.eclipse.che.workspace.infrastructure.openshift.Constants.PROJECT_DISPLAY_NAME_ATTRIBUTE; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; @@ -43,7 +46,10 @@ import java.util.List; import java.util.Map; import org.eclipse.che.api.workspace.server.WorkspaceManager; +import org.eclipse.che.api.workspace.server.model.impl.WorkspaceImpl; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; +import org.eclipse.che.api.workspace.shared.Constants; +import org.eclipse.che.commons.subject.SubjectImpl; import org.eclipse.che.inject.ConfigurationException; import org.eclipse.che.workspace.infrastructure.kubernetes.api.shared.KubernetesNamespaceMeta; import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespace; @@ -72,6 +78,8 @@ public class OpenShiftProjectFactoryTest { Project, ProjectList, DoneableProject, Resource> projectOperation; + @Mock private Resource projectResource; + @Mock private OpenShiftClient osClient; private OpenShiftProjectFactory projectFactory; @@ -80,6 +88,12 @@ public class OpenShiftProjectFactoryTest { public void setUp() throws Exception { lenient().when(clientFactory.createOC()).thenReturn(osClient); lenient().when(osClient.projects()).thenReturn(projectOperation); + + when(workspaceManager.getWorkspace(any())) + .thenReturn(WorkspaceImpl.builder().setId("1").setAttributes(emptyMap()).build()); + + when(projectOperation.withName(any())).thenReturn(projectResource); + when(projectResource.get()).thenReturn(mock(Project.class)); } @Test( @@ -429,6 +443,111 @@ public void shouldNotPrepareWorkspaceServiceAccountIfItIsNotConfiguredAndProject verify(toReturnProject, never()).prepare(); } + @Test + public void + testEvalNamespaceUsesNamespaceDefaultIfWorkspaceDoesntRecordNamespaceAndLegacyNamespaceDoesntExist() + throws Exception { + projectFactory = + new OpenShiftProjectFactory( + "blabol------", + "", + "", + "che-", + false, + clientFactory, + configFactory, + workspaceManager); + + when(projectResource.get()).thenReturn(null); + + String namespace = + projectFactory.evalNamespaceName(null, new SubjectImpl("JonDoe", "123", null, false)); + + assertEquals(namespace, "che-123"); + } + + @Test + public void + testEvalNamespaceUsesLegacyNamespaceIfWorkspaceDoesntRecordNamespaceAndLegacyNamespaceExists() + throws Exception { + + projectFactory = + new OpenShiftProjectFactory( + "blabol------", + "", + "", + "che-", + false, + clientFactory, + configFactory, + workspaceManager); + + String namespace = + projectFactory.evalNamespaceName(null, new SubjectImpl("JonDoe", "123", null, false)); + + assertEquals(namespace, "blabol-123-JonDoe-123-JonDoe--"); + } + + @Test + public void testEvalNamespaceUsesWorkspaceRecordedNamespaceIfWorkspaceRecordsIt() + throws Exception { + + projectFactory = + new OpenShiftProjectFactory( + "blabol------", + "", + "", + "che-", + false, + clientFactory, + configFactory, + workspaceManager); + + when(workspaceManager.getWorkspace(eq("42"))) + .thenReturn( + WorkspaceImpl.builder() + .setId("42") + .setAttributes( + singletonMap( + Constants.WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, "wkspcprj")) + .build()); + + String namespace = + projectFactory.evalNamespaceName("42", new SubjectImpl("JonDoe", "123", null, false)); + + assertEquals(namespace, "wkspcprj"); + } + + @Test + public void testEvalNamespaceTreatsWorkspaceRecordedNamespaceLiterally() throws Exception { + + projectFactory = + new OpenShiftProjectFactory( + "blabol------", + "", + "", + "che-", + false, + clientFactory, + configFactory, + workspaceManager); + + when(workspaceManager.getWorkspace(eq("42"))) + .thenReturn( + WorkspaceImpl.builder() + .setId("42") + .setAttributes( + singletonMap( + Constants.WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, "")) + .build()); + + String namespace = + projectFactory.evalNamespaceName("42", new SubjectImpl("JonDoe", "123", null, false)); + + // this is an invalid name, but that is not a purpose of this test. + assertEquals(namespace, ""); + } + private void prepareNamespaceToBeFoundByName(String name, Project project) throws Exception { @SuppressWarnings("unchecked") Resource getProjectByNameOperation = mock(Resource.class); From bb586f670831ffaaf928542349d5e70227974695 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 10 Oct 2019 14:04:31 +0200 Subject: [PATCH 03/25] Fix the Openshift tests to faithfully simulate the Openshift client behavior. Signed-off-by: Lukas Krejci --- .../openshift/project/OpenShiftProjectFactory.java | 8 ++++++-- .../openshift/project/OpenShiftProjectFactoryTest.java | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) 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 2b86b46d9b1..c8c95d84d73 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 @@ -127,7 +127,7 @@ protected String evalNamespaceName(String workspaceId, Subject currentUser) @Override protected boolean checkNamespaceExists(String namespaceName) throws InfrastructureException { - return clientFactory.createOC().projects().withName(namespaceName).get() != null; + return fetchNamespaceObject(namespaceName).isPresent(); } /** @@ -156,9 +156,13 @@ OpenShiftWorkspaceServiceAccount doCreateServiceAccount(String workspaceId, Stri @Override protected Optional fetchNamespace(String name) throws InfrastructureException { + return fetchNamespaceObject(name).map(this::asNamespaceMeta); + } + + private Optional fetchNamespaceObject(String name) throws InfrastructureException { try { Project project = clientFactory.createOC().projects().withName(name).get(); - return Optional.of(asNamespaceMeta(project)); + return Optional.ofNullable(project); } catch (KubernetesClientException e) { if (e.getCode() == 403) { // 403 means that the project does not exist 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 34fefc4d1e7..1ddba6d5582 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 @@ -458,7 +458,7 @@ public void shouldNotPrepareWorkspaceServiceAccountIfItIsNotConfiguredAndProject configFactory, workspaceManager); - when(projectResource.get()).thenReturn(null); + when(projectResource.get()).thenThrow(new KubernetesClientException("", 403, null)); String namespace = projectFactory.evalNamespaceName(null, new SubjectImpl("JonDoe", "123", null, false)); From 12c3fcfbe7498b02a91faf3a631c77882002d858 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 10 Oct 2019 14:04:46 +0200 Subject: [PATCH 04/25] Switch the default to -che. Signed-off-by: Lukas Krejci --- .../src/main/webapp/WEB-INF/classes/che/che.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 7ed10fa94b3..10f737cad6b 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 @@ -394,7 +394,7 @@ che.infra.kubernetes.namespace= # # BETA It's not fully supported by infra. # Use che.infra.kubernetes.namespace to configure workspaces' namespace -che.infra.kubernetes.namespace.default= +che.infra.kubernetes.namespace.default=-che # Defines if a user is able to specify Kubernetes namespace different from default. # It's NOT RECOMMENDED to configured true without OAuth configured. From c6e29c100699b384b048704666aba735d484dab7 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 10 Oct 2019 14:07:34 +0200 Subject: [PATCH 05/25] Update the javadoc a little. Signed-off-by: Lukas Krejci --- .../kubernetes/namespace/KubernetesNamespaceFactory.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 86c1efa31b5..54a06b5fc16 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 @@ -118,8 +118,8 @@ private boolean hasNoPlaceholders(String namespaceName) { } /** - * True if namespace is predefined for all workspaces. False if each workspace will be provided - * with a new namespace or provided for each user when using placeholders. + * True if namespace is the same (static) for all workspaces. False if each workspace will be + * provided with a new namespace or provided for each user when using placeholders. */ public boolean isNamespaceStatic() { return isStatic; From a240d74a33fb1d92c18cb9914e0e8aaedfaae1f7 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 10 Oct 2019 16:25:34 +0200 Subject: [PATCH 06/25] Update the property descriptions in che.properties and use the new CHE_INFRA_KUBERNETES_NAMESPACE_DEFAULT in the helm chart. Signed-off-by: Lukas Krejci --- .../webapp/WEB-INF/classes/che/che.properties | 22 +++++++++++++------ .../helm/che/templates/configmap.yaml | 2 +- 2 files changed, 16 insertions(+), 8 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 10f737cad6b..3ab7fd46324 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 @@ -377,6 +377,9 @@ che.infra.kubernetes.server_strategy=default-host # Used to generate domain for a server in a workspace in case property `che.infra.kubernetes.server_strategy` is set to `multi-host` che.infra.kubernetes.ingress.domain= +# DEPRECATED - please do not change the value of this property otherwise the existing workspaces will loose data. Do not +# set it on new installations. +# # Defines Kubernetes namespace in which all workspaces will be created. # If not set, every workspace will be created in a new namespace, where namespace = workspace id # It's possible to use and placeholders (e.g.: che-workspace-). @@ -384,24 +387,23 @@ che.infra.kubernetes.ingress.domain= # to create new namespace must be used. # # Ignored for OpenShift infra. Use `che.infra.openshift.project` instead +# +# If the namespace pointed to by this property exists, it will be used for all workspaces. If it does not exist, +# the namespace specified by the che.infra.kubernetes.namespace.default will be created and used. che.infra.kubernetes.namespace= # Defines Kubernetes default namespace in which user's workspaces are created # if user does not override it. -# It's possible to use and placeholders (e.g.: che-workspace-). -# In that case, new namespace will be created for each user. +# It's possible to use , and placeholders (e.g.: che-workspace-). +# In that case, new namespace will be created for each user (or workspace). # Is used by OpenShift infra as well to specify Project -# -# BETA It's not fully supported by infra. -# Use che.infra.kubernetes.namespace to configure workspaces' namespace che.infra.kubernetes.namespace.default=-che # Defines if a user is able to specify Kubernetes namespace different from default. # It's NOT RECOMMENDED to configured true without OAuth configured. # Is used by OpenShift infra as well to allows users choose Project # -# BETA It's not fully supported by infra. -# Use che.infra.kubernetes.namespace to configure workspaces' namespace +# BETA This is not currently supported and setting it to true doesn't have any effect. che.infra.kubernetes.namespace.allow_user_defined=false # Defines Kubernetes Service Account name which should be specified to be bound to all workspaces pods. @@ -606,11 +608,17 @@ che.infra.kubernetes.runtimes_consistency_check_period_min=-1 # OpenShift infrastructure reuse most of the Kubernetes configuration attributes. # +# DEPRECATED - please do not change the value of this property otherwise the existing workspaces will loose data. Do not +# set it on new installations. +# # Defines OpenShift namespace in which all workspaces will be created. # If not set, every workspace will be created in a new project, where project name = workspace id # It's possible to use and placeholders (e.g.: che-workspace-). # In that case, new project will be created for each user. OpenShift oauth or service account with # permission to create new projects must be used. +# +# If the project pointed to by this property exists, it will be used for all workspaces. If it does not exist, +# the namespace specified by the che.infra.kubernetes.namespace.default will be created and used. che.infra.openshift.project= # Single port mode wildcard domain host & port. nip.io is used by default diff --git a/deploy/kubernetes/helm/che/templates/configmap.yaml b/deploy/kubernetes/helm/che/templates/configmap.yaml index 233bbe6a28d..8dcbc1d3eef 100644 --- a/deploy/kubernetes/helm/che/templates/configmap.yaml +++ b/deploy/kubernetes/helm/che/templates/configmap.yaml @@ -50,7 +50,7 @@ data: {{- if and .Values.global.multiuser .Values.customOidcUsernameClaim }} CHE_KEYCLOAK_USERNAME__CLAIM: {{ .Values.customOidcUsernameClaim }} {{- end }} - CHE_INFRA_KUBERNETES_NAMESPACE: {{ .Values.global.cheWorkspacesNamespace | quote}} + CHE_INFRA_KUBERNETES_NAMESPACE_DEFAULT: {{ .Values.global.cheWorkspacesNamespace | quote}} CHE_INFRA_KUBERNETES_SERVICE__ACCOUNT__NAME: {{ .Values.global.cheWorkspaceServiceAccount }} CHE_INFRA_KUBERNETES_TRUST__CERTS: "false" CHE_INFRA_KUBERNETES_PVC_STRATEGY: "common" From a19ebb8355f5124e61f359add558eea6cbe43e47 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Fri, 11 Oct 2019 08:57:28 +0200 Subject: [PATCH 07/25] Provide the default cheWorkspaceNamespace in the helm chart Signed-off-by: Lukas Krejci --- deploy/kubernetes/helm/che/values.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deploy/kubernetes/helm/che/values.yaml b/deploy/kubernetes/helm/che/values.yaml index b2970314887..fdc219e71ac 100644 --- a/deploy/kubernetes/helm/che/values.yaml +++ b/deploy/kubernetes/helm/che/values.yaml @@ -53,7 +53,7 @@ global: gitHubClientID: "" gitHubClientSecret: "" pvcClaim: "1Gi" - cheWorkspacesNamespace: "" + cheWorkspacesNamespace: "-che" # Service account name that will be mounted to workspaces pods # Note that: # if `cheWorkspacesNamespace` is configured then service account with configured name will be created by helm chart during deploying Che From 8e242a5bb1ef6bca9c24dea93212800f91e0bcad Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Fri, 11 Oct 2019 14:05:55 +0200 Subject: [PATCH 08/25] Add some debugging to be able to trace namespace resolution in the debug logs. Signed-off-by: Lukas Krejci --- .../namespace/KubernetesNamespaceFactory.java | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) 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 54a06b5fc16..5d84deb784e 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 @@ -61,10 +61,14 @@ public class KubernetesNamespaceFactory { private static final Map> NAMESPACE_NAME_PLACEHOLDERS = new HashMap<>(); + private static final String USERNAME_PLACEHOLDER = ""; + private static final String USERID_PLACEHOLDER = ""; + private static final String WORKSPACEID_PLACEHOLDER = ""; + static { - NAMESPACE_NAME_PLACEHOLDERS.put("", ctx -> ctx.user.getUserName()); - NAMESPACE_NAME_PLACEHOLDERS.put("", ctx -> ctx.user.getUserId()); - NAMESPACE_NAME_PLACEHOLDERS.put("", ctx -> ctx.workspaceId); + NAMESPACE_NAME_PLACEHOLDERS.put(USERNAME_PLACEHOLDER, ctx -> ctx.user.getUserName()); + NAMESPACE_NAME_PLACEHOLDERS.put(USERID_PLACEHOLDER, ctx -> ctx.user.getUserId()); + NAMESPACE_NAME_PLACEHOLDERS.put(WORKSPACEID_PLACEHOLDER, ctx -> ctx.workspaceId); } private final String defaultNamespaceName; @@ -304,13 +308,22 @@ protected String evalNamespaceName(String workspaceId, Subject currentUser) String ns = wkspc.getAttributes().get(Constants.WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE); if (ns != null) { + LOG.debug( + "Found target namespace in workspace attributes. Using namespace {} for workspace {}", + ns, + workspaceId); return ns; } else { String effectiveOldLogicNamespace = - isNullOrEmpty(namespaceName) ? "" : namespaceName; + isNullOrEmpty(namespaceName) ? WORKSPACEID_PLACEHOLDER : namespaceName; String namespace = evalPlaceholders(effectiveOldLogicNamespace, currentUser, workspaceId); - if (!checkNamespaceExists(namespace)) { + if (checkNamespaceExists(namespace)) { + LOG.debug( + "The namespace specified using the legacy config exists: {}. Using it for workspace {}.", + namespace, + workspaceId); + } else { // ok, the namespace pointed to by the legacy config doesn't exist.. that means there can be // no damage done by storing the workspace in the namespace designated by the new way of // doing things... @@ -324,6 +337,11 @@ protected String evalNamespaceName(String workspaceId, Subject currentUser) } namespace = evalPlaceholders(defaultNamespaceName, currentUser, workspaceId); + + LOG.debug( + "Evaluated the namespace for workspace {} using the namespace default to {}", + workspaceId, + namespace); } // Now, believe it or not, the horror continues - since the callers are as of now unaware From 17ba3e4ff58745f412f9faf0a28a33cf15014320 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Fri, 11 Oct 2019 14:06:56 +0200 Subject: [PATCH 09/25] Make sure workspace startup finishers doesn't overwrite the stored infrastructureNamespace attribute. Signed-off-by: Lukas Krejci --- .../workspace/server/WorkspaceManager.java | 48 +++++++++++-------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java index 554e3ea3ae4..12cf4025dd4 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java @@ -415,7 +415,7 @@ public void stopWorkspace(String workspaceId, Map options) .whenComplete( (aVoid, throwable) -> { if (workspace.isTemporary()) { - removeWorkspaceQuietly(workspace); + removeWorkspaceQuietly(workspace.getId()); } }); } @@ -441,13 +441,13 @@ private void startAsync( runtimes .startAsync(workspace, envName, firstNonNull(options, Collections.emptyMap())) - .thenAccept(aVoid -> handleStartupSuccess(workspace)) + .thenAccept(aVoid -> handleStartupSuccess(workspace.getId())) .exceptionally( ex -> { if (workspace.isTemporary()) { - removeWorkspaceQuietly(workspace); + removeWorkspaceQuietly(workspace.getId()); } else { - handleStartupError(workspace, ex.getCause()); + handleStartupError(workspace.getId(), ex.getCause()); } return null; }); @@ -467,11 +467,11 @@ private void checkWorkspaceIsRunningOrStarting(WorkspaceImpl workspace) throws C } } - private void removeWorkspaceQuietly(Workspace workspace) { + private void removeWorkspaceQuietly(String workspaceId) { try { - workspaceDao.remove(workspace.getId()); + workspaceDao.remove(workspaceId); } catch (ServerException x) { - LOG.error("Unable to remove temporary workspace '{}'", workspace.getId()); + LOG.error("Unable to remove temporary workspace '{}'", workspaceId); } } @@ -493,35 +493,41 @@ private WorkspaceImpl normalizeState(WorkspaceImpl workspace, boolean includeRun return workspace; } - private void handleStartupError(Workspace workspace, Throwable t) { - workspace - .getAttributes() - .put( - ERROR_MESSAGE_ATTRIBUTE_NAME, - t instanceof RuntimeException ? t.getCause().getMessage() : t.getMessage()); - workspace.getAttributes().put(STOPPED_ATTRIBUTE_NAME, Long.toString(currentTimeMillis())); - workspace.getAttributes().put(STOPPED_ABNORMALLY_ATTRIBUTE_NAME, Boolean.toString(true)); + private void handleStartupError(String workspaceId, Throwable t) { try { + // we need to reload the workspace because the runtimes might have updated it + Workspace workspace = getWorkspace(workspaceId); + workspace + .getAttributes() + .put( + ERROR_MESSAGE_ATTRIBUTE_NAME, + t instanceof RuntimeException ? t.getCause().getMessage() : t.getMessage()); + workspace.getAttributes().put(STOPPED_ATTRIBUTE_NAME, Long.toString(currentTimeMillis())); + workspace.getAttributes().put(STOPPED_ABNORMALLY_ATTRIBUTE_NAME, Boolean.toString(true)); updateWorkspace(workspace.getId(), workspace); } catch (NotFoundException | ServerException | ValidationException | ConflictException e) { LOG.warn( String.format( "Cannot set error status of the workspace %s. Error is: %s", - workspace.getId(), e.getMessage())); + workspaceId, e.getMessage())); } } - private void handleStartupSuccess(Workspace workspace) { - workspace.getAttributes().remove(STOPPED_ATTRIBUTE_NAME); - workspace.getAttributes().remove(STOPPED_ABNORMALLY_ATTRIBUTE_NAME); - workspace.getAttributes().remove(ERROR_MESSAGE_ATTRIBUTE_NAME); + private void handleStartupSuccess(String workspaceId) { try { + // we need to reload the workspace because the runtimes might have updated it + Workspace workspace = getWorkspace(workspaceId); + + workspace.getAttributes().remove(STOPPED_ATTRIBUTE_NAME); + workspace.getAttributes().remove(STOPPED_ABNORMALLY_ATTRIBUTE_NAME); + workspace.getAttributes().remove(ERROR_MESSAGE_ATTRIBUTE_NAME); + updateWorkspace(workspace.getId(), workspace); } catch (NotFoundException | ServerException | ValidationException | ConflictException e) { LOG.warn( String.format( "Cannot clear error status status of the workspace %s. Error is: %s", - workspace.getId(), e.getMessage())); + workspaceId, e.getMessage())); } } From e30bfa55287603a34127fce97a27b2f6952040fb Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Fri, 11 Oct 2019 14:52:37 +0200 Subject: [PATCH 10/25] Fix the test regression - we need to use a different override for the workspace start to be able to read the workspace repeatedly using mocks. Signed-off-by: Lukas Krejci --- .../che/api/workspace/server/WorkspaceManagerTest.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java index e46956041b7..bc8a7788b1c 100644 --- a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java +++ b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java @@ -589,9 +589,10 @@ public void setsErrorAttributesAfterWorkspaceStartFailed() throws Exception { final WorkspaceImpl workspace = createAndMockWorkspace(workspaceConfig, NAMESPACE_1); mockAnyWorkspaceStartFailed(new ServerException("start failed")); - workspaceManager.startWorkspace(workspaceConfig, workspace.getNamespace(), false, emptyMap()); - verify(workspaceDao).update(workspaceCaptor.capture()); - Workspace ws = workspaceCaptor.getAllValues().get(workspaceCaptor.getAllValues().size() - 1); + workspaceManager.startWorkspace(workspace.getId(), null, null); + // the first update is capturing the start time, the second update is capturing the error + verify(workspaceDao, times(2)).update(workspaceCaptor.capture()); + Workspace ws = workspaceCaptor.getAllValues().get(1); assertNotNull(ws.getAttributes().get(STOPPED_ATTRIBUTE_NAME)); assertTrue(Boolean.valueOf(ws.getAttributes().get(STOPPED_ABNORMALLY_ATTRIBUTE_NAME))); assertEquals(ws.getAttributes().get(ERROR_MESSAGE_ATTRIBUTE_NAME), "start failed"); From 54493899f2c4dcbdb5b8a6f4232db48e7fc845fa Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Fri, 11 Oct 2019 16:24:43 +0200 Subject: [PATCH 11/25] Fix the helm chart to detect workspace namespace with placeholders. Signed-off-by: Lukas Krejci --- deploy/kubernetes/helm/che/templates/deployment.yaml | 4 ++-- deploy/kubernetes/helm/che/templates/exec-role.yaml | 2 +- .../helm/che/templates/workspace-exec-role-binding.yaml | 2 +- .../helm/che/templates/workspace-service-account.yaml | 2 +- .../helm/che/templates/workspace-view-role-binding.yaml | 2 +- deploy/kubernetes/helm/che/templates/workspace-view-role.yaml | 2 +- deploy/kubernetes/helm/che/values.yaml | 4 ++-- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/deploy/kubernetes/helm/che/templates/deployment.yaml b/deploy/kubernetes/helm/che/templates/deployment.yaml index 0fa452f5be3..339fb24e5db 100644 --- a/deploy/kubernetes/helm/che/templates/deployment.yaml +++ b/deploy/kubernetes/helm/che/templates/deployment.yaml @@ -85,9 +85,9 @@ spec: optional: false {{- end }} - # If workspaces are created in different namespace than Che Server's one + # If workspaces are created in a separate precreated namespace # then configure Che Server to propagate TLS secret to workspaces' namespaces - {{- if ne .Release.Namespace .Values.global.cheWorkspacesNamespace }} + {{- if not (contains "<" .Values.global.cheWorkspacesNamespace) }} - name: "CHE_INFRA_KUBERNETES_TLS__CERT" valueFrom: secretKeyRef: diff --git a/deploy/kubernetes/helm/che/templates/exec-role.yaml b/deploy/kubernetes/helm/che/templates/exec-role.yaml index 9801d644bf9..e5b8f05ccfc 100644 --- a/deploy/kubernetes/helm/che/templates/exec-role.yaml +++ b/deploy/kubernetes/helm/che/templates/exec-role.yaml @@ -7,7 +7,7 @@ # SPDX-License-Identifier: EPL-2.0 # -{{- if (.Values.global.cheWorkspacesNamespace) }} +{{- if not (contains "<" .Values.global.cheWorkspacesNamespace) }} kind: Role apiVersion: rbac.authorization.k8s.io/v1beta1 metadata: diff --git a/deploy/kubernetes/helm/che/templates/workspace-exec-role-binding.yaml b/deploy/kubernetes/helm/che/templates/workspace-exec-role-binding.yaml index 0c35fefe0f7..53df54a700a 100644 --- a/deploy/kubernetes/helm/che/templates/workspace-exec-role-binding.yaml +++ b/deploy/kubernetes/helm/che/templates/workspace-exec-role-binding.yaml @@ -7,7 +7,7 @@ # SPDX-License-Identifier: EPL-2.0 # -{{- if (.Values.global.cheWorkspacesNamespace) }} +{{- if not (contains "<" .Values.global.cheWorkspacesNamespace) }} kind: RoleBinding apiVersion: rbac.authorization.k8s.io/v1beta1 metadata: diff --git a/deploy/kubernetes/helm/che/templates/workspace-service-account.yaml b/deploy/kubernetes/helm/che/templates/workspace-service-account.yaml index 310667740a9..52bd1d46f22 100644 --- a/deploy/kubernetes/helm/che/templates/workspace-service-account.yaml +++ b/deploy/kubernetes/helm/che/templates/workspace-service-account.yaml @@ -7,7 +7,7 @@ # SPDX-License-Identifier: EPL-2.0 # -{{- if (.Values.global.cheWorkspacesNamespace) }} +{{- if not (contains "<" .Values.global.cheWorkspacesNamespace) }} kind: ServiceAccount apiVersion: v1 metadata: diff --git a/deploy/kubernetes/helm/che/templates/workspace-view-role-binding.yaml b/deploy/kubernetes/helm/che/templates/workspace-view-role-binding.yaml index 0bcd17c4e2b..5faa5f6b084 100644 --- a/deploy/kubernetes/helm/che/templates/workspace-view-role-binding.yaml +++ b/deploy/kubernetes/helm/che/templates/workspace-view-role-binding.yaml @@ -7,7 +7,7 @@ # SPDX-License-Identifier: EPL-2.0 # -{{- if (.Values.global.cheWorkspacesNamespace) }} +{{- if not (contains "<" .Values.global.cheWorkspacesNamespace) }} kind: RoleBinding apiVersion: rbac.authorization.k8s.io/v1beta1 metadata: diff --git a/deploy/kubernetes/helm/che/templates/workspace-view-role.yaml b/deploy/kubernetes/helm/che/templates/workspace-view-role.yaml index 59e3732c303..4cf8e2bb36c 100644 --- a/deploy/kubernetes/helm/che/templates/workspace-view-role.yaml +++ b/deploy/kubernetes/helm/che/templates/workspace-view-role.yaml @@ -7,7 +7,7 @@ # SPDX-License-Identifier: EPL-2.0 # -{{- if (.Values.global.cheWorkspacesNamespace) }} +{{- if not (contains "<" .Values.global.cheWorkspacesNamespace) }} kind: Role apiVersion: rbac.authorization.k8s.io/v1beta1 metadata: diff --git a/deploy/kubernetes/helm/che/values.yaml b/deploy/kubernetes/helm/che/values.yaml index fdc219e71ac..24ab39322c6 100644 --- a/deploy/kubernetes/helm/che/values.yaml +++ b/deploy/kubernetes/helm/che/values.yaml @@ -56,8 +56,8 @@ global: cheWorkspacesNamespace: "-che" # Service account name that will be mounted to workspaces pods # Note that: - # if `cheWorkspacesNamespace` is configured then service account with configured name will be created by helm chart during deploying Che - # if `cheWorkspacesNamespace` is empty then Che Server creates new namespace for each workspace and ensures that configured SA exists there + # if `cheWorkspacesNamespace` doesn't contain placeholders then service account with configured name will be created by helm chart during deploying Che + # if `cheWorkspacesNamespace` contains placeholders then Che Server creates new namespaces accordingly and ensures that configured SA exists there cheWorkspaceServiceAccount: "che-workspace" # If set, Che will bind the specified cluster role to the workspace service account when creating a workspace. cheWorkspaceClusterRole: "" From 4627ea946328fcaaf96f5f3d4c99c40ce7584c98 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Mon, 14 Oct 2019 10:35:47 +0200 Subject: [PATCH 12/25] The check for forbidden namespace access is required on Kubernetes as well. Signed-off-by: Lukas Krejci --- .../namespace/KubernetesNamespaceFactory.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) 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 5d84deb784e..7e154be1680 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 @@ -357,7 +357,18 @@ protected String evalNamespaceName(String workspaceId, Subject currentUser) } protected boolean checkNamespaceExists(String namespaceName) throws InfrastructureException { - return clientFactory.create().namespaces().withName(namespaceName).get() != null; + try { + return clientFactory.create().namespaces().withName(namespaceName).get() != null; + } catch (KubernetesClientException e) { + if (e.getCode() == 403) { + // 403 means that the project does not exist + // or a user really is not permitted to access it which is Che Server misconfiguration + return false; + } else { + throw new InfrastructureException( + "Error occurred when tried to fetch default project. Cause: " + e.getMessage(), e); + } + } } protected String evalPlaceholders(String namespace, Subject currentUser, String workspaceId) { From 0fbc552f8742e980e1d528bb757a7a260d78560b Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 15 Oct 2019 09:13:26 +0200 Subject: [PATCH 13/25] Do not try to base the logic on whether the workspace is static/predefined or not. This cannot work reliably anymore. Signed-off-by: Lukas Krejci --- .../namespace/KubernetesNamespaceFactory.java | 17 +------- .../namespace/pvc/WorkspacePVCCleaner.java | 2 +- .../KubernetesNamespaceFactoryTest.java | 42 ------------------- .../pvc/WorkspacePVCCleanerTest.java | 11 ----- .../project/OpenShiftProjectFactory.java | 2 +- 5 files changed, 3 insertions(+), 71 deletions(-) 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 7e154be1680..8481f8ca16b 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 @@ -75,7 +75,6 @@ public class KubernetesNamespaceFactory { private final boolean allowUserDefinedNamespaces; private final String namespaceName; - private final boolean isStatic; private final String serviceAccountName; private final String clusterRoleName; private final KubernetesClientFactory clientFactory; @@ -93,7 +92,6 @@ public KubernetesNamespaceFactory( WorkspaceManager workspaceManager) throws ConfigurationException { this.namespaceName = namespaceName; - this.isStatic = !isNullOrEmpty(namespaceName) && hasNoPlaceholders(namespaceName); this.serviceAccountName = serviceAccountName; this.clusterRoleName = clusterRoleName; this.clientFactory = clientFactory; @@ -109,7 +107,6 @@ public KubernetesNamespaceFactory( } // right now allowUserDefinedNamespaces can't be true, but eventually we will implement it. - // noinspection ConstantConditions if (isNullOrEmpty(defaultNamespaceName) && !allowUserDefinedNamespaces) { throw new ConfigurationException( "che.infra.kubernetes.namespace.default or " @@ -117,18 +114,6 @@ public KubernetesNamespaceFactory( } } - private boolean hasNoPlaceholders(String namespaceName) { - return NAMESPACE_NAME_PLACEHOLDERS.keySet().stream().noneMatch(namespaceName::contains); - } - - /** - * True if namespace is the same (static) for all workspaces. False if each workspace will be - * provided with a new namespace or provided for each user when using placeholders. - */ - public boolean isNamespaceStatic() { - return isStatic; - } - /** * Creates a Kubernetes namespace for the specified workspace. * @@ -282,7 +267,7 @@ public KubernetesNamespace create(String workspaceId) throws InfrastructureExcep KubernetesNamespace namespace = doCreateNamespace(workspaceId, namespaceName); namespace.prepare(); - if (!isNamespaceStatic() && !isNullOrEmpty(serviceAccountName)) { + if (!checkNamespaceExists(namespaceName) && !isNullOrEmpty(serviceAccountName)) { // prepare service account for workspace only if account name is configured // and project is not predefined // since predefined project should be prepared during Che deployment diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java index 116850a4dbf..931c95ec411 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java @@ -52,7 +52,7 @@ public WorkspacePVCCleaner( @Inject public void subscribe(EventService eventService) { - if (pvcEnabled && namespaceFactory.isNamespaceStatic()) + if (pvcEnabled) eventService.subscribe( event -> { final Workspace workspace = event.getWorkspace(); 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 55500898199..48be035a334 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 @@ -26,9 +26,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; -import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNull; -import static org.testng.Assert.assertTrue; import io.fabric8.kubernetes.api.model.DoneableNamespace; import io.fabric8.kubernetes.api.model.Namespace; @@ -242,46 +240,6 @@ public void shouldThrownExceptionWhenFailedToGetNamespaces() throws Exception { namespaceFactory.list(); } - @Test - public void shouldReturnTrueIfNamespaceIsNotEmptyOnCheckingIfNamespaceIsPredefined() { - // given - namespaceFactory = - new KubernetesNamespaceFactory( - "predefined", "", "", "che", false, clientFactory, workspaceManager); - - // when - boolean isPredefined = namespaceFactory.isNamespaceStatic(); - - // then - assertTrue(isPredefined); - } - - @Test - public void shouldReturnTrueIfNamespaceIsEmptyOnCheckingIfNamespaceIsPredefined() { - // given - namespaceFactory = - new KubernetesNamespaceFactory("", "", "", "che", false, clientFactory, workspaceManager); - - // when - boolean isPredefined = namespaceFactory.isNamespaceStatic(); - - // then - assertFalse(isPredefined); - } - - @Test - public void shouldReturnTrueIfNamespaceIsNullOnCheckingIfNamespaceIsPredefined() { - // given - namespaceFactory = - new KubernetesNamespaceFactory(null, "", "", "che", false, clientFactory, workspaceManager); - - // when - boolean isPredefined = namespaceFactory.isNamespaceStatic(); - - // then - assertFalse(isPredefined); - } - @Test public void shouldCreateAndPrepareNamespaceWithPredefinedValueIfItIsNotEmpty() throws Exception { // given diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java index 277a7f5f9f1..931b03ad117 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java @@ -50,7 +50,6 @@ public class WorkspacePVCCleanerTest { @BeforeMethod public void setUp() { - when(namespaceFactory.isNamespaceStatic()).thenReturn(true); workspacePVCCleaner = new WorkspacePVCCleaner(true, namespaceFactory, pvcStrategy); } @@ -63,16 +62,6 @@ public void testDoNotSubscribesCleanerWhenPVCDisabled() { verify(eventService, never()).subscribe(any(), eq(WorkspaceRemovedEvent.class)); } - @Test - public void testDoNotSubscribesCleanerWhenPVCEnabledAndNamespaceIsNotPredefined() { - when(namespaceFactory.isNamespaceStatic()).thenReturn(false); - workspacePVCCleaner = spy(new WorkspacePVCCleaner(false, namespaceFactory, pvcStrategy)); - - workspacePVCCleaner.subscribe(eventService); - - verify(eventService, never()).subscribe(any(), eq(WorkspaceRemovedEvent.class)); - } - @Test public void testSubscribesDeleteKubernetesOnWorkspaceRemove() throws Exception { workspacePVCCleaner.subscribe(eventService); 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 c8c95d84d73..ee078c03269 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 @@ -105,7 +105,7 @@ public OpenShiftProject create(String workspaceId) throws InfrastructureExceptio OpenShiftProject osProject = doCreateProject(workspaceId, projectName); osProject.prepare(); - if (!isNamespaceStatic() && !isNullOrEmpty(getServiceAccountName())) { + if (!checkNamespaceExists(projectName) && !isNullOrEmpty(getServiceAccountName())) { // prepare service account for workspace only if account name is configured // and project is not predefined // since predefined project should be prepared during Che deployment From 6c233425b46bdaf495bebfb97564761d3825e192 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 15 Oct 2019 10:05:33 +0200 Subject: [PATCH 14/25] Revert "Do not try to base the logic on whether the workspace is static/predefined" This reverts commit 0fbc552f8742e980e1d528bb757a7a260d78560b. --- .../namespace/KubernetesNamespaceFactory.java | 17 +++++++- .../namespace/pvc/WorkspacePVCCleaner.java | 2 +- .../KubernetesNamespaceFactoryTest.java | 42 +++++++++++++++++++ .../pvc/WorkspacePVCCleanerTest.java | 11 +++++ .../project/OpenShiftProjectFactory.java | 2 +- 5 files changed, 71 insertions(+), 3 deletions(-) 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 8481f8ca16b..7e154be1680 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 @@ -75,6 +75,7 @@ public class KubernetesNamespaceFactory { private final boolean allowUserDefinedNamespaces; private final String namespaceName; + private final boolean isStatic; private final String serviceAccountName; private final String clusterRoleName; private final KubernetesClientFactory clientFactory; @@ -92,6 +93,7 @@ public KubernetesNamespaceFactory( WorkspaceManager workspaceManager) throws ConfigurationException { this.namespaceName = namespaceName; + this.isStatic = !isNullOrEmpty(namespaceName) && hasNoPlaceholders(namespaceName); this.serviceAccountName = serviceAccountName; this.clusterRoleName = clusterRoleName; this.clientFactory = clientFactory; @@ -107,6 +109,7 @@ public KubernetesNamespaceFactory( } // right now allowUserDefinedNamespaces can't be true, but eventually we will implement it. + // noinspection ConstantConditions if (isNullOrEmpty(defaultNamespaceName) && !allowUserDefinedNamespaces) { throw new ConfigurationException( "che.infra.kubernetes.namespace.default or " @@ -114,6 +117,18 @@ public KubernetesNamespaceFactory( } } + private boolean hasNoPlaceholders(String namespaceName) { + return NAMESPACE_NAME_PLACEHOLDERS.keySet().stream().noneMatch(namespaceName::contains); + } + + /** + * True if namespace is the same (static) for all workspaces. False if each workspace will be + * provided with a new namespace or provided for each user when using placeholders. + */ + public boolean isNamespaceStatic() { + return isStatic; + } + /** * Creates a Kubernetes namespace for the specified workspace. * @@ -267,7 +282,7 @@ public KubernetesNamespace create(String workspaceId) throws InfrastructureExcep KubernetesNamespace namespace = doCreateNamespace(workspaceId, namespaceName); namespace.prepare(); - if (!checkNamespaceExists(namespaceName) && !isNullOrEmpty(serviceAccountName)) { + if (!isNamespaceStatic() && !isNullOrEmpty(serviceAccountName)) { // prepare service account for workspace only if account name is configured // and project is not predefined // since predefined project should be prepared during Che deployment diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java index 931c95ec411..116850a4dbf 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java @@ -52,7 +52,7 @@ public WorkspacePVCCleaner( @Inject public void subscribe(EventService eventService) { - if (pvcEnabled) + if (pvcEnabled && namespaceFactory.isNamespaceStatic()) eventService.subscribe( event -> { final Workspace workspace = event.getWorkspace(); 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 48be035a334..55500898199 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 @@ -26,7 +26,9 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNull; +import static org.testng.Assert.assertTrue; import io.fabric8.kubernetes.api.model.DoneableNamespace; import io.fabric8.kubernetes.api.model.Namespace; @@ -240,6 +242,46 @@ public void shouldThrownExceptionWhenFailedToGetNamespaces() throws Exception { namespaceFactory.list(); } + @Test + public void shouldReturnTrueIfNamespaceIsNotEmptyOnCheckingIfNamespaceIsPredefined() { + // given + namespaceFactory = + new KubernetesNamespaceFactory( + "predefined", "", "", "che", false, clientFactory, workspaceManager); + + // when + boolean isPredefined = namespaceFactory.isNamespaceStatic(); + + // then + assertTrue(isPredefined); + } + + @Test + public void shouldReturnTrueIfNamespaceIsEmptyOnCheckingIfNamespaceIsPredefined() { + // given + namespaceFactory = + new KubernetesNamespaceFactory("", "", "", "che", false, clientFactory, workspaceManager); + + // when + boolean isPredefined = namespaceFactory.isNamespaceStatic(); + + // then + assertFalse(isPredefined); + } + + @Test + public void shouldReturnTrueIfNamespaceIsNullOnCheckingIfNamespaceIsPredefined() { + // given + namespaceFactory = + new KubernetesNamespaceFactory(null, "", "", "che", false, clientFactory, workspaceManager); + + // when + boolean isPredefined = namespaceFactory.isNamespaceStatic(); + + // then + assertFalse(isPredefined); + } + @Test public void shouldCreateAndPrepareNamespaceWithPredefinedValueIfItIsNotEmpty() throws Exception { // given diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java index 931b03ad117..277a7f5f9f1 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java @@ -50,6 +50,7 @@ public class WorkspacePVCCleanerTest { @BeforeMethod public void setUp() { + when(namespaceFactory.isNamespaceStatic()).thenReturn(true); workspacePVCCleaner = new WorkspacePVCCleaner(true, namespaceFactory, pvcStrategy); } @@ -62,6 +63,16 @@ public void testDoNotSubscribesCleanerWhenPVCDisabled() { verify(eventService, never()).subscribe(any(), eq(WorkspaceRemovedEvent.class)); } + @Test + public void testDoNotSubscribesCleanerWhenPVCEnabledAndNamespaceIsNotPredefined() { + when(namespaceFactory.isNamespaceStatic()).thenReturn(false); + workspacePVCCleaner = spy(new WorkspacePVCCleaner(false, namespaceFactory, pvcStrategy)); + + workspacePVCCleaner.subscribe(eventService); + + verify(eventService, never()).subscribe(any(), eq(WorkspaceRemovedEvent.class)); + } + @Test public void testSubscribesDeleteKubernetesOnWorkspaceRemove() throws Exception { workspacePVCCleaner.subscribe(eventService); 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 ee078c03269..c8c95d84d73 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 @@ -105,7 +105,7 @@ public OpenShiftProject create(String workspaceId) throws InfrastructureExceptio OpenShiftProject osProject = doCreateProject(workspaceId, projectName); osProject.prepare(); - if (!checkNamespaceExists(projectName) && !isNullOrEmpty(getServiceAccountName())) { + if (!isNamespaceStatic() && !isNullOrEmpty(getServiceAccountName())) { // prepare service account for workspace only if account name is configured // and project is not predefined // since predefined project should be prepared during Che deployment From 8b4e40f936f24ee4458f10ca48b3df32db8c211b Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 15 Oct 2019 12:39:27 +0200 Subject: [PATCH 15/25] The decision on whether a namespace is static or not cannot be made at initialization time because it depends on existance of the workspace pointed to by the legacy prop. Signed-off-by: Lukas Krejci --- .../namespace/KubernetesNamespaceFactory.java | 15 ++- .../KubernetesNamespaceFactoryTest.java | 98 +++++++++++++++++-- 2 files changed, 103 insertions(+), 10 deletions(-) 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 7e154be1680..14dd6aece88 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 @@ -75,7 +75,6 @@ public class KubernetesNamespaceFactory { private final boolean allowUserDefinedNamespaces; private final String namespaceName; - private final boolean isStatic; private final String serviceAccountName; private final String clusterRoleName; private final KubernetesClientFactory clientFactory; @@ -93,7 +92,6 @@ public KubernetesNamespaceFactory( WorkspaceManager workspaceManager) throws ConfigurationException { this.namespaceName = namespaceName; - this.isStatic = !isNullOrEmpty(namespaceName) && hasNoPlaceholders(namespaceName); this.serviceAccountName = serviceAccountName; this.clusterRoleName = clusterRoleName; this.clientFactory = clientFactory; @@ -126,7 +124,18 @@ private boolean hasNoPlaceholders(String namespaceName) { * provided with a new namespace or provided for each user when using placeholders. */ public boolean isNamespaceStatic() { - return isStatic; + try { + if (!isNullOrEmpty(namespaceName) + && hasNoPlaceholders(namespaceName) + && checkNamespaceExists(namespaceName)) { + return true; + } else if (hasNoPlaceholders(defaultNamespaceName)) { + return true; + } + } catch (InfrastructureException e) { + LOG.debug("Failed to check whether workspace namespace is static.", e); + } + return false; } /** 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 55500898199..401104a9e22 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 @@ -79,11 +79,12 @@ public class KubernetesNamespaceFactoryTest { public void setUp() throws Exception { lenient().when(clientFactory.create()).thenReturn(k8sClient); lenient().when(k8sClient.namespaces()).thenReturn(namespaceOperation); - when(workspaceManager.getWorkspace(any())) + lenient() + .when(workspaceManager.getWorkspace(any())) .thenReturn(WorkspaceImpl.builder().setId("1").setAttributes(emptyMap()).build()); - when(namespaceOperation.withName(any())).thenReturn(namespaceResource); - when(namespaceResource.get()).thenReturn(mock(Namespace.class)); + lenient().when(namespaceOperation.withName(any())).thenReturn(namespaceResource); + lenient().when(namespaceResource.get()).thenReturn(mock(Namespace.class)); } @Test( @@ -257,7 +258,8 @@ public void shouldReturnTrueIfNamespaceIsNotEmptyOnCheckingIfNamespaceIsPredefin } @Test - public void shouldReturnTrueIfNamespaceIsEmptyOnCheckingIfNamespaceIsPredefined() { + public void + shouldReturnTrueIfNamespaceIsEmptyAndDefaultNamespaceIsNotEmptyOnCheckingIfNamespaceIsPredefined() { // given namespaceFactory = new KubernetesNamespaceFactory("", "", "", "che", false, clientFactory, workspaceManager); @@ -266,11 +268,12 @@ public void shouldReturnTrueIfNamespaceIsEmptyOnCheckingIfNamespaceIsPredefined( boolean isPredefined = namespaceFactory.isNamespaceStatic(); // then - assertFalse(isPredefined); + assertTrue(isPredefined); } @Test - public void shouldReturnTrueIfNamespaceIsNullOnCheckingIfNamespaceIsPredefined() { + public void + shouldReturnTrueIfNamespaceIsNullAndDefaultNamespaceIsNotEmptyOnCheckingIfNamespaceIsPredefined() { // given namespaceFactory = new KubernetesNamespaceFactory(null, "", "", "che", false, clientFactory, workspaceManager); @@ -278,10 +281,91 @@ public void shouldReturnTrueIfNamespaceIsNullOnCheckingIfNamespaceIsPredefined() // when boolean isPredefined = namespaceFactory.isNamespaceStatic(); + // then + assertTrue(isPredefined); + } + + @Test + public void + shouldReturnFalseIfBothNamespaceAndDefaultNamespaceAreTemplatizedOnCheckingIfNamespaceIsPredefined() { + // given + namespaceFactory = + new KubernetesNamespaceFactory( + "", "", "", "", false, clientFactory, workspaceManager); + + // when + boolean isPredefined = namespaceFactory.isNamespaceStatic(); + // then assertFalse(isPredefined); } + @Test + public void + shouldReturnFalseIfNamespaceIsEmptyAndDefaultNamespaceIsTemplatizedOnCheckingIfNamespaceIsPredefined() { + // given + namespaceFactory = + new KubernetesNamespaceFactory( + "", "", "", "", false, clientFactory, workspaceManager); + + // when + boolean isPredefined = namespaceFactory.isNamespaceStatic(); + + // then + assertFalse(isPredefined); + } + + @Test + public void + shouldReturnFalseIfNamespaceIsNullAndDefaultNamespaceIsTemplatizedOnCheckingIfNamespaceIsPredefined() { + // given + namespaceFactory = + new KubernetesNamespaceFactory( + null, "", "", "", false, clientFactory, workspaceManager); + + // when + boolean isPredefined = namespaceFactory.isNamespaceStatic(); + + // then + assertFalse(isPredefined); + } + + @Test + public void + shouldReturnFalseIfNamespacePointsToNonExistingOneAndDefaultNamespaceIsTemplatizedOnCheckingIfNamespaceIsPredefined() { + // given + namespaceFactory = + new KubernetesNamespaceFactory( + "nonexisting", "", "", "", false, clientFactory, workspaceManager); + + // this is modelling the non-existence of the namespace + when(namespaceResource.get()).thenReturn(null); + + // when + boolean isPredefined = namespaceFactory.isNamespaceStatic(); + + // then + assertFalse(isPredefined); + } + + @Test + public void + shouldReturnTrueIfNamespacePointsToNonExistingOneAndDefaultNamespaceIsNotTemplatizedOnCheckingIfNamespaceIsPredefined() { + // given + namespaceFactory = + new KubernetesNamespaceFactory( + "nonexisting", "", "", "che", false, clientFactory, workspaceManager); + + // this is modelling the non-existence of the namespace + when(namespaceResource.get()).thenReturn(null); + + // when + boolean isPredefined = namespaceFactory.isNamespaceStatic(); + + // then + assertTrue(isPredefined); + } + @Test public void shouldCreateAndPrepareNamespaceWithPredefinedValueIfItIsNotEmpty() throws Exception { // given @@ -349,7 +433,7 @@ public void shouldPrepareWorkspaceServiceAccountIfItIsConfiguredAndNamespaceIsNo namespaceFactory = spy( new KubernetesNamespaceFactory( - "", "serviceAccount", "", "che", false, clientFactory, workspaceManager)); + "", "serviceAccount", "", "", false, clientFactory, workspaceManager)); KubernetesNamespace toReturnNamespace = mock(KubernetesNamespace.class); doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespace(any(), any()); From 2b16123a0ca75b823d775e4c60c1b4486a748074 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 15 Oct 2019 13:15:16 +0200 Subject: [PATCH 16/25] Update the OpenShiftProjectFactoryTest. Signed-off-by: Lukas Krejci --- .../openshift/project/OpenShiftProjectFactoryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 1ddba6d5582..24da0326e39 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 @@ -353,7 +353,7 @@ public void shouldPrepareWorkspaceServiceAccountIfItIsConfiguredAndProjectIsNotP "", "serviceAccount", "", - "che", + "", false, clientFactory, configFactory, From dd3c4acdacfed2889b7ed9b4ec6caf3c89926981 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Fri, 18 Oct 2019 14:56:54 +0200 Subject: [PATCH 17/25] * Move the delete to the KubernetesNamespace/OpenShiftProject to concentrate the handling in one place. * split the isNamespaceStatic() (previously called isPredefined()) into isCreatingNamespaces() and isManagingNamespaces() to separately capture the two usecases in which that single method was previously incorrectly called. Also make these methods workspace-specific to be able to consistently work with the legacy logic across all methods. Signed-off-by: Lukas Krejci --- .../namespace/KubernetesNamespace.java | 27 +++ .../namespace/KubernetesNamespaceFactory.java | 98 +++++++-- .../RemoveNamespaceOnWorkspaceRemove.java | 41 +--- .../namespace/pvc/WorkspacePVCCleaner.java | 7 +- .../KubernetesNamespaceFactoryTest.java | 202 +++++++++--------- .../namespace/KubernetesNamespaceTest.java | 34 +++ .../RemoveNamespaceOnWorkspaceRemoveTest.java | 23 +- .../pvc/WorkspacePVCCleanerTest.java | 37 ++-- .../openshift/project/OpenShiftProject.java | 25 +++ .../project/OpenShiftProjectFactory.java | 16 +- .../RemoveProjectOnWorkspaceRemove.java | 41 +--- .../project/OpenShiftProjectTest.java | 29 +++ .../RemoveProjectOnWorkspaceRemoveTest.java | 26 ++- 13 files changed, 397 insertions(+), 209 deletions(-) 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 16cb46375ed..e761cac875d 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 @@ -11,6 +11,8 @@ */ package org.eclipse.che.workspace.infrastructure.kubernetes.namespace; +import static java.lang.String.format; + import com.google.common.annotations.VisibleForTesting; import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.api.model.DoneableServiceAccount; @@ -117,6 +119,14 @@ void prepare() throws InfrastructureException { } } + void delete() throws InfrastructureException { + String workspaceId = getWorkspaceId(); + String projectName = getName(); + + KubernetesClient client = clientFactory.create(workspaceId); + delete(projectName, client); + } + /** Returns namespace name */ public String getName() { return name; @@ -214,6 +224,23 @@ private void create(String namespaceName, KubernetesClient client) } } + private void delete(String namespaceName, KubernetesClient client) + throws InfrastructureException { + try { + client.namespaces().withName(namespaceName).delete(); + } catch (KubernetesClientException e) { + if (e.getCode() == 404) { + LOG.warn( + format( + "Tried to delete namespace '%s' but it doesn't exist in the cluster.", + namespaceName), + e); + } else { + throw new KubernetesInfrastructureException(e); + } + } + } + /** * Waits few seconds until 'default' service account become available otherwise an infrastructure * exception will be thrown. 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 14dd6aece88..944a4434317 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 @@ -107,7 +107,6 @@ public KubernetesNamespaceFactory( } // right now allowUserDefinedNamespaces can't be true, but eventually we will implement it. - // noinspection ConstantConditions if (isNullOrEmpty(defaultNamespaceName) && !allowUserDefinedNamespaces) { throw new ConfigurationException( "che.infra.kubernetes.namespace.default or " @@ -115,27 +114,66 @@ public KubernetesNamespaceFactory( } } - private boolean hasNoPlaceholders(String namespaceName) { - return NAMESPACE_NAME_PLACEHOLDERS.keySet().stream().noneMatch(namespaceName::contains); + private boolean hasPlaceholders(String namespaceName) { + return namespaceName != null + && NAMESPACE_NAME_PLACEHOLDERS.keySet().stream().anyMatch(namespaceName::contains); + } + + /** True if namespace is potentially created for the workspaces, false otherwise. */ + protected boolean isCreatingNamespaces(String workspaceId) throws InfrastructureException { + // ...namespace | ...namespace exists | ...namespace.default | creating? + // no-placeholders | no | null | error + // no-placeholders | no | no-placeholders | no + // no-placeholders | no | placeholders | yes + // no-placeholders | yes | null | no + // no-placeholders | yes | no-placeholders | no + // no-placeholders | yes | placeholders | no + // placeholders | no | null | error + // placeholders | no | no-placeholders | no + // placeholders | no | placeholders | yes + // placeholders | yes | null | yes + // placeholders | yes | no-placeholders | yes + // placeholders | yes | placeholders | yes + boolean legacyWithPlaceholders = isNullOrEmpty(namespaceName) || hasPlaceholders(namespaceName); + boolean legacyExists = + checkNamespaceExists( + resolveLegacyNamespaceName(EnvironmentContext.getCurrent().getSubject(), workspaceId)); + + if (isNullOrEmpty(defaultNamespaceName) && !legacyExists) { + throw new InfrastructureException( + "Cannot determine whether a new namespace and service account should be" + + " created for workspace %s. There is no pre-existing workspace namespace to be found using the legacy" + + " `che.infra.kubernetes.namespace` property yet the `che.infra.kubernetes.namespace.default` property" + + " is undefined."); + } + + boolean defaultHasPlaceholders = hasPlaceholders(defaultNamespaceName); + + return (legacyWithPlaceholders && legacyExists) || (!legacyExists && defaultHasPlaceholders); } /** - * True if namespace is the same (static) for all workspaces. False if each workspace will be - * provided with a new namespace or provided for each user when using placeholders. + * @return true if the namespaces are fully managed by Che (e.g. created, used and deleted), false + * otherwise */ - public boolean isNamespaceStatic() { - try { - if (!isNullOrEmpty(namespaceName) - && hasNoPlaceholders(namespaceName) - && checkNamespaceExists(namespaceName)) { - return true; - } else if (hasNoPlaceholders(defaultNamespaceName)) { - return true; - } - } catch (InfrastructureException e) { - LOG.debug("Failed to check whether workspace namespace is static.", e); + public boolean isManagingNamespaces(String workspaceId) throws InfrastructureException { + boolean canBeManaged = + isNullOrEmpty(namespaceName) + ? defaultNamespaceName != null && defaultNamespaceName.contains(WORKSPACEID_PLACEHOLDER) + : namespaceName.contains(WORKSPACEID_PLACEHOLDER); + + if (!canBeManaged) { + return false; + } + + if (isNullOrEmpty(namespaceName)) { + // if we're using the new logic, we don't have to check if the namespace exists + return true; + } else { + // the legacy prop is only applied if the namespace actually exists... + return checkNamespaceExists( + resolveLegacyNamespaceName(EnvironmentContext.getCurrent().getSubject(), workspaceId)); } - return false; } /** @@ -291,7 +329,7 @@ public KubernetesNamespace create(String workspaceId) throws InfrastructureExcep KubernetesNamespace namespace = doCreateNamespace(workspaceId, namespaceName); namespace.prepare(); - if (!isNamespaceStatic() && !isNullOrEmpty(serviceAccountName)) { + if (isCreatingNamespaces(workspaceId) && !isNullOrEmpty(serviceAccountName)) { // prepare service account for workspace only if account name is configured // and project is not predefined // since predefined project should be prepared during Che deployment @@ -303,6 +341,19 @@ public KubernetesNamespace create(String workspaceId) throws InfrastructureExcep return namespace; } + public void delete(String workspaceId) throws InfrastructureException { + final String namespaceName; + try { + namespaceName = evalNamespaceName(workspaceId, EnvironmentContext.getCurrent().getSubject()); + } catch (NotFoundException | ServerException | ConflictException | ValidationException e) { + throw new InfrastructureException( + format("Failed to determine the namespace of the workspace %s", workspaceId), e); + } + + KubernetesNamespace namespace = doCreateNamespace(workspaceId, namespaceName); + namespace.delete(); + } + protected String evalNamespaceName(String workspaceId, Subject currentUser) throws NotFoundException, ServerException, InfrastructureException, ConflictException, ValidationException { @@ -323,9 +374,7 @@ protected String evalNamespaceName(String workspaceId, Subject currentUser) workspaceId); return ns; } else { - String effectiveOldLogicNamespace = - isNullOrEmpty(namespaceName) ? WORKSPACEID_PLACEHOLDER : namespaceName; - String namespace = evalPlaceholders(effectiveOldLogicNamespace, currentUser, workspaceId); + String namespace = resolveLegacyNamespaceName(currentUser, workspaceId); if (checkNamespaceExists(namespace)) { LOG.debug( @@ -365,6 +414,13 @@ protected String evalNamespaceName(String workspaceId, Subject currentUser) } } + private String resolveLegacyNamespaceName(Subject user, String workspaceId) { + String effectiveOldLogicNamespace = + isNullOrEmpty(namespaceName) ? WORKSPACEID_PLACEHOLDER : namespaceName; + + return evalPlaceholders(effectiveOldLogicNamespace, user, workspaceId); + } + protected boolean checkNamespaceExists(String namespaceName) throws InfrastructureException { try { return clientFactory.create().namespaces().withName(namespaceName).get() != null; diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemove.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemove.java index 14815a4fdcc..743e3a3ba3f 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemove.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemove.java @@ -11,20 +11,12 @@ */ package org.eclipse.che.workspace.infrastructure.kubernetes.namespace; -import static com.google.common.base.Strings.isNullOrEmpty; - -import com.google.common.annotations.VisibleForTesting; import com.google.inject.Inject; import com.google.inject.Singleton; -import io.fabric8.kubernetes.client.KubernetesClientException; -import javax.inject.Named; import org.eclipse.che.api.core.notification.EventService; import org.eclipse.che.api.core.notification.EventSubscriber; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.api.workspace.shared.event.WorkspaceRemovedEvent; -import org.eclipse.che.commons.annotation.Nullable; -import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory; -import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesInfrastructureException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -37,45 +29,30 @@ public class RemoveNamespaceOnWorkspaceRemove implements EventSubscriber { private static final Logger LOG = LoggerFactory.getLogger(RemoveNamespaceOnWorkspaceRemove.class); - private final KubernetesClientFactory clientFactory; - private final String namespaceName; + private final KubernetesNamespaceFactory namespaceFactory; @Inject - public RemoveNamespaceOnWorkspaceRemove( - @Nullable @Named("che.infra.kubernetes.namespace") String namespaceName, - KubernetesClientFactory clientFactory) { - this.namespaceName = namespaceName; - this.clientFactory = clientFactory; + public RemoveNamespaceOnWorkspaceRemove(KubernetesNamespaceFactory namespaceFactory) { + this.namespaceFactory = namespaceFactory; } @Inject public void subscribe(EventService eventService) { - if (isNullOrEmpty(namespaceName)) { - eventService.subscribe(this); - } + eventService.subscribe(this); } @Override public void onEvent(WorkspaceRemovedEvent event) { + String workspaceId = event.getWorkspace().getId(); try { - doRemoveNamespace(event.getWorkspace().getId()); + if (namespaceFactory.isManagingNamespaces(workspaceId)) { + namespaceFactory.delete(workspaceId); + } } catch (InfrastructureException e) { LOG.warn( "Fail to remove Kubernetes namespace for workspace with id {}. Cause: {}", - event.getWorkspace().getId(), + workspaceId, e.getMessage()); } } - - @VisibleForTesting - void doRemoveNamespace(String namespaceName) throws InfrastructureException { - try { - clientFactory.create(namespaceName).namespaces().withName(namespaceName).delete(); - } catch (KubernetesClientException e) { - if (!(e.getCode() == 403)) { - throw new KubernetesInfrastructureException(e); - } - // namespace doesn't exist - } - } } diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java index 116850a4dbf..21a310f7672 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java @@ -52,11 +52,15 @@ public WorkspacePVCCleaner( @Inject public void subscribe(EventService eventService) { - if (pvcEnabled && namespaceFactory.isNamespaceStatic()) + if (pvcEnabled) { eventService.subscribe( event -> { final Workspace workspace = event.getWorkspace(); try { + if (namespaceFactory.isManagingNamespaces(workspace.getId())) { + // the namespaces of managed workspaces are deleted, so no need to do the cleanup + return; + } strategy.cleanup(workspace); } catch (InfrastructureException ex) { LOG.error( @@ -66,5 +70,6 @@ public void subscribe(EventService eventService) { } }, WorkspaceRemovedEvent.class); + } } } 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 401104a9e22..8d0eb31d6a2 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 @@ -11,6 +11,7 @@ */ package org.eclipse.che.workspace.infrastructure.kubernetes.namespace; +import static java.lang.String.format; import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; @@ -28,7 +29,7 @@ import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNull; -import static org.testng.Assert.assertTrue; +import static org.testng.Assert.fail; import io.fabric8.kubernetes.api.model.DoneableNamespace; import io.fabric8.kubernetes.api.model.Namespace; @@ -51,6 +52,7 @@ import org.mockito.Mock; import org.mockito.testng.MockitoTestNGListener; import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; import org.testng.annotations.Listeners; import org.testng.annotations.Test; @@ -243,127 +245,125 @@ public void shouldThrownExceptionWhenFailedToGetNamespaces() throws Exception { namespaceFactory.list(); } - @Test - public void shouldReturnTrueIfNamespaceIsNotEmptyOnCheckingIfNamespaceIsPredefined() { - // given - namespaceFactory = - new KubernetesNamespaceFactory( - "predefined", "", "", "che", false, clientFactory, workspaceManager); - - // when - boolean isPredefined = namespaceFactory.isNamespaceStatic(); - - // then - assertTrue(isPredefined); - } - - @Test - public void - shouldReturnTrueIfNamespaceIsEmptyAndDefaultNamespaceIsNotEmptyOnCheckingIfNamespaceIsPredefined() { - // given - namespaceFactory = - new KubernetesNamespaceFactory("", "", "", "che", false, clientFactory, workspaceManager); - - // when - boolean isPredefined = namespaceFactory.isNamespaceStatic(); + @DataProvider + public static Object[][] creatingNamespaceConditions() { + // Whether or not the factory potentially creates a namespace depends on the 3 conditions: + // 1) the value of the legacy property 'che.infra.kubernetes.namespace' + // 2) legacy property pointing to the existing namespace + // 3) value of the new property 'che.infra.kubernetes.namespace.default' + // the output is either true, false, or error (represented as a null here) + + // this is the truth table we need to follow + // ...namespace | ...namespace exists | ...namespace.default | creating? + // no-placeholders | no | null | error + // no-placeholders | no | no-placeholders | no + // no-placeholders | no | placeholders | yes + // no-placeholders | yes | null | no + // no-placeholders | yes | no-placeholders | no + // no-placeholders | yes | placeholders | no + // placeholders | no | null | error + // placeholders | no | no-placeholders | no + // placeholders | no | placeholders | yes + // placeholders | yes | null | yes + // placeholders | yes | no-placeholders | yes + // placeholders | yes | placeholders | yes + + // additionally, we want to test that if the legacy property is null, it behaves exactly + // the same as having a placeholder, because it should default to + return new Object[][] { + new Object[] {"some", false, null, null}, + new Object[] {"some", false, "some", false}, + new Object[] {"some", false, "", true}, + new Object[] {"some", true, null, false}, + new Object[] {"some", true, "some", false}, + new Object[] {"some", true, "", false}, + new Object[] {"", false, null, null}, + new Object[] {"", false, "some", false}, + new Object[] {"", false, "", true}, + new Object[] {"", true, null, true}, + new Object[] {"", true, "some", true}, + new Object[] {"", true, "", true}, + new Object[] {null, false, null, null}, + new Object[] {null, false, "some", false}, + new Object[] {null, false, "", true}, + new Object[] {null, true, null, true}, + new Object[] {null, true, "some", true}, + new Object[] {null, true, "", true}, + }; + } + + @Test(dataProvider = "creatingNamespaceConditions") + public void shouldDetermineWhenNamespaceCanBeCreated( + String legacyProperty, + boolean legacyNamespaceExists, + String namespaceProperty, + Boolean expectedOutcome) { - // then - assertTrue(isPredefined); - } - - @Test - public void - shouldReturnTrueIfNamespaceIsNullAndDefaultNamespaceIsNotEmptyOnCheckingIfNamespaceIsPredefined() { - // given - namespaceFactory = - new KubernetesNamespaceFactory(null, "", "", "che", false, clientFactory, workspaceManager); - - // when - boolean isPredefined = namespaceFactory.isNamespaceStatic(); - - // then - assertTrue(isPredefined); - } - - @Test - public void - shouldReturnFalseIfBothNamespaceAndDefaultNamespaceAreTemplatizedOnCheckingIfNamespaceIsPredefined() { // given namespaceFactory = new KubernetesNamespaceFactory( - "", "", "", "", false, clientFactory, workspaceManager); + legacyProperty, "", "", namespaceProperty, true, clientFactory, workspaceManager); - // when - boolean isPredefined = namespaceFactory.isNamespaceStatic(); - - // then - assertFalse(isPredefined); - } - - @Test - public void - shouldReturnFalseIfNamespaceIsEmptyAndDefaultNamespaceIsTemplatizedOnCheckingIfNamespaceIsPredefined() { - // given - namespaceFactory = - new KubernetesNamespaceFactory( - "", "", "", "", false, clientFactory, workspaceManager); + Namespace existingLegacyNamespace = legacyNamespaceExists ? mock(Namespace.class) : null; + when(namespaceResource.get()).thenReturn(existingLegacyNamespace); // when - boolean isPredefined = namespaceFactory.isNamespaceStatic(); + Boolean result; + try { + result = namespaceFactory.isCreatingNamespaces("123"); + } catch (InfrastructureException e) { + // this can happen and we test for it below... + result = null; + } // then - assertFalse(isPredefined); + assertEquals(result, expectedOutcome); } - @Test - public void - shouldReturnFalseIfNamespaceIsNullAndDefaultNamespaceIsTemplatizedOnCheckingIfNamespaceIsPredefined() { - // given - namespaceFactory = - new KubernetesNamespaceFactory( - null, "", "", "", false, clientFactory, workspaceManager); + @Test(dataProvider = "creatingNamespaceConditions") + public void testNotManagingNamespacesWheneverNotCreatingThem( + String legacyProperty, + boolean legacyNamespaceExists, + String namespaceProperty, + Boolean expectedCreating) + throws InfrastructureException { - // when - boolean isPredefined = namespaceFactory.isNamespaceStatic(); - - // then - assertFalse(isPredefined); - } + // it is possible that we are creating namespaces that we are not fully managing, e.g. + // namespaces are created but not fully deleted afterwards. We just clean them. + // However, whenever a namespace is NOT being created, we should never claim we're managing the + // namespace. + // This is what this test asserts. - @Test - public void - shouldReturnFalseIfNamespacePointsToNonExistingOneAndDefaultNamespaceIsTemplatizedOnCheckingIfNamespaceIsPredefined() { - // given - namespaceFactory = - new KubernetesNamespaceFactory( - "nonexisting", "", "", "", false, clientFactory, workspaceManager); - - // this is modelling the non-existence of the namespace - when(namespaceResource.get()).thenReturn(null); - - // when - boolean isPredefined = namespaceFactory.isNamespaceStatic(); - - // then - assertFalse(isPredefined); - } - - @Test - public void - shouldReturnTrueIfNamespacePointsToNonExistingOneAndDefaultNamespaceIsNotTemplatizedOnCheckingIfNamespaceIsPredefined() { // given namespaceFactory = new KubernetesNamespaceFactory( - "nonexisting", "", "", "che", false, clientFactory, workspaceManager); + legacyProperty, "", "", namespaceProperty, true, clientFactory, workspaceManager); - // this is modelling the non-existence of the namespace - when(namespaceResource.get()).thenReturn(null); + Namespace existingLegacyNamespace = legacyNamespaceExists ? mock(Namespace.class) : null; + when(namespaceResource.get()).thenReturn(existingLegacyNamespace); // when - boolean isPredefined = namespaceFactory.isNamespaceStatic(); + boolean creating; + try { + creating = namespaceFactory.isCreatingNamespaces("123"); + } catch (InfrastructureException e) { + // if we can't determine whether we're potentially creating a namespace, we shouldn't claim + // we're managing it + if (expectedCreating != null) { + fail("Shouldn't have failed."); + } + creating = false; + } + boolean managing = namespaceFactory.isManagingNamespaces("123"); // then - assertTrue(isPredefined); + if (!creating) { + assertFalse( + managing, + format( + "legacyProp=%s, legacyExists=%s, namespaceProp=%s, expectedCreating=%s", + legacyProperty, legacyNamespaceExists, namespaceProperty, expectedCreating)); + } } @Test 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 6cfa7eeb902..ec43d7db513 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 @@ -18,6 +18,7 @@ import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; @@ -98,11 +99,16 @@ public void setUp() throws Exception { @Test public void testKubernetesNamespacePreparingWhenNamespaceExists() throws Exception { // given + MetadataNested namespaceMeta = prepareCreateNamespaceRequest(); + prepareNamespace(NAMESPACE); KubernetesNamespace namespace = new KubernetesNamespace(clientFactory, NAMESPACE, WORKSPACE_ID); // when namespace.prepare(); + + // then + verify(namespaceMeta, never()).withName(NAMESPACE); } @Test @@ -216,6 +222,34 @@ public void testStopsWaitingServiceAccountEventJustAfterEventReceived() throws E verify(serviceAccountResource).watch(any()); } + @Test + public void testDeletesExistingNamespace() throws Exception { + // given + KubernetesNamespace namespace = new KubernetesNamespace(clientFactory, NAMESPACE, WORKSPACE_ID); + Resource resource = prepareNamespaceResource(NAMESPACE); + + // when + namespace.delete(); + + // then + verify(resource).delete(); + } + + @Test + public void testDoesntFailIfDeletedNamespaceDoesntExist() throws Exception { + // given + KubernetesNamespace namespace = new KubernetesNamespace(clientFactory, NAMESPACE, WORKSPACE_ID); + Resource resource = prepareNamespaceResource(NAMESPACE); + when(resource.delete()).thenThrow(new KubernetesClientException("err", 404, null)); + + // when + namespace.delete(); + + // then + verify(resource).delete(); + // and no exception is thrown + } + private MetadataNested prepareCreateNamespaceRequest() { DoneableNamespace namespace = mock(DoneableNamespace.class); MetadataNested metadataNested = mock(MetadataNested.class); diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemoveTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemoveTest.java index fbe81882a75..3848ddc5f6f 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemoveTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemoveTest.java @@ -11,9 +11,11 @@ */ package org.eclipse.che.workspace.infrastructure.kubernetes.namespace; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -38,14 +40,15 @@ public class RemoveNamespaceOnWorkspaceRemoveTest { private static final String WORKSPACE_ID = "workspace123"; @Mock private Workspace workspace; + @Mock private KubernetesNamespaceFactory namespaceFactory; private RemoveNamespaceOnWorkspaceRemove removeNamespaceOnWorkspaceRemove; @BeforeMethod public void setUp() throws Exception { - removeNamespaceOnWorkspaceRemove = spy(new RemoveNamespaceOnWorkspaceRemove(null, null)); + removeNamespaceOnWorkspaceRemove = spy(new RemoveNamespaceOnWorkspaceRemove(namespaceFactory)); - doNothing().when(removeNamespaceOnWorkspaceRemove).doRemoveNamespace(anyString()); + doNothing().when(namespaceFactory).delete(anyString()); when(workspace.getId()).thenReturn(WORKSPACE_ID); } @@ -60,9 +63,21 @@ public void shouldSubscribeListenerToEventService() { } @Test - public void shouldRemoveNamespaceOnWorkspaceRemovedEvent() throws Exception { + public void shouldRemoveNamespaceOnWorkspaceRemovedEventIfNamespaceIsManaged() throws Exception { + when(namespaceFactory.isManagingNamespaces(any())).thenReturn(true); + + removeNamespaceOnWorkspaceRemove.onEvent(new WorkspaceRemovedEvent(workspace)); + + verify(namespaceFactory).delete(WORKSPACE_ID); + } + + @Test + public void shouldNotRemoveNamespaceOnWorkspaceRemovedEventIfNamespaceIsNotManaged() + throws Exception { + when(namespaceFactory.isManagingNamespaces(any())).thenReturn(false); + removeNamespaceOnWorkspaceRemove.onEvent(new WorkspaceRemovedEvent(workspace)); - verify(removeNamespaceOnWorkspaceRemove).doRemoveNamespace(WORKSPACE_ID); + verify(namespaceFactory, never()).delete(any()); } } diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java index 277a7f5f9f1..f56e4c778b4 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java @@ -49,13 +49,13 @@ public class WorkspacePVCCleanerTest { private WorkspacePVCCleaner workspacePVCCleaner; @BeforeMethod - public void setUp() { - when(namespaceFactory.isNamespaceStatic()).thenReturn(true); + public void setUp() throws Exception { + when(namespaceFactory.isManagingNamespaces(any())).thenReturn(false); workspacePVCCleaner = new WorkspacePVCCleaner(true, namespaceFactory, pvcStrategy); } @Test - public void testDoNotSubscribesCleanerWhenPVCDisabled() { + public void testDoNotSubscribesCleanerWhenPVCDisabled() throws Exception { workspacePVCCleaner = spy(new WorkspacePVCCleaner(false, namespaceFactory, pvcStrategy)); workspacePVCCleaner.subscribe(eventService); @@ -64,24 +64,36 @@ public void testDoNotSubscribesCleanerWhenPVCDisabled() { } @Test - public void testDoNotSubscribesCleanerWhenPVCEnabledAndNamespaceIsNotPredefined() { - when(namespaceFactory.isNamespaceStatic()).thenReturn(false); - workspacePVCCleaner = spy(new WorkspacePVCCleaner(false, namespaceFactory, pvcStrategy)); - + public void testSubscribesDeleteKubernetesOnWorkspaceRemove() throws Exception { workspacePVCCleaner.subscribe(eventService); - verify(eventService, never()).subscribe(any(), eq(WorkspaceRemovedEvent.class)); + verify(eventService).subscribe(any(), eq(WorkspaceRemovedEvent.class)); } @Test - public void testSubscribesDeleteKubernetesOnWorkspaceRemove() throws Exception { + public void testInvokeCleanupWhenWorkspaceRemovedEventPublishedAndNamespaceIsManaged() + throws Exception { + doAnswer( + invocationOnMock -> { + final EventSubscriber argument = + invocationOnMock.getArgument(0); + final WorkspaceRemovedEvent event = mock(WorkspaceRemovedEvent.class); + when(event.getWorkspace()).thenReturn(workspace); + argument.onEvent(event); + return invocationOnMock; + }) + .when(eventService) + .subscribe(any(), eq(WorkspaceRemovedEvent.class)); + when(namespaceFactory.isManagingNamespaces(any())).thenReturn(true); + workspacePVCCleaner.subscribe(eventService); - verify(eventService).subscribe(any(), eq(WorkspaceRemovedEvent.class)); + verify(pvcStrategy).cleanup(workspace); } @Test - public void testInvokeCleanupWhenWorkspaceRemovedEventPublished() throws Exception { + public void testDontInvokeCleanupWhenWorkspaceRemovedEventPublishedAndNamespaceIsNotManaged() + throws Exception { doAnswer( invocationOnMock -> { final EventSubscriber argument = @@ -93,10 +105,11 @@ public void testInvokeCleanupWhenWorkspaceRemovedEventPublished() throws Excepti }) .when(eventService) .subscribe(any(), eq(WorkspaceRemovedEvent.class)); + when(namespaceFactory.isManagingNamespaces(any())).thenReturn(true); workspacePVCCleaner.subscribe(eventService); - verify(pvcStrategy).cleanup(workspace); + verify(pvcStrategy, never()).cleanup(workspace); } @Test 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 c6b148fbd76..f4ae38f1e05 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 @@ -11,6 +11,8 @@ */ package org.eclipse.che.workspace.infrastructure.openshift.project; +import static java.lang.String.format; + import com.google.common.annotations.VisibleForTesting; import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.KubernetesClientException; @@ -94,6 +96,14 @@ void prepare() throws InfrastructureException { } } + void delete() throws InfrastructureException { + String workspaceId = getWorkspaceId(); + String projectName = getName(); + + OpenShiftClient osClient = clientFactory.createOC(workspaceId); + delete(projectName, osClient); + } + /** Returns object for managing {@link Route} instances inside project. */ public OpenShiftRoutes routes() { return routes; @@ -128,6 +138,21 @@ private void create(String projectName, OpenShiftClient osClient) throws Infrast } } + private void delete(String projectName, OpenShiftClient osClient) throws InfrastructureException { + try { + osClient.projects().withName(projectName).delete(); + } catch (KubernetesClientException e) { + if (e.getCode() == 404) { + LOG.warn( + format( + "Tried to delete project '%s' but it doesn't exist in the cluster.", projectName), + e); + } else { + throw new KubernetesInfrastructureException(e); + } + } + } + private Project get(String projectName, OpenShiftClient client) throws InfrastructureException { try { return client.projects().withName(projectName).get(); 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 c8c95d84d73..7490017e70a 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 @@ -105,7 +105,7 @@ public OpenShiftProject create(String workspaceId) throws InfrastructureExceptio OpenShiftProject osProject = doCreateProject(workspaceId, projectName); osProject.prepare(); - if (!isNamespaceStatic() && !isNullOrEmpty(getServiceAccountName())) { + if (isCreatingNamespaces(workspaceId) && !isNullOrEmpty(getServiceAccountName())) { // prepare service account for workspace only if account name is configured // and project is not predefined // since predefined project should be prepared during Che deployment @@ -117,6 +117,20 @@ public OpenShiftProject create(String workspaceId) throws InfrastructureExceptio return osProject; } + @Override + public void delete(String workspaceId) throws InfrastructureException { + String projectName; + try { + projectName = evalNamespaceName(workspaceId, EnvironmentContext.getCurrent().getSubject()); + } catch (NotFoundException | ServerException | ConflictException | ValidationException e) { + throw new InfrastructureException( + format("Failed to evaluate the project name to use for workspace %s.", workspaceId), e); + } + + OpenShiftProject osProject = doCreateProject(workspaceId, projectName); + osProject.delete(); + } + @VisibleForTesting @Override protected String evalNamespaceName(String workspaceId, Subject currentUser) diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/RemoveProjectOnWorkspaceRemove.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/RemoveProjectOnWorkspaceRemove.java index 8af34840e7b..649c9da533f 100644 --- a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/RemoveProjectOnWorkspaceRemove.java +++ b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/RemoveProjectOnWorkspaceRemove.java @@ -11,20 +11,12 @@ */ package org.eclipse.che.workspace.infrastructure.openshift.project; -import static com.google.common.base.Strings.isNullOrEmpty; - -import com.google.common.annotations.VisibleForTesting; import com.google.inject.Inject; import com.google.inject.Singleton; -import io.fabric8.kubernetes.client.KubernetesClientException; -import javax.inject.Named; import org.eclipse.che.api.core.notification.EventService; import org.eclipse.che.api.core.notification.EventSubscriber; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.api.workspace.shared.event.WorkspaceRemovedEvent; -import org.eclipse.che.commons.annotation.Nullable; -import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesInfrastructureException; -import org.eclipse.che.workspace.infrastructure.openshift.OpenShiftClientFactory; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -38,45 +30,30 @@ public class RemoveProjectOnWorkspaceRemove implements EventSubscriber Date: Fri, 18 Oct 2019 17:36:24 +0200 Subject: [PATCH 18/25] Fix the tests and actually make them test anything.. Signed-off-by: Lukas Krejci --- .../RemoveNamespaceOnWorkspaceRemoveTest.java | 4 +- .../pvc/WorkspacePVCCleanerTest.java | 55 +++++-------------- 2 files changed, 16 insertions(+), 43 deletions(-) diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemoveTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemoveTest.java index 3848ddc5f6f..efdef79ae1a 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemoveTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemoveTest.java @@ -13,7 +13,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; @@ -48,7 +48,7 @@ public class RemoveNamespaceOnWorkspaceRemoveTest { public void setUp() throws Exception { removeNamespaceOnWorkspaceRemove = spy(new RemoveNamespaceOnWorkspaceRemove(namespaceFactory)); - doNothing().when(namespaceFactory).delete(anyString()); + lenient().doNothing().when(namespaceFactory).delete(anyString()); when(workspace.getId()).thenReturn(WORKSPACE_ID); } diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java index f56e4c778b4..f8a5e3a7aa9 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java @@ -13,9 +13,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; @@ -23,7 +21,6 @@ import org.eclipse.che.api.core.model.workspace.Workspace; import org.eclipse.che.api.core.notification.EventService; -import org.eclipse.che.api.core.notification.EventSubscriber; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.api.workspace.shared.event.WorkspaceRemovedEvent; import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory; @@ -42,9 +39,10 @@ public class WorkspacePVCCleanerTest { @Mock private WorkspaceVolumesStrategy pvcStrategy; - @Mock private EventService eventService; + private EventService eventService; @Mock private KubernetesNamespaceFactory namespaceFactory; @Mock private Workspace workspace; + @Mock WorkspaceRemovedEvent event; private WorkspacePVCCleaner workspacePVCCleaner; @@ -52,6 +50,10 @@ public class WorkspacePVCCleanerTest { public void setUp() throws Exception { when(namespaceFactory.isManagingNamespaces(any())).thenReturn(false); workspacePVCCleaner = new WorkspacePVCCleaner(true, namespaceFactory, pvcStrategy); + when(workspace.getId()).thenReturn("123"); + when(event.getWorkspace()).thenReturn(workspace); + + eventService = spy(new EventService()); } @Test @@ -71,64 +73,35 @@ public void testSubscribesDeleteKubernetesOnWorkspaceRemove() throws Exception { } @Test - public void testInvokeCleanupWhenWorkspaceRemovedEventPublishedAndNamespaceIsManaged() + public void testInvokeCleanupWhenWorkspaceRemovedEventPublishedAndNamespaceIsNotManaged() throws Exception { - doAnswer( - invocationOnMock -> { - final EventSubscriber argument = - invocationOnMock.getArgument(0); - final WorkspaceRemovedEvent event = mock(WorkspaceRemovedEvent.class); - when(event.getWorkspace()).thenReturn(workspace); - argument.onEvent(event); - return invocationOnMock; - }) - .when(eventService) - .subscribe(any(), eq(WorkspaceRemovedEvent.class)); - when(namespaceFactory.isManagingNamespaces(any())).thenReturn(true); - workspacePVCCleaner.subscribe(eventService); + eventService.publish(event); + verify(pvcStrategy).cleanup(workspace); } @Test - public void testDontInvokeCleanupWhenWorkspaceRemovedEventPublishedAndNamespaceIsNotManaged() + public void testNotInvokeCleanupWhenWorkspaceRemovedEventPublishedAndNamespaceIsManaged() throws Exception { - doAnswer( - invocationOnMock -> { - final EventSubscriber argument = - invocationOnMock.getArgument(0); - final WorkspaceRemovedEvent event = mock(WorkspaceRemovedEvent.class); - when(event.getWorkspace()).thenReturn(workspace); - argument.onEvent(event); - return invocationOnMock; - }) - .when(eventService) - .subscribe(any(), eq(WorkspaceRemovedEvent.class)); when(namespaceFactory.isManagingNamespaces(any())).thenReturn(true); workspacePVCCleaner.subscribe(eventService); + eventService.publish(event); + verify(pvcStrategy, never()).cleanup(workspace); } @Test public void testDoNotRethrowExceptionWhenErrorOnCleanupOccurs() throws Exception { - doAnswer( - invocationOnMock -> { - final EventSubscriber argument = - invocationOnMock.getArgument(0); - final WorkspaceRemovedEvent event = mock(WorkspaceRemovedEvent.class); - when(event.getWorkspace()).thenReturn(workspace); - argument.onEvent(event); - return invocationOnMock; - }) - .when(eventService) - .subscribe(any(), eq(WorkspaceRemovedEvent.class)); doThrow(InfrastructureException.class).when(pvcStrategy).cleanup(workspace); workspacePVCCleaner.subscribe(eventService); + eventService.publish(event); + verify(pvcStrategy).cleanup(workspace); } } From ff782f87f4f1d3dfd8d9998274a92e07502b0d7f Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Mon, 21 Oct 2019 17:51:32 +0200 Subject: [PATCH 19/25] Improve error handling while the namespace/project is being deleted. Signed-off-by: Lukas Krejci --- .../kubernetes/namespace/KubernetesNamespace.java | 6 +++--- .../namespace/KubernetesNamespaceTest.java | 15 +++++++++++++++ .../openshift/project/OpenShiftProject.java | 2 ++ .../openshift/project/OpenShiftProjectTest.java | 15 +++++++++++++++ 4 files changed, 35 insertions(+), 3 deletions(-) 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 e761cac875d..805630d5417 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 @@ -120,11 +120,9 @@ void prepare() throws InfrastructureException { } void delete() throws InfrastructureException { - String workspaceId = getWorkspaceId(); - String projectName = getName(); KubernetesClient client = clientFactory.create(workspaceId); - delete(projectName, client); + delete(name, client); } /** Returns namespace name */ @@ -235,6 +233,8 @@ private void delete(String namespaceName, KubernetesClient client) "Tried to delete namespace '%s' but it doesn't exist in the cluster.", namespaceName), e); + } else if (e.getCode() == 409) { + LOG.info(format("The namespace '%s' is currently being deleted.", namespaceName), e); } else { throw new KubernetesInfrastructureException(e); } 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 ec43d7db513..a57e1ba13aa 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 @@ -250,6 +250,21 @@ public void testDoesntFailIfDeletedNamespaceDoesntExist() throws Exception { // and no exception is thrown } + @Test + public void testDoesntFailIfDeletedNamespaceIsBeingDeleted() throws Exception { + // given + KubernetesNamespace namespace = new KubernetesNamespace(clientFactory, NAMESPACE, WORKSPACE_ID); + Resource resource = prepareNamespaceResource(NAMESPACE); + when(resource.delete()).thenThrow(new KubernetesClientException("err", 409, null)); + + // when + namespace.delete(); + + // then + verify(resource).delete(); + // and no exception is thrown + } + private MetadataNested prepareCreateNamespaceRequest() { DoneableNamespace namespace = mock(DoneableNamespace.class); MetadataNested metadataNested = mock(MetadataNested.class); 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 f4ae38f1e05..258e38dd93b 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 @@ -147,6 +147,8 @@ private void delete(String projectName, OpenShiftClient osClient) throws Infrast format( "Tried to delete project '%s' but it doesn't exist in the cluster.", projectName), e); + } else if (e.getCode() == 409) { + LOG.info(format("The project '%s' is currently being deleted.", projectName), e); } else { throw new KubernetesInfrastructureException(e); } 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 6dff8d2382b..74fca56e61a 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 @@ -189,6 +189,21 @@ public void testDoesntFailIfDeletedProjectDoesntExist() throws Exception { // and no exception is thrown } + @Test + public void testDoesntFailIfDeletedProjectIsBeingDeleted() throws Exception { + // given + OpenShiftProject project = new OpenShiftProject(clientFactory, PROJECT_NAME, WORKSPACE_ID); + Resource resource = prepareProjectResource(PROJECT_NAME); + when(resource.delete()).thenThrow(new KubernetesClientException("err", 409, null)); + + // when + project.delete(); + + // then + verify(resource).delete(); + // and no exception is thrown + } + private MetadataNested prepareProjectRequest() { ProjectRequestOperation projectRequestOperation = mock(ProjectRequestOperation.class); DoneableProjectRequest projectRequest = mock(DoneableProjectRequest.class); From dda099326e0f70e35f85a9d884c332d0f8b60517 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Mon, 21 Oct 2019 17:54:33 +0200 Subject: [PATCH 20/25] Rename isManagingNamespaces -> isManagingNamespace & isCreatingNamespaces -> isCreatingNamespace. Signed-off-by: Lukas Krejci --- .../kubernetes/namespace/KubernetesNamespaceFactory.java | 6 +++--- .../namespace/RemoveNamespaceOnWorkspaceRemove.java | 2 +- .../kubernetes/namespace/pvc/WorkspacePVCCleaner.java | 2 +- .../namespace/KubernetesNamespaceFactoryTest.java | 6 +++--- .../namespace/RemoveNamespaceOnWorkspaceRemoveTest.java | 4 ++-- .../kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java | 4 ++-- .../openshift/project/OpenShiftProjectFactory.java | 2 +- .../openshift/project/RemoveProjectOnWorkspaceRemove.java | 2 +- .../project/RemoveProjectOnWorkspaceRemoveTest.java | 4 ++-- 9 files changed, 16 insertions(+), 16 deletions(-) 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 944a4434317..ca89165334e 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 @@ -120,7 +120,7 @@ private boolean hasPlaceholders(String namespaceName) { } /** True if namespace is potentially created for the workspaces, false otherwise. */ - protected boolean isCreatingNamespaces(String workspaceId) throws InfrastructureException { + protected boolean isCreatingNamespace(String workspaceId) throws InfrastructureException { // ...namespace | ...namespace exists | ...namespace.default | creating? // no-placeholders | no | null | error // no-placeholders | no | no-placeholders | no @@ -156,7 +156,7 @@ protected boolean isCreatingNamespaces(String workspaceId) throws Infrastructure * @return true if the namespaces are fully managed by Che (e.g. created, used and deleted), false * otherwise */ - public boolean isManagingNamespaces(String workspaceId) throws InfrastructureException { + public boolean isManagingNamespace(String workspaceId) throws InfrastructureException { boolean canBeManaged = isNullOrEmpty(namespaceName) ? defaultNamespaceName != null && defaultNamespaceName.contains(WORKSPACEID_PLACEHOLDER) @@ -329,7 +329,7 @@ public KubernetesNamespace create(String workspaceId) throws InfrastructureExcep KubernetesNamespace namespace = doCreateNamespace(workspaceId, namespaceName); namespace.prepare(); - if (isCreatingNamespaces(workspaceId) && !isNullOrEmpty(serviceAccountName)) { + if (isCreatingNamespace(workspaceId) && !isNullOrEmpty(serviceAccountName)) { // prepare service account for workspace only if account name is configured // and project is not predefined // since predefined project should be prepared during Che deployment diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemove.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemove.java index 743e3a3ba3f..058ead96f7d 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemove.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemove.java @@ -45,7 +45,7 @@ public void subscribe(EventService eventService) { public void onEvent(WorkspaceRemovedEvent event) { String workspaceId = event.getWorkspace().getId(); try { - if (namespaceFactory.isManagingNamespaces(workspaceId)) { + if (namespaceFactory.isManagingNamespace(workspaceId)) { namespaceFactory.delete(workspaceId); } } catch (InfrastructureException e) { diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java index 21a310f7672..0cfa17d95ff 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java @@ -57,7 +57,7 @@ public void subscribe(EventService eventService) { event -> { final Workspace workspace = event.getWorkspace(); try { - if (namespaceFactory.isManagingNamespaces(workspace.getId())) { + if (namespaceFactory.isManagingNamespace(workspace.getId())) { // the namespaces of managed workspaces are deleted, so no need to do the cleanup return; } 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 8d0eb31d6a2..8065ba3e571 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 @@ -310,7 +310,7 @@ public void shouldDetermineWhenNamespaceCanBeCreated( // when Boolean result; try { - result = namespaceFactory.isCreatingNamespaces("123"); + result = namespaceFactory.isCreatingNamespace("123"); } catch (InfrastructureException e) { // this can happen and we test for it below... result = null; @@ -345,7 +345,7 @@ public void testNotManagingNamespacesWheneverNotCreatingThem( // when boolean creating; try { - creating = namespaceFactory.isCreatingNamespaces("123"); + creating = namespaceFactory.isCreatingNamespace("123"); } catch (InfrastructureException e) { // if we can't determine whether we're potentially creating a namespace, we shouldn't claim // we're managing it @@ -354,7 +354,7 @@ public void testNotManagingNamespacesWheneverNotCreatingThem( } creating = false; } - boolean managing = namespaceFactory.isManagingNamespaces("123"); + boolean managing = namespaceFactory.isManagingNamespace("123"); // then if (!creating) { diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemoveTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemoveTest.java index efdef79ae1a..94f808bb078 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemoveTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/RemoveNamespaceOnWorkspaceRemoveTest.java @@ -64,7 +64,7 @@ public void shouldSubscribeListenerToEventService() { @Test public void shouldRemoveNamespaceOnWorkspaceRemovedEventIfNamespaceIsManaged() throws Exception { - when(namespaceFactory.isManagingNamespaces(any())).thenReturn(true); + when(namespaceFactory.isManagingNamespace(any())).thenReturn(true); removeNamespaceOnWorkspaceRemove.onEvent(new WorkspaceRemovedEvent(workspace)); @@ -74,7 +74,7 @@ public void shouldRemoveNamespaceOnWorkspaceRemovedEventIfNamespaceIsManaged() t @Test public void shouldNotRemoveNamespaceOnWorkspaceRemovedEventIfNamespaceIsNotManaged() throws Exception { - when(namespaceFactory.isManagingNamespaces(any())).thenReturn(false); + when(namespaceFactory.isManagingNamespace(any())).thenReturn(false); removeNamespaceOnWorkspaceRemove.onEvent(new WorkspaceRemovedEvent(workspace)); diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java index f8a5e3a7aa9..48cda981845 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java @@ -48,7 +48,7 @@ public class WorkspacePVCCleanerTest { @BeforeMethod public void setUp() throws Exception { - when(namespaceFactory.isManagingNamespaces(any())).thenReturn(false); + when(namespaceFactory.isManagingNamespace(any())).thenReturn(false); workspacePVCCleaner = new WorkspacePVCCleaner(true, namespaceFactory, pvcStrategy); when(workspace.getId()).thenReturn("123"); when(event.getWorkspace()).thenReturn(workspace); @@ -85,7 +85,7 @@ public void testInvokeCleanupWhenWorkspaceRemovedEventPublishedAndNamespaceIsNot @Test public void testNotInvokeCleanupWhenWorkspaceRemovedEventPublishedAndNamespaceIsManaged() throws Exception { - when(namespaceFactory.isManagingNamespaces(any())).thenReturn(true); + when(namespaceFactory.isManagingNamespace(any())).thenReturn(true); workspacePVCCleaner.subscribe(eventService); 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 7490017e70a..77261f2739c 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 @@ -105,7 +105,7 @@ public OpenShiftProject create(String workspaceId) throws InfrastructureExceptio OpenShiftProject osProject = doCreateProject(workspaceId, projectName); osProject.prepare(); - if (isCreatingNamespaces(workspaceId) && !isNullOrEmpty(getServiceAccountName())) { + if (isCreatingNamespace(workspaceId) && !isNullOrEmpty(getServiceAccountName())) { // prepare service account for workspace only if account name is configured // and project is not predefined // since predefined project should be prepared during Che deployment diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/RemoveProjectOnWorkspaceRemove.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/RemoveProjectOnWorkspaceRemove.java index 649c9da533f..fd6c7fe1fd1 100644 --- a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/RemoveProjectOnWorkspaceRemove.java +++ b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/RemoveProjectOnWorkspaceRemove.java @@ -46,7 +46,7 @@ public void subscribe(EventService eventService) { public void onEvent(WorkspaceRemovedEvent event) { String workspaceId = event.getWorkspace().getId(); try { - if (projectFactory.isManagingNamespaces(workspaceId)) { + if (projectFactory.isManagingNamespace(workspaceId)) { projectFactory.delete(workspaceId); } } catch (InfrastructureException e) { diff --git a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/RemoveProjectOnWorkspaceRemoveTest.java b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/RemoveProjectOnWorkspaceRemoveTest.java index a30c4ab4252..92ab109e18a 100644 --- a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/RemoveProjectOnWorkspaceRemoveTest.java +++ b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/RemoveProjectOnWorkspaceRemoveTest.java @@ -65,7 +65,7 @@ public void shouldSubscribeListenerToEventService() { @Test public void shouldRemoveProjectOnWorkspaceRemovedEventIfFactoryIsManagingNamespaces() throws Exception { - when(projectFactory.isManagingNamespaces(any())).thenReturn(true); + when(projectFactory.isManagingNamespace(any())).thenReturn(true); removeProjectOnWorkspaceRemove.onEvent(new WorkspaceRemovedEvent(workspace)); @@ -75,7 +75,7 @@ public void shouldRemoveProjectOnWorkspaceRemovedEventIfFactoryIsManagingNamespa @Test public void shouldNotRemoveProjectOnWorkspaceRemovedEventIfFactoryIsNotManagingNamespaces() throws Exception { - when(projectFactory.isManagingNamespaces(any())).thenReturn(false); + when(projectFactory.isManagingNamespace(any())).thenReturn(false); removeProjectOnWorkspaceRemove.onEvent(new WorkspaceRemovedEvent(workspace)); From 26bacf80be6782b7f23140d90449914bb7890ab1 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 22 Oct 2019 12:24:34 +0200 Subject: [PATCH 21/25] Clarified the logic in isManagingNamespace, updated the javadoc. Signed-off-by: Lukas Krejci --- .../namespace/KubernetesNamespaceFactory.java | 85 +++++++++++-------- 1 file changed, 49 insertions(+), 36 deletions(-) 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 ca89165334e..67036f5a790 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 @@ -74,7 +74,7 @@ public class KubernetesNamespaceFactory { private final String defaultNamespaceName; private final boolean allowUserDefinedNamespaces; - private final String namespaceName; + private final String legacyNamespaceName; private final String serviceAccountName; private final String clusterRoleName; private final KubernetesClientFactory clientFactory; @@ -82,7 +82,7 @@ public class KubernetesNamespaceFactory { @Inject public KubernetesNamespaceFactory( - @Nullable @Named("che.infra.kubernetes.namespace") String namespaceName, + @Nullable @Named("che.infra.kubernetes.namespace") String legacyNamespaceName, @Nullable @Named("che.infra.kubernetes.service_account_name") String serviceAccountName, @Nullable @Named("che.infra.kubernetes.cluster_role_name") String clusterRoleName, @Nullable @Named("che.infra.kubernetes.namespace.default") String defaultNamespaceName, @@ -91,7 +91,7 @@ public KubernetesNamespaceFactory( KubernetesClientFactory clientFactory, WorkspaceManager workspaceManager) throws ConfigurationException { - this.namespaceName = namespaceName; + this.legacyNamespaceName = legacyNamespaceName; this.serviceAccountName = serviceAccountName; this.clusterRoleName = clusterRoleName; this.clientFactory = clientFactory; @@ -119,22 +119,32 @@ private boolean hasPlaceholders(String namespaceName) { && NAMESPACE_NAME_PLACEHOLDERS.keySet().stream().anyMatch(namespaceName::contains); } - /** True if namespace is potentially created for the workspaces, false otherwise. */ + /** + * True if namespace is potentially created for the workspace, false otherwise. + * + *

The logic is a little bit non-trivial and best expressed by just fully evaluting the truth + * table as below ({@code ...namespace} stands for the legacy namespace property, {@code + * ...namespace.default} stands for the namespace default property): + * + * 

{@code
+   * ...namespace    | ...namespace exists | ...namespace.default | creating?
+   * no-placeholders |       no            |       null           | error
+   * no-placeholders |       no            |   no-placeholders    | no
+   * no-placeholders |       no            |    placeholders      | yes
+   * no-placeholders |      yes            |       null           | no
+   * no-placeholders |      yes            |   no-placeholders    | no
+   * no-placeholders |      yes            |    placeholders      | no
+   *  placeholders   |       no            |        null          | error
+   *  placeholders   |       no            |   no-placeholders    | no
+   *  placeholders   |       no            |    placeholders      | yes
+   *  placeholders   |      yes            |        null          | yes
+   *  placeholders   |      yes            |   no-placeholders    | yes
+   *  placeholders   |      yes            |    placeholders      | yes
+   * }
+ */ protected boolean isCreatingNamespace(String workspaceId) throws InfrastructureException { - // ...namespace | ...namespace exists | ...namespace.default | creating? - // no-placeholders | no | null | error - // no-placeholders | no | no-placeholders | no - // no-placeholders | no | placeholders | yes - // no-placeholders | yes | null | no - // no-placeholders | yes | no-placeholders | no - // no-placeholders | yes | placeholders | no - // placeholders | no | null | error - // placeholders | no | no-placeholders | no - // placeholders | no | placeholders | yes - // placeholders | yes | null | yes - // placeholders | yes | no-placeholders | yes - // placeholders | yes | placeholders | yes - boolean legacyWithPlaceholders = isNullOrEmpty(namespaceName) || hasPlaceholders(namespaceName); + boolean legacyWithPlaceholders = + isNullOrEmpty(legacyNamespaceName) || hasPlaceholders(legacyNamespaceName); boolean legacyExists = checkNamespaceExists( resolveLegacyNamespaceName(EnvironmentContext.getCurrent().getSubject(), workspaceId)); @@ -153,26 +163,29 @@ protected boolean isCreatingNamespace(String workspaceId) throws InfrastructureE } /** - * @return true if the namespaces are fully managed by Che (e.g. created, used and deleted), false - * otherwise + * Returns true if the namespace is fully managed by Che (e.g. created, used and deleted), false + * otherwise. */ public boolean isManagingNamespace(String workspaceId) throws InfrastructureException { - boolean canBeManaged = - isNullOrEmpty(namespaceName) - ? defaultNamespaceName != null && defaultNamespaceName.contains(WORKSPACEID_PLACEHOLDER) - : namespaceName.contains(WORKSPACEID_PLACEHOLDER); - - if (!canBeManaged) { - return false; - } - - if (isNullOrEmpty(namespaceName)) { - // if we're using the new logic, we don't have to check if the namespace exists - return true; - } else { - // the legacy prop is only applied if the namespace actually exists... + // the new logic is quite simple. + boolean newLogic = + defaultNamespaceName != null && defaultNamespaceName.contains(WORKSPACEID_PLACEHOLDER); + + // but we must follow the same logic as #evalNamespaceName - we need to make sure that the old + // logic can't be used first... + // empty legacy namespace name ~ + if (isNullOrEmpty(legacyNamespaceName) + || legacyNamespaceName.contains(WORKSPACEID_PLACEHOLDER)) { + + // there's a chance of using the old logic - if the namespace exists, we're managing it. + // if it doesn't, we're using the new logic. return checkNamespaceExists( - resolveLegacyNamespaceName(EnvironmentContext.getCurrent().getSubject(), workspaceId)); + resolveLegacyNamespaceName(EnvironmentContext.getCurrent().getSubject(), workspaceId)) + || newLogic; + } else { + // there's no way the namespace of the workspace is managed using the old logic. Let's just + // use the result of the new logic. + return newLogic; } } @@ -416,7 +429,7 @@ protected String evalNamespaceName(String workspaceId, Subject currentUser) private String resolveLegacyNamespaceName(Subject user, String workspaceId) { String effectiveOldLogicNamespace = - isNullOrEmpty(namespaceName) ? WORKSPACEID_PLACEHOLDER : namespaceName; + isNullOrEmpty(legacyNamespaceName) ? WORKSPACEID_PLACEHOLDER : legacyNamespaceName; return evalPlaceholders(effectiveOldLogicNamespace, user, workspaceId); } From 1663ab5a8bc55ce1696d6063d2c7e3293a32f4f7 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 22 Oct 2019 12:34:46 +0200 Subject: [PATCH 22/25] Include the original error message when evaluating the namespace name fails Signed-off-by: Lukas Krejci --- .../kubernetes/namespace/KubernetesNamespaceFactory.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 67036f5a790..86781b81f9e 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 @@ -336,7 +336,11 @@ public KubernetesNamespace create(String workspaceId) throws InfrastructureExcep namespaceName = evalNamespaceName(workspaceId, EnvironmentContext.getCurrent().getSubject()); } catch (NotFoundException | ServerException | ConflictException | ValidationException e) { throw new InfrastructureException( - format("Failed to determine the namespace to put the workspace %s to", workspaceId), e); + format( + "Failed to determine the namespace to put the workspace %s to." + + " The error message was: %s", + workspaceId, e.getMessage()), + e); } KubernetesNamespace namespace = doCreateNamespace(workspaceId, namespaceName); From 5d530b17ad17139b33b6725ab0c61018fb71ea95 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 22 Oct 2019 18:24:54 +0200 Subject: [PATCH 23/25] javadoc clarifications, simplification of isCreatingNamespace() Signed-off-by: Lukas Krejci --- .../namespace/KubernetesNamespace.java | 7 ++++++- .../namespace/KubernetesNamespaceFactory.java | 21 +++++++++++-------- .../openshift/project/OpenShiftProject.java | 6 ++++++ .../project/OpenShiftProjectFactory.java | 6 +++++- .../che/api/workspace/shared/Constants.java | 4 ++-- 5 files changed, 31 insertions(+), 13 deletions(-) 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 805630d5417..cc4c9373a7d 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 @@ -119,8 +119,13 @@ void prepare() throws InfrastructureException { } } + /** + * 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. + * + * @throws InfrastructureException if any unexpected exception occurs during namespace deletion + */ void delete() throws InfrastructureException { - KubernetesClient client = clientFactory.create(workspaceId); delete(name, client); } 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 86781b81f9e..dd2718a0cfe 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 @@ -143,23 +143,26 @@ private boolean hasPlaceholders(String namespaceName) { * } */ protected boolean isCreatingNamespace(String workspaceId) throws InfrastructureException { - boolean legacyWithPlaceholders = - isNullOrEmpty(legacyNamespaceName) || hasPlaceholders(legacyNamespaceName); boolean legacyExists = checkNamespaceExists( resolveLegacyNamespaceName(EnvironmentContext.getCurrent().getSubject(), workspaceId)); - if (isNullOrEmpty(defaultNamespaceName) && !legacyExists) { + // legacy namespace exists and should be used + if (legacyExists) { + // if it contains any placeholder("" is ) - it indicates that Che created + // namespace by itself + return isNullOrEmpty(legacyNamespaceName) || hasPlaceholders(legacyNamespaceName); + } + + if (isNullOrEmpty(defaultNamespaceName)) { throw new InfrastructureException( "Cannot determine whether a new namespace and service account should be" - + " created for workspace %s. There is no pre-existing workspace namespace to be found using the legacy" - + " `che.infra.kubernetes.namespace` property yet the `che.infra.kubernetes.namespace.default` property" - + " is undefined."); + + " created for workspace %s. There is no pre-existing workspace namespace to be" + + " found using the legacy `che.infra.kubernetes.namespace` property yet the" + + " `che.infra.kubernetes.namespace.default` property is undefined."); } - boolean defaultHasPlaceholders = hasPlaceholders(defaultNamespaceName); - - return (legacyWithPlaceholders && legacyExists) || (!legacyExists && defaultHasPlaceholders); + return hasPlaceholders(defaultNamespaceName); } /** 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 258e38dd93b..bd5ec9c8e1c 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 @@ -96,6 +96,12 @@ void prepare() throws InfrastructureException { } } + /** + * Deletes the project. Deleting a non-existent projects is not an error as is not an attempt to + * delete a project that is already being deleted. + * + * @throws InfrastructureException if any unexpected exception occurs during project deletion + */ void delete() throws InfrastructureException { String workspaceId = getWorkspaceId(); String projectName = getName(); 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 77261f2739c..772aee57ee6 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 @@ -124,7 +124,11 @@ public void delete(String workspaceId) throws InfrastructureException { projectName = evalNamespaceName(workspaceId, EnvironmentContext.getCurrent().getSubject()); } catch (NotFoundException | ServerException | ConflictException | ValidationException e) { throw new InfrastructureException( - format("Failed to evaluate the project name to use for workspace %s.", workspaceId), e); + format( + "Failed to evaluate the project name to use for workspace %s." + + " The error message was: %s", + workspaceId, e.getMessage()), + e); } OpenShiftProject osProject = doCreateProject(workspaceId, projectName); diff --git a/wsmaster/che-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java b/wsmaster/che-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java index 1fa2c618a22..406e1868075 100644 --- a/wsmaster/che-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java +++ b/wsmaster/che-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java @@ -186,8 +186,8 @@ public final class Constants { public static final int WORKSPACE_GENERATE_NAME_CHARS_APPEND = 5; /** - * The attribute in the workspace config where we store the infrastructure namespace the workspace - * is deployed to + * The attribute of the workspace where we store the infrastructure namespace the workspace is + * deployed to */ public static final String WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE = "infrastructureNamespace"; From 639e98940b628661cbd0efb0107992f2eff9a8d3 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 22 Oct 2019 18:33:59 +0200 Subject: [PATCH 24/25] Improve error messages and logging. Signed-off-by: Lukas Krejci --- .../kubernetes/namespace/pvc/WorkspacePVCCleaner.java | 3 +++ .../openshift/project/OpenShiftProjectFactory.java | 6 +++++- .../eclipse/che/api/workspace/server/WorkspaceManager.java | 3 ++- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java index 0cfa17d95ff..89ce7d4247b 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java @@ -59,6 +59,9 @@ public void subscribe(EventService eventService) { try { if (namespaceFactory.isManagingNamespace(workspace.getId())) { // the namespaces of managed workspaces are deleted, so no need to do the cleanup + LOG.debug( + "Not cleaning up the PVCs of workspace %s, because its namespace is" + + " going to be deleted."); return; } strategy.cleanup(workspace); 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 772aee57ee6..f998faffbfe 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 @@ -100,7 +100,11 @@ public OpenShiftProject create(String workspaceId) throws InfrastructureExceptio projectName = evalNamespaceName(workspaceId, EnvironmentContext.getCurrent().getSubject()); } catch (NotFoundException | ServerException | ConflictException | ValidationException e) { throw new InfrastructureException( - format("Failed to evaluate the project name to use for workspace %s.", workspaceId), e); + format( + "Failed to evaluate the project name to use for workspace %s. The error message" + + " was: %s", + workspaceId, e.getMessage()), + e); } OpenShiftProject osProject = doCreateProject(workspaceId, projectName); osProject.prepare(); diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java index 12cf4025dd4..83879c69fb7 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java @@ -471,7 +471,8 @@ private void removeWorkspaceQuietly(String workspaceId) { try { workspaceDao.remove(workspaceId); } catch (ServerException x) { - LOG.error("Unable to remove temporary workspace '{}'", workspaceId); + LOG.error("Unable to remove temporary workspace '{}'. Error message was: {}", workspaceId, + x.getMessage()); } } From 1bcfc511fb535c65c064412c18efb7f108a5e79f Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 22 Oct 2019 18:40:22 +0200 Subject: [PATCH 25/25] Formatting. Signed-off-by: Lukas Krejci --- .../eclipse/che/api/workspace/server/WorkspaceManager.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java index 83879c69fb7..9a0a667b73c 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java @@ -471,8 +471,10 @@ private void removeWorkspaceQuietly(String workspaceId) { try { workspaceDao.remove(workspaceId); } catch (ServerException x) { - LOG.error("Unable to remove temporary workspace '{}'. Error message was: {}", workspaceId, - x.getMessage()); + LOG.error( + "Unable to remove temporary workspace '{}'. Error message was: {}", + workspaceId, + x.getMessage()); } }