template broker should use SAR, not impersonation #14216

jim-minter · 2017-05-16T12:40:41Z

https://trello.com/c/ursxF5mB/1209-8-template-broker-should-use-sar-not-impersonation-template-broker

jim-minter · 2017-05-16T12:41:51Z

[test][testextended][extended:core(templates)]

jim-minter · 2017-05-16T14:22:58Z

@deads2k, @bparees I haven't done the oadm migration piece yet, but everything else should be here. I'd appreciate it if you could take an early look and provide any feedback.

bparees · 2017-05-16T16:29:04Z

so i think my open questions are around symmetry:

should the controller SA use per project bindings instead of a cluster role, for the impersonate power (to align w/ what the broker SA does)

or

could the broker SA be granted cluster-wide impersonate (to align with what the controller does) (the broker SA would still need to be granted resource creation permissions on a project by project basis, but at least the impersonate permission would be symmetrical).

deads2k · 2017-05-16T20:12:25Z

pkg/cmd/server/bootstrappolicy/policy.go

+				},
+				{
+					APIGroups: []string{templateapi.GroupName},
+					// "impersonate" is required for the API server to accept creation of


How about verb=assign, group=templateapi.GroupName resource=templateinstances . You could then bind that role locally in a namespace to allow subdivision or clusterwide if you (cluster-admin) don't care about subdivision.

deads2k · 2017-05-16T20:13:08Z

pkg/template/registry/templateinstance/strategy.go

-		if err != nil || resp == nil || !resp.Allowed {
+		if err := s.authorize(userinfo, &authorizationapi.Action{
+			Namespace: templateInstance.Namespace,
+			Verb:      "impersonate",


I'd prefer "assign". It's distinct and descriptive.

deads2k · 2017-05-16T20:13:49Z

pkg/client/cache/template.go

@@ -8,7 +8,8 @@ import (

 type StoreToTemplateLister interface {
 	List() ([]*templateapi.Template, error)
-	GetTemplateByUID(uid string) (*templateapi.Template, error)
+	ListByNamespace(namespace string) ([]*templateapi.Template, error)


You should really switch to the generated lister which should already have a per-namespace option.

deads2k · 2017-05-16T20:14:27Z

pkg/cmd/server/api/types.go

+	// TemplateServiceBrokerConfig holds information related to the template
+	// service broker.  The broker is enabled if TemplateServiceBrokerConfig is
+	// non-nil.
+	TemplateServiceBrokerConfig *TemplateServiceBrokerConfig


wasn't this in another pull? I feel like I already approved it.

yes - there are 2 commits here - the first is last my trello card from last sprint which is currently in the merge queue

deads2k · 2017-05-16T20:15:05Z

pkg/cmd/server/bootstrappolicy/constants.go

@@ -129,4 +130,6 @@ const (
 	BuildStrategyJenkinsPipelineRoleBindingName = BuildStrategyJenkinsPipelineRoleName + "-binding"

 	OpenshiftSharedResourceViewRoleBindingName = OpenshiftSharedResourceViewRoleName + "s"
+
+	TemplateServiceBrokerRoleBindingName = TemplateServiceBrokerRoleName + "-binding"


let's try to start matching upstream. The names match exactly upstream.

deads2k · 2017-05-16T20:15:56Z

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

@@ -91,6 +92,12 @@ const (

 	InfraGarbageCollectorControllerServiceAccountName = "garbage-collector-controller"
 	GarbageCollectorControllerRoleName                = "system:garbage-collector-controller"
+
+	InfraTemplateInstanceControllerServiceAccountName = "template-instance-controller"


if #14150 merges ahead of you, I'd like to use the new pattern. It aligns us with usptream and allows us to start splitting out ownership of launch mechanism and config information.

deads2k · 2017-05-16T20:17:02Z

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

+					// "impersonate" is required for the API server to accept updates to
+					// TemplateInstance objects where the requester username is not the
+					// API caller.
+					Verbs:     sets.NewString("get", "list", "watch", "update", "impersonate"),


This "impersonate" I didn't expect. There's a controller that's mutating ownership? Where it get information?

If its the updating detection server-side, that should be based on a change, not just that information being present.

If its the updating detection server-side, that should be based on a change, not just that information being present.

Oh, you're updating spec from the controller, not just status? I could see that being required then, but why update spec?

The controller updates status only. At the moment, things are set up such that you must be the requester or have "impersonate" to edit any part of the object, hence "impersonate" is required here. Would it be better to allow anyone to change the status? Or is it somehow possible to restrict things so that just the controller can update status, and this can be done without "impersonate"?

deads2k · 2017-05-16T20:18:53Z

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

+					APIGroups: []string{"*"},
+					// delete is needed to avoid error: "cannot set an ownerRef on a
+					// resource you can't delete"
+					Verbs:     sets.NewString("create", "delete"),


This is the controller that actually does the creation of resources from the template? He can't be allowed to have this permission. This would allow me to create oauthaccesstokens which I could use to create a super-user token and own the cluster.

You will have to enumerate.

deads2k · 2017-05-16T20:19:29Z

pkg/cmd/server/bootstrappolicy/policy.go

+				},
+				{
+					APIGroups: []string{kapi.GroupName},
+					Verbs:     sets.NewString("list"),


this looks unusual to me. No watch paired to list?

No - the template service broker only lists services in a namespace in response to an incoming bind call.

deads2k · 2017-05-16T20:21:12Z

pkg/cmd/server/bootstrappolicy/project_policy.go

+
+func GetBootstrapProjectRoleBindings(namespace string) []authorizationapi.RoleBinding {
+	return append(GetBootstrapServiceAccountProjectRoleBindings(namespace),
+		GetBootstrapTemplateServiceBrokerProjectRoleBindings(namespace)...)


Granting rights for an alpha feature? Can you separate this particular bit to a different pull? I'd expect it to be gated based on whether the feature is enabled or not, but I don't want to block progress on the rest of your pull.

deads2k · 2017-05-16T20:22:45Z

pkg/template/controller/templateinstance_controller.go

-	c := TemplateInstanceController{restconfig: restconfig, templateclient: templateclientset.NewForConfigOrDie(&restconfig).Template()}
+func NewTemplateInstanceController(restconfig *rest.Config) *TemplateInstanceController {
+	c := TemplateInstanceController{
+		oc:             client.NewOrDie(restconfig),


It's unusual to create your own client. Please take them as arguments.

To confirm: is the pattern that I should cause a templateapi client to be created somewhere higher up the call stack and pass this in with the oc & kc clients?

deads2k · 2017-05-16T20:23:03Z

pkg/template/controller/templateinstance_controller.go

+		oc:             client.NewOrDie(restconfig),
+		kc:             kclientsetinternal.NewForConfigOrDie(restconfig),
+		templateclient: templateclientset.NewForConfigOrDie(restconfig).Template(),
+	}
 	_, c.controller = cache.NewInformer(


I think a generated shared informer is more appropriate.

deads2k · 2017-05-16T20:23:30Z

pkg/template/controller/templateinstance_controller.go

 )

 type TemplateInstanceController struct {
-	restconfig     rest.Config
+	oc             *client.Client


Generated client.

Or a dynamic client. Probably a dynamic client I guess.

deads2k · 2017-05-16T20:24:31Z

pkg/template/controller/templateinstance_controller.go

@@ -100,25 +110,35 @@ func (c *TemplateInstanceController) handle(templateInstance *templateapi.Templa
 	return err
 }

+func (c *TemplateInstanceController) authorize(u user.Info, action *authorizationapi.Action) error {
+	sar := authorizationapi.AddUserToSAR(u, &authorizationapi.SubjectAccessReview{Action: *action})


Use the upstream subjectaccessreviews.authorization.k8s.io We're going to (someday) remove our endpoint.

deads2k · 2017-05-16T20:25:30Z

pkg/template/controller/templateinstance_controller.go

+	if err = c.authorize(u, &authorizationapi.Action{
+		Namespace: templateInstance.Namespace,
+		Verb:      "create",
+		Group:     templateapi.LegacyGroupName,


no legacy group use here. We should be switched over for a new feature.

deads2k · 2017-05-16T20:26:14Z

pkg/template/controller/templateinstance_controller.go

+		return nil
+	}
+	if err == nil {
+		err = errors.New(resp.Reason)


why not forbidden here too?

deads2k · 2017-05-16T20:26:22Z

pkg/template/controller/templateinstance_controller.go

@@ -100,25 +110,35 @@ func (c *TemplateInstanceController) handle(templateInstance *templateapi.Templa
 	return err
 }

+func (c *TemplateInstanceController) authorize(u user.Info, action *authorizationapi.Action) error {


right idea.

deads2k · 2017-05-16T20:26:51Z

pkg/template/controller/templateinstance_controller.go

+				Namespace: namespace,
+				Verb:      "create",
+				Group:     info.Mapping.GroupVersionKind.Group,
+				Resource:  info.Mapping.Resource,


make sure that this is never empty.

deads2k · 2017-05-16T20:27:02Z

pkg/template/controller/templateinstance_controller.go

-		Op: cmd.Create,
+		Op: func(info *resource.Info, namespace string, obj runtime.Object) (runtime.Object, error) {
+			if err = c.authorize(u, &authorizationapi.Action{
+				Namespace: namespace,


make sure this is never empty.

deads2k · 2017-05-16T20:27:38Z

pkg/template/registry/templateinstance/strategy.go

@@ -118,23 +119,30 @@ func SelectableFields(obj *templateapi.TemplateInstance) fields.Set {
 	return templateapi.TemplateInstanceToSelectableFields(obj)
 }

+func (s *templateInstanceStrategy) authorize(u user.Info, action *authorizationapi.Action) error {


copy/paste surprises me. Should be shareable, right?

Yes - but I don't know where - is somewhere in pkg/authorization that this could/should go? I'm sort of surprised there isn't one already.

deads2k · 2017-05-16T20:30:59Z

@liggitt can you think of a reasonable and thread-safe way to build a transport wrapper that takes a user and does the check on all requests rather than trying to catch them all?

deads2k · 2017-05-22T17:42:36Z

pkg/template/controller/templateinstance_controller.go

+
+		utilruntime.HandleError(fmt.Errorf("TemplateInstance status update failed (%d/%d): %v", try+1, updateRetries, err))
+
+		if apierrors.IsConflict(err) && try+1 < updateRetries {


This logic looks exactly like RetryOnConflict.

Also, it's a little unusual to bother with retrying on conflict instead of just re-running the controller func. Something really expensive happen before this? Your controller needs to be level driven anyway, so you must be able to re-run your controller sync method or you're inherently unstable on failures and the like.

Honestly, I think this functionality is inherently unstable on failures and the like. I think this is where an imperative world meets a level driven world. @bparees @smarterclayton how do you want to proceed?

@jim-minter so the big issue here is that if we retry the event, we'll get create failures because the objects already exist, right? But we should be able to tell that we created those objects based on their ownerrefs and ignore the create failures and proceed to the status update.

So i'm inclined to say we can just retry the event as @deads2k suggests (if we add the other logic to be more intelligent about handling create failures), unless i'm forgetting something?

deads2k · 2017-05-22T17:45:04Z

pkg/template/registry/templateinstance/strategy.go

+	return false
+}
+
+func (statusStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) {


you shouldn't have these in the interface since you only need an UpdateStrategy

deads2k · 2017-05-22T17:46:49Z

pkg/template/servicebroker/servicebroker.go

+	return b
+}
+
+func (b *Broker) waitForReady() error {


I expected this to be called from a filter function in your handler chain so that we could easily make sure the entire handler was covered.

deads2k · 2017-05-22T17:54:49Z

pkg/template/servicebroker/servicebroker.go

 }

-func NewBroker(restconfig restclient.Config, localSAR client.LocalSubjectAccessReviewsNamespacer, secretsGetter internalversion.SecretsGetter, informers shared.InformerFactory, namespaces []string) *Broker {
+func NewBroker(privrestconfig restclient.Config, privkc kclientset.Interface, infraNamespace string, informer templateinformer.TemplateInformer, namespaces []string) *Broker {
+	informer.Informer().AddIndexers(cache.Indexers{


Do this right where you create the informers. We really shouldn't ever create one without this and later we can update the generator to allow it.

deads2k · 2017-05-22T17:55:44Z

pkg/template/servicebroker/provision.go

-		if err == nil {
-			err = errors.New("forbidden")
+	// TODO: enable SAR for template - at the moment I think this doesn't work
+	// properly because group information isn't populated in u.


does this mean that your information flow is incomplete somewhere? That is a severe problem.

deads2k · 2017-05-22T17:57:47Z

enable SAR for template in pkg/template/servicebroker/ provision.go - currently blocked because requesting user's groups are not known at this point

You sent an email a while back describing the information flow. This makes it sound like information is being dropped somewhere and that's concerning. Group information for the same user may not be stable across multiple requests (different certificates as a for instance) and the Extra information is equally critical in openshift since tokens may be scope limited and that information must be passed through. Permission to create objects of type foo is not the same as being allowed to create them of type bar and that can be expressed via scopes.

@liggitt you familiar with the information flow here?

liggitt · 2017-05-22T18:24:54Z

enable SAR for template in pkg/template/servicebroker/ provision.go - currently blocked because requesting user's groups are not known at this point

You sent an email a while back describing the information flow. This makes it sound like information is being dropped somewhere and that's concerning. Group information for the same user may not be stable across multiple requests (different certificates as a for instance) and the Extra information is equally critical in openshift since tokens may be scope limited and that information must be passed through. Permission to create objects of type foo is not the same as being allowed to create them of type bar and that can be expressed via scopes.

@liggitt you familiar with the information flow here?

@jim-minter wasn't this the header (X-Broker-Originating-Identity, I think) that we discussed last week that was for platforms to pass platform-specific identity info to a broker? That could be a serialization of user.Info in our case. @pmorie do you know the status/timeframe for that?

jim-minter · 2017-05-22T20:55:31Z

@liggitt I wasn't present for any discussion on X-Broker-Originating-Identity. If this allows groups and extra to be passed in, that sounds good. Waiting on @pmorie for update on status/timeframe.

jim-minter · 2017-05-23T11:29:41Z

@bparees @deads2k all points resolved (apart from passing through groups/extra which will come with improvements to the broker API).
@bparees reckon this can merge today?

openshift-bot · 2017-05-23T16:05:48Z

Evaluated for origin test up to 1f0aba8

openshift-bot · 2017-05-23T16:05:58Z

Evaluated for origin testextended up to 1f0aba8

openshift-bot · 2017-05-23T16:45:41Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/465/) (Base Commit: 48e5e40) (Extended Tests: core(templates))

openshift-bot · 2017-05-23T17:44:55Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1641/) (Base Commit: 48e5e40)

bparees

I'm going to merge this because none of my comments should block it, but i'd like to see a follow up PR that addresses them and also:

general addition of godoc (i didn't call out all the places it's missing)
more logging (at level 2 or 4) in the controllers+broker api so we can debug request flows/failures.

bparees · 2017-05-23T17:42:15Z

pkg/template/api/helpers.go

@@ -45,5 +45,16 @@ func AddObjectsToTemplate(template *Template, objects []runtime.Object, targetVe
 	}

 	return nil
+}
+
+func FilterTemplateInstanceCondition(conditions []TemplateInstanceCondition, condType TemplateInstanceConditionType) []TemplateInstanceCondition {


bparees · 2017-05-23T17:46:06Z

pkg/template/controller/templateinstance_controller.go

+	provisionErr := c.provision(templateInstance)
+
+	templateInstance.Status.Conditions = templateapi.FilterTemplateInstanceCondition(templateInstance.Status.Conditions, templateapi.TemplateInstanceReady)
+	templateInstance.Status.Conditions = templateapi.FilterTemplateInstanceCondition(templateInstance.Status.Conditions, templateapi.TemplateInstanceInstantiateFailure)


can a templateinstance go from a Failed condition to Ready condition?

No. Is your opinion that these lines are overkill? I was wanting to guarantee that whatever these conditions may currently contain, subsequently they only contain the result of this process.

yeah i was wondering if they were, but i agree w/ the goal of certainty, so i think it's fine to leave them.

bparees · 2017-05-23T17:55:37Z

pkg/template/registry/templateinstance/strategy.go

-		if err != nil || resp == nil || !resp.Allowed {
+		if err := util.Authorize(s.kc.Authorization().SubjectAccessReviews(), userinfo, &authorization.ResourceAttributes{
+			Namespace: templateInstance.Namespace,
+			Verb:      "assign",


should be a constant?

Can do, but my impression was that this seems to match wider existing code style.

[shrug]. we can leave it, i just worry about someone getting the name wrong or typo'ing it somewhere.

bparees · 2017-05-23T17:56:26Z

pkg/template/registry/templateinstance/strategy.go

+			Verb:      "assign",
+			Group:     templateapi.GroupName,
+			Resource:  "templateinstances",
+		}); err != nil {
 			return field.ErrorList{field.Forbidden(field.NewPath("spec.impersonateUser"), "impersonation forbidden")}


s/spec.impersonateUser/spec.requester.username/ ?

doh - thanks

bparees · 2017-05-23T17:59:28Z

pkg/template/servicebroker/provision.go

 	"github.com/openshift/origin/pkg/openservicebroker/api"
 	templateapi "github.com/openshift/origin/pkg/template/api"
 )

-func (b *Broker) ensureSecret(impersonatedKC internalversion.SecretsGetter, namespace string, instanceID string, preq *api.ProvisionRequest, didWork *bool) (*kapi.Secret, *api.Response) {
+func (b *Broker) ensureSecret(u user.Info, namespace string, instanceID string, preq *api.ProvisionRequest, didWork *bool) (*kapi.Secret, *api.Response) {


bparees · 2017-05-23T18:00:08Z

pkg/template/servicebroker/provision.go

@@ -50,26 +67,44 @@ func (b *Broker) ensureSecret(impersonatedKC internalversion.SecretsGetter, name
 	return nil, api.InternalServerError(err)
 }

-func (b *Broker) ensureTemplateInstance(impersonatedTemplateclient internalversiontemplate.TemplateInterface, namespace string, instanceID string, template *templateapi.Template, secret *kapi.Secret, impersonate string, didWork *bool) (*templateapi.TemplateInstance, *api.Response) {
+func (b *Broker) ensureTemplateInstance(u user.Info, namespace string, instanceID string, template *templateapi.Template, secret *kapi.Secret, didWork *bool) (*templateapi.TemplateInstance, *api.Response) {


bparees · 2017-05-23T18:04:11Z

pkg/template/servicebroker/servicebroker.go

+		b.kc = kc
+		b.templateclient = templateclientset.NewForConfigOrDie(restconfig).Template()
+
+		glog.Infof("Template service broker: waiting for informer sync")


lower log level.

bparees · 2017-05-23T18:05:53Z

pkg/template/servicebroker/servicebroker.go

+}
+
+func (b *Broker) WaitForReady() error {
+	timer := time.NewTimer(10 * time.Second)


10s sounds short. how confident are we this will always be sufficient?

Any incoming TSB API request would be held up for maximally that length of time before giving up if the broker hasn't initialised. Initialisation is a one-off event at startup and should be rapid. 10s is in use elsewhere in our codebase in similar scenarios. At the moment we return status code 500 if the timer expires, arguably that should be a different code, but there's nothing to cover this in the OSB spec and I'm not aware of a specific standardised "try again later" status code to use.

I think I should add some code documentation here :)

ok, if this is a check that's being run on each request that's fine. I was under the (mistaken, I guess) impression that if we hit this timeout, the broker api wasn't going to start up at all.

yes; that is not the case.

bparees · 2017-05-23T18:06:13Z

pkg/template/servicebroker/servicebroker.go

 }

-func NewBroker(restconfig restclient.Config, localSAR client.LocalSubjectAccessReviewsNamespacer, secretsGetter internalversion.SecretsGetter, informers shared.InformerFactory, namespaces []string) *Broker {
+func NewBroker(privrestconfig restclient.Config, privkc kclientset.Interface, infraNamespace string, informer templateinformer.TemplateInformer, namespaces []string) *Broker {


bparees · 2017-05-23T18:11:05Z

test/extended/templates/templateinstance_security.go

@@ -109,14 +109,14 @@ var _ = g.Describe("[templates] templateinstance security tests", func() {
 				},
 			},
 			{
-				by:              "checking adminuser can create a privileged object",
+				by:              "checking adminuser can't create a privileged object",


this is probably worth a comment that the reason this should fail is because the controller itself should not have these permissions, even though the adminuser does.

bparees · 2017-05-23T18:14:07Z

[merge]

openshift-bot · 2017-05-23T18:30:07Z

Evaluated for origin merge up to 1f0aba8

openshift-bot · 2017-05-24T03:05:49Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/765/) (Base Commit: 50b7254) (Image: devenv-rhel7_6260)

smarterclayton · 2017-06-11T00:40:50Z

pkg/cmd/server/origin/master_config.go

 	informerFactory := shared.NewInformerFactory(internalkubeInformerFactory, externalkubeInformerFactory, privilegedLoopbackKubeClientsetInternal, privilegedLoopbackOpenShiftClient, customListerWatchers, defaultInformerResyncPeriod)

+	err = templateInformers.Template().InternalVersion().Templates().Informer().AddIndexers(cache.Indexers{


This causes the template informer to always be started, even if the template broker is off. This needs to be guarded.

jim-minter self-assigned this May 16, 2017

jim-minter force-pushed the trello132-broker-sar branch 2 times, most recently from ef8e182 to 2629ca7 Compare May 16, 2017 14:09

jim-minter force-pushed the trello132-broker-sar branch from 2629ca7 to c9689fc Compare May 16, 2017 16:27

deads2k reviewed May 16, 2017

View reviewed changes

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

jim-minter force-pushed the trello132-broker-sar branch from c9689fc to bf9b178 Compare May 19, 2017 13:31

deads2k reviewed May 22, 2017

View reviewed changes

jim-minter force-pushed the trello132-broker-sar branch from 9630180 to c4da547 Compare May 23, 2017 11:28

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

jim-minter force-pushed the trello132-broker-sar branch 2 times, most recently from 096db99 to 965e3ea Compare May 23, 2017 14:01

jim-minter removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2017

template broker should use SAR, not impersonation

1f0aba8

jim-minter force-pushed the trello132-broker-sar branch from 965e3ea to 1f0aba8 Compare May 23, 2017 16:01

jim-minter changed the title ~~WIP template broker should use SAR, not impersonation~~ template broker should use SAR, not impersonation May 23, 2017

bparees requested changes May 23, 2017

View reviewed changes

openshift-bot merged commit e4d7a02 into openshift:master May 24, 2017

jim-minter mentioned this pull request May 24, 2017

Template service broker documentation and logging updates #14318

Merged

smarterclayton reviewed Jun 11, 2017

View reviewed changes

smarterclayton mentioned this pull request Jun 11, 2017

Duplicate caches in OpenShift master #8229

Closed

jim-minter mentioned this pull request Jun 12, 2017

don't start template informer unless templateservicebroker is enabled #14579

Merged


		utilruntime.HandleError(fmt.Errorf("TemplateInstance status update failed (%d/%d): %v", try+1, updateRetries, err))

		if apierrors.IsConflict(err) && try+1 < updateRetries {

		informerFactory := shared.NewInformerFactory(internalkubeInformerFactory, externalkubeInformerFactory, privilegedLoopbackKubeClientsetInternal, privilegedLoopbackOpenShiftClient, customListerWatchers, defaultInformerResyncPeriod)

		err = templateInformers.Template().InternalVersion().Templates().Informer().AddIndexers(cache.Indexers{

template broker should use SAR, not impersonation #14216

template broker should use SAR, not impersonation #14216

Conversation

jim-minter commented May 16, 2017

jim-minter commented May 16, 2017

jim-minter commented May 16, 2017

bparees commented May 16, 2017

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

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 May 16, 2017

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 May 22, 2017

liggitt commented May 22, 2017

jim-minter commented May 22, 2017

jim-minter commented May 23, 2017

openshift-bot commented May 23, 2017

openshift-bot commented May 23, 2017

openshift-bot commented May 23, 2017

openshift-bot commented May 23, 2017

bparees 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

bparees commented May 23, 2017

openshift-bot commented May 23, 2017

openshift-bot commented May 24, 2017 • edited Loading

Choose a reason for hiding this comment

openshift-bot commented May 24, 2017 •

edited

Loading