feat: report mutated files by letFunny · Pull Request #131 · canonical/chisel

letFunny · 2024-04-05T09:35:46Z

Have you signed the CLA?

The paths with mutable: true property can be "mutated" with mutation
scripts. Thus, the files matching those paths may have a different size
and hash after the mutation scripts have been run. The report should
reflect these changes by updating the entries of the mutated paths.

Paths can also have the until: mutate property. This means that
those files are available until the mutation scripts have been run and
are not present in the final file system. Thus, they should not be part
of the report either.

This commit adds support for both -- it updates the changed properties
of mutated files and makes sure not to add report entries for
until: mutate.

letFunny · 2024-04-16T08:56:35Z


-	release := options.Selection.Release
 	targetDir := filepath.Clean(options.TargetDir)
-	targetDirAbs := targetDir


[Note for reviewer]: There was an edge case caught by spread where the relative path where causing the report to misbehave and not add the mutated files. I have changed the test with relative paths in slicer_test.go to fail (regression test).

This happened because targetDir was used inconsistently together with targetDirAbs, that's why I have changed it so all the code in slicer uses the absolute version and there are no edge cases that we have to test separately.

Good catch, thanks.

The paths with ``mutable: true`` property can be "mutated" with mutation scripts. Thus, the files matching those paths may have a different size and hash after the mutation scripts have been run. The report should reflect these changes by updating the entries of the mutated paths. Paths can also have the ``until: mutate`` property. This means that those files are available until the mutation scripts have been run and are not present in the final file system. Thus, they should not be part of the report either. This commit adds support for both -- it updates the changed properties of mutated files and makes sure not to add report entries for ``until: mutate``.

letFunny · 2024-04-16T09:05:40Z

Apologies for force pushing, there was a problem with the authoring of one of the commits because Github rewrote Rafid's email when working on my fork and commiting. I have had to change it and force push to get the CLA passing.

rebornplusplus

LGTM 👍

niemeyer

This is going in a good direction, Alberto, thanks.

Here is a first pass.

niemeyer · 2024-04-22T15:30:33Z

 	}

-	if entry, ok := r.Entries[relPath]; ok {
+	if entry, ok := r.Entries[path]; ok {


The logic here used relative paths and absolute paths intentfully, for security but also clarity on the user interface. Note how we only reported the out-of-root issue independently, but then all the details below are inside the root path, and thus nicely short and clear. The variables were also aptly named to reduce the chances of mistakes while using them. All of this was careful design, and it doesn't look like the motivation for this PR requires touching these details.

~~Are you suggesting changing the name back to relPath that is indeed more informative?~~

If you are talking about the refactor into sanitizePath, I think having it as a separate function has value because we want the logic to be the same across all entry points to the report (Add and Mutate), this way we can change the logic in one place with no possibility for drift.

~~Maybe what we need to do is change the name of sanitizePath to something more informative.~~

Discussed this offline. There was a misunderstanding that we were no longer using relative path for the report itself and for error reporting, that could indeed be a major flaw and lead to some nasty bugs. However, that was not the case, the problem was that we renamed the path variable and it was no longer clear which one was relative and which one absolute. We have now changed the names to:

relPath, err := r.sanitizeAbsPath(fsEntry.Path, fsEntry.Mode.IsDir())

which hopefully makes it clear that fsEntry.Path is an absolute path and that we get back a relative one.

niemeyer

Thanks, Alberto. One more pass.

niemeyer · 2024-04-29T14:00:10Z

-		relPath = relPath + "/"
+	relPath, err := r.sanitizeAbsPath(fsEntry.Path, fsEntry.Mode.IsDir())
+	if err != nil {
+		return fmt.Errorf("cannot add path: %s", err)


Per last conversation/review, these errors need tuning so we can understand what they're trying to convey, and also so that they are more consistent with each other. For example:

"cannot add path to report: %s"
"cannot mutate path in report: %s"
"cannot mutate path in report: %s not previously reported"
"cannot mutate path in report: %s is a directory"

Agree with one minor change reported -> added in:

cannot mutate path in report: %s not previously added

to make it more accurate with the report API of Add and Mutate.

niemeyer

Looks good, except for the regression on Mode.

letFunny · 2024-05-03T10:44:50Z

-}
-
-// dumpFSEntry returns the file entry in the same format as [testutil.TreeDump].
-func dumpFSEntry(fsEntry *fsutil.Entry, root string) map[string]string {


[Comment for reviewer] Now it is redundant, better to unify it with testutils.TreeDumpEntry.

niemeyer

This is looking good, thanks Alberto.

A few minor points for our consideration.

niemeyer · 2024-06-21T13:52:44Z

+	options: fsutil.CreateOptions{
+		Path: "foo",
+		// Mode should be ignored for existing entry.
+		Mode: 0644,


This sounds correct for the context of this PR, but not in the context of a cut being done from the ground up. So we need to dig down a bit and find out the background here in more detail. For example, if we somehow have a dirty filesystem, we don't want to ignore the fact that it was dirty and potentially with a completely wrong mode, and then ship an image that reports a wrong mode as if it was correct.

As we agreed offline, we will follow up on this issue on another PR once we have dived deep into how overlayfs and docker works to correctly create the different layers.

niemeyer · 2024-06-21T14:10:04Z


-	release := options.Selection.Release
 	targetDir := filepath.Clean(options.TargetDir)
-	targetDirAbs := targetDir


Good catch, thanks.

niemeyer

Looks good, Alberto. Thanks!

Per our conversation, let's follow up on the file overwrite behavior.

letFunny force-pushed the report-mutated-files branch 4 times, most recently from 9b1efd5 to 78acfb4 Compare April 5, 2024 10:21

letFunny marked this pull request as ready for review April 5, 2024 10:22

cjdcordeiro requested a review from rebornplusplus April 5, 2024 13:05

cjdcordeiro added the Priority Look at me first label Apr 12, 2024

letFunny requested a review from niemeyer April 16, 2024 08:39

letFunny commented Apr 16, 2024

View reviewed changes

letFunny force-pushed the report-mutated-files branch from db70406 to a3abd95 Compare April 16, 2024 09:03

Merge branch 'main' into report-mutated-files

1868a1e

more tests for relative path in report

3400ca4

rebornplusplus mentioned this pull request Apr 18, 2024

feat: add Chisel DB letFunny/chisel#11

Merged

rebornplusplus approved these changes Apr 19, 2024

View reviewed changes

niemeyer requested changes Apr 22, 2024

View reviewed changes

cjdcordeiro added the Reviewed Supposedly ready for tuning or merging label Apr 23, 2024

letFunny added 6 commits April 23, 2024 12:06

make difference between rel and abs paths in report clear

c813351

remove CreateFile, use Mutated callback instead

443d402

remove new errors, remove unnecessary pathInfo

0ad38fd

rel path in error

e8417b7

named return value for better documentation

f076392

remove unnecessary closure

cf708ef

letFunny requested review from niemeyer and rebornplusplus April 23, 2024 10:42

letFunny removed the Reviewed Supposedly ready for tuning or merging label Apr 23, 2024

letFunny mentioned this pull request Apr 26, 2024

feat: create chisel.db #133

Closed

1 task done

niemeyer requested changes Apr 29, 2024

View reviewed changes

address review comments

d7abb99

letFunny added 5 commits April 29, 2024 16:35

change all paths in test to uniform nomenclature

30a19cf

add full treedump tests for entry

cbcbec9

extra test

69c780f

remove mutated flag

ec5e988

explicit field for documentation

108edb4

letFunny requested a review from niemeyer April 30, 2024 12:16

extra integ test

63a8463

niemeyer requested changes May 3, 2024

View reviewed changes

Comment thread internal/scripts/scripts.go Resolved

Comment thread internal/scripts/scripts_test.go Resolved

letFunny added 3 commits May 3, 2024 12:40

fsutil.Create always returns the mode of the disk entry

99ff548

script tests check mode

de6ae08

remove duplicated

1800f89

letFunny commented May 3, 2024

View reviewed changes

letFunny added 3 commits May 3, 2024 12:47

move hash back

6a8f252

minor rename, trigger spread

43b7c28

Merge branch 'main' into report-mutated-files

0422839

letFunny added Blocked Waiting for something external and removed Priority Look at me first labels May 21, 2024

letFunny added 3 commits June 18, 2024 16:42

Merge branch 'main' into report-mutated-files

4d54f6b

update tests

20e52cf

move report closer to usage

fc551f9

letFunny removed the Blocked Waiting for something external label Jun 18, 2024

letFunny requested a review from niemeyer June 18, 2024 14:48

letFunny added the Priority Look at me first label Jun 18, 2024

niemeyer requested changes Jun 21, 2024

View reviewed changes

letFunny added 2 commits June 24, 2024 11:24

change mode output to octal

5847c8a

change Mutated to OnWrite

d4057d0

cjdcordeiro mentioned this pull request Jun 25, 2024

Question: SBOM for chiseled Ubuntu images #51

Closed

niemeyer approved these changes Jun 28, 2024

View reviewed changes

niemeyer merged commit 81f351a into canonical:main Jun 28, 2024

letFunny deleted the report-mutated-files branch October 17, 2024 08:38

Conversation

letFunny commented Apr 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

letFunny commented Apr 16, 2024

Uh oh!

rebornplusplus left a comment

Choose a reason for hiding this comment

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niemeyer Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

letFunny Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niemeyer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

niemeyer Apr 22, 2024 •

edited

Loading

letFunny Apr 23, 2024 •

edited

Loading

niemeyer left a comment •

edited

Loading