From d01d7fcc755a120096a42bf9012d9d2e3643d317 Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 11 Nov 2024 15:26:27 +0000 Subject: [PATCH] refactor: common missing dependency errors Signed-off-by: KevFan --- controllers/auth_policies_validator.go | 10 ++- controllers/data_plane_policies_workflow.go | 5 +- controllers/dnspolicies_validator.go | 4 +- controllers/kuadrant_status_updater.go | 33 +++++--- controllers/ratelimit_policies_validator.go | 9 +- controllers/tlspolicies_validator.go | 4 +- pkg/kuadrant/errors.go | 26 ++++++ tests/bare_k8s/kuadrant_controller_test.go | 2 +- tests/gatewayapi/kuadrant_controller_test.go | 89 +++++++++++++++++--- 9 files changed, 149 insertions(+), 33 deletions(-) diff --git a/controllers/auth_policies_validator.go b/controllers/auth_policies_validator.go index 2de5372dc..8f66dec2b 100644 --- a/controllers/auth_policies_validator.go +++ b/controllers/auth_policies_validator.go @@ -17,6 +17,7 @@ import ( type AuthPolicyValidator struct { isGatewayAPIInstalled bool + isGatewayProviderInstalled bool isAuthorinoOperatorInstalled bool } @@ -45,12 +46,17 @@ func (r *AuthPolicyValidator) Validate(ctx context.Context, _ []controller.Resou state.Store(StateAuthPolicyValid, lo.SliceToMap(policies, func(policy machinery.Policy) (string, error) { if !r.isGatewayAPIInstalled { - return policy.GetLocator(), kuadrant.NewErrDependencyNotInstalled("Gateway API") + return policy.GetLocator(), kuadrant.MissingGatewayAPIError() + } + + if !r.isGatewayProviderInstalled { + return policy.GetLocator(), kuadrant.MissingGatewayProviderError() } if !r.isAuthorinoOperatorInstalled { - return policy.GetLocator(), kuadrant.NewErrDependencyNotInstalled("Authorino Operator") + return policy.GetLocator(), kuadrant.MissingAuthorinoOperatorError() } + var err error if len(policy.GetTargetRefs()) > 0 && len(topology.Targetables().Children(policy)) == 0 { ref := policy.GetTargetRefs()[0] diff --git a/controllers/data_plane_policies_workflow.go b/controllers/data_plane_policies_workflow.go index 3c094cf6d..6dc5c17b2 100644 --- a/controllers/data_plane_policies_workflow.go +++ b/controllers/data_plane_policies_workflow.go @@ -55,10 +55,11 @@ var ( //+kubebuilder:rbac:groups=kuadrant.io,resources=ratelimitpolicies/finalizers,verbs=update func NewDataPlanePoliciesWorkflow(client *dynamic.DynamicClient, isGatewayAPInstalled, isIstioInstalled, isEnvoyGatewayInstalled, isLimitadorOperatorInstalled, isAuthorinoOperatorInstalled bool) *controller.Workflow { + isGatewayProviderInstalled := isIstioInstalled || isEnvoyGatewayInstalled dataPlanePoliciesValidation := &controller.Workflow{ Tasks: []controller.ReconcileFunc{ - (&AuthPolicyValidator{isGatewayAPIInstalled: isGatewayAPInstalled, isAuthorinoOperatorInstalled: isAuthorinoOperatorInstalled}).Subscription().Reconcile, - (&RateLimitPolicyValidator{isGatewayAPIInstalled: isGatewayAPInstalled, isLimitadorOperatorInstalled: isLimitadorOperatorInstalled}).Subscription().Reconcile, + (&AuthPolicyValidator{isGatewayAPIInstalled: isGatewayAPInstalled, isAuthorinoOperatorInstalled: isAuthorinoOperatorInstalled, isGatewayProviderInstalled: isGatewayProviderInstalled}).Subscription().Reconcile, + (&RateLimitPolicyValidator{isGatewayAPIInstalled: isGatewayAPInstalled, isLimitadorOperatorInstalled: isLimitadorOperatorInstalled, isGatewayProviderInstalled: isGatewayProviderInstalled}).Subscription().Reconcile, }, } diff --git a/controllers/dnspolicies_validator.go b/controllers/dnspolicies_validator.go index 20321f0de..a84dccdbb 100644 --- a/controllers/dnspolicies_validator.go +++ b/controllers/dnspolicies_validator.go @@ -52,11 +52,11 @@ func (r *DNSPoliciesValidator) validate(ctx context.Context, _ []controller.Reso state.Store(StateDNSPolicyAcceptedKey, lo.SliceToMap(policies, func(p machinery.Policy) (string, error) { if !r.isGatewayAPIInstalled { - return p.GetLocator(), kuadrant.NewErrDependencyNotInstalled("Gateway API") + return p.GetLocator(), kuadrant.MissingGatewayAPIError() } if !r.isDNSOperatorInstalled { - return p.GetLocator(), kuadrant.NewErrDependencyNotInstalled("DNS Operator") + return p.GetLocator(), kuadrant.MissingDNSOperatorError() } policy := p.(*kuadrantv1.DNSPolicy) diff --git a/controllers/kuadrant_status_updater.go b/controllers/kuadrant_status_updater.go index 66f37e8de..2a6c8f8d2 100644 --- a/controllers/kuadrant_status_updater.go +++ b/controllers/kuadrant_status_updater.go @@ -29,7 +29,7 @@ const ( type KuadrantStatusUpdater struct { Client *dynamic.DynamicClient isGatwayAPIInstalled bool - HasGateway bool + isGatewayProviderInstalled bool isLimitadorOperatorInstalled bool isAuthorinoOperatorInstalled bool } @@ -38,7 +38,7 @@ func NewKuadrantStatusUpdater(client *dynamic.DynamicClient, isGatewayAPIInstall return &KuadrantStatusUpdater{ Client: client, isGatwayAPIInstalled: isGatewayAPIInstalled, - HasGateway: isGatewayProviderInstalled, + isGatewayProviderInstalled: isGatewayProviderInstalled, isLimitadorOperatorInstalled: isLimitadorOperatorInstalled, isAuthorinoOperatorInstalled: isAuthorinoOperatorInstalled, } @@ -130,23 +130,34 @@ func (r *KuadrantStatusUpdater) readyCondition(topology *machinery.Topology, log } if !r.isGatwayAPIInstalled { + err := kuadrant.MissingGatewayAPIError() cond.Status = metav1.ConditionFalse - cond.Reason = "GatewayAPI" - cond.Message = kuadrant.NewErrDependencyNotInstalled("Gateway API").Error() + cond.Reason = string(err.Reason()) + cond.Message = err.Error() return cond } - if !r.HasGateway { + if !r.isGatewayProviderInstalled { + err := kuadrant.MissingGatewayProviderError() cond.Status = metav1.ConditionFalse - cond.Reason = "GatewayAPIProviderNotFound" - cond.Message = kuadrant.NewErrDependencyNotInstalled("GatewayAPI provider").Error() + cond.Reason = string(err.Reason()) + cond.Message = err.Error() return cond } if !r.isLimitadorOperatorInstalled { + err := kuadrant.MissingLimitadorOperatorError() cond.Status = metav1.ConditionFalse - cond.Reason = "LimitadorAPIProviderNotFound" - cond.Message = kuadrant.NewErrDependencyNotInstalled("Limitador Operator").Error() + cond.Reason = string(err.Reason()) + cond.Message = err.Error() + return cond + } + + if !r.isAuthorinoOperatorInstalled { + err := kuadrant.MissingAuthorinoOperatorError() + cond.Status = metav1.ConditionFalse + cond.Reason = string(err.Reason()) + cond.Message = err.Error() return cond } @@ -174,7 +185,7 @@ func checkLimitadorReady(topology *machinery.Topology, logger logr.Logger) *stri return ptr.To(err.Error()) } - statusConditionReady := meta.FindStatusCondition(limitadorObj.Status.Conditions, "Ready") + statusConditionReady := meta.FindStatusCondition(limitadorObj.Status.Conditions, limitadorv1alpha1.StatusConditionReady) if statusConditionReady == nil { return ptr.To("Ready condition not found") } @@ -192,7 +203,7 @@ func checkAuthorinoAvailable(topology *machinery.Topology, logger logr.Logger) * return ptr.To(err.Error()) } - readyCondition := authorino.FindAuthorinoStatusCondition(authorinoObj.Status.Conditions, limitadorv1alpha1.StatusConditionReady) + readyCondition := authorino.FindAuthorinoStatusCondition(authorinoObj.Status.Conditions, "Ready") if readyCondition == nil { return ptr.To("Ready condition not found") } diff --git a/controllers/ratelimit_policies_validator.go b/controllers/ratelimit_policies_validator.go index f648b649e..9657c9b12 100644 --- a/controllers/ratelimit_policies_validator.go +++ b/controllers/ratelimit_policies_validator.go @@ -18,6 +18,7 @@ import ( type RateLimitPolicyValidator struct { isLimitadorOperatorInstalled bool isGatewayAPIInstalled bool + isGatewayProviderInstalled bool } // RateLimitPolicyValidator subscribes to events with potential to flip the validity of rate limit policies @@ -45,11 +46,15 @@ func (r *RateLimitPolicyValidator) Validate(ctx context.Context, _ []controller. state.Store(StateRateLimitPolicyValid, lo.SliceToMap(policies, func(policy machinery.Policy) (string, error) { if !r.isGatewayAPIInstalled { - return policy.GetLocator(), kuadrant.NewErrDependencyNotInstalled("Gateway API") + return policy.GetLocator(), kuadrant.MissingGatewayAPIError() + } + + if !r.isGatewayProviderInstalled { + return policy.GetLocator(), kuadrant.MissingGatewayProviderError() } if !r.isLimitadorOperatorInstalled { - return policy.GetLocator(), kuadrant.NewErrDependencyNotInstalled("Limitador Operator") + return policy.GetLocator(), kuadrant.MissingLimitadorOperatorError() } var err error diff --git a/controllers/tlspolicies_validator.go b/controllers/tlspolicies_validator.go index 630e86d2c..3432c584f 100644 --- a/controllers/tlspolicies_validator.go +++ b/controllers/tlspolicies_validator.go @@ -52,11 +52,11 @@ func (t *TLSPoliciesValidator) Validate(ctx context.Context, _ []controller.Reso state.Store(TLSPolicyAcceptedKey, lo.SliceToMap(policies, func(p machinery.Policy) (string, error) { if !t.isGatewayAPIInstalled { - return p.GetLocator(), kuadrant.NewErrDependencyNotInstalled("Gateway API") + return p.GetLocator(), kuadrant.MissingGatewayAPIError() } if !t.isCertManagerInstalled { - return p.GetLocator(), kuadrant.NewErrDependencyNotInstalled("Cert Manager") + return p.GetLocator(), kuadrant.MissingCertManagerError() } policy := p.(*kuadrantv1.TLSPolicy) diff --git a/pkg/kuadrant/errors.go b/pkg/kuadrant/errors.go index 3d272ca6d..85d0222ca 100644 --- a/pkg/kuadrant/errors.go +++ b/pkg/kuadrant/errors.go @@ -238,3 +238,29 @@ func (e ErrDependencyNotInstalled) Error() string { func (e ErrDependencyNotInstalled) Reason() gatewayapiv1alpha2.PolicyConditionReason { return PolicyReasonMissingDependency } + +// Common ErrDependencyNotInstalled errors + +func MissingGatewayAPIError() PolicyError { + return NewErrDependencyNotInstalled("Gateway API") +} + +func MissingGatewayProviderError() PolicyError { + return NewErrDependencyNotInstalled("Gateway API provider (istio / envoy gateway)") +} + +func MissingAuthorinoOperatorError() PolicyError { + return NewErrDependencyNotInstalled("Authorino Operator") +} + +func MissingLimitadorOperatorError() PolicyError { + return NewErrDependencyNotInstalled("Limitador Operator") +} + +func MissingDNSOperatorError() PolicyError { + return NewErrDependencyNotInstalled("DNS Operator") +} + +func MissingCertManagerError() PolicyError { + return NewErrDependencyNotInstalled("Cert Manager") +} diff --git a/tests/bare_k8s/kuadrant_controller_test.go b/tests/bare_k8s/kuadrant_controller_test.go index 164195352..e36d854ba 100644 --- a/tests/bare_k8s/kuadrant_controller_test.go +++ b/tests/bare_k8s/kuadrant_controller_test.go @@ -55,7 +55,7 @@ var _ = Describe("Kuadrant controller is disabled", func() { cond := meta.FindStatusCondition(kuadrantCR.Status.Conditions, controllers.ReadyConditionType) g.Expect(cond).ToNot(BeNil()) g.Expect(cond.Status).To(Equal(metav1.ConditionFalse)) - g.Expect(cond.Reason).To(Equal("GatewayAPI")) + g.Expect(cond.Reason).To(Equal("MissingDependency")) g.Expect(cond.Message).To(Equal("Gateway API is not installed, please restart pod once dependency has been installed")) }).WithContext(ctx).Should(Succeed()) }, testTimeOut) diff --git a/tests/gatewayapi/kuadrant_controller_test.go b/tests/gatewayapi/kuadrant_controller_test.go index ec02ddf05..c30ca7fa1 100644 --- a/tests/gatewayapi/kuadrant_controller_test.go +++ b/tests/gatewayapi/kuadrant_controller_test.go @@ -10,7 +10,10 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + kuadrantv1 "github.com/kuadrant/kuadrant-operator/api/v1" kuadrantv1beta1 "github.com/kuadrant/kuadrant-operator/api/v1beta1" "github.com/kuadrant/kuadrant-operator/controllers" "github.com/kuadrant/kuadrant-operator/tests" @@ -19,8 +22,8 @@ import ( var _ = Describe("Kuadrant controller when gateway provider is missing", func() { var ( testNamespace string - kuadrantName string = "local" - afterEachTimeOut = NodeTimeout(3 * time.Minute) + testTimeOut = SpecTimeout(15 * time.Second) + afterEachTimeOut = NodeTimeout(3 * time.Minute) ) BeforeEach(func(ctx SpecContext) { @@ -32,29 +35,93 @@ var _ = Describe("Kuadrant controller when gateway provider is missing", func() }, afterEachTimeOut) Context("when default kuadrant CR is created", func() { - It("Status reports error", func(ctx SpecContext) { + It("Status reports missing Gateway API provider (istio / envoy gateway)", func(ctx SpecContext) { kuadrantCR := &kuadrantv1beta1.Kuadrant{ TypeMeta: metav1.TypeMeta{ Kind: "Kuadrant", APIVersion: kuadrantv1beta1.GroupVersion.String(), }, ObjectMeta: metav1.ObjectMeta{ - Name: kuadrantName, + Name: "local", Namespace: testNamespace, }, } Expect(testClient().Create(ctx, kuadrantCR)).ToNot(HaveOccurred()) Eventually(func(g Gomega) { - kObj := &kuadrantv1beta1.Kuadrant{} - err := testClient().Get(ctx, client.ObjectKeyFromObject(kuadrantCR), kObj) + g.Expect(testClient().Get(ctx, client.ObjectKeyFromObject(kuadrantCR), kuadrantCR)).ToNot(HaveOccurred()) + cond := meta.FindStatusCondition(kuadrantCR.Status.Conditions, controllers.ReadyConditionType) + g.Expect(cond).ToNot(BeNil()) + g.Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + g.Expect(cond.Reason).To(Equal("MissingDependency")) + g.Expect(cond.Message).To(Equal("Gateway API provider (istio / envoy gateway) is not installed, please restart pod once dependency has been installed")) + }).WithContext(ctx).Should(Succeed()) + }, testTimeOut) + }) + + Context("when rate limit policy is created", func() { + It("Status is populated with missing Gateway API provider (istio / envoy gateway)", func(ctx SpecContext) { + policy := &kuadrantv1.RateLimitPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rlp", + Namespace: testNamespace, + }, + Spec: kuadrantv1.RateLimitPolicySpec{ + TargetRef: gatewayapiv1alpha2.LocalPolicyTargetReferenceWithSectionName{ + LocalPolicyTargetReference: gatewayapiv1alpha2.LocalPolicyTargetReference{ + Kind: "Gateway", + Group: gatewayapiv1.GroupName, + Name: "test", + }, + }, + }, + } + + Expect(testClient().Create(ctx, policy)).To(Succeed()) + + Eventually(func(g Gomega) { + err := testClient().Get(ctx, client.ObjectKeyFromObject(policy), policy) g.Expect(err).ToNot(HaveOccurred()) - cond := meta.FindStatusCondition(kObj.Status.Conditions, string(controllers.ReadyConditionType)) + + cond := meta.FindStatusCondition(policy.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) + g.Expect(cond).ToNot(BeNil()) + g.Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + g.Expect(cond.Reason).To(Equal("MissingDependency")) + g.Expect(cond.Message).To(Equal("Gateway API provider (istio / envoy gateway) is not installed, please restart pod once dependency has been installed")) + }).WithContext(ctx).Should(Succeed()) + }, testTimeOut) + }) + + Context("when auth policy is created", func() { + It("Status is populated with missing Gateway API provider (istio / envoy gateway)", func(ctx SpecContext) { + policy := &kuadrantv1.AuthPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth", + Namespace: testNamespace, + }, + Spec: kuadrantv1.AuthPolicySpec{ + TargetRef: gatewayapiv1alpha2.LocalPolicyTargetReferenceWithSectionName{ + LocalPolicyTargetReference: gatewayapiv1alpha2.LocalPolicyTargetReference{ + Kind: "Gateway", + Group: gatewayapiv1.GroupName, + Name: "test", + }, + }, + }, + } + + Expect(testClient().Create(ctx, policy)).To(Succeed()) + + Eventually(func(g Gomega) { + err := testClient().Get(ctx, client.ObjectKeyFromObject(policy), policy) + g.Expect(err).ToNot(HaveOccurred()) + + cond := meta.FindStatusCondition(policy.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) g.Expect(cond).ToNot(BeNil()) g.Expect(cond.Status).To(Equal(metav1.ConditionFalse)) - g.Expect(cond.Reason).To(Equal("GatewayAPIProviderNotFound")) - g.Expect(cond.Message).To(Equal("GatewayAPI provider is not installed, please restart pod once dependency has been installed")) - }, time.Minute, 15*time.Second).WithContext(ctx).Should(Succeed()) - }) + g.Expect(cond.Reason).To(Equal("MissingDependency")) + g.Expect(cond.Message).To(Equal("Gateway API provider (istio / envoy gateway) is not installed, please restart pod once dependency has been installed")) + }).WithContext(ctx).Should(Succeed()) + }, testTimeOut) }) })