Skip to content

Commit

Permalink
Improve error handling around loading recipes and add tests for Routes
Browse files Browse the repository at this point in the history
- Avoid error internal server error when yaml in recipe is
  invalid (e.g. specifies "Route1")
- Add validation for Routes in recipes (must refer to Service also in
  recipe)
- Add test cases for OpenShiftEnvironmentFactory handling of Routes
- Minor cleanup and simplification.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
  • Loading branch information
amisevsk committed Feb 1, 2019
1 parent d2e5fed commit 2f4c7c3
Show file tree
Hide file tree
Showing 5 changed files with 217 additions and 14 deletions.
Expand Up @@ -24,6 +24,7 @@
import io.fabric8.kubernetes.api.model.Service;
import io.fabric8.kubernetes.api.model.apps.Deployment;
import io.fabric8.kubernetes.api.model.extensions.Ingress;
import io.fabric8.kubernetes.client.KubernetesClientException;
import java.io.ByteArrayInputStream;
import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -97,8 +98,20 @@ protected KubernetesEnvironment doCreate(
+ "application/x-yaml, text/yaml, text/x-yaml");
}

final KubernetesList list =
clientFactory.create().lists().load(new ByteArrayInputStream(content.getBytes())).get();
final KubernetesList list;
try {
list =
clientFactory.create().lists().load(new ByteArrayInputStream(content.getBytes())).get();
} catch (KubernetesClientException e) {
// KubernetesClient wraps the error when a JsonMappingException occurs so we need the cause
String message = e.getCause() == null ? e.getMessage() : e.getCause().getMessage();
if (message.contains("\n")) {
// Clean up message if it comes from JsonMappingException. Format is e.g.
// `No resource type found for:v1#Route1\n at [...]`
message = message.split("\\n", 2)[0];
}
throw new ValidationException(format("Could not parse Kubernetes recipe: %s", message));
}

Map<String, Pod> pods = new HashMap<>();
Map<String, Deployment> deployments = new HashMap<>();
Expand Down Expand Up @@ -134,7 +147,9 @@ protected KubernetesEnvironment doCreate(
configMaps.put(configMap.getMetadata().getName(), configMap);
} else {
throw new ValidationException(
format("Found unknown object type '%s'", object.getMetadata()));
format(
"Found unknown object type in recipe -- name: '%s', kind: '%s'",
object.getMetadata().getName(), object.getKind()));
}
}

Expand Down
Expand Up @@ -23,6 +23,7 @@
import io.fabric8.kubernetes.api.model.Secret;
import io.fabric8.kubernetes.api.model.Service;
import io.fabric8.kubernetes.api.model.apps.Deployment;
import io.fabric8.kubernetes.client.KubernetesClientException;
import io.fabric8.openshift.api.model.DeploymentConfig;
import io.fabric8.openshift.api.model.Route;
import java.io.ByteArrayInputStream;
Expand All @@ -39,7 +40,6 @@
import org.eclipse.che.api.workspace.server.spi.environment.*;
import org.eclipse.che.commons.annotation.Nullable;
import org.eclipse.che.workspace.infrastructure.kubernetes.Names;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironmentValidator;
import org.eclipse.che.workspace.infrastructure.kubernetes.util.Containers;
import org.eclipse.che.workspace.infrastructure.openshift.OpenShiftClientFactory;

