templateservicebroker WIP #12953

jim-minter · 2017-02-14T15:58:09Z

@bparees @pmorie @smarterclayton initial review and comments on this would be welcome.

Currently the code is in a separate directory pkg/templateservicebroker, simply because that's how I started off. I now suppose that the vast majority of the code would be better off under pkg/template. I don't know if it's easier to start looking through this as it stands, or whether it'd be easier (less repeated boilerplate) if I did the merge before your initial review. LMK.

jim-minter · 2017-02-14T15:58:37Z

Also, I'm very interested in comment where there's // QUERY or // TODO in the codebase.

jim-minter · 2017-02-14T16:02:19Z

Also any suggestions on name changes. I'm supposing something like:

TemplateProvision -> TemplateInstance
UUIDMap -> TemplateServiceBrokerUUIDMap (?)

bparees · 2017-02-14T21:00:39Z

hack/update-generated-clientsets.sh

@@ -16,6 +16,7 @@ packages=(
  github.com/openshift/origin/pkg/route
  github.com/openshift/origin/pkg/sdn
  github.com/openshift/origin/pkg/template
+  github.com/openshift/origin/pkg/templateservicebroker


I agree w/ your suggestion that these resources (mapping, provision request) should move under the /template api.

bparees · 2017-02-14T21:05:16Z

pkg/templateservicebroker/api/types.go

+}
+
+type UUIDMapSpec struct {
+	TemplateProvisionRef kapi.ObjectReference


If, and it's probably a big if, we think that this is the only field we really need for UUIDMapping purposes, i think we should consider making UUIDMap a generic resource, rather than making it part of the template api. Then other brokers w/ a need for such mappings can also leverage it. (That would mean renaming the TemplateProvisionRef field to just Ref or something, of course).

Conversely, if we don't think that's a futureproof decision (we might later want to add template specific fields to the UUIDMap object) then I agree w/ your suggestion that we should name the resource object TemplateUUIDMap or something else more template specific, because otherwise oc get operations will be confusingly vague ("oc get uuidmaps").

I think the hope is that the use of this object goes away if the underlying API supports "contexts" (i.e. complex co-ordinates rather than just UUIDs) in the future.
My understanding from the call we had with @smarterclayton was that the same non-namespaced object kind used by the templateservicebroker shouldn't be reused elsewhere because it would blur the security boundaries between components.
But it would potentially be neat to have a generic mapping object, e.g.

Spec: From: Kind: TemplateBrokerUUID Value: <uuid> To: ObjectReference

sounds like this should be a TemplateServiceUUIDMapSpec then. (TemplateService because what we are creating from a provision request is, ostensibly, a service instance of type template)

bparees · 2017-02-14T22:34:00Z

pkg/cmd/server/origin/run_components.go

+
+func (c *MasterConfig) RunTemplateServiceBrokerControllers() {
+	go tsbcontroller.NewTemplateProvisionController(c.PrivilegedLoopbackKubernetesClientset, c.PrivilegedLoopbackOpenShiftClient, c.PrivilegedLoopbackKubeConfigPath).Run(utilwait.NeverStop)
+	go tsbcontroller.NewUUIDMapDeletionController(c.PrivilegedLoopbackOpenShiftClient).Run(utilwait.NeverStop)


probably just uuidmapcontroller as a name.

bparees · 2017-02-14T22:35:33Z

pkg/template/api/install/install.go

@@ -93,7 +93,7 @@ func newRESTMapper(externalVersions []unversioned.GroupVersion) meta.RESTMapper
 		worstToBestGroupVersions = append(worstToBestGroupVersions, externalVersions[i])
 	}

-	rootScoped := sets.NewString("SubjectAccessReview", "SelfSubjectAccessReview")
+	rootScoped := sets.NewString()


i don't understand the implications of this change (also not sure why something in here would need to be changing?)

I think I'm just fixing an old copy/paste mistake!

bparees · 2017-02-14T22:37:24Z

pkg/template/api/validation/validation.go

@@ -12,7 +12,7 @@ import (
 	unversionedvalidation "k8s.io/kubernetes/pkg/api/unversioned/validation"
 )

-var parameterNameExp = regexp.MustCompile(`^[a-zA-Z0-9\_]+$`)
+var parameterNameExp = regexp.MustCompile(`^[a-zA-Z0-9_]+$`)


was this broken before? what was the behavior? we need to be careful we're not making the parameter name field validation more restrictive. (I assume it's making it less restrictive by allowing _ when previously the escaping was doing who knows what)

