diff --git a/api/v1beta2/authpolicy_types.go b/api/v1beta2/authpolicy_types.go index d380d1a76..5e7cc687a 100644 --- a/api/v1beta2/authpolicy_types.go +++ b/api/v1beta2/authpolicy_types.go @@ -197,6 +197,8 @@ func (s *AuthPolicyStatus) Equals(other *AuthPolicyStatus, logger logr.Logger) b return true } +var _ common.KuadrantPolicy = &AuthPolicy{} + // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:metadata:labels="gateway.networking.k8s.io/policy=direct" diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index fd369bc38..4e0cccb0e 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -163,6 +163,8 @@ func (s *RateLimitPolicyStatus) Equals(other *RateLimitPolicyStatus, logger logr return true } +var _ common.KuadrantPolicy = &RateLimitPolicy{} + // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:metadata:labels="gateway.networking.k8s.io/policy=direct" diff --git a/controllers/authpolicy_controller.go b/controllers/authpolicy_controller.go index fe28dc12c..b43073a05 100644 --- a/controllers/authpolicy_controller.go +++ b/controllers/authpolicy_controller.go @@ -67,7 +67,7 @@ func (r *AuthPolicyReconciler) Reconcile(eventCtx context.Context, req ctrl.Requ if delResErr == nil { delResErr = err } - return r.reconcileStatus(ctx, ap, common.ErrTargetNotFound{Kind: ap.Kind(), TargetRef: ap.GetTargetRef(), Err: delResErr}) + return r.reconcileStatus(ctx, ap, common.NewErrTargetNotFound(ap.Kind(), ap.GetTargetRef(), delResErr)) } return ctrl.Result{}, err } @@ -133,11 +133,11 @@ func (r *AuthPolicyReconciler) Reconcile(eventCtx context.Context, req ctrl.Requ func (r *AuthPolicyReconciler) reconcileResources(ctx context.Context, ap *api.AuthPolicy, targetNetworkObject client.Object) error { // validate if err := ap.Validate(); err != nil { - return common.ErrInvalid{Kind: ap.Kind(), Err: err} + return common.NewErrInvalid(ap.Kind(), err) } if err := common.ValidateHierarchicalRules(ap, targetNetworkObject); err != nil { - return common.ErrInvalid{Kind: ap.Kind(), Err: err} + return common.NewErrInvalid(ap.Kind(), err) } // reconcile based on gateway diffs diff --git a/controllers/authpolicy_controller_test.go b/controllers/authpolicy_controller_test.go index 783805309..f9be1ac56 100644 --- a/controllers/authpolicy_controller_test.go +++ b/controllers/authpolicy_controller_test.go @@ -1095,15 +1095,8 @@ var _ = Describe("AuthPolicy controller", func() { }) Context("AuthPolicy accepted condition reasons", func() { - It("Target not found reason", func() { - policy := apFactory(nil) - - err := k8sClient.Create(context.Background(), policy) - logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(policy).String(), "error", err) - Expect(err).ToNot(HaveOccurred()) - - // check policy status - Eventually(func() bool { + assertAcceptedConditionFalse := func(policy *api.AuthPolicy, reason, message string) func() bool { + return func() bool { existingPolicy := &api.AuthPolicy{} err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(policy), existingPolicy) if err != nil { @@ -1114,10 +1107,22 @@ var _ = Describe("AuthPolicy controller", func() { return false } - return cond.Status == metav1.ConditionFalse && cond.Reason == string(gatewayapiv1alpha2.PolicyReasonTargetNotFound) && - cond.Message == fmt.Sprintf("AuthPolicy target %s was not found", testHTTPRouteName) + return cond.Status == metav1.ConditionFalse && cond.Reason == reason && cond.Message == message + } + } - }, 30*time.Second, 5*time.Second).Should(BeTrue()) + // Accepted reason is already tested generally by the existing tests + + It("Target not found reason", func() { + policy := apFactory(nil) + + err := k8sClient.Create(context.Background(), policy) + logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(policy).String(), "error", err) + Expect(err).ToNot(HaveOccurred()) + + // check policy status + Eventually(assertAcceptedConditionFalse(policy, string(gatewayapiv1alpha2.PolicyReasonTargetNotFound), + fmt.Sprintf("AuthPolicy target %s was not found", testHTTPRouteName)), 30*time.Second, 5*time.Second).Should(BeTrue()) }) It("Conflict reason", func() { route := testBuildBasicHttpRoute(testHTTPRouteName, testGatewayName, testNamespace, []string{"*.toystore.com"}) @@ -1138,48 +1143,26 @@ var _ = Describe("AuthPolicy controller", func() { Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(func() bool { - existingPolicy := &api.AuthPolicy{} - err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(policy2), existingPolicy) - if err != nil { - return false - } - cond := meta.FindStatusCondition(existingPolicy.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) - if cond == nil { - return false - } - - return cond.Status == metav1.ConditionFalse && cond.Reason == string(gatewayapiv1alpha2.PolicyReasonConflicted) && - cond.Message == fmt.Sprintf("AuthPolicy is conflicted by %[1]v/toystore: the gateway.networking.k8s.io/v1, Kind=HTTPRoute target %[1]v/toystore-route is already referenced by policy %[1]v/toystore", testNamespace) - - }, 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(assertAcceptedConditionFalse(policy2, string(gatewayapiv1alpha2.PolicyReasonConflicted), + fmt.Sprintf("AuthPolicy is conflicted by %[1]v/toystore: the gateway.networking.k8s.io/v1, Kind=HTTPRoute target %[1]v/toystore-route is already referenced by policy %[1]v/toystore", testNamespace), + ), 30*time.Second, 5*time.Second).Should(BeTrue()) }) It("Invalid reason", func() { + const targetRefName, targetRefNamespace = "istio-ingressgateway", "istio-system" + policy := apFactory(func(policy *api.AuthPolicy) { policy.Spec.TargetRef.Kind = "Gateway" - policy.Spec.TargetRef.Namespace = ptr.To(gatewayapiv1.Namespace("istio-system")) - policy.Spec.TargetRef.Name = "istio-ingressgateway" + policy.Spec.TargetRef.Name = targetRefName + policy.Spec.TargetRef.Namespace = ptr.To(gatewayapiv1.Namespace(targetRefNamespace)) }) err := k8sClient.Create(context.Background(), policy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(policy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) - Eventually(func() bool { - existingPolicy := &api.AuthPolicy{} - err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(policy), existingPolicy) - if err != nil { - return false - } - cond := meta.FindStatusCondition(existingPolicy.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) - if cond == nil { - return false - } - - return cond.Status == metav1.ConditionFalse && cond.Reason == string(gatewayapiv1alpha2.PolicyReasonInvalid) && - cond.Message == "AuthPolicy target is invalid: invalid targetRef.Namespace istio-system. Currently only supporting references to the same namespace" - - }, 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(assertAcceptedConditionFalse(policy, string(gatewayapiv1alpha2.PolicyReasonInvalid), + fmt.Sprintf("AuthPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", targetRefNamespace), + ), 30*time.Second, 5*time.Second).Should(BeTrue()) }) }) }) @@ -1194,7 +1177,7 @@ var _ = Describe("AuthPolicy CEL Validations", func() { AfterEach(DeleteNamespaceCallback(&testNamespace)) Context("Spec TargetRef Validations", func() { - It("Valid policy targeting HTTPRoute", func() { + policyFactory := func(mutateFn func(policy *api.AuthPolicy)) *api.AuthPolicy { policy := &api.AuthPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: "my-policy", @@ -1204,65 +1187,45 @@ var _ = Describe("AuthPolicy CEL Validations", func() { TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ Group: "gateway.networking.k8s.io", Kind: "HTTPRoute", - Name: "my-route", + Name: "my-target", }, }, } + + if mutateFn != nil { + mutateFn(policy) + } + + return policy + } + + It("Valid policy targeting HTTPRoute", func() { + policy := policyFactory(nil) err := k8sClient.Create(context.Background(), policy) Expect(err).To(BeNil()) }) It("Valid policy targeting Gateway", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "Gateway", - Name: "my-gw", - }, - }, - } + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + }) err := k8sClient.Create(context.Background(), policy) Expect(err).To(BeNil()) }) It("Invalid Target Ref Group", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "not-gateway.networking.k8s.io", - Kind: "HTTPRoute", - Name: "my-route", - }, - }, - } + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.TargetRef.Group = "not-gateway.networking.k8s.io" + }) err := k8sClient.Create(context.Background(), policy) Expect(err).To(Not(BeNil())) Expect(strings.Contains(err.Error(), "Invalid targetRef.group. The only supported value is 'gateway.networking.k8s.io'")).To(BeTrue()) }) It("Invalid Target Ref Kind", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "TCPRoute", - Name: "my-route", - }, - }, - } + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.TargetRef.Kind = "TCPRoute" + }) err := k8sClient.Create(context.Background(), policy) Expect(err).To(Not(BeNil())) Expect(strings.Contains(err.Error(), "Invalid targetRef.kind. The only supported values are 'HTTPRoute' and 'Gateway'")).To(BeTrue()) @@ -1291,7 +1254,7 @@ var _ = Describe("AuthPolicy CEL Validations", func() { commonAuthRuleSpec = api.CommonAuthRuleSpec{RouteSelectors: routeSelectors} ) - It("invalid usage of top-level route selectors with a gateway targetRef", func() { + policyFactory := func(mutateFn func(policy *api.AuthPolicy)) *api.AuthPolicy { policy := &api.AuthPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: "my-policy", @@ -1303,41 +1266,40 @@ var _ = Describe("AuthPolicy CEL Validations", func() { Kind: "Gateway", Name: "my-gw", }, - RouteSelectors: routeSelectors, }, } + if mutateFn != nil { + mutateFn(policy) + } + + return policy + } + It("invalid usage of top-level route selectors with a gateway targetRef", func() { + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.RouteSelectors = routeSelectors + }) + err := k8sClient.Create(context.Background(), policy) Expect(err).To(Not(BeNil())) Expect(strings.Contains(err.Error(), gateWayRouteSelectorErrorMessage)).To(BeTrue()) }) It("invalid usage of config-level route selectors with a gateway targetRef - authentication", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "Gateway", - Name: "my-gw", - }, - AuthScheme: api.AuthSchemeSpec{ - Authentication: map[string]api.AuthenticationSpec{ - "my-rule": { - AuthenticationSpec: authorinoapi.AuthenticationSpec{ - AuthenticationMethodSpec: authorinoapi.AuthenticationMethodSpec{ - AnonymousAccess: &authorinoapi.AnonymousAccessSpec{}, - }, + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.AuthScheme = api.AuthSchemeSpec{ + Authentication: map[string]api.AuthenticationSpec{ + "my-rule": { + AuthenticationSpec: authorinoapi.AuthenticationSpec{ + AuthenticationMethodSpec: authorinoapi.AuthenticationMethodSpec{ + AnonymousAccess: &authorinoapi.AnonymousAccessSpec{}, }, - CommonAuthRuleSpec: commonAuthRuleSpec, }, + CommonAuthRuleSpec: commonAuthRuleSpec, }, }, - }, - } + } + }) err := k8sClient.Create(context.Background(), policy) Expect(err).To(Not(BeNil())) @@ -1345,26 +1307,15 @@ var _ = Describe("AuthPolicy CEL Validations", func() { }) It("invalid usage of config-level route selectors with a gateway targetRef - metadata", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "Gateway", - Name: "my-gw", - }, - AuthScheme: api.AuthSchemeSpec{ - Metadata: map[string]api.MetadataSpec{ - "my-metadata": { - CommonAuthRuleSpec: commonAuthRuleSpec, - }, + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.AuthScheme = api.AuthSchemeSpec{ + Metadata: map[string]api.MetadataSpec{ + "my-metadata": { + CommonAuthRuleSpec: commonAuthRuleSpec, }, }, - }, - } + } + }) err := k8sClient.Create(context.Background(), policy) Expect(err).To(Not(BeNil())) @@ -1372,26 +1323,15 @@ var _ = Describe("AuthPolicy CEL Validations", func() { }) It("invalid usage of config-level route selectors with a gateway targetRef - authorization", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "Gateway", - Name: "my-gw", - }, - AuthScheme: api.AuthSchemeSpec{ - Authorization: map[string]api.AuthorizationSpec{ - "my-authZ": { - CommonAuthRuleSpec: commonAuthRuleSpec, - }, + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.AuthScheme = api.AuthSchemeSpec{ + Authorization: map[string]api.AuthorizationSpec{ + "my-authZ": { + CommonAuthRuleSpec: commonAuthRuleSpec, }, }, - }, - } + } + }) err := k8sClient.Create(context.Background(), policy) Expect(err).To(Not(BeNil())) @@ -1399,32 +1339,21 @@ var _ = Describe("AuthPolicy CEL Validations", func() { }) It("invalid usage of config-level route selectors with a gateway targetRef - response success headers", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "Gateway", - Name: "my-gw", - }, - AuthScheme: api.AuthSchemeSpec{ - Response: &api.ResponseSpec{ - Success: api.WrappedSuccessResponseSpec{ - Headers: map[string]api.HeaderSuccessResponseSpec{ - "header": { - SuccessResponseSpec: api.SuccessResponseSpec{ - CommonAuthRuleSpec: commonAuthRuleSpec, - }, + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.AuthScheme = api.AuthSchemeSpec{ + Response: &api.ResponseSpec{ + Success: api.WrappedSuccessResponseSpec{ + Headers: map[string]api.HeaderSuccessResponseSpec{ + "header": { + SuccessResponseSpec: api.SuccessResponseSpec{ + CommonAuthRuleSpec: commonAuthRuleSpec, }, }, }, }, }, - }, - } + } + }) err := k8sClient.Create(context.Background(), policy) Expect(err).To(Not(BeNil())) @@ -1432,30 +1361,19 @@ var _ = Describe("AuthPolicy CEL Validations", func() { }) It("invalid usage of config-level route selectors with a gateway targetRef - response success dynamic metadata", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "Gateway", - Name: "my-gw", - }, - AuthScheme: api.AuthSchemeSpec{ - Response: &api.ResponseSpec{ - Success: api.WrappedSuccessResponseSpec{ - DynamicMetadata: map[string]api.SuccessResponseSpec{ - "header": { - CommonAuthRuleSpec: commonAuthRuleSpec, - }, + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.AuthScheme = api.AuthSchemeSpec{ + Response: &api.ResponseSpec{ + Success: api.WrappedSuccessResponseSpec{ + DynamicMetadata: map[string]api.SuccessResponseSpec{ + "header": { + CommonAuthRuleSpec: commonAuthRuleSpec, }, }, }, }, - }, - } + } + }) err := k8sClient.Create(context.Background(), policy) Expect(err).To(Not(BeNil())) @@ -1463,33 +1381,22 @@ var _ = Describe("AuthPolicy CEL Validations", func() { }) It("invalid usage of config-level route selectors with a gateway targetRef - callbacks", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "Gateway", - Name: "my-gw", - }, - AuthScheme: api.AuthSchemeSpec{ - Callbacks: map[string]api.CallbackSpec{ - "callback": { - CallbackSpec: authorinoapi.CallbackSpec{ - CallbackMethodSpec: authorinoapi.CallbackMethodSpec{ - Http: &authorinoapi.HttpEndpointSpec{ - Url: "test.com", - }, + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.AuthScheme = api.AuthSchemeSpec{ + Callbacks: map[string]api.CallbackSpec{ + "callback": { + CallbackSpec: authorinoapi.CallbackSpec{ + CallbackMethodSpec: authorinoapi.CallbackMethodSpec{ + Http: &authorinoapi.HttpEndpointSpec{ + Url: "test.com", }, }, - CommonAuthRuleSpec: commonAuthRuleSpec, }, + CommonAuthRuleSpec: commonAuthRuleSpec, }, }, - }, - } + } + }) err := k8sClient.Create(context.Background(), policy) Expect(err).To(Not(BeNil())) diff --git a/controllers/authpolicy_status.go b/controllers/authpolicy_status.go index 20c0ca4b1..ba91cbac8 100644 --- a/controllers/authpolicy_status.go +++ b/controllers/authpolicy_status.go @@ -2,21 +2,18 @@ package controllers import ( "context" - "errors" "fmt" "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" - gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" - - authorinoapi "github.com/kuadrant/authorino/api/v1beta2" - api "github.com/kuadrant/kuadrant-operator/api/v1beta2" ) // reconcileStatus makes sure status block of AuthPolicy is up-to-date. @@ -79,41 +76,17 @@ func (r *AuthPolicyReconciler) calculateStatus(ap *api.AuthPolicy, specErr error ObservedGeneration: ap.Status.ObservedGeneration, } - targetNetworkObjectKind := string(ap.Spec.TargetRef.Kind) - availableCond := r.acceptedCondition(targetNetworkObjectKind, specErr, authConfigReady) + availableCond := r.acceptedCondition(ap, specErr, authConfigReady) meta.SetStatusCondition(&newStatus.Conditions, *availableCond) return newStatus } -func (r *AuthPolicyReconciler) acceptedCondition(targetNetworkObjectKind string, specErr error, authConfigReady bool) *metav1.Condition { - // Condition if there is no issue - cond := &metav1.Condition{ - Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), - Status: metav1.ConditionTrue, - Reason: string(gatewayapiv1alpha2.PolicyReasonAccepted), - Message: fmt.Sprintf("AuthPolicy has been accepted. %s is protected", targetNetworkObjectKind), - } +func (r *AuthPolicyReconciler) acceptedCondition(policy common.KuadrantPolicy, specErr error, authConfigReady bool) *metav1.Condition { + cond := common.AcceptedCondition(policy, specErr) - if specErr != nil { - cond.Status = metav1.ConditionFalse - cond.Message = specErr.Error() - - switch { - // TargetNotFound - case errors.As(specErr, &common.ErrTargetNotFound{}): - cond.Reason = string(gatewayapiv1alpha2.PolicyReasonTargetNotFound) - // Invalid - case errors.As(specErr, &common.ErrInvalid{}): - cond.Reason = string(gatewayapiv1alpha2.PolicyReasonInvalid) - // Conflicted - case errors.As(specErr, &common.ErrConflict{}): - cond.Reason = string(gatewayapiv1alpha2.PolicyReasonConflicted) - default: - cond.Reason = "ReconciliationError" - } - } else if !authConfigReady { + if !authConfigReady { cond.Status = metav1.ConditionFalse cond.Reason = "AuthSchemeNotReady" cond.Message = "AuthScheme is not ready yet" // TODO(rahul): need to take care if status change is delayed. diff --git a/controllers/ratelimitpolicy_controller.go b/controllers/ratelimitpolicy_controller.go index 135708173..d38ed2c64 100644 --- a/controllers/ratelimitpolicy_controller.go +++ b/controllers/ratelimitpolicy_controller.go @@ -93,7 +93,7 @@ func (r *RateLimitPolicyReconciler) Reconcile(eventCtx context.Context, req ctrl if delResErr == nil { delResErr = err } - return r.reconcileStatus(ctx, rlp, common.ErrTargetNotFound{Kind: rlp.Kind(), TargetRef: rlp.GetTargetRef(), Err: delResErr}) + return r.reconcileStatus(ctx, rlp, common.NewErrTargetNotFound(rlp.Kind(), rlp.GetTargetRef(), delResErr)) } return ctrl.Result{}, err } @@ -153,12 +153,12 @@ func (r *RateLimitPolicyReconciler) reconcileResources(ctx context.Context, rlp // validate err := rlp.Validate() if err != nil { - return common.ErrInvalid{Kind: rlp.Kind(), Err: err} + return common.NewErrInvalid(rlp.Kind(), err) } err = common.ValidateHierarchicalRules(rlp, targetNetworkObject) if err != nil { - return common.ErrInvalid{Kind: rlp.Kind(), Err: err} + return common.NewErrInvalid(rlp.Kind(), err) } // reconcile based on gateway diffs diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index c633a772e..1025c41a0 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -573,15 +573,9 @@ var _ = Describe("RateLimitPolicy controller", func() { }) Context("RLP accepted condition reasons", func() { - // Accepted reason is already tested generally by the existing tests - - It("Target not found reason", func() { - rlp := rlpFactory(nil) - err := k8sClient.Create(context.Background(), rlp) - Expect(err).ToNot(HaveOccurred()) - - rlpKey := client.ObjectKeyFromObject(rlp) - Eventually(func() bool { + assertAcceptedConditionFalse := func(rlp *kuadrantv1beta2.RateLimitPolicy, reason, message string) func() bool { + return func() bool { + rlpKey := client.ObjectKeyFromObject(rlp) existingRLP := &kuadrantv1beta2.RateLimitPolicy{} err := k8sClient.Get(context.Background(), rlpKey, existingRLP) if err != nil { @@ -593,9 +587,20 @@ var _ = Describe("RateLimitPolicy controller", func() { return false } - return cond.Status == metav1.ConditionFalse && cond.Reason == string(gatewayapiv1alpha2.PolicyReasonTargetNotFound) && - cond.Message == fmt.Sprintf("RateLimitPolicy target %s was not found", routeName) - }, time.Minute, 5*time.Second).Should(BeTrue()) + return cond.Status == metav1.ConditionFalse && cond.Reason == reason && cond.Message == message + } + } + + // Accepted reason is already tested generally by the existing tests + + It("Target not found reason", func() { + rlp := rlpFactory(nil) + err := k8sClient.Create(context.Background(), rlp) + Expect(err).ToNot(HaveOccurred()) + + Eventually(assertAcceptedConditionFalse(rlp, string(gatewayapiv1alpha2.PolicyReasonTargetNotFound), + fmt.Sprintf("RateLimitPolicy target %s was not found", routeName)), + time.Minute, 5*time.Second).Should(BeTrue()) }) It("Conflict reason", func() { @@ -614,49 +619,25 @@ var _ = Describe("RateLimitPolicy controller", func() { err = k8sClient.Create(context.Background(), rlp2) Expect(err).ToNot(HaveOccurred()) - rlpKey := client.ObjectKeyFromObject(rlp2) - Eventually(func() bool { - existingRLP := &kuadrantv1beta2.RateLimitPolicy{} - err := k8sClient.Get(context.Background(), rlpKey, existingRLP) - if err != nil { - return false - } - - cond := meta.FindStatusCondition(existingRLP.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) - if cond == nil { - return false - } - - return cond.Status == metav1.ConditionFalse && cond.Reason == string(gatewayapiv1alpha2.PolicyReasonConflicted) && - cond.Message == fmt.Sprintf("RateLimitPolicy is conflicted by %[1]v/toystore-rlp: the gateway.networking.k8s.io/v1, Kind=HTTPRoute target %[1]v/toystore-route is already referenced by policy %[1]v/toystore-rlp", testNamespace) - }, time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(assertAcceptedConditionFalse(rlp2, string(gatewayapiv1alpha2.PolicyReasonConflicted), + fmt.Sprintf("RateLimitPolicy is conflicted by %[1]v/toystore-rlp: the gateway.networking.k8s.io/v1, Kind=HTTPRoute target %[1]v/toystore-route is already referenced by policy %[1]v/toystore-rlp", testNamespace)), + time.Minute, 5*time.Second).Should(BeTrue()) }) It("Validation reason", func() { + const targetRefName, targetRefNamespace = "istio-ingressgateway", "istio-system" + rlp := rlpFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Spec.TargetRef.Kind = "Gateway" - policy.Spec.TargetRef.Namespace = ptr.To(gatewayapiv1.Namespace("istio-system")) - policy.Spec.TargetRef.Name = "istio-ingressgateway" + policy.Spec.TargetRef.Name = targetRefName + policy.Spec.TargetRef.Namespace = ptr.To(gatewayapiv1.Namespace(targetRefNamespace)) }) err := k8sClient.Create(context.Background(), rlp) Expect(err).ToNot(HaveOccurred()) - rlpKey := client.ObjectKeyFromObject(rlp) - Eventually(func() bool { - existingRLP := &kuadrantv1beta2.RateLimitPolicy{} - err := k8sClient.Get(context.Background(), rlpKey, existingRLP) - if err != nil { - return false - } - - cond := meta.FindStatusCondition(existingRLP.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) - if cond == nil { - return false - } - - return cond.Status == metav1.ConditionFalse && cond.Reason == string(gatewayapiv1alpha2.PolicyReasonInvalid) && - cond.Message == "RateLimitPolicy target is invalid: invalid targetRef.Namespace istio-system. Currently only supporting references to the same namespace" - }, time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(assertAcceptedConditionFalse(rlp, string(gatewayapiv1alpha2.PolicyReasonInvalid), + fmt.Sprintf("RateLimitPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", targetRefNamespace)), + time.Minute, 5*time.Second).Should(BeTrue()) }) }) }) @@ -671,7 +652,7 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { AfterEach(DeleteNamespaceCallback(&testNamespace)) Context("Spec TargetRef Validations", func() { - It("Valid policy targeting HTTPRoute", func() { + policyFactory := func(mutateFn func(policy *kuadrantv1beta2.RateLimitPolicy)) *kuadrantv1beta2.RateLimitPolicy { policy := &kuadrantv1beta2.RateLimitPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: "my-policy", @@ -681,65 +662,43 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ Group: "gateway.networking.k8s.io", Kind: "HTTPRoute", - Name: "my-route", + Name: "my-target", }, }, } + if mutateFn != nil { + mutateFn(policy) + } + + return policy + } + It("Valid policy targeting HTTPRoute", func() { + policy := policyFactory(nil) err := k8sClient.Create(context.Background(), policy) Expect(err).To(BeNil()) }) It("Valid policy targeting Gateway", func() { - policy := &kuadrantv1beta2.RateLimitPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: kuadrantv1beta2.RateLimitPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "Gateway", - Name: "my-gw", - }, - }, - } + policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + }) err := k8sClient.Create(context.Background(), policy) Expect(err).To(BeNil()) }) It("Invalid Target Ref Group", func() { - policy := &kuadrantv1beta2.RateLimitPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: kuadrantv1beta2.RateLimitPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "not-gateway.networking.k8s.io", - Kind: "HTTPRoute", - Name: "my-route", - }, - }, - } + policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Group = "not-gateway.networking.k8s.io" + }) err := k8sClient.Create(context.Background(), policy) Expect(err).To(Not(BeNil())) Expect(strings.Contains(err.Error(), "Invalid targetRef.group. The only supported value is 'gateway.networking.k8s.io'")).To(BeTrue()) }) It("Invalid Target Ref Kind", func() { - policy := &kuadrantv1beta2.RateLimitPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: testNamespace, - }, - Spec: kuadrantv1beta2.RateLimitPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "TCPRoute", - Name: "my-route", - }, - }, - } + policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Kind = "TCPRoute" + }) err := k8sClient.Create(context.Background(), policy) Expect(err).To(Not(BeNil())) Expect(strings.Contains(err.Error(), "Invalid targetRef.kind. The only supported values are 'HTTPRoute' and 'Gateway'")).To(BeTrue()) diff --git a/controllers/ratelimitpolicy_status.go b/controllers/ratelimitpolicy_status.go index 014e89e28..62a6149f8 100644 --- a/controllers/ratelimitpolicy_status.go +++ b/controllers/ratelimitpolicy_status.go @@ -2,20 +2,16 @@ package controllers import ( "context" - "errors" "fmt" "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" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/reconcile" - gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" - - kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" ) func (r *RateLimitPolicyReconciler) reconcileStatus(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, specErr error) (ctrl.Result, error) { @@ -61,40 +57,9 @@ func (r *RateLimitPolicyReconciler) calculateStatus(_ context.Context, rlp *kuad ObservedGeneration: rlp.Status.ObservedGeneration, } - acceptedCond := r.acceptedCondition(specErr) + acceptedCond := common.AcceptedCondition(rlp, specErr) meta.SetStatusCondition(&newStatus.Conditions, *acceptedCond) return newStatus } - -func (r *RateLimitPolicyReconciler) acceptedCondition(specErr error) *metav1.Condition { - // Accepted - cond := &metav1.Condition{ - Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), - Status: metav1.ConditionTrue, - Reason: string(gatewayapiv1alpha2.PolicyReasonAccepted), - Message: "RateLimitPolicy has been accepted", - } - - if specErr != nil { - cond.Status = metav1.ConditionFalse - cond.Message = specErr.Error() - - switch { - // TargetNotFound - case errors.As(specErr, &common.ErrTargetNotFound{}): - cond.Reason = string(gatewayapiv1alpha2.PolicyReasonTargetNotFound) - // Invalid - case errors.As(specErr, &common.ErrInvalid{}): - cond.Reason = string(gatewayapiv1alpha2.PolicyReasonInvalid) - // Conflicted - case errors.As(specErr, &common.ErrConflict{}): - cond.Reason = string(gatewayapiv1alpha2.PolicyReasonConflicted) - default: - cond.Reason = "ReconciliationError" - } - } - - return cond -} diff --git a/pkg/common/apimachinery_status_conditions.go b/pkg/common/apimachinery_status_conditions.go index a0f8640be..b9f353f71 100644 --- a/pkg/common/apimachinery_status_conditions.go +++ b/pkg/common/apimachinery_status_conditions.go @@ -2,10 +2,13 @@ package common import ( "encoding/json" + "errors" + "fmt" "slices" "sort" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" ) // ConditionMarshal marshals the set of conditions as a JSON array, sorted by condition type. @@ -16,3 +19,35 @@ func ConditionMarshal(conditions []metav1.Condition) ([]byte, error) { }) return json.Marshal(condCopy) } + +// AcceptedCondition returns an accepted conditions with common reasons for a kuadrant policy +func AcceptedCondition(policy KuadrantPolicy, err error) *metav1.Condition { + // Accepted + cond := &metav1.Condition{ + Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayapiv1alpha2.PolicyReasonAccepted), + Message: fmt.Sprintf("%s has been accepted", policy.Kind()), + } + + if err != nil { + cond.Status = metav1.ConditionFalse + cond.Message = err.Error() + + 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" + } + } + + return cond +} diff --git a/pkg/common/errors.go b/pkg/common/errors.go index 8ec5d0f3f..83c011a0d 100644 --- a/pkg/common/errors.go +++ b/pkg/common/errors.go @@ -21,6 +21,14 @@ func (e ErrTargetNotFound) Error() string { return fmt.Sprintf("%s target %s was not found: %s", e.Kind, e.TargetRef.Name, e.Err.Error()) } +func NewErrTargetNotFound(kind string, targetRef gatewayapiv1alpha2.PolicyTargetReference, err error) ErrTargetNotFound { + return ErrTargetNotFound{ + Kind: kind, + TargetRef: targetRef, + Err: err, + } +} + type ErrInvalid struct { Kind string Err error @@ -30,6 +38,13 @@ func (e ErrInvalid) Error() string { return fmt.Sprintf("%s target is invalid: %s", e.Kind, e.Err.Error()) } +func NewErrInvalid(kind string, err error) ErrInvalid { + return ErrInvalid{ + Kind: kind, + Err: err, + } +} + type ErrConflict struct { Kind string NameNamespace string @@ -39,3 +54,11 @@ type ErrConflict struct { func (e ErrConflict) Error() string { return fmt.Sprintf("%s is conflicted by %s: %s", e.Kind, e.NameNamespace, e.Err.Error()) } + +func NewErrConflict(kind string, nameNamespace string, err error) ErrConflict { + return ErrConflict{ + Kind: kind, + NameNamespace: nameNamespace, + Err: err, + } +} diff --git a/pkg/reconcilers/targetref_reconciler.go b/pkg/reconcilers/targetref_reconciler.go index 055e3b5c4..76a0dab91 100644 --- a/pkg/reconcilers/targetref_reconciler.go +++ b/pkg/reconcilers/targetref_reconciler.go @@ -166,7 +166,7 @@ func (r *TargetRefReconciler) ReconcileTargetBackReference(ctx context.Context, if val, ok := objAnnotations[annotationName]; ok { if val != policyKey.String() { - return common.ErrConflict{Kind: policy.Kind(), NameNamespace: val, Err: fmt.Errorf("the %s target %s is already referenced by policy %s", targetNetworkObjectKind, targetNetworkObjectKey, val)} + return common.NewErrConflict(policy.Kind(), val, fmt.Errorf("the %s target %s is already referenced by policy %s", targetNetworkObjectKind, targetNetworkObjectKey, val)) } } else { objAnnotations[annotationName] = policyKey.String()