From 783ee26e10ad47a7593b6239dced9a9bf279a755 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Mon, 14 Jan 2019 17:33:49 -0500 Subject: [PATCH 1/4] Add support for routes in k8s/OpenShift recipes Add infrastructure side support for specifying routes in OpenShift/Kubernetes recipes. Signed-off-by: Angel Misevski --- .../infrastructure/openshift/Warnings.java | 5 ----- .../OpenShiftEnvironmentFactory.java | 13 ++++-------- .../OpenShiftEnvironmentFactoryTest.java | 21 +++++++------------ 3 files changed, 12 insertions(+), 27 deletions(-) diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/Warnings.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/Warnings.java index 35bc8352b51..fb19de66419 100644 --- a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/Warnings.java +++ b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/Warnings.java @@ -18,11 +18,6 @@ */ public final class Warnings { - public static final int ROUTE_IGNORED_WARNING_CODE = 5100; - public static final String ROUTES_IGNORED_WARNING_MESSAGE = - "Routes specified in OpenShift recipe are ignored. " - + "To expose ports please define servers in machine configuration."; - public static final int SECRET_IGNORED_WARNING_CODE = org.eclipse.che.workspace.infrastructure.kubernetes.Warnings.SECRET_IGNORED_WARNING_CODE; public static final String SECRET_IGNORED_WARNING_MESSAGE = diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java index 3ed8f2be56f..883d7928d56 100644 --- a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java +++ b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java @@ -106,7 +106,7 @@ protected OpenShiftEnvironment doCreate( Map services = new HashMap<>(); Map configMaps = new HashMap<>(); Map pvcs = new HashMap<>(); - boolean isAnyRoutePresent = false; + Map routes = new HashMap<>(); boolean isAnySecretPresent = false; for (HasMetadata object : list.getItems()) { checkNotNull(object.getKind(), "Environment contains object without specified kind field"); @@ -127,7 +127,8 @@ protected OpenShiftEnvironment doCreate( Service service = (Service) object; services.put(service.getMetadata().getName(), service); } else if (object instanceof Route) { - isAnyRoutePresent = true; + Route route = (Route) object; + routes.put(route.getMetadata().getName(), route); } else if (object instanceof PersistentVolumeClaim) { PersistentVolumeClaim pvc = (PersistentVolumeClaim) object; pvcs.put(pvc.getMetadata().getName(), pvc); @@ -142,12 +143,6 @@ protected OpenShiftEnvironment doCreate( } } - if (isAnyRoutePresent) { - warnings.add( - new WarningImpl( - Warnings.ROUTE_IGNORED_WARNING_CODE, Warnings.ROUTES_IGNORED_WARNING_MESSAGE)); - } - if (isAnySecretPresent) { warnings.add( new WarningImpl( @@ -167,7 +162,7 @@ protected OpenShiftEnvironment doCreate( .setPersistentVolumeClaims(pvcs) .setSecrets(new HashMap<>()) .setConfigMaps(configMaps) - .setRoutes(new HashMap<>()) + .setRoutes(routes) .build(); envValidator.validate(osEnv); diff --git a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java index 0942af9ac6a..e7cdd8dd1f9 100644 --- a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java +++ b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java @@ -17,8 +17,6 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; import static org.eclipse.che.workspace.infrastructure.kubernetes.Constants.MACHINE_NAME_ANNOTATION_FMT; -import static org.eclipse.che.workspace.infrastructure.openshift.Warnings.ROUTES_IGNORED_WARNING_MESSAGE; -import static org.eclipse.che.workspace.infrastructure.openshift.Warnings.ROUTE_IGNORED_WARNING_CODE; import static org.eclipse.che.workspace.infrastructure.openshift.Warnings.SECRET_IGNORED_WARNING_CODE; import static org.eclipse.che.workspace.infrastructure.openshift.Warnings.SECRET_IGNORED_WARNING_MESSAGE; import static org.mockito.ArgumentMatchers.any; @@ -56,6 +54,7 @@ import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder; import io.fabric8.kubernetes.client.dsl.KubernetesListMixedOperation; import io.fabric8.kubernetes.client.dsl.RecreateFromServerGettable; +import io.fabric8.openshift.api.model.Route; import io.fabric8.openshift.api.model.RouteBuilder; import io.fabric8.openshift.client.OpenShiftClient; import java.io.InputStream; @@ -158,22 +157,18 @@ public void shouldCreateOpenShiftEnvironmentWithPVCsFromRecipe() throws Exceptio assertEquals(osEnv.getPersistentVolumeClaims().get("pvc2"), pvc2); } - @Test - public void ignoreRoutesWhenRecipeContainsThem() throws Exception { - final List objects = - asList( - new RouteBuilder().withNewMetadata().withName("route1").endMetadata().build(), - new RouteBuilder().withNewMetadata().withName("route2").endMetadata().build()); - when(parsedList.getItems()).thenReturn(objects); + public void addRoutesWhenRecipeContainsThem() throws Exception { + Route route = new RouteBuilder().withNewMetadata().withName("test-route").endMetadata().build(); + final List recipeObjects = singletonList(route); + when(parsedList.getItems()).thenReturn(recipeObjects); final OpenShiftEnvironment parsed = osEnvFactory.doCreate(internalRecipe, emptyMap(), emptyList()); - assertTrue(parsed.getRoutes().isEmpty()); - assertEquals(parsed.getWarnings().size(), 1); + assertEquals(parsed.getRoutes().size(), 1); assertEquals( - parsed.getWarnings().get(0), - new WarningImpl(ROUTE_IGNORED_WARNING_CODE, ROUTES_IGNORED_WARNING_MESSAGE)); + parsed.getRoutes().get("test-route").getMetadata().getName(), + route.getMetadata().getName()); } @Test From 88a37fdad0103c183996a1d97d1f464f19026211 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Mon, 14 Jan 2019 17:43:17 -0500 Subject: [PATCH 2/4] Add support for secrets in k8s/OpenShift recipes Add infrastructure side support for secrets in Kubernetes / OpenShift recipes. Signed-off-by: Angel Misevski --- .../infrastructure/kubernetes/Warnings.java | 4 --- .../KubernetesEnvironmentFactory.java | 13 +++------ .../KubernetesEnvironmentFactoryTest.java | 18 ++++++------- .../infrastructure/openshift/Warnings.java | 27 ------------------- .../OpenShiftEnvironmentFactory.java | 15 +++-------- .../OpenShiftEnvironmentFactoryTest.java | 20 ++++++-------- 6 files changed, 24 insertions(+), 73 deletions(-) delete mode 100644 infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/Warnings.java diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/Warnings.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/Warnings.java index 444eaa9b2df..60520313184 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/Warnings.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/Warnings.java @@ -23,10 +23,6 @@ public final class Warnings { "Ingresses specified in Kubernetes recipe are ignored. " + "To expose ports please define servers in machine configuration."; - public static final int SECRET_IGNORED_WARNING_CODE = 4102; - public static final String SECRET_IGNORED_WARNING_MESSAGE = - "Secrets specified in Kubernetes recipe are ignored."; - public static final int RESTART_POLICY_SET_TO_NEVER_WARNING_CODE = 4104; public static final String RESTART_POLICY_SET_TO_NEVER_WARNING_MESSAGE_FMT = "Restart policy '%s' for pod '%s' is rewritten with %s"; diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactory.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactory.java index bd9d7f25833..7d6fcba2f0c 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactory.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactory.java @@ -105,8 +105,8 @@ protected KubernetesEnvironment doCreate( Map services = new HashMap<>(); Map configMaps = new HashMap<>(); Map pvcs = new HashMap<>(); + Map secrets = new HashMap<>(); boolean isAnyIngressPresent = false; - boolean isAnySecretPresent = false; for (HasMetadata object : list.getItems()) { checkNotNull(object.getKind(), "Environment contains object without specified kind field"); checkNotNull(object.getMetadata(), "%s metadata must not be null", object.getKind()); @@ -127,7 +127,8 @@ protected KubernetesEnvironment doCreate( PersistentVolumeClaim pvc = (PersistentVolumeClaim) object; pvcs.put(pvc.getMetadata().getName(), pvc); } else if (object instanceof Secret) { - isAnySecretPresent = true; + Secret secret = (Secret) object; + secrets.put(secret.getMetadata().getName(), secret); } else if (object instanceof ConfigMap) { ConfigMap configMap = (ConfigMap) object; configMaps.put(configMap.getMetadata().getName(), configMap); @@ -143,12 +144,6 @@ protected KubernetesEnvironment doCreate( Warnings.INGRESSES_IGNORED_WARNING_CODE, Warnings.INGRESSES_IGNORED_WARNING_MESSAGE)); } - if (isAnySecretPresent) { - warnings.add( - new WarningImpl( - Warnings.SECRET_IGNORED_WARNING_CODE, Warnings.SECRET_IGNORED_WARNING_MESSAGE)); - } - addRamAttributes(machines, pods.values()); KubernetesEnvironment k8sEnv = @@ -162,7 +157,7 @@ protected KubernetesEnvironment doCreate( .setPersistentVolumeClaims(pvcs) .setIngresses(new HashMap<>()) .setPersistentVolumeClaims(new HashMap<>()) - .setSecrets(new HashMap<>()) + .setSecrets(secrets) .setConfigMaps(configMaps) .build(); diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactoryTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactoryTest.java index d3678b55e8e..3322d604e72 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactoryTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactoryTest.java @@ -19,8 +19,6 @@ import static org.eclipse.che.workspace.infrastructure.kubernetes.Constants.MACHINE_NAME_ANNOTATION_FMT; import static org.eclipse.che.workspace.infrastructure.kubernetes.Warnings.INGRESSES_IGNORED_WARNING_CODE; import static org.eclipse.che.workspace.infrastructure.kubernetes.Warnings.INGRESSES_IGNORED_WARNING_MESSAGE; -import static org.eclipse.che.workspace.infrastructure.kubernetes.Warnings.SECRET_IGNORED_WARNING_CODE; -import static org.eclipse.che.workspace.infrastructure.kubernetes.Warnings.SECRET_IGNORED_WARNING_MESSAGE; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.lenient; @@ -50,6 +48,7 @@ import io.fabric8.kubernetes.api.model.PodTemplateSpecBuilder; import io.fabric8.kubernetes.api.model.Quantity; import io.fabric8.kubernetes.api.model.ResourceRequirements; +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.ServiceBuilder; @@ -178,20 +177,19 @@ public void ignoreIgressesWhenRecipeContainsThem() throws Exception { } @Test - public void ignoreSecretsWhenRecipeContainsThem() throws Exception { - final List recipeObjects = - singletonList( - new SecretBuilder().withNewMetadata().withName("secret").endMetadata().build()); + public void addSecretsWhenRecipeContainsThem() throws Exception { + Secret secret = + new SecretBuilder().withNewMetadata().withName("test-secret").endMetadata().build(); + final List recipeObjects = singletonList(secret); when(parsedList.getItems()).thenReturn(recipeObjects); final KubernetesEnvironment parsed = k8sEnvFactory.doCreate(internalRecipe, emptyMap(), emptyList()); - assertTrue(parsed.getSecrets().isEmpty()); - assertEquals(parsed.getWarnings().size(), 1); + assertEquals(parsed.getSecrets().size(), 1); assertEquals( - parsed.getWarnings().get(0), - new WarningImpl(SECRET_IGNORED_WARNING_CODE, SECRET_IGNORED_WARNING_MESSAGE)); + parsed.getSecrets().get("test-secret").getMetadata().getName(), + secret.getMetadata().getName()); } @Test diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/Warnings.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/Warnings.java deleted file mode 100644 index fb19de66419..00000000000 --- a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/Warnings.java +++ /dev/null @@ -1,27 +0,0 @@ -/* - * 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; - -/** - * Constants for Kubernetes infrastructure specific warnings. - * - * @author Sergii Leshchenko - */ -public final class Warnings { - - public static final int SECRET_IGNORED_WARNING_CODE = - org.eclipse.che.workspace.infrastructure.kubernetes.Warnings.SECRET_IGNORED_WARNING_CODE; - public static final String SECRET_IGNORED_WARNING_MESSAGE = - "Secrets specified in OpenShift recipe are ignored."; - - private Warnings() {} -} diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java index 883d7928d56..c4e39bbbc15 100644 --- a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java +++ b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java @@ -35,7 +35,6 @@ import org.eclipse.che.api.core.ValidationException; import org.eclipse.che.api.core.model.workspace.Warning; import org.eclipse.che.api.installer.server.InstallerRegistry; -import org.eclipse.che.api.workspace.server.model.impl.WarningImpl; import org.eclipse.che.api.workspace.server.spi.InfrastructureException; import org.eclipse.che.api.workspace.server.spi.environment.*; import org.eclipse.che.commons.annotation.Nullable; @@ -43,7 +42,6 @@ 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; -import org.eclipse.che.workspace.infrastructure.openshift.Warnings; /** * Parses {@link InternalEnvironment} into {@link OpenShiftEnvironment}. @@ -107,7 +105,7 @@ protected OpenShiftEnvironment doCreate( Map configMaps = new HashMap<>(); Map pvcs = new HashMap<>(); Map routes = new HashMap<>(); - boolean isAnySecretPresent = false; + Map secrets = new HashMap<>(); for (HasMetadata object : list.getItems()) { checkNotNull(object.getKind(), "Environment contains object without specified kind field"); checkNotNull(object.getMetadata(), "%s metadata must not be null", object.getKind()); @@ -133,7 +131,8 @@ protected OpenShiftEnvironment doCreate( PersistentVolumeClaim pvc = (PersistentVolumeClaim) object; pvcs.put(pvc.getMetadata().getName(), pvc); } else if (object instanceof Secret) { - isAnySecretPresent = true; + Secret secret = (Secret) object; + secrets.put(secret.getMetadata().getName(), secret); } else if (object instanceof ConfigMap) { ConfigMap configMap = (ConfigMap) object; configMaps.put(configMap.getMetadata().getName(), configMap); @@ -143,12 +142,6 @@ protected OpenShiftEnvironment doCreate( } } - if (isAnySecretPresent) { - warnings.add( - new WarningImpl( - Warnings.SECRET_IGNORED_WARNING_CODE, Warnings.SECRET_IGNORED_WARNING_MESSAGE)); - } - addRamAttributes(machines, pods.values()); OpenShiftEnvironment osEnv = @@ -160,7 +153,7 @@ protected OpenShiftEnvironment doCreate( .setDeployments(deployments) .setServices(services) .setPersistentVolumeClaims(pvcs) - .setSecrets(new HashMap<>()) + .setSecrets(secrets) .setConfigMaps(configMaps) .setRoutes(routes) .build(); diff --git a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java index e7cdd8dd1f9..30e7a0d7fa7 100644 --- a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java +++ b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java @@ -17,15 +17,12 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; import static org.eclipse.che.workspace.infrastructure.kubernetes.Constants.MACHINE_NAME_ANNOTATION_FMT; -import static org.eclipse.che.workspace.infrastructure.openshift.Warnings.SECRET_IGNORED_WARNING_CODE; -import static org.eclipse.che.workspace.infrastructure.openshift.Warnings.SECRET_IGNORED_WARNING_MESSAGE; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; -import static org.testng.Assert.assertTrue; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -47,6 +44,7 @@ import io.fabric8.kubernetes.api.model.PodTemplateSpecBuilder; import io.fabric8.kubernetes.api.model.Quantity; import io.fabric8.kubernetes.api.model.ResourceRequirements; +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.ServiceBuilder; @@ -63,7 +61,6 @@ import java.util.Map; import java.util.Set; import org.eclipse.che.api.core.ValidationException; -import org.eclipse.che.api.workspace.server.model.impl.WarningImpl; import org.eclipse.che.api.workspace.server.spi.environment.InternalMachineConfig; import org.eclipse.che.api.workspace.server.spi.environment.InternalRecipe; import org.eclipse.che.api.workspace.server.spi.environment.MemoryAttributeProvisioner; @@ -172,20 +169,19 @@ public void addRoutesWhenRecipeContainsThem() throws Exception { } @Test - public void ignoreSecretsWhenRecipeContainsThem() throws Exception { - final List recipeObjects = - singletonList( - new SecretBuilder().withNewMetadata().withName("secret").endMetadata().build()); + public void addSecretsWhenRecipeContainsThem() throws Exception { + Secret secret = + new SecretBuilder().withNewMetadata().withName("test-secret").endMetadata().build(); + final List recipeObjects = singletonList(secret); when(parsedList.getItems()).thenReturn(recipeObjects); final OpenShiftEnvironment parsed = osEnvFactory.doCreate(internalRecipe, emptyMap(), emptyList()); - assertTrue(parsed.getSecrets().isEmpty()); - assertEquals(parsed.getWarnings().size(), 1); + assertEquals(parsed.getSecrets().size(), 1); assertEquals( - parsed.getWarnings().get(0), - new WarningImpl(SECRET_IGNORED_WARNING_CODE, SECRET_IGNORED_WARNING_MESSAGE)); + parsed.getSecrets().get("test-secret").getMetadata().getName(), + secret.getMetadata().getName()); } @Test From 1dfd892f4e8d1516f2a85f33ec46a90f10242e60 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Mon, 21 Jan 2019 12:46:55 -0500 Subject: [PATCH 3/4] Add user dashboard validation for secrets in recipes Add mild validation (metadata, kind) for recipes containing secrets. Note that routes are currently unvalidated since they are not permissible in Kubernetes recipes (as they are OpenShift objects) and all validation is done through the kubernetes parser / validator. Signed-off-by: Angel Misevski --- .../kubernetes-environment-recipe-parser.ts | 10 +++++-- .../kubernetes-machine-recipe-parser.ts | 29 ++++++++++++++++++- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/dashboard/src/components/api/environment/kubernetes-environment-recipe-parser.ts b/dashboard/src/components/api/environment/kubernetes-environment-recipe-parser.ts index a50b33aa3aa..8f3f9cffaf1 100644 --- a/dashboard/src/components/api/environment/kubernetes-environment-recipe-parser.ts +++ b/dashboard/src/components/api/environment/kubernetes-environment-recipe-parser.ts @@ -10,7 +10,7 @@ * Red Hat, Inc. - initial API and implementation */ 'use strict'; -import {ISupportedListItem, KubernetesMachineRecipeParser} from './kubernetes-machine-recipe-parser'; +import {ISupportedListItem, KubernetesMachineRecipeParser, isSupportedItem} from './kubernetes-machine-recipe-parser'; import {IParser} from './parser'; export interface ISupportedItemList { @@ -79,7 +79,7 @@ export class KubernetesEnvironmentRecipeParser implements IParser { if (!angular.isArray(items) || items.length === 0) { throw new TypeError(`Recipe kubernetes list should contain at least one 'item'.`); } else { - items.forEach((item: ISupportedListItem) => { + items.forEach((item: any) => { if (!item) { return; } @@ -87,6 +87,12 @@ export class KubernetesEnvironmentRecipeParser implements IParser { if (item.kind && item.kind.toLowerCase() === 'service') { return; } + if (!isSupportedItem(item)) { + // should throw a TypeError here but this code is currently used to validate OpenShift recipes + // (which support Routes) as well as Kubernetes recipes, so we need to ignore some elements + // rather than complain. Returning here prevents warning about typos in the `kind` section. + return; + } this.machineRecipeParser.validate(item); }); } diff --git a/dashboard/src/components/api/environment/kubernetes-machine-recipe-parser.ts b/dashboard/src/components/api/environment/kubernetes-machine-recipe-parser.ts index 8279d90f474..c3fa89b642c 100644 --- a/dashboard/src/components/api/environment/kubernetes-machine-recipe-parser.ts +++ b/dashboard/src/components/api/environment/kubernetes-machine-recipe-parser.ts @@ -13,7 +13,11 @@ import {IParser} from './parser'; -export type ISupportedListItem = IPodItem | IDeploymentItem | IConfigMapItem; +export type ISupportedListItem = IPodItem | IDeploymentItem | IConfigMapItem | ISecretItem; + +export function isSupportedItem(item: any): item is ISupportedListItem { + return isDeploymentItem(item) || isPodItem(item) || isConfigMapItem(item) || isSecretItem(item); +} export function isDeploymentItem(item: ISupportedListItem): item is IDeploymentItem { return (item.kind && item.kind.toLowerCase() === 'deployment'); @@ -27,6 +31,10 @@ export function isConfigMapItem(item: ISupportedListItem): item is IConfigMapIte return (item.kind && item.kind.toLowerCase() === 'configmap'); } +export function isSecretItem(item: ISupportedListItem): item is ISecretItem { + return (item.kind && item.kind.toLowerCase() === 'secret'); +} + export function getPodItemOrNull(item: ISupportedListItem): IPodItem { if (isDeploymentItem(item)) { return item.spec.template; @@ -84,6 +92,14 @@ export interface IConfigMapItem { data: { [propName: string]: string | Object }; } +export interface ISecretItem { + apiVersion: string; + kind: string; + metadata: IObjectMetadata; + data?: { [propName: string]: string | Object }; + stringData?: { [propName: string]: string | Object}; +} + /** * Wrapper for jsyaml and simple validator for kubernetes machine recipe. * @@ -141,6 +157,8 @@ export class KubernetesMachineRecipeParser implements IParser { this.validatePod(recipe); } else if (isConfigMapItem(recipe)) { this.validateConfigMap(recipe); + } else if (isSecretItem(recipe)) { + this.validateSecret( recipe); } } @@ -224,6 +242,15 @@ export class KubernetesMachineRecipeParser implements IParser { } } + validateSecret(secret: ISecretItem) { + this.validateMetadata(secret.metadata); + if (!secret.data && !secret.stringData) { + throw new TypeError(`Recipe secret item should contain either data or stringData section.`); + } + // secret.data values must also be base64 encoded but nodejs doesn't allow an easy way to check + // if the encoding is valid (ignores errors silently). + } + validateMetadata(metadata: IObjectMetadata) { if (!metadata) { throw new TypeError(`Recipe item should contain 'metadata' section.`); From 55edf1974fb29778a6d025f4a37b63c92def80ef Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Tue, 22 Jan 2019 20:27:08 -0500 Subject: [PATCH 4/4] Improve error handling around loading recipes and add tests for Routes - 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 --- .../KubernetesEnvironmentFactory.java | 21 +++- .../OpenShiftEnvironmentFactory.java | 28 +++-- .../OpenShiftEnvironmentValidator.java | 63 ++++++++++ .../OpenShiftEnvironmentFactoryTest.java | 6 +- .../OpenShiftEnvironmentValidatorTest.java | 113 ++++++++++++++++++ 5 files changed, 217 insertions(+), 14 deletions(-) create mode 100644 infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentValidator.java create mode 100644 infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentValidatorTest.java diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactory.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactory.java index 7d6fcba2f0c..151c76fe568 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactory.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/KubernetesEnvironmentFactory.java @@ -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; @@ -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 pods = new HashMap<>(); Map deployments = new HashMap<>(); @@ -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())); } } diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java index c4e39bbbc15..61b5ef3f6ea 100644 --- a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java +++ b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactory.java @@ -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; @@ -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; @@ -51,7 +51,7 @@ public class OpenShiftEnvironmentFactory extends InternalEnvironmentFactory { private final OpenShiftClientFactory clientFactory; - private final KubernetesEnvironmentValidator envValidator; + private final OpenShiftEnvironmentValidator envValidator; private final MemoryAttributeProvisioner memoryProvisioner; @Inject @@ -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; @@ -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 pods = new HashMap<>(); Map deployments = new HashMap<>(); @@ -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; @@ -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())); } } diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentValidator.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentValidator.java new file mode 100644 index 00000000000..272694be5f1 --- /dev/null +++ b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentValidator.java @@ -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 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)); + } + } + } +} diff --git a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java index 30e7a0d7fa7..885fb218b95 100644 --- a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java +++ b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentFactoryTest.java @@ -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; @@ -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; @@ -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); @@ -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 recipeObjects = singletonList(route); diff --git a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentValidatorTest.java b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentValidatorTest.java new file mode 100644 index 00000000000..81e2a556d98 --- /dev/null +++ b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/environment/OpenShiftEnvironmentValidatorTest.java @@ -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 services = ImmutableMap.of("service1", makeService("service1")); + Map 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 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 services = ImmutableMap.of("service1", makeService("service1")); + Map 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(); + } +}