diff --git a/controller/konnect/ops/conditions.go b/controller/konnect/ops/conditions.go index 88bbb74dc..81a9eea7f 100644 --- a/controller/konnect/ops/conditions.go +++ b/controller/konnect/ops/conditions.go @@ -1,6 +1,7 @@ package ops import ( + sdkkonnectcomp "github.com/Kong/sdk-konnect-go/models/components" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/kong/gateway-operator/pkg/consts" @@ -60,3 +61,67 @@ func _setKonnectEntityProgrammedConditon( obj, ) } + +const ( + // ControlPlaneGroupMembersReferenceResolvedConditionType sets the condition for control plane groups + // to show whether all of its members are programmed and attached to the group. + ControlPlaneGroupMembersReferenceResolvedConditionType = "MembersReferenceResolved" + // ControlPlaneGroupMembersReferenceResolvedReasonNoMembers indicates that there are no members specified in the control plane group. + ControlPlaneGroupMembersReferenceResolvedReasonNoMembers consts.ConditionReason = "NoMembers" + // ControlPlaneGroupMembersReferenceResolvedReasonResolved indicates that all members of the control plane group + // are created and attached to the group in Konnect. + ControlPlaneGroupMembersReferenceResolvedReasonResolved consts.ConditionReason = "Resolved" + // ControlPlaneGroupMembersReferenceResolvedReasonPartialNotResolved indicates that some members of the control plane group + // are not resolved (not found or not created in Konnect). + ControlPlaneGroupMembersReferenceResolvedReasonPartialNotResolved consts.ConditionReason = "SomeMemberNotResolved" + // ControlPlaneGroupMembersReferenceResolvedReasonFailedToSet indicates that error happened on setting control plane as + // member of the control plane. + ControlPlaneGroupMembersReferenceResolvedReasonFailedToSet consts.ConditionReason = "SetGroupMemberFailed" +) + +// SetControlPlaneGroupMembersReferenceResolvedCondition sets MembersReferenceResolved condition of control plane to True. +func SetControlPlaneGroupMembersReferenceResolvedCondition( + cpGroup *konnectv1alpha1.KonnectGatewayControlPlane, +) { + _setControlPlaneGroupMembersReferenceResolvedCondition( + cpGroup, + metav1.ConditionTrue, + ControlPlaneGroupMembersReferenceResolvedReasonResolved, + "", + ) +} + +// SetControlPlaneGroupMembersReferenceResolvedConditionFalse sets MembersReferenceResolved condition of control plane to False. +func SetControlPlaneGroupMembersReferenceResolvedConditionFalse( + cpGroup *konnectv1alpha1.KonnectGatewayControlPlane, + reason consts.ConditionReason, + msg string, +) { + _setControlPlaneGroupMembersReferenceResolvedCondition( + cpGroup, + metav1.ConditionFalse, + reason, + msg, + ) +} + +func _setControlPlaneGroupMembersReferenceResolvedCondition( + cpGroup *konnectv1alpha1.KonnectGatewayControlPlane, + status metav1.ConditionStatus, + reason consts.ConditionReason, + msg string, +) { + if cpGroup.Spec.ClusterType == nil || *cpGroup.Spec.ClusterType != sdkkonnectcomp.CreateControlPlaneRequestClusterTypeClusterTypeControlPlaneGroup { + return + } + k8sutils.SetCondition( + k8sutils.NewConditionWithGeneration( + ControlPlaneGroupMembersReferenceResolvedConditionType, + status, + reason, + msg, + cpGroup.GetGeneration(), + ), + cpGroup, + ) +} diff --git a/controller/konnect/ops/errors.go b/controller/konnect/ops/errors.go index 7d1472e51..a547addf4 100644 --- a/controller/konnect/ops/errors.go +++ b/controller/konnect/ops/errors.go @@ -39,3 +39,32 @@ type KonnectEntityCreatedButRelationsFailedError struct { func (e KonnectEntityCreatedButRelationsFailedError) Error() string { return fmt.Sprintf("Konnect entity (ID: %s) created but relations failed: %s: %v", e.KonnectID, e.Reason, e.Err) } + +// GetControlPlaneGroupMemberFailedError is an error type returned when +// failed to get member of control plane group. +type GetControlPlaneGroupMemberFailedError struct { + MemberName string + Err error +} + +// Error implements the error interface. +func (e GetControlPlaneGroupMemberFailedError) Error() string { + return fmt.Sprintf("failed to get control plane group member %s: %v", e.MemberName, e.Err.Error()) +} + +// Unwrap returns the underlying error. +func (e GetControlPlaneGroupMemberFailedError) Unwrap() error { + return e.Err +} + +// ControlPlaneGroupMemberNoKonnectIDError is an error type returned when +// member of control plane group does not have a Konnect ID. +type ControlPlaneGroupMemberNoKonnectIDError struct { + GroupName string + MemberName string +} + +// Error implements the error interface. +func (e ControlPlaneGroupMemberNoKonnectIDError) Error() string { + return fmt.Sprintf("control plane group %s member %s has no Konnect ID", e.GroupName, e.MemberName) +} diff --git a/controller/konnect/ops/ops.go b/controller/konnect/ops/ops.go index 89e4cd0e1..1805754a3 100644 --- a/controller/konnect/ops/ops.go +++ b/controller/konnect/ops/ops.go @@ -109,7 +109,7 @@ func Create[ var id string switch ent := any(e).(type) { case *konnectv1alpha1.KonnectGatewayControlPlane: - id, err = getControlPlaneForUID(ctx, sdk.GetControlPlaneSDK(), ent) + id, err = getControlPlaneForUID(ctx, sdk.GetControlPlaneSDK(), sdk.GetControlPlaneGroupSDK(), cl, ent) case *configurationv1alpha1.KongService: id, err = getKongServiceForUID(ctx, sdk.GetServicesSDK(), ent) case *configurationv1alpha1.KongRoute: diff --git a/controller/konnect/ops/ops_controlplane.go b/controller/konnect/ops/ops_controlplane.go index 9c1a6e5fe..27784cb18 100644 --- a/controller/konnect/ops/ops_controlplane.go +++ b/controller/konnect/ops/ops_controlplane.go @@ -128,12 +128,21 @@ func setGroupMembers( id string, sdkGroups sdkops.ControlPlaneGroupSDK, ) error { - if len(cp.Spec.Members) == 0 || - cp.Spec.ClusterType == nil || + if cp.Spec.ClusterType == nil || *cp.Spec.ClusterType != sdkkonnectcomp.CreateControlPlaneRequestClusterTypeClusterTypeControlPlaneGroup { return nil } + // Set MembersReferenceResolved condition to False if there are no members in the CP group. + if len(cp.Spec.Members) == 0 { + SetControlPlaneGroupMembersReferenceResolvedConditionFalse( + cp, + ControlPlaneGroupMembersReferenceResolvedReasonNoMembers, + "No members in the control plane group", + ) + return nil + } + members, err := iter.MapErr(cp.Spec.Members, func(member *corev1.LocalObjectReference) (sdkkonnectcomp.Members, error) { var ( @@ -145,17 +154,28 @@ func setGroupMembers( ) if err := cl.Get(ctx, nn, &memberCP); err != nil { return sdkkonnectcomp.Members{}, - fmt.Errorf("failed to get control plane group member %s: %w", member.Name, err) + GetControlPlaneGroupMemberFailedError{ + MemberName: memberCP.Name, + Err: err, + } } if memberCP.GetKonnectID() == "" { return sdkkonnectcomp.Members{}, - fmt.Errorf("control plane group %s member %s has no Konnect ID", cp.Name, member.Name) + ControlPlaneGroupMemberNoKonnectIDError{ + GroupName: cp.Name, + MemberName: memberCP.Name, + } } return sdkkonnectcomp.Members{ ID: lo.ToPtr(memberCP.GetKonnectID()), }, nil }) if err != nil { + SetControlPlaneGroupMembersReferenceResolvedConditionFalse( + cp, + ControlPlaneGroupMembersReferenceResolvedReasonPartialNotResolved, + err.Error(), + ) return fmt.Errorf("failed to set group members, some members couldn't be found: %w", err) } @@ -165,11 +185,19 @@ func setGroupMembers( } _, err = sdkGroups.PutControlPlanesIDGroupMemberships(ctx, id, &gm) if err != nil { + SetControlPlaneGroupMembersReferenceResolvedConditionFalse( + cp, + ControlPlaneGroupMembersReferenceResolvedReasonFailedToSet, + err.Error(), + ) return fmt.Errorf("failed to set members on control plane group %s: %w", client.ObjectKeyFromObject(cp), err, ) } + SetControlPlaneGroupMembersReferenceResolvedCondition( + cp, + ) return nil } @@ -184,6 +212,8 @@ func (m membersByID) Swap(i, j int) { m[i], m[j] = m[j], m[i] } func getControlPlaneForUID( ctx context.Context, sdk sdkops.ControlPlaneSDK, + sdkGroups sdkops.ControlPlaneGroupSDK, + cl client.Client, cp *konnectv1alpha1.KonnectGatewayControlPlane, ) (string, error) { reqList := sdkkonnectops.ListControlPlanesRequest{ @@ -202,5 +232,20 @@ func getControlPlaneForUID( return "", fmt.Errorf("failed listing %s: %w", cp.GetTypeName(), ErrNilResponse) } - return getMatchingEntryFromListResponseData(sliceToEntityWithIDSlice(resp.ListControlPlanesResponse.Data), cp) + id, err := getMatchingEntryFromListResponseData(sliceToEntityWithIDSlice(resp.ListControlPlanesResponse.Data), cp) + if err != nil { + return "", err + } + + if err := setGroupMembers(ctx, cl, cp, id, sdkGroups); err != nil { + // If we failed to set group membership, we should return a specific error with a reason + // so the downstream can handle it properly. + return id, KonnectEntityCreatedButRelationsFailedError{ + KonnectID: id, + Err: err, + Reason: konnectv1alpha1.KonnectGatewayControlPlaneProgrammedReasonFailedToSetControlPlaneGroupMembers, + } + } + + return id, nil } diff --git a/controller/konnect/ops/ops_controlplane_test.go b/controller/konnect/ops/ops_controlplane_test.go index d16835186..5c3ad6c4c 100644 --- a/controller/konnect/ops/ops_controlplane_test.go +++ b/controller/konnect/ops/ops_controlplane_test.go @@ -22,6 +22,7 @@ import ( sdkmocks "github.com/kong/gateway-operator/controller/konnect/ops/sdk/mocks" "github.com/kong/gateway-operator/modules/manager/scheme" + "github.com/kong/gateway-operator/pkg/consts" konnectv1alpha1 "github.com/kong/kubernetes-configuration/api/konnect/v1alpha1" ) @@ -638,11 +639,13 @@ func TestCreateAndUpdateControlPlane_KubernetesMetadataConsistency(t *testing.T) func TestSetGroupMembers(t *testing.T) { testcases := []struct { - name string - group *konnectv1alpha1.KonnectGatewayControlPlane - cps []client.Object - sdk func(t *testing.T) *sdkmocks.MockControlPlaneGroupSDK - expectedErr bool + name string + group *konnectv1alpha1.KonnectGatewayControlPlane + cps []client.Object + sdk func(t *testing.T) *sdkmocks.MockControlPlaneGroupSDK + expectedErr bool + memberRefResolvedStatus metav1.ConditionStatus + memberRefResolvedReason consts.ConditionReason }{ { name: "no members", @@ -662,6 +665,8 @@ func TestSetGroupMembers(t *testing.T) { sdk := sdkmocks.NewMockControlPlaneGroupSDK(t) return sdk }, + memberRefResolvedStatus: metav1.ConditionFalse, + memberRefResolvedReason: ControlPlaneGroupMembersReferenceResolvedReasonNoMembers, }, { name: "1 member with Konnect Status ID", @@ -715,6 +720,8 @@ func TestSetGroupMembers(t *testing.T) { ) return sdk }, + memberRefResolvedStatus: metav1.ConditionTrue, + memberRefResolvedReason: ControlPlaneGroupMembersReferenceResolvedReasonResolved, }, { name: "1 member without Konnect Status ID", @@ -748,7 +755,9 @@ func TestSetGroupMembers(t *testing.T) { sdk := sdkmocks.NewMockControlPlaneGroupSDK(t) return sdk }, - expectedErr: true, + expectedErr: true, + memberRefResolvedStatus: metav1.ConditionFalse, + memberRefResolvedReason: ControlPlaneGroupMembersReferenceResolvedReasonPartialNotResolved, }, { name: "2 member with Konnect Status IDs", @@ -819,6 +828,8 @@ func TestSetGroupMembers(t *testing.T) { ) return sdk }, + memberRefResolvedStatus: metav1.ConditionTrue, + memberRefResolvedReason: ControlPlaneGroupMembersReferenceResolvedReasonResolved, }, { name: "2 member, 1 with Konnect Status IDs, 1 without it", @@ -865,7 +876,9 @@ func TestSetGroupMembers(t *testing.T) { sdk := sdkmocks.NewMockControlPlaneGroupSDK(t) return sdk }, - expectedErr: true, + expectedErr: true, + memberRefResolvedStatus: metav1.ConditionFalse, + memberRefResolvedReason: ControlPlaneGroupMembersReferenceResolvedReasonPartialNotResolved, }, } @@ -885,6 +898,13 @@ func TestSetGroupMembers(t *testing.T) { } assert.NoError(t, err) assert.True(t, sdk.AssertExpectations(t)) + + membersRefResolvedCondition, conditionFound := lo.Find(tc.group.Status.Conditions, func(c metav1.Condition) bool { + return c.Type == ControlPlaneGroupMembersReferenceResolvedConditionType + }) + assert.True(t, conditionFound, "Should find MembersReferenceResolved condition") + assert.Equal(t, tc.memberRefResolvedStatus, membersRefResolvedCondition.Status, "Should have expected MembersReferenceResolved status") + assert.Equal(t, string(tc.memberRefResolvedReason), membersRefResolvedCondition.Reason, "Should have expected MembersReferenceResolved reason") }) } } diff --git a/test/envtest/konnect_entities_gatewaycontrolplane_test.go b/test/envtest/konnect_entities_gatewaycontrolplane_test.go index 98843c704..62da0bf02 100644 --- a/test/envtest/konnect_entities_gatewaycontrolplane_test.go +++ b/test/envtest/konnect_entities_gatewaycontrolplane_test.go @@ -222,6 +222,9 @@ var konnectGatewayControlPlaneTestCases = []konnectEntityReconcilerTestCase{ assert.True(t, conditionsContainProgrammedTrue(cpGroup.Status.Conditions), "Programmed condition should be set and it status should be true", ) + assert.True(t, conditionsContainMembersRefResolvedTrue(cpGroup.Status.Conditions), + "MembersReferenceResolved 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", ) @@ -367,6 +370,9 @@ var konnectGatewayControlPlaneTestCases = []konnectEntityReconcilerTestCase{ 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, conditionsContainMembersRefResolvedFalse(cpGroup.Status.Conditions), + "MembersReferenceResolved condition should be set and it status should be false", + ) assert.True(t, controllerutil.ContainsFinalizer(cpGroup, konnect.KonnectCleanupFinalizer), "Finalizer should be set on control plane group", ) @@ -448,6 +454,122 @@ var konnectGatewayControlPlaneTestCases = []konnectEntityReconcilerTestCase{ ) }, }, + { + name: "receiving HTTP Conflict 409 on creation for creating control plane group should have members set", + 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-5" + cp.Spec.Name = "cp-5" + }, + ) + + deploy.KonnectGatewayControlPlane(t, ctx, cl, auth, + func(obj client.Object) { + cp := obj.(*konnectv1alpha1.KonnectGatewayControlPlane) + cp.Name = "cp-group-1" + cp.Spec.Name = "cp-group-1" + cp.Spec.ClusterType = lo.ToPtr(sdkkonnectcomp.CreateControlPlaneRequestClusterTypeClusterTypeControlPlaneGroup) + cp.Spec.Members = []corev1.LocalObjectReference{ + {Name: "cp-5"}, + } + }, + ) + }, + + mockExpectations: func(t *testing.T, sdk *sdkmocks.MockSDKWrapper, cl client.Client, ns *corev1.Namespace) { + sdk.ControlPlaneSDK.EXPECT(). + CreateControlPlane( + mock.Anything, + mock.MatchedBy(func(req sdkkonnectcomp.CreateControlPlaneRequest) bool { + return req.Name == "cp-5" + }), + ). + Return( + &sdkkonnectops.CreateControlPlaneResponse{ + ControlPlane: &sdkkonnectcomp.ControlPlane{ + ID: "123456", + }, + }, nil, + ) + + sdk.ControlPlaneSDK.EXPECT(). + CreateControlPlane( + mock.Anything, + mock.MatchedBy(func(req sdkkonnectcomp.CreateControlPlaneRequest) bool { + return req.Name == "cp-group-1" + }), + ). + Return( + nil, + &sdkkonnecterrs.ConflictError{}, + ) + + sdk.ControlPlaneSDK.EXPECT(). + ListControlPlanes( + mock.Anything, + mock.MatchedBy(func(r sdkkonnectops.ListControlPlanesRequest) bool { + var cp konnectv1alpha1.KonnectGatewayControlPlane + require.NoError(t, cl.Get(context.Background(), client.ObjectKey{Name: "cp-group-1"}, &cp)) + // On conflict, we list cps by UID and check if there is already one created. + return r.Labels != nil && *r.Labels == ops.KubernetesUIDLabelKey+":"+string(cp.UID) + }), + ). + Return( + &sdkkonnectops.ListControlPlanesResponse{ + ListControlPlanesResponse: &sdkkonnectcomp.ListControlPlanesResponse{ + Data: []sdkkonnectcomp.ControlPlane{ + { + ID: "group-123456", + }, + }, + }, + }, + nil, + ) + + sdk.ControlPlaneGroupSDK.EXPECT().PutControlPlanesIDGroupMemberships( + mock.Anything, + "group-123456", + &sdkkonnectcomp.GroupMembership{ + Members: []sdkkonnectcomp.Members{ + { + ID: lo.ToPtr("123456"), + }, + }, + }, + ).Return(&sdkkonnectops.PutControlPlanesIDGroupMembershipsResponse{}, nil) + + // verify that mock SDK is called as expected. + t.Cleanup(func() { + require.True(t, sdk.ControlPlaneSDK.AssertExpectations(t)) + }) + }, + + eventuallyPredicate: func(ctx context.Context, t *assert.CollectT, cl client.Client, ns *corev1.Namespace) { + cpGroup := &konnectv1alpha1.KonnectGatewayControlPlane{} + if !assert.NoError(t, + cl.Get(ctx, + k8stypes.NamespacedName{ + Namespace: ns.Name, + Name: "cp-group-1", + }, + cpGroup, + ), + ) { + return + } + + assert.Equal(t, "group-123456", cpGroup.Status.ID, "ID should be set") + assert.True(t, conditionsContainProgrammedTrue(cpGroup.Status.Conditions), + "Programmed condition should be set and its status should be true", + ) + assert.True(t, conditionsContainMembersRefResolvedTrue(cpGroup.Status.Conditions), + "MembersRefernceResolved condition should be set and its status should be true") + }, + }, } func conditionsContainProgrammed(conds []metav1.Condition, status metav1.ConditionStatus) bool { @@ -466,3 +588,20 @@ func conditionsContainProgrammedFalse(conds []metav1.Condition) bool { func conditionsContainProgrammedTrue(conds []metav1.Condition) bool { return conditionsContainProgrammed(conds, metav1.ConditionTrue) } + +func conditionsContainMembersRefResolved(conds []metav1.Condition, status metav1.ConditionStatus) bool { + return lo.ContainsBy(conds, + func(condition metav1.Condition) bool { + return condition.Type == ops.ControlPlaneGroupMembersReferenceResolvedConditionType && + condition.Status == status + }, + ) +} + +func conditionsContainMembersRefResolvedFalse(conds []metav1.Condition) bool { + return conditionsContainMembersRefResolved(conds, metav1.ConditionFalse) +} + +func conditionsContainMembersRefResolvedTrue(conds []metav1.Condition) bool { + return conditionsContainMembersRefResolved(conds, metav1.ConditionTrue) +}