Support provisioning by external id #1756

carolynvs · 2018-02-22T14:58:29Z

Closes #1286.

In addition to provisioning by external id, I consolidated logic for interacting with PlanReference:

Add functions to the PlanReference type instead of requiring everyone who
works with PlanReference to know of the various ways a class/plan could
be specified.
Implement fmt.Formatter so that we can consistently format messages
about the plan. Many of the log messages assumed that a certain field,
like ClusterServicePlanExternalName would be set when now there are
multiple fields.

Formatting options for PlanReference

%c - Print specified class fields only
Examples:
{ClassExternalName:"foo"}
{ClassExternalID:"foo123"}
{ClassName:"k8s-foo123"}
%b - Print specified plan fields only
NOTE: %p is a reserved verb so we can't use it, and go vet fails for non-standard verbs
Examples:
{PlanExternalName:"bar"}
{PlanExternalID:"bar456"}
{PlanName:"k8s-bar456"}
%s - Print a short form of the plan and class
Examples:
foo/bar
foo123/bar456
k8s-foo123/k8s-bar456
%v - Print all specified fields
Examples:
{ClassExternalName:"foo", PlanExternalName:"bar"}
{ClassExternalID:"foo123", PlanExternalID:"bar456"}
{ClassName:"k8s-foo123", PlanName:"k8s-bar456"}

n3wscott · 2018-02-22T20:49:21Z

This looks good but my goodness it is large. Neat trick with Format method!! I need to look at this again.

carolynvs · 2018-02-22T20:53:29Z

@n3wscott Apologies for the mega PR. If I need to break it up, let me know!

n3wscott · 2018-02-22T20:59:31Z

A quick nit: All new files need the 2018 K8s Copyright

n3wscott · 2018-02-22T21:25:22Z

pkg/controller/controller_instance_test.go

