Skip to content
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

SCC: add {AllowedUnsafe,Forbidden}Sysctls #60

Merged
merged 2 commits into from
Jul 20, 2018

Conversation

stlaz
Copy link
Contributor

@stlaz stlaz commented Jun 25, 2018

This is a part of syncing with upstream which moved from {AllowedUnsafe,Forbidden}Sysctls annotations to their respective fields.

PTAL @php-coder @ingvagabund
CC @simo5

edit: openshift/origin#20151

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 25, 2018
@stlaz stlaz force-pushed the unsafe_sysctls branch 2 times, most recently from da655fb to b1603dc Compare June 25, 2018 11:44
@simo5
Copy link
Contributor

simo5 commented Jun 25, 2018

Please add a link in the commit and PR description to the upstream PR if you can (helps reviewing as well as gives reference). Other than that..
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2018
// AllowedUnsafeSysctls is a list of explicitly allowed unsafe sysctls, defaults to none.
// Each entry is either a plain sysctl name or ends in "*" in which case it is considered
// as a prefix of allowed sysctls. Single * means all unsafe sysctls are allowed.
// Kubelet has to whitelist all allowed unsafe sysctls explicitly to avoid rejection.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@openshift/api-review Should we modify the privileged SCC to allow all unsafe sysctls by default (like we did for allowedCapabilities: openshift/origin#12741)? In this case, we won't have a confusion when a cluster admin didn't have permissions for using usafe sysctls.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's what I would expect

@php-coder
Copy link
Contributor

@stlaz Thank you!

/lgtm

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2018
@php-coder
Copy link
Contributor

@stlaz 😉

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2018
@stlaz
Copy link
Contributor Author

stlaz commented Jun 27, 2018

@php-coder oh you monster!
Regenerated on top of the current current repo.

@deads2k
Copy link
Contributor

deads2k commented Jun 27, 2018

/hold

At this point the rebase is 6 tests from success and I think we've got all of them addressed (running CI now). When these are added into origin, you need to revert disable sysctl tests that don't support SCC. Needs to be reverted from openshift/origin#20033

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2018
@simo5
Copy link
Contributor

simo5 commented Jun 29, 2018

@deads2k is it ok to get this one in now ?

@liggitt
Copy link
Contributor

liggitt commented Jun 29, 2018

let's resolve the current 3 API changes not synced up in origin first (c.f. openshift/origin#19624 (comment))

@liggitt
Copy link
Contributor

liggitt commented Jul 3, 2018

1.11.0 rebase is landed, origin is synced up

is the origin PR ready to go once this is merged?

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2018
@stlaz
Copy link
Contributor Author

stlaz commented Jul 4, 2018

Hopefully it should be. We'll only need a openshift/client-go update prior to that, right?

@liggitt liggitt added this to the v3.11 milestone Jul 9, 2018
@liggitt
Copy link
Contributor

liggitt commented Jul 9, 2018

To ensure the origin PR is ready to merge as soon as this is merged:

  1. ensure this PR is based on latest openshift/api#master
  2. In your openshift/origin PR:
    1. ensure it is based on latest openshift/origin#master
    2. add a TMP commit that points glide.yaml at your fork/branch:
      - package: github.com/openshift/api
        repo:    https://github.com/<your fork, e.g. "stlaz">/api.git
        version: <branch of this PR, e.g. "unsafe_sysctls">
      
    3. update your bump(*) commit to include the result of running hack/update-deps.sh, which will pull in these changes
  3. make sure CI is green on your origin PR
  4. get LGTM on your origin PR

We will then merge this PR, and you will do the following:

  1. drop the TMP commit from your origin PR, pointing glide back at openshift/api#master
  2. rerun hack/update-deps.sh and update your bump(*) commit
  3. we'll then tag your origin PR for merge, and it can merge on green CI

@stlaz
Copy link
Contributor Author

stlaz commented Jul 10, 2018

Many thanks for clarifying the proper way of doing this 🙂 I'll try to rebase the PRs now plus do the changes, although if something fails, I'll probably only be able to get back to them at the beginning of the next week.

@stlaz
Copy link
Contributor Author

stlaz commented Jul 16, 2018

@liggitt When I follow your advice, a number (I'd say an unreasonable number) of dependencies get pulled at step 2, one of which corrupts the build, as seen in the origin PR. Am I doing something wrong?

@liggitt
Copy link
Contributor

liggitt commented Jul 16, 2018

@liggitt When I follow your advice, a number (I'd say an unreasonable number) of dependencies get pulled at step 2, one of which corrupts the build, as seen in the origin PR. Am I doing something wrong?

I checked out your origin branch, and ran

glide cc
hack/update-deps.sh

it included far fewer dependency changes than were in your PR... I updated the bump(*) commit and pushed the results to https://github.com/liggitt/origin/commits/stlaz-sysctl_promotion if you want to take a look

what version of glide are you running?

@stlaz
Copy link
Contributor Author

stlaz commented Jul 16, 2018

I tried doing the same (took a while), but the number of packages still seems to be the same. I'm running that in a clean repo (git clean -dfx), glide version is 0.13.1.

@stlaz
Copy link
Contributor Author

stlaz commented Jul 17, 2018

After a rebase on the current origin master, I am even getting errors when running ./hack/update-deps.sh after glide cc:

[ERROR]	Error scanning github.com/openshift/origin/pkg/oc/admin/policy: cannot find package "." in:
	/home/slaznick/go/src/github.com/openshift/origin/pkg/oc/admin/policy
...
[ERROR]	Failed to retrieve a list of dependencies: Error resolving imports
[ERROR] PID 21310: hack/update-deps.sh:37: `glide update --strip-vendor` exited with status 1.

That's both on Fedora with the upstream glide, and on Arch with its distribution specific glide (0.13.1-2).

I have both origin and kubernetes repositories git clone $URL to GOPATH being set to /home/<me>/go, in $GOPATH/src/github.com/openshift/origin and $GOPATH/src/k8s.io/kubernetes/ respectively.

I am pretty sure I must have forgotten something very crucial, I have no idea what that could be, though.

@deads2k
Copy link
Contributor

deads2k commented Jul 17, 2018

[ERROR] Error scanning github.com/openshift/origin/pkg/oc/admin/policy: cannot find package "." in:
/home/slaznick/go/src/github.com/openshift/origin/pkg/oc/admin/policy

That package doesn't exist. Try rebasing your pull on origin master and removing package refs that aren't present.

@simo5
Copy link
Contributor

simo5 commented Jul 17, 2018

git clean -f -x -d -f
then try again

@stlaz
Copy link
Contributor Author

stlaz commented Jul 18, 2018

Thanks. The actual workaround that helped me resolve my original issue with the great number of dependencies was to rm -rf $ORIGIN_GIT_ROOT/vendor/ and run ./hack/update-deps.sh afterwards (all credits for this go to Michal Fojtik). This did not work after the rebase, though.

Anyway, I fixed a wrong package reference in tools/testdebug/cmd/load_etcd.go, removed all of the vendor/ directory and finally got what I expected. Will update the origin PR once make update is done.

@stlaz
Copy link
Contributor Author

stlaz commented Jul 20, 2018

@liggitt I believe the requirements from your post #60 (comment) were now met by getting the LGTM label at the origin PR.

@liggitt
Copy link
Contributor

liggitt commented Jul 20, 2018

yes, thanks

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2018
@liggitt liggitt merged commit deb367a into openshift:master Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants