From b8641e170434f38dfac7e84fcdfb699eb78feee2 Mon Sep 17 00:00:00 2001 From: "yasin.lachiny" Date: Sun, 8 Jan 2023 23:01:55 +0100 Subject: [PATCH 1/4] fix: check default ingclass when ingclass is nill --- d | 0 pkg/ingress/class_loader.go | 36 +++++++++++++++++++++++++++++++++++- pkg/ingress/group_loader.go | 21 +++++++++------------ 3 files changed, 44 insertions(+), 13 deletions(-) create mode 100644 d diff --git a/d b/d new file mode 100644 index 000000000..e69de29bb diff --git a/pkg/ingress/class_loader.go b/pkg/ingress/class_loader.go index 7006a08ff..7c20b898e 100644 --- a/pkg/ingress/class_loader.go +++ b/pkg/ingress/class_loader.go @@ -3,6 +3,7 @@ package ingress import ( "context" "fmt" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" networking "k8s.io/api/networking/v1" @@ -20,6 +21,8 @@ const ( ingressClassControllerALB = "ingress.k8s.aws/alb" // the Kind for IngressClassParams CRD. ingressClassParamsKind = "IngressClassParams" + // default class from ingressClass + defaultClassAnnotation = "ingressclass.kubernetes.io/is-default-class" ) // ErrInvalidIngressClass is an sentinel error that represents the IngressClass configuration for Ingress is invalid. @@ -43,9 +46,40 @@ type defaultClassLoader struct { client client.Client } +// GetDefaultIngressClass returns the default IngressClass from the list of IngressClasses. +// If multiple IngressClasses are marked as the default, it returns an error. +func (l *defaultClassLoader) GetDefaultIngressClass(ctx context.Context) (string, error) { + var defaultClass string + var defaultClassFound bool + ingClassList := &networking.IngressClassList{} + if err := l.client.List(ctx, ingClassList); err != nil { + return "", fmt.Errorf("failed to fetch ingressClasses, %v", err.Error()) + } + for _, ingressClass := range ingClassList.Items { + if ingressClass.Annotations[defaultClassAnnotation] == "true" { + if defaultClassFound { + return "", errors.Errorf("multiple default IngressClasses found") + } + defaultClass = ingressClass.GetName() + defaultClassFound = true + } + } + + return defaultClass, nil +} + func (l *defaultClassLoader) Load(ctx context.Context, ing *networking.Ingress) (ClassConfiguration, error) { + if ing.Spec.IngressClassName == nil { - return ClassConfiguration{}, fmt.Errorf("%w: %v", ErrInvalidIngressClass, "spec.ingressClassName is nil") + defaultClass, err := l.GetDefaultIngressClass(ctx) + if err != nil { + return ClassConfiguration{}, err + } + if defaultClass != "" { + ing.Spec.IngressClassName = &defaultClass + } else { + return ClassConfiguration{}, err + } } ingClassKey := types.NamespacedName{Name: *ing.Spec.IngressClassName} diff --git a/pkg/ingress/group_loader.go b/pkg/ingress/group_loader.go index 8ca77bb70..7433799fd 100644 --- a/pkg/ingress/group_loader.go +++ b/pkg/ingress/group_loader.go @@ -81,7 +81,6 @@ func (m *defaultGroupLoader) Load(ctx context.Context, groupID GroupID) (Group, if err := m.client.List(ctx, ingList); err != nil { return Group{}, err } - var members []ClassifiedIngress var inactiveMembers []*networking.Ingress for index := range ingList.Items { @@ -207,17 +206,15 @@ func (m *defaultGroupLoader) classifyIngress(ctx context.Context, ing *networkin IngClassConfig: ClassConfiguration{}, }, false, nil } - - if ing.Spec.IngressClassName != nil { - ingClassConfig, err := m.classLoader.Load(ctx, ing) - if err != nil { - return ClassifiedIngress{ - Ing: ing, - IngClassConfig: ClassConfiguration{}, - }, false, err - } - - if matchesIngressClass := ingClassConfig.IngClass != nil && ingClassConfig.IngClass.Spec.Controller == ingressClassControllerALB; matchesIngressClass { + ingClassConfig, err := m.classLoader.Load(ctx, ing) + if err != nil { + return ClassifiedIngress{ + Ing: ing, + IngClassConfig: ClassConfiguration{}, + }, false, err + } + if matchesIngressClass := ingClassConfig.IngClass != nil; matchesIngressClass { + if ingClassConfig.IngClass.Spec.Controller == ingressClassControllerALB { return ClassifiedIngress{ Ing: ing, IngClassConfig: ingClassConfig, From f0b82e3fdfe9bf1230404a905c25a0ed3dc3e5a8 Mon Sep 17 00:00:00 2001 From: "yasin.lachiny" Date: Mon, 9 Jan 2023 18:27:47 +0100 Subject: [PATCH 2/4] fix: change the error when the defaul class and ingClassName are nill --- d | 0 pkg/ingress/class_loader.go | 2 +- pkg/ingress/class_loader_test.go | 5 +++-- pkg/ingress/group_loader.go | 14 +++++--------- 4 files changed, 9 insertions(+), 12 deletions(-) delete mode 100644 d diff --git a/d b/d deleted file mode 100644 index e69de29bb..000000000 diff --git a/pkg/ingress/class_loader.go b/pkg/ingress/class_loader.go index 7c20b898e..418e82e82 100644 --- a/pkg/ingress/class_loader.go +++ b/pkg/ingress/class_loader.go @@ -78,7 +78,7 @@ func (l *defaultClassLoader) Load(ctx context.Context, ing *networking.Ingress) if defaultClass != "" { ing.Spec.IngressClassName = &defaultClass } else { - return ClassConfiguration{}, err + return ClassConfiguration{}, nil } } diff --git a/pkg/ingress/class_loader_test.go b/pkg/ingress/class_loader_test.go index 9be287be2..97db7eac9 100644 --- a/pkg/ingress/class_loader_test.go +++ b/pkg/ingress/class_loader_test.go @@ -2,6 +2,8 @@ package ingress import ( "context" + "testing" + "github.com/aws/aws-sdk-go/aws" "github.com/golang/mock/gomock" "github.com/google/go-cmp/cmp" @@ -18,7 +20,6 @@ import ( "sigs.k8s.io/aws-load-balancer-controller/pkg/webhook" testclient "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "testing" ) func Test_defaultClassLoader_Load(t *testing.T) { @@ -59,7 +60,7 @@ func Test_defaultClassLoader_Load(t *testing.T) { }, }, }, - wantErr: errors.New("invalid ingress class: spec.ingressClassName is nil"), + wantErr: nil, }, { name: "when IngressClass not found", diff --git a/pkg/ingress/group_loader.go b/pkg/ingress/group_loader.go index 7433799fd..a6f70506a 100644 --- a/pkg/ingress/group_loader.go +++ b/pkg/ingress/group_loader.go @@ -206,6 +206,7 @@ func (m *defaultGroupLoader) classifyIngress(ctx context.Context, ing *networkin IngClassConfig: ClassConfiguration{}, }, false, nil } + ingClassConfig, err := m.classLoader.Load(ctx, ing) if err != nil { return ClassifiedIngress{ @@ -213,22 +214,17 @@ func (m *defaultGroupLoader) classifyIngress(ctx context.Context, ing *networkin IngClassConfig: ClassConfiguration{}, }, false, err } - if matchesIngressClass := ingClassConfig.IngClass != nil; matchesIngressClass { - if ingClassConfig.IngClass.Spec.Controller == ingressClassControllerALB { - return ClassifiedIngress{ - Ing: ing, - IngClassConfig: ingClassConfig, - }, true, nil - } + + if matchesIngressClass := ingClassConfig.IngClass != nil && ingClassConfig.IngClass.Spec.Controller == ingressClassControllerALB; matchesIngressClass { return ClassifiedIngress{ Ing: ing, IngClassConfig: ingClassConfig, - }, false, nil + }, true, nil } return ClassifiedIngress{ Ing: ing, - IngClassConfig: ClassConfiguration{}, + IngClassConfig: ingClassConfig, }, m.manageIngressesWithoutIngressClass, nil } From cd7bf2c8a05fb83a17cc89898823935c837ca2a7 Mon Sep 17 00:00:00 2001 From: "yasin.lachiny" Date: Tue, 10 Jan 2023 22:54:43 +0100 Subject: [PATCH 3/4] test: multiple default class specified --- pkg/ingress/class_loader.go | 3 +- pkg/ingress/class_loader_test.go | 137 ++++++++++++++++++++++++++++++- 2 files changed, 138 insertions(+), 2 deletions(-) diff --git a/pkg/ingress/class_loader.go b/pkg/ingress/class_loader.go index 418e82e82..6402e0a91 100644 --- a/pkg/ingress/class_loader.go +++ b/pkg/ingress/class_loader.go @@ -48,12 +48,13 @@ type defaultClassLoader struct { // GetDefaultIngressClass returns the default IngressClass from the list of IngressClasses. // If multiple IngressClasses are marked as the default, it returns an error. +// If no IngressClass is marked as the default, it returns an empty string. func (l *defaultClassLoader) GetDefaultIngressClass(ctx context.Context) (string, error) { var defaultClass string var defaultClassFound bool ingClassList := &networking.IngressClassList{} if err := l.client.List(ctx, ingClassList); err != nil { - return "", fmt.Errorf("failed to fetch ingressClasses, %v", err.Error()) + return "", fmt.Errorf("%w: fetching ingressClasses: %v", ErrInvalidIngressClass, err.Error()) } for _, ingressClass := range ingClassList.Items { if ingressClass.Annotations[defaultClassAnnotation] == "true" { diff --git a/pkg/ingress/class_loader_test.go b/pkg/ingress/class_loader_test.go index 97db7eac9..6d1e82db9 100644 --- a/pkg/ingress/class_loader_test.go +++ b/pkg/ingress/class_loader_test.go @@ -39,7 +39,7 @@ func Test_defaultClassLoader_Load(t *testing.T) { wantErr error }{ { - name: "when IngressClassName unspecified", + name: "when IngressClassName unspecified - Default class specified", env: env{ nsList: []*corev1.Namespace{ { @@ -48,6 +48,19 @@ func Test_defaultClassLoader_Load(t *testing.T) { }, }, }, + ingClassList: []*networking.IngressClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "awesome-ingress-class", + Annotations: map[string]string{ + "ingressclass.kubernetes.io/is-default-class": "true", + }, + }, + Spec: networking.IngressClassSpec{ + Controller: "awesome-ingress-controller", + }, + }, + }, }, args: args{ ing: &networking.Ingress{ @@ -60,6 +73,128 @@ func Test_defaultClassLoader_Load(t *testing.T) { }, }, }, + want: ClassConfiguration{ + IngClass: &networking.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "awesome-ingress-class", + Annotations: map[string]string{ + "ingressclass.kubernetes.io/is-default-class": "true", + }, + }, + Spec: networking.IngressClassSpec{ + Controller: "awesome-ingress-controller", + }, + }, + }, + wantErr: nil, + }, + { + name: "when IngressClassName unspecified - Multiple default classes specified", + env: env{ + nsList: []*corev1.Namespace{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "awesome-ns", + }, + }, + }, + ingClassList: []*networking.IngressClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "awesome-ingress-class-1", + Annotations: map[string]string{ + "ingressclass.kubernetes.io/is-default-class": "true", + }, + }, + Spec: networking.IngressClassSpec{ + Controller: "awesome-ingress-controller-1", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "awesome-ingress-class-2", + Annotations: map[string]string{ + "ingressclass.kubernetes.io/is-default-class": "true", + }, + }, + Spec: networking.IngressClassSpec{ + Controller: "awesome-ingress-controller-2", + }, + }, + }, + }, + args: args{ + ing: &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "awesome-ns", + Name: "awesome-ing", + }, + Spec: networking.IngressSpec{ + IngressClassName: nil, + }, + }, + }, + wantErr: errors.New("multiple default IngressClasses found"), + }, + { + name: "when IngressClass is ALB - Multiple default classes specified", + env: env{ + nsList: []*corev1.Namespace{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "awesome-ns", + }, + }, + }, + ingClassList: []*networking.IngressClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "awesome-ingress-class-1", + Annotations: map[string]string{ + "ingressclass.kubernetes.io/is-default-class": "true", + }, + }, + Spec: networking.IngressClassSpec{ + Controller: "ingress.k8s.aws/alb", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "awesome-ingress-class-2", + Annotations: map[string]string{ + "ingressclass.kubernetes.io/is-default-class": "true", + }, + }, + Spec: networking.IngressClassSpec{ + Controller: "awesome-ingress-controller-2", + }, + }, + }, + }, + args: args{ + ing: &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "awesome-ns", + Name: "awesome-ing", + }, + Spec: networking.IngressSpec{ + IngressClassName: aws.String("awesome-ingress-class-1"), + }, + }, + }, + want: ClassConfiguration{ + IngClass: &networking.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "awesome-ingress-class-1", + Annotations: map[string]string{ + "ingressclass.kubernetes.io/is-default-class": "true", + }, + }, + Spec: networking.IngressClassSpec{ + Controller: "ingress.k8s.aws/alb", + }, + }, + }, wantErr: nil, }, { From eb9f77309edd1178309328638833fc9811b31dee Mon Sep 17 00:00:00 2001 From: "yasin.lachiny" Date: Tue, 10 Jan 2023 23:11:58 +0100 Subject: [PATCH 4/4] test: when IngressClassName unspecified - Default class unspecified --- pkg/ingress/class_loader_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/pkg/ingress/class_loader_test.go b/pkg/ingress/class_loader_test.go index 6d1e82db9..acac23267 100644 --- a/pkg/ingress/class_loader_test.go +++ b/pkg/ingress/class_loader_test.go @@ -88,6 +88,30 @@ func Test_defaultClassLoader_Load(t *testing.T) { }, wantErr: nil, }, + { + name: "when IngressClassName unspecified - Default class unspecified", + env: env{ + nsList: []*corev1.Namespace{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "awesome-ns", + }, + }, + }, + }, + args: args{ + ing: &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "awesome-ns", + Name: "awesome-ing", + }, + Spec: networking.IngressSpec{ + IngressClassName: nil, + }, + }, + }, + want: ClassConfiguration{}, + }, { name: "when IngressClassName unspecified - Multiple default classes specified", env: env{