@@ -344,8 +344,8 @@ func TestReconcileServiceInstanceNonExistentClusterServicePlanK8SName(t *testing
 	events := getRecordedEvents(testController)

 	expectedEvent := warningEventBuilder(errorNonexistentClusterServicePlanReason).msgf(
-		"References a non-existent ClusterServicePlan with K8S name %q on ClusterServiceClass with K8S name %q",
-		"nothereplan", testClusterServiceClassGUID,
+		"References a non-existent ClusterServicePlan {ClassName:%q, PlanName:%q}",


Since you have already tested the formatting of the output using Format method, I don't see a need to reformat it in the same way in the tests. I think you can just use the nice Format method output in the tests too.

n3wscott · 2018-02-22T21:36:28Z

This LGTM, I will tag it after the copyrights are added.

carolynvs · 2018-02-22T21:47:16Z

I've added the missing 2018 copyright headers.

n3wscott · 2018-02-22T21:48:06Z

Thanks for this work!

pmorie · 2018-03-01T01:06:59Z

pkg/apis/servicecatalog/plan_reference.go

+// * ClusterServiceClassExternalName
+// * ClusterServiceClassExternalID
+// * ClusterServiceClassName
+func (pr PlanReference) GetSpecifiedClass() string {


Is this for formatting purposes? I get the impression that it is, and that's fine. If it is, we should specify in the method godoc that this is for presentation purposes only, since we are conflating different types of information in the return value of this method.

Yes it's for formatting in our logs and in the svcat cli. I'll add that to the godoc.

pmorie · 2018-03-01T01:07:09Z

pkg/apis/servicecatalog/plan_reference.go

+		pr.ClusterServicePlanName != ""
+}
+
+// GetSpecifiedClass returns the user-specified class value from either:


s/either/one of/

pmorie · 2018-03-01T01:08:15Z

pkg/apis/servicecatalog/plan_reference.go

+
+// GetPlanFilterFieldName returns the appropriate field name for filtering
+// a list of service catalog plans by the PlanReference.
+func (pr PlanReference) GetPlanFilterFieldName() string {


shouldn't this also return metadata.name ?

metadata.name isn't one of the supported fields for filtering during a list. Can we change that? It would be great to not have to use a Get sometimes and a filtered List other times.

from a user perspective, it doesn't make that much sense to filter by name because those are UUIDs, no?

Yeah the name is a uuid. If we added filtering by metadata.name, it would be purely for our own convenience in the controller code.

I ran into a situation where I wanted to filter by name as a uuid just the other day

Ok, cool 👍

pmorie · 2018-03-01T01:08:38Z

pkg/apis/servicecatalog/plan_reference.go

+//     {ClassExternalName:"foo", PlanExternalName:"bar"}
+//     {ClassExternalID:"foo123", PlanExternalID:"bar456"}
+//     {ClassName:"k8s-foo123", PlanName:"k8s-bar456"}
+func (pr PlanReference) Format(s fmt.State, verb rune) {


I did not know this was a thing in go and this is awesome

pmorie · 2018-03-01T01:09:23Z

pkg/apis/servicecatalog/types.go

+	// Immutable.
+	ClusterServiceClassExternalID string
+
+	// ClusterServicePlanExternalID is the broker's external id for the plan.


Add that this is immutable? also in v1beta1

It's not immutable though, right? Just like ClusterServicePlanExternalName and ClusterServicePlan, you can upgrade the plan later depending on the class.

iirc you can upgrade the plan later, so I'd agree

You're right

pmorie · 2018-03-01T01:10:42Z

pkg/apis/servicecatalog/validation/instance.go

-		allErrs = append(allErrs, field.Required(fldPath.Child("clusterServicePlanExternalName"), "exactly one of clusterServicePlanExternalName or clusterServicePlanName required"))
-		allErrs = append(allErrs, field.Required(fldPath.Child("clusterServicePlanName"), "exactly one of clusterServicePlanExternalName or clusterServicePlanName required"))
+	// Must specify exactly one source of the plan: external id, external name, k8s name.
+	if (b2i(externalPlanNameSet) + b2i(externalPlanIDSet) + b2i(k8sPlanSet)) != 1 {


I'm too tired to tell whether this is correct or not, so stopping here - wanted to get you preliminary feedback.

carolynvs · 2018-03-06T15:27:12Z

I've incorporated the suggested godoc changes.

MHBauer · 2018-03-14T00:34:28Z

pkg/apis/servicecatalog/v1beta1/types.go

 //  - ClusterServiceClassName and ClusterServicePlanName
 //
-// For both of these ways, if a ClusterServiceClass only has one plan
+// For any of these ways, if a ClusterServiceClass only has one plan
 // then leaving the *ServicePlanName is optional.


Does this comment count for the plan ID like plan name?

Yes thanks for noticing that, I'll update the comment.

MHBauer · 2018-03-14T00:37:55Z

pkg/apis/servicecatalog/validation/instance.go

-		allErrs = append(allErrs, field.Required(fldPath.Child("clusterServiceClassExternalName"), "exactly one of clusterServiceClassExternalName or clusterServiceClassName required"))
-		allErrs = append(allErrs, field.Required(fldPath.Child("clusterServiceClassName"), "exactly one of clusterServiceClassExternalName or clusterServiceClassName required"))
+	// Must specify exactly one source of the class: external id, external name, k8s name.
+	if (b2i(externalClassNameSet) + b2i(externalClassIDSet) + b2i(k8sClassSet)) != 1 {


no xor in golang, I guess.

If there is, I don't know about it! 😂

Talked with Paul, ^ is bitwise xor and only works on ints, not bool.

ya, bool xor

pmorie · 2018-03-22T18:02:59Z

@carolynvs probably want to rebase this one and verify that CI is good - LGTM from me

carolynvs · 2018-03-22T18:21:33Z

I'll add integration tests tomorrow and rebase.

carolynvs · 2018-03-26T14:05:59Z

@pmorie Any idea why the Jenkins build timed-out? I'm not sure where to start looking. 😀

Walkthrough completed successfully.
Creating artifacts...
Cleaning up resources...
Cancelling nested steps due to timeout
Sending interrupt signal to process
Terminated
[Pipeline] }
[Pipeline] // timeout
[Pipeline] echo
Run failed.

arschles

I went through this twice and I have an ok understanding of what's going on here. hopefully my comments etc... were reasonable

Thanks! 😄

arschles · 2018-03-26T21:34:24Z

pkg/apis/servicecatalog/plan_reference.go

+
+// GetPlanFilterFieldName returns the appropriate field name for filtering
+// a list of service catalog plans by the PlanReference.
+func (pr PlanReference) GetPlanFilterFieldName() string {


from a user perspective, it doesn't make that much sense to filter by name because those are UUIDs, no?

arschles · 2018-03-26T21:37:54Z

pkg/apis/servicecatalog/plan_reference.go

+		classFields = append(classFields, fmt.Sprintf("ClassName:%q", pr.ClusterServiceClassName))
+	}
+
+	planFields := make([]string, 0, 3)


Seems like it'd make sense to just create a []string{} and append to it as necessary. preallocating memory seems like an optimization we don't need (and we might not use any of it at all). am I missing something about how this works?

You're right, I just did that so y'all would think I was cool. 😀 Since only 1 at most is usually ever set, I'll switch to an empty slice.

arschles · 2018-03-26T21:42:19Z

pkg/apis/servicecatalog/v1beta1/plan_reference_test.go

+			}
+		})
+	}
+}


