Skip to content

Conversation

@grokspawn
Copy link
Contributor

@grokspawn grokspawn commented Aug 30, 2022

Signed-off-by: Jordan jordan@nimblewidget.com

Description of the change:
create opm serve cache artifacts to serve for any uid by generating as 0777 (dirs) / 0666 (files).

Several options were evaluated, and we landed on a combo of relaxing the umask during cache generation coupled with updated permission of created artifacts to provide world read-/write-ability.

characterization URI verdict
original perms conflict docker.io/grokspawn/catalog-permissions:orig 0700/0600 FAIL
0770/0660 perms docker.io/grokspawn/catalog-permissions:permbump 0700/0600 FAIL
0777/0666 perms docker.io/grokspawn/catalog-permissions:permbumpall 0700/0600 FAIL
0777/0666 perms + umask docker.io/grokspawn/catalog-permissions:final-umask 0777/0666 SUCCESS
0750/0640 perms + umask docker.io/grokspawn/opm:umask-no-other 0750/0640 SUCCESS

To build a candidate opm replacement image, !!IN LINUX!! do

cd $your-operator-registry-clone
GOENV=CGO_ENABLED=0 make static 
cp bin/opm .
docker build -f release/goreleaser.opm/Dockerfile -t [tag] .
docker push [tag]

Then create the catalog image:

mkdir -p /tmp/ctest
cd /tmp/ctest
mkdir catalog
[path-to-opm] generate dockerfile catalog

and finally, edit the FROM line in the catalog.Dockerfile to point to the pushed tag for the opm replacement image.

Motivation for the change:
c.f. OCPBUGS-650.

Cache generation code was creating for uid 1001 and permissions of 0700 (dirs) / 0600 (files), so later deployment in a pod would fail unless uid matched.

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 openshift-ci bot requested review from exdx and jmrodri August 30, 2022 19:37
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 30, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grokspawn

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2022
@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #1018 (e21be48) into master (7470c35) will decrease coverage by 0.00%.
The diff coverage is 29.41%.

@@            Coverage Diff             @@
##           master    #1018      +/-   ##
==========================================
- Coverage   51.67%   51.66%   -0.01%     
==========================================
  Files         102      102              
  Lines        9153     9164      +11     
==========================================
+ Hits         4730     4735       +5     
- Misses       3515     3521       +6     
  Partials      908      908              
Impacted Files Coverage Δ
alpha/model/model.go 92.64% <ø> (ø)
alpha/property/property.go 92.68% <0.00%> (-4.76%) ⬇️
pkg/registry/query.go 61.48% <25.00%> (+0.26%) ⬆️
alpha/declcfg/declcfg_to_model.go 93.13% <100.00%> (+0.06%) ⬆️
alpha/declcfg/model_to_declcfg.go 100.00% <100.00%> (ø)
alpha/property/scheme.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@grokspawn
Copy link
Contributor Author

/hold @joelanford asked if we can umask+0750/0640 with effect in a cluster, so ima test

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2022
@grokspawn
Copy link
Contributor Author

grokspawn commented Aug 30, 2022

/hold cancel
umask+0750/0640 appears to work for any arbitrary uid

@bentito
Copy link
Contributor

bentito commented Aug 30, 2022

Yeah I was wondering same, are any lesser RWX for any of the Group and Other perms possible.

@joelanford
Copy link
Member

This does mean that if a non-owner encounters a stale cache, they'll fail instead of repopulating the cache because they won't be able to delete the existing stale files. IMO, that's reasonable though. Outcome is "file/dir owner is responsible for making sure cache is not stale".

@grokspawn
Copy link
Contributor Author

As there was previously a 'user is root' implicit aspect in the past (which has been replaced with never-be-root-if-you-can-help-it), there appears to be a 'group is root' implicit aspect presently which allows other to have no privileges.

If/when that changes, this will also need to change.

@joelanford
Copy link
Member

https://github.com/operator-framework/operator-registry/runs/8100687382?check_suite_focus=true#step:4:155

Oh joy! Looks like we get to experience the fun of using a windows-specific source code file to do something different than syscall.Umask (which does not exist in Windows). My guess is that we'll just make that a no-op on Windows.

@grokspawn grokspawn force-pushed the serve-cache-perms branch 5 times, most recently from c314dbf to d50f149 Compare August 30, 2022 21:55
Signed-off-by: Jordan <jordan@nimblewidget.com>
@grokspawn grokspawn removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2022
@perdasilva
Copy link
Contributor

/hold in case you agree with my nit

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2022
@perdasilva
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2022
Signed-off-by: Jordan <jordan@nimblewidget.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2022
@perdasilva
Copy link
Contributor

/lgtm

@perdasilva
Copy link
Contributor

/hold cancel

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 31, 2022
@openshift-merge-robot openshift-merge-robot merged commit ce3bb44 into master Aug 31, 2022
@grokspawn grokspawn deleted the serve-cache-perms branch August 31, 2022 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants