base/*: fix uncompressed inline child resource with compressed parent #341
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cgwalters
approved these changes
Apr 22, 2022
|
Updated. |
When desugaring inline/local resource contents, the current rules are: 1. If the input config specifies compression: gzip, we pass through the compression field unchanged and refuse to compress again. This is useful for local resources that are stored precompressed alongside the config. 2. If we decide to compress, we set the compression field to gzip. 3. If we decide not to compress, we do not set the compression field, ostensibly so precompressed resources in child configs can inherit the compression field from the parent. But case 3 doesn't really work. Butane doesn't know that the child resource is precompressed, so is allowed to compress it again, and only the compressibility heuristics (which technically are not contractual) prevent it from doing so and producing invalid output. (There's no way to represent nested compression, so the file would be written to the provisioned system compressed.) And if the parent resource is not formally precompressed, but is actually just auto-compressed by Butane, a change to the parent's contents or to the Butane heuristics could cause the parent to become uncompressed in the future and break the merged config. And case 3 creates a problem for an inline/local child resource overriding an inline/local parent. If Butane opts to compress the parent but not the child, the merged config will have uncompressed contents but inherit "compression: gzip" from the parent, causing a provisioning failure. And if the parent/child contents change over time, this problem may suddenly appear in the future. Fix this by changing rule 3. When desugaring a non-precompressed inline/local resource, always set the compression field to either "" or "gzip". This intuitively makes sense: only we know whether we decided to compress the resource, so we need to say so. This involves an API change to MakeDataURL(). We now return a pointer to the selected compression value, or nil if we're passing through the compression value from the input (case 1). In the latter case, the caller should record a 1:1 translation for the compression field, rather than attributing its value to the inline/local input field. Fixes #332.
cgwalters
reviewed
Apr 29, 2022
jlebon
approved these changes
Apr 29, 2022
|
OK right, while this seems like a useful fix I don't think it will actually help the MCO today, see openshift/machine-config-operator#3126 (comment) |
We describe the field merging semantics in Ignition operator notes, so I don't think so? The issue here was that Butane wasn't taking those semantics into account. |
cgwalters
added a commit
to cgwalters/machine-config-operator
that referenced
this pull request
May 3, 2022
See coreos/butane#341 This obviates coreos/ignition#1329 which we are carrying currently.
|
Filed coreos/ignition#1357 for better config merging docs, including an explicit callout of overriding the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When desugaring inline/local resource contents, the current rules are:
compression: gzip, we pass through thecompressionfield unchanged and refuse to compress again. This is useful for local resources that are stored precompressed alongside the config.compressionfield togzip.compressionfield, ostensibly so precompressed resources in child configs can inherit thecompressionfield from the parent.But case 3 doesn't really work. Butane doesn't know that the child resource is precompressed, so is allowed to compress it again, and only the compressibility heuristics (which technically are not contractual) prevent it from doing so and producing invalid output. (There's no way to represent nested compression, so the file would be written to the provisioned system compressed.) And if the parent resource is not formally precompressed, but is actually just auto-compressed by Butane, a change to the parent's contents or to the Butane heuristics could cause the parent to become uncompressed in the future and break the merged config.
And case 3 creates a problem for an inline/local child resource overriding an inline/local parent. If Butane opts to compress the parent but not the child, the merged config will have uncompressed contents but inherit
compression: gzipfrom the parent, causing a provisioning failure. And if the parent/child contents change over time, this problem may suddenly appear in the future.Fix this by changing rule 3. When desugaring a non-precompressed inline/local resource, always set the
compressionfield to either "" orgzip. This intuitively makes sense: only we know whether we decided to compress the resource, so we need to say so.This involves an API change to
MakeDataURL(). We now return a pointer to the selectedcompressionvalue, ornilif we're passing through thecompressionvalue from the input (case 1). In the latter case, the caller should record a 1:1 translation for thecompressionfield, rather than attributing its value to theinline/localinput field.Fixes #332.