Skip to content

Commit

Permalink
refactor(kubernetes): Don't look up default namespace (#4501)
Browse files Browse the repository at this point in the history 
* refactor(kubernetes): Convert DeployManifestOperation test to java

The next refactor will require some changes to this test that are
more difficult than necessary because the test is in groovy and uses
more mocking than necessary. Convert the test to Java, and clean up
the mocking so it's easier to follow.

* refactor(kubernetes): Operations should return the created manifest

When creating a manifest, the result is returned by the API, but
is currently ignored in most cases by clouddriver. It is useful
to use the returned manifest because there are times where it has
updated information. One particular example is when we don't specify
a namespace on our manifest; in that case, the manifest will be
applied to the default namespace that is configured.

Currently we solve this problem by separately looking up the default
namespace before applying the manifest, but it would be easier to just
apply it and then check what kubernetes default it to. This change
will allow us to do that in a future commit.

* refactor(kubernetes): Don't look up default namespace

There are two places where we look up the default namespace
unnecessarily. The first is on every call to kubectl so that we
can pass in a "--namespace" flag; if we just leave that flag out,
we'll get the default namespace already. The second is when
deploying a manifest without a namespace; in that case, we look up
the default namespace so that we can apply it to the manifest before
deploying. Similarly, if we just deploy the manifest as is, it will
get the default namespace (which we'll see on the result from the
API).

I tested that this works both for the serviceAccount case and for
the regular kubeconfig case (testing the serviceAccount by ssh-ing
to a pod set up this way to check that kubectl already detected the
default).

This deletes a lot of unecessary code. When we move to use the client
library, we may need to solve this again as I'm not sure that it will
always detect a default namespace, but this code was complicated enough
and coupled pretty tightly to the kubectl implementation that I think
it will be better to start from scratch at that point.  (This was also
broken, as the command to get the default namespace from the kubeconfig
appears to fail every time so you always get 'default' regardless of what
is set.)

* fix(kubernetes): Fix Strings import

I accidentally imported the wrong Strings package; let's use Guava.
  • Loading branch information
@ezimanyi
ezimanyi committed Apr 9, 2020
1 parent e098c9f commit 9e3a73b
Show file tree
Hide file tree
Showing 11 changed files with 267 additions and 310 deletions.
12 changes: 8 additions & 4 deletions ...2/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/v2/op/handler/CanDeploy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ default OperationResult deploy(
return new OperationResult().addManifest(result);
}

KubernetesManifest deployedManifest;
switch (deployStrategy) {
case RECREATE:
try {
Expand All @@ -48,14 +49,17 @@ default OperationResult deploy(
new V1DeleteOptions());
} catch (KubectlJobExecutor.KubectlException ignored) {
}
credentials.deploy(manifest);
deployedManifest = credentials.deploy(manifest);
break;
case REPLACE:
credentials.replace(manifest);
deployedManifest = credentials.replace(manifest);
break;
case APPLY:
credentials.deploy(manifest);
deployedManifest = credentials.deploy(manifest);
break;
default:
throw new AssertionError(String.format("Unknown deploy strategy: %s", deployStrategy));
}
return new OperationResult().addManifest(manifest);
return new OperationResult().addManifest(deployedManifest);
}
}
25 changes: 18 additions & 7 deletions .../main/java/com/netflix/spinnaker/clouddriver/kubernetes/v2/op/job/KubectlJobExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -449,13 +449,16 @@ public ImmutableList<KubernetesManifest> list(
return status.getOutput();
}

