Fix nil function in buildContext return values

[komish]

Description of the change:

The change sets a return value for cleanup (type func()) to a no-op at the top of the function logic. This helps prevent a situation where returning before setting that function in buildContext() can cause a <nil> to be returned for that value which throws an error at runtime when called.

Concrete Context

During an opm index prune -

In pkg/lib/indexer/indexer.go:

func (i ImageIndexer) PruneFromIndex(request PruneFromIndexRequest) error {
    buildDir, outDockerfile, cleanup, err := buildContext(request.Generate, request.OutDockerfile)
    defer cleanup()
    if err != nil {
        return err
    }

    // ...

If the user has not passed --generate to opm index prune AND they cannot write to the underlying directory, buildContext(...) runs into an error trying to TempDir(...):

func buildContext(generate bool, requestedDockerfile string) (buildDir, outDockerfile string, cleanup func(), err error) {
    if generate {
        ...
    }

    // set a temp directory for building the new image
    buildDir, err = ioutil.TempDir(".", tmpBuildDirPrefix)     // error is here
    if err != nil {
        return
    }
    cleanup = func() {
        os.RemoveAll(buildDir)
    }

Up until that point cleanup hasn't been defined, so it's <nil>. Then, buildContext completes and the next line of PruneFromIndex calls defer cleanup() (i.e. defer <nil>()) which causes the panic.

Motivation for the change:

https://bugzilla.redhat.com/show_bug.cgi?id=1943284 - While needing write permissions is fair, other commands return a permission denied (bubbling up from ioutil.TempDir). I think that was probably the intention for this command as well.

Reviewer Checklist

• Implementation matches the proposed design, or proposal is updated to match implementation
• Sufficient unit test coverage
• Sufficient end-to-end test coverage
• Docs updated or added to /docs
• Commit messages sensible and descriptive

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: komish
To complete the pull request process, please assign ecordell after the PR has been reviewed.
You can assign the PR to them by writing /assign @ecordell in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

Hi @komish. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[codecov]

Codecov Report

Merging #610 (68ad13d) into master (607d373) will increase coverage by 2.73%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #610      +/-   ##
==========================================
+ Coverage   48.80%   51.53%   +2.73%     
==========================================
  Files          90      122      +32     
  Lines        6276    10542    +4266     
==========================================
+ Hits         3063     5433    +2370     
- Misses       2528     4097    +1569     
- Partials      685     1012     +327     

Impacted Files	Coverage Δ
pkg/lib/indexer/indexer.go	11.81% <0.00%> (+4.88%)	⬆️
pkg/sqlite/db_options.go	40.00% <0.00%> (-10.00%)	⬇️
pkg/registry/imageinput.go	69.23% <0.00%> (-0.27%)	⬇️
pkg/registry/empty.go	0.00% <0.00%> (ø)
pkg/sqlite/deprecate.go	0.00% <0.00%> (ø)
...qlite/migrations/006_associate_apis_with_bundle.go	60.57% <0.00%> (ø)
internal/declcfg/write.go	71.69% <0.00%> (ø)
internal/property/errors.go	0.00% <0.00%> (ø)
internal/action/init.go	100.00% <0.00%> (ø)
internal/property/property.go	97.81% <0.00%> (ø)
... and 42 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 607d373...68ad13d. Read the comment docs.

[komish]

I don't believe similar PR #583 addresses this issue (though it addresses another that I ran into as well, so that's awesome).

[tonyskapunk]

fwiw, I tested your change @komish and is showing a more clear error message:

$ REGISTRY_AUTH_FILE=/path/to/pull-secret.txt \
    ./bin/opm index prune \
    --from-index registry.redhat.io/redhat/redhat-operator-index:v${VER} \
    --packages performance-addon-operator,sriov-network-operator,ptp-operator,kubevirt-hyperconverged \
    --tag ${LOCAL_REGISTRY}/olm/redhat-operator-index:v${VER}

INFO[0000] pruning the index                             packages="[performance-addon-operator sriov-network-operator ptp-operator kubevirt-hyperconverged]"
Error: mkdir index_build_tmp647216914: permission denied

$ echo $?
1

Thanks!

[timflannagan]

/ok-to-test

[ylamgarchal]

LGTM, thanks komish !
Any chances to create these temporary files in /tmp for example ?

[komish]

LGTM, thanks komish !
Any chances to create these temporary files in /tmp for example ?

@ylamgarchal I think there are some places where you can specify the destination dir (see: opm index export flags, which has --download-folder) that might be somewhat reusable, but I don't think this does exactly what you're asking for in this case.

Either way, might make sense to request the option as a new issue as there are probably multiple throughout the code base where tmp location to store files could be used.

Maintainers can probably comment further.

[joelanford]

/lgtm

Would it be possible to add a unit test to cover this case to prevent regressions?

[komish]

@joelanford sure thing. I'm targeting pkg/lib/indexer/indexer_test.go and I assume it's ok if I add some additional tests for the buildContext() function? If not, I can always roll the changes back and just include a basic test for the nil function being returned.

I'll push a commit shortly.

[komish]

/lgtm

Would it be possible to add a unit test to cover this case to prevent regressions?

Done. Added some basic testing for buildContext. I didn't have a reliable way to determine expected values for some of the things that are generated (e.g. ioutil.TempFiles) so I tried to account for that in some way by optionally skipping a test if the resulting value would have been generated.

In hindsight, I probably should have added these as a separate PR to prevent the need for re-reviewing. My apologies.

[timflannagan]

/lgtm

[ylamgarchal]

LGTM, thanks komish !
Any chances to create these temporary files in /tmp for example ?

@ylamgarchal I think there are some places where you can specify the destination dir (see: opm index export flags, which has --download-folder) that might be somewhat reusable, but I don't think this does exactly what you're asking for in this case.

Either way, might make sense to request the option as a new issue as there are probably multiple throughout the code base where tmp location to store files could be used.

Maintainers can probably comment further.

Hi komish, this temporary directory is used "internally" by the function and cleaned before the command ends. I don't think it makes sense to have such option explicitly from the command line. Maybe a solution would be from the ioutil.TempDir documentation:

If dir is the empty string, TempDir uses the default directory for temporary files (see os.TempDir).

wdyt

[komish]

Just noticing this still hasn't merge. Is there anything needed on my end with regards to this PR? @joelanford @timflannagan

[timflannagan]

@komish Neither Joe or I have approval permissions for this repository - you can find the list of people that do in the root OWNERS file. In the meantime, maybe @kevinrizza or @njhale might be able to take a look when they have a chance?

[komish]

@komish Neither Joe or I have approval permissions for this repository - you can find the list of people that do in the root OWNERS file. In the meantime, maybe @kevinrizza or @njhale might be able to take a look when they have a chance?

Understood, my apologies - pinged you both because of the listed Assignees. Thanks for looping in the right folks!

[timflannagan]

(Hopefully removing some of the expired context's)

/test all

[openshift-ci]

@timflannagan: No presubmit jobs available for operator-framework/operator-registry@master

In response to this:

(Hopefully removing some of the expired context's)

/test all

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/test-infra repository.

[awgreene]

/approve

[dinhxuanvu]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, dinhxuanvu, komish

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:

• OWNERS [dinhxuanvu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment