Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add a predefined secret to store credentials #68

Merged
merged 9 commits into from
Aug 17, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
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;
Expand All @@ -47,6 +48,7 @@ public abstract class AbstractWorkspaceServiceAccount<
public static final String EXEC_ROLE_NAME = "exec";
public static final String VIEW_ROLE_NAME = "workspace-view";
public static final String METRICS_ROLE_NAME = "workspace-metrics";
public static final String SECRETS_ROLE_NAME = "workspace-secrets";

protected final String namespace;
protected final String serviceAccountName;
Expand Down Expand Up @@ -109,6 +111,7 @@ private void ensureImplicitRolesWithBindings(Client k8sClient) {
k8sClient,
EXEC_ROLE_NAME,
singletonList("pods/exec"),
null,
singletonList(""),
singletonList("create"),
serviceAccountName + "-exec");
Expand All @@ -118,6 +121,7 @@ private void ensureImplicitRolesWithBindings(Client k8sClient) {
k8sClient,
VIEW_ROLE_NAME,
Arrays.asList("pods", "services"),
null,
singletonList(""),
singletonList("list"),
serviceAccountName + "-view");
Expand All @@ -127,19 +131,31 @@ private void ensureImplicitRolesWithBindings(Client k8sClient) {
k8sClient,
METRICS_ROLE_NAME,
Arrays.asList("pods", "nodes"),
null,
singletonList("metrics.k8s.io"),
Arrays.asList("list", "get", "watch"),
serviceAccountName + "-metrics");

// credentials-secret role
ensureRoleWithBinding(
k8sClient,
SECRETS_ROLE_NAME,
singletonList("secrets"),
singletonList("che-credentials-secret"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we choose a Che-agnostic name -- e.g. workspace-credentials-secret? If we want to do a similar thing in DWO, it's a lot easier to justify if it's not labelled with a specific platform.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for workspace-credentials-secret

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+ please consider moving that secret name somewhere to constants

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

singletonList(""),
Arrays.asList("get", "patch"),
serviceAccountName + "-secrets");
}

private void ensureRoleWithBinding(
Client k8sClient,
String roleName,
List<String> resources,
@Nullable List<String> resourceNames,
sparkoo marked this conversation as resolved.
Show resolved Hide resolved
List<String> apiGroups,
List<String> verbs,
String bindingName) {
ensureRole(k8sClient, roleName, resources, apiGroups, verbs);
ensureRole(k8sClient, roleName, resources, resourceNames, apiGroups, verbs);
//noinspection unchecked
roleBindings
.apply(k8sClient)
Expand Down Expand Up @@ -180,11 +196,17 @@ 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 verbs the verbs the role allows
* @return the role object for the given type of Client
*/
protected abstract R buildRole(
String name, List<String> resources, List<String> apiGroups, List<String> verbs);
String name,
List<String> resources,
@Nullable List<String> resourceNames,
sparkoo marked this conversation as resolved.
Show resolved Hide resolved
List<String> apiGroups,
List<String> verbs);

/**
* Builds a new role binding but does not persist it.
Expand Down Expand Up @@ -213,13 +235,14 @@ private void ensureRole(
Client k8sClient,
String name,
List<String> resources,
@Nullable List<String> resourceNames,
sparkoo marked this conversation as resolved.
Show resolved Hide resolved
List<String> apiGroups,
List<String> verbs) {
//noinspection unchecked
roles
.apply(k8sClient)
.inNamespace(namespace)
.createOrReplace(buildRole(name, resources, apiGroups, verbs));
.createOrReplace(buildRole(name, resources, resourceNames, apiGroups, verbs));
}

public interface ClientFactory<C extends KubernetesClient> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
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;
Expand Down Expand Up @@ -126,7 +127,8 @@ public KubernetesNamespace(
/**
* Prepare namespace for using.
*
* <p>Preparing includes creating if needed and waiting for default service account.
* <p>Preparing includes creating if needed and waiting for default service account, then it
* creates a secret for storing credentials.
*
* <p>The method will try to label the namespace with provided `labels`. It does not matter if the
* namespace already exists or we create new one. If update labels operation fail due to lack of
Expand All @@ -148,6 +150,14 @@ void prepare(boolean canCreate, Map<String, String> labels) throws Infrastructur
format("Creating the namespace '%s' is not allowed, yet it was not found.", name));
}
namespace = create(name, client);
Secret secret =
mshaposhnik marked this conversation as resolved.
Show resolved Hide resolved
new SecretBuilder()
.withType("opaque")
.withNewMetadata()
.withName("che-credentials-secret")
.endMetadata()
.build();
client.secrets().inNamespace(name).create(secret);
}
label(namespace, labels);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
*/
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;
Expand All @@ -20,6 +22,7 @@
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;

