From e91186b6a70805f423be80c934ecd276b2a6151d Mon Sep 17 00:00:00 2001 From: Antoine CORDIER Date: Fri, 6 Sep 2024 18:23:50 +0200 Subject: [PATCH] fix: validate that started APIs have plans see https://gravitee.atlassian.net/browse/GKO-490 --- api/model/api/base/api.go | 4 ++ api/model/api/v2/api.go | 8 +++ api/model/api/v4/api.go | 8 +++ api/v1alpha1/apiv2definition_types.go | 8 +++ api/v1alpha1/apiv4definition_types.go | 8 +++ internal/admission/api/base/plans.go | 32 ++++++++++ internal/admission/api/base/validate.go | 1 + internal/admission/api/v4/validate.go | 23 -------- internal/core/interface.go | 3 + ...houtContext_andNoPlansWhileStarted_test.go | 58 +++++++++++++++++++ .../create_withoutContext_andNoPlans_test.go | 27 ++++----- .../update_withoutContext_andNoPlans_test.go | 29 ++++------ 12 files changed, 154 insertions(+), 55 deletions(-) create mode 100644 internal/admission/api/base/plans.go create mode 100644 test/integration/admission/api/v2/create_withoutContext_andNoPlansWhileStarted_test.go diff --git a/api/model/api/base/api.go b/api/model/api/base/api.go index 1ddd66129..d3de0922d 100644 --- a/api/model/api/base/api.go +++ b/api/model/api/base/api.go @@ -106,3 +106,7 @@ type ResponseTemplate struct { Headers map[string]string `json:"headers,omitempty"` Body string `json:"body,omitempty"` } + +func (api *ApiBase) GetName() string { + return api.Name +} diff --git a/api/model/api/v2/api.go b/api/model/api/v2/api.go index 1a196e798..5b8239981 100644 --- a/api/model/api/v2/api.go +++ b/api/model/api/v2/api.go @@ -81,6 +81,14 @@ func (api *Api) GetDefinitionVersion() core.ApiDefinitionVersion { return core.ApiV2 } +func (api *Api) GetState() string { + return string(api.State) +} + +func (api *Api) HasPlans() bool { + return len(api.Plans) > 0 +} + func (api *Api) GetContextPaths() []string { paths := make([]string, 0) proxy := api.Proxy diff --git a/api/model/api/v4/api.go b/api/model/api/v4/api.go index 02ed03293..dcaf5c9d4 100644 --- a/api/model/api/v4/api.go +++ b/api/model/api/v4/api.go @@ -175,6 +175,14 @@ func (api *Api) SetDefinitionContext(ctx core.DefinitionContext) { } } +func (api *Api) GetState() string { + return string(api.State) +} + +func (api *Api) HasPlans() bool { + return len(api.Plans) > 0 +} + // Converts the API to its gateway definition equivalent. func (api *Api) ToGatewayDefinition() GatewayDefinitionApi { def := GatewayDefinitionApi{Api: api} diff --git a/api/v1alpha1/apiv2definition_types.go b/api/v1alpha1/apiv2definition_types.go index 5dfe209c3..7b5002dc8 100644 --- a/api/v1alpha1/apiv2definition_types.go +++ b/api/v1alpha1/apiv2definition_types.go @@ -148,6 +148,14 @@ func (api *ApiDefinition) SetDefinitionContext(ctx core.DefinitionContext) { api.Spec.SetDefinitionContext(ctx) } +func (api *ApiDefinition) GetState() string { + return api.Spec.GetState() +} + +func (api *ApiDefinition) HasPlans() bool { + return api.Spec.HasPlans() +} + func (api *ApiDefinition) GetRef() core.ObjectRef { return &refs.NamespacedName{ Name: api.Name, diff --git a/api/v1alpha1/apiv4definition_types.go b/api/v1alpha1/apiv4definition_types.go index 05b5aa120..f4424d575 100644 --- a/api/v1alpha1/apiv4definition_types.go +++ b/api/v1alpha1/apiv4definition_types.go @@ -79,6 +79,14 @@ func (api *ApiV4Definition) GetResources() []core.ObjectOrRef[core.ResourceModel return api.Spec.GetResources() } +func (api *ApiV4Definition) GetState() string { + return api.Spec.GetState() +} + +func (api *ApiV4Definition) HasPlans() bool { + return api.Spec.HasPlans() +} + func (api *ApiV4Definition) PopulateIDs(context core.ContextModel) { api.Spec.ID = api.pickID(context) api.Spec.CrossID = api.pickCrossID() diff --git a/internal/admission/api/base/plans.go b/internal/admission/api/base/plans.go new file mode 100644 index 000000000..9e86af559 --- /dev/null +++ b/internal/admission/api/base/plans.go @@ -0,0 +1,32 @@ +// Copyright (C) 2015 The Gravitee team (http://gravitee.io) +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package base + +import ( + "github.com/gravitee-io/gravitee-kubernetes-operator/internal/core" + "github.com/gravitee-io/gravitee-kubernetes-operator/internal/errors" +) + +func validatePlans(api core.ApiDefinitionObject) *errors.AdmissionError { + if !api.HasPlans() && api.GetState() != "STOPPED" { + return errors.NewSeveref( + "cannot apply API [%s]. "+ + "Its state is set to STARTED, but the API has no plans. "+ + "APIs must have at least one plan in order to be deployed.", + api.GetName(), + ) + } + return nil +} diff --git a/internal/admission/api/base/validate.go b/internal/admission/api/base/validate.go index a215b08f5..30c341400 100644 --- a/internal/admission/api/base/validate.go +++ b/internal/admission/api/base/validate.go @@ -29,6 +29,7 @@ func ValidateCreate(ctx context.Context, obj runtime.Object) *errors.AdmissionEr errs.Add(ctxref.Validate(ctx, obj)) if api, ok := obj.(core.ApiDefinitionObject); ok { + errs.Add(validatePlans(api)) errs.Add(validateNoConflictingPath(ctx, api)) errs.MergeWith(validateResourceOrRefs(ctx, api)) } diff --git a/internal/admission/api/v4/validate.go b/internal/admission/api/v4/validate.go index bb446b3e7..7be9ecd6c 100644 --- a/internal/admission/api/v4/validate.go +++ b/internal/admission/api/v4/validate.go @@ -17,8 +17,6 @@ package v4 import ( "context" - baseModel "github.com/gravitee-io/gravitee-kubernetes-operator/api/model/api/base" - v4 "github.com/gravitee-io/gravitee-kubernetes-operator/api/model/api/v4" "github.com/gravitee-io/gravitee-kubernetes-operator/internal/admission/api/base" "github.com/gravitee-io/gravitee-kubernetes-operator/internal/apim" @@ -35,7 +33,6 @@ func validateCreate(ctx context.Context, obj runtime.Object) *errors.AdmissionEr return errs } - errs.Add(validateApiPlans(ctx, api)) if errs.IsSevere() { return errs } @@ -50,26 +47,6 @@ func validateCreate(ctx context.Context, obj runtime.Object) *errors.AdmissionEr return errs } -func validateApiPlans(_ context.Context, api core.ApiDefinitionObject) *errors.AdmissionError { - cp, _ := api.DeepCopyObject().(core.ApiDefinitionObject) - - apiDef, ok := cp.GetDefinition().(*v4.Api) - if !ok { - return errors.NewSevere("unable to validate the CRD because it is not a v4 API") - } - - if apiDef.State == baseModel.StateStarted && - len(apiDef.Plans) == 0 { - return errors.NewSeveref( - "cannot apply API [%s]. Its state is set to STARTED but the API has no plans. "+ - "APIs must have at least one plan in order to be deployed.", - apiDef.Name, - ) - } - - return nil -} - func validateDryRun(ctx context.Context, api core.ApiDefinitionObject) *errors.AdmissionErrors { errs := errors.NewAdmissionErrors() diff --git a/internal/core/interface.go b/internal/core/interface.go index 2f1a5ab3e..11b9010ed 100644 --- a/internal/core/interface.go +++ b/internal/core/interface.go @@ -60,11 +60,14 @@ type DefinitionContext interface { // +k8s:deepcopy-gen=false type ApiDefinitionModel interface { + GetName() string GetDefinitionVersion() ApiDefinitionVersion GetContextPaths() []string SetDefinitionContext(DefinitionContext) GetDefinitionContext() DefinitionContext GetResources() []ObjectOrRef[ResourceModel] + GetState() string + HasPlans() bool } // +k8s:deepcopy-gen=false diff --git a/test/integration/admission/api/v2/create_withoutContext_andNoPlansWhileStarted_test.go b/test/integration/admission/api/v2/create_withoutContext_andNoPlansWhileStarted_test.go new file mode 100644 index 000000000..2bb50c891 --- /dev/null +++ b/test/integration/admission/api/v2/create_withoutContext_andNoPlansWhileStarted_test.go @@ -0,0 +1,58 @@ +// Copyright (C) 2015 The Gravitee team (http://gravitee.io) +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package v2 + +import ( + "context" + + "github.com/gravitee-io/gravitee-kubernetes-operator/api/model/api/base" + adm "github.com/gravitee-io/gravitee-kubernetes-operator/internal/admission/api/v2" + "github.com/gravitee-io/gravitee-kubernetes-operator/internal/errors" + "github.com/gravitee-io/gravitee-kubernetes-operator/test/internal/integration/constants" + "github.com/gravitee-io/gravitee-kubernetes-operator/test/internal/integration/fixture" + "github.com/gravitee-io/gravitee-kubernetes-operator/test/internal/integration/labels" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Validate create", labels.WithoutContext, func() { + ctx := context.Background() + admissionCtrl := adm.AdmissionCtrl{} + + It("should return error on API creation with no plans and state 'STARTED'", func() { + fixtures := fixture. + Builder(). + WithAPI(constants.Api). + Build() + + By("removing plans while API is started") + + fixtures.API.Spec.Plans = nil + fixtures.API.Spec.State = base.StateStarted + + By("checking that API creation does not pass validation") + + _, err := admissionCtrl.ValidateCreate(ctx, fixtures.API) + + Expect(err).To(Equal( + errors.NewSeveref( + "cannot apply API [%s]. "+ + "Its state is set to STARTED, but the API has no plans. "+ + "APIs must have at least one plan in order to be deployed.", + fixtures.API.GetName(), + ), + )) + }) +}) diff --git a/test/integration/admission/api/v4/create_withoutContext_andNoPlans_test.go b/test/integration/admission/api/v4/create_withoutContext_andNoPlans_test.go index 065356418..b3f1b3997 100644 --- a/test/integration/admission/api/v4/create_withoutContext_andNoPlans_test.go +++ b/test/integration/admission/api/v4/create_withoutContext_andNoPlans_test.go @@ -19,7 +19,6 @@ import ( v4 "github.com/gravitee-io/gravitee-kubernetes-operator/internal/admission/api/v4" "github.com/gravitee-io/gravitee-kubernetes-operator/internal/errors" - "github.com/gravitee-io/gravitee-kubernetes-operator/test/internal/integration/assert" "github.com/gravitee-io/gravitee-kubernetes-operator/test/internal/integration/constants" "github.com/gravitee-io/gravitee-kubernetes-operator/test/internal/integration/fixture" "github.com/gravitee-io/gravitee-kubernetes-operator/test/internal/integration/labels" @@ -28,7 +27,6 @@ import ( ) var _ = Describe("Validate create", labels.WithContext, func() { - interval := constants.Interval ctx := context.Background() admissionCtrl := v4.AdmissionCtrl{} @@ -36,23 +34,22 @@ var _ = Describe("Validate create", labels.WithContext, func() { fixtures := fixture. Builder(). WithAPIv4(constants.ApiV4). - Build(). - Apply() + Build() By("removing existing plans") + clear(fixtures.APIv4.Spec.Plans) By("checking that API validation returns errors") - Eventually(func() error { - _, err := admissionCtrl.ValidateCreate(ctx, fixtures.APIv4) - - return assert.Equals( - "severe", - errors.NewSeveref("cannot apply API [%s]. Its state is set to STARTED"+ - " but the API has no plans. APIs must have at least one plan in order to"+ - " be deployed.", fixtures.APIv4.Name).Error(), - err.Error(), - ) - }, constants.EventualTimeout, interval).Should(Succeed()) + + _, err := admissionCtrl.ValidateCreate(ctx, fixtures.APIv4) + + Expect(err).To(Equal( + errors.NewSeveref("cannot apply API [%s]. Its state is set to STARTED, "+ + "but the API has no plans. APIs must have at least one plan in order to "+ + "be deployed.", + fixtures.APIv4.Name, + ), + )) }) }) diff --git a/test/integration/admission/api/v4/update_withoutContext_andNoPlans_test.go b/test/integration/admission/api/v4/update_withoutContext_andNoPlans_test.go index c3d83b6dd..0d8e62386 100644 --- a/test/integration/admission/api/v4/update_withoutContext_andNoPlans_test.go +++ b/test/integration/admission/api/v4/update_withoutContext_andNoPlans_test.go @@ -19,7 +19,6 @@ import ( v4 "github.com/gravitee-io/gravitee-kubernetes-operator/internal/admission/api/v4" "github.com/gravitee-io/gravitee-kubernetes-operator/internal/errors" - "github.com/gravitee-io/gravitee-kubernetes-operator/test/internal/integration/assert" "github.com/gravitee-io/gravitee-kubernetes-operator/test/internal/integration/constants" "github.com/gravitee-io/gravitee-kubernetes-operator/test/internal/integration/fixture" "github.com/gravitee-io/gravitee-kubernetes-operator/test/internal/integration/labels" @@ -28,7 +27,6 @@ import ( ) var _ = Describe("Validate update", labels.WithContext, func() { - interval := constants.Interval ctx := context.Background() admissionCtrl := v4.AdmissionCtrl{} @@ -36,25 +34,22 @@ var _ = Describe("Validate update", labels.WithContext, func() { fixtures := fixture. Builder(). WithAPIv4(constants.ApiV4). - Build(). - Apply() + Build() By("removing existing plans") + clear(fixtures.APIv4.Spec.Plans) By("checking that API validation returns errors") - Eventually(func() error { - _, err := admissionCtrl.ValidateUpdate(ctx, fixtures.APIv4, fixtures.APIv4) - - return assert.Equals( - "severe", - errors.NewSeveref("cannot apply API [%s]. Its state is set to STARTED"+ - " but the API has no plans. APIs must have at least one plan in order to"+ - " be deployed.", - fixtures.APIv4.Name, - ).Error(), - err.Error(), - ) - }, constants.EventualTimeout, interval).Should(Succeed()) + + _, err := admissionCtrl.ValidateUpdate(ctx, fixtures.APIv4, fixtures.APIv4) + + Expect(err).To(Equal( + errors.NewSeveref("cannot apply API [%s]. Its state is set to STARTED, "+ + "but the API has no plans. APIs must have at least one plan in order to "+ + "be deployed.", + fixtures.APIv4.Name, + ), + )) }) })