Add UserProvided field to service instances #2421

jberkhahn · 2018-10-17T23:42:43Z

This PR is a

Feature Implementation
Bug Fix
Documentation

What this PR does / why we need it:
This PR implements User Provided Services, where a user can provision a service instance that is not associated with an extant service plan, class, or broker. This is typically used to use some pre-existing database and manage use of it with the normal service-catalog workflow.

Docs to follow, submitting this now to get some input on the viability of this method of implementation.

Which issue(s) this PR fixes
Fixes #2189

Please leave this checklist in the PR comment so that maintainers can ensure a good PR.

Merge Checklist:

New feature
- Tests
- Documentation
SVCat CLI flag
Server Flag for config
- Chart changes
- removing a flag by marking deprecated and hiding to avoid
  breaking the chart release and existing clients who provide a
  flag that will get an error when they try to update

k8s-ci-robot · 2018-10-17T23:42:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jberkhahn

If they are not already assigned, you can assign the PR to them by writing /assign @jberkhahn in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

carolynvs · 2018-10-18T14:36:41Z

Before this can be merged, we'll need to have clear documentation on a few items:

How to create a user provided service instance
What happens when you deprovision a user provided service instance
Can you bind to a user provided service instance? (I see from your PR that the answer is yes, just saying we need doc).
What is the relationship between this and UPS broker?
An idea of what this will look like in the CLI (not implemented, just making sure we have the right wiring to support this properly)

carolynvs

Putting a hold on this for now, just so that we can make sure we have a proper discussion about the design first.

carolynvs · 2018-10-18T14:41:29Z

pkg/apis/servicecatalog/validation/instance.go

