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) +}