/**
Expand Down Expand Up @@ -50,14 +53,19 @@ public KubernetesWorkspaceServiceAccount(

@Override
protected Role buildRole(
String name, List<String> resources, List<String> apiGroups, List<String> verbs) {
String name,
List<String> resources,
@Nullable List<String> resourceNames,
List<String> apiGroups,
List<String> verbs) {
return new RoleBuilder()
.withNewMetadata()
.withName(name)
.endMetadata()
.withRules(
new PolicyRuleBuilder()
.withResources(resources)
.withResourceNames(resourceNames != null ? resourceNames : emptyList())
sparkoo marked this conversation as resolved.
Show resolved Hide resolved
.withApiGroups(apiGroups)
.withVerbs(verbs)
.build())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,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.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.anyMap;
Expand Down Expand Up @@ -43,6 +44,7 @@
import io.fabric8.kubernetes.api.model.ServiceAccountList;
import io.fabric8.kubernetes.api.model.Status;
import io.fabric8.kubernetes.api.model.rbac.ClusterRoleBuilder;
import io.fabric8.kubernetes.api.model.rbac.PolicyRule;
import io.fabric8.kubernetes.api.model.rbac.Role;
import io.fabric8.kubernetes.api.model.rbac.RoleBindingList;
import io.fabric8.kubernetes.api.model.rbac.RoleList;
Expand All @@ -57,6 +59,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
Expand Down Expand Up @@ -651,7 +654,7 @@ public void shouldBindToAllConfiguredClusterRoles() throws Exception {

RoleList roles = k8sClient.rbac().roles().inNamespace("workspace123").list();
assertEquals(
Sets.newHashSet("workspace-view", "workspace-metrics", "exec"),
Sets.newHashSet("workspace-view", "workspace-metrics", "workspace-secrets", "exec"),
roles.getItems().stream().map(r -> r.getMetadata().getName()).collect(Collectors.toSet()));
RoleBindingList bindings = k8sClient.rbac().roleBindings().inNamespace("workspace123").list();
assertEquals(
Expand All @@ -665,7 +668,65 @@ public void shouldBindToAllConfiguredClusterRoles() throws Exception {
"serviceAccount-cluster0",
"serviceAccount-cluster1",
"serviceAccount-view",
"serviceAccount-exec"));
"serviceAccount-exec",
"serviceAccount-secrets"));
}

@Test
public void shouldCreateAndBindCredentialsSecretRole() throws Exception {
// given
namespaceFactory =
spy(
new KubernetesNamespaceFactory(
"serviceAccount",
"cr2, cr3",
"<username>-che",
true,
true,
NAMESPACE_LABELS,
NAMESPACE_ANNOTATIONS,
clientFactory,
cheClientFactory,
userManager,
preferenceManager,
pool));
KubernetesNamespace toReturnNamespace = mock(KubernetesNamespace.class);
when(toReturnNamespace.getWorkspaceId()).thenReturn("workspace123");
when(toReturnNamespace.getName()).thenReturn("workspace123");
doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespaceAccess(any(), any());
when(clientFactory.create(any())).thenReturn(k8sClient);

// when
RuntimeIdentity identity =
new RuntimeIdentityImpl("workspace123", null, USER_ID, "workspace123");
namespaceFactory.getOrCreate(identity);

// then
Optional<Role> roleOptional =
k8sClient
.rbac()
.roles()
.inNamespace("workspace123")
.list()
.getItems()
.stream()
.filter(r -> r.getMetadata().getName().equals(SECRETS_ROLE_NAME))
.findAny();
assertTrue(roleOptional.isPresent());
PolicyRule rule = roleOptional.get().getRules().get(0);
assertEquals(rule.getResources(), singletonList("secrets"));
assertEquals(rule.getResourceNames(), singletonList("che-credentials-secret"));
assertEquals(rule.getApiGroups(), singletonList(""));
assertEquals(rule.getVerbs(), Arrays.asList("get", "patch"));
assertTrue(
k8sClient
.rbac()
.roleBindings()
.inNamespace("workspace123")
.list()
.getItems()
.stream()
.anyMatch(rb -> rb.getMetadata().getName().equals("serviceAccount-secrets")));
}

@Test
Expand Down Expand Up @@ -706,7 +767,7 @@ public void shouldCreateExecAndViewRolesAndBindings() throws Exception {

RoleList roles = k8sClient.rbac().roles().inNamespace("workspace123").list();
assertEquals(
Sets.newHashSet("workspace-view", "workspace-metrics", "exec"),
Sets.newHashSet("workspace-view", "workspace-metrics", "workspace-secrets", "exec"),
roles.getItems().stream().map(r -> r.getMetadata().getName()).collect(Collectors.toSet()));
Role role1 = roles.getItems().get(0);
Role role2 = roles.getItems().get(1);
Expand All @@ -723,7 +784,11 @@ public void shouldCreateExecAndViewRolesAndBindings() throws Exception {
.stream()
.map(r -> r.getMetadata().getName())
.collect(Collectors.toSet()),
Sets.newHashSet("serviceAccount-metrics", "serviceAccount-view", "serviceAccount-exec"));
Sets.newHashSet(
"serviceAccount-metrics",
"serviceAccount-view",
"serviceAccount-exec",
"serviceAccount-secrets"));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
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;
Expand All @@ -29,6 +30,7 @@

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;
Expand Down Expand Up @@ -88,8 +90,9 @@ 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().doReturn(namespaceOperation).when(mixedOperation).inNamespace(anyString());
lenient().when(mixedOperation.inNamespace(anyString())).thenReturn(namespaceOperation);
lenient().when(namespaceOperation.withName(anyString())).thenReturn(serviceAccountResource);
lenient().when(serviceAccountResource.get()).thenReturn(mock(ServiceAccount.class));
Expand Down Expand Up @@ -120,6 +123,7 @@ public void testKubernetesNamespacePreparingWhenNamespaceExists() throws Excepti

// then
verify(namespaceOperation, never()).create(any(Namespace.class));
verify(kubernetesClient, never()).secrets();
}

@Test
Expand All @@ -135,9 +139,14 @@ public void testKubernetesNamespacePreparingCreationWhenNamespaceDoesNotExist()
namespace.prepare(true, Map.of());

// then
ArgumentCaptor<Namespace> captor = ArgumentCaptor.forClass(Namespace.class);
verify(namespaceOperation).create(captor.capture());
Assert.assertEquals(captor.getValue().getMetadata().getName(), NAMESPACE);
ArgumentCaptor<Namespace> namespaceCaptor = ArgumentCaptor.forClass(Namespace.class);
ArgumentCaptor<Secret> 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(), "che-credentials-secret");
Assert.assertEquals(secret.getType(), "opaque");
}

@Test(expectedExceptions = InfrastructureException.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
*/
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;
Expand All @@ -21,6 +23,7 @@
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;

Expand Down Expand Up @@ -55,14 +58,19 @@ class OpenShiftWorkspaceServiceAccount

@Override
protected Role buildRole(
String name, List<String> resources, List<String> apiGroups, List<String> verbs) {
String name,
List<String> resources,
@Nullable List<String> resourceNames,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pass empty list, instread of nullable

List<String> apiGroups,
List<String> verbs) {
return new RoleBuilder()
.withNewMetadata()
.withName(name)
.endMetadata()
.withRules(
new PolicyRuleBuilder()
.withResources(resources)
.withResourceNames(resourceNames != null ? resourceNames : emptyList())
.withApiGroups(apiGroups)
.withVerbs(verbs)
.build())
Expand Down