From 2f9e93b68df05408507e122aced6daf1c852da0b Mon Sep 17 00:00:00 2001 From: Max Shaposhnik Date: Wed, 1 Sep 2021 10:12:23 +0300 Subject: [PATCH 1/3] Prevent workspace stertup failures due to metrics API --- .../AbstractWorkspaceServiceAccount.java | 31 +++++++++++++------ 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java index 223a1bff334..2a926e1da97 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/AbstractWorkspaceServiceAccount.java @@ -18,6 +18,7 @@ import io.fabric8.kubernetes.api.model.KubernetesResourceList; import io.fabric8.kubernetes.api.model.ServiceAccountBuilder; import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.client.KubernetesClientException; import io.fabric8.kubernetes.client.dsl.MixedOperation; import io.fabric8.kubernetes.client.dsl.Resource; import java.util.Arrays; @@ -130,15 +131,27 @@ private void ensureImplicitRolesWithBindings(Client k8sClient) { serviceAccountName + "-view"); // metrics role - ensureRoleWithBinding( - k8sClient, - buildRole( - METRICS_ROLE_NAME, - Arrays.asList("pods", "nodes"), - emptyList(), - singletonList("metrics.k8s.io"), - Arrays.asList("list", "get", "watch")), - serviceAccountName + "-metrics"); + if (k8sClient.supportsApiPath("/apis/metrics.k8s.io")) { + try { + ensureRoleWithBinding( + k8sClient, + buildRole( + METRICS_ROLE_NAME, + Arrays.asList("pods", "nodes"), + emptyList(), + singletonList("metrics.k8s.io"), + Arrays.asList("list", "get", "watch")), + serviceAccountName + "-metrics"); + } catch (KubernetesClientException e) { + // workaround to unblock workspace start if no permissions for metrics + if (e.getCode() == 403) { + LOG.warn( + "Unable to add metrics roles due to insufficient permissions. Workspace metrics will be disabled."); + } else { + throw e; + } + } + } // credentials-secret role ensureRoleWithBinding( From 4295563df73acf73f771d47d575ec74f503223f1 Mon Sep 17 00:00:00 2001 From: Max Shaposhnik Date: Wed, 1 Sep 2021 15:48:07 +0300 Subject: [PATCH 2/3] Add and fixup tests Signed-off-by: Max Shaposhnik --- .../KubernetesNamespaceFactoryTest.java | 10 ++-- ...KubernetesWorkspaceServiceAccountTest.java | 48 +++++++++++++++++++ 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactoryTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactoryTest.java index 4cef78dfa89..649bf04ca4b 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactoryTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactoryTest.java @@ -724,6 +724,7 @@ public void shouldBindToAllConfiguredClusterRoles() throws Exception { when(toReturnNamespace.getWorkspaceId()).thenReturn("workspace123"); when(toReturnNamespace.getName()).thenReturn("workspace123"); doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespaceAccess(any(), any()); + when(k8sClient.supportsApiPath(eq("/apis/metrics.k8s.io"))).thenReturn(true); when(clientFactory.create(any())).thenReturn(k8sClient); // pre-create the cluster roles @@ -754,8 +755,8 @@ public void shouldBindToAllConfiguredClusterRoles() throws Exception { RoleList roles = k8sClient.rbac().roles().inNamespace("workspace123").list(); assertEquals( - Sets.newHashSet("workspace-view", "workspace-metrics", "workspace-secrets", "exec"), - roles.getItems().stream().map(r -> r.getMetadata().getName()).collect(Collectors.toSet())); + roles.getItems().stream().map(r -> r.getMetadata().getName()).collect(Collectors.toSet()), + Sets.newHashSet("workspace-view", "workspace-metrics", "workspace-secrets", "exec")); RoleBindingList bindings = k8sClient.rbac().roleBindings().inNamespace("workspace123").list(); assertEquals( bindings @@ -855,6 +856,7 @@ public void shouldCreateExecAndViewRolesAndBindings() throws Exception { when(toReturnNamespace.getWorkspaceId()).thenReturn("workspace123"); when(toReturnNamespace.getName()).thenReturn("workspace123"); doReturn(toReturnNamespace).when(namespaceFactory).doCreateNamespaceAccess(any(), any()); + when(k8sClient.supportsApiPath(eq("/apis/metrics.k8s.io"))).thenReturn(true); when(clientFactory.create(any())).thenReturn(k8sClient); // when @@ -871,8 +873,8 @@ public void shouldCreateExecAndViewRolesAndBindings() throws Exception { RoleList roles = k8sClient.rbac().roles().inNamespace("workspace123").list(); assertEquals( - Sets.newHashSet("workspace-view", "workspace-metrics", "workspace-secrets", "exec"), - roles.getItems().stream().map(r -> r.getMetadata().getName()).collect(Collectors.toSet())); + roles.getItems().stream().map(r -> r.getMetadata().getName()).collect(Collectors.toSet()), + Sets.newHashSet("workspace-view", "workspace-metrics", "workspace-secrets", "exec")); Role role1 = roles.getItems().get(0); Role role2 = roles.getItems().get(1); diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccountTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccountTest.java index ac13d01c768..091271b35d8 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccountTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccountTest.java @@ -11,8 +11,11 @@ */ package org.eclipse.che.workspace.infrastructure.kubernetes.namespace; +import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.AbstractWorkspaceServiceAccount.METRICS_ROLE_NAME; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; import io.fabric8.kubernetes.api.model.ServiceAccountBuilder; @@ -52,6 +55,7 @@ public void setUp() throws Exception { serverMock = new KubernetesServer(true, true); serverMock.before(); + k8sClient = serverMock.getClient(); when(clientFactory.create(anyString())).thenReturn(k8sClient); } @@ -83,4 +87,48 @@ public void shouldProvisionSARolesEvenIfItAlreadyExists() throws Exception { RoleBindingList rbl = k8sClient.rbac().roleBindings().inNamespace(NAMESPACE).list(); assertTrue(rbl.getItems().size() > 1); } + + @Test + public void shouldCreateMetricsRoleIfAPIEnabledOnServer() throws Exception { + KubernetesClient localK8sClient = spy(serverMock.getClient()); + when(localK8sClient.supportsApiPath(eq("/apis/metrics.k8s.io"))).thenReturn(true); + when(clientFactory.create(anyString())).thenReturn(localK8sClient); + + // when + serviceAccount.prepare(); + + // then + // make sure metrics role & rb added + RoleList rl = k8sClient.rbac().roles().inNamespace(NAMESPACE).list(); + assertTrue( + rl.getItems().stream().anyMatch(r -> r.getMetadata().getName().equals(METRICS_ROLE_NAME))); + + RoleBindingList rbl = k8sClient.rbac().roleBindings().inNamespace(NAMESPACE).list(); + assertTrue( + rbl.getItems() + .stream() + .anyMatch(rb -> rb.getMetadata().getName().equals(SA_NAME + "-metrics"))); + } + + @Test + public void shouldNotCreateMetricsRoleIfAPIEnabledOnServer() throws Exception { + KubernetesClient localK8sClient = spy(serverMock.getClient()); + when(localK8sClient.supportsApiPath(eq("/apis/metrics.k8s.io"))).thenReturn(false); + when(clientFactory.create(anyString())).thenReturn(localK8sClient); + + // when + serviceAccount.prepare(); + + // then + // make sure metrics role & rb added + RoleList rl = k8sClient.rbac().roles().inNamespace(NAMESPACE).list(); + assertTrue( + rl.getItems().stream().noneMatch(r -> r.getMetadata().getName().equals(METRICS_ROLE_NAME))); + + RoleBindingList rbl = k8sClient.rbac().roleBindings().inNamespace(NAMESPACE).list(); + assertTrue( + rbl.getItems() + .stream() + .noneMatch(rb -> rb.getMetadata().getName().equals(SA_NAME + "-metrics"))); + } } From d22c7be7a92ea74f3736c89ac71ef7c83830654b Mon Sep 17 00:00:00 2001 From: Max Shaposhnik Date: Wed, 1 Sep 2021 15:55:29 +0300 Subject: [PATCH 3/3] Minor fixup Signed-off-by: Max Shaposhnik --- .../namespace/KubernetesWorkspaceServiceAccountTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccountTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccountTest.java index 091271b35d8..241c14b0d10 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccountTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesWorkspaceServiceAccountTest.java @@ -120,7 +120,7 @@ public void shouldNotCreateMetricsRoleIfAPIEnabledOnServer() throws Exception { serviceAccount.prepare(); // then - // make sure metrics role & rb added + // make sure metrics role & rb not added RoleList rl = k8sClient.rbac().roles().inNamespace(NAMESPACE).list(); assertTrue( rl.getItems().stream().noneMatch(r -> r.getMetadata().getName().equals(METRICS_ROLE_NAME)));