Fix: Add MembersReferenceResolved condition for cp groups (#824)

* add MembersReferenceResolved condition for cp groups * add consts and unit tests * add setGroupMembers in getControlPlaneByUID * add envtest for MemberRefResolved
Kong · Oct 30, 2024 · 8452e4a · 8452e4a
1 parent e5220bc
commit 8452e4a
Show file tree

Hide file tree

Showing 6 changed files with 311 additions and 13 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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")
 		})
 	}
 }