Skip to content

Commit dec523e

Browse files
kerenlahavkerenlahav
authored
refactor - add finalizer in webhook (#28)
* refactor - add finalizer in webhook
1 parent 16b2bd7 commit dec523e

File tree

6 files changed

+49
-57
lines changed

6 files changed

+49
-57
lines changed

‎api/v1alpha1/types.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ type ControllerName string
1111
const (
1212
ServiceInstanceController ControllerName = "ServiceInstance"
1313
ServiceBindingController ControllerName = "ServiceBinding"
14+
FinalizerName string = "services.cloud.sap.com/sap-btp-finalizer"
1415
)
1516

1617
const (

‎api/v1alpha1/webhooks/servicebinding_mutating_webhook.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,11 @@ package webhooks
33
import (
44
"context"
55
"encoding/json"
6+
"fmt"
67
"net/http"
78

9+
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
10+
811
"github.com/SAP/sap-btp-service-operator/api/v1alpha1"
912
v1 "k8s.io/api/authentication/v1"
1013
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -29,6 +32,11 @@ func (s *ServiceBindingDefaulter) Handle(_ context.Context, req admission.Reques
2932
return admission.Errored(http.StatusBadRequest, err)
3033
}
3134

35+
if binding.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(binding, v1alpha1.FinalizerName) {
36+
controllerutil.AddFinalizer(binding, v1alpha1.FinalizerName)
37+
bindinglog.Info(fmt.Sprintf("added finalizer '%s' to service binding", v1alpha1.FinalizerName))
38+
}
39+
3240
// mutate the fields
3341
if len(binding.Spec.ExternalName) == 0 {
3442
bindinglog.Info("externalName not provided, defaulting to k8s name", "name", binding.Name)

‎api/v1alpha1/webhooks/serviceinstance_mutating_webhook.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,11 @@ package webhooks
33
import (
44
"context"
55
"encoding/json"
6+
"fmt"
67
"net/http"
78

9+
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
10+
811
"github.com/SAP/sap-btp-service-operator/api/v1alpha1"
912
v1 "k8s.io/api/authentication/v1"
1013
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -29,6 +32,11 @@ func (s *ServiceInstanceDefaulter) Handle(_ context.Context, req admission.Reque
2932
return admission.Errored(http.StatusBadRequest, err)
3033
}
3134

35+
if instance.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(instance, v1alpha1.FinalizerName) {
36+
controllerutil.AddFinalizer(instance, v1alpha1.FinalizerName)
37+
instancelog.Info(fmt.Sprintf("added finalizer '%s' to service instance", v1alpha1.FinalizerName))
38+
}
39+
3240
// mutate the fields
3341
if len(instance.Spec.ExternalName) == 0 {
3442
instancelog.Info("externalName not provided, defaulting to k8s name", "name", instance.Name)

‎controllers/base_controller.go‎

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -99,24 +99,6 @@ func (r *BaseReconciler) removeFinalizer(ctx context.Context, object servicesv1a
9999
return nil
100100
}
101101

102-
func (r *BaseReconciler) addFinalizer(ctx context.Context, object servicesv1alpha1.SAPBTPResource, finalizerName string, log logr.Logger) error {
103-
if !controllerutil.ContainsFinalizer(object, finalizerName) {
104-
controllerutil.AddFinalizer(object, finalizerName)
105-
if err := r.Update(ctx, object); err != nil {
106-
if err := r.Get(ctx, apimachinerytypes.NamespacedName{Name: object.GetName(), Namespace: object.GetNamespace()}, object); err != nil {
107-
return client.IgnoreNotFound(err)
108-
}
109-
controllerutil.AddFinalizer(object, finalizerName)
110-
if err := r.Update(ctx, object); err != nil {
111-
return fmt.Errorf("failed to add the finalizer '%s'. Error: %v", finalizerName, err)
112-
}
113-
}
114-
log.Info(fmt.Sprintf("added finalizer '%s' to %s", finalizerName, object.GetControllerName()))
115-
return nil
116-
}
117-
return nil
118-
}
119-
120102
func (r *BaseReconciler) updateStatusWithRetries(ctx context.Context, object servicesv1alpha1.SAPBTPResource, log logr.Logger) error {
121103
logFailedAttempt := func(retries int, err error) {
122104
log.Info(fmt.Sprintf("failed to update status of %s attempt #%v, %s", object.GetControllerName(), retries, err.Error()))
@@ -157,10 +139,7 @@ func (r *BaseReconciler) updateStatus(ctx context.Context, object servicesv1alph
157139
return nil
158140
}
159141

160-
func (r *BaseReconciler) init(ctx context.Context, finalizer string, log logr.Logger, obj servicesv1alpha1.SAPBTPResource) error {
161-
if err := r.addFinalizer(ctx, obj, finalizer, log); err != nil {
162-
return err
163-
}
142+
func (r *BaseReconciler) init(ctx context.Context, log logr.Logger, obj servicesv1alpha1.SAPBTPResource) error {
164143
setInProgressCondition(smTypes.CREATE, "Pending", obj)
165144
if err := r.updateStatusWithRetries(ctx, obj, log); err != nil {
166145
return err

‎controllers/servicebinding_controller.go‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,6 @@ import (
3737
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
3838
)
3939

40-
const bindingFinalizerName string = "services.cloud.sap.com/binding-finalizer"
41-
4240
// ServiceBindingReconciler reconciles a ServiceBinding object
4341
type ServiceBindingReconciler struct {
4442
*BaseReconciler
@@ -67,6 +65,13 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
6765
}
6866
serviceBinding = serviceBinding.DeepCopy()
6967

68+
if len(serviceBinding.GetConditions()) == 0 {
69+
err := r.init(ctx, log, serviceBinding)
70+
if err != nil {
71+
return ctrl.Result{}, err
72+
}
73+
}
74+
7075
smClient, err := r.getSMClient(ctx, log, serviceBinding)
7176
if err != nil {
7277
setFailureConditions(smTypes.CREATE, err.Error(), serviceBinding)
@@ -84,13 +89,6 @@ func (r *ServiceBindingReconciler) Reconcile(ctx context.Context, req ctrl.Reque
8489
if isDelete(serviceBinding.ObjectMeta) {
8590
return r.delete(ctx, smClient, serviceBinding, log)
8691
}
87-
// The object is not being deleted, so if it does not have our finalizer,
88-
// then lets init it
89-
if !controllerutil.ContainsFinalizer(serviceBinding, bindingFinalizerName) {
90-
if err := r.init(ctx, bindingFinalizerName, log, serviceBinding); err != nil {
91-
return ctrl.Result{}, err
92-
}
93-
}
9492

9593
if serviceBinding.GetObservedGeneration() > 0 && !isInProgress(serviceBinding) {
9694
log.Info("Binding already created")
@@ -246,7 +244,7 @@ func (r *ServiceBindingReconciler) createBinding(ctx context.Context, smClient s
246244
}
247245

248246
func (r *ServiceBindingReconciler) delete(ctx context.Context, smClient sm.Client, serviceBinding *v1alpha1.ServiceBinding, log logr.Logger) (ctrl.Result, error) {
249-
if controllerutil.ContainsFinalizer(serviceBinding, bindingFinalizerName) {
247+
if controllerutil.ContainsFinalizer(serviceBinding, v1alpha1.FinalizerName) {
250248
if len(serviceBinding.Status.BindingID) == 0 {
251249
log.Info("No binding id found validating binding does not exists in SM before removing finalizer")
252250
smBinding, err := r.getBindingForRecovery(smClient, serviceBinding, log)
@@ -266,7 +264,7 @@ func (r *ServiceBindingReconciler) delete(ctx context.Context, smClient sm.Clien
266264
}
267265

268266
log.Info("Binding does not exists in SM, removing finalizer")
269-
if err := r.removeFinalizer(ctx, serviceBinding, bindingFinalizerName, log); err != nil {
267+
if err := r.removeFinalizer(ctx, serviceBinding, v1alpha1.FinalizerName, log); err != nil {
270268
return ctrl.Result{}, err
271269
}
272270
return ctrl.Result{}, nil
@@ -550,7 +548,7 @@ func (r *ServiceBindingReconciler) removeBindingFromKubernetes(ctx context.Conte
550548
}
551549

552550
// remove our finalizer from the list and update it.
553-
if err := r.removeFinalizer(ctx, serviceBinding, bindingFinalizerName, log); err != nil {
551+
if err := r.removeFinalizer(ctx, serviceBinding, v1alpha1.FinalizerName, log); err != nil {
554552
return ctrl.Result{}, err
555553
}
556554

‎controllers/serviceinstance_controller.go‎

Lines changed: 21 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -26,16 +26,14 @@ import (
2626

2727
smTypes "github.com/Peripli/service-manager/pkg/types"
2828
"github.com/Peripli/service-manager/pkg/web"
29-
servicesv1alpha1 "github.com/SAP/sap-btp-service-operator/api/v1alpha1"
29+
"github.com/SAP/sap-btp-service-operator/api/v1alpha1"
3030
"github.com/SAP/sap-btp-service-operator/client/sm/types"
3131
"github.com/go-logr/logr"
3232
apierrors "k8s.io/apimachinery/pkg/api/errors"
3333
ctrl "sigs.k8s.io/controller-runtime"
3434
"sigs.k8s.io/controller-runtime/pkg/client"
3535
)
3636

37-
const instanceFinalizerName string = "services.cloud.sap.com/instance-finalizer"
38-
3937
// ServiceInstanceReconciler reconciles a ServiceInstance object
4038
type ServiceInstanceReconciler struct {
4139
*BaseReconciler
@@ -48,7 +46,7 @@ type ServiceInstanceReconciler struct {
4846
func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
4947
log := r.Log.WithValues("serviceinstance", req.NamespacedName)
5048

51-
serviceInstance := &servicesv1alpha1.ServiceInstance{}
49+
serviceInstance := &v1alpha1.ServiceInstance{}
5250
if err := r.Get(ctx, req.NamespacedName, serviceInstance); err != nil {
5351
if !apierrors.IsNotFound(err) {
5452
log.Error(err, "unable to fetch ServiceInstance")
@@ -60,6 +58,13 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
6058
}
6159
serviceInstance = serviceInstance.DeepCopy()
6260

61+
if len(serviceInstance.GetConditions()) == 0 {
62+
err := r.init(ctx, log, serviceInstance)
63+
if err != nil {
64+
return ctrl.Result{}, err
65+
}
66+
}
67+
6368
smClient, err := r.getSMClient(ctx, log, serviceInstance)
6469
if err != nil {
6570
setFailureConditions(smTypes.CREATE, err.Error(), serviceInstance)
@@ -77,13 +82,6 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
7782
if isDelete(serviceInstance.ObjectMeta) {
7883
return r.deleteInstance(ctx, smClient, serviceInstance, log)
7984
}
80-
// The object is not being deleted, so if it does not have our finalizer,
81-
// then lets init it
82-
if !controllerutil.ContainsFinalizer(serviceInstance, instanceFinalizerName) {
83-
if err := r.init(ctx, instanceFinalizerName, log, serviceInstance); err != nil {
84-
return ctrl.Result{}, err
85-
}
86-
}
8785

8886
if serviceInstance.Generation == serviceInstance.Status.ObservedGeneration && !isInProgress(serviceInstance) {
8987
log.Info(fmt.Sprintf("Spec is not changed - ignoring... Generation is - %v", serviceInstance.Generation))
@@ -119,14 +117,14 @@ func (r *ServiceInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
119117
return r.updateInstance(ctx, smClient, serviceInstance, log)
120118
}
121119

122-
func (r *ServiceInstanceReconciler) poll(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1alpha1.ServiceInstance, log logr.Logger) (ctrl.Result, error) {
120+
func (r *ServiceInstanceReconciler) poll(ctx context.Context, smClient sm.Client, serviceInstance *v1alpha1.ServiceInstance, log logr.Logger) (ctrl.Result, error) {
123121
log.Info(fmt.Sprintf("resource is in progress, found operation url %s", serviceInstance.Status.OperationURL))
124122
status, statusErr := smClient.Status(serviceInstance.Status.OperationURL, nil)
125123
if statusErr != nil {
126124
log.Info(fmt.Sprintf("failed to fetch operation, got error from SM: %s", statusErr.Error()), "operationURL", serviceInstance.Status.OperationURL)
127125
setFailureConditions(serviceInstance.Status.OperationType, statusErr.Error(), serviceInstance)
128126
// if failed to read operation status we cleanup the status to trigger re-sync from SM
129-
freshStatus := servicesv1alpha1.ServiceInstanceStatus{Conditions: serviceInstance.GetConditions()}
127+
freshStatus := v1alpha1.ServiceInstanceStatus{Conditions: serviceInstance.GetConditions()}
130128
if isDelete(serviceInstance.ObjectMeta) {
131129
freshStatus.InstanceID = serviceInstance.Status.InstanceID
132130
}
@@ -161,7 +159,7 @@ func (r *ServiceInstanceReconciler) poll(ctx context.Context, smClient sm.Client
161159
setSuccessConditions(smTypes.OperationCategory(status.Type), serviceInstance)
162160
if serviceInstance.Status.OperationType == smTypes.DELETE {
163161
// delete was successful - remove our finalizer from the list and update it.
164-
if err := r.removeFinalizer(ctx, serviceInstance, instanceFinalizerName, log); err != nil {
162+
if err := r.removeFinalizer(ctx, serviceInstance, v1alpha1.FinalizerName, log); err != nil {
165163
return ctrl.Result{}, err
166164
}
167165
}
@@ -173,7 +171,7 @@ func (r *ServiceInstanceReconciler) poll(ctx context.Context, smClient sm.Client
173171
return ctrl.Result{}, r.updateStatusWithRetries(ctx, serviceInstance, log)
174172
}
175173

176-
func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1alpha1.ServiceInstance, log logr.Logger) (ctrl.Result, error) {
174+
func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient sm.Client, serviceInstance *v1alpha1.ServiceInstance, log logr.Logger) (ctrl.Result, error) {
177175
log.Info("Creating instance in SM")
178176
_, instanceParameters, err := buildParameters(r.Client, serviceInstance.Namespace, serviceInstance.Spec.ParametersFrom, serviceInstance.Spec.Parameters)
179177
if err != nil {
@@ -221,7 +219,7 @@ func (r *ServiceInstanceReconciler) createInstance(ctx context.Context, smClient
221219
return ctrl.Result{}, r.updateStatusWithRetries(ctx, serviceInstance, log)
222220
}
223221

224-
func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1alpha1.ServiceInstance, log logr.Logger) (ctrl.Result, error) {
222+
func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient sm.Client, serviceInstance *v1alpha1.ServiceInstance, log logr.Logger) (ctrl.Result, error) {
225223
var err error
226224

227225
log.Info(fmt.Sprintf("updating instance %s in SM", serviceInstance.Status.InstanceID))
@@ -261,8 +259,8 @@ func (r *ServiceInstanceReconciler) updateInstance(ctx context.Context, smClient
261259
return ctrl.Result{}, r.updateStatusWithRetries(ctx, serviceInstance, log)
262260
}
263261

264-
func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, smClient sm.Client, serviceInstance *servicesv1alpha1.ServiceInstance, log logr.Logger) (ctrl.Result, error) {
265-
if controllerutil.ContainsFinalizer(serviceInstance, instanceFinalizerName) {
262+
func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, smClient sm.Client, serviceInstance *v1alpha1.ServiceInstance, log logr.Logger) (ctrl.Result, error) {
263+
if controllerutil.ContainsFinalizer(serviceInstance, v1alpha1.FinalizerName) {
266264
if len(serviceInstance.Status.InstanceID) == 0 {
267265
log.Info("No instance id found validating instance does not exists in SM before removing finalizer")
268266

@@ -277,7 +275,7 @@ func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, smClient
277275
return ctrl.Result{}, r.updateStatusWithRetries(ctx, serviceInstance, log)
278276
}
279277
log.Info("instance does not exists in SM, removing finalizer")
280-
return ctrl.Result{}, r.removeFinalizer(ctx, serviceInstance, instanceFinalizerName, log)
278+
return ctrl.Result{}, r.removeFinalizer(ctx, serviceInstance, v1alpha1.FinalizerName, log)
281279
}
282280

283281
log.Info(fmt.Sprintf("Deleting instance with id %v from SM", serviceInstance.Status.InstanceID))
@@ -314,7 +312,7 @@ func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, smClient
314312
}
315313

316314
// remove our finalizer from the list and update it.
317-
if err := r.removeFinalizer(ctx, serviceInstance, instanceFinalizerName, log); err != nil {
315+
if err := r.removeFinalizer(ctx, serviceInstance, v1alpha1.FinalizerName, log); err != nil {
318316
return ctrl.Result{}, err
319317
}
320318

@@ -325,7 +323,7 @@ func (r *ServiceInstanceReconciler) deleteInstance(ctx context.Context, smClient
325323
return ctrl.Result{}, nil
326324
}
327325

328-
func (r *ServiceInstanceReconciler) resyncInstanceStatus(k8sInstance *servicesv1alpha1.ServiceInstance, smInstance *types.ServiceInstance) {
326+
func (r *ServiceInstanceReconciler) resyncInstanceStatus(k8sInstance *v1alpha1.ServiceInstance, smInstance *types.ServiceInstance) {
329327
//set observed generation to 0 because we dont know which generation the current state in SM represents,
330328
//unless the generation is 1 and SM is in the same state as operator
331329
if k8sInstance.Generation == 1 {
@@ -353,11 +351,11 @@ func (r *ServiceInstanceReconciler) resyncInstanceStatus(k8sInstance *servicesv1
353351

354352
func (r *ServiceInstanceReconciler) SetupWithManager(mgr ctrl.Manager) error {
355353
return ctrl.NewControllerManagedBy(mgr).
356-
For(&servicesv1alpha1.ServiceInstance{}).
354+
For(&v1alpha1.ServiceInstance{}).
357355
Complete(r)
358356
}
359357

360-
func (r *ServiceInstanceReconciler) getInstanceForRecovery(smClient sm.Client, serviceInstance *servicesv1alpha1.ServiceInstance, log logr.Logger) (*types.ServiceInstance, error) {
358+
func (r *ServiceInstanceReconciler) getInstanceForRecovery(smClient sm.Client, serviceInstance *v1alpha1.ServiceInstance, log logr.Logger) (*types.ServiceInstance, error) {
361359
parameters := sm.Parameters{
362360
FieldQuery: []string{
363361
fmt.Sprintf("name eq '%s'", serviceInstance.Spec.ExternalName),

0 commit comments

Comments
 (0)