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.

Sign up for GitHub

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

Add a role allowing che SA to stop workspaces #16532

Merged
merged 5 commits into from
Apr 21, 2020
Merged

Add a role allowing che SA to stop workspaces #16532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
60e17b3
Add a workspace-stop role and rolebinding to allow che service accoun…
Apr 1, 2020
3fa9757
Add OpenShiftCheInstallationLocation to get ns of Che installation, a…
Apr 9, 2020
650f205
Do nothing if installation location cannot be determined, inject envi…
Apr 20, 2020
32e2cc9
che #15906 Adding 'che.workspace.stop.role.enabled' property in order…
ibuziuk Apr 21, 2020
e187145
Merge pull request #1 from ibuziuk/che-15906
Apr 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,10 @@ che.workspace.server.liveness_probes=wsagent/http,exec-agent/http,terminal,theia
# default 10MB=10485760
che.workspace.startup_debug_log_limit_bytes=10485760

# If true, 'stop-workspace' role with the edit privileges will be granted to the 'che' ServiceAccount.
# This configuration is mainly required for workspace idling when the OpenShift OAuth is enabled.
che.workspace.stop.role.enabled=true

### Templates

# Folder that contains JSON files with code templates and samples
Expand Down
48 changes: 48 additions & 0 deletions .../che/workspace/infrastructure/openshift/environment/OpenShiftCheInstallationLocation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright (c) 2012-2018 Red Hat, Inc.
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
*/
package org.eclipse.che.workspace.infrastructure.openshift.environment;

import com.google.inject.Inject;
import javax.inject.Named;
import javax.inject.Singleton;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

skabashnyuk marked this conversation as resolved.
Show resolved Hide resolved
/**
* OpenShiftCheInstallationLocation checks the KUBERNETES_NAMESPACE and POD_NAMESPACE environment
* variables to determine what namespace Che is installed in. Users should use this class to
* retrieve the installation namespace name.
*
* @author Tom George
*/
@Singleton
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 have test for this class?

public class OpenShiftCheInstallationLocation {
Copy link
Member

Choose a reason for hiding this comment

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

nice, IMO in future we need to standardize the env var name and always use KUBERNETES_NAMESPACE that is defined in the operator

Copy link
Member

Choose a reason for hiding this comment

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

Adding docs would be also really nice to have

Copy link
Contributor

Choose a reason for hiding this comment

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

While this PR is to address an issue with OpenShift, it's a little strange to me that this class is in the openshift infra rather than k8s. Not a huge issue though.

Copy link
Member

Choose a reason for hiding this comment

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

good point, the problem is that we do need this PR in 7.12.0 and I'm not sure if @tomgeorge has time for further refactoring


private static final Logger LOG = LoggerFactory.getLogger(OpenShiftCheInstallationLocation.class);

@Inject(optional = true)
@Named("env.KUBERNETES_NAMESPACE")
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using constructor injection. It would be much easier to test.

Copy link
Contributor

Choose a reason for hiding this comment

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

And javax.inject instead of google inject.

Copy link
Member

Choose a reason for hiding this comment

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

@skabashnyuk I believe we need to refactor this class based on the valid comments in the review e.g. move to different location + remove completely the usage of env.POD_NAMESPACE. wondering if this will be ok do in a separate PR though, since we really need to have a fix in 7.12.0

Copy link
Contributor Author

@tomgeorge tomgeorge Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@skabashnyuk unfortunately Guice does not support constructor injection with optional dependencies[1]. We need to have these dependencies be optional for now because each installation method uses a different environment variable to specify the installation location. Perhaps when we refactor the installation methods to use the same environment variable, we can revisit this?

  1. https://github.com/google/guice/wiki/FrequentlyAskedQuestions#how-can-i-inject-optional-parameters-into-a-constructor

Copy link
Contributor

Choose a reason for hiding this comment

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

what about import org.eclipse.che.commons.annotation.Nullable?

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume it's not working this way

  @Inject
  public OpenShiftCheInstallationLocation(
      @Named("env.KUBERNETES_NAMESPACE") String kubernetesNamespace,
      @Named("env.POD_NAMESPACE") String podNamespace) {
    this.kubernetesNamespace = kubernetesNamespace;
    this.podNamespace = podNamespace;
  }

but should work

