From b80664fcafd52c9152cb88db0a7699463f60179b Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Mon, 6 May 2024 16:13:16 +0530 Subject: [PATCH] fix (jkube-kit/enricher): YAML multiline annotations incorrectly serialized on windows Related to eclipse-jkube#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 --- .../kit/common/util/SerializationTest.java | 77 +++++++++++++++++++ .../enricher/api/visitor/MetadataVisitor.java | 11 ++- .../api/visitor/MetadataVisitorTest.java | 20 ++++- 3 files changed, 102 insertions(+), 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 index 43bf1a4153..9a6484878d 100644 --- 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("Multi-line strings are serialized to YAML using scalar blocks") + class MultiLineStringsSerializedToScalarYamlBlocks { + @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/jkube-kit/enricher/api/src/main/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitor.java b/jkube-kit/enricher/api/src/main/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitor.java index 80b428f530..b739b11ef8 100644 --- a/jkube-kit/enricher/api/src/main/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitor.java +++ b/jkube-kit/enricher/api/src/main/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitor.java @@ -20,6 +20,8 @@ import java.util.Properties; import java.util.function.Function; import java.util.function.Supplier; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.eclipse.jkube.kit.config.resource.MetaDataConfig; import org.eclipse.jkube.kit.config.resource.ResourceConfig; @@ -73,6 +75,7 @@ public class MetadataVisitor extends TypedVisitor private final Supplier labelSupplier; private final Function> objectMeta; private final Function, Runnable> endMetadata; + private static final Pattern CONTAINS_LINE_BREAK = Pattern.compile("\r?\n"); public MetadataVisitor( Class clazz, @@ -112,8 +115,12 @@ private static MetaDataConfig getLabels(ResourceConfig resourceConfig) { } private String appendTrailingNewLineIfMultiline(String value) { - if (value.contains(System.lineSeparator()) && !value.endsWith(System.lineSeparator())) { - return value + System.lineSeparator(); + Matcher m = CONTAINS_LINE_BREAK.matcher(value); + if (m.find()) { + String eol = m.group(); + if (!value.endsWith(eol)) { + return value + eol; + } } return value; } diff --git a/jkube-kit/enricher/api/src/test/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitorTest.java b/jkube-kit/enricher/api/src/test/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitorTest.java index 50db56be99..37baa53af1 100644 --- a/jkube-kit/enricher/api/src/test/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitorTest.java +++ b/jkube-kit/enricher/api/src/test/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitorTest.java @@ -14,6 +14,7 @@ package org.eclipse.jkube.kit.enricher.api.visitor; import java.util.Properties; +import java.util.stream.Stream; import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; import org.eclipse.jkube.kit.config.resource.MetaDataConfig; @@ -38,6 +39,9 @@ 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.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.data.MapEntry.entry; @@ -350,12 +354,13 @@ void route() { assertThat(route.build().getMetadata().getLabels()).containsOnly(entry("route", "Yay")); } - @Test - void metadataVisit_whenMultilineAnnotationProvided_shouldAddTrailingNewline() { + @ParameterizedTest + @MethodSource + void metadataVisit_whenMultilineAnnotationProvided_shouldAddTrailingNewline(String multiLineScalar, String eol) { // 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 +372,13 @@ 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", String.format("proxyMetadata:%s ISTIO_META_DNS_CAPTURE: \"false\"%sholdUntilProxyStarts: true%s", eol, eol, eol))); + } + + private static Stream metadataVisit_whenMultilineAnnotationProvided_shouldAddTrailingNewline() { + return Stream.of( + Arguments.of("proxyMetadata:\n ISTIO_META_DNS_CAPTURE: \"false\"\nholdUntilProxyStarts: true", "\n"), + Arguments.of("proxyMetadata:\r\n ISTIO_META_DNS_CAPTURE: \"false\"\r\nholdUntilProxyStarts: true", "\r\n") + ); } } \ No newline at end of file