Add List/Relist Catalog Restrictions by n3wscott · Pull Request #1773 · kubernetes-retired/service-catalog

n3wscott · 2018-02-28T19:36:20Z

In this PR I am wrapping the functionality of label selectors to allow us to apply the label selector query/matching syntax for service classes and service plans. This will allow us to create whitelists or blacklists for either class or plan to give lots of control to the cluster operator on which items end up in the catalog.

As a follow-up I will add the needed namespaced version of the added Cluster* methods to be able to do the same work on the new ns objects.

Seeing if we can use labels.Selector to filter the catalog.

Things to do:
[x] Services that are filtered out need to remove their plans.
[x] Services that have all their plans filtered out should be removed.
[x] Testing needs a fuller catalog to test real filtering.
[x] Add filtering behind a feature flag.
[x] API validation should validate the filter string.
[X] Confirm the storage format is ok for the filter.
[X] ---> Internal representation might need to be the processed labels.Selector.Requirements object?

fixes #1604

pmorie · 2018-03-05T18:30:56Z

pkg/apis/servicecatalog/types.go

+type ClusterServiceClassRequirements string
+
+type ServiceClassCatalogRestrictions struct {
+	ServicePlan  ClusterServicePlanRequirements


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

I have added some more comments to help you understand.

staebler · 2018-03-06T20:19:12Z

[] The Admission Controller should validate the filter string.

Is there any reason to do this in an Admission Controller instead of in API validation?

n3wscott · 2018-03-06T20:35:33Z

Is there any reason to do this in an Admission Controller instead of in API validation?

whichever. If you think that API validation is better then yeah lets do that. I have not touched that area of the code yet so I am ignorant of the correct way.

staebler

Looks to me like a reasonably approach to filtering.

staebler · 2018-03-06T20:03:02Z

pkg/apis/servicecatalog/v1beta1/types.go

+// Solution: restrictions := ServiceClassCatalogRestrictions{
+// 		ServiceClass: "externalName in (FooService, BarService)",
+// }
+// This is an example of a whitelist on service externalName.


This is a blacklist not a whitelist, right? It is explicit blocking FooService and BarService.

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.

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.
notin means the thing must not 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.

@staebler @eriknelson

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)"

@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.

@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.

@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.

Ok updated to hopefully clear this up.

this is getting kind of pedantic

:D

@eriknelson we are staying the same thing and we agree. I updated the comment order and added whitespace to clear this up.

ack, sounds good.

pmorie · 2018-03-12T16:28:10Z

pkg/apis/servicecatalog/v1beta1/types.go

+// }
+type ServiceClassCatalogRestrictions struct {
+	// ServiceClass represents a selector for plans, used to filter catalog re-lists.
+	ServiceClass ClusterServiceClassRequirements `json:"serviceClass,omitempty"`


This should probably be an array

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.

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

Array of strings is fine for me.

pmorie · 2018-03-12T16:31:37Z

pkg/apis/servicecatalog/v1beta1/types.go

+type ClusterServicePlanRequirements string
+
+// ServiceClassCatalogRestrictions contains the restrictions used to on catalog re-list.
+// Some examples of this object are as follows:


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.

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

pmorie · 2018-03-14T18:28:34Z

pkg/apis/servicecatalog/v1beta1/types.go

+
+	// CatalogRestrictions allows adding restrictions onto Class/Plans on relist.
+	// +optional
+	CatalogRestrictions *ServiceClassCatalogRestrictions `json:"catalogRestrictions,omitempty"`


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

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.

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

I will update.

…g and orphan mitigation) (kubernetes-retired#1803)" This reverts commit 4b5d159.

pmorie

Looking really good, just a few things

pmorie · 2018-04-04T04:44:36Z

pkg/controller/controller.go

 	return serviceClasses, servicePlans, nil
 }

