Service-catalog: Helm Chart Does Not Create RBAC For Namespace Scoped Resources

[jeremyrickard]

Bug Report

What happened:

After deploying v0.1.24 with the namespaced broker enabled flag on a cluster with RBAC, service catalog experienced 40x errors while trying to work with Namespace scoped catalog resources. No serviceclass or serviceplan resources were created.

What you expected to happen:

ServiceClass or ServicePlan resources were created.

How to reproduce it (as minimally and precisely as possible):

helm install . --name catalog --namespace catalog \
   --set image=quay.io/kubernetes-service-catalog/service-catalog:v0.1.24 \
   --set namespacedServiceBrokerEnabled=true \
   --set controllerManager.healthcheck.enabled=false \
   --set apiserver.healthcheck.enabled=false

Anything else we need to know?:

Environment:

• Kubernetes version (use kubectl version):

Client Version: version.Info{Major:"1", Minor:"8", GitVersion:"v1.8.4", GitCommit:"9befc2b8928a9426501d3bf62f72849d5cbcd5a3", GitTreeState:"clean", BuildDate:"2017-11-20T19:12:24Z", GoVersion:"go1.9.2", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"9", GitVersion:"v1.9.6", GitCommit:"9f8ebd171479bec0ada837d7ee641dec2f8c6dd1", GitTreeState:"clean", BuildDate:"2018-03-21T15:13:31Z", GoVersion:"go1.9.3", Compiler:"gc", Platform:"linux/amd64"}

• service-catalog version:

v0.1.24

• Cloud provider or hardware configuration:

• Do you have api aggregation enabled?

  • Do you see the configmap in kube-system?
  • Does it have all the necessary fields?
  • kubectl get cm -n kube-system extension-apiserver-authentication -o yaml and look for requestheader-XXX fields

• Install tools:

  • Did you use helm? What were the helm arguments? Did you --set any extra values?

helm install . --name catalog --namespace catalog \
   --set image=quay.io/kubernetes-service-catalog/service-catalog:v0.1.24 \
   --set namespacedServiceBrokerEnabled=true \
   --set controllerManager.healthcheck.enabled=false \
   --set apiserver.healthcheck.enabled=false

• Are you trying to use ALPHA features? Did you enable them?

yes, namespaced brokers

[jeremyrickard]

dibs

[MHBauer]

are ns resources finished? I would only expect this if everything is considered finished. If it's not on the existing list of things to do for ns resources, it ought to be.

[jeremyrickard]

are ns resources finished? I would only expect this if everything is considered finished. If it's not on the existing list of things to do for ns resources, it ought to be.

So there are a few things to consider here.

1.) We do expose the feature gate via the Helm chart. This enables you to turn on the feature, but we don't create the RBAC so it turns on in a completely broken manner in any cluster with RBAC.
2.) This conditionally adds the RBAC resources if RBAC is enabled AND you've signaled that you want to turn that feature on. I think this is acceptable. Otherwise, we should turn off the feature gate in the helm chart and not expose it at all. That's why I submitted _this_ fix. We could remove the other and hold off on this. Either way, I think the chart as is can leave people in a bad spot.

To "are ns resources finished?" - I was able to do a full e2e with 0.1.24 (after I fixed the RBAC issue), so I think _functionally_ they are complete and people could experiment with it knowing it's an Alpha feature. There are some gaps in svcat that need to be implemented still. Others might have opinions about it being finished @jboyd01 @eriknelson what say you?

[eriknelson]

To "are ns resources finished?" - I was able to do a full e2e with 0.1.24 (after I fixed the RBAC issue), so I think functionally they are complete and people could experiment with it knowing it's an Alpha feature.

+1 to this statement. We've also been able to run e2e tests and are expecting to get this under some openshift QE once the code is released here with 0.1.24 and brought into Origin.

[MHBauer]

What is full e2e mean in this case? I see no e2e tests that exercise this?

[jeremyrickard]

What is full e2e mean in this case? I see no e2e tests that exercise this?

Manual e2e in my case.

• Create Namespaced Broker
• Verify Namespace Scoped Classes and Plans are created
• Provision an instance within the namespace containing the broker/class/plans.
• Bind to instance.
• Attempt to provision an instance within a different namespace than the one containing the broker/class/plans, this fails.

I agree we should not remove the feature gate until we have automated e2e tests, but I still feel that we _either_ need to remove the ability to set the feature flag from the helm chart or we should fix this. Right now, we have a broken flag :)

Since the user explicitly has to enable this, I think I'd go with the later because it seems to work well and will get additional people to possibly check it out and provide feedback before we remove the feature gate and open it up to the world.

I'll make an issue to address adding automated e2e for the capabilty.

[jboyd01]

Nice Jeremy. My understanding is we're ready to get some testing, use and refinements on this feature, including svcat as Jeremy mentioned and adding automated tests.

[eriknelson]

I mentioned we needed some more comprehensive integration testing in this project recently at Red Hat and I think @jmrodri started to investigate what that would look like. Sounds like an issue makes sense and we'll certainly help with any efforts.

[MHBauer]

https://github.com/kubernetes-incubator/service-catalog/issues/2193#issuecomment-404584017
Yes, e2e should be part of the requirements list.