config/types: allow compression on all file srcs #158
Conversation
e2730b5 to
6aef779
Compare
config/types/files.go
Outdated
| Remote Remote `yaml:"remote"` | ||
| Inline string `yaml:"inline"` | ||
| Local string `yaml:"local"` | ||
| Base64 *bool `yaml:"base64"` |
There was a problem hiding this comment.
I would rather we leave this up to the YAML parser to interpret this via type tagging. See http://yaml.org/type/binary.html for the binary tag.
There was a problem hiding this comment.
Hi @crawford,
Are you suggesting that I modify Inline to be a different type then? Or are you suggesting this as a CT example:
storage:
files:
- path: /etc/helloworld.txt
filesystem: root
mode: 0644
contents:
compression: gzip
inline: !!binary "\
H4sIAB/jFlsAA/NIzcnJ11Eozy/KSdHjAgDXu838DgAAAA=="There was a problem hiding this comment.
I'm suggesting the CLC example be changed to what you show:
storage:
files:
- path: /etc/helloworld.txt
filesystem: root
mode: 0644
contents:
compression: gzip
inline: !binary |-
H4sIAB/jFlsAA/NIzcnJ11Eozy/KSdHjAgDXu838DgAAAA==There was a problem hiding this comment.
Okay. I do not know to detect that tag in Go. I will look into it.
There was a problem hiding this comment.
The yaml parser should handle it automatically
There was a problem hiding this comment.
@crawford Hi Alex, Sorry to inject the conversation here, i tried the inline: !binary | method, with produced string
"source": "data:,U0NIRURVTEVSX09QVFM9Ii0tbGVhZGVyLWVsZWN0PXRydWUgXAotLWt1YmVjb25maWc9L3Zhci9saWIva3ViZS1zY2hlZHVsZXIva3ViZWNvbmZpZyBcCi0tdj0yIgo%3D%0A"
passed to VM, ignition does not able to decode it, it keep it as encoded in file, but if use data schema: https://tools.ietf.org/html/rfc2397 likedata:[<mediatype>][;base64],<data>, then the string been decoded properly in VM with ignition, looks like ignition does not do the job it suppose to do?
There was a problem hiding this comment.
@figo Please open an issue if you are having trouble.
There was a problem hiding this comment.
@crawford i am happy to do that if you could tell me where should the issue goes, it is ignition, could https://github.com/coreos/terraform-provider-ct/issues be a proper place? thanks
There was a problem hiding this comment.
config/types/files.go
Outdated
| Inline string `yaml:"inline"` | ||
| Local string `yaml:"local"` | ||
| Base64 *bool `yaml:"base64"` | ||
| Compression string `yaml:"compression"` |
There was a problem hiding this comment.
I'm not a fan of having both compression and remote.compression. Supporting compression for local/inline files is good, but it should be consistent.
There was a problem hiding this comment.
Hi @ajeddeloh,
That's fine. I just did this to maintain backwards compatibility.
There was a problem hiding this comment.
Hi @ajeddeloh,
FWIW, I believe if compression is added to local content, it should be removed as a field from remote. As I said, I did it the way I did it to maintain back-compat.
There was a problem hiding this comment.
Hi @ajeddeloh,
Thoughts on this? I don't mind moving the Compression field up a level in the struct hierarchy. I just didn't know if y'all were okay with breaking backwards compatibility. Obviously Compression shouldn't exist as a field in Remote if the former applies to local content as well.
There was a problem hiding this comment.
There is no guarantee of backward compatibility with CT, so we can (and I think we should) move the field. The reason we don't need to worry about backward compatibility is because it's expected that users pin to a specific version of CT for their config. As they upgrade CT over time, they'll need to potentially update their configs. This would be one of those cases.
There was a problem hiding this comment.
Uh, we're not 100% sure that's the case. We failed to decide how to version ct configs and definitely didn't communicate anything along the lines of "hey, this spec is subject to change". I'm not sure what the best path forward is, but I'd be inclined to deprecrate the old field while adding the new one and removing after a set amount of time instead of just removing it with no warning.
There was a problem hiding this comment.
CT has not hit 1.0 and since the spec isn't versioned, it's also subject to the same versioning rules. We can use this opportunity to explain to users how to properly use the tool. I think this will be a good first test since there are probably very few people making use of compression.
6aef779 to
3b83fa4
Compare
|
Can you also update the documentation to reflect this new syntax? |
|
Hi @crawford, I'm more than happy to update the docs if you will let me know which docs you'd like me to update. |
3b83fa4 to
5326376
Compare
|
Hi @crawford, Done and done. Thanks! |
|
One last request: Can you update the commit message and description to reflect that the only change is to allow compression on all file types (not, just remote). I'd leave the example in the body of the commit description (I love verbose history). Maybe something like |
5326376 to
4dd1082
Compare
|
Hi @crawford, Done and done. I also updated the failing test. I didn't run the tests locally (dumb, I know), I just compiled it. |
|
Hi @crawford, I also like verbose commit messages. Please see the guidelines I wrote regarding commit messages for one of my projects - http://rexray.readthedocs.io/en/latest/dev-guide/project-guidelines/#commit-messages. |
4dd1082 to
7d6ce4d
Compare
|
Hi @crawford, I just rebased, but one test is still failing. I'm not sure how to fix this as the error appears generated. $ make test
Checking gofix...
Checking gofmt...
Checking govet...
Running tests...
--- FAIL: TestParseAndConvert (0.00s)
Error Trace: config_test.go:2088
Error: Not equal: report.Report{Entries:[]report.Entry{report.Entry{Kind:0, Message:"path not absolute", Line:4, Column:13, Highlight:""}, report.Entry{Kind:0, Message:"invalid url scheme", Line:17, Column:16, Highlight:""}}} (expected)
!= report.Report{Entries:[]report.Entry{report.Entry{Kind:0, Message:"path not absolute", Line:4, Column:13, Highlight:""}, report.Entry{Kind:0, Message:"invalid url scheme", Line:18, Column:16, Highlight:""}}} (actual)
Diff:
--- Expected
+++ Actual
@@ -2,3 +2,3 @@
path not absolute
-error at line 17, column 16
+error at line 18, column 16
invalid url scheme
Messages: #4: bad report
FAIL
coverage: 72.7% of statements
FAIL github.com/coreos/container-linux-config-transpiler/config 0.018s
? github.com/coreos/container-linux-config-transpiler/config/astyaml [no test files]
? github.com/coreos/container-linux-config-transpiler/config/platform [no test files]
ok github.com/coreos/container-linux-config-transpiler/config/templating 0.013s coverage: 100.0% of statements
ok github.com/coreos/container-linux-config-transpiler/config/types 0.010s coverage: 6.5% of statements
ok github.com/coreos/container-linux-config-transpiler/config/types/util 0.011s coverage: 100.0% of statements
? github.com/coreos/container-linux-config-transpiler/internal [no test files]
? github.com/coreos/container-linux-config-transpiler/internal/util [no test files]
? github.com/coreos/container-linux-config-transpiler/internal/util/tools [no test files]
? github.com/coreos/container-linux-config-transpiler/internal/version [no test files]
make: *** [Makefile:30: test] Error 1 |
|
Hi @crawford, Nevermind, I figured it out. Now I'm seeing the following error: $ make test
Checking gofix...
Checking gofmt...
Checking govet...
Running tests...
ok github.com/coreos/container-linux-config-transpiler/config 0.017s coverage: 72.7% of statements
? github.com/coreos/container-linux-config-transpiler/config/astyaml [no test files]
? github.com/coreos/container-linux-config-transpiler/config/platform [no test files]
ok github.com/coreos/container-linux-config-transpiler/config/templating (cached) coverage: 100.0% of statements
ok github.com/coreos/container-linux-config-transpiler/config/types (cached) coverage: 6.5% of statements
ok github.com/coreos/container-linux-config-transpiler/config/types/util (cached) coverage: 100.0% of statements
? github.com/coreos/container-linux-config-transpiler/internal [no test files]
? github.com/coreos/container-linux-config-transpiler/internal/util [no test files]
? github.com/coreos/container-linux-config-transpiler/internal/util/tools [no test files]
? github.com/coreos/container-linux-config-transpiler/internal/version [no test files]
Checking documentation...
failed while validating docs: non-empty parsing report in examples.md: warning at line 8, column 11
Config has unrecognized key: compressionexit status 1
make: *** [Makefile:30: test] Error 1 |
This patch adds support for local, inline content as compressed, base64
encoded values. For example, the string "Hello, world." can be
compressed with gzip and base64 encoded with the following command:
$ echo Hello, world. | gzip | base64
H4sIAG/kFlsAA/NIzcnJ11Eozy/KSdHjAgDXu838DgAAAA==
A Container Linux Config could use the above contents and write them to
the file "/etc/helloworld.txt":
storage:
files:
- path: /etc/helloworld.txt
filesystem: root
mode: 0644
contents:
compression: gzip
inline: !binary |
H4sIAB/jFlsAA/NIzcnJ11Eozy/KSdHjAgDXu838DgAAAA==
The CT transpiler would transform the above configuration snippet into
the JSON below:
{
"filesystem": "root",
"path": "/etc/helloworld.txt",
"contents": {
"compression": "gzip",
"source": "data:,H4sIAB%2FjFlsAA%2FNIzcnJ11Eozy%2FKSdHjAgDXu838DgAAAA%3D%3D",
"verification": {}
},
"mode": 420
},
The following command may be used to verify the URL encoded data matches the
original "Hello, world." string:
$ echo H4sIAB%2FjFlsAA%2FNIzcnJ11Eozy%2FKSdHjAgDXu838DgAAAA%3D%3D | \
perl -pe 's/\%(\w\w)/chr hex $1/ge' | \
base64 -D | \
gzip -d
Hello, world.
7d6ce4d to
d1d0fb4
Compare
|
Hi @crawford, Figured this one out too. |
|
@bgilbert I'll let you make the final call on this one. The only point of contention is that this breaks backward compatibility. I'm arguing that we should do it anyway because CT (and its non-existent spec) is still pre-1.0 and because I don't think many people will be impacted (but have no number to back my claim). I think we should probably explain in our docs that users are expected to pick a version of CT and stick with it since there is no benefit to staying on the leading edge. |
|
Jumping a bit late into this discussion, I still find @ajeddeloh's comment reasonable. Perhaps this is a good point to introduce a |
|
@lucab Is the intention to yell at the user if the version doesn't exactly match or to accept multiple versions? |
|
Hi @crawford, May I suggest we merge it as backwards-compatible and decide with a follow-up PR about whether or not to deprecate the old field? That way the feature is added, and the decision regarding how to handle the old field is handled separately. |
CT is being vendored into things like Matchbox. Users presumably expect to upgrade Matchbox without breaking their configs, and they don't control what CT version is vendored in. We're going to have to add proper versioning sooner or later, and I'd rather not initiate that discussion by breaking compat. So let's do this in a backward-compatible way for now. |
It's Matchbox's responsibility to handle versioning though, since it's consuming ct as a library. If we were to add versioning to the ct spec, I would still assume Matchbox would be the one responsible for carrying multiple versions of the library and using the correct one. After all, Matchbox needs to be the one to decide just how many different config versions it's going to support.
Okay. |
ct is responsible for parsing CLCs, which makes ct responsible for versioning them. It's unreasonable to require every user of the ct library to vendor N versions of ct (bugs and all) and iterate over them in order to parse a config. We need to take responsibility for the full lifecycle of the CLC spec and not try to export that problem to someone else. |
|
The Ignition config package is also used as a library in some places (matchbox also imports this), and afaik there isn't an issue with the fact that Ignition will do the internal translation for library consumers. I'm in favor of versioning CLCs, I don't think that's complexity that we should be pushing up a layer. After the recent refactor of this in Ignition, I also think that the way translation is modelled over there would be a reasonable pattern to follow here. |
based on discussion at coreos/container-linux-config-transpiler#158 (comment) ignition should support content translated from: inline: !binary | ${base64encoded}
|
The question of whether and how to version CLCs has been an ongoing discussion topic for a while now. Sorry your PR got caught up in that. If you change the PR to add the new field and not remove the old one and add a validator adding a deprecation notice to the old field, this will LGTM. I've opened a new bug to track the CT versioning discussion seperately from this PR. |
This patch is related to discussion in coreos/bugs#1870 between myself, @crawford, and @dgonyeo. The patch adds support for local, inline content as compressed, base64 encoded values. For example, the string
Hello, world.can be compressed with gzip and base64 encoded with the following command:A Container Linux Config could use the above contents and write them to the file
/etc/helloworld.txt:The CT transpiler would transform the above configuration snippet into the JSON below:
{ "filesystem": "root", "path": "/etc/helloworld.txt", "contents": { "compression": "gzip", "source": "data:,H4sIAB%2FjFlsAA%2FNIzcnJ11Eozy%2FKSdHjAgDXu838DgAAAA%3D%3D", "verification": {} }, "mode": 420 },The following command may be used to verify the URL encoded data matches the original "Hello, world." string: