diff --git a/build/Build.Docker.cs b/build/Build.Docker.cs index c9bf2006f4..585b83b93a 100644 --- a/build/Build.Docker.cs +++ b/build/Build.Docker.cs @@ -1,12 +1,15 @@ using Calamari.Build.Utilities; using JetBrains.Annotations; +using Nuke.Common.Tooling; using Nuke.Common.Tools.Docker; -using Nuke.Common.Tools.PowerShell; namespace Calamari.Build; public partial class Build { + //Resolved from PATH so a missing gzip fails by name, rather than as a mystery exit code + static Tool Gzip => ToolResolver.GetPathTool("gzip"); + [PublicAPI] Target BuildDockerImages => d => @@ -82,10 +85,10 @@ public partial class Build return settings; }); - //compress with gzip - PowerShellTasks.PowerShell(_ => _ - .EnableNoProfile() - .SetCommand($"gzip -k -9 -f '{outputFile}'")); + //compress with gzip. Invoked directly rather than via pwsh, which only + //added a dependency on whatever .NET runtime the agent's `pwsh` global tool + //was built against - not the SDK this build pins. + Gzip($"-k -9 -f \"{outputFile}\""); //gzip always uses the .gz suffix var compressedZipPath = $"{outputFile}.gz"; diff --git a/source/Calamari.Common/Plumbing/Extensions/StringExtensions.cs b/source/Calamari.Common/Plumbing/Extensions/StringExtensions.cs index 4ea21405bc..8d2dbb9739 100644 --- a/source/Calamari.Common/Plumbing/Extensions/StringExtensions.cs +++ b/source/Calamari.Common/Plumbing/Extensions/StringExtensions.cs @@ -99,6 +99,11 @@ public static string ToCamelCase(this string text) : null; } + public static bool HasTrailingNewLine(this string? input) + { + return input != null && (input.EndsWith("\n") || input.EndsWith("\r")); + } + public static string EnsureDoubleQuoteIfContainsSpaces(this string text) => EnsureDoubleQuote(text, t => t.Contains(" ")); public static string EnsureDoubleQuote(this string text) => EnsureDoubleQuote(text, t => !t.EndsWith("\"") && !t.StartsWith("\"")); public static string EnsureDoubleQuote(this string text, Predicate shouldQuote) => shouldQuote(text) ? $"\"{text}\"" : text; diff --git a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesEditorTests.cs b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesEditorTests.cs index de6d2736c0..443bd28da2 100644 --- a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesEditorTests.cs +++ b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesEditorTests.cs @@ -98,29 +98,32 @@ public void GenerateVariableDictionary_ReturnsDictionaryOfNodeValuesWithValues() result.Should().BeEquivalentTo(expected); } - [Test] - public void UpdateNodeValue_ReturnsModifiedYaml() + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_ReturnsModifiedYaml(string newLine) { - const string yamlContent = @"root: - node1: ""node1value"" - node2: - node2Nest: - node2nestedValue: ""banana"" - node2Child1: ""node2child1value"" - node2Child2: 42 -"; + var yamlContent = string.Join(newLine, + "root:", + " node1: \"node1value\"", + " node2:", + " node2Nest:", + " node2nestedValue: \"banana\"", + " node2Child1: \"node2child1value\"", + " node2Child2: 42", + ""); + var result = HelmValuesEditor.UpdateNodeValue(yamlContent, "root.node1", "awesome new value"); - const string expected = @"root: - node1: ""awesome new value"" - node2: - node2Nest: - node2nestedValue: ""banana"" - node2Child1: ""node2child1value"" - node2Child2: 42 -"; - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expected.ReplaceLineEndings()); + result.Should() + .Be(string.Join(newLine, + "root:", + " node1: \"awesome new value\"", + " node2:", + " node2Nest:", + " node2nestedValue: \"banana\"", + " node2Child1: \"node2child1value\"", + " node2Child2: 42", + "")); } } } diff --git a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs index e68151e7b3..c063f5e888 100644 --- a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs +++ b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs @@ -1,4 +1,3 @@ -using System; using System.Collections.Generic; using Calamari.ArgoCD; using Calamari.ArgoCD.Conventions; @@ -119,16 +118,37 @@ public void StructuredValue_ImageOnNonDefaultRegistry_UpdatesFullRefAndTracksWit result.UpdatedContents.Should().Contain("name: us-docker.pkg.dev/shared-gke-dev-gqtrxy/argo-test/helloworld:v2"); } - [Test] - public void TwoImagesWithSameTag_OnlyUpdatesConfiguredPath() + // Line endings are spelled out rather than taken from a verbatim literal: .gitattributes checks + // .cs files out with native endings, so a literal's endings always match Environment.NewLine and + // an assertion against it cannot distinguish "preserved the file's endings" from "used the + // agent's". The customer's case was an LF file on a Windows agent, where those differ. + [TestCase("\n")] + [TestCase("\r\n")] + public void TwoImagesWithSameTag_OnlyUpdatesConfiguredPath(string newLine) { - const string yaml = @" -nginx: - tag: 1.0 -redis: - tag: 1.0 -"; - + var yaml = string.Join(newLine, "", "nginx:", " tag: 1.0", "redis:", " tag: 1.0", ""); + + var replacer = new HelmValuesImageReplaceStepVariables(yaml, DefaultRegistry, log); + var images = new List + { + new(ContainerImageReference.FromReferenceString("nginx:1.27.1", DefaultRegistry), "nginx.tag") + }; + + var result = replacer.UpdateImages(images); + + using var scope = new AssertionScope(); + result.UpdatedImageReferences.Should().BeEquivalentTo(["nginx:1.27.1"]); + result.UpdatedContents + .Should() + .Be(string.Join(newLine, "", "nginx:", " tag: 1.27.1", "redis:", " tag: 1.0", "")); + } + + [TestCase("\n")] + [TestCase("\r\n")] + public void TwoImagesWithSameTag_WithoutATrailingNewline_OnlyUpdatesConfiguredPath(string newLine) + { + var yaml = string.Join(newLine, "", "nginx:", " tag: 1.0", "redis:", " tag: 1.0"); + var replacer = new HelmValuesImageReplaceStepVariables(yaml, DefaultRegistry, log); var images = new List { @@ -139,8 +159,9 @@ public void TwoImagesWithSameTag_OnlyUpdatesConfiguredPath() using var scope = new AssertionScope(); result.UpdatedImageReferences.Should().BeEquivalentTo(["nginx:1.27.1"]); - result.UpdatedContents.Should().Contain($"nginx:{Environment.NewLine} tag: 1.27.1"); - result.UpdatedContents.Should().Contain($"redis:{Environment.NewLine} tag: 1.0"); + result.UpdatedContents + .Should() + .Be(string.Join(newLine, "", "nginx:", " tag: 1.27.1", "redis:", " tag: 1.0")); } [Test] diff --git a/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs b/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs index fff879ad7d..85a42b666a 100644 --- a/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs +++ b/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs @@ -1,5 +1,6 @@ using System; using Calamari.ArgoCD.Helm; +using Calamari.Common.Commands; using FluentAssertions; using NUnit.Framework; @@ -51,96 +52,130 @@ public void GetValueAtPath_ReturnsTheValueOfTheSpecifiedNode(string path, string result.Should().Be(expected); } - [Test] - public void UpdateNodeValue_WithNonDelimitedNodeValue_ReplacesValueInDocument() + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_WithNonDelimitedNodeValue_ReplacesValueInDocument(string newLine) { - const string yamlContent = @" -root: - node1: 42 - node2: stable -"; + var yamlContent = string.Join(newLine, "", "root:", " node1: 42", " node2: stable", ""); var sut = new HelmYamlParser(yamlContent); - const string expectedUpdate = @" -root: - node1: 69 - node2: stable -"; + var result = sut.UpdateContentForPath("root.node1", "69"); + + result.Should().Be(string.Join(newLine, "", "root:", " node1: 69", " node2: stable", "")); + } + + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_WithDoubleQuoteDelimitedNodeValue_PreservesDelimitersWithNewValue(string newLine) + { + var yamlContent = string.Join(newLine, "", "root:", " node1: 42", " node2: \"latest\"", ""); + + var sut = new HelmYamlParser(yamlContent); + + var result = sut.UpdateContentForPath("root.node2", "stable"); + + result.Should().Be(string.Join(newLine, "", "root:", " node1: 42", " node2: \"stable\"", "")); + } + + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_WithSingleQuoteDelimitedNodeValue_PreservesDelimitersWithNewValue(string newLine) + { + var yamlContent = string.Join(newLine, "", "root:", " node1: 42", " node2: 'latest'", ""); + + var sut = new HelmYamlParser(yamlContent); + + var result = sut.UpdateContentForPath("root.node2", "stable"); + + result.Should().Be(string.Join(newLine, "", "root:", " node1: 42", " node2: 'stable'", "")); + } + + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_RespectsTrailingWhitespaceFromInput(string newLine) + { + var yamlContent = string.Join(newLine, "", "root:", " node1: 42", " ", ""); + + var sut = new HelmYamlParser(yamlContent); var result = sut.UpdateContentForPath("root.node1", "69"); - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expectedUpdate.ReplaceLineEndings()); + result.Should().Be(string.Join(newLine, "", "root:", " node1: 69", " ", "")); } [Test] - public void UpdateNodeValue_WithDoubleQuoteDelimitedNodeValue_PreservesDelimitersWithNewValue() + public void UpdateNodeValue_WithCrlfLineEndings_PreservesCrlfOnEveryLine() { - const string yamlContent = @" -root: - node1: 42 - node2: ""latest"" -"; + const string yamlContent = "root:\r\n node1: 42\r\n node2: stable\r\n"; var sut = new HelmYamlParser(yamlContent); - const string expectedUpdate = @" -root: - node1: 42 - node2: ""stable"" -"; + var result = sut.UpdateContentForPath("root.node1", "69"); - var result = sut.UpdateContentForPath("root.node2", "stable"); + result.Should().Be("root:\r\n node1: 69\r\n node2: stable\r\n"); + } + + [Test] + public void UpdateNodeValue_WithLfLineEndings_PreservesLfOnEveryLine() + { + const string yamlContent = "root:\n node1: 42\n node2: stable\n"; + + var sut = new HelmYamlParser(yamlContent); + + var result = sut.UpdateContentForPath("root.node1", "69"); - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expectedUpdate.ReplaceLineEndings()); + result.Should().Be("root:\n node1: 69\n node2: stable\n"); } [Test] - public void UpdateNodeValue_WithSingleQuoteDelimitedNodeValue_PreservesDelimitersWithNewValue() + public void UpdateNodeValue_WithNoTrailingNewline_DoesNotAddOne() { - const string yamlContent = @" -root: - node1: 42 - node2: 'latest' -"; + const string yamlContent = "root:\n node1: 42"; var sut = new HelmYamlParser(yamlContent); - const string expectedUpdate = @" -root: - node1: 42 - node2: 'stable' -"; + var result = sut.UpdateContentForPath("root.node1", "69"); + + result.Should().Be("root:\n node1: 69"); + } + + [Test] + public void UpdateNodeValue_WithCrlfAndNoTrailingNewline_PreservesBoth() + { + const string yamlContent = "root:\r\n node1: 42\r\n node2: \"latest\""; + + var sut = new HelmYamlParser(yamlContent); var result = sut.UpdateContentForPath("root.node2", "stable"); - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expectedUpdate.ReplaceLineEndings()); + result.Should().Be("root:\r\n node1: 42\r\n node2: \"stable\""); } [Test] - public void UpdateNodeValue_RespectsTrailingWhitespaceFromInput() + public void UpdateNodeValue_WithUnchangedPath_ReturnsContentByteForByte() { - const string yamlContent = @" -root: - node1: 42 - -"; + const string yamlContent = "root:\r\n node1: 42\r\n"; var sut = new HelmYamlParser(yamlContent); - const string expectedUpdate = @" -root: - node1: 69 - -"; + var result = sut.UpdateContentForPath("root.missing", "69"); - var result = sut.UpdateContentForPath("root.node1", "69"); + result.Should().Be(yamlContent); + } + + [Test] + public void UpdateNodeValue_WithAFoldedValue_FailsWithAMessageNamingThePathAndTheFix() + { + const string yamlContent = "image:\n tag: >\n 1.21\n"; + + var sut = new HelmYamlParser(yamlContent); + + var act = () => sut.UpdateContentForPath("image.tag", "1.25"); - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expectedUpdate.ReplaceLineEndings()); + act.Should() + .Throw() + .WithMessage("*image.tag*line 2*folded block scalar (>)*literal block (|)*"); } [Test] diff --git a/source/Calamari.Tests/ArgoCD/ImageReplacerInvarianceTests.cs b/source/Calamari.Tests/ArgoCD/ImageReplacerInvarianceTests.cs new file mode 100644 index 0000000000..bbd96843e7 --- /dev/null +++ b/source/Calamari.Tests/ArgoCD/ImageReplacerInvarianceTests.cs @@ -0,0 +1,242 @@ +using System.Collections.Generic; +using System.Linq; +using Calamari.ArgoCD; +using Calamari.ArgoCD.Conventions; +using Calamari.ArgoCD.Conventions.UpdateImageTag; +using Calamari.ArgoCD.Models; +using Calamari.Common.Plumbing.Logging; +using Calamari.Testing.Helpers; +using FluentAssertions; +using NUnit.Framework; + +namespace Calamari.Tests.ArgoCD +{ + /// + /// Systematically varies the formatting a customer's file might use and asserts the only thing a + /// replacer may ever do is replace the image reference. Anything else — reformatting, a lost + /// anchor, a duplicated or truncated fragment — fails here, whatever the cause. + /// + [TestFixture] + public class ImageReplacerInvarianceTests + { + const string OldImage = "nginx:1.21"; + readonly ILog log = new InMemoryLog(); + + [TestCaseSource(nameof(KustomizationVariants))] + public void InlineJsonPatch_OnlyEverReplacesTheImageReference(Variant variant) + { + AssertOnlyTheImageMayChange(variant, + (content, images) => new InlineJsonPatchReplacer(content, ArgoCDConstants.DefaultContainerRegistry, log).UpdateImages(images)); + } + + [TestCaseSource(nameof(StrategicMergeVariants))] + public void InlineStrategicMerge_OnlyEverReplacesTheImageReference(Variant variant) + { + AssertOnlyTheImageMayChange(variant, + (content, images) => new InlineStrategicMergeImageReplacer(content, ArgoCDConstants.DefaultContainerRegistry, log).UpdateImages(images)); + } + + [TestCaseSource(nameof(Json6902Variants))] + public void Json6902Patch_OnlyEverReplacesTheImageReference(Variant variant) + { + AssertOnlyTheImageMayChange(variant, + (content, images) => new YamlJson6902PatchImageReplacer(content, ArgoCDConstants.DefaultContainerRegistry, log).UpdateImages(images)); + } + + [TestCaseSource(nameof(KustomizationVariants))] + public void InlineJsonPatch_WithNoMatchingImage_ReturnsTheFileUnchanged(Variant variant) + { + var noMatch = Images("redis:7.0"); + + var result = new InlineJsonPatchReplacer(variant.Content, ArgoCDConstants.DefaultContainerRegistry, log).UpdateImages(noMatch); + + result.UpdatedContents.Should().Be(variant.Content); + result.UpdatedImageReferences.Should().BeEmpty(); + } + + void AssertOnlyTheImageMayChange(Variant variant, System.Func, ImageReplacementResult> replace) + { + YamlStreamLoader.TryLoadSilent(variant.Content).Should().NotBeNull($"the generated variant {variant.Name} should be valid YAML"); + + foreach (var newTag in new[] { "nginx:9", "nginx:1.25", "nginx:1.25-alpine-with-a-long-suffix" }) + { + var result = replace(variant.Content, Images(newTag)); + + var unchanged = variant.Content; + var surgicallyChanged = variant.Content.Replace(OldImage, newTag); + + if (variant.ExpectUpdate) + { + // Asserting the exact result, not "unchanged or changed", so that a regression to + // silently declining every update is caught rather than passing as a safe no-op. + result.UpdatedContents.Should() + .Be(surgicallyChanged, $"{variant.Name} with {newTag} must change only the image reference"); + } + else + { + result.UpdatedContents.Should() + .BeOneOf(new[] { unchanged, surgicallyChanged }, + $"{variant.Name} with {newTag} must either decline to change the file or change only the image reference"); + } + + YamlStreamLoader.TryLoadSilent(result.UpdatedContents) + .Should() + .NotBeNull($"{variant.Name} with {newTag} must still be valid YAML"); + } + } + + static List Images(string image) + { + return new List + { + new(ContainerImageReference.FromReferenceString(image, ArgoCDConstants.DefaultContainerRegistry)) + }; + } + + public record Variant(string Name, string Content, bool ExpectUpdate) + { + public override string ToString() => Name; + } + + static IEnumerable KustomizationVariants() + { + foreach (var shape in Shapes()) + { + var body = new List + { + "apiVersion: kustomize.config.k8s.io/v1beta1", + "kind: Kustomization", + "patches:", + " - target:", + $" kind: Deployment{shape.InlineComment}", + $" patch: {shape.BlockIndicator}", + }; + body.AddRange(PatchLines(shape)); + + yield return Build($"kustomization[{shape.Name}]", shape, body); + } + } + + static IEnumerable StrategicMergeVariants() + { + foreach (var shape in Shapes()) + { + var body = new List + { + "apiVersion: kustomize.config.k8s.io/v1beta1", + "kind: Kustomization", + "patchesStrategicMerge:", + $" - {shape.BlockIndicator}", + }; + body.AddRange(PatchLines(shape, blockIndent: " ")); + + yield return Build($"strategicMerge[{shape.Name}]", shape, body); + } + } + + static IEnumerable Json6902Variants() + { + foreach (var quote in new[] { "", "\"", "'" }) + foreach (var anchor in new[] { "", "&img " }) + foreach (var newLine in new[] { "\n", "\r\n" }) + foreach (var trailing in new[] { true, false }) + { + var name = $"6902[quote={(quote == "" ? "none" : quote)},anchor={(anchor == "" ? "no" : "yes")},nl={(newLine == "\n" ? "LF" : "CRLF")},trailingNl={trailing}]"; + var body = new List + { + "# rollout patch", + "- op: replace", + " path: /spec/template/spec/containers/0/image", + $" value: {anchor}{quote}{OldImage}{quote}", + }; + + var content = string.Join(newLine, body) + (trailing ? newLine : ""); + yield return new Variant(name, content, ExpectUpdate: true); + } + + foreach (var aliased in AliasSharingVariants()) + yield return aliased; + } + + /// + /// An alias resolves to the same node object, so the same scalar gets visited — and edited — + /// more than once. Equal-length tags can hide a splice applied at stale offsets, so the + /// invariance assertions deliberately try tags shorter and longer than the original. + /// + static IEnumerable AliasSharingVariants() + { + foreach (var newLine in new[] { "\n", "\r\n" }) + foreach (var quote in new[] { "", "\"" }) + foreach (var trailing in new[] { true, false }) + { + var name = $"6902-alias[nl={(newLine == "\n" ? "LF" : "CRLF")},quote={(quote == "" ? "none" : quote)},trailingNl={trailing}]"; + var body = new List + { + "- op: add", + " path: /spec/template/spec/containers", + " value:", + " - &web", + " name: nginx", + $" image: {quote}{OldImage}{quote}", + "- op: add", + " path: /spec/template/spec/initContainers", + " value:", + " - *web", + }; + + var content = string.Join(newLine, body) + (trailing ? newLine : ""); + yield return new Variant(name, content, ExpectUpdate: true); + } + } + + static IEnumerable PatchLines(Shape shape, string blockIndent = " ") + { + yield return $"{blockIndent}apiVersion: apps/v1"; + yield return $"{blockIndent}kind: Deployment"; + if (shape.BlankLineInBlock) + yield return ""; + yield return $"{blockIndent}spec:"; + yield return $"{blockIndent} template:"; + yield return $"{blockIndent} spec:"; + yield return $"{blockIndent} containers:"; + yield return $"{blockIndent} - name: nginx{shape.InlineComment}"; + yield return $"{blockIndent} image: {shape.Quote}{OldImage}{shape.Quote}"; + } + + static Variant Build(string name, Shape shape, List body) + { + var lines = new List(); + if (shape.LeadingComment) + lines.Add("# managed by the platform team"); + lines.AddRange(body); + + var content = string.Join(shape.NewLine, lines) + (shape.TrailingNewLine ? shape.NewLine : ""); + return new Variant($"{name}", content, ExpectUpdate: true); + } + + record Shape(string Name, string NewLine, bool TrailingNewLine, bool LeadingComment, string InlineComment, bool BlankLineInBlock, string BlockIndicator, string Quote); + + static IEnumerable Shapes() + { + foreach (var newLine in new[] { "\n", "\r\n" }) + foreach (var trailing in new[] { true, false }) + foreach (var leadingComment in new[] { true, false }) + foreach (var inlineComment in new[] { "", " # the web tier" }) + foreach (var blankInBlock in new[] { true, false }) + foreach (var indicator in new[] { "|", "|-" }) + foreach (var quote in new[] { "", "\"", "'" }) + { + var name = string.Join(",", + newLine == "\n" ? "LF" : "CRLF", + $"trailingNl={trailing}", + $"leadComment={leadingComment}", + $"inlineComment={inlineComment != ""}", + $"blankInBlock={blankInBlock}", + $"block={indicator}", + $"quote={(quote == "" ? "none" : quote)}"); + + yield return new Shape(name, newLine, trailing, leadingComment, inlineComment, blankInBlock, indicator, quote); + } + } + } +} diff --git a/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs b/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs index 1037b720d0..5e31a1a74d 100644 --- a/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs +++ b/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs @@ -3,6 +3,7 @@ using Calamari.ArgoCD; using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Models; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.Logging; using Calamari.Testing.Helpers; using FluentAssertions; @@ -21,6 +22,75 @@ public class InlineJsonPatchReplacerTests ILog log = new InMemoryLog(); + [Test] + public void UpdateImages_WithAFoldedPatchThatHasNoRelevantImage_DoesNotFail() + { + var inMemoryLog = new InMemoryLog(); + const string inputYaml = "patches:\n" + + " - patch: >\n" + + " some: folded prose that mentions no image at all\n" + + " - target:\n" + + " kind: Deployment\n" + + " patch: |-\n" + + " apiVersion: apps/v1\n" + + " kind: Deployment\n" + + " spec:\n" + + " template:\n" + + " spec:\n" + + " containers:\n" + + " - name: nginx\n" + + " image: nginx:1.21\n"; + + var result = new InlineJsonPatchReplacer(inputYaml, ArgoCDConstants.DefaultContainerRegistry, inMemoryLog).UpdateImages(imagesToUpdate); + + result.UpdatedContents.Should().Be(inputYaml.Replace("nginx:1.21", "nginx:1.25")); + inMemoryLog.MessagesWarnFormatted.Should().BeEmpty(); + } + + [Test] + public void UpdateImages_WithAFoldedPatchThatDoesHoldTheImage_FailsWithAnActionableMessage() + { + const string inputYaml = "patches:\n" + + " - patch: >\n" + + " [{\"op\": \"replace\", \"path\": \"/spec/template/spec/containers/0/image\", \"value\": \"nginx:1.21\"}]\n"; + + var replacer = new InlineJsonPatchReplacer(inputYaml, ArgoCDConstants.DefaultContainerRegistry, log); + + var act = () => replacer.UpdateImages(imagesToUpdate); + + act.Should() + .Throw() + .WithMessage("*inline patch on line 2*folded block scalar (>)*literal block (|)*"); + } + + [Test] + public void UpdateImages_ChangesOnlyTheImageReference_PreservingCommentsAndLineEndings() + { + const string inputYaml = "# managed by the platform team\r\n" + + "apiVersion: kustomize.config.k8s.io/v1beta1\r\n" + + "kind: Kustomization\r\n" + + "\r\n" + + "patches:\r\n" + + " - target:\r\n" + + " kind: Deployment # only the web tier\r\n" + + " patch: |-\r\n" + + " apiVersion: apps/v1\r\n" + + " kind: Deployment\r\n" + + " spec:\r\n" + + " template:\r\n" + + " spec:\r\n" + + " containers:\r\n" + + " - name: nginx\r\n" + + " image: nginx:1.21\r\n"; + + var replacer = new InlineJsonPatchReplacer(inputYaml, ArgoCDConstants.DefaultContainerRegistry, log); + + var result = replacer.UpdateImages(imagesToUpdate); + + result.UpdatedImageReferences.Should().ContainSingle().Which.Should().Be("nginx:1.25"); + result.UpdatedContents.Should().Be(inputYaml.Replace("nginx:1.21", "nginx:1.25")); + } + [Test] public void UpdateImages_WithInlinePatchContainerImage_UpdatesImageReference() { diff --git a/source/Calamari.Tests/ArgoCD/InlineStrategicMergeTest.cs b/source/Calamari.Tests/ArgoCD/InlineStrategicMergeTest.cs index 106a25198b..8ff131add4 100644 --- a/source/Calamari.Tests/ArgoCD/InlineStrategicMergeTest.cs +++ b/source/Calamari.Tests/ArgoCD/InlineStrategicMergeTest.cs @@ -14,6 +14,37 @@ namespace Calamari.Tests.ArgoCD; public class InlineStrategicMergeTest { readonly ILog log = new InMemoryLog(); + [Test] + public void UpdateImages_ChangesOnlyTheImageReference_PreservingCommentsAndLineEndings() + { + const string content = "# strategic merge patches\r\n" + + "apiVersion: kustomize.config.k8s.io/v1beta1\r\n" + + "kind: Kustomization\r\n" + + "\r\n" + + "patchesStrategicMerge:\r\n" + + " - |\r\n" + + " apiVersion: apps/v1\r\n" + + " kind: Deployment\r\n" + + " spec:\r\n" + + " template:\r\n" + + " spec:\r\n" + + " containers:\r\n" + + " - name: nginx # the web tier\r\n" + + " image: nginx:1.21\r\n"; + + var imagesToUpdate = new List + { + new(ContainerImageReference.FromReferenceString("nginx:1.25", "default-registry")) + }; + + var replacer = new InlineStrategicMergeImageReplacer(content, "default-registry", log); + + var result = replacer.UpdateImages(imagesToUpdate); + + result.UpdatedImageReferences.Should().ContainSingle().Which.Should().Be("nginx:1.25"); + result.UpdatedContents.Should().Be(content.Replace("nginx:1.21", "nginx:1.25")); + } + [Test] public void ProcessInlineStrategicMergePatches_WithInlinePatches_UpdatesImages() { diff --git a/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs b/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs index 1ace1e4aec..710202c27c 100644 --- a/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs +++ b/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs @@ -1,8 +1,10 @@ +using System; using System.Collections.Generic; using System.Linq; using Calamari.ArgoCD; using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Models; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.Logging; using Calamari.Testing.Helpers; using FluentAssertions; @@ -196,6 +198,87 @@ public void UpdateImages_WithInvalidYaml_ReturnsNoChange() result.UpdatedContents.Should().Be(invalidYaml); } + [Test] + public void UpdateImages_WithAnAliasedContainer_DoesNotCorruptTheDocument() + { + const string yamlContent = "- op: add\n" + + " path: /spec/template/spec/containers\n" + + " value:\n" + + " - &web\n" + + " name: a\n" + + " image: nginx:1.21\n" + + "- op: add\n" + + " path: /spec/template/spec/initContainers\n" + + " value:\n" + + " - *web\n"; + + var shorterTag = new List + { + new(ContainerImageReference.FromReferenceString("nginx:9", ArgoCDConstants.DefaultContainerRegistry)) + }; + + var replacer = new YamlJson6902PatchImageReplacer(yamlContent, ArgoCDConstants.DefaultContainerRegistry, log); + + var result = replacer.UpdateImages(shorterTag); + + result.UpdatedContents.Should().Be(yamlContent.Replace("nginx:1.21", "nginx:9")); + } + + [Test] + public void UpdateImages_WithAFoldedImageValue_FailsWithAMessageNamingTheImageAndTheFix() + { + const string yamlContent = "- op: replace\n path: /spec/template/spec/containers/0/image\n value: >\n nginx:1.21\n"; + + var replacer = new YamlJson6902PatchImageReplacer(yamlContent, ArgoCDConstants.DefaultContainerRegistry, log); + + var act = () => replacer.UpdateImages(imagesToUpdate); + + act.Should() + .Throw() + .WithMessage("*nginx:1.21*line 3*folded block scalar (>)*literal block (|)*"); + } + + [Test] + public void UpdateImages_WithAFoldedScalarElsewhereInTheFile_DoesNotWarn() + { + var inMemoryLog = new InMemoryLog(); + const string yamlContent = "- op: add\n" + + " path: /metadata/annotations/notes\n" + + " value: >\n" + + " some folded prose\n" + + " spanning lines\n" + + "- op: replace\n" + + " path: /spec/template/spec/containers/0/image\n" + + " value: nginx:1.21\n"; + + var replacer = new YamlJson6902PatchImageReplacer(yamlContent, ArgoCDConstants.DefaultContainerRegistry, inMemoryLog); + + var result = replacer.UpdateImages(imagesToUpdate); + + result.UpdatedContents.Should().Be(yamlContent.Replace("nginx:1.21", "nginx:1.25")); + inMemoryLog.MessagesWarnFormatted.Should().BeEmpty(); + } + + [Test] + public void UpdateImages_ChangesOnlyTheImageReference_PreservingCommentsAndLineEndings() + { + const string yamlContent = "# rollout patch\r\n" + + "- op: replace\r\n" + + " path: /spec/template/spec/containers/0/image # web\r\n" + + " value: nginx:1.21\r\n" + + "\r\n" + + "- op: replace\r\n" + + " path: /spec/template/spec/initContainers/0/image\r\n" + + " value: \"nginx:1.21\"\r\n"; + + var replacer = new YamlJson6902PatchImageReplacer(yamlContent, ArgoCDConstants.DefaultContainerRegistry, log); + + var result = replacer.UpdateImages(imagesToUpdate); + + result.UpdatedImageReferences.Should().ContainSingle().Which.Should().Be("nginx:1.25"); + result.UpdatedContents.Should().Be(yamlContent.Replace("nginx:1.21", "nginx:1.25")); + } + [Test] public void UpdateImages_WithComplexPatch_UpdatesCorrectly() { diff --git a/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs b/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs new file mode 100644 index 0000000000..1d0712f476 --- /dev/null +++ b/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs @@ -0,0 +1,232 @@ +using System.Linq; +using Calamari.ArgoCD; +using FluentAssertions; +using NUnit.Framework; +using YamlDotNet.RepresentationModel; + +namespace Calamari.Tests.ArgoCD +{ + [TestFixture] + public class YamlScalarSplicerTests + { + [Test] + public void ReplaceValue_OnPlainScalar_LeavesTheRestOfTheFileUntouched() + { + const string document = "# top comment\r\nimage: nginx:1.21 # pinned\r\n\r\nother: keep\r\n"; + + var result = Replace(document, "image", "nginx:1.25"); + + result.Should().Be("# top comment\r\nimage: nginx:1.25 # pinned\r\n\r\nother: keep\r\n"); + } + + [Test] + public void ReplaceValue_OnQuotedScalar_KeepsTheOriginalQuotes() + { + const string document = "image: \"nginx:1.21\"\nsingle: 'nginx:1.21'\n"; + + var result = Replace(document, "image", "nginx:1.25"); + + result.Should().Be("image: \"nginx:1.25\"\nsingle: 'nginx:1.21'\n"); + } + + [Test] + public void ReplaceValue_OnLiteralBlock_ReindentsAndKeepsSurroundingText() + { + const string document = "patch: |-\r\n kind: Deployment\r\n image: nginx:1.21\r\ntarget: x\r\n"; + + var result = Replace(document, "patch", "kind: Deployment\nimage: nginx:1.25"); + + result.Should().Be("patch: |-\r\n kind: Deployment\r\n image: nginx:1.25\r\ntarget: x\r\n"); + } + + [Test] + public void ReplaceValue_OnLiteralBlockWithBlankLine_DoesNotIndentTheBlankLine() + { + const string document = "patch: |\n one\n\n two\nafter: x\n"; + + var result = Replace(document, "patch", "one\n\ntwo\n"); + + result.Should().Be("patch: |\n one\n\n two\nafter: x\n"); + } + + [Test] + public void ReplaceValue_OnLiteralBlockAtEndOfFileWithoutTrailingNewline_DoesNotAddOne() + { + const string document = "patch: |-\n image: nginx:1.21"; + + var result = Replace(document, "patch", "image: nginx:1.25"); + + result.Should().Be("patch: |-\n image: nginx:1.25"); + } + + [Test] + public void ReplaceValues_WithSeveralEdits_AppliesThemAll() + { + const string document = "a: nginx:1.21\r\nb: nginx:1.21\r\nc: nginx:1.21\r\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var edits = new[] { "a", "b", "c" } + .Select(key => new YamlScalarEdit((YamlScalarNode)root.Children[new YamlScalarNode(key)], "nginx:1.25")); + + var result = YamlScalarSplicer.ReplaceValues(document, edits); + + result.Should().Be("a: nginx:1.25\r\nb: nginx:1.25\r\nc: nginx:1.25\r\n"); + } + + [Test] + public void ReplaceValues_WithTheSameNodeEditedTwice_AppliesItOnce() + { + // An alias makes YamlDotNet hand back the same node object, so callers can collect the + // same edit twice. Splicing it twice would corrupt the document. + const string document = "a: &x nginx:1.21\nb: *x\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var shared = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + var edits = new[] { new YamlScalarEdit(shared, "nginx:9"), new YamlScalarEdit(shared, "nginx:9") }; + + var result = YamlScalarSplicer.ReplaceValues(document, edits); + + result.Should().Be("a: &x nginx:9\nb: *x\n"); + } + + [Test] + public void CanReplaceValue_IsFalseForStylesThatCannotBeSpliced() + { + const string document = "a: >\n folded\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + + YamlScalarSplicer.CanReplaceValue(document, node).Should().BeFalse(); + } + + [Test] + public void CanReplaceValue_IsTrueForPlainQuotedAndLiteralScalars() + { + const string document = "a: plain\nb: \"quoted\"\nc: |\n block\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + + foreach (var key in new[] { "a", "b", "c" }) + { + var node = (YamlScalarNode)root.Children[new YamlScalarNode(key)]; + YamlScalarSplicer.CanReplaceValue(document, node).Should().BeTrue($"{key} should be spliceable"); + } + } + + [Test] + public void ReplaceValue_OnBlockWithAnIndentIndicatorAndSurplusIndentation_KeepsTheSurplus() + { + // |2 declares two spaces of structure, so the remaining four on each line are part of the + // value and must survive the replacement rather than being indented a second time. + const string document = "a: |2\n one\n two\nb: keep\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + + node.Value.Should().Be(" one\n two\n"); + YamlScalarSplicer.CanReplaceValue(document, node).Should().BeTrue(); + + var result = YamlScalarSplicer.ReplaceValue(document, node, " one\n three\n"); + + result.Should().Be("a: |2\n one\n three\nb: keep\n"); + } + + [Test] + public void CanReplaceValue_IsTrueForBlockIndicatorsWhoseIndentationWeCanAccountFor() + { + foreach (var indicator in new[] { "|", "|-", "|+", "|2" }) + { + const string indent = " "; + var document = $"a: {indicator}\n{indent}one\n{indent}two\nb: keep\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + + YamlScalarSplicer.CanReplaceValue(document, node) + .Should() + .BeTrue($"{indicator} with matching indentation should be spliceable"); + } + } + + [Test] + public void ReplaceValue_ReplacingABlockValueWithItself_IsAByteForByteNoOp() + { + foreach (var indicator in new[] { "|", "|-", "|+" }) + foreach (var newLine in new[] { "\n", "\r\n" }) + { + var document = $"a: {indicator}{newLine} one{newLine}{newLine} two{newLine}b: keep{newLine}"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + + if (!YamlScalarSplicer.CanReplaceValue(document, node)) + continue; + + YamlScalarSplicer.ReplaceValue(document, node, node.Value) + .Should() + .Be(document, $"{indicator} should round-trip unchanged"); + } + } + + [Test] + public void ReplaceValue_OnBlockWithLineEndingsDifferingFromTheFile_HarmonisesOnlyThatBlock() + { + // A CRLF file whose block content uses LF. The block is rewritten with the file's CRLF; + // everything outside it stays byte-identical. + const string document = "before: x\r\npatch: |-\r\n kind: Deployment\n image: nginx:1.21\r\nafter: y\r\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("patch")]; + + YamlScalarSplicer.CanReplaceValue(document, node).Should().BeTrue(); + + var result = YamlScalarSplicer.ReplaceValue(document, node, "kind: Deployment\nimage: nginx:1.25"); + + result.Should().Be("before: x\r\npatch: |-\r\n kind: Deployment\r\n image: nginx:1.25\r\nafter: y\r\n"); + } + + [Test] + public void ReplaceValue_OnBlockWithAnIndentIndicatorAndMixedLineEndings_StillKeepsTheSurplus() + { + const string document = "a: |2\r\n one\n two\r\nb: keep\r\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + + YamlScalarSplicer.CanReplaceValue(document, node).Should().BeTrue(); + + var result = YamlScalarSplicer.ReplaceValue(document, node, node.Value); + + result.Should().Be("a: |2\r\n one\r\n two\r\nb: keep\r\n"); + } + + [Test] + public void ReplaceValue_ReplacingAnIndentIndicatorBlockWithItself_IsAByteForByteNoOp() + { + foreach (var surplus in new[] { "", " ", " " }) + { + var document = $"a: |2\n {surplus}one\n {surplus}two\nb: keep\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + + YamlScalarSplicer.ReplaceValue(document, node, node.Value) + .Should() + .Be(document, $"surplus of {surplus.Length} spaces should round-trip"); + } + } + + static string Replace(string document, string key, string newValue) + { + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode(key)]; + return YamlScalarSplicer.ReplaceValue(document, node, newValue); + } + + static YamlStream Load(string document) + { + return YamlStreamLoader.TryLoadSilent(document)!; + } + } +} diff --git a/source/Calamari.Tests/Fixtures/Util/StringExtensionsFixture.cs b/source/Calamari.Tests/Fixtures/Util/StringExtensionsFixture.cs index ef72f9a9c1..9c48a6b7a4 100644 --- a/source/Calamari.Tests/Fixtures/Util/StringExtensionsFixture.cs +++ b/source/Calamari.Tests/Fixtures/Util/StringExtensionsFixture.cs @@ -44,6 +44,20 @@ public void AsRelativePathFrom(string source, string baseDirectory, string expec Assert.AreEqual(expected, source.AsRelativePathFrom(baseDirectory)); } + [TestCase("a\n", true)] + [TestCase("a\r\n", true)] + [TestCase("a\r", true)] + [TestCase("a\n\n", true)] + [TestCase("a", false)] + [TestCase("a ", false)] + [TestCase("", false)] + [TestCase(null, false)] + [Test] + public void HasTrailingNewLine_DetectsATrailingLineBreak(string input, bool expected) + { + input.HasTrailingNewLine().Should().Be(expected); + } + [TestCase("to_camel_case_function", "toCamelCaseFunction")] [TestCase("My S3 Bucket", "myS3Bucket")] [TestCase("-only-$AlphaNUMERIC-characters%^", "onlyAlphanumericCharacters")] diff --git a/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs b/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs index 484e7c72ed..0f7aa19b9c 100644 --- a/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs +++ b/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs @@ -1,7 +1,7 @@ using System.Collections.Generic; -using System.IO; using System.Linq; using Calamari.ArgoCD.Models; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.Logging; using YamlDotNet.Core; using YamlDotNet.RepresentationModel; @@ -35,8 +35,11 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection(); + var edits = new List(); foreach (var patchNode in patchSequence.Children) { + // A literal block is inline patch content; a plain entry is a path to a patch file, which + // is not ours to rewrite. if (patchNode is YamlScalarNode patchScalar && patchScalar.Style == ScalarStyle.Literal) { var patchContent = patchScalar.Value ?? ""; @@ -45,7 +48,10 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection 0) { - patchScalar.Value = result.UpdatedContents; + if (!YamlScalarSplicer.CanReplaceValue(input, patchScalar)) + throw new CommandException($"Cannot update images in the strategic merge patch on line {patchScalar.Start.Line}: {YamlScalarSplicer.DescribeUnsupportedValue(patchScalar)}."); + + edits.Add(new YamlScalarEdit(patchScalar, result.UpdatedContents)); allUpdatedImages.UnionWith(result.UpdatedImageReferences); } } @@ -56,9 +62,7 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection(), new HashSet()); } - using var writer = new StringWriter(); - yamlStream.Save(writer, false); - var modifiedContent = writer.ToString().TrimEnd(); + var modifiedContent = YamlScalarSplicer.ReplaceValues(input, edits); return new ImageReplacementResult(modifiedContent, allUpdatedImages, new HashSet()); } diff --git a/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs b/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs index 8c1f2cd014..833fae3291 100644 --- a/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs +++ b/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs @@ -3,8 +3,7 @@ using System.Collections.Generic; using System.IO; using System.Linq; -using System.Text; -using YamlDotNet.Core; +using Calamari.Common.Commands; using YamlDotNet.RepresentationModel; using YamlDotNet.Serialization; @@ -22,12 +21,10 @@ public HelmYamlParser(string yamlContent) var reader = new StringReader(yamlString); yamlStream = new YamlStream(); yamlStream.Load(reader); - endsWithNewline = yamlString.EndsWith(Environment.NewLine); } readonly string yamlString; readonly YamlStream yamlStream; - readonly bool endsWithNewline; public string GetValueAtPath(string path) { @@ -75,56 +72,13 @@ public List CreateDotPathsForNodes() public string UpdateContentForPath(string path, string newValue) { var nodeAtPath = GetNodeAtPath(path); - if (nodeAtPath != null) - { - return ReplaceNodeContent(nodeAtPath, newValue); - } - - return yamlString; - } - - string ReplaceNodeContent(YamlScalarNode node, string newValue) - { - var result = new StringBuilder(); - using var reader = new StringReader(yamlString); - - var targetLine = (int)node.Start.Line; - int startColumn; - int endColumn; - switch (node.Style) - { - case ScalarStyle.Literal: - case ScalarStyle.Plain: - startColumn = (int)node.Start.Column - 1; - endColumn = (int)node.End.Column - 1; - break; - case ScalarStyle.DoubleQuoted: - case ScalarStyle.SingleQuoted: - startColumn = (int)node.Start.Column; - endColumn = (int)node.End.Column - 2; - break; - default: - throw new NotSupportedException("Modifying Folded or Ambiguous Scar Values is not supported."); - } - int currentLine = 1; + if (nodeAtPath == null) + return yamlString; - while (reader.ReadLine() is { } line) - { - if (currentLine == targetLine) - { - // Replace in this line - var before = line[..startColumn]; - var after = line[endColumn..]; - result.AppendLine(before + newValue + after); - } - else - { - result.AppendLine(line); - } - currentLine++; - } + if (!YamlScalarSplicer.CanReplaceValue(yamlString, nodeAtPath)) + throw new CommandException($"Cannot update the value at '{path}' on line {nodeAtPath.Start.Line}: {YamlScalarSplicer.DescribeUnsupportedValue(nodeAtPath)}."); - return endsWithNewline ? result.ToString() : result.ToString().TrimEnd(); + return YamlScalarSplicer.ReplaceValue(yamlString, nodeAtPath, newValue); } static void FlattenObject(object? obj, string currentPath, List paths) diff --git a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs index e7a0ec585c..2b5e20478b 100644 --- a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs +++ b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs @@ -6,6 +6,7 @@ using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Conventions.UpdateImageTag; using Calamari.ArgoCD.Models; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.Extensions; using Calamari.Common.Plumbing.Logging; using Calamari.Kubernetes; @@ -80,9 +81,10 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection(); + var edits = new List(); foreach (var patchNode in patchesSequence.OfType()) { - var changes = ProcessPatchNode(patchNode, imagesToUpdate); + var changes = ProcessPatchNode(patchNode, imagesToUpdate, edits); replacementsMade.UnionWith(changes); } @@ -91,13 +93,11 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection()); } - HashSet ProcessPatchNode(YamlMappingNode patchNode, IReadOnlyCollection imagesToUpdate) + HashSet ProcessPatchNode(YamlMappingNode patchNode, IReadOnlyCollection imagesToUpdate, List edits) { var changes = new HashSet(); @@ -108,7 +108,7 @@ HashSet ProcessPatchNode(YamlMappingNode patchNode, IReadOnlyCollection< { if (kvp.Key is YamlScalarNode scalar && scalar.Value == FieldNames.Patch && kvp.Value is YamlScalarNode patchContentScalar) { - var patchChanges = ProcessInlinePatchContent(patchContentScalar, imagesToUpdate); + var patchChanges = ProcessInlinePatchContent(patchContentScalar, imagesToUpdate, edits); changes.UnionWith(patchChanges); break; } @@ -118,7 +118,7 @@ HashSet ProcessPatchNode(YamlMappingNode patchNode, IReadOnlyCollection< return changes; } - HashSet ProcessInlinePatchContent(YamlScalarNode patchContentNode, IReadOnlyCollection imagesToUpdate) + HashSet ProcessInlinePatchContent(YamlScalarNode patchContentNode, IReadOnlyCollection imagesToUpdate, List edits) { var changes = new HashSet(); @@ -139,12 +139,20 @@ HashSet ProcessInlinePatchContent(YamlScalarNode patchContentNode, IRead } var result = patchImageReplacer.UpdateImages(imagesToUpdate); - changes.UnionWith(result.UpdatedImageReferences); + if (result.UpdatedImageReferences.Count == 0) + return changes; - if (result.UpdatedImageReferences.Count > 0) - { - patchContentNode.Value = result.UpdatedContents; - } + // Checked only once there is something to write, so a patch we cannot write back but + // have no reason to touch stays silent. + if (!YamlScalarSplicer.CanReplaceValue(yamlContent, patchContentNode)) + throw new CommandException($"Cannot update images in the inline patch on line {patchContentNode.Start.Line}: {YamlScalarSplicer.DescribeUnsupportedValue(patchContentNode)}."); + + changes.UnionWith(result.UpdatedImageReferences); + edits.Add(new YamlScalarEdit(patchContentNode, result.UpdatedContents)); + } + catch (CommandException) + { + throw; } catch (Exception ex) { diff --git a/source/Calamari/ArgoCD/JsonPatchImageReplacer.cs b/source/Calamari/ArgoCD/JsonPatchImageReplacer.cs index b9365eb3ab..2acc7740d9 100644 --- a/source/Calamari/ArgoCD/JsonPatchImageReplacer.cs +++ b/source/Calamari/ArgoCD/JsonPatchImageReplacer.cs @@ -6,6 +6,7 @@ using System.Text.Json.Nodes; using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Models; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.Logging; namespace Calamari.ArgoCD @@ -101,6 +102,12 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection()); } + catch (CommandException) + { + // A failure we raised deliberately, with an actionable message. Degrading it to a + // warning here would report success while leaving the image at its old tag. + throw; + } catch (Exception ex) { log.WarnFormat("Error processing JSON patch file: {0}", ex.Message); diff --git a/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs b/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs index 0796ea902c..9ecba1fb25 100644 --- a/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs +++ b/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs @@ -1,12 +1,11 @@ #nullable enable using System; using System.Collections.Generic; -using System.IO; using System.Linq; using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Models; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.Logging; -using YamlDotNet.Core; using YamlDotNet.RepresentationModel; namespace Calamari.ArgoCD @@ -51,6 +50,7 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection(); + var edits = new List(); // Process each document in the YAML stream foreach (var document in stream.Documents) @@ -60,7 +60,7 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection()) { - var operationResult = ProcessPatchOperation(operationNode, imagesToUpdate); + var operationResult = ProcessPatchOperation(operationNode, imagesToUpdate, edits); results.Add(operationResult); } } @@ -72,21 +72,14 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection 0) - { - var singleDocStream = new YamlStream(stream.Documents[0]); - singleDocStream.Save(writer, false); - } - var modifiedContent = writer.ToString().TrimEnd(); + var modifiedContent = YamlScalarSplicer.ReplaceValues(yamlContent, edits); return new ImageReplacementResult(modifiedContent, combinedResult.UpdatedImageReferences, combinedResult.AlreadyUpToDateImages); } ImageReplacementResult ProcessPatchOperation(YamlMappingNode operationNode, - IReadOnlyCollection imagesToUpdate) + IReadOnlyCollection imagesToUpdate, + List edits) { var opValue = operationNode.GetStringValue(FieldNames.Op); var pathValue = operationNode.GetStringValue(FieldNames.Path); @@ -98,20 +91,21 @@ ImageReplacementResult ProcessPatchOperation(YamlMappingNode operationNode, return opValue switch { - OpValues.Replace => ProcessReplaceOperation(operationNode, pathValue, imagesToUpdate), - OpValues.Add => ProcessAddOperation(operationNode, pathValue, imagesToUpdate), + OpValues.Replace => ProcessReplaceOperation(operationNode, pathValue, imagesToUpdate, edits), + OpValues.Add => ProcessAddOperation(operationNode, pathValue, imagesToUpdate, edits), _ => NoChangeResult }; } ImageReplacementResult ProcessReplaceOperation(YamlMappingNode operationNode, string path, - IReadOnlyCollection imagesToUpdate) + IReadOnlyCollection imagesToUpdate, + List edits) { if (IsImagePath(path) && operationNode.Children.TryGetValue(new YamlScalarNode(FieldNames.Value), out var valueNode)) { if (valueNode is YamlScalarNode imageScalar && !string.IsNullOrEmpty(imageScalar.Value)) { - return ProcessImageReference(imageScalar, imagesToUpdate); + return ProcessImageReference(imageScalar, imagesToUpdate, edits); } } @@ -119,17 +113,18 @@ ImageReplacementResult ProcessReplaceOperation(YamlMappingNode operationNode, st } ImageReplacementResult ProcessAddOperation(YamlMappingNode operationNode, string path, - IReadOnlyCollection imagesToUpdate) + IReadOnlyCollection imagesToUpdate, + List edits) { if (IsContainersPath(path) && operationNode.Children.TryGetValue(new YamlScalarNode(FieldNames.Value), out var valueNode)) { if (valueNode is YamlSequenceNode containersSequence) { - return ProcessContainersSequence(containersSequence, imagesToUpdate); + return ProcessContainersSequence(containersSequence, imagesToUpdate, edits); } else if (valueNode is YamlMappingNode singleContainer) { - return ProcessContainerMapping(singleContainer, imagesToUpdate); + return ProcessContainerMapping(singleContainer, imagesToUpdate, edits); } } @@ -137,13 +132,14 @@ ImageReplacementResult ProcessAddOperation(YamlMappingNode operationNode, string } ImageReplacementResult ProcessContainersSequence(YamlSequenceNode containersSequence, - IReadOnlyCollection imagesToUpdate) + IReadOnlyCollection imagesToUpdate, + List edits) { var results = new List(); foreach (var containerNode in containersSequence.Children.OfType()) { - var result = ProcessContainerMapping(containerNode, imagesToUpdate); + var result = ProcessContainerMapping(containerNode, imagesToUpdate, edits); results.Add(result); } @@ -151,19 +147,21 @@ ImageReplacementResult ProcessContainersSequence(YamlSequenceNode containersSequ } ImageReplacementResult ProcessContainerMapping(YamlMappingNode containerNode, - IReadOnlyCollection imagesToUpdate) + IReadOnlyCollection imagesToUpdate, + List edits) { if (containerNode.Children.TryGetValue(new YamlScalarNode(FieldNames.Image), out var imageNode) && imageNode is YamlScalarNode imageScalar) { - return ProcessImageReference(imageScalar, imagesToUpdate); + return ProcessImageReference(imageScalar, imagesToUpdate, edits); } return NoChangeResult; } ImageReplacementResult ProcessImageReference(YamlScalarNode imageScalar, - IReadOnlyCollection imagesToUpdate) + IReadOnlyCollection imagesToUpdate, + List edits) { if (string.IsNullOrEmpty(imageScalar.Value)) return NoChangeResult; @@ -175,13 +173,13 @@ ImageReplacementResult ProcessImageReference(YamlScalarNode imageScalar, if (matchedUpdate != null && !matchedUpdate.Comparison.TagMatch) { - var newImageRef = currentImageRef.WithTag(matchedUpdate.Reference.Tag); - imageScalar.Value = newImageRef.FriendlyName(); + if (!YamlScalarSplicer.CanReplaceValue(yamlContent, imageScalar)) + // Trimmed because a folded value carries the line break that made it unwritable, + // which would otherwise break the message across lines. + throw new CommandException($"Cannot update the image reference '{imageScalar.Value?.Trim()}' on line {imageScalar.Start.Line}: {YamlScalarSplicer.DescribeUnsupportedValue(imageScalar)}."); - if (imageScalar.Style != ScalarStyle.SingleQuoted && imageScalar.Style != ScalarStyle.DoubleQuoted) - { - imageScalar.Style = ScalarStyle.DoubleQuoted; - } + var newImageRef = currentImageRef.WithTag(matchedUpdate.Reference.Tag); + edits.Add(new YamlScalarEdit(imageScalar, newImageRef.FriendlyName())); log.Verbose($"Updated container image in YAML JSON 6902 patch: {newImageRef.FriendlyName()}"); diff --git a/source/Calamari/ArgoCD/YamlScalarSplicer.cs b/source/Calamari/ArgoCD/YamlScalarSplicer.cs new file mode 100644 index 0000000000..51f3fe1aa3 --- /dev/null +++ b/source/Calamari/ArgoCD/YamlScalarSplicer.cs @@ -0,0 +1,277 @@ +#nullable enable +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using Calamari.Common.Plumbing.Extensions; +using YamlDotNet.Core; +using YamlDotNet.RepresentationModel; + +namespace Calamari.ArgoCD +{ + public record YamlScalarEdit(YamlScalarNode Node, string NewValue); + + /// + /// Replaces scalar values by splicing into the original YAML text. Re-emitting a parsed document + /// drops comments and blank lines and reflows indentation, quoting and folded scalars, which turns + /// a one-line image tag change into an unreviewable whole-file diff. Splicing leaves every byte + /// outside the replaced value exactly as it was, line endings included. + /// + public static class YamlScalarSplicer + { + /// + /// Why a value cannot be replaced, phrased for the person whose file it is. Callers add their + /// own context (which image, which file) and raise it as a CommandException. + /// + public static string DescribeUnsupportedValue(YamlScalarNode node) + { + return node.Style == ScalarStyle.Folded + ? "it is written as a folded block scalar (>), which does not record where the original line breaks were. Updating it would collapse the block onto one line and reformat the file. Use a literal block (|) or a plain value instead" + : $"the layout of this {node.Style} scalar could not be interpreted, so updating it could corrupt the file"; + } + + public static string ReplaceValue(string document, YamlScalarNode node, string newValue) + { + return ReplaceValues(document, new[] { new YamlScalarEdit(node, newValue) }); + } + + /// + /// True when this scalar's value can be replaced by splicing. Callers MUST check before + /// reporting an image as updated: an unsupported style, or a position we cannot verify, + /// leaves the file untouched rather than corrupting it or failing the step. + /// + public static bool CanReplaceValue(string document, YamlScalarNode node) + { + return node.Style == ScalarStyle.Literal + ? TryGetBlockRegion(document, node) != null + : TryGetInlineValueRegion(document, node, out _, out _); + } + + /// + /// Locates a block scalar's indented content and works out how much of the leading whitespace + /// is structure. Rendering the parser's own value back must then reproduce the original + /// content, so a block we cannot describe is refused rather than corrupted. Line endings are + /// excluded from that comparison: a block whose own endings differ from the rest of the file is + /// harmonised to the document's ending, which still catches indentation differences. + /// + static BlockRegion? TryGetBlockRegion(string document, YamlScalarNode node) + { + if (node.End.Line <= node.Start.Line) + return null; + + var start = OffsetOfLine(document, (int)node.Start.Line + 1); + var end = OffsetOfLine(document, (int)node.End.Line) + (int)node.End.Column - 1; + + if (end < start || end > document.Length) + return null; + + var region = document[start..end]; + var value = node.Value ?? ""; + + var indent = StructuralIndent(region, value); + if (indent == null) + return null; + + var block = new BlockRegion(start, end, indent, region.HasTrailingNewLine()); + + return Render(value, block, document).ReplaceLineEndings("\n") == region.ReplaceLineEndings("\n") + ? block + : null; + } + + record BlockRegion(int Start, int End, string Indent, bool EndsWithBreak); + + /// + /// Locates the value inside an inline scalar and confirms the located text is exactly what the + /// parser reported as the value. Start can point at an anchor or tag rather than the value + /// itself (a: &x nginx:1.21), and quoted values can carry escapes, so the region is only + /// safe to replace once it has been checked against the value. + /// + static bool TryGetInlineValueRegion(string document, YamlScalarNode node, out int start, out int end) + { + start = 0; + end = 0; + + if (node.Start.Line != node.End.Line) + return false; + + var lineStart = OffsetOfLine(document, (int)node.Start.Line); + var scalarStart = lineStart + (int)node.Start.Column - 1; + + switch (node.Style) + { + case ScalarStyle.Plain: + start = SkipNodeProperties(document, scalarStart); + end = lineStart + (int)node.End.Column - 1; + break; + case ScalarStyle.SingleQuoted: + case ScalarStyle.DoubleQuoted: + start = SkipNodeProperties(document, scalarStart) + 1; + end = lineStart + (int)node.End.Column - 2; + break; + default: + return false; + } + + return start >= 0 + && end >= start + && end <= document.Length + && document[start..end] == node.Value; + } + + /// + /// Skips any anchor (&name) and tag (!tag) properties preceding the value. + /// + static int SkipNodeProperties(string document, int index) + { + while (index < document.Length && (document[index] == '&' || document[index] == '!')) + { + while (index < document.Length && !char.IsWhiteSpace(document[index])) + index++; + while (index < document.Length && (document[index] == ' ' || document[index] == '\t')) + index++; + } + + return index; + } + + public static string ReplaceValues(string document, IEnumerable edits) + { + // An alias resolves to the same node object, so the same edit can be collected more than + // once. Splicing it twice would apply the second edit at offsets the first has invalidated, + // so keep one edit per node — by reference, since YamlScalarNode compares by value. + var distinct = edits.Distinct(new NodeIdentityComparer()); + + // Applying last-to-first keeps the offsets of the remaining, earlier edits valid. + var ordered = distinct.OrderByDescending(e => e.Node.Start.Line) + .ThenByDescending(e => e.Node.Start.Column); + + return ordered.Aggregate(document, Splice); + } + + class NodeIdentityComparer : IEqualityComparer + { + public bool Equals(YamlScalarEdit? x, YamlScalarEdit? y) => ReferenceEquals(x?.Node, y?.Node); + + public int GetHashCode(YamlScalarEdit edit) => System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(edit.Node); + } + + static string Splice(string document, YamlScalarEdit edit) + { + return edit.Node.Style == ScalarStyle.Literal + ? SpliceBlockScalar(document, edit.Node, edit.NewValue) + : SpliceInlineScalar(document, edit.Node, edit.NewValue); + } + + static string SpliceInlineScalar(string document, YamlScalarNode node, string newValue) + { + if (!TryGetInlineValueRegion(document, node, out var start, out var end)) + throw new NotSupportedException($"Cannot locate the value of this {node.Style} scalar to replace it. Check CanReplaceValue before creating an edit."); + + return document[..start] + newValue + document[end..]; + } + + /// + /// A block scalar starts at its indicator (| or |- …) and its content is the indented lines + /// that follow. End is column 1 of the line after the content, except when the block ends the + /// file without a trailing newline, where it is the end of the last content line instead. + /// + static string SpliceBlockScalar(string document, YamlScalarNode node, string newValue) + { + var block = TryGetBlockRegion(document, node) + ?? throw new NotSupportedException("Cannot account for this block scalar's indentation to replace it. Check CanReplaceValue before creating an edit."); + + return document[..block.Start] + Render(newValue, block, document) + document[block.End..]; + } + + /// + /// Renders a value as a block scalar's content: indented to match the block, using the + /// document's line ending, and keeping the original region's trailing break or lack of one. + /// + static string Render(string value, BlockRegion block, string document) + { + var rendered = Reindent(value, block.Indent, document.DetectLineEnding() ?? "\n"); + + return block.EndsWithBreak ? rendered : rendered.TrimEnd('\r', '\n'); + } + + static string Reindent(string value, string indent, string newLine) + { + var builder = new StringBuilder(); + foreach (var line in ContentLines(value)) + { + if (line.Length > 0) + builder.Append(indent); + builder.Append(line).Append(newLine); + } + + return builder.ToString(); + } + + /// + /// A clipped or kept block's value ends with a line break, which would otherwise yield a + /// trailing empty element that is not a content line of its own. + /// + static IEnumerable ContentLines(string value) + { + var lines = value.Split('\n'); + var count = lines.Length > 0 && lines[lines.Length - 1].Length == 0 + ? lines.Length - 1 + : lines.Length; + + return lines.Take(count).Select(line => line.TrimEnd('\r')); + } + + /// + /// The portion of a content line's leading whitespace that is structure rather than part of the + /// string. YAML normally takes it from the first non-empty line, but an explicit indicator + /// (|2) declares it, leaving any surplus as content. Deriving it from the difference between + /// the raw line and the parsed line covers both without having to interpret the indicator. + /// Returns null when the two do not correspond, so the caller refuses the block. + /// + static string? StructuralIndent(string region, string value) + { + var regionLines = ContentLines(region).ToList(); + var valueLines = ContentLines(value).ToList(); + + for (var index = 0; index < valueLines.Count; index++) + { + if (valueLines[index].Length == 0) + continue; + + if (index >= regionLines.Count) + return null; + + var indentLength = regionLines[index].Length - valueLines[index].Length; + if (indentLength < 0) + return null; + + var indent = regionLines[index][..indentLength]; + + return indent.All(character => character == ' ' || character == '\t') ? indent : null; + } + + return ""; + } + + /// + /// Index of the first character of the given 1-based line, counting YAML line breaks + /// (\r\n, \n and \r). + /// + static int OffsetOfLine(string document, int line) + { + var currentLine = 1; + var index = 0; + while (currentLine < line && index < document.Length) + { + var character = document[index++]; + if (character == '\r' && index < document.Length && document[index] == '\n') + index++; + if (character == '\r' || character == '\n') + currentLine++; + } + + return index; + } + } +} diff --git a/source/Calamari/ArgoCD/YamlStreamLoader.cs b/source/Calamari/ArgoCD/YamlStreamLoader.cs index 83965b535a..17b3835e1b 100644 --- a/source/Calamari/ArgoCD/YamlStreamLoader.cs +++ b/source/Calamari/ArgoCD/YamlStreamLoader.cs @@ -1,9 +1,6 @@ #nullable enable using System; -using System.Collections.Generic; using System.IO; -using System.Linq; -using Calamari.Common.Plumbing.Extensions; using Calamari.Common.Plumbing.Logging; using YamlDotNet.RepresentationModel; @@ -92,44 +89,5 @@ public static class YamlStreamLoader return stream.Documents[0].RootNode as YamlMappingNode; } - - /// - /// Serializes YAML documents back to string format, preserving line endings and handling document separators. - /// - /// The YAML documents to serialize - /// Original YAML content to detect line endings from (optional) - /// Serialized YAML string - public static string SerializeDocuments(IEnumerable documents, string? originalContent = null) - { - if (documents == null) - throw new ArgumentNullException(nameof(documents)); - - var documentList = documents.ToList(); - if (documentList.Count == 0) - return string.Empty; - - var newLine = originalContent?.DetectLineEnding() ?? "\n"; - var serializedDocs = new List(); - - foreach (var doc in documentList) - { - using var writer = new StringWriter(); - var tempStream = new YamlStream(doc); - tempStream.Save(writer, false); - var serialized = writer.ToString(); - - serialized = serialized.TrimEnd(); - if (serialized.EndsWith("...")) - { - serialized = serialized.Substring(0, serialized.Length - 3).TrimEnd(); - } - - serializedDocs.Add(serialized); - } - - return documentList.Count == 1 - ? serializedDocs[0] - : string.Join($"{newLine}---{newLine}", serializedDocs); - } } } \ No newline at end of file