Skip to content

Commit

Permalink
fix (jkube-kit/enricher): YAML multiline annotations incorrectly seri…
Browse files Browse the repository at this point in the history 
…alized 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]>
  • Loading branch information
@rohanKanojia
rohanKanojia authored May 6, 2024
1 parent a77a1d7 commit b80664f
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 6 deletions.
77 changes: 77 additions & 0 deletions jkube-kit/common/src/test/java/org/eclipse/jkube/kit/common/util/SerializationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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");
}
}
}
11 changes: 9 additions & 2 deletions ...nricher/api/src/main/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -73,6 +75,7 @@ public class MetadataVisitor<T extends VisitableBuilder> extends TypedVisitor<T>
private final Supplier<Properties> labelSupplier;
private final Function<T, ObjectMetaFluent<?>> objectMeta;
private final Function<ObjectMetaFluent<?>, Runnable> endMetadata;
private static final Pattern CONTAINS_LINE_BREAK = Pattern.compile("\r?\n");

public MetadataVisitor(
Class<T> clazz,
Expand Down Expand Up @@ -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;
}
Expand Down
20 changes: 16 additions & 4 deletions ...her/api/src/test/java/org/eclipse/jkube/kit/enricher/api/visitor/MetadataVisitorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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)
Expand All @@ -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<Arguments> 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")
);
}
}

0 comments on commit b80664f

Please sign in to comment.