Skip to content
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

fix: check default ingclass when ingclass is nill #2963

Merged
merged 4 commits into from
Feb 4, 2023
Merged

fix: check default ingclass when ingclass is nill #2963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b8641e1
fix: check default ingclass when ingclass is nill
yasinlachiny Jan 8, 2023
f0b82e3
fix: change the error when the defaul class and ingClassName are nill
yasinlachiny Jan 9, 2023
cd7bf2c
test: multiple default class specified
yasinlachiny Jan 10, 2023
eb9f773
test: when IngressClassName unspecified - Default class unspecified
yasinlachiny Jan 10, 2023
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
37 changes: 36 additions & 1 deletion pkg/ingress/class_loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand All @@ -43,9 +46,41 @@ 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document the behavior for when there is no default IngressClass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @johngmyers
Thank you for reviewing my code.

I added the document about it.

// 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("%w: fetching ingressClasses: %v", ErrInvalidIngressClass, 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{}, nil
}
}

ingClassKey := types.NamespacedName{Name: *ing.Spec.IngressClassName}
Expand Down
166 changes: 163 additions & 3 deletions pkg/ingress/class_loader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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) {
Expand All @@ -38,7 +39,57 @@ 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{
{
ObjectMeta: metav1.ObjectMeta{
Name: "awesome-ns",
},
},
},
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{
ObjectMeta: metav1.ObjectMeta{
Namespace: "awesome-ns",
Name: "awesome-ing",
},
Spec: networking.IngressSpec{
IngressClassName: nil,
},
},
},
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 - Default class unspecified",
env: env{
nsList: []*corev1.Namespace{
{
Expand All @@ -59,7 +110,116 @@ func Test_defaultClassLoader_Load(t *testing.T) {
},
},
},
wantErr: errors.New("invalid ingress class: spec.ingressClassName is nil"),
want: ClassConfiguration{},
},
{
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should specify want instead of wantErr.

Need to add a test where there is a default IngressClass. It should assert that the default IngressClass was used.

Need to add a test where there are two IngressClasses annotated as being the default.

Copy link
Contributor Author

@yasinlachiny yasinlachiny Jan 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added multiple test cases

  • when IngressClass is ALB - Multiple default classes specified
  • when IngressClassName unspecified - Multiple default classes specified
  • when IngressClassName unspecified - Default class specified
  • when IngressClassName unspecified - Default class unspecified

},
{
name: "when IngressClass not found",
Expand Down
27 changes: 10 additions & 17 deletions pkg/ingress/group_loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -208,30 +207,24 @@ func (m *defaultGroupLoader) classifyIngress(ctx context.Context, ing *networkin
}, 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
}
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 {
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
}

Expand Down