From 3ca2c691a9ef6975b2c9bd43d0e882770e1124a4 Mon Sep 17 00:00:00 2001 From: Michael Nairn Date: Fri, 8 Nov 2024 17:09:31 +0000 Subject: [PATCH] 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 --- api/v1/dnspolicy_types.go | 1 + api/v1/tlspolicy_types.go | 1 + controllers/dnspolicies_validator.go | 48 +++++++++- controllers/tlspolicies_validator.go | 2 +- .../dnspolicy/dnspolicy_controller_test.go | 92 +++++++++++++++++++ 5 files changed, 138 insertions(+), 6 deletions(-) diff --git a/api/v1/dnspolicy_types.go b/api/v1/dnspolicy_types.go index 02a0eb31e..41fa901e7 100644 --- a/api/v1/dnspolicy_types.go +++ b/api/v1/dnspolicy_types.go @@ -197,6 +197,7 @@ func (p *DNSPolicy) Validate() error { return p.Spec.ExcludeAddresses.Validate() } +// Deprecated: Use GetTargetRefs instead 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 index 1269317d7..b0c57c0f7 100644 --- a/api/v1/tlspolicy_types.go +++ b/api/v1/tlspolicy_types.go @@ -175,6 +175,7 @@ func (p *TLSPolicy) Kind() string { return TLSPolicyGroupKind.Kind } +// Deprecated: Use GetTargetRefs instead func (p *TLSPolicy) GetTargetRef() gatewayapiv1alpha2.LocalPolicyTargetReference { return p.Spec.TargetRef.LocalPolicyTargetReference } diff --git a/controllers/dnspolicies_validator.go b/controllers/dnspolicies_validator.go index 2679060a8..776d98b4d 100644 --- 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,15 +37,24 @@ 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() })) @@ -50,3 +62,29 @@ func (r *DNSPoliciesValidator) validate(ctx context.Context, _ []controller.Reso 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 index 596c549a2..61e0df16c 100644 --- 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 index 5912cf3fd..ab8e1014b 100644 --- a/tests/common/dnspolicy/dnspolicy_controller_test.go +++ b/tests/common/dnspolicy/dnspolicy_controller_test.go @@ -1660,4 +1660,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) + }) })