Proposal: ns-scoped resources for brokers, services, and plans by pmorie · Pull Request #1826 · kubernetes-retired/service-catalog

pmorie · 2018-03-12T19:51:03Z

Proposal to resolve #1200

carolynvs

LGTM

As part of the Prosopal for Namespaced Resources (kubernetes-retired#1826), this extracts the common elements from the ClusterServicePlanSpec into a SharedServicePlanSpec that can be embedded in both the ClusterServicePlanSpec and a new namesapced plan

jeremyrickard · 2018-03-15T17:16:21Z

There were some comments on the current PRs about the use of "Shared" as the prefix on these extracted objects. Do we want to revise the proposal before we get too deep into the refactor?

cc: @n3wscott

n3wscott · 2018-03-15T17:41:28Z

docs/proposals/ns-scoped-catalog.md

+
+To extract the shared fields, we will:
+
+- Extract the identified shared fields onto embedable types: `SharedServiceBrokerSpec`, `SharedServiceClassSpec`, and `SharedServicePlanSpec`.


I am questioning Shared, how about Common or Core

Paul suggested SharedServerFooFields and I like that.

These don't show up to the user, do they?

As long as the notation is consistent and there is documentation describing what they are, I'm probably fine with most suggested names.

Nope, the names of these types never surface to users.

* Extracting common plan spec elements into embeddable struct As part of the Prosopal for Namespaced Resources (#1826), this extracts the common elements from the ClusterServicePlanSpec into a SharedServicePlanSpec that can be embedded in both the ClusterServicePlanSpec and a new namesapced plan * Cleaned up comments on shared fields to remove "Cluster" from description. * Cleaned up comments on shared fields to remove "Cluster" from description. * Refactored ClusterPlanStatus to match proposal * Revert "Refactored ClusterPlanStatus to match proposal" This reverts commit 6b36cf7.

nilebox

See the comments.
Also I remember @duglin mentioning that he doesn't like merging proposals. Do we want to eventually merge it or just use PR for review?

nilebox · 2018-03-19T09:49:16Z

docs/proposals/ns-scoped-catalog.md

+
+Next, we will make the required changes to the binding controller loop to make it possible to create bindings against service instances that are associated with the ns-scoped variants of serviceclasses and plans.
+
+### Remove feature flag


This is the first time feature flag is mentioned in this proposal, i.e. there is no "add feature flag" anywhere.
Also, I'm not even sure we need feature flag at all, given that the proposed changes are backward compatible. Is that to be able to iteratively implement this proposal, i.e. having it merged without enabling for a first couple iterations until we get MVP? I would probably prefer to have an MVP in the first PR.

I think he means feature gate

Yep, feature gate.

nilebox · 2018-03-19T09:50:25Z

docs/proposals/ns-scoped-catalog.md

+
+### Use Case: Developing new services
+
+Similar to to highly privileged services, there are a class of users who are service developers, and would be interested in publishing their services via a broker to the rest of the cluster. As part of their development cycle, they would like to work within their own private namespace while iterating on their service.


nit: "to to"

nilebox · 2018-03-19T09:51:42Z

docs/proposals/ns-scoped-catalog.md

+- Create a policy that adds certain services and plans to multiple namespaces
+- Creating a virtual resource that allows users to see all services or plans available to them across the cluster and namespace scopes
+
+We should take care to acheive the goals of this proposal without preventing further progress on other issues that are out of scope.


s/acheive/achieve

nilebox · 2018-03-19T09:52:02Z

docs/proposals/ns-scoped-catalog.md

+
+## Goals and non-goals
+
+There are many related problems in service-catalog that users are interested in solutions for, but we need to keep the scope of this proposal controlled. In that light, let's establish the goals of this proposal:


service-catalog -> Service Catalog

nilebox · 2018-03-19T09:54:53Z

docs/proposals/ns-scoped-catalog.md

+
+Adding namespaced resources for brokers, services and plans is necessary but not sufficient to control access to services and plans. A single broker may offer a mix of services that all users should be able to access and services that should only be usable by some users.
+
+In order to prevent a broker that offers a mix of unprivileged and privileged services to the cluster-scoped catalog, there must be a way to filter the services and plans exposed by a broker. This can be accomplished through the use of white/black lists that control which services and plans in a broker's catalog have service-catalog resources created for them. For example:


As I said before in person, I think that the entire Filtering services and plans from a broker paragraph is unrelated to the proposal, and should be just briefly mentioned in the The following are valid goals but outside the scope of this proposal: list instead.

The initial proposal should be about namespace-local services/plans only without support for mix of services with different access level.

I would like to leave it in because it's an important part of the story we're trying to make possible here. Is it really necessary to rewrite it out of this PR?

@pmorie it seems that you have removed it already, but I will still reply :)
I didn't want to be part of this proposal, as it addresses a different concern, and it's harder to sell/buy 2 proposals bundled into one. Unless your plan was to silently slip the black/whitelist proposal as part of the bigger one haha :)

