OCPBUGS-56818, OPRUN-3869: [Default Catalog Consistency Test] (feat) add check for bin/opm files in filesystem #362
Conversation
camilamacedo86
changed the title
|
@camilamacedo86: This pull request references OPRUN-3869 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
c/c @joelanford wdyt? |
camilamacedo86
changed the title
camilamacedo86
changed the title
camilamacedo86
changed the title
|
@camilamacedo86: An error was encountered updating to the POST state for bug OCPBUGS-56818 on the Jira server at https://issues.redhat.com/. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Post "https://issues.redhat.com/rest/api/2/issue/17015461/transitions": POST https://issues.redhat.com/rest/api/2/issue/17015461/transitions giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/severity-important
|
@camilamacedo86: This pull request references Jira Issue OCPBUGS-56818, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (bandrade@redhat.com), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. This pull request references OPRUN-3869 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@camilamacedo86: This pull request references Jira Issue OCPBUGS-56818, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (bandrade@redhat.com), skipping review request. This pull request references OPRUN-3869 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@camilamacedo86: This pull request references Jira Issue OCPBUGS-56818, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (bandrade@redhat.com), skipping review request. This pull request references OPRUN-3869 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
camilamacedo86
force-pushed
the
opm-path
branch
2 times, most recently
from
003f374 to
9cfb5dc
Compare
| // 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.
@joelanford @bentito ^ see I hope that clarifies your previous comment.
The Go fs.FS doesn't support checking or following symlinks.
For all images we create a symlink for the opm.
I clarified it in the doc of the func as well.
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.
maybe it would be clearer to just not take the abstract FS as a parameter?
There was a problem hiding this comment.
maybe it would be clearer to just not take the abstract FS as a parameter?
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.
|
|
||
| if _, err := os.Stat(resolved); err != nil { | ||
| errs = append(errs, fmt.Errorf("symlink %q points to missing target %q: %w", rel, resolved, err)) | ||
| } |
There was a problem hiding this comment.
Sadly, my fist try to check if is an executable did not work out:
$ ls -la /var/folders/n4/j272tr6d7hq63mf7_skv6zr80000gn/T/oci-community-operator-index-2618243349/fs/bin/registry/opm
-rw------- 1 camilam staff 91558832 20 May 10:30 /var/folders/n4/j272tr6d7hq63mf7_skv6zr80000gn/T/oci-community-operator-index-2618243349/fs/bin/registry/opm
When we unpack the image we lose the permissions bits, see: https://github.com/openshift/operator-framework-operator-controller/blob/main/openshift/default-catalog-consistency/pkg/extract/extract.go#L181-L185
|
@camilamacedo86: This pull request references Jira Issue OCPBUGS-56818, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (bandrade@redhat.com), skipping review request. This pull request references OPRUN-3869 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
camilamacedo86
changed the title
| // FilesystemHasValidPathsOrLinks 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. |
There was a problem hiding this comment.
do we care if it points to a particular target or not? or only whether it exists?
There was a problem hiding this comment.
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.
do we care if it points to a particular target or not? or only whether it exists?
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?
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...?
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.
maybe doing something like: go version -m bin/opm ?
…k for executable files in filesystem Checks if given paths exist and point to executable files or valid symlinks.
| hdr.Gid = os.Getgid() | ||
| if hdr.FileInfo().IsDir() { | ||
| hdr.Mode = 0700 | ||
| hdr.Mode = 0755 |
There was a problem hiding this comment.
maybe just:
hdr.Mode = 0755
now that we're assigning the same mode for files and dirs?
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, perdasilva The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@camilamacedo86: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@camilamacedo86: Jira Issue OCPBUGS-56818: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-56818 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@camilamacedo86: new pull request created: #368 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[ART PR BUILD NOTIFIER] Distgit: ose-olm-catalogd |
|
[ART PR BUILD NOTIFIER] Distgit: ose-olm-operator-controller |
7 participants
Checks if given paths exist and point to executable files or valid symlinks.
See that the execution of the new tests added can be checked via the CI job.
P.S.: Tested with previous releases as well. All worked fine.
/cherrypick release-4.19 release-4.18 release-4.17