From 80c13f088a8bce5b5385212e41c714e076f9622e Mon Sep 17 00:00:00 2001 From: Igor Vinokur Date: Mon, 9 Aug 2021 17:33:37 +0300 Subject: [PATCH] fixup! fix: Add a predefined secret to store credentials --- .../AbstractWorkspaceServiceAccount.java | 13 +++--- .../namespace/KubernetesNamespace.java | 17 +++---- .../namespace/KubernetesNamespaceFactory.java | 26 +++++++++-- .../KubernetesWorkspaceServiceAccount.java | 7 +-- .../KubernetesNamespaceFactoryTest.java | 45 ++++++++++++++++++- .../namespace/KubernetesNamespaceTest.java | 17 ++----- .../OpenShiftWorkspaceServiceAccount.java | 7 +-- 7 files changed, 85 insertions(+), 47 deletions(-) diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java index 2d7b9edc8f5..223a1bff334 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java @@ -11,6 +11,7 @@ */ package org.eclipse.che.workspace.infrastructure.kubernetes.namespace; +import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import io.fabric8.kubernetes.api.model.HasMetadata; @@ -26,7 +27,6 @@ import org.eclipse.che.api.user.server.PreferenceManager; import org.eclipse.che.api.user.server.UserManager; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; -import org.eclipse.che.commons.annotation.Nullable; import org.eclipse.che.workspace.infrastructure.kubernetes.CheServerKubernetesClientFactory; import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory; import org.eclipse.che.workspace.infrastructure.kubernetes.util.KubernetesSharedPool; @@ -113,7 +113,7 @@ private void ensureImplicitRolesWithBindings(Client k8sClient) { buildRole( EXEC_ROLE_NAME, singletonList("pods/exec"), - null, + emptyList(), singletonList(""), singletonList("create")), serviceAccountName + "-exec"); @@ -124,7 +124,7 @@ private void ensureImplicitRolesWithBindings(Client k8sClient) { buildRole( VIEW_ROLE_NAME, Arrays.asList("pods", "services"), - null, + emptyList(), singletonList(""), singletonList("list")), serviceAccountName + "-view"); @@ -135,7 +135,7 @@ private void ensureImplicitRolesWithBindings(Client k8sClient) { buildRole( METRICS_ROLE_NAME, Arrays.asList("pods", "nodes"), - null, + emptyList(), singletonList("metrics.k8s.io"), Arrays.asList("list", "get", "watch")), serviceAccountName + "-metrics"); @@ -194,15 +194,14 @@ private void ensureExplicitClusterRoleBindings(Client k8sClient) { * * @param name the name of the role * @param resources the resources the role grants access to - * @param resourceNames specific resource names witch the role grants access to. {@code null} if - * empty. + * @param resourceNames specific resource names witch the role grants access to. * @param verbs the verbs the role allows * @return the role object for the given type of Client */ protected abstract R buildRole( String name, List resources, - @Nullable List resourceNames, + List resourceNames, List apiGroups, List verbs); 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 eeae703c3a3..8d756e7d313 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 @@ -13,7 +13,6 @@ import static io.fabric8.kubernetes.api.model.DeletionPropagation.BACKGROUND; import static java.lang.String.format; -import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.AbstractWorkspaceServiceAccount.CREDENTIALS_SECRET_NAME; import com.google.common.annotations.VisibleForTesting; import io.fabric8.kubernetes.api.model.ConfigMap; @@ -22,7 +21,6 @@ import io.fabric8.kubernetes.api.model.PersistentVolumeClaim; import io.fabric8.kubernetes.api.model.Pod; import io.fabric8.kubernetes.api.model.Secret; -import io.fabric8.kubernetes.api.model.SecretBuilder; import io.fabric8.kubernetes.api.model.Service; import io.fabric8.kubernetes.api.model.ServiceAccount; import io.fabric8.kubernetes.api.model.extensions.Ingress; @@ -145,29 +143,26 @@ public KubernetesNamespace( * @param annotations annotations that should be set to the namespace * @throws InfrastructureException if any exception occurs during namespace preparation or if the * namespace doesn't exist and {@code canCreate} is {@code false}. + * @return {@code true} if the namespace didn't exist and namespace creation was invoked, {@code + * false} if the namespace was already created in the previous calls. */ - void prepare(boolean canCreate, Map labels, Map annotations) + boolean prepare(boolean canCreate, Map labels, Map annotations) throws InfrastructureException { KubernetesClient client = clientFactory.create(workspaceId); Namespace namespace = get(name, client); + boolean needToCreateNewNamespace = false; if (namespace == null) { + needToCreateNewNamespace = true; if (!canCreate) { throw new InfrastructureException( format("Creating the namespace '%s' is not allowed, yet it was not found.", name)); } namespace = create(name, client); - Secret secret = - new SecretBuilder() - .withType("opaque") - .withNewMetadata() - .withName(CREDENTIALS_SECRET_NAME) - .endMetadata() - .build(); - client.secrets().inNamespace(name).create(secret); } label(namespace, labels); annotate(namespace, annotations); + return needToCreateNewNamespace; } /** diff --git a/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 75a0d33328a..8185f987084 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 @@ -20,6 +20,7 @@ import static org.eclipse.che.api.workspace.shared.Constants.WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE; import static org.eclipse.che.workspace.infrastructure.kubernetes.api.shared.KubernetesNamespaceMeta.DEFAULT_ATTRIBUTE; import static org.eclipse.che.workspace.infrastructure.kubernetes.api.shared.KubernetesNamespaceMeta.PHASE_ATTRIBUTE; +import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.AbstractWorkspaceServiceAccount.CREDENTIALS_SECRET_NAME; import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.NamespaceNameValidator.METADATA_NAME_MAX_LENGTH; import com.google.common.annotations.VisibleForTesting; @@ -30,6 +31,8 @@ import com.google.inject.Singleton; import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.Namespace; +import io.fabric8.kubernetes.api.model.Secret; +import io.fabric8.kubernetes.api.model.SecretBuilder; import io.fabric8.kubernetes.client.KubernetesClientException; import java.util.Collections; import java.util.HashMap; @@ -336,10 +339,25 @@ public KubernetesNamespace getOrCreate(RuntimeIdentity identity) throws Infrastr Map namespaceAnnotationsEvaluated = evaluateAnnotationPlaceholders(resolutionCtx); - namespace.prepare( - canCreateNamespace(identity), - labelNamespaces ? namespaceLabels : emptyMap(), - annotateNamespaces ? namespaceAnnotationsEvaluated : emptyMap()); + boolean newNamespace = + namespace.prepare( + canCreateNamespace(identity), + labelNamespaces ? namespaceLabels : emptyMap(), + annotateNamespaces ? namespaceAnnotationsEvaluated : emptyMap()); + if (newNamespace) { + Secret secret = + new SecretBuilder() + .withType("opaque") + .withNewMetadata() + .withName(CREDENTIALS_SECRET_NAME) + .endMetadata() + .build(); + clientFactory + .create() + .secrets() + .inNamespace(identity.getInfrastructureNamespace()) + .create(secret); + } if (!isNullOrEmpty(serviceAccountName)) { KubernetesWorkspaceServiceAccount workspaceServiceAccount = diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccount.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccount.java index 0dd56f83c3c..c1f4346c1d6 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccount.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccount.java @@ -11,8 +11,6 @@ */ package org.eclipse.che.workspace.infrastructure.kubernetes.namespace; -import static java.util.Collections.emptyList; - import io.fabric8.kubernetes.api.model.rbac.PolicyRuleBuilder; import io.fabric8.kubernetes.api.model.rbac.Role; import io.fabric8.kubernetes.api.model.rbac.RoleBinding; @@ -22,7 +20,6 @@ import io.fabric8.kubernetes.client.KubernetesClient; import java.util.List; import java.util.Set; -import org.eclipse.che.commons.annotation.Nullable; import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory; /** @@ -55,7 +52,7 @@ public KubernetesWorkspaceServiceAccount( protected Role buildRole( String name, List resources, - @Nullable List resourceNames, + List resourceNames, List apiGroups, List verbs) { return new RoleBuilder() @@ -65,7 +62,7 @@ protected Role buildRole( .withRules( new PolicyRuleBuilder() .withResources(resources) - .withResourceNames(resourceNames != null ? resourceNames : emptyList()) + .withResourceNames(resourceNames) .withApiGroups(apiGroups) .withVerbs(verbs) .build()) diff --git a/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 eb0dd76d882..202cb1e1316 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 @@ -20,6 +20,7 @@ import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.AbstractWorkspaceServiceAccount.SECRETS_ROLE_NAME; import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory.NAMESPACE_TEMPLATE_ATTRIBUTE; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyMap; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; @@ -43,6 +44,7 @@ import io.fabric8.kubernetes.api.model.Namespace; import io.fabric8.kubernetes.api.model.NamespaceBuilder; import io.fabric8.kubernetes.api.model.NamespaceList; +import io.fabric8.kubernetes.api.model.Secret; import io.fabric8.kubernetes.api.model.ServiceAccountList; import io.fabric8.kubernetes.api.model.Status; import io.fabric8.kubernetes.api.model.rbac.ClusterRoleBuilder; @@ -53,6 +55,7 @@ import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.KubernetesClientException; import io.fabric8.kubernetes.client.dsl.FilterWatchListDeletable; +import io.fabric8.kubernetes.client.dsl.MixedOperation; import io.fabric8.kubernetes.client.dsl.NonNamespaceOperation; import io.fabric8.kubernetes.client.dsl.Resource; import io.fabric8.kubernetes.client.server.mock.KubernetesServer; @@ -121,8 +124,7 @@ public class KubernetesNamespaceFactoryTest { @Mock private PreferenceManager preferenceManager; @Mock Appender mockedAppender; - @Mock - private NonNamespaceOperation> namespaceOperation; + @Mock private NonNamespaceOperation namespaceOperation; @Mock private Resource namespaceResource; @@ -457,6 +459,45 @@ public void shouldReturnDefaultNamespaceWhenItDoesNotExistAndUserDefinedIsNotAll .get(PHASE_ATTRIBUTE)); // no phase - means such namespace does not exist } + @Test + public void shouldCreateCredentialsSecretWhenNamespaceDoesNotExist() throws Exception { + // given + namespaceFactory = + spy( + new KubernetesNamespaceFactory( + "", + "", + "-che", + true, + true, + true, + NAMESPACE_LABELS, + NAMESPACE_ANNOTATIONS, + clientFactory, + cheClientFactory, + userManager, + preferenceManager, + pool)); + KubernetesNamespace toReturnNamespace = mock(KubernetesNamespace.class); + when(toReturnNamespace.prepare(anyBoolean(), anyMap(), anyMap())).thenReturn(true); + doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespaceAccess(any(), any()); + MixedOperation mixedOperation = mock(MixedOperation.class); + lenient().when(k8sClient.secrets()).thenReturn(mixedOperation); + lenient().when(mixedOperation.inNamespace(anyString())).thenReturn(namespaceOperation); + + // when + RuntimeIdentity identity = + new RuntimeIdentityImpl("workspace123", null, USER_ID, "workspace123"); + namespaceFactory.getOrCreate(identity); + + // then + ArgumentCaptor secretsCaptor = ArgumentCaptor.forClass(Secret.class); + verify(namespaceOperation).create(secretsCaptor.capture()); + Secret secret = secretsCaptor.getValue(); + Assert.assertEquals(secret.getMetadata().getName(), CREDENTIALS_SECRET_NAME); + Assert.assertEquals(secret.getType(), "opaque"); + } + @Test( expectedExceptions = InfrastructureException.class, expectedExceptionsMessageRegExp = diff --git a/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 cba059152af..964bf2ce3b8 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 @@ -13,7 +13,6 @@ import static io.fabric8.kubernetes.api.model.DeletionPropagation.BACKGROUND; import static java.util.Collections.emptyMap; -import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.AbstractWorkspaceServiceAccount.CREDENTIALS_SECRET_NAME; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; @@ -23,7 +22,6 @@ import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; -import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; @@ -32,7 +30,6 @@ import io.fabric8.kubernetes.api.model.Namespace; import io.fabric8.kubernetes.api.model.NamespaceBuilder; -import io.fabric8.kubernetes.api.model.Secret; import io.fabric8.kubernetes.api.model.ServiceAccount; import io.fabric8.kubernetes.api.model.Status; import io.fabric8.kubernetes.client.KubernetesClient; @@ -91,8 +88,8 @@ public void setUp() throws Exception { lenient().doReturn(namespaceOperation).when(kubernetesClient).namespaces(); final MixedOperation mixedOperation = mock(MixedOperation.class); + final NonNamespaceOperation namespaceOperation = mock(NonNamespaceOperation.class); lenient().doReturn(mixedOperation).when(kubernetesClient).serviceAccounts(); - lenient().doReturn(mixedOperation).when(kubernetesClient).secrets(); lenient().when(mixedOperation.inNamespace(anyString())).thenReturn(namespaceOperation); lenient().when(namespaceOperation.withName(anyString())).thenReturn(serviceAccountResource); lenient().when(serviceAccountResource.get()).thenReturn(mock(ServiceAccount.class)); @@ -123,7 +120,6 @@ public void testKubernetesNamespacePreparingWhenNamespaceExists() throws Excepti // then verify(namespaceOperation, never()).create(any(Namespace.class)); - verify(kubernetesClient, never()).secrets(); } @Test @@ -139,14 +135,9 @@ public void testKubernetesNamespacePreparingCreationWhenNamespaceDoesNotExist() namespace.prepare(true, Map.of(), Map.of()); // then - ArgumentCaptor namespaceCaptor = ArgumentCaptor.forClass(Namespace.class); - ArgumentCaptor secretsCaptor = ArgumentCaptor.forClass(Secret.class); - verify(namespaceOperation, times(2)).create(namespaceCaptor.capture()); - verify(namespaceOperation, times(2)).create(secretsCaptor.capture()); - Assert.assertEquals(namespaceCaptor.getAllValues().get(0).getMetadata().getName(), NAMESPACE); - Secret secret = secretsCaptor.getAllValues().get(1); - Assert.assertEquals(secret.getMetadata().getName(), CREDENTIALS_SECRET_NAME); - Assert.assertEquals(secret.getType(), "opaque"); + ArgumentCaptor captor = ArgumentCaptor.forClass(Namespace.class); + verify(namespaceOperation).create(captor.capture()); + Assert.assertEquals(captor.getValue().getMetadata().getName(), NAMESPACE); } @Test(expectedExceptions = InfrastructureException.class) diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftWorkspaceServiceAccount.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftWorkspaceServiceAccount.java index 13781334b00..7870a2d2991 100644 --- a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftWorkspaceServiceAccount.java +++ b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftWorkspaceServiceAccount.java @@ -11,8 +11,6 @@ */ package org.eclipse.che.workspace.infrastructure.openshift.project; -import static java.util.Collections.emptyList; - import io.fabric8.kubernetes.api.model.ObjectReferenceBuilder; import io.fabric8.openshift.api.model.PolicyRuleBuilder; import io.fabric8.openshift.api.model.Role; @@ -23,7 +21,6 @@ import io.fabric8.openshift.client.OpenShiftClient; import java.util.List; import java.util.Set; -import org.eclipse.che.commons.annotation.Nullable; import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.AbstractWorkspaceServiceAccount; import org.eclipse.che.workspace.infrastructure.openshift.OpenShiftClientFactory; @@ -60,7 +57,7 @@ class OpenShiftWorkspaceServiceAccount protected Role buildRole( String name, List resources, - @Nullable List resourceNames, + List resourceNames, List apiGroups, List verbs) { return new RoleBuilder() @@ -70,7 +67,7 @@ protected Role buildRole( .withRules( new PolicyRuleBuilder() .withResources(resources) - .withResourceNames(resourceNames != null ? resourceNames : emptyList()) + .withResourceNames(resourceNames) .withApiGroups(apiGroups) .withVerbs(verbs) .build())