MHBauer

Read through it.

MHBauer · 2018-03-19T17:50:04Z

docs/proposals/ns-scoped-catalog.md

+There are many related problems in service-catalog that users are interested in solutions for, but we need to keep the scope of this proposal controlled. In that light, let's establish the goals of this proposal:
+
+- Make it possible to keep the cluster-scoped catalog limited to services and plans that everyone should be able to use
+- Make it possible to add privileged services and plans into specific namespaces


strike the word privileged.

not sure if add is the correct word, but that is a nit.

MHBauer · 2018-03-19T17:53:20Z

docs/proposals/ns-scoped-catalog.md

+1. Not all authorizers are able to provide a list of subjects with access to a resource
+2. An external authorizer may have its state changed at any time out of band to kubernetes, making it impossible to do implement a correct `LIST` or `WATCH` operation from a certain resource version
+
+Since ACL-filtering the cluster-scoped list of services and plans is not a realistic option, we must find another method of controlling read/write access to resources. In Kubernetes, namespaces are the defacto way of performing this type of access control.


I don't run or operate an application in a cluster. Any documentation on this use of namespaces? Blogs or official guidance. I've never understood the original use case for namespaces.

https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/

MHBauer · 2018-03-19T17:55:43Z

docs/proposals/ns-scoped-catalog.md

+There are two ingredients to successfully controlling access to services and plans:
+
+- Namespaced versions of these resources are required to control access to services along the boundaries of namespaces
+- API surfaces to allow black/whitelisting which services and plans from a broker's catalog have k8s resources created for them


From the rest of the proposal, this seems outside the boundaries. Limit this to resources, management later.

That's addressed later in the doc

black/whitelisting was removed from the doc, so probably remove this paragraph as well.

MHBauer · 2018-03-19T17:59:54Z

docs/proposals/ns-scoped-catalog.md

+
+The API for the `ServiceBroker` resource should differ from `ClusterServiceBroker` in exactly one area:
+
+- A user should only be able to specify a secret within the same namespace to hold the auth information


Does this mean a localref of some sort? It would mean a special version of AuthInfo structs. Otherwise this is more of a policy thing done by either an admission controller or the controller proper.

This is handled directly by kubernetes I believe. Nothing special we need to do here as long as bindings are namespaced.

@MHBauer you're right; the plan was to introduce a LocalObjectRefthat only contains the name, since the namespace is implied. There is ClusterServiceBrokerAuthInfo with ClusterBasicAuthConfig and ClusterBearerTokenAuthConfig. This is in service of making room for namespaced variants without the Cluster prefix that use the LocalObjectRef.

MHBauer · 2018-03-19T18:02:48Z

docs/proposals/ns-scoped-catalog.md

+
+Differences between `ClusterServiceClass` and `ServiceClass`:
+
+- `ServiceClass.Spec` should have `ServiceBrokerName` instead of `ClusterServiceBrokerName`


Should we genericize this field to share it in struct data representation? To me it would seem 'obvious' that the cluster scoped class one should look for a cluster scoped broker, and that a namespace scoped one should look in the same namespace.

That's dealt with later, in the implementation plan.

MHBauer · 2018-03-19T18:17:23Z

docs/proposals/ns-scoped-catalog.md

