fix(kubernetes): Restore adding a suffix to jobs (#4610)

* test(kubernetes): Slightly refactor run job test

Pull a string to a variable to make the upcoming changes easier.

* fix(kubernetes): Restore adding a suffix to jobs

In Spinnaker 1.20, we removed the behavior where Spinnaker would
automatically add a random suffix to Kubernetes jobs before deploying
them, in favor of having users set the metatdata.generateName field
to have Kubernetes do this.

This ended up being more of a breaking change than anticipated,
particularly for users with a lot of preconfigured jobs, as by
nature these jobs re-use the same name/namespace for jobs in
different pipelines.

While we would like to longer-term still push users towards using
the Kubernetes-native API of generateName for this (and avoid
introducing more Spinnaker-specific flags/terminology that users
need to learn), it is clear that we need to provide a much
smoother upgrade path than we initially did.

This commit puts the new behavior behind a global flag,
'kubernetes.jobs.appendSuffix'. When the flag is true (which will
be the default), a random suffix will be added to all jobs as
was the case in 1.19 and earlier.

I propose the following rollout plan:
* Remainder of 1.20 and all of 1.21. The flag defaults to 'true',
and a suffix is added to all jobs when a name is specified. A warning
is printed to the log upon adding this suffix.
* 1.22: The flag defaults to false. A suffix is no longer added to jobs
that are specified by name, but the flag can still be enabled to restore
the old behavior.
* 1.23: The flag is removed.

Throughout this, jobs which specify a generateName will be passed through
to Kubernetes as is. This allows an incremental upgrade process, where
users can update all necessary manifests during 1.20 and 1.21 (as well
as during 1.22 if they manually update the flag).

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

clouddriver-kubernetes/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/converter/job/KubernetesRunJobOperationConverter.java

@@ -28,20 +28,27 @@
 import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsSupport;
 import com.netflix.spinnaker.clouddriver.security.ProviderVersion;
 import java.util.Map;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.beans.factory.annotation.Value;
 import org.springframework.stereotype.Component;
 
 @KubernetesOperation(RUN_JOB)
 @Component
 public class KubernetesRunJobOperationConverter extends AbstractAtomicOperationsCredentialsSupport {
   private final KubernetesV2ArtifactProvider artifactProvider;
+  private final boolean appendSuffix;
 
-  public KubernetesRunJobOperationConverter(KubernetesV2ArtifactProvider artifactProvider) {
+  @Autowired
+  public KubernetesRunJobOperationConverter(
+      KubernetesV2ArtifactProvider artifactProvider,
+      @Value("${kubernetes.jobs.append-suffix:true}") boolean appendSuffix) {
     this.artifactProvider = artifactProvider;
+    this.appendSuffix = appendSuffix;
   }
 
   @Override
   public AtomicOperation convertOperation(Map input) {
-    return new KubernetesRunJobOperation(convertDescription(input), artifactProvider);
+    return new KubernetesRunJobOperation(convertDescription(input), artifactProvider, appendSuffix);
   }
 
   @Override

clouddriver-kubernetes/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/op/job/KubernetesRunJobOperation.java

@@ -39,11 +39,15 @@ public class KubernetesRunJobOperation
   private static final String OP_NAME = "RUN_KUBERNETES_JOB";
   private final KubernetesRunJobOperationDescription description;
   private final ArtifactProvider provider;
+  private final boolean appendSuffix;
 
   public KubernetesRunJobOperation(
-      KubernetesRunJobOperationDescription description, ArtifactProvider provider) {
+      KubernetesRunJobOperationDescription description,
+      ArtifactProvider provider,
+      boolean appendSuffix) {
     this.description = description;
     this.provider = provider;
+    this.appendSuffix = appendSuffix;
   }
 
   private static Task getTask() {
@@ -65,13 +69,25 @@ public KubernetesRunJobDeploymentResult operate(List _unused) {
       jobSpec.setNamespace(this.description.getNamespace());
     }
 
-    // We require that the recreate strategy be used; this is because jobs are immutable and trying
-    // to re-run a job with apply will either:
-    // (1) succeed and leave the job unchanged (but will not trigger a re-run)
-    // (2) fail if we try to change anything
-    // As the purpose of a run job stage is to ensure that each execution causes a job to run, we'll
-    // force a new job to be created each time.
-    KubernetesManifestAnnotater.setDeploymentStrategy(jobSpec, DeployStrategy.RECREATE);
+    if (appendSuffix && !jobSpec.hasGenerateName()) {
+      log.warn(
+          "Appending a random suffix to job with name {} before deploying. In Spinnaker 1.22, this suffix"
+              + " will no longer be added. To continue having a random suffix added, please update the job"
+              + " to specify a metadata.generateName. To immediately disable this suffix, set"
+              + " kubernetes.jobs.addSuffix to false in your clouddriver config.",
+          jobSpec.getName());
+      String currentName = jobSpec.getName();
+      String postfix = Long.toHexString(Double.doubleToLongBits(Math.random()));
+      jobSpec.setName(currentName + "-" + postfix);
+    } else {
+      // We require that the recreate strategy be used; this is because jobs are immutable and
+      // trying to re-run a job with apply will either:
+      // (1) succeed and leave the job unchanged (but will not trigger a re-run)
+      // (2) fail if we try to change anything
+      // As the purpose of a run job stage is to ensure that each execution causes a job to run,
+      // we'll force a new job to be created each time.
+      KubernetesManifestAnnotater.setDeploymentStrategy(jobSpec, DeployStrategy.RECREATE);
+    }
 
     KubernetesDeployManifestDescription deployManifestDescription =
         new KubernetesDeployManifestDescription();

clouddriver-kubernetes/src/test/java/com/netflix/spinnaker/clouddriver/kubernetes/op/job/KubernetesRunJobOperationTest.java

@@ -24,6 +24,7 @@
 
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterators;
 import com.netflix.spinnaker.clouddriver.data.task.DefaultTask;
 import com.netflix.spinnaker.clouddriver.data.task.TaskRepository;
 import com.netflix.spinnaker.clouddriver.kubernetes.KubernetesCloudProvider;
@@ -47,13 +48,16 @@
 import java.util.Map;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.junit.platform.runner.JUnitPlatform;
 import org.junit.runner.RunWith;
 
 @RunWith(JUnitPlatform.class)
 final class KubernetesRunJobOperationTest {
   private static final String NAMESPACE = "my-namespace";
   private static final String GENERATE_SUFFIX = "-abcd";
+  private static final String DEPLOYED_JOB = "job my-job";
   private static final ResourcePropertyRegistry resourcePropertyRegistry =
       new GlobalResourcePropertyRegistry(
           ImmutableList.of(new KubernetesReplicaSetHandler(), new KubernetesServiceHandler()),
@@ -67,34 +71,60 @@ void setTask() {
   @Test
   void deploysJobWithName() {
     KubernetesRunJobOperationDescription runJobDescription = baseJobDescription("job.yml");
-    OperationResult result = operate(runJobDescription);
+    OperationResult result = operate(runJobDescription, false);
 
     assertThat(result.getManifestNamesByNamespace()).containsOnlyKeys(NAMESPACE);
     assertThat(result.getManifestNamesByNamespace().get(NAMESPACE))
-        .containsExactlyInAnyOrder("job my-job");
+        .containsExactlyInAnyOrder(DEPLOYED_JOB);
   }
 
   @Test
-  void deploysJobWithGenerateName() {
+  void deploysJobWithNameAndAddsSuffix() {
+    KubernetesRunJobOperationDescription runJobDescription = baseJobDescription("job.yml");
+    OperationResult result = operate(runJobDescription, true);
+
+    assertThat(result.getManifestNamesByNamespace()).containsOnlyKeys(NAMESPACE);
+    assertThat(result.getManifestNamesByNamespace().get(NAMESPACE)).hasSize(1);
+    String job =
+        Iterators.getOnlyElement(result.getManifestNamesByNamespace().get(NAMESPACE).iterator());
+    assertThat(job).startsWith(DEPLOYED_JOB);
+    String suffix = job.substring(DEPLOYED_JOB.length());
+    assertThat(suffix).startsWith("-");
+    // We're asserting that at least 4 characters are added after the hyphen; rather than
+    // overspecify the test by looking up exactly how many are added, we're just making sure that a
+    // reasonable number to guarantee randomness are added.
+    assertThat(suffix.length()).isGreaterThanOrEqualTo(5);
+  }
+
+  @ParameterizedTest
+  @ValueSource(booleans = {true, false})
+  void deploysJobWithGenerateName(boolean appendSuffix) {
     KubernetesRunJobOperationDescription runJobDescription =
         baseJobDescription("job-generate-name.yml");
-    OperationResult result = operate(runJobDescription);
+    OperationResult result = operate(runJobDescription, appendSuffix);
 
     assertThat(result.getManifestNamesByNamespace()).containsOnlyKeys(NAMESPACE);
     assertThat(result.getManifestNamesByNamespace().get(NAMESPACE))
-        .containsExactlyInAnyOrder("job my-job" + GENERATE_SUFFIX);
+        .containsExactlyInAnyOrder(DEPLOYED_JOB + GENERATE_SUFFIX);
   }
 
-  @Test
-  void overridesNamespace() {
+  @ParameterizedTest
+  @ValueSource(booleans = {true, false})
+  void overridesNamespace(boolean appendSuffix) {
     String overrideNamespace = "override-namespace";
     KubernetesRunJobOperationDescription runJobDescription =
         baseJobDescription("job.yml").setNamespace(overrideNamespace);
-    OperationResult result = operate(runJobDescription);
+    OperationResult result = operate(runJobDescription, appendSuffix);
 
     assertThat(result.getManifestNamesByNamespace()).containsOnlyKeys(overrideNamespace);
-    assertThat(result.getManifestNamesByNamespace().get(overrideNamespace))
-        .containsExactlyInAnyOrder("job my-job");
+    assertThat(result.getManifestNamesByNamespace().get(overrideNamespace)).hasSize(1);
+    String job =
+        Iterators.getOnlyElement(
+            result.getManifestNamesByNamespace().get(overrideNamespace).iterator());
+    // In this test, we don't care whether a suffix was added, we're just checking that the job
+    // ended up in the right namespace, so we only check that the entry starts with the expected
+    // job name.
+    assertThat(job).startsWith(DEPLOYED_JOB);
   }
 
   private static KubernetesRunJobOperationDescription baseJobDescription(String manifest) {
@@ -159,10 +189,12 @@ private static KubernetesV2Credentials getMockKubernetesV2Credentials() {
     return credentialsMock;
   }
 
-  private static OperationResult operate(KubernetesRunJobOperationDescription description) {
+  private static OperationResult operate(
+      KubernetesRunJobOperationDescription description, boolean appendSuffix) {
     ArtifactProvider provider = mock(ArtifactProvider.class);
     when(provider.getArtifacts(any(String.class), any(String.class), any(String.class)))
         .thenReturn(ImmutableList.of());
-    return new KubernetesRunJobOperation(description, provider).operate(ImmutableList.of());
+    return new KubernetesRunJobOperation(description, provider, appendSuffix)
+        .operate(ImmutableList.of());
   }
 }