diff --git a/controllers/ingress/group_controller.go b/controllers/ingress/group_controller.go index 4fd5c0725b..f1483ebb62 100644 --- a/controllers/ingress/group_controller.go +++ b/controllers/ingress/group_controller.go @@ -52,7 +52,7 @@ func NewGroupReconciler(cloud aws.Cloud, k8sClient client.Client, eventRecorder stackDeployer := deploy.NewDefaultStackDeployer(cloud, k8sClient, networkingSGManager, networkingSGReconciler, config, ingressTagPrefix, logger) ingressConfig := config.IngressConfig - groupLoader := ingress.NewDefaultGroupLoader(k8sClient, annotationParser, ingressConfig.IngressClass) + groupLoader := ingress.NewDefaultGroupLoader(k8sClient, eventRecorder, annotationParser, ingressConfig.IngressClass) groupFinalizerManager := ingress.NewDefaultFinalizerManager(finalizerManager) return &groupReconciler{ diff --git a/pkg/config/ingress_config.go b/pkg/config/ingress_config.go index ddd466a8ab..624f8af773 100644 --- a/pkg/config/ingress_config.go +++ b/pkg/config/ingress_config.go @@ -14,6 +14,7 @@ type IngressConfig struct { // Name of the Ingress class this controller satisfies // If empty, all Ingresses without ingress.class annotation, or ingress.class==alb get considered IngressClass string + // Max concurrent reconcile loops for Ingress objects MaxConcurrentReconciles int } diff --git a/pkg/ingress/group_loader.go b/pkg/ingress/group_loader.go index 6cd81a46dd..4893b0b2da 100644 --- a/pkg/ingress/group_loader.go +++ b/pkg/ingress/group_loader.go @@ -3,8 +3,11 @@ package ingress import ( "context" "github.com/pkg/errors" + corev1 "k8s.io/api/core/v1" networking "k8s.io/api/networking/v1beta1" + "k8s.io/apimachinery/pkg/types" "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" @@ -13,11 +16,13 @@ import ( ) const ( - defaultGroupOrder int64 = 0 - minGroupOrder int64 = 1 - maxGroupOder int64 = 1000 - maxGroupNameLength int = 63 - defaultIngressClass = "alb" + defaultGroupOrder int64 = 0 + minGroupOrder int64 = 1 + maxGroupOder int64 = 1000 + maxGroupNameLength int = 63 + ingressClassALB = "alb" + // the controller name used in IngressClass for ALB. + ingressClassControllerALB = "ingress.k8s.aws/alb" ) var ( @@ -36,9 +41,10 @@ type GroupLoader interface { } // NewDefaultGroupLoader constructs new GroupLoader instance. -func NewDefaultGroupLoader(client client.Client, annotationParser annotations.Parser, ingressClass string) *defaultGroupLoader { +func NewDefaultGroupLoader(client client.Client, eventRecorder record.EventRecorder, annotationParser annotations.Parser, ingressClass string) *defaultGroupLoader { return &defaultGroupLoader{ client: client, + eventRecorder: eventRecorder, annotationParser: annotationParser, ingressClass: ingressClass, } @@ -49,13 +55,18 @@ var _ GroupLoader = (*defaultGroupLoader)(nil) // default implementation for GroupLoader type defaultGroupLoader struct { client client.Client + eventRecorder record.EventRecorder annotationParser annotations.Parser ingressClass string } func (m *defaultGroupLoader) FindGroupID(ctx context.Context, ing *networking.Ingress) (*GroupID, error) { - if !m.matchesIngressClass(ing) { + matchesIngressClass, err := m.matchesIngressClass(ctx, ing) + if err != nil { + return nil, err + } + if !matchesIngressClass { return nil, nil } @@ -85,7 +96,7 @@ func (m *defaultGroupLoader) Load(ctx context.Context, groupID GroupID) (Group, ing := &ingList.Items[index] isGroupMember, err := m.isGroupMember(ctx, groupID, ing) if err != nil { - return Group{}, err + return Group{}, errors.Wrapf(err, "ingress: %v", k8s.NamespacedName(ing)) } if isGroupMember { members = append(members, ing) @@ -104,13 +115,52 @@ func (m *defaultGroupLoader) Load(ctx context.Context, groupID GroupID) (Group, }, nil } -// matchesIngressClass tests whether Ingress matches ingress class of this group manager. -func (m *defaultGroupLoader) matchesIngressClass(ing *networking.Ingress) bool { - ingClass := ing.Annotations[annotations.IngressClass] - if m.ingressClass == "" { - return ingClass == "" || ingClass == defaultIngressClass +// matchesIngressClass tests whether provided Ingress are matched by this group loader. +func (m *defaultGroupLoader) matchesIngressClass(ctx context.Context, ing *networking.Ingress) (bool, error) { + var matchesIngressClassResults []bool + if ingClassAnnotation, exists := ing.Annotations[annotations.IngressClass]; exists { + matchesIngressClass := m.matchesIngressClassAnnotation(ctx, ingClassAnnotation) + matchesIngressClassResults = append(matchesIngressClassResults, matchesIngressClass) + } + + if ing.Spec.IngressClassName != nil { + matchesIngressClass, err := m.matchesIngressClassName(ctx, *ing.Spec.IngressClassName) + if err != nil { + return false, err + } + matchesIngressClassResults = append(matchesIngressClassResults, matchesIngressClass) + } + + if len(matchesIngressClassResults) == 2 { + if matchesIngressClassResults[0] != matchesIngressClassResults[1] { + m.eventRecorder.Event(ing, corev1.EventTypeWarning, k8s.IngressEventReasonConflictingIngressClass, "conflicting values for IngressClass by `spec.IngressClass` and `kubernetes.io/ingress.class` annotation") + } + return matchesIngressClassResults[0], nil + } + if len(matchesIngressClassResults) == 1 { + return matchesIngressClassResults[0], nil + } + + return m.ingressClass == "", nil +} + +// matchesIngressClassAnnotation tests whether provided ingClassAnnotation are matched by this group loader. +func (m *defaultGroupLoader) matchesIngressClassAnnotation(_ context.Context, ingClassAnnotation string) bool { + if m.ingressClass == "" && ingClassAnnotation == ingressClassALB { + return true + } + return ingClassAnnotation == m.ingressClass +} + +// matchesIngressClassName tests whether provided ingClassName are matched by this group loader. +func (m *defaultGroupLoader) matchesIngressClassName(ctx context.Context, ingClassName string) (bool, error) { + ingClassKey := types.NamespacedName{Name: ingClassName} + ingClass := &networking.IngressClass{} + if err := m.client.Get(ctx, ingClassKey, ingClass); err != nil { + return false, err } - return ingClass == m.ingressClass + matchesIngressClass := ingClass.Spec.Controller == ingressClassControllerALB + return matchesIngressClass, nil } // isGroupMember checks whether an ingress is member of a Ingress group diff --git a/pkg/ingress/group_loader_test.go b/pkg/ingress/group_loader_test.go index d147081270..394de9aa85 100644 --- a/pkg/ingress/group_loader_test.go +++ b/pkg/ingress/group_loader_test.go @@ -2,13 +2,18 @@ package ingress import ( "context" + awssdk "github.com/aws/aws-sdk-go/aws" "github.com/golang/mock/gomock" "github.com/pkg/errors" "github.com/stretchr/testify/assert" networking "k8s.io/api/networking/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/tools/record" mock_client "sigs.k8s.io/aws-load-balancer-controller/mocks/controller-runtime/client" "sigs.k8s.io/aws-load-balancer-controller/pkg/annotations" + testclient "sigs.k8s.io/controller-runtime/pkg/client/fake" "testing" ) @@ -594,15 +599,25 @@ func Test_defaultGroupLoader_Load(t *testing.T) { } func Test_defaultGroupLoader_matchesIngressClass(t *testing.T) { - tests := []struct { - name string + type env struct { + ingClasses []*networking.IngressClass + } + type fields struct { ingressClass string - ing *networking.Ingress - want bool + } + tests := []struct { + name string + env env + fields fields + ing *networking.Ingress + want bool + wantErr error }{ { - name: "desire empty ingress class and no ingress class specified", - ingressClass: "", + name: "desire empty ingress class and no ingress class specified", + fields: fields{ + ingressClass: "", + }, ing: &networking.Ingress{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{}, @@ -611,8 +626,10 @@ func Test_defaultGroupLoader_matchesIngressClass(t *testing.T) { want: true, }, { - name: "desire empty ingress class and alb ingress class specified", - ingressClass: "", + name: "desire empty ingress class and alb ingress class specified", + fields: fields{ + ingressClass: "", + }, ing: &networking.Ingress{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -623,8 +640,10 @@ func Test_defaultGroupLoader_matchesIngressClass(t *testing.T) { want: true, }, { - name: "desire empty ingress class but ingress class specified", - ingressClass: "", + name: "desire empty ingress class but another ingress class specified", + fields: fields{ + ingressClass: "", + }, ing: &networking.Ingress{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -635,8 +654,10 @@ func Test_defaultGroupLoader_matchesIngressClass(t *testing.T) { want: false, }, { - name: "desire alb ingress class and alb ingress class specified", - ingressClass: "alb", + name: "desire alb ingress class and alb ingress class specified", + fields: fields{ + ingressClass: "alb", + }, ing: &networking.Ingress{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -647,8 +668,10 @@ func Test_defaultGroupLoader_matchesIngressClass(t *testing.T) { want: true, }, { - name: "desire alb ingress class but no ingress class specified", - ingressClass: "alb", + name: "desire alb ingress class but no ingress class specified", + fields: fields{ + ingressClass: "alb", + }, ing: &networking.Ingress{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{}, @@ -657,8 +680,10 @@ func Test_defaultGroupLoader_matchesIngressClass(t *testing.T) { want: false, }, { - name: "desire alb ingress class but another ingress class specified", - ingressClass: "alb", + name: "desire alb ingress class but another ingress class specified", + fields: fields{ + ingressClass: "alb", + }, ing: &networking.Ingress{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -668,19 +693,338 @@ func Test_defaultGroupLoader_matchesIngressClass(t *testing.T) { }, want: false, }, + { + name: "ingressClass exists and matches alb controller", + env: env{ + ingClasses: []*networking.IngressClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "my-ing-class", + }, + Spec: networking.IngressClassSpec{ + Controller: "ingress.k8s.aws/alb", + }, + }, + }, + }, + fields: fields{ + ingressClass: "alb", + }, + ing: &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + Spec: networking.IngressSpec{ + IngressClassName: awssdk.String("my-ing-class"), + }, + }, + want: true, + }, + { + name: "ingressClass exists and matches another controller", + env: env{ + ingClasses: []*networking.IngressClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "my-ing-class", + }, + Spec: networking.IngressClassSpec{ + Controller: "kubernetes.io/nginx", + }, + }, + }, + }, + fields: fields{ + ingressClass: "alb", + }, + ing: &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + Spec: networking.IngressSpec{ + IngressClassName: awssdk.String("my-ing-class"), + }, + }, + want: false, + }, + { + name: "matches alb ingress class via both annotation and spec.ingressClassName", + env: env{ + ingClasses: []*networking.IngressClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "my-ing-class", + }, + Spec: networking.IngressClassSpec{ + Controller: "ingress.k8s.aws/alb", + }, + }, + }, + }, + fields: fields{ + ingressClass: "alb", + }, + ing: &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "alb", + }, + }, + Spec: networking.IngressSpec{ + IngressClassName: awssdk.String("my-ing-class"), + }, + }, + want: true, + }, + { + name: "matches alb ingress class via annotation but not via spec.ingressClassName", + env: env{ + ingClasses: []*networking.IngressClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "my-ing-class", + }, + Spec: networking.IngressClassSpec{ + Controller: "kubernetes.io/nginx", + }, + }, + }, + }, + fields: fields{ + ingressClass: "alb", + }, + ing: &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "alb", + }, + }, + Spec: networking.IngressSpec{ + IngressClassName: awssdk.String("my-ing-class"), + }, + }, + want: true, + }, + { + name: "matches alb ingress class via spec.ingressClassName but not via annotation", + env: env{ + ingClasses: []*networking.IngressClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "my-ing-class", + }, + Spec: networking.IngressClassSpec{ + Controller: "ingress.k8s.aws/alb", + }, + }, + }, + }, + fields: fields{ + ingressClass: "alb", + }, + ing: &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "nginx", + }, + }, + Spec: networking.IngressSpec{ + IngressClassName: awssdk.String("my-ing-class"), + }, + }, + want: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + k8sSchema := runtime.NewScheme() + clientgoscheme.AddToScheme(k8sSchema) + k8sClient := testclient.NewFakeClientWithScheme(k8sSchema) + for _, ingClass := range tt.env.ingClasses { + assert.NoError(t, k8sClient.Create(ctx, ingClass.DeepCopy())) + } + m := &defaultGroupLoader{ - ingressClass: tt.ingressClass, + client: k8sClient, + eventRecorder: record.NewFakeRecorder(10), + ingressClass: tt.fields.ingressClass, } - got := m.matchesIngressClass(tt.ing) + got, err := m.matchesIngressClass(ctx, tt.ing) + if tt.wantErr != nil { + assert.EqualError(t, err, tt.wantErr.Error()) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.want, got) + } + }) + } +} + +func Test_defaultGroupLoader_matchesIngressClassAnnotation(t *testing.T) { + type fields struct { + ingressClass string + } + type args struct { + ingClassAnnotation string + } + tests := []struct { + name string + fields fields + args args + want bool + }{ + { + name: "specified non-empty ingressClass and matches", + fields: fields{ + ingressClass: "alb", + }, + args: args{ + ingClassAnnotation: "alb", + }, + want: true, + }, + { + name: "specified non-empty ingressClass and mismatches", + fields: fields{ + ingressClass: "alb", + }, + args: args{ + ingClassAnnotation: "nginx", + }, + want: false, + }, + { + name: "specified empty ingressClass with empty ingressClassAnnotation", + fields: fields{ + ingressClass: "", + }, + args: args{ + ingClassAnnotation: "", + }, + want: true, + }, + { + name: "specified empty ingressClass with alb ingressClassAnnotation", + fields: fields{ + ingressClass: "", + }, + args: args{ + ingClassAnnotation: "alb", + }, + want: true, + }, + { + name: "specified empty ingressClass with non-empty and non-alb ingressClassAnnotation", + fields: fields{ + ingressClass: "", + }, + args: args{ + ingClassAnnotation: "nginx", + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + m := &defaultGroupLoader{ + ingressClass: tt.fields.ingressClass, + } + got := m.matchesIngressClassAnnotation(context.Background(), tt.args.ingClassAnnotation) assert.Equal(t, tt.want, got) }) } } +func Test_defaultGroupLoader_matchesIngressClassName(t *testing.T) { + type env struct { + ingClasses []*networking.IngressClass + } + type args struct { + ingClassName string + } + tests := []struct { + name string + env env + args args + want bool + wantErr error + }{ + { + name: "ingressClass exists and matches controller", + env: env{ + ingClasses: []*networking.IngressClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "my-ing-class", + }, + Spec: networking.IngressClassSpec{ + Controller: "ingress.k8s.aws/alb", + }, + }, + }, + }, + args: args{ + ingClassName: "my-ing-class", + }, + want: true, + }, + { + name: "ingressClass exists and mismatches controller", + env: env{ + ingClasses: []*networking.IngressClass{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "my-ing-class", + }, + Spec: networking.IngressClassSpec{ + Controller: "kubernetes.io/nginx", + }, + }, + }, + }, + args: args{ + ingClassName: "my-ing-class", + }, + want: false, + }, + { + name: "ingressClass doesn't exists", + env: env{ + ingClasses: nil, + }, + args: args{ + ingClassName: "my-ing-class", + }, + want: false, + wantErr: errors.New("ingressclasses.networking.k8s.io \"my-ing-class\" not found"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + k8sSchema := runtime.NewScheme() + clientgoscheme.AddToScheme(k8sSchema) + k8sClient := testclient.NewFakeClientWithScheme(k8sSchema) + for _, ingClass := range tt.env.ingClasses { + assert.NoError(t, k8sClient.Create(ctx, ingClass.DeepCopy())) + } + + m := &defaultGroupLoader{ + client: k8sClient, + } + got, err := m.matchesIngressClassName(ctx, tt.args.ingClassName) + if tt.wantErr != nil { + assert.EqualError(t, err, tt.wantErr.Error()) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.want, got) + } + }) + } +} + func Test_defaultGroupLoader_isGroupMember(t *testing.T) { now := metav1.Now() tests := []struct { diff --git a/pkg/k8s/events.go b/pkg/k8s/events.go index ad52b5b88b..dd783bb35b 100644 --- a/pkg/k8s/events.go +++ b/pkg/k8s/events.go @@ -2,13 +2,14 @@ package k8s const ( // Ingress events - IngressEventReasonFailedLoadGroupID = "FailedLoadGroupID" - IngressEventReasonFailedAddFinalizer = "FailedAddFinalizer" - IngressEventReasonFailedRemoveFinalizer = "FailedRemoveFinalizer" - IngressEventReasonFailedUpdateStatus = "FailedUpdateStatus" - IngressEventReasonFailedBuildModel = "FailedBuildModel" - IngressEventReasonFailedDeployModel = "FailedDeployModel" - IngressEventReasonSuccessfullyReconciled = "SuccessfullyReconciled" + IngressEventReasonConflictingIngressClass = "ConflictingIngressClass" + IngressEventReasonFailedLoadGroupID = "FailedLoadGroupID" + IngressEventReasonFailedAddFinalizer = "FailedAddFinalizer" + IngressEventReasonFailedRemoveFinalizer = "FailedRemoveFinalizer" + IngressEventReasonFailedUpdateStatus = "FailedUpdateStatus" + IngressEventReasonFailedBuildModel = "FailedBuildModel" + IngressEventReasonFailedDeployModel = "FailedDeployModel" + IngressEventReasonSuccessfullyReconciled = "SuccessfullyReconciled" // Service events ServiceEventReasonFailedAddFinalizer = "FailedAddFinalizer"