-
Notifications
You must be signed in to change notification settings - Fork 373
Add List/Relist Catalog Restrictions #1773
Changes from 23 commits
c0de801
4060ca1
3eac4e2
c1fd237
9dd7bc2
7ecd694
72f7539
8cb4639
815dba4
4bc662a
e6c99d8
bd96336
25cf545
6cdeb3b
48f4444
84f52a9
94c89b0
4b7bc89
2e08402
f959fd2
1590bd8
ad38b27
ebff045
2fe49b8
5785b69
0ce636c
f0a23ad
0eb1529
9dcfa79
76b5450
9039cf0
5de5989
d33b2a0
4285b72
7144232
b6507f8
71a7d85
d6f8448
88022e6
114dcb1
1b105b4
ce24688
5e9e946
b7276aa
60650e8
e3aed41
63f5691
14c39d1
6cbedc4
0b99c5d
a75f8ee
362278f
dfdabaa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,12 @@ limitations under the License. | |
|
|
||
| package v1beta1 | ||
|
|
||
| import "fmt" | ||
| import ( | ||
| "fmt" | ||
|
|
||
| "github.com/kubernetes-incubator/service-catalog/pkg/filter" | ||
| "k8s.io/apimachinery/pkg/labels" | ||
| ) | ||
|
|
||
| // These functions are used for field selectors. They are only needed if | ||
| // field selection is made available for types, hence we only have them for | ||
|
|
@@ -88,3 +93,32 @@ func ServiceInstanceFieldLabelConversionFunc(label, value string) (string, strin | |
| return "", "", fmt.Errorf("field label not supported: %s", label) | ||
| } | ||
| } | ||
|
|
||
| // ConvertClusterServiceClassToProperties takes a Service Class and pulls out the | ||
| // properties we support for filtering, converting them into a map in the | ||
| // expected format. | ||
| func ConvertClusterServiceClassToProperties(serviceClass *ClusterServiceClass) filter.Properties { | ||
|
||
| if serviceClass == nil { | ||
| return labels.Set{} | ||
| } | ||
| return labels.Set{ | ||
| FilterName: serviceClass.Name, | ||
| FilterSpecExternalName: serviceClass.Spec.ExternalName, | ||
| FilterSpecExternalID: serviceClass.Spec.ExternalID, | ||
| } | ||
| } | ||
|
|
||
| // ConvertServicePlanToProperties takes a Service Plan and pulls out the | ||
| // properties we support for filtering, converting them into a map in the | ||
| // expected format. | ||
| func ConvertClusterServicePlanToProperties(servicePlan *ClusterServicePlan) filter.Properties { | ||
| if servicePlan == nil { | ||
| return labels.Set{} | ||
| } | ||
| return labels.Set{ | ||
| FilterName: servicePlan.Name, | ||
| FilterSpecExternalName: servicePlan.Spec.ExternalName, | ||
| FilterSpecExternalID: servicePlan.Spec.ExternalID, | ||
| FilterSpecClusterServiceClassName: servicePlan.Spec.ClusterServiceClassRef.Name, | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,8 +17,13 @@ limitations under the License. | |
| package v1beta1 | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
|
|
||
| "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog" | ||
| ) | ||
|
|
||
| type conversionFunc func(string, string) (string, string, error) | ||
|
|
@@ -235,3 +240,129 @@ func runTestCases(t *testing.T, cases []testcase, testFuncName string, testFunc | |
| } | ||
| } | ||
| } | ||
|
|
||
| func TestConvert_v1beta1_CatalogRestrictions_To_servicecatalog_CatalogRestrictions_AndBack(t *testing.T) { | ||
|
||
| originalIn := CatalogRestrictions{ | ||
| ServiceClass: []string{"name not in (foo)"}, | ||
| ServicePlan: []string{"name == bar", "externalName==baz"}, | ||
| } | ||
| var originalOut servicecatalog.CatalogRestrictions | ||
|
|
||
| Convert_v1beta1_CatalogRestrictions_To_servicecatalog_CatalogRestrictions(&originalIn, &originalOut, nil) | ||
|
|
||
| var convertedOut CatalogRestrictions | ||
|
|
||
| Convert_servicecatalog_CatalogRestrictions_To_v1beta1_CatalogRestrictions(&originalOut, &convertedOut, nil) | ||
|
|
||
| // original in and converted out should match, but string formatting and order modifications are allowed. | ||
|
|
||
| for _, r := range convertedOut.ServiceClass { | ||
| if !findInRequirementsIgnoreSpaces(r, originalIn.ServiceClass) { | ||
| t.Fail() | ||
| } | ||
| } | ||
|
|
||
| for _, r := range convertedOut.ServicePlan { | ||
| if !findInRequirementsIgnoreSpaces(r, originalIn.ServicePlan) { | ||
| t.Fail() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func findInRequirementsIgnoreSpaces(requirement string, requirements []string) bool { | ||
| find := strings.Replace(requirement, " ", "", -1) | ||
| for _, r := range requirements { | ||
| found := strings.Replace(r, " ", "", -1) | ||
| if find == found { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func TestConvertClusterServiceClassToProperties(t *testing.T) { | ||
| cases := []struct { | ||
| name string | ||
| sc *ClusterServiceClass | ||
| json string | ||
| }{ | ||
| { | ||
| name: "nil object", | ||
| json: "{}", | ||
| }, | ||
| { | ||
| name: "normal object", | ||
| sc: &ClusterServiceClass{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "service-class"}, | ||
| Spec: ClusterServiceClassSpec{ | ||
| CommonServiceClassSpec: CommonServiceClassSpec{ | ||
| ExternalName: "external-class-name", | ||
| ExternalID: "external-id", | ||
| }, | ||
| }, | ||
| }, | ||
| json: "{\"name\":\"service-class\",\"spec.externalID\":\"external-id\",\"spec.externalName\":\"external-class-name\"}", | ||
|
||
| }, | ||
| } | ||
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| p := ConvertClusterServiceClassToProperties(tc.sc) | ||
| if p == nil { | ||
| t.Fatalf("Failed to create Properties object from %+v", tc.sc) | ||
| } | ||
| b, err := json.Marshal(p) | ||
| if err != nil { | ||
| t.Fatalf("Unexpected error with json marchal, %v", err) | ||
| } | ||
| js := string(b) | ||
| if js != tc.json { | ||
| t.Fatalf("Failed to create expected Properties object,\n\texpected: \t%q,\n \tgot: \t\t%q", tc.json, js) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestConvertClusterServicePlanToProperties(t *testing.T) { | ||
| cases := []struct { | ||
| name string | ||
| sp *ClusterServicePlan | ||
| json string | ||
| }{ | ||
| { | ||
| name: "nil object", | ||
| json: "{}", | ||
| }, | ||
| { | ||
| name: "normal object", | ||
| sp: &ClusterServicePlan{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "service-plan"}, | ||
| Spec: ClusterServicePlanSpec{ | ||
| CommonServicePlanSpec: CommonServicePlanSpec{ | ||
| ExternalName: "external-plan-name", | ||
| ExternalID: "external-id", | ||
| }, | ||
| ClusterServiceClassRef: ClusterObjectReference{ | ||
| Name: "cluster-service-class-name", | ||
| }, | ||
| }, | ||
| }, | ||
| json: "{\"name\":\"service-plan\",\"spec.clusterServiceClass.name\":\"cluster-service-class-name\",\"spec.externalID\":\"external-id\",\"spec.externalName\":\"external-plan-name\"}", | ||
| }, | ||
| } | ||
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| p := ConvertClusterServicePlanToProperties(tc.sp) | ||
| if p == nil { | ||
| t.Fatalf("Failed to create Properties object from %+v", tc.sp) | ||
| } | ||
| b, err := json.Marshal(p) | ||
| if err != nil { | ||
| t.Fatalf("Unexpected error with json marchal, %v", err) | ||
| } | ||
| js := string(b) | ||
| if js != tc.json { | ||
| t.Fatalf("Failed to create expected Properties object,\n\texpected: \t%q,\n \tgot: \t\t%q", tc.json, js) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -121,6 +121,57 @@ type CommonServiceBrokerSpec 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. | ||
|
||
| // +optional | ||
| CatalogRestrictions *CatalogRestrictions `json:"catalogRestrictions,omitempty"` | ||
| } | ||
|
|
||
| // ServiceCatalogRestrictions contains the restrictions used to on catalog re-list. | ||
| // Some examples of this object are as follows: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| // | ||
| // This is an example of a whitelist on service externalName. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a blacklist not a whitelist, right? It is explicit blocking FooService and BarService.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this could be interpreted in a couple ways: either "Allow all services that are NOT Foo/BarService", which would be an allowance rule and a whitelist. However, ultimately what you're saying here is "Block FooService and BarService", which sounds much more like a blacklist to me.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment may be confusing, I am staying the previous solution is the whitelist. Above example is whitelist: "externalName in (FooService, BarService)" Below example is a blacklist: "externalName notin (FooService, BarService)", in means the thing must be contained in the following list to pass the predicate. I hope it is just confusion around the Goal, Solution, and English Description format. I can fix that.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or you folks are getting caught up on the restrictions name. In that case, I am open to other names that are allowed to house both white and black lists together, example: "externalName in (Demo),name not in (AABBCC)"
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @n3wscott this is getting kind of pedantic, but I interpret a blacklist as "allow everything but these", compared to a whitelist that is "Allow nothing, but pass these". The distinction being a default allowing everything, or nothing. Given that definition, your examples are exactly correct, and the comment is describing a blacklist.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @n3wscott OK. The confusion for me was the ordering of the comments. I read it to apply to the following rather than the preceding. An empty line between each example would have made it less confusing for me.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @n3wscott And, for what it's worth, I had no problem understanding the examples. I don't think that there is a need to change the restrictions name.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok updated to hopefully clear this up.
:D @eriknelson we are staying the same thing and we agree. I updated the comment order and added whitespace to clear this up.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ack, sounds good. |
||
| // Goal: Only list Services with the externalName of FooService and BarService, | ||
| // Solution: restrictions := ServiceCatalogRestrictions{ | ||
| // 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 := ServiceCatalogRestrictions{ | ||
| // 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 := ServiceCatalogRestrictions{ | ||
| // ServiceClass: ["name!=AABBB-CCDD-EEGG-HIJK"] | ||
| // ServicePlan: ["externalName in (Demo)", "name!=AABBCC"] | ||
| // } | ||
| // | ||
| // CatalogRestrictions strings have a special format similar to Label Selectors, | ||
| // except the catalog supports only a very specific property set. | ||
| // | ||
| // 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 (,) | ||
| // | ||
| // ServiceClass allowed property names: | ||
| // name - the value set to [Cluster]ServiceClass.Name | ||
| // externalName - the value set to [Cluster]ServiceClass.Spec.ExternalName | ||
| // | ||
| // ServicePlan allowed property names: | ||
| // name - the value set to [Cluster]ServiceClass.Name | ||
| // externalName - the value set to [Cluster]ServiceClass.Spec.ExternalName | ||
| type CatalogRestrictions struct { | ||
| // ServiceClass represents a selector for plans, used to filter catalog re-lists. | ||
| ServiceClass []string `json:"serviceClass,omitempty"` | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note I removed custom typed strings because it was not adding value to the code. |
||
| // ServicePlan represents a selector for classes, used to filter catalog re-lists. | ||
| ServicePlan []string `json:"servicePlan,omitempty"` | ||
| } | ||
|
|
||
| // ClusterServiceBrokerSpec represents a description of a Broker. | ||
|
|
@@ -1241,6 +1292,18 @@ type ClusterObjectReference struct { | |
| Name string `json:"name,omitempty"` | ||
| } | ||
|
|
||
| // Filter path for Properties | ||
| const ( | ||
| // Name field. | ||
| FilterName = "name" | ||
| // SpecExternalName is the external name of the object. | ||
| FilterSpecExternalName = "spec.externalName" | ||
| // SpecExternalID is the external id of the object. | ||
| FilterSpecExternalID = "spec.externalID" | ||
| // SpecClusterServiceClassName is only used for plans, the parent service class name. | ||
| FilterSpecClusterServiceClassName = "spec.clusterServiceClass.name" | ||
| ) | ||
|
|
||
| // SecretTransform is a single transformation that is applied to the | ||
| // credentials returned from the broker before they are inserted into | ||
| // the Secret associated with the ServiceBinding. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder: copy godoc in from v1beta1 here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.