Add admission control to support default service plans. Resolves issue 1111 #1131

vaikas · 2017-08-14T17:30:27Z

… go style guide. Wire admission controller into the apiserver

kibbles-n-bytes

Overall looks fine by me, a few nits.

kibbles-n-bytes · 2017-08-14T20:59:03Z

contrib/examples/walkthrough/ups-instance-default-sp.yaml

@@ -0,0 +1,11 @@
+apiVersion: servicecatalog.k8s.io/v1alpha1


I'd be fine with making this the one-and-only ups-instance.yaml for the walkthrough. I don't think we need multiple walkthrough yamls for every possible code path; there's plenty we're not covering as is.

In fact, for the very same user experience issues @duglin had been describing with the user provided services, I think it would be ideal to have this be our preferred story.

I agree that it would be a great story, I'm just worried when a user tries to create their own instance they won't have a complete example. Any case, I'd prefer to keep this here, but again if you feel strongly, I'll yank it.

I don't feel strongly. I think in the future we will want to migrate away from using user provided services as our walkthrough example anyway, as it is a bit of a special case (potentially bundled with catalog, no auth, plan concept not making sense, doesn't tell the main "consuming third party services is easy!" OSB story).

@vaikas-google what do you think about moving the deployment of ups to be part of the svc-cat chart so its just deployed by default? As @kibbles-n-bytes was saying, having a walkthru (which is meant to be a learning example) not show the use of plans might not be the best example. Of course we'd need a real broker to be deployed as part of the walkthru instead.

kibbles-n-bytes · 2017-08-14T21:04:45Z

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

+		if !errors.IsNotFound(err) {
+			return errors.NewInternalError(err)
+		}
+		msg := fmt.Sprintf("ServiceClass %q does not exist, PlanName must be specified", instance.Spec.ServiceClassName)


The "PlanName must be specified" part of the error seems irrelevant to this particular situation where we simply could not get the service class.

talked offline, changed as per that discussion.

To document for posterity: it's a bit of a strange error case, as normally an error with the service class would still be allowed to reach the API server, and the user is responsible for getting the service class working correct so that the instance will eventually succeed.

In this case, the error is actually representative of the inability to assign a default plan due to the fetching of the service class having an error. We changed the message to more clearly address that nuance.

kibbles-n-bytes · 2017-08-14T21:18:32Z

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

