From 2d377a6f56386a9ee3b8cbd0781b23c0e37647f8 Mon Sep 17 00:00:00 2001 From: Jintao Zhang Date: Mon, 21 Oct 2024 20:36:25 +0800 Subject: [PATCH] fix(konnect): handle CACertificate creation conflicts Signed-off-by: Jintao Zhang --- controller/konnect/ops/ops.go | 2 + .../konnect/ops/ops_kongcacertificate.go | 31 ++++++- .../konnect/ops/sdk/kongcacertificate.go | 1 + .../zz_generated.kongcacertificate_mock.go | 74 +++++++++++++++++ ...konnect_entities_kongcacertificate_test.go | 80 ++++++++++++++++++- test/helpers/deploy/deploy_resources.go | 4 + 6 files changed, 187 insertions(+), 5 deletions(-) diff --git a/controller/konnect/ops/ops.go b/controller/konnect/ops/ops.go index 08ce7d91f..452e79bdb 100644 --- a/controller/konnect/ops/ops.go +++ b/controller/konnect/ops/ops.go @@ -145,6 +145,8 @@ func Create[ id, err = getKongCredentialACLForUID(ctx, sdk.GetACLCredentialsSDK(), ent) case *configurationv1alpha1.KongCertificate: id, err = getKongCertificateForUID(ctx, sdk.GetCertificatesSDK(), ent) + case *configurationv1alpha1.KongCACertificate: + id, err = getKongCACertificateForUID(ctx, sdk.GetCACertificatesSDK(), ent) // --------------------------------------------------------------------- // TODO: add other Konnect types default: diff --git a/controller/konnect/ops/ops_kongcacertificate.go b/controller/konnect/ops/ops_kongcacertificate.go index d047ba8cb..d4530e34c 100644 --- a/controller/konnect/ops/ops_kongcacertificate.go +++ b/controller/konnect/ops/ops_kongcacertificate.go @@ -2,6 +2,8 @@ package ops import ( "context" + "fmt" + "github.com/samber/lo" sdkkonnectcomp "github.com/Kong/sdk-konnect-go/models/components" sdkkonnectops "github.com/Kong/sdk-konnect-go/models/operations" @@ -32,12 +34,15 @@ func createCACertificate( // Can't adopt it as it will cause conflicts between the controller // that created that entity and already manages it, hm if errWrap := wrapErrIfKonnectOpFailed(err, CreateOp, cert); errWrap != nil { - SetKonnectEntityProgrammedConditionFalse(cert, "FailedToCreate", errWrap.Error()) return errWrap } - cert.Status.Konnect.SetKonnectID(*resp.CACertificate.ID) - SetKonnectEntityProgrammedCondition(cert) + if resp == nil || resp.CACertificate == nil || resp.CACertificate.ID == nil || *resp.CACertificate.ID == "" { + return fmt.Errorf("failed creating %s: %w", cert.GetTypeName(), ErrNilResponse) + } + + // At this point, the CACertificate has been created successfully. + cert.SetKonnectID(*resp.CACertificate.ID) return nil } @@ -100,3 +105,23 @@ func kongCACertificateToCACertificateInput(cert *configurationv1alpha1.KongCACer Tags: GenerateTagsForObject(cert, cert.Spec.Tags...), } } + +func getKongCACertificateForUID( + ctx context.Context, + sdk sdkops.CACertificatesSDK, + cert *configurationv1alpha1.KongCACertificate, +) (string, error) { + resp, err := sdk.ListCaCertificate(ctx, sdkkonnectops.ListCaCertificateRequest{ + ControlPlaneID: cert.GetControlPlaneID(), + Tags: lo.ToPtr(UIDLabelForObject(cert)), + }) + if err != nil { + return "", fmt.Errorf("failed to list %s: %w", cert.GetTypeName(), err) + } + + if resp == nil || resp.Object == nil { + return "", fmt.Errorf("failed listing %s: %w", cert.GetTypeName(), ErrNilResponse) + } + + return getMatchingEntryFromListResponseData(sliceToEntityWithIDSlice(resp.Object.Data), cert) +} diff --git a/controller/konnect/ops/sdk/kongcacertificate.go b/controller/konnect/ops/sdk/kongcacertificate.go index ff1b4f699..520b9b212 100644 --- a/controller/konnect/ops/sdk/kongcacertificate.go +++ b/controller/konnect/ops/sdk/kongcacertificate.go @@ -12,4 +12,5 @@ type CACertificatesSDK interface { CreateCaCertificate(ctx context.Context, controlPlaneID string, caCertificate sdkkonnectcomp.CACertificateInput, opts ...sdkkonnectops.Option) (*sdkkonnectops.CreateCaCertificateResponse, error) UpsertCaCertificate(ctx context.Context, request sdkkonnectops.UpsertCaCertificateRequest, opts ...sdkkonnectops.Option) (*sdkkonnectops.UpsertCaCertificateResponse, error) DeleteCaCertificate(ctx context.Context, controlPlaneID string, caCertificateID string, opts ...sdkkonnectops.Option) (*sdkkonnectops.DeleteCaCertificateResponse, error) + ListCaCertificate(ctx context.Context, request sdkkonnectops.ListCaCertificateRequest, opts ...sdkkonnectops.Option) (*sdkkonnectops.ListCaCertificateResponse, error) } diff --git a/controller/konnect/ops/sdk/mocks/zz_generated.kongcacertificate_mock.go b/controller/konnect/ops/sdk/mocks/zz_generated.kongcacertificate_mock.go index 07b4f3c41..deefa9e20 100644 --- a/controller/konnect/ops/sdk/mocks/zz_generated.kongcacertificate_mock.go +++ b/controller/konnect/ops/sdk/mocks/zz_generated.kongcacertificate_mock.go @@ -175,6 +175,80 @@ func (_c *MockCACertificatesSDK_DeleteCaCertificate_Call) RunAndReturn(run func( return _c } +// ListCaCertificate provides a mock function with given fields: ctx, request, opts +func (_m *MockCACertificatesSDK) ListCaCertificate(ctx context.Context, request operations.ListCaCertificateRequest, opts ...operations.Option) (*operations.ListCaCertificateResponse, 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 ListCaCertificate") + } + + var r0 *operations.ListCaCertificateResponse + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, operations.ListCaCertificateRequest, ...operations.Option) (*operations.ListCaCertificateResponse, error)); ok { + return rf(ctx, request, opts...) + } + if rf, ok := ret.Get(0).(func(context.Context, operations.ListCaCertificateRequest, ...operations.Option) *operations.ListCaCertificateResponse); ok { + r0 = rf(ctx, request, opts...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*operations.ListCaCertificateResponse) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, operations.ListCaCertificateRequest, ...operations.Option) error); ok { + r1 = rf(ctx, request, opts...) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// MockCACertificatesSDK_ListCaCertificate_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ListCaCertificate' +type MockCACertificatesSDK_ListCaCertificate_Call struct { + *mock.Call +} + +// ListCaCertificate is a helper method to define mock.On call +// - ctx context.Context +// - request operations.ListCaCertificateRequest +// - opts ...operations.Option +func (_e *MockCACertificatesSDK_Expecter) ListCaCertificate(ctx interface{}, request interface{}, opts ...interface{}) *MockCACertificatesSDK_ListCaCertificate_Call { + return &MockCACertificatesSDK_ListCaCertificate_Call{Call: _e.mock.On("ListCaCertificate", + append([]interface{}{ctx, request}, opts...)...)} +} + +func (_c *MockCACertificatesSDK_ListCaCertificate_Call) Run(run func(ctx context.Context, request operations.ListCaCertificateRequest, opts ...operations.Option)) *MockCACertificatesSDK_ListCaCertificate_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.ListCaCertificateRequest), variadicArgs...) + }) + return _c +} + +func (_c *MockCACertificatesSDK_ListCaCertificate_Call) Return(_a0 *operations.ListCaCertificateResponse, _a1 error) *MockCACertificatesSDK_ListCaCertificate_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *MockCACertificatesSDK_ListCaCertificate_Call) RunAndReturn(run func(context.Context, operations.ListCaCertificateRequest, ...operations.Option) (*operations.ListCaCertificateResponse, error)) *MockCACertificatesSDK_ListCaCertificate_Call { + _c.Call.Return(run) + return _c +} + // UpsertCaCertificate provides a mock function with given fields: ctx, request, opts func (_m *MockCACertificatesSDK) UpsertCaCertificate(ctx context.Context, request operations.UpsertCaCertificateRequest, opts ...operations.Option) (*operations.UpsertCaCertificateResponse, error) { _va := make([]interface{}, len(opts)) diff --git a/test/envtest/konnect_entities_kongcacertificate_test.go b/test/envtest/konnect_entities_kongcacertificate_test.go index 6ac0733c4..2dec5a72d 100644 --- a/test/envtest/konnect_entities_kongcacertificate_test.go +++ b/test/envtest/konnect_entities_kongcacertificate_test.go @@ -2,10 +2,12 @@ package envtest import ( "context" + "slices" "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" @@ -53,7 +55,8 @@ func TestKongCACertificate(t *testing.T) { t.Log("Setting up SDK expectations on KongCACertificate creation") sdk.CACertificatesSDK.EXPECT().CreateCaCertificate(mock.Anything, cp.GetKonnectStatus().GetKonnectID(), mock.MatchedBy(func(input sdkkonnectcomp.CACertificateInput) bool { - return input.Cert == deploy.TestValidCACertPEM + return input.Cert == deploy.TestValidCACertPEM && + slices.Contains(input.Tags, "tag1") }), ).Return(&sdkkonnectops.CreateCaCertificateResponse{ CACertificate: &sdkkonnectcomp.CACertificate{ @@ -65,7 +68,12 @@ func TestKongCACertificate(t *testing.T) { w := setupWatch[configurationv1alpha1.KongCACertificateList](t, ctx, cl, client.InNamespace(ns.Name)) t.Log("Creating KongCACertificate") - createdCert := deploy.KongCACertificateAttachedToCP(t, ctx, clientNamespaced, cp) + createdCert := deploy.KongCACertificateAttachedToCP(t, ctx, clientNamespaced, cp, + func(obj client.Object) { + cert := obj.(*configurationv1alpha1.KongCACertificate) + cert.Spec.Tags = []string{"tag1"} + }, + ) t.Log("Waiting for KongCACertificate to be programmed") watchFor(t, ctx, w, watch.Modified, func(c *configurationv1alpha1.KongCACertificate) bool { @@ -110,4 +118,72 @@ func TestKongCACertificate(t *testing.T) { assert.EventuallyWithT(t, func(c *assert.CollectT) { assert.True(c, factory.SDK.CACertificatesSDK.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 CA certificate and listing CA certificates by UID") + cpID := cp.GetKonnectStatus().GetKonnectID() + sdk.CACertificatesSDK.EXPECT(). + CreateCaCertificate(mock.Anything, cpID, + mock.MatchedBy(func(input sdkkonnectcomp.CACertificateInput) bool { + return input.Cert == deploy.TestValidCACertPEM && + slices.Contains(input.Tags, "xconflictx") + }), + ). + Return(nil, + &sdkkonnecterrs.SDKError{ + StatusCode: 400, + Body: ErrBodyDataConstraintError, + }, + ) + + sdk.CACertificatesSDK.EXPECT(). + ListCaCertificate( + mock.Anything, + mock.MatchedBy(func(req sdkkonnectops.ListCaCertificateRequest) bool { + return req.ControlPlaneID == cpID + }), + ). + Return( + &sdkkonnectops.ListCaCertificateResponse{ + Object: &sdkkonnectops.ListCaCertificateResponseBody{ + Data: []sdkkonnectcomp.CACertificate{ + { + ID: lo.ToPtr(certID), + }, + }, + }, + }, nil, + ) + + t.Log("Creating a KongCACertificate") + createdCert := deploy.KongCACertificateAttachedToCP(t, ctx, clientNamespaced, cp, + func(obj client.Object) { + cert := obj.(*configurationv1alpha1.KongCACertificate) + cert.Spec.Tags = []string{"xconflictx"} + }, + ) + + t.Log("Watching for KongCACertificates to verify the created KongCACertificate gets programmed") + watchFor(t, ctx, w, watch.Modified, func(c *configurationv1alpha1.KongCACertificate) bool { + if c.GetName() != createdCert.GetName() { + return false + } + if !slices.Equal(c.Spec.Tags, createdCert.Spec.Tags) { + return false + } + + return c.GetKonnectID() == certID && lo.ContainsBy(c.Status.Conditions, func(condition metav1.Condition) bool { + return condition.Type == konnectv1alpha1.KonnectEntityProgrammedConditionType && + condition.Status == metav1.ConditionTrue + }) + }, "KongCACertificate should be programmed and have ID in status after handling conflict") + + t.Log("Ensuring that the SDK's create and list methods are called") + assert.EventuallyWithT(t, func(c *assert.CollectT) { + assert.True(c, sdk.CACertificatesSDK.AssertExpectations(t)) + }, waitTime, tickTime) + }) } diff --git a/test/helpers/deploy/deploy_resources.go b/test/helpers/deploy/deploy_resources.go index 69b5b66b3..b70463431 100644 --- a/test/helpers/deploy/deploy_resources.go +++ b/test/helpers/deploy/deploy_resources.go @@ -484,6 +484,7 @@ func KongCACertificateAttachedToCP( ctx context.Context, cl client.Client, cp *konnectv1alpha1.KonnectGatewayControlPlane, + opts ...objOption, ) *configurationv1alpha1.KongCACertificate { t.Helper() @@ -503,6 +504,9 @@ func KongCACertificateAttachedToCP( }, }, } + for _, opt := range opts { + opt(cert) + } require.NoError(t, cl.Create(ctx, cert)) logObjectCreate(t, cert)