Skip to content

Commit

Permalink
Make Ingress validating webhook ignore ingresses not managed by AWS L…
Browse files Browse the repository at this point in the history 
…BC (#3272)

* Don't manage ingress classes with non-matching controller even when managing ingresses without classes

* Make Ingress validating webhook ignore ingresses not managed by AWS LBC
  • Loading branch information
@johngmyers
johngmyers authored Aug 4, 2023
1 parent 9e8f685 commit 5c892f5
Show file tree
Hide file tree
Showing 10 changed files with 355 additions and 29 deletions.
2 changes: 1 addition & 1 deletion controllers/ingress/group_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func NewGroupReconciler(cloud aws.Cloud, k8sClient client.Client, eventRecorder
stackMarshaller := deploy.NewDefaultStackMarshaller()
stackDeployer := deploy.NewDefaultStackDeployer(cloud, k8sClient, networkingSGManager, networkingSGReconciler,
controllerConfig, ingressTagPrefix, logger)
classLoader := ingress.NewDefaultClassLoader(k8sClient)
classLoader := ingress.NewDefaultClassLoader(k8sClient, true)
classAnnotationMatcher := ingress.NewDefaultClassAnnotationMatcher(controllerConfig.IngressConfig.IngressClass)
manageIngressesWithoutIngressClass := controllerConfig.IngressConfig.IngressClass == ""
groupLoader := ingress.NewDefaultGroupLoader(k8sClient, eventRecorder, annotationParser, classLoader, classAnnotationMatcher, manageIngressesWithoutIngressClass)
Expand Down
11 changes: 7 additions & 4 deletions pkg/config/ingress_config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,17 @@ const (

// IngressConfig contains the configurations for the Ingress controller
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
// Name of the Ingress class this controller satisfies.
// If empty, all with a `kubernetes.io/ingress.class`
// annotation of `alb` get considered.
// Also, if empty, all ingresses without either a `kubernetes.io/ingress.class` annotation or
// an IngressClassName get considered.
IngressClass string

// DisableIngressClassAnnotation specifies whether to disable new usage of kubernetes.io/ingress.class annotation.
// DisableIngressClassAnnotation specifies whether to disable new use of the `kubernetes.io/ingress.class` annotation.
DisableIngressClassAnnotation bool

// DisableIngressGroupNameAnnotation specifies whether to disable new usage of alb.ingress.kubernetes.io/group.name annotation.
// DisableIngressGroupNameAnnotation specifies whether to disable new use of the `alb.ingress.kubernetes.io/group.name` annotation.
DisableIngressGroupNameAnnotation bool

// Max concurrent reconcile loops for Ingress objects
Expand Down
2 changes: 1 addition & 1 deletion pkg/ingress/class_annotation_matcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ type ClassAnnotationMatcher interface {
}

// NewDefaultClassAnnotationMatcher constructs new defaultClassAnnotationMatcher.
func NewDefaultClassAnnotationMatcher(ingressClass string) *defaultClassAnnotationMatcher {
func NewDefaultClassAnnotationMatcher(ingressClass string) ClassAnnotationMatcher {
return &defaultClassAnnotationMatcher{
ingressClass: ingressClass,
}
Expand Down
12 changes: 7 additions & 5 deletions pkg/ingress/class_loader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (

const (
// the controller name used in IngressClass for ALB.
ingressClassControllerALB = "ingress.k8s.aws/alb"
IngressClassControllerALB = "ingress.k8s.aws/alb"
// the Kind for IngressClassParams CRD.
ingressClassParamsKind = "IngressClassParams"
// default class from ingressClass
Expand All @@ -35,15 +35,17 @@ type ClassLoader interface {
}

// NewDefaultClassLoader constructs new defaultClassLoader instance.
func NewDefaultClassLoader(client client.Client) *defaultClassLoader {
func NewDefaultClassLoader(client client.Client, loadParams bool) ClassLoader {
return &defaultClassLoader{
client: client,
client: client,
loadParams: loadParams,
}
}

// default implementation for ClassLoader
type defaultClassLoader struct {
client client.Client
client client.Client
loadParams bool
}

// GetDefaultIngressClass returns the default IngressClass from the list of IngressClasses.
Expand Down Expand Up @@ -91,7 +93,7 @@ func (l *defaultClassLoader) Load(ctx context.Context, ing *networking.Ingress)
}
return ClassConfiguration{}, err
}
if ingClass.Spec.Controller != ingressClassControllerALB || ingClass.Spec.Parameters == nil {
if ingClass.Spec.Controller != IngressClassControllerALB || ingClass.Spec.Parameters == nil || !l.loadParams {
return ClassConfiguration{
IngClass: ingClass,
}, nil
Expand Down
6 changes: 4 additions & 2 deletions pkg/ingress/class_loader_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,8 @@ func Test_defaultClassLoader_Load(t *testing.T) {
}

l := &defaultClassLoader{
client: k8sClient,
client: k8sClient,
loadParams: true,
}
got, err := l.Load(ctx, tt.args.ing)
if tt.wantErr != nil {
Expand Down Expand Up @@ -852,7 +853,8 @@ func Test_defaultClassLoader_validateIngressClassParamsNamespaceRestriction(t *t
}

l := &defaultClassLoader{
client: k8sClient,
client: k8sClient,
loadParams: true,
}
if tt.args.admissionReq != nil {
ctx = webhook.ContextWithAdmissionRequest(ctx, *tt.args.admissionReq)
Expand Down
4 changes: 2 additions & 2 deletions pkg/ingress/group_loader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,11 +215,11 @@ func (m *defaultGroupLoader) classifyIngress(ctx context.Context, ing *networkin
}, false, err
}

if matchesIngressClass := ingClassConfig.IngClass != nil && ingClassConfig.IngClass.Spec.Controller == ingressClassControllerALB; matchesIngressClass {
if ingClassConfig.IngClass != nil {
return ClassifiedIngress{
Ing: ing,
IngClassConfig: ingClassConfig,
}, true, nil
}, ingClassConfig.IngClass.Spec.Controller == IngressClassControllerALB, nil
}

return ClassifiedIngress{
Expand Down
62 changes: 58 additions & 4 deletions pkg/ingress/group_loader_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,7 @@ func Test_defaultGroupLoader_Load(t *testing.T) {
}

annotationParser := annotations.NewSuffixAnnotationParser("alb.ingress.kubernetes.io")
classLoader := NewDefaultClassLoader(k8sClient)
classLoader := NewDefaultClassLoader(k8sClient, true)
classAnnotationMatcher := NewDefaultClassAnnotationMatcher("alb")
m := &defaultGroupLoader{
client: k8sClient,
Expand Down Expand Up @@ -1485,7 +1485,7 @@ func Test_defaultGroupLoader_checkGroupMembershipType(t *testing.T) {
}

annotationParser := annotations.NewSuffixAnnotationParser("alb.ingress.kubernetes.io")
classLoader := NewDefaultClassLoader(k8sClient)
classLoader := NewDefaultClassLoader(k8sClient, true)
classAnnotationMatcher := NewDefaultClassAnnotationMatcher("alb")
m := &defaultGroupLoader{
client: k8sClient,
Expand Down Expand Up @@ -1759,7 +1759,7 @@ func Test_defaultGroupLoader_loadGroupIDIfAnyHelper(t *testing.T) {
}

annotationParser := annotations.NewSuffixAnnotationParser("alb.ingress.kubernetes.io")
classLoader := NewDefaultClassLoader(k8sClient)
classLoader := NewDefaultClassLoader(k8sClient, true)
classAnnotationMatcher := NewDefaultClassAnnotationMatcher("alb")
m := &defaultGroupLoader{
client: k8sClient,
Expand Down Expand Up @@ -2107,6 +2107,60 @@ func Test_defaultGroupLoader_classifyIngress(t *testing.T) {
},
wantIngressClassMatches: false,
},
{
name: "class specified via ingressClassName - mismatches - manageIngressesWithoutIngressClass is set",
env: env{
ingClassList: []*networking.IngressClass{
{
ObjectMeta: metav1.ObjectMeta{
Name: "ing-class",
},
Spec: networking.IngressClassSpec{
Controller: "some.other/nginx",
},
},
},
},
fields: fields{
ingressClass: "",
manageIngressesWithoutIngressClass: true,
},
args: args{
ing: &networking.Ingress{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ing-ns",
Name: "ing-name",
Annotations: map[string]string{},
},
Spec: networking.IngressSpec{
IngressClassName: awssdk.String("ing-class"),
},
},
},
wantClassifiedIng: ClassifiedIngress{
Ing: &networking.Ingress{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ing-ns",
Name: "ing-name",
Annotations: map[string]string{},
},
Spec: networking.IngressSpec{
IngressClassName: awssdk.String("ing-class"),
},
},
IngClassConfig: ClassConfiguration{
IngClass: &networking.IngressClass{
ObjectMeta: metav1.ObjectMeta{
Name: "ing-class",
},
Spec: networking.IngressClassSpec{
Controller: "some.other/nginx",
},
},
},
},
wantIngressClassMatches: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -2122,7 +2176,7 @@ func Test_defaultGroupLoader_classifyIngress(t *testing.T) {
}

annotationParser := annotations.NewSuffixAnnotationParser("alb.ingress.kubernetes.io")
classLoader := NewDefaultClassLoader(k8sClient)
classLoader := NewDefaultClassLoader(k8sClient, true)
classAnnotationMatcher := NewDefaultClassAnnotationMatcher(tt.fields.ingressClass)
m := &defaultGroupLoader{
client: k8sClient,
Expand Down
2 changes: 1 addition & 1 deletion pkg/ingress/reference_indexer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func (i *defaultReferenceIndexer) BuildIngressClassRefIndexes(_ context.Context,
}

func (i *defaultReferenceIndexer) BuildIngressClassParamsRefIndexes(_ context.Context, ingClass *networking.IngressClass) []string {
if ingClass.Spec.Controller != ingressClassControllerALB || ingClass.Spec.Parameters == nil {
if ingClass.Spec.Controller != IngressClassControllerALB || ingClass.Spec.Parameters == nil {
return nil
}
if ingClass.Spec.Parameters.APIGroup == nil ||
Expand Down
39 changes: 32 additions & 7 deletions webhooks/networking/ingress_validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,13 @@ const (
// NewIngressValidator returns a validator for Ingress API.
func NewIngressValidator(client client.Client, ingConfig config.IngressConfig, logger logr.Logger) *ingressValidator {
return &ingressValidator{
annotationParser: annotations.NewSuffixAnnotationParser(annotations.AnnotationPrefixIngress),
classAnnotationMatcher: ingress.NewDefaultClassAnnotationMatcher(ingConfig.IngressClass),
classLoader: ingress.NewDefaultClassLoader(client),
disableIngressClassAnnotation: ingConfig.DisableIngressClassAnnotation,
disableIngressGroupAnnotation: ingConfig.DisableIngressGroupNameAnnotation,
logger: logger,
annotationParser: annotations.NewSuffixAnnotationParser(annotations.AnnotationPrefixIngress),
classAnnotationMatcher: ingress.NewDefaultClassAnnotationMatcher(ingConfig.IngressClass),
classLoader: ingress.NewDefaultClassLoader(client, false),
disableIngressClassAnnotation: ingConfig.DisableIngressClassAnnotation,
disableIngressGroupAnnotation: ingConfig.DisableIngressGroupNameAnnotation,
manageIngressesWithoutIngressClass: ingConfig.IngressClass == "",
logger: logger,
}
}

Expand All @@ -42,7 +43,10 @@ type ingressValidator struct {
classLoader ingress.ClassLoader
disableIngressClassAnnotation bool
disableIngressGroupAnnotation bool
logger logr.Logger
// manageIngressesWithoutIngressClass specifies whether ingresses without "kubernetes.io/ingress.class" annotation
// and "spec.ingressClassName" should be managed or not.
manageIngressesWithoutIngressClass bool
logger logr.Logger
}

func (v *ingressValidator) Prototype(req admission.Request) (runtime.Object, error) {
Expand All @@ -51,6 +55,9 @@ func (v *ingressValidator) Prototype(req admission.Request) (runtime.Object, err

func (v *ingressValidator) ValidateCreate(ctx context.Context, obj runtime.Object) error {
ing := obj.(*networking.Ingress)
if skip, err := v.checkIngressClass(ctx, ing); skip || err != nil {
return err
}
if err := v.checkIngressClassAnnotationUsage(ing, nil); err != nil {
return err
}
Expand All @@ -69,6 +76,9 @@ func (v *ingressValidator) ValidateCreate(ctx context.Context, obj runtime.Objec
func (v *ingressValidator) ValidateUpdate(ctx context.Context, obj runtime.Object, oldObj runtime.Object) error {
ing := obj.(*networking.Ingress)
oldIng := oldObj.(*networking.Ingress)
if skip, err := v.checkIngressClass(ctx, ing); skip || err != nil {
return err
}
if err := v.checkIngressClassAnnotationUsage(ing, oldIng); err != nil {
return err
}
Expand All @@ -88,6 +98,21 @@ func (v *ingressValidator) ValidateDelete(ctx context.Context, obj runtime.Objec
return nil
}

// checkIngressClass checks to see if this ingress is handled by this controller.
func (v *ingressValidator) checkIngressClass(ctx context.Context, ing *networking.Ingress) (bool, error) {
if ingClassAnnotation, exists := ing.Annotations[annotations.IngressClass]; exists {
return !v.classAnnotationMatcher.Matches(ingClassAnnotation), nil
}
classConfiguration, err := v.classLoader.Load(ctx, ing)
if err != nil {
return false, err
}
if classConfiguration.IngClass != nil {
return classConfiguration.IngClass.Spec.Controller != ingress.IngressClassControllerALB, nil
}
return !v.manageIngressesWithoutIngressClass, nil
}

// checkIngressClassAnnotationUsage checks the usage of kubernetes.io/ingress.class annotation.
// kubernetes.io/ingress.class annotation cannot be set to the ingress class for this controller once disabled,
// so that we enforce users to use spec.ingressClassName in Ingress and IngressClass resource instead.
Expand Down
Loading

0 comments on commit 5c892f5

Please sign in to comment.