  @Inject
  public OpenShiftCheInstallationLocation(
   @Nullable  @Named("env.KUBERNETES_NAMESPACE") String kubernetesNamespace,
    @Nullable  @Named("env.POD_NAMESPACE") String podNamespace) {
    this.kubernetesNamespace = kubernetesNamespace;
    this.podNamespace = podNamespace;
  }

No?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/eclipse/che/blob/master/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectFactory.java#L56-L61

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, neither one worked. They both complained about not being able to find a binding for env.POD_NAMESPACE when it was not defined in the container, even though it was defined as @Nullable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think those work because they are defined in che.properties, either null or with a value. POD_NAMESPACE and KUBERNETES_NAMESPACE are not defined in those files, and @Nullable @Named("env.POD_NAMESPACE") String podNamespace fails to create the injector because there is no implementation bound to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

you right. Probably the first variant without injection was good too.

private String kubernetesNamespace = null;

@Inject(optional = true)
@Named("env.POD_NAMESPACE")
private String podNamespace = null;

/** @return The name of the namespace where Che is installed */
public String getInstallationLocationNamespace() {
if (kubernetesNamespace == null && podNamespace == null) {
LOG.warn(
"Neither KUBERNETES_NAMESPACE nor POD_NAMESPACE is defined. Unable to determine Che installation location");
}
return kubernetesNamespace == null ? podNamespace : kubernetesNamespace;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a warn if both podNamespace and kubernetesNamespace are defined but not matching would also make sense? I don't know if this can occur naturally but might be a hard-to-debug issue.

Copy link
Member

Choose a reason for hiding this comment

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

in general, we do need to standardize the env var name (use only KUBERNETES_NAMESPACE) and remove usage of POD_NAMESPACE e.g. in helm chart https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/templates/deployment.yaml#L38

But we should do it in a separate PRs I beleive after 7.12.0

}
}
6 changes: 5 additions & 1 deletion ...a/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.eclipse.che.workspace.infrastructure.openshift.Constants;
import org.eclipse.che.workspace.infrastructure.openshift.OpenShiftClientConfigFactory;
import org.eclipse.che.workspace.infrastructure.openshift.OpenShiftClientFactory;
import org.eclipse.che.workspace.infrastructure.openshift.provision.OpenShiftStopWorkspaceRoleProvisioner;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -52,6 +53,7 @@ public class OpenShiftProjectFactory extends KubernetesNamespaceFactory {
private static final Logger LOG = LoggerFactory.getLogger(OpenShiftProjectFactory.class);

private final OpenShiftClientFactory clientFactory;
private final OpenShiftStopWorkspaceRoleProvisioner stopWorkspaceRoleProvisioner;

@Inject
public OpenShiftProjectFactory(
Expand All @@ -63,6 +65,7 @@ public OpenShiftProjectFactory(
boolean allowUserDefinedNamespaces,
OpenShiftClientFactory clientFactory,
OpenShiftClientConfigFactory clientConfigFactory,
OpenShiftStopWorkspaceRoleProvisioner stopWorkspaceRoleProvisioner,
UserManager userManager,
KubernetesSharedPool sharedPool) {
super(
Expand All @@ -81,6 +84,7 @@ public OpenShiftProjectFactory(
+ "OAuth to personalize credentials that will be used for cluster access.");
}
this.clientFactory = clientFactory;
this.stopWorkspaceRoleProvisioner = stopWorkspaceRoleProvisioner;
}

public OpenShiftProject getOrCreate(RuntimeIdentity identity) throws InfrastructureException {
Expand All @@ -93,7 +97,7 @@ public OpenShiftProject getOrCreate(RuntimeIdentity identity) throws Infrastruct
doCreateServiceAccount(osProject.getWorkspaceId(), osProject.getName());
osWorkspaceServiceAccount.prepare();
}

stopWorkspaceRoleProvisioner.provision(osProject.getName());
return osProject;
}

Expand Down
119 changes: 119 additions & 0 deletions ...e/workspace/infrastructure/openshift/provision/OpenShiftStopWorkspaceRoleProvisioner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/*
* Copyright (c) 2012-2018 Red Hat, Inc.
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
*/
package org.eclipse.che.workspace.infrastructure.openshift.provision;

import io.fabric8.kubernetes.api.model.ObjectReferenceBuilder;
import io.fabric8.openshift.api.model.*;
import io.fabric8.openshift.client.OpenShiftClient;
import javax.inject.Inject;
import javax.inject.Named;
import org.eclipse.che.api.workspace.server.spi.InfrastructureException;
import org.eclipse.che.workspace.infrastructure.openshift.OpenShiftClientFactory;
import org.eclipse.che.workspace.infrastructure.openshift.environment.OpenShiftCheInstallationLocation;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

skabashnyuk marked this conversation as resolved.
Show resolved Hide resolved
/**
* This class creates the necessary role and rolebindings to allow the che serviceaccount to stop
* user workspaces.
*
* @author Tom George
*/
public class OpenShiftStopWorkspaceRoleProvisioner {
skabashnyuk marked this conversation as resolved.
Show resolved Hide resolved

private final OpenShiftClientFactory clientFactory;
private final String installationLocation;
private final boolean stopWorkspaceRoleEnabled;

private static final Logger LOG = LoggerFactory.getLogger(OpenShiftCheInstallationLocation.class);

@Inject
public OpenShiftStopWorkspaceRoleProvisioner(
OpenShiftClientFactory clientFactory,
OpenShiftCheInstallationLocation installationLocation,
@Named("che.workspace.stop.role.enabled") boolean stopWorkspaceRoleEnabled) {
this.clientFactory = clientFactory;
this.installationLocation = installationLocation.getInstallationLocationNamespace();
this.stopWorkspaceRoleEnabled = stopWorkspaceRoleEnabled;
}

public void provision(String projectName) throws InfrastructureException {
Copy link
Member

Choose a reason for hiding this comment

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

technically speaking installationLocation can be null based on implementation, so in this case can we log error and avoid doing anything in the provision method?

Copy link
Member

Choose a reason for hiding this comment

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

@tomgeorge can we check getInstallationLocationNamespace here and do nothing (log error) if null?

if (stopWorkspaceRoleEnabled && installationLocation != null) {
OpenShiftClient osClient = clientFactory.createOC();
String stopWorkspacesRoleName = "workspace-stop";
if (osClient.roles().inNamespace(projectName).withName(stopWorkspacesRoleName).get()
== null) {
osClient
.roles()
.inNamespace(projectName)
.createOrReplace(createStopWorkspacesRole(stopWorkspacesRoleName));
}
osClient
.roleBindings()
.inNamespace(projectName)
.createOrReplace(createStopWorkspacesRoleBinding(projectName));
} else {
LOG.warn(
"Stop workspace Role and RoleBinding will not be provisioned to the '{}' namespace. 'che.workspace.stop.role.enabled' property is set to '{}'",
installationLocation,
stopWorkspaceRoleEnabled);
}
}

protected OpenshiftRole createStopWorkspacesRole(String name) {
return new OpenshiftRoleBuilder()
.withNewMetadata()
.withName(name)
.endMetadata()
.withRules(
new PolicyRuleBuilder()
.withApiGroups("")
.withResources("pods")
.withVerbs("get", "list", "watch", "delete")
.build(),
new PolicyRuleBuilder()
.withApiGroups("")
.withResources("configmaps", "services", "secrets")
.withVerbs("delete", "list", "get")
.build(),
new PolicyRuleBuilder()
.withApiGroups("route.openshift.io")
.withResources("routes")
.withVerbs("delete", "list")
.build(),
new PolicyRuleBuilder()
.withApiGroups("apps")
.withResources("deployments", "replicasets")
.withVerbs("delete", "list", "get", "patch")
.build())
.build();
}

protected OpenshiftRoleBinding createStopWorkspacesRoleBinding(String projectName) {
return new OpenshiftRoleBindingBuilder()
.withNewMetadata()
.withName("che-workspace-stop")
.withNamespace(projectName)
.endMetadata()
.withNewRoleRef()
.withName("workspace-stop")
.withNamespace(projectName)
.endRoleRef()
.withSubjects(
new ObjectReferenceBuilder()
.withKind("ServiceAccount")
.withName("che")
.withNamespace(installationLocation)
.build())
.build();
}
}
74 changes: 68 additions & 6 deletions ...g/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import org.eclipse.che.workspace.infrastructure.kubernetes.util.KubernetesSharedPool;
import org.eclipse.che.workspace.infrastructure.openshift.OpenShiftClientConfigFactory;
import org.eclipse.che.workspace.infrastructure.openshift.OpenShiftClientFactory;
import org.eclipse.che.workspace.infrastructure.openshift.provision.OpenShiftStopWorkspaceRoleProvisioner;
import org.mockito.Mock;
import org.mockito.testng.MockitoTestNGListener;
import org.testng.annotations.BeforeMethod;
Expand All @@ -76,6 +77,7 @@ public class OpenShiftProjectFactoryTest {

@Mock private OpenShiftClientConfigFactory configFactory;
@Mock private OpenShiftClientFactory clientFactory;
@Mock private OpenShiftStopWorkspaceRoleProvisioner stopWorkspaceRoleProvisioner;
@Mock private WorkspaceManager workspaceManager;
@Mock private UserManager userManager;
@Mock private KubernetesSharedPool pool;
Expand Down Expand Up @@ -113,7 +115,16 @@ public void shouldNotThrowExceptionIfDefaultNamespaceIsSpecifiedOnCheckingIfName
throws Exception {
projectFactory =
new OpenShiftProjectFactory(
"legacy", "", "", "defaultNs", false, clientFactory, configFactory, userManager, pool);
"legacy",
"",
"",
"defaultNs",
false,
clientFactory,
configFactory,
stopWorkspaceRoleProvisioner,
userManager,
pool);

projectFactory.checkIfNamespaceIsAllowed("defaultNs");
}
Expand All @@ -124,7 +135,16 @@ public void shouldNotThrowExceptionIfDefaultNamespaceIsSpecifiedOnCheckingIfName
throws Exception {
projectFactory =
new OpenShiftProjectFactory(
"legacy", "", "", "defaultNs", true, clientFactory, configFactory, userManager, pool);
"legacy",
"",
"",
"defaultNs",
true,
clientFactory,
configFactory,
stopWorkspaceRoleProvisioner,
userManager,
pool);

projectFactory.checkIfNamespaceIsAllowed("any-namespace");
}
Expand All @@ -138,7 +158,16 @@ public void shouldNotThrowExceptionIfDefaultNamespaceIsSpecifiedOnCheckingIfName
throws Exception {
projectFactory =
new OpenShiftProjectFactory(
"legacy", "", "", "defaultNs", false, clientFactory, configFactory, userManager, pool);
"legacy",
"",
"",
"defaultNs",
false,
clientFactory,
configFactory,
stopWorkspaceRoleProvisioner,
userManager,
pool);

