cmd: validate: add --strict mode by cyphar · Pull Request #211 · vbatts/go-mtree

cyphar · 2025-09-16T14:01:05Z

Due to FreeBSD compatibility, we have so far been forced into some
unfortunate behaviour with regards to which comparison errors we
actually return an error code for. FreeBSD generally does not return
errors if a file or keyword only exists in one of the manifests being
compared.

Some users do not care about FreeBSD compatibility and just want to get
errors if there is any difference between a manifest and directory tree.
Commit 9cdd915 ("cmd: gomtree: re-enable errors when there is a
Modified entry") added a TODO to implement this, but I've only just got
around to implementing it.

This patch adds a new --strict flag to "gomtree validate", which causes
it to error out if any changes were detected and also disables the
FreeBSD compatibility keyword delta filters. This is more akin to what
modern users would expect from a manifest validation tool.

This patch resolves the remaining issues in opencontainers/umoci#620.

Closes #210
Fixes: 21723a3 ("*: fix comparison of missing keywords")
Fixes: 9cdd915 ("cmd: gomtree: re-enable errors when there is a Modified entry")
Signed-off-by: Aleksa Sarai cyphar@cyphar.com

cyphar · 2025-09-16T14:02:46Z

@vbatts I think this includes all of the key bits that would be nice to have for a 0.6.1 (or 0.7). I didn't add anything extra to TestCompare and instead opted to add CLI integration tests but let me know if you want me to add some more unit tests instead.

cyphar · 2025-09-16T14:16:19Z

Hmmm, TestCheck passes locally on my machine...

=== RUN   TestCheck
    check_test.go:25: []mtree.InodeDelta{mtree.InodeDelta{diff:"modified", path:"xattr", new:mtree.Entry{Parent:(*mtree.Entry)(0xc000001200), Children:[]*mtree.Entry(nil), Prev:(*mtree.Entry)(0xc000001200), Next:(*mtree.Entry)(nil), Set:(*mtree.Entry)(0xc000000f00), Pos:979, Raw:"", Name:"xattr", Keywords:[]mtree.KeyVal{"type=dir", "nlink=2", "mode=0755", "size=4096", "time=1758031397.687220504"}, Type:4}, old:mtree.Entry{Parent:(*mtree.Entry)(0xc000132600), Children:[]*mtree.Entry(nil), Prev:(*mtree.Entry)(0xc000132600), Next:(*mtree.Entry)(nil), Set:(*mtree.Entry)(0xc000132300), Pos:979, Raw:"", Name:"xattr", Keywords:[]mtree.KeyVal{"size=4096", "type=dir", "mode=0755", "nlink=2", "time=1758031378.373259893"}, Type:4}, keys:[]mtree.KeyDelta{mtree.KeyDelta{diff:"modified", name:"time", old:"1758031378.373259893", new:"1758031397.687220504", err:error(nil)}}}}
--- FAIL: TestCheck (0.22s)

It's failing because the timestamps are different for some reason...?

cyphar · 2025-09-16T15:17:44Z

Oh, this is also broken in main. Very strange...

vbatts

I began working from oldest threads in my inbox, and see the commits I was reviewing have been split to #212. Moving to review that PR

Aleksa Sarai (7): cmd: validate: add --strict mode compare: move FreeBSD loose keyword comparisons to gomtree command cmd: validate: restructure filtering to use filter functions cmd: validate: remove dead "current keyword" code compare: modernise DirectoryHierarchy compare impl compare: modernise compareEntry compare: export official way to modify InodeDelta.Diff LGTMs: cyphar

At the moment, filtering out keyword changes from an InodeDelta after doing Compare is a little complicated and error-prone. The simplest solution is to allow for callers to access a pointer to the underlying slice so it can be modified properly. The filtering logic in the gomtree command-line implicitly depends on the behaviour of InodeDelta.Diff -- DiffPtr would be a far more appropriate replacement. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

The previous logic was overly complicated and can be simplified a lot (especially now that we have helpful generic stdlib packages for dealing with maps). There are still several bugs in this, but the intention of this patch is to just modernise the code without making any functional changes. Bugfixes will come later. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

This is fairly similar to the compareEntry modernisation, but there was no need to use maps.Insert or maps.Collect since we want to pre-allocate the map sizes anyway. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

commit 21723a3 ("*: fix comparison of missing keywords") removed the error that this for loop was intended for but left the no-op for loop in its place. Fixes: 21723a3 ("*: fix comparison of missing keywords") Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

This lets us modernise the filtering logic a little bit, and also allows us to configure which filters we wish to apply (a future patch will move some of the filtering done in the top-level go-mtree package to the cmd/validate package). Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

FreeBSD has quite unfortunate behaviour when dealing with keywords that are missing in one of the manifests being compared -- namely, they ignore these instances. Commit 21723a3 ("*: fix comparison of missing keywords") re-added this behaviour after the introduction of the Compare API, but unfortunately it was implemented in the Compare API itself -- meaning that library users (which didn't want this behaviour) were silently opted into it. This patch moves the behaviour to the command-line, where it belongs (a future patch in this series will allow users to opt-out of this unfortunate behaviour, as well as some other unfortunate FreeBSD compatibility behaviours). Fixes: 21723a3 ("*: fix comparison of missing keywords") Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

Due to FreeBSD compatibility, we have so far been forced into some unfortunate behaviour with regards to which comparison errors we actually return an error code for. FreeBSD generally does not return errors if a file or keyword only exists in one of the manifests being compared. Some users do not care about FreeBSD compatibility and just want to get errors if there is any difference between a manifest and directory tree. Commit 9cdd915 ("cmd: gomtree: re-enable errors when there is a Modified entry") added a TODO to implement this, but I've only just got around to implementing it. This patch adds a new --strict flag to "gomtree validate", which causes it to error out if any changes were detected and also disables the FreeBSD compatibility keyword delta filters. This is more akin to what modern users would expect from a manifest validation tool. Fixes: 21723a3 ("*: fix comparison of missing keywords") Fixes: 9cdd915 ("cmd: gomtree: re-enable errors when there is a Modified entry") Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

mikemccracken · 2025-12-18T22:26:17Z

Hi, is this active? It'd simplify keeping umoci builds up to date if it could move off its fork, which apparently depends on this.

(I am looking into building an umoci with an updated golang.org/x/crypto to fix a CVE)

cyphar · 2025-12-19T01:17:53Z

@mikemccracken Yes, I'm just waiting for @vbatts to have the time to review this (as well as #214).

Aleksa Sarai (7): cmd: validate: add --strict mode compare: move FreeBSD loose keyword comparisons to gomtree command cmd: validate: restructure filtering to use filter functions cmd: validate: remove dead "current keyword" code compare: modernise DirectoryHierarchy compare impl compare: modernise compareEntry compare: export official way to modify InodeDelta.Diff LGTMs: cyphar

vbatts · 2026-01-05T16:49:17Z

Ohman sorry! I got to one of the PR's and thought the others were draft or needing to be rebased. Is everything ready for review?

cyphar · 2026-01-05T22:02:02Z

Yes, everything should be ready for review. FWIW I've been shipping these patches in umoci for a few months now.

vbatts · 2026-01-09T01:52:42Z

LGTM

cyphar force-pushed the strict-mode branch from cceadb7 to c2f0860 Compare September 16, 2025 14:02

cyphar mentioned this pull request Sep 16, 2025

umoci: add raw-mtree-validate helper to replace go-mtree opencontainers/umoci#620

Merged

cyphar force-pushed the strict-mode branch 9 times, most recently from 4dd69ba to 722413f Compare September 23, 2025 15:17

vbatts reviewed Sep 26, 2025

View reviewed changes

Comment thread .github/workflows/go.yml

Comment thread check_test.go

vbatts mentioned this pull request Sep 26, 2025

test: migrate most tests to testify #212

Merged

cyphar force-pushed the strict-mode branch from 722413f to b06e4de Compare September 28, 2025 23:50

cyphar added 7 commits October 25, 2025 22:22

compare: modernise DirectoryHierarchy compare impl

2184266

This is fairly similar to the compareEntry modernisation, but there was no need to use maps.Insert or maps.Collect since we want to pre-allocate the map sizes anyway. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

cyphar force-pushed the strict-mode branch from b06e4de to 0592459 Compare October 25, 2025 11:22

cyphar marked this pull request as ready for review October 25, 2025 11:22

cyphar mentioned this pull request Oct 31, 2025

Unable to install v0.6.0 - go v1.25.1 opencontainers/umoci#646

Closed

vbatts merged commit 256bc38 into vbatts:main Jan 9, 2026
3 checks passed

cyphar deleted the strict-mode branch January 9, 2026 06:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

cmd: validate: add --strict mode#211

cmd: validate: add --strict mode#211
vbatts merged 7 commits intovbatts:mainfrom
cyphar:strict-mode

cyphar commented Sep 16, 2025 •

edited

Loading

Uh oh!

cyphar commented Sep 16, 2025

Uh oh!

cyphar commented Sep 16, 2025 •

edited

Loading

Uh oh!

cyphar commented Sep 16, 2025

Uh oh!

vbatts left a comment

Uh oh!

Uh oh!

Uh oh!

mikemccracken commented Dec 18, 2025

Uh oh!

cyphar commented Dec 19, 2025

Uh oh!

vbatts commented Jan 5, 2026 via email

Uh oh!

cyphar commented Jan 5, 2026

Uh oh!

Uh oh!

vbatts commented Jan 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

cyphar commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyphar commented Sep 16, 2025

Uh oh!

cyphar commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyphar commented Sep 16, 2025

Uh oh!

vbatts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mikemccracken commented Dec 18, 2025

Uh oh!

cyphar commented Dec 19, 2025

Uh oh!

vbatts commented Jan 5, 2026 via email

Uh oh!

cyphar commented Jan 5, 2026

Uh oh!

Uh oh!

vbatts commented Jan 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cyphar commented Sep 16, 2025 •

edited

Loading

cyphar commented Sep 16, 2025 •

edited

Loading