From c24b8a2f968cab93f8c4042c13d545c3a7b6ab64 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 30 Jun 2020 13:05:23 -0700 Subject: [PATCH 01/47] fix: correct function name typo --- internal/ingress/store/fake_store.go | 2 +- internal/ingress/store/store.go | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/internal/ingress/store/fake_store.go b/internal/ingress/store/fake_store.go index 81928f5d91..f160b09011 100644 --- a/internal/ingress/store/fake_store.go +++ b/internal/ingress/store/fake_store.go @@ -140,7 +140,7 @@ func NewFakeStore( KnativeIngress: knativeIngressStore, }, - isValidIngresClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong"), + isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong"), } return s, nil } diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index fe36c30690..b86698fd99 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -82,7 +82,7 @@ type Store struct { ingressClass string - isValidIngresClass func(objectMeta *metav1.ObjectMeta) bool + isValidIngressClass func(objectMeta *metav1.ObjectMeta) bool } // CacheStores stores cache.Store for all Kinds of k8s objects that @@ -106,9 +106,9 @@ type CacheStores struct { // New creates a new object store to be used in the ingress controller func New(cs CacheStores, ingressClass string) Storer { return Store{ - stores: cs, - ingressClass: ingressClass, - isValidIngresClass: annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass), + stores: cs, + ingressClass: ingressClass, + isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass), } } @@ -144,7 +144,7 @@ func (s Store) ListIngresses() []*networking.Ingress { var ingresses []*networking.Ingress for _, item := range s.stores.Ingress.List() { ing := networkingIngressV1Beta1(item) - if !s.isValidIngresClass(&ing.ObjectMeta) { + if !s.isValidIngressClass(&ing.ObjectMeta) { continue } ingresses = append(ingresses, ing) @@ -160,7 +160,7 @@ func (s Store) ListTCPIngresses() ([]*configurationv1beta1.TCPIngress, error) { err := cache.ListAll(s.stores.TCPIngress, labels.NewSelector(), func(ob interface{}) { ing, ok := ob.(*configurationv1beta1.TCPIngress) - if ok && s.isValidIngresClass(&ing.ObjectMeta) { + if ok && s.isValidIngressClass(&ing.ObjectMeta) { ingresses = append(ingresses, ing) } }) @@ -270,7 +270,7 @@ func (s Store) ListKongConsumers() []*configurationv1.KongConsumer { var consumers []*configurationv1.KongConsumer for _, item := range s.stores.Consumer.List() { c, ok := item.(*configurationv1.KongConsumer) - if ok && s.isValidIngresClass(&c.ObjectMeta) { + if ok && s.isValidIngressClass(&c.ObjectMeta) { consumers = append(consumers, c) } } @@ -284,7 +284,7 @@ func (s Store) ListKongCredentials() []*configurationv1.KongCredential { var credentials []*configurationv1.KongCredential for _, item := range s.stores.Credential.List() { c, ok := item.(*configurationv1.KongCredential) - if ok && s.isValidIngresClass(&c.ObjectMeta) { + if ok && s.isValidIngressClass(&c.ObjectMeta) { credentials = append(credentials, c) } } @@ -307,7 +307,7 @@ func (s Store) ListGlobalKongPlugins() ([]*configurationv1.KongPlugin, error) { labels.NewSelector().Add(*req), func(ob interface{}) { p, ok := ob.(*configurationv1.KongPlugin) - if ok && s.isValidIngresClass(&p.ObjectMeta) { + if ok && s.isValidIngressClass(&p.ObjectMeta) { plugins = append(plugins, p) } }) @@ -331,7 +331,7 @@ func (s Store) ListGlobalKongClusterPlugins() ([]*configurationv1.KongClusterPlu labels.NewSelector().Add(*req), func(ob interface{}) { p, ok := ob.(*configurationv1.KongClusterPlugin) - if ok && s.isValidIngresClass(&p.ObjectMeta) { + if ok && s.isValidIngressClass(&p.ObjectMeta) { plugins = append(plugins, p) } }) From 8f1cd94ff8d402b3a250e2d78813aef2e05c5eef Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 30 Jun 2020 14:51:39 -0700 Subject: [PATCH 02/47] wip: allow classless always --- cli/ingress-controller/main.go | 6 +++--- cli/ingress-controller/main_test.go | 2 +- internal/ingress/annotations/annotations.go | 18 ++++++++++-------- .../ingress/annotations/annotations_test.go | 4 ++-- internal/ingress/controller/event_handler.go | 12 ++++++------ internal/ingress/store/fake_store.go | 2 +- internal/ingress/store/store.go | 18 +++++++++--------- 7 files changed, 32 insertions(+), 30 deletions(-) diff --git a/cli/ingress-controller/main.go b/cli/ingress-controller/main.go index 53d8ec3629..0a8f5a184d 100644 --- a/cli/ingress-controller/main.go +++ b/cli/ingress-controller/main.go @@ -286,8 +286,8 @@ func main() { var synced []cache.InformerSynced updateChannel := channels.NewRingChannel(1024) reh := controller.ResourceEventHandler{ - UpdateCh: updateChannel, - IsValidIngresClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass), + UpdateCh: updateChannel, + IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass, true), } var informers []cache.SharedIndexInformer var cacheStores store.CacheStores @@ -366,7 +366,7 @@ func main() { runtime.HandleError(fmt.Errorf("Timed out waiting for caches to sync")) } - store := store.New(cacheStores, cliConfig.IngressClass) + store := store.New(cacheStores, cliConfig.IngressClass, true) kong, err := controller.NewKongController(&controllerConfig, updateChannel, store) if err != nil { diff --git a/cli/ingress-controller/main_test.go b/cli/ingress-controller/main_test.go index 44b8a43221..dba30d91a4 100644 --- a/cli/ingress-controller/main_test.go +++ b/cli/ingress-controller/main_test.go @@ -76,7 +76,7 @@ func TestHandleSigterm(t *testing.T) { KubeClient: kubeClient, }, channels.NewRingChannel(1024), - store.New(store.CacheStores{}, conf.IngressClass), + store.New(store.CacheStores{}, conf.IngressClass, true), ) exitCh := make(chan int, 1) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 885787eb07..f81a1cc0b0 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -49,7 +49,7 @@ const ( DefaultIngressClass = "kong" ) -func validIngress(ingressAnnotationValue, ingressClass string) bool { +func validIngress(ingressAnnotationValue, ingressClass string, allowClassless bool) bool { // we have 2 valid combinations // 1 - ingress with default class | blank annotation on ingress // 2 - ingress with specific class | same annotation on ingress @@ -58,7 +58,9 @@ func validIngress(ingressAnnotationValue, ingressClass string) bool { // 3 - ingress with default class | fixed annotation on ingress // 4 - ingress with specific class | different annotation on ingress if ingressAnnotationValue == "" && ingressClass == DefaultIngressClass { - return true + if allowClassless { + return true + } } return ingressAnnotationValue == ingressClass } @@ -66,22 +68,22 @@ func validIngress(ingressAnnotationValue, ingressClass string) bool { // IngressClassValidatorFunc returns a function which can validate if an Object // belongs to an the ingressClass or not. func IngressClassValidatorFunc( - ingressClass string) func(obj metav1.Object) bool { + ingressClass string, allowClassless bool) func(obj metav1.Object, allowClassless bool) bool { - return func(obj metav1.Object) bool { + return func(obj metav1.Object, allowClassless bool) bool { ingress := obj.GetAnnotations()[ingressClassKey] - return validIngress(ingress, ingressClass) + return validIngress(ingress, ingressClass, allowClassless) } } // IngressClassValidatorFuncFromObjectMeta returns a function which // can validate if an ObjectMeta belongs to an the ingressClass or not. func IngressClassValidatorFuncFromObjectMeta( - ingressClass string) func(obj *metav1.ObjectMeta) bool { + ingressClass string, allowClassless bool) func(obj *metav1.ObjectMeta, allowClassless bool) bool { - return func(obj *metav1.ObjectMeta) bool { + return func(obj *metav1.ObjectMeta, allowClassless bool) bool { ingress := obj.GetAnnotations()[ingressClassKey] - return validIngress(ingress, ingressClass) + return validIngress(ingress, ingressClass, allowClassless) } } diff --git a/internal/ingress/annotations/annotations_test.go b/internal/ingress/annotations/annotations_test.go index 55231d2dc3..1a0c577a90 100644 --- a/internal/ingress/annotations/annotations_test.go +++ b/internal/ingress/annotations/annotations_test.go @@ -50,8 +50,8 @@ func TestIngressClassValidatorFunc(t *testing.T) { ing.SetAnnotations(data) for _, test := range tests { ing.Annotations[ingressClassKey] = test.ingress - f := IngressClassValidatorFunc(test.controller) - b := f(&ing.ObjectMeta) + f := IngressClassValidatorFunc(test.controller, true) + b := f(&ing.ObjectMeta, true) if b != test.isValid { t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) } diff --git a/internal/ingress/controller/event_handler.go b/internal/ingress/controller/event_handler.go index 1abe5b9bd0..543c5aae91 100644 --- a/internal/ingress/controller/event_handler.go +++ b/internal/ingress/controller/event_handler.go @@ -12,8 +12,8 @@ import ( // ResourceEventHandler is "ingress.class" aware resource // handler. type ResourceEventHandler struct { - IsValidIngresClass func(object metav1.Object) bool - UpdateCh *channels.RingChannel + IsValidIngressClass func(object metav1.Object, allowClassless bool) bool + UpdateCh *channels.RingChannel } // EventType type of event associated with an informer @@ -43,7 +43,7 @@ func (reh ResourceEventHandler) OnAdd(obj interface{}) { if err != nil { return } - if !reh.IsValidIngresClass(object) { + if !reh.IsValidIngressClass(object, true) { return } reh.UpdateCh.In() <- Event{ @@ -58,7 +58,7 @@ func (reh ResourceEventHandler) OnDelete(obj interface{}) { if err != nil { return } - if !reh.IsValidIngresClass(object) { + if !reh.IsValidIngressClass(object, true) { return } @@ -79,8 +79,8 @@ func (reh ResourceEventHandler) OnUpdate(old, cur interface{}) { if err != nil { return } - validOld := reh.IsValidIngresClass(oldObj) - validCur := reh.IsValidIngresClass(curObj) + validOld := reh.IsValidIngressClass(oldObj, true) + validCur := reh.IsValidIngressClass(curObj, true) if !validCur && !validOld { return diff --git a/internal/ingress/store/fake_store.go b/internal/ingress/store/fake_store.go index f160b09011..774c969758 100644 --- a/internal/ingress/store/fake_store.go +++ b/internal/ingress/store/fake_store.go @@ -140,7 +140,7 @@ func NewFakeStore( KnativeIngress: knativeIngressStore, }, - isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong"), + isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong", true), } return s, nil } diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index b86698fd99..e2f5584f4b 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -82,7 +82,7 @@ type Store struct { ingressClass string - isValidIngressClass func(objectMeta *metav1.ObjectMeta) bool + isValidIngressClass func(objectMeta *metav1.ObjectMeta, allowClassless bool) bool } // CacheStores stores cache.Store for all Kinds of k8s objects that @@ -104,11 +104,11 @@ type CacheStores struct { } // New creates a new object store to be used in the ingress controller -func New(cs CacheStores, ingressClass string) Storer { +func New(cs CacheStores, ingressClass string, allowClassless bool) Storer { return Store{ stores: cs, ingressClass: ingressClass, - isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass), + isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass, allowClassless), } } @@ -144,7 +144,7 @@ func (s Store) ListIngresses() []*networking.Ingress { var ingresses []*networking.Ingress for _, item := range s.stores.Ingress.List() { ing := networkingIngressV1Beta1(item) - if !s.isValidIngressClass(&ing.ObjectMeta) { + if !s.isValidIngressClass(&ing.ObjectMeta, true) { continue } ingresses = append(ingresses, ing) @@ -160,7 +160,7 @@ func (s Store) ListTCPIngresses() ([]*configurationv1beta1.TCPIngress, error) { err := cache.ListAll(s.stores.TCPIngress, labels.NewSelector(), func(ob interface{}) { ing, ok := ob.(*configurationv1beta1.TCPIngress) - if ok && s.isValidIngressClass(&ing.ObjectMeta) { + if ok && s.isValidIngressClass(&ing.ObjectMeta, true) { ingresses = append(ingresses, ing) } }) @@ -270,7 +270,7 @@ func (s Store) ListKongConsumers() []*configurationv1.KongConsumer { var consumers []*configurationv1.KongConsumer for _, item := range s.stores.Consumer.List() { c, ok := item.(*configurationv1.KongConsumer) - if ok && s.isValidIngressClass(&c.ObjectMeta) { + if ok && s.isValidIngressClass(&c.ObjectMeta, true) { consumers = append(consumers, c) } } @@ -284,7 +284,7 @@ func (s Store) ListKongCredentials() []*configurationv1.KongCredential { var credentials []*configurationv1.KongCredential for _, item := range s.stores.Credential.List() { c, ok := item.(*configurationv1.KongCredential) - if ok && s.isValidIngressClass(&c.ObjectMeta) { + if ok && s.isValidIngressClass(&c.ObjectMeta, true) { credentials = append(credentials, c) } } @@ -307,7 +307,7 @@ func (s Store) ListGlobalKongPlugins() ([]*configurationv1.KongPlugin, error) { labels.NewSelector().Add(*req), func(ob interface{}) { p, ok := ob.(*configurationv1.KongPlugin) - if ok && s.isValidIngressClass(&p.ObjectMeta) { + if ok && s.isValidIngressClass(&p.ObjectMeta, true) { plugins = append(plugins, p) } }) @@ -331,7 +331,7 @@ func (s Store) ListGlobalKongClusterPlugins() ([]*configurationv1.KongClusterPlu labels.NewSelector().Add(*req), func(ob interface{}) { p, ok := ob.(*configurationv1.KongClusterPlugin) - if ok && s.isValidIngressClass(&p.ObjectMeta) { + if ok && s.isValidIngressClass(&p.ObjectMeta, true) { plugins = append(plugins, p) } }) From 1c9ab54857a4f842496efe36b40f3ebedca654e4 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 30 Jun 2020 14:58:39 -0700 Subject: [PATCH 03/47] wip: fix tests --- internal/ingress/store/fake_store_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/internal/ingress/store/fake_store_test.go b/internal/ingress/store/fake_store_test.go index 3ed7bb36d1..199fcc018f 100644 --- a/internal/ingress/store/fake_store_test.go +++ b/internal/ingress/store/fake_store_test.go @@ -160,6 +160,27 @@ func TestFakeStoreListTCPIngress(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, + }, + Spec: configurationv1beta1.IngressSpec{ + Rules: []configurationv1beta1.IngressRule{ + { + Port: 9000, + Backend: configurationv1beta1.IngressBackend{ + ServiceName: "foo-svc", + ServicePort: 80, + }, + }, + }, + }, + }, + { + // this TCPIngress should *not* be loaded, as it lacks a class + ObjectMeta: metav1.ObjectMeta{ + Name: "baz", + Namespace: "default", }, Spec: configurationv1beta1.IngressSpec{ Rules: []configurationv1beta1.IngressRule{ From b5d972d4260da1c6952190c9a7a5116df9b00486 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 30 Jun 2020 15:04:26 -0700 Subject: [PATCH 04/47] wip: ignore classless KongClusterPlugin --- internal/ingress/controller/parser/parser_test.go | 6 ++++++ internal/ingress/store/fake_store_test.go | 6 +++++- internal/ingress/store/store.go | 4 ++-- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/internal/ingress/controller/parser/parser_test.go b/internal/ingress/controller/parser/parser_test.go index f6731f2e87..3cfe7cd31a 100644 --- a/internal/ingress/controller/parser/parser_test.go +++ b/internal/ingress/controller/parser/parser_test.go @@ -163,6 +163,9 @@ func TestGlobalPlugin(t *testing.T) { Labels: map[string]string{ "global": "true", }, + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Protocols: []string{"http"}, PluginName: "basic-auth", @@ -307,6 +310,9 @@ func TestSecretConfigurationPlugin(t *testing.T) { Labels: map[string]string{ "global": "true", }, + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Protocols: []string{"http"}, PluginName: "basic-auth", diff --git a/internal/ingress/store/fake_store_test.go b/internal/ingress/store/fake_store_test.go index 199fcc018f..af36da1be6 100644 --- a/internal/ingress/store/fake_store_test.go +++ b/internal/ingress/store/fake_store_test.go @@ -419,9 +419,13 @@ func TestFakeStoreClusterPlugins(t *testing.T) { Labels: map[string]string{ "global": "true", }, + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, }, { + // invalid due to lack of class, not loaded ObjectMeta: metav1.ObjectMeta{ Name: "bar", Labels: map[string]string{ @@ -439,7 +443,7 @@ func TestFakeStoreClusterPlugins(t *testing.T) { assert.Nil(err) assert.NotNil(store) plugins, err = store.ListGlobalKongClusterPlugins() - assert.Len(plugins, 2) + assert.Len(plugins, 1) plugin, err := store.GetKongClusterPlugin("foo") assert.NotNil(plugin) diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index e2f5584f4b..adc30b0a98 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -160,7 +160,7 @@ func (s Store) ListTCPIngresses() ([]*configurationv1beta1.TCPIngress, error) { err := cache.ListAll(s.stores.TCPIngress, labels.NewSelector(), func(ob interface{}) { ing, ok := ob.(*configurationv1beta1.TCPIngress) - if ok && s.isValidIngressClass(&ing.ObjectMeta, true) { + if ok && s.isValidIngressClass(&ing.ObjectMeta, false) { ingresses = append(ingresses, ing) } }) @@ -331,7 +331,7 @@ func (s Store) ListGlobalKongClusterPlugins() ([]*configurationv1.KongClusterPlu labels.NewSelector().Add(*req), func(ob interface{}) { p, ok := ob.(*configurationv1.KongClusterPlugin) - if ok && s.isValidIngressClass(&p.ObjectMeta, true) { + if ok && s.isValidIngressClass(&p.ObjectMeta, false) { plugins = append(plugins, p) } }) From 2f431a3716f89ddb47fe2cbff55aa1737a259a9b Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 30 Jun 2020 15:16:57 -0700 Subject: [PATCH 05/47] wip: update comment --- internal/ingress/annotations/annotations.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index f81a1cc0b0..97c1c173d2 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -53,6 +53,7 @@ func validIngress(ingressAnnotationValue, ingressClass string, allowClassless bo // we have 2 valid combinations // 1 - ingress with default class | blank annotation on ingress // 2 - ingress with specific class | same annotation on ingress + // Listers can opt out of (1) by setting allowClassless == false // // and 2 invalid combinations // 3 - ingress with default class | fixed annotation on ingress From 64098fe2cfc3c28e09917f840a3c82cbe29dc26e Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Thu, 2 Jul 2020 14:22:24 -0700 Subject: [PATCH 06/47] wip: add warnings --- internal/ingress/annotations/annotations.go | 30 ++++++++++++++++----- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 97c1c173d2..575596eb9c 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -19,6 +19,8 @@ package annotations import ( "strings" + "github.com/golang/glog" + "github.com/pkg/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -49,21 +51,24 @@ const ( DefaultIngressClass = "kong" ) -func validIngress(ingressAnnotationValue, ingressClass string, allowClassless bool) bool { +func validIngress(ingressAnnotationValue, ingressClass string, allowClassless bool) (bool, error) { // we have 2 valid combinations // 1 - ingress with default class | blank annotation on ingress // 2 - ingress with specific class | same annotation on ingress - // Listers can opt out of (1) by setting allowClassless == false + // Listers can opt out of (1) by setting allowClassless == false, + // in which case we report an error as well. // // and 2 invalid combinations // 3 - ingress with default class | fixed annotation on ingress // 4 - ingress with specific class | different annotation on ingress if ingressAnnotationValue == "" && ingressClass == DefaultIngressClass { if allowClassless { - return true + return true, nil + } else { + return false, errors.Errorf("resource requires kubernetes.io/ingress.class annotation") } } - return ingressAnnotationValue == ingressClass + return ingressAnnotationValue == ingressClass, nil } // IngressClassValidatorFunc returns a function which can validate if an Object @@ -73,7 +78,15 @@ func IngressClassValidatorFunc( return func(obj metav1.Object, allowClassless bool) bool { ingress := obj.GetAnnotations()[ingressClassKey] - return validIngress(ingress, ingressClass, allowClassless) + validity, err := validIngress(ingress, ingressClass, allowClassless) + // validity always reports whether the resource has a valid class + // we only care about why sometimes, when the resource cannot possibly be valid for + // *any* controller, versus resources that may be valid for others + if err != nil { + glog.Errorf("resource '%s/%s' is invalid: %s", obj.GetNamespace(), obj.GetName(), err) + return validity + } + return validity } } @@ -84,7 +97,12 @@ func IngressClassValidatorFuncFromObjectMeta( return func(obj *metav1.ObjectMeta, allowClassless bool) bool { ingress := obj.GetAnnotations()[ingressClassKey] - return validIngress(ingress, ingressClass, allowClassless) + validity, err := validIngress(ingress, ingressClass, allowClassless) + if err != nil { + glog.Errorf("resource '%s/%s' is invalid: %s", obj.GetNamespace(), obj.GetName(), err) + return validity + } + return validity } } From cad2b23e4c8bc0095fd65280f85e356e62de736c Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Thu, 2 Jul 2020 15:01:11 -0700 Subject: [PATCH 07/47] wip: hacky hacky logging --- internal/ingress/annotations/annotations.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 575596eb9c..e16dcdd20b 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -22,6 +22,7 @@ import ( "github.com/golang/glog" "github.com/pkg/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + runtime "k8s.io/apimachinery/pkg/runtime" ) const ( @@ -71,6 +72,14 @@ func validIngress(ingressAnnotationValue, ingressClass string, allowClassless bo return ingressAnnotationValue == ingressClass, nil } +func ObjectMetaToObjectKind(obj metav1.Object) string { + robj, ok := obj.(runtime.Object) + if !ok { + return "" + } + return robj.GetObjectKind().GroupVersionKind().Kind +} + // IngressClassValidatorFunc returns a function which can validate if an Object // belongs to an the ingressClass or not. func IngressClassValidatorFunc( @@ -83,7 +92,8 @@ func IngressClassValidatorFunc( // we only care about why sometimes, when the resource cannot possibly be valid for // *any* controller, versus resources that may be valid for others if err != nil { - glog.Errorf("resource '%s/%s' is invalid: %s", obj.GetNamespace(), obj.GetName(), err) + glog.Errorf("%s resource '%s/%s' is invalid: %s", ObjectMetaToObjectKind(obj), + obj.GetNamespace(), obj.GetName(), err) return validity } return validity @@ -99,7 +109,8 @@ func IngressClassValidatorFuncFromObjectMeta( ingress := obj.GetAnnotations()[ingressClassKey] validity, err := validIngress(ingress, ingressClass, allowClassless) if err != nil { - glog.Errorf("resource '%s/%s' is invalid: %s", obj.GetNamespace(), obj.GetName(), err) + glog.Errorf("%s resource '%s/%s' is invalid: %s", ObjectMetaToObjectKind(obj), + obj.GetNamespace(), obj.GetName(), err) return validity } return validity From 75cc4dae9e1b3bac8156b2ce7b04f9eb3affee97 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Mon, 6 Jul 2020 11:26:50 -0700 Subject: [PATCH 08/47] wip: add notes from mtg --- internal/ingress/controller/controller.go | 1 + internal/ingress/controller/event_handler.go | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 249a17f415..3a6605bbbb 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -119,6 +119,7 @@ func (n *KongController) syncIngress(interface{}) error { }) glog.V(2).Infof("syncing Ingress configuration...") + // NOTE: runs on everything? no filter before here? state, err := n.parser.Build() if err != nil { return errors.Wrap(err, "error building kong state") diff --git a/internal/ingress/controller/event_handler.go b/internal/ingress/controller/event_handler.go index 543c5aae91..5a77c3ad51 100644 --- a/internal/ingress/controller/event_handler.go +++ b/internal/ingress/controller/event_handler.go @@ -37,6 +37,11 @@ type Event struct { Old interface{} } +// NOTE: the magic happens here, but it doesnt have to, +// could use fieldSelector +// We can create different IsValidIngressClass functions, prob with a more generic name +// to just fetch everything always, or use some different criteria +// could try and duck-type Secrets to see if they look like a plugin secret // OnAdd is invoked whenever a resource is added. func (reh ResourceEventHandler) OnAdd(obj interface{}) { object, err := meta.Accessor(obj) From bc0a387c8f6974d5b634a0e073acad81c073bbb5 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Mon, 6 Jul 2020 16:06:38 -0700 Subject: [PATCH 09/47] wip: refactor class handling Refactor the class filter to have ternary modes of operation: * Required: the resource must have a class annotation, and it must match. * Lazy: the resource can have a class annotation, but if we use the default class annotation and the resource has none, accept the classless resource. * Ignored: the resource does not require a class annotation. We watch it always, and assume that it cannot be used unless some other resource that has Required or Lazy behavior pulls it in. --- cli/ingress-controller/main.go | 4 +- cli/ingress-controller/main_test.go | 3 +- internal/ingress/annotations/annotations.go | 52 +++++++++++++++---- .../ingress/annotations/annotations_test.go | 4 +- internal/ingress/controller/event_handler.go | 11 ++-- internal/ingress/store/fake_store.go | 2 +- internal/ingress/store/store.go | 18 +++---- 7 files changed, 63 insertions(+), 31 deletions(-) diff --git a/cli/ingress-controller/main.go b/cli/ingress-controller/main.go index 0a8f5a184d..19339241a5 100644 --- a/cli/ingress-controller/main.go +++ b/cli/ingress-controller/main.go @@ -287,7 +287,7 @@ func main() { updateChannel := channels.NewRingChannel(1024) reh := controller.ResourceEventHandler{ UpdateCh: updateChannel, - IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass, true), + IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass, annotations.ClassLazy), } var informers []cache.SharedIndexInformer var cacheStores store.CacheStores @@ -366,7 +366,7 @@ func main() { runtime.HandleError(fmt.Errorf("Timed out waiting for caches to sync")) } - store := store.New(cacheStores, cliConfig.IngressClass, true) + store := store.New(cacheStores, cliConfig.IngressClass, annotations.ClassLazy) kong, err := controller.NewKongController(&controllerConfig, updateChannel, store) if err != nil { diff --git a/cli/ingress-controller/main_test.go b/cli/ingress-controller/main_test.go index dba30d91a4..d0a60930cc 100644 --- a/cli/ingress-controller/main_test.go +++ b/cli/ingress-controller/main_test.go @@ -23,6 +23,7 @@ import ( "testing" "github.com/eapache/channels" + "github.com/kong/kubernetes-ingress-controller/internal/ingress/annotations" "github.com/kong/kubernetes-ingress-controller/internal/ingress/controller" "github.com/kong/kubernetes-ingress-controller/internal/ingress/store" ) @@ -76,7 +77,7 @@ func TestHandleSigterm(t *testing.T) { KubeClient: kubeClient, }, channels.NewRingChannel(1024), - store.New(store.CacheStores{}, conf.IngressClass, true), + store.New(store.CacheStores{}, conf.IngressClass, annotations.ClassLazy), ) exitCh := make(chan int, 1) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index e16dcdd20b..d9c51e0cb6 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -47,27 +47,57 @@ const ( hostHeaderKey = "/host-header" methodsKey = "/methods" + ClassRequired = "required" + ClassIgnored = "ignored" + ClassLazy = "optional" + // DefaultIngressClass defines the default class used // by Kong's ingress controller. DefaultIngressClass = "kong" ) -func validIngress(ingressAnnotationValue, ingressClass string, allowClassless bool) (bool, error) { +func validIngress(ingressAnnotationValue, ingressClass string, classHandling string) (bool, error) { // we have 2 valid combinations // 1 - ingress with default class | blank annotation on ingress // 2 - ingress with specific class | same annotation on ingress - // Listers can opt out of (1) by setting allowClassless == false, + // Listers can opt out of (1) by setting classHandling == false, // in which case we report an error as well. // // and 2 invalid combinations // 3 - ingress with default class | fixed annotation on ingress // 4 - ingress with specific class | different annotation on ingress - if ingressAnnotationValue == "" && ingressClass == DefaultIngressClass { - if allowClassless { + + emptyMatch := ingressAnnotationValue == "" && ingressAnnotationValue != ingressClass + lazyMatch := ingressAnnotationValue == "" && ingressClass == DefaultIngressClass + exactMatch := ingressAnnotationValue == ingressClass + if classHandling == ClassRequired { + // this MUST have ingress.class, and it must match + if exactMatch { + return true, nil + } else if lazyMatch { + return false, errors.Errorf("resource requires kubernetes.io/ingress.class annotation") + } + return false, nil + } else if classHandling == ClassIgnored { + // this does not require ingress.class. we watch events if it is empty + // do we watch events if it doesn't match? shouldn't happen but might, because legacy + if emptyMatch { return true, nil - } else { + } + return false, nil + } else if classHandling == ClassLazy { + // this can have a class. we'll watch empty class resources if we use the default + if exactMatch || lazyMatch { + return true, nil + } + return false, nil + } + + if ingressAnnotationValue == "" && ingressClass == DefaultIngressClass { + if classHandling == ClassRequired { return false, errors.Errorf("resource requires kubernetes.io/ingress.class annotation") } + return true, nil } return ingressAnnotationValue == ingressClass, nil } @@ -83,11 +113,11 @@ func ObjectMetaToObjectKind(obj metav1.Object) string { // IngressClassValidatorFunc returns a function which can validate if an Object // belongs to an the ingressClass or not. func IngressClassValidatorFunc( - ingressClass string, allowClassless bool) func(obj metav1.Object, allowClassless bool) bool { + ingressClass string, classHandling string) func(obj metav1.Object, classHandling string) bool { - return func(obj metav1.Object, allowClassless bool) bool { + return func(obj metav1.Object, classHandling string) bool { ingress := obj.GetAnnotations()[ingressClassKey] - validity, err := validIngress(ingress, ingressClass, allowClassless) + validity, err := validIngress(ingress, ingressClass, classHandling) // validity always reports whether the resource has a valid class // we only care about why sometimes, when the resource cannot possibly be valid for // *any* controller, versus resources that may be valid for others @@ -103,11 +133,11 @@ func IngressClassValidatorFunc( // IngressClassValidatorFuncFromObjectMeta returns a function which // can validate if an ObjectMeta belongs to an the ingressClass or not. func IngressClassValidatorFuncFromObjectMeta( - ingressClass string, allowClassless bool) func(obj *metav1.ObjectMeta, allowClassless bool) bool { + ingressClass string, classHandling string) func(obj *metav1.ObjectMeta, classHandling string) bool { - return func(obj *metav1.ObjectMeta, allowClassless bool) bool { + return func(obj *metav1.ObjectMeta, classHandling string) bool { ingress := obj.GetAnnotations()[ingressClassKey] - validity, err := validIngress(ingress, ingressClass, allowClassless) + validity, err := validIngress(ingress, ingressClass, classHandling) if err != nil { glog.Errorf("%s resource '%s/%s' is invalid: %s", ObjectMetaToObjectKind(obj), obj.GetNamespace(), obj.GetName(), err) diff --git a/internal/ingress/annotations/annotations_test.go b/internal/ingress/annotations/annotations_test.go index 1a0c577a90..c7e356f812 100644 --- a/internal/ingress/annotations/annotations_test.go +++ b/internal/ingress/annotations/annotations_test.go @@ -50,8 +50,8 @@ func TestIngressClassValidatorFunc(t *testing.T) { ing.SetAnnotations(data) for _, test := range tests { ing.Annotations[ingressClassKey] = test.ingress - f := IngressClassValidatorFunc(test.controller, true) - b := f(&ing.ObjectMeta, true) + f := IngressClassValidatorFunc(test.controller, ClassLazy) + b := f(&ing.ObjectMeta, ClassLazy) if b != test.isValid { t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) } diff --git a/internal/ingress/controller/event_handler.go b/internal/ingress/controller/event_handler.go index 5a77c3ad51..e5b4ecfe01 100644 --- a/internal/ingress/controller/event_handler.go +++ b/internal/ingress/controller/event_handler.go @@ -4,6 +4,7 @@ import ( "reflect" "github.com/eapache/channels" + "github.com/kong/kubernetes-ingress-controller/internal/ingress/annotations" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -12,7 +13,7 @@ import ( // ResourceEventHandler is "ingress.class" aware resource // handler. type ResourceEventHandler struct { - IsValidIngressClass func(object metav1.Object, allowClassless bool) bool + IsValidIngressClass func(object metav1.Object, classHandling string) bool UpdateCh *channels.RingChannel } @@ -48,7 +49,7 @@ func (reh ResourceEventHandler) OnAdd(obj interface{}) { if err != nil { return } - if !reh.IsValidIngressClass(object, true) { + if !reh.IsValidIngressClass(object, annotations.ClassLazy) { return } reh.UpdateCh.In() <- Event{ @@ -63,7 +64,7 @@ func (reh ResourceEventHandler) OnDelete(obj interface{}) { if err != nil { return } - if !reh.IsValidIngressClass(object, true) { + if !reh.IsValidIngressClass(object, annotations.ClassLazy) { return } @@ -84,8 +85,8 @@ func (reh ResourceEventHandler) OnUpdate(old, cur interface{}) { if err != nil { return } - validOld := reh.IsValidIngressClass(oldObj, true) - validCur := reh.IsValidIngressClass(curObj, true) + validOld := reh.IsValidIngressClass(oldObj, annotations.ClassLazy) + validCur := reh.IsValidIngressClass(curObj, annotations.ClassLazy) if !validCur && !validOld { return diff --git a/internal/ingress/store/fake_store.go b/internal/ingress/store/fake_store.go index 774c969758..9586474c2b 100644 --- a/internal/ingress/store/fake_store.go +++ b/internal/ingress/store/fake_store.go @@ -140,7 +140,7 @@ func NewFakeStore( KnativeIngress: knativeIngressStore, }, - isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong", true), + isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong", annotations.ClassLazy), } return s, nil } diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index adc30b0a98..1deb817ffe 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -82,7 +82,7 @@ type Store struct { ingressClass string - isValidIngressClass func(objectMeta *metav1.ObjectMeta, allowClassless bool) bool + isValidIngressClass func(objectMeta *metav1.ObjectMeta, classHandling string) bool } // CacheStores stores cache.Store for all Kinds of k8s objects that @@ -104,11 +104,11 @@ type CacheStores struct { } // New creates a new object store to be used in the ingress controller -func New(cs CacheStores, ingressClass string, allowClassless bool) Storer { +func New(cs CacheStores, ingressClass string, classHandling string) Storer { return Store{ stores: cs, ingressClass: ingressClass, - isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass, allowClassless), + isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass, classHandling), } } @@ -144,7 +144,7 @@ func (s Store) ListIngresses() []*networking.Ingress { var ingresses []*networking.Ingress for _, item := range s.stores.Ingress.List() { ing := networkingIngressV1Beta1(item) - if !s.isValidIngressClass(&ing.ObjectMeta, true) { + if !s.isValidIngressClass(&ing.ObjectMeta, annotations.ClassLazy) { continue } ingresses = append(ingresses, ing) @@ -160,7 +160,7 @@ func (s Store) ListTCPIngresses() ([]*configurationv1beta1.TCPIngress, error) { err := cache.ListAll(s.stores.TCPIngress, labels.NewSelector(), func(ob interface{}) { ing, ok := ob.(*configurationv1beta1.TCPIngress) - if ok && s.isValidIngressClass(&ing.ObjectMeta, false) { + if ok && s.isValidIngressClass(&ing.ObjectMeta, annotations.ClassRequired) { ingresses = append(ingresses, ing) } }) @@ -270,7 +270,7 @@ func (s Store) ListKongConsumers() []*configurationv1.KongConsumer { var consumers []*configurationv1.KongConsumer for _, item := range s.stores.Consumer.List() { c, ok := item.(*configurationv1.KongConsumer) - if ok && s.isValidIngressClass(&c.ObjectMeta, true) { + if ok && s.isValidIngressClass(&c.ObjectMeta, annotations.ClassLazy) { consumers = append(consumers, c) } } @@ -284,7 +284,7 @@ func (s Store) ListKongCredentials() []*configurationv1.KongCredential { var credentials []*configurationv1.KongCredential for _, item := range s.stores.Credential.List() { c, ok := item.(*configurationv1.KongCredential) - if ok && s.isValidIngressClass(&c.ObjectMeta, true) { + if ok && s.isValidIngressClass(&c.ObjectMeta, annotations.ClassLazy) { credentials = append(credentials, c) } } @@ -307,7 +307,7 @@ func (s Store) ListGlobalKongPlugins() ([]*configurationv1.KongPlugin, error) { labels.NewSelector().Add(*req), func(ob interface{}) { p, ok := ob.(*configurationv1.KongPlugin) - if ok && s.isValidIngressClass(&p.ObjectMeta, true) { + if ok && s.isValidIngressClass(&p.ObjectMeta, annotations.ClassLazy) { plugins = append(plugins, p) } }) @@ -331,7 +331,7 @@ func (s Store) ListGlobalKongClusterPlugins() ([]*configurationv1.KongClusterPlu labels.NewSelector().Add(*req), func(ob interface{}) { p, ok := ob.(*configurationv1.KongClusterPlugin) - if ok && s.isValidIngressClass(&p.ObjectMeta, false) { + if ok && s.isValidIngressClass(&p.ObjectMeta, annotations.ClassRequired) { plugins = append(plugins, p) } }) From dd917accac9c3fcdf35f2ffb7caf90f85cf3013d Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 7 Jul 2020 09:56:29 -0700 Subject: [PATCH 10/47] wip: remove superfluous check --- internal/ingress/annotations/annotations.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index d9c51e0cb6..37f3f62117 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -93,12 +93,6 @@ func validIngress(ingressAnnotationValue, ingressClass string, classHandling str return false, nil } - if ingressAnnotationValue == "" && ingressClass == DefaultIngressClass { - if classHandling == ClassRequired { - return false, errors.Errorf("resource requires kubernetes.io/ingress.class annotation") - } - return true, nil - } return ingressAnnotationValue == ingressClass, nil } From 513e394871a2a9bfcad5c7c0d394448ac0728b13 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 7 Jul 2020 10:17:30 -0700 Subject: [PATCH 11/47] wip: rename class handlers --- cli/ingress-controller/main.go | 4 ++-- cli/ingress-controller/main_test.go | 2 +- internal/ingress/annotations/annotations.go | 12 ++++++------ internal/ingress/annotations/annotations_test.go | 4 ++-- internal/ingress/controller/event_handler.go | 8 ++++---- internal/ingress/store/fake_store.go | 2 +- internal/ingress/store/store.go | 12 ++++++------ 7 files changed, 22 insertions(+), 22 deletions(-) diff --git a/cli/ingress-controller/main.go b/cli/ingress-controller/main.go index 19339241a5..f3632471c4 100644 --- a/cli/ingress-controller/main.go +++ b/cli/ingress-controller/main.go @@ -287,7 +287,7 @@ func main() { updateChannel := channels.NewRingChannel(1024) reh := controller.ResourceEventHandler{ UpdateCh: updateChannel, - IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass, annotations.ClassLazy), + IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass, annotations.LazyClassHandling), } var informers []cache.SharedIndexInformer var cacheStores store.CacheStores @@ -366,7 +366,7 @@ func main() { runtime.HandleError(fmt.Errorf("Timed out waiting for caches to sync")) } - store := store.New(cacheStores, cliConfig.IngressClass, annotations.ClassLazy) + store := store.New(cacheStores, cliConfig.IngressClass, annotations.LazyClassHandling) kong, err := controller.NewKongController(&controllerConfig, updateChannel, store) if err != nil { diff --git a/cli/ingress-controller/main_test.go b/cli/ingress-controller/main_test.go index d0a60930cc..a8d20839a7 100644 --- a/cli/ingress-controller/main_test.go +++ b/cli/ingress-controller/main_test.go @@ -77,7 +77,7 @@ func TestHandleSigterm(t *testing.T) { KubeClient: kubeClient, }, channels.NewRingChannel(1024), - store.New(store.CacheStores{}, conf.IngressClass, annotations.ClassLazy), + store.New(store.CacheStores{}, conf.IngressClass, annotations.LazyClassHandling), ) exitCh := make(chan int, 1) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 37f3f62117..457c0fe471 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -47,9 +47,9 @@ const ( hostHeaderKey = "/host-header" methodsKey = "/methods" - ClassRequired = "required" - ClassIgnored = "ignored" - ClassLazy = "optional" + RequireClassHandling = "required" + IgnoreClassHandling = "ignored" + LazyClassHandling = "optional" // DefaultIngressClass defines the default class used // by Kong's ingress controller. @@ -70,7 +70,7 @@ func validIngress(ingressAnnotationValue, ingressClass string, classHandling str emptyMatch := ingressAnnotationValue == "" && ingressAnnotationValue != ingressClass lazyMatch := ingressAnnotationValue == "" && ingressClass == DefaultIngressClass exactMatch := ingressAnnotationValue == ingressClass - if classHandling == ClassRequired { + if classHandling == RequireClassHandling { // this MUST have ingress.class, and it must match if exactMatch { return true, nil @@ -78,14 +78,14 @@ func validIngress(ingressAnnotationValue, ingressClass string, classHandling str return false, errors.Errorf("resource requires kubernetes.io/ingress.class annotation") } return false, nil - } else if classHandling == ClassIgnored { + } else if classHandling == IgnoreClassHandling { // this does not require ingress.class. we watch events if it is empty // do we watch events if it doesn't match? shouldn't happen but might, because legacy if emptyMatch { return true, nil } return false, nil - } else if classHandling == ClassLazy { + } else if classHandling == LazyClassHandling { // this can have a class. we'll watch empty class resources if we use the default if exactMatch || lazyMatch { return true, nil diff --git a/internal/ingress/annotations/annotations_test.go b/internal/ingress/annotations/annotations_test.go index c7e356f812..16f71b02b6 100644 --- a/internal/ingress/annotations/annotations_test.go +++ b/internal/ingress/annotations/annotations_test.go @@ -50,8 +50,8 @@ func TestIngressClassValidatorFunc(t *testing.T) { ing.SetAnnotations(data) for _, test := range tests { ing.Annotations[ingressClassKey] = test.ingress - f := IngressClassValidatorFunc(test.controller, ClassLazy) - b := f(&ing.ObjectMeta, ClassLazy) + f := IngressClassValidatorFunc(test.controller, LazyClassHandling) + b := f(&ing.ObjectMeta, LazyClassHandling) if b != test.isValid { t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) } diff --git a/internal/ingress/controller/event_handler.go b/internal/ingress/controller/event_handler.go index e5b4ecfe01..632fb7b2f3 100644 --- a/internal/ingress/controller/event_handler.go +++ b/internal/ingress/controller/event_handler.go @@ -49,7 +49,7 @@ func (reh ResourceEventHandler) OnAdd(obj interface{}) { if err != nil { return } - if !reh.IsValidIngressClass(object, annotations.ClassLazy) { + if !reh.IsValidIngressClass(object, annotations.LazyClassHandling) { return } reh.UpdateCh.In() <- Event{ @@ -64,7 +64,7 @@ func (reh ResourceEventHandler) OnDelete(obj interface{}) { if err != nil { return } - if !reh.IsValidIngressClass(object, annotations.ClassLazy) { + if !reh.IsValidIngressClass(object, annotations.LazyClassHandling) { return } @@ -85,8 +85,8 @@ func (reh ResourceEventHandler) OnUpdate(old, cur interface{}) { if err != nil { return } - validOld := reh.IsValidIngressClass(oldObj, annotations.ClassLazy) - validCur := reh.IsValidIngressClass(curObj, annotations.ClassLazy) + validOld := reh.IsValidIngressClass(oldObj, annotations.LazyClassHandling) + validCur := reh.IsValidIngressClass(curObj, annotations.LazyClassHandling) if !validCur && !validOld { return diff --git a/internal/ingress/store/fake_store.go b/internal/ingress/store/fake_store.go index 9586474c2b..6429c2882a 100644 --- a/internal/ingress/store/fake_store.go +++ b/internal/ingress/store/fake_store.go @@ -140,7 +140,7 @@ func NewFakeStore( KnativeIngress: knativeIngressStore, }, - isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong", annotations.ClassLazy), + isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong", annotations.LazyClassHandling), } return s, nil } diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index 1deb817ffe..a711a2de78 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -144,7 +144,7 @@ func (s Store) ListIngresses() []*networking.Ingress { var ingresses []*networking.Ingress for _, item := range s.stores.Ingress.List() { ing := networkingIngressV1Beta1(item) - if !s.isValidIngressClass(&ing.ObjectMeta, annotations.ClassLazy) { + if !s.isValidIngressClass(&ing.ObjectMeta, annotations.LazyClassHandling) { continue } ingresses = append(ingresses, ing) @@ -160,7 +160,7 @@ func (s Store) ListTCPIngresses() ([]*configurationv1beta1.TCPIngress, error) { err := cache.ListAll(s.stores.TCPIngress, labels.NewSelector(), func(ob interface{}) { ing, ok := ob.(*configurationv1beta1.TCPIngress) - if ok && s.isValidIngressClass(&ing.ObjectMeta, annotations.ClassRequired) { + if ok && s.isValidIngressClass(&ing.ObjectMeta, annotations.RequireClassHandling) { ingresses = append(ingresses, ing) } }) @@ -270,7 +270,7 @@ func (s Store) ListKongConsumers() []*configurationv1.KongConsumer { var consumers []*configurationv1.KongConsumer for _, item := range s.stores.Consumer.List() { c, ok := item.(*configurationv1.KongConsumer) - if ok && s.isValidIngressClass(&c.ObjectMeta, annotations.ClassLazy) { + if ok && s.isValidIngressClass(&c.ObjectMeta, annotations.LazyClassHandling) { consumers = append(consumers, c) } } @@ -284,7 +284,7 @@ func (s Store) ListKongCredentials() []*configurationv1.KongCredential { var credentials []*configurationv1.KongCredential for _, item := range s.stores.Credential.List() { c, ok := item.(*configurationv1.KongCredential) - if ok && s.isValidIngressClass(&c.ObjectMeta, annotations.ClassLazy) { + if ok && s.isValidIngressClass(&c.ObjectMeta, annotations.LazyClassHandling) { credentials = append(credentials, c) } } @@ -307,7 +307,7 @@ func (s Store) ListGlobalKongPlugins() ([]*configurationv1.KongPlugin, error) { labels.NewSelector().Add(*req), func(ob interface{}) { p, ok := ob.(*configurationv1.KongPlugin) - if ok && s.isValidIngressClass(&p.ObjectMeta, annotations.ClassLazy) { + if ok && s.isValidIngressClass(&p.ObjectMeta, annotations.LazyClassHandling) { plugins = append(plugins, p) } }) @@ -331,7 +331,7 @@ func (s Store) ListGlobalKongClusterPlugins() ([]*configurationv1.KongClusterPlu labels.NewSelector().Add(*req), func(ob interface{}) { p, ok := ob.(*configurationv1.KongClusterPlugin) - if ok && s.isValidIngressClass(&p.ObjectMeta, annotations.ClassRequired) { + if ok && s.isValidIngressClass(&p.ObjectMeta, annotations.RequireClassHandling) { plugins = append(plugins, p) } }) From dcc319cd98fd3eb7b0e034b46e50f0d50db1d14d Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 7 Jul 2020 10:42:59 -0700 Subject: [PATCH 12/47] wip: rename in prep to split reh --- cli/ingress-controller/main.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/cli/ingress-controller/main.go b/cli/ingress-controller/main.go index f3632471c4..8fb93213cf 100644 --- a/cli/ingress-controller/main.go +++ b/cli/ingress-controller/main.go @@ -285,7 +285,7 @@ func main() { var synced []cache.InformerSynced updateChannel := channels.NewRingChannel(1024) - reh := controller.ResourceEventHandler{ + lazyReh := controller.ResourceEventHandler{ UpdateCh: updateChannel, IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass, annotations.LazyClassHandling), } @@ -299,7 +299,7 @@ func main() { ingInformer = coreInformerFactory.Extensions().V1beta1().Ingresses().Informer() } - ingInformer.AddEventHandler(reh) + ingInformer.AddEventHandler(lazyReh) cacheStores.Ingress = ingInformer.GetStore() informers = append(informers, ingInformer) @@ -311,48 +311,48 @@ func main() { informers = append(informers, endpointsInformer) secretsInformer := coreInformerFactory.Core().V1().Secrets().Informer() - secretsInformer.AddEventHandler(reh) + secretsInformer.AddEventHandler(lazyReh) cacheStores.Secret = secretsInformer.GetStore() informers = append(informers, secretsInformer) servicesInformer := coreInformerFactory.Core().V1().Services().Informer() - servicesInformer.AddEventHandler(reh) + servicesInformer.AddEventHandler(lazyReh) cacheStores.Service = servicesInformer.GetStore() informers = append(informers, servicesInformer) tcpIngressInformer := kongInformerFactory.Configuration().V1beta1().TCPIngresses().Informer() - tcpIngressInformer.AddEventHandler(reh) + tcpIngressInformer.AddEventHandler(lazyReh) cacheStores.TCPIngress = tcpIngressInformer.GetStore() informers = append(informers, tcpIngressInformer) kongIngressInformer := kongInformerFactory.Configuration().V1().KongIngresses().Informer() - kongIngressInformer.AddEventHandler(reh) + kongIngressInformer.AddEventHandler(lazyReh) cacheStores.Configuration = kongIngressInformer.GetStore() informers = append(informers, kongIngressInformer) kongPluginInformer := kongInformerFactory.Configuration().V1().KongPlugins().Informer() - kongPluginInformer.AddEventHandler(reh) + kongPluginInformer.AddEventHandler(lazyReh) cacheStores.Plugin = kongPluginInformer.GetStore() informers = append(informers, kongPluginInformer) kongClusterPluginInformer := kongInformerFactory.Configuration().V1().KongClusterPlugins().Informer() - kongClusterPluginInformer.AddEventHandler(reh) + kongClusterPluginInformer.AddEventHandler(lazyReh) cacheStores.ClusterPlugin = kongClusterPluginInformer.GetStore() informers = append(informers, kongClusterPluginInformer) kongConsumerInformer := kongInformerFactory.Configuration().V1().KongConsumers().Informer() - kongConsumerInformer.AddEventHandler(reh) + kongConsumerInformer.AddEventHandler(lazyReh) cacheStores.Consumer = kongConsumerInformer.GetStore() informers = append(informers, kongConsumerInformer) kongCredentialInformer := kongInformerFactory.Configuration().V1().KongCredentials().Informer() - kongCredentialInformer.AddEventHandler(reh) + kongCredentialInformer.AddEventHandler(lazyReh) cacheStores.Credential = kongCredentialInformer.GetStore() informers = append(informers, kongCredentialInformer) if controllerConfig.EnableKnativeIngressSupport { knativeIngressInformer := knativeInformerFactory.Networking().V1alpha1().Ingresses().Informer() - knativeIngressInformer.AddEventHandler(reh) + knativeIngressInformer.AddEventHandler(lazyReh) cacheStores.KnativeIngress = knativeIngressInformer.GetStore() informers = append(informers, knativeIngressInformer) } From 7c696a693f603af8ec49ea7af5f64e87aab15094 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 7 Jul 2020 10:50:07 -0700 Subject: [PATCH 13/47] wip: split REHs --- cli/ingress-controller/main.go | 19 +++++++++++++++---- internal/ingress/controller/event_handler.go | 3 ++- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/cli/ingress-controller/main.go b/cli/ingress-controller/main.go index 8fb93213cf..93762af1eb 100644 --- a/cli/ingress-controller/main.go +++ b/cli/ingress-controller/main.go @@ -288,6 +288,17 @@ func main() { lazyReh := controller.ResourceEventHandler{ UpdateCh: updateChannel, IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass, annotations.LazyClassHandling), + ClassHandling: annotations.LazyClassHandling, + } + strictReh := controller.ResourceEventHandler{ + UpdateCh: updateChannel, + IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass, annotations.RequireClassHandling), + ClassHandling: annotations.RequireClassHandling, + } + allReh := controller.ResourceEventHandler{ + UpdateCh: updateChannel, + IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass, annotations.IgnoreClassHandling), + ClassHandling: annotations.IgnoreClassHandling, } var informers []cache.SharedIndexInformer var cacheStores store.CacheStores @@ -311,7 +322,7 @@ func main() { informers = append(informers, endpointsInformer) secretsInformer := coreInformerFactory.Core().V1().Secrets().Informer() - secretsInformer.AddEventHandler(lazyReh) + secretsInformer.AddEventHandler(allReh) cacheStores.Secret = secretsInformer.GetStore() informers = append(informers, secretsInformer) @@ -321,7 +332,7 @@ func main() { informers = append(informers, servicesInformer) tcpIngressInformer := kongInformerFactory.Configuration().V1beta1().TCPIngresses().Informer() - tcpIngressInformer.AddEventHandler(lazyReh) + tcpIngressInformer.AddEventHandler(strictReh) cacheStores.TCPIngress = tcpIngressInformer.GetStore() informers = append(informers, tcpIngressInformer) @@ -331,12 +342,12 @@ func main() { informers = append(informers, kongIngressInformer) kongPluginInformer := kongInformerFactory.Configuration().V1().KongPlugins().Informer() - kongPluginInformer.AddEventHandler(lazyReh) + kongPluginInformer.AddEventHandler(allReh) cacheStores.Plugin = kongPluginInformer.GetStore() informers = append(informers, kongPluginInformer) kongClusterPluginInformer := kongInformerFactory.Configuration().V1().KongClusterPlugins().Informer() - kongClusterPluginInformer.AddEventHandler(lazyReh) + kongClusterPluginInformer.AddEventHandler(strictReh) cacheStores.ClusterPlugin = kongClusterPluginInformer.GetStore() informers = append(informers, kongClusterPluginInformer) diff --git a/internal/ingress/controller/event_handler.go b/internal/ingress/controller/event_handler.go index 632fb7b2f3..8554e14733 100644 --- a/internal/ingress/controller/event_handler.go +++ b/internal/ingress/controller/event_handler.go @@ -15,6 +15,7 @@ import ( type ResourceEventHandler struct { IsValidIngressClass func(object metav1.Object, classHandling string) bool UpdateCh *channels.RingChannel + ClassHandling string } // EventType type of event associated with an informer @@ -49,7 +50,7 @@ func (reh ResourceEventHandler) OnAdd(obj interface{}) { if err != nil { return } - if !reh.IsValidIngressClass(object, annotations.LazyClassHandling) { + if !reh.IsValidIngressClass(object, reh.ClassHandling) { return } reh.UpdateCh.In() <- Event{ From 25e77302afc3b37f1d8432685842cb324a4e4816 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 7 Jul 2020 12:14:36 -0700 Subject: [PATCH 14/47] wip: refactor validator calls --- cli/ingress-controller/main.go | 8 ++++---- cli/ingress-controller/main_test.go | 3 +-- internal/ingress/annotations/annotations.go | 6 +++--- internal/ingress/annotations/annotations_test.go | 2 +- internal/ingress/store/fake_store.go | 2 +- internal/ingress/store/store.go | 4 ++-- 6 files changed, 12 insertions(+), 13 deletions(-) diff --git a/cli/ingress-controller/main.go b/cli/ingress-controller/main.go index 93762af1eb..4463c85a55 100644 --- a/cli/ingress-controller/main.go +++ b/cli/ingress-controller/main.go @@ -287,17 +287,17 @@ func main() { updateChannel := channels.NewRingChannel(1024) lazyReh := controller.ResourceEventHandler{ UpdateCh: updateChannel, - IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass, annotations.LazyClassHandling), + IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass), ClassHandling: annotations.LazyClassHandling, } strictReh := controller.ResourceEventHandler{ UpdateCh: updateChannel, - IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass, annotations.RequireClassHandling), + IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass), ClassHandling: annotations.RequireClassHandling, } allReh := controller.ResourceEventHandler{ UpdateCh: updateChannel, - IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass, annotations.IgnoreClassHandling), + IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass), ClassHandling: annotations.IgnoreClassHandling, } var informers []cache.SharedIndexInformer @@ -377,7 +377,7 @@ func main() { runtime.HandleError(fmt.Errorf("Timed out waiting for caches to sync")) } - store := store.New(cacheStores, cliConfig.IngressClass, annotations.LazyClassHandling) + store := store.New(cacheStores, cliConfig.IngressClass) kong, err := controller.NewKongController(&controllerConfig, updateChannel, store) if err != nil { diff --git a/cli/ingress-controller/main_test.go b/cli/ingress-controller/main_test.go index a8d20839a7..44b8a43221 100644 --- a/cli/ingress-controller/main_test.go +++ b/cli/ingress-controller/main_test.go @@ -23,7 +23,6 @@ import ( "testing" "github.com/eapache/channels" - "github.com/kong/kubernetes-ingress-controller/internal/ingress/annotations" "github.com/kong/kubernetes-ingress-controller/internal/ingress/controller" "github.com/kong/kubernetes-ingress-controller/internal/ingress/store" ) @@ -77,7 +76,7 @@ func TestHandleSigterm(t *testing.T) { KubeClient: kubeClient, }, channels.NewRingChannel(1024), - store.New(store.CacheStores{}, conf.IngressClass, annotations.LazyClassHandling), + store.New(store.CacheStores{}, conf.IngressClass), ) exitCh := make(chan int, 1) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 457c0fe471..7b5325c1fa 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -49,7 +49,7 @@ const ( RequireClassHandling = "required" IgnoreClassHandling = "ignored" - LazyClassHandling = "optional" + LazyClassHandling = "optional" // DefaultIngressClass defines the default class used // by Kong's ingress controller. @@ -107,7 +107,7 @@ func ObjectMetaToObjectKind(obj metav1.Object) string { // IngressClassValidatorFunc returns a function which can validate if an Object // belongs to an the ingressClass or not. func IngressClassValidatorFunc( - ingressClass string, classHandling string) func(obj metav1.Object, classHandling string) bool { + ingressClass string) func(obj metav1.Object, classHandling string) bool { return func(obj metav1.Object, classHandling string) bool { ingress := obj.GetAnnotations()[ingressClassKey] @@ -127,7 +127,7 @@ func IngressClassValidatorFunc( // IngressClassValidatorFuncFromObjectMeta returns a function which // can validate if an ObjectMeta belongs to an the ingressClass or not. func IngressClassValidatorFuncFromObjectMeta( - ingressClass string, classHandling string) func(obj *metav1.ObjectMeta, classHandling string) bool { + ingressClass string) func(obj *metav1.ObjectMeta, classHandling string) bool { return func(obj *metav1.ObjectMeta, classHandling string) bool { ingress := obj.GetAnnotations()[ingressClassKey] diff --git a/internal/ingress/annotations/annotations_test.go b/internal/ingress/annotations/annotations_test.go index 16f71b02b6..51fbe29d0c 100644 --- a/internal/ingress/annotations/annotations_test.go +++ b/internal/ingress/annotations/annotations_test.go @@ -50,7 +50,7 @@ func TestIngressClassValidatorFunc(t *testing.T) { ing.SetAnnotations(data) for _, test := range tests { ing.Annotations[ingressClassKey] = test.ingress - f := IngressClassValidatorFunc(test.controller, LazyClassHandling) + f := IngressClassValidatorFunc(test.controller) b := f(&ing.ObjectMeta, LazyClassHandling) if b != test.isValid { t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) diff --git a/internal/ingress/store/fake_store.go b/internal/ingress/store/fake_store.go index 6429c2882a..f160b09011 100644 --- a/internal/ingress/store/fake_store.go +++ b/internal/ingress/store/fake_store.go @@ -140,7 +140,7 @@ func NewFakeStore( KnativeIngress: knativeIngressStore, }, - isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong", annotations.LazyClassHandling), + isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong"), } return s, nil } diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index a711a2de78..50ed66b7f7 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -104,11 +104,11 @@ type CacheStores struct { } // New creates a new object store to be used in the ingress controller -func New(cs CacheStores, ingressClass string, classHandling string) Storer { +func New(cs CacheStores, ingressClass string) Storer { return Store{ stores: cs, ingressClass: ingressClass, - isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass, classHandling), + isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass), } } From 284880f8dccfaf002e4208fd1c349f8bb9bd8c4e Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Thu, 9 Jul 2020 14:02:02 -0700 Subject: [PATCH 15/47] wip: fix incorrect merge --- internal/ingress/store/store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index a51eb0184e..32762fa6c7 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -155,7 +155,7 @@ func (s Store) ListIngresses() []*networking.Ingress { // filter ingress rules var ingresses []*networking.Ingress for _, item := range s.stores.Ingress.List() { - ing := networkingIngressV1Beta1(item) + ing := s.networkingIngressV1Beta1(item) if !s.isValidIngressClass(&ing.ObjectMeta, s.ingressClassHandling) { continue } From cc030d2a67520098e522d6ed42a8de8300fc94cb Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Thu, 9 Jul 2020 14:35:03 -0700 Subject: [PATCH 16/47] wip: fix lint/mod issues --- go.mod | 3 ++- internal/ingress/annotations/annotations.go | 15 +++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index d36e3f29bc..b428df43b0 100644 --- a/go.mod +++ b/go.mod @@ -10,7 +10,6 @@ require ( github.com/eapache/channels v1.1.0 github.com/fatih/color v1.9.0 github.com/ghodss/yaml v1.0.0 - github.com/google/go-containerregistry v0.0.0-20200131185320-aec8da010de2 // indirect github.com/hashicorp/go-memdb v1.2.0 // indirect github.com/hashicorp/go-uuid v1.0.1 github.com/hbagdi/deck v1.1.0 @@ -36,5 +35,7 @@ require ( k8s.io/client-go v0.17.4 k8s.io/klog v1.0.0 knative.dev/pkg v0.0.0-20200205160431-4ec5e09f716b + "" + github.com/sirupsen/logrus v1.4.2 knative.dev/serving v0.12.1 ) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 7b5325c1fa..464c2d849c 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -19,8 +19,8 @@ package annotations import ( "strings" - "github.com/golang/glog" "github.com/pkg/errors" + "github.com/sirupsen/logrus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" runtime "k8s.io/apimachinery/pkg/runtime" ) @@ -47,9 +47,12 @@ const ( hostHeaderKey = "/host-header" methodsKey = "/methods" + // RequireClassHandling indicates that ingress.class must be present and match RequireClassHandling = "required" - IgnoreClassHandling = "ignored" - LazyClassHandling = "optional" + // IgnoreClassHandling indicates that ingress.class is not required and is ignored + IgnoreClassHandling = "ignored" + // LazyClassHandling indicates that ingress.class can be empty if using the default class + LazyClassHandling = "optional" // DefaultIngressClass defines the default class used // by Kong's ingress controller. @@ -96,7 +99,7 @@ func validIngress(ingressAnnotationValue, ingressClass string, classHandling str return ingressAnnotationValue == ingressClass, nil } -func ObjectMetaToObjectKind(obj metav1.Object) string { +func objectMetaToObjectKind(obj metav1.Object) string { robj, ok := obj.(runtime.Object) if !ok { return "" @@ -116,7 +119,7 @@ func IngressClassValidatorFunc( // we only care about why sometimes, when the resource cannot possibly be valid for // *any* controller, versus resources that may be valid for others if err != nil { - glog.Errorf("%s resource '%s/%s' is invalid: %s", ObjectMetaToObjectKind(obj), + logrus.Errorf("%s resource '%s/%s' is invalid: %s", objectMetaToObjectKind(obj), obj.GetNamespace(), obj.GetName(), err) return validity } @@ -133,7 +136,7 @@ func IngressClassValidatorFuncFromObjectMeta( ingress := obj.GetAnnotations()[ingressClassKey] validity, err := validIngress(ingress, ingressClass, classHandling) if err != nil { - glog.Errorf("%s resource '%s/%s' is invalid: %s", ObjectMetaToObjectKind(obj), + logrus.Errorf("%s resource '%s/%s' is invalid: %s", objectMetaToObjectKind(obj), obj.GetNamespace(), obj.GetName(), err) return validity } From 0b8c7d1d2be85e0d7ac8e8cbb6259a4df9cf7065 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Thu, 9 Jul 2020 14:38:23 -0700 Subject: [PATCH 17/47] wip: deps? --- go.mod | 3 +-- go.sum | 2 ++ 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index b428df43b0..d36e3f29bc 100644 --- a/go.mod +++ b/go.mod @@ -10,6 +10,7 @@ require ( github.com/eapache/channels v1.1.0 github.com/fatih/color v1.9.0 github.com/ghodss/yaml v1.0.0 + github.com/google/go-containerregistry v0.0.0-20200131185320-aec8da010de2 // indirect github.com/hashicorp/go-memdb v1.2.0 // indirect github.com/hashicorp/go-uuid v1.0.1 github.com/hbagdi/deck v1.1.0 @@ -35,7 +36,5 @@ require ( k8s.io/client-go v0.17.4 k8s.io/klog v1.0.0 knative.dev/pkg v0.0.0-20200205160431-4ec5e09f716b - "" - github.com/sirupsen/logrus v1.4.2 knative.dev/serving v0.12.1 ) diff --git a/go.sum b/go.sum index 59239571b4..53e809f7d5 100644 --- a/go.sum +++ b/go.sum @@ -214,6 +214,8 @@ github.com/hashicorp/go-memdb v1.1.2 h1:/xeGrWlD1+X26mLdgLDKzQHPsMG8Z3u7N/S1M7/G github.com/hashicorp/go-memdb v1.1.2/go.mod h1:LWQ8R70vPrS4OEY9k28D2z8/Zzyu34NVzeRibGAzHO0= github.com/hashicorp/go-memdb v1.2.0 h1:nF9fNAZZJ16ttLenacrsVX3Y8GPD+Be9cc1TUWIbvGY= github.com/hashicorp/go-memdb v1.2.0/go.mod h1:7vWdFrpS3/w/8LMPBDKg4CERFHoMVysv3uW8DtdxfhI= +github.com/hashicorp/go-memdb v1.2.1 h1:wI9btDjYUOJJHTCnRlAG/TkRyD/ij7meJMrLK9X31Cc= +github.com/hashicorp/go-memdb v1.2.1/go.mod h1:OSvLJ662Jim8hMM+gWGyhktyWk2xPCnWMc7DWIqtkGA= github.com/hashicorp/go-uuid v1.0.0 h1:RS8zrF7PhGwyNPOtxSClXXj9HA8feRnJzgnI1RJCSnM= github.com/hashicorp/go-uuid v1.0.0/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= github.com/hashicorp/go-uuid v1.0.1 h1:fv1ep09latC32wFoVwnqcnKJGnMSdBanPczbHAYm1BE= From fd8eac40f2678aa46223965dcd5b1b5ad7634c47 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Thu, 9 Jul 2020 14:42:27 -0700 Subject: [PATCH 18/47] wip: go decide what you want --- go.sum | 2 -- 1 file changed, 2 deletions(-) diff --git a/go.sum b/go.sum index 53e809f7d5..59239571b4 100644 --- a/go.sum +++ b/go.sum @@ -214,8 +214,6 @@ github.com/hashicorp/go-memdb v1.1.2 h1:/xeGrWlD1+X26mLdgLDKzQHPsMG8Z3u7N/S1M7/G github.com/hashicorp/go-memdb v1.1.2/go.mod h1:LWQ8R70vPrS4OEY9k28D2z8/Zzyu34NVzeRibGAzHO0= github.com/hashicorp/go-memdb v1.2.0 h1:nF9fNAZZJ16ttLenacrsVX3Y8GPD+Be9cc1TUWIbvGY= github.com/hashicorp/go-memdb v1.2.0/go.mod h1:7vWdFrpS3/w/8LMPBDKg4CERFHoMVysv3uW8DtdxfhI= -github.com/hashicorp/go-memdb v1.2.1 h1:wI9btDjYUOJJHTCnRlAG/TkRyD/ij7meJMrLK9X31Cc= -github.com/hashicorp/go-memdb v1.2.1/go.mod h1:OSvLJ662Jim8hMM+gWGyhktyWk2xPCnWMc7DWIqtkGA= github.com/hashicorp/go-uuid v1.0.0 h1:RS8zrF7PhGwyNPOtxSClXXj9HA8feRnJzgnI1RJCSnM= github.com/hashicorp/go-uuid v1.0.0/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= github.com/hashicorp/go-uuid v1.0.1 h1:fv1ep09latC32wFoVwnqcnKJGnMSdBanPczbHAYm1BE= From baace98c56d883470037ea5e76f76edb99c0805b Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Thu, 9 Jul 2020 16:32:52 -0700 Subject: [PATCH 19/47] wip: fix misaligned/misconfigured tests --- .../ingress/controller/parser/parser_test.go | 20 +++++++++++++++++++ internal/ingress/store/fake_store.go | 3 ++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/internal/ingress/controller/parser/parser_test.go b/internal/ingress/controller/parser/parser_test.go index 85bc07c876..374325de43 100644 --- a/internal/ingress/controller/parser/parser_test.go +++ b/internal/ingress/controller/parser/parser_test.go @@ -290,6 +290,26 @@ func TestSecretConfigurationPlugin(t *testing.T) { }, }, }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "global-broken-bar-plugin", + Labels: map[string]string{ + "global": "true", + }, + Annotations: map[string]string{ + // explicitly none, this should not get rendered + }, + }, + Protocols: []string{"http"}, + PluginName: "basic-auth", + ConfigFrom: configurationv1.NamespacedConfigSource{ + SecretValue: configurationv1.NamespacedSecretValueFromSource{ + Key: "basic-auth-config", + Secret: "conf-secret", + Namespace: "default", + }, + }, + }, { ObjectMeta: metav1.ObjectMeta{ Name: "bar-plugin", diff --git a/internal/ingress/store/fake_store.go b/internal/ingress/store/fake_store.go index 7743c344e9..958e3418ec 100644 --- a/internal/ingress/store/fake_store.go +++ b/internal/ingress/store/fake_store.go @@ -141,7 +141,8 @@ func NewFakeStore( KnativeIngress: knativeIngressStore, }, - isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong"), + isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong"), + ingressClassHandling: annotations.LazyClassHandling, } return s, nil } From 960900a6c15737d42f68980af7d1d0f17fc8f340 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Thu, 9 Jul 2020 17:50:40 -0700 Subject: [PATCH 20/47] wip: wrangle tests more --- internal/ingress/annotations/annotations_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/ingress/annotations/annotations_test.go b/internal/ingress/annotations/annotations_test.go index 0bd0cdf7ef..bee0fd272e 100644 --- a/internal/ingress/annotations/annotations_test.go +++ b/internal/ingress/annotations/annotations_test.go @@ -56,7 +56,13 @@ func TestIngressClassValidatorFunc(t *testing.T) { for _, test := range tests { ing.Annotations[ingressClassKey] = test.ingress f := IngressClassValidatorFunc(test.controller) - b := f(&ing.ObjectMeta, LazyClassHandling) + var b bool + if test.skipClasslessIngress { + b = f(&ing.ObjectMeta, RequireClassHandling) + } else { + b = f(&ing.ObjectMeta, LazyClassHandling) + } + if b != test.isValid { t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) } From 9456797e189dd2f9e475313508efce0ba0df48bf Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Fri, 10 Jul 2020 14:52:06 -0700 Subject: [PATCH 21/47] wip: fix fake store handling --- internal/ingress/store/fake_store.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/ingress/store/fake_store.go b/internal/ingress/store/fake_store.go index 958e3418ec..0a6945970d 100644 --- a/internal/ingress/store/fake_store.go +++ b/internal/ingress/store/fake_store.go @@ -124,7 +124,12 @@ func NewFakeStore( return nil, err } } - + var ingressClassHandling string + if objects.SkipClasslessIngress { + ingressClassHandling = annotations.RequireClassHandling + } else { + ingressClassHandling = annotations.LazyClassHandling + } s = Store{ stores: CacheStores{ Ingress: ingressStore, @@ -142,7 +147,7 @@ func NewFakeStore( KnativeIngress: knativeIngressStore, }, isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong"), - ingressClassHandling: annotations.LazyClassHandling, + ingressClassHandling: ingressClassHandling, } return s, nil } From 9d704710ab373c3dee2eb9559ee41849d5a20a6b Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Mon, 3 Aug 2020 11:33:18 -0700 Subject: [PATCH 22/47] wip: tidy go.mod --- go.mod | 1 + 1 file changed, 1 insertion(+) diff --git a/go.mod b/go.mod index 40cb8e4063..d36e3f29bc 100644 --- a/go.mod +++ b/go.mod @@ -19,6 +19,7 @@ require ( github.com/mattbaird/jsonpatch v0.0.0-20171005235357-81af80346b1a // indirect github.com/mitchellh/mapstructure v1.2.2 github.com/openzipkin/zipkin-go v0.2.2 // indirect + github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v1.0.0 github.com/sirupsen/logrus v1.4.2 github.com/spf13/pflag v1.0.5 From b00d7edeb1150b8af9ced172cec838466ec8df7d Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Mon, 3 Aug 2020 11:51:02 -0700 Subject: [PATCH 23/47] wip: neutralize class filters in REH Disable actual checks of class filters within the REHs, to instead have the store handle all class checks. --- internal/ingress/controller/event_handler.go | 32 -------------------- 1 file changed, 32 deletions(-) diff --git a/internal/ingress/controller/event_handler.go b/internal/ingress/controller/event_handler.go index 8554e14733..1648e5995c 100644 --- a/internal/ingress/controller/event_handler.go +++ b/internal/ingress/controller/event_handler.go @@ -4,9 +4,7 @@ import ( "reflect" "github.com/eapache/channels" - "github.com/kong/kubernetes-ingress-controller/internal/ingress/annotations" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -46,13 +44,6 @@ type Event struct { // could try and duck-type Secrets to see if they look like a plugin secret // OnAdd is invoked whenever a resource is added. func (reh ResourceEventHandler) OnAdd(obj interface{}) { - object, err := meta.Accessor(obj) - if err != nil { - return - } - if !reh.IsValidIngressClass(object, reh.ClassHandling) { - return - } reh.UpdateCh.In() <- Event{ Type: CreateEvent, Obj: obj, @@ -61,14 +52,6 @@ func (reh ResourceEventHandler) OnAdd(obj interface{}) { // OnDelete is invoked whenever a resource is deleted. func (reh ResourceEventHandler) OnDelete(obj interface{}) { - object, err := meta.Accessor(obj) - if err != nil { - return - } - if !reh.IsValidIngressClass(object, annotations.LazyClassHandling) { - return - } - reh.UpdateCh.In() <- Event{ Type: DeleteEvent, Obj: obj, @@ -78,21 +61,6 @@ func (reh ResourceEventHandler) OnDelete(obj interface{}) { // OnUpdate is invoked whenever a resource is changed. old holds // the previous resource and cur is the updated resource. func (reh ResourceEventHandler) OnUpdate(old, cur interface{}) { - oldObj, err := meta.Accessor(old) - if err != nil { - return - } - curObj, err := meta.Accessor(cur) - if err != nil { - return - } - validOld := reh.IsValidIngressClass(oldObj, annotations.LazyClassHandling) - validCur := reh.IsValidIngressClass(curObj, annotations.LazyClassHandling) - - if !validCur && !validOld { - return - } - reh.UpdateCh.In() <- Event{ Type: UpdateEvent, Obj: cur, From 5f621c34421e86cab3641b40f147b17c2ffed714 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Mon, 3 Aug 2020 12:04:49 -0700 Subject: [PATCH 24/47] wip: disable credential class checks --- internal/ingress/store/store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index 32762fa6c7..2f98508e62 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -296,7 +296,7 @@ func (s Store) ListKongCredentials() []*configurationv1.KongCredential { var credentials []*configurationv1.KongCredential for _, item := range s.stores.Credential.List() { c, ok := item.(*configurationv1.KongCredential) - if ok && s.isValidIngressClass(&c.ObjectMeta, s.ingressClassHandling) { + if ok && s.isValidIngressClass(&c.ObjectMeta, annotations.IgnoreClassHandling) { credentials = append(credentials, c) } } From 6158c255f578ba4640345c30748e92dedd0b26de Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 4 Aug 2020 15:16:27 -0700 Subject: [PATCH 25/47] pr: remove remaining REH class filters --- cli/ingress-controller/main.go | 13 +++---------- internal/ingress/controller/event_handler.go | 5 +---- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/cli/ingress-controller/main.go b/cli/ingress-controller/main.go index 1e04b976fc..212ae78c1a 100644 --- a/cli/ingress-controller/main.go +++ b/cli/ingress-controller/main.go @@ -38,7 +38,6 @@ import ( "github.com/hashicorp/go-uuid" "github.com/hbagdi/go-kong/kong" "github.com/kong/kubernetes-ingress-controller/internal/admission" - "github.com/kong/kubernetes-ingress-controller/internal/ingress/annotations" "github.com/kong/kubernetes-ingress-controller/internal/ingress/controller" "github.com/kong/kubernetes-ingress-controller/internal/ingress/store" "github.com/kong/kubernetes-ingress-controller/internal/ingress/utils" @@ -347,19 +346,13 @@ func main() { var synced []cache.InformerSynced updateChannel := channels.NewRingChannel(1024) lazyReh := controller.ResourceEventHandler{ - UpdateCh: updateChannel, - IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass), - ClassHandling: annotations.LazyClassHandling, + UpdateCh: updateChannel, } strictReh := controller.ResourceEventHandler{ - UpdateCh: updateChannel, - IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass), - ClassHandling: annotations.RequireClassHandling, + UpdateCh: updateChannel, } allReh := controller.ResourceEventHandler{ - UpdateCh: updateChannel, - IsValidIngressClass: annotations.IngressClassValidatorFunc(cliConfig.IngressClass), - ClassHandling: annotations.IgnoreClassHandling, + UpdateCh: updateChannel, } var informers []cache.SharedIndexInformer diff --git a/internal/ingress/controller/event_handler.go b/internal/ingress/controller/event_handler.go index 1648e5995c..222f7409f9 100644 --- a/internal/ingress/controller/event_handler.go +++ b/internal/ingress/controller/event_handler.go @@ -5,15 +5,12 @@ import ( "github.com/eapache/channels" corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // ResourceEventHandler is "ingress.class" aware resource // handler. type ResourceEventHandler struct { - IsValidIngressClass func(object metav1.Object, classHandling string) bool - UpdateCh *channels.RingChannel - ClassHandling string + UpdateCh *channels.RingChannel } // EventType type of event associated with an informer From 8474870835a6b7d2b2f37997e07c4ba2c469d23d Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 4 Aug 2020 16:12:26 -0700 Subject: [PATCH 26/47] pr: remove additional obsolete code --- cli/ingress-controller/main.go | 41 +++++++++------------------------- 1 file changed, 11 insertions(+), 30 deletions(-) diff --git a/cli/ingress-controller/main.go b/cli/ingress-controller/main.go index 212ae78c1a..4ebfdd6aa7 100644 --- a/cli/ingress-controller/main.go +++ b/cli/ingress-controller/main.go @@ -345,13 +345,7 @@ func main() { var synced []cache.InformerSynced updateChannel := channels.NewRingChannel(1024) - lazyReh := controller.ResourceEventHandler{ - UpdateCh: updateChannel, - } - strictReh := controller.ResourceEventHandler{ - UpdateCh: updateChannel, - } - allReh := controller.ResourceEventHandler{ + reh := controller.ResourceEventHandler{ UpdateCh: updateChannel, } @@ -365,20 +359,7 @@ func main() { ingInformer = coreInformerFactory.Extensions().V1beta1().Ingresses().Informer() } - // per user preference, we default to either: - var baseReh controller.ResourceEventHandler - if cliConfig.SkipClasslessIngressV1beta1 { - // skip classless resources, even if using the default ingress.class - // only load resources with our ingress.class and their dependencies - baseReh = strictReh - } else { - // ingest classless resources, if and only if using the default class, either: - // load resources with no ingress.class or ingress.class == "kong" and their dependencies - // only load resources with our (custom, ingress.class != "kong") ingress.class and their dependencies - baseReh = lazyReh - } - - ingInformer.AddEventHandler(baseReh) + ingInformer.AddEventHandler(reh) cacheStores.Ingress = ingInformer.GetStore() informers = append(informers, ingInformer) @@ -390,48 +371,48 @@ func main() { informers = append(informers, endpointsInformer) secretsInformer := coreInformerFactory.Core().V1().Secrets().Informer() - secretsInformer.AddEventHandler(allReh) + secretsInformer.AddEventHandler(reh) cacheStores.Secret = secretsInformer.GetStore() informers = append(informers, secretsInformer) servicesInformer := coreInformerFactory.Core().V1().Services().Informer() - servicesInformer.AddEventHandler(baseReh) + servicesInformer.AddEventHandler(reh) cacheStores.Service = servicesInformer.GetStore() informers = append(informers, servicesInformer) tcpIngressInformer := kongInformerFactory.Configuration().V1beta1().TCPIngresses().Informer() - tcpIngressInformer.AddEventHandler(strictReh) + tcpIngressInformer.AddEventHandler(reh) cacheStores.TCPIngress = tcpIngressInformer.GetStore() informers = append(informers, tcpIngressInformer) kongIngressInformer := kongInformerFactory.Configuration().V1().KongIngresses().Informer() - kongIngressInformer.AddEventHandler(baseReh) + kongIngressInformer.AddEventHandler(reh) cacheStores.Configuration = kongIngressInformer.GetStore() informers = append(informers, kongIngressInformer) kongPluginInformer := kongInformerFactory.Configuration().V1().KongPlugins().Informer() - kongPluginInformer.AddEventHandler(allReh) + kongPluginInformer.AddEventHandler(reh) cacheStores.Plugin = kongPluginInformer.GetStore() informers = append(informers, kongPluginInformer) kongClusterPluginInformer := kongInformerFactory.Configuration().V1().KongClusterPlugins().Informer() - kongClusterPluginInformer.AddEventHandler(strictReh) + kongClusterPluginInformer.AddEventHandler(reh) cacheStores.ClusterPlugin = kongClusterPluginInformer.GetStore() informers = append(informers, kongClusterPluginInformer) kongConsumerInformer := kongInformerFactory.Configuration().V1().KongConsumers().Informer() - kongConsumerInformer.AddEventHandler(baseReh) + kongConsumerInformer.AddEventHandler(reh) cacheStores.Consumer = kongConsumerInformer.GetStore() informers = append(informers, kongConsumerInformer) kongCredentialInformer := kongInformerFactory.Configuration().V1().KongCredentials().Informer() - kongCredentialInformer.AddEventHandler(baseReh) + kongCredentialInformer.AddEventHandler(reh) cacheStores.Credential = kongCredentialInformer.GetStore() informers = append(informers, kongCredentialInformer) if controllerConfig.EnableKnativeIngressSupport { knativeIngressInformer := knativeInformerFactory.Networking().V1alpha1().Ingresses().Informer() - knativeIngressInformer.AddEventHandler(baseReh) + knativeIngressInformer.AddEventHandler(reh) cacheStores.KnativeIngress = knativeIngressInformer.GetStore() informers = append(informers, knativeIngressInformer) } From 1ccd6eb753cb3da5162bf3bf9ea610b293a5cfaf Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 4 Aug 2020 16:21:31 -0700 Subject: [PATCH 27/47] pr: replace old error handler --- internal/ingress/annotations/annotations.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 0a2caedd93..a168b2ad90 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -17,9 +17,9 @@ limitations under the License. package annotations import ( + "fmt" "strings" - "github.com/pkg/errors" "github.com/sirupsen/logrus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" runtime "k8s.io/apimachinery/pkg/runtime" @@ -78,7 +78,7 @@ func validIngress(ingressAnnotationValue, ingressClass string, classHandling str if exactMatch { return true, nil } else if lazyMatch { - return false, errors.Errorf("resource requires kubernetes.io/ingress.class annotation") + return false, fmt.Errorf("resource requires kubernetes.io/ingress.class annotation") } return false, nil } else if classHandling == IgnoreClassHandling { From be804bfad25f505af36bbe8b62c9ac16ce64d031 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 4 Aug 2020 16:23:15 -0700 Subject: [PATCH 28/47] chore: remove temporary comment --- internal/ingress/controller/event_handler.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/internal/ingress/controller/event_handler.go b/internal/ingress/controller/event_handler.go index 222f7409f9..62eed21235 100644 --- a/internal/ingress/controller/event_handler.go +++ b/internal/ingress/controller/event_handler.go @@ -34,12 +34,6 @@ type Event struct { Old interface{} } -// NOTE: the magic happens here, but it doesnt have to, -// could use fieldSelector -// We can create different IsValidIngressClass functions, prob with a more generic name -// to just fetch everything always, or use some different criteria -// could try and duck-type Secrets to see if they look like a plugin secret -// OnAdd is invoked whenever a resource is added. func (reh ResourceEventHandler) OnAdd(obj interface{}) { reh.UpdateCh.In() <- Event{ Type: CreateEvent, From 109061dded1adeb6950d94fceba3489acd34fc14 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Wed, 12 Aug 2020 16:03:07 -0700 Subject: [PATCH 29/47] wip: refactor class filter --- go.mod | 1 - internal/ingress/annotations/annotations.go | 79 ++++++++----------- .../ingress/annotations/annotations_test.go | 6 +- internal/ingress/store/fake_store.go | 6 +- internal/ingress/store/store.go | 46 +++++++---- 5 files changed, 69 insertions(+), 69 deletions(-) diff --git a/go.mod b/go.mod index d36e3f29bc..40cb8e4063 100644 --- a/go.mod +++ b/go.mod @@ -19,7 +19,6 @@ require ( github.com/mattbaird/jsonpatch v0.0.0-20171005235357-81af80346b1a // indirect github.com/mitchellh/mapstructure v1.2.2 github.com/openzipkin/zipkin-go v0.2.2 // indirect - github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v1.0.0 github.com/sirupsen/logrus v1.4.2 github.com/spf13/pflag v1.0.5 diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index a168b2ad90..1d69c3a6e8 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -17,7 +17,6 @@ limitations under the License. package annotations import ( - "fmt" "strings" "github.com/sirupsen/logrus" @@ -25,6 +24,8 @@ import ( runtime "k8s.io/apimachinery/pkg/runtime" ) +type ClassHandling int + const ( ingressClassKey = "kubernetes.io/ingress.class" @@ -47,56 +48,38 @@ const ( hostHeaderKey = "/host-header" methodsKey = "/methods" - // RequireClassHandling indicates that ingress.class must be present and match - RequireClassHandling = "required" - // IgnoreClassHandling indicates that ingress.class is not required and is ignored - IgnoreClassHandling = "ignored" - // LazyClassHandling indicates that ingress.class can be empty if using the default class - LazyClassHandling = "optional" - + IgnoreClassMatch ClassHandling = iota + ExactOrEmptyClassMatch ClassHandling = iota + ExactClassMatch ClassHandling = iota // DefaultIngressClass defines the default class used // by Kong's ingress controller. DefaultIngressClass = "kong" ) -func validIngress(ingressAnnotationValue, ingressClass string, classHandling string) (bool, error) { - // we have 2 valid combinations - // 1 - ingress with default class | blank annotation on ingress - // 2 - ingress with specific class | same annotation on ingress - // Listers can opt out of (1) by setting classHandling == false, - // in which case we report an error as well. - // - // and 2 invalid combinations - // 3 - ingress with default class | fixed annotation on ingress - // 4 - ingress with specific class | different annotation on ingress - - emptyMatch := ingressAnnotationValue == "" && ingressAnnotationValue != ingressClass - lazyMatch := ingressAnnotationValue == "" && ingressClass == DefaultIngressClass - exactMatch := ingressAnnotationValue == ingressClass - if classHandling == RequireClassHandling { - // this MUST have ingress.class, and it must match - if exactMatch { - return true, nil - } else if lazyMatch { - return false, fmt.Errorf("resource requires kubernetes.io/ingress.class annotation") - } - return false, nil - } else if classHandling == IgnoreClassHandling { - // this does not require ingress.class. we watch events if it is empty - // do we watch events if it doesn't match? shouldn't happen but might, because legacy - if emptyMatch { +func validIngress(ingressAnnotationValue, ingressClass string, handling ClassHandling) (bool, error) { + switch handling { + case IgnoreClassMatch: + // class is not considered at all. any value, even a mismatch, is valid. + // may want to consider warning on mismatches, since while valid, they probably indicate a conflict with user + // intent, and probably should be something users clear for good config hygiene + return true, nil + case ExactOrEmptyClassMatch: + // aka lazy. exact match desired, but empty permitted + if ingressAnnotationValue == "" || ingressAnnotationValue == ingressClass { return true, nil - } - return false, nil - } else if classHandling == LazyClassHandling { - // this can have a class. we'll watch empty class resources if we use the default - if exactMatch || lazyMatch { + } // no alternative case, since everything else should be a mismatch + // would only report mismatches at a high debug level if at all + case ExactClassMatch: + // what it says on the tin + if ingressAnnotationValue == ingressClass { return true, nil - } - return false, nil + } // probably want to warn on empty + default: + panic("invalid ingress class handling option received") } - return ingressAnnotationValue == ingressClass, nil + // no match with our chosen match behavior, so ignore this resource + return false, nil } func objectMetaToObjectKind(obj metav1.Object) string { @@ -110,11 +93,11 @@ func objectMetaToObjectKind(obj metav1.Object) string { // IngressClassValidatorFunc returns a function which can validate if an Object // belongs to an the ingressClass or not. func IngressClassValidatorFunc( - ingressClass string) func(obj metav1.Object, classHandling string) bool { + ingressClass string) func(obj metav1.Object, handling ClassHandling) bool { - return func(obj metav1.Object, classHandling string) bool { + return func(obj metav1.Object, handling ClassHandling) bool { ingress := obj.GetAnnotations()[ingressClassKey] - validity, err := validIngress(ingress, ingressClass, classHandling) + validity, err := validIngress(ingress, ingressClass, handling) // validity always reports whether the resource has a valid class // we only care about why sometimes, when the resource cannot possibly be valid for // *any* controller, versus resources that may be valid for others @@ -130,11 +113,11 @@ func IngressClassValidatorFunc( // IngressClassValidatorFuncFromObjectMeta returns a function which // can validate if an ObjectMeta belongs to an the ingressClass or not. func IngressClassValidatorFuncFromObjectMeta( - ingressClass string) func(obj *metav1.ObjectMeta, classHandling string) bool { + ingressClass string) func(obj *metav1.ObjectMeta, handling ClassHandling) bool { - return func(obj *metav1.ObjectMeta, classHandling string) bool { + return func(obj *metav1.ObjectMeta, handling ClassHandling) bool { ingress := obj.GetAnnotations()[ingressClassKey] - validity, err := validIngress(ingress, ingressClass, classHandling) + validity, err := validIngress(ingress, ingressClass, handling) if err != nil { logrus.Errorf("%s resource '%s/%s' is invalid: %s", objectMetaToObjectKind(obj), obj.GetNamespace(), obj.GetName(), err) diff --git a/internal/ingress/annotations/annotations_test.go b/internal/ingress/annotations/annotations_test.go index 5e83e65141..8e555db07b 100644 --- a/internal/ingress/annotations/annotations_test.go +++ b/internal/ingress/annotations/annotations_test.go @@ -38,7 +38,7 @@ func TestIngressClassValidatorFunc(t *testing.T) { {"kong", false, "kong", true}, {"kong", true, "kong", true}, {"custom", false, "custom", true}, - {"", false, "killer", false}, + {"", false, "killer", true}, {"custom", false, "kong", false}, {"custom", true, "kong", false}, {"", true, "custom", false}, @@ -58,9 +58,9 @@ func TestIngressClassValidatorFunc(t *testing.T) { f := IngressClassValidatorFunc(test.controller) var b bool if test.skipClasslessIngress { - b = f(&ing.ObjectMeta, RequireClassHandling) + b = f(&ing.ObjectMeta, ExactClassMatch) } else { - b = f(&ing.ObjectMeta, LazyClassHandling) + b = f(&ing.ObjectMeta, ExactOrEmptyClassMatch) } if b != test.isValid { diff --git a/internal/ingress/store/fake_store.go b/internal/ingress/store/fake_store.go index 0a6945970d..921e3628d1 100644 --- a/internal/ingress/store/fake_store.go +++ b/internal/ingress/store/fake_store.go @@ -124,11 +124,11 @@ func NewFakeStore( return nil, err } } - var ingressClassHandling string + var ingressClassHandling annotations.ClassHandling if objects.SkipClasslessIngress { - ingressClassHandling = annotations.RequireClassHandling + ingressClassHandling = annotations.ExactClassMatch } else { - ingressClassHandling = annotations.LazyClassHandling + ingressClassHandling = annotations.ExactOrEmptyClassMatch } s = Store{ stores: CacheStores{ diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index 2f98508e62..e76e389e73 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -82,9 +82,10 @@ type Store struct { ingressClass string - ingressClassHandling string + ingressClassHandling annotations.ClassHandling + kongConsumerClassHandling annotations.ClassHandling - isValidIngressClass func(objectMeta *metav1.ObjectMeta, classHandling string) bool + isValidIngressClass func(objectMeta *metav1.ObjectMeta, handling annotations.ClassHandling) bool logger logrus.FieldLogger } @@ -109,18 +110,31 @@ type CacheStores struct { // New creates a new object store to be used in the ingress controller func New(cs CacheStores, ingressClass string, skipClasslessIngress bool, logger logrus.FieldLogger) Storer { - var ingressClassHandling string + var ingressClassHandling annotations.ClassHandling + var kongConsumerClassHandling annotations.ClassHandling if skipClasslessIngress { - ingressClassHandling = annotations.RequireClassHandling + ingressClassHandling = annotations.ExactClassMatch + // TODO this is a placeholder; it should retrieve this from a new KongConsumer flag + // for now, since it's the same type, just copy the ingressClassHandling + kongConsumerClassHandling = annotations.ExactClassMatch } else { - ingressClassHandling = annotations.LazyClassHandling + ingressClassHandling = annotations.ExactOrEmptyClassMatch + // ditto + kongConsumerClassHandling = annotations.ExactOrEmptyClassMatch + } + foo := kongConsumerClassHandling + if foo == 0 { + panic("why") + } else { + foo = 4 } return Store{ - stores: cs, - ingressClass: ingressClass, - ingressClassHandling: ingressClassHandling, - isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass), - logger: logger, + stores: cs, + ingressClass: ingressClass, + ingressClassHandling: ingressClassHandling, + kongConsumerClassHandling: kongConsumerClassHandling, + isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass), + logger: logger, } } @@ -172,7 +186,7 @@ func (s Store) ListTCPIngresses() ([]*configurationv1beta1.TCPIngress, error) { err := cache.ListAll(s.stores.TCPIngress, labels.NewSelector(), func(ob interface{}) { ing, ok := ob.(*configurationv1beta1.TCPIngress) - if ok && s.isValidIngressClass(&ing.ObjectMeta, annotations.RequireClassHandling) { + if ok && s.isValidIngressClass(&ing.ObjectMeta, annotations.ExactClassMatch) { ingresses = append(ingresses, ing) } }) @@ -282,7 +296,7 @@ func (s Store) ListKongConsumers() []*configurationv1.KongConsumer { var consumers []*configurationv1.KongConsumer for _, item := range s.stores.Consumer.List() { c, ok := item.(*configurationv1.KongConsumer) - if ok && s.isValidIngressClass(&c.ObjectMeta, s.ingressClassHandling) { + if ok && s.isValidIngressClass(&c.ObjectMeta, s.kongConsumerClassHandling) { consumers = append(consumers, c) } } @@ -296,7 +310,9 @@ func (s Store) ListKongCredentials() []*configurationv1.KongCredential { var credentials []*configurationv1.KongCredential for _, item := range s.stores.Credential.List() { c, ok := item.(*configurationv1.KongCredential) - if ok && s.isValidIngressClass(&c.ObjectMeta, annotations.IgnoreClassHandling) { + // TODO arguably we can just remove the second clause, as this always returns true + // ListCACerts is like this already + if ok && s.isValidIngressClass(&c.ObjectMeta, annotations.IgnoreClassMatch) { credentials = append(credentials, c) } } @@ -321,6 +337,8 @@ func (s Store) ListGlobalKongPlugins() ([]*configurationv1.KongPlugin, error) { labels.NewSelector().Add(*req), func(ob interface{}) { p, ok := ob.(*configurationv1.KongPlugin) + // TODO this isn't in spec, but we're just getting rid of global KongPlugins, correct? + // should be possible to just delete this function if ok && s.isValidIngressClass(&p.ObjectMeta, s.ingressClassHandling) { plugins = append(plugins, p) } @@ -345,7 +363,7 @@ func (s Store) ListGlobalKongClusterPlugins() ([]*configurationv1.KongClusterPlu labels.NewSelector().Add(*req), func(ob interface{}) { p, ok := ob.(*configurationv1.KongClusterPlugin) - if ok && s.isValidIngressClass(&p.ObjectMeta, annotations.RequireClassHandling) { + if ok && s.isValidIngressClass(&p.ObjectMeta, annotations.ExactClassMatch) { plugins = append(plugins, p) } }) From e946eb8f50ce5cd73a84204f42e3f78246f886dc Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Wed, 12 Aug 2020 16:26:03 -0700 Subject: [PATCH 30/47] wip: remove garbage --- internal/ingress/store/store.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index e76e389e73..9797bc9334 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -124,15 +124,15 @@ func New(cs CacheStores, ingressClass string, skipClasslessIngress bool, logger } foo := kongConsumerClassHandling if foo == 0 { - panic("why") + logrus.Errorf("ohno") } else { - foo = 4 + logrus.Errorf("wat") } return Store{ stores: cs, ingressClass: ingressClass, ingressClassHandling: ingressClassHandling, - kongConsumerClassHandling: kongConsumerClassHandling, + kongConsumerClassHandling: 18, isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass), logger: logger, } From a2997ab9d9ad40570ff8e1b05b3b976489991b1b Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Wed, 12 Aug 2020 16:32:34 -0700 Subject: [PATCH 31/47] wip: remove garbage --- internal/ingress/annotations/annotations.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 1d69c3a6e8..1e159c4c63 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -17,6 +17,7 @@ limitations under the License. package annotations import ( + "fmt" "strings" "github.com/sirupsen/logrus" @@ -73,7 +74,9 @@ func validIngress(ingressAnnotationValue, ingressClass string, handling ClassHan // what it says on the tin if ingressAnnotationValue == ingressClass { return true, nil - } // probably want to warn on empty + } else if ingressAnnotationValue == "" { + return false, fmt.Errorf("oh no") + } default: panic("invalid ingress class handling option received") } From d8693246e73dbf7168b1bb8a82fb466084d9336f Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Wed, 12 Aug 2020 16:41:50 -0700 Subject: [PATCH 32/47] wip: clean up trial and error --- internal/ingress/store/store.go | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index 9797bc9334..eac3bdd275 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -111,28 +111,19 @@ type CacheStores struct { // New creates a new object store to be used in the ingress controller func New(cs CacheStores, ingressClass string, skipClasslessIngress bool, logger logrus.FieldLogger) Storer { var ingressClassHandling annotations.ClassHandling - var kongConsumerClassHandling annotations.ClassHandling + // TODO this is a placeholder; it should retrieve this from a new KongConsumer flag + // for now, since it's the same type, just copy the ingressClassHandling + kongConsumerClassHandling := annotations.ExactOrEmptyClassMatch if skipClasslessIngress { ingressClassHandling = annotations.ExactClassMatch - // TODO this is a placeholder; it should retrieve this from a new KongConsumer flag - // for now, since it's the same type, just copy the ingressClassHandling - kongConsumerClassHandling = annotations.ExactClassMatch } else { ingressClassHandling = annotations.ExactOrEmptyClassMatch - // ditto - kongConsumerClassHandling = annotations.ExactOrEmptyClassMatch - } - foo := kongConsumerClassHandling - if foo == 0 { - logrus.Errorf("ohno") - } else { - logrus.Errorf("wat") } return Store{ stores: cs, ingressClass: ingressClass, ingressClassHandling: ingressClassHandling, - kongConsumerClassHandling: 18, + kongConsumerClassHandling: kongConsumerClassHandling, isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass), logger: logger, } From daf05f5e4ba11210655ceb03f8b077dc5cea14f9 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Thu, 13 Aug 2020 15:03:55 -0700 Subject: [PATCH 33/47] pr: fix remaining store issues --- internal/ingress/store/fake_store.go | 8 ++++++-- internal/ingress/store/store.go | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/ingress/store/fake_store.go b/internal/ingress/store/fake_store.go index 921e3628d1..70ce6f4961 100644 --- a/internal/ingress/store/fake_store.go +++ b/internal/ingress/store/fake_store.go @@ -130,6 +130,9 @@ func NewFakeStore( } else { ingressClassHandling = annotations.ExactOrEmptyClassMatch } + // TODO this is a placeholder for the eventual consumer flag + // for now it hard-codes the default + kongConsumerClassHandling := annotations.ExactOrEmptyClassMatch s = Store{ stores: CacheStores{ Ingress: ingressStore, @@ -146,8 +149,9 @@ func NewFakeStore( KnativeIngress: knativeIngressStore, }, - isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong"), - ingressClassHandling: ingressClassHandling, + isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong"), + ingressClassHandling: ingressClassHandling, + kongConsumerClassHandling: kongConsumerClassHandling, } return s, nil } diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index eac3bdd275..a6f4447d48 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -111,8 +111,8 @@ type CacheStores struct { // New creates a new object store to be used in the ingress controller func New(cs CacheStores, ingressClass string, skipClasslessIngress bool, logger logrus.FieldLogger) Storer { var ingressClassHandling annotations.ClassHandling - // TODO this is a placeholder; it should retrieve this from a new KongConsumer flag - // for now, since it's the same type, just copy the ingressClassHandling + // TODO this is a placeholder for the eventual consumer flag + // for now it hard-codes the default kongConsumerClassHandling := annotations.ExactOrEmptyClassMatch if skipClasslessIngress { ingressClassHandling = annotations.ExactClassMatch From 02335b6e17cf370172389fcedb2d24ba849a27a4 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Thu, 13 Aug 2020 15:04:08 -0700 Subject: [PATCH 34/47] pr: strip out added errors --- internal/ingress/annotations/annotations.go | 43 ++++----------------- 1 file changed, 8 insertions(+), 35 deletions(-) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 1e159c4c63..19083f7353 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -17,12 +17,9 @@ limitations under the License. package annotations import ( - "fmt" "strings" - "github.com/sirupsen/logrus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - runtime "k8s.io/apimachinery/pkg/runtime" ) type ClassHandling int @@ -57,40 +54,31 @@ const ( DefaultIngressClass = "kong" ) -func validIngress(ingressAnnotationValue, ingressClass string, handling ClassHandling) (bool, error) { +func validIngress(ingressAnnotationValue, ingressClass string, handling ClassHandling) bool { switch handling { case IgnoreClassMatch: // class is not considered at all. any value, even a mismatch, is valid. // may want to consider warning on mismatches, since while valid, they probably indicate a conflict with user // intent, and probably should be something users clear for good config hygiene - return true, nil + return true case ExactOrEmptyClassMatch: // aka lazy. exact match desired, but empty permitted if ingressAnnotationValue == "" || ingressAnnotationValue == ingressClass { - return true, nil + return true } // no alternative case, since everything else should be a mismatch // would only report mismatches at a high debug level if at all case ExactClassMatch: // what it says on the tin + // this may be another place we want to return a warning, since an empty-class resource will never be valid if ingressAnnotationValue == ingressClass { - return true, nil - } else if ingressAnnotationValue == "" { - return false, fmt.Errorf("oh no") + return true } default: panic("invalid ingress class handling option received") } // no match with our chosen match behavior, so ignore this resource - return false, nil -} - -func objectMetaToObjectKind(obj metav1.Object) string { - robj, ok := obj.(runtime.Object) - if !ok { - return "" - } - return robj.GetObjectKind().GroupVersionKind().Kind + return false } // IngressClassValidatorFunc returns a function which can validate if an Object @@ -100,16 +88,7 @@ func IngressClassValidatorFunc( return func(obj metav1.Object, handling ClassHandling) bool { ingress := obj.GetAnnotations()[ingressClassKey] - validity, err := validIngress(ingress, ingressClass, handling) - // validity always reports whether the resource has a valid class - // we only care about why sometimes, when the resource cannot possibly be valid for - // *any* controller, versus resources that may be valid for others - if err != nil { - logrus.Errorf("%s resource '%s/%s' is invalid: %s", objectMetaToObjectKind(obj), - obj.GetNamespace(), obj.GetName(), err) - return validity - } - return validity + return validIngress(ingress, ingressClass, handling) } } @@ -120,13 +99,7 @@ func IngressClassValidatorFuncFromObjectMeta( return func(obj *metav1.ObjectMeta, handling ClassHandling) bool { ingress := obj.GetAnnotations()[ingressClassKey] - validity, err := validIngress(ingress, ingressClass, handling) - if err != nil { - logrus.Errorf("%s resource '%s/%s' is invalid: %s", objectMetaToObjectKind(obj), - obj.GetNamespace(), obj.GetName(), err) - return validity - } - return validity + return validIngress(ingress, ingressClass, handling) } } From e361c0ab44491b6c2903f5daed08c3974aac9e80 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 18 Aug 2020 13:26:59 -0700 Subject: [PATCH 35/47] pr: rearrange constant definition --- internal/ingress/annotations/annotations.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 19083f7353..ecca5e01c8 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -24,6 +24,12 @@ import ( type ClassHandling int +const ( + IgnoreClassMatch ClassHandling = iota + ExactOrEmptyClassMatch ClassHandling = iota + ExactClassMatch ClassHandling = iota +) + const ( ingressClassKey = "kubernetes.io/ingress.class" @@ -46,9 +52,6 @@ const ( hostHeaderKey = "/host-header" methodsKey = "/methods" - IgnoreClassMatch ClassHandling = iota - ExactOrEmptyClassMatch ClassHandling = iota - ExactClassMatch ClassHandling = iota // DefaultIngressClass defines the default class used // by Kong's ingress controller. DefaultIngressClass = "kong" From 33e32ef8bc46596691bdac9cc41a8bb2945ecbaa Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 18 Aug 2020 13:31:04 -0700 Subject: [PATCH 36/47] pr: rename handling variables --- internal/ingress/annotations/annotations.go | 18 ++++++++-------- internal/ingress/store/fake_store.go | 12 +++++------ internal/ingress/store/store.go | 24 ++++++++++----------- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index ecca5e01c8..5b9f77d0aa 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -22,12 +22,12 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -type ClassHandling int +type ClassMatching int const ( - IgnoreClassMatch ClassHandling = iota - ExactOrEmptyClassMatch ClassHandling = iota - ExactClassMatch ClassHandling = iota + IgnoreClassMatch ClassMatching = iota + ExactOrEmptyClassMatch ClassMatching = iota + ExactClassMatch ClassMatching = iota ) const ( @@ -57,7 +57,7 @@ const ( DefaultIngressClass = "kong" ) -func validIngress(ingressAnnotationValue, ingressClass string, handling ClassHandling) bool { +func validIngress(ingressAnnotationValue, ingressClass string, handling ClassMatching) bool { switch handling { case IgnoreClassMatch: // class is not considered at all. any value, even a mismatch, is valid. @@ -87,9 +87,9 @@ func validIngress(ingressAnnotationValue, ingressClass string, handling ClassHan // IngressClassValidatorFunc returns a function which can validate if an Object // belongs to an the ingressClass or not. func IngressClassValidatorFunc( - ingressClass string) func(obj metav1.Object, handling ClassHandling) bool { + ingressClass string) func(obj metav1.Object, handling ClassMatching) bool { - return func(obj metav1.Object, handling ClassHandling) bool { + return func(obj metav1.Object, handling ClassMatching) bool { ingress := obj.GetAnnotations()[ingressClassKey] return validIngress(ingress, ingressClass, handling) } @@ -98,9 +98,9 @@ func IngressClassValidatorFunc( // IngressClassValidatorFuncFromObjectMeta returns a function which // can validate if an ObjectMeta belongs to an the ingressClass or not. func IngressClassValidatorFuncFromObjectMeta( - ingressClass string) func(obj *metav1.ObjectMeta, handling ClassHandling) bool { + ingressClass string) func(obj *metav1.ObjectMeta, handling ClassMatching) bool { - return func(obj *metav1.ObjectMeta, handling ClassHandling) bool { + return func(obj *metav1.ObjectMeta, handling ClassMatching) bool { ingress := obj.GetAnnotations()[ingressClassKey] return validIngress(ingress, ingressClass, handling) } diff --git a/internal/ingress/store/fake_store.go b/internal/ingress/store/fake_store.go index 70ce6f4961..733f20ebd8 100644 --- a/internal/ingress/store/fake_store.go +++ b/internal/ingress/store/fake_store.go @@ -124,15 +124,15 @@ func NewFakeStore( return nil, err } } - var ingressClassHandling annotations.ClassHandling + var ingressClassMatching annotations.ClassMatching if objects.SkipClasslessIngress { - ingressClassHandling = annotations.ExactClassMatch + ingressClassMatching = annotations.ExactClassMatch } else { - ingressClassHandling = annotations.ExactOrEmptyClassMatch + ingressClassMatching = annotations.ExactOrEmptyClassMatch } // TODO this is a placeholder for the eventual consumer flag // for now it hard-codes the default - kongConsumerClassHandling := annotations.ExactOrEmptyClassMatch + kongConsumerClassMatching := annotations.ExactOrEmptyClassMatch s = Store{ stores: CacheStores{ Ingress: ingressStore, @@ -150,8 +150,8 @@ func NewFakeStore( KnativeIngress: knativeIngressStore, }, isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong"), - ingressClassHandling: ingressClassHandling, - kongConsumerClassHandling: kongConsumerClassHandling, + ingressClassMatching: ingressClassMatching, + kongConsumerClassMatching: kongConsumerClassMatching, } return s, nil } diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index a6f4447d48..5f323d1012 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -82,10 +82,10 @@ type Store struct { ingressClass string - ingressClassHandling annotations.ClassHandling - kongConsumerClassHandling annotations.ClassHandling + ingressClassMatching annotations.ClassMatching + kongConsumerClassMatching annotations.ClassMatching - isValidIngressClass func(objectMeta *metav1.ObjectMeta, handling annotations.ClassHandling) bool + isValidIngressClass func(objectMeta *metav1.ObjectMeta, handling annotations.ClassMatching) bool logger logrus.FieldLogger } @@ -110,20 +110,20 @@ type CacheStores struct { // New creates a new object store to be used in the ingress controller func New(cs CacheStores, ingressClass string, skipClasslessIngress bool, logger logrus.FieldLogger) Storer { - var ingressClassHandling annotations.ClassHandling + var ingressClassMatching annotations.ClassMatching // TODO this is a placeholder for the eventual consumer flag // for now it hard-codes the default - kongConsumerClassHandling := annotations.ExactOrEmptyClassMatch + kongConsumerClassMatching := annotations.ExactOrEmptyClassMatch if skipClasslessIngress { - ingressClassHandling = annotations.ExactClassMatch + ingressClassMatching = annotations.ExactClassMatch } else { - ingressClassHandling = annotations.ExactOrEmptyClassMatch + ingressClassMatching = annotations.ExactOrEmptyClassMatch } return Store{ stores: cs, ingressClass: ingressClass, - ingressClassHandling: ingressClassHandling, - kongConsumerClassHandling: kongConsumerClassHandling, + ingressClassMatching: ingressClassMatching, + kongConsumerClassMatching: kongConsumerClassMatching, isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta(ingressClass), logger: logger, } @@ -161,7 +161,7 @@ func (s Store) ListIngresses() []*networking.Ingress { var ingresses []*networking.Ingress for _, item := range s.stores.Ingress.List() { ing := s.networkingIngressV1Beta1(item) - if !s.isValidIngressClass(&ing.ObjectMeta, s.ingressClassHandling) { + if !s.isValidIngressClass(&ing.ObjectMeta, s.ingressClassMatching) { continue } ingresses = append(ingresses, ing) @@ -287,7 +287,7 @@ func (s Store) ListKongConsumers() []*configurationv1.KongConsumer { var consumers []*configurationv1.KongConsumer for _, item := range s.stores.Consumer.List() { c, ok := item.(*configurationv1.KongConsumer) - if ok && s.isValidIngressClass(&c.ObjectMeta, s.kongConsumerClassHandling) { + if ok && s.isValidIngressClass(&c.ObjectMeta, s.kongConsumerClassMatching) { consumers = append(consumers, c) } } @@ -330,7 +330,7 @@ func (s Store) ListGlobalKongPlugins() ([]*configurationv1.KongPlugin, error) { p, ok := ob.(*configurationv1.KongPlugin) // TODO this isn't in spec, but we're just getting rid of global KongPlugins, correct? // should be possible to just delete this function - if ok && s.isValidIngressClass(&p.ObjectMeta, s.ingressClassHandling) { + if ok && s.isValidIngressClass(&p.ObjectMeta, s.ingressClassMatching) { plugins = append(plugins, p) } }) From eca131e8b961c387c624e2238e824adeb0777735 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 18 Aug 2020 16:47:24 -0700 Subject: [PATCH 37/47] pr: simplify returns and remove WIP comments --- internal/ingress/annotations/annotations.go | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 5b9f77d0aa..5800993bfc 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -60,22 +60,15 @@ const ( func validIngress(ingressAnnotationValue, ingressClass string, handling ClassMatching) bool { switch handling { case IgnoreClassMatch: - // class is not considered at all. any value, even a mismatch, is valid. - // may want to consider warning on mismatches, since while valid, they probably indicate a conflict with user - // intent, and probably should be something users clear for good config hygiene + // class is not considered at all. any value, even a mismatch, is valid return true case ExactOrEmptyClassMatch: // aka lazy. exact match desired, but empty permitted - if ingressAnnotationValue == "" || ingressAnnotationValue == ingressClass { - return true - } // no alternative case, since everything else should be a mismatch - // would only report mismatches at a high debug level if at all + return ingressAnnotationValue == "" || ingressAnnotationValue == ingressClass case ExactClassMatch: // what it says on the tin // this may be another place we want to return a warning, since an empty-class resource will never be valid - if ingressAnnotationValue == ingressClass { - return true - } + return ingressAnnotationValue == ingressClass default: panic("invalid ingress class handling option received") } From b076e9513207f714dd9d110f3d205330fe92ae73 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 18 Aug 2020 16:47:53 -0700 Subject: [PATCH 38/47] pr: remove WIP comments --- internal/ingress/store/store.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index 5f323d1012..028ea888ce 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -301,8 +301,6 @@ func (s Store) ListKongCredentials() []*configurationv1.KongCredential { var credentials []*configurationv1.KongCredential for _, item := range s.stores.Credential.List() { c, ok := item.(*configurationv1.KongCredential) - // TODO arguably we can just remove the second clause, as this always returns true - // ListCACerts is like this already if ok && s.isValidIngressClass(&c.ObjectMeta, annotations.IgnoreClassMatch) { credentials = append(credentials, c) } @@ -328,8 +326,6 @@ func (s Store) ListGlobalKongPlugins() ([]*configurationv1.KongPlugin, error) { labels.NewSelector().Add(*req), func(ob interface{}) { p, ok := ob.(*configurationv1.KongPlugin) - // TODO this isn't in spec, but we're just getting rid of global KongPlugins, correct? - // should be possible to just delete this function if ok && s.isValidIngressClass(&p.ObjectMeta, s.ingressClassMatching) { plugins = append(plugins, p) } From d9fbc206a9fee595744e9d6be29de87e3135baad Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 18 Aug 2020 16:53:00 -0700 Subject: [PATCH 39/47] pr: remove superfluous return --- internal/ingress/annotations/annotations.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 5800993bfc..727e9f68c9 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -72,9 +72,6 @@ func validIngress(ingressAnnotationValue, ingressClass string, handling ClassMat default: panic("invalid ingress class handling option received") } - - // no match with our chosen match behavior, so ignore this resource - return false } // IngressClassValidatorFunc returns a function which can validate if an Object From b0bfaa3df1d59a2c44963a18ba1a4947823ef716 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Tue, 18 Aug 2020 18:29:18 -0700 Subject: [PATCH 40/47] pr: add explanatory comments for test table --- internal/ingress/annotations/annotations_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/ingress/annotations/annotations_test.go b/internal/ingress/annotations/annotations_test.go index 8e555db07b..04a6759c26 100644 --- a/internal/ingress/annotations/annotations_test.go +++ b/internal/ingress/annotations/annotations_test.go @@ -27,10 +27,10 @@ import ( func TestIngressClassValidatorFunc(t *testing.T) { tests := []struct { - ingress string - skipClasslessIngress bool - controller string - isValid bool + ingress string // the class set on the Ingress resource + skipClasslessIngress bool // the "user" classless ingress flag value + controller string // the class set on the controller + isValid bool // the expected verdict }{ {"", false, "", true}, {"", false, "kong", true}, From fb3ab86329020776ca81a6eb0acd47f433b32a0a Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Mon, 17 Aug 2020 16:03:42 -0700 Subject: [PATCH 41/47] store: establish new classless default behaviors Rename the classless ingress handling flag and invert its default. Ingress objects with no class annotation are now ignored by default. Rework tests to account for this default, adding class annotations to Ingresses that previously lacked them. Add a flag to control consumer class handling. --- cli/ingress-controller/flags.go | 12 ++- cli/ingress-controller/main.go | 4 +- cli/ingress-controller/main_test.go | 3 +- .../ingress/controller/parser/parser_test.go | 97 ++++++++++++++----- internal/ingress/store/fake_store.go | 40 ++++---- internal/ingress/store/fake_store_test.go | 6 ++ internal/ingress/store/store.go | 18 ++-- 7 files changed, 124 insertions(+), 56 deletions(-) diff --git a/cli/ingress-controller/flags.go b/cli/ingress-controller/flags.go index 6836cf3e3e..223baf6e52 100644 --- a/cli/ingress-controller/flags.go +++ b/cli/ingress-controller/flags.go @@ -58,10 +58,11 @@ type cliConfig struct { KongCustomEntitiesSecret string // Resource filtering - WatchNamespace string - SkipClasslessIngressV1beta1 bool - IngressClass string - ElectionID string + WatchNamespace string + ProcessClasslessIngressV1beta1 bool + ProcessClasslessKongConsumer bool + IngressClass string + ElectionID string // Ingress Status publish resource PublishService string @@ -325,7 +326,8 @@ func parseFlags() (cliConfig, error) { // Resource filtering config.WatchNamespace = viper.GetString("watch-namespace") - config.SkipClasslessIngressV1beta1 = viper.GetBool("skip-classless-ingress-v1beta1") + config.ProcessClasslessIngressV1beta1 = viper.GetBool("process-classless-ingress-v1beta1") + config.ProcessClasslessKongConsumer = viper.GetBool("process-classless-kong-consumer") config.IngressClass = viper.GetString("ingress-class") config.ElectionID = viper.GetString("election-id") diff --git a/cli/ingress-controller/main.go b/cli/ingress-controller/main.go index 4ebfdd6aa7..049dde9387 100644 --- a/cli/ingress-controller/main.go +++ b/cli/ingress-controller/main.go @@ -426,8 +426,8 @@ func main() { runtime.HandleError(fmt.Errorf("Timed out waiting for caches to sync")) } - store := store.New(cacheStores, cliConfig.IngressClass, cliConfig.SkipClasslessIngressV1beta1, - log.WithField("component", "store")) + store := store.New(cacheStores, cliConfig.IngressClass, cliConfig.ProcessClasslessIngressV1beta1, + cliConfig.ProcessClasslessKongConsumer, log.WithField("component", "store")) kong, err := controller.NewKongController(&controllerConfig, updateChannel, store) if err != nil { diff --git a/cli/ingress-controller/main_test.go b/cli/ingress-controller/main_test.go index 068ad3fce1..a04bd4d86f 100644 --- a/cli/ingress-controller/main_test.go +++ b/cli/ingress-controller/main_test.go @@ -77,7 +77,8 @@ func TestHandleSigterm(t *testing.T) { KubeClient: kubeClient, }, channels.NewRingChannel(1024), - store.New(store.CacheStores{}, conf.IngressClass, conf.SkipClasslessIngressV1beta1, logrus.New()), + store.New(store.CacheStores{}, conf.IngressClass, conf.ProcessClasslessIngressV1beta1, + conf.ProcessClasslessKongConsumer, logrus.New()), ) exitCh := make(chan int, 1) diff --git a/internal/ingress/controller/parser/parser_test.go b/internal/ingress/controller/parser/parser_test.go index 088e04db3f..6c120a9c16 100644 --- a/internal/ingress/controller/parser/parser_test.go +++ b/internal/ingress/controller/parser/parser_test.go @@ -197,7 +197,8 @@ func TestSecretConfigurationPlugin(t *testing.T) { Name: "foo", Namespace: "default", Annotations: map[string]string{ - "plugins.konghq.com": "foo-plugin", + "plugins.konghq.com": "foo-plugin", + "kubernetes.io/ingress.class": "kong", }, }, Spec: networking.IngressSpec{ @@ -226,7 +227,8 @@ func TestSecretConfigurationPlugin(t *testing.T) { Name: "bar", Namespace: "default", Annotations: map[string]string{ - "plugins.konghq.com": "bar-plugin", + "plugins.konghq.com": "bar-plugin", + "kubernetes.io/ingress.class": "kong", }, }, Spec: networking.IngressSpec{ @@ -842,6 +844,9 @@ func TestServiceClientCertificate(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Spec: networking.IngressSpec{ Rules: []networking.IngressRule{ @@ -921,6 +926,9 @@ func TestServiceClientCertificate(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Spec: networking.IngressSpec{ Rules: []networking.IngressRule{ @@ -989,6 +997,7 @@ func TestKongRouteAnnotations(t *testing.T) { Namespace: "default", Annotations: map[string]string{ "configuration.konghq.com/strip-path": "trUe", + "kubernetes.io/ingress.class": "kong", }, }, Spec: networking.IngressSpec{ @@ -1065,6 +1074,7 @@ func TestKongRouteAnnotations(t *testing.T) { Name: "bar", Namespace: "default", Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", "configuration.konghq.com/strip-path": "false", }, }, @@ -1143,6 +1153,7 @@ func TestKongRouteAnnotations(t *testing.T) { Name: "bar", Namespace: "default", Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", "konghq.com/https-redirect-status-code": "301", }, }, @@ -1222,6 +1233,7 @@ func TestKongRouteAnnotations(t *testing.T) { Name: "bar", Namespace: "default", Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", "konghq.com/https-redirect-status-code": "whoops", }, }, @@ -1300,7 +1312,8 @@ func TestKongRouteAnnotations(t *testing.T) { Name: "bar", Namespace: "default", Annotations: map[string]string{ - "konghq.com/preserve-host": "faLsE", + "konghq.com/preserve-host": "faLsE", + "kubernetes.io/ingress.class": "kong", }, }, Spec: networking.IngressSpec{ @@ -1378,7 +1391,8 @@ func TestKongRouteAnnotations(t *testing.T) { Name: "bar", Namespace: "default", Annotations: map[string]string{ - "konghq.com/preserve-host": "wiggle wiggle wiggle", + "kubernetes.io/ingress.class": "kong", + "konghq.com/preserve-host": "wiggle wiggle wiggle", }, }, Spec: networking.IngressSpec{ @@ -1456,7 +1470,8 @@ func TestKongRouteAnnotations(t *testing.T) { Name: "bar", Namespace: "default", Annotations: map[string]string{ - "konghq.com/regex-priority": "10", + "konghq.com/regex-priority": "10", + "kubernetes.io/ingress.class": "kong", }, }, Spec: networking.IngressSpec{ @@ -1534,7 +1549,8 @@ func TestKongRouteAnnotations(t *testing.T) { Name: "bar", Namespace: "default", Annotations: map[string]string{ - "konghq.com/regex-priority": "IAmAString", + "konghq.com/regex-priority": "IAmAString", + "kubernetes.io/ingress.class": "kong", }, }, Spec: networking.IngressSpec{ @@ -1606,15 +1622,17 @@ func TestKongRouteAnnotations(t *testing.T) { }) } -func TestKongSkipClasslessIngress(t *testing.T) { +func TestKongProcessClasslessIngress(t *testing.T) { assert := assert.New(t) t.Run("Kong classless ingress evaluated (true)", func(t *testing.T) { ingresses := []*networking.Ingress{ { ObjectMeta: metav1.ObjectMeta{ - Name: "bar", - Namespace: "default", - Annotations: map[string]string{}, + Name: "bar", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Spec: networking.IngressSpec{ Rules: []networking.IngressRule{ @@ -1648,9 +1666,9 @@ func TestKongSkipClasslessIngress(t *testing.T) { } store, err := store.NewFakeStore(store.FakeObjects{ - Ingresses: ingresses, - Services: services, - SkipClasslessIngress: false, + Ingresses: ingresses, + Services: services, + ProcessClasslessIngress: true, }) assert.Nil(err) parser := New(store, logrus.New()) @@ -1665,9 +1683,8 @@ func TestKongSkipClasslessIngress(t *testing.T) { ingresses := []*networking.Ingress{ { ObjectMeta: metav1.ObjectMeta{ - Name: "bar", - Namespace: "default", - Annotations: map[string]string{}, + Name: "bar", + Namespace: "default", }, Spec: networking.IngressSpec{ Rules: []networking.IngressRule{ @@ -1701,9 +1718,9 @@ func TestKongSkipClasslessIngress(t *testing.T) { } store, err := store.NewFakeStore(store.FakeObjects{ - Ingresses: ingresses, - Services: services, - SkipClasslessIngress: true, + Ingresses: ingresses, + Services: services, + ProcessClasslessIngress: false, }) assert.Nil(err) parser := New(store, logrus.New()) @@ -1851,6 +1868,9 @@ func TestKongServiceAnnotations(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "bar", Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Spec: networking.IngressSpec{ Rules: []networking.IngressRule{ @@ -1929,6 +1949,9 @@ func TestKongServiceAnnotations(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "bar", Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Spec: networking.IngressSpec{ Rules: []networking.IngressRule{ @@ -2016,7 +2039,8 @@ func TestKongServiceAnnotations(t *testing.T) { Name: "bar", Namespace: "default", Annotations: map[string]string{ - "konghq.com/methods": "POST,GET", + "konghq.com/methods": "POST,GET", + "kubernetes.io/ingress.class": "kong", }, }, Spec: networking.IngressSpec{ @@ -2097,6 +2121,9 @@ func TestDefaultBackend(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "ing-with-default-backend", Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Spec: networking.IngressSpec{ Backend: &networking.IngressBackend{ @@ -2140,6 +2167,9 @@ func TestDefaultBackend(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Spec: networking.IngressSpec{ Rules: []networking.IngressRule{ @@ -2206,6 +2236,9 @@ func TestParserSecret(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Spec: networking.IngressSpec{ TLS: []networking.IngressTLS{ @@ -2262,6 +2295,9 @@ func TestParserSecret(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Spec: networking.IngressSpec{ TLS: []networking.IngressTLS{ @@ -2276,6 +2312,9 @@ func TestParserSecret(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "bar", Namespace: "ns1", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Spec: networking.IngressSpec{ TLS: []networking.IngressTLS{ @@ -2351,6 +2390,9 @@ func TestParserSecret(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Spec: networking.IngressSpec{ TLS: []networking.IngressTLS{ @@ -2431,7 +2473,8 @@ func TestPluginAnnotations(t *testing.T) { Name: "foo", Namespace: "default", Annotations: map[string]string{ - "plugins.konghq.com": "foo-plugin", + "plugins.konghq.com": "foo-plugin", + "kubernetes.io/ingress.class": "kong", }, }, Spec: networking.IngressSpec{ @@ -2520,7 +2563,8 @@ func TestPluginAnnotations(t *testing.T) { Name: "foo", Namespace: "default", Annotations: map[string]string{ - "plugins.konghq.com": "foo-plugin", + "plugins.konghq.com": "foo-plugin", + "kubernetes.io/ingress.class": "kong", }, }, Spec: networking.IngressSpec{ @@ -2604,7 +2648,8 @@ func TestPluginAnnotations(t *testing.T) { Name: "foo", Namespace: "default", Annotations: map[string]string{ - "plugins.konghq.com": "foo-plugin", + "plugins.konghq.com": "foo-plugin", + "kubernetes.io/ingress.class": "kong", }, }, Spec: networking.IngressSpec{ @@ -2665,7 +2710,8 @@ func TestPluginAnnotations(t *testing.T) { Name: "foo", Namespace: "default", Annotations: map[string]string{ - "plugins.konghq.com": "does-not-exist", + "plugins.konghq.com": "does-not-exist", + "kubernetes.io/ingress.class": "kong", }, }, Spec: networking.IngressSpec{ @@ -2715,6 +2761,9 @@ func TestParseIngressRules(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "foo-namespace", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Spec: networking.IngressSpec{ Rules: []networking.IngressRule{ diff --git a/internal/ingress/store/fake_store.go b/internal/ingress/store/fake_store.go index 733f20ebd8..e124b776f9 100644 --- a/internal/ingress/store/fake_store.go +++ b/internal/ingress/store/fake_store.go @@ -26,17 +26,18 @@ func clusterResourceKeyFunc(obj interface{}) (string, error) { // FakeObjects can be used to populate a fake Store. type FakeObjects struct { - Ingresses []*networking.Ingress - TCPIngresses []*configurationv1beta1.TCPIngress - Services []*apiv1.Service - Endpoints []*apiv1.Endpoints - Secrets []*apiv1.Secret - KongPlugins []*configurationv1.KongPlugin - KongClusterPlugins []*configurationv1.KongClusterPlugin - KongIngresses []*configurationv1.KongIngress - KongConsumers []*configurationv1.KongConsumer - KongCredentials []*configurationv1.KongCredential - SkipClasslessIngress bool + Ingresses []*networking.Ingress + TCPIngresses []*configurationv1beta1.TCPIngress + Services []*apiv1.Service + Endpoints []*apiv1.Endpoints + Secrets []*apiv1.Secret + KongPlugins []*configurationv1.KongPlugin + KongClusterPlugins []*configurationv1.KongClusterPlugin + KongIngresses []*configurationv1.KongIngress + KongConsumers []*configurationv1.KongConsumer + KongCredentials []*configurationv1.KongCredential + ProcessClasslessIngress bool + ProcessClasslessKongConsumer bool KnativeIngresses []*knative.Ingress } @@ -124,15 +125,18 @@ func NewFakeStore( return nil, err } } - var ingressClassMatching annotations.ClassMatching - if objects.SkipClasslessIngress { - ingressClassMatching = annotations.ExactClassMatch + var ingressClassHandling annotations.ClassHandling + var kongConsumerClassHandling annotations.ClassHandling + if objects.ProcessClasslessIngress { + ingressClassHandling = annotations.ExactOrEmptyClassMatch } else { - ingressClassMatching = annotations.ExactOrEmptyClassMatch + ingressClassHandling = annotations.ExactClassMatch + } + if objects.ProcessClasslessKongConsumer { + kongConsumerClassHandling = annotations.ExactOrEmptyClassMatch + } else { + kongConsumerClassHandling = annotations.ExactClassMatch } - // TODO this is a placeholder for the eventual consumer flag - // for now it hard-codes the default - kongConsumerClassMatching := annotations.ExactOrEmptyClassMatch s = Store{ stores: CacheStores{ Ingress: ingressStore, diff --git a/internal/ingress/store/fake_store_test.go b/internal/ingress/store/fake_store_test.go index 80ec45a660..3c3ace4152 100644 --- a/internal/ingress/store/fake_store_test.go +++ b/internal/ingress/store/fake_store_test.go @@ -94,6 +94,9 @@ func TestFakeStoreIngress(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, Spec: networking.IngressSpec{ Rules: []networking.IngressRule{ @@ -320,6 +323,9 @@ func TestFakeStoreConsumer(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "kong", + }, }, }, } diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index 028ea888ce..71662af449 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -109,15 +109,21 @@ type CacheStores struct { } // New creates a new object store to be used in the ingress controller -func New(cs CacheStores, ingressClass string, skipClasslessIngress bool, logger logrus.FieldLogger) Storer { - var ingressClassMatching annotations.ClassMatching +func New(cs CacheStores, ingressClass string, processClasslessIngress bool, processClasslessKongConsumer bool, + logger logrus.FieldLogger) Storer { + var ingressClassHandling annotations.ClassHandling // TODO this is a placeholder for the eventual consumer flag // for now it hard-codes the default - kongConsumerClassMatching := annotations.ExactOrEmptyClassMatch - if skipClasslessIngress { - ingressClassMatching = annotations.ExactClassMatch + kongConsumerClassHandling := annotations.ExactOrEmptyClassMatch + if processClasslessIngress { + ingressClassHandling = annotations.ExactOrEmptyClassMatch } else { - ingressClassMatching = annotations.ExactOrEmptyClassMatch + ingressClassHandling = annotations.ExactClassMatch + } + if processClasslessKongConsumer { + kongConsumerClassHandling = annotations.ExactOrEmptyClassMatch + } else { + kongConsumerClassHandling = annotations.ExactClassMatch } return Store{ stores: cs, From 2d0d44bc59315060e5037250062d47f7a70f3d49 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Wed, 19 Aug 2020 05:43:16 -0700 Subject: [PATCH 42/47] pr: reconcile name changes --- internal/ingress/store/fake_store.go | 12 ++++++------ internal/ingress/store/store.go | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/ingress/store/fake_store.go b/internal/ingress/store/fake_store.go index e124b776f9..e5f71af543 100644 --- a/internal/ingress/store/fake_store.go +++ b/internal/ingress/store/fake_store.go @@ -125,17 +125,17 @@ func NewFakeStore( return nil, err } } - var ingressClassHandling annotations.ClassHandling - var kongConsumerClassHandling annotations.ClassHandling + var ingressClassMatching annotations.ClassMatching + var kongConsumerClassMatching annotations.ClassMatching if objects.ProcessClasslessIngress { - ingressClassHandling = annotations.ExactOrEmptyClassMatch + ingressClassMatching = annotations.ExactOrEmptyClassMatch } else { - ingressClassHandling = annotations.ExactClassMatch + ingressClassMatching = annotations.ExactClassMatch } if objects.ProcessClasslessKongConsumer { - kongConsumerClassHandling = annotations.ExactOrEmptyClassMatch + kongConsumerClassMatching = annotations.ExactOrEmptyClassMatch } else { - kongConsumerClassHandling = annotations.ExactClassMatch + kongConsumerClassMatching = annotations.ExactClassMatch } s = Store{ stores: CacheStores{ diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index 71662af449..bf8ddc014f 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -111,19 +111,19 @@ type CacheStores struct { // New creates a new object store to be used in the ingress controller func New(cs CacheStores, ingressClass string, processClasslessIngress bool, processClasslessKongConsumer bool, logger logrus.FieldLogger) Storer { - var ingressClassHandling annotations.ClassHandling + var ingressClassMatching annotations.ClassMatching // TODO this is a placeholder for the eventual consumer flag // for now it hard-codes the default - kongConsumerClassHandling := annotations.ExactOrEmptyClassMatch + kongConsumerClassMatching := annotations.ExactOrEmptyClassMatch if processClasslessIngress { - ingressClassHandling = annotations.ExactOrEmptyClassMatch + ingressClassMatching = annotations.ExactOrEmptyClassMatch } else { - ingressClassHandling = annotations.ExactClassMatch + ingressClassMatching = annotations.ExactClassMatch } if processClasslessKongConsumer { - kongConsumerClassHandling = annotations.ExactOrEmptyClassMatch + kongConsumerClassMatching = annotations.ExactOrEmptyClassMatch } else { - kongConsumerClassHandling = annotations.ExactClassMatch + kongConsumerClassMatching = annotations.ExactClassMatch } return Store{ stores: cs, From d933951c0057524fe1faa2cd5f6ee7f62f614fdc Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Wed, 19 Aug 2020 06:05:50 -0700 Subject: [PATCH 43/47] store: remove placeholder code --- internal/ingress/store/store.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/ingress/store/store.go b/internal/ingress/store/store.go index bf8ddc014f..27b641cebb 100644 --- a/internal/ingress/store/store.go +++ b/internal/ingress/store/store.go @@ -112,9 +112,7 @@ type CacheStores struct { func New(cs CacheStores, ingressClass string, processClasslessIngress bool, processClasslessKongConsumer bool, logger logrus.FieldLogger) Storer { var ingressClassMatching annotations.ClassMatching - // TODO this is a placeholder for the eventual consumer flag - // for now it hard-codes the default - kongConsumerClassMatching := annotations.ExactOrEmptyClassMatch + var kongConsumerClassMatching annotations.ClassMatching if processClasslessIngress { ingressClassMatching = annotations.ExactOrEmptyClassMatch } else { From 03fefa7521122922fe92a1608ad7388b3f6db7df Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Thu, 20 Aug 2020 14:32:35 -0700 Subject: [PATCH 44/47] test: check both default and non-default classes Use both the default ingress class and an arbitrary non-default ingress class when testing annotations. This confirms that annotation handling has no special cases for the default class. --- .../ingress/annotations/annotations_test.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/internal/ingress/annotations/annotations_test.go b/internal/ingress/annotations/annotations_test.go index 04a6759c26..655f49c860 100644 --- a/internal/ingress/annotations/annotations_test.go +++ b/internal/ingress/annotations/annotations_test.go @@ -33,15 +33,22 @@ func TestIngressClassValidatorFunc(t *testing.T) { isValid bool // the expected verdict }{ {"", false, "", true}, - {"", false, "kong", true}, - {"", true, "kong", false}, - {"kong", false, "kong", true}, - {"kong", true, "kong", true}, + {"", false, DefaultIngressClass, true}, + {"", true, DefaultIngressClass, false}, + {DefaultIngressClass, false, DefaultIngressClass, true}, + {DefaultIngressClass, true, DefaultIngressClass, true}, {"custom", false, "custom", true}, {"", false, "killer", true}, - {"custom", false, "kong", false}, - {"custom", true, "kong", false}, + {"custom", false, DefaultIngressClass, false}, + {"custom", true, DefaultIngressClass, false}, {"", true, "custom", false}, + {"", false, "kozel", true}, + {"", true, "kozel", false}, + {"kozel", false, "kozel", true}, + {"kozel", true, "kozel", true}, + {"", false, "killer", true}, + {"custom", false, "kozel", false}, + {"custom", true, "kozel", false}, } ing := &extensions.Ingress{ From 1bac620d4c51f6f21acb7123e1cf9c87a00a4fdd Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Mon, 24 Aug 2020 12:24:35 -0700 Subject: [PATCH 45/47] pr: convert annotation test to use matching values --- .../ingress/annotations/annotations_test.go | 53 +++++++++---------- 1 file changed, 24 insertions(+), 29 deletions(-) diff --git a/internal/ingress/annotations/annotations_test.go b/internal/ingress/annotations/annotations_test.go index 655f49c860..22c7e41de9 100644 --- a/internal/ingress/annotations/annotations_test.go +++ b/internal/ingress/annotations/annotations_test.go @@ -27,28 +27,28 @@ import ( func TestIngressClassValidatorFunc(t *testing.T) { tests := []struct { - ingress string // the class set on the Ingress resource - skipClasslessIngress bool // the "user" classless ingress flag value - controller string // the class set on the controller - isValid bool // the expected verdict + ingress string // the class set on the Ingress resource + classMatching ClassMatching // the "user" classless ingress flag value, translated to its match strategy + controller string // the class set on the controller + isValid bool // the expected verdict }{ - {"", false, "", true}, - {"", false, DefaultIngressClass, true}, - {"", true, DefaultIngressClass, false}, - {DefaultIngressClass, false, DefaultIngressClass, true}, - {DefaultIngressClass, true, DefaultIngressClass, true}, - {"custom", false, "custom", true}, - {"", false, "killer", true}, - {"custom", false, DefaultIngressClass, false}, - {"custom", true, DefaultIngressClass, false}, - {"", true, "custom", false}, - {"", false, "kozel", true}, - {"", true, "kozel", false}, - {"kozel", false, "kozel", true}, - {"kozel", true, "kozel", true}, - {"", false, "killer", true}, - {"custom", false, "kozel", false}, - {"custom", true, "kozel", false}, + {"", ExactOrEmptyClassMatch, "", true}, + {"", ExactOrEmptyClassMatch, DefaultIngressClass, true}, + {"", ExactClassMatch, DefaultIngressClass, false}, + {DefaultIngressClass, ExactOrEmptyClassMatch, DefaultIngressClass, true}, + {DefaultIngressClass, ExactClassMatch, DefaultIngressClass, true}, + {"custom", ExactOrEmptyClassMatch, "custom", true}, + {"", ExactOrEmptyClassMatch, "killer", true}, + {"custom", ExactOrEmptyClassMatch, DefaultIngressClass, false}, + {"custom", ExactClassMatch, DefaultIngressClass, false}, + {"", ExactOrEmptyClassMatch, "custom", true}, + {"", ExactOrEmptyClassMatch, "kozel", true}, + {"", ExactClassMatch, "kozel", false}, + {"kozel", ExactOrEmptyClassMatch, "kozel", true}, + {"kozel", ExactClassMatch, "kozel", true}, + {"", ExactOrEmptyClassMatch, "killer", true}, + {"custom", ExactOrEmptyClassMatch, "kozel", false}, + {"custom", ExactClassMatch, "kozel", false}, } ing := &extensions.Ingress{ @@ -63,15 +63,10 @@ func TestIngressClassValidatorFunc(t *testing.T) { for _, test := range tests { ing.Annotations[ingressClassKey] = test.ingress f := IngressClassValidatorFunc(test.controller) - var b bool - if test.skipClasslessIngress { - b = f(&ing.ObjectMeta, ExactClassMatch) - } else { - b = f(&ing.ObjectMeta, ExactOrEmptyClassMatch) - } - if b != test.isValid { - t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) + result := f(&ing.ObjectMeta, test.classMatching) + if result != test.isValid { + t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, result) } } } From e737aad882660fa4d655ea697e06c4e347584175 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Mon, 24 Aug 2020 14:06:52 -0700 Subject: [PATCH 46/47] pr: remove matching logic from fake store --- .../ingress/controller/parser/parser_test.go | 10 ++--- internal/ingress/store/fake_store.go | 38 ++++++------------- 2 files changed, 16 insertions(+), 32 deletions(-) diff --git a/internal/ingress/controller/parser/parser_test.go b/internal/ingress/controller/parser/parser_test.go index 6c120a9c16..c48ed28d5e 100644 --- a/internal/ingress/controller/parser/parser_test.go +++ b/internal/ingress/controller/parser/parser_test.go @@ -1666,9 +1666,8 @@ func TestKongProcessClasslessIngress(t *testing.T) { } store, err := store.NewFakeStore(store.FakeObjects{ - Ingresses: ingresses, - Services: services, - ProcessClasslessIngress: true, + Ingresses: ingresses, + Services: services, }) assert.Nil(err) parser := New(store, logrus.New()) @@ -1718,9 +1717,8 @@ func TestKongProcessClasslessIngress(t *testing.T) { } store, err := store.NewFakeStore(store.FakeObjects{ - Ingresses: ingresses, - Services: services, - ProcessClasslessIngress: false, + Ingresses: ingresses, + Services: services, }) assert.Nil(err) parser := New(store, logrus.New()) diff --git a/internal/ingress/store/fake_store.go b/internal/ingress/store/fake_store.go index e5f71af543..634f3fba78 100644 --- a/internal/ingress/store/fake_store.go +++ b/internal/ingress/store/fake_store.go @@ -26,18 +26,16 @@ func clusterResourceKeyFunc(obj interface{}) (string, error) { // FakeObjects can be used to populate a fake Store. type FakeObjects struct { - Ingresses []*networking.Ingress - TCPIngresses []*configurationv1beta1.TCPIngress - Services []*apiv1.Service - Endpoints []*apiv1.Endpoints - Secrets []*apiv1.Secret - KongPlugins []*configurationv1.KongPlugin - KongClusterPlugins []*configurationv1.KongClusterPlugin - KongIngresses []*configurationv1.KongIngress - KongConsumers []*configurationv1.KongConsumer - KongCredentials []*configurationv1.KongCredential - ProcessClasslessIngress bool - ProcessClasslessKongConsumer bool + Ingresses []*networking.Ingress + TCPIngresses []*configurationv1beta1.TCPIngress + Services []*apiv1.Service + Endpoints []*apiv1.Endpoints + Secrets []*apiv1.Secret + KongPlugins []*configurationv1.KongPlugin + KongClusterPlugins []*configurationv1.KongClusterPlugin + KongIngresses []*configurationv1.KongIngress + KongConsumers []*configurationv1.KongConsumer + KongCredentials []*configurationv1.KongCredential KnativeIngresses []*knative.Ingress } @@ -125,18 +123,6 @@ func NewFakeStore( return nil, err } } - var ingressClassMatching annotations.ClassMatching - var kongConsumerClassMatching annotations.ClassMatching - if objects.ProcessClasslessIngress { - ingressClassMatching = annotations.ExactOrEmptyClassMatch - } else { - ingressClassMatching = annotations.ExactClassMatch - } - if objects.ProcessClasslessKongConsumer { - kongConsumerClassMatching = annotations.ExactOrEmptyClassMatch - } else { - kongConsumerClassMatching = annotations.ExactClassMatch - } s = Store{ stores: CacheStores{ Ingress: ingressStore, @@ -154,8 +140,8 @@ func NewFakeStore( KnativeIngress: knativeIngressStore, }, isValidIngressClass: annotations.IngressClassValidatorFuncFromObjectMeta("kong"), - ingressClassMatching: ingressClassMatching, - kongConsumerClassMatching: kongConsumerClassMatching, + ingressClassMatching: annotations.ExactClassMatch, + kongConsumerClassMatching: annotations.ExactClassMatch, } return s, nil } From 1bac3a4e3b102159a62137db30d8bdf75ecdd432 Mon Sep 17 00:00:00 2001 From: Travis Raines Date: Wed, 26 Aug 2020 11:22:17 -0700 Subject: [PATCH 47/47] pr: remove superfluous test --- internal/ingress/annotations/annotations_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/ingress/annotations/annotations_test.go b/internal/ingress/annotations/annotations_test.go index 22c7e41de9..205dd62928 100644 --- a/internal/ingress/annotations/annotations_test.go +++ b/internal/ingress/annotations/annotations_test.go @@ -42,7 +42,6 @@ func TestIngressClassValidatorFunc(t *testing.T) { {"custom", ExactOrEmptyClassMatch, DefaultIngressClass, false}, {"custom", ExactClassMatch, DefaultIngressClass, false}, {"", ExactOrEmptyClassMatch, "custom", true}, - {"", ExactOrEmptyClassMatch, "kozel", true}, {"", ExactClassMatch, "kozel", false}, {"kozel", ExactOrEmptyClassMatch, "kozel", true}, {"kozel", ExactClassMatch, "kozel", true},