I always 😢 when code has to be copied across versioned/unversioned types. C'est la vie for now

arschles · 2018-03-26T21:46:23Z

pkg/apis/servicecatalog/types.go

+	// Immutable.
+	ClusterServiceClassExternalID string
+
+	// ClusterServicePlanExternalID is the broker's external id for the plan.


iirc you can upgrade the plan later, so I'd agree

arschles · 2018-03-26T21:54:22Z

pkg/apis/servicecatalog/validation/instance.go

+			allErrs = append(allErrs, field.Invalid(fldPath.Child("clusterServicePlanExternalName"), p.ClusterServicePlanExternalName, msg))
+		}
+	}
+	if externalClassIDSet {


shouldn't this be an else if? if name isn't set, seems like we'd need to check ID instead, not in addition to

arschles · 2018-03-26T21:56:11Z

pkg/controller/controller_instance.go

@@ -889,25 +889,31 @@ func (c *controller) resolveReferences(instance *v1beta1.ServiceInstance) (bool,
 // If ClusterServiceClass can not be resolved, returns an error, records an
 // Event, and sets the InstanceCondition with the appropriate error message.
 func (c *controller) resolveClusterServiceClassRef(instance *v1beta1.ServiceInstance) (*v1beta1.ServiceInstance, *v1beta1.ClusterServiceClass, error) {
+	if !instance.Spec.ClassSpecified() {
+		// ServiceInstance is in invalid state, should not ever happen. check
+		return nil, nil, fmt.Errorf("ServiceInstance is in inconsistent state, neither ClusterServiceClassExternalName, ClusterServiceClassExternalID, nor ClusterServiceClassName is set: %+v", instance.Spec)


outputting the entire spec is a lot, does it make sense to output it at all?

arschles · 2018-03-26T21:57:27Z

pkg/controller/controller_instance_test.go

@@ -92,7 +92,7 @@ func TestReconcileServiceInstanceNonExistentClusterServiceClass(t *testing.T) {
 	events := getRecordedEvents(testController)

 	expectedEvent := warningEventBuilder(errorNonexistentClusterServiceClassReason).msgf(
-		"References a non-existent ClusterServiceClass (ExternalName: %q) or there is more than one (found: %d)",
+		"References a non-existent ClusterServiceClass {ClassExternalName:%q} or there is more than one (found: %d)",
 		"nothere", 0,


necessary to format the 0, or can you put it into the string? same with a few below

arschles · 2018-03-26T22:07:24Z

@carolynvs I restarted the jenkins build, it's pending now

carolynvs · 2018-03-27T18:42:29Z

@arschles I've incorporated everyone's review feedback, but the build is still failing on a flake. I checked and other PRs, not just mine, are hitting that timeout during the walkthrough too occasionally.

pmorie · 2018-03-27T21:03:29Z

@carolynvs looks like CI failed

pmorie · 2018-03-28T05:20:39Z

servicebinding "ups-binding" created
/var/lib/jenkins/workspace/service-catalog-PR-testing2/src/github.com/kubernetes-incubator/service-catalog/contrib/hack/utilities.sh: line 36: 5637

This is easily the least helpful error message I have seen today.

@carolynvs does test_walkthrough.sh pass for you locally?

carolynvs · 2018-03-28T14:13:29Z

@pmorie Yes, it passes when run locally.

Here's the command I used to run it:

REGISTRY=quay.io/kubernetes-service-catalog/ VERSION=v0.1.11 ./contrib/jenkins/test_walkthrough.sh

EDIT: I'm a huge dork and just realized that didn't test anything. 😊 I'm figuring out how to run that properly locally and will report back.

carolynvs · 2018-03-28T14:46:43Z

@pmorie Okay, now I think I've run it properly locally and it passed. Let me know if I actually got it right this time! 😊

REGISTRY=carolynvs/ VERSION=canary make push
REGISTRY=carolynvs/ VERSION=canary ./contrib/jenkins/test_walkthrough.sh

* Add functions to the PlanReference type instead of requring everyone who works with PlanReference to know of the various ways a class/plan could be specified. * Implement fmt.Formatter so that we can consistently format messages about the plan. Many of the log messages assumed that a certain field, like ClusterServicePlanExternalName would be set when now there are multiple fields. Formatting options for PlanReference * %c - Print specified class fields only Examples: {ClassExternalName:"foo"} {ClassExternalID:"foo123"} {ClassName:"k8s-foo123"} * %b - Print specified plan fields only NOTE: %p is a reserved verb so we can't use it, and go vet fails for non-standard verbs Examples: {PlanExternalName:"bar"} {PlanExternalID:"bar456"} {PlanName:"k8s-bar456"} * %s - Print a short form of the plan and class Examples: foo/bar foo123/bar456 k8s-foo123/k8s-bar456 * %v - Print all specified fields Examples: {ClassExternalName:"foo", PlanExternalName:"bar"} {ClassExternalID:"foo123", PlanExternalID:"bar456"} {ClassName:"k8s-foo123", PlanName:"k8s-bar456"}

carolynvs · 2018-04-02T14:35:44Z

@pmorie The jenkins build problem was caused by other changes that I had picked up when rebasing, that caused the build to go over 30 minutes. After I rebased again, I picked up their change to bump the timeout on the build, and now my PR builds fine. 🤷‍♀️

arschles

LGTM, thanks @carolynvs

* master: Support provisioning by external id (kubernetes-retired#1756) Add namespaced ServiceBroker API (kubernetes-retired#1881) Enable verbose logging in svcat (kubernetes-retired#1822) Adding ServicePlan (namespaced plans) API (kubernetes-retired#1894) Svcat bind params now supports --params-json (kubernetes-retired#1889) words discussing the automated builds (kubernetes-retired#1885) v0.1.12 release changes don't overwrite generated files before verifying (kubernetes-retired#1891) Update registry code from broker to clusterservicebroker (kubernetes-retired#1880) Credentials remapping (kubernetes-retired#1868) Rename/move existing "ServicePlan" things (kubernetes-retired#1883) Start orphan mitigation after receiving a last operation status for async provisioning (kubernetes-retired#1886) Update of name in example ServiceClass (kubernetes-retired#1878) Cluster-id configmap (kubernetes-retired#1658) Add namespaced ServiceClass API (kubernetes-retired#1859) Fix common validations regression (kubernetes-retired#1882)

n3wscott · 2018-05-21T22:44:03Z

plugin/pkg/admission/serviceplan/defaultserviceplan/admission.go

@@ -83,7 +83,8 @@ func (d *defaultServicePlan) Admit(a admission.Attributes) error {
 		if !apierrors.IsNotFound(err) {
 			return admission.NewForbidden(a, err)
 		}
-		msg := fmt.Sprintf("ClusterServiceClass %q does not exist, can not figure out the default ClusterServicePlan.", instance.Spec.ClusterServiceClassExternalName)
+		msg := fmt.Sprintf("ClusterServiceClass %c does not exist, can not figure out the default ClusterServicePlan.",


I am getting an error from this line today.

plugin/pkg/admission/serviceplan/defaultserviceplan/admission.go:86: arg instance.Spec.PlanReference for printf verb %c of wrong type: github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog.PlanReference exit status 1

# Verify that vendor/ is in sync with Gopkg.lock docker run --security-opt label:disable --rm -v /Users/nicholss/go/src/github.com/kubernetes-incubator/service-catalog:/go/src/github.com/kubernetes-incubator/service-catalog -v /Users/nicholss/go/src/github.com/kubernetes-incubator/service-catalog/.pkg:/go/pkg --env AZURE_STORAGE_CONNECTION_STRING scbuildimage build/verify-vendor.sh Verified that vendor/ is in sync with Gopkg.lock Running gofmt: Running golint and go vet: docker run --security-opt label:disable --rm -v /Users/nicholss/go/src/github.com/kubernetes-incubator/service-catalog:/go/src/github.com/kubernetes-incubator/service-catalog -v /Users/nicholss/go/src/github.com/kubernetes-incubator/service-catalog/.pkg:/go/pkg --env AZURE_STORAGE_CONNECTION_STRING scbuildimage go vet github.com/kubernetes-incubator/service-catalog/... warning: ignoring symlink /go/src/github.com/kubernetes-incubator/service-catalog/docsite/docs warning: ignoring symlink /go/src/github.com/kubernetes-incubator/service-catalog/vendor/github.com/coreos/etcd/cmd/etcd warning: ignoring symlink /go/src/github.com/kubernetes-incubator/service-catalog/vendor/github.com/prometheus/procfs/fixtures/self pkg/controller/controller.go:506: arg instance.Spec.PlanReference for printf verb %c of wrong type: github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1.PlanReference pkg/controller/controller_instance.go:931: arg instance.Spec.PlanReference for printf verb %c of wrong type: github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1.PlanReference pkg/controller/controller_instance.go:965: arg instance.Spec.PlanReference for printf verb %c of wrong type: github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1.PlanReference pkg/controller/controller_instance.go:1041: arg instance.Spec.PlanReference for printf verb %b of wrong type: github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1.PlanReference pkg/controller/controller_clusterid_test.go:85: github.com/kubernetes-incubator/service-catalog/vendor/k8s.io/apimachinery/pkg/runtime/schema.GroupResource composite literal uses unkeyed fields pkg/controller/controller_clusterid_test.go:110: github.com/kubernetes-incubator/service-catalog/vendor/k8s.io/apimachinery/pkg/runtime/schema.GroupResource composite literal uses unkeyed fields pkg/controller/controller_instance_test.go:4332: arg instance.Spec.PlanReference for printf verb %c of wrong type: github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1.PlanReference pkg/controller/controller_test.go:556: github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1.ClusterServiceClassStatus composite literal uses unkeyed fields exit status 1 plugin/pkg/admission/serviceplan/defaultserviceplan/admission.go:86: arg instance.Spec.PlanReference for printf verb %c of wrong type: github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog.PlanReference exit status 1 make: *** [verify] Error 1

This was caused by a stale build image and was fixed after running make clean-build-image.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 22, 2018

n3wscott reviewed Feb 22, 2018

View reviewed changes

n3wscott added the LGTM1 label Feb 22, 2018

pmorie reviewed Mar 1, 2018

View reviewed changes

MHBauer reviewed Mar 14, 2018

View reviewed changes

carolynvs force-pushed the provision-by-external-id branch from 1f88f07 to 2835211 Compare March 21, 2018 18:45

carolynvs force-pushed the provision-by-external-id branch from a1089b9 to c7f2a50 Compare March 23, 2018 21:39

arschles suggested changes Mar 26, 2018

View reviewed changes

carolynvs-msft added 4 commits March 30, 2018 14:32

Support provisioning by external id

aff8013

Add tests for provisioning with an external id

ccda2fe

carolynvs-msft added 5 commits March 30, 2018 14:32

Incorporate review feedback

a52b814

Fix comment explaining when the service plan is optional

1ef723d

Add integration tests for provision by external id

0666624

Incorporate PR feedback

e516c73

Use plan reference formatting in unit tests

8af0e94

carolynvs force-pushed the provision-by-external-id branch from 5c6c300 to 8af0e94 Compare March 30, 2018 19:32

arschles approved these changes Apr 3, 2018

View reviewed changes

arschles added the LGTM2 label Apr 3, 2018

arschles merged commit 0d741d5 into kubernetes-retired:master Apr 3, 2018

carolynvs deleted the provision-by-external-id branch April 4, 2018 14:45

jboyd01 mentioned this pull request May 7, 2018

don't require specifying a plan when creating an instance #2016

Merged

n3wscott reviewed May 21, 2018

View reviewed changes

Support provisioning by external id #1756

Support provisioning by external id #1756

Conversation

carolynvs commented Feb 22, 2018

n3wscott commented Feb 22, 2018

carolynvs commented Feb 22, 2018

n3wscott commented Feb 22, 2018

Choose a reason for hiding this comment

n3wscott commented Feb 22, 2018

carolynvs commented Feb 22, 2018

n3wscott commented Feb 22, 2018

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

carolynvs commented Mar 6, 2018

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

pmorie commented Mar 22, 2018

carolynvs commented Mar 22, 2018

carolynvs commented Mar 26, 2018

arschles 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

arschles commented Mar 26, 2018

carolynvs commented Mar 27, 2018

pmorie commented Mar 27, 2018

pmorie commented Mar 28, 2018

carolynvs commented Mar 28, 2018 • edited Loading

carolynvs commented Mar 28, 2018

carolynvs commented Apr 2, 2018

arschles 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

carolynvs commented Mar 28, 2018 •

edited

Loading