-
Notifications
You must be signed in to change notification settings - Fork 31
OCPBUGS-56818, OPRUN-3869: [Default Catalog Consistency Test] (feat) add check for bin/opm files in filesystem #362
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 all commits
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 |
---|---|---|
|
@@ -5,11 +5,15 @@ import ( | |
"errors" | ||
"fmt" | ||
"io/fs" | ||
"os" | ||
"os/exec" | ||
"path/filepath" | ||
) | ||
|
||
// AllFilesystemChecks returns a list of filesystem checks to be performed on the image filesystem. | ||
func AllFilesystemChecks() []FilesystemCheck { | ||
return []FilesystemCheck{ | ||
FilesystemHasGoExecutables("bin/opm"), | ||
FilesystemHasDirectories( | ||
"configs", | ||
"tmp/cache", | ||
|
@@ -21,7 +25,7 @@ func AllFilesystemChecks() []FilesystemCheck { | |
func FilesystemHasDirectories(paths ...string) FilesystemCheck { | ||
return FilesystemCheck{ | ||
Name: "FilesystemHasDirectories" + fmt.Sprintf(":(%q) ", paths), | ||
Fn: func(ctx context.Context, imageFS fs.FS) error { | ||
Fn: func(ctx context.Context, imageFS fs.FS, _ string) error { | ||
var errs []error | ||
for _, path := range paths { | ||
stat, err := fs.Stat(imageFS, path) | ||
|
@@ -38,3 +42,80 @@ func FilesystemHasDirectories(paths ...string) FilesystemCheck { | |
}, | ||
} | ||
} | ||
|
||
// FilesystemHasGoExecutables checks that the given paths exist inside the extracted image filesystem. | ||
// It supports regular files and symlinks: | ||
// - If it's a regular file, we just check that it exists. | ||
// - If it's a symlink, we check that the link target exists. | ||
// | ||
// This is useful for verifying container images where binaries may be symlinks, | ||
// In our image builds (see: | ||
// https://github.com/openshift/operator-framework-olm/blob/082d59a819afc43b24e9ca23c531bdfc35418722/operator-registry.Dockerfile#L16-L19), | ||
// the actual binaries are in /bin/registry/*, and /bin/* just has symlinks that point to them. | ||
func FilesystemHasGoExecutables(paths ...string) FilesystemCheck { | ||
return FilesystemCheck{ | ||
Name: "FilesystemHasGoExecutables" + fmt.Sprintf(":(%q)", paths), | ||
|
||
// We use the `os` package instead of fs.FS because fs.FS does not support Lstat or Readlink, | ||
// which are needed to detect and resolve symlinks correctly. | ||
// Therefore, we are looking to real file paths under the unpacked image (in tmpDir/fs). | ||
Fn: func(_ context.Context, _ fs.FS, tmpDir string) error { | ||
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. @joelanford @bentito ^ see I hope that clarifies your previous comment. For all images we create a symlink for the opm. Since we're working with a real unpacked image on disk, using `os.* functions' brings the correct result, I mean, do the check properly. As far as I see, we're not breaking any abstraction — we’re checking the actual filesystem, which includes symlinks. See when we pass an invalid path. <check.Error>{
CheckName: "[FS]:FilesystemHasValidPathsOrLinks:([\"bin/invalid\" \"bin/registry/opm\"])",
Err: <*errors.joinError | 0x1400106b350>{
errs: [
<*fmt.wrapError | 0x14000452680>{
msg: "error stating \"bin/invalid\": lstat /var/folders/n4/j272tr6d7hq63mf7_skv6zr80000gn/T/oci-redhat-marketplace-index-353931148/fs/bin/invalid: no such file or directory",
err: <*fs.PathError | 0x140006f8cc0>{
Op: "lstat",
Path: "/var/folders/n4/j272tr6d7hq63mf7_skv6zr80000gn/T/oci-redhat-marketplace-index-353931148/fs/bin/invalid",
Err: <syscall.Errno>0x2,
},
},
],
},
},
], So, I think that is the best that we can do to verify this one 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. maybe it would be clearer to just not take the abstract FS as a parameter? 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.
We need to keep the fs.FS parameter because this function follows a shared interface used by all the filesystem checks. This one doesn’t use it, but the others do — so we keep it for consistency. |
||
var errs []error | ||
root := filepath.Join(tmpDir, "fs") | ||
|
||
for _, rel := range paths { | ||
fullPath := filepath.Join(root, rel) | ||
|
||
binaryPath, err := resolveImageSymlink(root, fullPath) | ||
if err != nil { | ||
errs = append(errs, fmt.Errorf("path %q: %w", rel, err)) | ||
continue | ||
} | ||
|
||
out, err := exec.Command("go", "version", "-m", binaryPath).CombinedOutput() | ||
if err != nil { | ||
errs = append(errs, fmt.Errorf("not a Go binary or unreadable metadata for %q: %v\n%s", rel, err, out)) | ||
continue | ||
} | ||
} | ||
|
||
return errors.Join(errs...) | ||
}, | ||
} | ||
} | ||
|
||
// resolveImageSymlink resolve the symlink target. | ||
// Example 1: if the target is absolute like "/bin/registry/opm", | ||
// that means it should exist under the image root at tmpDir/fs/bin/registry/opm. | ||
// Example 2: if the target is relative like "registry/opm" or "../registry/opm", | ||
// resolve it relative to the symlink’s own directory. | ||
func resolveImageSymlink(root, fullPath string) (string, error) { | ||
fi, err := os.Lstat(fullPath) | ||
if err != nil { | ||
return "", fmt.Errorf("error stating file: %w", err) | ||
} | ||
|
||
if fi.Mode()&os.ModeSymlink == 0 { | ||
return fullPath, nil | ||
} | ||
|
||
target, err := os.Readlink(fullPath) | ||
if err != nil { | ||
return "", fmt.Errorf("unreadable symlink: %w", err) | ||
} | ||
|
||
var resolved string | ||
if filepath.IsAbs(target) { | ||
// remove leading "/" and resolve from root | ||
resolved = filepath.Join(root, target[1:]) | ||
} else { | ||
// If it's not a symlink, we just check that the file exists. | ||
resolved = filepath.Join(filepath.Dir(fullPath), target) | ||
} | ||
|
||
if _, err := os.Stat(resolved); err != nil { | ||
return "", fmt.Errorf("symlink points to missing target %q: %w", resolved, err) | ||
} | ||
|
||
return resolved, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,9 +179,9 @@ func extractLayers(ctx context.Context, layoutPath, fsPath, tag string) error { | |
hdr.Uid = os.Getuid() | ||
hdr.Gid = os.Getgid() | ||
if hdr.FileInfo().IsDir() { | ||
hdr.Mode = 0700 | ||
hdr.Mode = 0755 | ||
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. maybe just:
now that we're assigning the same mode for files and dirs? |
||
} else { | ||
hdr.Mode = 0600 | ||
hdr.Mode = 0755 | ||
} | ||
return true, 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.
do we care if it points to a particular target or not? or only whether it exists?
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.
at the same time, this feels a bit like testing implementation - should we rather just do something like execute opm --version and grep for some string (or something like that). I.e. it shouldn't really matter if its a symlink or not, just that it can be executed and that its opm...?
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 know this isn’t a bulletproof check,
but if the image was built without the binary (using opm in binless mode), we won’t have the symlink at all — so this will catch that.
If the symlink exists but is broken (e.g., points to a non-existent target), we’ll also catch that here.
But it doesn’t go beyond that.
Still, it does reliably cover 100% of the cases where the pipeline uses the option that skips adding the binary with is what ofen would endup in the scenario that we want to avoid right?
Yep, I started looking into options like that.
But on macOS, if we call bin/opm, it’s not built for darwin/arm64, so it won’t work at all.
That adds a lot of complexity for anyone on macOS trying to contribute — so it’s worth asking: is the value really worth the pain? 🤔
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.
maybe doing something like:
go version -m bin/opm
?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.
Done 👍