Both work identically, but I believe the \ is a syntax error which is being tolerated by the Golang regex parser.

bparees · 2017-02-14T23:22:29Z

pkg/templateservicebroker/brokerapi/templateservicebroker/templateservicebroker.go

+	return &Broker{kc: kc, oc: oc}
+}
+
+// QUERY: does this need to be unique per cluster?


@bparees do you have any suggestions about where such a cluster-unique UUID should be generated and stashed?

no, i'd ask @deads2k if there is already a uuid for the cluster that we can leverage.

no, i'd ask @deads2k if there is already a uuid for the cluster that we can leverage.

@marun @derekwaynecarr What does kube federation use?

bparees · 2017-02-14T23:23:40Z

pkg/templateservicebroker/brokerapi/types.go

@@ -0,0 +1,146 @@
+package brokerapi


this seems like it belongs under a more generic path than templateservicebroker (presumably ultimately it will be provided by the brokerSDK though)

bparees · 2017-02-14T23:27:56Z

pkg/templateservicebroker/controller/templateprovision_controller.go

+		meta, _ := meta.Accessor(obj)
+		ref := meta.GetOwnerReferences()
+		ref = append(ref, metatypes.OwnerReference{
+			APIVersion: "v1",


obviously this can't stay hardcoded, but i'm not sure how we know what the version should be.

per the IRC discussion with @deads2k, this is supposed to be the apigroup value, not a version.

My takeaway from that conversation was that whatever goes in, it is going to have to change. I could do APIVersion: v1.SchemeGroupVersion.String() where v1 refers to github.com/openshift/origin/pkg/template/api/v1, but fundamentally that's just a complicated way of saying "v1".

bparees · 2017-02-14T23:28:44Z

pkg/templateservicebroker/controller/templateprovision_controller.go

+		return errs[0]
+	}
+
+	// TODO: rollback on error?


no, just mark the provision as failed and rely on the user(or service catalog?) to de-provision the failed service.

(for now at least)

bparees · 2017-02-14T23:30:11Z

pkg/templateservicebroker/controller/templateprovision_controller.go

+	}
+
+	// TODO: rollback on error?
+	// TODO: wait for the template to finish deploying?


what defines "finish"? if the template defines a buildconfig+deploymentconfig and we need the buildconfig to build, produce an image, and deploy that image, to be usable, are we going to try to codify that logic? presumably not, so unless we want to introduce a template-readiness-probe, i'm inclined to say "no".

jim-minter · 2017-02-15T12:08:46Z

jim-minter · 2017-02-23T19:54:20Z

@bparees @pmorie @smarterclayton @openshift/api-review review and comments on this PR would be welcome.

jim-minter · 2017-02-23T19:57:27Z

pkg/client/templatebrokeruuidmaps.go

+}
+
+// Delete deletes a templatebrokeruuidmap, returns error if one occurs.
+// QUERY: the opts *kapi.DeleteOptions here seems non-standard, is that OK?


request for comment

what's non-standard about it?

the addition of opts *kapi.DeleteOptions to the arguments - didn't think that seemed to appear elsewhere in pkg/client

probably just haven't needed to expose it before...seems fine.

yes, that's fine

jim-minter · 2017-02-23T20:01:26Z

pkg/template/api/validation/validation.go

+	return allErrs
+}
+
+func validateTemplateInstanceImpersonation(oc *client.Client, ctx kapi.Context, ti *api.TemplateInstance) field.ErrorList {


@deads2k this is where I'm trying to confirm that the current user has the right to impersonate ti.Spec.ImpersonateUser.

@deads2k this is where I'm trying to confirm that the current user has the right to impersonate ti.Spec.ImpersonateUser.

Validation should not rely on anything outside of the object itself. Otherwise you can get inconsistent results depends on when or where you run it.

jim-minter · 2017-02-23T20:08:37Z

pkg/template/servicebroker/servicebroker.go

+	return &Broker{kc: kc, oc: oc, lister: informers.Templates().Lister(), templateNamespace: templateNamespace}
+}
+
+// TODO: UUID this should be unique per cluster


