allow deletion of binding with incorrect service reference for #1562 implemented with Jonathan Berkhahn #1827
Conversation
k8s-ci-robot
added
cncf-cla: yes
| )) | ||
| return c.processUnbindSuccess(binding) | ||
| } | ||
|
|
||
| msg := fmt.Sprintf( |
There was a problem hiding this comment.
Is this message still applicable now that we are explicitly handling a not found above? It seems that once you fall through to this point, the error wasn't a non-existent instance, but instead some other unspecified plumbing error that caused the retrieve to fail.
There was a problem hiding this comment.
I don't think the message was ever fully correct. I'm not familiar with the errors returned by a client.
WDYT about change it to something like error getting the and try to add the err to the message? @jberkhahn
|
Can you update the title of this PR to a real title please? |
MHBauer
changed the title
pkg/controller/controller_binding.go
Outdated
| )) | ||
| return c.processUnbindSuccess(binding) | ||
| } | ||
|
|
||
| msg := fmt.Sprintf( | ||
| `References a non-existent %s "%s/%s"`, | ||
| pretty.ServiceInstance, binding.Namespace, binding.Spec.ServiceInstanceRef.Name, |
There was a problem hiding this comment.
For line 379: https://github.com/kubernetes-incubator/service-catalog/pull/1827/files#diff-ac77d94525ae05a353ce905a06988cc9L372
Can the todo be cleaned up?
| glog.V(5).Info(pcb.Messagef(`References a non-existent %s "%s/%s"`, | ||
| pretty.ServiceInstance, binding.Namespace, binding.Spec.ServiceInstanceRef.Name, | ||
| )) | ||
| return c.processUnbindSuccess(binding) |
There was a problem hiding this comment.
I am worried that this allows a binding to be lost if the class changes underneath the instance type temporarily.
In the very least this check needs to look at binding.Status.UnbindStatus to see if there had been a successful binding request sent to the broker before we delete this from kubernetes because otherwise this will leak a binding on the broker side even if it happens to be in a bad state at the moment.
There was a problem hiding this comment.
According to the OSB, classes can't change. You can have a new class that's offered, but it's never related to the old one.
So if there is no reference to an instance or class, we will never be able to issue a meaningful request to the broker to delete the binding. If it's in a bad state now, how is it ever going to not be in a bad state in the future?
There was a problem hiding this comment.
I think we should throw an error instead and keep the binding in the "broken deletion" state instead of silently proceeding.
|
@MHBauer @jberkhahn sorry, I fail to follow the issue this PR is trying to resolve:
What does it mean? Why does instance deletion proceed with remaining binding, and what does it have to do with secrets? Sounds like a bug in the instance deletion logic that we try to mitigate there? Also, would be nice to create a linked issue to discuss it there. |
|
@nilebox |
if the instance is not found, print a message and proceed to success This happens after deletion of secrets. There is no valid instance to call, so we cannot figure out what to unbind, so the unbind call is skipped.
- remove outdated todo
|
@jberkhahn invalid instance != missing instance. So I still don't get why instance is missing and binding is still there. |
|
The only valid case of instance missing I can think of is this one:
In that case, I would not handle this in the controller, or just explicitly error out instead of silently deleting. What user can do to force the deletion is to again manually remove the finalizer on a binding. |
|
I have no input on the specifics of why this was needed. Perhaps @n3wscott or @kibbles-n-bytes could comment on how it's possible to enter this state? |
|
one example is if you delete classes/plans from k8s and force a relist from the broker. I would not expect my instances to be garbage collected by service catalog when the broker had been successfully communicated with in the past. |
|
closing per discussion in Mar 26, 2018 service catalog meeting. |
Fix for #1562
if the instance is not found, print a message and proceed to success
This happens after deletion of secrets. There is no valid instance to call,
so we cannot figure out what to unbind, so the unbind call is skipped.
@jberkhahn