allow review endpoints on missing namespaces #11321
Conversation
| @@ -205,6 +205,7 @@ var accessReviewResources = map[unversioned.GroupResource]bool{ | |||
| unversioned.GroupResource{Group: "", Resource: "resourceaccessreviews"}: true, | |||
| unversioned.GroupResource{Group: "", Resource: "localresourceaccessreviews"}: true, | |||
| unversioned.GroupResource{Group: "", Resource: "selfsubjectrulesreviews"}: true, | |||
| unversioned.GroupResource{Group: "", Resource: "subjectrulesreviews"}: true, | |||
There was a problem hiding this comment.
@mfojtik we need to fix the need to add to two lists.
There was a problem hiding this comment.
@deads2k can we generate this list from the upper list?
There was a problem hiding this comment.
@deads2k can we generate this list from the upper list?
With enough restructuring, we might be able to use the upstream admission plugin. Right now, no.
|
[test] |
|
|
||
| os::test::junit::declare_suite_start "cmd/projects/lifecycle" | ||
| # resourceaccessreview | ||
| os::cmd::expect_success 'oc policy who-can get pods -n missing-ns' |
There was a problem hiding this comment.
This is a weak test
| # selfsubjectaccessreview | ||
| os::cmd::expect_success 'oc policy can-i get pods -n missing-ns' | ||
| # selfsubjectrulesreivew | ||
| os::cmd::expect_success 'oc policy can-i --list -n missing-ns' |
There was a problem hiding this comment.
This is a weak test
Non-zero return checks exactly what need. Did this fail? No? Ok, success. The value returned doesn't matter. |
|
Why not check that the rules review correctly finds nothing for the missing namespace? The CLI could exit with success and do the wrong thing ... |
Because it in most cases, the answer isn't nothing. |
8a94479 to
e6d056f
Compare
|
yum re[test] |
|
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9920/ flaked on #11240 re[test] |
| authorizationapi.Resource("subjectaccessreviews"): true, | ||
| authorizationapi.Resource("localsubjectaccessreviews"): true, | ||
| authorizationapi.Resource("selfsubjectrulesreviews"): true, | ||
| authorizationapi.Resource("subjectrulesreviews"): true, |
There was a problem hiding this comment.
subjectrulesreviews isn't in the upstream set of accessReviewResources, need a carry commit to add it there
|
@mfojtik look good to you otherwise? if so, I'll update and merge. |
|
@deads2k looks good |
e6d056f to
e4c061f
Compare
|
[merge] |
|
yum re[test] re[merge] |
|
Evaluated for origin merge up to e4c061f |
|
re[test] |
|
Evaluated for origin test up to e4c061f |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10126/) (Base Commit: 92a71d4) |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10204/) (Base Commit: a941850) (Image: devenv-rhel7_5196) |
fixes #11300