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

felixfontein · 2020-12-29T10:29:20Z

This is my first shot at using yaml.v3 instead of the modified version of yaml.v2 for handling YAML files.

So far, I only implemented reading YAML files. My first conclusions:

yaml.v3 seems to also have comment handling problems (comments placed in wrong places).
yaml.v3 only supports one YAML document per file.

autrilla · 2020-12-29T10:48:49Z

This is my first shot at using yaml.v3 instead of the modified version of yaml.v2 for handling YAML files.

Great! Glad to see someone taking a stab at this.

So far, I only implemented reading YAML files. My first conclusions:

yaml.v3 seems to also have comment handling problems (comments placed in wrong places).

Well, that's very unfortunate. That's the biggest benefit I expected from migrating to v3. Even so, longer term I think it's better to migrate, since I imagine contributions to v3 will be more common than to the mozilla-services fork, and eventually it'll all be ironed out.

yaml.v3 only supports one YAML document per file.

From reading the documentation (probably less thoroughly than you did, though), ISTM that it's supported, but through the more cumbersome Decoder API.

felixfontein · 2020-12-29T10:55:00Z

yaml.v3 seems to also have comment handling problems (comments placed in wrong places).

Well, that's very unfortunate. That's the biggest benefit I expected from migrating to v3. Even so, longer term I think it's better to migrate, since I imagine contributions to v3 will be more common than to the mozilla-services fork, and eventually it'll all be ironed out.

I hope so too!

yaml.v3 only supports one YAML document per file.

From reading the documentation (probably less thoroughly than you did, though), ISTM that it's supported, but through the more cumbersome Decoder API.

I might not have done that. I mainly saw the note in https://github.com/go-yaml/yaml/tree/v3#compatibility that it only supports one document. The Decoder API sounds interesting, I have to take a closer look at how it works.

For now I implemented a hack which simply splits YAML files by "\n---\n" and parses every part independently.

felixfontein · 2020-12-29T11:01:59Z

Decoder was a good idea, df78351 implements it instead of the hack.

felixfontein · 2020-12-29T19:23:20Z

Ok, I think this is a first working version.

It unfortunately fails tests due to a problem with comments that was fixed with the modified yaml.v2 copy, namely TestComment3 fails with difference:

@@ -4,5 +4,5 @@
     podLabels:
+        jobLabel: node-exporter
         ## Add the 'node-exporter' label to be used by serviceMonitor to match standard common usage in rules and grafana dashboards
         ##
-        jobLabel: node-exporter
     extraArgs:

autrilla · 2020-12-29T21:56:32Z

This is great to see! Have you found any instances that the previous version did not handle well, but this one does?

felixfontein · 2020-12-30T08:14:51Z

I've cherry-picked #793 into this PR so I can fully compile sops from this PR. So far I've been mostly sticking to the YAML store tests, since they work without the INI store that doesn't compile. I'll now do some more tests...

felixfontein · 2020-12-30T09:04:23Z

One improvement of this branch I found: if encrypting something like

a:
  b:
    c: d

# comment
e: f

the comment was moved to the c: d level with the old version. The new version keeps it at the correct level.

Also, indentation of list elements differs (IMO it is better): instead of

list:
-   item:
    -   another item

you get

list:
    - item:
        - another_item

autrilla · 2020-12-30T11:06:37Z

Ah, good, so at least we're also getting some improvement. Because of the indentation change, we should recommend to our users to "rotate" all their files with the new release, to avoid difficult to read diffs in the future.

felixfontein · 2020-12-30T12:49:39Z

Yes, definitely. This should also be more than a bugfix release, since this is potentially breaking (especially since comments might "jump" to new places, I guess this already was the cause for #695 (comment), and this PR will have the same problem).

I've tried fixing some of the YAML problems in yaml.v3 in go-yaml/yaml#684.

felixfontein · 2021-02-04T09:59:44Z

I think I'll comment out the tests that wait for the yaml.v3 fix, so maybe we can get this merged already (and update again later once the yaml.v3 fix lands).

felixfontein · 2021-02-04T18:06:01Z

@autrilla I cannot reproduce the functional test errors locally:

$ ./sops -d res/comments.enc.yaml
# first comment in file
lorem: ipsum
# this-is-a-comment
dolor: sit
$ ./sops -d res/comments_unencrypted_comments.yaml 
[SOPS]	 WARN[0000] Found possibly unencrypted comment in file. This is to be expected if the file being decrypted was created with an older version of SOPS.  comment=" first comment in file"
[SOPS]	 WARN[0000] Found possibly unencrypted comment in file. This is to be expected if the file being decrypted was created with an older version of SOPS.  comment=" this-is-a-comment"
# first comment in file
lorem: ipsum
# this-is-a-comment
dolor: sit

From how I interpret functional-tests/src/lib.rs, it executes the two above commands and expects first comment in file and this-is-a-comment to show up in stdout. Which they do in the above runs.

felixfontein · 2021-02-09T19:16:24Z

@autrilla any idea what could be the problem here?

autrilla · 2021-02-09T19:31:01Z

No idea. It passes locally. Let's try rerunning the action.

felixfontein · 2021-02-10T06:59:49Z

Ok, so from the debug output, the first comment is really missing. So for some reason the binary built for the functional tests seems to use a different yaml.v3 version.

autrilla · 2021-02-10T09:32:11Z

That binary is just the one built with make, but it could be that the binary built in CI is different from the one built locally

…

On Wed, 10 Feb 2021 at 08:00, Felix Fontein ***@***.***> wrote: Ok, so from the debug output, the first comment is really missing. So for some reason the binary built for the functional tests seems to use a different yaml.v3 version. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#791 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARH4VZXK44CLCTCD2AMKJDS6IVHNANCNFSM4VNBYF4A> .

felixfontein · 2021-02-21T06:39:47Z

@autrilla I noticed that the build step (whose result is used in the functional tests) uses the -v argument, which prints the package names as they are being compiled. It lists two YAML packages:

gopkg.in/yaml.v2
github.com/mozilla-services/yaml

But it does not list gopkg.in/yaml.v3. So whatever it is building in that step, it is NOT the version from this branch.

Maybe the checkout command needs to be adjusted to check out the repo in the correct place ($GOPATH/src/go.mozilla.org/sops/), maybe then it will work better? Maybe similar to here: https://github.com/go-yaml/yaml/blob/v3/.github/workflows/go.yaml#L28-L31

autrilla · 2021-02-21T07:57:01Z

That's weird and concerning, glad you found out. ISTM like we might be building the code from master every time in Github Actions, which is... not what we want to run PR tests against.

felixfontein · 2021-02-21T08:46:22Z

I've squashed the commits and created a new PR for the CI fix commit: #820

felixfontein · 2021-02-21T17:23:58Z

I think I'll comment out the tests that wait for the yaml.v3 fix, so maybe we can get this merged already (and update again later once the yaml.v3 fix lands).

Now that tests finally pass: @autrilla, what do you think?

autrilla

Thanks a lot!

felixfontein · 2021-02-21T20:04:48Z

@autrilla thanks for reviewing and merging both PRs!

ikedam · 2021-07-24T06:05:27Z

stores/yaml/store.go

+		mapping.Kind = yaml.MappingNode
+		// Marshal branch to global mapping node
+		store.appendTreeBranch(branch, &mapping)
+		if len(mapping.Content) == 0 {


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).

Created #908

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.

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".

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).

felixfontein force-pushed the yaml-v3 branch from fcee748 to 333c227 Compare December 30, 2020 08:22

felixfontein mentioned this pull request Dec 30, 2020

[v3] Improve comment handling go-yaml/yaml#684

Closed

felixfontein mentioned this pull request Dec 30, 2020

Do not discard comments at beginning of YAML documents #757

Merged

felixfontein force-pushed the yaml-v3 branch from c1a24a9 to d02620f Compare January 6, 2021 21:57

felixfontein closed this Jan 7, 2021

felixfontein reopened this Jan 7, 2021

felixfontein mentioned this pull request Feb 2, 2021

Decripting failure: yaml comment next to inline array #811

Open

onedr0p mentioned this pull request Feb 3, 2021

Investigate using another secret method onedr0p/home-ops#692

Closed

felixfontein mentioned this pull request Feb 4, 2021

Sops swallows YAML comment of each first key:value pair #384

Open

felixfontein force-pushed the yaml-v3 branch from d79412c to 3bc88e8 Compare February 4, 2021 17:44

felixfontein closed this Feb 9, 2021

felixfontein reopened this Feb 9, 2021

felixfontein force-pushed the yaml-v3 branch from d2f04a5 to 0e029ae Compare February 9, 2021 20:19

