Bug 2070805: pkg/cvo/updatepayload: Restore shell for rm globbing#767
Conversation
Some background on recent changes: * 507b474 (pkg/cvo/updatepayload: Prune previous payload downloads, 2022-04-03, openshift#760) attempted to add CVO-side directory removal, but that failed because the CVO mounts the shared volume 'readOnly: true'. * a5af89d (pkg/cvo/updatepayload: Shift previous-download removal into the job, 2022-04-18, openshift#765) shifted removal into the job itself. As far as I can tell, this worked. * c45a981 (pkg/cvo/updatepayload: Use initContainers instead of shell &&-chains, 2022-04-20, openshift#765) addressed concerns with unquoted shell arguments by pivoting to initContainers and dropping the shell. This broke the * pathname expansion that rm depends on to find directories to remove. This commit returns to using the shell to invoke the rm call, so we get pathname expansion back [1]. But I avoid the possibility of unquoted argument injection by using workingDir to bring in baseDir. [1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_06
wking
changed the title
openshift-ci
Bot
added
bugzilla/severity-medium
|
@wking: This pull request references Bugzilla bug 2070805, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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/test-infra repository. |
openshift-ci
Bot
added
the
approved
|
@wking: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jottofar, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@wking: All pull requests linked via external trackers have merged:
Bugzilla bug 2070805 has been moved to the MODIFIED state. DetailsIn 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/test-infra repository. |
3 participants
Some background on recent changes:
507b474 (Bug 2070805: pkg/cvo/updatepayload: Prune previous payload downloads #760) attempted to add CVO-side directory removal, but that failed because the CVO mounts the shared volume
readOnly: true.a5af89d (Bug 2070805: pkg/cvo/updatepayload: Shift previous-download removal into the job #765) shifted removal into the job itself. As far as I can tell, this worked.
c45a981 (Bug 2070805: pkg/cvo/updatepayload: Shift previous-download removal into the job #765) addressed concerns with unquoted shell arguments by pivoting to
initContainersand dropping the shell. This broke the*pathname expansion thatrmdepends on to find directories to remove.This commit returns to using the shell to invoke the
rmcall, so we get pathname expansion back. But I avoid the possibility of unquoted argument injection by usingworkingDirto bring inbaseDir.