+func convertAndFilterCatalog(in *osb.CatalogResponse, restrictions *v1beta1.CatalogRestrictions) ([]*v1beta1.ClusterServiceClass, []*v1beta1.ClusterServicePlan, error) {


In the future, we could make this just work with CommonService(Class|Plan)Spec, and have wrappers that built the right types for cluster and ns-scoped. WDYT?

this currently uses the parent service class name in part of the plan query to aid in the filtering. We are going to need a general strategy on how to deal with this all over the place in the codebase.

also why I did not just copy/paste yet, I wanted to look into seeing if I can make it work as you asked.

pmorie · 2018-04-04T04:46:26Z

pkg/features/features.go

 	// alpha: v0.1.6
 	PodPreset utilfeature.Feature = "PodPreset"

+	// CatalogRestrictions controls whether the ClusterServiceBroker will use


Can we change the verbiage here around "control which services and plans have k8s resources created for them" ?

updated comment

pmorie · 2018-04-04T04:48:58Z

pkg/filter/predicate.go

+)
+
+// Want to filter the allowed labels.
+// want to use only


Clarify comment

I did not mean to leave that in... removed.

pmorie · 2018-04-04T04:49:14Z

pkg/filter/predicate.go

+//	NotIn        Operator = "notin"
+//)
+
+// Predicate is used to test if this rule accepts the properties given.


pmorie

Just a couple nits on godoc stuff, but would you also add a simple integration test?

pmorie · 2018-04-04T21:18:46Z

pkg/apis/servicecatalog/v1beta1/types.go

 	// +optional
 	RelistRequests int64 `json:"relistRequests"`
+
+	// CatalogRestrictions allows adding restrictions onto Class/Plans on relist.


bump - can we change the wording here?

pmorie · 2018-04-04T21:19:08Z

pkg/apis/servicecatalog/types.go

 	// can be manually incremented by a user to manually trigger a relist.
 	RelistRequests int64
+
+	// CatalogRestrictions is a set of restrictions on which of a broker's services


Need to put the same godoc as v1beta1 here.

ok updated. too much codez... (will push soon)

pmorie · 2018-04-05T20:32:31Z

pkg/apis/servicecatalog/validation/broker.go

+						spec.CatalogRestrictions.ServiceClass, err.Error()))
+			}
+		}
+	}


One thing I just thought of: I think we should not allow you to set these fields if the feature gate isn't enabled.

meh, I am not sure about that. I could see a use for setting this up then turning on or off the feature.

It's conventional not to allow fields for a feature that isn't enabled.

If you accept the fields, users will expect them to work.

the flag has been removed.

pmorie · 2018-04-17T15:47:07Z

charts/catalog/values.yaml

 originatingIdentityEnabled: false
 # Whether the AsyncBindingOperations alpha feature should be enabled
 asyncBindingOperationsEnabled: false
+# Whether the CatalogRestrictions alpha feature should be enabled


Do we still need this flag, since the feature gate has been removed?

killed. thanks!

pmorie · 2018-04-18T21:01:45Z

Looks like CI failure was a flake - I bounced it.

…bels

n3wscott · 2018-04-20T17:19:02Z

/retest

n3wscott · 2018-04-20T17:37:55Z

Dear Travis,

You suck.

Sincerely,
-Scott

n3wscott · 2018-04-20T22:17:03Z

Sorted it out, I had old compiled generators locally giving me different things than master expected.

PTAL.

jeremyrickard · 2018-04-23T16:07:43Z

This is cool! I'm excited to see this make it in! LGTM.

carolynvs

LGTM

carolynvs · 2018-04-23T16:14:30Z

