fix(konnect): fix reconciling entities when they got created on Konne…

…ct but an error was returned (#716) * fix(konnect): fix reconciling entities when they got created on Konnect but an error was returned
Kong · Oct 10, 2024 · 742b401 · 742b401
1 parent 3e37556
commit 742b401
Show file tree

Hide file tree

Showing 5 changed files with 274 additions and 32 deletions.
diff --git a/controller/konnect/index_konnectgatewaycontrolplane.go b/controller/konnect/index_konnectgatewaycontrolplane.go
@@ -1,19 +1,28 @@
 package konnect
 
 import (
+	sdkkonnectcomp "github.com/Kong/sdk-konnect-go/models/components"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	konnectv1alpha1 "github.com/kong/kubernetes-configuration/api/konnect/v1alpha1"
 )
 
 const (
+	// IndexFieldKonnectGatewayControlPlaneGroupOnMembers is the index field for KonnectGatewayControlPlane -> its members.
+	IndexFieldKonnectGatewayControlPlaneGroupOnMembers = "konnectGatewayControlPlaneGroupMembers"
+
 	// IndexFieldKonnectGatewayControlPlaneOnAPIAuthConfiguration is the index field for KonnectGatewayControlPlane -> APIAuthConfiguration.
 	IndexFieldKonnectGatewayControlPlaneOnAPIAuthConfiguration = "konnectGatewayControlPlaneAPIAuthConfigurationRef"
 )
 
 // IndexOptionsForKonnectGatewayControlPlane returns required Index options for KonnectGatewayControlPlane reconciler.
 func IndexOptionsForKonnectGatewayControlPlane() []ReconciliationIndexOption {
 	return []ReconciliationIndexOption{
+		{
+			IndexObject:  &konnectv1alpha1.KonnectGatewayControlPlane{},
+			IndexField:   IndexFieldKonnectGatewayControlPlaneGroupOnMembers,
+			ExtractValue: konnectGatewayControlPlaneGroupMembers,
+		},
 		{
 			IndexObject:  &konnectv1alpha1.KonnectGatewayControlPlane{},
 			IndexField:   IndexFieldKonnectGatewayControlPlaneOnAPIAuthConfiguration,
@@ -22,6 +31,28 @@ func IndexOptionsForKonnectGatewayControlPlane() []ReconciliationIndexOption {
 	}
 }
 
+func konnectGatewayControlPlaneGroupMembers(object client.Object) []string {
+	cp, ok := object.(*konnectv1alpha1.KonnectGatewayControlPlane)
+	if !ok {
+		return nil
+	}
+	clusterType := cp.Spec.ClusterType
+	if clusterType == nil {
+		return nil
+	}
+
+	if string(*clusterType) != string(sdkkonnectcomp.ClusterTypeClusterTypeControlPlaneGroup) {
+		return nil
+	}
+
+	ret := make([]string, 0, len(cp.Spec.Members))
+	for _, member := range cp.Spec.Members {
+		ret = append(ret, member.Name)
+	}
+
+	return ret
+}
+
 func konnectGatewayControlPlaneAPIAuthConfigurationRef(object client.Object) []string {
 	cp, ok := object.(*konnectv1alpha1.KonnectGatewayControlPlane)
 	if !ok {

diff --git a/controller/konnect/ops/ops.go b/controller/konnect/ops/ops.go
@@ -186,10 +186,10 @@ func shouldUpdate[
 		requeueAfter := syncPeriod - timeFromLastUpdate
 		log.Debug(ctrllog.FromContext(ctx),
 			"no need for update, requeueing after configured sync period", ent,
-			"last_update", condProgrammed.LastTransitionTime.Time,
-			"time_from_last_update", timeFromLastUpdate,
-			"requeue_after", requeueAfter,
-			"requeue_at", now.Add(requeueAfter),
+			"last_update", condProgrammed.LastTransitionTime.Time.String(),
+			"time_from_last_update", timeFromLastUpdate.String(),
+			"requeue_after", requeueAfter.String(),
+			"requeue_at", now.Add(requeueAfter).String(),
 		)
 		return false, ctrl.Result{
 			RequeueAfter: requeueAfter,

diff --git a/controller/konnect/reconciler_generic.go b/controller/konnect/reconciler_generic.go
@@ -483,39 +483,54 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile(
 	// We should look at the "expectations" for this:
 	// https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/controller_utils.go
 	if status := ent.GetKonnectStatus(); status == nil || status.GetKonnectID() == "" {
+		obj := ent.DeepCopyObject().(client.Object)
 		_, err := ops.Create[T, TEnt](ctx, sdk, r.Client, ent)
-		if err != nil {
-			// TODO(pmalek): this is actually not 100% error prone because when status
-			// update fails we don't store the Konnect ID and hence the reconciler
-			// will try to create the resource again on next reconciliation.
-			if err := r.Client.Status().Update(ctx, ent); err != nil {
-				if k8serrors.IsConflict(err) {
-					return ctrl.Result{Requeue: true}, nil
+
+		// TODO: this is actually not 100% error prone because when status
+		// update fails we don't store the Konnect ID and hence the reconciler
+		// will try to create the resource again on next reconciliation.
+
+		// Regardless of the error reported from Create(), if the Konnect ID has been
+		// set then add the finalizer so that the resource can be cleaned up from Konnect on deletion.
+		if ent.GetKonnectStatus().ID != "" {
+			objWithFinalizer := ent.DeepCopyObject().(client.Object)
+			if controllerutil.AddFinalizer(objWithFinalizer, KonnectCleanupFinalizer) {
+				if errUpd := r.Client.Patch(ctx, objWithFinalizer, client.MergeFrom(ent)); errUpd != nil {
+					if k8serrors.IsConflict(errUpd) {
+						return ctrl.Result{Requeue: true}, nil
+					}
+					if err != nil {
+						return ctrl.Result{}, fmt.Errorf(
+							"failed to update finalizer %s: %w, object create operation failed against Konnect API: %w",
+							KonnectCleanupFinalizer, errUpd, err,
+						)
+					}
+					return ctrl.Result{}, fmt.Errorf(
+						"failed to update finalizer %s: %w",
+						KonnectCleanupFinalizer, errUpd,
+					)
 				}
-				return ctrl.Result{}, fmt.Errorf("failed to update status after creating object: %w", err)
 			}
+		}
 
-			return ctrl.Result{}, ops.FailedKonnectOpError[T]{
-				Op:  ops.CreateOp,
-				Err: err,
-			}
+		if err == nil {
+			setServerURLAndOrgIDFromAPIAuthConfiguration(ent, apiAuth)
 		}
 
-		ent.GetKonnectStatus().ServerURL = apiAuth.Spec.ServerURL
-		ent.GetKonnectStatus().OrgID = apiAuth.Status.OrganizationID
-		if err := r.Client.Status().Update(ctx, ent); err != nil {
+		// Regardless of the error, set the status as it can contain the Konnect ID
+		// and/or status conditions set in Create() and that ID will be needed to
+		// update/delete the object in Konnect.
+		if err := r.Client.Status().Patch(ctx, ent, client.MergeFrom(obj)); err != nil {
 			if k8serrors.IsConflict(err) {
 				return ctrl.Result{Requeue: true}, nil
 			}
-			return ctrl.Result{}, fmt.Errorf("failed to update status: %w", err)
+			return ctrl.Result{}, fmt.Errorf("failed to update status after creating object: %w", err)
 		}
 
-		if controllerutil.AddFinalizer(ent, KonnectCleanupFinalizer) {
-			if err := r.Client.Update(ctx, ent); err != nil {
-				if k8serrors.IsConflict(err) {
-					return ctrl.Result{Requeue: true}, nil
-				}
-				return ctrl.Result{}, fmt.Errorf("failed to update finalizer: %w", err)
+		if err != nil {
+			return ctrl.Result{}, ops.FailedKonnectOpError[T]{
+				Op:  ops.CreateOp,
+				Err: err,
 			}
 		}
 
@@ -524,8 +539,7 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile(
 	}
 
 	if res, err := ops.Update[T, TEnt](ctx, sdk, r.SyncPeriod, r.Client, ent); err != nil {
-		ent.GetKonnectStatus().ServerURL = apiAuth.Spec.ServerURL
-		ent.GetKonnectStatus().OrgID = apiAuth.Status.OrganizationID
+		setServerURLAndOrgIDFromAPIAuthConfiguration(ent, apiAuth)
 		if errUpd := r.Client.Status().Update(ctx, ent); errUpd != nil {
 			if k8serrors.IsConflict(errUpd) {
 				return ctrl.Result{Requeue: true}, nil
@@ -538,8 +552,7 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile(
 		return res, nil
 	}
 
-	ent.GetKonnectStatus().ServerURL = apiAuth.Spec.ServerURL
-	ent.GetKonnectStatus().OrgID = apiAuth.Status.OrganizationID
+	setServerURLAndOrgIDFromAPIAuthConfiguration(ent, apiAuth)
 	if err := r.Client.Status().Update(ctx, ent); err != nil {
 		if k8serrors.IsConflict(err) {
 			return ctrl.Result{Requeue: true}, nil
@@ -555,6 +568,16 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile(
 	}, nil
 }
 
+func setServerURLAndOrgIDFromAPIAuthConfiguration(
+	ent interface {
+		GetKonnectStatus() *konnectv1alpha1.KonnectEntityStatus
+	},
+	apiAuth konnectv1alpha1.KonnectAPIAuthConfiguration,
+) {
+	ent.GetKonnectStatus().ServerURL = apiAuth.Spec.ServerURL
+	ent.GetKonnectStatus().OrgID = apiAuth.Status.OrganizationID
+}
+
 // EntityWithControlPlaneRef is an interface for entities that have a ControlPlaneRef.
 type EntityWithControlPlaneRef interface {
 	SetControlPlaneID(string)

diff --git a/controller/konnect/watch_konnectcontrolplane.go b/controller/konnect/watch_konnectcontrolplane.go
@@ -36,6 +36,38 @@ func KonnectGatewayControlPlaneReconciliationWatchOptions(
 				),
 			)
 		},
+		func(b *ctrl.Builder) *ctrl.Builder {
+			return b.Watches(
+				&konnectv1alpha1.KonnectGatewayControlPlane{},
+				handler.EnqueueRequestsFromMapFunc(
+					enqueueKonnectGatewayControlPlaneGroupForMembers(cl),
+				),
+			)
+		},
+	}
+}
+
+func enqueueKonnectGatewayControlPlaneGroupForMembers(
+	cl client.Client,
+) func(ctx context.Context, obj client.Object) []reconcile.Request {
+	return func(ctx context.Context, obj client.Object) []reconcile.Request {
+		cp, ok := obj.(*konnectv1alpha1.KonnectGatewayControlPlane)
+		if !ok {
+			return nil
+		}
+		var l konnectv1alpha1.KonnectGatewayControlPlaneList
+		if err := cl.List(ctx, &l,
+			// TODO: change this when cross namespace refs are allowed.
+			client.InNamespace(cp.GetNamespace()),
+			client.MatchingFields{
+				// List groups that this control plane is a member of.
+				IndexFieldKonnectGatewayControlPlaneGroupOnMembers: cp.Name,
+			},
+		); err != nil {
+			return nil
+		}
+
+		return objectListToReconcileRequests(l.Items)
 	}
 }