From 9d4d8b85173a8292b6cdc7c7fb653d7f35edf11c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Tue, 8 Oct 2024 14:55:49 +0200 Subject: [PATCH 1/2] fix(konnect): fix reconciling entities when they got created on Konnect but an error was returned --- .../index_konnectgatewaycontrolplane.go | 31 ++++++++ controller/konnect/ops/ops.go | 8 +- controller/konnect/reconciler_generic.go | 74 ++++++++++++------- .../konnect/watch_konnectcontrolplane.go | 32 ++++++++ 4 files changed, 114 insertions(+), 31 deletions(-) diff --git a/controller/konnect/index_konnectgatewaycontrolplane.go b/controller/konnect/index_konnectgatewaycontrolplane.go index 89b4fcd18..b79931735 100644 --- a/controller/konnect/index_konnectgatewaycontrolplane.go +++ b/controller/konnect/index_konnectgatewaycontrolplane.go @@ -1,12 +1,16 @@ 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" ) @@ -14,6 +18,11 @@ const ( // 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 index 2188eb1d6..859f0bf51 100644 --- 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 index c6005858d..cf95b180a 100644 --- a/controller/konnect/reconciler_generic.go +++ b/controller/konnect/reconciler_generic.go @@ -483,39 +483,51 @@ 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 - } - 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, - } + // 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. + + 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 + // 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 finalizer: %w", err) + } + } + + if err != nil { + return ctrl.Result{}, ops.FailedKonnectOpError[T]{ + Op: ops.CreateOp, + Err: err, } } @@ -524,8 +536,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 +549,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 +565,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 index 99f328d0c..6e7b0db9c 100644 --- 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) } } From 65aa40e568bf956445cb6d2c465cd81d83830a0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Tue, 8 Oct 2024 17:18:28 +0200 Subject: [PATCH 2/2] tests(konnect): add test case --- controller/konnect/reconciler_generic.go | 33 ++-- ...nnect_entities_gatewaycontrolplane_test.go | 160 +++++++++++++++++- 2 files changed, 176 insertions(+), 17 deletions(-) diff --git a/controller/konnect/reconciler_generic.go b/controller/konnect/reconciler_generic.go index cf95b180a..1b557ab24 100644 --- a/controller/konnect/reconciler_generic.go +++ b/controller/konnect/reconciler_generic.go @@ -490,20 +490,6 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( // 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 == nil { - setServerURLAndOrgIDFromAPIAuthConfiguration(ent, apiAuth) - } - - // 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 after creating object: %w", err) - } - // 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 != "" { @@ -519,11 +505,28 @@ func (r *KonnectEntityReconciler[T, TEnt]) Reconcile( KonnectCleanupFinalizer, errUpd, err, ) } - return ctrl.Result{}, fmt.Errorf("failed to update finalizer %s: %w", KonnectCleanupFinalizer, errUpd) + return ctrl.Result{}, fmt.Errorf( + "failed to update finalizer %s: %w", + KonnectCleanupFinalizer, errUpd, + ) } } } + if err == nil { + setServerURLAndOrgIDFromAPIAuthConfiguration(ent, apiAuth) + } + + // 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 after creating object: %w", err) + } + if err != nil { return ctrl.Result{}, ops.FailedKonnectOpError[T]{ Op: ops.CreateOp, diff --git a/test/envtest/konnect_entities_gatewaycontrolplane_test.go b/test/envtest/konnect_entities_gatewaycontrolplane_test.go index b17c71eb8..bcc79789a 100644 --- a/test/envtest/konnect_entities_gatewaycontrolplane_test.go +++ b/test/envtest/konnect_entities_gatewaycontrolplane_test.go @@ -2,6 +2,7 @@ package envtest import ( "context" + "errors" "testing" sdkkonnectcomp "github.com/Kong/sdk-konnect-go/models/components" @@ -14,7 +15,9 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8stypes "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "github.com/kong/gateway-operator/controller/konnect" "github.com/kong/gateway-operator/controller/konnect/ops" "github.com/kong/gateway-operator/test/helpers/deploy" @@ -74,6 +77,9 @@ var konnectGatewayControlPlaneTestCases = []konnectEntityReconcilerTestCase{ assert.True(t, conditionsContainProgrammedTrue(cp.Status.Conditions), "Programmed condition should be set and it status should be true", ) + assert.True(t, controllerutil.ContainsFinalizer(cp, konnect.KonnectCleanupFinalizer), + "Finalizer should be set on control plane group", + ) }, }, { @@ -188,6 +194,9 @@ var konnectGatewayControlPlaneTestCases = []konnectEntityReconcilerTestCase{ assert.True(t, conditionsContainProgrammedTrue(cp.Status.Conditions), "Programmed condition should be set and it status should be true", ) + assert.True(t, controllerutil.ContainsFinalizer(cp, konnect.KonnectCleanupFinalizer), + "Finalizer should be set on control plane", + ) cpGroup := &konnectv1alpha1.KonnectGatewayControlPlane{} if !assert.NoError(t, @@ -206,15 +215,162 @@ var konnectGatewayControlPlaneTestCases = []konnectEntityReconcilerTestCase{ assert.True(t, conditionsContainProgrammedTrue(cpGroup.Status.Conditions), "Programmed condition should be set and it status should be true", ) + assert.True(t, controllerutil.ContainsFinalizer(cpGroup, konnect.KonnectCleanupFinalizer), + "Finalizer should be set on control plane group", + ) + }, + }, + { + name: "control plane group with members when receiving an error from PutControlPlanesIDGroupMemberships, correctly sets the ID and finalizer on group", + objectOps: func(ctx context.Context, t *testing.T, cl client.Client, ns *corev1.Namespace) { + auth := deploy.KonnectAPIAuthConfigurationWithProgrammed(t, ctx, cl) + deploy.KonnectGatewayControlPlane(t, ctx, cl, auth, + func(obj client.Object) { + cp := obj.(*konnectv1alpha1.KonnectGatewayControlPlane) + cp.Name = "cp-groupmember-2" + cp.Spec.Name = "cp-groupmember-2" + }, + ) + deploy.KonnectGatewayControlPlane(t, ctx, cl, auth, + func(obj client.Object) { + cp := obj.(*konnectv1alpha1.KonnectGatewayControlPlane) + cp.Name = "cp-3" + cp.Spec.Name = "cp-3" + cp.Spec.ClusterType = lo.ToPtr(sdkkonnectcomp.ClusterTypeClusterTypeControlPlaneGroup) + cp.Spec.Members = []corev1.LocalObjectReference{ + { + Name: "cp-groupmember-2", + }, + } + }, + ) + }, + mockExpectations: func(t *testing.T, sdk *ops.MockSDKWrapper, ns *corev1.Namespace) { + sdk.ControlPlaneSDK.EXPECT(). + CreateControlPlane( + mock.Anything, + mock.MatchedBy(func(req sdkkonnectcomp.CreateControlPlaneRequest) bool { + return req.Name == "cp-groupmember-2" + }), + ). + Return( + &sdkkonnectops.CreateControlPlaneResponse{ + ControlPlane: &sdkkonnectcomp.ControlPlane{ + ID: lo.ToPtr("12345"), + }, + }, + nil, + ) + sdk.ControlPlaneSDK.EXPECT(). + CreateControlPlane( + mock.Anything, + mock.MatchedBy(func(req sdkkonnectcomp.CreateControlPlaneRequest) bool { + return req.Name == "cp-3" && + req.ClusterType != nil && + *req.ClusterType == sdkkonnectcomp.ClusterTypeClusterTypeControlPlaneGroup + }), + ). + Return( + &sdkkonnectops.CreateControlPlaneResponse{ + ControlPlane: &sdkkonnectcomp.ControlPlane{ + ID: lo.ToPtr("123467"), + }, + }, + nil, + ) + + sdk.ControlPlaneGroupSDK.EXPECT(). + PutControlPlanesIDGroupMemberships( + mock.Anything, + "123467", + &sdkkonnectcomp.GroupMembership{ + Members: []sdkkonnectcomp.Members{ + { + ID: lo.ToPtr("12345"), + }, + }, + }, + ). + Return( + &sdkkonnectops.PutControlPlanesIDGroupMembershipsResponse{}, + errors.New("some error"), + ) + + sdk.ControlPlaneSDK.EXPECT(). + UpdateControlPlane( + mock.Anything, + "123467", + mock.MatchedBy(func(req sdkkonnectcomp.UpdateControlPlaneRequest) bool { + return req.Name != nil && *req.Name == "cp-3" + }), + ). + Return( + &sdkkonnectops.UpdateControlPlaneResponse{ + ControlPlane: &sdkkonnectcomp.ControlPlane{ + ID: lo.ToPtr("123467"), + }, + }, + nil) + // verify that mock SDK is called as expected. + t.Cleanup(func() { + require.True(t, sdk.ControlPlaneSDK.AssertExpectations(t)) + require.True(t, sdk.ControlPlaneGroupSDK.AssertExpectations(t)) + }) + }, + eventuallyPredicate: func(ctx context.Context, t *assert.CollectT, cl client.Client, ns *corev1.Namespace) { + cp := &konnectv1alpha1.KonnectGatewayControlPlane{} + if !assert.NoError(t, + cl.Get(ctx, + k8stypes.NamespacedName{ + Namespace: ns.Name, + Name: "cp-groupmember-2", + }, + cp, + ), + ) { + return + } + assert.True(t, controllerutil.ContainsFinalizer(cp, konnect.KonnectCleanupFinalizer), + "Finalizer should be set on control plane", + ) + + cpGroup := &konnectv1alpha1.KonnectGatewayControlPlane{} + if !assert.NoError(t, + cl.Get(ctx, + k8stypes.NamespacedName{ + Namespace: ns.Name, + Name: "cp-3", + }, + cpGroup, + ), + ) { + return + } + + assert.Equal(t, "123467", cpGroup.Status.ID) + assert.True(t, conditionsContainProgrammedFalse(cpGroup.Status.Conditions), + "Programmed condition should be set and its status should be false because of an error returned by Konnect API when setting group members", + ) + assert.True(t, controllerutil.ContainsFinalizer(cpGroup, konnect.KonnectCleanupFinalizer), + "Finalizer should be set on control plane group", + ) }, }, } -func conditionsContainProgrammedTrue(conds []metav1.Condition) bool { +func conditionsContainProgrammed(conds []metav1.Condition, status metav1.ConditionStatus) bool { return lo.ContainsBy(conds, func(condition metav1.Condition) bool { return condition.Type == konnectv1alpha1.KonnectEntityProgrammedConditionType && - condition.Status == metav1.ConditionTrue + condition.Status == status }, ) } + +func conditionsContainProgrammedFalse(conds []metav1.Condition) bool { + return conditionsContainProgrammed(conds, metav1.ConditionFalse) +} + +func conditionsContainProgrammedTrue(conds []metav1.Condition) bool { + return conditionsContainProgrammed(conds, metav1.ConditionTrue) +}