projectFactory.checkIfNamespaceIsAllowed("any-namespace");
}
Expand All @@ -151,7 +180,16 @@ public void shouldNotThrowExceptionIfDefaultNamespaceIsSpecifiedOnCheckingIfName
throws Exception {
projectFactory =
new OpenShiftProjectFactory(
"projectName", "", "", null, false, clientFactory, configFactory, userManager, pool);
"projectName",
"",
"",
null,
false,
clientFactory,
configFactory,
stopWorkspaceRoleProvisioner,
userManager,
pool);
}

@Test
Expand Down Expand Up @@ -182,6 +220,7 @@ public void shouldReturnDefaultProjectWhenItExistsAndUserDefinedIsNotAllowed() t
false,
clientFactory,
configFactory,
stopWorkspaceRoleProvisioner,
userManager,
pool);

Expand Down Expand Up @@ -213,6 +252,7 @@ public void shouldReturnDefaultProjectWhenItDoesNotExistAndUserDefinedIsNotAllow
false,
clientFactory,
configFactory,
stopWorkspaceRoleProvisioner,
userManager,
pool);

Expand Down Expand Up @@ -244,6 +284,7 @@ public void shouldThrowExceptionWhenFailedToGetInfoAboutDefaultNamespace() throw
false,
clientFactory,
configFactory,
stopWorkspaceRoleProvisioner,
userManager,
pool);

