From fc11c4ef33cde4515cb7611f00c04b7e15428206 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Thu, 17 Oct 2024 17:35:49 +0200 Subject: [PATCH] fix: handle KongService conflict on creation --- controller/konnect/ops/kongservice.go | 1 + controller/konnect/ops/kongservice_mock.go | 74 +++++++++ controller/konnect/ops/ops.go | 32 +++- controller/konnect/ops/ops_controlplane.go | 23 ++- .../konnect/ops/ops_controlplane_test.go | 2 +- controller/konnect/ops/ops_errors.go | 62 +++++++- controller/konnect/ops/ops_kongservice.go | 65 ++++++-- .../konnect/ops/ops_kongservice_test.go | 96 +++++++----- ...onnect_entities_kongcacertificate_test.go} | 0 ... konnect_entities_kongcertificate_test.go} | 0 ...es_kongdataplaneclientcertificate_test.go} | 0 ...st.go => konnect_entities_kongkey_test.go} | 0 ...go => konnect_entities_kongkeyset_test.go} | 0 .../konnect_entities_kongservice_test.go | 146 ++++++++++++++++++ ...st.go => konnect_entities_kongsni_test.go} | 0 ...go => konnect_entities_kongtarget_test.go} | 1 - 16 files changed, 431 insertions(+), 71 deletions(-) rename test/envtest/{konnect_entities_cacertificate_test.go => konnect_entities_kongcacertificate_test.go} (100%) rename test/envtest/{konnect_entities_certificate_test.go => konnect_entities_kongcertificate_test.go} (100%) rename test/envtest/{konnect_entities_dataplaneclientcertificate_test.go => konnect_entities_kongdataplaneclientcertificate_test.go} (100%) rename test/envtest/{konnect_entities_key_test.go => konnect_entities_kongkey_test.go} (100%) rename test/envtest/{konnect_entities_keyset_test.go => konnect_entities_kongkeyset_test.go} (100%) create mode 100644 test/envtest/konnect_entities_kongservice_test.go rename test/envtest/{konnect_entities_sni_test.go => konnect_entities_kongsni_test.go} (100%) rename test/envtest/{konnect_entities_target_test.go => konnect_entities_kongtarget_test.go} (99%) diff --git a/controller/konnect/ops/kongservice.go b/controller/konnect/ops/kongservice.go index 502ae5649..f5a7702eb 100644 --- a/controller/konnect/ops/kongservice.go +++ b/controller/konnect/ops/kongservice.go @@ -12,4 +12,5 @@ type ServicesSDK interface { CreateService(ctx context.Context, controlPlaneID string, service sdkkonnectcomp.ServiceInput, opts ...sdkkonnectops.Option) (*sdkkonnectops.CreateServiceResponse, error) UpsertService(ctx context.Context, req sdkkonnectops.UpsertServiceRequest, opts ...sdkkonnectops.Option) (*sdkkonnectops.UpsertServiceResponse, error) DeleteService(ctx context.Context, controlPlaneID, serviceID string, opts ...sdkkonnectops.Option) (*sdkkonnectops.DeleteServiceResponse, error) + ListService(ctx context.Context, request sdkkonnectops.ListServiceRequest, opts ...sdkkonnectops.Option) (*sdkkonnectops.ListServiceResponse, error) } diff --git a/controller/konnect/ops/kongservice_mock.go b/controller/konnect/ops/kongservice_mock.go index 9340df0f0..166b64bc0 100644 --- a/controller/konnect/ops/kongservice_mock.go +++ b/controller/konnect/ops/kongservice_mock.go @@ -175,6 +175,80 @@ func (_c *MockServicesSDK_DeleteService_Call) RunAndReturn(run func(context.Cont return _c } +// ListService provides a mock function with given fields: ctx, request, opts +func (_m *MockServicesSDK) ListService(ctx context.Context, request operations.ListServiceRequest, opts ...operations.Option) (*operations.ListServiceResponse, error) { + _va := make([]interface{}, len(opts)) + for _i := range opts { + _va[_i] = opts[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, request) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + if len(ret) == 0 { + panic("no return value specified for ListService") + } + + var r0 *operations.ListServiceResponse + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, operations.ListServiceRequest, ...operations.Option) (*operations.ListServiceResponse, error)); ok { + return rf(ctx, request, opts...) + } + if rf, ok := ret.Get(0).(func(context.Context, operations.ListServiceRequest, ...operations.Option) *operations.ListServiceResponse); ok { + r0 = rf(ctx, request, opts...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*operations.ListServiceResponse) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, operations.ListServiceRequest, ...operations.Option) error); ok { + r1 = rf(ctx, request, opts...) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// MockServicesSDK_ListService_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ListService' +type MockServicesSDK_ListService_Call struct { + *mock.Call +} + +// ListService is a helper method to define mock.On call +// - ctx context.Context +// - request operations.ListServiceRequest +// - opts ...operations.Option +func (_e *MockServicesSDK_Expecter) ListService(ctx interface{}, request interface{}, opts ...interface{}) *MockServicesSDK_ListService_Call { + return &MockServicesSDK_ListService_Call{Call: _e.mock.On("ListService", + append([]interface{}{ctx, request}, opts...)...)} +} + +func (_c *MockServicesSDK_ListService_Call) Run(run func(ctx context.Context, request operations.ListServiceRequest, opts ...operations.Option)) *MockServicesSDK_ListService_Call { + _c.Call.Run(func(args mock.Arguments) { + variadicArgs := make([]operations.Option, len(args)-2) + for i, a := range args[2:] { + if a != nil { + variadicArgs[i] = a.(operations.Option) + } + } + run(args[0].(context.Context), args[1].(operations.ListServiceRequest), variadicArgs...) + }) + return _c +} + +func (_c *MockServicesSDK_ListService_Call) Return(_a0 *operations.ListServiceResponse, _a1 error) *MockServicesSDK_ListService_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *MockServicesSDK_ListService_Call) RunAndReturn(run func(context.Context, operations.ListServiceRequest, ...operations.Option) (*operations.ListServiceResponse, error)) *MockServicesSDK_ListService_Call { + _c.Call.Return(run) + return _c +} + // UpsertService provides a mock function with given fields: ctx, req, opts func (_m *MockServicesSDK) UpsertService(ctx context.Context, req operations.UpsertServiceRequest, opts ...operations.Option) (*operations.UpsertServiceResponse, error) { _va := make([]interface{}, len(opts)) diff --git a/controller/konnect/ops/ops.go b/controller/konnect/ops/ops.go index 9135b8e49..457f269a8 100644 --- a/controller/konnect/ops/ops.go +++ b/controller/konnect/ops/ops.go @@ -58,10 +58,12 @@ func Create[ switch ent := any(e).(type) { case *konnectv1alpha1.KonnectGatewayControlPlane: err = createControlPlane(ctx, sdk.GetControlPlaneSDK(), sdk.GetControlPlaneGroupSDK(), cl, ent) - // TODO: modify the create* operation wrappers to not set Programmed conditions and return - // a KonnectEntityCreatedButRelationsFailedError if the entity was created but its relations assignment failed. case *configurationv1alpha1.KongService: err = createService(ctx, sdk.GetServicesSDK(), ent) + + // TODO: modify the create* operation wrappers to not set Programmed conditions and return + // a KonnectEntityCreatedButRelationsFailedError if the entity was created but its relations assignment failed. + case *configurationv1alpha1.KongRoute: err = createRoute(ctx, sdk.GetRoutesSDK(), ent) case *configurationv1.KongConsumer: @@ -104,19 +106,32 @@ func Create[ return nil, fmt.Errorf("unsupported entity type %T", ent) } + // NOTE: Konnect APIs specific for Konnect only APIs like Gateway Control Planes + // return 409 conflict for already existing entities. APIs that are shared with + // Kong Admin API return 400 for conflicts. var ( errConflict *sdkkonnecterrs.ConflictError errRelationsFailed KonnectEntityCreatedButRelationsFailedError + errSdk *sdkkonnecterrs.SDKError + errSdkBody sdkErrorBody ) + if err != nil && (errors.As(err, &errSdk)) { + errSdkBody, _ = ParseSDKErrorBody(errSdk.Body) + } + switch { - case errors.As(err, &errConflict): + case errors.As(err, &errConflict) || + // Service API returns 400 for conflicts + errSdkBody.Code == 3 && errSdkBody.Message == "data constraint error": // If there was a conflict on the create request, we can assume the entity already exists. // We'll get its Konnect ID by listing all entities of its type filtered by the Kubernetes object UID. var id string switch ent := any(e).(type) { case *konnectv1alpha1.KonnectGatewayControlPlane: id, err = getControlPlaneForUID(ctx, sdk.GetControlPlaneSDK(), ent) + case *configurationv1alpha1.KongService: + id, err = getKongServiceForUID(ctx, sdk.GetServicesSDK(), ent) // --------------------------------------------------------------------- // TODO: add other Konnect types default: @@ -388,3 +403,14 @@ func wrapErrIfKonnectOpFailed[ } return nil } + +func logEntityNotFoundRecreating[ + T constraints.SupportedKonnectEntityType, +](ctx context.Context, _ *T, id string) { + ctrllog.FromContext(ctx). + Info( + "entity not found in Konnect, trying to recreate", + "type", constraints.EntityTypeName[T](), + "id", id, + ) +} diff --git a/controller/konnect/ops/ops_controlplane.go b/controller/konnect/ops/ops_controlplane.go index c2fe643ed..612204adb 100644 --- a/controller/konnect/ops/ops_controlplane.go +++ b/controller/konnect/ops/ops_controlplane.go @@ -43,15 +43,17 @@ func getControlPlaneForUID( } var id string - for _, listCP := range respList.ListControlPlanesResponse.Data { - if listCP.ID != nil && *listCP.ID != "" { - id = *listCP.ID + for _, entry := range respList.ListControlPlanesResponse.Data { + if entry.ID != nil && *entry.ID != "" { + id = *entry.ID break } } if id == "" { - return "", fmt.Errorf("control plane %s (matching UID %s) not found", cp.Name, cp.UID) + return "", EntityWithMatchingUIDNotFoundError{ + Entity: cp, + } } return id, nil @@ -72,16 +74,16 @@ func createControlPlane( req.Labels = WithKubernetesMetadataLabels(cp, req.Labels) resp, err := sdk.CreateControlPlane(ctx, req) - // TODO: handle already exists + // Can't adopt it as it will cause conflicts between the controller - // that created that entity and already manages it, hm + // that created that entity and already manages it. // TODO: implement entity adoption https://github.com/Kong/gateway-operator/issues/460 if errWrap := wrapErrIfKonnectOpFailed(err, CreateOp, cp); errWrap != nil { return errWrap } - if resp == nil || resp.ControlPlane == nil { - return fmt.Errorf("failed creating ControlPlane: %w", ErrNilResponse) + if resp == nil || resp.ControlPlane == nil || resp.ControlPlane.ID == nil { + return fmt.Errorf("failed creating %s: %w", cp.GetTypeName(), ErrNilResponse) } // At this point, the ControlPlane has been created in Konnect. @@ -159,10 +161,7 @@ func updateControlPlane( resp, err := sdk.UpdateControlPlane(ctx, id, req) var sdkError *sdkkonnecterrs.NotFoundError if errors.As(err, &sdkError) { - ctrllog.FromContext(ctx). - Info("entity not found in Konnect, trying to recreate", - "type", cp.GetTypeName(), "id", id, - ) + logEntityNotFoundRecreating(ctx, cp, id) if err := createControlPlane(ctx, sdk, sdkGroups, cl, cp); err != nil { return FailedKonnectOpError[konnectv1alpha1.KonnectGatewayControlPlane]{ Op: UpdateOp, diff --git a/controller/konnect/ops/ops_controlplane_test.go b/controller/konnect/ops/ops_controlplane_test.go index de5e1dbd0..d05b94374 100644 --- a/controller/konnect/ops/ops_controlplane_test.go +++ b/controller/konnect/ops/ops_controlplane_test.go @@ -257,7 +257,7 @@ func TestCreateControlPlane(t *testing.T) { err := createControlPlane(ctx, sdk, sdkGroups, fakeClient, cp) if tc.expectedErrContains != "" { if tc.expectedErrType != nil { - assert.IsType(t, err, tc.expectedErrType) + assert.ErrorAs(t, err, &tc.expectedErrType) } assert.ErrorContains(t, err, tc.expectedErrContains) return diff --git a/controller/konnect/ops/ops_errors.go b/controller/konnect/ops/ops_errors.go index 1921ba90d..6218c4f65 100644 --- a/controller/konnect/ops/ops_errors.go +++ b/controller/konnect/ops/ops_errors.go @@ -1,7 +1,67 @@ package ops -import "errors" +import ( + "encoding/json" + "errors" + "fmt" + + "k8s.io/apimachinery/pkg/types" +) // ErrNilResponse is an error indicating that a Konnect operation returned an empty response. // This can sometimes happen regardless of the err being nil. var ErrNilResponse = errors.New("nil response received") + +// EntityWithMatchingUIDNotFoundError is an error indicating that an entity with a matching UID was not found on Konnect. +type EntityWithMatchingUIDNotFoundError struct { + Entity interface { + GetTypeName() string + GetName() string + GetUID() types.UID + } +} + +// Error implements the error interface. +func (e EntityWithMatchingUIDNotFoundError) Error() string { + return fmt.Sprintf( + "%s %s (matching UID %s) not found on Konnect", + e.Entity.GetTypeName(), e.Entity.GetName(), e.Entity.GetUID(), + ) +} + +type sdkErrorBody struct { + Code int `json:"code"` + Message string `json:"message"` + Details []struct { + TypeAt string `json:"@type"` + Type string `json:"type"` + Field string `json:"field"` + Messages []string `json:"messages"` + } `json:"details"` +} + +// ParseSDKErrorBody parses the body of an SDK error response. +// Exemplary body: +// +// { +// "code": 3, +// "message": "data constraint error", +// "details": [ +// { +// "@type": "type.googleapis.com/kong.admin.model.v1.ErrorDetail", +// "type": "ERROR_TYPE_REFERENCE", +// "field": "name", +// "messages": [ +// "name (type: unique) constraint failed" +// ] +// } +// ] +// } +func ParseSDKErrorBody(body string) (sdkErrorBody, error) { + var sdkErr sdkErrorBody + if err := json.Unmarshal([]byte(body), &sdkErr); err != nil { + return sdkErr, err + } + + return sdkErr, nil +} diff --git a/controller/konnect/ops/ops_kongservice.go b/controller/konnect/ops/ops_kongservice.go index 4126594d1..68364e7c7 100644 --- a/controller/konnect/ops/ops_kongservice.go +++ b/controller/konnect/ops/ops_kongservice.go @@ -8,12 +8,54 @@ import ( sdkkonnectcomp "github.com/Kong/sdk-konnect-go/models/components" sdkkonnectops "github.com/Kong/sdk-konnect-go/models/operations" sdkkonnecterrs "github.com/Kong/sdk-konnect-go/models/sdkerrors" + "github.com/samber/lo" "sigs.k8s.io/controller-runtime/pkg/client" ctrllog "sigs.k8s.io/controller-runtime/pkg/log" configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" ) +// getKongServiceForUID returns the Konnect ID of the KongService +// that matches the UID of the provided KongService. +func getKongServiceForUID( + ctx context.Context, + sdk ServicesSDK, + svc *configurationv1alpha1.KongService, +) (string, error) { + reqList := sdkkonnectops.ListServiceRequest{ + // NOTE: only filter on object's UID. + // Other fields like name might have changed in the meantime but that's OK. + // Those will be enforced via subsequent updates. + Tags: lo.ToPtr(UIDLabelForObject(svc)), + ControlPlaneID: svc.GetControlPlaneID(), + } + + respList, err := sdk.ListService(ctx, reqList) + if err != nil { + return "", err + } + + if respList == nil || respList.Object == nil { + return "", fmt.Errorf("failed listing KongServices: %w", ErrNilResponse) + } + + var id string + for _, entry := range respList.Object.Data { + if entry.ID != nil && *entry.ID != "" { + id = *entry.ID + break + } + } + + if id == "" { + return "", EntityWithMatchingUIDNotFoundError{ + Entity: svc, + } + } + + return id, nil +} + func createService( ctx context.Context, sdk ServicesSDK, @@ -31,16 +73,20 @@ func createService( kongServiceToSDKServiceInput(svc), ) - // TODO: handle already exists // Can't adopt it as it will cause conflicts between the controller - // that created that entity and already manages it, hm + // that created that entity and already manages it. + // TODO: implement entity adoption https://github.com/Kong/gateway-operator/issues/460 if errWrap := wrapErrIfKonnectOpFailed(err, CreateOp, svc); errWrap != nil { - SetKonnectEntityProgrammedConditionFalse(svc, "FailedToCreate", errWrap.Error()) return errWrap } - svc.Status.Konnect.SetKonnectID(*resp.Service.ID) - SetKonnectEntityProgrammedCondition(svc) + if resp == nil || resp.Service == nil || resp.Service.ID == nil { + return fmt.Errorf("failed creating %s: %w", svc.GetTypeName(), ErrNilResponse) + } + + // At this point, the ControlPlane has been created in Konnect. + id := *resp.Service.ID + svc.SetKonnectID(id) return nil } @@ -55,7 +101,7 @@ func updateService( svc *configurationv1alpha1.KongService, ) error { if svc.GetControlPlaneID() == "" { - return fmt.Errorf("can't update %T %s without a Konnect ControlPlane ID", + return fmt.Errorf("can't update %T %s without a Konnect ControlPlane ID", svc, client.ObjectKeyFromObject(svc), ) } @@ -78,8 +124,8 @@ func updateService( if errors.As(errWrap, &sdkError) { switch sdkError.StatusCode { case 404: - err := createService(ctx, sdk, svc) - if err != nil { + logEntityNotFoundRecreating(ctx, svc, id) + if err := createService(ctx, sdk, svc); err != nil { return FailedKonnectOpError[configurationv1alpha1.KongService]{ Op: UpdateOp, Err: err, @@ -95,12 +141,9 @@ func updateService( } } - SetKonnectEntityProgrammedConditionFalse(svc, "FailedToUpdate", errWrap.Error()) return errWrap } - SetKonnectEntityProgrammedCondition(svc) - return nil } diff --git a/controller/konnect/ops/ops_kongservice_test.go b/controller/konnect/ops/ops_kongservice_test.go index a43161e1a..b17ad1ff6 100644 --- a/controller/konnect/ops/ops_kongservice_test.go +++ b/controller/konnect/ops/ops_kongservice_test.go @@ -15,7 +15,6 @@ import ( k8stypes "k8s.io/apimachinery/pkg/types" konnectconsts "github.com/kong/gateway-operator/controller/konnect/consts" - k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" konnectv1alpha1 "github.com/kong/kubernetes-configuration/api/konnect/v1alpha1" @@ -24,10 +23,11 @@ import ( func TestCreateKongService(t *testing.T) { ctx := context.Background() testCases := []struct { - name string - mockServicePair func(*testing.T) (*MockServicesSDK, *configurationv1alpha1.KongService) - expectedErr bool - assertions func(*testing.T, *configurationv1alpha1.KongService) + name string + mockServicePair func(*testing.T) (*MockServicesSDK, *configurationv1alpha1.KongService) + assertions func(*testing.T, *configurationv1alpha1.KongService) + expectedErrContains string + expectedErrType error }{ { name: "success", @@ -69,11 +69,6 @@ func TestCreateKongService(t *testing.T) { }, assertions: func(t *testing.T, svc *configurationv1alpha1.KongService) { assert.Equal(t, "12345", svc.GetKonnectStatus().GetKonnectID()) - cond, ok := k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, svc) - require.True(t, ok, "Programmed condition not set on KongService") - assert.Equal(t, metav1.ConditionTrue, cond.Status) - assert.Equal(t, konnectv1alpha1.KonnectEntityProgrammedReasonProgrammed, cond.Reason) - assert.Equal(t, svc.GetGeneration(), cond.ObservedGeneration) }, }, { @@ -99,7 +94,7 @@ func TestCreateKongService(t *testing.T) { assert.Equal(t, "", svc.GetKonnectStatus().GetKonnectID()) // TODO: we should probably set a condition when the control plane ID is missing in the status. }, - expectedErr: true, + expectedErrContains: `can't create *v1alpha1.KongService default/svc-1 without a Konnect ControlPlane ID`, }, { name: "fail", @@ -138,14 +133,46 @@ func TestCreateKongService(t *testing.T) { }, assertions: func(t *testing.T, svc *configurationv1alpha1.KongService) { assert.Equal(t, "", svc.GetKonnectStatus().GetKonnectID()) - cond, ok := k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, svc) - require.True(t, ok, "Programmed condition not set on KonnectGatewayControlPlane") - assert.Equal(t, metav1.ConditionFalse, cond.Status) - assert.Equal(t, "FailedToCreate", cond.Reason) - assert.Equal(t, svc.GetGeneration(), cond.ObservedGeneration) - assert.Equal(t, `failed to create KongService default/svc-1: {"status":400,"title":"","instance":"","detail":"bad request","invalid_parameters":null}`, cond.Message) }, - expectedErr: true, + expectedErrContains: "failed to create KongService default/svc-1: {\"status\":400,\"title\":\"\",\"instance\":\"\",\"detail\":\"bad request\",\"invalid_parameters\":null}", + }, + { + name: "409 Conflict causes a list to find a matching (by UID) service and update it instead of creating a new one", + mockServicePair: func(t *testing.T) (*MockServicesSDK, *configurationv1alpha1.KongService) { + sdk := NewMockServicesSDK(t) + svc := &configurationv1alpha1.KongService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-1", + Namespace: "default", + }, + Spec: configurationv1alpha1.KongServiceSpec{ + KongServiceAPISpec: configurationv1alpha1.KongServiceAPISpec{ + Name: lo.ToPtr("svc-1"), + Host: "example.com", + }, + }, + Status: configurationv1alpha1.KongServiceStatus{ + Konnect: &konnectv1alpha1.KonnectEntityStatusWithControlPlaneRef{ + ControlPlaneID: "123456789", + }, + }, + } + + sdk. + EXPECT(). + CreateService(ctx, "123456789", kongServiceToSDKServiceInput(svc)). + Return( + nil, + &sdkkonnecterrs.ConflictError{ + Status: 409, + Detail: "Conflict", + }, + ) + + return sdk, svc + }, + expectedErrType: &sdkkonnecterrs.ConflictError{}, + expectedErrContains: "failed to create KongService default/svc-1: {\"status\":409,\"title\":null,\"instance\":null,\"detail\":\"Conflict\"}", }, } @@ -155,10 +182,15 @@ func TestCreateKongService(t *testing.T) { err := createService(ctx, sdk, svc) - tc.assertions(t, svc) + if tc.assertions != nil { + tc.assertions(t, svc) + } - if tc.expectedErr { - assert.Error(t, err) + if tc.expectedErrContains != "" { + assert.ErrorContains(t, err, tc.expectedErrContains) + if tc.expectedErrType != nil { + assert.ErrorAs(t, err, &tc.expectedErrType) + } return } @@ -347,12 +379,6 @@ func TestUpdateKongService(t *testing.T) { }, assertions: func(t *testing.T, svc *configurationv1alpha1.KongService) { assert.Equal(t, "123456789", svc.GetKonnectStatus().GetKonnectID()) - cond, ok := k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, svc) - require.True(t, ok, "Programmed condition not set on KonnectGatewayControlPlane") - assert.Equal(t, metav1.ConditionTrue, cond.Status) - assert.Equal(t, konnectv1alpha1.KonnectEntityProgrammedReasonProgrammed, cond.Reason) - assert.Equal(t, svc.GetGeneration(), cond.ObservedGeneration) - assert.Equal(t, "", cond.Message) }, }, { @@ -398,15 +424,7 @@ func TestUpdateKongService(t *testing.T) { return sdk, svc }, assertions: func(t *testing.T, svc *configurationv1alpha1.KongService) { - // TODO: When we fail to update a KongService, do we want to clear - // the Konnect ID from the status? Probably not. - // assert.Equal(t, "", svc.GetKonnectStatus().GetKonnectID()) - cond, ok := k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, svc) - require.True(t, ok, "Programmed condition not set on KonnectGatewayControlPlane") - assert.Equal(t, metav1.ConditionFalse, cond.Status) - assert.Equal(t, "FailedToUpdate", cond.Reason) - assert.Equal(t, svc.GetGeneration(), cond.ObservedGeneration) - assert.Equal(t, `failed to update KongService default/svc-1: {"status":400,"title":"bad request","instance":"","detail":"","invalid_parameters":null}`, cond.Message) + assert.Equal(t, "123456789", svc.GetKonnectStatus().GetKonnectID()) }, expectedErr: true, }, @@ -467,12 +485,6 @@ func TestUpdateKongService(t *testing.T) { }, assertions: func(t *testing.T, svc *configurationv1alpha1.KongService) { assert.Equal(t, "123456789", svc.GetKonnectStatus().GetKonnectID()) - cond, ok := k8sutils.GetCondition(konnectv1alpha1.KonnectEntityProgrammedConditionType, svc) - require.True(t, ok, "Programmed condition not set on KonnectGatewayControlPlane") - assert.Equal(t, metav1.ConditionTrue, cond.Status) - assert.Equal(t, konnectv1alpha1.KonnectEntityProgrammedReasonProgrammed, cond.Reason) - assert.Equal(t, svc.GetGeneration(), cond.ObservedGeneration) - assert.Equal(t, "", cond.Message) }, }, } diff --git a/test/envtest/konnect_entities_cacertificate_test.go b/test/envtest/konnect_entities_kongcacertificate_test.go similarity index 100% rename from test/envtest/konnect_entities_cacertificate_test.go rename to test/envtest/konnect_entities_kongcacertificate_test.go diff --git a/test/envtest/konnect_entities_certificate_test.go b/test/envtest/konnect_entities_kongcertificate_test.go similarity index 100% rename from test/envtest/konnect_entities_certificate_test.go rename to test/envtest/konnect_entities_kongcertificate_test.go diff --git a/test/envtest/konnect_entities_dataplaneclientcertificate_test.go b/test/envtest/konnect_entities_kongdataplaneclientcertificate_test.go similarity index 100% rename from test/envtest/konnect_entities_dataplaneclientcertificate_test.go rename to test/envtest/konnect_entities_kongdataplaneclientcertificate_test.go diff --git a/test/envtest/konnect_entities_key_test.go b/test/envtest/konnect_entities_kongkey_test.go similarity index 100% rename from test/envtest/konnect_entities_key_test.go rename to test/envtest/konnect_entities_kongkey_test.go diff --git a/test/envtest/konnect_entities_keyset_test.go b/test/envtest/konnect_entities_kongkeyset_test.go similarity index 100% rename from test/envtest/konnect_entities_keyset_test.go rename to test/envtest/konnect_entities_kongkeyset_test.go diff --git a/test/envtest/konnect_entities_kongservice_test.go b/test/envtest/konnect_entities_kongservice_test.go new file mode 100644 index 000000000..a1eca241a --- /dev/null +++ b/test/envtest/konnect_entities_kongservice_test.go @@ -0,0 +1,146 @@ +package envtest + +import ( + "context" + "testing" + + sdkkonnectcomp "github.com/Kong/sdk-konnect-go/models/components" + sdkkonnectops "github.com/Kong/sdk-konnect-go/models/operations" + "github.com/samber/lo" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/watch" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/kong/gateway-operator/controller/konnect" + "github.com/kong/gateway-operator/controller/konnect/ops" + "github.com/kong/gateway-operator/modules/manager/scheme" + k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" + "github.com/kong/gateway-operator/test/helpers/deploy" + + configurationv1alpha1 "github.com/kong/kubernetes-configuration/api/configuration/v1alpha1" +) + +func TestKongService(t *testing.T) { + t.Parallel() + ctx, cancel := Context(t, context.Background()) + defer cancel() + cfg, ns := Setup(t, ctx, scheme.Get()) + + t.Log("Setting up the manager with reconcilers") + mgr, logs := NewManager(t, ctx, cfg, scheme.Get()) + factory := ops.NewMockSDKFactory(t) + sdk := factory.SDK + StartReconcilers(ctx, t, mgr, logs, + konnect.NewKonnectEntityReconciler(factory, false, mgr.GetClient(), + konnect.WithKonnectEntitySyncPeriod[configurationv1alpha1.KongService](konnectInfiniteSyncTime), + ), + ) + + t.Log("Setting up clients") + cl, err := client.NewWithWatch(mgr.GetConfig(), client.Options{ + Scheme: scheme.Get(), + }) + require.NoError(t, err) + clientNamespaced := client.NewNamespacedClient(mgr.GetClient(), ns.Name) + + t.Log("Creating KonnectAPIAuthConfiguration and KonnectGatewayControlPlane") + apiAuth := deploy.KonnectAPIAuthConfigurationWithProgrammed(t, ctx, clientNamespaced) + cp := deploy.KonnectGatewayControlPlaneWithID(t, ctx, clientNamespaced, apiAuth) + + t.Run("adding, patching and deleting KongService", func(t *testing.T) { + const ( + upstreamID = "kup-12345" + serviceID = "service-12345" + host = "example.com" + port = int64(8081) + ) + + t.Log("Creating a KongUpstream and setting it to programmed") + upstream := deploy.KongUpstreamAttachedToCP(t, ctx, clientNamespaced, cp) + updateKongUpstreamStatusWithProgrammed(t, ctx, clientNamespaced, upstream, upstreamID, cp.GetKonnectID()) + + t.Log("Setting up a watch for KongService events") + w := setupWatch[configurationv1alpha1.KongServiceList](t, ctx, cl, client.InNamespace(ns.Name)) + + t.Log("Setting up SDK expectations on Service creation") + sdk.ServicesSDK.EXPECT(). + CreateService( + mock.Anything, + cp.GetKonnectID(), + mock.MatchedBy(func(req sdkkonnectcomp.ServiceInput) bool { + return req.Host == host + }), + ). + Return( + &sdkkonnectops.CreateServiceResponse{ + Service: &sdkkonnectcomp.Service{ + ID: lo.ToPtr(serviceID), + }, + }, + nil, + ) + + t.Log("Creating a KongService") + createdService := deploy.KongServiceAttachedToCP(t, ctx, clientNamespaced, cp, + func(obj client.Object) { + s := obj.(*configurationv1alpha1.KongService) + s.Spec.KongServiceAPISpec.Host = host + }, + ) + t.Log("Checking SDK KongService operations") + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.True(c, factory.SDK.ServicesSDK.AssertExpectations(t)) + }, waitTime, tickTime) + + t.Log("Waiting for Service to be programmed and get Konnect ID") + watchFor(t, ctx, w, watch.Modified, func(kt *configurationv1alpha1.KongService) bool { + return kt.GetKonnectID() == serviceID && k8sutils.IsProgrammed(kt) + }, "KongService didn't get Programmed status condition or didn't get the correct (service-12345) Konnect ID assigned") + + t.Log("Setting up SDK expectations on Service update") + sdk.ServicesSDK.EXPECT(). + UpsertService( + mock.Anything, + mock.MatchedBy(func(req sdkkonnectops.UpsertServiceRequest) bool { + return req.ServiceID == serviceID && req.Service.Port != nil && *req.Service.Port == port + }), + ). + Return(&sdkkonnectops.UpsertServiceResponse{}, nil) + + t.Log("Patching KongService") + serviceToPatch := createdService.DeepCopy() + serviceToPatch.Spec.Port = lo.ToPtr(port) + require.NoError(t, clientNamespaced.Patch(ctx, serviceToPatch, client.MergeFrom(createdService))) + + t.Log("Waiting for Service to be updated in the SDK") + assert.EventuallyWithT(t, func(c *assert.CollectT) { + assert.True(c, factory.SDK.ServicesSDK.AssertExpectations(t)) + }, waitTime, tickTime) + + t.Log("Setting up SDK expectations on Service deletion") + sdk.ServicesSDK.EXPECT(). + DeleteService( + mock.Anything, + cp.GetKonnectID(), + serviceID, + ). + Return(&sdkkonnectops.DeleteServiceResponse{}, nil) + + t.Log("Deleting KongService") + require.NoError(t, clientNamespaced.Delete(ctx, createdService)) + + t.Log("Waiting for KongService to disappear") + assert.EventuallyWithT(t, func(c *assert.CollectT) { + err := clientNamespaced.Get(ctx, client.ObjectKeyFromObject(createdService), createdService) + assert.True(c, err != nil && k8serrors.IsNotFound(err)) + }, waitTime, tickTime) + + t.Log("Waiting for Service to be deleted in the SDK") + assert.EventuallyWithT(t, func(c *assert.CollectT) { + assert.True(c, factory.SDK.ServicesSDK.AssertExpectations(t)) + }, waitTime, tickTime) + }) +} diff --git a/test/envtest/konnect_entities_sni_test.go b/test/envtest/konnect_entities_kongsni_test.go similarity index 100% rename from test/envtest/konnect_entities_sni_test.go rename to test/envtest/konnect_entities_kongsni_test.go diff --git a/test/envtest/konnect_entities_target_test.go b/test/envtest/konnect_entities_kongtarget_test.go similarity index 99% rename from test/envtest/konnect_entities_target_test.go rename to test/envtest/konnect_entities_kongtarget_test.go index da59dc637..1440180dd 100644 --- a/test/envtest/konnect_entities_target_test.go +++ b/test/envtest/konnect_entities_kongtarget_test.go @@ -137,6 +137,5 @@ func TestKongTarget(t *testing.T) { assert.EventuallyWithT(t, func(c *assert.CollectT) { assert.True(c, factory.SDK.TargetsSDK.AssertExpectations(t)) }, waitTime, tickTime) - }) }