Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use yaml.v3 instead of modified yaml.v2 for handling YAML files #791

Merged
merged 15 commits into from
Feb 21, 2021
Merged

Use yaml.v3 instead of modified yaml.v2 for handling YAML files #791

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f61dbe2
Add another test (that currently fails).
felixfontein Dec 28, 2020
75bd91e
First shot at using yaml.v3 for reading YAML files with comments.
felixfontein Dec 29, 2020
840f45c
Allow parsing multi-document YAML files.
felixfontein Dec 29, 2020
1cb55be
Use Decoder to parse multi-part documents.
felixfontein Dec 29, 2020
fadc415
Use yaml.v3 for config and audit.
felixfontein Dec 29, 2020
1c3ba93
First step of serializing YAML using yaml.v3.
felixfontein Dec 29, 2020
7780ea2
Always serialize with yaml.v3.
felixfontein Dec 29, 2020
9c88101
Remove debug prints.
felixfontein Dec 29, 2020
796084a
Remove traces of github.com/mozilla-services/yaml.
felixfontein Dec 29, 2020
61ca8c8
Improve serialization of documents consisting only of comments.
felixfontein Dec 30, 2020
f6c066b
Improve handling of some empty documents.
felixfontein Dec 30, 2020
a4f9d11
Adjust to latest changes in go-yaml/yaml#684.
felixfontein Dec 30, 2020
c82c500
Bump yaml.v3 version, temporarily disable failing tests.
felixfontein Feb 4, 2021
b9f667a
Run go mod tidy.
felixfontein Feb 20, 2021
ab081ea
Fix CI.
felixfontein Feb 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Improve serialization of documents consisting only of comments.
  • Loading branch information
@felixfontein
felixfontein committed Feb 21, 2021
commit 61ca8c8d499e433b92572597e402f2cc0ca5448c
6 changes: 5 additions & 1 deletion stores/yaml/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,9 +355,13 @@ func (store *Store) EmitPlainFile(branches sops.TreeBranches) ([]byte, error) {
// Add global mapping
var mapping = yaml.Node{}
mapping.Kind = yaml.MappingNode
doc.Content = append(doc.Content, &mapping)
// Marshal branch to global mapping node
store.appendTreeBranch(branch, &mapping)
if len(mapping.Content) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you tell me what this does?
This looks cause #907 , and removing this looks cause nothing bad (as I don't know what good this does).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #908

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is needed so that empty document works well with go-yaml/yaml#690. Unfortunately that fix for yaml.v3 isn't progressing, so I guess for now we can remove that branch. Once that fix landed in yaml.v3, this branch is definitely needed though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the behavior of sops with your implementation of yaml.v3 by replacing in go.mod:

replace gopkg.in/yaml.v3 v3.0.0-20210107172259-749611fa9fcc => github.com/felixfontein/yaml v0.0.0-20210209202929-35d69a41298b

(35d69a41298b is the last commit of https://github.com/felixfontein/yaml/tree/improve-empty-document-handling)

It results sops confuse "no document" and "empty mapping":

# echo -e "# no document\n---\n# empty mapping\n{}\n" | sops --input-type yaml --output-type yaml -e /dev/stdin | sops --input-type yaml --output-type yaml -d /dev/stdin
# no document
---
# empty mapping

They are distinguished in yaml: http://ben-kiki.org/ypaste/data/21512/index.html

Unfortunately, the internal structure of sops (TreeBranch) cannot distinguish "no document" and "empty mapping".
I think we need to extend TreeBranch to have a flag that indicates "no document".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It results sops confuse "no document" and "empty mapping":
Unfortunately, the internal structure of sops (TreeBranch) cannot distinguish "no document" and "empty mapping".

Exactly. The need to represent "no document" seems to be more important than "empty mapping" - see #695 (comment). I haven't seen (or at least can't remember :) ) any request yet for being able to recover {}.

I think we need to extend TreeBranch to have a flag that indicates "no document".

I don't think it can be solved that simply. While this works when cycling between YAML and the internal data structures, it does not help at all when a YAML file is encrypted. Adjusting the metadata won't help since that will not work for multi-document YAML files (metadata is only there once per YAML file).

doc.HeadComment = mapping.HeadComment
} else {
doc.Content = append(doc.Content, &mapping)
}
// Encode YAML
err := e.Encode(&doc)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion stores/yaml/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ prometheus-node-exporter:
`)

var COMMENT_4 = []byte(`# foo
{}

`)

func TestUnmarshalMetadataFromNonSOPSFile(t *testing.T) {
Expand Down