Cascading binding deletion

[piotrmiskiewicz]

Implements proposal: #2734

Adds a cascading deletion feature which deletes existing service bindings for a service instance which is being deleted.

Which issue(s) this PR fixes
Fixes: #481 kyma-project/kyma#5180

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: piotrmiskiewicz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [piotrmiskiewicz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[piotrmiskiewicz]

How to test:
Install the service-catalog with the CascadingDeletion feature flag.

Install the test-broker:

helm install charts/test-broker --name test-broker --namespace test-broker

and register it in the service-catalog:

cat <<EOF | kubectl apply -f -
apiVersion: servicecatalog.k8s.io/v1beta1
kind: ClusterServiceBroker
metadata:
  name: test-broker
spec:
  url: http://test-broker-test-broker.test-broker.svc.cluster.local
EOF

Create a namespace:
kubectl create ns test-ns

Create an instance

cat <<EOF | kubectl apply -f -
apiVersion: servicecatalog.k8s.io/v1beta1
kind: ServiceInstance
metadata:
  name: test-instance
  namespace: test-ns
spec:
  clusterServiceClassExternalName: test-service
  clusterServicePlanExternalName: default
EOF

Create 3 bindings:

cat <<EOF | kubectl apply -f -
apiVersion: servicecatalog.k8s.io/v1beta1
kind: ServiceBinding
metadata:
  name: test-binding1
  namespace: test-ns
spec:
  instanceRef:
    name: test-instance
---
apiVersion: servicecatalog.k8s.io/v1beta1
kind: ServiceBinding
metadata:
  name: test-binding2
  namespace: test-ns
spec:
  instanceRef:
    name: test-instance
---
apiVersion: servicecatalog.k8s.io/v1beta1
kind: ServiceBinding
metadata:
  name: test-binding3
  namespace: test-ns
spec:
  instanceRef:
    name: test-instance     
EOF

Then, delete the instance:

kubectl delete -n test-ns serviceinstance test-instance

check events:

kubectl get events -n test-ns --sort-by=.metadata.creationTimestamp

[piotrmiskiewicz]

The PR implements service bindings deletion in the controller due to a business requirement - deprovision must be done after all unbind operations.
The idea of having ownerReference in the ServiceBinding instances and use K8s garbage collector does not work. The k8s garbage collector will try to delete bindings, when service instance is deleted, but it can be deleted, when all unbind operation is done.

[MHBauer]

OMG, this is part of the matrix of deleting things! #2229

[jberkhahn]

/retest

[mszostok]

I still want to discuss that in details

• why kubectl delete .. --cascade=true cannot be used? Maybe we can reuse "propagationPolicy":"Foreground" concept but implement it on our own?
• or why the spec was not changed? Maybe we should have a new field under ServiceInstance spec, sth like deletionPolicy. By default, it will have old behavior. If a user will change that then the cascading opt will be used.

The current solution is not backward compatible and user cannot select different strategies for different ServiceInstance. With current impl you enable/disable it globally. Let's discuss that if we want to implement this feature in that way.

/hold

[piotrmiskiewicz]

There is a feature flag which enables cascading deletion, so the change is backward compatible. The ownerReference cannot be used because of a problem when the delete is done with propagationPolicy=background. The binding must be deleted before the instance. The risk is someone could do kubectl delete serviceinstance ... without the --cascade=true flag

[mszostok]

Feature gates are a set of key=value pairs that describe alpha or experimental features and can be turned on or off

so it's not backward compatible because you cannot remove that feature flag and make that behavior as the core one because u will change the deletion policy for all clients when they will update to new service catalog version.

I'm talking about the "propagationPolicy":"Foreground" not the ownerReference. We can impl that GC on our own as we did here but based on propagationPolicy but I'm not fully convinced to do that in that way. It's just an option and maybe better will be to extend the ServiceInstance Spec. IMO it will be also worth to show those two approaches in sig-api-machinery and ask them about the recommendation.

let's discuss that offline and post here clear strategy for having that feature in ServiceCatalog:)

[mszostok]

Suggested change

	| `CascadingDeletion` | ` false` | Alpha | v0.2.3 | |
	| `CascadingDeletion` | ` false` | Alpha | v0.3.0 | |

[mszostok]

just inline all of those statements:

c.bindingLister.ServiceBindings(instance.Namespace).List(labels.NewSelector())

[mszostok]

u're missing context for each error
e.g. here u should have

errors.Wrapf(err, "while fetching ServiceBindings for instance %s", instance.Name) 

[piotrmiskiewicz]

I've added wrapping errors

[mszostok]

also, adjust the chart with the new param that can be set by our clients

[piotrmiskiewicz]

/test pull-build-all-images-for-s390x

[mszostok]

@piotrmiskiewicz you probably missed this comment #2711 (comment). The Helm Chart is still not adjusted, please add opt to enable it, sth similar what we have already: https://github.com/kubernetes-sigs/service-catalog/blob/master/charts/catalog/values.yaml#L105-L106

[piotrmiskiewicz]

/test pull-service-catalog-test-migration

[mszostok]

just add disable-cascading-deletion flag to Helm Chart beside that
/lgtm

more info can be found here: #2734

[piotrmiskiewicz]

/test pull-service-catalog-test-integration

[jberkhahn]

why the double feature gate? i dont really understand how this is supposed to work if I set one and not the other.

[jberkhahn]

so, i can't seem to get this to work. I set the "cascadingDeletionEnabled" flag to true, and leave the "cascadingDeletionDisabled" to false, as it's a negative, which should make this work? these flags are confusing. why do we not just have a single top level flag like the other feature gates?

I go to deprovision the instance, and it gets stuck in the "DeprovisionBlockedByExistingCredentials" state. I'm using 'svcat' for this, btw.

[piotrmiskiewicz]

As I understand the process, the feature gate will be removed in the future and the feature will be enabled by default. The second (disabling) flag is designed for all users which does not want this feature and it should be set explicitly. Long term the feature gate will be removed and the cascading deletion will be enabled by default. There will be only the cascadingDeletionDisabled flag.

Currently, the cascading deletion feature is an alpha feature. This is the reason to use a feature gate.
I understand the names in the values.yaml could be confusing: cascadingDeletionEnabled and cascadingDeletionDisabled. If you have better ideas I can change it.

[piotrmiskiewicz]

I've made a mistake in the chart, which I've just fixed.

[jberkhahn]

why do we not leave the to-be-used-in-the-future-flag out of the chart now, and add it it later, then? I also think the double negative is weird, wouldn't it make more sense to make it --enable-foobar and set it to true by default?

[piotrmiskiewicz]

I understand that two options may be confusing, at the beginning I had only the feature gate.
On the other hand, the whole feature "cascading binding deletion" is an alpha feature and needs to be explicitly enabled (by default disabled). After some time - it can be enabled by default.
Are you sure we can change the way how the controller works just with the next release? No "alpha" stage for few releases?

[jberkhahn]

you can leave the option on the actual controller itself, although I would prefer it was a simple "enable-feature-foobar" flag rather than a weird double-negative one. I'm just saying take the ability to set it out of the helm charts so people don't get confused about why there's two settings that look like they do the same thing.

[piotrmiskiewicz]

We want to have the cascading deletion feature enabled by default in the future.
Do you mean something like: --enable-cascading-deletion which is enabled by default.
I someone doesn't want to use this feature, he could pass: --enable-cascading-deletion=false

Sounds good, but the problem is we are adding such feature without an alpha stage and this feature breaks the backward compatibility.

[piotrmiskiewicz]

/retest

[piotrmiskiewicz]

I removed the "disabling" flag. Now, there is only enabling flag

[jberkhahn]

/lgtm

[mszostok]

/hold cancel