public Void deploy(KubernetesV2Credentials credentials, KubernetesManifest manifest) {
public KubernetesManifest deploy(
KubernetesV2Credentials credentials, KubernetesManifest manifest) {
List<String> command = kubectlAuthPrefix(credentials);

String manifestAsJson = gson.toJson(manifest);

// Read from stdin
command.add("apply");
command.add("-o");
command.add("json");
command.add("-f");
command.add("-");

Expand All @@ -467,16 +470,23 @@ public Void deploy(KubernetesV2Credentials credentials, KubernetesManifest manif
throw new KubectlException("Deploy failed: " + status.getError());
}

return null;
try {
return gson.fromJson(status.getOutput(), KubernetesManifest.class);
} catch (JsonSyntaxException e) {
throw new KubectlException("Failed to parse kubectl output: " + e.getMessage(), e);
}
}

public Void replace(KubernetesV2Credentials credentials, KubernetesManifest manifest) {
public KubernetesManifest replace(
KubernetesV2Credentials credentials, KubernetesManifest manifest) {
List<String> command = kubectlAuthPrefix(credentials);

String manifestAsJson = gson.toJson(manifest);

// Read from stdin
command.add("replace");
command.add("-o");
command.add("json");
command.add("-f");
command.add("-");

Expand All @@ -488,7 +498,11 @@ public Void replace(KubernetesV2Credentials credentials, KubernetesManifest mani
throw new KubectlException("Replace failed: " + status.getError());
}

return null;
try {
return gson.fromJson(status.getOutput(), KubernetesManifest.class);
} catch (JsonSyntaxException e) {
throw new KubectlException("Failed to parse kubectl output: " + e.getMessage(), e);
}
}

public KubernetesManifest create(
Expand Down Expand Up @@ -577,9 +591,6 @@ private List<String> kubectlLookupInfo(
private List<String> kubectlNamespacedAuthPrefix(
KubernetesV2Credentials credentials, String namespace) {
List<String> command = kubectlAuthPrefix(credentials);
if (StringUtils.isEmpty(namespace)) {
namespace = credentials.getDefaultNamespace();
}

if (StringUtils.isNotEmpty(namespace)) {
command.add("--namespace=" + namespace);
Expand Down
9 changes: 3 additions & 6 deletions ...ix/spinnaker/clouddriver/kubernetes/v2/op/manifest/KubernetesDeployManifestOperation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,9 @@ public OperationResult operate(List _unused) {
Set<Artifact> boundArtifacts = new HashSet<>();

for (KubernetesManifest manifest : inputManifests) {
if (credentials.getKindProperties(manifest.getKind()).isNamespaced()) {
if (!StringUtils.isEmpty(description.getNamespaceOverride())) {
manifest.setNamespace(description.getNamespaceOverride());
} else if (StringUtils.isEmpty(manifest.getNamespace())) {
manifest.setNamespace(credentials.getDefaultNamespace());
}
if (credentials.getKindProperties(manifest.getKind()).isNamespaced()
&& !StringUtils.isEmpty(description.getNamespaceOverride())) {
manifest.setNamespace(description.getNamespaceOverride());
}

KubernetesResourceProperties properties = findResourceProperties(manifest);
Expand Down
60 changes: 4 additions & 56 deletions ...ava/com/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesV2Credentials.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,6 @@
import com.netflix.spinnaker.kork.configserver.ConfigFileService;
import io.kubernetes.client.openapi.models.V1DeleteOptions;
import io.kubernetes.client.openapi.models.V1beta1CustomResourceDefinition;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.*;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
Expand All @@ -80,9 +75,6 @@
public class KubernetesV2Credentials implements KubernetesCredentials {
private static final int CRD_EXPIRY_SECONDS = 30;
private static final int NAMESPACE_EXPIRY_SECONDS = 30;
private static final Path SERVICE_ACCOUNT_NAMESPACE_PATH =
Paths.get("/var/run/secrets/kubernetes.io/serviceaccount/namespace");
private static final String DEFAULT_NAMESPACE = "default";

private final Registry registry;
private final Clock clock;
Expand Down Expand Up @@ -128,7 +120,6 @@ public class KubernetesV2Credentials implements KubernetesCredentials {

@Include @Getter private final boolean debug;

private String cachedDefaultNamespace;
@Getter private final ResourcePropertyRegistry resourcePropertyRegistry;
private final KubernetesKindRegistry kindRegistry;
private final KubernetesSpinnakerKindMap kubernetesSpinnakerKindMap;
Expand Down Expand Up @@ -287,14 +278,6 @@ private Optional<KubernetesKindProperties> getCrdProperties(
return Optional.ofNullable(crdSupplier.get().get(kubernetesKind));
}

public String getDefaultNamespace() {
if (StringUtils.isEmpty(cachedDefaultNamespace)) {
cachedDefaultNamespace = lookupDefaultNamespace();
}

return cachedDefaultNamespace;
}

@Nonnull
public ImmutableList<KubernetesKind> getGlobalKinds() {
return kindRegistry.getGlobalKinds().stream()
Expand All @@ -307,41 +290,6 @@ public KubernetesKindProperties getKindProperties(@Nonnull KubernetesKind kind)
return kindRegistry.getKindPropertiesOrDefault(kind);
}

private Optional<String> serviceAccountNamespace() {
try {
return Files.lines(SERVICE_ACCOUNT_NAMESPACE_PATH, StandardCharsets.UTF_8).findFirst();
} catch (IOException e) {
log.debug("Failure looking up desired namespace", e);
return Optional.empty();
}
}

private Optional<String> kubectlNamespace() {
try {
return Optional.of(jobExecutor.defaultNamespace(this));
} catch (KubectlException e) {
log.debug("Failure looking up desired namespace", e);
return Optional.empty();
}
}

private String lookupDefaultNamespace() {
try {
if (serviceAccount) {
return serviceAccountNamespace().orElse(DEFAULT_NAMESPACE);
} else {
return kubectlNamespace().orElse(DEFAULT_NAMESPACE);
}
} catch (Exception e) {
log.debug(
"Error encountered looking up default namespace in account '{}', defaulting to {}",
accountName,
DEFAULT_NAMESPACE,
e);
return DEFAULT_NAMESPACE;
}
}

@Nonnull
public ImmutableList<KubernetesKind> getCrds() {
return crdSupplier.get().keySet().stream().filter(this::isValidKind).collect(toImmutableList());
Expand Down Expand Up @@ -512,16 +460,16 @@ public Collection<KubernetesPodMetric> topPod(String namespace, String pod) {
"top", KubernetesKind.POD, namespace, () -> jobExecutor.topPod(this, namespace, pod));
}

public void deploy(KubernetesManifest manifest) {
runAndRecordMetrics(
public KubernetesManifest deploy(KubernetesManifest manifest) {
return runAndRecordMetrics(
"deploy",
manifest.getKind(),
manifest.getNamespace(),
() -> jobExecutor.deploy(this, manifest));
}

public void replace(KubernetesManifest manifest) {
runAndRecordMetrics(
public KubernetesManifest replace(KubernetesManifest manifest) {
return runAndRecordMetrics(
"replace",
manifest.getKind(),
manifest.getNamespace(),
Expand Down
Loading

0 comments on commit 9e3a73b

Please sign in to comment.