Compare layers of images #233
Conversation
There was a problem hiding this comment.
Nice! Looking pretty good overall, a few comments inline.
I'm a little unsure about diffing with an empty directory in the case where an image has more layers than another: this would cause the entire file contents of that layer to be outputted, which can be huge. Maybe instead we warn the user that a layer is present in one image that isn't in another, and have them run analyze if they want to see the contents?
Also, maybe out of the scope of this PR....but it would be really cool if we could do something like
container-diff analyze daemon://my_image:tag --type=layer --layer layer_id and see the contents of an individual layer :)
differs/file_diff.go
Outdated
| if err != nil { | ||
| return util.FileLayerAnalyzeResult{}, err | ||
| } | ||
| directoryEntry := pkgutil.GetDirectoryEntries(layerDir) |
There was a problem hiding this comment.
nit: combine with line below so you don't create a new local var
directoryEntries = append(pkgutil.GetDirectoryEntries(layerDir), directoryEntry)
| if err != nil { | ||
| return err | ||
| } | ||
| return unpackTar(tar.NewReader(contents), root, whitelist) |
There was a problem hiding this comment.
As part of switching to go-containerregistry, I removed all the whiteout handling in unpackTar (since it gets handled implicitly by mutate.Extract). I don't think it should cause any problems here since a tombstoned file would be treated like any normal file would, but just making a note of that here in case you can think of any cases where it would be an issue.
| analysis, valid := r.Analysis.([][]util.DirectoryEntry) | ||
| if !valid { | ||
| logrus.Error("Unexpected structure of Analysis. Should be of type []DirectoryEntry") | ||
| return errors.New("Could not output FileAnalyzer analysis result") |
There was a problem hiding this comment.
I think this error will get outputted somewhere further up the stack right? Can you just do an errors.Wrapf here and return that
There was a problem hiding this comment.
Sorry, what error do you mean? There isn't a specific error to wrap, which is why I think all the AnalyzeResults are returning new ones.
There was a problem hiding this comment.
Ah sorry, I meant to combine these errors into one and return them without logging anything here. I see that you're just following the pattern from what was already here though, so it's fine for now. We should clean this error handling up later.
There was a problem hiding this comment.
Cool, sounds good
| analysis, valid := r.Analysis.([][]util.DirectoryEntry) | ||
| if !valid { | ||
| logrus.Error("Unexpected structure of Analysis. Should be of type []DirectoryEntry") | ||
| return errors.New("Could not output FileAnalyzer analysis result") |
There was a problem hiding this comment.
Same comment w.r.t. error here
differs/differs.go
Outdated
| "history": HistoryAnalyzer{}, | ||
| "metadata": MetadataAnalyzer{}, | ||
| "file": FileAnalyzer{}, | ||
| "file-layer": FileLayerAnalyzer{}, |
There was a problem hiding this comment.
I think this would be a little cleaner as layer instead of file-layer
|
Yah makes sense, only layers that are present in both images should be diffed now, and I definitely could add diffing specific layers in another PR. I think this is RFAL! |
I added a "file-layer" type which unpacks each layer of each image to a different temp dir, and then compares the files within corresponding layers.
If one of the images contains more layers than the other, I compare the file system of the extra layer with an empty directory.
#222