From db7a73bc0210147b37b89c1a64366019951d0cb0 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Fri, 3 May 2024 13:08:03 +0200 Subject: [PATCH] fix: do not reconcile status of target objects multiple times (#607) --- controllers/target_status_controller.go | 47 ++++++++--- controllers/target_status_controller_test.go | 52 ++++++++++++ pkg/library/gatewayapi/types_test.go | 9 +- pkg/library/gatewayapi/utils.go | 11 +++ pkg/library/gatewayapi/utils_test.go | 86 ++++++++++++++++++++ 5 files changed, 189 insertions(+), 16 deletions(-) diff --git a/controllers/target_status_controller.go b/controllers/target_status_controller.go index d7cafd6b0..efea4b5e2 100644 --- a/controllers/target_status_controller.go +++ b/controllers/target_status_controller.go @@ -110,11 +110,11 @@ func (r *TargetStatusReconciler) reconcileResourcesForPolicyKind(parentCtx conte return err } policies := topology.PoliciesFromGateway(gw) - gatewayPolicyExists := len(policies) > 0 && utils.Index(policies, func(p kuadrantgatewayapi.Policy) bool { return kuadrantgatewayapi.IsTargetRefGateway(p.GetTargetRef()) }) >= 0 var errs error // if no policies of a kind affecting the gateway → remove condition from the gateway and routes + gatewayPolicyExists := len(policies) > 0 && utils.Index(policies, func(p kuadrantgatewayapi.Policy) bool { return kuadrantgatewayapi.IsTargetRefGateway(p.GetTargetRef()) }) >= 0 if !gatewayPolicyExists { // remove the condition from the gateway conditionType := policyAffectedConditionType(policyKind) @@ -136,26 +136,28 @@ func (r *TargetStatusReconciler) reconcileResourcesForPolicyKind(parentCtx conte } } - var gatewayStatusUpdated bool + reconciledTargets := make(map[string]struct{}) - // update the status of the gateway and its routes - for i := range policies { - policy := policies[i] - condition := buildPolicyAffectedCondition(policy) + reconcileFunc := func(policy kuadrantgatewayapi.Policy) error { + targetRefKey := targetRefKey(policy) // update status of targeted route if route := topology.GetPolicyHTTPRoute(policy); route != nil { - if err := r.addRouteCondition(ctx, route, condition, gatewayKey, kuadrant.ControllerName); err != nil { + if _, updated := reconciledTargets[targetRefKey]; updated { // do not update the same route twice + return nil + } + if err := r.addRouteCondition(ctx, route, buildPolicyAffectedCondition(policy), gatewayKey, kuadrant.ControllerName); err != nil { errs = errors.Join(errs, err) } - continue + reconciledTargets[targetRefKey] = struct{}{} + return nil } - // update status of the gateway if not already updated before after observing another policy of same kind - // this assumes that the gateway is targeted by at most one policy of each kind - if gatewayStatusUpdated { - continue + // update status of targeted gateway and routes not targeted by any policy + if _, updated := reconciledTargets[targetRefKey]; updated { // do not update the same gateway twice + return nil } + condition := buildPolicyAffectedCondition(policy) if c := meta.FindStatusCondition(gw.Status.Conditions, condition.Type); c != nil && c.Status == condition.Status && c.Reason == condition.Reason && c.Message == condition.Message && c.ObservedGeneration == gw.GetGeneration() { logger.V(1).Info("condition already up-to-date, skipping", "condition", condition.Type, "status", condition.Status) } else { @@ -173,7 +175,21 @@ func (r *TargetStatusReconciler) reconcileResourcesForPolicyKind(parentCtx conte return err } } - gatewayStatusUpdated = true + reconciledTargets[targetRefKey] = struct{}{} + + return nil + } + + // update for policies with status condition Accepted: true + for i := range utils.Filter(policies, kuadrantgatewayapi.IsPolicyAccepted) { + policy := policies[i] + errs = errors.Join(errs, reconcileFunc(policy)) + } + + // update for policies with status condition Accepted: false + for i := range utils.Filter(policies, kuadrantgatewayapi.IsNotPolicyAccepted) { + policy := policies[i] + errs = errors.Join(errs, reconcileFunc(policy)) } return errs @@ -393,3 +409,8 @@ func buildPolicyAffectedCondition(policy kuadrantgatewayapi.Policy) metav1.Condi func policyAffectedConditionType(policyKind string) string { return fmt.Sprintf(PolicyAffectedConditionPattern, policyKind) } + +func targetRefKey(policy kuadrantgatewayapi.Policy) string { + targetRef := policy.GetTargetRef() + return fmt.Sprintf("%s.%s/%s/%s", targetRef.Group, targetRef.Kind, ptr.Deref(targetRef.Namespace, gatewayapiv1.Namespace(policy.GetNamespace())), targetRef.Name) +} diff --git a/controllers/target_status_controller_test.go b/controllers/target_status_controller_test.go index b7bbd000b..e2db1ce93 100644 --- a/controllers/target_status_controller_test.go +++ b/controllers/target_status_controller_test.go @@ -99,6 +99,20 @@ var _ = Describe("Target status reconciler", Ordered, func() { return condition.Status == metav1.ConditionTrue && strings.Contains(condition.Message, policyKey.String()) } + routeNotAffected := func(ctx context.Context, routeName, conditionType string, policyKey client.ObjectKey) bool { + route := &gatewayapiv1.HTTPRoute{} + err := k8sClient.Get(ctx, client.ObjectKey{Name: routeName, Namespace: testNamespace}, route) + if err != nil { + return false + } + routeParentStatus, found := utils.Find(route.Status.RouteStatus.Parents, findRouteParentStatusFunc(route, client.ObjectKey{Name: testGatewayName, Namespace: testNamespace}, kuadrant.ControllerName)) + if !found { + return false + } + condition := meta.FindStatusCondition(routeParentStatus.Conditions, conditionType) + return condition.Status == metav1.ConditionFalse && strings.Contains(condition.Message, policyKey.String()) + } + targetsAffected := func(ctx context.Context, policyKey client.ObjectKey, conditionType string, targetRef gatewayapiv1alpha2.PolicyTargetReference, routeNames ...string) bool { switch string(targetRef.Kind) { case "Gateway": @@ -177,6 +191,44 @@ var _ = Describe("Target status reconciler", Ordered, func() { Eventually(policyAcceptedAndTargetsAffected(ctx, policy)).WithContext(ctx).Should(BeTrue()) }, testTimeOut) + It("Adds truthy PolicyAffected status condition if there is at least one policy accepted", func(ctx SpecContext) { + routePolicy1 := policyFactory(func(p *v1beta2.AuthPolicy) { + p.Name = "route-auth-1" + }) + Expect(k8sClient.Create(ctx, routePolicy1)).To(Succeed()) + + Eventually(policyAcceptedAndTargetsAffected(ctx, routePolicy1)).WithContext(ctx).Should(BeTrue()) + + routePolicy2 := policyFactory(func(p *v1beta2.AuthPolicy) { // another policy that targets the same route. this policy will not be accepted + p.Name = "route-auth-2" + }) + Expect(k8sClient.Create(ctx, routePolicy2)).To(Succeed()) + + Eventually(func() bool { + return policyAcceptedAndTargetsAffected(ctx, routePolicy1)() && + !isAuthPolicyAccepted(ctx, routePolicy2)() && !routeAffected(ctx, testHTTPRouteName, policyAffectedCondition, client.ObjectKeyFromObject(routePolicy2)) + }).WithContext(ctx).Should(BeTrue()) + }, testTimeOut) + + It("Adds falsey PolicyAffected status condition if no policy is accepted", func(ctx SpecContext) { + routePolicy1 := policyFactory(func(p *v1beta2.AuthPolicy) { // create a policy with an invalid route selector so the policy is not accepted + p.Name = "route-auth-1" + p.Spec.Defaults.RouteSelectors = []v1beta2.RouteSelector{{Hostnames: []gatewayapiv1.Hostname{"invalid.example.com"}}} + }) + Expect(k8sClient.Create(ctx, routePolicy1)).To(Succeed()) + + routePolicy2 := policyFactory(func(p *v1beta2.AuthPolicy) { // create another policy with an invalid route selector so the policy is not accepted + p.Name = "route-auth-2" + p.Spec.Defaults.RouteSelectors = []v1beta2.RouteSelector{{Hostnames: []gatewayapiv1.Hostname{"invalid.example.com"}}} + }) + Expect(k8sClient.Create(ctx, routePolicy2)).To(Succeed()) + + Eventually(func() bool { + return !isAuthPolicyAccepted(ctx, routePolicy1)() && routeNotAffected(ctx, testHTTPRouteName, policyAffectedCondition, client.ObjectKeyFromObject(routePolicy1)) && + !isAuthPolicyAccepted(ctx, routePolicy2)() && !routeAffected(ctx, testHTTPRouteName, policyAffectedCondition, client.ObjectKeyFromObject(routePolicy2)) + }).WithContext(ctx).Should(BeTrue()) + }, testTimeOut) + It("removes PolicyAffected status condition from the targeted route when the policy is deleted", func(ctx SpecContext) { policy := policyFactory() Expect(k8sClient.Create(ctx, policy)).To(Succeed()) diff --git a/pkg/library/gatewayapi/types_test.go b/pkg/library/gatewayapi/types_test.go index 35addb4b3..6d6fe6bd8 100644 --- a/pkg/library/gatewayapi/types_test.go +++ b/pkg/library/gatewayapi/types_test.go @@ -24,6 +24,7 @@ type TestPolicy struct { metav1.ObjectMeta `json:"metadata,omitempty"` TargetRef gatewayapiv1alpha2.PolicyTargetReference `json:"targetRef"` + Status FakePolicyStatus `json:"status"` } func (p *TestPolicy) PolicyClass() PolicyClass { @@ -35,7 +36,7 @@ func (p *TestPolicy) GetTargetRef() gatewayapiv1alpha2.PolicyTargetReference { } func (p *TestPolicy) GetStatus() PolicyStatus { - return &FakePolicyStatus{} + return &p.Status } func (p *TestPolicy) DeepCopyObject() runtime.Object { @@ -61,10 +62,12 @@ func (p *TestPolicy) DeepCopyInto(out *TestPolicy) { p.TargetRef.DeepCopyInto(&out.TargetRef) } -type FakePolicyStatus struct{} +type FakePolicyStatus struct { + Conditions []metav1.Condition +} func (s *FakePolicyStatus) GetConditions() []metav1.Condition { - return nil + return s.Conditions } func TestPolicyByCreationTimestamp(t *testing.T) { diff --git a/pkg/library/gatewayapi/utils.go b/pkg/library/gatewayapi/utils.go index 21f7ec620..f9a1b6487 100644 --- a/pkg/library/gatewayapi/utils.go +++ b/pkg/library/gatewayapi/utils.go @@ -7,6 +7,7 @@ import ( "strings" "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" @@ -91,6 +92,16 @@ func IsHTTPRouteAccepted(httpRoute *gatewayapiv1.HTTPRoute) bool { return len(acceptedParentRefs) == len(httpRoute.Spec.ParentRefs) } +func IsPolicyAccepted(policy Policy) bool { + condition := meta.FindStatusCondition(policy.GetStatus().GetConditions(), string(gatewayapiv1alpha2.PolicyConditionAccepted)) + return condition != nil && condition.Status == metav1.ConditionTrue +} + +func IsNotPolicyAccepted(policy Policy) bool { + condition := meta.FindStatusCondition(policy.GetStatus().GetConditions(), string(gatewayapiv1alpha2.PolicyConditionAccepted)) + return condition == nil || condition.Status != metav1.ConditionTrue +} + // GetRouteAcceptedGatewayParentKeys returns the object keys of all gateways that have accepted a given route func GetRouteAcceptedGatewayParentKeys(route *gatewayapiv1.HTTPRoute) []client.ObjectKey { acceptedParentRefs := GetRouteAcceptedParentRefs(route) diff --git a/pkg/library/gatewayapi/utils_test.go b/pkg/library/gatewayapi/utils_test.go index 5633ef29b..a8af7d98d 100644 --- a/pkg/library/gatewayapi/utils_test.go +++ b/pkg/library/gatewayapi/utils_test.go @@ -15,6 +15,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) func TestGetGatewayWorkloadSelector(t *testing.T) { @@ -238,6 +241,89 @@ func TestIsHTTPRouteAccepted(t *testing.T) { } } +type isPolicyAcceptedTestCase struct { + name string + policy Policy + expected bool +} + +var isPolicyAcceptedTestCases = []isPolicyAcceptedTestCase{ + { + name: "no status", + policy: &TestPolicy{}, + expected: false, + }, + { + name: "no policy accepted condition", + policy: &TestPolicy{ + Status: FakePolicyStatus{ + Conditions: []metav1.Condition{ + { + Type: "Other", + Status: metav1.ConditionTrue, + }, + }, + }, + }, + expected: false, + }, + { + name: "truthy accepted condition", + policy: &TestPolicy{ + Status: FakePolicyStatus{ + Conditions: []metav1.Condition{ + { + Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + }, + }, + }, + }, + expected: true, + }, + { + name: "falsey accepted condition", + policy: &TestPolicy{ + Status: FakePolicyStatus{ + Conditions: []metav1.Condition{ + { + Type: string(gatewayapiv1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionFalse, + }, + }, + }, + }, + expected: false, + }, +} + +func TestIsPolicyAccepted(t *testing.T) { + testCases := isPolicyAcceptedTestCases + for _, tc := range testCases { + t.Run(tc.name, func(subT *testing.T) { + res := IsPolicyAccepted(tc.policy) + if res != tc.expected { + subT.Errorf("result (%t) does not match expected (%t)", res, tc.expected) + } + }) + } +} + +func TestIsNotPolicyAccepted(t *testing.T) { + testCases := utils.Map(isPolicyAcceptedTestCases, func(tc isPolicyAcceptedTestCase) isPolicyAcceptedTestCase { + tc.expected = !tc.expected + return tc + }) + for _, tc := range testCases { + t.Run(tc.name, func(subT *testing.T) { + res := IsNotPolicyAccepted(tc.policy) + if res != tc.expected { + subT.Errorf("result (%t) does not match expected (%t)", res, tc.expected) + } + }) + } +} + func TestGetRouteAcceptedParentRefs(t *testing.T) { testCases := []struct { name string