Expand All @@ -51,7 +51,7 @@
public class OpenShiftEnvironmentFactory extends InternalEnvironmentFactory<OpenShiftEnvironment> {

private final OpenShiftClientFactory clientFactory;
private final KubernetesEnvironmentValidator envValidator;
private final OpenShiftEnvironmentValidator envValidator;
private final MemoryAttributeProvisioner memoryProvisioner;

@Inject
Expand All @@ -60,7 +60,7 @@ public OpenShiftEnvironmentFactory(
RecipeRetriever recipeRetriever,
MachineConfigsValidator machinesValidator,
OpenShiftClientFactory clientFactory,
KubernetesEnvironmentValidator envValidator,
OpenShiftEnvironmentValidator envValidator,
MemoryAttributeProvisioner memoryProvisioner) {
super(installerRegistry, recipeRetriever, machinesValidator);
this.clientFactory = clientFactory;
Expand Down Expand Up @@ -96,8 +96,20 @@ protected OpenShiftEnvironment doCreate(
+ "application/x-yaml, text/yaml, text/x-yaml");
}

final KubernetesList list =
clientFactory.create().lists().load(new ByteArrayInputStream(content.getBytes())).get();
final KubernetesList list;
try {
list =
clientFactory.create().lists().load(new ByteArrayInputStream(content.getBytes())).get();
} catch (KubernetesClientException e) {
// KubernetesClient wraps the error when a JsonMappingException occurs so we need the cause
String message = e.getCause() == null ? e.getMessage() : e.getCause().getMessage();
if (message.contains("\n")) {
// Clean up message if it comes from JsonMappingException. Format is e.g.
// `No resource type found for:v1#Route1\n at [...]`
message = message.split("\\n", 2)[0];
}
throw new ValidationException(format("Could not parse OpenShift recipe: %s", message));
}

Map<String, Pod> pods = new HashMap<>();
Map<String, Deployment> deployments = new HashMap<>();
Expand All @@ -115,8 +127,6 @@ protected OpenShiftEnvironment doCreate(
throw new ValidationException("Supporting of deployment configs is not implemented yet.");
} else if (object instanceof Pod) {
Pod pod = (Pod) object;
checkNotNull(pod.getMetadata(), "Pod metadata must not be null");
checkNotNull(pod.getMetadata().getName(), "Pod metadata name must not be null");
pods.put(pod.getMetadata().getName(), pod);
} else if (object instanceof Deployment) {
Deployment deployment = (Deployment) object;
Expand All @@ -138,7 +148,9 @@ protected OpenShiftEnvironment doCreate(
configMaps.put(configMap.getMetadata().getName(), configMap);
} else {
throw new ValidationException(
format("Found unknown object type '%s'", object.getMetadata()));
format(
"Found unknown object type in recipe -- name: '%s', kind: '%s'",
object.getMetadata().getName(), object.getKind()));
}
}

