validate & recreate SA keys if necessary #160

danielnorberg · 2017-05-01T09:46:10Z

Also makes SA key secret checking and creation single threaded to avoid concurrent executions of workflows using the same service account from stepping on each others feet.

danielnorberg · 2017-05-02T07:27:17Z

@honnix PTAL

honnix · 2017-05-02T21:20:29Z

styx-scheduler-service/src/main/java/com/spotify/styx/ServiceAccountKeyManager.java

@@ -43,6 +44,20 @@ public ServiceAccountKey createP12Key(String serviceAccount) throws IOException
        .setPrivateKeyType("TYPE_PKCS12_FILE"));
  }

+  public boolean keyExists(String serviceAccount, String keyName) throws IOException {
+    try {
+      ServiceAccountKey key = iam.projects().serviceAccounts().keys()


no need of this variable.

honnix · 2017-05-02T21:23:36Z

styx-scheduler-service/src/main/java/com/spotify/styx/docker/KubernetesDockerRunner.java

+    final ServiceAccountKey jsonKey;
+    final ServiceAccountKey p12Key;
+    try {
+      jsonKey = Json.OBJECT_MAPPER.readValue(jsonKeyJson, ServiceAccountKey.class);


It's only private key gets stored in kube secret, not the complete JSON. https://github.com/spotify/styx/pull/160/files#diff-d96c43ca98b2c4d50df1a802530ab077R266

Get key name from annotation instead?

honnix · 2017-05-02T21:25:01Z

styx-scheduler-service/src/main/java/com/spotify/styx/docker/KubernetesDockerRunner.java

+      keysExist = serviceAccountKeyManager.keyExists(serviceAccount, jsonKey.getName())
+          && serviceAccountKeyManager.keyExists(serviceAccount, p12Key.getName());
+    } catch (IOException e) {
+      throw new RuntimeException(e);


Let's stick to InvalidExecutionException.

honnix · 2017-05-02T21:32:48Z

styx-scheduler-service/src/test/java/com/spotify/styx/docker/KubernetesDockerRunnerTest.java

@@ -25,9 +25,11 @@
 import static com.spotify.styx.docker.KubernetesPodEventTranslatorTest.running;
 import static com.spotify.styx.docker.KubernetesPodEventTranslatorTest.terminated;
 import static com.spotify.styx.docker.KubernetesPodEventTranslatorTest.waiting;
+import static java.nio.charset.StandardCharsets.UTF_8;


Unused import

honnix · 2017-05-02T21:50:57Z

styx-scheduler-service/src/main/java/com/spotify/styx/docker/KubernetesDockerRunner.java

-    if (runSpec.secret().isPresent()) {
-      final WorkflowConfiguration.Secret specSecret = runSpec.secret().get();
+  private void ensureCustomSecret(WorkflowInstance workflowInstance, RunSpec runSpec) {
+    runSpec.secret().ifPresent(specSecret -> {
      final Secret secret = client.secrets().withName(specSecret.name()).get();


Feel like to incorporate this check https://github.com/spotify/styx/pull/153/files#diff-d96c43ca98b2c4d50df1a802530ab077R160 ? Or I can open another PR after this one has been merged. Having #153 opened doesn't make much sense due to the massive change brought by this PR.

honnix · 2017-05-02T21:51:12Z

styx-scheduler-service/src/main/java/com/spotify/styx/docker/KubernetesDockerRunner.java

+    });
+  }
+
+  private void ensureServiceAccountKeySecret(WorkflowInstance workflowInstance, RunSpec runSpec) {


So as we discussed, this method is supposed to be synchronized as the first simplest approach.

danielnorberg · 2017-05-03T06:27:54Z

@honnix PTAL again

honnix · 2017-05-03T15:16:02Z

@bergman PTAL as well

ulzha · 2017-05-04T12:51:03Z

styx-scheduler-service/src/main/java/com/spotify/styx/ServiceAccountKeyManager.java

+      if (e.getStatusCode() == 404) {
+        return false;
+      }
+      throw e;


In most of the code we use throw Throwables.propagate(e)...

There was a discussion that Throwables.propagate has been deprecated by Google. Also in this case we don't need a RuntimeException I think.

ulzha · 2017-05-04T13:04:13Z

styx-scheduler-service/src/main/java/com/spotify/styx/docker/KubernetesDockerRunner.java

+    if (!serviceAccountExists) {
+      LOG.error("[AUDIT] Workflow {} refers to non-existent service account {}", workflowInstance.workflowId(),
+          serviceAccount);
+      throw new InvalidExecutionException("Referenced service account '" + serviceAccount + "' was not found");


Inconsistent quoting (here 's surround service account and not elsewhere) :] I do see that this was unchanged in this PR though.

Good catch. I don't think there is a need to quote. It's a full string without any space anyway.

ulzha · 2017-05-04T13:48:06Z

styx-scheduler-service/src/main/java/com/spotify/styx/docker/KubernetesDockerRunner.java

+          return;
+        }
+
+        LOG.warn("[AUDIT] Service account keys have been deleted for {}, recreating",


This is a lie the first time round, they just haven't been created, right?

Also it seems noisy to warn about it - isn't info more appropriate, as key rotation is legitimate business?

(Suggested convention: error for internal errors, "this line should never happen" type of things; warn for external errors that will stop workflows from running if they persist; info or lower for everything else.)

If the keys are not created the secret shouldn't be in kube. We check secret first.

Yeah. That I agree.

ulzha · 2017-05-04T14:03:07Z

styx-scheduler-service/src/main/java/com/spotify/styx/docker/KubernetesDockerRunner.java

+      final Map<String, String> annotations = ImmutableMap.of(
+          STYX_WORKFLOW_SA_JSON_KEY_NAME_ANNOTATION, jsonKey.getName(),
+          STYX_WORKFLOW_SA_P12_KEY_NAME_ANNOTATION, p12Key.getName(),
+          STYX_WORKFLOW_SA_SECRET_ANNOTATION, serviceAccount


Isn't it confusing to call it "secret annotation" if it stores serviceAccount? I expect "SA_ID_ANNOTATION" or "SA_ANNOTATION".

Yeah. Confusing indeed. Will fix.

ulzha · 2017-05-04T14:09:15Z

styx-scheduler-service/src/main/java/com/spotify/styx/docker/KubernetesDockerRunner.java

+
+      client.secrets().create(newSecret);
+
+      LOG.info("[AUDIT] Secret {} created for {} referred to by workflow {}",


"For" might read ambiguously, as in "for [use by this service account]", while it's actually "storing [this service account's private key!!!11]".

How about "Secret [secret1] created to store keys of [sa] referred by ..."?

ulzha · 2017-05-04T14:12:45Z

styx-scheduler-service/src/test/java/com/spotify/styx/docker/KubernetesDockerRunnerTest.java

+
+    stateManager.receive(Event.terminate(WORKFLOW_INSTANCE, Optional.of(0)));
+    stateManager.receive(Event.success(WORKFLOW_INSTANCE));
+    kdr.close();


Why here too? It seems it's done in tearDown?

In general cases we create in setup and for a few cases the default is not good enough so we close it and create a new one. Not very good indeed. Shall we address this in separated PR?

ulzha · 2017-05-04T14:20:19Z

LGTM apart from those naming and wording nitpickings.

danielnorberg changed the title ~~validate & recreate SA keys if necessary~~ [wip] validate & recreate SA keys if necessary May 2, 2017

honnix reviewed May 2, 2017

View reviewed changes

danielnorberg force-pushed the validate-recreate-SA-keys branch 3 times, most recently from 21c1e27 to a585405 Compare May 3, 2017 05:22

danielnorberg changed the title ~~[wip] validate & recreate SA keys if necessary~~ validate & recreate SA keys if necessary May 3, 2017

danielnorberg force-pushed the validate-recreate-SA-keys branch from a585405 to 9c57da2 Compare May 3, 2017 05:28

validate & recreate SA keys if necessary

b597cd5

danielnorberg force-pushed the validate-recreate-SA-keys branch from 9c57da2 to b597cd5 Compare May 3, 2017 05:31

check mount path

e03edd0

honnix mentioned this pull request May 4, 2017

let's be paranoid #153

Closed

ulzha reviewed May 4, 2017

View reviewed changes

honnix added 2 commits May 4, 2017 15:14

more consistent message

2d74bca

fix test case

16001e8

ulzha reviewed May 4, 2017

View reviewed changes

minor fixes to be less confusing

6ead180

honnix merged commit 228e7b1 into master May 4, 2017

honnix deleted the validate-recreate-SA-keys branch May 4, 2017 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validate & recreate SA keys if necessary #160

validate & recreate SA keys if necessary #160

danielnorberg commented May 1, 2017 •

edited

Loading

danielnorberg commented May 2, 2017

honnix May 2, 2017

honnix May 2, 2017

honnix May 2, 2017

honnix May 2, 2017

honnix May 2, 2017

honnix May 2, 2017

danielnorberg commented May 3, 2017

honnix commented May 3, 2017

ulzha May 4, 2017

honnix May 4, 2017

ulzha May 4, 2017

honnix May 4, 2017

ulzha May 4, 2017

ulzha May 4, 2017

honnix May 4, 2017

honnix May 4, 2017

ulzha May 4, 2017

honnix May 4, 2017

ulzha May 4, 2017

honnix May 4, 2017

ulzha May 4, 2017

honnix May 4, 2017

ulzha commented May 4, 2017


		client.secrets().create(newSecret);

		LOG.info("[AUDIT] Secret {} created for {} referred to by workflow {}",

validate & recreate SA keys if necessary #160

validate & recreate SA keys if necessary #160

Conversation

danielnorberg commented May 1, 2017 • edited Loading

danielnorberg commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnorberg commented May 3, 2017

honnix commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulzha commented May 4, 2017

danielnorberg commented May 1, 2017 •

edited

Loading