Expand All @@ -260,7 +301,16 @@ public void shouldReturnListOfExistingProjectsAlongWithDefaultIfUserDefinedIsAll

projectFactory =
new OpenShiftProjectFactory(
"predefined", "", "", "default", true, clientFactory, configFactory, userManager, pool);
"predefined",
"",
"",
"default",
true,
clientFactory,
configFactory,
stopWorkspaceRoleProvisioner,
userManager,
pool);

List<KubernetesNamespaceMeta> availableNamespaces = projectFactory.list();

Expand Down Expand Up @@ -291,7 +341,16 @@ public void shouldReturnListOfExistingProjectsAlongWithNonExistingDefaultIfUserD

projectFactory =
new OpenShiftProjectFactory(
"predefined", "", "", "default", true, clientFactory, configFactory, userManager, pool);
"predefined",
"",
"",
"default",
true,
clientFactory,
configFactory,
stopWorkspaceRoleProvisioner,
userManager,
pool);

List<KubernetesNamespaceMeta> availableNamespaces = projectFactory.list();
assertEquals(availableNamespaces.size(), 2);
Expand Down Expand Up @@ -324,6 +383,7 @@ public void shouldThrowExceptionWhenFailedToGetNamespaces() throws Exception {
true,
clientFactory,
configFactory,
stopWorkspaceRoleProvisioner,
userManager,
pool);

Expand All @@ -350,6 +410,7 @@ public void shouldRequireNamespacePriorExistenceIfDifferentFromDefaultAndUserDef
false,
clientFactory,
configFactory,
stopWorkspaceRoleProvisioner,
userManager,
pool));
OpenShiftProject toReturnProject = mock(OpenShiftProject.class);
Expand Down Expand Up @@ -380,6 +441,7 @@ public void shouldPrepareWorkspaceServiceAccountIfItIsConfiguredAndProjectIsNotP
false,
clientFactory,
configFactory,
stopWorkspaceRoleProvisioner,
userManager,
pool));
OpenShiftProject toReturnProject = mock(OpenShiftProject.class);
Expand Down
Loading