Skip to content
This repository was archived by the owner on May 6, 2022. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
c0de801
Pause, going to try to use the label selector again.
Feb 28, 2018
4060ca1
A working example using label selectors backing code for class.
Feb 28, 2018
3eac4e2
Working simple example of classes and plans filtering.
Feb 28, 2018
c1fd237
Cleaning up the types.
Feb 28, 2018
9dd7bc2
Add comments for current changes.
Feb 28, 2018
7ecd694
Adding feature flag for Catalog Restrictions.
Feb 28, 2018
72f7539
A better way to filter the broker response for while/blacklists.
Mar 5, 2018
8cb4639
Adding documentation for how to use the label selectors.
Mar 6, 2018
815dba4
Adding whitespace and reordering comments to avoid confusion and delays.
Mar 9, 2018
4bc662a
Changing to use list, adding tests for helper.
Mar 12, 2018
e6c99d8
Call the catalog restriction type ServiceCatalogRestrictions
Mar 14, 2018
bd96336
Call the catalog restriction type ClusterServiceCatalogRestrictions
Mar 14, 2018
25cf545
Document the consts for filter.
Mar 14, 2018
6cdeb3b
document comments are strict.
Mar 15, 2018
48f4444
No comma. I blame Doug.
Mar 15, 2018
84f52a9
Revert "Pass correct plan ID in deprovision request (for both deletin…
Mar 16, 2018
94c89b0
Merge remote-tracking branch 'upstream/master'
Mar 16, 2018
4b7bc89
Merge remote-tracking branch 'upstream/master'
Mar 23, 2018
2e08402
Merge with master.
Mar 23, 2018
f959fd2
Changing types to just be lists of strings. moving some conversion to…
Mar 23, 2018
1590bd8
Getting Can't handle <nil> from the fuzzer.
Mar 23, 2018
ad38b27
just use []string
Mar 26, 2018
ebff045
Merge with master.
Apr 2, 2018
2fe49b8
Fixing up feature gate.
Apr 2, 2018
5785b69
Make the flag generic in the unit test.
Apr 3, 2018
0ce636c
Fix ConvertToSelector comment.
Apr 3, 2018
f0a23ad
Moving convert functions out of conversion.go.
Apr 3, 2018
0eb1529
make it clear we will use for list/relist.
Apr 3, 2018
9dcfa79
Fix comment for NewPredicate
Apr 3, 2018
76b5450
Fix CatalogRestrictions comment.
Apr 3, 2018
9039cf0
fix openapi.
Apr 3, 2018
5de5989
fix ConvertCluster function comments
Apr 3, 2018
d33b2a0
Update comment in type.
Apr 4, 2018
4285b72
Remove the old test no longer needed.
Apr 4, 2018
7144232
move filter test code to the new file location.
Apr 4, 2018
b6507f8
Forgot the C header.
Apr 4, 2018
71a7d85
copy godoc for unversioned type.
Apr 4, 2018
d6f8448
Reword commend from feedback.
Apr 4, 2018
88022e6
Open api changed after generation. update.
Apr 4, 2018
114dcb1
upate comments.
Apr 4, 2018
1b105b4
adding integration test, super simple.
Apr 5, 2018
ce24688
Merge branch 'master' into whitelist_from_labels
n3wscott Apr 16, 2018
5e9e946
fmt'ed the features.
Apr 16, 2018
b7276aa
yolo, no flag.
Apr 16, 2018
60650e8
Rework integration test to not assume the flag.
Apr 16, 2018
e3aed41
Remove more cruft from the flag.
Apr 17, 2018
63f5691
Merge branch 'master' into whitelist_from_labels
Apr 19, 2018
14c39d1
godocs match 100% now.
Apr 19, 2018
6cbedc4
Generator generated.
Apr 20, 2018
0b99c5d
Merge remote-tracking branch 'upstream/master' into whitelist_from_la…
Apr 20, 2018
a75f8ee
ran deps ensure, got this
Apr 20, 2018
362278f
Merge with master.
Apr 20, 2018
dfdabaa
Revert sort of... back to k8s 1.10 gen.
Apr 20, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions pkg/apis/servicecatalog/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,17 @@ type ClusterServiceBrokerSpec struct {
// RelistRequests is a strictly increasing, non-negative integer counter that
// can be manually incremented by a user to manually trigger a relist.
RelistRequests int64

// CatalogRestrictions allows adding restrictions onto Class/Plans on relist.
CatalogRestrictions *ServiceClassCatalogRestrictions
}

