Repository navigation
Always validate YAML - #121
Conversation
ede39fa to
4fc2d0b
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the CloudInit admission validation so spec.contents is always parsed/validated (even when spec.filename is unchanged) and tightens validation to require a YAML document with a mapping at the root. It also introduces go.yaml.in/yaml/v4 (vendored) as the YAML parser to enable parsing into yaml.Node for structure validation.
Changes:
- Remove the update-time early return and always validate
spec.contents; adjust duplicate-filename checks to ignore the object being updated. - Add/extend tests to cover the update regression and YAML-structure validation behavior.
- Vendor
go.yaml.in/yaml/v4and updatego.mod/go.sum/vendor/modules.txtaccordingly.
Reviewed changes
Copilot reviewed 3 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/admitter/cloudinit.go | Always validates contents; ignores self in duplicate filename check; adds YAML structure validation via yaml.Node. |
| pkg/admitter/cloudinit_test.go | Adds regression coverage for update behavior and unit tests for the new YAML validation helper. |
| go.mod | Adds dependency on go.yaml.in/yaml/v4 v4.0.0-rc.4 and adjusts YAML deps. |
| go.sum | Adds checksums for go.yaml.in/yaml/v4. |
| vendor/modules.txt | Records vendored go.yaml.in/yaml/v4 module entries. |
| vendor/go.yaml.in/yaml/v4/yaml.go | Vendored YAML v4 API surface and classic APIs. |
| vendor/go.yaml.in/yaml/v4/loader.go | Vendored loader implementation used by the webhook (NewLoader, Load). |
| vendor/go.yaml.in/yaml/v4/dumper.go | Vendored dumper implementation (supporting the library’s API completeness). |
| vendor/go.yaml.in/yaml/v4/doc.go | Vendored package docs describing v4 options and behavior. |
| vendor/go.yaml.in/yaml/v4/README.md | Vendored upstream README for yaml v4. |
| vendor/go.yaml.in/yaml/v4/LICENSE | Vendored upstream license text. |
| vendor/go.yaml.in/yaml/v4/NOTICE | Vendored upstream notice file. |
| vendor/go.yaml.in/yaml/v4/CONTRIBUTING.md | Vendored upstream contributing guide. |
| vendor/go.yaml.in/yaml/v4/GNUmakefile | Vendored upstream build/test automation. |
| vendor/go.yaml.in/yaml/v4/.gitignore | Vendored upstream ignore rules. |
| vendor/go.yaml.in/yaml/v4/.typos.toml | Vendored upstream spelling/typos tool config. |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/api.go | Vendored libyaml API helpers. |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/composer.go | Vendored composer producing node trees for loaders. |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/doc.go | Vendored internal package docs. |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/errors.go | Vendored YAML error types used by loader/constructor paths. |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/node.go | Vendored node representation used for structure validation. |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/options.go | Vendored options system used by loader/dumper defaults. |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/reader.go | Vendored reader/encoding logic used by parser. |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/representer.go | Vendored representer for marshaling (library completeness). |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/resolver.go | Vendored scalar resolver (tag/type resolution). |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/serializer.go | Vendored serializer stage for emitting. |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/writer.go | Vendored writer buffering for emitter. |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/yaml.go | Vendored core types/constants for parser/emitter. |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/yamlprivate.go | Vendored internal constants/helpers. |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/yamldatatest_loader.go | Vendored test-data loader utility from upstream. |
| vendor/go.yaml.in/yaml/v4/internal/libyaml/README.md | Vendored internal libyaml documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4fc2d0b to
49830b6
Compare
9a90268 to
9cedb9b
Compare
tserong
left a comment
There was a problem hiding this comment.
LGTM. The only thing I wondered about was the bump to go.yaml.in/yaml/v4, but if we're moving that way anyway (harvester/harvester#10355) including it here makes sense. I did just notice though that pkg/utils/oem.go has a "gopkg.in/yaml.v1" - probably should bump that one too.
Additional improvements: - Switch to `go.yaml.in/yaml/v4` because `gopkg.in/yaml.v(1|3)` is not maintained anymore. Related to: harvester/harvester#9164 Signed-off-by: Volker Theile <vtheile@suse.com>
9cedb9b to
9595b71
Compare
Fixed |
Problem:
The content of the CR is not parsed when the filename has not been modified.
Solution:
spec.contents.go.yaml.in/yaml/v4becausegopkg.in/yaml.v3is not maintained anymore.Related Issue(s):
harvester/harvester#9164
Test plan:
Case 1
cloudinit-passwd.yamlwith the content:kubectl apply -f ./cloudinit-passwd.yaml.ls -alh /oemon any node of the cluster. The output should look like:cat /oem/99_passwd.yamlon any node of the cluster. The output should look like:Case 2
cloudinit-passwd.yamlcreated in test case 1 and change the password.kubectl apply -f ./cloudinit-passwd.yaml.cat /oem/99_passwd.yaml. The new password should be shown.Case 3
cloudinit-bad-document.yamlwith the content:kubectl apply -f ./cloudinit-bad-document.yaml. The output should look like:ls -alh /oem/99_bad-document.yamlon any node, which should fail.Additional documentation or context