Allow negative and duplicate group.orders (kubernetes-sigs#2634)

This commit relaxes the restrictions around the `group.order`
annotation, allowing both negative and duplicate orders. Duplicate
orders are sorted lexicographically.

Author: jtdoepke

docs/guide/ingress/annotations.md

@@ -88,12 +88,9 @@ By default, Ingresses don't belong to any IngressGroup, and we treat it as a "im
 - <a name="group.order">`alb.ingress.kubernetes.io/group.order`</a> specifies the order across all Ingresses within IngressGroup.
 
     !!!note ""
-        - You can explicitly denote the order using a number between 1-1000
+        - You can explicitly denote the order using a number between -1000 and 1000
         - The smaller the order, the rule will be evaluated first. All Ingresses without an explicit order setting get order value as 0
-        - By default the rule order between Ingresses within IngressGroup is determined by the lexical order of Ingress’s namespace/name.
-
-    !!!warning "" 
-        You may not have duplicate group order explicitly defined for Ingresses within IngressGroup.
+        - Rules with the same order are sorted lexicographically by the Ingress’s namespace/name.
 
     !!!example
         ```

pkg/ingress/group_loader.go

@@ -3,21 +3,21 @@ package ingress
 import (
 	"context"
 	"fmt"
+	"regexp"
+	"sort"
+	"strings"
+
 	"github.com/pkg/errors"
 	networking "k8s.io/api/networking/v1"
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/record"
-	"regexp"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/annotations"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/k8s"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sort"
-	"strings"
 )
 
 const (
 	defaultGroupOrder  int64 = 0
-	minGroupOrder      int64 = 1
+	minGroupOrder      int64 = -1000
 	maxGroupOder       int64 = 1000
 	maxGroupNameLength int   = 63
 )
@@ -251,7 +251,7 @@ type groupMemberWithOrder struct {
 
 // sortGroupMembers will sort Ingresses within Ingress group in ascending order.
 // the order for an ingress can be set as below:
-// * explicit denote the order via "group.order" annotation.(It's an error if two Ingress have same explicit order)
+// * explicit denote the order via "group.order" annotation.
 // * implicit denote the order of ${defaultGroupOrder}.
 // If two Ingress are of same order, they are sorted by lexical order of their full-qualified name.
 func (m *defaultGroupLoader) sortGroupMembers(members []ClassifiedIngress) ([]ClassifiedIngress, error) {
@@ -260,7 +260,6 @@ func (m *defaultGroupLoader) sortGroupMembers(members []ClassifiedIngress) ([]Cl
 	}
 
 	groupMemberWithOrderList := make([]groupMemberWithOrder, 0, len(members))
-	explicitOrders := sets.NewInt64()
 	for _, member := range members {
 		var order = defaultGroupOrder
 		exists, err := m.annotationParser.ParseInt64Annotation(annotations.IngressSuffixGroupOrder, &order, member.Ing.Annotations)
@@ -272,10 +271,6 @@ func (m *defaultGroupLoader) sortGroupMembers(members []ClassifiedIngress) ([]Cl
 				return nil, errors.Errorf("explicit Ingress group order must be within [%v:%v], Ingress: %v, order: %v",
 					minGroupOrder, maxGroupOder, k8s.NamespacedName(member.Ing), order)
 			}
-			if explicitOrders.Has(order) {
-				return nil, errors.Errorf("conflict Ingress group order: %v", order)
-			}
-			explicitOrders.Insert(order)
 		}
 
 		groupMemberWithOrderList = append(groupMemberWithOrderList, groupMemberWithOrder{member: member, order: order})

pkg/ingress/group_loader_test.go

@@ -2,6 +2,9 @@ package ingress
 
 import (
 	"context"
+	"testing"
+	"time"
+
 	awssdk "github.com/aws/aws-sdk-go/aws"
 	"github.com/golang/mock/gomock"
 	"github.com/google/go-cmp/cmp"
@@ -16,8 +19,6 @@ import (
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/annotations"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/equality"
 	testclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
-	"testing"
-	"time"
 )
 
 func Test_defaultGroupLoader_Load(t *testing.T) {
@@ -2617,27 +2618,80 @@ func Test_defaultGroupLoader_sortGroupMembers(t *testing.T) {
 			wantErr: errors.New("failed to load Ingress group order for ingress: namespace/ingress: failed to parse int64 annotation, alb.ingress.kubernetes.io/group.order: x: strconv.ParseInt: parsing \"x\": invalid syntax"),
 		},
 		{
-			name: "invalid group order range",
+			name: "two ingress with the same order should be sorted lexically",
 			members: []ClassifiedIngress{
 				{
 					Ing: &networking.Ingress{
 						ObjectMeta: metav1.ObjectMeta{
 							Namespace: "namespace",
-							Name:      "ingress",
+							Name:      "ingress-b",
+							Annotations: map[string]string{
+								"kubernetes.io/ingress.class":           "alb",
+								"alb.ingress.kubernetes.io/group.name":  "awesome-group",
+								"alb.ingress.kubernetes.io/group.order": "42",
+							},
+						},
+					},
+				},
+				{
+					Ing: &networking.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: "namespace",
+							Name:      "ingress-a",
 							Annotations: map[string]string{
 								"kubernetes.io/ingress.class":           "alb",
-								"alb.ingress.kubernetes.io/group.order": "1001",
+								"alb.ingress.kubernetes.io/group.name":  "awesome-group",
+								"alb.ingress.kubernetes.io/group.order": "42",
+							},
+						},
+					},
+				},
+			},
+			want: []ClassifiedIngress{
+				{
+					Ing: &networking.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: "namespace",
+							Name:      "ingress-a",
+							Annotations: map[string]string{
+								"kubernetes.io/ingress.class":           "alb",
+								"alb.ingress.kubernetes.io/group.name":  "awesome-group",
+								"alb.ingress.kubernetes.io/group.order": "42",
+							},
+						},
+					},
+				},
+				{
+					Ing: &networking.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: "namespace",
+							Name:      "ingress-b",
+							Annotations: map[string]string{
+								"kubernetes.io/ingress.class":           "alb",
+								"alb.ingress.kubernetes.io/group.name":  "awesome-group",
+								"alb.ingress.kubernetes.io/group.order": "42",
 							},
 						},
 					},
 				},
 			},
-			want:    nil,
-			wantErr: errors.New("explicit Ingress group order must be within [1:1000], Ingress: namespace/ingress, order: 1001"),
 		},
 		{
-			name: "two ingress shouldn't have same explicit order",
+			name: "negative orders are allow",
 			members: []ClassifiedIngress{
+				{
+					Ing: &networking.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: "namespace",
+							Name:      "ingress-b",
+							Annotations: map[string]string{
+								"kubernetes.io/ingress.class":           "alb",
+								"alb.ingress.kubernetes.io/group.name":  "awesome-group",
+								"alb.ingress.kubernetes.io/group.order": "0",
+							},
+						},
+					},
+				},
 				{
 					Ing: &networking.Ingress{
 						ObjectMeta: metav1.ObjectMeta{
@@ -2646,7 +2700,22 @@ func Test_defaultGroupLoader_sortGroupMembers(t *testing.T) {
 							Annotations: map[string]string{
 								"kubernetes.io/ingress.class":           "alb",
 								"alb.ingress.kubernetes.io/group.name":  "awesome-group",
-								"alb.ingress.kubernetes.io/group.order": "42",
+								"alb.ingress.kubernetes.io/group.order": "-1",
+							},
+						},
+					},
+				},
+			},
+			want: []ClassifiedIngress{
+				{
+					Ing: &networking.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: "namespace",
+							Name:      "ingress-a",
+							Annotations: map[string]string{
+								"kubernetes.io/ingress.class":           "alb",
+								"alb.ingress.kubernetes.io/group.name":  "awesome-group",
+								"alb.ingress.kubernetes.io/group.order": "-1",
 							},
 						},
 					},
@@ -2659,14 +2728,12 @@ func Test_defaultGroupLoader_sortGroupMembers(t *testing.T) {
 							Annotations: map[string]string{
 								"kubernetes.io/ingress.class":           "alb",
 								"alb.ingress.kubernetes.io/group.name":  "awesome-group",
-								"alb.ingress.kubernetes.io/group.order": "42",
+								"alb.ingress.kubernetes.io/group.order": "0",
 							},
 						},
 					},
 				},
 			},
-			want:    nil,
-			wantErr: errors.New("conflict Ingress group order: 42"),
 		},
 	}
 	for _, tt := range tests {

test/e2e/ingress/multi_path_backend.go

@@ -32,7 +32,7 @@ type BackendConfig struct {
 
 type MultiPathIngressConfig struct {
 	GroupName  string
-	GroupOrder int32
+	GroupOrder int64
 	PathCFGs   []PathConfig
 }