type ClusterServicePlanRequirements string
type ClusterServiceClassRequirements string

type ServiceClassCatalogRestrictions struct {
ServicePlan ClusterServicePlanRequirements
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me how I would say:

  • From this broker, create resources only for service X
  • From this broker, create resources for all services but Y

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added some more comments to help you understand.

ServiceClass ClusterServiceClassRequirements
}

// ServiceBrokerRelistBehavior represents a type of broker relist behavior.
Expand Down
63 changes: 63 additions & 0 deletions pkg/apis/servicecatalog/v1beta1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,69 @@ type ClusterServiceBrokerSpec struct {
// can be manually incremented by a user to manually trigger a relist.
// +optional
RelistRequests int64 `json:"relistRequests"`

// CatalogRestrictions allows adding restrictions onto Class/Plans on relist.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the wording here a little to:

// CatalogRestrictions is a set of restrictions on which of a broker's services and plans have resources created for them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump - can we change the wording here?

// +optional
CatalogRestrictions *ServiceClassCatalogRestrictions `json:"catalogRestrictions,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just make these arrays instead of pointer to typedef?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ServiceClassCatalogRestrictions is a struct that has arrays for class and plan. So this is a pointer to a struct, not a pointer to a typedef'ed array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now looking at it, I did not mean for this to contain "Class", this type is suppose to be ServiceCatalogRestrictions

I will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

}

// *Requirements type strings have a special format similar to Label Selectors,
// except the catalog supports only a very specific property set per type as
// documented below.
// The predicate format is expected to be `<property><conditional><requirement>`
// Check the *Requirements type definition for which <property> strings will be allowed.
// <conditional> is allowed to be one of the following: ==, !=, in, notin
// <requirement> will be a string value if `==` or `!=` are used.
// <requirement> will be a set of string values if `in` or `notin` are used.
// Multiple predicates are allowed to be chained with a comma (,)

// ClusterServiceClassRequirements represents a list of selectors for classes used to filter.
// Requirements are AND'ed together from the list.
type ClusterServiceClassRequirements []ClusterServiceClassRequirement

// ClusterServiceClassRequirement represents a selector for classes used to filter.
// Allowed property names:
// name - the value set to ClusterServiceClass.Name
// externalName - the value set to ClusterServiceClass.Spec.ExternalName
type ClusterServiceClassRequirement string

// ClusterServicePlanRequirements represents a list of selectors for plans used to filter.
// Requirements are AND'ed together from the list.
type ClusterServicePlanRequirements []ClusterServicePlanRequirement

// ClusterServicePlanRequirement represents a selector for plans used to filter.
// Allowed property names:
// name - the value set to ClusterServiceClass.Name
// externalName - the value set to ClusterServiceClass.Spec.ExternalName
type ClusterServicePlanRequirement string

// ServiceClassCatalogRestrictions contains the restrictions used to on catalog re-list.
// Some examples of this object are as follows:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to be more clear that the restriction expressions are in terms of the k8s API resources. It would also be more correct to say spec.externalName than externalName.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good call. I think we should make that clear. I had this with field selectors and it got lost when I was attempting label selectors. I can add it back. Then free = true makes a lot more sense when it is next to spec.externalName

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

//
// This is an example of a whitelist on service externalName.
// Goal: Only list Services with the externalName of FooService and BarService,
// Solution: restrictions := ServiceClassCatalogRestrictions{
// ServiceClass: "externalName in (FooService, BarService)",
// }
//
// This is an example of a blacklist on service externalName.
// Goal: Allow all services except the ones with the externalName of FooService and BarService,
// Solution: restrictions := ServiceClassCatalogRestrictions{
// ServiceClass: "externalName notin (FooService, BarService)",
// }
//
// This whitelists plans called "Demo", and blacklists (but only a single element in
// the list) a service and a plan.
// Goal: Allow all plans with the externalName demo, but not AABBCC, and not a specific service by name,
// Solution: restrictions := ServiceClassCatalogRestrictions{
// ServiceClass: "name!=AABBB-CCDD-EEGG-HIJK",
// ServicePlan: "externalName in (Demo),name!=AABBCC",
// }
type ServiceClassCatalogRestrictions struct {
// ServiceClass represents a selector for plans, used to filter catalog re-lists.
ServiceClass ClusterServiceClassRequirements `json:"serviceClass,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be an array

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to be able to translate these strings between API versions, so you might consider whether it would be easier to have a structured definition of a restriction than a string that has to be parsed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could do an array, makes it less easy to fat finger a long string.

As for the structured definition, k8s does both. The object that this turns into is non-trivial because it is pretty powerful and then we either leak the fact we are using label selectors or we have to duplicate the label selector code.

I vote array of strings for now, if that pleases you

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array of strings is fine for me.

// ServicePlan represents a selector for classes, used to filter catalog re-lists.
ServicePlan ClusterServicePlanRequirements `json:"servicePlan,omitempty"`
}

