PSP reviews: client #12478

sdminonne · 2017-01-13T09:08:31Z

@soltysh Thanks to have a look. For all-service-accounts flags I'm going to propose a fix for the API (working on it).

mfojtik

CC @openshift/cli-review

mfojtik · 2017-01-13T13:07:53Z

pkg/cmd/cli/describe/printer.go

+
+func printPspSubjectReview(pspSubjectReview *securityapi.PodSecurityPolicySubjectReview, w io.Writer, options kctl.PrintOptions) error {
+	if pspSubjectReview.Status.AllowedBy != nil {
+		if _, err := fmt.Fprintf(w, "%s\n", pspSubjectReview.Status.AllowedBy.Name); err != nil {


you can use fmt.Fprinln()

mfojtik · 2017-01-13T13:08:33Z

pkg/cmd/cli/describe/printer.go

+
+func printPspSelfSubjectReview(pspSelfSubjectReview *securityapi.PodSecurityPolicySelfSubjectReview, w io.Writer, options kctl.PrintOptions) error {
+	if pspSelfSubjectReview.Status.AllowedBy != nil {
+		if _, err := fmt.Fprintf(w, "%s\n", pspSelfSubjectReview.Status.AllowedBy.Name); err != nil {


mfojtik · 2017-01-13T13:09:51Z

pkg/cmd/cli/describe/printer.go

@@ -1131,3 +1139,36 @@ func printRoleBindingRestrictionList(list *authorizationapi.RoleBindingRestricti
 	}
 	return nil
 }
+
+//pspReviewColumns = []string{"SERVICE ACCOUNT", "ALLOWED BY"}


is this comment related to the function?

mfojtik · 2017-01-13T13:11:22Z

pkg/cmd/admin/policy/psp_review.go

+	return cmd
+}
+
+func returnPodTemplateSpec(obj runtime.Object) (*kapi.PodTemplateSpec, error) {


@smarterclayton don't we have this as helper in kube or origin (for things like oc set image)

We do - it's on the factory.

It's in

origin/pkg/api/meta/pods.go

Line 52 in 283a990

func GetPodSpec(obj runtime.Object) (*kapi.PodSpec, *field.Path, error) {

actually.

@soltysh that func returns a PodSpec while here we need a PodTemplateSpec (with the right meta) In factory as pointed out by @smarterclayton I found this
My only concern is that upstream AttachablePodForObject
invoked here
does not handle batch.CronJob and apps.StatefulSet which I would say it's a bug...
I'm going to submit an upstream PR if you agree.

mfojtik · 2017-01-13T13:12:43Z

pkg/cmd/admin/policy/psp_review.go

+func NewCmdReview(name, fullName string, f *clientcmd.Factory, out io.Writer) *cobra.Command {
+	o := &reviewOptions{}
+	cmd := &cobra.Command{
+		Use:     fmt.Sprintf("%s", name),


no need for Sprintf

mfojtik · 2017-01-13T13:14:06Z

pkg/cmd/admin/policy/psp_review.go

+		return fmt.Errorf("unable to find a resource: %v", err)
+	}
+	if len(infos) != 1 {
+		return fmt.Errorf("expected a resource for path: %s", o.Filename)


expected a single resource?

I raised the point int here and we agreed that only one resource should be handled

Hmmm... that's not quite how I'm reading this. I'd say we want to be able to pass a list of resources and handle that. Generally, this entire code should go into Run. Not quite sure what to do with PrintObject, yet, but I'll get to that.

What if a user pass a directory with multiple files, we want to handle those.

So we're going to handle -R handling recursion as well?

That's what I understood from the discussion we've had the other day.

OK, I understood exactly the opposite. Only one resource and no -R :)
Anyway, reworking it to have multiple resources.

mfojtik · 2017-01-13T13:14:38Z

pkg/cmd/admin/policy/psp_review.go

+	if err != nil {
+		return fmt.Errorf("unable to obtain client: %v", err)
+	}
+	o.client = oclient


o.client, _, err := f.Clients()
if err != nil {}

mfojtik · 2017-01-13T13:15:02Z

pkg/cmd/admin/policy/psp_review.go

+	The Pod is inferred from the PodTemplateSpec in the provided resource.
+	If no ServiceAccount is provided the ServiceAccount specified in podTemplateSpec.spec.serviceAccountName is used,
+	unless the podTemplateSpec.spec.serviceAccountName is empty, in which case "default" is used.
+	If ServiceAccounts are provided the podTemplateSpec.spec.serviceAccountName is ignored.`)


can you add newline here?

mfojtik · 2017-01-13T13:16:16Z

pkg/cmd/admin/policy/psp_review.go

+
+// Related to PodSecurityPolicyReview
+var (
+	reviewLong = templates.LongDesc(`Checks which ServiceAccount can create a Pod.


Checks which Service Account can create a Pod

Move to separate file, please. And like mentioned before, try to keep the flow similar to how other commands are.

mfojtik · 2017-01-13T13:16:45Z

pkg/cmd/admin/policy/psp_review.go

+		},
+	}
+
+	cmd.Flags().StringVarP(&o.User, "user", "u", o.User, "User")


maybe add more description to "User"?

sdminonne · 2017-01-13T15:10:14Z

@mfojtik thanks!. Going to wait for Clayton feedback and I'll modify accordingly

soltysh · 2017-01-16T12:22:43Z

(5) Create CLI for SCC access check

soltysh

Left you some comments.

soltysh · 2017-01-16T12:18:34Z

pkg/cmd/admin/policy/psp_review.go

+
+// PodSecurityPolicySubjectReview and PodSecurityPolicySelfSubjectReview
+var (
+	subjectReviewLong = templates.LongDesc(`Check whether a User, ServiceAccount, Group can create a Pod.


a User, ServiceAcount, or a Group...

soltysh · 2017-01-16T12:20:00Z

pkg/cmd/admin/policy/psp_review.go

+// PodSecurityPolicySubjectReview and PodSecurityPolicySelfSubjectReview
+var (
+	subjectReviewLong = templates.LongDesc(`Check whether a User, ServiceAccount, Group can create a Pod.
+	If "User" is specified but not "Group", the it is interpreted as "What if User were not member of any groups".


~~the~~it is interpreted as "What if User is not a member of any group"

and I wouldn't quote User nor Group.

soltysh · 2017-01-16T12:20:54Z

pkg/cmd/admin/policy/psp_review.go

+var (
+	subjectReviewLong = templates.LongDesc(`Check whether a User, ServiceAccount, Group can create a Pod.
+	If "User" is specified but not "Group", the it is interpreted as "What if User were not member of any groups".
+	If "User" and "Groups" are empty, then the check is performed using *only* the ServiceAccountName in the PodTemplateSpec`)


If User and Group is empty, the check is ...

Btw. iirc from the discussion we've had in the trello, not specifying user nor group would result in SelfSubjectReview, which means against current user, has something changed here?

soltysh · 2017-01-16T12:27:34Z

pkg/cmd/admin/policy/psp_review.go

+	return nil
+}
+
+func NewCmdSubjectReview(name, fullName string, f *clientcmd.Factory, out io.Writer) *cobra.Command {


<me ranting>
It's nice and readable to have the NewCmd* function right after *Options structure.
</me ranting>

soltysh · 2017-01-16T12:28:59Z

pkg/cmd/admin/policy/psp_review.go

+				kcmdutil.CheckErr(kcmdutil.UsageError(cmd, err.Error()))
+			}
+			err := o.Run(out)
+			kcmdutil.CheckErr(err)


The body of this method should go towards:

kcmdutil.CheckErr(opts.Complete(f, cmd, args, out)) kcmdutil.CheckErr(opts.Validate()) kcmdutil.CheckErr(opts.Run())

Not sure if you need Validate, yet ;)

soltysh · 2017-01-16T12:39:31Z

pkg/cmd/admin/policy/psp_review.go

+
+	response, err := o.client.PodSecurityPolicyReviews(o.Namespace).Create(review)
+	if err != nil {
+		return fmt.Errorf("unable to compute PSP rviews: %v", err)


typo: reviews, beside use full name Pod Security Policy Review

soltysh · 2017-01-16T12:40:18Z

pkg/cmd/admin/policy/psp_review.go

+	if err != nil {
+		return fmt.Errorf("unable to find a resource: %v", err)
+	}
+	if len(infos) != 1 {


Same limitation, we don't want to have.

soltysh · 2017-01-16T12:41:03Z

pkg/cmd/admin/policy/psp_review.go

+func NewCmdSubjectReview(name, fullName string, f *clientcmd.Factory, out io.Writer) *cobra.Command {
+	o := &subjectReviewOptions{}
+	cmd := &cobra.Command{
+		Use:     fmt.Sprintf("%s", name),


Same here about not using Sprintf.

soltysh · 2017-01-16T12:41:41Z

pkg/cmd/admin/policy/psp_review.go

+		Long:    reviewLong,
+		Example: fmt.Sprintf(reviewExamples, fullName),
+		Run: func(cmd *cobra.Command, args []string) {
+			if err := o.Complete(f, args, cmd); err != nil {


Now I can say validate is not needed, but the shape here should be:

kcmdutil.CheckErr(opts.Complete(f, cmd, args, out)) kcmdutil.CheckErr(opts.Run())

soltysh · 2017-01-16T12:42:15Z

pkg/cmd/admin/policy/psp_review.go

+	return cmd
+}
+
+func returnPodTemplateSpec(obj runtime.Object) (*kapi.PodTemplateSpec, error) {


It's in

origin/pkg/api/meta/pods.go

Line 52 in 283a990

func GetPodSpec(obj runtime.Object) (*kapi.PodSpec, *field.Path, error) {

actually.

sdminonne · 2017-01-20T16:37:18Z

@soltysh @mfojtik two commits one for each review. No generated code (anyway this must be squashed).
PTAL

soltysh

My comments apply to both commands, if mentioned only in one file :)

soltysh · 2017-01-23T14:32:58Z

pkg/cmd/cli/describe/printer.go

@@ -1131,3 +1139,34 @@ func printRoleBindingRestrictionList(list *authorizationapi.RoleBindingRestricti
 	}
 	return nil
 }
+
+func printPspReview(pspreview *securityapi.PodSecurityPolicyReview, w io.Writer, options kctl.PrintOptions) error {
+	for _, allowedSA := range pspreview.Status.AllowedServiceAccounts {


If we're not allowed, we need to say so. This applies to all three printPsp* methods.

btw. name methods, printPSPReview*, PSP here is an acronym, so it should be spelled like one.

soltysh · 2017-01-23T14:35:21Z

pkg/cmd/admin/policy/review.go

+	securityapi "github.com/openshift/origin/pkg/security/api"
+)
+
+// Related to PodSecurityPolicyReview


Comment not needed, since you split that to separate files.

soltysh · 2017-01-23T14:35:29Z

pkg/cmd/admin/policy/subject_review.go

+	securityapi "github.com/openshift/origin/pkg/security/api"
+)
+
+// PodSecurityPolicySubjectReview and PodSecurityPolicySelfSubjectReview


Same, not needed.

soltysh · 2017-01-23T14:36:12Z

pkg/cmd/admin/policy/review.go

+var (
+	reviewLong = templates.LongDesc(`Checks which Service Account can create a Pod.
+	The Pod is inferred from the PodTemplateSpec in the provided resource.
+	If no Service Account is provided the Service Account specified in podTemplateSpec.spec.serviceAccountName is used,


If not Service Account is provided the one specified in ...

unless ~~the podTemplateSpec.spec.serviceAccoutName~~ it is empty, ...

soltysh · 2017-01-23T14:40:34Z

pkg/cmd/admin/policy/review.go

+		Infos()
+	if err != nil {
+		return err
+	}


That block should be moved to Run, not needed here.

Or change it to return you just the builder object.

soltysh · 2017-01-23T14:46:31Z

pkg/cmd/admin/policy/review.go

+		if err != nil {
+			return err
+		}
+		o.printer = kubectl.NewVersionedPrinter(p, kapi.Scheme, version)


I'm not sure we need this block. If you care about raw output you can always hit the endpoint by yourself. This is a convenience command, that gives you nice output. Similarly to how oc import-image works. In that case just go with the describer and you'll be good.

Btw. you're missing the actual implementation of a PSP* describers in https://github.com/openshift/origin/blob/master/pkg/cmd/cli/describe/describer.go

sdminonne · 2017-01-24T14:44:23Z

@soltysh thanks for the review! PTAL

soltysh

Some minor nits in the form but most importantly I'm missing tests for this in test-cmd.sh. Make sure to have a few positive tests and negative, where you're trying to check an object that does not have PodTemplate.

soltysh · 2017-01-26T13:31:22Z

pkg/cmd/admin/policy/review.go

+	ServiceAccountNames []string
+
+	//AllServiceAccounts  bool TODO: defines a way in the API to defines all service accounts
+	approximatePodTemplateForObject func(runtime.Object) (*kapi.PodTemplateSpec, error)


I like to have the options nicely sorted external values, set by users (ServiceAccountNames, FilenameOptions in your case) and internal values. Reads better :-)

soltysh · 2017-01-26T13:32:37Z

pkg/cmd/admin/policy/subject_review.go

+	FilenameOptions         resource.FilenameOptions
+	User                    string
+	Groups                  []string
+	ServiceAccount          string


Same wrt to options.

soltysh · 2017-01-26T13:33:32Z

pkg/cmd/admin/policy/review.go

+		},
+	}
+
+	usage := "Filename, directory, or URL to a file identifying the resource to get from a server."


No need for value, just put into below function.

soltysh · 2017-01-26T13:37:08Z

pkg/cmd/admin/policy/subject_review.go

+	}
+
+	switch {
+	case len(userOrSA) > 0 || len(o.Groups) > 0:


I doubt you'll be doing both at the same time, either you do specify user and/or group or you don't and invoke self check. I'd split that into two methods one for each check. The code for the two can be shared start from line 148 until 154. The only difference will be setting .spec.User and/or .spec.Groups fields and the actual client calls. Printing should be shared as well.

soltysh · 2017-01-26T13:42:17Z

pkg/cmd/admin/policy/review.go

+	kcmdutil.AddFilenameOptionFlags(cmd, &o.FilenameOptions, usage)
+
+	cmd.Flags().StringSliceVarP(&o.ServiceAccountNames, "serviceaccounts", "s", o.ServiceAccountNames, "List of ServiceAccount names, comma separated")
+	// TODO: defines a way in the API to defines all service accounts: removing omitempty for PodSecurityPolicyReviewSpec.ServiceAccountNames?


You're missing kcmdutil.AddPrinterFlags(cmd).

soltysh · 2017-01-26T13:46:16Z

This doesn't seem to work as I expect:

$ oc policy review -f job.yaml
error:  "hello" cannot create pod: User "test-admin" cannot list all pods in the cluster

$ oc policy subject-review -f job.yaml
error:  "hello" cannot create pod: User "test-admin" cannot list all pods in the cluster

It looks from --loglevel=8 that it's trying to GET https://localhost:8443/api/v1/pods?labelSelector=%3Cnull%3E which it obviously fails to do.

soltysh · 2017-01-26T13:48:08Z

When I run this from cluster admin, I got this instead:

error:  "hello" cannot create pod: found '<', expected: !, identifier, or 'end of string'

My job.yaml for tests is:

apiVersion: batch/v1
kind: Job
metadata:
  name: hello
spec:
  template:
    metadata:
      name: hello
    spec:
      containers:
      - name: hello
        image: python:3.5.1
        command: ["python", "-c", "print('Hello world!')"]
      restartPolicy: Never

soltysh

Some more comments.

soltysh · 2017-02-02T14:22:39Z

pkg/cmd/admin/policy/subject_review.go

+)
+
+var (
+	subjectReviewLong = templates.LongDesc(`Check whether a User, Service Account or a Group can create a Pod.


Maybe a word or two that it returns a list of SCC that will allow creating those. I've struggled for a second what's the return value 😉

soltysh · 2017-02-02T14:23:46Z

pkg/cmd/admin/policy/review.go

+	FilenameOptions     resource.FilenameOptions
+	ServiceAccountNames []string
+
+	//AllServiceAccounts  bool TODO: defines a way in the API to defines all service accounts


IIRC, this is should be no more.

going to write a PR to remove in APIs as well.

API agreed, here should be removed within this PR.

soltysh · 2017-02-02T14:24:02Z

pkg/cmd/admin/policy/review.go

+
+	cmd.Flags().StringSliceVarP(&o.ServiceAccountNames, "serviceaccounts", "s", o.ServiceAccountNames, "List of ServiceAccount names, comma separated")
+	// TODO: defines a way in the API to defines all service accounts: removing omitempty for PodSecurityPolicyReviewSpec.ServiceAccountNames?
+	//cmd.Flags().BoolVar(&o.AllServiceAccounts, "allserviceaccounts", false, "If true, all accessible service accounts will be considered")


Same - remove.

soltysh · 2017-02-02T14:26:57Z

pkg/cmd/admin/policy/review.go

+}
+
+func (o *reviewOptions) Complete(f *clientcmd.Factory, args []string, cmd *cobra.Command, out io.Writer) error {
+	if len(args) == 0 && len(o.FilenameOptions.Filenames) == 0 {


You need something like that in subject_review.go

:\ ok, thanks

soltysh · 2017-02-02T14:36:21Z

pkg/cmd/cli/describe/printer.go

+
+func printPSPReview(pspreview *securityapi.PodSecurityPolicyReview, w io.Writer, options kctl.PrintOptions) error {
+	for _, allowedSA := range pspreview.Status.AllowedServiceAccounts {
+		allowedByName := "Not allowed"


Maybe <none> rather than 'Not allowed', so it's easier to spot.

soltysh · 2017-02-02T14:36:47Z

pkg/cmd/cli/describe/printer.go

+			return err
+		}
+	} else {
+		if _, err := fmt.Fprintln(w, "Not allowed"); err != nil {


Same <none> here and below. @fabianofranz wdyt?

soltysh · 2017-02-02T14:39:16Z

test/cmd/policy.sh

+os::cmd::expect_success_and_text 'oc policy subject-review -z default -g system:authenticated -f ${OS_ROOT}/test/testdata/job.yaml' 'restricted'
+os::cmd::expect_success 'oc create -f ${OS_ROOT}/test/testdata/scc_lax.yaml'
+os::cmd::expect_success "oc login -u bob -p bob"
+os::cmd::expect_success_and_text 'oc policy review -f ./test/testdata/job.yaml --no-headers=true' 'default   lax'


Use ${OS_ROOT}/test/testdata/job.yaml

soltysh · 2017-02-02T14:40:48Z

test/cmd/policy.sh

+os::cmd::expect_success "oc login -u bob -p bob"
+os::cmd::expect_success_and_text 'oc policy review -f ./test/testdata/job.yaml --no-headers=true' 'default   lax'
+os::cmd::expect_success "oc login -u system:admin -n '${project}'"
+os::cmd::expect_success 'oc delete project bob'


It would be nice if you could use different resources for each test, iow. job, deployment, just pod, etc.

sdminonne · 2017-02-03T10:48:09Z

[test]

soltysh · 2017-02-03T13:04:24Z

This LGTM, thank you Dario!

Now we need @openshift/cli-review approval for the cli part and @pweil- sign off, if we want to merge this for 1.5

sdminonne · 2017-02-03T16:32:38Z

@soltysh added code to verify statefulSet with volumeClaimTemplates.
cmd/policy.sh updated with an example of unsupported type

pweil- · 2017-02-07T13:34:11Z

bump @openshift/cli-review

pweil-

Also, please check godoc on all public methods.

pweil- · 2017-02-07T14:48:22Z

pkg/cmd/admin/policy/review.go

+	`)
+)
+
+const ReviewRecommendedName = "review"


these names may be confusing. Should we prefix with psp or scc (or register with both)?

I agree it would be good to prefix them with either, although I am not sure which prefix would best help describe this

psp may be confusing until it's turned on in OpenShift. I'd go with scc if we go with any prefix

Modified both to scc-review and scc-subject-review.
Data strcture changed as well

pweil- · 2017-02-07T14:52:59Z

test/cmd/policy.sh

@@ -97,6 +97,31 @@ os::cmd::expect_success_and_not_text 'oc policy can-i --list --groups system:una
 os::cmd::expect_success_and_not_text 'oc policy can-i --list --user harold --groups system:authenticated' 'get update.*imagestreams/layers'
 os::cmd::expect_success_and_text 'oc policy can-i --list --user harold --groups system:authenticated' 'create get.*buildconfigs/webhooks'

+os::cmd::expect_failure_and_text 'oc policy subject-review -f ${OS_ROOT}/test/testdata/pspreview_unsupported_statefulset.yaml' 'error: StatefulSet "rd" with spec.volumeClaimTemplates currently not supported.'


add tests for invalid input that is checked by Complete

juanvallejo · 2017-02-07T15:03:41Z

A few comments from @pweil- , but other than that this LGTM from a cli perspective

juanvallejo · 2017-02-07T15:11:39Z

pkg/cmd/admin/policy/review.go

+		ContinueOnError().
+		Flatten().
+		Do().
+		Infos()


Although I would not be opposed if not done this way, having this block return a resource.Result, and then using the result's .Visitor(func...) to iterate through each info would match how this is done in other commands.

sdminonne · 2017-02-08T08:34:29Z

@pweil- @juanvallejo thanks for the feedback, I'm modifying.

sdminonne · 2017-02-08T11:02:51Z

No squash and no code generated. As soon you're OK I'll follow up.

soltysh · 2017-02-08T15:23:39Z

@pweil- oh well, I'm getting old 😝

sdminonne · 2017-02-08T18:43:02Z

[test]

soltysh · 2017-02-09T09:34:49Z

@pweil- mering or leaving out for now?

pweil- · 2017-02-09T13:38:34Z

double checking priority of this with PM since it is low risk (new commands). My recommendation though is that we get this in a fork ami so it can be tested and merge to master AFTER the 3.5 cut.

soltysh

Comments from https://bugzilla.redhat.com/show_bug.cgi?id=1421570 and https://bugzilla.redhat.com/show_bug.cgi?id=1421616 needs addressing. One I've pointed above. The other is about printing resource file name or resource name (depending on what's doable) before printing the actual result of the check. This applies to both commands.

soltysh · 2017-02-15T10:57:07Z

pkg/cmd/admin/policy/review.go

+		},
+	}
+
+	cmd.Flags().StringSliceVarP(&o.ServiceAccountNames, "serviceaccounts", "s", o.ServiceAccountNames, "List of ServiceAccount names, comma separated")


Shorthand should be -z to be consistent with other commands where serviceaccounts are specified.

soltysh · 2017-02-15T11:03:14Z

I'm taking back my LGTM, until comments from BZ are fixed.

sdminonne · 2017-02-17T11:03:09Z

@soltysh PTAL

soltysh · 2017-02-17T13:12:03Z

Flake #11452.

[test]

soltysh · 2017-02-17T15:27:07Z

Actually that was #12995 and it happened again. I'm not gonna re-kick it.

soltysh · 2017-02-17T15:51:12Z

This looks LGTM.

soltysh · 2017-02-20T11:12:15Z

I'm re-running tests to get a clean pass. Previously it was flake #12995.

[test]

soltysh · 2017-02-21T14:44:37Z

@smarterclayton this will be merged when last issues is address, @sdminonne is currently working on a fix. Additionally, this will be cherry-picked into 1.5 release. So I'm removing your merge tag for now.

sdminonne · 2017-02-21T16:27:24Z

@soltysh: PTAL

soltysh · 2017-02-21T17:06:10Z

This LGTM. I'm spinning fork_ami and let qa test it.

soltysh · 2017-02-21T19:13:58Z

Flake #9490.

re-[test]

openshift-bot · 2017-02-21T19:21:32Z

Evaluated for origin test up to 20bcb5d

openshift-bot · 2017-02-21T20:49:05Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/400/) (Base Commit: 2d20080)

soltysh · 2017-02-22T05:39:13Z

Based on qa approval, and verbal approval from @pweil- and @mfojtik

[merge]

openshift-bot · 2017-02-22T05:45:26Z

Evaluated for origin merge up to 20bcb5d

openshift-bot · 2017-02-22T07:05:34Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/415/) (Base Commit: 0ea6f5d) (Image: devenv-rhel7_5954)

sdminonne force-pushed the SCC_check_client branch from f887fff to 912e7e9 Compare January 13, 2017 09:35

mfojtik suggested changes Jan 13, 2017

View reviewed changes

soltysh suggested changes Jan 16, 2017

View reviewed changes

sdminonne force-pushed the SCC_check_client branch from 39d93f8 to a72ddcd Compare January 20, 2017 16:36

soltysh reviewed Jan 23, 2017

View reviewed changes

soltysh suggested changes Jan 26, 2017

View reviewed changes

sdminonne force-pushed the SCC_check_client branch from af4ca23 to a1898d9 Compare February 1, 2017 11:06

soltysh suggested changes Feb 2, 2017

View reviewed changes

soltysh reviewed Feb 2, 2017

View reviewed changes

sdminonne force-pushed the SCC_check_client branch from a1898d9 to 81bdcca Compare February 3, 2017 10:37

soltysh approved these changes Feb 3, 2017

View reviewed changes

sdminonne force-pushed the SCC_check_client branch from 81bdcca to b350cd5 Compare February 3, 2017 16:29

pweil- suggested changes Feb 7, 2017

View reviewed changes

juanvallejo reviewed Feb 7, 2017

View reviewed changes

sdminonne force-pushed the SCC_check_client branch from b350cd5 to 2e32743 Compare February 8, 2017 11:02

soltysh suggested changes Feb 15, 2017

View reviewed changes

sdminonne force-pushed the SCC_check_client branch 2 times, most recently from 6ffc033 to eabb7e4 Compare February 17, 2017 11:02

soltysh approved these changes Feb 17, 2017

View reviewed changes

sdminonne added 2 commits February 21, 2017 11:38

PSP reviews: client

0f291fd

fix bugzilla 1421616 and 1421570

d8e75e3

sdminonne added 2 commits February 21, 2017 17:13

SCC review client: fix bugzilla 1424946

6b4edfe

SCC review client: generated code

20bcb5d

sdminonne force-pushed the SCC_check_client branch from eabb7e4 to 20bcb5d Compare February 21, 2017 16:26

openshift-bot merged commit 9f27ffa into openshift:master Feb 22, 2017

soltysh mentioned this pull request Feb 22, 2017

PSP reviews: client #13045

Merged

PSP reviews: client #12478

PSP reviews: client #12478

Conversation

sdminonne commented Jan 13, 2017

mfojtik left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdminonne commented Jan 13, 2017

soltysh commented Jan 16, 2017

soltysh left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdminonne commented Jan 20, 2017

soltysh left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdminonne commented Jan 24, 2017

soltysh left a 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

soltysh commented Jan 26, 2017

soltysh commented Jan 26, 2017

soltysh left a 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

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

soltysh left a comment •

edited

Loading