fix (jkube-kit/enricher) : YAML multiline annotations incorrectly ser…

…ialized on windows Related to #2841 Currently for enforcing trailing newline in case of multi line strings in resource annotations, we were relying on presence of `System.lineSeparator()`. This was causing problems as YAML multiline scalar values are converted into strings with Line Feed (`\n`) endings when they're deserialized into objects. Modify logic in MetadataVisitor with respect to this behavior. Signed-off-by: Rohan Kumar <[email protected]>
eclipse-jkube · May 3, 2024 · 8fdf3cb · 8fdf3cb
1 parent dc0ce29
commit 8fdf3cb
Show file tree

Hide file tree

Showing 3 changed files with 90 additions and 6 deletions.
diff --git a/jkube-kit/common/src/test/java/org/eclipse/jkube/kit/common/util/SerializationTest.java b/jkube-kit/common/src/test/java/org/eclipse/jkube/kit/common/util/SerializationTest.java
@@ -20,6 +20,8 @@
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.api.model.Pod;
 import io.fabric8.openshift.api.model.Template;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
 
@@ -215,4 +217,79 @@ void saveYaml_withConfigMap_savesFile(@TempDir Path targetDir) throws IOExceptio
         "data:%n" +
         "  key: value%n"));
   }
+
+  @Nested
+  @DisplayName("Yaml scalar with multi line values")
+  class YamlScalarWithMultilineString {
+    @Test
+    @DisplayName("string ends with newline, then add | to use block style in serialized object")
+    void whenStringEndingWithNewline_thenAddBlockIndicatorInSerializedObject(@TempDir Path targetDir) throws IOException {
+      // Given
+      final File targetFile = targetDir.resolve("cm.yaml").toFile();
+      final ConfigMap source = new ConfigMapBuilder()
+              .withNewMetadata()
+              .addToAnnotations("proxy.istio.io/config", String.format("proxyMetadata:%n    ISTIO_META_DNS_CAPTURE: \"false\"%nholdApplicationUntilProxyStarts: true\n"))
+              .endMetadata()
+              .build();
+      // When
+      Serialization.saveYaml(targetFile, source);
+      // Then
+      assertThat(targetFile)
+          .content()
+          .isEqualTo(String.format("---%n" +
+              "apiVersion: v1%n" +
+              "kind: ConfigMap%n" +
+              "metadata:%n" +
+              "  annotations:%n" +
+              "    proxy.istio.io/config: |%n" +
+              "      proxyMetadata:%n" +
+              "          ISTIO_META_DNS_CAPTURE: \"false\"%n"+
+              "      holdApplicationUntilProxyStarts: true%n"));
+    }
+
+    @Test
+    @DisplayName("when string contains windows line breaks, then convert then to unix line breaks during deserialization")
+    void unmarshal_withWindowsLineEndings_shouldDeserializeMultilineStringWithLineFeeds() {
+      // Given
+      String input = "apiVersion: v1\r\n" +
+          "kind: ConfigMap\r\n" +
+          "metadata:\r\n" +
+          "  annotations:\r\n" +
+          "    proxy.istio.io/config: |\r\n" +
+          "      proxyMetadata:\r\n" +
+          "        ISTIO_META_DNS_CAPTURE: \"false\"\r\n"+
+          "      holdApplicationUntilProxyStarts: true\r\n";
+
+      // When
+      ConfigMap configMap = Serialization.unmarshal(input, ConfigMap.class);
+
+      // Then
+      assertThat(configMap.getMetadata().getAnnotations())
+              .containsEntry("proxy.istio.io/config", "proxyMetadata:\n" +
+                      "  ISTIO_META_DNS_CAPTURE: \"false\"\n" +
+              "holdApplicationUntilProxyStarts: true\n");
+    }
+
+    @Test
+    @DisplayName("when string contains unix line breaks, then line breaks remain unchanged during deserialization")
+    void unmarshal_withUnixLineEndings_shouldDeserializeMultilineStringWithLineFeeds() {
+      // Given
+      String input = "apiVersion: v1\n" +
+          "kind: ConfigMap\n" +
+          "metadata:\n" +
+          "  annotations:\n" +
+          "    proxy.istio.io/config: |\n" +
+          "      proxyMetadata:\n" +
+          "        ISTIO_META_DNS_CAPTURE: \"false\"\n"+
+          "      holdApplicationUntilProxyStarts: true\n";
+      // When
+      ConfigMap configMap = Serialization.unmarshal(input, ConfigMap.class);
+
+      // Then
+      assertThat(configMap.getMetadata().getAnnotations())
+              .containsEntry("proxy.istio.io/config", "proxyMetadata:\n" +
+                      "  ISTIO_META_DNS_CAPTURE: \"false\"\n" +
+                      "holdApplicationUntilProxyStarts: true\n");
+    }
+  }
 }