+}
+
+// newInstance returns a new instance for the specified namespace.
+func newInstance(namespace string) servicecatalog.Instance {


This function is duplicated from the namespace lifecycle admission controller tests. It seems to me it'd be useful to have a util package shared across all admission controller tests, as functions such as these seem to be common, and we will be adding more admission controllers as time goes on.

Totes agree if there are others, seems more clutter to add another package right now just for this one function however? If you feel strongly though, I can do it.

My thoughts were that newMockClientForTest(), newBroker(), and newInstance(namespace) from namespace/lifecycle/admission_test.go would move there, as well as newServiceClass(name, plans...) (and the now-duplicate newInstance(namespace)) from this PR.

I don't have super strong feelings about it, just that it'll pretty much have to happen once we hit a third admission controller, so might as well get it out of the way.

I agree it'd be nice. @kibbles-n-bytes how about in a follow-up, in the interest of getting this functionality + its tests in?

@arschles Yeah that's totally fine.

kibbles-n-bytes · 2017-08-14T21:30:00Z

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

@@ -0,0 +1,122 @@
+/*


This admission controller's behavior as it stands would be completely undocumented. Could we get a short message somewhere in our docs that mentions it?

I guess the most appropriate place would be: https://github.com/kubernetes-incubator/service-catalog/blob/master/docs/design.md#creating-a-service-instance . It's rough around the edges for now, but at least having a mention would remind us to include it in a rewrite should we do so later.

would you be ok with a follow on PR for this?

That's fine. Could you make an issue for it?

I guess no followup happened then... @kibbles-n-bytes

…ce class

duglin · 2017-08-15T02:18:27Z

What's going to happen if someone forgets to deploy the ac?

pmorie · 2017-08-15T02:49:33Z

Then this won't work, just like any of the other numerous features in k8s that depend on admission controllers.

…

On Mon, Aug 14, 2017 at 10:18 PM Doug Davis ***@***.***> wrote: What's going to happen if someone forgets to deploy the ac? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1131 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWXmKKQCyEeUAIM_hBmrddqADDtqjE9ks5sYP_zgaJpZM4O2qfp> .

duglin · 2017-08-15T10:49:20Z

yea but what will the user see? It seems like the Instance should have a condition saying that it can't proceed because the plan is empty. Otherwise the user will never know what's going on and can't try to fix it.

duglin · 2017-08-15T10:53:27Z

From a quick look at the code it seems like we'll just call the broker with a blank planID, which will probably result in the broker generating an error - maybe. It seems that as part of this PR we should check for an empty planID before we try to provision the instance, since the spec mandates that it not be blank, and set an appropriate Condition.

duglin · 2017-08-15T11:38:05Z

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

+	sc, err := d.scLister.Get(instance.Spec.ServiceClassName)
+	if err != nil {
+		if !errors.IsNotFound(err) {
+			return errors.NewInternalError(err)


any reason this one isn't wrapped by the "NewForbidden" logic?

duglin · 2017-08-15T11:41:39Z

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

+		}
+		msg := fmt.Sprintf("ServiceClass %q does not exist, can not figure out the default Service Plan.", instance.Spec.ServiceClassName)
+		glog.V(4).Info(msg)
+		return admission.NewForbidden(a, fmt.Errorf(msg))


shouldn't the fmt.Errorf() just be errors.New() since by that point in the flow "msg" isn't a string needing %'s substitutions any more? I kind of expected lint/vet to catch it.

Just FYI: vet doesn't detect this because they claim it's a style preference, not one of correctness (golang/go#17173).

that's really weird because I know I've seen some complaint from some golang checking tool complain about using things like Printf() w/o any variable args. Maybe it was just a dream... :-)

duglin · 2017-08-15T11:41:50Z

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

+	if len(sc.Plans) > 1 {
+		msg := fmt.Sprintf("ServiceClass %q has more than one plan, PlanName must be specified", instance.Spec.ServiceClassName)
+		glog.V(4).Info(msg)
+		return admission.NewForbidden(a, fmt.Errorf(msg))


errors.New()

duglin · 2017-08-15T11:43:06Z

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

+
+func (d *defaultServicePlan) Validate() error {
+	if d.scLister == nil {
+		return fmt.Errorf("missing service class lister")


errors.New()

duglin · 2017-08-15T11:46:33Z

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

+	err = handler.Admit(admission.NewAttributesRecord(&instance, nil, servicecatalog.Kind("Instance").WithVersion("version"), instance.Namespace, instance.Name, servicecatalog.Resource("instances").WithVersion("version"), "", admission.Create, nil))
+	if err == nil {
+		t.Errorf("unexpected success with no ServiceClasses.List succeeding")
+	} else if !strings.Contains(err.Error(), "not yet ready to handle request") {


I can't express how much doing strcmp on error string like this in golang disturbs me :-( nothing to change, just venting

FYI, it doesn't have to be this way. We can create specific error types and compare types (i.e. err != errNotYetReadyToHandleRequest). Nothing needed right now; I'm willing to go back and correct instances where we have these comparisons post-beta.

pmorie · 2017-08-15T14:09:45Z

If a service instance reaches validation without a plan, it should be rejected.

…

On Tue, Aug 15, 2017 at 7:46 AM Doug Davis ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In plugin/pkg/admission/serviceplan/defaultserviceplan/admission_test.go <#1131 (comment)> : > + fakeClient.AddReactor("list", "serviceclasses", func(action core.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("simulated test failure") + }) + handler, informerFactory, err := newHandlerForTest(fakeClient) + if err != nil { + t.Errorf("unexpected error initializing handler: %v", err) + } + informerFactory.Start(wait.NeverStop) + + instance := newInstance("dummy") + instance.Spec.ServiceClassName = "foo" + + err = handler.Admit(admission.NewAttributesRecord(&instance, nil, servicecatalog.Kind("Instance").WithVersion("version"), instance.Namespace, instance.Name, servicecatalog.Resource("instances").WithVersion("version"), "", admission.Create, nil)) + if err == nil { + t.Errorf("unexpected success with no ServiceClasses.List succeeding") + } else if !strings.Contains(err.Error(), "not yet ready to handle request") { I can't express how much doing strcmp on error string like this in golang disturbs me :-( nothing to change, just venting — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1131 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWXmB_EVxtXh0_FH6acONJM6FOKV63Tks5sYYUagaJpZM4O2qfp> .

kibbles-n-bytes · 2017-08-15T16:29:30Z

@duglin As Paul said, the apiserver validates that the plan is non-empty, so we wouldn't call the broker.

When we have more mature docs, it'd probably be helpful to mention in the section where we say that the catalog defaults to a plan if there's only one something like: if you're getting an error saying "planName is required" when instantiating a service from a class with only a single plan, make sure that the `DefaultServicePlan` admission controller is enabled.

duglin · 2017-08-15T16:33:00Z

@kibbles-n-bytes - agreed, I just couldn't find that spot in the code yesterday. Got a file/line ref?

kibbles-n-bytes · 2017-08-15T16:53:16Z

@duglin Lines 56-57 in pkg/apis/servicecatalog/validation/instance.go

vaikas · 2017-08-15T17:16:15Z

I believe I've addressed all the comments, PTAL.

duglin · 2017-08-15T17:32:05Z

@vaikas-google thanks for the ptr

duglin · 2017-08-15T19:08:14Z

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

+	}
+
+	p := sc.Plans[0]
+	glog.V(4).Infof("Using default plan %s for Service Class %s for instance %s",


can you convert the %s here to %q's so things are quoted and easier to read?

duglin · 2017-08-15T19:12:49Z

one very minor nit and then its good to me

vaikas · 2017-08-15T19:42:12Z

Thanks for the reviews, PTAL.

duglin · 2017-08-15T19:42:28Z

mucho thanks! LGTM

duglin · 2017-08-15T19:42:51Z

just waiting on CI then I'll merge

arschles

LGTM, thanks Ville!

arschles · 2017-08-15T19:58:38Z

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

+}
+
+// newInstance returns a new instance for the specified namespace.
+func newInstance(namespace string) servicecatalog.Instance {


I agree it'd be nice. @kibbles-n-bytes how about in a follow-up, in the interest of getting this functionality + its tests in?

arschles · 2017-08-15T20:00:27Z

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

+	err = handler.Admit(admission.NewAttributesRecord(&instance, nil, servicecatalog.Kind("Instance").WithVersion("version"), instance.Namespace, instance.Name, servicecatalog.Resource("instances").WithVersion("version"), "", admission.Create, nil))
+	if err == nil {
+		t.Errorf("unexpected success with no ServiceClasses.List succeeding")
+	} else if !strings.Contains(err.Error(), "not yet ready to handle request") {


FYI, it doesn't have to be this way. We can create specific error types and compare types (i.e. err != errNotYetReadyToHandleRequest). Nothing needed right now; I'm willing to go back and correct instances where we have these comparisons post-beta.

kibbles-n-bytes · 2017-08-15T21:27:48Z

Just FYI @duglin, @arschles , it looks like there's a backlog of work in Travis at the moment since we share our concurrent build allotment across all the incubator projects, so it will probably take quite a while before Travis actually runs.

Jenkins passed, though, so up to you whether you feel that's enough!

arschles · 2017-08-15T21:30:29Z

@kibbles-n-bytes let's wait for travis. @vaikas-google I will merge this when it's ready to go. If you happen to be watching it, let me know when it's ready

Ville Aikas added 7 commits August 10, 2017 16:07

Implement the default plan in admission controller

6489d90

generated file changes

08276c6

clean up old cruft

dd24e5c

finish unit tests, passing

9241296

handle failures from list and test the not ready condition, cleanup

76c1d93

Rename the directory from default to defaultservicename to conform to…

ef585c4

… go style guide. Wire admission controller into the apiserver

generated type changes

dee796a

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 14, 2017

fix whitespace in the copyright section

b0af5fc

MHBauer assigned duglin Aug 14, 2017

MHBauer requested review from jpeeler and duglin August 14, 2017 20:23

kibbles-n-bytes reviewed Aug 14, 2017

View reviewed changes

Ville Aikas added 2 commits August 14, 2017 15:30

address PR comments

33417cc

fix test expectation to match the new error message for missing servi…

652a83b

…ce class

kibbles-n-bytes added the LGTM1 label Aug 14, 2017

duglin reviewed Aug 15, 2017

View reviewed changes

address PR comments

ddcbbad

duglin reviewed Aug 15, 2017

View reviewed changes

address one more PR comment

e7c5ab3

arschles approved these changes Aug 15, 2017

View reviewed changes

arschles added awaiting-ci LGTM2 labels Aug 15, 2017

MHBauer merged commit e7c5ab3 into kubernetes-retired:master Aug 16, 2017

cblecker unassigned duglin Jun 4, 2019

Add admission control to support default service plans. Resolves issue 1111 #1131

Add admission control to support default service plans. Resolves issue 1111 #1131

Uh oh!

Conversation

vaikas commented Aug 14, 2017 • edited by arschles Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibbles-n-bytes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibbles-n-bytes Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Aug 15, 2017

Uh oh!

pmorie commented Aug 15, 2017 via email

Uh oh!

duglin commented Aug 15, 2017

Uh oh!

duglin commented Aug 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vaikas commented Aug 14, 2017 •

edited by arschles

Loading

kibbles-n-bytes left a comment •

edited

Loading

kibbles-n-bytes Aug 14, 2017 •

edited

Loading

kibbles-n-bytes commented Aug 15, 2017 •

edited

Loading

kibbles-n-bytes commented Aug 15, 2017 •

edited

Loading

duglin Aug 15, 2017 •

edited

Loading