fix: Add policy conflict to dnspolicy validation

Updates the dnspolicies validator task to check for polices with conflicting target refs, only one policy can currently target a specific gateway or listener. Uses the same logic as TLS, first policy created is given preference over any created later. Some other small changes to align DNSPolicy and TLSPolicy. Signed-off-by: Michael Nairn <[email protected]>
Kuadrant · Nov 8, 2024 · e40b44b · e40b44b
1 parent c3a16d8
commit e40b44b
Show file tree

Hide file tree

Showing 5 changed files with 138 additions and 7 deletions.
diff --git a/api/v1/dnspolicy_types.go b/api/v1/dnspolicy_types.go
@@ -208,6 +208,7 @@ func (p *DNSPolicy) GetRulesHostnames() []string {
 	return make([]string, 0)
 }
 
+// Deprecated: kuadrant.Policy.
 func (p *DNSPolicy) GetTargetRef() gatewayapiv1alpha2.LocalPolicyTargetReference {
 	return p.Spec.TargetRef.LocalPolicyTargetReference
 }

diff --git a/api/v1/tlspolicy_types.go b/api/v1/tlspolicy_types.go
@@ -191,7 +191,7 @@ func (p *TLSPolicy) GetRulesHostnames() []string {
 	return make([]string, 0)
 }
 
-// DEPRECATED: Use GetTargetRefs instead
+// Deprecated: kuadrant.Policy.
 func (p *TLSPolicy) GetTargetRef() gatewayapiv1alpha2.LocalPolicyTargetReference {
 	return p.Spec.TargetRef.LocalPolicyTargetReference
 }

