From b8ec7c4b5dec9dd8c67cc3370a35e9eda66d2c56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=82=9F=E7=A9=BA?= Date: Tue, 22 Apr 2025 10:33:09 +0800 Subject: [PATCH 1/4] feat: update HTTPRoutePolicy status on HTTPRoute/Ingress deleting --- internal/controller/httproutepolicy.go | 58 +++++++++++++++++++++++--- internal/controller/indexer/indexer.go | 15 +++---- internal/provider/provider.go | 3 +- 3 files changed, 62 insertions(+), 14 deletions(-) diff --git a/internal/controller/httproutepolicy.go b/internal/controller/httproutepolicy.go index 8b2a9bd41..c991d2949 100644 --- a/internal/controller/httproutepolicy.go +++ b/internal/controller/httproutepolicy.go @@ -1,12 +1,15 @@ package controller import ( + "cmp" "context" "time" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" + "k8s.io/utils/strings/slices" "sigs.k8s.io/controller-runtime/pkg/client" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" @@ -23,14 +26,33 @@ func (r *HTTPRouteReconciler) processHTTPRoutePolicies(tctx *provider.TranslateC object: httpRoute, policies: make(map[targetRefKey][]v1alpha1.HTTPRoutePolicy), } - listForAllRules v1alpha1.HTTPRoutePolicyList - key = indexer.GenHTTPRoutePolicyIndexKey(gatewayv1.GroupName, "HTTPRoute", httpRoute.GetNamespace(), httpRoute.GetName(), "") + list v1alpha1.HTTPRoutePolicyList + key = indexer.GenIndexKeyWithGK(gatewayv1.GroupName, "HTTPRoute", httpRoute.GetNamespace(), httpRoute.GetName()) ) - if err := r.List(context.Background(), &listForAllRules, client.MatchingFields{indexer.PolicyTargetRefs: key}); err != nil { + if err := r.List(context.Background(), &list, client.MatchingFields{indexer.PolicyTargetRefs: key}); err != nil { return err } - for _, item := range listForAllRules.Items { + tctx.HTTPRoutePolicies = list.Items + if len(tctx.HTTPRoutePolicies) == 0 { + return nil + } + + var conflict = false +Loop: + for _, rule := range httpRoute.Spec.Rules { + if rule.Name == nil || *rule.Name == "" { + priority := tctx.HTTPRoutePolicies[0].Spec.Priority + for _, policy := range tctx.HTTPRoutePolicies { + if !ptr.Equal(priority, policy.Spec.Priority) { + conflict = true + break Loop + } + } + } + } + + for _, item := range list.Items { checker.append("", item) tctx.HTTPRoutePolicies["*"] = append(tctx.HTTPRoutePolicies["*"], item) } @@ -105,7 +127,7 @@ func (r *IngressReconciler) processHTTPRoutePolicies(tctx *provider.TranslateCon conflict: false, } list v1alpha1.HTTPRoutePolicyList - key = indexer.GenHTTPRoutePolicyIndexKey(networkingv1.GroupName, "Ingress", ingress.GetNamespace(), ingress.GetName(), "") + key = indexer.GenIndexKeyWithGK(networkingv1.GroupName, "Ingress", ingress.GetNamespace(), ingress.GetName()) ) if err := r.List(context.Background(), &list, client.MatchingFields{indexer.PolicyTargetRefs: key}); err != nil { return err @@ -188,3 +210,29 @@ func (c *conflictChecker) append(sectionName string, policy v1alpha1.HTTPRoutePo } } } + +func isHTTPRoutePolicyConflictOnHTTPRoute(rules []gatewayv1.HTTPRouteRule, policies []v1alpha1.HTTPRoutePolicy) bool { + var m = make(map[targetRefKey]v1alpha1.HTTPRoutePolicy) + for _, policy := range policies { + for _, ref := range policy.Spec.TargetRefs { + var sectionName gatewayv1.SectionName + if ref.SectionName != nil { + sectionName = *ref.SectionName + } + key := targetRefKey{ + Group: ref.Group, + Namespace: gatewayv1.Namespace(policy.GetNamespace()), + Name: ref.Name, + SectionName: sectionName, + } + m[key] = policy + } + } + for _, rule := range rules { + if rule.Name == nil || *rule.Name == "" { + + } else { + + } + } +} diff --git a/internal/controller/indexer/indexer.go b/internal/controller/indexer/indexer.go index 7a606b274..fd1b69896 100644 --- a/internal/controller/indexer/indexer.go +++ b/internal/controller/indexer/indexer.go @@ -322,19 +322,20 @@ func HTTPRouteExtensionIndexFunc(rawObj client.Object) []string { return keys } -func GenHTTPRoutePolicyIndexKey(group, kind, namespace, name, sectionName string) string { - return schema.GroupKind{Group: group, Kind: kind}.String() + "/" + client.ObjectKey{Namespace: namespace, Name: name}.String() + "/" + sectionName -} +// func GenHTTPRoutePolicyIndexKey(group, kind, namespace, name, sectionName string) string { +// return schema.GroupKind{Group: group, Kind: kind}.String() + "/" + client.ObjectKey{Namespace: namespace, Name: name}.String() + "/" + sectionName +// } func HTTPRoutePolicyIndexFunc(rawObj client.Object) []string { hrp := rawObj.(*v1alpha1.HTTPRoutePolicy) var keys = make([]string, 0, len(hrp.Spec.TargetRefs)) + var m = make(map[string]struct{}) for _, ref := range hrp.Spec.TargetRefs { - var sectionName string - if ref.SectionName != nil { - sectionName = string(*ref.SectionName) + key := GenIndexKeyWithGK(string(ref.Group), string(ref.Kind), hrp.GetNamespace(), string(ref.Name)) + if _, ok := m[key]; !ok { + m[key] = struct{}{} + keys = append(keys, key) } - keys = append(keys, GenHTTPRoutePolicyIndexKey(string(ref.Group), string(ref.Kind), hrp.GetNamespace(), string(ref.Name), sectionName)) } return keys } diff --git a/internal/provider/provider.go b/internal/provider/provider.go index cc0a362ff..b359c31d9 100644 --- a/internal/provider/provider.go +++ b/internal/provider/provider.go @@ -38,7 +38,7 @@ type TranslateContext struct { BackendTrafficPolicies map[types.NamespacedName]*v1alpha1.BackendTrafficPolicy GatewayProxies map[ResourceKind]v1alpha1.GatewayProxy ResourceParentRefs map[ResourceKind][]ResourceKind - HTTPRoutePolicies map[string][]v1alpha1.HTTPRoutePolicy + HTTPRoutePolicies []v1alpha1.HTTPRoutePolicy StatusUpdaters []client.Object } @@ -53,6 +53,5 @@ func NewDefaultTranslateContext(ctx context.Context) *TranslateContext { BackendTrafficPolicies: make(map[types.NamespacedName]*v1alpha1.BackendTrafficPolicy), GatewayProxies: make(map[ResourceKind]v1alpha1.GatewayProxy), ResourceParentRefs: make(map[ResourceKind][]ResourceKind), - HTTPRoutePolicies: make(map[string][]v1alpha1.HTTPRoutePolicy), } } From cf9f3ef1d06d5946ce64730bcfd0fe613e88ae12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=82=9F=E7=A9=BA?= Date: Fri, 25 Apr 2025 16:01:43 +0800 Subject: [PATCH 2/4] Refactor HTTPRoutePolicy conflict detection and handling --- internal/controller/httproute_controller.go | 21 +- internal/controller/httproutepolicy.go | 193 +++++------------- internal/controller/indexer/indexer.go | 4 - internal/provider/adc/translator/httproute.go | 23 ++- test/e2e/gatewayapi/httproute.go | 1 + 5 files changed, 70 insertions(+), 172 deletions(-) diff --git a/internal/controller/httproute_controller.go b/internal/controller/httproute_controller.go index f4f9b7f60..8ba3cb5e3 100644 --- a/internal/controller/httproute_controller.go +++ b/internal/controller/httproute_controller.go @@ -7,11 +7,13 @@ import ( "github.com/go-logr/logr" "github.com/pkg/errors" + "golang.org/x/exp/slices" corev1 "k8s.io/api/core/v1" discoveryv1 "k8s.io/api/discovery/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -511,21 +513,14 @@ func httpRoutePolicyPredicateFuncs(channel chan event.GenericEvent) predicate.Pr if !ok0 || !ok1 { return false } - var discardsRefs = make(map[string]v1alpha2.LocalPolicyTargetReferenceWithSectionName) - for _, ref := range oldPolicy.Spec.TargetRefs { - key := indexer.GenHTTPRoutePolicyIndexKey(string(ref.Group), string(ref.Kind), e.ObjectOld.GetNamespace(), string(ref.Name), "") - discardsRefs[key] = ref - } - for _, ref := range newPolicy.Spec.TargetRefs { - key := indexer.GenHTTPRoutePolicyIndexKey(string(ref.Group), string(ref.Kind), e.ObjectOld.GetNamespace(), string(ref.Name), "") - delete(discardsRefs, key) - } + discardsRefs := slices.DeleteFunc(oldPolicy.Spec.TargetRefs, func(oldRef v1alpha2.LocalPolicyTargetReferenceWithSectionName) bool { + return slices.ContainsFunc(newPolicy.Spec.TargetRefs, func(newRef v1alpha2.LocalPolicyTargetReferenceWithSectionName) bool { + return oldRef.LocalPolicyTargetReference == newRef.LocalPolicyTargetReference && ptr.Equal(oldRef.SectionName, newRef.SectionName) + }) + }) if len(discardsRefs) > 0 { dump := oldPolicy.DeepCopy() - dump.Spec.TargetRefs = make([]v1alpha2.LocalPolicyTargetReferenceWithSectionName, 0, len(discardsRefs)) - for _, ref := range discardsRefs { - dump.Spec.TargetRefs = append(dump.Spec.TargetRefs, ref) - } + dump.Spec.TargetRefs = discardsRefs channel <- event.GenericEvent{Object: dump} } return true diff --git a/internal/controller/httproutepolicy.go b/internal/controller/httproutepolicy.go index c991d2949..5c0da6059 100644 --- a/internal/controller/httproutepolicy.go +++ b/internal/controller/httproutepolicy.go @@ -1,15 +1,13 @@ package controller import ( - "cmp" "context" - "time" + "encoding/json" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" - "k8s.io/utils/strings/slices" "sigs.k8s.io/controller-runtime/pkg/client" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" @@ -22,10 +20,6 @@ import ( func (r *HTTPRouteReconciler) processHTTPRoutePolicies(tctx *provider.TranslateContext, httpRoute *gatewayv1.HTTPRoute) error { // list HTTPRoutePolices which sectionName is not specified var ( - checker = conflictChecker{ - object: httpRoute, - policies: make(map[targetRefKey][]v1alpha1.HTTPRoutePolicy), - } list v1alpha1.HTTPRoutePolicyList key = indexer.GenIndexKeyWithGK(gatewayv1.GroupName, "HTTPRoute", httpRoute.GetNamespace(), httpRoute.GetName()) ) @@ -33,87 +27,41 @@ func (r *HTTPRouteReconciler) processHTTPRoutePolicies(tctx *provider.TranslateC return err } - tctx.HTTPRoutePolicies = list.Items - if len(tctx.HTTPRoutePolicies) == 0 { + if len(list.Items) == 0 { return nil } - var conflict = false -Loop: + var conflicts = make(map[types.NamespacedName]v1alpha1.HTTPRoutePolicy) for _, rule := range httpRoute.Spec.Rules { - if rule.Name == nil || *rule.Name == "" { - priority := tctx.HTTPRoutePolicies[0].Spec.Priority - for _, policy := range tctx.HTTPRoutePolicies { - if !ptr.Equal(priority, policy.Spec.Priority) { - conflict = true - break Loop - } + var policies = findPolicyWhichTargetRefTheRule(rule.Name, "HTTPRoute", list) + if conflict := isPoliciesConflict(policies); conflict { + for _, policy := range policies { + namespacedName := types.NamespacedName{Namespace: policy.GetNamespace(), Name: policy.GetName()} + conflicts[namespacedName] = policy } } } + data, _ := json.MarshalIndent(conflicts, "", " ") + r.Log.Info("conflicts policies", "data", string(data)) - for _, item := range list.Items { - checker.append("", item) - tctx.HTTPRoutePolicies["*"] = append(tctx.HTTPRoutePolicies["*"], item) - } - - for _, rule := range httpRoute.Spec.Rules { - if rule.Name == nil { - continue - } - + for i := range list.Items { var ( - ruleName = string(*rule.Name) - listForSectionRules v1alpha1.HTTPRoutePolicyList - key = indexer.GenHTTPRoutePolicyIndexKey(gatewayv1.GroupName, "HTTPRoute", httpRoute.GetNamespace(), httpRoute.GetName(), ruleName) - ) - if err := r.List(context.Background(), &listForSectionRules, client.MatchingFields{indexer.PolicyTargetRefs: key}); err != nil { - continue - } - for _, item := range listForSectionRules.Items { - checker.append(ruleName, item) - tctx.HTTPRoutePolicies[ruleName] = append(tctx.HTTPRoutePolicies[ruleName], item) - } - } - - // todo: unreachable - // if the HTTPRoute is deleted, clear tctx.HTTPRoutePolicies and delete Ancestors from HTTPRoutePolicies status - // if !httpRoute.GetDeletionTimestamp().IsZero() { - // for _, policies := range checker.policies { - // for i := range policies { - // policy := policies[i] - // _ = DeleteAncestors(&policy.Status, httpRoute.Spec.ParentRefs) - // data, _ := json.Marshal(policy.Status) - // r.Log.Info("policy status after delete ancestor", "data", string(data)) - // if err := r.Status().Update(context.Background(), &policy); err != nil { - // r.Log.Error(err, "failed to Update policy status") - // } - // // tctx.StatusUpdaters = append(tctx.StatusUpdaters, &policy) - // } - // } - // return nil - // } - - var ( - status = true - reason = string(v1alpha2.PolicyReasonAccepted) - message string - ) - if checker.conflict { - status = false - reason = string(v1alpha2.PolicyReasonConflicted) - message = "HTTPRoutePolicy conflict with others target to the HTTPRoute" + policy = list.Items[i] + namespacedName = types.NamespacedName{Namespace: policy.GetNamespace(), Name: policy.GetName()} - // clear HTTPRoutePolices from TranslateContext - tctx.HTTPRoutePolicies = make(map[string][]v1alpha1.HTTPRoutePolicy) - } + status = false + reason = string(v1alpha2.PolicyReasonConflicted) + message = "HTTPRoutePolicy conflict with others target to the HTTPRoute" + ) + if _, conflict := conflicts[namespacedName]; !conflict { + status = true + reason = string(v1alpha2.PolicyReasonAccepted) + message = "" - for _, policies := range checker.policies { - for i := range policies { - policy := policies[i] - modifyHTTPRoutePolicyStatus(httpRoute.Spec.ParentRefs, &policy, status, reason, message) - tctx.StatusUpdaters = append(tctx.StatusUpdaters, &policy) + tctx.HTTPRoutePolicies = append(tctx.HTTPRoutePolicies, policy) } + modifyHTTPRoutePolicyStatus(httpRoute.Spec.ParentRefs, &policy, status, reason, message) + tctx.StatusUpdaters = append(tctx.StatusUpdaters, &policy) } return nil @@ -121,11 +69,6 @@ Loop: func (r *IngressReconciler) processHTTPRoutePolicies(tctx *provider.TranslateContext, ingress *networkingv1.Ingress) error { var ( - checker = conflictChecker{ - object: ingress, - policies: make(map[targetRefKey][]v1alpha1.HTTPRoutePolicy), - conflict: false, - } list v1alpha1.HTTPRoutePolicyList key = indexer.GenIndexKeyWithGK(networkingv1.GroupName, "Ingress", ingress.GetNamespace(), ingress.GetName()) ) @@ -133,23 +76,21 @@ func (r *IngressReconciler) processHTTPRoutePolicies(tctx *provider.TranslateCon return err } - for _, item := range list.Items { - checker.append("", item) - tctx.HTTPRoutePolicies["*"] = append(tctx.HTTPRoutePolicies["*"], item) + if len(list.Items) == 0 { + return nil } var ( - status = true - reason = string(v1alpha2.PolicyReasonAccepted) - message string - ) - if checker.conflict { - status = false - reason = string(v1alpha2.PolicyReasonConflicted) + status = false + reason = string(v1alpha2.PolicyReasonConflicted) message = "HTTPRoutePolicy conflict with others target to the Ingress" + ) + if conflict := isPoliciesConflict(list.Items); !conflict { + status = true + reason = string(v1alpha2.PolicyReasonAccepted) + message = "" - // clear HTTPRoutePolicies from TranslateContext - tctx.HTTPRoutePolicies = make(map[string][]v1alpha1.HTTPRoutePolicy) + tctx.HTTPRoutePolicies = list.Items } for i := range list.Items { @@ -166,7 +107,7 @@ func modifyHTTPRoutePolicyStatus(parentRefs []gatewayv1.ParentReference, policy Type: string(v1alpha2.PolicyConditionAccepted), Status: metav1.ConditionTrue, ObservedGeneration: policy.GetGeneration(), - LastTransitionTime: metav1.Time{Time: time.Now()}, + LastTransitionTime: metav1.Now(), Reason: reason, Message: message, } @@ -176,63 +117,27 @@ func modifyHTTPRoutePolicyStatus(parentRefs []gatewayv1.ParentReference, policy _ = SetAncestors(&policy.Status, parentRefs, condition) } -type conflictChecker struct { - object client.Object - policies map[targetRefKey][]v1alpha1.HTTPRoutePolicy - conflict bool -} - -type targetRefKey struct { - Group gatewayv1.Group - Namespace gatewayv1.Namespace - Name gatewayv1.ObjectName - SectionName gatewayv1.SectionName -} - -func (c *conflictChecker) append(sectionName string, policy v1alpha1.HTTPRoutePolicy) { - key := targetRefKey{ - Group: gatewayv1.GroupName, - Namespace: gatewayv1.Namespace(c.object.GetNamespace()), - Name: gatewayv1.ObjectName(c.object.GetName()), - SectionName: gatewayv1.SectionName(sectionName), +func isPoliciesConflict(policies []v1alpha1.HTTPRoutePolicy) bool { + if len(policies) == 0 { + return false } - c.policies[key] = append(c.policies[key], policy) - - if !c.conflict { - Loop: - for _, items := range c.policies { - for _, item := range items { - if !ptr.Equal(item.Spec.Priority, policy.Spec.Priority) { - c.conflict = true - break Loop - } - } + priority := policies[0].Spec.Priority + for _, policy := range policies { + if !ptr.Equal(policy.Spec.Priority, priority) { + return true } } + return false } -func isHTTPRoutePolicyConflictOnHTTPRoute(rules []gatewayv1.HTTPRouteRule, policies []v1alpha1.HTTPRoutePolicy) bool { - var m = make(map[targetRefKey]v1alpha1.HTTPRoutePolicy) - for _, policy := range policies { +func findPolicyWhichTargetRefTheRule(ruleName *gatewayv1.SectionName, kind string, list v1alpha1.HTTPRoutePolicyList) (policies []v1alpha1.HTTPRoutePolicy) { + for _, policy := range list.Items { for _, ref := range policy.Spec.TargetRefs { - var sectionName gatewayv1.SectionName - if ref.SectionName != nil { - sectionName = *ref.SectionName - } - key := targetRefKey{ - Group: ref.Group, - Namespace: gatewayv1.Namespace(policy.GetNamespace()), - Name: ref.Name, - SectionName: sectionName, + if string(ref.Kind) == kind && (ref.SectionName == nil || *ref.SectionName == "" || ptr.Equal(ref.SectionName, ruleName)) { + policies = append(policies, policy) + break } - m[key] = policy - } - } - for _, rule := range rules { - if rule.Name == nil || *rule.Name == "" { - - } else { - } } + return } diff --git a/internal/controller/indexer/indexer.go b/internal/controller/indexer/indexer.go index fd1b69896..3b959b6d5 100644 --- a/internal/controller/indexer/indexer.go +++ b/internal/controller/indexer/indexer.go @@ -322,10 +322,6 @@ func HTTPRouteExtensionIndexFunc(rawObj client.Object) []string { return keys } -// func GenHTTPRoutePolicyIndexKey(group, kind, namespace, name, sectionName string) string { -// return schema.GroupKind{Group: group, Kind: kind}.String() + "/" + client.ObjectKey{Namespace: namespace, Name: name}.String() + "/" + sectionName -// } - func HTTPRoutePolicyIndexFunc(rawObj client.Object) []string { hrp := rawObj.(*v1alpha1.HTTPRoutePolicy) var keys = make([]string, 0, len(hrp.Spec.TargetRefs)) diff --git a/internal/provider/adc/translator/httproute.go b/internal/provider/adc/translator/httproute.go index f7af81bb5..0f59a9b53 100644 --- a/internal/provider/adc/translator/httproute.go +++ b/internal/provider/adc/translator/httproute.go @@ -233,23 +233,24 @@ func (t *Translator) fillPluginFromHTTPRequestRedirectFilter(plugins adctypes.Pl } func (t *Translator) fillHTTPRoutePoliciesForHTTPRoute(tctx *provider.TranslateContext, routes []*adctypes.Route, rule gatewayv1.HTTPRouteRule) { - var keys = []string{"*"} - if rule.Name != nil { - keys = append(keys, string(*rule.Name)) + var policies []v1alpha1.HTTPRoutePolicy + for _, policy := range tctx.HTTPRoutePolicies { + for _, ref := range policy.Spec.TargetRefs { + if string(ref.Kind) == "HTTPRoute" && (ref.SectionName == nil || *ref.SectionName == "" || ptr.Equal(ref.SectionName, rule.Name)) { + policies = append(policies, policy) + break + } + } } - t.fillHTTPRoutePolicies(tctx, routes, keys...) + + t.fillHTTPRoutePolicies(routes, policies) } func (t *Translator) fillHTTPRoutePoliciesForIngress(tctx *provider.TranslateContext, routes []*adctypes.Route) { - t.fillHTTPRoutePolicies(tctx, routes, "*") + t.fillHTTPRoutePolicies(routes, tctx.HTTPRoutePolicies) } -func (t *Translator) fillHTTPRoutePolicies(tctx *provider.TranslateContext, routes []*adctypes.Route, ctxKeys ...string) { - var policies []v1alpha1.HTTPRoutePolicy - for _, key := range ctxKeys { - policies = append(policies, tctx.HTTPRoutePolicies[key]...) - } - +func (t *Translator) fillHTTPRoutePolicies(routes []*adctypes.Route, policies []v1alpha1.HTTPRoutePolicy) { for _, policy := range policies { for _, route := range routes { route.Priority = policy.Spec.Priority diff --git a/test/e2e/gatewayapi/httproute.go b/test/e2e/gatewayapi/httproute.go index 5838be81b..c850a08f0 100644 --- a/test/e2e/gatewayapi/httproute.go +++ b/test/e2e/gatewayapi/httproute.go @@ -477,6 +477,7 @@ spec: Status(200) }) }) + Context("HTTPRoute Rule Match", func() { var exactRouteByGet = ` apiVersion: gateway.networking.k8s.io/v1 From d8ea4b6f101421d5898772ba3580db41b3f35139 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=82=9F=E7=A9=BA?= Date: Sun, 27 Apr 2025 10:17:02 +0800 Subject: [PATCH 3/4] Update HTTPRoutePolicy status on resource deletion Add logic to update HTTPRoutePolicy status when associated Ingress or HTTPRoute resources are deleted. Introduce new methods to handle status updates and remove ancestor references, along with corresponding e2e tests for ingress and gateway API scenarios. --- internal/controller/httproute_controller.go | 3 + internal/controller/httproutepolicy.go | 96 ++++++++++++++++++++- internal/controller/ingress_controller.go | 4 + internal/controller/policies.go | 18 +--- test/e2e/gatewayapi/httproute.go | 8 +- test/e2e/ingress/ingress.go | 41 +++++++++ 6 files changed, 151 insertions(+), 19 deletions(-) diff --git a/internal/controller/httproute_controller.go b/internal/controller/httproute_controller.go index 8ba3cb5e3..71a148037 100644 --- a/internal/controller/httproute_controller.go +++ b/internal/controller/httproute_controller.go @@ -97,6 +97,9 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( hr := new(gatewayv1.HTTPRoute) if err := r.Get(ctx, req.NamespacedName, hr); err != nil { if client.IgnoreNotFound(err) == nil { + if err := r.updateHTTPRouteStatusOnDeleting(req.NamespacedName); err != nil { + return ctrl.Result{}, err + } hr.Namespace = req.Namespace hr.Name = req.Name diff --git a/internal/controller/httproutepolicy.go b/internal/controller/httproutepolicy.go index 5c0da6059..06433e97e 100644 --- a/internal/controller/httproutepolicy.go +++ b/internal/controller/httproutepolicy.go @@ -2,8 +2,9 @@ package controller import ( "context" - "encoding/json" + "slices" + "github.com/go-logr/logr" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -41,8 +42,6 @@ func (r *HTTPRouteReconciler) processHTTPRoutePolicies(tctx *provider.TranslateC } } } - data, _ := json.MarshalIndent(conflicts, "", " ") - r.Log.Info("conflicts policies", "data", string(data)) for i := range list.Items { var ( @@ -67,6 +66,39 @@ func (r *HTTPRouteReconciler) processHTTPRoutePolicies(tctx *provider.TranslateC return nil } +func (r *HTTPRouteReconciler) updateHTTPRouteStatusOnDeleting(nn types.NamespacedName) error { + var ( + list v1alpha1.HTTPRoutePolicyList + key = indexer.GenIndexKeyWithGK(gatewayv1.GroupName, "HTTPRoute", nn.Namespace, nn.Name) + ) + if err := r.List(context.Background(), &list, client.MatchingFields{indexer.PolicyTargetRefs: key}); err != nil { + return err + } + var ( + objs = make(map[types.NamespacedName]struct{}) + parentRefs []gatewayv1.ParentReference + ) + // collect all parentRefs + for _, policy := range list.Items { + for _, ref := range policy.Spec.TargetRefs { + var obj = types.NamespacedName{Namespace: policy.GetNamespace(), Name: string(ref.Name)} + if _, ok := objs[obj]; !ok { + objs[obj] = struct{}{} + + var httpRoute gatewayv1.HTTPRoute + if err := r.Get(context.Background(), obj, &httpRoute); err != nil { + continue + } + parentRefs = append(parentRefs, httpRoute.Spec.ParentRefs...) + } + } + } + // delete AncestorRef which is not exist in the all parentRefs for each policy + updateDeleteAncestors(r.Client, r.Log, list.Items, parentRefs) + + return nil +} + func (r *IngressReconciler) processHTTPRoutePolicies(tctx *provider.TranslateContext, ingress *networkingv1.Ingress) error { var ( list v1alpha1.HTTPRoutePolicyList @@ -102,6 +134,47 @@ func (r *IngressReconciler) processHTTPRoutePolicies(tctx *provider.TranslateCon return nil } +func (r *IngressReconciler) updateHTTPRoutePolicyStatusOnDeleting(nn types.NamespacedName) error { + var ( + list v1alpha1.HTTPRoutePolicyList + key = indexer.GenIndexKeyWithGK(networkingv1.GroupName, "Ingress", nn.Namespace, nn.Name) + ) + if err := r.List(context.Background(), &list, client.MatchingFields{indexer.PolicyTargetRefs: key}); err != nil { + return err + } + var ( + objs = make(map[types.NamespacedName]struct{}) + parentRefs []gatewayv1.ParentReference + ) + // collect all parentRefs + for _, policy := range list.Items { + for _, ref := range policy.Spec.TargetRefs { + var obj = types.NamespacedName{Namespace: policy.GetNamespace(), Name: string(ref.Name)} + if _, ok := objs[obj]; !ok { + objs[obj] = struct{}{} + + var ingress networkingv1.Ingress + if err := r.Get(context.Background(), obj, &ingress); err != nil { + continue + } + ingressClass, err := r.getIngressClass(&ingress) + if err != nil { + continue + } + parentRefs = append(parentRefs, gatewayv1.ParentReference{ + Group: ptr.To(gatewayv1.Group(ingressClass.GroupVersionKind().Group)), + Kind: ptr.To(gatewayv1.Kind("IngressClass")), + Name: gatewayv1.ObjectName(ingressClass.Name), + }) + } + } + } + // delete AncestorRef which is not exist in the all parentRefs + updateDeleteAncestors(r.Client, r.Log, list.Items, parentRefs) + + return nil +} + func modifyHTTPRoutePolicyStatus(parentRefs []gatewayv1.ParentReference, policy *v1alpha1.HTTPRoutePolicy, status bool, reason, message string) { condition := metav1.Condition{ Type: string(v1alpha2.PolicyConditionAccepted), @@ -141,3 +214,20 @@ func findPolicyWhichTargetRefTheRule(ruleName *gatewayv1.SectionName, kind strin } return } + +func updateDeleteAncestors(client client.Client, logger logr.Logger, policies []v1alpha1.HTTPRoutePolicy, parentRefs []gatewayv1.ParentReference) { + for i := range policies { + policy := policies[i] + length := len(policy.Status.Ancestors) + policy.Status.Ancestors = slices.DeleteFunc(policy.Status.Ancestors, func(status v1alpha2.PolicyAncestorStatus) bool { + return !slices.ContainsFunc(parentRefs, func(ref gatewayv1.ParentReference) bool { + return parentRefValueEqual(status.AncestorRef, ref) + }) + }) + if length != len(policy.Status.Ancestors) { + if err := client.Status().Update(context.Background(), &policy); err != nil { + logger.Error(err, "failed to update HTTPRoutePolicy status") + } + } + } +} diff --git a/internal/controller/ingress_controller.go b/internal/controller/ingress_controller.go index 8a49b00c5..35c810e7c 100644 --- a/internal/controller/ingress_controller.go +++ b/internal/controller/ingress_controller.go @@ -97,6 +97,10 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct ingress := new(networkingv1.Ingress) if err := r.Get(ctx, req.NamespacedName, ingress); err != nil { if client.IgnoreNotFound(err) == nil { + if err := r.updateHTTPRoutePolicyStatusOnDeleting(req.NamespacedName); err != nil { + return ctrl.Result{}, err + } + // Ingress was deleted, clean up corresponding resources ingress.Namespace = req.Namespace ingress.Name = req.Name diff --git a/internal/controller/policies.go b/internal/controller/policies.go index 2ef04a3b4..477b09221 100644 --- a/internal/controller/policies.go +++ b/internal/controller/policies.go @@ -2,20 +2,18 @@ package controller import ( "fmt" - "slices" - - "k8s.io/utils/ptr" - gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" - gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" "github.com/api7/api7-ingress-controller/api/v1alpha1" "github.com/api7/api7-ingress-controller/internal/controller/config" @@ -192,16 +190,6 @@ func SetAncestorStatus(status *v1alpha1.PolicyStatus, ancestorStatus gatewayv1al return true } -func DeleteAncestors(status *v1alpha1.PolicyStatus, parentRefs []gatewayv1.ParentReference) bool { - var length = len(status.Ancestors) - for _, parentRef := range parentRefs { - status.Ancestors = slices.DeleteFunc(status.Ancestors, func(status gatewayv1alpha2.PolicyAncestorStatus) bool { - return parentRefValueEqual(parentRef, status.AncestorRef) - }) - } - return length != len(status.Ancestors) -} - func parentRefValueEqual(a, b gatewayv1.ParentReference) bool { return ptr.Equal(a.Group, b.Group) && ptr.Equal(a.Kind, b.Kind) && diff --git a/test/e2e/gatewayapi/httproute.go b/test/e2e/gatewayapi/httproute.go index c850a08f0..32afedfed 100644 --- a/test/e2e/gatewayapi/httproute.go +++ b/test/e2e/gatewayapi/httproute.go @@ -843,13 +843,19 @@ spec: }).WithTimeout(8 * time.Second).ProbeEvery(time.Second).Should(Equal(http.StatusOK)) }) - PIt("HTTPRoutePolicy status changes on HTTPRoute deleting", func() { + It("HTTPRoutePolicy status changes on HTTPRoute deleting", func() { By("create HTTPRoute") ResourceApplied("HTTPRoute", "httpbin", varsRoute, 1) By("create HTTPRoutePolicy") ResourceApplied("HTTPRoutePolicy", "http-route-policy-0", httpRoutePolicy, 1) + Eventually(func() string { + spec, err := s.GetResourceYaml("HTTPRoutePolicy", "http-route-policy-0") + Expect(err).NotTo(HaveOccurred(), "getting HTTPRoutePolicy") + return spec + }).WithTimeout(8 * time.Second).ProbeEvery(time.Second).Should(ContainSubstring("type: Accepted")) + By("access dataplane to check the HTTPRoutePolicy") s.NewAPISIXClient(). GET("/get"). diff --git a/test/e2e/ingress/ingress.go b/test/e2e/ingress/ingress.go index 0e810b4da..335944774 100644 --- a/test/e2e/ingress/ingress.go +++ b/test/e2e/ingress/ingress.go @@ -637,5 +637,46 @@ spec: }). WithTimeout(8 * time.Second).ProbeEvery(time.Second).Should(Equal(http.StatusOK)) }) + + It("HTTPRoutePolicy status changes on Ingress deleting", func() { + By("create Ingress") + err := s.CreateResourceFromString(ingressSpec) + Expect(err).NotTo(HaveOccurred(), "creating Ingress") + + By("create HTTPRoutePolicy") + err = s.CreateResourceFromString(httpRoutePolicySpec0) + Expect(err).NotTo(HaveOccurred(), "creating HTTPRoutePolicy") + Eventually(func() string { + spec, err := s.GetResourceYaml("HTTPRoutePolicy", "http-route-policy-0") + Expect(err).NotTo(HaveOccurred(), "HTTPRoutePolicy status should be True") + return spec + }). + WithTimeout(8 * time.Second).ProbeEvery(time.Second).Should(ContainSubstring(`status: "True"`)) + + By("request the route without vars should be Not Found") + Eventually(func() int { + return s.NewAPISIXClient().GET("/get").WithHost("example.com").Expect().Raw().StatusCode + }). + WithTimeout(8 * time.Second).ProbeEvery(time.Second).Should(Equal(http.StatusNotFound)) + + By("request the route with the correct vars should be OK") + s.NewAPISIXClient().GET("/get").WithHost("example.com"). + WithHeader("X-HRP-Name", "http-route-policy-0").Expect().Status(http.StatusOK) + + By("delete ingress") + err = s.DeleteResource("Ingress", "default") + Expect(err).NotTo(HaveOccurred(), "delete Ingress") + Eventually(func() int { + return s.NewAPISIXClient().GET("/get").WithHost("example.com"). + WithHeader("X-HRP-Name", "http-route-policy-0").Expect().Raw().StatusCode + }). + WithTimeout(8 * time.Second).ProbeEvery(time.Second).Should(Equal(http.StatusNotFound)) + + Eventually(func() string { + spec, err := s.GetResourceYaml("HTTPRoutePolicy", "http-route-policy-0") + Expect(err).NotTo(HaveOccurred(), "getting HTTPRoutePolicy") + return spec + }).WithTimeout(8 * time.Second).ProbeEvery(time.Second).ShouldNot(ContainSubstring("ancestorRef:")) + }) }) }) From cc394f6b1c9519b8fe17d9c813464676ff6fd44d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=82=9F=E7=A9=BA?= Date: Sun, 27 Apr 2025 11:51:36 +0800 Subject: [PATCH 4/4] Refactor HTTPRoutePolicy conflict checks and status updates Rename `isPoliciesConflict` to `checkPoliciesConflict` for clarity and improve related functions for better readability and accuracy. Update method names and logic for deleting ancestors and finding matching policies, while also optimizing the status update process for deleted HTTPRoute resources. --- internal/controller/httproute_controller.go | 2 +- internal/controller/httproutepolicy.go | 88 +++++++++++---------- 2 files changed, 47 insertions(+), 43 deletions(-) diff --git a/internal/controller/httproute_controller.go b/internal/controller/httproute_controller.go index 71a148037..b55a23be1 100644 --- a/internal/controller/httproute_controller.go +++ b/internal/controller/httproute_controller.go @@ -97,7 +97,7 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( hr := new(gatewayv1.HTTPRoute) if err := r.Get(ctx, req.NamespacedName, hr); err != nil { if client.IgnoreNotFound(err) == nil { - if err := r.updateHTTPRouteStatusOnDeleting(req.NamespacedName); err != nil { + if err := r.updateHTTPRoutePolicyStatusOnDeleting(req.NamespacedName); err != nil { return ctrl.Result{}, err } hr.Namespace = req.Namespace diff --git a/internal/controller/httproutepolicy.go b/internal/controller/httproutepolicy.go index 06433e97e..dd9502672 100644 --- a/internal/controller/httproutepolicy.go +++ b/internal/controller/httproutepolicy.go @@ -34,8 +34,8 @@ func (r *HTTPRouteReconciler) processHTTPRoutePolicies(tctx *provider.TranslateC var conflicts = make(map[types.NamespacedName]v1alpha1.HTTPRoutePolicy) for _, rule := range httpRoute.Spec.Rules { - var policies = findPolicyWhichTargetRefTheRule(rule.Name, "HTTPRoute", list) - if conflict := isPoliciesConflict(policies); conflict { + var policies = findPoliciesWhichTargetRefTheRule(rule.Name, "HTTPRoute", list) + if conflict := checkPoliciesConflict(policies); conflict { for _, policy := range policies { namespacedName := types.NamespacedName{Namespace: policy.GetNamespace(), Name: policy.GetName()} conflicts[namespacedName] = policy @@ -66,7 +66,7 @@ func (r *HTTPRouteReconciler) processHTTPRoutePolicies(tctx *provider.TranslateC return nil } -func (r *HTTPRouteReconciler) updateHTTPRouteStatusOnDeleting(nn types.NamespacedName) error { +func (r *HTTPRouteReconciler) updateHTTPRoutePolicyStatusOnDeleting(nn types.NamespacedName) error { var ( list v1alpha1.HTTPRoutePolicyList key = indexer.GenIndexKeyWithGK(gatewayv1.GroupName, "HTTPRoute", nn.Namespace, nn.Name) @@ -75,26 +75,25 @@ func (r *HTTPRouteReconciler) updateHTTPRouteStatusOnDeleting(nn types.Namespace return err } var ( - objs = make(map[types.NamespacedName]struct{}) - parentRefs []gatewayv1.ParentReference + httpRoutes = make(map[types.NamespacedName]gatewayv1.HTTPRoute) ) - // collect all parentRefs for _, policy := range list.Items { + // collect all parentRefs for the HTTPRoutePolicy + var parentRefs []gatewayv1.ParentReference for _, ref := range policy.Spec.TargetRefs { - var obj = types.NamespacedName{Namespace: policy.GetNamespace(), Name: string(ref.Name)} - if _, ok := objs[obj]; !ok { - objs[obj] = struct{}{} - - var httpRoute gatewayv1.HTTPRoute - if err := r.Get(context.Background(), obj, &httpRoute); err != nil { + var namespacedName = types.NamespacedName{Namespace: policy.GetNamespace(), Name: string(ref.Name)} + httpRoute, ok := httpRoutes[namespacedName] + if !ok { + if err := r.Get(context.Background(), namespacedName, &httpRoute); err != nil { continue } - parentRefs = append(parentRefs, httpRoute.Spec.ParentRefs...) + httpRoutes[namespacedName] = httpRoute } + parentRefs = append(parentRefs, httpRoute.Spec.ParentRefs...) } + // delete AncestorRef which is not exist in the all parentRefs for each policy + updateDeleteAncestors(r.Client, r.Log, policy, parentRefs) } - // delete AncestorRef which is not exist in the all parentRefs for each policy - updateDeleteAncestors(r.Client, r.Log, list.Items, parentRefs) return nil } @@ -117,7 +116,7 @@ func (r *IngressReconciler) processHTTPRoutePolicies(tctx *provider.TranslateCon reason = string(v1alpha2.PolicyReasonConflicted) message = "HTTPRoutePolicy conflict with others target to the Ingress" ) - if conflict := isPoliciesConflict(list.Items); !conflict { + if conflict := checkPoliciesConflict(list.Items); !conflict { status = true reason = string(v1alpha2.PolicyReasonAccepted) message = "" @@ -143,34 +142,35 @@ func (r *IngressReconciler) updateHTTPRoutePolicyStatusOnDeleting(nn types.Names return err } var ( - objs = make(map[types.NamespacedName]struct{}) - parentRefs []gatewayv1.ParentReference + ingress2ParentRef = make(map[types.NamespacedName]gatewayv1.ParentReference) ) - // collect all parentRefs for _, policy := range list.Items { + // collect all parentRefs for the HTTPRoutePolicy + var parentRefs []gatewayv1.ParentReference for _, ref := range policy.Spec.TargetRefs { - var obj = types.NamespacedName{Namespace: policy.GetNamespace(), Name: string(ref.Name)} - if _, ok := objs[obj]; !ok { - objs[obj] = struct{}{} - + var namespacedName = types.NamespacedName{Namespace: policy.GetNamespace(), Name: string(ref.Name)} + parentRef, ok := ingress2ParentRef[namespacedName] + if !ok { var ingress networkingv1.Ingress - if err := r.Get(context.Background(), obj, &ingress); err != nil { + if err := r.Get(context.Background(), namespacedName, &ingress); err != nil { continue } ingressClass, err := r.getIngressClass(&ingress) if err != nil { continue } - parentRefs = append(parentRefs, gatewayv1.ParentReference{ + parentRef = gatewayv1.ParentReference{ Group: ptr.To(gatewayv1.Group(ingressClass.GroupVersionKind().Group)), Kind: ptr.To(gatewayv1.Kind("IngressClass")), Name: gatewayv1.ObjectName(ingressClass.Name), - }) + } + ingress2ParentRef[namespacedName] = parentRef } + parentRefs = append(parentRefs, parentRef) } + // delete AncestorRef which is not exist in the all parentRefs + updateDeleteAncestors(r.Client, r.Log, policy, parentRefs) } - // delete AncestorRef which is not exist in the all parentRefs - updateDeleteAncestors(r.Client, r.Log, list.Items, parentRefs) return nil } @@ -190,7 +190,11 @@ func modifyHTTPRoutePolicyStatus(parentRefs []gatewayv1.ParentReference, policy _ = SetAncestors(&policy.Status, parentRefs, condition) } -func isPoliciesConflict(policies []v1alpha1.HTTPRoutePolicy) bool { +// checkPoliciesConflict determines if there is a conflict among the given HTTPRoutePolicy objects based on their priority values. +// It returns true if any policy has a different priority than the first policy in the list, otherwise false. +// An empty or single-element policies slice is considered non-conflicting. +// The function assumes all policies have a valid Spec.Priority field for comparison. +func checkPoliciesConflict(policies []v1alpha1.HTTPRoutePolicy) bool { if len(policies) == 0 { return false } @@ -203,7 +207,9 @@ func isPoliciesConflict(policies []v1alpha1.HTTPRoutePolicy) bool { return false } -func findPolicyWhichTargetRefTheRule(ruleName *gatewayv1.SectionName, kind string, list v1alpha1.HTTPRoutePolicyList) (policies []v1alpha1.HTTPRoutePolicy) { +// findPoliciesWhichTargetRefTheRule filters HTTPRoutePolicy objects whose TargetRefs match the given ruleName and kind. +// A match occurs if the TargetRef's Kind equals the provided kind and its SectionName is nil, empty, or equal to ruleName. +func findPoliciesWhichTargetRefTheRule(ruleName *gatewayv1.SectionName, kind string, list v1alpha1.HTTPRoutePolicyList) (policies []v1alpha1.HTTPRoutePolicy) { for _, policy := range list.Items { for _, ref := range policy.Spec.TargetRefs { if string(ref.Kind) == kind && (ref.SectionName == nil || *ref.SectionName == "" || ptr.Equal(ref.SectionName, ruleName)) { @@ -215,19 +221,17 @@ func findPolicyWhichTargetRefTheRule(ruleName *gatewayv1.SectionName, kind strin return } -func updateDeleteAncestors(client client.Client, logger logr.Logger, policies []v1alpha1.HTTPRoutePolicy, parentRefs []gatewayv1.ParentReference) { - for i := range policies { - policy := policies[i] - length := len(policy.Status.Ancestors) - policy.Status.Ancestors = slices.DeleteFunc(policy.Status.Ancestors, func(status v1alpha2.PolicyAncestorStatus) bool { - return !slices.ContainsFunc(parentRefs, func(ref gatewayv1.ParentReference) bool { - return parentRefValueEqual(status.AncestorRef, ref) - }) +// updateDeleteAncestors removes ancestor references from HTTPRoutePolicy statuses that are no longer present in the provided parentRefs. +func updateDeleteAncestors(client client.Client, logger logr.Logger, policy v1alpha1.HTTPRoutePolicy, parentRefs []gatewayv1.ParentReference) { + length := len(policy.Status.Ancestors) + policy.Status.Ancestors = slices.DeleteFunc(policy.Status.Ancestors, func(status v1alpha2.PolicyAncestorStatus) bool { + return !slices.ContainsFunc(parentRefs, func(ref gatewayv1.ParentReference) bool { + return parentRefValueEqual(status.AncestorRef, ref) }) - if length != len(policy.Status.Ancestors) { - if err := client.Status().Update(context.Background(), &policy); err != nil { - logger.Error(err, "failed to update HTTPRoutePolicy status") - } + }) + if length != len(policy.Status.Ancestors) { + if err := client.Status().Update(context.Background(), &policy); err != nil { + logger.Error(err, "failed to update HTTPRoutePolicy status") } } }