Automatically-compressed resources interact badly with config merging

[cgwalters]

I'm working on updating the MCO to use the latest butane instead of fcct in openshift/machine-config-operator#2976

There were a few minor bugs on the MCO side this unveiled, but this one looks like a clear Ignition bug. Basically in the MCO we heavily do config merging.

Butane now uses compression by default (which is fine; but I had to fix up various places in the MCO to correctly handle this). But, Ignition's native config merging seems to incorrectly carry the compression flag forward from old files when trying to override with an uncompressed file:

diff --git a/pkg/controller/common/helpers_test.go b/pkg/controller/common/helpers_test.go
index 8934b7d98..c667bbc33 100644
--- a/pkg/controller/common/helpers_test.go
+++ b/pkg/controller/common/helpers_test.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/clarketm/json"
 	ign2types "github.com/coreos/ignition/config/v2_2/types"
+	ign3 "github.com/coreos/ignition/v2/config/v3_2"
 	ign3types "github.com/coreos/ignition/v2/config/v3_2/types"
 	validate3 "github.com/coreos/ignition/v2/config/validate"
 	"github.com/stretchr/testify/assert"
@@ -135,6 +136,30 @@ func TestValidateIgnition(t *testing.T) {
 	require.NotNil(t, isValid3)
 }
 
+func TestIgnitionMergeCompressed(t *testing.T) {
+	testIgn3Config := ign3types.Config{}
+	testIgn3Config.Ignition.Version = "3.2.0"
+	mode := 420
+	testfiledata := "data:;base64,H4sIAAAAAAAAA0vLz+cCAKhlMn4EAAAA"
+	compression := "gzip"
+	tempFile := ign3types.File{Node: ign3types.Node{Path: "/etc/testfileconfig"},
+		FileEmbedded1: ign3types.FileEmbedded1{Contents: ign3types.Resource{Source: &testfiledata, Compression: &compression}, Mode: &mode}}
+	testIgn3Config.Storage.Files = append(testIgn3Config.Storage.Files, tempFile)
+
+	testIgn3Config2 := ign3types.Config{}
+	testIgn3Config2.Ignition.Version = "3.2.0"
+	testfiledata = "data:hello world"
+	tempFile = ign3types.File{Node: ign3types.Node{Path: "/etc/testfileconfig"},
+		FileEmbedded1: ign3types.FileEmbedded1{Contents: ign3types.Resource{Source: &testfiledata}, Mode: &mode}}
+	testIgn3Config2.Storage.Files = append(testIgn3Config2.Storage.Files, tempFile)
+
+	merged := ign3.Merge(testIgn3Config, testIgn3Config2)
+	assert.NotNil(t, merged)
+	mergedFile := merged.Storage.Files[0]
+	assert.Equal(t, "data:hello world", *mergedFile.Contents.Source)
+	assert.Equal(t, nil, mergedFile.Contents.Compression)
+}
+
 func TestConvertIgnition2to3(t *testing.T) {
 	// Make a new Ign spec v2 config
 	testIgn2Config := ign2types.Config{}

[bgilbert]

Config merging is defined to be field-by-field. If a child config wants to override a parent field, it needs to say so:

variant: fcos
version: 1.3.0
ignition:
  config:
    merge:
      - inline: |
          {
            "ignition": {
              "version": "3.1.0"
            },
            "storage": {
              "files": [
                {
                  "path": "/etc/test-file",
                  "contents": {
                    "source": "data:,foo",
                    "compression": ""
                  }
                }
              ]
            }
          }
storage:
  files:
    - path: /etc/test-file
      contents:
        inline: |
          Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
          eiusmod tempor incididunt ut labore et dolore magna aliqua.  Ut
          enim ad minim veniam, quis nostrud exercitation ullamco laboris
          nisi ut aliquip ex ea commodo consequat.  Duis aute irure dolor in
          reprehenderit in voluptate velit esse cillum dolore eu fugiat
          nulla pariatur.  Excepteur sint occaecat cupidatat non proident,
          sunt in culpa qui officia deserunt mollit anim id est laborum.

I hadn't recognized the interaction with automatic compression, which is indeed unfortunate. This behavior is bad enough when compression is applied to newly added inline or local resources, but also, Butane might switch an existing resource from uncompressed to compressed if its contents change to be more compressible. That could suddenly break previously-working child configs.

For uncompressed inline and local resources, I think it's pretty clear that Butane should emit "compression": "". It knows whether it has compressed the contents, so it needs to override any parent field saying otherwise. (As part of that change, we should also update the slop value that estimates the size of the compression field.)

source resources are a harder problem. It's possible for a child source to override a parent inline, but it's also possible that a child source intends to override a parent source and inherit the latter's compression. Since we don't know the contents of the parent config, I don't think we can second-guess the child's intentions. (In fact, we don't even know whether we're rendering a child or a standalone config.)

Or, we could solve the problem categorically by disabling automatic compression for any config that has child configs. That feels like a pretty big hammer though.

I'm open to ideas. The good news is that the failure case will typically produce an Ignition error at runtime while trying to decompress uncompressed data (unless the user intends to write gzip-compressed data), so users can at least identify the problem and fix their configs.

Moving this bug to Butane.

[cgwalters]

Config merging is defined to be field-by-field.

OK but...a few things going on here. First, in the MCO all the internals today are oriented around Ignition merging, not butane merging. I don't think we can really change that unless we had code that converted Ignition to butane. (here's a free project name for that).