// ServiceBrokerRelistBehavior represents a type of broker relist behavior.
Expand Down
90 changes: 88 additions & 2 deletions pkg/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ import (
informers "github.com/kubernetes-incubator/service-catalog/pkg/client/informers_generated/externalversions/servicecatalog/v1beta1"
listers "github.com/kubernetes-incubator/service-catalog/pkg/client/listers_generated/servicecatalog/v1beta1"
scfeatures "github.com/kubernetes-incubator/service-catalog/pkg/features"
pretty "github.com/kubernetes-incubator/service-catalog/pkg/pretty"
"github.com/kubernetes-incubator/service-catalog/pkg/filter"
"github.com/kubernetes-incubator/service-catalog/pkg/pretty"
)

const (
Expand Down Expand Up @@ -478,6 +479,8 @@ func getBearerConfig(secret *corev1.Secret) (*osb.BearerConfig, error) {
// convertCatalog converts a service broker catalog into an array of
// ClusterServiceClasses and an array of ClusterServicePlans. The ClusterServiceClasses and
// ClusterServicePlans returned by this method are named in K8S with the OSB ID.
// *deprecated* use convertAndFilterCatalog
// TODO: delete convertCatalog when features.CatalogRestrictions flag is removed.
func convertCatalog(in *osb.CatalogResponse) ([]*v1beta1.ClusterServiceClass, []*v1beta1.ClusterServicePlan, error) {
serviceClasses := make([]*v1beta1.ClusterServiceClass, len(in.Services))
servicePlans := []*v1beta1.ClusterServicePlan{}
Expand Down Expand Up @@ -520,6 +523,90 @@ func convertCatalog(in *osb.CatalogResponse) ([]*v1beta1.ClusterServiceClass, []
return serviceClasses, servicePlans, nil
}

func convertAndFilterCatalog(in *osb.CatalogResponse, restrictions *v1beta1.ServiceClassCatalogRestrictions) ([]*v1beta1.ClusterServiceClass, []*v1beta1.ClusterServicePlan, error) {
predicate, err := filter.CreatePredicateForServiceClassesFromRestrictions(restrictions)

if err != nil {
return nil, nil, err
}
serviceClasses := []*v1beta1.ClusterServiceClass(nil)
servicePlans := []*v1beta1.ClusterServicePlan(nil)
for _, svc := range in.Services {
serviceClass := &v1beta1.ClusterServiceClass{
Spec: v1beta1.ClusterServiceClassSpec{
Bindable: svc.Bindable,
PlanUpdatable: svc.PlanUpdatable != nil && *svc.PlanUpdatable,
ExternalID: svc.ID,
ExternalName: svc.Name,
Tags: svc.Tags,
Description: svc.Description,
Requires: svc.Requires,
},
}

if utilfeature.DefaultFeatureGate.Enabled(scfeatures.AsyncBindingOperations) {
serviceClass.Spec.BindingRetrievable = svc.BindingRetrievable
}

if svc.Metadata != nil {
metadata, err := json.Marshal(svc.Metadata)
if err != nil {
err = fmt.Errorf("Failed to marshal metadata\n%+v\n %v", svc.Metadata, err)
glog.Error(err)
return nil, nil, err
}
serviceClass.Spec.ExternalMetadata = &runtime.RawExtension{Raw: metadata}
}
serviceClass.SetName(svc.ID)

// If this service class passes the predicate, process the plans for the class.
if fields := filter.ConvertServiceClassToProperties(serviceClass); predicate.Accepts(fields) {
// set up the plans using the ClusterServiceClass Name
plans, err := convertClusterServicePlans(svc.Plans, serviceClass.Name)
if err != nil {
return nil, nil, err
}

acceptedPlans, _, err := filterServicePlans(restrictions, plans)
if err != nil {
return nil, nil, err
}

// If there are accepted plans, then append the class and all of the accepted plans to the master list.
if len(acceptedPlans) > 0 {
serviceClasses = append(serviceClasses, serviceClass)
servicePlans = append(servicePlans, acceptedPlans...)
}
}
}
return serviceClasses, servicePlans, nil
}

func filterServicePlans(restrictions *v1beta1.ServiceClassCatalogRestrictions, servicePlans []*v1beta1.ClusterServicePlan) ([]*v1beta1.ClusterServicePlan, []*v1beta1.ClusterServicePlan, error) {
predicate, err := filter.CreatePredicateForServicePlansFromRestrictions(restrictions)
if err != nil {
return nil, nil, err
}

// If the predicate is empty, all plans will pass. No need to run through the list.
if predicate.Empty() {
return servicePlans, []*v1beta1.ClusterServicePlan(nil), nil
}

accepted := []*v1beta1.ClusterServicePlan(nil)
rejected := []*v1beta1.ClusterServicePlan(nil)
for _, sp := range servicePlans {
fields := filter.ConvertServicePlanToProperties(sp)
if predicate.Accepts(fields) {
accepted = append(accepted, sp)
} else {
rejected = append(rejected, sp)
}
}

return accepted, rejected, nil
}

func convertClusterServicePlans(plans []osb.Plan, serviceClassID string) ([]*v1beta1.ClusterServicePlan, error) {
if 0 == len(plans) {
return nil, fmt.Errorf("ClusterServiceClass (K8S: %q) must have at least one plan", serviceClassID)
Expand Down Expand Up @@ -585,7 +672,6 @@ func convertClusterServicePlans(plans []osb.Plan, serviceClassID string) ([]*v1b
}
}
}

}
return servicePlans, nil
}
Expand Down
32 changes: 22 additions & 10 deletions pkg/controller/controller_broker.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,18 @@ import (

"github.com/golang/glog"

"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1"
"github.com/kubernetes-incubator/service-catalog/pkg/metrics"
"github.com/kubernetes-incubator/service-catalog/pkg/pretty"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/util/sets"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/client-go/tools/cache"

"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/pkg/metrics"
"github.com/kubernetes-incubator/service-catalog/pkg/pretty"
)

// the Message strings have a terminating period and space so they can
Expand Down Expand Up @@ -255,16 +258,25 @@ func (c *controller) reconcileClusterServiceBroker(broker *v1beta1.ClusterServic

// convert the broker's catalog payload into our API objects
glog.V(4).Info(pcb.Message("Converting catalog response into service-catalog API"))
payloadServiceClasses, payloadServicePlans, err := convertCatalog(brokerCatalog)
if err != nil {
s := fmt.Sprintf("Error converting catalog payload for broker %q to service-catalog API: %s", broker.Name, err)
glog.Warning(pcb.Message(s))
c.recorder.Eventf(broker, corev1.EventTypeWarning, errorSyncingCatalogReason, s)
if err := c.updateClusterServiceBrokerCondition(broker, v1beta1.ServiceBrokerConditionReady, v1beta1.ConditionFalse, errorSyncingCatalogReason, errorSyncingCatalogMessage+s); err != nil {

var payloadServiceClasses []*v1beta1.ClusterServiceClass
var payloadServicePlans []*v1beta1.ClusterServicePlan
if utilfeature.DefaultFeatureGate.Enabled(scfeatures.CatalogRestrictions) {
if payloadServiceClasses, payloadServicePlans, err = convertAndFilterCatalog(brokerCatalog, broker.Spec.CatalogRestrictions); err != nil {

}
} else {
if payloadServiceClasses, payloadServicePlans, err = convertCatalog(brokerCatalog); err != nil {
s := fmt.Sprintf("Error converting catalog payload for broker %q to service-catalog API: %s", broker.Name, err)
glog.Warning(pcb.Message(s))
c.recorder.Eventf(broker, corev1.EventTypeWarning, errorSyncingCatalogReason, s)
if err := c.updateClusterServiceBrokerCondition(broker, v1beta1.ServiceBrokerConditionReady, v1beta1.ConditionFalse, errorSyncingCatalogReason, errorSyncingCatalogMessage+s); err != nil {
return err
}
return err
}
return err
}

glog.V(5).Info(pcb.Message("Successfully converted catalog payload from to service-catalog API"))

// brokers must return at least one service; enforce this constraint
Expand Down
Loading