From ed590ecfe7f5921351c75bb9d78bc934cdf75273 Mon Sep 17 00:00:00 2001 From: Kevin Fan Date: Tue, 9 Jan 2024 14:05:52 +0000 Subject: [PATCH] feat: accepted policy status condition (#347) * feat: rlp accepted condition * feat: rlp target not found reason * feat: rlp invalid reason * feat: rlp conflicted reason * feat: auth policy accepted condition * feat: auth policy invalid reason * feat: auth policy conflict reason * test: rlp accepted reason conditions * feat: auth policy target not found reason * test: auth policy accepted condition reasons integration tests * refactor: use common acception condition function instead * refactor: integration test policy factory mutateFn * refactor: interface for standard policy errors --- api/v1beta2/authpolicy_types.go | 6 + api/v1beta2/ratelimitpolicy_types.go | 6 + controllers/authpolicy_controller.go | 24 +- controllers/authpolicy_controller_test.go | 1040 +++++++---------- controllers/authpolicy_status.go | 22 +- ...dor_cluster_envoyfilter_controller_test.go | 2 +- controllers/ratelimitpolicy_controller.go | 26 +- .../ratelimitpolicy_controller_test.go | 350 +++--- controllers/ratelimitpolicy_status.go | 27 +- pkg/common/apimachinery_status_conditions.go | 28 + .../apimachinery_status_conditions_test.go | 110 ++ pkg/common/common.go | 1 + pkg/common/errors.go | 87 ++ pkg/common/gatewayapi_utils_test.go | 18 - pkg/common/test_utils.go | 29 + pkg/reconcilers/targetref_reconciler.go | 5 +- pkg/reconcilers/targetref_reconciler_test.go | 9 +- 17 files changed, 926 insertions(+), 864 deletions(-) create mode 100644 pkg/common/errors.go create mode 100644 pkg/common/test_utils.go diff --git a/api/v1beta2/authpolicy_types.go b/api/v1beta2/authpolicy_types.go index 0522ad7a5..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" @@ -277,6 +279,10 @@ func (ap *AuthPolicy) GetRulesHostnames() (ruleHosts []string) { return } +func (ap *AuthPolicy) Kind() string { + return ap.TypeMeta.Kind +} + //+kubebuilder:object:root=true // AuthPolicyList contains a list of AuthPolicy diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index 7fc773f90..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" @@ -240,6 +242,10 @@ func (r *RateLimitPolicy) GetRulesHostnames() (ruleHosts []string) { return } +func (r *RateLimitPolicy) Kind() string { + return r.TypeMeta.Kind +} + func init() { SchemeBuilder.Register(&RateLimitPolicy{}, &RateLimitPolicyList{}) } diff --git a/controllers/authpolicy_controller.go b/controllers/authpolicy_controller.go index efa719830..0ab6a45f2 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, delResErr) + return r.reconcileStatus(ctx, ap, common.NewErrTargetNotFound(ap.Kind(), ap.GetTargetRef(), delResErr)) } return ctrl.Result{}, err } @@ -130,13 +130,21 @@ 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 err + return common.NewErrInvalid(ap.Kind(), err) } if err := common.ValidateHierarchicalRules(ap, targetNetworkObject); err != nil { + 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 } @@ -159,7 +167,7 @@ func (r *AuthPolicyReconciler) reconcileResources(ctx context.Context, ap *api.A return err } - // set annotation of policies afftecting the gateway - should be the last step, only when all the reconciliation steps succeed + // set annotation of policies affecting the gateway - should be the last step, only when all the reconciliation steps succeed return r.ReconcileGatewayPolicyReferences(ctx, ap, gatewayDiffObj) } @@ -181,13 +189,13 @@ func (r *AuthPolicyReconciler) deleteResources(ctx context.Context, ap *api.Auth } } - // update annotation of policies afftecting the gateway + // update annotation of policies affecting the gateway return r.ReconcileGatewayPolicyReferences(ctx, ap, gatewayDiffObj) } // Ensures only one RLP targets the network resource -func (r *AuthPolicyReconciler) reconcileNetworkResourceDirectBackReference(ctx context.Context, ap *api.AuthPolicy, targetNetworkObject client.Object) error { - return r.ReconcileTargetBackReference(ctx, client.ObjectKeyFromObject(ap), targetNetworkObject, common.AuthPolicyBackRefAnnotation) +func (r *AuthPolicyReconciler) reconcileNetworkResourceDirectBackReference(ctx context.Context, ap common.KuadrantPolicy, targetNetworkObject client.Object) error { + return r.ReconcileTargetBackReference(ctx, ap, targetNetworkObject, common.AuthPolicyBackRefAnnotation) } func (r *AuthPolicyReconciler) deleteNetworkResourceDirectBackReference(ctx context.Context, targetNetworkObject client.Object) error { diff --git a/controllers/authpolicy_controller_test.go b/controllers/authpolicy_controller_test.go index dede13448..3a6bd3b0b 100644 --- a/controllers/authpolicy_controller_test.go +++ b/controllers/authpolicy_controller_test.go @@ -49,6 +49,28 @@ var _ = Describe("AuthPolicy controller", func() { AfterEach(DeleteNamespaceCallback(&testNamespace)) + policyFactory := func(mutateFns ...func(policy *api.AuthPolicy)) *api.AuthPolicy { + policy := &api.AuthPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "toystore", + Namespace: testNamespace, + }, + Spec: api.AuthPolicySpec{ + TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ + Group: "gateway.networking.k8s.io", + Kind: "HTTPRoute", + Name: testHTTPRouteName, + Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), + }, + AuthScheme: testBasicAuthScheme(), + }, + } + for _, mutateFn := range mutateFns { + mutateFn(policy) + } + return policy + } + Context("Basic HTTPRoute", func() { BeforeEach(func() { err := ApplyResources(filepath.Join("..", "examples", "toystore", "toystore.yaml"), k8sClient, testNamespace) @@ -61,29 +83,20 @@ var _ = Describe("AuthPolicy controller", func() { }) It("Attaches policy to the Gateway", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "gw-auth", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "Gateway", - Name: testGatewayName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - AuthScheme: testBasicAuthScheme(), - }, - } - policy.Spec.AuthScheme.Authentication["apiKey"].ApiKey.Selector.MatchLabels["admin"] = "yes" + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Name = "gw-auth" + policy.Spec.TargetRef.Group = "gateway.networking.k8s.io" + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = testGatewayName + policy.Spec.AuthScheme.Authentication["apiKey"].ApiKey.Selector.MatchLabels["admin"] = "yes" + }) 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(testPolicyIsReady(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(testPolicyIsAccepted(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) // check istio authorizationpolicy iapKey := types.NamespacedName{Name: istioAuthorizationPolicyName(testGatewayName, policy.Spec.TargetRef), Namespace: testNamespace} @@ -139,28 +152,19 @@ var _ = Describe("AuthPolicy controller", func() { Expect(err).ToNot(HaveOccurred()) Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(route)), time.Minute, 5*time.Second).Should(BeTrue()) - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "gw-auth", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "Gateway", - Name: gatewayapiv1.ObjectName(gatewayName), - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - AuthScheme: testBasicAuthScheme(), - }, - } + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Name = "gw-auth" + policy.Spec.TargetRef.Group = "gateway.networking.k8s.io" + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gatewayName) + }) 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(testPolicyIsReady(policy), 60*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(testPolicyIsAccepted(policy), 60*time.Second, 5*time.Second).Should(BeTrue()) // check authorino authconfig hosts authConfigKey := types.NamespacedName{Name: authConfigName(client.ObjectKeyFromObject(policy)), Namespace: testNamespace} @@ -175,28 +179,14 @@ var _ = Describe("AuthPolicy controller", func() { }) It("Attaches policy to the HTTPRoute", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "toystore", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "HTTPRoute", - Name: testHTTPRouteName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - AuthScheme: testBasicAuthScheme(), - }, - } + policy := policyFactory() 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(testPolicyIsReady(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(testPolicyIsAccepted(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) // check istio authorizationpolicy iapKey := types.NamespacedName{Name: istioAuthorizationPolicyName(testGatewayName, policy.Spec.TargetRef), Namespace: testNamespace} @@ -235,28 +225,14 @@ var _ = Describe("AuthPolicy controller", func() { }) It("Attaches policy to the Gateway while having other policies attached to some HTTPRoutes", func() { - routePolicy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "toystore", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "HTTPRoute", - Name: testHTTPRouteName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - AuthScheme: testBasicAuthScheme(), - }, - } + routePolicy := policyFactory() err := k8sClient.Create(context.Background(), routePolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(routePolicy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(testPolicyIsReady(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(testPolicyIsAccepted(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) // create second (policyless) httproute otherRoute := testBuildBasicHttpRoute("policyless-route", testGatewayName, testNamespace, []string{"*.other"}) @@ -274,28 +250,19 @@ var _ = Describe("AuthPolicy controller", func() { Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(otherRoute)), time.Minute, 5*time.Second).Should(BeTrue()) // attach policy to the gatewaay - gwPolicy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "gw-auth", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "Gateway", - Name: testGatewayName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - AuthScheme: testBasicAuthScheme(), - }, - } + gwPolicy := policyFactory(func(policy *api.AuthPolicy) { + policy.Name = "gw-auth" + policy.Spec.TargetRef.Group = "gateway.networking.k8s.io" + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = testGatewayName + }) err = k8sClient.Create(context.Background(), gwPolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(gwPolicy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(testPolicyIsReady(gwPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(testPolicyIsAccepted(gwPolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) // check istio authorizationpolicy iapKey := types.NamespacedName{Name: istioAuthorizationPolicyName(testGatewayName, gwPolicy.Spec.TargetRef), Namespace: testNamespace} @@ -335,45 +302,22 @@ var _ = Describe("AuthPolicy controller", func() { }) It("Attaches policy to the Gateway while having other policies attached to all HTTPRoutes", func() { - routePolicy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "toystore", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "HTTPRoute", - Name: testHTTPRouteName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - AuthScheme: testBasicAuthScheme(), - }, - } + routePolicy := policyFactory() err := k8sClient.Create(context.Background(), routePolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(routePolicy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(testPolicyIsReady(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(testPolicyIsAccepted(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) // attach policy to the gatewaay - gwPolicy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "gw-auth", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "Gateway", - Name: testGatewayName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - AuthScheme: testBasicAuthScheme(), - }, - } + gwPolicy := policyFactory(func(policy *api.AuthPolicy) { + policy.Name = "gw-auth" + policy.Spec.TargetRef.Group = "gateway.networking.k8s.io" + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = testGatewayName + }) err = k8sClient.Create(context.Background(), gwPolicy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(gwPolicy).String(), "error", err) @@ -386,7 +330,7 @@ var _ = Describe("AuthPolicy controller", func() { if err != nil { return false } - condition := meta.FindStatusCondition(existingPolicy.Status.Conditions, APAvailableConditionType) + condition := meta.FindStatusCondition(existingPolicy.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) return condition != nil && condition.Reason == "AuthSchemeNotReady" }, 30*time.Second, 5*time.Second).Should(BeTrue()) @@ -407,30 +351,17 @@ var _ = Describe("AuthPolicy controller", func() { }) It("Rejects policy with only unmatching top-level route selectors while trying to configure the gateway", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "toystore", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "HTTPRoute", - Name: testHTTPRouteName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - RouteSelectors: []api.RouteSelector{ - { // does not select any HTTPRouteRule - Matches: []gatewayapiv1alpha2.HTTPRouteMatch{ - { - Method: ptr.To(gatewayapiv1alpha2.HTTPMethod("DELETE")), - }, + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.RouteSelectors = []api.RouteSelector{ + { // does not select any HTTPRouteRule + Matches: []gatewayapiv1alpha2.HTTPRouteMatch{ + { + Method: ptr.To(gatewayapiv1alpha2.HTTPMethod("DELETE")), }, }, }, - AuthScheme: testBasicAuthScheme(), - }, - } + } + }) err := k8sClient.Create(context.Background(), policy) logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(policy).String(), "error", err) @@ -443,7 +374,7 @@ var _ = Describe("AuthPolicy controller", func() { if err != nil { return false } - condition := meta.FindStatusCondition(existingPolicy.Status.Conditions, APAvailableConditionType) + condition := meta.FindStatusCondition(existingPolicy.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) return condition != nil && condition.Reason == "ReconciliationError" && strings.Contains(condition.Message, "cannot match any route rules, check for invalid route selectors in the policy") }, 30*time.Second, 5*time.Second).Should(BeTrue()) @@ -464,21 +395,7 @@ var _ = Describe("AuthPolicy controller", func() { }) It("Rejects policy with only unmatching config-level route selectors post-configuring the gateway", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "toystore", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "HTTPRoute", - Name: testHTTPRouteName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - AuthScheme: testBasicAuthScheme(), - }, - } + policy := policyFactory() config := policy.Spec.AuthScheme.Authentication["apiKey"] config.RouteSelectors = []api.RouteSelector{ { // does not select any HTTPRouteRule @@ -502,7 +419,7 @@ var _ = Describe("AuthPolicy controller", func() { if err != nil { return false } - condition := meta.FindStatusCondition(existingPolicy.Status.Conditions, APAvailableConditionType) + condition := meta.FindStatusCondition(existingPolicy.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) return condition != nil && condition.Reason == "ReconciliationError" && strings.Contains(condition.Message, "cannot match any route rules, check for invalid route selectors in the policy") }, 30*time.Second, 5*time.Second).Should(BeTrue()) @@ -529,27 +446,13 @@ var _ = Describe("AuthPolicy controller", func() { }) It("Deletes resources when the policy is deleted", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "toystore", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "HTTPRoute", - Name: testHTTPRouteName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - AuthScheme: testBasicAuthScheme(), - }, - } + policy := policyFactory() err := k8sClient.Create(context.Background(), policy) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(testPolicyIsReady(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(testPolicyIsAccepted(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) // delete policy err = k8sClient.Delete(context.Background(), policy) @@ -573,119 +476,107 @@ var _ = Describe("AuthPolicy controller", func() { }) It("Maps to all fields of the AuthConfig", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "toystore", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "HTTPRoute", - Name: testHTTPRouteName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - NamedPatterns: map[string]authorinoapi.PatternExpressions{ - "internal-source": []authorinoapi.PatternExpression{ - { - Selector: "source.ip", - Operator: authorinoapi.PatternExpressionOperator("matches"), - Value: `192\.168\..*`, - }, - }, - "authz-and-rl-required": []authorinoapi.PatternExpression{ - { - Selector: "source.ip", - Operator: authorinoapi.PatternExpressionOperator("neq"), - Value: "192.168.0.10", - }, + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.NamedPatterns = map[string]authorinoapi.PatternExpressions{ + "internal-source": []authorinoapi.PatternExpression{ + { + Selector: "source.ip", + Operator: authorinoapi.PatternExpressionOperator("matches"), + Value: `192\.168\..*`, }, }, - Conditions: []authorinoapi.PatternExpressionOrRef{ + "authz-and-rl-required": []authorinoapi.PatternExpression{ { - PatternRef: authorinoapi.PatternRef{ - Name: "internal-source", - }, + Selector: "source.ip", + Operator: authorinoapi.PatternExpressionOperator("neq"), + Value: "192.168.0.10", }, }, - AuthScheme: api.AuthSchemeSpec{ - Authentication: map[string]api.AuthenticationSpec{ - "jwt": { - AuthenticationSpec: authorinoapi.AuthenticationSpec{ - CommonEvaluatorSpec: authorinoapi.CommonEvaluatorSpec{ - Conditions: []authorinoapi.PatternExpressionOrRef{ - { - PatternExpression: authorinoapi.PatternExpression{ - Selector: `filter_metadata.envoy\.filters\.http\.jwt_authn|verified_jwt`, - Operator: "neq", - Value: "", - }, + } + policy.Spec.Conditions = []authorinoapi.PatternExpressionOrRef{ + { + PatternRef: authorinoapi.PatternRef{ + Name: "internal-source", + }, + }, + } + policy.Spec.AuthScheme = api.AuthSchemeSpec{ + Authentication: map[string]api.AuthenticationSpec{ + "jwt": { + AuthenticationSpec: authorinoapi.AuthenticationSpec{ + CommonEvaluatorSpec: authorinoapi.CommonEvaluatorSpec{ + Conditions: []authorinoapi.PatternExpressionOrRef{ + { + PatternExpression: authorinoapi.PatternExpression{ + Selector: `filter_metadata.envoy\.filters\.http\.jwt_authn|verified_jwt`, + Operator: "neq", + Value: "", }, }, }, - AuthenticationMethodSpec: authorinoapi.AuthenticationMethodSpec{ - Plain: &authorinoapi.PlainIdentitySpec{ - Selector: `filter_metadata.envoy\.filters\.http\.jwt_authn|verified_jwt`, - }, + }, + AuthenticationMethodSpec: authorinoapi.AuthenticationMethodSpec{ + Plain: &authorinoapi.PlainIdentitySpec{ + Selector: `filter_metadata.envoy\.filters\.http\.jwt_authn|verified_jwt`, }, }, }, }, - Metadata: map[string]api.MetadataSpec{ - "user-groups": { - MetadataSpec: authorinoapi.MetadataSpec{ - CommonEvaluatorSpec: authorinoapi.CommonEvaluatorSpec{ - Conditions: []authorinoapi.PatternExpressionOrRef{ - { - PatternExpression: authorinoapi.PatternExpression{ - Selector: "auth.identity.admin", - Operator: authorinoapi.PatternExpressionOperator("neq"), - Value: "true", - }, + }, + Metadata: map[string]api.MetadataSpec{ + "user-groups": { + MetadataSpec: authorinoapi.MetadataSpec{ + CommonEvaluatorSpec: authorinoapi.CommonEvaluatorSpec{ + Conditions: []authorinoapi.PatternExpressionOrRef{ + { + PatternExpression: authorinoapi.PatternExpression{ + Selector: "auth.identity.admin", + Operator: authorinoapi.PatternExpressionOperator("neq"), + Value: "true", }, }, }, - MetadataMethodSpec: authorinoapi.MetadataMethodSpec{ - Http: &authorinoapi.HttpEndpointSpec{ - Url: "http://user-groups/username={auth.identity.username}", - }, + }, + MetadataMethodSpec: authorinoapi.MetadataMethodSpec{ + Http: &authorinoapi.HttpEndpointSpec{ + Url: "http://user-groups/username={auth.identity.username}", }, }, }, }, - Authorization: map[string]api.AuthorizationSpec{ - "admin-or-privileged": { - AuthorizationSpec: authorinoapi.AuthorizationSpec{ - CommonEvaluatorSpec: authorinoapi.CommonEvaluatorSpec{ - Conditions: []authorinoapi.PatternExpressionOrRef{ - { - PatternRef: authorinoapi.PatternRef{ - Name: "authz-and-rl-required", - }, + }, + Authorization: map[string]api.AuthorizationSpec{ + "admin-or-privileged": { + AuthorizationSpec: authorinoapi.AuthorizationSpec{ + CommonEvaluatorSpec: authorinoapi.CommonEvaluatorSpec{ + Conditions: []authorinoapi.PatternExpressionOrRef{ + { + PatternRef: authorinoapi.PatternRef{ + Name: "authz-and-rl-required", }, }, }, - AuthorizationMethodSpec: authorinoapi.AuthorizationMethodSpec{ - PatternMatching: &authorinoapi.PatternMatchingAuthorizationSpec{ - Patterns: []authorinoapi.PatternExpressionOrRef{ - { - Any: []authorinoapi.UnstructuredPatternExpressionOrRef{ - { - PatternExpressionOrRef: authorinoapi.PatternExpressionOrRef{ - PatternExpression: authorinoapi.PatternExpression{ - Selector: "auth.identity.admin", - Operator: authorinoapi.PatternExpressionOperator("eq"), - Value: "true", - }, + }, + AuthorizationMethodSpec: authorinoapi.AuthorizationMethodSpec{ + PatternMatching: &authorinoapi.PatternMatchingAuthorizationSpec{ + Patterns: []authorinoapi.PatternExpressionOrRef{ + { + Any: []authorinoapi.UnstructuredPatternExpressionOrRef{ + { + PatternExpressionOrRef: authorinoapi.PatternExpressionOrRef{ + PatternExpression: authorinoapi.PatternExpression{ + Selector: "auth.identity.admin", + Operator: authorinoapi.PatternExpressionOperator("eq"), + Value: "true", }, }, - { - PatternExpressionOrRef: authorinoapi.PatternExpressionOrRef{ - PatternExpression: authorinoapi.PatternExpression{ - Selector: "auth.metadata.user-groups", - Operator: authorinoapi.PatternExpressionOperator("incl"), - Value: "privileged", - }, + }, + { + PatternExpressionOrRef: authorinoapi.PatternExpressionOrRef{ + PatternExpression: authorinoapi.PatternExpression{ + Selector: "auth.metadata.user-groups", + Operator: authorinoapi.PatternExpressionOperator("incl"), + Value: "privileged", }, }, }, @@ -696,63 +587,63 @@ var _ = Describe("AuthPolicy controller", func() { }, }, }, - Response: &api.ResponseSpec{ - Unauthenticated: &authorinoapi.DenyWithSpec{ - Message: &authorinoapi.ValueOrSelector{ - Value: k8sruntime.RawExtension{Raw: []byte(`"Missing verified JWT injected by the gateway"`)}, - }, + }, + Response: &api.ResponseSpec{ + Unauthenticated: &authorinoapi.DenyWithSpec{ + Message: &authorinoapi.ValueOrSelector{ + Value: k8sruntime.RawExtension{Raw: []byte(`"Missing verified JWT injected by the gateway"`)}, }, - Unauthorized: &authorinoapi.DenyWithSpec{ - Message: &authorinoapi.ValueOrSelector{ - Value: k8sruntime.RawExtension{Raw: []byte(`"User must be admin or member of privileged group"`)}, - }, + }, + Unauthorized: &authorinoapi.DenyWithSpec{ + Message: &authorinoapi.ValueOrSelector{ + Value: k8sruntime.RawExtension{Raw: []byte(`"User must be admin or member of privileged group"`)}, }, - Success: api.WrappedSuccessResponseSpec{ - Headers: map[string]api.HeaderSuccessResponseSpec{ - "x-username": { - SuccessResponseSpec: api.SuccessResponseSpec{ - SuccessResponseSpec: authorinoapi.SuccessResponseSpec{ - CommonEvaluatorSpec: authorinoapi.CommonEvaluatorSpec{ - Conditions: []authorinoapi.PatternExpressionOrRef{ - { - PatternExpression: authorinoapi.PatternExpression{ - Selector: "request.headers.x-propagate-username.@case:lower", - Operator: authorinoapi.PatternExpressionOperator("matches"), - Value: "1|yes|true", - }, + }, + Success: api.WrappedSuccessResponseSpec{ + Headers: map[string]api.HeaderSuccessResponseSpec{ + "x-username": { + SuccessResponseSpec: api.SuccessResponseSpec{ + SuccessResponseSpec: authorinoapi.SuccessResponseSpec{ + CommonEvaluatorSpec: authorinoapi.CommonEvaluatorSpec{ + Conditions: []authorinoapi.PatternExpressionOrRef{ + { + PatternExpression: authorinoapi.PatternExpression{ + Selector: "request.headers.x-propagate-username.@case:lower", + Operator: authorinoapi.PatternExpressionOperator("matches"), + Value: "1|yes|true", }, }, }, - AuthResponseMethodSpec: authorinoapi.AuthResponseMethodSpec{ - Plain: &authorinoapi.PlainAuthResponseSpec{ - Selector: "auth.identity.username", - }, + }, + AuthResponseMethodSpec: authorinoapi.AuthResponseMethodSpec{ + Plain: &authorinoapi.PlainAuthResponseSpec{ + Selector: "auth.identity.username", }, }, }, }, }, - DynamicMetadata: map[string]api.SuccessResponseSpec{ - "x-auth-data": { - SuccessResponseSpec: authorinoapi.SuccessResponseSpec{ - CommonEvaluatorSpec: authorinoapi.CommonEvaluatorSpec{ - Conditions: []authorinoapi.PatternExpressionOrRef{ - { - PatternRef: authorinoapi.PatternRef{ - Name: "authz-and-rl-required", - }, + }, + DynamicMetadata: map[string]api.SuccessResponseSpec{ + "x-auth-data": { + SuccessResponseSpec: authorinoapi.SuccessResponseSpec{ + CommonEvaluatorSpec: authorinoapi.CommonEvaluatorSpec{ + Conditions: []authorinoapi.PatternExpressionOrRef{ + { + PatternRef: authorinoapi.PatternRef{ + Name: "authz-and-rl-required", }, }, }, - AuthResponseMethodSpec: authorinoapi.AuthResponseMethodSpec{ - Json: &authorinoapi.JsonAuthResponseSpec{ - Properties: authorinoapi.NamedValuesOrSelectors{ - "username": { - Selector: "auth.identity.username", - }, - "groups": { - Selector: "auth.metadata.user-groups", - }, + }, + AuthResponseMethodSpec: authorinoapi.AuthResponseMethodSpec{ + Json: &authorinoapi.JsonAuthResponseSpec{ + Properties: authorinoapi.NamedValuesOrSelectors{ + "username": { + Selector: "auth.identity.username", + }, + "groups": { + Selector: "auth.metadata.user-groups", }, }, }, @@ -761,48 +652,48 @@ var _ = Describe("AuthPolicy controller", func() { }, }, }, - Callbacks: map[string]api.CallbackSpec{ - "unauthorized-attempt": { - CallbackSpec: authorinoapi.CallbackSpec{ - CommonEvaluatorSpec: authorinoapi.CommonEvaluatorSpec{ - Conditions: []authorinoapi.PatternExpressionOrRef{ - { - PatternRef: authorinoapi.PatternRef{ - Name: "authz-and-rl-required", - }, + }, + Callbacks: map[string]api.CallbackSpec{ + "unauthorized-attempt": { + CallbackSpec: authorinoapi.CallbackSpec{ + CommonEvaluatorSpec: authorinoapi.CommonEvaluatorSpec{ + Conditions: []authorinoapi.PatternExpressionOrRef{ + { + PatternRef: authorinoapi.PatternRef{ + Name: "authz-and-rl-required", }, - { - PatternExpression: authorinoapi.PatternExpression{ - Selector: "auth.authorization.admin-or-privileged", - Operator: authorinoapi.PatternExpressionOperator("neq"), - Value: "true", - }, + }, + { + PatternExpression: authorinoapi.PatternExpression{ + Selector: "auth.authorization.admin-or-privileged", + Operator: authorinoapi.PatternExpressionOperator("neq"), + Value: "true", }, }, }, - CallbackMethodSpec: authorinoapi.CallbackMethodSpec{ - Http: &authorinoapi.HttpEndpointSpec{ - Url: "http://events/unauthorized", - Method: ptr.To(authorinoapi.HttpMethod("POST")), - ContentType: authorinoapi.HttpContentType("application/json"), - Body: &authorinoapi.ValueOrSelector{ - Selector: `\{"identity":{auth.identity},"request-id":{request.id}\}`, - }, + }, + CallbackMethodSpec: authorinoapi.CallbackMethodSpec{ + Http: &authorinoapi.HttpEndpointSpec{ + Url: "http://events/unauthorized", + Method: ptr.To(authorinoapi.HttpMethod("POST")), + ContentType: authorinoapi.HttpContentType("application/json"), + Body: &authorinoapi.ValueOrSelector{ + Selector: `\{"identity":{auth.identity},"request-id":{request.id}\}`, }, }, }, }, }, }, - }, - } + } + }) 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(testPolicyIsReady(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(testPolicyIsAccepted(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) // check authorino authconfig authConfigKey := types.NamespacedName{Name: authConfigName(client.ObjectKeyFromObject(policy)), Namespace: testNamespace} @@ -829,27 +720,13 @@ var _ = Describe("AuthPolicy controller", func() { }) It("Attaches simple policy to the HTTPRoute", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "toystore", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "HTTPRoute", - Name: testHTTPRouteName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - AuthScheme: testBasicAuthScheme(), - }, - } + policy := policyFactory() err := k8sClient.Create(context.Background(), policy) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(testPolicyIsReady(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(testPolicyIsAccepted(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) // check istio authorizationpolicy iapKey := types.NamespacedName{Name: istioAuthorizationPolicyName(testGatewayName, policy.Spec.TargetRef), Namespace: testNamespace} @@ -914,50 +791,37 @@ var _ = Describe("AuthPolicy controller", func() { }) It("Attaches policy with top-level route selectors to the HTTPRoute", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "toystore", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "HTTPRoute", - Name: testHTTPRouteName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - RouteSelectors: []api.RouteSelector{ - { // Selects: POST|DELETE *.admin.toystore.com/admin* - Matches: []gatewayapiv1alpha2.HTTPRouteMatch{ - { - Path: &gatewayapiv1alpha2.HTTPPathMatch{ - Type: ptr.To(gatewayapiv1alpha2.PathMatchType("PathPrefix")), - Value: ptr.To("/admin"), - }, + policy := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.RouteSelectors = []api.RouteSelector{ + { // Selects: POST|DELETE *.admin.toystore.com/admin* + Matches: []gatewayapiv1alpha2.HTTPRouteMatch{ + { + Path: &gatewayapiv1alpha2.HTTPPathMatch{ + Type: ptr.To(gatewayapiv1alpha2.PathMatchType("PathPrefix")), + Value: ptr.To("/admin"), }, }, - Hostnames: []gatewayapiv1.Hostname{"*.admin.toystore.com"}, }, - { // Selects: GET /private* - Matches: []gatewayapiv1alpha2.HTTPRouteMatch{ - { - Path: &gatewayapiv1alpha2.HTTPPathMatch{ - Type: ptr.To(gatewayapiv1alpha2.PathMatchType("PathPrefix")), - Value: ptr.To("/private"), - }, + Hostnames: []gatewayapiv1.Hostname{"*.admin.toystore.com"}, + }, + { // Selects: GET /private* + Matches: []gatewayapiv1alpha2.HTTPRouteMatch{ + { + Path: &gatewayapiv1alpha2.HTTPPathMatch{ + Type: ptr.To(gatewayapiv1alpha2.PathMatchType("PathPrefix")), + Value: ptr.To("/private"), }, }, }, }, - AuthScheme: testBasicAuthScheme(), - }, - } + } + }) err := k8sClient.Create(context.Background(), policy) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(testPolicyIsReady(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(testPolicyIsAccepted(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) // check istio authorizationpolicy iapKey := types.NamespacedName{Name: istioAuthorizationPolicyName(testGatewayName, policy.Spec.TargetRef), Namespace: testNamespace} @@ -1031,42 +895,29 @@ var _ = Describe("AuthPolicy controller", func() { }) It("Attaches policy with config-level route selectors to the HTTPRoute", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "toystore", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "HTTPRoute", - Name: testHTTPRouteName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - AuthScheme: testBasicAuthScheme(), - }, - } - config := policy.Spec.AuthScheme.Authentication["apiKey"] - config.RouteSelectors = []api.RouteSelector{ - { // Selects: POST|DELETE *.admin.toystore.com/admin* - Matches: []gatewayapiv1alpha2.HTTPRouteMatch{ - { - Path: &gatewayapiv1alpha2.HTTPPathMatch{ - Type: ptr.To(gatewayapiv1alpha2.PathMatchType("PathPrefix")), - Value: ptr.To("/admin"), + policy := policyFactory(func(policy *api.AuthPolicy) { + config := policy.Spec.AuthScheme.Authentication["apiKey"] + config.RouteSelectors = []api.RouteSelector{ + { // Selects: POST|DELETE *.admin.toystore.com/admin* + Matches: []gatewayapiv1alpha2.HTTPRouteMatch{ + { + Path: &gatewayapiv1alpha2.HTTPPathMatch{ + Type: ptr.To(gatewayapiv1alpha2.PathMatchType("PathPrefix")), + Value: ptr.To("/admin"), + }, }, }, + Hostnames: []gatewayapiv1.Hostname{"*.admin.toystore.com"}, }, - Hostnames: []gatewayapiv1.Hostname{"*.admin.toystore.com"}, - }, - } - policy.Spec.AuthScheme.Authentication["apiKey"] = config + } + policy.Spec.AuthScheme.Authentication["apiKey"] = config + }) err := k8sClient.Create(context.Background(), policy) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(testPolicyIsReady(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(testPolicyIsAccepted(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) // check istio authorizationpolicy iapKey := types.NamespacedName{Name: istioAuthorizationPolicyName(testGatewayName, policy.Spec.TargetRef), Namespace: testNamespace} @@ -1158,51 +1009,38 @@ var _ = Describe("AuthPolicy controller", func() { }) It("Mixes route selectors into other conditions", func() { - policy := &api.AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "toystore", - Namespace: testNamespace, - }, - Spec: api.AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: "gateway.networking.k8s.io", - Kind: "HTTPRoute", - Name: testHTTPRouteName, - Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), - }, - AuthScheme: testBasicAuthScheme(), - }, - } - config := policy.Spec.AuthScheme.Authentication["apiKey"] - config.RouteSelectors = []api.RouteSelector{ - { // Selects: GET /private* - Matches: []gatewayapiv1.HTTPRouteMatch{ - { - Path: &gatewayapiv1.HTTPPathMatch{ - Type: ptr.To(gatewayapiv1.PathMatchType("PathPrefix")), - Value: ptr.To("/private"), + policy := policyFactory(func(policy *api.AuthPolicy) { + config := policy.Spec.AuthScheme.Authentication["apiKey"] + config.RouteSelectors = []api.RouteSelector{ + { // Selects: GET /private* + Matches: []gatewayapiv1.HTTPRouteMatch{ + { + Path: &gatewayapiv1.HTTPPathMatch{ + Type: ptr.To(gatewayapiv1.PathMatchType("PathPrefix")), + Value: ptr.To("/private"), + }, + Method: ptr.To(gatewayapiv1.HTTPMethod("GET")), }, - Method: ptr.To(gatewayapiv1.HTTPMethod("GET")), }, }, - }, - } - config.Conditions = []authorinoapi.PatternExpressionOrRef{ - { - PatternExpression: authorinoapi.PatternExpression{ - Selector: "context.source.address.Address.SocketAddress.address", - Operator: authorinoapi.PatternExpressionOperator("matches"), - Value: `192\.168\.0\..*`, + } + config.Conditions = []authorinoapi.PatternExpressionOrRef{ + { + PatternExpression: authorinoapi.PatternExpression{ + Selector: "context.source.address.Address.SocketAddress.address", + Operator: authorinoapi.PatternExpressionOperator("matches"), + Value: `192\.168\.0\..*`, + }, }, - }, - } - policy.Spec.AuthScheme.Authentication["apiKey"] = config + } + policy.Spec.AuthScheme.Authentication["apiKey"] = config + }) err := k8sClient.Create(context.Background(), policy) Expect(err).ToNot(HaveOccurred()) // check policy status - Eventually(testPolicyIsReady(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(testPolicyIsAccepted(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) // check authorino authconfig authConfigKey := types.NamespacedName{Name: authConfigName(client.ObjectKeyFromObject(policy)), Namespace: testNamespace} @@ -1256,7 +1094,77 @@ var _ = Describe("AuthPolicy controller", func() { }) }) - Context("TODO: Targeted resource does not exist", func() {}) + Context("AuthPolicy accepted condition reasons", func() { + 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 { + return false + } + cond := meta.FindStatusCondition(existingPolicy.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) + if cond == nil { + return false + } + + 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() { + policy := policyFactory() + + 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"}) + err := k8sClient.Create(context.Background(), route) + Expect(err).ToNot(HaveOccurred()) + Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(route)), time.Minute, 5*time.Second).Should(BeTrue()) + + policy := policyFactory() + err = k8sClient.Create(context.Background(), policy) + logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(policy).String(), "error", err) + Expect(err).ToNot(HaveOccurred()) + + policy2 := policyFactory(func(policy *api.AuthPolicy) { + policy.Name = "conflicting-ap" + }) + err = k8sClient.Create(context.Background(), policy2) + logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(policy2).String(), "error", err) + Expect(err).ToNot(HaveOccurred()) + + // check policy status + 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 := policyFactory(func(policy *api.AuthPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + 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(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()) + }) + }) }) var _ = Describe("AuthPolicy CEL Validations", func() { @@ -1269,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(mutateFns ...func(policy *api.AuthPolicy)) *api.AuthPolicy { policy := &api.AuthPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: "my-policy", @@ -1279,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", }, }, } + + for _, mutateFn := range mutateFns { + mutateFn(policy) + } + + return policy + } + + It("Valid policy targeting HTTPRoute", func() { + policy := policyFactory() 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()) @@ -1366,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", @@ -1378,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())) @@ -1420,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())) @@ -1447,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())) @@ -1474,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())) @@ -1507,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())) @@ -1538,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())) @@ -1598,10 +1430,10 @@ func testBasicAuthScheme() api.AuthSchemeSpec { } } -func testPolicyIsReady(policy *api.AuthPolicy) func() bool { +func testPolicyIsAccepted(policy *api.AuthPolicy) func() bool { return func() bool { existingPolicy := &api.AuthPolicy{} err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(policy), existingPolicy) - return err == nil && meta.IsStatusConditionTrue(existingPolicy.Status.Conditions, "Available") + return err == nil && meta.IsStatusConditionTrue(existingPolicy.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) } } diff --git a/controllers/authpolicy_status.go b/controllers/authpolicy_status.go index 23e4a573b..3c36e5d9c 100644 --- a/controllers/authpolicy_status.go +++ b/controllers/authpolicy_status.go @@ -14,10 +14,9 @@ import ( authorinoapi "github.com/kuadrant/authorino/api/v1beta2" api "github.com/kuadrant/kuadrant-operator/api/v1beta2" + "github.com/kuadrant/kuadrant-operator/pkg/common" ) -const APAvailableConditionType string = "Available" - // reconcileStatus makes sure status block of AuthPolicy is up-to-date. func (r *AuthPolicyReconciler) reconcileStatus(ctx context.Context, ap *api.AuthPolicy, specErr error) (ctrl.Result, error) { logger, _ := logr.FromContext(ctx) @@ -78,28 +77,17 @@ func (r *AuthPolicyReconciler) calculateStatus(ap *api.AuthPolicy, specErr error ObservedGeneration: ap.Status.ObservedGeneration, } - targetNetworkObjectKind := string(ap.Spec.TargetRef.Kind) - availableCond := r.availableCondition(targetNetworkObjectKind, specErr, authConfigReady) + availableCond := r.acceptedCondition(ap, specErr, authConfigReady) meta.SetStatusCondition(&newStatus.Conditions, *availableCond) return newStatus } -func (r *AuthPolicyReconciler) availableCondition(targetNetworkObjectectKind string, specErr error, authConfigReady bool) *metav1.Condition { - // Condition if there is no issue - cond := &metav1.Condition{ - Type: APAvailableConditionType, - Status: metav1.ConditionTrue, - Reason: fmt.Sprintf("%sProtected", targetNetworkObjectectKind), - Message: fmt.Sprintf("%s is protected", targetNetworkObjectectKind), - } +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.Reason = "ReconciliationError" - cond.Message = specErr.Error() - } 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/limitador_cluster_envoyfilter_controller_test.go b/controllers/limitador_cluster_envoyfilter_controller_test.go index be2cec803..98fb86c57 100644 --- a/controllers/limitador_cluster_envoyfilter_controller_test.go +++ b/controllers/limitador_cluster_envoyfilter_controller_test.go @@ -105,7 +105,7 @@ var _ = Describe("Limitador Cluster EnvoyFilter controller", func() { Expect(err).ToNot(HaveOccurred()) // Check RLP status is available rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} - Eventually(testRLPIsAvailable(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check envoy filter Eventually(func() bool { diff --git a/controllers/ratelimitpolicy_controller.go b/controllers/ratelimitpolicy_controller.go index 83d22affe..88ac46e30 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, delResErr) + return r.reconcileStatus(ctx, rlp, common.NewErrTargetNotFound(rlp.Kind(), rlp.GetTargetRef(), delResErr)) } return ctrl.Result{}, err } @@ -149,15 +149,21 @@ 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 { - return err +// 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 } @@ -180,7 +186,7 @@ func (r *RateLimitPolicyReconciler) reconcileResources(ctx context.Context, rlp return err } - // set annotation of policies afftecting the gateway - should be the last step, only when all the reconciliation steps succeed + // set annotation of policies affecting the gateway - should be the last step, only when all the reconciliation steps succeed return r.ReconcileGatewayPolicyReferences(ctx, rlp, gatewayDiffObj) } @@ -212,7 +218,7 @@ func (r *RateLimitPolicyReconciler) deleteResources(ctx context.Context, rlp *ku // Ensures only one RLP targets the network resource func (r *RateLimitPolicyReconciler) reconcileNetworkResourceDirectBackReference(ctx context.Context, policy common.KuadrantPolicy, targetNetworkObject client.Object) error { - return r.ReconcileTargetBackReference(ctx, client.ObjectKeyFromObject(policy), targetNetworkObject, common.RateLimitPolicyBackRefAnnotation) + return r.ReconcileTargetBackReference(ctx, policy, targetNetworkObject, common.RateLimitPolicyBackRefAnnotation) } func (r *RateLimitPolicyReconciler) deleteNetworkResourceDirectBackReference(ctx context.Context, targetNetworkObject client.Object) error { diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 1b055f6c6..b6ba8793a 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -5,6 +5,7 @@ package controllers import ( "context" "encoding/json" + "fmt" "strings" "time" @@ -35,6 +36,40 @@ var _ = Describe("RateLimitPolicy controller", func() { gateway *gatewayapiv1.Gateway ) + policyFactory := func(mutateFns ...func(policy *kuadrantv1beta2.RateLimitPolicy)) *kuadrantv1beta2.RateLimitPolicy { + policy := &kuadrantv1beta2.RateLimitPolicy{ + TypeMeta: metav1.TypeMeta{ + Kind: "RateLimitPolicy", + APIVersion: kuadrantv1beta2.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: rlpName, + Namespace: testNamespace, + }, + Spec: kuadrantv1beta2.RateLimitPolicySpec{ + TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1.Group("gateway.networking.k8s.io"), + Kind: "HTTPRoute", + Name: gatewayapiv1.ObjectName(routeName), + }, + Limits: map[string]kuadrantv1beta2.Limit{ + "l1": { + Rates: []kuadrantv1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), + }, + }, + }, + }, + }, + } + for _, mutateFn := range mutateFns { + mutateFn(policy) + } + + return policy + } + beforeEachCallback := func() { CreateNamespace(&testNamespace) gateway = testBuildBasicGateway(gwName, testNamespace) @@ -85,38 +120,13 @@ var _ = Describe("RateLimitPolicy controller", func() { Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute)), time.Minute, 5*time.Second).Should(BeTrue()) // create ratelimitpolicy - rlp := &kuadrantv1beta2.RateLimitPolicy{ - TypeMeta: metav1.TypeMeta{ - Kind: "RateLimitPolicy", - APIVersion: kuadrantv1beta2.GroupVersion.String(), - }, - ObjectMeta: metav1.ObjectMeta{ - Name: rlpName, - Namespace: testNamespace, - }, - Spec: kuadrantv1beta2.RateLimitPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: gatewayapiv1.Group("gateway.networking.k8s.io"), - Kind: "HTTPRoute", - Name: gatewayapiv1.ObjectName(routeName), - }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), - }, - }, - }, - }, - }, - } + rlp := policyFactory() err = k8sClient.Create(context.Background(), rlp) Expect(err).ToNot(HaveOccurred()) // Check RLP status is available rlpKey := client.ObjectKeyFromObject(rlp) - Eventually(testRLPIsAvailable(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check HTTPRoute direct back reference routeKey := client.ObjectKey{Name: routeName, Namespace: testNamespace} @@ -242,75 +252,60 @@ var _ = Describe("RateLimitPolicy controller", func() { Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute)), time.Minute, 5*time.Second).Should(BeTrue()) // create ratelimitpolicy - rlp := &kuadrantv1beta2.RateLimitPolicy{ - TypeMeta: metav1.TypeMeta{ - Kind: "RateLimitPolicy", - APIVersion: kuadrantv1beta2.GroupVersion.String(), - }, - ObjectMeta: metav1.ObjectMeta{ - Name: rlpName, - Namespace: testNamespace, - }, - Spec: kuadrantv1beta2.RateLimitPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: gatewayapiv1.Group("gateway.networking.k8s.io"), - Kind: "HTTPRoute", - Name: gatewayapiv1.ObjectName(routeName), - }, - Limits: map[string]kuadrantv1beta2.Limit{ - "toys": { - Rates: []kuadrantv1beta2.Rate{ - {Limit: 50, Duration: 1, Unit: kuadrantv1beta2.TimeUnit("minute")}, - }, - Counters: []kuadrantv1beta2.ContextSelector{"auth.identity.username"}, - RouteSelectors: []kuadrantv1beta2.RouteSelector{ - { // selects the 1st HTTPRouteRule (i.e. get|post /toys*) for one of the hostnames - Matches: []gatewayapiv1.HTTPRouteMatch{ - { - Path: &gatewayapiv1.HTTPPathMatch{ - Type: ptr.To(gatewayapiv1.PathMatchPathPrefix), - Value: ptr.To("/toys"), - }, + rlp := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.Limits = map[string]kuadrantv1beta2.Limit{ + "toys": { + Rates: []kuadrantv1beta2.Rate{ + {Limit: 50, Duration: 1, Unit: kuadrantv1beta2.TimeUnit("minute")}, + }, + Counters: []kuadrantv1beta2.ContextSelector{"auth.identity.username"}, + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { // selects the 1st HTTPRouteRule (i.e. get|post /toys*) for one of the hostnames + Matches: []gatewayapiv1.HTTPRouteMatch{ + { + Path: &gatewayapiv1.HTTPPathMatch{ + Type: ptr.To(gatewayapiv1.PathMatchPathPrefix), + Value: ptr.To("/toys"), }, }, - Hostnames: []gatewayapiv1.Hostname{"*.toystore.acme.com"}, - }, - }, - When: []kuadrantv1beta2.WhenCondition{ - { - Selector: "auth.identity.group", - Operator: kuadrantv1beta2.WhenConditionOperator("neq"), - Value: "admin", }, + Hostnames: []gatewayapiv1.Hostname{"*.toystore.acme.com"}, }, }, - "assets": { - Rates: []kuadrantv1beta2.Rate{ - {Limit: 5, Duration: 1, Unit: kuadrantv1beta2.TimeUnit("minute")}, - {Limit: 100, Duration: 12, Unit: kuadrantv1beta2.TimeUnit("hour")}, + When: []kuadrantv1beta2.WhenCondition{ + { + Selector: "auth.identity.group", + Operator: kuadrantv1beta2.WhenConditionOperator("neq"), + Value: "admin", }, - RouteSelectors: []kuadrantv1beta2.RouteSelector{ - { // selects the 2nd HTTPRouteRule (i.e. /assets*) for all hostnames - Matches: []gatewayapiv1.HTTPRouteMatch{ - { - Path: &gatewayapiv1.HTTPPathMatch{ - Type: ptr.To(gatewayapiv1.PathMatchPathPrefix), - Value: ptr.To("/assets"), - }, + }, + }, + "assets": { + Rates: []kuadrantv1beta2.Rate{ + {Limit: 5, Duration: 1, Unit: kuadrantv1beta2.TimeUnit("minute")}, + {Limit: 100, Duration: 12, Unit: kuadrantv1beta2.TimeUnit("hour")}, + }, + RouteSelectors: []kuadrantv1beta2.RouteSelector{ + { // selects the 2nd HTTPRouteRule (i.e. /assets*) for all hostnames + Matches: []gatewayapiv1.HTTPRouteMatch{ + { + Path: &gatewayapiv1.HTTPPathMatch{ + Type: ptr.To(gatewayapiv1.PathMatchPathPrefix), + Value: ptr.To("/assets"), }, }, }, }, }, }, - }, - } + } + }) err = k8sClient.Create(context.Background(), rlp) Expect(err).ToNot(HaveOccurred()) // Check RLP status is available rlpKey := client.ObjectKeyFromObject(rlp) - Eventually(testRLPIsAvailable(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check wasm plugin wasmPluginKey := client.ObjectKey{Name: rlptools.WASMPluginName(gateway), Namespace: testNamespace} @@ -426,38 +421,16 @@ var _ = Describe("RateLimitPolicy controller", func() { Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute)), time.Minute, 5*time.Second).Should(BeTrue()) // create ratelimitpolicy - rlp := &kuadrantv1beta2.RateLimitPolicy{ - TypeMeta: metav1.TypeMeta{ - Kind: "RateLimitPolicy", - APIVersion: kuadrantv1beta2.GroupVersion.String(), - }, - ObjectMeta: metav1.ObjectMeta{ - Name: rlpName, - Namespace: testNamespace, - }, - Spec: kuadrantv1beta2.RateLimitPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: gatewayapiv1.Group("gateway.networking.k8s.io"), - Kind: "Gateway", - Name: gatewayapiv1.ObjectName(gwName), - }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), - }, - }, - }, - }, - }, - } + rlp := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gwName) + }) err = k8sClient.Create(context.Background(), rlp) Expect(err).ToNot(HaveOccurred()) // Check RLP status is available rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} - Eventually(testRLPIsAvailable(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check Gateway direct back reference gwKey := client.ObjectKeyFromObject(gateway) @@ -544,38 +517,16 @@ var _ = Describe("RateLimitPolicy controller", func() { It("Creates all the resources for a basic Gateway and RateLimitPolicy when missing a HTTPRoute attached to the Gateway", func() { // create ratelimitpolicy - rlp := &kuadrantv1beta2.RateLimitPolicy{ - TypeMeta: metav1.TypeMeta{ - Kind: "RateLimitPolicy", - APIVersion: kuadrantv1beta2.GroupVersion.String(), - }, - ObjectMeta: metav1.ObjectMeta{ - Name: rlpName, - Namespace: testNamespace, - }, - Spec: kuadrantv1beta2.RateLimitPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: gatewayapiv1.Group("gateway.networking.k8s.io"), - Kind: "Gateway", - Name: gatewayapiv1.ObjectName(gwName), - }, - Limits: map[string]kuadrantv1beta2.Limit{ - "l1": { - Rates: []kuadrantv1beta2.Rate{ - { - Limit: 1, Duration: 3, Unit: kuadrantv1beta2.TimeUnit("minute"), - }, - }, - }, - }, - }, - } + rlp := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gwName) + }) err := k8sClient.Create(context.Background(), rlp) Expect(err).ToNot(HaveOccurred()) // Check RLP status is available rlpKey := client.ObjectKey{Name: rlpName, Namespace: testNamespace} - Eventually(testRLPIsAvailable(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) + Eventually(testRLPIsAccepted(rlpKey), time.Minute, 5*time.Second).Should(BeTrue()) // Check Gateway direct back reference gwKey := client.ObjectKeyFromObject(gateway) @@ -620,6 +571,75 @@ var _ = Describe("RateLimitPolicy controller", func() { Expect(existingGateway.GetAnnotations()).To(HaveKeyWithValue(common.RateLimitPoliciesBackRefAnnotation, string(serialized))) }) }) + + Context("RLP accepted condition reasons", func() { + 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 { + return false + } + + cond := meta.FindStatusCondition(existingRLP.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) + if cond == nil { + return false + } + + 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 := policyFactory() + 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() { + httpRoute := testBuildBasicHttpRoute(routeName, gwName, testNamespace, []string{"*.example.com"}) + err := k8sClient.Create(context.Background(), httpRoute) + Expect(err).ToNot(HaveOccurred()) + Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(httpRoute)), time.Minute, 5*time.Second).Should(BeTrue()) + + rlp := policyFactory() + err = k8sClient.Create(context.Background(), rlp) + Expect(err).ToNot(HaveOccurred()) + + rlp2 := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Name = "conflicting-rlp" + }) + err = k8sClient.Create(context.Background(), rlp2) + Expect(err).ToNot(HaveOccurred()) + + 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 := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { + policy.Spec.TargetRef.Kind = "Gateway" + policy.Spec.TargetRef.Name = targetRefName + policy.Spec.TargetRef.Namespace = ptr.To(gatewayapiv1.Namespace(targetRefNamespace)) + }) + err := k8sClient.Create(context.Background(), rlp) + Expect(err).ToNot(HaveOccurred()) + + 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()) + }) + }) }) var _ = Describe("RateLimitPolicy CEL Validations", func() { @@ -632,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(mutateFns ...func(policy *kuadrantv1beta2.RateLimitPolicy)) *kuadrantv1beta2.RateLimitPolicy { policy := &kuadrantv1beta2.RateLimitPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: "my-policy", @@ -642,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", }, }, } + for _, mutateFn := range mutateFns { + mutateFn(policy) + } + + return policy + } + It("Valid policy targeting HTTPRoute", func() { + policy := policyFactory() 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()) @@ -755,14 +753,14 @@ var _ = Describe("RateLimitPolicy CEL Validations", func() { }) }) -func testRLPIsAvailable(rlpKey client.ObjectKey) func() bool { +func testRLPIsAccepted(rlpKey client.ObjectKey) func() bool { return func() bool { existingRLP := &kuadrantv1beta2.RateLimitPolicy{} err := k8sClient.Get(context.Background(), rlpKey, existingRLP) if err != nil { return false } - if !meta.IsStatusConditionTrue(existingRLP.Status.Conditions, "Available") { + if !meta.IsStatusConditionTrue(existingRLP.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) { return false } diff --git a/controllers/ratelimitpolicy_status.go b/controllers/ratelimitpolicy_status.go index 5fda71235..f6e2bd461 100644 --- a/controllers/ratelimitpolicy_status.go +++ b/controllers/ratelimitpolicy_status.go @@ -8,15 +8,11 @@ import ( "github.com/go-logr/logr" 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" kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" -) - -const ( - RLPAvailableConditionType string = "Available" + "github.com/kuadrant/kuadrant-operator/pkg/common" ) func (r *RateLimitPolicyReconciler) reconcileStatus(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, specErr error) (ctrl.Result, error) { @@ -62,26 +58,9 @@ func (r *RateLimitPolicyReconciler) calculateStatus(_ context.Context, rlp *kuad ObservedGeneration: rlp.Status.ObservedGeneration, } - availableCond := r.availableCondition(specErr) + acceptedCond := common.AcceptedCondition(rlp, specErr) - meta.SetStatusCondition(&newStatus.Conditions, *availableCond) + meta.SetStatusCondition(&newStatus.Conditions, *acceptedCond) return newStatus } - -func (r *RateLimitPolicyReconciler) availableCondition(specErr error) *metav1.Condition { - cond := &metav1.Condition{ - Type: RLPAvailableConditionType, - Status: metav1.ConditionTrue, - Reason: "HTTPRouteProtected", - Message: "HTTPRoute is ratelimited", - } - - if specErr != nil { - cond.Status = metav1.ConditionFalse - cond.Reason = "ReconcilliationError" - cond.Message = specErr.Error() - } - - return cond -} diff --git a/pkg/common/apimachinery_status_conditions.go b/pkg/common/apimachinery_status_conditions.go index a0f8640be..eeef51c0e 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,28 @@ 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 { + return cond + } + + cond.Status = metav1.ConditionFalse + cond.Message = err.Error() + 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/common.go b/pkg/common/common.go index 20a8b8496..534a3ebb6 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -43,6 +43,7 @@ type KuadrantPolicy interface { GetTargetRef() gatewayapiv1alpha2.PolicyTargetReference GetWrappedNamespace() gatewayapiv1.Namespace GetRulesHostnames() []string + Kind() string } type KuadrantPolicyList interface { diff --git a/pkg/common/errors.go b/pkg/common/errors.go new file mode 100644 index 000000000..2d0e7df78 --- /dev/null +++ b/pkg/common/errors.go @@ -0,0 +1,87 @@ +package common + +import ( + "fmt" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + 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 + Err error +} + +func (e ErrTargetNotFound) Error() string { + if apierrors.IsNotFound(e.Err) { + return fmt.Sprintf("%s target %s was not found", e.Kind, e.TargetRef.Name) + } + + 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, + TargetRef: targetRef, + Err: err, + } +} + +var _ PolicyError = ErrInvalid{} + +type ErrInvalid struct { + Kind string + Err error +} + +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, + Err: err, + } +} + +var _ PolicyError = ErrConflict{} + +type ErrConflict struct { + Kind string + NameNamespace string + Err error +} + +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, + NameNamespace: nameNamespace, + Err: err, + } +} diff --git a/pkg/common/gatewayapi_utils_test.go b/pkg/common/gatewayapi_utils_test.go index 464ad0e94..c15f1a1d9 100644 --- a/pkg/common/gatewayapi_utils_test.go +++ b/pkg/common/gatewayapi_utils_test.go @@ -1302,24 +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 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" +} diff --git a/pkg/reconcilers/targetref_reconciler.go b/pkg/reconcilers/targetref_reconciler.go index 01040cb57..76a0dab91 100644 --- a/pkg/reconcilers/targetref_reconciler.go +++ b/pkg/reconcilers/targetref_reconciler.go @@ -154,9 +154,10 @@ func (r *TargetRefReconciler) TargetedGatewayKeys(_ context.Context, targetNetwo } // ReconcileTargetBackReference adds policy key in annotations of the target object -func (r *TargetRefReconciler) ReconcileTargetBackReference(ctx context.Context, policyKey client.ObjectKey, targetNetworkObject client.Object, annotationName string) error { +func (r *TargetRefReconciler) ReconcileTargetBackReference(ctx context.Context, policy common.KuadrantPolicy, targetNetworkObject client.Object, annotationName string) error { logger, _ := logr.FromContext(ctx) + policyKey := client.ObjectKeyFromObject(policy) targetNetworkObjectKey := client.ObjectKeyFromObject(targetNetworkObject) targetNetworkObjectKind := targetNetworkObject.GetObjectKind().GroupVersionKind() @@ -165,7 +166,7 @@ func (r *TargetRefReconciler) ReconcileTargetBackReference(ctx context.Context, if val, ok := objAnnotations[annotationName]; ok { if val != policyKey.String() { - return fmt.Errorf("the %s target %s is already referenced by policy %s", targetNetworkObjectKind, targetNetworkObjectKey, policyKey.String()) + 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() diff --git a/pkg/reconcilers/targetref_reconciler_test.go b/pkg/reconcilers/targetref_reconciler_test.go index c39fce7d1..532395afb 100644 --- a/pkg/reconcilers/targetref_reconciler_test.go +++ b/pkg/reconcilers/targetref_reconciler_test.go @@ -24,6 +24,7 @@ import ( "testing" "github.com/go-logr/logr" + "github.com/kuadrant/kuadrant-operator/api/v1beta2" appsv1 "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -302,7 +303,7 @@ func TestReconcileTargetBackReference(t *testing.T) { t.Fatal(err) } - policyKey := client.ObjectKey{Name: "someName", Namespace: "someNamespace"} + policy := &v1beta2.RateLimitPolicy{ObjectMeta: metav1.ObjectMeta{Name: "someName", Namespace: "someNamespace"}} existingRoute := &gatewayapiv1.HTTPRoute{ TypeMeta: metav1.TypeMeta{ @@ -354,7 +355,7 @@ func TestReconcileTargetBackReference(t *testing.T) { BaseReconciler: baseReconciler, } - err = targetRefReconciler.ReconcileTargetBackReference(ctx, policyKey, existingRoute, annotationName) + err = targetRefReconciler.ReconcileTargetBackReference(ctx, policy, existingRoute, annotationName) if err != nil { t.Fatal(err) } @@ -378,8 +379,8 @@ func TestReconcileTargetBackReference(t *testing.T) { t.Fatal("expected annotation not found") } - if val != policyKey.String() { - t.Fatalf("annotation value (%s) does not match expected (%s)", val, policyKey.String()) + if val != client.ObjectKeyFromObject(policy).String() { + t.Fatalf("annotation value (%s) does not match expected (%s)", val, client.ObjectKeyFromObject(policy).String()) } }