but it's also possible that a child source intends to override a parent source and inherit the latter's compression.

I'm dubious. In what scenarios would the child source not be written by something that knows the compression type?

Or, we could solve the problem categorically by disabling automatic compression for any config that has child configs.

Wouldn't work for the MCO case, we are generating ignition configs from butane, then merging those configs with later with other configs via the Merge API in Go code.

I think the canonical fix here is to ensure that anywhere we're generating a file, also set the compression field (usually to "" because all this code predates Ignition compression).

Or alternatively, walk over the config just before merging and apply that.

[cgwalters]

Another alternative is Ignition could gain a merge2 behavior that is defined to handle compression in a not-surprising way. Maybe best done by waiting a bit, seeing if any other config merging surprises are discovered, and roll those in too/2.

[cgwalters]

For uncompressed inline and local resources, I think it's pretty clear that Butane should emit "compression": "". It knows whether it has compressed the contents, so it needs to override any parent field saying otherwise.

OK yes though, this would fix things for us I think.

[bgilbert]

but it's also possible that a child source intends to override a parent source and inherit the latter's compression.

I'm dubious. In what scenarios would the child source not be written by something that knows the compression type?

If I'm handwriting a child config that overrides a compressed file in the parent, and the overriding file is also compressed, I might take advantage of the merge semantics to avoid writing compression: gzip again.

Or, we could solve the problem categorically by disabling automatic compression for any config that has child configs.

Wouldn't work for the MCO case, we are generating ignition configs from butane, then merging those configs with later with other configs via the Merge API in Go code.

Ah, right.

I think the canonical fix here is to ensure that anywhere we're generating a file, also set the compression field (usually to "" because all this code predates Ignition compression).

Or alternatively, walk over the config just before merging and apply that.

If "we" is the MCO here, then yeah, that sounds right.

[mkenigs]

So sounds like for now the best fix is for Butane to emit "compression": ""?

[bgilbert]

Yup, for uncompressed inline and local resources.

[bgilbert]

Fix in #341.

[cgwalters]

OK I think to say it simply, everything generating Ignition should be setting the compression field. I think what would help here is an opinionated API for this like https://github.com/openshift/machine-config-operator/blob/a0fd73657a571bdbf75e19d06ab0371e2de5638b/test/helpers/helpers.go#L155 but part of Ignition or so?

[cgwalters]

openshift/machine-config-operator#3128

[bgilbert]

OK I think to say it simply, everything generating Ignition should be setting the compression field.

The requirement to clear the compression field only applies to child configs that are overriding possibly-compressed contents of a resource in a parent config. In general, child configs already need to know when they're doing that. (The parent config was writing e.g. a config file that implemented certain semantics, and clobbering that file will have semantic consequences that the child will need to account for, e.g. by copying some parts of the file from the parent config.) It should be sufficient to set the compression field only when overriding a parent resource in this way.

The MCO might plausibly choose to set compression: "" everywhere for convenience. If it doesn't know what it's overriding, though, I wonder if there are latent bugs where files from one Ignition fragment are being clobbered by a different Ignition fragment without anyone noticing.