Scc check only api #8941

sdminonne · 2016-05-19T13:55:12Z

@deads2k ref #8199 and #8776
As discussed here this PR contains API/types only. Thanks to have a look

Main stuff:

internal types json tags removed
removed v1beta3
Added validation for new types and unit tests, and registered

deads2k · 2016-05-23T20:19:54Z

pkg/security/api/types.go

+}
+
+// PodSpecReviewResult contains information related the a specifc service account for the requested PodSpec.
+type PodSpecReviewResult struct {


I think you can use PodSpecSubjectReviewStatus in place of this type.

deads2k · 2016-05-23T20:26:55Z

@openshift/api-review roughly what we talked about here: https://github.com/openshift/origin/pull/8199/files

This is just the types, impl will come separately to make the review a bit easier. It requires some SCC refactors along the way.

sdminonne · 2016-05-24T08:24:28Z

@deads2k thanks waiting for api-review I'm modifying this with your feedback

sdminonne · 2016-05-26T11:53:08Z

@deads2k commited on top for clarity. PTAL

deads2k · 2016-05-26T12:14:37Z

gofmt hack/verify-gofmt.sh | xargs -n 1 gofmt -s -w

deads2k · 2016-05-26T12:20:13Z

pkg/scheduler/admission/podnodeconstraints/admission.go

@@ -77,6 +78,9 @@ var resourcesToCheck = map[unversioned.GroupResource]unversioned.GroupKind{
 // we choose not to handle in this plugin
 var resourcesToIgnore = []unversioned.GroupKind{
 	extensions.Kind("DaemonSet"),
+	securityapi.Kind("PodSecurityPolicySelfSubjectReview"), // TODO: should this go through admission?


Yeah, let's run these. The rejection messages will help users figure what's happening to their pods that are being created indirectly.

Yeah, let's run these. The rejection messages will help users figure what's happening to their pods that are being created indirectly.

I still see this as outstanding. You should just need to provide a way to get the podspec.

OK. I'm going to do. For some reasons I thought this could be done in the next PR.

OK. I'm going to do. For some reasons I thought this could be done in the next PR.

If its small, I'd like it here, if its not, we can defer. Just let me know.

deads2k · 2016-05-26T12:35:14Z

Looks pretty good to me.

@openshift/api-review comments on the types? I think I've talked myself into keep the *SubjectReview checks since there's no other way for a user to figure out if he can create a given pod without trying.

smarterclayton · 2016-05-26T15:43:19Z

pkg/security/api/v1/types.go

+	// PodSpec is the PodSpec to check.
+	PodSpec kapi.PodSpec `json:"podSpec"`
+
+	// User is the user you're testing for.


Be use to use lower case in the names - since the godoc is used in swagger here, you'll want to refer to the field name as it would be in json. I.e. user is the user you're testing for and if you specify "user" but not "group".

Done (but only for versioned .../v1/types.go).

sdminonne · 2016-05-27T15:46:15Z

PTAL

smarterclayton · 2016-05-27T19:20:29Z

pkg/security/api/types.go

+
+// PodSecurityPolicyReviewSpec defines specification for PodSecurityPolicyReview
+type PodSecurityPolicyReviewSpec struct {
+	// PodSpec is the PodSpec to check. The ServiceAccountName field is ignored for this check.


David and I discussed, I think we're ok on the semantics of:

If ServiceAccountNames is empty, the PodSpec ServiceAccountName is used, unless it's empty, in which case "default" is used instead. If ServiceAccountNames is specified, PodSpec ServiceAccountName is ignored. There should be a way to say "all service accounts", and it should be obvious as it would be fairly common.

Update this comment to match the one below for ServiceAccountNames. The ServiceAccountName in the podspec is sometimes used.

smarterclayton · 2016-05-27T19:21:09Z

Approved with those caveats.

sdminonne · 2016-05-29T20:13:01Z

@smarterclayton sorry only now I realized that I didn't modify the comment.
I'm going to do and I'll squash and rebase.

sdminonne · 2016-05-30T21:26:34Z

@deads2k @smarterclayton squashed and rebased

sdminonne · 2016-06-01T14:13:13Z

[test]

sdminonne · 2016-06-01T16:15:53Z

Failure not related to my code (pretty sure).

smarterclayton · 2016-06-01T16:42:35Z

You're hitting a merge queue break caused by today's change from schema1 to schema2 on the Docker hub.

sdminonne · 2016-06-01T18:20:10Z

Thanks Clayton, should I re-run it?

smarterclayton · 2016-06-01T18:28:00Z

We haven't fixed it yet.

deads2k · 2016-06-02T14:28:53Z

pkg/scheduler/admission/podnodeconstraints/admission_test.go

@@ -430,6 +430,7 @@ nodeSelectorLabelBlacklist:
 func TestResourcesToCheck(t *testing.T) {
 	known := knownResourceKinds()
 	detected := kindsWithPodSpecs()
+	//t.Errorf("Detected ->  %#v", detected)


deads2k · 2016-06-02T16:37:07Z

minor comments, update and squash.

sdminonne · 2016-06-03T08:54:13Z

@deads2k

cruft removed (sorry again)
comments updated
admission check not implemented in this PR (working on it).

Hope it's ok.

0xmichalis · 2016-06-03T09:17:55Z

pkg/security/api/types.go

+
+	// ServiceAccountNames is an optional set of ServiceAccounts to run the check with.
+	// If ServiceAccountNames is empty, the PodSpec ServiceAccountName is used,
+	// unless it's empty, in which case "default" is used instead.


this comment doesn't make a lot of sense

@Kargakis the idea is (if I'm not wrong)

Look a ServiceAccountNames: if there's some ServiceAccountNames then use them and stop

If ServiceAccountNames it's empty look at the PodSpec.ServiceAccount.

Is PodSpec.ServiceAccount NOT empty? Then use it and stop.

Otherwise use default.

Makes sense, thanks! I feel that the comment needs to be more clear though.

deads2k · 2016-06-03T12:53:01Z

lgtm [merge]

openshift-bot · 2016-06-03T14:10:31Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4432/) (Image: devenv-rhel7_4312)

deads2k · 2016-06-03T15:37:30Z

re[merge]

openshift-bot · 2016-06-03T15:40:32Z

Evaluated for origin merge up to c214793

deads2k · 2016-06-03T17:27:48Z

yum, re[test]

openshift-bot · 2016-06-03T17:30:21Z

Evaluated for origin test up to c214793

openshift-bot · 2016-06-03T18:38:31Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/4432/)

smarterclayton · 2016-07-18T20:06:35Z

pkg/security/api/types.go

+	// if ServiceAccountNames is empty, unless the PodSpec.ServiceAccountName is empty,
+	// in which case "default" is used.
+	// If ServiceAccountNames is specified, PodSpec.ServiceAccountName is ignored.
+	PodSpec kapi.PodSpec


I'd like to change this API prior to 1.3 to be PodTemplate, not PodSpec, which fixes our annotation problem and is consistent with all other "template" APIs.

php-coder · 2017-08-09T17:02:43Z

pkg/security/api/validation/validation_test.go

+		}
+	}
+
+	koCases := map[string]securityapi.PodSecurityPolicySelfSubjectReview{


I'm curious what ko means here? Captain Obvious?

it's not an OK it's a KO

deads2k reviewed May 23, 2016
View reviewed changes

deads2k added the needs-api-review label May 23, 2016

sdminonne force-pushed the SCC_check_only_API branch from 4f2d020 to 2f4122d Compare May 26, 2016 11:50

deads2k reviewed May 26, 2016
View reviewed changes

smarterclayton reviewed May 26, 2016
View reviewed changes

sdminonne force-pushed the SCC_check_only_API branch from 2f4122d to 0a77a95 Compare May 27, 2016 15:41

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2016

smarterclayton reviewed May 27, 2016
View reviewed changes

smarterclayton added api-approved and removed needs-api-review labels May 27, 2016

sdminonne force-pushed the SCC_check_only_API branch from 0a77a95 to d66728a Compare May 30, 2016 21:25

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2016

sdminonne force-pushed the SCC_check_only_API branch 2 times, most recently from 3d10dd4 to 5500275 Compare May 31, 2016 07:33

deads2k reviewed Jun 2, 2016
View reviewed changes

sdminonne force-pushed the SCC_check_only_API branch from 5500275 to 10ebc9a Compare June 3, 2016 08:52

SCC check: API and validation

c214793

0xmichalis reviewed Jun 3, 2016
View reviewed changes

sdminonne force-pushed the SCC_check_only_API branch from 10ebc9a to c214793 Compare June 3, 2016 12:50

openshift-bot merged commit 5c82a30 into openshift:master Jun 3, 2016

sdminonne mentioned this pull request Jun 6, 2016

SCC check API: add admission in podnodeconstraints #9173

Merged

smarterclayton reviewed Jul 18, 2016
View reviewed changes

php-coder reviewed Aug 9, 2017

View reviewed changes

Scc check only api #8941

Scc check only api #8941

Conversation

sdminonne commented May 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented May 23, 2016

sdminonne commented May 24, 2016

sdminonne commented May 26, 2016

deads2k commented May 26, 2016

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 May 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdminonne commented May 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented May 27, 2016

sdminonne commented May 29, 2016

sdminonne commented May 30, 2016

sdminonne commented Jun 1, 2016

sdminonne commented Jun 1, 2016

smarterclayton commented Jun 1, 2016

sdminonne commented Jun 1, 2016

smarterclayton commented Jun 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jun 2, 2016

sdminonne commented Jun 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jun 3, 2016

openshift-bot commented Jun 3, 2016 • edited Loading

deads2k commented Jun 3, 2016

openshift-bot commented Jun 3, 2016

deads2k commented Jun 3, 2016

openshift-bot commented Jun 3, 2016

openshift-bot commented Jun 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Jun 3, 2016 •

edited

Loading