From 8db0005beae096c1a55f4949eb3480ae058c1b50 Mon Sep 17 00:00:00 2001 From: Daniel Norberg Date: Tue, 9 May 2017 16:01:44 +0900 Subject: [PATCH] add more failure mode tests --- .../styx/docker/KubernetesDockerRunner.java | 2 +- .../docker/KubernetesDockerRunnerTest.java | 108 +++++++++--------- 2 files changed, 58 insertions(+), 52 deletions(-) diff --git a/styx-scheduler-service/src/main/java/com/spotify/styx/docker/KubernetesDockerRunner.java b/styx-scheduler-service/src/main/java/com/spotify/styx/docker/KubernetesDockerRunner.java index 8e8a6dfaa8..b55366b656 100644 --- a/styx-scheduler-service/src/main/java/com/spotify/styx/docker/KubernetesDockerRunner.java +++ b/styx-scheduler-service/src/main/java/com/spotify/styx/docker/KubernetesDockerRunner.java @@ -194,7 +194,7 @@ public void cleanup(Set workflows) throws IOException { LOG.info("[AUDIT] Deleting unused service account {} secret {}", serviceAcountEmail, name); client.secrets().delete(secret); - } catch (IOException e) { + } catch (IOException | KubernetesClientException e) { LOG.warn("[AUDIT] Failed to delete unused service account {} keys and/or secret {}", serviceAcountEmail, name); } diff --git a/styx-scheduler-service/src/test/java/com/spotify/styx/docker/KubernetesDockerRunnerTest.java b/styx-scheduler-service/src/test/java/com/spotify/styx/docker/KubernetesDockerRunnerTest.java index ad617fd5a3..b4efb492e8 100644 --- a/styx-scheduler-service/src/test/java/com/spotify/styx/docker/KubernetesDockerRunnerTest.java +++ b/styx-scheduler-service/src/test/java/com/spotify/styx/docker/KubernetesDockerRunnerTest.java @@ -64,7 +64,6 @@ import com.spotify.styx.state.StateManager.IsClosed; import com.spotify.styx.state.SyncStateManager; import com.spotify.styx.testdata.TestData; -import com.spotify.styx.util.GcpUtil; import io.fabric8.kubernetes.api.model.DoneablePod; import io.fabric8.kubernetes.api.model.DoneableSecret; import io.fabric8.kubernetes.api.model.ListMeta; @@ -75,6 +74,7 @@ import io.fabric8.kubernetes.api.model.Secret; import io.fabric8.kubernetes.api.model.SecretBuilder; import io.fabric8.kubernetes.api.model.SecretList; +import io.fabric8.kubernetes.client.KubernetesClientException; import io.fabric8.kubernetes.client.NamespacedKubernetesClient; import io.fabric8.kubernetes.client.Watch; import io.fabric8.kubernetes.client.Watcher; @@ -282,28 +282,7 @@ public void shouldRunIfSecretExists() throws IOException, StateManager.IsClosed @Test public void shouldCleanupServiceAccountSecrets() throws Exception { - final String jsonKeyId = "json-key"; - final String p12KeyId = "p12-key"; - - final String jsonKeyName = keyName(SERVICE_ACCOUNT, jsonKeyId); - final String p12KeyName = keyName(SERVICE_ACCOUNT, p12KeyId); - - final String jsonKeyData = "json-private-key-data"; - final String p12KeyData = "p12-private-key-data"; - - final ObjectMeta metadata = new ObjectMeta(); - metadata.setName("styx-wf-sa-keys"); - metadata.setAnnotations(ImmutableMap.of( - "styx-wf-sa", SERVICE_ACCOUNT, - "styx-wf-sa-json-key-name", jsonKeyName, - "styx-wf-sa-p12-key-name", p12KeyName)); - - final Secret secret = new SecretBuilder() - .withMetadata(metadata) - .withData(ImmutableMap.of( - "styx-wf-sa.json", jsonKeyData, - "styx-wf-sa.p12", p12KeyData)) - .build(); + final Secret secret = fakeServiceAccountKeySecret(SERVICE_ACCOUNT, "json-key", "p12-key"); when(k8sClient.secrets()).thenReturn(secrets); when(secrets.list()).thenReturn(secretList); @@ -321,50 +300,77 @@ public void shouldCleanupServiceAccountSecrets() throws Exception { // Verify that an unused service account key secret is deleted kdr.cleanup(ImmutableSet.of()); - verify(serviceAccountKeyManager).deleteKey(jsonKeyName); - verify(serviceAccountKeyManager).deleteKey(p12KeyName); + verify(serviceAccountKeyManager).deleteKey(keyName(SERVICE_ACCOUNT, "json-key")); + verify(serviceAccountKeyManager).deleteKey(keyName(SERVICE_ACCOUNT, "p12-key")); verify(secrets).delete(secret); } @Test public void shouldHandlePermissionDeniedErrorsWhenDeletingServiceAccountKeys() throws Exception { - final String jsonKeyId = "json-key"; - final String p12KeyId = "p12-key"; + final Secret secret = fakeServiceAccountKeySecret(SERVICE_ACCOUNT, "json-key", "p12-key"); - final String jsonKeyName = keyName(SERVICE_ACCOUNT, jsonKeyId); - final String p12KeyName = keyName(SERVICE_ACCOUNT, p12KeyId); + when(k8sClient.secrets()).thenReturn(secrets); + when(secrets.list()).thenReturn(secretList); + when(secretList.getItems()).thenReturn(ImmutableList.of(secret)); - final String jsonKeyData = "json-private-key-data"; - final String p12KeyData = "p12-private-key-data"; + // Verify that the secret is delete even if we get permission denied errors on deleting the keys + final GoogleJsonResponseException permissionDenied = new GoogleJsonResponseException( + new Builder(403, "Forbidden", new HttpHeaders()), new GoogleJsonError() + .set("status", "PERMISSION_DENIED")); + doThrow(permissionDenied).when(serviceAccountKeyManager).deleteKey(keyName(SERVICE_ACCOUNT, "json-key")); + doThrow(permissionDenied).when(serviceAccountKeyManager).deleteKey(keyName(SERVICE_ACCOUNT, "p12-key")); + kdr.cleanup(ImmutableSet.of()); + verify(serviceAccountKeyManager).deleteKey(keyName(SERVICE_ACCOUNT, "json-key")); + verify(serviceAccountKeyManager).deleteKey(keyName(SERVICE_ACCOUNT, "p12-key")); + verify(secrets).delete(secret); + } + + @Test + public void shouldHandleErrorsWhenDeletingServiceAccountKeys() throws Exception { + final Secret secret1 = fakeServiceAccountKeySecret(SERVICE_ACCOUNT, "json-key-1", "p12-key-1"); + final Secret secret2 = fakeServiceAccountKeySecret(SERVICE_ACCOUNT, "json-key-2", "p12-key-2"); + final Secret secret3 = fakeServiceAccountKeySecret(SERVICE_ACCOUNT, "json-key-3", "p12-key-3"); + + when(k8sClient.secrets()).thenReturn(secrets); + when(secrets.list()).thenReturn(secretList); + when(secretList.getItems()).thenReturn(ImmutableList.of(secret1, secret2, secret3)); + + when(secrets.delete(secret1)).thenThrow(new KubernetesClientException("fail delete secret1")); + doThrow(new IOException("fail delete json-key-2")).when(serviceAccountKeyManager).deleteKey("json-key-2"); + doThrow(new IOException("fail delete p12-key-2")).when(serviceAccountKeyManager).deleteKey("p12-key-2"); + + kdr.cleanup(ImmutableSet.of()); + + verify(serviceAccountKeyManager).deleteKey(keyName(SERVICE_ACCOUNT, "json-key-1")); + verify(serviceAccountKeyManager).deleteKey(keyName(SERVICE_ACCOUNT, "p12-key-1")); + verify(secrets).delete(secret1); + + verify(serviceAccountKeyManager).deleteKey(keyName(SERVICE_ACCOUNT, "json-key-2")); + verify(serviceAccountKeyManager).deleteKey(keyName(SERVICE_ACCOUNT, "p12-key-2")); + verify(secrets).delete(secret2); + + verify(serviceAccountKeyManager).deleteKey(keyName(SERVICE_ACCOUNT, "json-key-3")); + verify(serviceAccountKeyManager).deleteKey(keyName(SERVICE_ACCOUNT, "p12-key-3")); + verify(secrets).delete(secret3); + } + + private static Secret fakeServiceAccountKeySecret(String serviceAccount, String jsonKeyId, String p12KeyId) { + final String jsonKeyName = keyName(serviceAccount, jsonKeyId); + final String p12KeyName = keyName(serviceAccount, p12KeyId); final ObjectMeta metadata = new ObjectMeta(); metadata.setName("styx-wf-sa-keys"); metadata.setAnnotations(ImmutableMap.of( - "styx-wf-sa", SERVICE_ACCOUNT, + "styx-wf-sa", serviceAccount, "styx-wf-sa-json-key-name", jsonKeyName, "styx-wf-sa-p12-key-name", p12KeyName)); - final Secret secret = new SecretBuilder() + return new SecretBuilder() .withMetadata(metadata) .withData(ImmutableMap.of( - "styx-wf-sa.json", jsonKeyData, - "styx-wf-sa.p12", p12KeyData)) + "styx-wf-sa.json", "json-private-key-data", + "styx-wf-sa.p12", "p12-private-key-data")) .build(); - - when(k8sClient.secrets()).thenReturn(secrets); - when(secrets.list()).thenReturn(secretList); - when(secretList.getItems()).thenReturn(ImmutableList.of(secret)); - - // Verify that the secret is delete even if we get permission denied errors on deleting the keys - final GoogleJsonResponseException permissionDenied = new GoogleJsonResponseException( - new Builder(403, "Forbidden", new HttpHeaders()), new GoogleJsonError() - .set("status", "PERMISSION_DENIED")); - doThrow(permissionDenied).when(serviceAccountKeyManager).deleteKey(jsonKeyName); - doThrow(permissionDenied).when(serviceAccountKeyManager).deleteKey(p12KeyName); - kdr.cleanup(ImmutableSet.of()); - verify(serviceAccountKeyManager).deleteKey(jsonKeyName); - verify(serviceAccountKeyManager).deleteKey(p12KeyName); - verify(secrets).delete(secret); } @Test @@ -454,7 +460,7 @@ public void shouldRunIfSASecretExists() throws StateManager.IsClosed, IOExceptio verify(secrets, never()).create(any(Secret.class)); } - public String keyName(String serviceAccount, String keyId) { + private static String keyName(String serviceAccount, String keyId) { return "projects/-/serviceAccounts/" + serviceAccount + "/keys/" + keyId; }