From 87923c47573d1ab718fcbd8e60ce585990d3e6fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Wed, 19 Feb 2025 14:21:09 +0100 Subject: [PATCH] fix(konnect): fix handling conflicts (#1176) * fix(konnect): fix handling conflicts * chore: refactor SDKErrorIsConflict and add exemplary body * fix: handle already existing Certificate when calling create --- CHANGELOG.md | 2 + controller/konnect/ops/ops.go | 1 + controller/konnect/ops/ops_errors.go | 42 +++- controller/konnect/ops/ops_errors_test.go | 100 ++++++++ controller/konnect/ops/ops_kongcertificate.go | 28 +++ .../konnect_entities_kongcertificate_test.go | 231 ++++++++++-------- 6 files changed, 299 insertions(+), 105 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b09bb6d6..3d9422d96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -159,6 +159,8 @@ [#1115](https://github.com/Kong/gateway-operator/pull/1115) - Fix setting `DataPlane`'s readiness probe using `GatewayConfiguration`. [#1118](https://github.com/Kong/gateway-operator/pull/1118) +- Fix handling Konnect API conflicts. + [#1176](https://github.com/Kong/gateway-operator/pull/1176) ## [v1.4.2] diff --git a/controller/konnect/ops/ops.go b/controller/konnect/ops/ops.go index 1cd81c0f7..c4a92bfb4 100644 --- a/controller/konnect/ops/ops.go +++ b/controller/konnect/ops/ops.go @@ -58,6 +58,7 @@ func Create[ entityType = e.GetTypeName() statusCode int ) + switch ent := any(e).(type) { case *konnectv1alpha1.KonnectGatewayControlPlane: err = createControlPlane(ctx, sdk.GetControlPlaneSDK(), sdk.GetControlPlaneGroupSDK(), cl, ent) diff --git a/controller/konnect/ops/ops_errors.go b/controller/konnect/ops/ops_errors.go index 1e7ea0d5e..e86adf098 100644 --- a/controller/konnect/ops/ops_errors.go +++ b/controller/konnect/ops/ops_errors.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "slices" + "strings" sdkkonnecterrs "github.com/Kong/sdk-konnect-go/models/sdkerrors" "github.com/go-logr/logr" @@ -94,7 +95,6 @@ func ParseSDKErrorBody(body string) (sdkErrorBody, error) { } const ( - dataConstraintMesasge = "data constraint error" validationErrorMessage = "validation error" apiErrorOccurredMessage = "API error occurred" ) @@ -196,20 +196,50 @@ func ErrorIsCreateConflict(err error) bool { } // SDKErrorIsConflict returns true if the provided SDKError indicates a conflict. +// We currently handle two codes (as mapped in +// https://grpc.io/docs/guides/status-codes/#the-full-list-of-status-codes) +// from SDK error: +// - 3: INVALID_ARGUMENT +// - 6: ALREADY_EXISTS +// +// Exemplary body: +// +// { +// "code": 3, +// "message": "name (type: unique) constraint failed", +// "details": [ +// { +// "@type": "type.googleapis.com/kong.admin.model.v1.ErrorDetail", +// "type": "ERROR_TYPE_REFERENCE", +// "field": "name", +// "messages": [ +// "name (type: unique) constraint failed" +// ] +// } +// ] +// } func SDKErrorIsConflict(sdkError *sdkkonnecterrs.SDKError) bool { sdkErrorBody, err := ParseSDKErrorBody(sdkError.Body) if err != nil { return false } - if sdkErrorBody.Message != dataConstraintMesasge { - return false - } - switch sdkErrorBody.Code { - case 3, 6: + case 3: // INVALID_ARGUMENT + const ( + dataConstraintMesasge = "data constraint error" + typeUniqueConstraintFailed = "(type: unique) constraint failed" + ) + + if sdkErrorBody.Message == dataConstraintMesasge || + strings.Contains(sdkErrorBody.Message, typeUniqueConstraintFailed) { + return true + } + + case 6: // ALREADY_EXISTS return true } + return false } diff --git a/controller/konnect/ops/ops_errors_test.go b/controller/konnect/ops/ops_errors_test.go index cbf5fba14..fa23cbec6 100644 --- a/controller/konnect/ops/ops_errors_test.go +++ b/controller/konnect/ops/ops_errors_test.go @@ -126,6 +126,28 @@ func TestErrorIsCreateConflict(t *testing.T) { }, want: false, }, + { + name: "error is SDKError with code 6", + err: &sdkkonnecterrs.SDKError{ + Body: `{ + "code": 6, + "message": "already exists", + "details": [] + }`, + }, + want: true, + }, + { + name: "error is SDKError with code 7", + err: &sdkkonnecterrs.SDKError{ + Body: `{ + "code": 7, + "message": "other error", + "details": [] + }`, + }, + want: false, + }, { name: "error is not ConflictError or SDKError", err: errors.New("some other error"), @@ -140,3 +162,81 @@ func TestErrorIsCreateConflict(t *testing.T) { }) } } + +func TestSDKErrorIsConflict(t *testing.T) { + tests := []struct { + name string + err *sdkkonnecterrs.SDKError + want bool + }{ + { + name: "SDKError with data constraint error message and code 3", + err: &sdkkonnecterrs.SDKError{ + Body: `{ + "code": 3, + "message": "data constraint error", + "details": [] + }`, + }, + want: true, + }, + { + name: "SDKError with data constraint error message and code 6", + err: &sdkkonnecterrs.SDKError{ + Body: `{ + "code": 6, + "message": "data constraint error", + "details": [] + }`, + }, + want: true, + }, + { + name: "SDKError with unique constraint failed message", + err: &sdkkonnecterrs.SDKError{ + Body: `{ + "code": 3, + "message": "name (type: unique) constraint failed", + "details": [] + }`, + }, + want: true, + }, + { + name: "SDKError with non-conflict message", + err: &sdkkonnecterrs.SDKError{ + Body: `{ + "code": 3, + "message": "some other error", + "details": [] + }`, + }, + want: false, + }, + { + name: "SDKError with conflict message but different code", + err: &sdkkonnecterrs.SDKError{ + Body: `{ + "code": 4, + "message": "data constraint error", + "details": [] + }`, + }, + want: false, + }, + { + name: "SDKError with invalid JSON body", + err: &sdkkonnecterrs.SDKError{ + Body: `invalid json`, + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := SDKErrorIsConflict(tt.err) + require.Equal(t, tt.want, got) + }) + } +} diff --git a/controller/konnect/ops/ops_kongcertificate.go b/controller/konnect/ops/ops_kongcertificate.go index 7aca86764..88890d277 100644 --- a/controller/konnect/ops/ops_kongcertificate.go +++ b/controller/konnect/ops/ops_kongcertificate.go @@ -3,6 +3,7 @@ package ops import ( "context" "fmt" + "strings" sdkkonnectcomp "github.com/Kong/sdk-konnect-go/models/components" sdkkonnectops "github.com/Kong/sdk-konnect-go/models/operations" @@ -25,6 +26,33 @@ func createCertificate( return CantPerformOperationWithoutControlPlaneIDError{Entity: cert, Op: CreateOp} } + // NOTE: This is a workaround for the fact that the Konnect SDK does not + // return a conflict error when creating a Certificate as there are no criteria + // that would prevent the creation of a Certificate with the same spec fields. + // This can be expanded to other entities by changing the order of get -> list + // to list -> get in ops.go's Create() function. + + // Check if the Certificate already exists in Konnect: it is possible that + // the Certificate was already created and cache the controller is using + // is outdated, so we can't rely on the status.konnect.id. + // If it does, set the KonnectID in the KongCertificate status and return. + + respList, err := sdk.ListCertificate(ctx, sdkkonnectops.ListCertificateRequest{ + ControlPlaneID: cpID, + Tags: lo.ToPtr(strings.Join(GenerateTagsForObject(cert, cert.Spec.Tags...), ",")), + }) + if err != nil { + return fmt.Errorf("failed to list: %w", err) + } + if respList.Object != nil && len(respList.Object.Data) > 0 { + certList := respList.Object.Data[0] + if certList.ID == nil { + return fmt.Errorf("failed listing: found a cert without ID") + } + cert.SetKonnectID(*certList.ID) + return nil + } + resp, err := sdk.CreateCertificate(ctx, cpID, kongCertificateToCertificateInput(cert), diff --git a/test/envtest/konnect_entities_kongcertificate_test.go b/test/envtest/konnect_entities_kongcertificate_test.go index 882454776..b37d46ed4 100644 --- a/test/envtest/konnect_entities_kongcertificate_test.go +++ b/test/envtest/konnect_entities_kongcertificate_test.go @@ -4,11 +4,11 @@ import ( "context" "fmt" "slices" + "strings" "testing" 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" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -18,6 +18,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/kong/gateway-operator/controller/konnect" + "github.com/kong/gateway-operator/controller/konnect/ops" sdkmocks "github.com/kong/gateway-operator/controller/konnect/ops/sdk/mocks" "github.com/kong/gateway-operator/modules/manager/scheme" k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" @@ -52,102 +53,116 @@ func TestKongCertificate(t *testing.T) { t.Log("Creating KonnectAPIAuthConfiguration and KonnectGatewayControlPlane") apiAuth := deploy.KonnectAPIAuthConfigurationWithProgrammed(t, ctx, clientNamespaced) - cp := deploy.KonnectGatewayControlPlaneWithID(t, ctx, clientNamespaced, apiAuth) - - t.Log("Setting up SDK expectations on KongCertificate creation") - sdk.CertificatesSDK.EXPECT().CreateCertificate(mock.Anything, cp.GetKonnectStatus().GetKonnectID(), - mock.MatchedBy(func(input sdkkonnectcomp.CertificateInput) bool { - return input.Cert == deploy.TestValidCertPEM && - input.Key == deploy.TestValidCertKeyPEM && - slices.Contains(input.Tags, "tag1") - }), - ).Return(&sdkkonnectops.CreateCertificateResponse{ - Certificate: &sdkkonnectcomp.Certificate{ - ID: lo.ToPtr("cert-12345"), - }, - }, nil) - - t.Log("Setting up a watch for KongCertificate events") - w := setupWatch[configurationv1alpha1.KongCertificateList](t, ctx, cl, client.InNamespace(ns.Name)) - - t.Log("Creating KongCertificate") - createdCert := deploy.KongCertificateAttachedToCP(t, ctx, clientNamespaced, cp, - func(obj client.Object) { - cert := obj.(*configurationv1alpha1.KongCertificate) - cert.Spec.Tags = []string{"tag1"} - }, - ) - - t.Log("Waiting for KongCertificate to be programmed") - watchFor(t, ctx, w, watch.Modified, func(c *configurationv1alpha1.KongCertificate) bool { - if c.GetName() != createdCert.GetName() { - return false - } - return lo.ContainsBy(c.Status.Conditions, func(condition metav1.Condition) bool { - return condition.Type == konnectv1alpha1.KonnectEntityProgrammedConditionType && - condition.Status == metav1.ConditionTrue - }) - }, "KongCertificate's Programmed condition should be true eventually") - - t.Log("Waiting for KongCertificate to be created in the SDK") - require.EventuallyWithT(t, func(c *assert.CollectT) { - assert.True(c, factory.SDK.CertificatesSDK.AssertExpectations(t)) - }, waitTime, tickTime) - - t.Log("Setting up SDK expectations on KongCertificate update") - sdk.CertificatesSDK.EXPECT().UpsertCertificate(mock.Anything, mock.MatchedBy(func(r sdkkonnectops.UpsertCertificateRequest) bool { - return r.CertificateID == "cert-12345" && - lo.Contains(r.Certificate.Tags, "addedTag") - })).Return(&sdkkonnectops.UpsertCertificateResponse{}, nil) - - t.Log("Patching KongCertificate") - certToPatch := createdCert.DeepCopy() - certToPatch.Spec.Tags = append(certToPatch.Spec.Tags, "addedTag") - require.NoError(t, clientNamespaced.Patch(ctx, certToPatch, client.MergeFrom(createdCert))) - - t.Log("Waiting for KongCertificate to be updated in the SDK") - assert.EventuallyWithT(t, func(c *assert.CollectT) { - assert.True(c, factory.SDK.CertificatesSDK.AssertExpectations(t)) - }, waitTime, tickTime) - - t.Log("Setting up SDK expectations on KongCertificate deletion") - sdk.CertificatesSDK.EXPECT().DeleteCertificate(mock.Anything, cp.GetKonnectStatus().GetKonnectID(), "cert-12345"). - Return(&sdkkonnectops.DeleteCertificateResponse{}, nil) - - t.Log("Deleting KongCertificate") - require.NoError(t, cl.Delete(ctx, createdCert)) - - t.Log("Waiting for KongCertificate to be deleted in the SDK") - assert.EventuallyWithT(t, func(c *assert.CollectT) { - assert.True(c, factory.SDK.CertificatesSDK.AssertExpectations(t)) - }, waitTime, tickTime) - t.Run("should handle conflict in creation correctly", func(t *testing.T) { - const ( - certID = "id-conflict" - ) - t.Log("Setup mock SDK for creating certificate and listing certificates by UID") + t.Run("base", func(t *testing.T) { + cp := deploy.KonnectGatewayControlPlaneWithID(t, ctx, clientNamespaced, apiAuth) cpID := cp.GetKonnectStatus().GetKonnectID() + + t.Log("Setting up SDK expectations on KongCertificate creation") sdk.CertificatesSDK.EXPECT(). CreateCertificate(mock.Anything, cpID, mock.MatchedBy(func(input sdkkonnectcomp.CertificateInput) bool { return input.Cert == deploy.TestValidCertPEM && input.Key == deploy.TestValidCertKeyPEM && - slices.Contains(input.Tags, "xconflictx") + slices.Contains(input.Tags, "tag1") }), ). - Return(nil, - &sdkkonnecterrs.SDKError{ - StatusCode: 400, - Body: ErrBodyDataConstraintError, + Return(&sdkkonnectops.CreateCertificateResponse{ + Certificate: &sdkkonnectcomp.Certificate{ + ID: lo.ToPtr("cert-12345"), }, - ) + }, nil) + + sdk.CertificatesSDK.EXPECT(). + ListCertificate(mock.Anything, + mock.MatchedBy(func(input sdkkonnectops.ListCertificateRequest) bool { + return input.ControlPlaneID == cpID + }), + ). + Return(&sdkkonnectops.ListCertificateResponse{ + Object: &sdkkonnectops.ListCertificateResponseBody{}, + }, nil) + + t.Log("Setting up a watch for KongCertificate events") + w := setupWatch[configurationv1alpha1.KongCertificateList](t, ctx, cl, client.InNamespace(ns.Name)) + + t.Log("Creating KongCertificate") + createdCert := deploy.KongCertificateAttachedToCP(t, ctx, clientNamespaced, cp, + func(obj client.Object) { + cert := obj.(*configurationv1alpha1.KongCertificate) + cert.Spec.Tags = []string{"tag1", ops.KubernetesUIDLabelKey + ":12345"} + cert.UID = "12345" + }, + ) + + t.Log("Waiting for KongCertificate to be programmed") + watchFor(t, ctx, w, watch.Modified, func(c *configurationv1alpha1.KongCertificate) bool { + if c.GetName() != createdCert.GetName() { + return false + } + return lo.ContainsBy(c.Status.Conditions, func(condition metav1.Condition) bool { + return condition.Type == konnectv1alpha1.KonnectEntityProgrammedConditionType && + condition.Status == metav1.ConditionTrue + }) + }, "KongCertificate's Programmed condition should be true eventually") + + t.Log("Waiting for KongCertificate to be created in the SDK") + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.True(c, factory.SDK.CertificatesSDK.AssertExpectations(t)) + }, waitTime, tickTime) + + t.Log("Setting up SDK expectations on KongCertificate update") + sdk.CertificatesSDK.EXPECT().UpsertCertificate(mock.Anything, mock.MatchedBy(func(r sdkkonnectops.UpsertCertificateRequest) bool { + return r.CertificateID == "cert-12345" && + lo.Contains(r.Certificate.Tags, "addedTag") + })).Return(&sdkkonnectops.UpsertCertificateResponse{}, nil) + + t.Log("Patching KongCertificate") + certToPatch := createdCert.DeepCopy() + certToPatch.Spec.Tags = append(certToPatch.Spec.Tags, "addedTag") + require.NoError(t, clientNamespaced.Patch(ctx, certToPatch, client.MergeFrom(createdCert))) + + t.Log("Waiting for KongCertificate to be updated in the SDK") + assert.EventuallyWithT(t, func(c *assert.CollectT) { + assert.True(c, factory.SDK.CertificatesSDK.AssertExpectations(t)) + }, waitTime, tickTime) + + t.Log("Setting up SDK expectations on KongCertificate deletion") + sdk.CertificatesSDK.EXPECT().DeleteCertificate(mock.Anything, cpID, "cert-12345"). + Return(&sdkkonnectops.DeleteCertificateResponse{}, nil) + + t.Log("Deleting KongCertificate") + require.NoError(t, cl.Delete(ctx, createdCert)) + + t.Log("Waiting for KongCertificate to be deleted in the SDK") + assert.EventuallyWithT(t, func(c *assert.CollectT) { + assert.True(c, factory.SDK.CertificatesSDK.AssertExpectations(t)) + }, waitTime, tickTime) + }) + t.Run("should handle conflict in creation correctly", func(t *testing.T) { + const ( + certID = "id-conflict" + ) + cp := deploy.KonnectGatewayControlPlaneWithID(t, ctx, clientNamespaced, apiAuth) + cpID := cp.GetKonnectStatus().GetKonnectID() + + t.Log("Setup mock SDK for creating certificate and listing certificates by UID") + + w := setupWatch[configurationv1alpha1.KongCertificateList](t, ctx, cl, client.InNamespace(ns.Name)) + t.Log("Creating a KongCertificate") + createdCert := deploy.KongCertificateAttachedToCP(t, ctx, clientNamespaced, cp, + func(obj client.Object) { + cert := obj.(*configurationv1alpha1.KongCertificate) + cert.Spec.Tags = []string{"xconflictx"} + }, + ) sdk.CertificatesSDK.EXPECT(). ListCertificate( mock.Anything, mock.MatchedBy(func(req sdkkonnectops.ListCertificateRequest) bool { - return req.ControlPlaneID == cpID + return req.ControlPlaneID == cpID && + strings.Contains(*req.Tags, "xconflictx") }), ). Return( @@ -162,14 +177,6 @@ func TestKongCertificate(t *testing.T) { }, nil, ) - t.Log("Creating a KongCertificate") - createdCert := deploy.KongCertificateAttachedToCP(t, ctx, clientNamespaced, cp, - func(obj client.Object) { - cert := obj.(*configurationv1alpha1.KongCertificate) - cert.Spec.Tags = []string{"xconflictx"} - }, - ) - t.Log("Watching for KongCertificates to verify the created KongCertificate gets programmed") watchFor(t, ctx, w, watch.Modified, func(c *configurationv1alpha1.KongCertificate) bool { if c.GetName() != createdCert.GetName() { @@ -188,12 +195,28 @@ func TestKongCertificate(t *testing.T) { }) t.Run("should handle konnectID control plane reference", func(t *testing.T) { + cp := deploy.KonnectGatewayControlPlaneWithID(t, ctx, clientNamespaced, apiAuth) + cpID := cp.GetKonnectStatus().GetKonnectID() + t.Log("Setting up SDK expectations on KongCertificate creation") - sdk.CertificatesSDK.EXPECT().CreateCertificate(mock.Anything, cp.GetKonnectStatus().GetKonnectID(), + + sdk.CertificatesSDK.EXPECT(). + ListCertificate( + mock.Anything, + mock.MatchedBy(func(req sdkkonnectops.ListCertificateRequest) bool { + return req.ControlPlaneID == cpID && + strings.Contains(*req.Tags, "tag2") + }), + ). + Return( + &sdkkonnectops.ListCertificateResponse{}, nil, + ) + + sdk.CertificatesSDK.EXPECT().CreateCertificate(mock.Anything, cpID, mock.MatchedBy(func(input sdkkonnectcomp.CertificateInput) bool { return input.Cert == deploy.TestValidCertPEM && input.Key == deploy.TestValidCertKeyPEM && - slices.Contains(input.Tags, "tag1") + slices.Contains(input.Tags, "tag2") }), ).Return(&sdkkonnectops.CreateCertificateResponse{ Certificate: &sdkkonnectcomp.Certificate{ @@ -201,11 +224,12 @@ func TestKongCertificate(t *testing.T) { }, }, nil) + w := setupWatch[configurationv1alpha1.KongCertificateList](t, ctx, cl, client.InNamespace(ns.Name)) t.Log("Creating KongCertificate with ControlPlaneRef type=konnectID") createdCert := deploy.KongCertificateAttachedToCP(t, ctx, clientNamespaced, cp, func(obj client.Object) { cert := obj.(*configurationv1alpha1.KongCertificate) - cert.Spec.Tags = []string{"tag1"} + cert.Spec.Tags = []string{"tag2"} }, deploy.WithKonnectIDControlPlaneRef(cp), ) @@ -235,22 +259,31 @@ func TestKongCertificate(t *testing.T) { id = "abc-12345" ) - var tags []string = []string{"tag1"} - - t.Log("Creating KonnectAPIAuthConfiguration and KonnectGatewayControlPlane") - apiAuth := deploy.KonnectAPIAuthConfigurationWithProgrammed(t, ctx, clientNamespaced) cp := deploy.KonnectGatewayControlPlaneWithID(t, ctx, clientNamespaced, apiAuth) + cpID := cp.GetKonnectStatus().GetKonnectID() t.Log("Setting up a watch for KongCertifcate events") w := setupWatch[configurationv1alpha1.KongCertificateList](t, ctx, cl, client.InNamespace(ns.Name)) t.Log("Setting up SDK expectations on KongCertifcate creation") + sdk.CertificatesSDK.EXPECT(). + ListCertificate( + mock.Anything, + mock.MatchedBy(func(req sdkkonnectops.ListCertificateRequest) bool { + return req.ControlPlaneID == cpID && + strings.Contains(*req.Tags, "tag3") + }), + ). + Return( + &sdkkonnectops.ListCertificateResponse{}, nil, + ) + sdk.CertificatesSDK.EXPECT(). CreateCertificate( mock.Anything, cp.GetKonnectID(), mock.MatchedBy(func(req sdkkonnectcomp.CertificateInput) bool { - return slices.Contains(req.Tags, "tag1") + return slices.Contains(req.Tags, "tag3") }), ). Return( @@ -266,7 +299,7 @@ func TestKongCertificate(t *testing.T) { deploy.WithKonnectIDControlPlaneRef(cp), func(obj client.Object) { cert := obj.(*configurationv1alpha1.KongCertificate) - cert.Spec.Tags = tags + cert.Spec.Tags = []string{"tag3"} }, ) t.Log("Checking SDK Certificate operations")