diff --git a/controllers/dnspolicies_validator.go b/controllers/dnspolicies_validator.go
@@ -2,11 +2,14 @@ package controllers
 
 import (
 	"context"
+	"errors"
+	"reflect"
 	"sync"
 
 	"github.com/samber/lo"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/kuadrant/policy-machinery/controller"
 	"github.com/kuadrant/policy-machinery/machinery"
@@ -34,19 +37,54 @@ func (r *DNSPoliciesValidator) Subscription() controller.Subscription {
 func (r *DNSPoliciesValidator) validate(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, state *sync.Map) error {
 	logger := controller.LoggerFromContext(ctx).WithName("DNSPoliciesValidator")
 
-	policies := lo.FilterMap(topology.Policies().Items(), dnsPolicyTypeFilterFunc())
+	policies := lo.Filter(topology.Policies().Items(), func(p machinery.Policy, _ int) bool {
+		_, ok := p.(*kuadrantv1.DNSPolicy)
+		return ok
+	})
 
 	logger.V(1).Info("validating dns policies", "policies", len(policies))
 
-	state.Store(StateDNSPolicyAcceptedKey, lo.SliceToMap(policies, func(policy *kuadrantv1.DNSPolicy) (string, error) {
-		if len(policy.GetTargetRefs()) == 0 || len(topology.Targetables().Children(policy)) == 0 {
-			return policy.GetLocator(), kuadrant.NewErrTargetNotFound(kuadrantv1.DNSPolicyGroupKind.Kind, policy.GetTargetRef(),
-				apierrors.NewNotFound(controller.GatewaysResource.GroupResource(), policy.GetName()))
+	state.Store(StateDNSPolicyAcceptedKey, lo.SliceToMap(policies, func(p machinery.Policy) (string, error) {
+		policy := p.(*kuadrantv1.DNSPolicy)
+
+		if err := isTargetRefsFound(topology, policy); err != nil {
+			return policy.GetLocator(), err
+		}
+
+		if err := isConflict(policies, policy); err != nil {
+			return policy.GetLocator(), err
 		}
+
 		return policy.GetLocator(), policy.Validate()
 	}))
 
 	logger.V(1).Info("finished validating dns policies")
 
 	return nil
 }
+
+// isTargetRefsFound Policies are already linked to their targets.
+// If the target ref length and length of targetables by this policy is not the same, then the policy could not find the target.
+func isTargetRefsFound(topology *machinery.Topology, p *kuadrantv1.DNSPolicy) error {
+	if len(p.GetTargetRefs()) != len(topology.Targetables().Children(p)) {
+		return kuadrant.NewErrTargetNotFound(kuadrantv1.DNSPolicyGroupKind.Kind, p.Spec.TargetRef.LocalPolicyTargetReference, apierrors.NewNotFound(controller.GatewaysResource.GroupResource(), p.GetName()))
+	}
+
+	return nil
+}
+
+// isConflict Validates if there's already an older policy with the same target ref
+func isConflict(policies []machinery.Policy, p *kuadrantv1.DNSPolicy) error {
+	conflictingP, ok := lo.Find(policies, func(item machinery.Policy) bool {
+		policy := item.(*kuadrantv1.DNSPolicy)
+		return p != policy && policy.DeletionTimestamp == nil &&
+			policy.CreationTimestamp.Before(&p.CreationTimestamp) &&
+			reflect.DeepEqual(policy.GetTargetRefs(), p.GetTargetRefs())
+	})
+
+	if ok {
+		return kuadrant.NewErrConflict(kuadrantv1.DNSPolicyGroupKind.Kind, client.ObjectKeyFromObject(conflictingP.(*kuadrantv1.DNSPolicy)).String(), errors.New("conflicting policy"))
+	}
+
+	return nil
+}
diff --git a/controllers/tlspolicies_validator.go b/controllers/tlspolicies_validator.go
@@ -98,7 +98,7 @@ func (t *TLSPoliciesValidator) Validate(ctx context.Context, _ []controller.Reso
 // TODO: What should happen if multiple target refs is supported in the future in terms of reporting in log and policy status?
 func (t *TLSPoliciesValidator) isTargetRefsFound(topology *machinery.Topology, p *kuadrantv1.TLSPolicy) error {
 	if len(p.GetTargetRefs()) != len(topology.Targetables().Children(p)) {
-		return kuadrant.NewErrTargetNotFound(kuadrantv1.TLSPolicyGroupKind.Kind, p.GetTargetRef(), apierrors.NewNotFound(controller.GatewaysResource.GroupResource(), p.GetName()))
+		return kuadrant.NewErrTargetNotFound(kuadrantv1.TLSPolicyGroupKind.Kind, p.Spec.TargetRef.LocalPolicyTargetReference, apierrors.NewNotFound(controller.GatewaysResource.GroupResource(), p.GetName()))
 	}
 
 	return nil

diff --git a/tests/common/dnspolicy/dnspolicy_controller_test.go b/tests/common/dnspolicy/dnspolicy_controller_test.go
@@ -1661,4 +1661,96 @@ var _ = Describe("DNSPolicy controller", func() {
 			}, tests.TimeoutMedium, tests.RetryIntervalMedium).Should(Succeed())
 		})
 	})
+
+	Context("multiple policies with same target ref", func() {
+		BeforeEach(func(ctx SpecContext) {
+			gateway = tests.NewGatewayBuilder(tests.GatewayName, gatewayClass.Name, testNamespace).
+				WithHTTPListener(tests.ListenerNameOne, tests.HostOne(domain)).
+				Gateway
+			Expect(k8sClient.Create(ctx, gateway)).To(Succeed())
+		})
+
+		It("should conflict on the second created policy", func(ctx SpecContext) {
+			By("creating a dns policy")
+			dnsPolicy = tests.NewDNSPolicy("test-dns-policy", testNamespace).
+				WithProviderSecret(*dnsProviderSecret).
+				WithTargetGateway(gateway.Name)
+			Expect(k8sClient.Create(ctx, dnsPolicy)).To(Succeed())
+
+			Eventually(func(g Gomega) {
+				//Check policy is accepted and enforced
+				g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsPolicy), dnsPolicy)).To(Succeed())
+				g.Expect(dnsPolicy.Status.Conditions).To(
+					ContainElements(
+						MatchFields(IgnoreExtras, Fields{
+							"Type":   Equal(string(gatewayapiv1alpha2.PolicyConditionAccepted)),
+							"Status": Equal(metav1.ConditionTrue),
+						}),
+						MatchFields(IgnoreExtras, Fields{
+							"Type":   Equal(string(kuadrant.PolicyConditionEnforced)),
+							"Status": Equal(metav1.ConditionTrue),
+						}),
+					),
+				)
+			}, tests.TimeoutMedium, time.Second).Should(Succeed())
+
+			By("creating a second dns policy")
+			dnsPolicy2 := tests.NewDNSPolicy("test-dns-policy-2", testNamespace).
+				WithProviderSecret(*dnsProviderSecret).
+				WithTargetGateway(gateway.Name)
+			Expect(k8sClient.Create(ctx, dnsPolicy2)).To(Succeed())
+
+			Eventually(func(g Gomega) {
+				//Check second policy is not accepted or enforced
+				g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsPolicy2), dnsPolicy2)).To(Succeed())
+				g.Expect(dnsPolicy2.Status.Conditions).To(
+					ContainElement(
+						MatchFields(IgnoreExtras, Fields{
+							"Type":    Equal(string(gatewayapiv1alpha2.PolicyConditionAccepted)),
+							"Status":  Equal(metav1.ConditionFalse),
+							"Reason":  Equal(string(gatewayapiv1alpha2.PolicyReasonConflicted)),
+							"Message": Equal(fmt.Sprintf("DNSPolicy is conflicted by %s: conflicting policy", client.ObjectKeyFromObject(dnsPolicy).String())),
+						}),
+					),
+				)
+				g.Expect(dnsPolicy2.Status.Conditions).ToNot(ContainElement(MatchFields(IgnoreExtras, Fields{
+					"Type": Equal(string(kuadrant.PolicyConditionEnforced)),
+				})))
+				//Check first policy is still accepted and enforced
+				g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsPolicy), dnsPolicy)).To(Succeed())
+				g.Expect(dnsPolicy.Status.Conditions).To(
+					ContainElements(
+						MatchFields(IgnoreExtras, Fields{
+							"Type":   Equal(string(gatewayapiv1alpha2.PolicyConditionAccepted)),
+							"Status": Equal(metav1.ConditionTrue),
+						}),
+						MatchFields(IgnoreExtras, Fields{
+							"Type":   Equal(string(kuadrant.PolicyConditionEnforced)),
+							"Status": Equal(metav1.ConditionTrue),
+						}),
+					),
+				)
+			}, tests.TimeoutMedium, time.Second).Should(Succeed())
+
+			By("deleting the first dns policy")
+			Expect(k8sClient.Delete(ctx, dnsPolicy)).To(Succeed())
+
+			Eventually(func(g Gomega) {
+				//Check second policy is now accepted and enforced
+				g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsPolicy2), dnsPolicy2)).To(Succeed())
+				g.Expect(dnsPolicy2.Status.Conditions).To(
+					ContainElements(
+						MatchFields(IgnoreExtras, Fields{
+							"Type":   Equal(string(gatewayapiv1alpha2.PolicyConditionAccepted)),
+							"Status": Equal(metav1.ConditionTrue),
+						}),
+						MatchFields(IgnoreExtras, Fields{
+							"Type":   Equal(string(kuadrant.PolicyConditionEnforced)),
+							"Status": Equal(metav1.ConditionTrue),
+						}),
+					),
+				)
+			}, tests.TimeoutMedium, time.Second).Should(Succeed())
+		}, testTimeOut)
+	})
 })