Skip to content

Allow negative and duplicate group.orders #2634

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 9, 2022
Merged

Allow negative and duplicate group.orders #2634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions docs/guide/ingress/annotations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
```
Expand Down
17 changes: 6 additions & 11 deletions pkg/ingress/group_loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down Expand Up @@ -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) {
Expand All @@ -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)
Expand All @@ -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})
Expand Down
91 changes: 79 additions & 12 deletions pkg/ingress/group_loader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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) {
Expand Down Expand Up @@ -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{
Expand All @@ -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",
},
},
},
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/ingress/multi_path_backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ type BackendConfig struct {

type MultiPathIngressConfig struct {
GroupName string
GroupOrder int32
GroupOrder int64
PathCFGs []PathConfig
}

Expand Down