diff --git a/.github/PR_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md similarity index 100% rename from .github/PR_TEMPLATE.md rename to .github/PULL_REQUEST_TEMPLATE.md diff --git a/UPSTREAM-VERSION b/UPSTREAM-VERSION index 3a121d002a0..22f60e410f8 100644 --- a/UPSTREAM-VERSION +++ b/UPSTREAM-VERSION @@ -1 +1 @@ -v0.1.30 +v0.1.31 diff --git a/charts/catalog/Chart.yaml b/charts/catalog/Chart.yaml index d8488dd47c1..c6c95200812 100644 --- a/charts/catalog/Chart.yaml +++ b/charts/catalog/Chart.yaml @@ -1,3 +1,3 @@ name: catalog description: service-catalog API server and controller-manager helm chart -version: 0.1.30 +version: 0.1.31 diff --git a/charts/catalog/README.md b/charts/catalog/README.md index 36c91e33e19..ff5f342afd5 100644 --- a/charts/catalog/README.md +++ b/charts/catalog/README.md @@ -1,7 +1,7 @@ # Service Catalog Service Catalog is a Kubernetes Incubator project that provides a -Kubernetes-native workflow for integrating with +Kubernetes-native workflow for integrating with [Open Service Brokers](https://www.openservicebrokerapi.org/) to provision and bind to application dependencies like databases, object storage, message-oriented middleware, and more. @@ -40,7 +40,7 @@ chart and their default values. | Parameter | Description | Default | |-----------|-------------|---------| -| `image` | apiserver image to use | `quay.io/kubernetes-service-catalog/service-catalog:v0.1.30` | +| `image` | apiserver image to use | `quay.io/kubernetes-service-catalog/service-catalog:v0.1.31` | | `imagePullPolicy` | `imagePullPolicy` for the service catalog | `Always` | | `apiserver.annotations` | Annotations for apiserver pods | `{}` | | `apiserver.nodeSelector` | A nodeSelector value to apply to the apiserver pods. If not specified, no nodeSelector will be applied | | @@ -48,8 +48,9 @@ chart and their default values. | `apiserver.aggregator.groupPriorityMinimum` | The minimum priority the group should have. | `10000` | | `apiserver.aggregator.versionPriority` | The ordering of this API inside of the group | `20` | | `apiserver.tls.requestHeaderCA` | Base64-encoded CA used to validate request-header authentication, when receiving delegated authentication from an aggregator. If not set, the service catalog API server will inherit this CA from the `extension-apiserver-authentication` ConfigMap if available. | `nil` | -| `apiserver.service.type` | Type of service; valid values are `LoadBalancer` and `NodePort` | `NodePort` | +| `apiserver.service.type` | Type of service; valid values are `LoadBalancer` , `NodePort` and `ClusterIP` | `NodePort` | | `apiserver.service.nodePort.securePort` | If service type is `NodePort`, specifies a port in allowable range (e.g. 30000 - 32767 on minikube); The TLS-enabled endpoint will be exposed here | `30443` | +| `apiserver.service.clusterIP` | If service type is ClusterIP, specify clusterIP as `None` for `headless services` OR specify your own specific IP OR leave blank to let Kubernetes assign a cluster IP | | | `apiserver.storage.type` | The storage backend to use; the only valid value is `etcd`, left for other storages support in future, e.g. `crd` | `etcd` | | `apiserver.storage.etcd.useEmbedded` | If storage type is `etcd`: Whether to embed an etcd container in the apiserver pod; THIS IS INADEQUATE FOR PRODUCTION USE! | `true` | | `apiserver.storage.etcd.servers` | If storage type is `etcd`: etcd URL(s); override this if NOT using embedded etcd. Only etcd v3 is supported. | `http://localhost:2379` | diff --git a/charts/catalog/templates/apiserver-service.yaml b/charts/catalog/templates/apiserver-service.yaml index bf6aea21754..249cc2a5062 100644 --- a/charts/catalog/templates/apiserver-service.yaml +++ b/charts/catalog/templates/apiserver-service.yaml @@ -9,6 +9,11 @@ metadata: heritage: "{{ .Release.Service }}" spec: type: {{ .Values.apiserver.service.type }} + {{- if eq .Values.apiserver.service.type "ClusterIP" }} + {{- if .Values.apiserver.service.clusterIP }} + clusterIP: {{ .Values.apiserver.service.clusterIP }} + {{- end }} + {{- end }} selector: app: {{ template "fullname" . }}-apiserver ports: diff --git a/charts/catalog/values.yaml b/charts/catalog/values.yaml index c78e3de7274..f82f6d0b2a2 100644 --- a/charts/catalog/values.yaml +++ b/charts/catalog/values.yaml @@ -1,6 +1,6 @@ # Default values for Service Catalog # service-catalog image to use -image: quay.io/kubernetes-service-catalog/service-catalog:v0.1.30 +image: quay.io/kubernetes-service-catalog/service-catalog:v0.1.31 # imagePullPolicy for the service-catalog; valid values are "IfNotPresent", # "Never", and "Always" imagePullPolicy: Always diff --git a/charts/ups-broker/README.md b/charts/ups-broker/README.md index 2b3e6e91fab..bce32950480 100644 --- a/charts/ups-broker/README.md +++ b/charts/ups-broker/README.md @@ -34,7 +34,7 @@ Service Broker | Parameter | Description | Default | |-----------|-------------|---------| -| `image` | Image to use | `quay.io/kubernetes-service-catalog/user-broker:v0.1.29` | +| `image` | Image to use | `quay.io/kubernetes-service-catalog/user-broker:v0.1.31` | | `imagePullPolicy` | `imagePullPolicy` for the ups-broker | `Always` | Specify each parameter using the `--set key=value[,key=value]` argument to diff --git a/charts/ups-broker/values.yaml b/charts/ups-broker/values.yaml index d6970c88fd1..83ce2b07c74 100644 --- a/charts/ups-broker/values.yaml +++ b/charts/ups-broker/values.yaml @@ -1,6 +1,6 @@ # Default values for User-Provided Service Broker # Image to use -image: quay.io/kubernetes-service-catalog/user-broker:v0.1.29 +image: quay.io/kubernetes-service-catalog/user-broker:v0.1.31 # ImagePullPolicy; valid values are "IfNotPresent", "Never", and "Always" imagePullPolicy: Always # Whether the broker should also log to stderr instead of to files only diff --git a/cmd/svcat/broker/deregister_cmd.go b/cmd/svcat/broker/deregister_cmd.go index f6bcffb8e36..5c7059b990b 100644 --- a/cmd/svcat/broker/deregister_cmd.go +++ b/cmd/svcat/broker/deregister_cmd.go @@ -68,6 +68,6 @@ func (c *DeregisterCmd) Deregister() error { return err } - fmt.Fprintf(c.Context.Output, "Successfully removed broker %q", c.BrokerName) + fmt.Fprintf(c.Context.Output, "Successfully removed broker %q\n", c.BrokerName) return nil } diff --git a/cmd/svcat/broker/deregister_cmd_test.go b/cmd/svcat/broker/deregister_cmd_test.go index a60d37f9fa6..3a6d0ceb4ac 100644 --- a/cmd/svcat/broker/deregister_cmd_test.go +++ b/cmd/svcat/broker/deregister_cmd_test.go @@ -71,7 +71,7 @@ var _ = Describe("Deregister Command", func() { Expect(returnedName).To(Equal(brokerName)) output := outputBuffer.String() - Expect(output).To(Equal("Successfully removed broker \"foobarbroker\"")) + Expect(output).To(Equal("Successfully removed broker \"foobarbroker\"\n")) }) }) }) diff --git a/cmd/svcat/class/describe_cmd.go b/cmd/svcat/class/describe_cmd.go index daea641c3da..60851d9e74b 100644 --- a/cmd/svcat/class/describe_cmd.go +++ b/cmd/svcat/class/describe_cmd.go @@ -22,6 +22,7 @@ import ( "github.com/kubernetes-incubator/service-catalog/cmd/svcat/command" "github.com/kubernetes-incubator/service-catalog/cmd/svcat/output" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + servicecatalog "github.com/kubernetes-incubator/service-catalog/pkg/svcat/service-catalog" "github.com/spf13/cobra" ) @@ -88,7 +89,7 @@ func (c *describeCmd) describe() error { output.WriteClassDetails(c.Output, class) - plans, err := c.App.RetrievePlansByClass(class) + plans, err := c.App.RetrievePlans(servicecatalog.RetrievePlanOptions{Scope: servicecatalog.AllScope, ClassID: class.Name}) if err != nil { return err } diff --git a/cmd/svcat/instance/deprovision_cmd.go b/cmd/svcat/instance/deprovision_cmd.go index ab718a01e08..c48ab0f0b7b 100644 --- a/cmd/svcat/instance/deprovision_cmd.go +++ b/cmd/svcat/instance/deprovision_cmd.go @@ -77,10 +77,10 @@ func (c *deprovisonCmd) deprovision() error { fmt.Fprintln(c.Output, "Waiting for the instance to be deleted...") var instance *v1beta1.ServiceInstance - instance, err = c.App.WaitForInstance(c.Namespace, c.instanceName, c.Interval, c.Timeout) + instance, err = c.App.WaitForInstanceToNotExist(c.Namespace, c.instanceName, c.Interval, c.Timeout) // The instance failed to deprovision cleanly, dump out more information on why - if c.App.IsInstanceFailed(instance) { + if instance != nil && c.App.IsInstanceFailed(instance) { output.WriteInstanceDetails(c.Output, instance) } } diff --git a/cmd/svcat/main.go b/cmd/svcat/main.go index 3804dca5697..c124c8a1fa4 100644 --- a/cmd/svcat/main.go +++ b/cmd/svcat/main.go @@ -119,6 +119,7 @@ func buildRootCommand(cxt *command.Context) *cobra.Command { cmd.AddCommand(newGetCmd(cxt)) cmd.AddCommand(newDescribeCmd(cxt)) cmd.AddCommand(broker.NewRegisterCmd(cxt)) + cmd.AddCommand(broker.NewDeregisterCmd(cxt)) cmd.AddCommand(instance.NewProvisionCmd(cxt)) cmd.AddCommand(instance.NewDeprovisionCmd(cxt)) cmd.AddCommand(binding.NewBindCmd(cxt)) diff --git a/cmd/svcat/output/plan.go b/cmd/svcat/output/plan.go index 631a5ec5480..1aa8064aeb1 100644 --- a/cmd/svcat/output/plan.go +++ b/cmd/svcat/output/plan.go @@ -35,7 +35,7 @@ func getPlanStatusShort(status v1beta1.ClusterServicePlanStatus) string { // ByAge implements sort.Interface for []Person based on // the Age field. -type byClass []v1beta1.ClusterServicePlan +type byClass []servicecatalog.Plan func (a byClass) Len() int { return len(a) @@ -44,45 +44,44 @@ func (a byClass) Swap(i, j int) { a[i], a[j] = a[j], a[i] } func (a byClass) Less(i, j int) bool { - return a[i].Spec.ClusterServiceClassRef.Name < a[j].Spec.ClusterServiceClassRef.Name + return a[i].GetClassID() < a[j].GetClassID() } -func writePlanListTable(w io.Writer, plans []v1beta1.ClusterServicePlan, classNames map[string]string) { +func writePlanListTable(w io.Writer, plans []servicecatalog.Plan, classNames map[string]string) { sort.Sort(byClass(plans)) t := NewListTable(w) t.SetHeader([]string{ "Name", + "Namespace", "Class", "Description", }) for _, plan := range plans { t.Append([]string{ - plan.Spec.ExternalName, - classNames[plan.Spec.ClusterServiceClassRef.Name], - plan.Spec.Description, + plan.GetExternalName(), + plan.GetNamespace(), + classNames[plan.GetClassID()], + plan.GetDescription(), }) } - t.SetVariableColumn(3) + t.SetVariableColumn(4) t.Render() } // WritePlanList prints a list of plans in the specified output format. -func WritePlanList(w io.Writer, outputFormat string, plans []v1beta1.ClusterServicePlan, classes []servicecatalog.Class) { +func WritePlanList(w io.Writer, outputFormat string, plans []servicecatalog.Plan, classes []servicecatalog.Class) { classNames := map[string]string{} for _, class := range classes { classNames[class.GetName()] = class.GetExternalName() } - list := v1beta1.ClusterServicePlanList{ - Items: plans, - } switch outputFormat { case FormatJSON: - writeJSON(w, list) + writeJSON(w, plans) case FormatYAML: - writeYAML(w, list, 0) + writeYAML(w, plans, 0) case FormatTable: writePlanListTable(w, plans, classNames) } @@ -99,12 +98,12 @@ func WritePlan(w io.Writer, outputFormat string, plan v1beta1.ClusterServicePlan case FormatTable: classNames := map[string]string{} classNames[class.Name] = class.Spec.ExternalName - writePlanListTable(w, []v1beta1.ClusterServicePlan{plan}, classNames) + writePlanListTable(w, []servicecatalog.Plan{&plan}, classNames) } } // WriteAssociatedPlans prints a list of plans associated with a class. -func WriteAssociatedPlans(w io.Writer, plans []v1beta1.ClusterServicePlan) { +func WriteAssociatedPlans(w io.Writer, plans []servicecatalog.Plan) { fmt.Fprintln(w, "\nPlans:") if len(plans) == 0 { fmt.Fprintln(w, "No plans defined") @@ -118,8 +117,8 @@ func WriteAssociatedPlans(w io.Writer, plans []v1beta1.ClusterServicePlan) { }) for _, plan := range plans { t.Append([]string{ - plan.Spec.ExternalName, - plan.Spec.Description, + plan.GetExternalName(), + plan.GetDescription(), }) } t.Render() diff --git a/cmd/svcat/plan/get_cmd.go b/cmd/svcat/plan/get_cmd.go index 3cd4bb1b716..416aea30aae 100644 --- a/cmd/svcat/plan/get_cmd.go +++ b/cmd/svcat/plan/get_cmd.go @@ -28,7 +28,8 @@ import ( ) type getCmd struct { - *command.Context + *command.Namespaced + *command.Scoped *command.Formatted lookupByUUID bool uuid string @@ -40,17 +41,20 @@ type getCmd struct { } // NewGetCmd builds a "svcat get plans" command -func NewGetCmd(cxt *command.Context) *cobra.Command { +func NewGetCmd(ctx *command.Context) *cobra.Command { getCmd := &getCmd{ - Context: cxt, - Formatted: command.NewFormatted(), + Namespaced: command.NewNamespaced(ctx), + Scoped: command.NewScoped(), + Formatted: command.NewFormatted(), } cmd := &cobra.Command{ Use: "plans [NAME]", Aliases: []string{"plan", "pl"}, - Short: "List plans, optionally filtered by name or class", + Short: "List plans, optionally filtered by name, class, scope or namespace", Example: command.NormalizeExamples(` svcat get plans + svcat get plans --scope cluster + svcat get plans --scope namespace --namespace dev svcat get plan PLAN_NAME svcat get plan CLASS_NAME/PLAN_NAME svcat get plan --uuid PLAN_UUID @@ -77,6 +81,8 @@ func NewGetCmd(cxt *command.Context) *cobra.Command { "Filter plans based on class. When --uuid is specified, the class name is interpreted as a uuid.", ) getCmd.AddOutputFlags(cmd.Flags()) + getCmd.AddNamespaceFlags(cmd.Flags(), true) + getCmd.AddScopedFlags(cmd.Flags(), true) return cmd } @@ -117,16 +123,19 @@ func (c *getCmd) Run() error { func (c *getCmd) getAll() error { // Retrieve the classes as well because plans don't have the external class name - // TODO: When we implement ns-scoped support for get plans, we need to pass in the current namespace classOpts := servicecatalog.ScopeOptions{ - Scope: servicecatalog.AllScope, + Namespace: c.Namespace, + Scope: c.Scope, } classes, err := c.App.RetrieveClasses(classOpts) if err != nil { return fmt.Errorf("unable to list classes (%s)", err) } - var opts *servicecatalog.FilterOptions + opts := servicecatalog.RetrievePlanOptions{ + Namespace: c.Namespace, + Scope: c.Scope, + } if c.classFilter != "" { if !c.lookupByUUID { // Map the external class name to the class name. @@ -137,9 +146,7 @@ func (c *getCmd) getAll() error { } } } - opts = &servicecatalog.FilterOptions{ - ClassID: c.classUUID, - } + opts.ClassID = c.classUUID } plans, err := c.App.RetrievePlans(opts) diff --git a/cmd/svcat/plan/get_cmd_test.go b/cmd/svcat/plan/get_cmd_test.go new file mode 100644 index 00000000000..14f56860495 --- /dev/null +++ b/cmd/svcat/plan/get_cmd_test.go @@ -0,0 +1,302 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package plan + +import ( + "bytes" + "errors" + "strings" + "testing" + + "github.com/kubernetes-incubator/service-catalog/cmd/svcat/command" + "github.com/kubernetes-incubator/service-catalog/cmd/svcat/test" + "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + svcatfake "github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/clientset/fake" + "github.com/kubernetes-incubator/service-catalog/pkg/svcat" + "github.com/kubernetes-incubator/service-catalog/pkg/svcat/service-catalog" + servicecatalogfakes "github.com/kubernetes-incubator/service-catalog/pkg/svcat/service-catalog/service-catalogfakes" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + k8sfake "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" +) + +func TestListPlans(t *testing.T) { + const ns = "default" + testcases := []struct { + name string + scope servicecatalog.Scope + fakeClusterPlans []string + fakeNamespacedPlans []string + wantResults int + wantOutput string + wantError bool + }{ + { + name: "get plans from cluster and current namespace", + scope: servicecatalog.AllScope, + fakeClusterPlans: []string{"my-cluster-plan"}, + fakeNamespacedPlans: []string{"my-ns-plan"}, + wantResults: 2, + wantOutput: "my-ns-plan\nmy-cluster-plan", + wantError: false, + }, + { + name: "get plans from cluster only", + scope: servicecatalog.ClusterScope, + fakeClusterPlans: []string{"my-cluster-plan"}, + fakeNamespacedPlans: []string{"my-ns-plan"}, + wantResults: 1, + wantOutput: "my-cluster-plan", + wantError: false, + }, + { + name: "get plans current namespace only", + scope: servicecatalog.NamespaceScope, + fakeClusterPlans: []string{"my-cluster-plan"}, + fakeNamespacedPlans: []string{"my-ns-plan"}, + wantResults: 1, + wantOutput: "my-ns-plan", + wantError: false, + }, + { + name: "get plans - bubbles cluster errors", + scope: servicecatalog.AllScope, + fakeClusterPlans: []string{"badplan"}, + fakeNamespacedPlans: []string{"my-ns-plan"}, + wantOutput: "unable to list cluster-scoped plans (sabotaged)", + wantError: true, + }, + { + name: "get plans - bubbles namespace errors", + scope: servicecatalog.AllScope, + fakeClusterPlans: []string{"my-cluster-plan"}, + fakeNamespacedPlans: []string{"badplan"}, + wantOutput: "unable to list plans in \"default\" (sabotaged)", + wantError: true, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + + // Setup fake data for the app + k8sClient := k8sfake.NewSimpleClientset() + var fakes []runtime.Object + for _, name := range tc.fakeClusterPlans { + fakes = append(fakes, &v1beta1.ClusterServicePlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Spec: v1beta1.ClusterServicePlanSpec{ + CommonServicePlanSpec: v1beta1.CommonServicePlanSpec{ + ExternalName: name, + }, + }, + }) + } + for _, name := range tc.fakeNamespacedPlans { + fakes = append(fakes, &v1beta1.ServicePlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns, + }, + Spec: v1beta1.ServicePlanSpec{ + CommonServicePlanSpec: v1beta1.CommonServicePlanSpec{ + ExternalName: name, + }, + }, + }) + } + svcatClient := svcatfake.NewSimpleClientset(fakes...) + output := &bytes.Buffer{} + fakeApp, _ := svcat.NewApp(k8sClient, svcatClient, ns) + cxt := svcattest.NewContext(output, fakeApp) + + // Sabotage the get calls, if necessary + for _, name := range tc.fakeClusterPlans { + if strings.Contains(name, "bad") { + svcatClient.PrependReactor("list", "clusterserviceplans", + func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, errors.New("sabotaged") + }) + break + } + } + for _, name := range tc.fakeNamespacedPlans { + if strings.Contains(name, "bad") { + svcatClient.PrependReactor("list", "serviceplans", + func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, errors.New("sabotaged") + }) + break + } + } + + // Initialize the command arguments + cmd := &getCmd{ + Namespaced: command.NewNamespaced(cxt), + Scoped: command.NewScoped(), + Formatted: command.NewFormatted(), + } + cmd.Namespace = ns + cmd.Scope = tc.scope + + err := cmd.Run() + + if tc.wantError && err == nil { + t.Errorf("expected a non-zero exit code, but the command succeeded") + } + if !tc.wantError && err != nil { + t.Errorf("expected the command to succeed but it failed with %q", err) + } + + gotOutput := output.String() + if err != nil { + gotOutput += err.Error() + } + if !svcattest.OutputMatches(gotOutput, tc.wantOutput, true) { + t.Errorf("unexpected output \n\nWANT:\n%q\n\nGOT:\n%q\n", tc.wantOutput, gotOutput) + } + }) + } +} + +var _ = Describe("Get Plans Command", func() { + Describe("NewGetPlansCmd", func() { + It("Builds and returns a cobra command", func() { + cxt := &command.Context{} + cmd := NewGetCmd(cxt) + Expect(*cmd).NotTo(BeNil()) + Expect(cmd.Use).To(Equal("plans [NAME]")) + Expect(cmd.Short).To(ContainSubstring("List plans, optionally filtered by name, class, scope or namespace")) + Expect(cmd.Example).To(ContainSubstring("svcat get plans")) + Expect(cmd.Example).To(ContainSubstring("svcat get plans --scope cluster")) + Expect(cmd.Example).To(ContainSubstring("svcat get plans --scope namespace --namespace dev")) + Expect(len(cmd.Aliases)).To(Equal(2)) + }) + }) + Describe("Validate", func() { + It("allows plan name arg to be empty", func() { + cmd := &getCmd{} + err := cmd.Validate([]string{}) + Expect(err).To(BeNil()) + }) + It("optionally parses the plan name argument", func() { + cmd := &getCmd{} + err := cmd.Validate([]string{"myplan"}) + Expect(err).To(BeNil()) + Expect(cmd.name).To(Equal("myplan")) + }) + }) + Describe("Run", func() { + It("Calls the pkg/svcat libs RetrievePlans with namespace scope and current namespace", func() { + outputBuffer := &bytes.Buffer{} + + fakeApp, _ := svcat.NewApp(nil, nil, "default") + fakeSDK := new(servicecatalogfakes.FakeSvcatClient) + fakeSDK.RetrievePlansReturns( + []servicecatalog.Plan{&v1beta1.ServicePlan{ObjectMeta: metav1.ObjectMeta{Name: "myplan", Namespace: "default"}}}, + nil) + fakeApp.SvcatClient = fakeSDK + cmd := getCmd{ + Namespaced: &command.Namespaced{Context: svcattest.NewContext(outputBuffer, fakeApp)}, + Scoped: command.NewScoped(), + } + cmd.Namespace = "default" + cmd.Scope = servicecatalog.NamespaceScope + + err := cmd.Run() + + Expect(err).NotTo(HaveOccurred()) + scopeArg := fakeSDK.RetrievePlansArgsForCall(0) + Expect(scopeArg).To(Equal(servicecatalog.ScopeOptions{ + Namespace: "default", + Scope: servicecatalog.NamespaceScope, + })) + + output := outputBuffer.String() + Expect(output).To(ContainSubstring("myplan")) + }) + It("Calls the pkg/svcat libs RetrievePlans with namespace scope and all namespaces", func() { + outputBuffer := &bytes.Buffer{} + + fakeApp, _ := svcat.NewApp(nil, nil, "default") + fakeSDK := new(servicecatalogfakes.FakeSvcatClient) + fakeSDK.RetrievePlansReturns( + []servicecatalog.Plan{ + &v1beta1.ServicePlan{ObjectMeta: metav1.ObjectMeta{Name: "myplan", Namespace: "default"}}, + &v1beta1.ServicePlan{ObjectMeta: metav1.ObjectMeta{Name: "anotherplan", Namespace: "test-ns"}}, + }, + nil) + fakeApp.SvcatClient = fakeSDK + cmd := getCmd{ + Namespaced: &command.Namespaced{Context: svcattest.NewContext(outputBuffer, fakeApp)}, + Scoped: command.NewScoped(), + } + cmd.Namespace = "" + cmd.Scope = servicecatalog.NamespaceScope + + err := cmd.Run() + + Expect(err).NotTo(HaveOccurred()) + scopeArg := fakeSDK.RetrievePlansArgsForCall(0) + Expect(scopeArg).To(Equal(servicecatalog.ScopeOptions{ + Namespace: "", + Scope: servicecatalog.NamespaceScope, + })) + + output := outputBuffer.String() + Expect(output).To(ContainSubstring("myplan")) + Expect(output).To(ContainSubstring("anotherplan")) + }) + It("Calls the pkg/svcat libs RetrievePlans with all scope and current namespaces", func() { + outputBuffer := &bytes.Buffer{} + + fakeApp, _ := svcat.NewApp(nil, nil, "default") + fakeSDK := new(servicecatalogfakes.FakeSvcatClient) + fakeSDK.RetrievePlansReturns( + []servicecatalog.Plan{ + &v1beta1.ClusterServicePlan{ObjectMeta: metav1.ObjectMeta{Name: "myplan"}}, + &v1beta1.ServicePlan{ObjectMeta: metav1.ObjectMeta{Name: "anotherplan", Namespace: "default"}}, + }, + nil) + fakeApp.SvcatClient = fakeSDK + cmd := getCmd{ + Namespaced: &command.Namespaced{Context: svcattest.NewContext(outputBuffer, fakeApp)}, + Scoped: command.NewScoped(), + } + cmd.Namespace = "default" + cmd.Scope = servicecatalog.AllScope + + err := cmd.Run() + + Expect(err).NotTo(HaveOccurred()) + scopeArg := fakeSDK.RetrievePlansArgsForCall(0) + Expect(scopeArg).To(Equal(servicecatalog.ScopeOptions{ + Namespace: "default", + Scope: servicecatalog.AllScope, + })) + + output := outputBuffer.String() + Expect(output).To(ContainSubstring("myplan")) + Expect(output).To(ContainSubstring("anotherplan")) + }) + }) +}) diff --git a/cmd/svcat/svcat_test.go b/cmd/svcat/svcat_test.go index 9a858f8d15e..6a8f3db4d1e 100644 --- a/cmd/svcat/svcat_test.go +++ b/cmd/svcat/svcat_test.go @@ -187,6 +187,7 @@ func TestCommandOutput(t *testing.T) { {name: "get broker (yaml)", cmd: "get broker ups-broker -o yaml", golden: "output/get-broker.yaml"}, {name: "describe broker", cmd: "describe broker ups-broker", golden: "output/describe-broker.txt"}, {name: "register broker", cmd: "register ups-broker --url http://upsbroker.com", golden: "output/register-broker.txt"}, + {name: "deregister broker", cmd: "deregister ups-broker", golden: "output/deregister-broker.txt"}, {name: "list all classes", cmd: "get classes", golden: "output/get-classes.txt"}, {name: "list all classes (json)", cmd: "get classes -o json", golden: "output/get-classes.json"}, @@ -202,6 +203,9 @@ func TestCommandOutput(t *testing.T) { {name: "list all plans", cmd: "get plans", golden: "output/get-plans.txt"}, {name: "list all plans (json)", cmd: "get plans -o json", golden: "output/get-plans.json"}, {name: "list all plans (yaml)", cmd: "get plans -o yaml", golden: "output/get-plans.yaml"}, + {name: "list all namespaced plans", cmd: "get plans --scope namespace", golden: "output/get-namespaced-plans.txt"}, + {name: "list all namespaced plans (json)", cmd: "get plans --scope namespace -o json", golden: "output/get-namespaced-plans.json"}, + {name: "list all namespaced plans (yaml)", cmd: "get plans --scope namespace -o yaml", golden: "output/get-namespaced-plans.yaml"}, {name: "get plan by name", cmd: "get plan default", golden: "output/get-plan.txt"}, {name: "get plan by name (json)", cmd: "get plan default -o json", golden: "output/get-plan.json"}, {name: "get plan by name (yaml)", cmd: "get plan default -o yaml", golden: "output/get-plan.yaml"}, @@ -236,8 +240,6 @@ func TestCommandOutput(t *testing.T) { {name: "provision instance", cmd: "provision ups-instance -n test-ns --class user-provided-service --plan default", golden: "output/provision-instance.txt"}, {name: "provision instance and wait", cmd: "provision ups-instance -n test-ns --class user-provided-service --plan default --wait", golden: "output/provision-instance-and-wait.txt"}, {name: "deprovision instance", cmd: "deprovision ups-instance -n test-ns", golden: "output/deprovision-instance.txt"}, - {name: "deprovision instance and wait", cmd: "deprovision ups-instance -n test-ns --wait", golden: "output/deprovision-instance-and-wait.txt"}, - {name: "list all bindings in a namespace", cmd: "get bindings -n test-ns", golden: "output/get-bindings.txt"}, {name: "list all bindings in a namespace (json)", cmd: "get bindings -n test-ns -o json", golden: "output/get-bindings.json"}, {name: "list all bindings in a namespace (yaml)", cmd: "get bindings -n test-ns -o yaml", golden: "output/get-bindings.yaml"}, @@ -648,6 +650,7 @@ func apihandler(w http.ResponseWriter, r *http.Request) { match = filepath.Join("core", coreMatch[1]) } + match = strings.Replace(match, "?", "_", -1) // windows doesn't allow '?' in filenames relpath, err := url.PathUnescape(match) if err != nil { w.WriteHeader(500) diff --git a/cmd/svcat/testdata/output/completion-bash.txt b/cmd/svcat/testdata/output/completion-bash.txt index 16f3c310505..0621b5e605b 100644 --- a/cmd/svcat/testdata/output/completion-bash.txt +++ b/cmd/svcat/testdata/output/completion-bash.txt @@ -388,6 +388,28 @@ _svcat_deprovision() noun_aliases=() } +_svcat_deregister() +{ + last_command="svcat_deregister" + commands=() + + flags=() + two_word_flags=() + local_nonpersistent_flags=() + flags_with_completion=() + flags_completion=() + + flags+=("--context=") + flags+=("--kubeconfig=") + flags+=("--logtostderr") + flags+=("--v=") + two_word_flags+=("-v") + + must_have_one_flag=() + must_have_one_noun=() + noun_aliases=() +} + _svcat_describe_binding() { last_command="svcat_describe_binding" @@ -685,12 +707,19 @@ _svcat_get_plans() flags_with_completion=() flags_completion=() + flags+=("--all-namespaces") + local_nonpersistent_flags+=("--all-namespaces") flags+=("--class=") two_word_flags+=("-c") local_nonpersistent_flags+=("--class=") + flags+=("--namespace=") + two_word_flags+=("-n") + local_nonpersistent_flags+=("--namespace=") flags+=("--output=") two_word_flags+=("-o") local_nonpersistent_flags+=("--output=") + flags+=("--scope=") + local_nonpersistent_flags+=("--scope=") flags+=("--uuid") flags+=("-u") local_nonpersistent_flags+=("--uuid") @@ -1036,6 +1065,7 @@ _svcat_root_command() commands+=("completion") commands+=("create") commands+=("deprovision") + commands+=("deregister") commands+=("describe") commands+=("get") commands+=("install") diff --git a/cmd/svcat/testdata/output/completion-zsh.txt b/cmd/svcat/testdata/output/completion-zsh.txt index 711c9cb7b5c..96b9536d3d9 100644 --- a/cmd/svcat/testdata/output/completion-zsh.txt +++ b/cmd/svcat/testdata/output/completion-zsh.txt @@ -522,6 +522,28 @@ _svcat_deprovision() noun_aliases=() } +_svcat_deregister() +{ + last_command="svcat_deregister" + commands=() + + flags=() + two_word_flags=() + local_nonpersistent_flags=() + flags_with_completion=() + flags_completion=() + + flags+=("--context=") + flags+=("--kubeconfig=") + flags+=("--logtostderr") + flags+=("--v=") + two_word_flags+=("-v") + + must_have_one_flag=() + must_have_one_noun=() + noun_aliases=() +} + _svcat_describe_binding() { last_command="svcat_describe_binding" @@ -819,12 +841,19 @@ _svcat_get_plans() flags_with_completion=() flags_completion=() + flags+=("--all-namespaces") + local_nonpersistent_flags+=("--all-namespaces") flags+=("--class=") two_word_flags+=("-c") local_nonpersistent_flags+=("--class=") + flags+=("--namespace=") + two_word_flags+=("-n") + local_nonpersistent_flags+=("--namespace=") flags+=("--output=") two_word_flags+=("-o") local_nonpersistent_flags+=("--output=") + flags+=("--scope=") + local_nonpersistent_flags+=("--scope=") flags+=("--uuid") flags+=("-u") local_nonpersistent_flags+=("--uuid") @@ -1170,6 +1199,7 @@ _svcat_root_command() commands+=("completion") commands+=("create") commands+=("deprovision") + commands+=("deregister") commands+=("describe") commands+=("get") commands+=("install") diff --git a/cmd/svcat/testdata/output/deregister-broker.txt b/cmd/svcat/testdata/output/deregister-broker.txt new file mode 100644 index 00000000000..5607f3751eb --- /dev/null +++ b/cmd/svcat/testdata/output/deregister-broker.txt @@ -0,0 +1 @@ +Successfully removed broker "ups-broker" diff --git a/cmd/svcat/testdata/output/get-namespaced-plans.json b/cmd/svcat/testdata/output/get-namespaced-plans.json new file mode 100644 index 00000000000..cd5f15c5e3d --- /dev/null +++ b/cmd/svcat/testdata/output/get-namespaced-plans.json @@ -0,0 +1,23 @@ +[ + { + "metadata": { + "name": "ac9694d9-7ea2-af93-467b-860647926d52", + "namespace": "default", + "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/serviceplans/ac9694d9-7ea2-af93-467b-860647926d52", + "uid": "0242ac11-11e7-f711-aa44-7b3d01900005", + "resourceVersion": "4", + "creationTimestamp": "2018-09-04T23:11:31Z" + }, + "spec": { + "externalName": "user-provided-namespace-plan", + "externalID": "ac9694d9-7ea2-af93-467b-860647926d52", + "description": "Sample namespace plan description", + "free": true, + "serviceBrokerName": "namespace-ups-broker", + "serviceClassRef": {} + }, + "status": { + "removedFromBrokerCatalog": false + } + } +] \ No newline at end of file diff --git a/cmd/svcat/testdata/output/get-namespaced-plans.txt b/cmd/svcat/testdata/output/get-namespaced-plans.txt new file mode 100644 index 00000000000..625d3f35ae5 --- /dev/null +++ b/cmd/svcat/testdata/output/get-namespaced-plans.txt @@ -0,0 +1,4 @@ + NAME NAMESPACE CLASS DESCRIPTION ++------------------------------+-----------+-------+--------------------------------+ + user-provided-namespace-plan default Sample namespace plan + description diff --git a/cmd/svcat/testdata/output/get-namespaced-plans.yaml b/cmd/svcat/testdata/output/get-namespaced-plans.yaml new file mode 100644 index 00000000000..26bc4850dc2 --- /dev/null +++ b/cmd/svcat/testdata/output/get-namespaced-plans.yaml @@ -0,0 +1,16 @@ +- metadata: + creationTimestamp: 2018-09-04T23:11:31Z + name: ac9694d9-7ea2-af93-467b-860647926d52 + namespace: default + resourceVersion: "4" + selfLink: /apis/servicecatalog.k8s.io/v1beta1/serviceplans/ac9694d9-7ea2-af93-467b-860647926d52 + uid: 0242ac11-11e7-f711-aa44-7b3d01900005 + spec: + description: Sample namespace plan description + externalID: ac9694d9-7ea2-af93-467b-860647926d52 + externalName: user-provided-namespace-plan + free: true + serviceBrokerName: namespace-ups-broker + serviceClassRef: {} + status: + removedFromBrokerCatalog: false diff --git a/cmd/svcat/testdata/output/get-plan.txt b/cmd/svcat/testdata/output/get-plan.txt index 0403ae796a0..242ed00303e 100644 --- a/cmd/svcat/testdata/output/get-plan.txt +++ b/cmd/svcat/testdata/output/get-plan.txt @@ -1,3 +1,3 @@ - NAME CLASS DESCRIPTION -+---------+-----------------------+-------------------------+ - default user-provided-service Sample plan description + NAME NAMESPACE CLASS DESCRIPTION ++---------+-----------+-----------------------+-------------------------+ + default user-provided-service Sample plan description diff --git a/cmd/svcat/testdata/output/get-plans-by-class.txt b/cmd/svcat/testdata/output/get-plans-by-class.txt index 162882bb7c4..061d32e49f8 100644 --- a/cmd/svcat/testdata/output/get-plans-by-class.txt +++ b/cmd/svcat/testdata/output/get-plans-by-class.txt @@ -1,4 +1,4 @@ - NAME CLASS DESCRIPTION -+---------+-----------------------+-------------------------+ - default user-provided-service Sample plan description - premium user-provided-service Premium plan + NAME NAMESPACE CLASS DESCRIPTION ++---------+-----------+-----------------------+-------------------------+ + default user-provided-service Sample plan description + premium user-provided-service Premium plan diff --git a/cmd/svcat/testdata/output/get-plans.json b/cmd/svcat/testdata/output/get-plans.json index b4ed84b8fae..5aff55d072c 100644 --- a/cmd/svcat/testdata/output/get-plans.json +++ b/cmd/svcat/testdata/output/get-plans.json @@ -1,129 +1,147 @@ -{ - "metadata": {}, - "items": [ - { - "metadata": { - "name": "86064792-7ea2-467b-af93-ac9694d96d52", - "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/clusterserviceplans/86064792-7ea2-467b-af93-ac9694d96d52", - "uid": "7b3d0190-f711-11e7-aa44-0242ac110005", - "resourceVersion": "4", - "creationTimestamp": "2018-01-11T20:53:31Z" - }, - "spec": { - "externalName": "default", - "externalID": "86064792-7ea2-467b-af93-ac9694d96d52", - "description": "Sample plan description", - "free": true, - "clusterServiceBrokerName": "ups-broker", - "clusterServiceClassRef": { - "name": "4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468" - } - }, - "status": { - "removedFromBrokerCatalog": false +[ + { + "metadata": { + "name": "86064792-7ea2-467b-af93-ac9694d96d52", + "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/clusterserviceplans/86064792-7ea2-467b-af93-ac9694d96d52", + "uid": "7b3d0190-f711-11e7-aa44-0242ac110005", + "resourceVersion": "4", + "creationTimestamp": "2018-01-11T20:53:31Z" + }, + "spec": { + "externalName": "default", + "externalID": "86064792-7ea2-467b-af93-ac9694d96d52", + "description": "Sample plan description", + "free": true, + "clusterServiceBrokerName": "ups-broker", + "clusterServiceClassRef": { + "name": "4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468" } }, - { - "metadata": { - "name": "cc0d7529-18e8-416d-8946-6f7456acd589", - "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/clusterserviceplans/cc0d7529-18e8-416d-8946-6f7456acd589", - "uid": "7b497b48-f711-11e7-aa44-0242ac110005", - "resourceVersion": "5", - "creationTimestamp": "2018-01-11T20:53:31Z" - }, - "spec": { - "externalName": "premium", - "externalID": "cc0d7529-18e8-416d-8946-6f7456acd589", - "description": "Premium plan", - "free": false, - "instanceCreateParameterSchema": { - "properties": { - "testInstanceProperty": { - "description": "A test instance property.", - "type": "string" - } - }, - "required": [ - "testInstanceProperty" - ], - "type": "object" + "status": { + "removedFromBrokerCatalog": false + } + }, + { + "metadata": { + "name": "cc0d7529-18e8-416d-8946-6f7456acd589", + "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/clusterserviceplans/cc0d7529-18e8-416d-8946-6f7456acd589", + "uid": "7b497b48-f711-11e7-aa44-0242ac110005", + "resourceVersion": "5", + "creationTimestamp": "2018-01-11T20:53:31Z" + }, + "spec": { + "externalName": "premium", + "externalID": "cc0d7529-18e8-416d-8946-6f7456acd589", + "description": "Premium plan", + "free": false, + "instanceCreateParameterSchema": { + "properties": { + "testInstanceProperty": { + "description": "A test instance property.", + "type": "string" + } }, - "serviceBindingCreateParameterSchema": { - "properties": { - "testBindingProperty": { - "description": "A test binding property.", - "type": "string" - } - }, - "required": [ - "testBindingProperty" - ], - "type": "object" + "required": [ + "testInstanceProperty" + ], + "type": "object" + }, + "serviceBindingCreateParameterSchema": { + "properties": { + "testBindingProperty": { + "description": "A test binding property.", + "type": "string" + } }, - "clusterServiceBrokerName": "ups-broker", - "clusterServiceClassRef": { - "name": "4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468" - } + "required": [ + "testBindingProperty" + ], + "type": "object" }, - "status": { - "removedFromBrokerCatalog": false + "clusterServiceBrokerName": "ups-broker", + "clusterServiceClassRef": { + "name": "4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468" } }, - { - "metadata": { - "name": "25b9b299-b0b3-4e14-aa1a-242eeb788aca", - "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/clusterserviceplans/25b9b299-b0b3-4e14-aa1a-242eeb788aca", - "uid": "7b3d0190-f711-11e7-aa44-0242ac110005", - "resourceVersion": "4", - "creationTimestamp": "2018-01-11T20:53:31Z" - }, - "spec": { - "externalName": "default", - "externalID": "090b5eac-dfa4-49f3-827d-8bcaf3a5bd7c", - "description": "Another sample plan description that's really really really really really, kinda, wide", - "free": true, - "clusterServiceBrokerName": "ups-broker", - "clusterServiceClassRef": { - "name": "f1a80068-e366-494e-92d6-a0782337945b" - } - }, - "status": { - "removedFromBrokerCatalog": false + "status": { + "removedFromBrokerCatalog": false + } + }, + { + "metadata": { + "name": "25b9b299-b0b3-4e14-aa1a-242eeb788aca", + "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/clusterserviceplans/25b9b299-b0b3-4e14-aa1a-242eeb788aca", + "uid": "7b3d0190-f711-11e7-aa44-0242ac110005", + "resourceVersion": "4", + "creationTimestamp": "2018-01-11T20:53:31Z" + }, + "spec": { + "externalName": "default", + "externalID": "090b5eac-dfa4-49f3-827d-8bcaf3a5bd7c", + "description": "Another sample plan description that's really really really really really, kinda, wide", + "free": true, + "clusterServiceBrokerName": "ups-broker", + "clusterServiceClassRef": { + "name": "f1a80068-e366-494e-92d6-a0782337945b" } }, - { - "metadata": { - "name": "c1dbdafe-f987-4d36-8c9b-2aaaff740d4a", - "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/clusterserviceplans/c1dbdafe-f987-4d36-8c9b-2aaaff740d4a", - "uid": "357feef4-0445-4a4c-a3bf-99762f2d36a2", - "resourceVersion": "5", - "creationTimestamp": "2018-01-11T20:53:31Z" - }, - "spec": { - "externalName": "premium", - "externalID": "adf134dc-0b0d-4c74-a6da-6ee1a5e34b8a", - "description": "Another premium plan", - "free": false, - "instanceCreateParameterSchema": { - "properties": { - "testInstanceProperty": { - "description": "Another test instance property.", - "type": "string" - } - }, - "required": [ - "testInstanceProperty" - ], - "type": "object" + "status": { + "removedFromBrokerCatalog": false + } + }, + { + "metadata": { + "name": "c1dbdafe-f987-4d36-8c9b-2aaaff740d4a", + "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/clusterserviceplans/c1dbdafe-f987-4d36-8c9b-2aaaff740d4a", + "uid": "357feef4-0445-4a4c-a3bf-99762f2d36a2", + "resourceVersion": "5", + "creationTimestamp": "2018-01-11T20:53:31Z" + }, + "spec": { + "externalName": "premium", + "externalID": "adf134dc-0b0d-4c74-a6da-6ee1a5e34b8a", + "description": "Another premium plan", + "free": false, + "instanceCreateParameterSchema": { + "properties": { + "testInstanceProperty": { + "description": "Another test instance property.", + "type": "string" + } }, - "clusterServiceBrokerName": "ups-broker", - "clusterServiceClassRef": { - "name": "f1a80068-e366-494e-92d6-a0782337945b" - } + "required": [ + "testInstanceProperty" + ], + "type": "object" }, - "status": { - "removedFromBrokerCatalog": false + "clusterServiceBrokerName": "ups-broker", + "clusterServiceClassRef": { + "name": "f1a80068-e366-494e-92d6-a0782337945b" } + }, + "status": { + "removedFromBrokerCatalog": false + } + }, + { + "metadata": { + "name": "ac9694d9-7ea2-af93-467b-860647926d52", + "namespace": "default", + "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/serviceplans/ac9694d9-7ea2-af93-467b-860647926d52", + "uid": "0242ac11-11e7-f711-aa44-7b3d01900005", + "resourceVersion": "4", + "creationTimestamp": "2018-09-04T23:11:31Z" + }, + "spec": { + "externalName": "user-provided-namespace-plan", + "externalID": "ac9694d9-7ea2-af93-467b-860647926d52", + "description": "Sample namespace plan description", + "free": true, + "serviceBrokerName": "namespace-ups-broker", + "serviceClassRef": {} + }, + "status": { + "removedFromBrokerCatalog": false } - ] -} \ No newline at end of file + } +] \ No newline at end of file diff --git a/cmd/svcat/testdata/output/get-plans.txt b/cmd/svcat/testdata/output/get-plans.txt index 17aaa850512..d75f2d8c28c 100644 --- a/cmd/svcat/testdata/output/get-plans.txt +++ b/cmd/svcat/testdata/output/get-plans.txt @@ -1,8 +1,11 @@ - NAME CLASS DESCRIPTION -+---------+--------------------------+----------------------------------------+ - default user-provided-service Sample plan description - premium user-provided-service Premium plan - default another-provided-service Another sample plan description that's - really really really really really, - kinda, wide - premium another-provided-service Another premium plan + NAME NAMESPACE CLASS DESCRIPTION ++------------------------------+-----------+--------------------------+--------------------------------+ + user-provided-namespace-plan default Sample namespace plan + description + default user-provided-service Sample plan description + premium user-provided-service Premium plan + default another-provided-service Another sample plan + description that's really + really really really really, + kinda, wide + premium another-provided-service Another premium plan diff --git a/cmd/svcat/testdata/output/get-plans.yaml b/cmd/svcat/testdata/output/get-plans.yaml index 1eafb6ce844..e42dcdc9f43 100644 --- a/cmd/svcat/testdata/output/get-plans.yaml +++ b/cmd/svcat/testdata/output/get-plans.yaml @@ -1,4 +1,3 @@ -items: - metadata: creationTimestamp: 2018-01-11T20:53:31Z name: 86064792-7ea2-467b-af93-ac9694d96d52 @@ -88,4 +87,19 @@ items: type: object status: removedFromBrokerCatalog: false -metadata: {} +- metadata: + creationTimestamp: 2018-09-04T23:11:31Z + name: ac9694d9-7ea2-af93-467b-860647926d52 + namespace: default + resourceVersion: "4" + selfLink: /apis/servicecatalog.k8s.io/v1beta1/serviceplans/ac9694d9-7ea2-af93-467b-860647926d52 + uid: 0242ac11-11e7-f711-aa44-7b3d01900005 + spec: + description: Sample namespace plan description + externalID: ac9694d9-7ea2-af93-467b-860647926d52 + externalName: user-provided-namespace-plan + free: true + serviceBrokerName: namespace-ups-broker + serviceClassRef: {} + status: + removedFromBrokerCatalog: false diff --git a/cmd/svcat/testdata/plugin.yaml b/cmd/svcat/testdata/plugin.yaml index 4441dd186e7..3c976fa09af 100644 --- a/cmd/svcat/testdata/plugin.yaml +++ b/cmd/svcat/testdata/plugin.yaml @@ -88,6 +88,11 @@ tree: -1 to wait indefinitely.' - name: wait desc: Wait until the operation completes. +- name: deregister + use: deregister NAME + shortDesc: Deregisters an existing broker with service catalog + example: ' svcat deregister mysqlbroker' + command: ./svcat deregister - name: describe use: describe shortDesc: Show details of a specific resource @@ -227,9 +232,11 @@ tree: desc: If present, specify the plan used as a filter for this request - name: plans use: plans [NAME] - shortDesc: List plans, optionally filtered by name or class + shortDesc: List plans, optionally filtered by name, class, scope or namespace example: |2- svcat get plans + svcat get plans --scope cluster + svcat get plans --scope namespace --namespace dev svcat get plan PLAN_NAME svcat get plan CLASS_NAME/PLAN_NAME svcat get plan --uuid PLAN_UUID @@ -239,6 +246,9 @@ tree: svcat get plan --uuid --class CLASS_UUID PLAN_UUID command: ./svcat get plans flags: + - name: all-namespaces + desc: If present, list the requested object(s) across all namespaces. Namespace + in current context is ignored even if specified with --namespace - name: class shorthand: c desc: Filter plans based on class. When --uuid is specified, the class name @@ -247,6 +257,8 @@ tree: shorthand: o desc: The output format to use. Valid options are table, json or yaml. If not present, defaults to table + - name: scope + desc: 'Limit the results to a particular scope: cluster, namespace or all' - name: uuid shorthand: u desc: Whether or not to get the plan by UUID (the default is by name) diff --git a/cmd/svcat/testdata/responses/catalog/clusterserviceclasses?fieldSelector=spec.externalName=user-provided-service.json b/cmd/svcat/testdata/responses/catalog/clusterserviceclasses_fieldSelector=spec.externalName=user-provided-service.json similarity index 100% rename from cmd/svcat/testdata/responses/catalog/clusterserviceclasses?fieldSelector=spec.externalName=user-provided-service.json rename to cmd/svcat/testdata/responses/catalog/clusterserviceclasses_fieldSelector=spec.externalName=user-provided-service.json diff --git a/cmd/svcat/testdata/responses/catalog/clusterserviceplans?fieldSelector=spec.clusterServiceClassRef.name=4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468,spec.externalName=default.json b/cmd/svcat/testdata/responses/catalog/clusterserviceplans_fieldSelector=spec.clusterServiceClassRef.name=4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468,spec.externalName=default.json similarity index 100% rename from cmd/svcat/testdata/responses/catalog/clusterserviceplans?fieldSelector=spec.clusterServiceClassRef.name=4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468,spec.externalName=default.json rename to cmd/svcat/testdata/responses/catalog/clusterserviceplans_fieldSelector=spec.clusterServiceClassRef.name=4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468,spec.externalName=default.json diff --git a/cmd/svcat/testdata/responses/catalog/clusterserviceplans?fieldSelector=spec.clusterServiceClassRef.name=4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468.json b/cmd/svcat/testdata/responses/catalog/clusterserviceplans_fieldSelector=spec.clusterServiceClassRef.name=4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468.json similarity index 100% rename from cmd/svcat/testdata/responses/catalog/clusterserviceplans?fieldSelector=spec.clusterServiceClassRef.name=4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468.json rename to cmd/svcat/testdata/responses/catalog/clusterserviceplans_fieldSelector=spec.clusterServiceClassRef.name=4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468.json diff --git a/cmd/svcat/testdata/responses/catalog/clusterserviceplans?fieldSelector=spec.externalName=default.json b/cmd/svcat/testdata/responses/catalog/clusterserviceplans_fieldSelector=spec.externalName=default.json similarity index 100% rename from cmd/svcat/testdata/responses/catalog/clusterserviceplans?fieldSelector=spec.externalName=default.json rename to cmd/svcat/testdata/responses/catalog/clusterserviceplans_fieldSelector=spec.externalName=default.json diff --git a/cmd/svcat/testdata/responses/catalog/clusterserviceplans?fieldSelector=spec.externalName=premium.json b/cmd/svcat/testdata/responses/catalog/clusterserviceplans_fieldSelector=spec.externalName=premium.json similarity index 100% rename from cmd/svcat/testdata/responses/catalog/clusterserviceplans?fieldSelector=spec.externalName=premium.json rename to cmd/svcat/testdata/responses/catalog/clusterserviceplans_fieldSelector=spec.externalName=premium.json diff --git a/cmd/svcat/testdata/responses/catalog/namespaces/default/serviceplans.json b/cmd/svcat/testdata/responses/catalog/namespaces/default/serviceplans.json new file mode 100644 index 00000000000..a57358d2c30 --- /dev/null +++ b/cmd/svcat/testdata/responses/catalog/namespaces/default/serviceplans.json @@ -0,0 +1,31 @@ +{ + "kind": "ServicePlanList", + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "metadata": { + "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/serviceplans", + "resourceVersion": "114" + }, + "items": [ + { + "metadata": { + "name": "ac9694d9-7ea2-af93-467b-860647926d52", + "namespace": "default", + "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/serviceplans/ac9694d9-7ea2-af93-467b-860647926d52", + "uid": "0242ac11-11e7-f711-aa44-7b3d01900005", + "resourceVersion": "4", + "creationTimestamp": "2018-09-04T23:11:31Z" + }, + "spec": { + "serviceBrokerName": "namespace-ups-broker", + "externalName": "user-provided-namespace-plan", + "externalID": "ac9694d9-7ea2-af93-467b-860647926d52", + "description": "Sample namespace plan description", + "free": true, + "serviceClassRef": {} + }, + "status": { + "removedFromBrokerCatalog": false + } + } + ] +} diff --git a/cmd/svcat/testdata/responses/catalog/serviceinstances?fieldSelector=spec.clusterServicePlanRef.name=86064792-7ea2-467b-af93-ac9694d96d52.json b/cmd/svcat/testdata/responses/catalog/serviceinstances_fieldSelector=spec.clusterServicePlanRef.name=86064792-7ea2-467b-af93-ac9694d96d52.json similarity index 100% rename from cmd/svcat/testdata/responses/catalog/serviceinstances?fieldSelector=spec.clusterServicePlanRef.name=86064792-7ea2-467b-af93-ac9694d96d52.json rename to cmd/svcat/testdata/responses/catalog/serviceinstances_fieldSelector=spec.clusterServicePlanRef.name=86064792-7ea2-467b-af93-ac9694d96d52.json diff --git a/cmd/svcat/testdata/responses/catalog/serviceinstances?fieldSelector=spec.clusterServicePlanRef.name=cc0d7529-18e8-416d-8946-6f7456acd589.json b/cmd/svcat/testdata/responses/catalog/serviceinstances_fieldSelector=spec.clusterServicePlanRef.name=cc0d7529-18e8-416d-8946-6f7456acd589.json similarity index 100% rename from cmd/svcat/testdata/responses/catalog/serviceinstances?fieldSelector=spec.clusterServicePlanRef.name=cc0d7529-18e8-416d-8946-6f7456acd589.json rename to cmd/svcat/testdata/responses/catalog/serviceinstances_fieldSelector=spec.clusterServicePlanRef.name=cc0d7529-18e8-416d-8946-6f7456acd589.json diff --git a/cmd/svcat/testdata/responses/catalog/serviceplans.json b/cmd/svcat/testdata/responses/catalog/serviceplans.json new file mode 100644 index 00000000000..8f149fe537c --- /dev/null +++ b/cmd/svcat/testdata/responses/catalog/serviceplans.json @@ -0,0 +1,9 @@ +{ + "kind": "ServicePlanList", + "apiVersion": "servicecatalog.k8s.io/v1beta1", + "metadata": { + "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/serviceplans", + "resourceVersion": "114" + }, + "items": [] +} diff --git a/docs/catalog-restrictions.md b/docs/catalog-restrictions.md index 9b4cecc5bd6..b3383bd0068 100644 --- a/docs/catalog-restrictions.md +++ b/docs/catalog-restrictions.md @@ -62,21 +62,24 @@ subset of properties on service class and service plan resources. The following `ClusterServiceClass` allowed property names: -| Property Key | Description | +| Property Key | Description | +| ------------- | ------------- | | name | This key will match the ClusterServiceClass.Name property | | spec.externalName | This key will match the ClusterServiceClass.Spec.ExternalName property | | spec.externalID | This key will match the ClusterServiceClass.Spec.ExternalID property | `ServiceClass` allowed property names: -| Property Key | Description | +| Property Key | Description | +| ------------ | ------------ | | name | This key will match the ServiceClass.Name | | spec.externalName | This key will match the ServiceClass.Spec.ExternalName property | | spec.externalID | This key will match the ServiceClass.Spec.ExternalID property | `ClusterServicePlan` allowed property names: -| Property Key | Description | +| Property Key | Description | +| ------------ | ------------ | | name | This key will match the ClusterServicePlan.Name | | spec.externalName | This key will match the ClusterServicePlan.Spec.ExternalName property | | spec.externalID | This key will match the ClusterServicePlan.Spec.ExternalID property | @@ -85,7 +88,8 @@ subset of properties on service class and service plan resources. The following `ServicePlan` allowed property names: -| Property Key | Description | +| Property Key | Description | +| ------------ | ------------ | | name | This key will match the ServicePlan.Name property | | spec.externalName | This key will match the ServicePlan.Spec.ExternalName property | | spec.externalID | This key will match the ServicePlan.Spec.ExternalID property | @@ -191,4 +195,4 @@ spec: - "spec.externalName in (Demo)" - "spec.free=true" url: http://sample-broker.brokers.svc.cluster.local -``` \ No newline at end of file +``` diff --git a/pkg/apis/servicecatalog/types.go b/pkg/apis/servicecatalog/types.go index 990b9f89a89..29835e97bdc 100644 --- a/pkg/apis/servicecatalog/types.go +++ b/pkg/apis/servicecatalog/types.go @@ -155,9 +155,9 @@ type CommonServiceBrokerSpec struct { // spec.clusterServiceClass.name - the value set to ClusterServicePlan.Spec.ClusterServiceClassRef.Name type CatalogRestrictions struct { // ServiceClass represents a selector for plans, used to filter catalog re-lists. - ServicePlan []string - // ServicePlan represents a selector for classes, used to filter catalog re-lists. ServiceClass []string + // ServicePlan represents a selector for classes, used to filter catalog re-lists. + ServicePlan []string } // ClusterServiceBrokerSpec represents a description of a Broker. diff --git a/pkg/apis/servicecatalog/v1beta1/filter.go b/pkg/apis/servicecatalog/v1beta1/filter.go index 16aa52bba18..6c9974adcfb 100644 --- a/pkg/apis/servicecatalog/v1beta1/filter.go +++ b/pkg/apis/servicecatalog/v1beta1/filter.go @@ -40,6 +40,12 @@ func ConvertServiceClassToProperties(serviceClass *ServiceClass) filter.Properti } } +// IsValidServiceClassProperty returns true if the specified property +// is a valid filterable property of ServiceClasses +func IsValidServiceClassProperty(p string) bool { + return p == FilterName || p == FilterSpecExternalName || p == FilterSpecExternalID +} + // ConvertServicePlanToProperties takes a Service Plan and pulls out the // properties we support for filtering, converting them into a map in the // expected format. @@ -56,6 +62,12 @@ func ConvertServicePlanToProperties(servicePlan *ServicePlan) filter.Properties } } +// IsValidServicePlanProperty returns true if the specified property +// is a valid filterable property of ServicePlans +func IsValidServicePlanProperty(p string) bool { + return p == FilterName || p == FilterSpecExternalName || p == FilterSpecExternalID || p == FilterSpecServiceClassName || p == FilterSpecFree +} + // ConvertClusterServiceClassToProperties takes a Service Class and pulls out the // properties we support for filtering, converting them into a map in the // expected format. @@ -70,6 +82,12 @@ func ConvertClusterServiceClassToProperties(serviceClass *ClusterServiceClass) f } } +// IsValidClusterServiceClassProperty returns true if the specified property +// is a valid filterable property of ClusterServiceClasses +func IsValidClusterServiceClassProperty(p string) bool { + return p == FilterName || p == FilterSpecExternalName || p == FilterSpecExternalID +} + // ConvertClusterServicePlanToProperties takes a Service Plan and pulls out the // properties we support for filtering, converting them into a map in the // expected format. @@ -85,3 +103,9 @@ func ConvertClusterServicePlanToProperties(servicePlan *ClusterServicePlan) filt FilterSpecFree: strconv.FormatBool(servicePlan.Spec.Free), } } + +// IsValidClusterServicePlanProperty returns true if the specified property +// is a valid filterable property of ServicePlans +func IsValidClusterServicePlanProperty(p string) bool { + return p == FilterName || p == FilterSpecExternalName || p == FilterSpecExternalID || p == FilterSpecClusterServiceClassName || p == FilterSpecFree +} diff --git a/pkg/apis/servicecatalog/v1beta1/plan.go b/pkg/apis/servicecatalog/v1beta1/plan.go index 4e9d3dbf104..51a123c6230 100644 --- a/pkg/apis/servicecatalog/v1beta1/plan.go +++ b/pkg/apis/servicecatalog/v1beta1/plan.go @@ -26,6 +26,16 @@ func (p *ServicePlan) GetName() string { return p.Name } +// GetNamespace for cluster-scoped plans always returns "". +func (p *ClusterServicePlan) GetNamespace() string { + return "" +} + +// GetNamespace returns the plan's namespace. +func (p *ServicePlan) GetNamespace() string { + return p.Namespace +} + // GetExternalName returns the plan's external name. func (p *ClusterServicePlan) GetExternalName() string { return p.Spec.ExternalName @@ -45,3 +55,13 @@ func (p *ClusterServicePlan) GetDescription() string { func (p *ServicePlan) GetDescription() string { return p.Spec.Description } + +// GetClassID returns the class name from plan. +func (p *ClusterServicePlan) GetClassID() string { + return p.Spec.ClusterServiceClassRef.Name +} + +// GetClassID returns the class name from plan. +func (p *ServicePlan) GetClassID() string { + return p.Spec.ServiceClassRef.Name +} diff --git a/pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go b/pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go index ad01860f0ef..1781f77320e 100644 --- a/pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go +++ b/pkg/apis/servicecatalog/v1beta1/zz_generated.conversion.go @@ -262,8 +262,8 @@ func Convert_v1beta1_CatalogRestrictions_To_servicecatalog_CatalogRestrictions(i } func autoConvert_servicecatalog_CatalogRestrictions_To_v1beta1_CatalogRestrictions(in *servicecatalog.CatalogRestrictions, out *CatalogRestrictions, s conversion.Scope) error { - out.ServicePlan = *(*[]string)(unsafe.Pointer(&in.ServicePlan)) out.ServiceClass = *(*[]string)(unsafe.Pointer(&in.ServiceClass)) + out.ServicePlan = *(*[]string)(unsafe.Pointer(&in.ServicePlan)) return nil } diff --git a/pkg/apis/servicecatalog/validation/binding_test.go b/pkg/apis/servicecatalog/validation/binding_test.go index f38d00858ab..b239976eaec 100644 --- a/pkg/apis/servicecatalog/validation/binding_test.go +++ b/pkg/apis/servicecatalog/validation/binding_test.go @@ -567,14 +567,15 @@ func TestValidateServiceBinding(t *testing.T) { } for _, tc := range cases { - errs := internalValidateServiceBinding(tc.binding, tc.create) - errs = append(errs, validateServiceBindingStatus(&tc.binding.Status, field.NewPath("status"), false)...) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + t.Run(tc.name, func(t *testing.T) { + errs := internalValidateServiceBinding(tc.binding, tc.create) + errs = append(errs, validateServiceBindingStatus(&tc.binding.Status, field.NewPath("status"), false)...) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } } @@ -612,28 +613,29 @@ func TestInternalValidateServiceBindingUpdateAllowed(t *testing.T) { } for _, tc := range cases { - oldBinding := validServiceBinding() - if tc.onGoingSpecChange { - oldBinding.Generation = 2 - } else { - oldBinding.Generation = 1 - } - oldBinding.Status.ReconciledGeneration = 1 + t.Run(tc.name, func(t *testing.T) { + oldBinding := validServiceBinding() + if tc.onGoingSpecChange { + oldBinding.Generation = 2 + } else { + oldBinding.Generation = 1 + } + oldBinding.Status.ReconciledGeneration = 1 - newBinding := validServiceBinding() - if tc.newSpecChange { - newBinding.Generation = oldBinding.Generation + 1 - } else { - newBinding.Generation = oldBinding.Generation - } - newBinding.Status.ReconciledGeneration = 1 + newBinding := validServiceBinding() + if tc.newSpecChange { + newBinding.Generation = oldBinding.Generation + 1 + } else { + newBinding.Generation = oldBinding.Generation + } + newBinding.Status.ReconciledGeneration = 1 - errs := internalValidateServiceBindingUpdateAllowed(newBinding, oldBinding) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + errs := internalValidateServiceBindingUpdateAllowed(newBinding, oldBinding) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } } diff --git a/pkg/apis/servicecatalog/validation/broker.go b/pkg/apis/servicecatalog/validation/broker.go index 4a62e5d0ba3..89ed1899675 100644 --- a/pkg/apis/servicecatalog/validation/broker.go +++ b/pkg/apis/servicecatalog/validation/broker.go @@ -17,11 +17,14 @@ limitations under the License. package validation import ( + "fmt" + apivalidation "k8s.io/apimachinery/pkg/api/validation" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog" + "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" "github.com/kubernetes-incubator/service-catalog/pkg/filter" ) @@ -88,7 +91,7 @@ func validateClusterServiceBrokerSpec(spec *sc.ClusterServiceBrokerSpec, fldPath } } - commonErrs := validateCommonServiceBrokerSpec(&spec.CommonServiceBrokerSpec, fldPath) + commonErrs := validateCommonServiceBrokerSpec(&spec.CommonServiceBrokerSpec, fldPath, true) if len(commonErrs) != 0 { allErrs = append(allErrs, commonErrs...) @@ -150,7 +153,7 @@ func validateServiceBrokerSpec(spec *sc.ServiceBrokerSpec, fldPath *field.Path) } } - commonErrs := validateCommonServiceBrokerSpec(&spec.CommonServiceBrokerSpec, fldPath) + commonErrs := validateCommonServiceBrokerSpec(&spec.CommonServiceBrokerSpec, fldPath, false) if len(commonErrs) != 0 { allErrs = append(allErrs, commonErrs...) @@ -159,7 +162,7 @@ func validateServiceBrokerSpec(spec *sc.ServiceBrokerSpec, fldPath *field.Path) return allErrs } -func validateCommonServiceBrokerSpec(spec *sc.CommonServiceBrokerSpec, fldPath *field.Path) field.ErrorList { +func validateCommonServiceBrokerSpec(spec *sc.CommonServiceBrokerSpec, fldPath *field.Path, isClusterServiceBroker bool) field.ErrorList { commonErrs := field.ErrorList{} if "" == spec.URL { @@ -205,8 +208,6 @@ func validateCommonServiceBrokerSpec(spec *sc.CommonServiceBrokerSpec, fldPath * } } - // TODO: could validate if the fields being selected are on the approve list, but this will require breaking - // apart the label selector. if spec.CatalogRestrictions != nil && len(spec.CatalogRestrictions.ServiceClass) > 0 { // confirm that the restrictions can turn into a predicate. _, err := filter.CreatePredicate(spec.CatalogRestrictions.ServiceClass) @@ -214,6 +215,16 @@ func validateCommonServiceBrokerSpec(spec *sc.CommonServiceBrokerSpec, fldPath * commonErrs = append(commonErrs, field.Invalid(fldPath.Child("catalogRestrictions", "serviceClass"), spec.CatalogRestrictions.ServiceClass, err.Error())) + } else { + for _, restriction := range spec.CatalogRestrictions.ServiceClass { + p := filter.ExtractProperty(restriction) + if !isClusterServiceBroker && !v1beta1.IsValidServiceClassProperty(p) || + isClusterServiceBroker && !v1beta1.IsValidClusterServiceClassProperty(p) { + commonErrs = append(commonErrs, + field.Invalid(fldPath.Child("catalogRestrictions", "serviceClass"), + spec.CatalogRestrictions.ServiceClass, fmt.Sprintf("Invalid property: %s", p))) + } + } } } if spec.CatalogRestrictions != nil && len(spec.CatalogRestrictions.ServicePlan) > 0 { @@ -222,7 +233,17 @@ func validateCommonServiceBrokerSpec(spec *sc.CommonServiceBrokerSpec, fldPath * if err != nil { commonErrs = append(commonErrs, field.Invalid(fldPath.Child("catalogRestrictions", "servicePlan"), - spec.CatalogRestrictions.ServiceClass, err.Error())) + spec.CatalogRestrictions.ServicePlan, err.Error())) + } else { + for _, restriction := range spec.CatalogRestrictions.ServicePlan { + p := filter.ExtractProperty(restriction) + if !isClusterServiceBroker && !v1beta1.IsValidServicePlanProperty(p) || + isClusterServiceBroker && !v1beta1.IsValidClusterServicePlanProperty(p) { + commonErrs = append(commonErrs, + field.Invalid(fldPath.Child("catalogRestrictions", "servicePlan"), + spec.CatalogRestrictions.ServicePlan, fmt.Sprintf("Invalid property: %s", p))) + } + } } } diff --git a/pkg/apis/servicecatalog/validation/broker_test.go b/pkg/apis/servicecatalog/validation/broker_test.go index b5ac657bd3e..d5757f2340a 100644 --- a/pkg/apis/servicecatalog/validation/broker_test.go +++ b/pkg/apis/servicecatalog/validation/broker_test.go @@ -402,7 +402,7 @@ func TestValidateClusterServiceBroker(t *testing.T) { CatalogRestrictions: &servicecatalog.CatalogRestrictions{ ServiceClass: []string{ "name==foobar", - "externalName in (foobar, bazboof, wizzbang)", + "spec.externalName in (foobar, bazboof, wizzbang)", }, }, }, @@ -430,6 +430,26 @@ func TestValidateClusterServiceBroker(t *testing.T) { }, valid: false, }, + { + name: "invalid clusterservicebroker - catalogRequirements.serviceClass - invalid property", + broker: &servicecatalog.ClusterServiceBroker{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-broker", + }, + Spec: servicecatalog.ClusterServiceBrokerSpec{ + CommonServiceBrokerSpec: servicecatalog.CommonServiceBrokerSpec{ + URL: "http://example.com", + RelistBehavior: servicecatalog.ServiceBrokerRelistBehaviorManual, + CatalogRestrictions: &servicecatalog.CatalogRestrictions{ + ServiceClass: []string{ + "spec.invalidProperty==foobar", + }, + }, + }, + }, + }, + valid: false, + }, { name: "valid clusterservicebroker - catalogRequirements.servicePlan", broker: &servicecatalog.ClusterServiceBroker{ @@ -463,7 +483,7 @@ func TestValidateClusterServiceBroker(t *testing.T) { CatalogRestrictions: &servicecatalog.CatalogRestrictions{ ServicePlan: []string{ "name==foobar", - "externalName in (foobar, bazboof, wizzbang)", + "spec.externalName in (foobar, bazboof, wizzbang)", }, }, }, @@ -491,6 +511,26 @@ func TestValidateClusterServiceBroker(t *testing.T) { }, valid: false, }, + { + name: "invalid clusterservicebroker - catalogRequirements.servicePlan - invalid property", + broker: &servicecatalog.ClusterServiceBroker{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-broker", + }, + Spec: servicecatalog.ClusterServiceBrokerSpec{ + CommonServiceBrokerSpec: servicecatalog.CommonServiceBrokerSpec{ + URL: "http://example.com", + RelistBehavior: servicecatalog.ServiceBrokerRelistBehaviorManual, + CatalogRestrictions: &servicecatalog.CatalogRestrictions{ + ServicePlan: []string{ + "spec.invalidProperty notin (foo, bar)", + }, + }, + }, + }, + }, + valid: false, + }, { name: "valid clusterservicebroker - catalogRequirements with serviceClass and servicePlan", broker: &servicecatalog.ClusterServiceBroker{ @@ -503,12 +543,12 @@ func TestValidateClusterServiceBroker(t *testing.T) { RelistBehavior: servicecatalog.ServiceBrokerRelistBehaviorManual, CatalogRestrictions: &servicecatalog.CatalogRestrictions{ ServiceClass: []string{ - "name==barfoobar", - "externalName in (barfoobar, batbazboof, batwizzbang)", + "name=barfoobar", + "spec.externalName in (barfoobar, batbazboof, batwizzbang)", }, ServicePlan: []string{ "name==foobar", - "externalName in (foobar, bazboof, wizzbang)", + "spec.externalName in (foobar, bazboof, wizzbang)", }, }, }, @@ -519,13 +559,14 @@ func TestValidateClusterServiceBroker(t *testing.T) { } for _, tc := range cases { - errs := ValidateClusterServiceBroker(tc.broker) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - return - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + t.Run(tc.name, func(t *testing.T) { + errs := ValidateClusterServiceBroker(tc.broker) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } updateCases := []struct { @@ -626,13 +667,14 @@ func TestValidateClusterServiceBroker(t *testing.T) { }, } for _, tc := range updateCases { - errs := ValidateClusterServiceBrokerUpdate(tc.newBroker, tc.oldBroker) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + t.Run(tc.name, func(t *testing.T) { + errs := ValidateClusterServiceBrokerUpdate(tc.newBroker, tc.oldBroker) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } } @@ -945,13 +987,14 @@ func TestValidateServiceBroker(t *testing.T) { } for _, tc := range cases { - errs := ValidateServiceBroker(tc.broker) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + t.Run(tc.name, func(t *testing.T) { + errs := ValidateServiceBroker(tc.broker) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } updateCases := []struct { @@ -1058,12 +1101,13 @@ func TestValidateServiceBroker(t *testing.T) { }, } for _, tc := range updateCases { - errs := ValidateServiceBrokerUpdate(tc.newBroker, tc.oldBroker) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + t.Run(tc.name, func(t *testing.T) { + errs := ValidateServiceBrokerUpdate(tc.newBroker, tc.oldBroker) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } } diff --git a/pkg/apis/servicecatalog/validation/instance_test.go b/pkg/apis/servicecatalog/validation/instance_test.go index f07e95472ba..11e7ea881f9 100644 --- a/pkg/apis/servicecatalog/validation/instance_test.go +++ b/pkg/apis/servicecatalog/validation/instance_test.go @@ -17,6 +17,7 @@ limitations under the License. package validation import ( + "fmt" "reflect" "strings" "testing" @@ -890,13 +891,14 @@ func TestValidateServiceInstance(t *testing.T) { } for _, tc := range cases { - errs := internalValidateServiceInstance(tc.instance, tc.create) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + t.Run(tc.name, func(t *testing.T) { + errs := internalValidateServiceInstance(tc.instance, tc.create) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } } @@ -934,93 +936,95 @@ func TestInternalValidateServiceInstanceUpdateAllowed(t *testing.T) { } for _, tc := range cases { - oldInstance := &servicecatalog.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - Namespace: "test-ns", - }, - Spec: servicecatalog.ServiceInstanceSpec{ - PlanReference: servicecatalog.PlanReference{ - ClusterServiceClassExternalName: clusterServiceClassExternalName, - ClusterServicePlanExternalName: clusterServicePlanExternalName, + t.Run(tc.name, func(t *testing.T) { + oldInstance := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", }, - }, - } - oldInstance.Generation = 1 - if tc.onGoingOperation { - oldInstance.Status.CurrentOperation = servicecatalog.ServiceInstanceOperationProvision - } + Spec: servicecatalog.ServiceInstanceSpec{ + PlanReference: servicecatalog.PlanReference{ + ClusterServiceClassExternalName: clusterServiceClassExternalName, + ClusterServicePlanExternalName: clusterServicePlanExternalName, + }, + }, + } + oldInstance.Generation = 1 + if tc.onGoingOperation { + oldInstance.Status.CurrentOperation = servicecatalog.ServiceInstanceOperationProvision + } - newInstance := &servicecatalog.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - Namespace: "test-ns", - }, - Spec: servicecatalog.ServiceInstanceSpec{ - PlanReference: servicecatalog.PlanReference{ - ClusterServiceClassExternalName: clusterServiceClassExternalName, - ClusterServicePlanExternalName: clusterServicePlanExternalName, + newInstance := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", }, - }, - } - if tc.specChange { - newInstance.Generation = oldInstance.Generation + 1 - } else { - newInstance.Generation = oldInstance.Generation - } + Spec: servicecatalog.ServiceInstanceSpec{ + PlanReference: servicecatalog.PlanReference{ + ClusterServiceClassExternalName: clusterServiceClassExternalName, + ClusterServicePlanExternalName: clusterServicePlanExternalName, + }, + }, + } + if tc.specChange { + newInstance.Generation = oldInstance.Generation + 1 + } else { + newInstance.Generation = oldInstance.Generation + } - errs := internalValidateServiceInstanceUpdateAllowed(newInstance, oldInstance) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + errs := internalValidateServiceInstanceUpdateAllowed(newInstance, oldInstance) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } for _, tc := range cases { - oldInstance := &servicecatalog.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - Namespace: "test-ns", - }, - Spec: servicecatalog.ServiceInstanceSpec{ - PlanReference: servicecatalog.PlanReference{ - ServiceClassExternalName: serviceClassExternalName, - ServicePlanExternalName: servicePlanExternalName, + t.Run(tc.name, func(t *testing.T) { + oldInstance := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", }, - }, - } - oldInstance.Generation = 1 - if tc.onGoingOperation { - oldInstance.Status.CurrentOperation = servicecatalog.ServiceInstanceOperationProvision - } + Spec: servicecatalog.ServiceInstanceSpec{ + PlanReference: servicecatalog.PlanReference{ + ServiceClassExternalName: serviceClassExternalName, + ServicePlanExternalName: servicePlanExternalName, + }, + }, + } + oldInstance.Generation = 1 + if tc.onGoingOperation { + oldInstance.Status.CurrentOperation = servicecatalog.ServiceInstanceOperationProvision + } - newInstance := &servicecatalog.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - Namespace: "test-ns", - }, - Spec: servicecatalog.ServiceInstanceSpec{ - PlanReference: servicecatalog.PlanReference{ - ServiceClassExternalName: serviceClassExternalName, - ServicePlanExternalName: servicePlanExternalName, + newInstance := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", }, - }, - } - if tc.specChange { - newInstance.Generation = oldInstance.Generation + 1 - } else { - newInstance.Generation = oldInstance.Generation - } + Spec: servicecatalog.ServiceInstanceSpec{ + PlanReference: servicecatalog.PlanReference{ + ServiceClassExternalName: serviceClassExternalName, + ServicePlanExternalName: servicePlanExternalName, + }, + }, + } + if tc.specChange { + newInstance.Generation = oldInstance.Generation + 1 + } else { + newInstance.Generation = oldInstance.Generation + } - errs := internalValidateServiceInstanceUpdateAllowed(newInstance, oldInstance) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + errs := internalValidateServiceInstanceUpdateAllowed(newInstance, oldInstance) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } } @@ -1097,37 +1101,38 @@ func TestInternalValidateServiceInstanceUpdateAllowedForClusterPlanChange(t *tes } for _, tc := range cases { - oldInstance := &servicecatalog.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - Namespace: "test-ns", - }, - Spec: servicecatalog.ServiceInstanceSpec{ - PlanReference: tc.oldPlan, - ClusterServiceClassRef: &servicecatalog.ClusterObjectReference{}, - ClusterServicePlanRef: &servicecatalog.ClusterObjectReference{}, - }, - } + t.Run(tc.name, func(t *testing.T) { + oldInstance := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", + }, + Spec: servicecatalog.ServiceInstanceSpec{ + PlanReference: tc.oldPlan, + ClusterServiceClassRef: &servicecatalog.ClusterObjectReference{}, + ClusterServicePlanRef: &servicecatalog.ClusterObjectReference{}, + }, + } - newInstance := &servicecatalog.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - Namespace: "test-ns", - }, - Spec: servicecatalog.ServiceInstanceSpec{ - PlanReference: tc.newPlan, - ClusterServiceClassRef: &servicecatalog.ClusterObjectReference{}, - ClusterServicePlanRef: tc.newPlanRef, - }, - } + newInstance := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", + }, + Spec: servicecatalog.ServiceInstanceSpec{ + PlanReference: tc.newPlan, + ClusterServiceClassRef: &servicecatalog.ClusterObjectReference{}, + ClusterServicePlanRef: tc.newPlanRef, + }, + } - errs := internalValidateServiceInstanceUpdateAllowed(newInstance, oldInstance) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + errs := internalValidateServiceInstanceUpdateAllowed(newInstance, oldInstance) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } } @@ -1204,37 +1209,38 @@ func TestInternalValidateServiceInstanceUpdateAllowedForPlanChange(t *testing.T) } for _, tc := range cases { - oldInstance := &servicecatalog.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - Namespace: "test-ns", - }, - Spec: servicecatalog.ServiceInstanceSpec{ - PlanReference: tc.oldPlan, - ServiceClassRef: &servicecatalog.LocalObjectReference{}, - ServicePlanRef: &servicecatalog.LocalObjectReference{}, - }, - } + t.Run(tc.name, func(t *testing.T) { + oldInstance := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", + }, + Spec: servicecatalog.ServiceInstanceSpec{ + PlanReference: tc.oldPlan, + ServiceClassRef: &servicecatalog.LocalObjectReference{}, + ServicePlanRef: &servicecatalog.LocalObjectReference{}, + }, + } - newInstance := &servicecatalog.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - Namespace: "test-ns", - }, - Spec: servicecatalog.ServiceInstanceSpec{ - PlanReference: tc.newPlan, - ServiceClassRef: &servicecatalog.LocalObjectReference{}, - ServicePlanRef: tc.newPlanRef, - }, - } + newInstance := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", + }, + Spec: servicecatalog.ServiceInstanceSpec{ + PlanReference: tc.newPlan, + ServiceClassRef: &servicecatalog.LocalObjectReference{}, + ServicePlanRef: tc.newPlanRef, + }, + } - errs := internalValidateServiceInstanceUpdateAllowed(newInstance, oldInstance) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + errs := internalValidateServiceInstanceUpdateAllowed(newInstance, oldInstance) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } } @@ -1379,107 +1385,109 @@ func TestValidateServiceInstanceStatusUpdate(t *testing.T) { } for _, tc := range cases { - old := &servicecatalog.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - Namespace: "test-ns", - Generation: 2, - }, - Spec: servicecatalog.ServiceInstanceSpec{ - PlanReference: servicecatalog.PlanReference{ - ClusterServiceClassExternalName: clusterServiceClassExternalName, - ClusterServicePlanExternalName: clusterServicePlanExternalName, + t.Run(tc.name, func(t *testing.T) { + old := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", + Generation: 2, }, - ClusterServiceClassRef: &servicecatalog.ClusterObjectReference{}, - ClusterServicePlanRef: &servicecatalog.ClusterObjectReference{}, - }, - Status: *tc.old, - } - old.Status.ReconciledGeneration = 1 - new := &servicecatalog.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - Namespace: "test-ns", - Generation: 2, - }, - Spec: servicecatalog.ServiceInstanceSpec{ - PlanReference: servicecatalog.PlanReference{ - ClusterServiceClassExternalName: clusterServiceClassExternalName, - ClusterServicePlanExternalName: clusterServicePlanExternalName, + Spec: servicecatalog.ServiceInstanceSpec{ + PlanReference: servicecatalog.PlanReference{ + ClusterServiceClassExternalName: clusterServiceClassExternalName, + ClusterServicePlanExternalName: clusterServicePlanExternalName, + }, + ClusterServiceClassRef: &servicecatalog.ClusterObjectReference{}, + ClusterServicePlanRef: &servicecatalog.ClusterObjectReference{}, }, - ClusterServiceClassRef: &servicecatalog.ClusterObjectReference{}, - ClusterServicePlanRef: &servicecatalog.ClusterObjectReference{}, - }, - Status: *tc.new, - } - new.Status.ReconciledGeneration = 1 + Status: *tc.old, + } + old.Status.ReconciledGeneration = 1 + new := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", + Generation: 2, + }, + Spec: servicecatalog.ServiceInstanceSpec{ + PlanReference: servicecatalog.PlanReference{ + ClusterServiceClassExternalName: clusterServiceClassExternalName, + ClusterServicePlanExternalName: clusterServicePlanExternalName, + }, + ClusterServiceClassRef: &servicecatalog.ClusterObjectReference{}, + ClusterServicePlanRef: &servicecatalog.ClusterObjectReference{}, + }, + Status: *tc.new, + } + new.Status.ReconciledGeneration = 1 - errs := ValidateServiceInstanceStatusUpdate(new, old) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } - if !tc.valid { - for _, err := range errs { - if !strings.Contains(err.Detail, tc.err) { - t.Errorf("%v: Error %q did not contain expected message %q", tc.name, err.Detail, tc.err) + errs := ValidateServiceInstanceStatusUpdate(new, old) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + if !tc.valid { + for _, err := range errs { + if !strings.Contains(err.Detail, tc.err) { + t.Errorf("Error %q did not contain expected message %q", err.Detail, tc.err) + } } } - } + }) } for _, tc := range cases { - old := &servicecatalog.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - Namespace: "test-ns", - Generation: 2, - }, - Spec: servicecatalog.ServiceInstanceSpec{ - PlanReference: servicecatalog.PlanReference{ - ServiceClassExternalName: serviceClassExternalName, - ServicePlanExternalName: servicePlanExternalName, + t.Run(tc.name, func(t *testing.T) { + old := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", + Generation: 2, }, - ServiceClassRef: &servicecatalog.LocalObjectReference{}, - ServicePlanRef: &servicecatalog.LocalObjectReference{}, - }, - Status: *tc.old, - } - old.Status.ReconciledGeneration = 1 - new := &servicecatalog.ServiceInstance{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - Namespace: "test-ns", - Generation: 2, - }, - Spec: servicecatalog.ServiceInstanceSpec{ - PlanReference: servicecatalog.PlanReference{ - ClusterServiceClassExternalName: clusterServiceClassExternalName, - ClusterServicePlanExternalName: clusterServicePlanExternalName, + Spec: servicecatalog.ServiceInstanceSpec{ + PlanReference: servicecatalog.PlanReference{ + ServiceClassExternalName: serviceClassExternalName, + ServicePlanExternalName: servicePlanExternalName, + }, + ServiceClassRef: &servicecatalog.LocalObjectReference{}, + ServicePlanRef: &servicecatalog.LocalObjectReference{}, }, - ClusterServiceClassRef: &servicecatalog.ClusterObjectReference{}, - ClusterServicePlanRef: &servicecatalog.ClusterObjectReference{}, - }, - Status: *tc.new, - } - new.Status.ReconciledGeneration = 1 + Status: *tc.old, + } + old.Status.ReconciledGeneration = 1 + new := &servicecatalog.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "test-ns", + Generation: 2, + }, + Spec: servicecatalog.ServiceInstanceSpec{ + PlanReference: servicecatalog.PlanReference{ + ClusterServiceClassExternalName: clusterServiceClassExternalName, + ClusterServicePlanExternalName: clusterServicePlanExternalName, + }, + ClusterServiceClassRef: &servicecatalog.ClusterObjectReference{}, + ClusterServicePlanRef: &servicecatalog.ClusterObjectReference{}, + }, + Status: *tc.new, + } + new.Status.ReconciledGeneration = 1 - errs := ValidateServiceInstanceStatusUpdate(new, old) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } - if !tc.valid { - for _, err := range errs { - if !strings.Contains(err.Detail, tc.err) { - t.Errorf("%v: Error %q did not contain expected message %q", tc.name, err.Detail, tc.err) + errs := ValidateServiceInstanceStatusUpdate(new, old) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + if !tc.valid { + for _, err := range errs { + if !strings.Contains(err.Detail, tc.err) { + t.Errorf("Error %q did not contain expected message %q", err.Detail, tc.err) + } } } - } + }) } } @@ -1601,13 +1609,14 @@ func TestValidateServiceInstanceReferencesUpdate(t *testing.T) { } for _, tc := range cases { - errs := ValidateServiceInstanceReferencesUpdate(tc.new, tc.old) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + t.Run(tc.name, func(t *testing.T) { + errs := ValidateServiceInstanceReferencesUpdate(tc.new, tc.old) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } } @@ -1624,33 +1633,40 @@ func TestValidateClusterOrNamespacedPlanReference(t *testing.T) { } // Test permutations of cluster & plan fields set, these should never be valid - cases := []servicecatalog.PlanReference{} + type testcase struct { + name string + ref servicecatalog.PlanReference + } + + cases := []testcase{} for _, c := range cFields { for _, p := range pFields { pref := servicecatalog.PlanReference{} elem := reflect.ValueOf(&pref).Elem() elem.FieldByName(c).SetString("foo") elem.FieldByName(p).SetString("bar") - cases = append(cases, pref) + cases = append(cases, testcase{fmt.Sprintf("%s-%s", c, p), pref}) } } - for _, testPlanRef := range cases { - expectedErr := "instances can only refer to a cluster or namespaced class or plan type, but not both" - errs := validatePlanReference(&testPlanRef, field.NewPath("spec")) - if len(errs) == 0 { - t.Fatalf(`Expected error "%s", but no error was found`, expectedErr) - } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + expectedErr := "instances can only refer to a cluster or namespaced class or plan type, but not both" + errs := validatePlanReference(&tc.ref, field.NewPath("spec")) + if len(errs) == 0 { + t.Fatalf(`Expected error "%s", but no error was found`, expectedErr) + } - found := false - for _, e := range errs { - if strings.Contains(e.Error(), expectedErr) { - found = true + found := false + for _, e := range errs { + if strings.Contains(e.Error(), expectedErr) { + found = true + } } - } - if !found { - t.Fatalf(`TestValidateClusterOrNamespacedPlanReference: did not find expected error "%s" in errors: %v`, expectedErr, errs) - } + if !found { + t.Fatalf(`did not find expected error "%s" in errors: %v`, expectedErr, errs) + } + }) } } @@ -1904,25 +1920,25 @@ func TestValidatePlanReference(t *testing.T) { }, } for _, tc := range cases { - errs := validatePlanReference(&tc.ref, field.NewPath("spec")) - if len(errs) != 0 { - if tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } - found := false - for _, e := range errs { - if strings.Contains(e.Error(), tc.expectedError) { - found = true + t.Run(tc.name, func(t *testing.T) { + errs := validatePlanReference(&tc.ref, field.NewPath("spec")) + if len(errs) != 0 { + if tc.valid { + t.Errorf("unexpected error: %v", errs) } + found := false + for _, e := range errs { + if strings.Contains(e.Error(), tc.expectedError) { + found = true + } + } + if !found { + t.Errorf("did not find expected error %q in errors: %v", tc.expectedError, errs) + } + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") } - if !found { - t.Errorf("%v: did not find expected error %q in errors: %v", tc.name, tc.expectedError, errs) - continue - } - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + }) } } @@ -2005,24 +2021,24 @@ func TestValidatePlanReferenceUpdate(t *testing.T) { }, } for _, tc := range cases { - errs := validatePlanReferenceUpdate(&tc.old, &tc.new, field.NewPath("spec")) - if len(errs) != 0 { - if tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } - found := false - for _, e := range errs { - if strings.Contains(e.Error(), tc.expectedError) { - found = true + t.Run(tc.name, func(t *testing.T) { + errs := validatePlanReferenceUpdate(&tc.old, &tc.new, field.NewPath("spec")) + if len(errs) != 0 { + if tc.valid { + t.Errorf("unexpected error: %v", errs) } + found := false + for _, e := range errs { + if strings.Contains(e.Error(), tc.expectedError) { + found = true + } + } + if !found { + t.Errorf("did not find expected error %q in errors: %v", tc.expectedError, errs) + } + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") } - if !found { - t.Errorf("%v: did not find expected error %q in errors: %v", tc.name, tc.expectedError, errs) - continue - } - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + }) } } diff --git a/pkg/apis/servicecatalog/validation/serviceclass.go b/pkg/apis/servicecatalog/validation/serviceclass.go index f86f16ebc98..6b22fd1542f 100644 --- a/pkg/apis/servicecatalog/validation/serviceclass.go +++ b/pkg/apis/servicecatalog/validation/serviceclass.go @@ -31,13 +31,8 @@ const commonServiceClassNameMaxLength int = 63 var commonServiceClassNameRegexp = regexp.MustCompile("^" + commonServiceClassNameFmt + "$") -const guidFmt string = "[a-zA-Z0-9]([-a-zA-Z0-9.]*[a-zA-Z0-9])?" const guidMaxLength int = 63 -// guidRegexp is a loosened validation for -// DNS1123 labels that allows uppercase characters. -var guidRegexp = regexp.MustCompile("^" + guidFmt + "$") - // validateCommonServiceClassName is the common validation function for // service class types. func validateCommonServiceClassName(value string, prefix bool) []string { @@ -64,8 +59,8 @@ func validateExternalID(value string) []string { if len(value) > guidMaxLength { errs = append(errs, utilvalidation.MaxLenError(guidMaxLength)) } - if !guidRegexp.MatchString(value) { - errs = append(errs, utilvalidation.RegexError(guidFmt, "my-name", "123-abc", "456-DEF")) + if len(value) == 0 { + errs = append(errs, utilvalidation.EmptyError()) } return errs } diff --git a/pkg/apis/servicecatalog/validation/serviceclass_test.go b/pkg/apis/servicecatalog/validation/serviceclass_test.go index cea3d31e2e6..c40eb1cf90e 100644 --- a/pkg/apis/servicecatalog/validation/serviceclass_test.go +++ b/pkg/apis/servicecatalog/validation/serviceclass_test.go @@ -53,7 +53,7 @@ func TestValidateClusterServiceClass(t *testing.T) { valid: true, }, { - name: "valid serviceClass - uppercase in GUID", + name: "valid serviceClass - uppercase in externalID", serviceClass: func() *servicecatalog.ClusterServiceClass { s := validClusterServiceClass() s.Spec.ExternalID = "40D-0983-1b89" @@ -62,7 +62,7 @@ func TestValidateClusterServiceClass(t *testing.T) { valid: true, }, { - name: "valid serviceClass - period in GUID", + name: "valid serviceClass - period in externalID", serviceClass: func() *servicecatalog.ClusterServiceClass { s := validClusterServiceClass() s.Spec.ExternalID = "4315f5e1-0139-4ecf-9706-9df0aff33e5a.plan-name" @@ -71,37 +71,37 @@ func TestValidateClusterServiceClass(t *testing.T) { valid: true, }, { - name: "valid serviceClass - period in externalName", + name: "valid serviceClass - underscore in ExternalID", serviceClass: func() *servicecatalog.ClusterServiceClass { s := validClusterServiceClass() - s.Spec.ExternalName = "abc.com" + s.Spec.ExternalID = "4315f5e1-0139-4ecf-9706-9df0aff33e5a_plan-name" return s }(), valid: true, }, { - name: "invalid serviceClass - has namespace", + name: "valid serviceClass - period in externalName", serviceClass: func() *servicecatalog.ClusterServiceClass { s := validClusterServiceClass() - s.Namespace = "test-ns" + s.Spec.ExternalName = "abc.com" return s }(), - valid: false, + valid: true, }, { - name: "invalid serviceClass - missing guid", + name: "invalid serviceClass - has namespace", serviceClass: func() *servicecatalog.ClusterServiceClass { s := validClusterServiceClass() - s.Spec.ExternalID = "" + s.Namespace = "test-ns" return s }(), valid: false, }, { - name: "invalid serviceClass - invalid guid", + name: "invalid serviceClass - empty externalID", serviceClass: func() *servicecatalog.ClusterServiceClass { s := validClusterServiceClass() - s.Spec.ExternalID = "1234-4354a\\%-49b" + s.Spec.ExternalID = "" return s }(), valid: false, @@ -163,13 +163,14 @@ func TestValidateClusterServiceClass(t *testing.T) { } for _, tc := range cases { - errs := ValidateClusterServiceClass(tc.serviceClass) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + t.Run(tc.name, func(t *testing.T) { + errs := ValidateClusterServiceClass(tc.serviceClass) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } } @@ -203,7 +204,7 @@ func TestValidateServiceClass(t *testing.T) { valid: true, }, { - name: "valid serviceClass - uppercase in GUID", + name: "valid serviceClass - uppercase in externalID", serviceClass: func() *servicecatalog.ServiceClass { s := validServiceClass() s.Spec.ExternalID = "40D-0983-1b89" @@ -212,7 +213,7 @@ func TestValidateServiceClass(t *testing.T) { valid: true, }, { - name: "valid serviceClass - period in GUID", + name: "valid serviceClass - period in externalID", serviceClass: func() *servicecatalog.ServiceClass { s := validServiceClass() s.Spec.ExternalID = "4315f5e1-0139-4ecf-9706-9df0aff33e5a.plan-name" @@ -230,28 +231,28 @@ func TestValidateServiceClass(t *testing.T) { valid: true, }, { - name: "invalid serviceClass - has no namespace", + name: "valid serviceClass - underscore in ExternalID", serviceClass: func() *servicecatalog.ServiceClass { s := validServiceClass() - s.Namespace = "" + s.Spec.ExternalID = "4315f5e1-0139-4ecf-9706-9df0aff33e5a_plan-name" return s }(), - valid: false, + valid: true, }, { - name: "invalid serviceClass - missing guid", + name: "invalid serviceClass - has no namespace", serviceClass: func() *servicecatalog.ServiceClass { s := validServiceClass() - s.Spec.ExternalID = "" + s.Namespace = "" return s }(), valid: false, }, { - name: "invalid serviceClass - invalid guid", + name: "invalid serviceClass - empty externalID", serviceClass: func() *servicecatalog.ServiceClass { s := validServiceClass() - s.Spec.ExternalID = "1234-4354a\\%-49b" + s.Spec.ExternalID = "" return s }(), valid: false, @@ -313,12 +314,13 @@ func TestValidateServiceClass(t *testing.T) { } for _, tc := range cases { - errs := ValidateServiceClass(tc.serviceClass) - if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - continue - } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) - } + t.Run(tc.name, func(t *testing.T) { + errs := ValidateServiceClass(tc.serviceClass) + if len(errs) != 0 && tc.valid { + t.Errorf("unexpected error: %v", errs) + } else if len(errs) == 0 && !tc.valid { + t.Error("unexpected success") + } + }) } } diff --git a/pkg/apis/servicecatalog/validation/serviceplan_test.go b/pkg/apis/servicecatalog/validation/serviceplan_test.go index 14d3abbe8f0..f3df7a3648f 100644 --- a/pkg/apis/servicecatalog/validation/serviceplan_test.go +++ b/pkg/apis/servicecatalog/validation/serviceplan_test.go @@ -202,9 +202,9 @@ func TestValidateClusterServicePlan(t *testing.T) { errs := ValidateClusterServicePlan(tc.clusterServicePlan) t.Log(errs) if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) + t.Errorf("unexpected error: %v", errs) } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) + t.Error("unexpected success") } }) } @@ -360,9 +360,9 @@ func TestValidateServicePlan(t *testing.T) { errs := ValidateServicePlan(tc.servicePlan) t.Log(errs) if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) + t.Errorf("unexpected error: %v", errs) } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) + t.Error("unexpected success") } }) } @@ -409,9 +409,9 @@ func TestValidateClusterServicePlanUpdate(t *testing.T) { errs := ValidateClusterServicePlanUpdate(tc.new, tc.old) t.Log(errs) if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) + t.Errorf("unexpected error: %v", errs) } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) + t.Error("unexpected success") } }) } @@ -458,9 +458,9 @@ func TestValidateServicePlanUpdate(t *testing.T) { errs := ValidateServicePlanUpdate(tc.new, tc.old) t.Log(errs) if len(errs) != 0 && tc.valid { - t.Errorf("%v: unexpected error: %v", tc.name, errs) + t.Errorf("unexpected error: %v", errs) } else if len(errs) == 0 && !tc.valid { - t.Errorf("%v: unexpected success", tc.name) + t.Error("unexpected success") } }) } diff --git a/pkg/apis/servicecatalog/zz_generated.deepcopy.go b/pkg/apis/servicecatalog/zz_generated.deepcopy.go index d623d4367a4..f3dae1d33ef 100644 --- a/pkg/apis/servicecatalog/zz_generated.deepcopy.go +++ b/pkg/apis/servicecatalog/zz_generated.deepcopy.go @@ -142,13 +142,13 @@ func (in *BearerTokenAuthConfig) DeepCopy() *BearerTokenAuthConfig { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *CatalogRestrictions) DeepCopyInto(out *CatalogRestrictions) { *out = *in - if in.ServicePlan != nil { - in, out := &in.ServicePlan, &out.ServicePlan + if in.ServiceClass != nil { + in, out := &in.ServiceClass, &out.ServiceClass *out = make([]string, len(*in)) copy(*out, *in) } - if in.ServiceClass != nil { - in, out := &in.ServiceClass, &out.ServiceClass + if in.ServicePlan != nil { + in, out := &in.ServicePlan, &out.ServicePlan *out = make([]string, len(*in)) copy(*out, *in) } diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index b0427bd63a8..a24942a068d 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -1426,3 +1426,62 @@ func (c *controller) getServiceBrokerForServiceBinding(instance *v1beta1.Service } return broker, nil } + +// shouldReconcileServiceBroker determines whether a broker should be reconciled; it +// returns true unless the broker has a ready condition with status true and +// the controller's broker relist interval has not elapsed since the broker's +// ready condition became true, or if the broker's RelistBehavior is set to Manual. +func shouldReconcileServiceBrokerCommon(pcb *pretty.ContextBuilder, brokerMeta *metav1.ObjectMeta, brokerSpec *v1beta1.CommonServiceBrokerSpec, brokerStatus *v1beta1.CommonServiceBrokerStatus, now time.Time, defaultRelistInterval time.Duration) bool { + if brokerStatus.ReconciledGeneration != brokerMeta.Generation { + // If the spec has changed, we should reconcile the broker. + return true + } + if brokerMeta.DeletionTimestamp != nil || len(brokerStatus.Conditions) == 0 { + // If the deletion timestamp is set or the broker has no status + // conditions, we should reconcile it. + return true + } + + // find the ready condition in the broker's status + for _, condition := range brokerStatus.Conditions { + if condition.Type == v1beta1.ServiceBrokerConditionReady { + // The broker has a ready condition + + if condition.Status == v1beta1.ConditionTrue { + + // The broker's ready condition has status true, meaning that + // at some point, we successfully listed the broker's catalog. + if brokerSpec.RelistBehavior == v1beta1.ServiceBrokerRelistBehaviorManual { + // If a broker is configured with RelistBehaviorManual, it should + // ignore the Duration and only relist based on spec changes + + glog.V(10).Info(pcb.Message("Not processing because RelistBehavior is set to Manual")) + return false + } + + // By default, the broker should relist if it has been longer than the + // RelistDuration since the last time we fetched the Catalog + duration := defaultRelistInterval + if brokerSpec.RelistDuration != nil { + duration = brokerSpec.RelistDuration.Duration + } + + intervalPassed := true + if brokerStatus.LastCatalogRetrievalTime != nil { + intervalPassed = now.After(brokerStatus.LastCatalogRetrievalTime.Time.Add(duration)) + } + if intervalPassed == false { + glog.V(10).Info(pcb.Message("Not processing because RelistDuration has not elapsed since the last relist")) + } + return intervalPassed + } + + // The broker's ready condition wasn't true; we should try to re- + // list the broker. + return true + } + } + + // The broker didn't have a ready condition; we should reconcile it. + return true +} diff --git a/pkg/controller/controller_clusterservicebroker.go b/pkg/controller/controller_clusterservicebroker.go index 1ce6395634d..734b3794982 100644 --- a/pkg/controller/controller_clusterservicebroker.go +++ b/pkg/controller/controller_clusterservicebroker.go @@ -94,60 +94,14 @@ func (c *controller) clusterServiceBrokerDelete(obj interface{}) { // the controller's broker relist interval has not elapsed since the broker's // ready condition became true, or if the broker's RelistBehavior is set to Manual. func shouldReconcileClusterServiceBroker(broker *v1beta1.ClusterServiceBroker, now time.Time, defaultRelistInterval time.Duration) bool { - pcb := pretty.NewClusterServiceBrokerContextBuilder(broker) - if broker.Status.ReconciledGeneration != broker.Generation { - // If the spec has changed, we should reconcile the broker. - return true - } - if broker.DeletionTimestamp != nil || len(broker.Status.Conditions) == 0 { - // If the deletion timestamp is set or the broker has no status - // conditions, we should reconcile it. - return true - } - - // find the ready condition in the broker's status - for _, condition := range broker.Status.Conditions { - if condition.Type == v1beta1.ServiceBrokerConditionReady { - // The broker has a ready condition - - if condition.Status == v1beta1.ConditionTrue { - - // The broker's ready condition has status true, meaning that - // at some point, we successfully listed the broker's catalog. - if broker.Spec.RelistBehavior == v1beta1.ServiceBrokerRelistBehaviorManual { - // If a broker is configured with RelistBehaviorManual, it should - // ignore the Duration and only relist based on spec changes - - glog.V(10).Info(pcb.Message("Not processing because RelistBehavior is set to Manual")) - return false - } - - // By default, the broker should relist if it has been longer than the - // RelistDuration since the last time we fetched the Catalog - duration := defaultRelistInterval - - if broker.Spec.RelistDuration != nil { - duration = broker.Spec.RelistDuration.Duration - } - - intervalPassed := true - if broker.Status.LastCatalogRetrievalTime != nil { - intervalPassed = now.After(broker.Status.LastCatalogRetrievalTime.Time.Add(duration)) - } - if intervalPassed == false { - glog.V(10).Info(pcb.Message("Not processing because RelistDuration has not elapsed since the last relist")) - } - return intervalPassed - } - - // The broker's ready condition wasn't true; we should try to re- - // list the broker. - return true - } - } - - // The broker didn't have a ready condition; we should reconcile it. - return true + return shouldReconcileServiceBrokerCommon( + pretty.NewClusterServiceBrokerContextBuilder(broker), + &broker.ObjectMeta, + &broker.Spec.CommonServiceBrokerSpec, + &broker.Status.CommonServiceBrokerStatus, + now, + defaultRelistInterval, + ) } func (c *controller) reconcileClusterServiceBrokerKey(key string) error { diff --git a/pkg/controller/controller_clusterservicebroker_test.go b/pkg/controller/controller_clusterservicebroker_test.go index 9f47350083e..23730d73046 100644 --- a/pkg/controller/controller_clusterservicebroker_test.go +++ b/pkg/controller/controller_clusterservicebroker_test.go @@ -173,9 +173,10 @@ func TestShouldReconcileClusterServiceBroker(t *testing.T) { reconcile: true, }, { - name: "ready, duration behavior, nil duration", + name: "ready, duration behavior, nil duration, interval not elapsed", broker: func() *v1beta1.ClusterServiceBroker { - broker := getTestClusterServiceBrokerWithStatus(v1beta1.ConditionTrue) + t := metav1.NewTime(time.Now().Add(-23 * time.Hour)) + broker := getTestClusterServiceBrokerWithStatusAndTime(v1beta1.ConditionTrue, t, t) broker.Spec.RelistBehavior = v1beta1.ServiceBrokerRelistBehaviorDuration broker.Spec.RelistDuration = nil return broker @@ -183,6 +184,18 @@ func TestShouldReconcileClusterServiceBroker(t *testing.T) { now: time.Now(), reconcile: false, }, + { + name: "ready, duration behavior, nil duration, interval elapsed", + broker: func() *v1beta1.ClusterServiceBroker { + t := metav1.NewTime(time.Now().Add(-25 * time.Hour)) + broker := getTestClusterServiceBrokerWithStatusAndTime(v1beta1.ConditionTrue, t, t) + broker.Spec.RelistBehavior = v1beta1.ServiceBrokerRelistBehaviorDuration + broker.Spec.RelistDuration = nil + return broker + }(), + now: time.Now(), + reconcile: true, + }, { name: "ready, manual behavior", broker: func() *v1beta1.ClusterServiceBroker { @@ -196,24 +209,26 @@ func TestShouldReconcileClusterServiceBroker(t *testing.T) { } for _, tc := range cases { - var ltt *time.Time - if len(tc.broker.Status.Conditions) != 0 { - ltt = &tc.broker.Status.Conditions[0].LastTransitionTime.Time - } + t.Run(tc.name, func(t *testing.T) { + var ltt *time.Time + if len(tc.broker.Status.Conditions) != 0 { + ltt = &tc.broker.Status.Conditions[0].LastTransitionTime.Time + } - if tc.broker.Spec.RelistDuration != nil { - interval := tc.broker.Spec.RelistDuration.Duration - lastRelistTime := tc.broker.Status.LastCatalogRetrievalTime - t.Logf("%v: now: %v, interval: %v, last transition time: %v, last relist time: %v", tc.name, tc.now, interval, ltt, lastRelistTime) - } else { - t.Logf("broker.Spec.RelistDuration set to nil") - } + if tc.broker.Spec.RelistDuration != nil { + interval := tc.broker.Spec.RelistDuration.Duration + lastRelistTime := tc.broker.Status.LastCatalogRetrievalTime + t.Logf("now: %v, interval: %v, last transition time: %v, last relist time: %v", tc.now, interval, ltt, lastRelistTime) + } else { + t.Logf("broker.Spec.RelistDuration set to nil") + } - actual := shouldReconcileClusterServiceBroker(tc.broker, tc.now, 24*time.Hour) + actual := shouldReconcileClusterServiceBroker(tc.broker, tc.now, 24*time.Hour) - if e, a := tc.reconcile, actual; e != a { - t.Errorf("%v: unexpected result: %s", tc.name, expectedGot(e, a)) - } + if e, a := tc.reconcile, actual; e != a { + t.Errorf("unexpected result: %s", expectedGot(e, a)) + } + }) } } diff --git a/pkg/controller/controller_clusterserviceclass.go b/pkg/controller/controller_clusterserviceclass.go index 9626d96c712..2c508b74243 100644 --- a/pkg/controller/controller_clusterserviceclass.go +++ b/pkg/controller/controller_clusterserviceclass.go @@ -55,7 +55,7 @@ func (c *controller) clusterServiceClassDelete(obj interface{}) { // reconciliation loop for ClusterServiceClass. ClusterServiceClasses are primarily // reconciled in a separate flow when a ClusterServiceBroker is reconciled. func (c *controller) reconcileClusterServiceClassKey(key string) error { - plan, err := c.clusterServiceClassLister.Get(key) + class, err := c.clusterServiceClassLister.Get(key) if errors.IsNotFound(err) { glog.Infof("ClusterServiceClass %q: Not doing work because it has been deleted", key) return nil @@ -65,7 +65,7 @@ func (c *controller) reconcileClusterServiceClassKey(key string) error { return err } - return c.reconcileClusterServiceClass(plan) + return c.reconcileClusterServiceClass(class) } func (c *controller) reconcileClusterServiceClass(serviceClass *v1beta1.ClusterServiceClass) error { diff --git a/pkg/controller/controller_servicebroker.go b/pkg/controller/controller_servicebroker.go index cd8488a27d9..b7696b55b55 100644 --- a/pkg/controller/controller_servicebroker.go +++ b/pkg/controller/controller_servicebroker.go @@ -82,63 +82,15 @@ func (c *controller) serviceBrokerDelete(obj interface{}) { // returns true unless the broker has a ready condition with status true and // the controller's broker relist interval has not elapsed since the broker's // ready condition became true, or if the broker's RelistBehavior is set to Manual. -func shouldReconcileServiceBroker(broker *v1beta1.ServiceBroker, now time.Time) bool { - // ERIK TODO: This should get refactored out into a shared method because it - // only relies on Common components. - pcb := pretty.NewServiceBrokerContextBuilder(broker) - if broker.Status.ReconciledGeneration != broker.Generation { - // If the spec has changed, we should reconcile the broker. - return true - } - if broker.DeletionTimestamp != nil || len(broker.Status.Conditions) == 0 { - // If the deletion timestamp is set or the broker has no status - // conditions, we should reconcile it. - return true - } - - // find the ready condition in the broker's status - for _, condition := range broker.Status.Conditions { - if condition.Type == v1beta1.ServiceBrokerConditionReady { - // The broker has a ready condition - - if condition.Status == v1beta1.ConditionTrue { - - // The broker's ready condition has status true, meaning that - // at some point, we successfully listed the broker's catalog. - if broker.Spec.RelistBehavior == v1beta1.ServiceBrokerRelistBehaviorManual { - // If a broker is configured with RelistBehaviorManual, it should - // ignore the Duration and only relist based on spec changes - - glog.V(10).Info(pcb.Message("Not processing because RelistBehavior is set to Manual")) - return false - } - - if broker.Spec.RelistDuration == nil { - glog.Error(pcb.Message("Unable to process because RelistBehavior is set to Duration with a nil RelistDuration value")) - return false - } - - // By default, the broker should relist if it has been longer than the - // RelistDuration since the last time we fetched the Catalog - duration := broker.Spec.RelistDuration.Duration - intervalPassed := true - if broker.Status.LastCatalogRetrievalTime != nil { - intervalPassed = now.After(broker.Status.LastCatalogRetrievalTime.Time.Add(duration)) - } - if intervalPassed == false { - glog.V(10).Info(pcb.Message("Not processing because RelistDuration has not elapsed since the last relist")) - } - return intervalPassed - } - - // The broker's ready condition wasn't true; we should try to re- - // list the broker. - return true - } - } - - // The broker didn't have a ready condition; we should reconcile it. - return true +func shouldReconcileServiceBroker(broker *v1beta1.ServiceBroker, now time.Time, defaultRelistInterval time.Duration) bool { + return shouldReconcileServiceBrokerCommon( + pretty.NewServiceBrokerContextBuilder(broker), + &broker.ObjectMeta, + &broker.Spec.CommonServiceBrokerSpec, + &broker.Status.CommonServiceBrokerStatus, + now, + defaultRelistInterval, + ) } func (c *controller) reconcileServiceBrokerKey(key string) error { @@ -171,7 +123,7 @@ func (c *controller) reconcileServiceBroker(broker *v1beta1.ServiceBroker) error // set to Manual, do not reconcile it. // * If the broker's ready condition is true and the relist interval has not // elapsed, do not reconcile it. - if !shouldReconcileServiceBroker(broker, time.Now()) { + if !shouldReconcileServiceBroker(broker, time.Now(), c.brokerRelistInterval) { return nil } diff --git a/pkg/controller/controller_servicebroker_test.go b/pkg/controller/controller_servicebroker_test.go index d3d6b602371..e04523a6ca4 100644 --- a/pkg/controller/controller_servicebroker_test.go +++ b/pkg/controller/controller_servicebroker_test.go @@ -20,15 +20,27 @@ import ( "errors" "fmt" "testing" + "time" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" scfeatures "github.com/kubernetes-incubator/service-catalog/pkg/features" "github.com/kubernetes-incubator/service-catalog/test/fake" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" utilfeature "k8s.io/apiserver/pkg/util/feature" clientgotesting "k8s.io/client-go/testing" ) +// NOTE: This only tests a single test case. Others are tested in TestShouldReconcileClusterServiceBroker. +func TestShouldReconcileServiceBroker(t *testing.T) { + broker := getTestClusterServiceBroker() + broker.Spec.RelistDuration = &metav1.Duration{Duration: 3 * time.Minute} + + if !shouldReconcileClusterServiceBroker(broker, time.Now(), 24*time.Hour) { + t.Error("expected true, bot got false") + } +} + func TestReconcileServiceClassFromServiceBrokerCatalog(t *testing.T) { updatedClass := func() *v1beta1.ServiceClass { p := getTestServiceClass() diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 22fbfab8859..3d7827eb8ae 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -490,19 +490,8 @@ func getTestClusterServiceBroker() *v1beta1.ClusterServiceBroker { } func getTestClusterServiceBrokerWithStatus(status v1beta1.ConditionStatus) *v1beta1.ClusterServiceBroker { - lastTransitionTime := metav1.NewTime(time.Now().Add(-5 * time.Minute)) - broker := getTestClusterServiceBroker() - broker.Status = v1beta1.ClusterServiceBrokerStatus{ - CommonServiceBrokerStatus: v1beta1.CommonServiceBrokerStatus{ - Conditions: []v1beta1.ServiceBrokerCondition{{ - Type: v1beta1.ServiceBrokerConditionReady, - Status: status, - LastTransitionTime: lastTransitionTime, - }}, - LastCatalogRetrievalTime: &lastTransitionTime, - }, - } - return broker + t := metav1.NewTime(time.Now().Add(-5 * time.Minute)) + return getTestClusterServiceBrokerWithStatusAndTime(status, t, t) } func getTestClusterServiceBrokerWithStatusAndTime(status v1beta1.ConditionStatus, lastTransitionTime, lastRelistTime metav1.Time) *v1beta1.ClusterServiceBroker { diff --git a/pkg/filter/helper.go b/pkg/filter/helper.go index 904bf1af22c..7fdc8e06995 100644 --- a/pkg/filter/helper.go +++ b/pkg/filter/helper.go @@ -18,10 +18,13 @@ package filter import ( "fmt" + "regexp" "k8s.io/apimachinery/pkg/labels" ) +var conditionalsRegex = regexp.MustCompile("=|==|!=| in | notin ") + // CreatePredicate creates the Predicate that will be used to // test if acceptance is allowed for service classes. func CreatePredicate(restrictions []string) (Predicate, error) { @@ -47,3 +50,10 @@ func CreatePredicate(restrictions []string) (Predicate, error) { func ConvertToSelector(p Predicate) (labels.Selector, error) { return labels.Parse(p.String()) } + +// ExtractProperty extracts the property from the given restriction +// E.g., for the restriction "spec.externalName=foo", the function +// returns "spec.externalName" +func ExtractProperty(restriction string) string { + return conditionalsRegex.Split(restriction, 2)[0] +} diff --git a/pkg/svcat/service-catalog/instance.go b/pkg/svcat/service-catalog/instance.go index 0f0060833ad..d2fde6049ed 100644 --- a/pkg/svcat/service-catalog/instance.go +++ b/pkg/svcat/service-catalog/instance.go @@ -22,6 +22,7 @@ import ( "time" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" @@ -37,7 +38,7 @@ const ( func (sdk *SDK) RetrieveInstances(ns, classFilter, planFilter string) (*v1beta1.ServiceInstanceList, error) { instances, err := sdk.ServiceCatalog().ServiceInstances(ns).List(v1.ListOptions{}) if err != nil { - return nil, fmt.Errorf("unable to list instances in %s (%s)", ns, err) + return nil, errors.Wrapf(err, "unable to list instances in %s", ns) } if classFilter == "" && planFilter == "" { @@ -225,6 +226,27 @@ func (sdk *SDK) TouchInstance(ns, name string, retries int) error { return fmt.Errorf("could not sync service broker after %d tries", retries) } +// WaitForInstanceToNotExist waits for the specified instance to no longer exist. +func (sdk *SDK) WaitForInstanceToNotExist(ns, name string, interval time.Duration, timeout *time.Duration) (instance *v1beta1.ServiceInstance, err error) { + if timeout == nil { + notimeout := time.Duration(math.MaxInt64) + timeout = ¬imeout + } + + err = wait.PollImmediate(interval, *timeout, + func() (bool, error) { + instance, err = sdk.ServiceCatalog().ServiceInstances(ns).Get(name, v1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + err = nil + } + return true, err + } + return false, err + }) + return instance, err +} + // WaitForInstance waits for the instance to complete the current operation (or fail). func (sdk *SDK) WaitForInstance(ns, name string, interval time.Duration, timeout *time.Duration) (instance *v1beta1.ServiceInstance, err error) { if timeout == nil { @@ -236,9 +258,6 @@ func (sdk *SDK) WaitForInstance(ns, name string, interval time.Duration, timeout func() (bool, error) { instance, err = sdk.RetrieveInstance(ns, name) if nil != err { - if apierrors.IsNotFound(err) { - return true, nil - } return false, err } diff --git a/pkg/svcat/service-catalog/instance_test.go b/pkg/svcat/service-catalog/instance_test.go index 39d7c1e8f40..0fff93ed807 100644 --- a/pkg/svcat/service-catalog/instance_test.go +++ b/pkg/svcat/service-catalog/instance_test.go @@ -18,9 +18,12 @@ package servicecatalog_test import ( "fmt" + "strings" + "time" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" "github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/clientset/fake" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" @@ -448,7 +451,7 @@ var _ = Describe("Instances", func() { }) }) Describe("Provision", func() { - It("Calls the v1beta1 Create method with the passed in arguements", func() { + It("Calls the v1beta1 Create method with the passed in arguments", func() { namespace := "cherry_namespace" instanceName := "cherry" externalID := "cherry-external-id" @@ -516,7 +519,7 @@ var _ = Describe("Instances", func() { }) }) Describe("Deprovision", func() { - It("Calls the v1beta1 Delete method wiht the passed in service instance name", func() { + It("Calls the v1beta1 Delete method with the passed in service instance name", func() { err := sdk.Deprovision(si.Namespace, si.Name) Expect(err).NotTo(HaveOccurred()) actions := svcCatClient.Actions() @@ -539,4 +542,51 @@ var _ = Describe("Instances", func() { Expect(actions[0].Matches("delete", "serviceinstances")).To(BeTrue()) Expect(actions[0].(testing.DeleteActionImpl).Name).To(Equal(si.Name)) }) + Describe("WaitForInstanceToNotExist", func() { + It("Calls the v1beta1 WaitForInstanceToNotExist method with the passed in service instance name", func() { + badClient := &fake.Clientset{} + badClient.AddReactor("get", "serviceinstances", func(action testing.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewNotFound(v1beta1.Resource("serviceinstance"), "instance not found") + }) + sdk.ServiceCatalogClient = badClient + timeout := 5 * time.Second + instance, err := sdk.WaitForInstanceToNotExist(si.Namespace, si.Name, 1*time.Second, &timeout) + Expect(err).NotTo(HaveOccurred()) + Expect(instance).To(BeNil()) + actions := badClient.Actions() + Expect(actions[0].Matches("get", "serviceinstances")).To(BeTrue()) + Expect(actions[0].(testing.GetActionImpl).Name).To(Equal("foobar")) + Expect(actions[0].(testing.GetActionImpl).Namespace).To(Equal("foobar_namespace")) + }) + }) + It("Bubbles up errors", func() { + si = &v1beta1.ServiceInstance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foobar", + Namespace: "foobar_namespace", + }, + Spec: v1beta1.ServiceInstanceSpec{ + ClusterServicePlanRef: &v1beta1.ClusterObjectReference{ + Name: "not_real_plan", + }, + ClusterServiceClassRef: &v1beta1.ClusterObjectReference{ + Name: "not_real_class", + }, + }, + } + badClient := &fake.Clientset{} + badClient.AddReactor("get", "serviceinstances", func(action testing.Action) (bool, runtime.Object, error) { + return true, si, nil + }) + sdk.ServiceCatalogClient = badClient + timeout := 1 * time.Second + instance, err := sdk.WaitForInstanceToNotExist(si.Namespace, si.Name, 1*time.Second, &timeout) + Expect(err).To(HaveOccurred()) + Expect(strings.Contains(err.Error(), "timed out waiting for the condition")) + Expect(instance).ToNot(BeNil()) + actions := badClient.Actions() + Expect(actions[0].Matches("get", "serviceinstances")).To(BeTrue()) + Expect(actions[0].(testing.GetActionImpl).Name).To(Equal("foobar")) + Expect(actions[0].(testing.GetActionImpl).Namespace).To(Equal("foobar_namespace")) + }) }) diff --git a/pkg/svcat/service-catalog/plan.go b/pkg/svcat/service-catalog/plan.go index a30bf0c6e14..1969733049e 100644 --- a/pkg/svcat/service-catalog/plan.go +++ b/pkg/svcat/service-catalog/plan.go @@ -20,6 +20,7 @@ import ( "fmt" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" + "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" ) @@ -32,37 +33,72 @@ const ( FieldServiceClassRef = "spec.clusterServiceClassRef.name" ) +// RetrievePlanOptions allows to specify which plans will be retrieved +type RetrievePlanOptions struct { + ClassID string + Namespace string + Scope Scope +} + // Plan provides a unifying layer of cluster and namespace scoped plan resources. type Plan interface { // GetName returns the plan's name. GetName() string + // GetNamespace returns the plan's namespace, or "" if it's cluster-scoped. + GetNamespace() string + // GetExternalName returns the plan's external name. GetExternalName() string // GetDescription returns the plan description. GetDescription() string + + // GetClassID returns the plan's class name. + GetClassID() string } // RetrievePlans lists all plans defined in the cluster. -func (sdk *SDK) RetrievePlans(opts *FilterOptions) ([]v1beta1.ClusterServicePlan, error) { - plans, err := sdk.ServiceCatalog().ClusterServicePlans().List(v1.ListOptions{}) - if err != nil { - return nil, fmt.Errorf("unable to list plans (%s)", err) +func (sdk *SDK) RetrievePlans(opts RetrievePlanOptions) ([]Plan, error) { + var plans []Plan + + if opts.Scope.Matches(ClusterScope) { + csp, err := sdk.ServiceCatalog().ClusterServicePlans().List(v1.ListOptions{}) + if err != nil { + return nil, fmt.Errorf("unable to list cluster-scoped plans (%s)", err) + } + + for _, p := range csp.Items { + if opts.ClassID != "" && p.GetClassID() != opts.ClassID { + continue + } + + plan := p + plans = append(plans, &plan) + } } - if opts != nil && opts.ClassID != "" { - plansFiltered := make([]v1beta1.ClusterServicePlan, 0) - for _, p := range plans.Items { - if p.Spec.ClusterServiceClassRef.Name == opts.ClassID { - plansFiltered = append(plansFiltered, p) + if opts.Scope.Matches(NamespaceScope) { + sp, err := sdk.ServiceCatalog().ServicePlans(opts.Namespace).List(v1.ListOptions{}) + if err != nil { + // Gracefully handle when the feature-flag for namespaced broker resources isn't enabled on the server. + if errors.IsNotFound(err) { + return plans, nil } + return nil, fmt.Errorf("unable to list plans in %q (%s)", opts.Namespace, err) + } + for _, p := range sp.Items { + if opts.ClassID != "" && p.GetClassID() != opts.ClassID { + continue + } + + plan := p + plans = append(plans, &plan) } - return plansFiltered, nil } - return plans.Items, nil + return plans, nil } // RetrievePlanByName gets a plan by its external name. @@ -92,20 +128,6 @@ func (sdk *SDK) RetrievePlanByID(uuid string) (*v1beta1.ClusterServicePlan, erro return plan, nil } -// RetrievePlansByClass retrieves all plans for a class. -func (sdk *SDK) RetrievePlansByClass(class *v1beta1.ClusterServiceClass, -) ([]v1beta1.ClusterServicePlan, error) { - planOpts := v1.ListOptions{ - FieldSelector: fields.OneTermEqualSelector(FieldServiceClassRef, class.Name).String(), - } - plans, err := sdk.ServiceCatalog().ClusterServicePlans().List(planOpts) - if err != nil { - return nil, fmt.Errorf("unable to list plans (%s)", err) - } - - return plans.Items, nil -} - // RetrievePlanByClassAndPlanNames gets a plan by its class/plan name combination. func (sdk *SDK) RetrievePlanByClassAndPlanNames(className, planName string, ) (*v1beta1.ClusterServicePlan, error) { diff --git a/pkg/svcat/service-catalog/plan_test.go b/pkg/svcat/service-catalog/plan_test.go index 8da84f790d7..2f410bf354e 100644 --- a/pkg/svcat/service-catalog/plan_test.go +++ b/pkg/svcat/service-catalog/plan_test.go @@ -36,14 +36,25 @@ var _ = Describe("Plan", func() { var ( sdk *SDK svcCatClient *fake.Clientset - sp *v1beta1.ClusterServicePlan - sp2 *v1beta1.ClusterServicePlan + csc *v1beta1.ClusterServiceClass + csp *v1beta1.ClusterServicePlan + csp2 *v1beta1.ClusterServicePlan + sp *v1beta1.ServicePlan + sp2 *v1beta1.ServicePlan ) BeforeEach(func() { - sp = &v1beta1.ClusterServicePlan{ObjectMeta: metav1.ObjectMeta{Name: "foobar"}} - sp2 = &v1beta1.ClusterServicePlan{ObjectMeta: metav1.ObjectMeta{Name: "barbaz"}} - svcCatClient = fake.NewSimpleClientset(sp, sp2) + csc = &v1beta1.ClusterServiceClass{ObjectMeta: metav1.ObjectMeta{Name: "someclass"}} + csp = &v1beta1.ClusterServicePlan{ObjectMeta: metav1.ObjectMeta{Name: "foobar"}} + csp2 = &v1beta1.ClusterServicePlan{ + ObjectMeta: metav1.ObjectMeta{Name: "barbaz"}, + Spec: v1beta1.ClusterServicePlanSpec{ + ClusterServiceClassRef: v1beta1.ClusterObjectReference{Name: csc.Name}, + }, + } + sp = &v1beta1.ServicePlan{ObjectMeta: metav1.ObjectMeta{Name: "foobar", Namespace: "default"}} + sp2 = &v1beta1.ServicePlan{ObjectMeta: metav1.ObjectMeta{Name: "barbaz", Namespace: "ns2"}} + svcCatClient = fake.NewSimpleClientset(csc, csp, csp2, sp, sp2) sdk = &SDK{ ServiceCatalogClient: svcCatClient, } @@ -51,12 +62,38 @@ var _ = Describe("Plan", func() { Describe("RetrivePlans", func() { It("Calls the generated v1beta1 List method", func() { - plans, err := sdk.RetrievePlans(nil) + plans, err := sdk.RetrievePlans(RetrievePlanOptions{Scope: AllScope}) + + Expect(err).NotTo(HaveOccurred()) + Expect(plans).Should(ConsistOf(csp, csp2, sp, sp2)) + Expect(svcCatClient.Actions()[0].Matches("list", "clusterserviceplans")).To(BeTrue()) + Expect(svcCatClient.Actions()[1].Matches("list", "serviceplans")).To(BeTrue()) + }) + It("Filters by namespace scope", func() { + plans, err := sdk.RetrievePlans(RetrievePlanOptions{Scope: NamespaceScope, Namespace: "default"}) + + Expect(err).NotTo(HaveOccurred()) + Expect(plans).Should(ConsistOf(sp)) + Expect(len(svcCatClient.Actions())).Should(Equal(1)) + Expect(svcCatClient.Actions()[0].Matches("list", "serviceplans")).To(BeTrue()) + }) + It("Filters by cluster scope", func() { + plans, err := sdk.RetrievePlans(RetrievePlanOptions{Scope: ClusterScope}) Expect(err).NotTo(HaveOccurred()) - Expect(plans).Should(ConsistOf(*sp, *sp2)) + Expect(plans).Should(ConsistOf(csp, csp2)) + Expect(len(svcCatClient.Actions())).Should(Equal(1)) Expect(svcCatClient.Actions()[0].Matches("list", "clusterserviceplans")).To(BeTrue()) }) + It("Filter by class", func() { + plans, err := sdk.RetrievePlans(RetrievePlanOptions{Scope: AllScope, ClassID: csc.Name}) + + Expect(err).NotTo(HaveOccurred()) + Expect(plans).Should(ConsistOf(csp2)) + Expect(len(svcCatClient.Actions())).Should(Equal(2)) + Expect(svcCatClient.Actions()[0].Matches("list", "clusterserviceplans")).To(BeTrue()) + Expect(svcCatClient.Actions()[1].Matches("list", "serviceplans")).To(BeTrue()) + }) It("Bubbles up errors", func() { errorMessage := "error retrieving list" badClient := &fake.Clientset{} @@ -64,7 +101,7 @@ var _ = Describe("Plan", func() { return true, nil, fmt.Errorf(errorMessage) }) sdk.ServiceCatalogClient = badClient - _, err := sdk.RetrievePlans(nil) + _, err := sdk.RetrievePlans(RetrievePlanOptions{Scope: AllScope}) Expect(err).To(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring(errorMessage)) @@ -73,10 +110,10 @@ var _ = Describe("Plan", func() { }) Describe("RetrievePlanByName", func() { It("Calls the generated v1beta1 List method with the passed in plan name", func() { - planName := sp.Name + planName := csp.Name singleClient := &fake.Clientset{} singleClient.AddReactor("list", "clusterserviceplans", func(action testing.Action) (bool, runtime.Object, error) { - return true, &v1beta1.ClusterServicePlanList{Items: []v1beta1.ClusterServicePlan{*sp}}, nil + return true, &v1beta1.ClusterServicePlanList{Items: []v1beta1.ClusterServicePlan{*csp}}, nil }) sdk.ServiceCatalogClient = singleClient @@ -113,7 +150,7 @@ var _ = Describe("Plan", func() { }) Describe("RetrievePlanByID", func() { It("Calls the generated v1beta1 get method with the passed in uuid", func() { - planID := sp.Name + planID := csp.Name _, err := sdk.RetrievePlanByID(planID) Expect(err).NotTo(HaveOccurred()) actions := svcCatClient.Actions() @@ -141,60 +178,4 @@ var _ = Describe("Plan", func() { Expect(actions[0].(testing.GetActionImpl).Name).To(Equal(planID)) }) }) - Describe("RetrievePlansByClass", func() { - It("Calls the generated v1beta1 List method with an opts containing the passed in class' name", func() { - class := &v1beta1.ClusterServiceClass{ - ObjectMeta: metav1.ObjectMeta{ - Name: "durian_class", - }, - Spec: v1beta1.ClusterServiceClassSpec{}, - } - plan := &v1beta1.ClusterServicePlan{ - ObjectMeta: metav1.ObjectMeta{ - Name: "durian", - }, - Spec: v1beta1.ClusterServicePlanSpec{ - ClusterServiceClassRef: v1beta1.ClusterObjectReference{ - Name: class.Name, - }, - }, - } - linkedClient := fake.NewSimpleClientset(class, plan) - linkedClient.AddReactor("list", "clusterserviceplans", func(action testing.Action) (bool, runtime.Object, error) { - return true, &v1beta1.ClusterServicePlanList{Items: []v1beta1.ClusterServicePlan{*plan}}, nil - }) - sdk.ServiceCatalogClient = linkedClient - retPlans, err := sdk.RetrievePlansByClass(class) - Expect(retPlans).To(ConsistOf(*plan)) - Expect(err).NotTo(HaveOccurred()) - actions := linkedClient.Actions() - Expect(len(actions)).To(Equal(1)) - Expect(actions[0].Matches("list", "clusterserviceplans")).To(BeTrue()) - opts := fields.Set{"spec.clusterServiceClassRef.name": class.Name} - Expect(actions[0].(testing.ListActionImpl).GetListRestrictions().Fields.Matches(opts)).To(BeTrue()) - }) - It("Bubbles up errors", func() { - errorMessage := "no plans found" - class := &v1beta1.ClusterServiceClass{ - ObjectMeta: metav1.ObjectMeta{ - Name: "durian_class", - }, - Spec: v1beta1.ClusterServiceClassSpec{}, - } - badClient := &fake.Clientset{} - badClient.AddReactor("list", "clusterserviceplans", func(action testing.Action) (bool, runtime.Object, error) { - return true, nil, fmt.Errorf(errorMessage) - }) - sdk.ServiceCatalogClient = badClient - - plans, err := sdk.RetrievePlansByClass(class) - Expect(plans).To(BeNil()) - Expect(err).To(HaveOccurred()) - actions := badClient.Actions() - Expect(len(actions)).To(Equal(1)) - Expect(actions[0].Matches("list", "clusterserviceplans")).To(BeTrue()) - opts := fields.Set{"spec.clusterServiceClassRef.name": class.Name} - Expect(actions[0].(testing.ListActionImpl).GetListRestrictions().Fields.Matches(opts)).To(BeTrue()) - }) - }) }) diff --git a/pkg/svcat/service-catalog/sdk.go b/pkg/svcat/service-catalog/sdk.go index 678a394fbf9..4e5bbd9c61f 100644 --- a/pkg/svcat/service-catalog/sdk.go +++ b/pkg/svcat/service-catalog/sdk.go @@ -69,11 +69,11 @@ type SvcatClient interface { RetrieveInstancesByPlan(*apiv1beta1.ClusterServicePlan) ([]apiv1beta1.ServiceInstance, error) TouchInstance(string, string, int) error WaitForInstance(string, string, time.Duration, *time.Duration) (*apiv1beta1.ServiceInstance, error) + WaitForInstanceToNotExist(string, string, time.Duration, *time.Duration) (*apiv1beta1.ServiceInstance, error) - RetrievePlans(*FilterOptions) ([]apiv1beta1.ClusterServicePlan, error) + RetrievePlans(RetrievePlanOptions) ([]Plan, error) RetrievePlanByName(string) (*apiv1beta1.ClusterServicePlan, error) RetrievePlanByID(string) (*apiv1beta1.ClusterServicePlan, error) - RetrievePlansByClass(*apiv1beta1.ClusterServiceClass) ([]apiv1beta1.ClusterServicePlan, error) RetrievePlanByClassAndPlanNames(string, string) (*apiv1beta1.ClusterServicePlan, error) RetrieveSecretByBinding(*apiv1beta1.ServiceBinding) (*apicorev1.Secret, error) diff --git a/pkg/svcat/service-catalog/service-catalogfakes/fake_svcat_client.go b/pkg/svcat/service-catalog/service-catalogfakes/fake_svcat_client.go index f35bebe948b..a051a84a432 100644 --- a/pkg/svcat/service-catalog/service-catalogfakes/fake_svcat_client.go +++ b/pkg/svcat/service-catalog/service-catalogfakes/fake_svcat_client.go @@ -495,17 +495,33 @@ type FakeSvcatClient struct { result1 *apiv1beta1.ServiceInstance result2 error } - RetrievePlansStub func(*servicecatalog.FilterOptions) ([]apiv1beta1.ClusterServicePlan, error) + WaitForInstanceToNotExistStub func(string, string, time.Duration, *time.Duration) (*apiv1beta1.ServiceInstance, error) + waitForInstanceToNotExistMutex sync.RWMutex + waitForInstanceToNotExistArgsForCall []struct { + arg1 string + arg2 string + arg3 time.Duration + arg4 *time.Duration + } + waitForInstanceToNotExistReturns struct { + result1 *apiv1beta1.ServiceInstance + result2 error + } + waitForInstanceToNotExistReturnsOnCall map[int]struct { + result1 *apiv1beta1.ServiceInstance + result2 error + } + RetrievePlansStub func(servicecatalog.RetrievePlanOptions) ([]servicecatalog.Plan, error) retrievePlansMutex sync.RWMutex retrievePlansArgsForCall []struct { - arg1 *servicecatalog.FilterOptions + arg1 servicecatalog.RetrievePlanOptions } retrievePlansReturns struct { - result1 []apiv1beta1.ClusterServicePlan + result1 []servicecatalog.Plan result2 error } retrievePlansReturnsOnCall map[int]struct { - result1 []apiv1beta1.ClusterServicePlan + result1 []servicecatalog.Plan result2 error } RetrievePlanByNameStub func(string) (*apiv1beta1.ClusterServicePlan, error) @@ -534,19 +550,6 @@ type FakeSvcatClient struct { result1 *apiv1beta1.ClusterServicePlan result2 error } - RetrievePlansByClassStub func(*apiv1beta1.ClusterServiceClass) ([]apiv1beta1.ClusterServicePlan, error) - retrievePlansByClassMutex sync.RWMutex - retrievePlansByClassArgsForCall []struct { - arg1 *apiv1beta1.ClusterServiceClass - } - retrievePlansByClassReturns struct { - result1 []apiv1beta1.ClusterServicePlan - result2 error - } - retrievePlansByClassReturnsOnCall map[int]struct { - result1 []apiv1beta1.ClusterServicePlan - result2 error - } RetrievePlanByClassAndPlanNamesStub func(string, string) (*apiv1beta1.ClusterServicePlan, error) retrievePlanByClassAndPlanNamesMutex sync.RWMutex retrievePlanByClassAndPlanNamesArgsForCall []struct { @@ -2295,6 +2298,26 @@ func (fake *FakeSvcatClient) TouchInstanceReturnsOnCall(i int, result1 error) { }{result1} } +func (fake *FakeSvcatClient) WaitForInstanceToNotExist(arg1 string, arg2 string, arg3 time.Duration, arg4 *time.Duration) (*apiv1beta1.ServiceInstance, error) { + fake.waitForInstanceToNotExistMutex.Lock() + ret, specificReturn := fake.waitForInstanceToNotExistReturnsOnCall[len(fake.waitForInstanceToNotExistArgsForCall)] + fake.waitForInstanceToNotExistArgsForCall = append(fake.waitForInstanceToNotExistArgsForCall, struct { + arg1 string + arg2 string + arg3 time.Duration + arg4 *time.Duration + }{arg1, arg2, arg3, arg4}) + fake.recordInvocation("WaitForInstanceToNotExist", []interface{}{arg1, arg2, arg3, arg4}) + fake.waitForInstanceToNotExistMutex.Unlock() + if fake.WaitForInstanceToNotExistStub != nil { + return fake.WaitForInstanceToNotExistStub(arg1, arg2, arg3, arg4) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fake.waitForInstanceToNotExistReturns.result1, fake.waitForInstanceToNotExistReturns.result2 +} + func (fake *FakeSvcatClient) WaitForInstance(arg1 string, arg2 string, arg3 time.Duration, arg4 *time.Duration) (*apiv1beta1.ServiceInstance, error) { fake.waitForInstanceMutex.Lock() ret, specificReturn := fake.waitForInstanceReturnsOnCall[len(fake.waitForInstanceArgsForCall)] @@ -2349,11 +2372,11 @@ func (fake *FakeSvcatClient) WaitForInstanceReturnsOnCall(i int, result1 *apiv1b }{result1, result2} } -func (fake *FakeSvcatClient) RetrievePlans(arg1 *servicecatalog.FilterOptions) ([]apiv1beta1.ClusterServicePlan, error) { +func (fake *FakeSvcatClient) RetrievePlans(arg1 servicecatalog.RetrievePlanOptions) ([]servicecatalog.Plan, error) { fake.retrievePlansMutex.Lock() ret, specificReturn := fake.retrievePlansReturnsOnCall[len(fake.retrievePlansArgsForCall)] fake.retrievePlansArgsForCall = append(fake.retrievePlansArgsForCall, struct { - arg1 *servicecatalog.FilterOptions + arg1 servicecatalog.RetrievePlanOptions }{arg1}) fake.recordInvocation("RetrievePlans", []interface{}{arg1}) fake.retrievePlansMutex.Unlock() @@ -2372,30 +2395,30 @@ func (fake *FakeSvcatClient) RetrievePlansCallCount() int { return len(fake.retrievePlansArgsForCall) } -func (fake *FakeSvcatClient) RetrievePlansArgsForCall(i int) *servicecatalog.FilterOptions { +func (fake *FakeSvcatClient) RetrievePlansArgsForCall(i int) servicecatalog.RetrievePlanOptions { fake.retrievePlansMutex.RLock() defer fake.retrievePlansMutex.RUnlock() return fake.retrievePlansArgsForCall[i].arg1 } -func (fake *FakeSvcatClient) RetrievePlansReturns(result1 []apiv1beta1.ClusterServicePlan, result2 error) { +func (fake *FakeSvcatClient) RetrievePlansReturns(result1 []servicecatalog.Plan, result2 error) { fake.RetrievePlansStub = nil fake.retrievePlansReturns = struct { - result1 []apiv1beta1.ClusterServicePlan + result1 []servicecatalog.Plan result2 error }{result1, result2} } -func (fake *FakeSvcatClient) RetrievePlansReturnsOnCall(i int, result1 []apiv1beta1.ClusterServicePlan, result2 error) { +func (fake *FakeSvcatClient) RetrievePlansReturnsOnCall(i int, result1 []servicecatalog.Plan, result2 error) { fake.RetrievePlansStub = nil if fake.retrievePlansReturnsOnCall == nil { fake.retrievePlansReturnsOnCall = make(map[int]struct { - result1 []apiv1beta1.ClusterServicePlan + result1 []servicecatalog.Plan result2 error }) } fake.retrievePlansReturnsOnCall[i] = struct { - result1 []apiv1beta1.ClusterServicePlan + result1 []servicecatalog.Plan result2 error }{result1, result2} } @@ -2502,57 +2525,6 @@ func (fake *FakeSvcatClient) RetrievePlanByIDReturnsOnCall(i int, result1 *apiv1 }{result1, result2} } -func (fake *FakeSvcatClient) RetrievePlansByClass(arg1 *apiv1beta1.ClusterServiceClass) ([]apiv1beta1.ClusterServicePlan, error) { - fake.retrievePlansByClassMutex.Lock() - ret, specificReturn := fake.retrievePlansByClassReturnsOnCall[len(fake.retrievePlansByClassArgsForCall)] - fake.retrievePlansByClassArgsForCall = append(fake.retrievePlansByClassArgsForCall, struct { - arg1 *apiv1beta1.ClusterServiceClass - }{arg1}) - fake.recordInvocation("RetrievePlansByClass", []interface{}{arg1}) - fake.retrievePlansByClassMutex.Unlock() - if fake.RetrievePlansByClassStub != nil { - return fake.RetrievePlansByClassStub(arg1) - } - if specificReturn { - return ret.result1, ret.result2 - } - return fake.retrievePlansByClassReturns.result1, fake.retrievePlansByClassReturns.result2 -} - -func (fake *FakeSvcatClient) RetrievePlansByClassCallCount() int { - fake.retrievePlansByClassMutex.RLock() - defer fake.retrievePlansByClassMutex.RUnlock() - return len(fake.retrievePlansByClassArgsForCall) -} - -func (fake *FakeSvcatClient) RetrievePlansByClassArgsForCall(i int) *apiv1beta1.ClusterServiceClass { - fake.retrievePlansByClassMutex.RLock() - defer fake.retrievePlansByClassMutex.RUnlock() - return fake.retrievePlansByClassArgsForCall[i].arg1 -} - -func (fake *FakeSvcatClient) RetrievePlansByClassReturns(result1 []apiv1beta1.ClusterServicePlan, result2 error) { - fake.RetrievePlansByClassStub = nil - fake.retrievePlansByClassReturns = struct { - result1 []apiv1beta1.ClusterServicePlan - result2 error - }{result1, result2} -} - -func (fake *FakeSvcatClient) RetrievePlansByClassReturnsOnCall(i int, result1 []apiv1beta1.ClusterServicePlan, result2 error) { - fake.RetrievePlansByClassStub = nil - if fake.retrievePlansByClassReturnsOnCall == nil { - fake.retrievePlansByClassReturnsOnCall = make(map[int]struct { - result1 []apiv1beta1.ClusterServicePlan - result2 error - }) - } - fake.retrievePlansByClassReturnsOnCall[i] = struct { - result1 []apiv1beta1.ClusterServicePlan - result2 error - }{result1, result2} -} - func (fake *FakeSvcatClient) RetrievePlanByClassAndPlanNames(arg1 string, arg2 string) (*apiv1beta1.ClusterServicePlan, error) { fake.retrievePlanByClassAndPlanNamesMutex.Lock() ret, specificReturn := fake.retrievePlanByClassAndPlanNamesReturnsOnCall[len(fake.retrievePlanByClassAndPlanNamesArgsForCall)] @@ -2766,6 +2738,8 @@ func (fake *FakeSvcatClient) Invocations() map[string][][]interface{} { defer fake.retrieveInstancesByPlanMutex.RUnlock() fake.touchInstanceMutex.RLock() defer fake.touchInstanceMutex.RUnlock() + fake.waitForInstanceToNotExistMutex.RLock() + defer fake.waitForInstanceToNotExistMutex.RUnlock() fake.waitForInstanceMutex.RLock() defer fake.waitForInstanceMutex.RUnlock() fake.retrievePlansMutex.RLock() @@ -2774,8 +2748,6 @@ func (fake *FakeSvcatClient) Invocations() map[string][][]interface{} { defer fake.retrievePlanByNameMutex.RUnlock() fake.retrievePlanByIDMutex.RLock() defer fake.retrievePlanByIDMutex.RUnlock() - fake.retrievePlansByClassMutex.RLock() - defer fake.retrievePlansByClassMutex.RUnlock() fake.retrievePlanByClassAndPlanNamesMutex.RLock() defer fake.retrievePlanByClassAndPlanNamesMutex.RUnlock() fake.retrieveSecretByBindingMutex.RLock()