@@ -88,7 +89,9 @@ func validateServiceInstanceSpec(spec *sc.ServiceInstanceSpec, fldPath *field.Pa
 	allErrs := field.ErrorList{}

 	allErrs = append(allErrs, validateObjectReferences(spec, fldPath)...)
-	allErrs = append(allErrs, validatePlanReference(&spec.PlanReference, fldPath)...)
+	if !spec.UserProvided {


I am concerned about having such an important property of instances suddenly becoming optional, mostly because we have so many assumptions in our code, and this may lead to bugs.

I didn't see any proposal or implementation suggestion before this PR came about. It would have been helpful before diving into the code. Was there a design discussion and I missed it?

One idea that I'm not sure if it was considered is to have service catalog provide a placeholder class/plan for user provided services. Just so that we don't need a bunch of special cases for when it's okay to have the plan reference empty. Also it would make the code in the cli and other areas more consistent on how to list/query and display things.

I'm a bit confused about if this replaces the user provided service broker? Does it make things possible that weren't previously possible with that broker? Are they complimentary?

To be clear, I'm not totally against this change at all. I just want to have a bit of discussion before we merge, to make sure that we understand how this will work and if it replaces anything.

We discussed this at the last f2f. It's meant to replace the ups broker, although the ups broker is really just a toy while this is intended for actual use.

A blank class/plan is an interesting idea though. What other use cases need such a thing, because I agree it's a bit cumbersome to have to many specific cases that don't require a class/plan.

Yeah, if you are OK with redoing this to try out using a placeholder class/plan (maybe in another branch to not lose your work) and see if you like it better?

jberkhahn · 2018-11-08T20:56:12Z

Removing the hold as we discussed on the call on Monday.

jberkhahn · 2018-11-20T23:09:52Z

Putting the hold back on this, think I found a better way to do this.

duglin · 2018-11-20T23:16:24Z

pkg/apis/servicecatalog/validation/instance.go

-	allErrs = append(allErrs, validatePlanReference(&spec.PlanReference, fldPath)...)
+	if !spec.UserProvided {
+		allErrs = append(allErrs, validatePlanReference(&spec.PlanReference, fldPath)...)
+	}


Should this check to make sure that Plan is blank in the case of a UPS Instance?

duglin · 2018-11-20T23:21:09Z

pkg/apis/servicecatalog/validation/instance.go

 	var errMsg string
 	allErrs := field.ErrorList{}
+	if !userProvided {


I think in the "else" case we need to make sure Plan is empty.

duglin · 2018-11-20T23:33:10Z

pkg/controller/controller_binding_test.go

+	paramSecret := &corev1.Secret{
+		Data: map[string][]byte{
+			"credentials": []byte("{\"credentials\":\"{\\\"username\\\":\\\"foo\\\",\\\"password\\\":\\\"bar\\\"}\"}"),
+			//"credentials": []byte("{\"username\":\"foo\",\"password\":\"bar\"}"),


is this comment needed?

duglin · 2018-11-20T23:35:39Z

pkg/controller/controller_binding_test.go

+// TestReconcileServiceBindingUserProvided tests reconcile binding
+// to ensure bindings that reference an instance with UserProvided
+// set and no class/plan work
+func TestReconcileServiceBindingUserProvided(t *testing.T) {


Do we have a test for a UPS that has empty creds? Meaning no auth (or even endpoint URL) is required to use the service.

duglin · 2018-11-20T23:37:33Z

pkg/controller/controller_instance.go

+		upsDash := ""
+		instance.Status.InProgressProperties = inProgressProperties
+		instance.Status.UserProvided = true
+		//readyCond := newServiceInstanceReadyCondition(v1beta1.ConditionFalse, errorProvisionCallFailedReason, "User provided instance successfully created")


do we still need the comment?

duglin · 2018-11-20T23:42:55Z

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

@@ -558,6 +558,41 @@ func TestWithNoPlanSucceedsWithMultiplePlansFromDifferentClasses(t *testing.T) {
 	}
 }

+func TestWithUPSWorks(t *testing.T) {


Is this just testing to make sure that the admission controller doesn't mess with the UPS flag?

If so, shouldn't we have 2 tests? one with userProvided set to true and one set to false - and then line 579 have tc.userProvided on the right side of the expression?

To test that it doesn't mess with it in both situations

k8s-ci-robot · 2019-01-20T21:14:56Z

@jberkhahn: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot requested review from eriknelson and jeremyrickard October 17, 2018 23:42

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 17, 2018

jberkhahn requested a review from carolynvs October 17, 2018 23:42

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 17, 2018

jberkhahn added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 17, 2018

jberkhahn force-pushed the ups2 branch 4 times, most recently from 428b7a0 to ecc82f4 Compare October 18, 2018 00:20

carolynvs suggested changes Oct 18, 2018

View reviewed changes

jberkhahn force-pushed the ups2 branch 2 times, most recently from d2e90d1 to 1b63ba3 Compare October 18, 2018 18:56

Add UserProvided field to service instances

5a28f9b

jberkhahn force-pushed the ups2 branch from 1b63ba3 to 5a28f9b Compare October 18, 2018 19:47

jberkhahn removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2018

jberkhahn added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 20, 2018

duglin reviewed Nov 20, 2018

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2019

jberkhahn closed this Apr 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UserProvided field to service instances #2421

Add UserProvided field to service instances #2421

jberkhahn commented Oct 17, 2018

k8s-ci-robot commented Oct 17, 2018

carolynvs commented Oct 18, 2018 •

edited

Loading

carolynvs left a comment

carolynvs Oct 18, 2018

jberkhahn Oct 18, 2018

carolynvs Oct 31, 2018

jberkhahn commented Nov 8, 2018

jberkhahn commented Nov 20, 2018

duglin Nov 20, 2018

duglin Nov 20, 2018

duglin Nov 20, 2018

duglin Nov 20, 2018 •

edited

Loading

duglin Nov 20, 2018

duglin Nov 20, 2018

duglin Nov 20, 2018

duglin Nov 20, 2018

k8s-ci-robot commented Jan 20, 2019

Add UserProvided field to service instances #2421

Add UserProvided field to service instances #2421

Conversation

jberkhahn commented Oct 17, 2018

k8s-ci-robot commented Oct 17, 2018

carolynvs commented Oct 18, 2018 • edited Loading

carolynvs 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

jberkhahn commented Nov 8, 2018

jberkhahn commented Nov 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin Nov 20, 2018 • 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

k8s-ci-robot commented Jan 20, 2019

carolynvs commented Oct 18, 2018 •

edited

Loading

duglin Nov 20, 2018 •

edited

Loading