+
+To extract the shared fields, we will:
+
+- Extract the identified shared fields onto embedable types: `SharedServiceBrokerSpec`, `SharedServiceClassSpec`, and `SharedServicePlanSpec`.


These don't show up to the user, do they?

As long as the notation is consistent and there is documentation describing what they are, I'm probably fine with most suggested names.

MHBauer · 2018-03-19T18:19:14Z

docs/proposals/ns-scoped-catalog.md

+- Minor controller changes to reflect the fact that these fields now belong to an embedded type.
+- Required updates to the fuzzer, validations, and defaults where necessary.
+
+This PR will result in no behavioral changes, and should remain entirely transparent to users. It is effectively a no-op.


I like this plan.

MHBauer · 2018-03-19T18:20:30Z

docs/proposals/ns-scoped-catalog.md

+
+Next, we will make the required changes to the binding controller loop to make it possible to create bindings against service instances that are associated with the ns-scoped variants of serviceclasses and plans.
+
+### Remove feature flag


I think he means feature gate

MHBauer · 2018-03-19T18:21:14Z

docs/proposals/ns-scoped-catalog.md

+type ServiceBrokerSpec struct {
+    SharedServiceBrokerSpec `json:",inline"`
+
+    AuthInfo *ServiceBrokerAuthInfo `json:"authInfo,omitempty"`


I think this is what I mean by localref version of struct above.

This is the old name, but the content needs to switch to some kind of localref.

Yeah, that's what's implied. I can add the type description here if people think it's necessary

MHBauer · 2018-03-19T18:21:59Z

docs/proposals/ns-scoped-catalog.md

+type ClusterServiceBrokerSpec struct {
+    SharedServiceBrokerSpec `json:",inline"`
+
+    AuthInfo *ClusterServiceBrokerAuthInfo `json:"authInfo,omitempty"`


This is new and needs to be defined as the existing spec.

pmorie · 2018-03-21T17:24:38Z

Addressed comments

n3wscott · 2018-03-22T21:51:11Z

LGTM

carolynvs · 2018-04-17T21:13:48Z

EDIT: Updated s/SCOPE/NAMESPACE to match agreement below.

At the F2F, @eriknelson @jeremyrickard and I talked about what svcat would look like with namespaced scoped brokers.

get by namespace by default. This would change the current behavior of svcat to be consistent with how kubectl and svcat treat namespace scoped resources today. When no scope is specified, the current namespace defined on the kube context is used.
support --namespace and --all-namespaces flags
introduce --cluster flag to indicate listing at the cluster level
combine --all-namespaces and --cluster to show everything

$ svcat get classes
	NAME          NAMESPACE       DESCRIPTION
+------------------+-------------+---------------------+
  azure-mysql       test-ns   	     mysql on azure

$ svcat describe class azure-mysql
  Name: azure-mysql
  Namespace: test-ns
  ... TRUNCATED

$ svcat get classes --namespace qa-ns
	NAME          NAMESPACE       DESCRIPTION
+------------------+-------------+---------------------+
  google-mysql        qa-ns   	     mysql on google

$ svcat get classes --cluster
	NAME          NAMESPACE       DESCRIPTION
+------------------+-------------+---------------------+
  rando-mysql                     mysql on rando cloud

$ svcat describe class rando-mysql --cluster
  Name: rando-mysql
  ... TRUNCATED

$ svcat get classes --cluster --all-namespaces
	NAME          NAMESPACE       DESCRIPTION
+------------------+-------------+---------------------+
  rando-mysql                     mysql on rando cloud
  google-mysql        qa-ns   	     mysql on google
  azure-mysql       test-ns   	     mysql on azure

I didn't include UUID because I'm lazy and am just focusing on the new column, SCOPE.
I am suggesting SCOPE and not just NAMESPACE, because that will work well with user-defined classes and plans from my proposal and give us a way to indicate if a plan came from the broker or not. https://github.com/carolynvs/service-catalog/blob/3de6f842996056489d4c9aac53d4efa2b26a6381/docs/proposals/default-service-plans.md#service-class-discovery.

n3wscott · 2018-04-18T20:34:25Z

new column, SCOPE.

What is the convention in k8s? It might just make sense to call it NAMESPACE

jeremyrickard · 2018-04-18T20:43:25Z

If we label that field namespace, would we leave the cluster scoped resources blank in that column?

n3wscott · 2018-04-18T22:26:59Z

If we label that field namespace, would we leave the cluster scoped resources blank in that column?

yeah, it has no namespace.

jeremyrickard · 2018-04-19T00:01:12Z

yeah, it has no namespace.

@n3wscott Ok, that's what I assumed when I read your reply. I think I would vote in favor of the following:

Namespace Column on the table, with empty value for cluster level things
On describe, include the "scope" : [cluster|namespace] and then an optional namespace like...
JSON and YAML probably also the later case (is Type Metadata + object metadata sufficient in the yaml or json output to cover that?)

$ svcat describe class azure-mysql
  Name: azure-mysql
  Scope: namespace
  Namespace: test-ns
  ... TRUNCATED

and

$ svcat describe class rando-mysql --cluster
  Name: rando-mysql
  Scope: cluster
  ... TRUNCATED

I think it's clear from absence of a value in the namespace column, but on the describe i think the explicit fields help. What do you think @carolynvs

carolynvs · 2018-04-19T13:58:37Z

I think it's clear from absence of a value in the namespace column, but on the describe i think the explicit fields help. What do you think @carolynvs

PK85 · 2018-06-11T07:33:39Z

docs/proposals/ns-scoped-catalog.md

+
+## Design
+
+In this proposal we'll focus on adding the namespaced `ServiceBroker`, `ServiceClass`, and `ServicePlan` resources. For details on filtering which services and plans in a broker's catalog have k8s resources created for them, see https://github.com/kubernetes-incubator/Service Catalog/pull/1773.


@pmorie I have a general question about namespaced brokers.
Does it mean I can register same service-broker many times? For example Azure Broker as ClusterServiceBroker, and then as ServiceBroker twice for particular namespaces.

Also have the same question. Seems it should be possible to have the same broker as a CSB and a SB but would be good to know for sure

Yes, you can absolutely do that. I'd be happy to make that explicit if people care about the design proposal beyond this PR.

@pmorie If user can register the same service-broker for many times, such as for clusterservicebroker and then as ServiceBroker for a namespace, how about the serviceclass fetched from the broker ?
As I can see, the for a particular apb, the class ID the unique, but it can be fetched by the clusterservicebroker and servicebroker, how to distinguish the two ？

I was also thinking about this. Would this perhaps be handled by the catalog restrictions filter on the broker? IE if using the same broker for a service broker and cluster broker then set a filter https://github.com/kubernetes-incubator/service-catalog/pull/1773/files#diff-316f664334f2a9051f4897bee901c5f5R135 to filter out what you want to expose as a ServiceClass and what you want to expose as a ClusterServicePlan. An example of doing this would be very useful I think (If this is the way it would be done)

@zihantang-rh, IMO when getting list of service classes you will use different API to distinguish same class with same uuid by SB or CSB API. During provisioning you always provide clusterServiceClassExternalName or serviceClassExternalName.

maleck13 · 2018-06-21T11:56:45Z

docs/proposals/ns-scoped-catalog.md

+
+## Design
+
+In this proposal we'll focus on adding the namespaced `ServiceBroker`, `ServiceClass`, and `ServicePlan` resources. For details on filtering which services and plans in a broker's catalog have k8s resources created for them, see https://github.com/kubernetes-incubator/Service Catalog/pull/1773.


The link here looks incorrect should it be https://github.com/kubernetes-incubator/service-catalog/pull/1773

That does look like a more correct link :)

carolynvs · 2018-08-13T21:25:41Z

@pmorie Since namespaced brokers is implemented, and documented, what's the end state for this PR? Can I merge/close it?

carolynvs · 2018-09-10T20:49:39Z

We discussed this at the SIG meeting today. Since this doc is going to go stale fast (if it isn't already), we decided to close the proposal, and then create follow-up issues to mine the proposal for any missing docs that should be added to our website.

There a few features yet to be implemented, but they are all on the svcat cli side, and we have people working on them already.

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 Mar 12, 2018

carolynvs approved these changes Mar 13, 2018

View reviewed changes

carolynvs added the LGTM1 label Mar 13, 2018

jeremyrickard mentioned this pull request Mar 14, 2018

Extracting common plan spec into embeddable struct #1833

Merged

eriknelson mentioned this pull request Mar 15, 2018

Extract common service class spec fields #1834

Merged

n3wscott reviewed Mar 15, 2018

View reviewed changes

eriknelson mentioned this pull request Mar 15, 2018

Extracting common broker spec elements into embeddable struct #1841

Merged

nilebox reviewed Mar 19, 2018

View reviewed changes

nilebox added the do-not-merge label Mar 19, 2018

MHBauer reviewed Mar 19, 2018

View reviewed changes

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

157f9f5

pmorie force-pushed the ns-scoped-resources-proposal branch from 648d0c0 to 157f9f5 Compare March 21, 2018 16:49

pmorie mentioned this pull request Apr 19, 2018

Determine scoping of Cluster registry types kubernetes-retired/cluster-registry#220

Closed

carolynvs mentioned this pull request Apr 24, 2018

Do not include UUID in get class|plan output #1977

Merged

fmui mentioned this pull request May 9, 2018

K8S Platform Specific Deployment for Service Broker Proxy module Peripli/service-manager#29

Open

djzager mentioned this pull request May 16, 2018

Allow clusterwide actions within apb openshift/ansible-service-broker#576

Closed

carolynvs added this to the 1.0.0 milestone May 21, 2018

PK85 reviewed Jun 11, 2018

View reviewed changes

carolynvs mentioned this pull request Jun 12, 2018

Improve kubectl get experience for service catalog resources #2091

Merged

carolynvs added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge labels Jun 12, 2018

maleck13 reviewed Jun 21, 2018

View reviewed changes

carolynvs closed this Sep 10, 2018


		To extract the shared fields, we will:

		- Extract the identified shared fields onto embedable types: `SharedServiceBrokerSpec`, `SharedServiceClassSpec`, and `SharedServicePlanSpec`.


		Next, we will make the required changes to the binding controller loop to make it possible to create bindings against service instances that are associated with the ns-scoped variants of serviceclasses and plans.

		### Remove feature flag


		### Use Case: Developing new services

		Similar to to highly privileged services, there are a class of users who are service developers, and would be interested in publishing their services via a broker to the rest of the cluster. As part of their development cycle, they would like to work within their own private namespace while iterating on their service.


		## Goals and non-goals

		There are many related problems in service-catalog that users are interested in solutions for, but we need to keep the scope of this proposal controlled. In that light, let's establish the goals of this proposal:


		Adding namespaced resources for brokers, services and plans is necessary but not sufficient to control access to services and plans. A single broker may offer a mix of services that all users should be able to access and services that should only be usable by some users.

		In order to prevent a broker that offers a mix of unprivileged and privileged services to the cluster-scoped catalog, there must be a way to filter the services and plans exposed by a broker. This can be accomplished through the use of white/black lists that control which services and plans in a broker's catalog have service-catalog resources created for them. For example:


		The API for the `ServiceBroker` resource should differ from `ClusterServiceBroker` in exactly one area:

		- A user should only be able to specify a secret within the same namespace to hold the auth information


		Differences between `ClusterServiceClass` and `ServiceClass`:

		- `ServiceClass.Spec` should have `ServiceBrokerName` instead of `ClusterServiceBrokerName`


		## Design

		In this proposal we'll focus on adding the namespaced `ServiceBroker`, `ServiceClass`, and `ServicePlan` resources. For details on filtering which services and plans in a broker's catalog have k8s resources created for them, see https://github.com/kubernetes-incubator/Service Catalog/pull/1773.

Conversation

pmorie commented Mar 12, 2018

Uh oh!

carolynvs left a comment

Choose a reason for hiding this comment

Uh oh!

jeremyrickard commented Mar 15, 2018

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilebox Mar 22, 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!

MHBauer 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!

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 commented Mar 21, 2018

nilebox left a comment •

edited

Loading

nilebox Mar 22, 2018 •

edited

Loading

carolynvs commented Apr 17, 2018 •

edited

Loading

maleck13 Jun 27, 2018 •

edited

Loading