fix(kubernetes): fix patchBody typing depending on strategy type (#3283…

…) (#3287) * fix(kubernetes): fix patchBody typing depending on strategy type * fix(kubernetes): PatchManifestContext.getManifests Nonnull and immutable return
spinnaker · Nov 11, 2019 · 22ab949 · 22ab949
1 parent 97366b9
commit 22ab949
Show file tree

Hide file tree

Showing 3 changed files with 87 additions and 9 deletions.
diff --git a/...in/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/PatchManifestContext.java b/...in/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/PatchManifestContext.java
@@ -16,21 +16,22 @@
 
 package com.netflix.spinnaker.orca.clouddriver.tasks.manifest;
 
+import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
 import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
+import com.google.common.collect.ImmutableList;
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-import javax.annotation.Nullable;
+import javax.annotation.Nonnull;
 import lombok.Builder;
 import lombok.Value;
 
 @Value
 @JsonDeserialize(builder = PatchManifestContext.PatchManifestContextBuilder.class)
 @Builder(builderClassName = "PatchManifestContextBuilder", toBuilder = true)
 public class PatchManifestContext implements ManifestContext {
-  private Map<Object, Object> patchBody;
+  private Object patchBody;
   private Source source;
 
   private String manifestArtifactId;
@@ -43,10 +44,49 @@ public class PatchManifestContext implements ManifestContext {
 
   @Builder.Default private boolean skipExpressionEvaluation = false;
 
-  @Nullable
+  @Builder.Default
+  private PatchManifestContext.Options options = PatchManifestContext.Options.builder().build();
+
+  @Builder(builderClassName = "OptionsBuilder", toBuilder = true)
+  @JsonDeserialize(builder = PatchManifestContext.Options.OptionsBuilder.class)
+  @Value
+  static class Options {
+    @Builder.Default private MergeStrategy mergeStrategy = MergeStrategy.STRATEGIC;
+    @Builder.Default private boolean record = true;
+
+    @JsonPOJOBuilder(withPrefix = "")
+    static class OptionsBuilder {}
+  }
+
+  public enum MergeStrategy {
+    @JsonProperty("strategic")
+    STRATEGIC,
+
+    @JsonProperty("json")
+    JSON,
+
+    @JsonProperty("merge")
+    MERGE
+  }
+
+  @Nonnull
   @Override
   public List<Map<Object, Object>> getManifests() {
-    return Collections.singletonList(patchBody);
+    /*
+     * Clouddriver expects patchBody to be a List when the mergeStrategy is json, and a Map otherwise.
+     * Prior to Spinnaker 1.15, Deck sent either a List or Map depending on merge strategy, and Orca
+     * deserialized it as an Object. In Spinnaker 1.15 and 1.16, a bug was introduced where patchBody
+     * was deserialized as a Map regardless of merge strategy. Starting in 1.17, Deck will send a List
+     * regardless of merge strategy, but we should handle receiving a Map from pipelines configured
+     * in 1.15 and 1.16.
+     */
+    if (patchBody == null) {
+      return ImmutableList.of();
+    }
+    if (patchBody instanceof List) {
+      return ImmutableList.copyOf((List) patchBody);
+    }
+    return ImmutableList.of((Map<Object, Object>) patchBody);
   }
 
   @JsonPOJOBuilder(withPrefix = "")

diff --git a/.../main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/PatchManifestTask.java b/.../main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/PatchManifestTask.java
@@ -19,8 +19,10 @@
 import com.netflix.spinnaker.orca.Task;
 import com.netflix.spinnaker.orca.TaskResult;
 import com.netflix.spinnaker.orca.clouddriver.tasks.AbstractCloudProviderAwareTask;
+import com.netflix.spinnaker.orca.clouddriver.tasks.manifest.PatchManifestContext.MergeStrategy;
 import com.netflix.spinnaker.orca.pipeline.model.Stage;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import javax.annotation.Nonnull;
 import lombok.RequiredArgsConstructor;
@@ -39,11 +41,22 @@ public class PatchManifestTask extends AbstractCloudProviderAwareTask implements
   @Override
   public TaskResult execute(@Nonnull Stage stage) {
     PatchManifestContext context = stage.mapTo(PatchManifestContext.class);
+    MergeStrategy mergeStrategy = context.getOptions().getMergeStrategy();
     ManifestEvaluator.Result result = manifestEvaluator.evaluate(stage, context);
+    List<Map<Object, Object>> patchBody = result.getManifests();
+
+    if (patchBody == null || patchBody.isEmpty()) {
+      throw new IllegalArgumentException(
+          "The Patch (Manifest) stage requires a valid patch body. Please add a patch body inline or with an artifact.");
+    }
+    if (mergeStrategy != MergeStrategy.JSON && patchBody.size() > 1) {
+      throw new IllegalArgumentException(
+          "Only one patch object is valid when patching with `strategic` and `merge` patch strategies.");
+    }
 
     Map<String, Object> task = new HashMap<>(stage.getContext());
     task.put("source", "text");
-    task.put("patchBody", result.getManifests().get(0));
+    task.put("patchBody", mergeStrategy == MergeStrategy.JSON ? patchBody : patchBody.get(0));
     task.put("requiredArtifacts", result.getRequiredArtifacts());
     task.put("allArtifacts", result.getOptionalArtifacts());
 

diff --git a/.../test/java/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/ManifestContextTest.java b/.../test/java/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/ManifestContextTest.java
@@ -55,7 +55,7 @@ void deserialize() throws IOException {
   }
 
   @Test
-  void deserializePatchManifestNoArtifacts() throws IOException {
+  void deserializeInlineMapPatchManifest() throws IOException {
     String json =
         "{\n"
             + "  \"manifestArtifactAccount\": \"account\",\n"
@@ -70,8 +70,33 @@ void deserializePatchManifestNoArtifacts() throws IOException {
     PatchManifestContext context = new ObjectMapper().readValue(json, PatchManifestContext.class);
     assertThat(context.getSource()).isEqualTo(ManifestContext.Source.Text);
     assertThat(context.getManifestArtifactAccount()).isEqualTo("account");
-    assertThat(context.getPatchBody()).containsOnlyKeys("spec");
-    assertThat(context.getPatchBody().get("spec"))
+    assertThat(context.getManifests()).isNotNull();
+    assertThat(context.getManifests().get(0)).containsOnlyKeys("spec");
+    assertThat(context.getManifests().get(0).get("spec"))
+        .isEqualTo(Collections.singletonMap("replicas", "3"));
+  }
+
+  @Test
+  void deserializeInlineListPatchManifest() throws IOException {
+    String json =
+        "{\n"
+            + "  \"manifestArtifactAccount\": \"account\",\n"
+            + "  \"source\": \"text\",\n"
+            + "  \"patchBody\": [\n"
+            + "    {\n"
+            + "         \"spec\": {\n"
+            + "             \"replicas\": \"3\"\n"
+            + "         }\n"
+            + "    }\n"
+            + "  ]\n"
+            + "}";
+
+    PatchManifestContext context = new ObjectMapper().readValue(json, PatchManifestContext.class);
+    assertThat(context.getSource()).isEqualTo(ManifestContext.Source.Text);
+    assertThat(context.getManifestArtifactAccount()).isEqualTo("account");
+    assertThat(context.getManifests()).isNotNull();
+    assertThat(context.getManifests().get(0)).containsOnlyKeys("spec");
+    assertThat(context.getManifests().get(0).get("spec"))
         .isEqualTo(Collections.singletonMap("replicas", "3"));
   }
 }