add some cmd-test for `oc policy scc-subject-review/scc-review` #16122

shiywang · 2017-09-04T02:28:02Z

this is a testcase enhance from OCP-12839
@soltysh @stuartchuan @akostadinov ptal

bparees · 2017-09-04T04:15:34Z

/unassign

shiywang · 2017-09-05T02:48:03Z

/test cmd

akostadinov · 2017-09-05T11:55:56Z

btw if only unit tests are added and they pass, but some other unit tests fail, then they should be ignored [1]. I'm not sure this is the case here. Just saying no need to retest until everything pass when only some unstable units are failing.

[1] or probably reported somewhere as flakes

danwinship · 2017-09-05T13:41:58Z

/unassign
/assign @stevekuznetsov
(I should remove myself from this OWNERS file... the patch looks fine to me, but I don't feel like I know test/cmd enough that I should be approving people's changes to it.)

stevekuznetsov · 2017-09-05T14:00:08Z

/lgtm
/retest

openshift-bot · 2017-09-05T18:09:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-09-06T08:16:09Z

/retest

Please review the full test history for this PR and help us cut down flakes.

shiywang · 2017-09-06T11:44:01Z

/test extended_conformance_gce

brenton · 2017-09-06T12:37:26Z

@deads2k, would you be able to give a quick review of this PR? QE would like to upstream some of their tests.

deads2k · 2017-09-06T12:41:16Z

test/cmd/policy.sh

@@ -105,6 +105,7 @@ os::cmd::expect_success_and_text 'oc policy can-i --list --user harold --groups

 os::cmd::expect_failure 'oc policy scc-subject-review'
 os::cmd::expect_failure 'oc policy scc-review'
+os::cmd::expect_failure 'oc policy scc-subject-review -u invalid --namespace=noexist'


The ones above failed on arg problems, but this one is probably failing on something else. Add a text check.

oh, sorry, I think this is just another arg check case, because inputfile in this command is required.

origin/pkg/oc/admin/policy/subject_review.go

Line 82 in 717e308

if len(args) == 0 && len(o.FilenameOptions.Filenames) == 0 {

deads2k · 2017-09-06T12:43:10Z

test/cmd/policy.sh

 os::cmd::expect_success 'oc create -f ${OS_ROOT}/test/testdata/scc_lax.yaml'
 os::cmd::expect_success "oc login -u bob -p bobpassword"
 os::cmd::expect_success_and_text 'oc policy scc-review -f ${OS_ROOT}/test/testdata/job.yaml --no-headers=true' 'Job/hello   default   lax'
 os::cmd::expect_success_and_text 'oc policy scc-review -z default  -f ${OS_ROOT}/test/testdata/job.yaml --no-headers=true' 'Job/hello   default   lax'
 os::cmd::expect_success_and_text 'oc policy scc-review -z system:serviceaccount:policy-second:default  -f ${OS_ROOT}/test/testdata/job.yaml --no-headers=true' 'Job/hello   default   lax'
 os::cmd::expect_success_and_text 'oc policy scc-review -f ${OS_ROOT}/test/extended/testdata/deployments/deployment-simple.yaml --no-headers=true' 'DeploymentConfig/deployment-simple   default   lax'
 os::cmd::expect_success_and_text 'oc policy scc-review -f ${OS_ROOT}/test/testdata/nginx_pod.yaml --no-headers=true' ''
+os::cmd::expect_failure_and_text 'oc policy scc-review -z default -f ${OS_ROOT}/test/testdata/job.yaml --namespace=no-exist' 'error: unable to compute Pod Security Policy Review for "hello": User "bob" cannot create podsecuritypolicyreviews in project "no-exist"'
+os::cmd::expect_failure_and_text 'oc policy scc-review -z default -f ${OS_ROOT}/test/testdata/pspreview_unsupported_statefulset.yaml' 'error: StatefulSet "rd" with spec.volumeClaimTemplates currently not supported.'


this looks like a bug. Any idea why we don't support it?

this looks like a bug. Any idea why we don't support it?

hmmm... maybe we don't know how to complete it.

It's fine, we don't support stateful sets, see line 109 and 112 for same errors.

It's fine, we don't support stateful sets, see line 109 and 112 for same errors.

Why don't we? Is not possible given the info?

Maybe at the time of writing it we didn't. It's worth double checking now.

Here's your answer:

origin/pkg/oc/admin/policy/review.go

Line 174 in 50094ed

// TODO remove this as soon upstream statefulSet validation for podSpec is fixed.

I'm checking this right now and will open a PR shortly.

It looks like it's still not fixed upstream, either: https://github.com/kubernetes/kubernetes/blob/ea017719e52138263cb6e82eff8652cdd465d322/pkg/apis/apps/validation/validation.go#L57 so I don't think we can fix it on our end as well.

@soltysh are you working on this? if not, I would like to take a look : )

Feel free to pick it up.

deads2k · 2017-09-06T12:44:49Z

@deads2k, would you be able to give a quick review of this PR? QE would like to upstream some of their tests.

Looks ok. Needs a squash and maybe a comment or two.