Expand Down
@@ -0,0 +1,63 @@
/*
* 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 io.fabric8.openshift.api.model.Route;
import java.util.Set;
import java.util.stream.Collectors;
import javax.inject.Inject;
import org.eclipse.che.api.core.ValidationException;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironmentPodsValidator;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironmentValidator;

/**
* Adds additional OpenShift-specific validation to {@link KubernetesEnvironmentValidator}
*
* @author Angel Misevski
*/
public class OpenShiftEnvironmentValidator extends KubernetesEnvironmentValidator {

private static final String SERVICE_KIND = "Service";

@Inject
public OpenShiftEnvironmentValidator(KubernetesEnvironmentPodsValidator podsValidator) {
super(podsValidator);
}

public void validate(OpenShiftEnvironment env) throws ValidationException {
super.validate(env);
validateRoutesMatchServices(env);
}

private void validateRoutesMatchServices(OpenShiftEnvironment env) throws ValidationException {
Set<String> recipeServices =
env.getServices()
.values()
.stream()
.map(s -> s.getMetadata().getName())
.collect(Collectors.toSet());
for (Route route : env.getRoutes().values()) {
if (route.getSpec() == null
|| route.getSpec().getTo() == null
|| !route.getSpec().getTo().getKind().equals(SERVICE_KIND)) {
continue;
}
String serviceName = route.getSpec().getTo().getName();
if (!recipeServices.contains(serviceName)) {
throw new ValidationException(
String.format(
"Route '%s' refers to Service '%s'. Routes must refer to Services included in recipe",
route.getMetadata().getName(), serviceName));
}
}
}
}
Expand Up @@ -65,7 +65,6 @@
import org.eclipse.che.api.workspace.server.spi.environment.InternalRecipe;
import org.eclipse.che.api.workspace.server.spi.environment.MemoryAttributeProvisioner;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironmentValidator;
import org.eclipse.che.workspace.infrastructure.openshift.OpenShiftClientFactory;
import org.mockito.Mock;
import org.mockito.testng.MockitoTestNGListener;
Expand All @@ -89,7 +88,7 @@ public class OpenShiftEnvironmentFactoryTest {
private OpenShiftEnvironmentFactory osEnvFactory;

@Mock private OpenShiftClientFactory clientFactory;
@Mock private KubernetesEnvironmentValidator k8sEnvValidator;
@Mock private OpenShiftEnvironmentValidator openShiftEnvValidator;
@Mock private OpenShiftClient client;
@Mock private InternalRecipe internalRecipe;
@Mock private KubernetesListMixedOperation listMixedOperation;
Expand All @@ -108,7 +107,7 @@ public class OpenShiftEnvironmentFactoryTest {
public void setup() throws Exception {
osEnvFactory =
new OpenShiftEnvironmentFactory(
null, null, null, clientFactory, k8sEnvValidator, memoryProvisioner);
null, null, null, clientFactory, openShiftEnvValidator, memoryProvisioner);
when(clientFactory.create()).thenReturn(client);
when(client.lists()).thenReturn(listMixedOperation);
when(listMixedOperation.load(any(InputStream.class))).thenReturn(serverGettable);
Expand Down Expand Up @@ -154,6 +153,7 @@ public void shouldCreateOpenShiftEnvironmentWithPVCsFromRecipe() throws Exceptio
assertEquals(osEnv.getPersistentVolumeClaims().get("pvc2"), pvc2);
}

@Test
public void addRoutesWhenRecipeContainsThem() throws Exception {
Route route = new RouteBuilder().withNewMetadata().withName("test-route").endMetadata().build();
final List<HasMetadata> recipeObjects = singletonList(route);
Expand Down
@@ -0,0 +1,113 @@
/*
* 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 static org.mockito.Mockito.when;

import com.google.common.collect.ImmutableMap;
import io.fabric8.kubernetes.api.model.Service;
import io.fabric8.kubernetes.api.model.ServiceBuilder;
import io.fabric8.openshift.api.model.Route;
import io.fabric8.openshift.api.model.RouteBuilder;
import java.util.Map;
import org.eclipse.che.api.core.ValidationException;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironmentPodsValidator;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.testng.MockitoTestNGListener;
import org.testng.annotations.Listeners;
import org.testng.annotations.Test;

/**
* Tests {@link OpenShiftEnvironmentValidator}.
*
* @author Angel Misevski
*/
@Listeners(MockitoTestNGListener.class)
public class OpenShiftEnvironmentValidatorTest {
@Mock private KubernetesEnvironmentPodsValidator podsValidator;

@Mock private OpenShiftEnvironment environment;

@InjectMocks private OpenShiftEnvironmentValidator envValidator;

@Test
public void shouldDoNothingWhenRoutesMatchServices() throws Exception {
// Given
Map<String, Service> services = ImmutableMap.of("service1", makeService("service1"));
Map<String, Route> routes = ImmutableMap.of("route1", makeRoute("route1", "service1"));
when(environment.getRoutes()).thenReturn(routes);
when(environment.getServices()).thenReturn(services);

// When
envValidator.validate(environment);

// Then (nothing)
}

@Test
public void shouldDoNothingWhenRoutesDoNotHaveToField() throws Exception {
// Given
Map<String, Route> routes = ImmutableMap.of("route1", makeRoute("route1", null));
when(environment.getRoutes()).thenReturn(routes);

// When
envValidator.validate(environment);

// Then (nothing)
}

@Test(expectedExceptions = ValidationException.class)
public void shouldThrowExceptionWhenRouteRefersToMissingService() throws Exception {
// Given
Map<String, Service> services = ImmutableMap.of("service1", makeService("service1"));
Map<String, Route> routes = ImmutableMap.of("route1", makeRoute("route1", "notservice1"));
when(environment.getRoutes()).thenReturn(routes);
when(environment.getServices()).thenReturn(services);

// When
envValidator.validate(environment);

// Then (ValidationException)
}

private Service makeService(String serviceName) {
return new ServiceBuilder()
.withNewMetadata()
.withName(serviceName)
.endMetadata()
.withNewSpec()
.endSpec()
.build();
}

/**
* Make route for use in tests. If serviceName is null, return a route that does not refer to a
* service.
*/
private Route makeRoute(String routeName, String serviceName) {
RouteBuilder routeBuilder =
new RouteBuilder().withNewMetadata().withName(routeName).endMetadata();
if (serviceName != null) {
routeBuilder
.withNewSpec()
.withNewTo()
.withKind("Service")
.withName(serviceName)
.endTo()
.endSpec();
} else {
routeBuilder.withNewSpec().endSpec();
}
return routeBuilder.build();
}
}

0 comments on commit 2f4c7c3

Please sign in to comment.