pkg/apis/servicecatalog/validation/broker_test.go

 		if len(errs) != 0 && tc.valid {
 			t.Errorf("%v: unexpected error: %v", tc.name, errs)
-			continue
+			return


Scott Nichols added 5 commits February 28, 2018 07:51

Pause, going to try to use the label selector again.

c0de801

A working example using label selectors backing code for class.

4060ca1

Working simple example of classes and plans filtering.

3eac4e2

Cleaning up the types.

c1fd237

Add comments for current changes.

9dd7bc2

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 28, 2018

Scott Nichols added 2 commits February 28, 2018 13:23

Adding feature flag for Catalog Restrictions.

7ecd694

A better way to filter the broker response for while/blacklists.

72f7539

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 5, 2018

pmorie reviewed Mar 5, 2018

View reviewed changes

Adding documentation for how to use the label selectors.

8cb4639

staebler reviewed Mar 6, 2018

View reviewed changes

Adding whitespace and reordering comments to avoid confusion and delays.

815dba4

pmorie reviewed Mar 12, 2018

View reviewed changes

Changing to use list, adding tests for helper.

4bc662a

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 12, 2018

pmorie reviewed Mar 14, 2018

View reviewed changes

Scott Nichols added 8 commits March 14, 2018 14:38

Call the catalog restriction type ServiceCatalogRestrictions

e6c99d8

Call the catalog restriction type ClusterServiceCatalogRestrictions

bd96336

Document the consts for filter.

25cf545

document comments are strict.

6cdeb3b

No comma. I blame Doug.

48f4444

Revert "Pass correct plan ID in deprovision request (for both deletin…

84f52a9

…g and orphan mitigation) (kubernetes-retired#1803)" This reverts commit 4b5d159.

Merge remote-tracking branch 'upstream/master'

94c89b0

Merge remote-tracking branch 'upstream/master'

4b7bc89

pmorie reviewed Apr 4, 2018

View reviewed changes

upate comments.

114dcb1

pmorie reviewed Apr 4, 2018

View reviewed changes

adding integration test, super simple.

1b105b4

pmorie reviewed Apr 5, 2018

View reviewed changes

n3wscott and others added 4 commits April 16, 2018 10:34

Merge branch 'master' into whitelist_from_labels

ce24688

fmt'ed the features.

5e9e946

yolo, no flag.

b7276aa

Rework integration test to not assume the flag.

60650e8

pmorie reviewed Apr 17, 2018

View reviewed changes

Remove more cruft from the flag.

e3aed41

carolynvs mentioned this pull request Apr 18, 2018

ACL for controlling access to ServiceClasses #250

Closed

Scott Nichols added 2 commits April 19, 2018 10:05

Merge branch 'master' into whitelist_from_labels

63f5691

godocs match 100% now.

14c39d1

pmorie added the LGTM1 label Apr 19, 2018

Scott Nichols added 2 commits April 20, 2018 09:14

Generator generated.

6cbedc4

Merge remote-tracking branch 'upstream/master' into whitelist_from_la…

0b99c5d

…bels

Scott Nichols added 3 commits April 20, 2018 11:00

ran deps ensure, got this

a75f8ee

Merge with master.

362278f

Revert sort of... back to k8s 1.10 gen.

dfdabaa

carolynvs approved these changes Apr 23, 2018

View reviewed changes

carolynvs merged commit 5c43dec into kubernetes-retired:master Apr 23, 2018

maleck13 mentioned this pull request Jun 21, 2018

Proposal: ns-scoped resources for brokers, services, and plans #1826

Closed

carolynvs mentioned this pull request Jun 26, 2018

Document catalog relist restrictions #2156

Closed

krancour mentioned this pull request Aug 2, 2018

Question: Is it possible to limit access to certain resources? Azure/open-service-broker-azure#539

Closed

Conversation

n3wscott commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staebler commented Mar 6, 2018

Uh oh!

n3wscott commented Mar 6, 2018

Uh oh!

staebler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eriknelson Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staebler Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmorie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Feb 28, 2018 •

edited

Loading

eriknelson Mar 9, 2018 •

edited

Loading

staebler Mar 9, 2018 •

edited

Loading

n3wscott Apr 4, 2018 •

edited

Loading