Skip to content

Commit 806c514

Browse files
dspodspo
authored
fix: Refactor HTTPRoutePolicy status handling and tests (#142)
1 parent 89a430b commit 806c514

File tree

6 files changed

+219
-82
lines changed

6 files changed

+219
-82
lines changed

‎internal/controller/httproute_controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
114114
hr := new(gatewayv1.HTTPRoute)
115115
if err := r.Get(ctx, req.NamespacedName, hr); err != nil {
116116
if client.IgnoreNotFound(err) == nil {
117-
if err := r.updateHTTPRoutePolicyStatusOnDeleting(req.NamespacedName); err != nil {
117+
if err := r.updateHTTPRoutePolicyStatusOnDeleting(ctx, req.NamespacedName); err != nil {
118118
return ctrl.Result{}, err
119119
}
120120
hr.Namespace = req.Namespace

‎internal/controller/httproutepolicy.go‎

Lines changed: 38 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
package controller
1414

1515
import (
16+
"cmp"
1617
"context"
1718
"slices"
1819

@@ -59,26 +60,25 @@ func (r *HTTPRouteReconciler) processHTTPRoutePolicies(tctx *provider.TranslateC
5960
var (
6061
policy = list.Items[i]
6162
namespacedName = types.NamespacedName{Namespace: policy.GetNamespace(), Name: policy.GetName()}
62-
63-
status = false
64-
reason = string(v1alpha2.PolicyReasonConflicted)
65-
message = "HTTPRoutePolicy conflict with others target to the HTTPRoute"
63+
condition metav1.Condition
6664
)
67-
if _, conflict := conflicts[namespacedName]; !conflict {
68-
status = true
69-
reason = string(v1alpha2.PolicyReasonAccepted)
70-
message = ""
71-
65+
if _, conflict := conflicts[namespacedName]; conflict {
66+
condition.Status = metav1.ConditionFalse
67+
condition.Reason = string(v1alpha2.PolicyReasonConflicted)
68+
condition.Message = "HTTPRoutePolicy conflict with others target to the HTTPRoute"
69+
} else {
7270
tctx.HTTPRoutePolicies = append(tctx.HTTPRoutePolicies, policy)
7371
}
74-
modifyHTTPRoutePolicyStatus(httpRoute.Spec.ParentRefs, &policy, status, reason, message)
75-
tctx.StatusUpdaters = append(tctx.StatusUpdaters, &policy)
72+
73+
if updated := setAncestorsForHTTPRoutePolicyStatus(httpRoute.Spec.ParentRefs, &policy, condition); updated {
74+
tctx.StatusUpdaters = append(tctx.StatusUpdaters, &policy)
75+
}
7676
}
7777

7878
return nil
7979
}
8080

81-
func (r *HTTPRouteReconciler) updateHTTPRoutePolicyStatusOnDeleting(nn types.NamespacedName) error {
81+
func (r *HTTPRouteReconciler) updateHTTPRoutePolicyStatusOnDeleting(ctx context.Context, nn types.NamespacedName) error {
8282
var (
8383
list v1alpha1.HTTPRoutePolicyList
8484
key = indexer.GenIndexKeyWithGK(gatewayv1.GroupName, "HTTPRoute", nn.Namespace, nn.Name)
@@ -104,7 +104,7 @@ func (r *HTTPRouteReconciler) updateHTTPRoutePolicyStatusOnDeleting(nn types.Nam
104104
parentRefs = append(parentRefs, httpRoute.Spec.ParentRefs...)
105105
}
106106
// delete AncestorRef which is not exist in the all parentRefs for each policy
107-
updateDeleteAncestors(r.Client, r.Log, policy, parentRefs)
107+
updateDeleteAncestors(ctx, r.Client, r.Log, policy, parentRefs)
108108
}
109109

110110
return nil
@@ -124,33 +124,32 @@ func (r *IngressReconciler) processHTTPRoutePolicies(tctx *provider.TranslateCon
124124
}
125125

126126
var (
127-
status = false
128-
reason = string(v1alpha2.PolicyReasonConflicted)
129-
message = "HTTPRoutePolicy conflict with others target to the Ingress"
127+
condition metav1.Condition
130128
)
131-
if conflict := checkPoliciesConflict(list.Items); !conflict {
132-
status = true
133-
reason = string(v1alpha2.PolicyReasonAccepted)
134-
message = ""
135-
129+
if conflict := checkPoliciesConflict(list.Items); conflict {
130+
condition.Status = metav1.ConditionFalse
131+
condition.Reason = string(v1alpha2.PolicyReasonConflicted)
132+
condition.Message = "HTTPRoutePolicy conflict with others target to the Ingress"
133+
} else {
136134
tctx.HTTPRoutePolicies = list.Items
137135
}
138136

139137
for i := range list.Items {
140138
policy := list.Items[i]
141-
modifyHTTPRoutePolicyStatus(tctx.RouteParentRefs, &policy, status, reason, message)
142-
tctx.StatusUpdaters = append(tctx.StatusUpdaters, &policy)
139+
if updated := setAncestorsForHTTPRoutePolicyStatus(tctx.RouteParentRefs, &policy, condition); updated {
140+
tctx.StatusUpdaters = append(tctx.StatusUpdaters, &policy)
141+
}
143142
}
144143

145144
return nil
146145
}
147146

148-
func (r *IngressReconciler) updateHTTPRoutePolicyStatusOnDeleting(nn types.NamespacedName) error {
147+
func (r *IngressReconciler) updateHTTPRoutePolicyStatusOnDeleting(ctx context.Context, nn types.NamespacedName) error {
149148
var (
150149
list v1alpha1.HTTPRoutePolicyList
151150
key = indexer.GenIndexKeyWithGK(networkingv1.GroupName, "Ingress", nn.Namespace, nn.Name)
152151
)
153-
if err := r.List(context.Background(), &list, client.MatchingFields{indexer.PolicyTargetRefs: key}); err != nil {
152+
if err := r.List(ctx, &list, client.MatchingFields{indexer.PolicyTargetRefs: key}); err != nil {
154153
return err
155154
}
156155
var (
@@ -164,7 +163,7 @@ func (r *IngressReconciler) updateHTTPRoutePolicyStatusOnDeleting(nn types.Names
164163
parentRef, ok := ingress2ParentRef[namespacedName]
165164
if !ok {
166165
var ingress networkingv1.Ingress
167-
if err := r.Get(context.Background(), namespacedName, &ingress); err != nil {
166+
if err := r.Get(ctx, namespacedName, &ingress); err != nil {
168167
continue
169168
}
170169
ingressClass, err := r.getIngressClass(&ingress)
@@ -173,33 +172,29 @@ func (r *IngressReconciler) updateHTTPRoutePolicyStatusOnDeleting(nn types.Names
173172
}
174173
parentRef = gatewayv1.ParentReference{
175174
Group: ptr.To(gatewayv1.Group(ingressClass.GroupVersionKind().Group)),
176-
Kind: ptr.To(gatewayv1.Kind("IngressClass")),
175+
Kind: ptr.To(gatewayv1.Kind(KindIngressClass)),
177176
Name: gatewayv1.ObjectName(ingressClass.Name),
178177
}
179178
ingress2ParentRef[namespacedName] = parentRef
180179
}
181180
parentRefs = append(parentRefs, parentRef)
182181
}
183182
// delete AncestorRef which is not exist in the all parentRefs
184-
updateDeleteAncestors(r.Client, r.Log, policy, parentRefs)
183+
updateDeleteAncestors(ctx, r.Client, r.Log, policy, parentRefs)
185184
}
186185

187186
return nil
188187
}
189188

190-
func modifyHTTPRoutePolicyStatus(parentRefs []gatewayv1.ParentReference, policy *v1alpha1.HTTPRoutePolicy, status bool, reason, message string) {
191-
condition := metav1.Condition{
192-
Type: string(v1alpha2.PolicyConditionAccepted),
193-
Status: metav1.ConditionTrue,
189+
func setAncestorsForHTTPRoutePolicyStatus(parentRefs []gatewayv1.ParentReference, policy *v1alpha1.HTTPRoutePolicy, condition metav1.Condition) bool {
190+
return SetAncestors(&policy.Status, parentRefs, metav1.Condition{
191+
Type: cmp.Or(condition.Type, string(v1alpha2.PolicyConditionAccepted)),
192+
Status: cmp.Or(condition.Status, metav1.ConditionTrue),
194193
ObservedGeneration: policy.GetGeneration(),
195194
LastTransitionTime: metav1.Now(),
196-
Reason: reason,
197-
Message: message,
198-
}
199-
if !status {
200-
condition.Status = metav1.ConditionFalse
201-
}
202-
_ = SetAncestors(&policy.Status, parentRefs, condition)
195+
Reason: cmp.Or(condition.Reason, string(v1alpha2.PolicyReasonAccepted)),
196+
Message: condition.Message,
197+
})
203198
}
204199

205200
// checkPoliciesConflict determines if there is a conflict among the given HTTPRoutePolicy objects based on their priority values.
@@ -234,15 +229,15 @@ func findPoliciesWhichTargetRefTheRule(ruleName *gatewayv1.SectionName, kind str
234229
}
235230

236231
// updateDeleteAncestors removes ancestor references from HTTPRoutePolicy statuses that are no longer present in the provided parentRefs.
237-
func updateDeleteAncestors(client client.Client, logger logr.Logger, policy v1alpha1.HTTPRoutePolicy, parentRefs []gatewayv1.ParentReference) {
232+
func updateDeleteAncestors(ctx context.Context, client client.Client, logger logr.Logger, policy v1alpha1.HTTPRoutePolicy, parentRefs []gatewayv1.ParentReference) {
238233
length := len(policy.Status.Ancestors)
239-
policy.Status.Ancestors = slices.DeleteFunc(policy.Status.Ancestors, func(status v1alpha2.PolicyAncestorStatus) bool {
234+
policy.Status.Ancestors = slices.DeleteFunc(policy.Status.Ancestors, func(ancestor v1alpha2.PolicyAncestorStatus) bool {
240235
return !slices.ContainsFunc(parentRefs, func(ref gatewayv1.ParentReference) bool {
241-
return parentRefValueEqual(status.AncestorRef, ref)
236+
return parentRefValueEqual(ancestor.AncestorRef, ref)
242237
})
243238
})
244239
if length != len(policy.Status.Ancestors) {
245-
if err := client.Status().Update(context.Background(), &policy); err != nil {
240+
if err := client.Status().Update(ctx, &policy); err != nil {
246241
logger.Error(err, "failed to update HTTPRoutePolicy status")
247242
}
248243
}

‎internal/controller/ingress_controller.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
112112
ingress := new(networkingv1.Ingress)
113113
if err := r.Get(ctx, req.NamespacedName, ingress); err != nil {
114114
if client.IgnoreNotFound(err) == nil {
115-
if err := r.updateHTTPRoutePolicyStatusOnDeleting(req.NamespacedName); err != nil {
115+
if err := r.updateHTTPRoutePolicyStatusOnDeleting(ctx, req.NamespacedName); err != nil {
116116
return ctrl.Result{}, err
117117
}
118118

@@ -148,7 +148,7 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
148148

149149
tctx.RouteParentRefs = append(tctx.RouteParentRefs, gatewayv1.ParentReference{
150150
Group: ptr.To(gatewayv1.Group(ingressClass.GroupVersionKind().Group)),
151-
Kind: ptr.To(gatewayv1.Kind("IngressClass")),
151+
Kind: ptr.To(gatewayv1.Kind(KindIngressClass)),
152152
Name: gatewayv1.ObjectName(ingressClass.Name),
153153
})
154154

‎test/e2e/framework/utils.go‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,20 +20,32 @@ import (
2020
"encoding/json"
2121
"fmt"
2222
"io"
23+
"log"
2324
"net/http"
2425
"net/url"
26+
"slices"
2527
"strings"
2628
"sync"
2729
"time"
2830

2931
"github.com/gavv/httpexpect/v2"
32+
"github.com/gruntwork-io/terratest/modules/testing"
3033
"github.com/onsi/gomega"
34+
"github.com/pkg/errors"
35+
"github.com/stretchr/testify/require"
3136
"golang.org/x/net/html"
3237
corev1 "k8s.io/api/core/v1"
3338
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
39+
"k8s.io/apimachinery/pkg/types"
40+
"k8s.io/apimachinery/pkg/util/wait"
3441
"k8s.io/client-go/kubernetes/scheme"
3542
"k8s.io/client-go/tools/remotecommand"
3643
"k8s.io/utils/ptr"
44+
"sigs.k8s.io/controller-runtime/pkg/client"
45+
gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"
46+
"sigs.k8s.io/gateway-api/conformance/utils/kubernetes"
47+
48+
"github.com/apache/apisix-ingress-controller/api/v1alpha1"
3749
)
3850

3951
func (f *Framework) NewExpectResponse(httpBody any) *httpexpect.Response {
@@ -369,6 +381,43 @@ func CreateTestZipFile(sourceCode, metadata string) ([]byte, error) {
369381
return zipBuffer.Bytes(), nil
370382
}
371383

384+
func HTTPRoutePolicyMustHaveCondition(t testing.TestingT, client client.Client, timeout time.Duration, refNN, hrpNN types.NamespacedName,
385+
condition metav1.Condition) {
386+
err := EventuallyHTTPRoutePolicyHaveStatus(client, timeout, hrpNN, func(httpRoutePolicy v1alpha1.HTTPRoutePolicy, status v1alpha1.PolicyStatus) bool {
387+
for _, ancestor := range status.Ancestors {
388+
if err := kubernetes.ConditionsHaveLatestObservedGeneration(&httpRoutePolicy, ancestor.Conditions); err != nil {
389+
log.Printf("HTTPRoutePolicy %s (parentRef=%v) %v", hrpNN, parentRefToString(ancestor.AncestorRef), err)
390+
return false
391+
}
392+
393+
if ancestor.AncestorRef.Name == gatewayv1.ObjectName(refNN.Name) &&
394+
(ancestor.AncestorRef.Namespace == nil || refNN.Namespace == "" || string(*ancestor.AncestorRef.Namespace) == refNN.Namespace) {
395+
if findConditionInList(ancestor.Conditions, condition) {
396+
log.Printf("found condition %v in list [%v] for %s reference %s", condition, ancestor.Conditions, hrpNN, refNN)
397+
return true
398+
} else {
399+
log.Printf("not found condition %v in list [%v] for %s reference %s", condition, ancestor.Conditions, hrpNN, refNN)
400+
}
401+
}
402+
}
403+
return false
404+
})
405+
406+
require.NoError(t, err, "error waiting for HTTPRoutePolicy status to have a Condition matching expectations")
407+
}
408+
409+
func EventuallyHTTPRoutePolicyHaveStatus(client client.Client, timeout time.Duration, hrpNN types.NamespacedName,
410+
f func(httpRoutePolicy v1alpha1.HTTPRoutePolicy, status v1alpha1.PolicyStatus) bool) error {
411+
_ = v1alpha1.AddToScheme(client.Scheme())
412+
return wait.PollUntilContextTimeout(context.Background(), time.Second, timeout, true, func(ctx context.Context) (done bool, err error) {
413+
var httpRoutePolicy v1alpha1.HTTPRoutePolicy
414+
if err = client.Get(ctx, hrpNN, &httpRoutePolicy); err != nil {
415+
return false, errors.Errorf("error fetching HTTPRoutePolicy %v: %v", hrpNN, err)
416+
}
417+
return f(httpRoutePolicy, httpRoutePolicy.Status), nil
418+
})
419+
}
420+
372421
func addFileToZip(zipWriter *zip.Writer, fileName, fileContent string) error {
373422
file, err := zipWriter.Create(fileName)
374423
if err != nil {
@@ -378,3 +427,18 @@ func addFileToZip(zipWriter *zip.Writer, fileName, fileContent string) error {
378427
_, err = file.Write([]byte(fileContent))
379428
return err
380429
}
430+
431+
func parentRefToString(p gatewayv1.ParentReference) string {
432+
if p.Namespace != nil && *p.Namespace != "" {
433+
return fmt.Sprintf("%v/%v", p.Namespace, p.Name)
434+
}
435+
return string(p.Name)
436+
}
437+
438+
func findConditionInList(conditions []metav1.Condition, expected metav1.Condition) bool {
439+
return slices.ContainsFunc(conditions, func(item metav1.Condition) bool {
440+
// an empty Status string means "Match any status".
441+
// an empty Reason string means "Match any reason".
442+
return expected.Type == item.Type && (expected.Status == "" || expected.Status == item.Status) && (expected.Reason == "" || expected.Reason == item.Reason)
443+
})
444+
}

0 commit comments

Comments
 (0)