Distinguish between user-defined and managed resources #2095
Conversation
k8s-ci-robot
added
cncf-cla: yes
carolynvs
changed the title
Classes and plans that were created by the service catalog controller during a list now have a controller reference set to indicate that they are managed by service catalog. User defined classes and plans will not have a controller reference set, and are left alone during a list.
carolynvs
force-pushed
the
mark-managed-classes-plans
branch
from
589584c to
ea17ab9
Compare
carolynvs
changed the title
| obj.SetOwnerReferences(append(obj.GetOwnerReferences(), controllerRef)) | ||
| } | ||
|
|
||
| func isServiceCatalogManagedResource(resource metav1.Object) bool { |
There was a problem hiding this comment.
I'm not sure about this one. My reservation is that ClusterServiceClass and ClusterServicePlan already have a field (spec.ClusterServiceBrokerName) that carries information about which ClusterServiceBroker manages the resource.
does some research
Alright, I think I understand this now. I hate to do design in this PR, another option to achieve the same behavior (ignoring user-defined serviceclasses/serviceplans) would be to have an annotation saying that the serviceclass/plan is user-defined. Have you considered that yet? I guess the merit of this approach over the opt-in annotation would be that a user doesn't have to know about the special annotation and will get the right behavior, in the naive path. WDYT?
There was a problem hiding this comment.
One thing I wonder about is whether setting the owner reference will have some unanticipated effects on garbage collection.
There was a problem hiding this comment.
I went down this path for a couple reasons:
- When a user makes a custom plan,
ClusterServiceBrokerNamewill be populated in their custom plan too. That's not enough to tell them apart. - Anything that wasn't made by our controller is automatically considered user defined. So that's a nicer UX.
- We already wanted to add controller references anyway (see Use
OwnerReferencecontroller refs for parent/child relationships #1432). - As far as I understand owner / controller references, what we are trying to indicate "our controller made this, don't mess with it" fits perfectly into how they are supposed to be used.
- I am explicitly turning off the garbage collection flag,
BlockOwnerDeletionso that we don't affect current delete behavior.
There was a problem hiding this comment.
Anything that wasn't made by our controller is automatically considered user defined. So that's a nicer UX.
I'm +1 on this approach because of this alone
| obj.SetOwnerReferences(append(obj.GetOwnerReferences(), controllerRef)) | ||
| } | ||
|
|
||
| func isServiceCatalogManagedResource(resource metav1.Object) bool { |
There was a problem hiding this comment.
Anything that wasn't made by our controller is automatically considered user defined. So that's a nicer UX.
I'm +1 on this approach because of this alone
|
LGTM |
|
I'm still getting caught up on User Defined Plans but from what I understand, this makes sense to me. LGTM. |
|
@jboyd01 This is part of the first milestone (User Defined Plans). I broke up the full proposal into smaller pieces so that we are shipping useful stuff incrementally. The tldr of the feature is to get to the point where cluster operators can specify default params or secrets or key transforms, so that each binding or instance doesn't have to know all those details. |
…tired#2095) Classes and plans that were created by the service catalog controller during a list now have a controller reference set to indicate that they are managed by service catalog. User defined classes and plans will not have a controller reference set, and are left alone during a list.
This is the first step towards supporting user-defined classes and plans from #1896. Before we can allow users to create them, we need to make sure that our controller doesn't immediately nuke them. 😀
Closes #2060