Seems like you might want your own files to make sure that they don't get scrubbed away by accident. Something you're an owner/approver on.

openshift-bot · 2017-09-06T16:20:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

stevekuznetsov · 2017-09-06T16:24:38Z

/lgtm cancel

shiywang · 2017-09-08T09:53:22Z

@soltysh @deads2k squashed, ptal, thanks

shiywang · 2017-09-11T02:20:37Z

hi @soltysh I met a really weird problem, in this line : https://github.com/openshift/origin/pull/16122/files#diff-9a8e8ef8bf78b44957b8d3a6c9163dbdR144

previously the failed error message was podsecuritypolicyreviews in project "no-exist" and it passed.
but yesterday it failed, and the error message change to podsecuritypolicyreviews in the namespace "no-exist"', should we use project instead of namespace, right ?

@akostadinov I think this is a typical reason why we should move those checks into upstream.

soltysh · 2017-09-11T15:03:36Z

but yesterday it failed, and the error message change to podsecuritypolicyreviews in the namespace "no-exist"', should we use project instead of namespace, right ?

Yes, that changed just recently.

soltysh · 2017-09-11T15:04:21Z

/assign

soltysh · 2017-09-11T15:04:50Z

/lgtm
/approve

shiywang · 2017-09-12T00:41:22Z

ping @stevekuznetsov for approval

stevekuznetsov · 2017-09-12T15:35:50Z

/approve

openshift-merge-robot · 2017-09-12T15:35:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shiywang, soltysh, stevekuznetsov

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

Needs approval from an approver in each of these OWNERS Files:

~~test/cmd/OWNERS~~ [stevekuznetsov]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-bot · 2017-09-12T21:33:32Z

/retest

Please review the full test history for this PR and help us cut down flakes.

shiywang · 2017-09-13T02:31:33Z

/retest

shiywang · 2017-09-13T07:31:01Z

/retest

shiywang · 2017-09-13T11:52:05Z

/retest

stevekuznetsov · 2017-09-13T14:49:00Z

/retest

soltysh · 2017-09-13T15:03:50Z

No need to retest, this is blocked on #16323

openshift-bot · 2017-09-14T05:49:32Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2017-09-14T08:56:48Z

Automatic merge from submit-queue

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 4, 2017

openshift-merge-robot assigned bparees and danwinship Sep 4, 2017

openshift-ci-robot unassigned bparees Sep 4, 2017

openshift-ci-robot assigned stevekuznetsov and unassigned danwinship Sep 5, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2017

deads2k reviewed Sep 6, 2017

View reviewed changes

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2017

openshift-merge-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 6, 2017

shiywang force-pushed the test-extend branch from 708719b to 410b7dc Compare September 8, 2017 09:52

shiywang force-pushed the test-extend branch from 410b7dc to 0c599b1 Compare September 10, 2017 08:38

add some policy cmd-test

79de996

shiywang force-pushed the test-extend branch from 0dff65b to 79de996 Compare September 11, 2017 02:14

openshift-ci-robot assigned soltysh Sep 11, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 12, 2017

openshift-merge-robot merged commit 8417ef5 into openshift:master Sep 14, 2017

shiywang deleted the test-extend branch September 14, 2017 10:04

add some cmd-test for oc policy scc-subject-review/scc-review #16122

add some cmd-test for oc policy scc-subject-review/scc-review #16122

Conversation

shiywang commented Sep 4, 2017 • edited by akostadinov Loading

bparees commented Sep 4, 2017

shiywang commented Sep 5, 2017

akostadinov commented Sep 5, 2017

danwinship commented Sep 5, 2017

stevekuznetsov commented Sep 5, 2017

openshift-bot commented Sep 5, 2017

openshift-bot commented Sep 6, 2017

shiywang commented Sep 6, 2017

brenton commented Sep 6, 2017

Choose a reason for hiding this comment

shiywang Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Sep 6, 2017

openshift-bot commented Sep 6, 2017

stevekuznetsov commented Sep 6, 2017

shiywang commented Sep 8, 2017

shiywang commented Sep 11, 2017 • edited Loading

soltysh commented Sep 11, 2017

soltysh commented Sep 11, 2017

soltysh commented Sep 11, 2017

shiywang commented Sep 12, 2017

stevekuznetsov commented Sep 12, 2017

openshift-merge-robot commented Sep 12, 2017

openshift-bot commented Sep 12, 2017

shiywang commented Sep 13, 2017

shiywang commented Sep 13, 2017

shiywang commented Sep 13, 2017

stevekuznetsov commented Sep 13, 2017

soltysh commented Sep 13, 2017

openshift-bot commented Sep 14, 2017

openshift-merge-robot commented Sep 14, 2017

add some cmd-test for `oc policy scc-subject-review/scc-review` #16122

add some cmd-test for `oc policy scc-subject-review/scc-review` #16122

shiywang commented Sep 4, 2017 •

edited by akostadinov

Loading

shiywang Sep 8, 2017 •

edited

Loading

shiywang commented Sep 11, 2017 •

edited

Loading