-
Notifications
You must be signed in to change notification settings - Fork 0
feat: search, load and validates manifests #1
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.
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
Changes from 38 commits
60320ba
c67c661
22ae256
6cb437f
00825de
442bcc6
e19f6f1
b6ecc27
1406b18
8aba035
70b2c5e
c8b7a39
1181871
31f1d3d
df49dfc
f4169b9
aade454
e0bd4a4
2e572fb
3c5de1c
7eff973
b7c7c66
c7196e9
7539db7
cd22c1f
27d8645
373f3c1
1414990
2ce5b21
17162f4
c21ef34
77f5771
40ef8e8
0a25844
71c4496
4696dcc
089109d
09faa6a
bd7a707
3ea70ce
3efb7d2
a421818
e88de2d
acac421
0262cb2
7445fcd
5fca0c5
fb6500b
d7fa155
43e8a71
46d2119
164c68b
15550ad
4ecb555
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,8 +9,10 @@ import ( | |
|
|
||
| "github.com/canonical/chisel/internal/archive" | ||
| "github.com/canonical/chisel/internal/cache" | ||
| "github.com/canonical/chisel/internal/manifestutil" | ||
| "github.com/canonical/chisel/internal/setup" | ||
| "github.com/canonical/chisel/internal/slicer" | ||
| "github.com/canonical/chisel/public/manifest" | ||
| ) | ||
|
|
||
| var shortCutHelp = "Cut a tree with selected slices" | ||
|
|
@@ -73,6 +75,31 @@ func (cmd *cmdCut) Execute(args []string) error { | |
| } | ||
| } | ||
|
|
||
| manifestPaths := manifestutil.FindPathsInRelease(release) | ||
| if len(manifestPaths) >= 0 { | ||
| mfest, mfestPath, err := manifestutil.FromDir(manifestPaths, cmd.RootDir) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if mfest != nil { | ||
| err = manifestutil.CheckDir(mfest, mfestPath, cmd.RootDir) | ||
| if err != nil { | ||
| return err | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Note to reviewer]: At this point a valid manifest was extracted from the root dir but the dir is invalid (or we failed to validate it) so we return the error. |
||
| } else { | ||
| // Merge the slice keys used to build the existing rootfs with the ones | ||
| // explicitly requested. | ||
| mfest.IterateSlices("", func(slice *manifest.Slice) error { | ||
| sk, err := setup.ParseSliceKey(slice.Name) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| sliceKeys = append(sliceKeys, sk) | ||
| return nil | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| selection, err := setup.Select(release, sliceKeys, cmd.Arch) | ||
| if err != nil { | ||
| return err | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| package manifestutil | ||
|
|
||
| import ( | ||
| "crypto/sha256" | ||
| "encoding/hex" | ||
| "fmt" | ||
| "io" | ||
| "io/fs" | ||
| "os" | ||
| "path/filepath" | ||
| "syscall" | ||
|
|
||
| "github.com/canonical/chisel/public/manifest" | ||
| ) | ||
|
|
||
| type pathInfo struct { | ||
| mode string | ||
| size int64 | ||
| link string | ||
| hash string | ||
| } | ||
|
|
||
| // CheckDir checks the content of the target directory matches with | ||
| // the manifest. | ||
| // This function works under the assumption the manifest is valid. | ||
| // Files not managed by chisel are ignored. | ||
| func CheckDir(mfest *manifest.Manifest, mfestPath string, rootDir string) error { | ||
| mfestFullPath := filepath.Join(rootDir, mfestPath) | ||
| h, err := contentHash(mfestFullPath) | ||
| if err != nil { | ||
| return fmt.Errorf("cannot compute hash for %q: %w", mfestFullPath, err) | ||
| } | ||
| mfestHash := hex.EncodeToString(h) | ||
|
|
||
| mfestInodeToFSInode := make(map[uint64]uint64) | ||
| err = mfest.IteratePaths("", func(path *manifest.Path) error { | ||
| fullPath := filepath.Join(rootDir, path.Path) | ||
| pathHash := recordedHash(path) | ||
| if filepath.Base(path.Path) == DefaultFilename { | ||
| // Recorded hash is empty for a manifest path, | ||
| // so set the hash of the reference as the "recorded" value. | ||
| pathHash = mfestHash | ||
| } | ||
| mfestPathInfo := &pathInfo{ | ||
| mode: path.Mode, | ||
| size: int64(path.Size), | ||
| link: path.Link, | ||
| hash: pathHash, | ||
| } | ||
|
|
||
| fsEntryInfo := &pathInfo{} | ||
| info, err := os.Lstat(fullPath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| mode := info.Mode() | ||
| fsEntryInfo.mode = fmt.Sprintf("0%o", unixPerm(mode)) | ||
| ftype := mode & fs.ModeType | ||
| switch ftype { | ||
| case fs.ModeDir: | ||
| // Nothing to do | ||
| case fs.ModeSymlink: | ||
| fsEntryInfo.link, err = os.Readlink(fullPath) | ||
| if err != nil { | ||
| return fmt.Errorf("cannot read symlink %q: %w", fullPath, err) | ||
| } | ||
| case 0: // Regular | ||
| h, err := contentHash(fullPath) | ||
| if err != nil { | ||
| return fmt.Errorf("cannot compute hash for %q: %w", fullPath, err) | ||
| } | ||
| fsEntryInfo.hash = hex.EncodeToString(h) | ||
| fsEntryInfo.size = info.Size() | ||
| default: | ||
| return fmt.Errorf("inconsistent content: %q has unrecognized type %s", fullPath, mode.String()) | ||
| } | ||
|
|
||
| if mfestPathInfo.mode != fsEntryInfo.mode { | ||
| return fmt.Errorf("inconsistent mode at %q: recorded %+v, observed %+v", path.Path, mfestPathInfo.mode, fsEntryInfo.mode) | ||
| } | ||
| if mfestPathInfo.size != fsEntryInfo.size { | ||
| return fmt.Errorf("inconsistent size at %q: recorded %+v, observed %+v", path.Path, mfestPathInfo.size, fsEntryInfo.size) | ||
| } | ||
| if mfestPathInfo.link != fsEntryInfo.link { | ||
| return fmt.Errorf("inconsistent link at %q: recorded %+v, observed %+v", path.Path, mfestPathInfo.link, fsEntryInfo.link) | ||
| } | ||
| if mfestPathInfo.hash != fsEntryInfo.hash { | ||
| return fmt.Errorf("inconsistent hash at %q: recorded %+v, observed %+v", path.Path, mfestPathInfo.hash, fsEntryInfo.hash) | ||
| } | ||
|
|
||
| // Check hardlink | ||
| if path.Inode != 0 { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question that needs to be discussed with Gustavo, should we check that no new hard-links exist outside of Chisel? That is, what happens if the user linked two files that Chisel recorded as not being hardlinks.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there are multiple use cases:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, but to me the last one should clearly be an error. If the manifest says two files are not hardlinks and now they are it should be an error.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree and the new implementation I proposed today is now detecting it. |
||
| if ftype == fs.ModeDir { | ||
| return fmt.Errorf("iconsistent type at %q: recorded a hardlinked path, observed a directory", info.Name()) | ||
| } | ||
| stat, ok := info.Sys().(*syscall.Stat_t) | ||
| if !ok { | ||
| return fmt.Errorf("cannot get syscall stat info for %q", info.Name()) | ||
| } | ||
| inode := stat.Ino | ||
| recordedInode, ok := mfestInodeToFSInode[path.Inode] | ||
| if !ok { | ||
| mfestInodeToFSInode[path.Inode] = inode | ||
| } else if recordedInode != inode { | ||
| return fmt.Errorf("inconsistent content at %q: file hardlinked to a different inode", path.Path) | ||
| } | ||
| } | ||
| return nil | ||
| }) | ||
| return err | ||
| } | ||
|
|
||
| func contentHash(path string) ([]byte, error) { | ||
| f, err := os.Open(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer f.Close() | ||
|
|
||
| h := sha256.New() | ||
| if _, err := io.Copy(h, f); err != nil { | ||
| return nil, err | ||
| } | ||
| return h.Sum(nil), nil | ||
| } | ||
|
|
||
| func recordedHash(path *manifest.Path) string { | ||
| expectedHash := path.FinalSHA256 | ||
| if expectedHash == "" { | ||
| expectedHash = path.SHA256 | ||
| } | ||
| return expectedHash | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,9 @@ import ( | |
| "fmt" | ||
| "io" | ||
| "io/fs" | ||
| "maps" | ||
| "os" | ||
| "path" | ||
| "path/filepath" | ||
| "slices" | ||
| "sort" | ||
|
|
@@ -14,6 +17,7 @@ import ( | |
| "github.com/canonical/chisel/internal/setup" | ||
| "github.com/canonical/chisel/public/jsonwall" | ||
| "github.com/canonical/chisel/public/manifest" | ||
| "github.com/klauspost/compress/zstd" | ||
| ) | ||
|
|
||
| const DefaultFilename = "manifest.wall" | ||
|
|
@@ -34,6 +38,19 @@ func FindPaths(slices []*setup.Slice) map[string][]*setup.Slice { | |
| return manifestSlices | ||
| } | ||
|
|
||
| // FindPathsInRelease finds all the paths marked with "generate:manifest" | ||
| // for the given release. | ||
| func FindPathsInRelease(r *setup.Release) []string { | ||
| allSlices := []*setup.Slice{} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use the
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate? Do you mean use |
||
| for _, pkg := range r.Packages { | ||
| for _, slice := range pkg.Slices { | ||
| allSlices = append(allSlices, slice) | ||
| } | ||
| } | ||
| manifestMap := FindPaths(allSlices) | ||
|
upils marked this conversation as resolved.
Outdated
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit unfortunate that we have to iterate over all slices to collect them just to iterate over them again. Because the real business logic takes a path + info and returns the manifest path by validating then appending a filename, what about extracting only that bit into an auxiliary function instead? Then have both functions call the auxiliary one. Thoughts?
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After implementing a naive version of
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reworked in 4ecb555 |
||
| return slices.Sorted(maps.Keys(manifestMap)) | ||
| } | ||
|
|
||
| type WriteOptions struct { | ||
| PackageInfo []*archive.PackageInfo | ||
| Selection []*setup.Slice | ||
|
|
@@ -340,3 +357,55 @@ func Validate(mfest *manifest.Manifest) (err error) { | |
| } | ||
| return nil | ||
| } | ||
|
|
||
| // FromDir extracts, validates and returns the first manifest found in a rootDir. | ||
| // Also returns the path of the manifest. | ||
| func FromDir(manifestPaths []string, rootDir string) (*manifest.Manifest, string, error) { | ||
| targetDir := filepath.Clean(rootDir) | ||
| if !filepath.IsAbs(targetDir) { | ||
| dir, err := os.Getwd() | ||
| if err != nil { | ||
| return nil, "", fmt.Errorf("cannot obtain current directory: %w", err) | ||
| } | ||
| targetDir = filepath.Join(dir, targetDir) | ||
| } | ||
| var mfest *manifest.Manifest | ||
| var err error | ||
| for _, p := range manifestPaths { | ||
| manifestPath := path.Join(targetDir, p) | ||
| mfest, err = load(manifestPath) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| continue | ||
| } | ||
| return nil, "", fmt.Errorf("cannot read manifest %q from the root directory: %v", manifestPath, err) | ||
| } | ||
| err = Validate(mfest) | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Note to reviewer]: The manifest validation was moved out of the |
||
| if err != nil { | ||
| return nil, "", err | ||
| } | ||
| return mfest, p, nil | ||
| } | ||
| return nil, "", nil | ||
| } | ||
|
|
||
| // load reads and returns a manifest. | ||
| func load(manifestPath string) (*manifest.Manifest, error) { | ||
|
upils marked this conversation as resolved.
Outdated
|
||
| f, err := os.Open(manifestPath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer f.Close() | ||
|
|
||
| r, err := zstd.NewReader(f) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer r.Close() | ||
|
|
||
| mfest, err := manifest.Read(r) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return mfest, nil | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Note to reviewer]: I am not very happy about having to also return the manifest path so I am open to suggestions to avoid that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same suggestion as in all the comments above :)