From 64b8232e52990bdc59357c6c4a8db01167176858 Mon Sep 17 00:00:00 2001 From: KevFan Date: Fri, 15 Dec 2023 14:34:41 +0000 Subject: [PATCH] refactor: interface for standard policy errors --- controllers/authpolicy_controller.go | 12 +- controllers/authpolicy_status.go | 7 +- controllers/ratelimitpolicy_controller.go | 18 ++- controllers/ratelimitpolicy_status.go | 5 +- pkg/common/apimachinery_status_conditions.go | 25 ++-- .../apimachinery_status_conditions_test.go | 110 ++++++++++++++++++ pkg/common/errors.go | 23 ++++ pkg/common/gatewayapi_utils_test.go | 22 ---- pkg/common/test_utils.go | 29 +++++ 9 files changed, 200 insertions(+), 51 deletions(-) create mode 100644 pkg/common/test_utils.go diff --git a/controllers/authpolicy_controller.go b/controllers/authpolicy_controller.go index b43073a05..0ab6a45f2 100644 --- a/controllers/authpolicy_controller.go +++ b/controllers/authpolicy_controller.go @@ -130,8 +130,8 @@ func (r *AuthPolicyReconciler) Reconcile(eventCtx context.Context, req ctrl.Requ return ctrl.Result{}, nil } -func (r *AuthPolicyReconciler) reconcileResources(ctx context.Context, ap *api.AuthPolicy, targetNetworkObject client.Object) error { - // validate +// validate performs validation before proceeding with the reconcile loop, returning a common.ErrInvalid on any failing validation +func (r *AuthPolicyReconciler) validate(ap *api.AuthPolicy, targetNetworkObject client.Object) error { if err := ap.Validate(); err != nil { return common.NewErrInvalid(ap.Kind(), err) } @@ -140,6 +140,14 @@ func (r *AuthPolicyReconciler) reconcileResources(ctx context.Context, ap *api.A return common.NewErrInvalid(ap.Kind(), err) } + return nil +} + +func (r *AuthPolicyReconciler) reconcileResources(ctx context.Context, ap *api.AuthPolicy, targetNetworkObject client.Object) error { + if err := r.validate(ap, targetNetworkObject); err != nil { + return err + } + // reconcile based on gateway diffs gatewayDiffObj, err := r.ComputeGatewayDiffs(ctx, ap, targetNetworkObject, &common.KuadrantAuthPolicyRefsConfig{}) if err != nil { diff --git a/controllers/authpolicy_status.go b/controllers/authpolicy_status.go index ba91cbac8..3c36e5d9c 100644 --- a/controllers/authpolicy_status.go +++ b/controllers/authpolicy_status.go @@ -6,14 +6,15 @@ import ( "slices" "github.com/go-logr/logr" - authorinoapi "github.com/kuadrant/authorino/api/v1beta2" - api "github.com/kuadrant/kuadrant-operator/api/v1beta2" - "github.com/kuadrant/kuadrant-operator/pkg/common" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + + authorinoapi "github.com/kuadrant/authorino/api/v1beta2" + api "github.com/kuadrant/kuadrant-operator/api/v1beta2" + "github.com/kuadrant/kuadrant-operator/pkg/common" ) // reconcileStatus makes sure status block of AuthPolicy is up-to-date. diff --git a/controllers/ratelimitpolicy_controller.go b/controllers/ratelimitpolicy_controller.go index d38ed2c64..88ac46e30 100644 --- a/controllers/ratelimitpolicy_controller.go +++ b/controllers/ratelimitpolicy_controller.go @@ -149,18 +149,24 @@ func (r *RateLimitPolicyReconciler) Reconcile(eventCtx context.Context, req ctrl return ctrl.Result{}, nil } -func (r *RateLimitPolicyReconciler) reconcileResources(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, targetNetworkObject client.Object) error { - // validate - err := rlp.Validate() - if err != nil { +// validate performs validation before proceeding with the reconcile loop, returning a common.ErrInvalid on failing validation +func (r *RateLimitPolicyReconciler) validate(rlp *kuadrantv1beta2.RateLimitPolicy, targetNetworkObject client.Object) error { + if err := rlp.Validate(); err != nil { return common.NewErrInvalid(rlp.Kind(), err) } - err = common.ValidateHierarchicalRules(rlp, targetNetworkObject) - if err != nil { + if err := common.ValidateHierarchicalRules(rlp, targetNetworkObject); err != nil { return common.NewErrInvalid(rlp.Kind(), err) } + return nil +} + +func (r *RateLimitPolicyReconciler) reconcileResources(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, targetNetworkObject client.Object) error { + if err := r.validate(rlp, targetNetworkObject); err != nil { + return err + } + // reconcile based on gateway diffs gatewayDiffObj, err := r.ComputeGatewayDiffs(ctx, rlp, targetNetworkObject, &common.KuadrantRateLimitPolicyRefsConfig{}) if err != nil { diff --git a/controllers/ratelimitpolicy_status.go b/controllers/ratelimitpolicy_status.go index 62a6149f8..f6e2bd461 100644 --- a/controllers/ratelimitpolicy_status.go +++ b/controllers/ratelimitpolicy_status.go @@ -6,12 +6,13 @@ import ( "slices" "github.com/go-logr/logr" - kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" - "github.com/kuadrant/kuadrant-operator/pkg/common" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/reconcile" + + kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" + "github.com/kuadrant/kuadrant-operator/pkg/common" ) func (r *RateLimitPolicyReconciler) reconcileStatus(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, specErr error) (ctrl.Result, error) { diff --git a/pkg/common/apimachinery_status_conditions.go b/pkg/common/apimachinery_status_conditions.go index b9f353f71..eeef51c0e 100644 --- a/pkg/common/apimachinery_status_conditions.go +++ b/pkg/common/apimachinery_status_conditions.go @@ -29,24 +29,17 @@ func AcceptedCondition(policy KuadrantPolicy, err error) *metav1.Condition { Reason: string(gatewayapiv1alpha2.PolicyReasonAccepted), Message: fmt.Sprintf("%s has been accepted", policy.Kind()), } + if err == nil { + return cond + } - if err != nil { - cond.Status = metav1.ConditionFalse - cond.Message = err.Error() + cond.Status = metav1.ConditionFalse + cond.Message = err.Error() + cond.Reason = "ReconciliationError" - switch { - // TargetNotFound - case errors.As(err, &ErrTargetNotFound{}): - cond.Reason = string(gatewayapiv1alpha2.PolicyReasonTargetNotFound) - // Invalid - case errors.As(err, &ErrInvalid{}): - cond.Reason = string(gatewayapiv1alpha2.PolicyReasonInvalid) - // Conflicted - case errors.As(err, &ErrConflict{}): - cond.Reason = string(gatewayapiv1alpha2.PolicyReasonConflicted) - default: - cond.Reason = "ReconciliationError" - } + var policyErr PolicyError + if errors.As(err, &policyErr) { + cond.Reason = string(policyErr.Reason()) } return cond diff --git a/pkg/common/apimachinery_status_conditions_test.go b/pkg/common/apimachinery_status_conditions_test.go index 29c152358..789ae9984 100644 --- a/pkg/common/apimachinery_status_conditions_test.go +++ b/pkg/common/apimachinery_status_conditions_test.go @@ -3,11 +3,16 @@ package common import ( + "fmt" + "reflect" "testing" "time" goCmp "github.com/google/go-cmp/cmp" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" ) func TestConditionMarshal(t *testing.T) { @@ -113,3 +118,108 @@ func TestConditionMarshal(t *testing.T) { }) } } + +func TestAcceptedCondition(t *testing.T) { + type args struct { + policy KuadrantPolicy + err error + } + tests := []struct { + name string + args args + want *metav1.Condition + }{ + { + name: "accepted reason", + args: args{ + policy: &FakePolicy{}, + }, + want: &metav1.Condition{ + Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayapiv1alpha2.PolicyReasonAccepted), + Message: "FakePolicy has been accepted", + }, + }, + { + name: "target not found reason", + args: args{ + policy: &FakePolicy{}, + err: NewErrTargetNotFound("FakePolicy", gatewayapiv1alpha2.PolicyTargetReference{ + Group: "gateway.networking.k8s.io", + Kind: "HTTPRoute", + Name: "my-target-ref", + }, errors.NewNotFound(schema.GroupResource{}, "my-target-ref")), + }, + want: &metav1.Condition{ + Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(gatewayapiv1alpha2.PolicyReasonTargetNotFound), + Message: "FakePolicy target my-target-ref was not found", + }, + }, + { + name: "target not found reason with err", + args: args{ + policy: &FakePolicy{}, + err: NewErrTargetNotFound("FakePolicy", gatewayapiv1alpha2.PolicyTargetReference{ + Group: "gateway.networking.k8s.io", + Kind: "HTTPRoute", + Name: "my-target-ref", + }, fmt.Errorf("deletion err")), + }, + want: &metav1.Condition{ + Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(gatewayapiv1alpha2.PolicyReasonTargetNotFound), + Message: "FakePolicy target my-target-ref was not found: deletion err", + }, + }, + { + name: "invalid reason", + args: args{ + policy: &FakePolicy{}, + err: NewErrInvalid("FakePolicy", fmt.Errorf("invalid err")), + }, + want: &metav1.Condition{ + Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(gatewayapiv1alpha2.PolicyReasonInvalid), + Message: "FakePolicy target is invalid: invalid err", + }, + }, + { + name: "conflicted reason", + args: args{ + policy: &FakePolicy{}, + err: NewErrConflict("FakePolicy", "testNs/policy1", fmt.Errorf("conflict err")), + }, + want: &metav1.Condition{ + Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(gatewayapiv1alpha2.PolicyReasonConflicted), + Message: "FakePolicy is conflicted by testNs/policy1: conflict err", + }, + }, + { + name: "reconciliation error reason", + args: args{ + policy: &FakePolicy{}, + err: fmt.Errorf("reconcile err"), + }, + want: &metav1.Condition{ + Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionFalse, + Reason: "ReconciliationError", + Message: "reconcile err", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := AcceptedCondition(tt.args.policy, tt.args.err); !reflect.DeepEqual(got, tt.want) { + t.Errorf("AcceptedCondition() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/common/errors.go b/pkg/common/errors.go index 83c011a0d..2d0e7df78 100644 --- a/pkg/common/errors.go +++ b/pkg/common/errors.go @@ -7,6 +7,13 @@ import ( gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" ) +type PolicyError interface { + error + Reason() gatewayapiv1alpha2.PolicyConditionReason +} + +var _ PolicyError = ErrTargetNotFound{} + type ErrTargetNotFound struct { Kind string TargetRef gatewayapiv1alpha2.PolicyTargetReference @@ -21,6 +28,10 @@ func (e ErrTargetNotFound) Error() string { return fmt.Sprintf("%s target %s was not found: %s", e.Kind, e.TargetRef.Name, e.Err.Error()) } +func (e ErrTargetNotFound) Reason() gatewayapiv1alpha2.PolicyConditionReason { + return gatewayapiv1alpha2.PolicyReasonTargetNotFound +} + func NewErrTargetNotFound(kind string, targetRef gatewayapiv1alpha2.PolicyTargetReference, err error) ErrTargetNotFound { return ErrTargetNotFound{ Kind: kind, @@ -29,6 +40,8 @@ func NewErrTargetNotFound(kind string, targetRef gatewayapiv1alpha2.PolicyTarget } } +var _ PolicyError = ErrInvalid{} + type ErrInvalid struct { Kind string Err error @@ -38,6 +51,10 @@ func (e ErrInvalid) Error() string { return fmt.Sprintf("%s target is invalid: %s", e.Kind, e.Err.Error()) } +func (e ErrInvalid) Reason() gatewayapiv1alpha2.PolicyConditionReason { + return gatewayapiv1alpha2.PolicyReasonInvalid +} + func NewErrInvalid(kind string, err error) ErrInvalid { return ErrInvalid{ Kind: kind, @@ -45,6 +62,8 @@ func NewErrInvalid(kind string, err error) ErrInvalid { } } +var _ PolicyError = ErrConflict{} + type ErrConflict struct { Kind string NameNamespace string @@ -55,6 +74,10 @@ func (e ErrConflict) Error() string { return fmt.Sprintf("%s is conflicted by %s: %s", e.Kind, e.NameNamespace, e.Err.Error()) } +func (e ErrConflict) Reason() gatewayapiv1alpha2.PolicyConditionReason { + return gatewayapiv1alpha2.PolicyReasonConflicted +} + func NewErrConflict(kind string, nameNamespace string, err error) ErrConflict { return ErrConflict{ Kind: kind, diff --git a/pkg/common/gatewayapi_utils_test.go b/pkg/common/gatewayapi_utils_test.go index d5bf0b4aa..c15f1a1d9 100644 --- a/pkg/common/gatewayapi_utils_test.go +++ b/pkg/common/gatewayapi_utils_test.go @@ -1302,28 +1302,6 @@ func TestGetKuadrantNamespaceFromPolicyTargetRef(t *testing.T) { } } -type FakePolicy struct { - client.Object - Hosts []string - targetRef gatewayapiv1alpha2.PolicyTargetReference -} - -func (p *FakePolicy) GetTargetRef() gatewayapiv1alpha2.PolicyTargetReference { - return p.targetRef -} - -func (p *FakePolicy) GetWrappedNamespace() gatewayapiv1.Namespace { - return gatewayapiv1.Namespace(p.GetNamespace()) -} - -func (p *FakePolicy) GetRulesHostnames() []string { - return p.Hosts -} - -func (p *FakePolicy) Kind() string { - return "FakePolicy" -} - func TestValidateHierarchicalRules(t *testing.T) { hostname := gatewayapiv1.Hostname("*.example.com") gateway := &gatewayapiv1.Gateway{ diff --git a/pkg/common/test_utils.go b/pkg/common/test_utils.go new file mode 100644 index 000000000..8195b085d --- /dev/null +++ b/pkg/common/test_utils.go @@ -0,0 +1,29 @@ +package common + +import ( + "sigs.k8s.io/controller-runtime/pkg/client" + gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" +) + +type FakePolicy struct { + client.Object + Hosts []string + targetRef gatewayapiv1alpha2.PolicyTargetReference +} + +func (p *FakePolicy) GetTargetRef() gatewayapiv1alpha2.PolicyTargetReference { + return p.targetRef +} + +func (p *FakePolicy) GetWrappedNamespace() gatewayapiv1.Namespace { + return gatewayapiv1.Namespace(p.GetNamespace()) +} + +func (p *FakePolicy) GetRulesHostnames() []string { + return p.Hosts +} + +func (p *FakePolicy) Kind() string { + return "FakePolicy" +}