felixfontein added 15 commits February 21, 2021 09:42

Add another test (that currently fails).

f61dbe2

First shot at using yaml.v3 for reading YAML files with comments.

75bd91e

Allow parsing multi-document YAML files.

840f45c

Use Decoder to parse multi-part documents.

1cb55be

Use yaml.v3 for config and audit.

fadc415

First step of serializing YAML using yaml.v3.

1c3ba93

Always serialize with yaml.v3.

7780ea2

Remove debug prints.

9c88101

Remove traces of github.com/mozilla-services/yaml.

796084a

Improve serialization of documents consisting only of comments.

61ca8c8

Improve handling of some empty documents.

f6c066b

Adjust to latest changes in go-yaml/yaml#684.

a4f9d11

Bump yaml.v3 version, temporarily disable failing tests.

c82c500

Run go mod tidy.

b9f667a

Fix CI.

ab081ea

felixfontein force-pushed the yaml-v3 branch from 90f3358 to ab081ea Compare February 21, 2021 08:44

felixfontein mentioned this pull request Feb 21, 2021

Fix CI #820

Merged

felixfontein changed the title ~~[WIP] Use yaml.v3 instead of modified yaml.v2 for handling YAML files~~ Use yaml.v3 instead of modified yaml.v2 for handling YAML files Feb 21, 2021

autrilla approved these changes Feb 21, 2021

View reviewed changes

autrilla merged commit e2d6d0f into getsops:develop Feb 21, 2021

felixfontein deleted the yaml-v3 branch February 21, 2021 20:04

ajvb mentioned this pull request Mar 24, 2021

v3.7.0 - develop -> master #838

Merged

ikedam reviewed Jul 24, 2021

View reviewed changes

felixfontein mentioned this pull request Aug 5, 2021

Cannot output empty yaml file since sops-v3.7.0 #907

Closed

Ph0tonic mentioned this pull request Nov 25, 2023

sops/3.6.0 encrypted yaml file is broken (even to sops) #695

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

felixfontein commented Dec 29, 2020

autrilla commented Dec 29, 2020

felixfontein commented Dec 29, 2020

felixfontein commented Dec 29, 2020

felixfontein commented Dec 29, 2020

autrilla commented Dec 29, 2020

felixfontein commented Dec 30, 2020

felixfontein commented Dec 30, 2020

autrilla commented Dec 30, 2020

felixfontein commented Dec 30, 2020

felixfontein commented Feb 4, 2021

felixfontein commented Feb 4, 2021

felixfontein commented Feb 9, 2021

autrilla commented Feb 9, 2021

felixfontein commented Feb 10, 2021

autrilla commented Feb 10, 2021 via email

felixfontein commented Feb 21, 2021

autrilla commented Feb 21, 2021

felixfontein commented Feb 21, 2021

felixfontein commented Feb 21, 2021

autrilla left a comment

felixfontein commented Feb 21, 2021

ikedam Jul 24, 2021

ikedam Jul 24, 2021

felixfontein Jul 24, 2021

ikedam Jul 25, 2021

felixfontein Jul 25, 2021

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

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

Conversation

felixfontein commented Dec 29, 2020

autrilla commented Dec 29, 2020

felixfontein commented Dec 29, 2020

felixfontein commented Dec 29, 2020

felixfontein commented Dec 29, 2020

autrilla commented Dec 29, 2020

felixfontein commented Dec 30, 2020

felixfontein commented Dec 30, 2020

autrilla commented Dec 30, 2020

felixfontein commented Dec 30, 2020

felixfontein commented Feb 4, 2021

felixfontein commented Feb 4, 2021

felixfontein commented Feb 9, 2021

autrilla commented Feb 9, 2021

felixfontein commented Feb 10, 2021

autrilla commented Feb 10, 2021 via email

felixfontein commented Feb 21, 2021

autrilla commented Feb 21, 2021

felixfontein commented Feb 21, 2021

felixfontein commented Feb 21, 2021

autrilla left a comment

Choose a reason for hiding this comment

felixfontein commented Feb 21, 2021

ikedam Jul 24, 2021

Choose a reason for hiding this comment

ikedam Jul 24, 2021

Choose a reason for hiding this comment

felixfontein Jul 24, 2021

Choose a reason for hiding this comment

ikedam Jul 25, 2021

Choose a reason for hiding this comment

felixfontein Jul 25, 2021

Choose a reason for hiding this comment