fix(k8s): improve determineOmitKinds (#3870)

* fix(k8s): improve determineOmitKinds improved perf on determineOmitKinds while we consider longer term approaches. previously, we made a kubectl call for every kind spinnaker knew about and every custom resource, regardless of if the API could handle it or not. this approach uses 2 methods to determine readability: 1. fetch all resources the API says it knows about via `kubectl api-resources`. this allows us to build a list of all known kinds. `determineOmitKinds` first checks against this list, preventing us from making list calls to kinds we already know we can't deploy. 2. for all available kinds, use the `SelfSubjectAccessReview` API to determine if the account has access to the kind. We're checking against the `list` verb, as we were previously, but don't have to actually deserialize the entire list of manifests that a regular `kubectl list` call returns.
spinnaker · Jul 15, 2019 · 6e30e56 · 6e30e56
1 parent dfe14a9
commit 6e30e56
Show file tree

Hide file tree

Showing 5 changed files with 215 additions and 18 deletions.
diff --git a/...ain/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/op/job/KubectlJobExecutor.java b/...ain/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/op/job/KubectlJobExecutor.java
@@ -30,6 +30,7 @@
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.KubernetesPodMetric.ContainerMetric;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKind;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesManifest;
+import com.netflix.spinnaker.clouddriver.kubernetes.v2.security.KubernetesApiResourceParser;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.security.KubernetesSelectorList;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.security.KubernetesV2Credentials;
 import io.kubernetes.client.models.V1DeleteOptions;
@@ -577,6 +578,51 @@ private String getOAuthToken(KubernetesV2Credentials credentials) {
     return status.getOutput();
   }
 
+  public Set<KubernetesKind.ScopedKind> apiResources(KubernetesV2Credentials credentials) {
+    List<String> command = kubectlAuthPrefix(credentials);
+    command.add("api-resources");
+
+    JobResult<String> status = jobExecutor.runJob(new JobRequest(command));
+
+    // api-resources can return a non-zero status code but still return data
+    // log here as a warning
+    if (!status.getResult().equals(JobResult.Result.SUCCESS)) {
+      log.warn("There was an error reading api-resources. All available kinds may not be present.");
+    }
+
+    String output = status.getOutput().trim();
+    if (StringUtils.isEmpty(output)) {
+      return new HashSet<>();
+    }
+
+    return KubernetesApiResourceParser.parse(output);
+  }
+
+  public boolean authCanI(KubernetesV2Credentials credentials, String kind, String verb) {
+    List<String> command = kubectlAuthPrefix(credentials);
+    command.add("auth");
+    command.add("can-i");
+    command.add(verb);
+    command.add(kind);
+
+    JobResult<String> status = jobExecutor.runJob(new JobRequest(command));
+
+    return status.getResult() == JobResult.Result.SUCCESS;
+  }
+
+  public boolean authCanINamespaced(
+      KubernetesV2Credentials credentials, String namespace, String kind, String verb) {
+    List<String> command = kubectlNamespacedAuthPrefix(credentials, namespace);
+    command.add("auth");
+    command.add("can-i");
+    command.add(verb);
+    command.add(kind);
+
+    JobResult<String> status = jobExecutor.runJob(new JobRequest(command));
+
+    return status.getResult() == JobResult.Result.SUCCESS;
+  }
+
   public Collection<KubernetesPodMetric> topPod(
       KubernetesV2Credentials credentials, String namespace, String pod) {
     List<String> command = kubectlNamespacedAuthPrefix(credentials, namespace);

diff --git a/...com/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesApiResourceParser.java b/...com/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesApiResourceParser.java
@@ -0,0 +1,53 @@
+/*
+ * Copyright 2019 Armory
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License")
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.clouddriver.kubernetes.v2.security;
+
+import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesApiGroup;
+import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKind;
+import java.util.HashSet;
+import java.util.Set;
+
+public class KubernetesApiResourceParser {
+
+  public static Set<KubernetesKind.ScopedKind> parse(String input) {
+    String[] lines = input.trim().split("\n");
+    String headerRow = lines[0];
+    int nameIndex = headerRow.indexOf("NAME");
+    int apiGroupIndex = headerRow.indexOf("APIGROUP");
+    int namespaceIndex = headerRow.indexOf("NAMESPACED");
+    int kindIndex = headerRow.indexOf("KIND");
+
+    // we expect NAME to be at index 0 of the first row
+    // if it isn't, then we didn't get the data in the
+    // format we expected
+    if (nameIndex != 0) {
+      throw new IllegalArgumentException(
+          "api-resources input not in the proper format. expected to find NAME header.");
+    }
+
+    Set<KubernetesKind.ScopedKind> kinds = new HashSet<>();
+
+    for (int i = 1; i < lines.length; i++) {
+      String line = lines[i];
+      String apiGroup = line.substring(apiGroupIndex, namespaceIndex).trim();
+      String kind = line.substring(kindIndex).trim();
+      kinds.add(new KubernetesKind.ScopedKind(kind, KubernetesApiGroup.fromString(apiGroup)));
+    }
+
+    return kinds;
+  }
+}
diff --git a/...ovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesV2Credentials.java b/...ovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesV2Credentials.java
@@ -365,13 +365,32 @@ private void determineOmitKinds() {
 
     log.info(
         "Checking permissions on configured kinds for account {}... {}", accountName, allKinds);
+    long startTime = System.nanoTime();
+
+    // compute list of kinds we explicitly know the server doesn't support
+    try {
+      Set<KubernetesKind.ScopedKind> availableResources = jobExecutor.apiResources(this);
+      Map<KubernetesKind, InvalidKindReason> unavailableKinds =
+          allKinds.stream()
+              .filter(k -> k != KubernetesKind.NONE)
+              .filter(k -> !availableResources.contains(k.getScopedKind()))
+              .collect(Collectors.toConcurrentMap(k -> k, k -> InvalidKindReason.READ_ERROR));
+
+      omitKindsComputed.putAll(unavailableKinds);
+    } catch (Exception e) {
+      log.warn("Failed to evaluate kinds available on server. {}.", e.getMessage());
+    }
+
     Map<KubernetesKind, InvalidKindReason> unreadableKinds =
         allKinds
             .parallelStream()
             .filter(k -> k != KubernetesKind.NONE)
             .filter(k -> !omitKindsComputed.keySet().contains(k))
             .filter(k -> !canReadKind(k, checkNamespace))
             .collect(Collectors.toConcurrentMap(k -> k, k -> InvalidKindReason.READ_ERROR));
+    long endTime = System.nanoTime();
+    long duration = (endTime - startTime) / 1000000;
+    log.info("determineOmitKinds for account {} took {} ms", accountName, duration);
     omitKindsComputed.putAll(unreadableKinds);
 
     if (metricsComputed) {
@@ -390,23 +409,24 @@ private void determineOmitKinds() {
   }
 
   private boolean canReadKind(KubernetesKind kind, String checkNamespace) {
-    try {
-      log.info("Checking if {} is readable in account '{}'...", kind, accountName);
-      if (kind.isNamespaced()) {
-        list(kind, checkNamespace);
-      } else {
-        list(kind, null);
-      }
-      return true;
-    } catch (Exception e) {
+    log.info("Checking if {} is readable in account '{}'...", kind, accountName);
+    boolean allowed;
+    if (kind.isNamespaced()) {
+      allowed =
+          jobExecutor.authCanINamespaced(
+              this, checkNamespace, kind.getScopedKind().getName(), "list");
+    } else {
+      allowed = jobExecutor.authCanI(this, kind.getScopedKind().getName(), "list");
+    }
+
+    if (!allowed) {
       log.info(
-          "Kind '{}' will not be cached in account '{}' for reason: '{}'",
+          "Kind {} will not be cached in account '{}' because it cannot be listed.",
           kind,
-          accountName,
-          e.getMessage());
-      log.debug("Reading kind '{}' in account '{}' failed with exception: ", kind, accountName, e);
-      return false;
+          accountName);
     }
+
+    return allowed;
   }
 
   public KubernetesManifest get(KubernetesKind kind, String namespace, String name) {

diff --git a/...tflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesApiResourceParserSpec.groovy b/...tflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesApiResourceParserSpec.groovy
@@ -0,0 +1,41 @@
+/*
+ * Copyright 2019 Armory
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License")
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.clouddriver.kubernetes.v2.security
+
+import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesApiGroup
+import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKind
+import spock.lang.Specification
+
+class KubernetesApiResourceParserSpec extends Specification {
+
+  def inputWithHeaders = """
+NAME                              SHORTNAMES   APIGROUP                       NAMESPACED   KIND
+bindings                                                                      true         Binding
+mutatingwebhookconfigurations                  admissionregistration.k8s.io   false        MutatingWebhookConfiguration
+"""
+
+  def "parses api-resources response properly"() {
+    when:
+    def result = KubernetesApiResourceParser.parse(inputWithHeaders)
+
+    then:
+    [
+      new KubernetesKind.ScopedKind("Binding", KubernetesApiGroup.fromString("")),
+      new KubernetesKind.ScopedKind("MutatingWebhookConfiguration", KubernetesApiGroup.fromString("admissionregistration.k8s.io"))
+    ] as Set == result
+  }
+}
diff --git a/...m/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesV2CredentialsSpec.groovy b/...m/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesV2CredentialsSpec.groovy
@@ -18,6 +18,7 @@ package com.netflix.spinnaker.clouddriver.kubernetes.v2.security
 
 import com.netflix.spectator.api.Registry
 import com.netflix.spinnaker.clouddriver.kubernetes.config.KubernetesConfigurationProperties
+import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesApiGroup
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKind
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.op.job.KubectlJobExecutor
 import spock.lang.Specification
@@ -94,6 +95,32 @@ class KubernetesV2CredentialsSpec extends Specification {
     credentials.isValidKind(KubernetesKind.REPLICA_SET) == true
   }
 
+  void "Kinds that are not available on the server are considered invalid"() {
+    when:
+    KubernetesV2Credentials credentials = buildCredentials(
+      new KubernetesConfigurationProperties.ManagedAccount(
+        namespaces: [NAMESPACE],
+        checkPermissionsOnStartup: true,
+      )
+    )
+    credentials.initialize()
+
+    then:
+
+    kubectlJobExecutor.apiResources(_) >> {
+      return [
+        new KubernetesKind.ScopedKind("Deployment", KubernetesApiGroup.APPS)
+      ]
+    }
+
+    kubectlJobExecutor.authCanINamespaced(_, _, "deployment", _) >> {
+      return true
+    }
+
+    credentials.isValidKind(KubernetesKind.DEPLOYMENT) == true
+    credentials.isValidKind(KubernetesKind.REPLICA_SET) == false
+  }
+
   void "Kinds that are not readable are considered invalid"() {
     when:
     KubernetesV2Credentials credentials = buildCredentials(
@@ -105,12 +132,22 @@ class KubernetesV2CredentialsSpec extends Specification {
     credentials.initialize()
 
     then:
-    kubectlJobExecutor.list(_, { it.contains(KubernetesKind.DEPLOYMENT) }, _, _) >> {
-      throw new KubectlJobExecutor.KubectlException()
+
+    kubectlJobExecutor.apiResources(_) >> {
+      return [
+        new KubernetesKind.ScopedKind("Deployment", KubernetesApiGroup.APPS),
+        new KubernetesKind.ScopedKind("ReplicaSet", KubernetesApiGroup.APPS)
+      ]
+    }
+
+    kubectlJobExecutor.authCanINamespaced(_, _, "deployment", _) >> {
+      return false
     }
-    kubectlJobExecutor.list(_, { !it.contains(KubernetesKind.DEPLOYMENT) }, _, _) >> {
-      return Collections.emptyList()
+
+    kubectlJobExecutor.authCanINamespaced(_, _, "replicaSet", _) >> {
+      return true
     }
+
     credentials.isValidKind(KubernetesKind.DEPLOYMENT) == false
     credentials.isValidKind(KubernetesKind.REPLICA_SET) == true
   }