diff --git a/...nricher/api/src/main/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitor.java b/...nricher/api/src/main/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitor.java
@@ -60,6 +60,7 @@
 import io.fabric8.openshift.api.model.RouteFluent;
 import lombok.AllArgsConstructor;
 
+import static org.apache.commons.lang3.StringUtils.containsAny;
 import static org.eclipse.jkube.kit.common.util.PropertiesUtil.toMap;
 
 /**
@@ -112,8 +113,8 @@ private static MetaDataConfig getLabels(ResourceConfig resourceConfig) {
   }
 
   private String appendTrailingNewLineIfMultiline(String value) {
-    if (value.contains(System.lineSeparator()) && !value.endsWith(System.lineSeparator())) {
-      return value + System.lineSeparator();
+    if (containsAny(value, "\r?\n") && !value.endsWith("\n")) {
+      return value.replaceAll("\r?\n", "\n") + "\n";
     }
     return value;
   }

diff --git a/...her/api/src/test/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitorTest.java b/...her/api/src/test/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitorTest.java
@@ -38,6 +38,8 @@
 import io.fabric8.openshift.api.model.RouteBuilder;
 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 static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.data.MapEntry.entry;
@@ -350,12 +352,16 @@ void route() {
     assertThat(route.build().getMetadata().getLabels()).containsOnly(entry("route", "Yay"));
   }
 
-  @Test
-  void metadataVisit_whenMultilineAnnotationProvided_shouldAddTrailingNewline() {
+  @ParameterizedTest
+  @ValueSource(strings = {
+          "proxyMetadata:\n ISTIO_META_DNS_CAPTURE: \"false\"\nholdUntilProxyStarts: true",
+          "proxyMetadata:\r\n ISTIO_META_DNS_CAPTURE: \"false\"\r\nholdUntilProxyStarts: true"
+  })
+  void metadataVisit_whenMultilineAnnotationProvided_shouldAddTrailingNewline(String multiLineScalar) {
     // Given
     Properties allProps = new Properties();
     final ObjectMetaBuilder db = new ObjectMetaBuilder();
-    allProps.put("multiline/config", String.format("proxyMetadata:%n ISTIO_META_DNS_CAPTURE: \"false\"%nholdUntilProxyStarts: true"));
+    allProps.put("multiline/config", String.format(multiLineScalar));
     ResourceConfig rc = ResourceConfig.builder()
         .annotations(MetaDataConfig.builder()
             .all(allProps)
@@ -367,6 +373,6 @@ void metadataVisit_whenMultilineAnnotationProvided_shouldAddTrailingNewline() {
 
     // Then
     assertThat(db.build().getAnnotations())
-        .containsOnly(entry("multiline/config", String.format("proxyMetadata:%n ISTIO_META_DNS_CAPTURE: \"false\"%nholdUntilProxyStarts: true%n")));
+        .containsOnly(entry("multiline/config", "proxyMetadata:\n ISTIO_META_DNS_CAPTURE: \"false\"\nholdUntilProxyStarts: true\n"));
   }
 }