@smarterclayton @bparees @deads2k @marun @derekwaynecarr I'm stuck on what the strategy should be for generating and storing a UUID for this that is different per installed cluster.

deads2k · 2017-02-23T20:21:34Z

pkg/template/api/validation/validation.go

@@ -51,3 +55,116 @@ func validateTemplateBody(template *api.Template) (allErrs field.ErrorList) {
 	allErrs = append(allErrs, unversionedvalidation.ValidateLabels(template.ObjectLabels, field.NewPath("labels"))...)
 	return
 }
+
+// ValidateTemplate tests if required fields in the Template are set.
+func ValidateTemplateInstance(oc *client.Client, ctx kapi.Context, ti *api.TemplateInstance) (allErrs field.ErrorList) {


Maybe this is different than what I'm expecting, but I expect everything in this package to be related to static validation of an object.

Validation packages cannot rely on clients or context. This looks a lot like an admission plugin.

yeah i think your concern is valid, this validation logic should just make sure the username is a valid value (fits whatever regex we enforce on usernames).

After that, if it turns out the user doesn't have the requisite permissions (or doesn't exist), we should find that out when we actually go to instantiate the template as the user, and when it fails, mark the provision request as failed.

@bparees this isn't checking if the impersonated user can instantiate arbitrary objects from the template (indeed, that is already done as you state). This is checking if the user creating the TemplateInstance object with a given ImpersonteUser has permissions to impersonate that user. I can move this to an admission plugin.

deads2k · 2017-02-23T20:22:49Z

pkg/template/api/validation/validation.go

+		return field.ErrorList{field.InternalError(field.NewPath("spec.impersonateUser"), errors.New("no user context available"))}
+	}
+
+	if ti.Spec.ImpersonateUser != user.GetName() {


I haven't seen the types, but you're going to need the full subject. user, groups, extra (we store scopes here). Otherwise your impersonation may fail to create objects they only have permission to access via group bindings.

I'd understood that I'd only receive a principal name (a username) from the servicebroker API caller (for example, it would be hard for the caller to enumerate group memberships to send them, and also hard for me to trust on receipt).

When I make API calls later impersonating that username, I understood that the ImpersonationFilter backend would look up the group memberships for the passed username dynamically at that point? Thus it is not necessary/appropriate to persist any group information at this stage?

Are you saying that a design whereby I only receive a username will not fly? If so, what things must I receive and is there a pre-existing type which encompasses them?

deads2k · 2017-02-23T20:23:43Z

pkg/template/api/validation/validation.go

+	if ti.Spec.ImpersonateUser != user.GetName() {
+		sar := authorizationapi.AddUserToSAR(user,
+			&authorizationapi.SubjectAccessReview{
+				Action: authorizationapi.Action{


Clever, reasonable, deserving of a comment and worthy of refactoring the API server filter to ensure consistency.

bparees · 2017-02-24T21:56:08Z

pkg/client/client.go

@@ -193,6 +195,16 @@ func (c *Client) Templates(namespace string) TemplateInterface {
 	return newTemplates(c, namespace)
 }

+// Templates provides a REST client for Templates


TemplateInstances

thanks - fixed

bparees · 2017-02-24T22:02:16Z

pkg/openservicebroker/route.go

+	ws.Route(ws.DELETE("/service_instances/{instance_id}").To(shim(deprovision)))
+	ws.Route(ws.GET("/service_instances/{instance_id}/last_operation").To(shim(lastOperation)))
+	ws.Route(ws.PUT("/service_instances/{instance_id}/service_bindings/{binding_id}").To(shim(bind)))
+	ws.Route(ws.DELETE("/service_instances/{instance_id}/service_bindings/{binding_id}").To(shim(unbind)))


are these routes protected by auth or not? we will ultimately need them to be protected by auth (and then the SC will need to have openshift credentials to invoke the broker). At least that is my understanding from @smarterclayton. Probably worth confirming with @pmorie.

Yes, these are protected by authn/authz, although I haven't currently defined a bootstrap ClusterRole or ClusterRoleBinding so currently OOTB only system:admin can call them. Should this be done?

yes i think we're going to want a specific set of SC credentials used to call this, and not give the SC system:admin credentials.

ClusterRole defined. I think setting up the user and binding is an install time activity.

Agree that we want auth on these routes. Unfortunately the API only supports basic auth at this point :(

You guys need David and Jordan to review how this is going to fit into the authz and authn stuff. I'm ok delaying slightly, but i really don't want two different ways to authorize service broker and regular APIs.

bparees · 2017-02-24T22:04:41Z

pkg/openservicebroker/validation.go

+			key != templateapi.ImpersonateUserParameterKey {
+			errors = append(errors, field.Invalid(field.NewPath("parameters."+key), key, fmt.Sprintf("does not match %v", validation.ParameterNameRegexp)))
+		}
+	}


consider making it an error if the impersonate or namespace keys are not present since we can't doing anything safe or sensible without them.

This is already done in the next layer down - I was trying to keep pkg/openservicebroker more generic.

bparees · 2017-02-24T22:05:32Z

pkg/openservicebroker/validation.go

+	for key := range breq.Parameters {
+		if !validation.ParameterNameRegexp.MatchString(key) {
+			errors = append(errors, field.Invalid(field.NewPath("parameters."+key), key, fmt.Sprintf("does not match %v", validation.ParameterNameRegexp)))
+		}


what parameters do we expect on a bind? what would we do with them?

perhaps it's an error if we get any parameters? either that or we should just ignore any parameters that are provided.

The next layer down currently rejects all binds with any parameters set.

bparees · 2017-02-24T22:07:35Z

pkg/template/api/v1/types.go

+	// status is ...
+	Status kapi.ConditionStatus `json:"status" protobuf:"bytes,2,opt,name=status"`
+	// lasttransitiontime is ...
+	LastTransitionTime unversioned.Time `json:"lastTransitionTime" protobuf:"bytes,3,opt,name=lastTransitionTime"`


"last" transition time? would this just be the time at which this condition was added?

The field isn't necessary for our purposes, I was following https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#typical-status-properties ...

We want transitionTime for conditions if a client would need to show "created 2 minutes ago" or similar.

bparees · 2017-02-24T22:08:05Z

pkg/template/api/v1/types.go

+	// TemplateInstanceCreated is ...
+	TemplateInstanceCreated TemplateInstanceConditionType = "Created"
+	// TemplateInstanceFailed is ...
+	TemplateInstanceFailed TemplateInstanceConditionType = "Failed"


"new"? "creating"?

I don't believe these are conventional according to https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md ...

So "New" will be implied by the absence of any condition?

Effectively, yes.

I find that strange since these conditions are going to be what users see when they are trying to understand the state of their provision request. Seems like we can be more helpful by providing more descriptive/specific conditions.

bparees · 2017-02-24T22:12:28Z

pkg/template/api/validation/validation.go

@@ -51,3 +55,116 @@ func validateTemplateBody(template *api.Template) (allErrs field.ErrorList) {
 	allErrs = append(allErrs, unversionedvalidation.ValidateLabels(template.ObjectLabels, field.NewPath("labels"))...)
 	return
 }
+
+// ValidateTemplate tests if required fields in the Template are set.
+func ValidateTemplateInstance(oc *client.Client, ctx kapi.Context, ti *api.TemplateInstance) (allErrs field.ErrorList) {


yeah i think your concern is valid, this validation logic should just make sure the username is a valid value (fits whatever regex we enforce on usernames).

After that, if it turns out the user doesn't have the requisite permissions (or doesn't exist), we should find that out when we actually go to instantiate the template as the user, and when it fails, mark the provision request as failed.

bparees · 2017-02-24T22:19:55Z

pkg/template/servicebroker/bind.go

+
+	namespace := uuidmap.Spec.TemplateInstanceRef.Namespace
+
+	services, resp := b.getServices(namespace, instance_id)


need to discuss with @pmorie and @smarterclayton but i suspect binding is also going to need to do impersonation... impersonating the user requesting the binding (which can be different from the user who requested the provisioning).

so at a minimum: should use the provision impersonate user to do this

better: bind call should provide a user name to impersonate when doing this

otherwise i can theoretically bind to something i can't otherwise access and exfiltrate the credentials. (or we can make the argument that the SC should not have allowed that bind call to be made, and we don't need to do anything. again @pmorie and @smarterclayton)

OK - as a starting point I will have the bind call read the impersonation user from its parameters.

bparees · 2017-02-24T22:22:43Z

pkg/template/servicebroker/provision.go

+	return nil, openservicebroker.NewResponse(http.StatusInternalServerError, nil, err)
+}
+
+func (b *Broker) ensureTP(namespace string, instance_id string, template *api.Template, secret *kapi.Secret, impersonate string, didWork *bool) (*api.TemplateInstance, *openservicebroker.Response) {


nit: naming on ensureTP isn't obvious.

bparees · 2017-02-24T22:26:31Z

pkg/template/controller/templatebrokeruuidmap_deletion_controller.go

+)
+
+// This controller watches for TemplateBrokerUUIDMap objects which are set to gracefully
+// delete, and deletes them when their time is up.


we have to do this? i assumed k8s would handle the graceful deletion itself.

That was my understanding - @smarterclayton ?

Need to have a discussion about this specifically - graceful can be used to signal deletion in this fashion (synchronous GC will use it), but I'm not clear the intent yet.

0xmichalis · 2017-02-28T17:24:00Z

docs/man/man1/oadm-policy-scc-review.1

@@ -1,3 +0,0 @@
-This file is autogenerated, but we've stopped checking such files into the


You need to update to latest master, these should stay

@Kargakis I'm confused - on latest master, if I do a make update, these files are removed?

ah, I see - some issue with my local build environment - using hack/env seems to have fixed this, thanks

jim-minter · 2017-03-01T10:34:58Z

[test]

jim-minter · 2017-03-21T20:53:26Z

(pushed latest codebase)

@smarterclayton all your changes made, ptal.

smarterclayton · 2017-03-21T21:23:26Z

API approved. May have some iteration prerelease but this is fine for now.

bparees · 2017-03-22T20:07:39Z

[testextended][extended:core(templates)]

bparees

i'll hit merge if the extended tests pass, but may need a follow up depending on the answers to my last 2 questions.

bparees · 2017-03-22T20:11:38Z

test/extended/templates/helpers.go

+	}
+}
+
+func loopWithTimeout(d time.Duration, f func() bool) {


couldn't use kubernetes/pkg/util/wait.Poll?

bparees · 2017-03-22T20:11:56Z

test/extended/setup.sh

@@ -147,6 +150,9 @@ function os::test::extended::setup () {

 	os::log::info "Creating image streams"
 	oc create -n openshift -f "${OS_ROOT}/examples/image-streams/image-streams-centos7.json" --config="${ADMIN_KUBECONFIG}"
+
+	os::log::info "Creating sample app"
+	oc create -n openshift -f "${OS_ROOT}/examples/sample-app/application-template-stibuild.json" --config="${ADMIN_KUBECONFIG}"


why did this need to be added?

discussed on IRC, it's needed by the broker (to have a valid template to instantiate) and creating it in the TC risks flakes due to tests running in parallel.

That's not a great reason....

This won't work on GCE

why's that? it doesn't seem to assume anything the line directly above it doesn't also assume.

@jim-minter evidently this script isn't run on GCE environments. let's chat tomorrow about how to rework this requirement, but for now i'm going to merge this since this test isn't being run on GCE today anyway.

openshift-bot · 2017-03-22T20:37:33Z

Evaluated for origin testextended up to 0ae2afe

openshift-bot · 2017-03-22T20:37:45Z

Evaluated for origin test up to 0ae2afe

openshift-bot · 2017-03-22T21:05:36Z

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

openshift-bot · 2017-03-22T21:50:22Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/412/) (Base Commit: 04d74e0)

bparees · 2017-03-22T22:09:53Z

[merge]

openshift-bot · 2017-03-22T22:21:36Z

Evaluated for origin merge up to 0ae2afe

openshift-bot · 2017-03-23T04:05:29Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/184/) (Base Commit: 15e3269)

stevekuznetsov · 2017-03-26T21:13:54Z

test/extended/setup.sh

@@ -121,6 +121,9 @@ function os::test::extended::setup () {
 	cp "${SERVER_CONFIG_DIR}/master/master-config.yaml" "${SERVER_CONFIG_DIR}/master/master-config.orig2.yaml"
 	openshift ex config patch "${SERVER_CONFIG_DIR}/master/master-config.orig2.yaml" --patch="{\"auditConfig\": {\"enabled\": true}}"  > "${SERVER_CONFIG_DIR}/master/master-config.yaml"

+	cp "${SERVER_CONFIG_DIR}/master/master-config.yaml" "${SERVER_CONFIG_DIR}/master/master-config.orig2.yaml"
+	openshift ex config patch "${SERVER_CONFIG_DIR}/master/master-config.orig2.yaml" --patch="{\"enableTemplateServiceBroker\": true}"  > "${SERVER_CONFIG_DIR}/master/master-config.yaml"


This should just string the patches together like we do in hack/lib/start.sh

jim-minter self-assigned this Feb 14, 2017

jim-minter force-pushed the template_service_broker branch 2 times, most recently from 75dae28 to 2c562cb Compare February 14, 2017 16:15

bparees reviewed Feb 14, 2017

View reviewed changes

jim-minter force-pushed the template_service_broker branch from 2c562cb to 70f211c Compare February 16, 2017 17:51

jim-minter force-pushed the template_service_broker branch from 70f211c to fa932dd Compare February 23, 2017 19:51

jim-minter added the needs-api-review label Feb 23, 2017

jim-minter commented Feb 23, 2017

View reviewed changes

deads2k reviewed Feb 23, 2017

View reviewed changes

bparees reviewed Feb 24, 2017

View reviewed changes

jim-minter force-pushed the template_service_broker branch from fa932dd to 637db06 Compare February 27, 2017 16:42

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

jim-minter force-pushed the template_service_broker branch from 637db06 to 9ca3f6c Compare February 28, 2017 17:08

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

0xmichalis reviewed Feb 28, 2017

View reviewed changes

jim-minter force-pushed the template_service_broker branch 6 times, most recently from a196973 to 188e0a3 Compare March 2, 2017 13:13

jim-minter force-pushed the template_service_broker branch from 9743ad1 to c8c17b7 Compare March 21, 2017 20:52

smarterclayton added api-approved and removed needs-api-review labels Mar 21, 2017

jim-minter force-pushed the template_service_broker branch 6 times, most recently from d5fe55c to bc7f714 Compare March 22, 2017 17:54

deads2k and others added 4 commits March 22, 2017 14:05

enable generation of normal clientsets

c542b5e

UPSTREAM: <carry>: update clientset generator for openshift groups

f1c02ef

generated clients

5807b6a

Template service broker API

94d4523

jim-minter force-pushed the template_service_broker branch 2 times, most recently from 874f5cd to c02902f Compare March 22, 2017 18:22

bparees reviewed Mar 22, 2017

View reviewed changes

Template service broker

0ae2afe

jim-minter force-pushed the template_service_broker branch from c02902f to 0ae2afe Compare March 22, 2017 20:30

openshift-bot merged commit 7811e4b into openshift:master Mar 23, 2017

stevekuznetsov reviewed Mar 26, 2017

View reviewed changes


		namespace := uuidmap.Spec.TemplateInstanceRef.Namespace

		services, resp := b.getServices(namespace, instance_id)

		@@ -1,3 +0,0 @@
		This file is autogenerated, but we've stopped checking such files into the

templateservicebroker WIP #12953

templateservicebroker WIP #12953

Conversation

jim-minter commented Feb 14, 2017

jim-minter commented Feb 14, 2017

jim-minter commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jim-minter Feb 15, 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

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

jim-minter commented Feb 15, 2017 • edited Loading

jim-minter commented Feb 23, 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

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

jim-minter commented Mar 1, 2017

jim-minter commented Mar 21, 2017

smarterclayton commented Mar 21, 2017

bparees commented Mar 22, 2017

bparees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jim-minter Feb 15, 2017 •

edited

Loading

jim-minter commented Feb 15, 2017 •

edited

Loading