From b77ed2da878dc2bc33c962be29a62710a80c09d8 Mon Sep 17 00:00:00 2001 From: Nicolas MACHEREY Date: Fri, 28 Dec 2018 20:28:59 +0100 Subject: [PATCH 1/4] Added annotation validation methods for Custom Resources --- docs/custom-resources.md | 27 ++- internal/ingress/annotations/class/main.go | 74 ++++++- .../ingress/annotations/class/main_test.go | 138 +++++++++++++ internal/ingress/annotations/parser/main.go | 66 +++++++ .../ingress/annotations/parser/main_test.go | 149 ++++++++++++++ internal/ingress/controller/controller.go | 1 + internal/ingress/controller/kong.go | 1 + internal/ingress/controller/store/store.go | 187 +++++++++++++++++- 8 files changed, 634 insertions(+), 9 deletions(-) diff --git a/docs/custom-resources.md b/docs/custom-resources.md index aa9d0d59bc..2ecd1d945b 100644 --- a/docs/custom-resources.md +++ b/docs/custom-resources.md @@ -15,6 +15,30 @@ Following CRDs enables users to declaratively configure all aspects of Kong: - [**KongCredential**](#kongcredential): These resources map to credentials (key-auth, basic-auth, etc) that belong to consumers. +**IMPORTANT NOTE**: Kong Custom Resources are using the `kubernetes.io/ingress.class` annotation which defaults to `kong`. In +case you have more than one Kong Ingress deployed on your cluster, make sure you label all your ressources with the appropriate +annotation. For example: + +```yaml +apiVersion: configuration.konghq.com/v1 +kind: KongPlugin +metadata: + name: + namespace: + annotations: + kubernetes.io/ingress.class: + labels: + global: "true" # optional, please note the quotes around true + # configures the plugin Globally in Kong +consumerRef: # optional + # applies the plugin + # in on specific route and consumer +disabled: # optionally disable the plugin in Kong +config: + key: value +plugin: # like key-auth, rate-limiting etc +``` + ## KongPlugin This resource allows the configuration of @@ -74,7 +98,7 @@ apiVersion: configuration.konghq.com/v1 kind: KongPlugin metadata: name: http-svc-consumer-ratelimiting - namespace: default #this should match the namespace of the route or service you're adding it too. + namespace: default #this should match the namespace of the route or service you're adding it too. config: key: value plugin: my-plugin @@ -122,7 +146,6 @@ spec: servicePort: 80 ``` - ## KongIngress Ingress resource spec in Kubernetes can define routing policies diff --git a/internal/ingress/annotations/class/main.go b/internal/ingress/annotations/class/main.go index 6b5a6d06c1..3025481601 100644 --- a/internal/ingress/annotations/class/main.go +++ b/internal/ingress/annotations/class/main.go @@ -18,6 +18,9 @@ package class import ( "github.com/golang/glog" + consumerv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/consumer/v1" + credentialv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/credential/v1" + pluginv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/plugin/v1" extensions "k8s.io/api/extensions/v1beta1" ) @@ -44,7 +47,76 @@ var ( func IsValid(ing *extensions.Ingress) bool { ingress, ok := ing.GetAnnotations()[IngressKey] if !ok { - glog.V(3).Infof("annotation %v is not present in ingress %v/%v", IngressKey, ing.Namespace, ing.Name) + glog.V(3).Infof("annotation %v is not present in object %v/%v", IngressKey, ing.Namespace, ing.Name) + } + + // we have 2 valid combinations + // 1 - ingress with default class | blank annotation on ingress + // 2 - ingress with specific class | same annotation on ingress + // + // and 2 invalid combinations + // 3 - ingress with default class | fixed annotation on ingress + // 4 - ingress with specific class | different annotation on ingress + if ingress == "" && IngressClass == DefaultClass { + return true + } + + return ingress == IngressClass +} + +// IsValid returns true if the given KongPlugin either doesn't specify +// the ingress.class annotation, or it's set to the configured in the +// ingress controller. +func IsValidPlugin(plugin *pluginv1.KongPlugin) bool { + ingress, ok := plugin.GetAnnotations()[IngressKey] + if !ok { + glog.V(3).Infof("annotation %v is not present in plugin %v/%v", IngressKey, plugin.Namespace, plugin.Name) + } + + // we have 2 valid combinations + // 1 - ingress with default class | blank annotation on ingress + // 2 - ingress with specific class | same annotation on ingress + // + // and 2 invalid combinations + // 3 - ingress with default class | fixed annotation on ingress + // 4 - ingress with specific class | different annotation on ingress + if ingress == "" && IngressClass == DefaultClass { + return true + } + + return ingress == IngressClass +} + +// IsValid returns true if the given KongConsumer either doesn't specify +// the ingress.class annotation, or it's set to the configured in the +// ingress controller. +func IsValidConsumer(consumer *consumerv1.KongConsumer) bool { + ingress, ok := consumer.GetAnnotations()[IngressKey] + if !ok { + glog.V(3).Infof("annotation %v is not present in consumer %v/%v", IngressKey, consumer.Namespace, consumer.Name) + } + + // we have 2 valid combinations + // 1 - ingress with default class | blank annotation on ingress + // 2 - ingress with specific class | same annotation on ingress + // + // and 2 invalid combinations + // 3 - ingress with default class | fixed annotation on ingress + // 4 - ingress with specific class | different annotation on ingress + if ingress == "" && IngressClass == DefaultClass { + return true + } + + return ingress == IngressClass +} + +// IsValid returns true if the given KongCredential either doesn't specify +// the ingress.class annotation, or it's set to the configured in the +// ingress controller. +func IsValidCredential(credential *credentialv1.KongCredential) bool { + ingress, ok := credential.GetAnnotations()[IngressKey] + if !ok { + glog.V(3).Infof("annotation %v is not present in credential %v/%v", IngressKey, credential.Namespace, credential.Name) } // we have 2 valid combinations diff --git a/internal/ingress/annotations/class/main_test.go b/internal/ingress/annotations/class/main_test.go index 8b85e31920..1e384f40d2 100644 --- a/internal/ingress/annotations/class/main_test.go +++ b/internal/ingress/annotations/class/main_test.go @@ -19,6 +19,9 @@ package class import ( "testing" + consumerv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/consumer/v1" + credentialv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/credential/v1" + pluginv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/plugin/v1" api "k8s.io/api/core/v1" extensions "k8s.io/api/extensions/v1beta1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -68,3 +71,138 @@ func TestIsValidClass(t *testing.T) { } } } + +func TestIsValidPlugin(t *testing.T) { + dc := DefaultClass + ic := IngressClass + // restore original values after the tests + defer func() { + DefaultClass = dc + IngressClass = ic + }() + + tests := []struct { + ingress string + controller string + defClass string + isValid bool + }{ + {"", "", "nginx", true}, + {"", "nginx", "nginx", true}, + {"nginx", "nginx", "nginx", true}, + {"custom", "custom", "nginx", true}, + {"", "killer", "nginx", false}, + {"custom", "nginx", "nginx", false}, + } + + plugin := &pluginv1.KongPlugin{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "foo", + Namespace: api.NamespaceDefault, + }, + } + + data := map[string]string{} + plugin.SetAnnotations(data) + for _, test := range tests { + plugin.Annotations[IngressKey] = test.ingress + + IngressClass = test.controller + DefaultClass = test.defClass + + b := IsValidPlugin(plugin) + if b != test.isValid { + t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) + } + } +} + +func TestIsValidConsumer(t *testing.T) { + dc := DefaultClass + ic := IngressClass + // restore original values after the tests + defer func() { + DefaultClass = dc + IngressClass = ic + }() + + tests := []struct { + ingress string + controller string + defClass string + isValid bool + }{ + {"", "", "nginx", true}, + {"", "nginx", "nginx", true}, + {"nginx", "nginx", "nginx", true}, + {"custom", "custom", "nginx", true}, + {"", "killer", "nginx", false}, + {"custom", "nginx", "nginx", false}, + } + + consumer := &consumerv1.KongConsumer{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "foo", + Namespace: api.NamespaceDefault, + }, + } + + data := map[string]string{} + consumer.SetAnnotations(data) + for _, test := range tests { + consumer.Annotations[IngressKey] = test.ingress + + IngressClass = test.controller + DefaultClass = test.defClass + + b := IsValidConsumer(consumer) + if b != test.isValid { + t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) + } + } +} + +func TestIsValidCredential(t *testing.T) { + dc := DefaultClass + ic := IngressClass + // restore original values after the tests + defer func() { + DefaultClass = dc + IngressClass = ic + }() + + tests := []struct { + ingress string + controller string + defClass string + isValid bool + }{ + {"", "", "nginx", true}, + {"", "nginx", "nginx", true}, + {"nginx", "nginx", "nginx", true}, + {"custom", "custom", "nginx", true}, + {"", "killer", "nginx", false}, + {"custom", "nginx", "nginx", false}, + } + + credential := &credentialv1.KongCredential{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "foo", + Namespace: api.NamespaceDefault, + }, + } + + data := map[string]string{} + credential.SetAnnotations(data) + for _, test := range tests { + credential.Annotations[IngressKey] = test.ingress + + IngressClass = test.controller + DefaultClass = test.defClass + + b := IsValidCredential(credential) + if b != test.isValid { + t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) + } + } +} diff --git a/internal/ingress/annotations/parser/main.go b/internal/ingress/annotations/parser/main.go index 084a01700c..4837795873 100644 --- a/internal/ingress/annotations/parser/main.go +++ b/internal/ingress/annotations/parser/main.go @@ -20,6 +20,9 @@ import ( "fmt" "strconv" + consumerv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/consumer/v1" + credentialv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/credential/v1" + pluginv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/plugin/v1" extensions "k8s.io/api/extensions/v1beta1" "github.com/kong/kubernetes-ingress-controller/internal/ingress/errors" @@ -80,6 +83,39 @@ func checkAnnotation(name string, ing *extensions.Ingress) error { return nil } +func checkAnnotationPlugin(name string, plugin *pluginv1.KongPlugin) error { + if plugin == nil || len(plugin.GetAnnotations()) == 0 { + return errors.ErrMissingAnnotations + } + if name == "" { + return errors.ErrInvalidAnnotationName + } + + return nil +} + +func checkAnnotationCredential(name string, credential *credentialv1.KongCredential) error { + if credential == nil || len(credential.GetAnnotations()) == 0 { + return errors.ErrMissingAnnotations + } + if name == "" { + return errors.ErrInvalidAnnotationName + } + + return nil +} + +func checkAnnotationConsumer(name string, consumer *consumerv1.KongConsumer) error { + if consumer == nil || len(consumer.GetAnnotations()) == 0 { + return errors.ErrMissingAnnotations + } + if name == "" { + return errors.ErrInvalidAnnotationName + } + + return nil +} + // GetBoolAnnotation extracts a boolean from an Ingress annotation func GetBoolAnnotation(name string, ing *extensions.Ingress) (bool, error) { v := GetAnnotationWithPrefix(name) @@ -100,6 +136,36 @@ func GetStringAnnotation(name string, ing *extensions.Ingress) (string, error) { return ingAnnotations(ing.GetAnnotations()).parseString(v) } +// GetStringAnnotationPlugin extracts a string from an Ingress annotation +func GetStringAnnotationPlugin(name string, plugin *pluginv1.KongPlugin) (string, error) { + v := GetAnnotationWithPrefix(name) + err := checkAnnotationPlugin(v, plugin) + if err != nil { + return "", err + } + return ingAnnotations(plugin.GetAnnotations()).parseString(v) +} + +// GetStringAnnotationCredential extracts a string from an Ingress annotation +func GetStringAnnotationCredential(name string, credential *credentialv1.KongCredential) (string, error) { + v := GetAnnotationWithPrefix(name) + err := checkAnnotationCredential(v, credential) + if err != nil { + return "", err + } + return ingAnnotations(credential.GetAnnotations()).parseString(v) +} + +// GetStringAnnotationConsumer extracts a string from an Ingress annotation +func GetStringAnnotationConsumer(name string, consumer *consumerv1.KongConsumer) (string, error) { + v := GetAnnotationWithPrefix(name) + err := checkAnnotationConsumer(v, consumer) + if err != nil { + return "", err + } + return ingAnnotations(consumer.GetAnnotations()).parseString(v) +} + // GetIntAnnotation extracts an int from an Ingress annotation func GetIntAnnotation(name string, ing *extensions.Ingress) (int, error) { v := GetAnnotationWithPrefix(name) diff --git a/internal/ingress/annotations/parser/main_test.go b/internal/ingress/annotations/parser/main_test.go index f65fbdbad2..e68966fbae 100644 --- a/internal/ingress/annotations/parser/main_test.go +++ b/internal/ingress/annotations/parser/main_test.go @@ -19,6 +19,9 @@ package parser import ( "testing" + consumerv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/consumer/v1" + credentialv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/credential/v1" + pluginv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/plugin/v1" api "k8s.io/api/core/v1" extensions "k8s.io/api/extensions/v1beta1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -34,6 +37,32 @@ func buildIngress() *extensions.Ingress { } } +func buildPlugin() *pluginv1.KongPlugin { + return &pluginv1.KongPlugin{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "foo", + Namespace: api.NamespaceDefault, + }, + } +} + +func buildCredential() *credentialv1.KongCredential { + return &credentialv1.KongCredential{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "foo", + Namespace: api.NamespaceDefault, + }, + } +} + +func buildConsumer() *consumerv1.KongConsumer { + return &consumerv1.KongConsumer{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "foo", + Namespace: api.NamespaceDefault, + }, + } +} func TestGetBoolAnnotation(t *testing.T) { ing := buildIngress() @@ -114,6 +143,126 @@ func TestGetStringAnnotation(t *testing.T) { } } +func TestGetStringAnnotationPlugin(t *testing.T) { + res := buildPlugin() + + _, err := GetStringAnnotationPlugin("", nil) + if err == nil { + t.Errorf("expected error but retuned nil") + } + + tests := []struct { + name string + field string + value string + exp string + expErr bool + }{ + {"valid - A", "string", "A", "A", false}, + {"valid - B", "string", "B", "B", false}, + } + + data := map[string]string{} + res.SetAnnotations(data) + + for _, test := range tests { + data[GetAnnotationWithPrefix(test.field)] = test.value + + s, err := GetStringAnnotationPlugin(test.field, res) + if test.expErr { + if err == nil { + t.Errorf("%v: expected error but retuned nil", test.name) + } + continue + } + if s != test.exp { + t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.name, test.exp, s) + } + + delete(data, test.field) + } +} + +func TestGetStringAnnotationCredential(t *testing.T) { + res := buildCredential() + + _, err := GetStringAnnotationCredential("", nil) + if err == nil { + t.Errorf("expected error but retuned nil") + } + + tests := []struct { + name string + field string + value string + exp string + expErr bool + }{ + {"valid - A", "string", "A", "A", false}, + {"valid - B", "string", "B", "B", false}, + } + + data := map[string]string{} + res.SetAnnotations(data) + + for _, test := range tests { + data[GetAnnotationWithPrefix(test.field)] = test.value + + s, err := GetStringAnnotationCredential(test.field, res) + if test.expErr { + if err == nil { + t.Errorf("%v: expected error but retuned nil", test.name) + } + continue + } + if s != test.exp { + t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.name, test.exp, s) + } + + delete(data, test.field) + } +} + +func TestGetStringAnnotationConsumer(t *testing.T) { + res := buildConsumer() + + _, err := GetStringAnnotationConsumer("", nil) + if err == nil { + t.Errorf("expected error but retuned nil") + } + + tests := []struct { + name string + field string + value string + exp string + expErr bool + }{ + {"valid - A", "string", "A", "A", false}, + {"valid - B", "string", "B", "B", false}, + } + + data := map[string]string{} + res.SetAnnotations(data) + + for _, test := range tests { + data[GetAnnotationWithPrefix(test.field)] = test.value + + s, err := GetStringAnnotationConsumer(test.field, res) + if test.expErr { + if err == nil { + t.Errorf("%v: expected error but retuned nil", test.name) + } + continue + } + if s != test.exp { + t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.name, test.exp, s) + } + + delete(data, test.field) + } +} + func TestGetIntAnnotation(t *testing.T) { ing := buildIngress() diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 3da1f95bd1..aa3771f650 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -41,6 +41,7 @@ const ( rootLocation = "/" ) +// Kong Represents a Kong client and connexion information type Kong struct { URL string // Headers are injected into every request to Kong's Admin API diff --git a/internal/ingress/controller/kong.go b/internal/ingress/controller/kong.go index 143abf2e94..89fea4635b 100644 --- a/internal/ingress/controller/kong.go +++ b/internal/ingress/controller/kong.go @@ -125,6 +125,7 @@ func (n *NGINXController) syncGlobalPlugins() error { if name == "" { continue } + if _, ok := targetPluginMap[name]; ok { glog.Error("Multiple KongPlugin definitions found with 'global' annotation for :", name, ", the plugin will not be applied") diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index 55d877be25..9dffd91d2a 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -358,6 +358,178 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, store.informers.Secret.AddEventHandler(secrEventHandler) store.informers.Service.AddEventHandler(serviceEventHandler) + pluginEventHandler := cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj interface{}) { + plugin := obj.(*pluginv1.KongPlugin) + if !class.IsValidPlugin(plugin) { + a, _ := parser.GetStringAnnotationPlugin(class.IngressKey, plugin) + glog.Infof("ignoring add for plugin %v based on annotation %v with value %v", plugin.Name, class.IngressKey, a) + return + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: obj, + } + }, + DeleteFunc: func(obj interface{}) { + plugin, ok := obj.(*pluginv1.KongPlugin) + if !ok { + // If we reached here it means the ingress was deleted but its final state is unrecorded. + tombstone, ok := obj.(cache.DeletedFinalStateUnknown) + if !ok { + glog.Errorf("couldn't get object from tombstone %#v", obj) + return + } + plugin, ok = tombstone.Obj.(*pluginv1.KongPlugin) + if !ok { + glog.Errorf("Tombstone contained object that is not a KongPlugin: %#v", obj) + return + } + } + if !class.IsValidPlugin(plugin) { + glog.Infof("ignoring delete for plugin %v based on annotation %v", plugin.Name, class.IngressKey) + return + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: obj, + } + }, + UpdateFunc: func(old, cur interface{}) { + oldPlugin := old.(*pluginv1.KongPlugin) + curPlugin := cur.(*pluginv1.KongPlugin) + validOld := class.IsValidPlugin(oldPlugin) + validCur := class.IsValidPlugin(curPlugin) + + if !validOld && validCur { + glog.Infof("creating plugin %v based on annotation %v", curPlugin.Name, class.IngressKey) + } else if validOld && !validCur { + glog.Infof("removing plugin %v based on annotation %v", curPlugin.Name, class.IngressKey) + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: cur, + } + }, + } + + credentialEventHandler := cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj interface{}) { + credential := obj.(*credentialv1.KongCredential) + if !class.IsValidCredential(credential) { + a, _ := parser.GetStringAnnotationCredential(class.IngressKey, credential) + glog.Infof("ignoring add for credential %v based on annotation %v with value %v", credential.Name, class.IngressKey, a) + return + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: obj, + } + }, + DeleteFunc: func(obj interface{}) { + credential, ok := obj.(*credentialv1.KongCredential) + if !ok { + // If we reached here it means the ingress was deleted but its final state is unrecorded. + tombstone, ok := obj.(cache.DeletedFinalStateUnknown) + if !ok { + glog.Errorf("couldn't get object from tombstone %#v", obj) + return + } + credential, ok = tombstone.Obj.(*credentialv1.KongCredential) + if !ok { + glog.Errorf("Tombstone contained object that is not a KongCredential: %#v", obj) + return + } + } + if !class.IsValidCredential(credential) { + glog.Infof("ignoring delete for credential %v based on annotation %v", credential.Name, class.IngressKey) + return + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: obj, + } + }, + UpdateFunc: func(old, cur interface{}) { + oldCredential := old.(*credentialv1.KongCredential) + curCredential := cur.(*credentialv1.KongCredential) + validOld := class.IsValidCredential(oldCredential) + validCur := class.IsValidCredential(curCredential) + if !validOld && validCur { + glog.Infof("creating credential %v based on annotation %v", curCredential.Name, class.IngressKey) + } else if validOld && !validCur { + glog.Infof("removing credential %v based on annotation %v", curCredential.Name, class.IngressKey) + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: cur, + } + }, + } + + consumerEventHandler := cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj interface{}) { + consumer := obj.(*consumerv1.KongConsumer) + if !class.IsValidConsumer(consumer) { + a, _ := parser.GetStringAnnotationConsumer(class.IngressKey, consumer) + glog.Infof("ignoring add for consumer %v based on annotation %v with value %v", consumer.Name, class.IngressKey, a) + return + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: obj, + } + }, + DeleteFunc: func(obj interface{}) { + consumer, ok := obj.(*consumerv1.KongConsumer) + if !ok { + // If we reached here it means the ingress was deleted but its final state is unrecorded. + tombstone, ok := obj.(cache.DeletedFinalStateUnknown) + if !ok { + glog.Errorf("couldn't get object from tombstone %#v", obj) + return + } + consumer, ok = tombstone.Obj.(*consumerv1.KongConsumer) + if !ok { + glog.Errorf("Tombstone contained object that is not a KongConsumer: %#v", obj) + return + } + } + if !class.IsValidConsumer(consumer) { + glog.Infof("ignoring delete for consumer %v based on annotation %v", consumer.Name, class.IngressKey) + return + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: obj, + } + }, + UpdateFunc: func(old, cur interface{}) { + oldConsumer := old.(*consumerv1.KongConsumer) + curConsumer := cur.(*consumerv1.KongConsumer) + validOld := class.IsValidConsumer(oldConsumer) + validCur := class.IsValidConsumer(curConsumer) + if !validOld && validCur { + glog.Infof("creating consumer %v based on annotation %v", curConsumer.Name, class.IngressKey) + } else if validOld && !validCur { + glog.Infof("removing consumer %v based on annotation %v", curConsumer.Name, class.IngressKey) + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: cur, + } + }, + } + crdEventHandler := cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { updateCh.In() <- Event{ @@ -384,21 +556,21 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, store.informers.Kong.Plugin = pluginFactory.Configuration().V1().KongPlugins().Informer() store.listers.Kong.Plugin = store.informers.Kong.Plugin.GetStore() - store.informers.Kong.Plugin.AddEventHandler(crdEventHandler) + store.informers.Kong.Plugin.AddEventHandler(pluginEventHandler) consumerClient, _ := consumerclientv1.NewForConfig(clientConf) consumerFactory := consumerinformer.NewFilteredSharedInformerFactory(consumerClient, resyncPeriod, namespace, func(*metav1.ListOptions) {}) store.informers.Kong.Consumer = consumerFactory.Configuration().V1().KongConsumers().Informer() store.listers.Kong.Consumer = store.informers.Kong.Consumer.GetStore() - store.informers.Kong.Consumer.AddEventHandler(crdEventHandler) + store.informers.Kong.Consumer.AddEventHandler(consumerEventHandler) credClient, _ := credentialclientv1.NewForConfig(clientConf) credentialFactory := credentialinformer.NewFilteredSharedInformerFactory(credClient, resyncPeriod, namespace, func(*metav1.ListOptions) {}) store.informers.Kong.Credential = credentialFactory.Configuration().V1().KongCredentials().Informer() store.listers.Kong.Credential = store.informers.Kong.Credential.GetStore() - store.informers.Kong.Credential.AddEventHandler(crdEventHandler) + store.informers.Kong.Credential.AddEventHandler(credentialEventHandler) confClient, _ := configurationclientv1.NewForConfig(clientConf) configFactory := configurationinformer.NewFilteredSharedInformerFactory(confClient, resyncPeriod, namespace, func(*metav1.ListOptions) {}) @@ -491,7 +663,8 @@ func (s k8sStore) GetKongConsumer(namespace, name string) (*consumerv1.KongConsu func (s k8sStore) ListKongConsumers() []*consumerv1.KongConsumer { var consumers []*consumerv1.KongConsumer for _, item := range s.listers.Kong.Consumer.List() { - if c, ok := item.(*consumerv1.KongConsumer); ok { + c, ok := item.(*consumerv1.KongConsumer) + if ok && class.IsValidConsumer(c) { consumers = append(consumers, c) } } @@ -502,7 +675,8 @@ func (s k8sStore) ListKongConsumers() []*consumerv1.KongConsumer { func (s k8sStore) ListKongCredentials() []*credentialv1.KongCredential { var credentials []*credentialv1.KongCredential for _, item := range s.listers.Kong.Credential.List() { - if c, ok := item.(*credentialv1.KongCredential); ok { + c, ok := item.(*credentialv1.KongCredential) + if ok && class.IsValidCredential(c) { credentials = append(credentials, c) } } @@ -521,7 +695,8 @@ func (s k8sStore) ListGlobalKongPlugins() ([]*pluginv1.KongPlugin, error) { err = cache.ListAll(s.listers.Kong.Plugin, labels.NewSelector().Add(*req), func(ob interface{}) { - if p, ok := ob.(*pluginv1.KongPlugin); ok { + p, ok := ob.(*pluginv1.KongPlugin) + if ok && class.IsValidPlugin(p) { plugins = append(plugins, p) } }) From 12f17d29f0a752fdc018a259569c85bea8a9729f Mon Sep 17 00:00:00 2001 From: Nicolas MACHEREY Date: Thu, 3 Jan 2019 06:01:46 +0100 Subject: [PATCH 2/4] Removed duplicated IsValid copde --- internal/ingress/annotations/class/main.go | 80 +--------------- .../ingress/annotations/class/main_test.go | 8 +- internal/ingress/annotations/parser/main.go | 91 ++++--------------- .../ingress/annotations/parser/main_test.go | 14 +-- internal/ingress/controller/store/store.go | 48 +++++----- 5 files changed, 59 insertions(+), 182 deletions(-) diff --git a/internal/ingress/annotations/class/main.go b/internal/ingress/annotations/class/main.go index 3025481601..8184ff5145 100644 --- a/internal/ingress/annotations/class/main.go +++ b/internal/ingress/annotations/class/main.go @@ -18,10 +18,7 @@ package class import ( "github.com/golang/glog" - consumerv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/consumer/v1" - credentialv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/credential/v1" - pluginv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/plugin/v1" - extensions "k8s.io/api/extensions/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) const ( @@ -41,82 +38,13 @@ var ( IngressClass = "nginx" ) -// IsValid returns true if the given Ingress either doesn't specify -// the ingress.class annotation, or it's set to the configured in the -// ingress controller. -func IsValid(ing *extensions.Ingress) bool { - ingress, ok := ing.GetAnnotations()[IngressKey] - if !ok { - glog.V(3).Infof("annotation %v is not present in object %v/%v", IngressKey, ing.Namespace, ing.Name) - } - - // we have 2 valid combinations - // 1 - ingress with default class | blank annotation on ingress - // 2 - ingress with specific class | same annotation on ingress - // - // and 2 invalid combinations - // 3 - ingress with default class | fixed annotation on ingress - // 4 - ingress with specific class | different annotation on ingress - if ingress == "" && IngressClass == DefaultClass { - return true - } - - return ingress == IngressClass -} - -// IsValid returns true if the given KongPlugin either doesn't specify -// the ingress.class annotation, or it's set to the configured in the -// ingress controller. -func IsValidPlugin(plugin *pluginv1.KongPlugin) bool { - ingress, ok := plugin.GetAnnotations()[IngressKey] - if !ok { - glog.V(3).Infof("annotation %v is not present in plugin %v/%v", IngressKey, plugin.Namespace, plugin.Name) - } - - // we have 2 valid combinations - // 1 - ingress with default class | blank annotation on ingress - // 2 - ingress with specific class | same annotation on ingress - // - // and 2 invalid combinations - // 3 - ingress with default class | fixed annotation on ingress - // 4 - ingress with specific class | different annotation on ingress - if ingress == "" && IngressClass == DefaultClass { - return true - } - - return ingress == IngressClass -} - // IsValid returns true if the given KongConsumer either doesn't specify // the ingress.class annotation, or it's set to the configured in the // ingress controller. -func IsValidConsumer(consumer *consumerv1.KongConsumer) bool { - ingress, ok := consumer.GetAnnotations()[IngressKey] - if !ok { - glog.V(3).Infof("annotation %v is not present in consumer %v/%v", IngressKey, consumer.Namespace, consumer.Name) - } - - // we have 2 valid combinations - // 1 - ingress with default class | blank annotation on ingress - // 2 - ingress with specific class | same annotation on ingress - // - // and 2 invalid combinations - // 3 - ingress with default class | fixed annotation on ingress - // 4 - ingress with specific class | different annotation on ingress - if ingress == "" && IngressClass == DefaultClass { - return true - } - - return ingress == IngressClass -} - -// IsValid returns true if the given KongCredential either doesn't specify -// the ingress.class annotation, or it's set to the configured in the -// ingress controller. -func IsValidCredential(credential *credentialv1.KongCredential) bool { - ingress, ok := credential.GetAnnotations()[IngressKey] +func IsValid(objectMeta *metav1.ObjectMeta) bool { + ingress, ok := objectMeta.GetAnnotations()[IngressKey] if !ok { - glog.V(3).Infof("annotation %v is not present in credential %v/%v", IngressKey, credential.Namespace, credential.Name) + glog.V(3).Infof("annotation %v is not present in custom resources %v/%v", IngressKey, objectMeta.Namespace, objectMeta.Name) } // we have 2 valid combinations diff --git a/internal/ingress/annotations/class/main_test.go b/internal/ingress/annotations/class/main_test.go index 1e384f40d2..3ec99cbdc7 100644 --- a/internal/ingress/annotations/class/main_test.go +++ b/internal/ingress/annotations/class/main_test.go @@ -65,7 +65,7 @@ func TestIsValidClass(t *testing.T) { IngressClass = test.controller DefaultClass = test.defClass - b := IsValid(ing) + b := IsValid(&ing.ObjectMeta) if b != test.isValid { t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) } @@ -110,7 +110,7 @@ func TestIsValidPlugin(t *testing.T) { IngressClass = test.controller DefaultClass = test.defClass - b := IsValidPlugin(plugin) + b := IsValid(&plugin.ObjectMeta) if b != test.isValid { t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) } @@ -155,7 +155,7 @@ func TestIsValidConsumer(t *testing.T) { IngressClass = test.controller DefaultClass = test.defClass - b := IsValidConsumer(consumer) + b := IsValid(&consumer.ObjectMeta) if b != test.isValid { t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) } @@ -200,7 +200,7 @@ func TestIsValidCredential(t *testing.T) { IngressClass = test.controller DefaultClass = test.defClass - b := IsValidCredential(credential) + b := IsValid(&credential.ObjectMeta) if b != test.isValid { t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) } diff --git a/internal/ingress/annotations/parser/main.go b/internal/ingress/annotations/parser/main.go index 4837795873..02c3c3455d 100644 --- a/internal/ingress/annotations/parser/main.go +++ b/internal/ingress/annotations/parser/main.go @@ -20,10 +20,8 @@ import ( "fmt" "strconv" - consumerv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/consumer/v1" - credentialv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/credential/v1" - pluginv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/plugin/v1" extensions "k8s.io/api/extensions/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/kong/kubernetes-ingress-controller/internal/ingress/errors" ) @@ -72,41 +70,8 @@ func (a ingAnnotations) parseInt(name string) (int, error) { return 0, errors.ErrMissingAnnotations } -func checkAnnotation(name string, ing *extensions.Ingress) error { - if ing == nil || len(ing.GetAnnotations()) == 0 { - return errors.ErrMissingAnnotations - } - if name == "" { - return errors.ErrInvalidAnnotationName - } - - return nil -} - -func checkAnnotationPlugin(name string, plugin *pluginv1.KongPlugin) error { - if plugin == nil || len(plugin.GetAnnotations()) == 0 { - return errors.ErrMissingAnnotations - } - if name == "" { - return errors.ErrInvalidAnnotationName - } - - return nil -} - -func checkAnnotationCredential(name string, credential *credentialv1.KongCredential) error { - if credential == nil || len(credential.GetAnnotations()) == 0 { - return errors.ErrMissingAnnotations - } - if name == "" { - return errors.ErrInvalidAnnotationName - } - - return nil -} - -func checkAnnotationConsumer(name string, consumer *consumerv1.KongConsumer) error { - if consumer == nil || len(consumer.GetAnnotations()) == 0 { +func checkAnnotation(name string, objectMeta *metav1.ObjectMeta) error { + if objectMeta == nil || len(objectMeta.GetAnnotations()) == 0 { return errors.ErrMissingAnnotations } if name == "" { @@ -119,57 +84,41 @@ func checkAnnotationConsumer(name string, consumer *consumerv1.KongConsumer) err // GetBoolAnnotation extracts a boolean from an Ingress annotation func GetBoolAnnotation(name string, ing *extensions.Ingress) (bool, error) { v := GetAnnotationWithPrefix(name) - err := checkAnnotation(v, ing) - if err != nil { - return false, err + if ing == nil { + return false, errors.ErrMissingAnnotations } - return ingAnnotations(ing.GetAnnotations()).parseBool(v) -} -// GetStringAnnotation extracts a string from an Ingress annotation -func GetStringAnnotation(name string, ing *extensions.Ingress) (string, error) { - v := GetAnnotationWithPrefix(name) - err := checkAnnotation(v, ing) - if err != nil { - return "", err - } - return ingAnnotations(ing.GetAnnotations()).parseString(v) -} + err := checkAnnotation(v, &ing.ObjectMeta) -// GetStringAnnotationPlugin extracts a string from an Ingress annotation -func GetStringAnnotationPlugin(name string, plugin *pluginv1.KongPlugin) (string, error) { - v := GetAnnotationWithPrefix(name) - err := checkAnnotationPlugin(v, plugin) if err != nil { - return "", err + return false, err } - return ingAnnotations(plugin.GetAnnotations()).parseString(v) + + return ingAnnotations(ing.GetAnnotations()).parseBool(v) } -// GetStringAnnotationCredential extracts a string from an Ingress annotation -func GetStringAnnotationCredential(name string, credential *credentialv1.KongCredential) (string, error) { +// GetStringAnnotation extracts a string from an Ingress annotation +func GetStringAnnotation(name string, objectMeta *metav1.ObjectMeta) (string, error) { v := GetAnnotationWithPrefix(name) - err := checkAnnotationCredential(v, credential) - if err != nil { - return "", err + if objectMeta == nil { + return "", errors.ErrMissingAnnotations } - return ingAnnotations(credential.GetAnnotations()).parseString(v) -} -// GetStringAnnotationConsumer extracts a string from an Ingress annotation -func GetStringAnnotationConsumer(name string, consumer *consumerv1.KongConsumer) (string, error) { - v := GetAnnotationWithPrefix(name) - err := checkAnnotationConsumer(v, consumer) + err := checkAnnotation(v, objectMeta) if err != nil { return "", err } - return ingAnnotations(consumer.GetAnnotations()).parseString(v) + return ingAnnotations(objectMeta.GetAnnotations()).parseString(v) } // GetIntAnnotation extracts an int from an Ingress annotation func GetIntAnnotation(name string, ing *extensions.Ingress) (int, error) { v := GetAnnotationWithPrefix(name) - err := checkAnnotation(v, ing) + if ing == nil { + return 0, errors.ErrMissingAnnotations + } + + err := checkAnnotation(v, &ing.ObjectMeta) if err != nil { return 0, err } diff --git a/internal/ingress/annotations/parser/main_test.go b/internal/ingress/annotations/parser/main_test.go index e68966fbae..4aff2f7ddb 100644 --- a/internal/ingress/annotations/parser/main_test.go +++ b/internal/ingress/annotations/parser/main_test.go @@ -128,7 +128,7 @@ func TestGetStringAnnotation(t *testing.T) { for _, test := range tests { data[GetAnnotationWithPrefix(test.field)] = test.value - s, err := GetStringAnnotation(test.field, ing) + s, err := GetStringAnnotation(test.field, &ing.ObjectMeta) if test.expErr { if err == nil { t.Errorf("%v: expected error but retuned nil", test.name) @@ -146,7 +146,7 @@ func TestGetStringAnnotation(t *testing.T) { func TestGetStringAnnotationPlugin(t *testing.T) { res := buildPlugin() - _, err := GetStringAnnotationPlugin("", nil) + _, err := GetStringAnnotation("", nil) if err == nil { t.Errorf("expected error but retuned nil") } @@ -168,7 +168,7 @@ func TestGetStringAnnotationPlugin(t *testing.T) { for _, test := range tests { data[GetAnnotationWithPrefix(test.field)] = test.value - s, err := GetStringAnnotationPlugin(test.field, res) + s, err := GetStringAnnotation(test.field, &res.ObjectMeta) if test.expErr { if err == nil { t.Errorf("%v: expected error but retuned nil", test.name) @@ -186,7 +186,7 @@ func TestGetStringAnnotationPlugin(t *testing.T) { func TestGetStringAnnotationCredential(t *testing.T) { res := buildCredential() - _, err := GetStringAnnotationCredential("", nil) + _, err := GetStringAnnotation("", nil) if err == nil { t.Errorf("expected error but retuned nil") } @@ -208,7 +208,7 @@ func TestGetStringAnnotationCredential(t *testing.T) { for _, test := range tests { data[GetAnnotationWithPrefix(test.field)] = test.value - s, err := GetStringAnnotationCredential(test.field, res) + s, err := GetStringAnnotation(test.field, &res.ObjectMeta) if test.expErr { if err == nil { t.Errorf("%v: expected error but retuned nil", test.name) @@ -226,7 +226,7 @@ func TestGetStringAnnotationCredential(t *testing.T) { func TestGetStringAnnotationConsumer(t *testing.T) { res := buildConsumer() - _, err := GetStringAnnotationConsumer("", nil) + _, err := GetStringAnnotation("", nil) if err == nil { t.Errorf("expected error but retuned nil") } @@ -248,7 +248,7 @@ func TestGetStringAnnotationConsumer(t *testing.T) { for _, test := range tests { data[GetAnnotationWithPrefix(test.field)] = test.value - s, err := GetStringAnnotationConsumer(test.field, res) + s, err := GetStringAnnotation(test.field, &res.ObjectMeta) if test.expErr { if err == nil { t.Errorf("%v: expected error but retuned nil", test.name) diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index 9dffd91d2a..704ef58442 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -216,8 +216,8 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, ingEventHandler := cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { ing := obj.(*extensions.Ingress) - if !class.IsValid(ing) { - a, _ := parser.GetStringAnnotation(class.IngressKey, ing) + if !class.IsValid(&ing.ObjectMeta) { + a, _ := parser.GetStringAnnotation(class.IngressKey, &ing.ObjectMeta) glog.Infof("ignoring add for ingress %v based on annotation %v with value %v", ing.Name, class.IngressKey, a) return } @@ -244,7 +244,7 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, return } } - if !class.IsValid(ing) { + if !class.IsValid(&ing.ObjectMeta) { glog.Infof("ignoring delete for ingress %v based on annotation %v", ing.Name, class.IngressKey) return } @@ -258,8 +258,8 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, UpdateFunc: func(old, cur interface{}) { oldIng := old.(*extensions.Ingress) curIng := cur.(*extensions.Ingress) - validOld := class.IsValid(oldIng) - validCur := class.IsValid(curIng) + validOld := class.IsValid(&oldIng.ObjectMeta) + validCur := class.IsValid(&curIng.ObjectMeta) if !validOld && validCur { glog.Infof("creating ingress %v based on annotation %v", curIng.Name, class.IngressKey) recorder.Eventf(curIng, corev1.EventTypeNormal, "CREATE", fmt.Sprintf("Ingress %s/%s", curIng.Namespace, curIng.Name)) @@ -361,8 +361,8 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, pluginEventHandler := cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { plugin := obj.(*pluginv1.KongPlugin) - if !class.IsValidPlugin(plugin) { - a, _ := parser.GetStringAnnotationPlugin(class.IngressKey, plugin) + if !class.IsValid(&plugin.ObjectMeta) { + a, _ := parser.GetStringAnnotation(class.IngressKey, &plugin.ObjectMeta) glog.Infof("ignoring add for plugin %v based on annotation %v with value %v", plugin.Name, class.IngressKey, a) return } @@ -387,7 +387,7 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, return } } - if !class.IsValidPlugin(plugin) { + if !class.IsValid(&plugin.ObjectMeta) { glog.Infof("ignoring delete for plugin %v based on annotation %v", plugin.Name, class.IngressKey) return } @@ -400,8 +400,8 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, UpdateFunc: func(old, cur interface{}) { oldPlugin := old.(*pluginv1.KongPlugin) curPlugin := cur.(*pluginv1.KongPlugin) - validOld := class.IsValidPlugin(oldPlugin) - validCur := class.IsValidPlugin(curPlugin) + validOld := class.IsValid(&oldPlugin.ObjectMeta) + validCur := class.IsValid(&curPlugin.ObjectMeta) if !validOld && validCur { glog.Infof("creating plugin %v based on annotation %v", curPlugin.Name, class.IngressKey) @@ -419,8 +419,8 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, credentialEventHandler := cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { credential := obj.(*credentialv1.KongCredential) - if !class.IsValidCredential(credential) { - a, _ := parser.GetStringAnnotationCredential(class.IngressKey, credential) + if !class.IsValid(&credential.ObjectMeta) { + a, _ := parser.GetStringAnnotation(class.IngressKey, &credential.ObjectMeta) glog.Infof("ignoring add for credential %v based on annotation %v with value %v", credential.Name, class.IngressKey, a) return } @@ -445,7 +445,7 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, return } } - if !class.IsValidCredential(credential) { + if !class.IsValid(&credential.ObjectMeta) { glog.Infof("ignoring delete for credential %v based on annotation %v", credential.Name, class.IngressKey) return } @@ -458,8 +458,8 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, UpdateFunc: func(old, cur interface{}) { oldCredential := old.(*credentialv1.KongCredential) curCredential := cur.(*credentialv1.KongCredential) - validOld := class.IsValidCredential(oldCredential) - validCur := class.IsValidCredential(curCredential) + validOld := class.IsValid(&oldCredential.ObjectMeta) + validCur := class.IsValid(&curCredential.ObjectMeta) if !validOld && validCur { glog.Infof("creating credential %v based on annotation %v", curCredential.Name, class.IngressKey) } else if validOld && !validCur { @@ -476,8 +476,8 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, consumerEventHandler := cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { consumer := obj.(*consumerv1.KongConsumer) - if !class.IsValidConsumer(consumer) { - a, _ := parser.GetStringAnnotationConsumer(class.IngressKey, consumer) + if !class.IsValid(&consumer.ObjectMeta) { + a, _ := parser.GetStringAnnotation(class.IngressKey, &consumer.ObjectMeta) glog.Infof("ignoring add for consumer %v based on annotation %v with value %v", consumer.Name, class.IngressKey, a) return } @@ -502,7 +502,7 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, return } } - if !class.IsValidConsumer(consumer) { + if !class.IsValid(&consumer.ObjectMeta) { glog.Infof("ignoring delete for consumer %v based on annotation %v", consumer.Name, class.IngressKey) return } @@ -515,8 +515,8 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, UpdateFunc: func(old, cur interface{}) { oldConsumer := old.(*consumerv1.KongConsumer) curConsumer := cur.(*consumerv1.KongConsumer) - validOld := class.IsValidConsumer(oldConsumer) - validCur := class.IsValidConsumer(curConsumer) + validOld := class.IsValid(&oldConsumer.ObjectMeta) + validCur := class.IsValid(&curConsumer.ObjectMeta) if !validOld && validCur { glog.Infof("creating consumer %v based on annotation %v", curConsumer.Name, class.IngressKey) } else if validOld && !validCur { @@ -603,7 +603,7 @@ func (s k8sStore) ListIngresses() []*extensions.Ingress { var ingresses []*extensions.Ingress for _, item := range s.listers.Ingress.List() { ing := item.(*extensions.Ingress) - if !class.IsValid(ing) { + if !class.IsValid(&ing.ObjectMeta) { continue } @@ -664,7 +664,7 @@ func (s k8sStore) ListKongConsumers() []*consumerv1.KongConsumer { var consumers []*consumerv1.KongConsumer for _, item := range s.listers.Kong.Consumer.List() { c, ok := item.(*consumerv1.KongConsumer) - if ok && class.IsValidConsumer(c) { + if ok && class.IsValid(&c.ObjectMeta) { consumers = append(consumers, c) } } @@ -676,7 +676,7 @@ func (s k8sStore) ListKongCredentials() []*credentialv1.KongCredential { var credentials []*credentialv1.KongCredential for _, item := range s.listers.Kong.Credential.List() { c, ok := item.(*credentialv1.KongCredential) - if ok && class.IsValidCredential(c) { + if ok && class.IsValid(&c.ObjectMeta) { credentials = append(credentials, c) } } @@ -696,7 +696,7 @@ func (s k8sStore) ListGlobalKongPlugins() ([]*pluginv1.KongPlugin, error) { labels.NewSelector().Add(*req), func(ob interface{}) { p, ok := ob.(*pluginv1.KongPlugin) - if ok && class.IsValidPlugin(p) { + if ok && class.IsValid(&p.ObjectMeta) { plugins = append(plugins, p) } }) From 974b7bb93a3e122c8bc4c951ed17aad5f6469dba Mon Sep 17 00:00:00 2001 From: Nicolas MACHEREY Date: Sun, 6 Jan 2019 10:55:54 +0100 Subject: [PATCH 3/4] Fix PR comments --- docs/custom-resources.md | 57 ++++--- internal/ingress/annotations/class/main.go | 95 +++++++++++ .../ingress/annotations/class/main_test.go | 30 ++-- .../ingress/annotations/parser/main_test.go | 149 ----------------- internal/ingress/controller/store/store.go | 154 ++---------------- 5 files changed, 153 insertions(+), 332 deletions(-) diff --git a/docs/custom-resources.md b/docs/custom-resources.md index 2ecd1d945b..5a42994df3 100644 --- a/docs/custom-resources.md +++ b/docs/custom-resources.md @@ -15,30 +15,6 @@ Following CRDs enables users to declaratively configure all aspects of Kong: - [**KongCredential**](#kongcredential): These resources map to credentials (key-auth, basic-auth, etc) that belong to consumers. -**IMPORTANT NOTE**: Kong Custom Resources are using the `kubernetes.io/ingress.class` annotation which defaults to `kong`. In -case you have more than one Kong Ingress deployed on your cluster, make sure you label all your ressources with the appropriate -annotation. For example: - -```yaml -apiVersion: configuration.konghq.com/v1 -kind: KongPlugin -metadata: - name: - namespace: - annotations: - kubernetes.io/ingress.class: - labels: - global: "true" # optional, please note the quotes around true - # configures the plugin Globally in Kong -consumerRef: # optional - # applies the plugin - # in on specific route and consumer -disabled: # optionally disable the plugin in Kong -config: - key: value -plugin: # like key-auth, rate-limiting etc -``` - ## KongPlugin This resource allows the configuration of @@ -289,3 +265,36 @@ config: [kong-upstream]: https://getkong.org/docs/0.14.x/admin-api/#upstream-objects [kong-service]: https://getkong.org/docs/0.14.x/admin-api/#service-object [kong-route]: https://getkong.org/docs/0.14.x/admin-api/#route-object + +## Using mutilple ingresses on the same cluster + +Kong Custom Resources are using the `kubernetes.io/ingress.class` +annotation which defaults to `nginx`. In case you have more than one Ingress deployed +on your cluster, make sure you label all your resources with the appropriate +annotation. For example: + +```yaml +apiVersion: configuration.konghq.com/v1 +kind: KongPlugin +metadata: + name: + namespace: + annotations: + kubernetes.io/ingress.class: + labels: + global: "true" # optional, please note the quotes around true + # configures the plugin Globally in Kong +consumerRef: # optional + # applies the plugin + # in on specific route and consumer +disabled: # optionally disable the plugin in Kong +config: + key: value +plugin: # like key-auth, rate-limiting etc +``` + +Here are the list of custom resources that supports the annotation: + +- KongPlugin +- KongCredential +- KongConsumer \ No newline at end of file diff --git a/internal/ingress/annotations/class/main.go b/internal/ingress/annotations/class/main.go index 8184ff5145..aca6c67908 100644 --- a/internal/ingress/annotations/class/main.go +++ b/internal/ingress/annotations/class/main.go @@ -18,6 +18,10 @@ package class import ( "github.com/golang/glog" + consumerv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/consumer/v1" + credentialv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/credential/v1" + pluginv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/plugin/v1" + "github.com/kong/kubernetes-ingress-controller/internal/ingress/annotations/parser" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -60,3 +64,94 @@ func IsValid(objectMeta *metav1.ObjectMeta) bool { return ingress == IngressClass } + +// CanAddResource checks if a custom resources can be added to the ingress +func CanAddResource(obj interface{}) bool { + var objectMeta *metav1.ObjectMeta + resourceType := "" + resourceName := "" + + if p, ok := obj.(*pluginv1.KongPlugin); ok { + resourceType = "plugin" + resourceName = p.Name + objectMeta = &p.ObjectMeta + } else if c, ok := obj.(*consumerv1.KongConsumer); ok { + resourceType = "consumer" + resourceName = c.Name + objectMeta = &c.ObjectMeta + } else if c, ok := obj.(*credentialv1.KongCredential); ok { + resourceType = "credential" + resourceName = c.Name + objectMeta = &c.ObjectMeta + } else { + return false + } + + if !IsValid(objectMeta) { + a, _ := parser.GetStringAnnotation(IngressKey, objectMeta) + glog.Infof("ignoring add for %v %v based on annotation %v with value %v", resourceType, resourceName, IngressKey, a) + return false + } + + return true +} + +// CanDeleteResource checks if a custom resources can be deleted from the ingress +func CanDeleteResource(obj interface{}) bool { + var objectMeta *metav1.ObjectMeta + resourceType := "" + resourceName := "" + + if p, ok := obj.(*pluginv1.KongPlugin); ok { + resourceType = "plugin" + resourceName = p.Name + objectMeta = &p.ObjectMeta + } else if c, ok := obj.(*consumerv1.KongConsumer); ok { + resourceType = "consumer" + resourceName = c.Name + objectMeta = &c.ObjectMeta + } else if c, ok := obj.(*credentialv1.KongCredential); ok { + resourceType = "credential" + resourceName = c.Name + objectMeta = &c.ObjectMeta + } else { + return false + } + + if !IsValid(objectMeta) { + a, _ := parser.GetStringAnnotation(IngressKey, objectMeta) + glog.Infof("ignoring delete for %v %v based on annotation %v with value %v", resourceType, resourceName, IngressKey, a) + return false + } + + return true +} + +// CanUpdateResource checks if a custom resources can be updated from the ingress +func CanUpdateResource(obj interface{}) (bool, string, string) { + var objectMeta *metav1.ObjectMeta + resourceType := "" + resourceName := "" + + if p, ok := obj.(*pluginv1.KongPlugin); ok { + resourceType = "plugin" + resourceName = p.Name + objectMeta = &p.ObjectMeta + } else if c, ok := obj.(*consumerv1.KongConsumer); ok { + resourceType = "consumer" + resourceName = c.Name + objectMeta = &c.ObjectMeta + } else if c, ok := obj.(*credentialv1.KongCredential); ok { + resourceType = "credential" + resourceName = c.Name + objectMeta = &c.ObjectMeta + } else { + return false, "", "" + } + + if !IsValid(objectMeta) { + return false, resourceType, resourceName + } + + return true, resourceType, resourceName +} diff --git a/internal/ingress/annotations/class/main_test.go b/internal/ingress/annotations/class/main_test.go index 3ec99cbdc7..9826b5da4d 100644 --- a/internal/ingress/annotations/class/main_test.go +++ b/internal/ingress/annotations/class/main_test.go @@ -72,7 +72,7 @@ func TestIsValidClass(t *testing.T) { } } -func TestIsValidPlugin(t *testing.T) { +func TestCandAddResource(t *testing.T) { dc := DefaultClass ic := IngressClass // restore original values after the tests @@ -95,7 +95,7 @@ func TestIsValidPlugin(t *testing.T) { {"custom", "nginx", "nginx", false}, } - plugin := &pluginv1.KongPlugin{ + crd := &pluginv1.KongPlugin{ ObjectMeta: meta_v1.ObjectMeta{ Name: "foo", Namespace: api.NamespaceDefault, @@ -103,21 +103,21 @@ func TestIsValidPlugin(t *testing.T) { } data := map[string]string{} - plugin.SetAnnotations(data) + crd.SetAnnotations(data) for _, test := range tests { - plugin.Annotations[IngressKey] = test.ingress + crd.Annotations[IngressKey] = test.ingress IngressClass = test.controller DefaultClass = test.defClass - b := IsValid(&plugin.ObjectMeta) + b := CanAddResource(crd) if b != test.isValid { t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) } } } -func TestIsValidConsumer(t *testing.T) { +func TestCandDeleteResource(t *testing.T) { dc := DefaultClass ic := IngressClass // restore original values after the tests @@ -140,7 +140,7 @@ func TestIsValidConsumer(t *testing.T) { {"custom", "nginx", "nginx", false}, } - consumer := &consumerv1.KongConsumer{ + crd := &credentialv1.KongCredential{ ObjectMeta: meta_v1.ObjectMeta{ Name: "foo", Namespace: api.NamespaceDefault, @@ -148,21 +148,21 @@ func TestIsValidConsumer(t *testing.T) { } data := map[string]string{} - consumer.SetAnnotations(data) + crd.SetAnnotations(data) for _, test := range tests { - consumer.Annotations[IngressKey] = test.ingress + crd.Annotations[IngressKey] = test.ingress IngressClass = test.controller DefaultClass = test.defClass - b := IsValid(&consumer.ObjectMeta) + b := CanDeleteResource(crd) if b != test.isValid { t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) } } } -func TestIsValidCredential(t *testing.T) { +func TestCandUpdateResource(t *testing.T) { dc := DefaultClass ic := IngressClass // restore original values after the tests @@ -185,7 +185,7 @@ func TestIsValidCredential(t *testing.T) { {"custom", "nginx", "nginx", false}, } - credential := &credentialv1.KongCredential{ + crd := &consumerv1.KongConsumer{ ObjectMeta: meta_v1.ObjectMeta{ Name: "foo", Namespace: api.NamespaceDefault, @@ -193,14 +193,14 @@ func TestIsValidCredential(t *testing.T) { } data := map[string]string{} - credential.SetAnnotations(data) + crd.SetAnnotations(data) for _, test := range tests { - credential.Annotations[IngressKey] = test.ingress + crd.Annotations[IngressKey] = test.ingress IngressClass = test.controller DefaultClass = test.defClass - b := IsValid(&credential.ObjectMeta) + b, _, _ := CanUpdateResource(crd) if b != test.isValid { t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) } diff --git a/internal/ingress/annotations/parser/main_test.go b/internal/ingress/annotations/parser/main_test.go index 4aff2f7ddb..dab1c71778 100644 --- a/internal/ingress/annotations/parser/main_test.go +++ b/internal/ingress/annotations/parser/main_test.go @@ -19,9 +19,6 @@ package parser import ( "testing" - consumerv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/consumer/v1" - credentialv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/credential/v1" - pluginv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/plugin/v1" api "k8s.io/api/core/v1" extensions "k8s.io/api/extensions/v1beta1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -37,32 +34,6 @@ func buildIngress() *extensions.Ingress { } } -func buildPlugin() *pluginv1.KongPlugin { - return &pluginv1.KongPlugin{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "foo", - Namespace: api.NamespaceDefault, - }, - } -} - -func buildCredential() *credentialv1.KongCredential { - return &credentialv1.KongCredential{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "foo", - Namespace: api.NamespaceDefault, - }, - } -} - -func buildConsumer() *consumerv1.KongConsumer { - return &consumerv1.KongConsumer{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "foo", - Namespace: api.NamespaceDefault, - }, - } -} func TestGetBoolAnnotation(t *testing.T) { ing := buildIngress() @@ -143,126 +114,6 @@ func TestGetStringAnnotation(t *testing.T) { } } -func TestGetStringAnnotationPlugin(t *testing.T) { - res := buildPlugin() - - _, err := GetStringAnnotation("", nil) - if err == nil { - t.Errorf("expected error but retuned nil") - } - - tests := []struct { - name string - field string - value string - exp string - expErr bool - }{ - {"valid - A", "string", "A", "A", false}, - {"valid - B", "string", "B", "B", false}, - } - - data := map[string]string{} - res.SetAnnotations(data) - - for _, test := range tests { - data[GetAnnotationWithPrefix(test.field)] = test.value - - s, err := GetStringAnnotation(test.field, &res.ObjectMeta) - if test.expErr { - if err == nil { - t.Errorf("%v: expected error but retuned nil", test.name) - } - continue - } - if s != test.exp { - t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.name, test.exp, s) - } - - delete(data, test.field) - } -} - -func TestGetStringAnnotationCredential(t *testing.T) { - res := buildCredential() - - _, err := GetStringAnnotation("", nil) - if err == nil { - t.Errorf("expected error but retuned nil") - } - - tests := []struct { - name string - field string - value string - exp string - expErr bool - }{ - {"valid - A", "string", "A", "A", false}, - {"valid - B", "string", "B", "B", false}, - } - - data := map[string]string{} - res.SetAnnotations(data) - - for _, test := range tests { - data[GetAnnotationWithPrefix(test.field)] = test.value - - s, err := GetStringAnnotation(test.field, &res.ObjectMeta) - if test.expErr { - if err == nil { - t.Errorf("%v: expected error but retuned nil", test.name) - } - continue - } - if s != test.exp { - t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.name, test.exp, s) - } - - delete(data, test.field) - } -} - -func TestGetStringAnnotationConsumer(t *testing.T) { - res := buildConsumer() - - _, err := GetStringAnnotation("", nil) - if err == nil { - t.Errorf("expected error but retuned nil") - } - - tests := []struct { - name string - field string - value string - exp string - expErr bool - }{ - {"valid - A", "string", "A", "A", false}, - {"valid - B", "string", "B", "B", false}, - } - - data := map[string]string{} - res.SetAnnotations(data) - - for _, test := range tests { - data[GetAnnotationWithPrefix(test.field)] = test.value - - s, err := GetStringAnnotation(test.field, &res.ObjectMeta) - if test.expErr { - if err == nil { - t.Errorf("%v: expected error but retuned nil", test.name) - } - continue - } - if s != test.exp { - t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.name, test.exp, s) - } - - delete(data, test.field) - } -} - func TestGetIntAnnotation(t *testing.T) { ing := buildIngress() diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index 704ef58442..21f9bdc9b6 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -358,12 +358,9 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, store.informers.Secret.AddEventHandler(secrEventHandler) store.informers.Service.AddEventHandler(serviceEventHandler) - pluginEventHandler := cache.ResourceEventHandlerFuncs{ + annotatedCrdEventHandler := cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - plugin := obj.(*pluginv1.KongPlugin) - if !class.IsValid(&plugin.ObjectMeta) { - a, _ := parser.GetStringAnnotation(class.IngressKey, &plugin.ObjectMeta) - glog.Infof("ignoring add for plugin %v based on annotation %v with value %v", plugin.Name, class.IngressKey, a) + if ok := class.CanAddResource(obj); !ok { return } @@ -373,22 +370,7 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, } }, DeleteFunc: func(obj interface{}) { - plugin, ok := obj.(*pluginv1.KongPlugin) - if !ok { - // If we reached here it means the ingress was deleted but its final state is unrecorded. - tombstone, ok := obj.(cache.DeletedFinalStateUnknown) - if !ok { - glog.Errorf("couldn't get object from tombstone %#v", obj) - return - } - plugin, ok = tombstone.Obj.(*pluginv1.KongPlugin) - if !ok { - glog.Errorf("Tombstone contained object that is not a KongPlugin: %#v", obj) - return - } - } - if !class.IsValid(&plugin.ObjectMeta) { - glog.Infof("ignoring delete for plugin %v based on annotation %v", plugin.Name, class.IngressKey) + if ok := class.CanDeleteResource(obj); !ok { return } @@ -398,129 +380,13 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, } }, UpdateFunc: func(old, cur interface{}) { - oldPlugin := old.(*pluginv1.KongPlugin) - curPlugin := cur.(*pluginv1.KongPlugin) - validOld := class.IsValid(&oldPlugin.ObjectMeta) - validCur := class.IsValid(&curPlugin.ObjectMeta) - - if !validOld && validCur { - glog.Infof("creating plugin %v based on annotation %v", curPlugin.Name, class.IngressKey) - } else if validOld && !validCur { - glog.Infof("removing plugin %v based on annotation %v", curPlugin.Name, class.IngressKey) - } - - updateCh.In() <- Event{ - Type: ConfigurationEvent, - Obj: cur, - } - }, - } + validOld, _, _ := class.CanUpdateResource(old) + validCur, curType, curName := class.CanUpdateResource(cur) - credentialEventHandler := cache.ResourceEventHandlerFuncs{ - AddFunc: func(obj interface{}) { - credential := obj.(*credentialv1.KongCredential) - if !class.IsValid(&credential.ObjectMeta) { - a, _ := parser.GetStringAnnotation(class.IngressKey, &credential.ObjectMeta) - glog.Infof("ignoring add for credential %v based on annotation %v with value %v", credential.Name, class.IngressKey, a) - return - } - - updateCh.In() <- Event{ - Type: ConfigurationEvent, - Obj: obj, - } - }, - DeleteFunc: func(obj interface{}) { - credential, ok := obj.(*credentialv1.KongCredential) - if !ok { - // If we reached here it means the ingress was deleted but its final state is unrecorded. - tombstone, ok := obj.(cache.DeletedFinalStateUnknown) - if !ok { - glog.Errorf("couldn't get object from tombstone %#v", obj) - return - } - credential, ok = tombstone.Obj.(*credentialv1.KongCredential) - if !ok { - glog.Errorf("Tombstone contained object that is not a KongCredential: %#v", obj) - return - } - } - if !class.IsValid(&credential.ObjectMeta) { - glog.Infof("ignoring delete for credential %v based on annotation %v", credential.Name, class.IngressKey) - return - } - - updateCh.In() <- Event{ - Type: ConfigurationEvent, - Obj: obj, - } - }, - UpdateFunc: func(old, cur interface{}) { - oldCredential := old.(*credentialv1.KongCredential) - curCredential := cur.(*credentialv1.KongCredential) - validOld := class.IsValid(&oldCredential.ObjectMeta) - validCur := class.IsValid(&curCredential.ObjectMeta) - if !validOld && validCur { - glog.Infof("creating credential %v based on annotation %v", curCredential.Name, class.IngressKey) - } else if validOld && !validCur { - glog.Infof("removing credential %v based on annotation %v", curCredential.Name, class.IngressKey) - } - - updateCh.In() <- Event{ - Type: ConfigurationEvent, - Obj: cur, - } - }, - } - - consumerEventHandler := cache.ResourceEventHandlerFuncs{ - AddFunc: func(obj interface{}) { - consumer := obj.(*consumerv1.KongConsumer) - if !class.IsValid(&consumer.ObjectMeta) { - a, _ := parser.GetStringAnnotation(class.IngressKey, &consumer.ObjectMeta) - glog.Infof("ignoring add for consumer %v based on annotation %v with value %v", consumer.Name, class.IngressKey, a) - return - } - - updateCh.In() <- Event{ - Type: ConfigurationEvent, - Obj: obj, - } - }, - DeleteFunc: func(obj interface{}) { - consumer, ok := obj.(*consumerv1.KongConsumer) - if !ok { - // If we reached here it means the ingress was deleted but its final state is unrecorded. - tombstone, ok := obj.(cache.DeletedFinalStateUnknown) - if !ok { - glog.Errorf("couldn't get object from tombstone %#v", obj) - return - } - consumer, ok = tombstone.Obj.(*consumerv1.KongConsumer) - if !ok { - glog.Errorf("Tombstone contained object that is not a KongConsumer: %#v", obj) - return - } - } - if !class.IsValid(&consumer.ObjectMeta) { - glog.Infof("ignoring delete for consumer %v based on annotation %v", consumer.Name, class.IngressKey) - return - } - - updateCh.In() <- Event{ - Type: ConfigurationEvent, - Obj: obj, - } - }, - UpdateFunc: func(old, cur interface{}) { - oldConsumer := old.(*consumerv1.KongConsumer) - curConsumer := cur.(*consumerv1.KongConsumer) - validOld := class.IsValid(&oldConsumer.ObjectMeta) - validCur := class.IsValid(&curConsumer.ObjectMeta) if !validOld && validCur { - glog.Infof("creating consumer %v based on annotation %v", curConsumer.Name, class.IngressKey) + glog.Infof("creating %v %v based on annotation %v", curType, curName, class.IngressKey) } else if validOld && !validCur { - glog.Infof("removing consumer %v based on annotation %v", curConsumer.Name, class.IngressKey) + glog.Infof("removing %v %v based on annotation %v", curType, curName, class.IngressKey) } updateCh.In() <- Event{ @@ -556,21 +422,21 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, store.informers.Kong.Plugin = pluginFactory.Configuration().V1().KongPlugins().Informer() store.listers.Kong.Plugin = store.informers.Kong.Plugin.GetStore() - store.informers.Kong.Plugin.AddEventHandler(pluginEventHandler) + store.informers.Kong.Plugin.AddEventHandler(annotatedCrdEventHandler) consumerClient, _ := consumerclientv1.NewForConfig(clientConf) consumerFactory := consumerinformer.NewFilteredSharedInformerFactory(consumerClient, resyncPeriod, namespace, func(*metav1.ListOptions) {}) store.informers.Kong.Consumer = consumerFactory.Configuration().V1().KongConsumers().Informer() store.listers.Kong.Consumer = store.informers.Kong.Consumer.GetStore() - store.informers.Kong.Consumer.AddEventHandler(consumerEventHandler) + store.informers.Kong.Consumer.AddEventHandler(annotatedCrdEventHandler) credClient, _ := credentialclientv1.NewForConfig(clientConf) credentialFactory := credentialinformer.NewFilteredSharedInformerFactory(credClient, resyncPeriod, namespace, func(*metav1.ListOptions) {}) store.informers.Kong.Credential = credentialFactory.Configuration().V1().KongCredentials().Informer() store.listers.Kong.Credential = store.informers.Kong.Credential.GetStore() - store.informers.Kong.Credential.AddEventHandler(credentialEventHandler) + store.informers.Kong.Credential.AddEventHandler(annotatedCrdEventHandler) confClient, _ := configurationclientv1.NewForConfig(clientConf) configFactory := configurationinformer.NewFilteredSharedInformerFactory(confClient, resyncPeriod, namespace, func(*metav1.ListOptions) {}) From 48bbee566d5c03439ebb168020aa8862b41ddc28 Mon Sep 17 00:00:00 2001 From: Nicolas MACHEREY Date: Mon, 7 Jan 2019 21:56:57 +0100 Subject: [PATCH 4/4] Fix PR comments --- internal/ingress/annotations/class/main.go | 95 ---------- .../ingress/annotations/class/main_test.go | 138 -------------- internal/ingress/controller/store/store.go | 171 +++++++++++++++++- 3 files changed, 161 insertions(+), 243 deletions(-) diff --git a/internal/ingress/annotations/class/main.go b/internal/ingress/annotations/class/main.go index aca6c67908..8184ff5145 100644 --- a/internal/ingress/annotations/class/main.go +++ b/internal/ingress/annotations/class/main.go @@ -18,10 +18,6 @@ package class import ( "github.com/golang/glog" - consumerv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/consumer/v1" - credentialv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/credential/v1" - pluginv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/plugin/v1" - "github.com/kong/kubernetes-ingress-controller/internal/ingress/annotations/parser" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -64,94 +60,3 @@ func IsValid(objectMeta *metav1.ObjectMeta) bool { return ingress == IngressClass } - -// CanAddResource checks if a custom resources can be added to the ingress -func CanAddResource(obj interface{}) bool { - var objectMeta *metav1.ObjectMeta - resourceType := "" - resourceName := "" - - if p, ok := obj.(*pluginv1.KongPlugin); ok { - resourceType = "plugin" - resourceName = p.Name - objectMeta = &p.ObjectMeta - } else if c, ok := obj.(*consumerv1.KongConsumer); ok { - resourceType = "consumer" - resourceName = c.Name - objectMeta = &c.ObjectMeta - } else if c, ok := obj.(*credentialv1.KongCredential); ok { - resourceType = "credential" - resourceName = c.Name - objectMeta = &c.ObjectMeta - } else { - return false - } - - if !IsValid(objectMeta) { - a, _ := parser.GetStringAnnotation(IngressKey, objectMeta) - glog.Infof("ignoring add for %v %v based on annotation %v with value %v", resourceType, resourceName, IngressKey, a) - return false - } - - return true -} - -// CanDeleteResource checks if a custom resources can be deleted from the ingress -func CanDeleteResource(obj interface{}) bool { - var objectMeta *metav1.ObjectMeta - resourceType := "" - resourceName := "" - - if p, ok := obj.(*pluginv1.KongPlugin); ok { - resourceType = "plugin" - resourceName = p.Name - objectMeta = &p.ObjectMeta - } else if c, ok := obj.(*consumerv1.KongConsumer); ok { - resourceType = "consumer" - resourceName = c.Name - objectMeta = &c.ObjectMeta - } else if c, ok := obj.(*credentialv1.KongCredential); ok { - resourceType = "credential" - resourceName = c.Name - objectMeta = &c.ObjectMeta - } else { - return false - } - - if !IsValid(objectMeta) { - a, _ := parser.GetStringAnnotation(IngressKey, objectMeta) - glog.Infof("ignoring delete for %v %v based on annotation %v with value %v", resourceType, resourceName, IngressKey, a) - return false - } - - return true -} - -// CanUpdateResource checks if a custom resources can be updated from the ingress -func CanUpdateResource(obj interface{}) (bool, string, string) { - var objectMeta *metav1.ObjectMeta - resourceType := "" - resourceName := "" - - if p, ok := obj.(*pluginv1.KongPlugin); ok { - resourceType = "plugin" - resourceName = p.Name - objectMeta = &p.ObjectMeta - } else if c, ok := obj.(*consumerv1.KongConsumer); ok { - resourceType = "consumer" - resourceName = c.Name - objectMeta = &c.ObjectMeta - } else if c, ok := obj.(*credentialv1.KongCredential); ok { - resourceType = "credential" - resourceName = c.Name - objectMeta = &c.ObjectMeta - } else { - return false, "", "" - } - - if !IsValid(objectMeta) { - return false, resourceType, resourceName - } - - return true, resourceType, resourceName -} diff --git a/internal/ingress/annotations/class/main_test.go b/internal/ingress/annotations/class/main_test.go index 9826b5da4d..1e29cb7caf 100644 --- a/internal/ingress/annotations/class/main_test.go +++ b/internal/ingress/annotations/class/main_test.go @@ -19,9 +19,6 @@ package class import ( "testing" - consumerv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/consumer/v1" - credentialv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/credential/v1" - pluginv1 "github.com/kong/kubernetes-ingress-controller/internal/apis/plugin/v1" api "k8s.io/api/core/v1" extensions "k8s.io/api/extensions/v1beta1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -71,138 +68,3 @@ func TestIsValidClass(t *testing.T) { } } } - -func TestCandAddResource(t *testing.T) { - dc := DefaultClass - ic := IngressClass - // restore original values after the tests - defer func() { - DefaultClass = dc - IngressClass = ic - }() - - tests := []struct { - ingress string - controller string - defClass string - isValid bool - }{ - {"", "", "nginx", true}, - {"", "nginx", "nginx", true}, - {"nginx", "nginx", "nginx", true}, - {"custom", "custom", "nginx", true}, - {"", "killer", "nginx", false}, - {"custom", "nginx", "nginx", false}, - } - - crd := &pluginv1.KongPlugin{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "foo", - Namespace: api.NamespaceDefault, - }, - } - - data := map[string]string{} - crd.SetAnnotations(data) - for _, test := range tests { - crd.Annotations[IngressKey] = test.ingress - - IngressClass = test.controller - DefaultClass = test.defClass - - b := CanAddResource(crd) - if b != test.isValid { - t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) - } - } -} - -func TestCandDeleteResource(t *testing.T) { - dc := DefaultClass - ic := IngressClass - // restore original values after the tests - defer func() { - DefaultClass = dc - IngressClass = ic - }() - - tests := []struct { - ingress string - controller string - defClass string - isValid bool - }{ - {"", "", "nginx", true}, - {"", "nginx", "nginx", true}, - {"nginx", "nginx", "nginx", true}, - {"custom", "custom", "nginx", true}, - {"", "killer", "nginx", false}, - {"custom", "nginx", "nginx", false}, - } - - crd := &credentialv1.KongCredential{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "foo", - Namespace: api.NamespaceDefault, - }, - } - - data := map[string]string{} - crd.SetAnnotations(data) - for _, test := range tests { - crd.Annotations[IngressKey] = test.ingress - - IngressClass = test.controller - DefaultClass = test.defClass - - b := CanDeleteResource(crd) - if b != test.isValid { - t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) - } - } -} - -func TestCandUpdateResource(t *testing.T) { - dc := DefaultClass - ic := IngressClass - // restore original values after the tests - defer func() { - DefaultClass = dc - IngressClass = ic - }() - - tests := []struct { - ingress string - controller string - defClass string - isValid bool - }{ - {"", "", "nginx", true}, - {"", "nginx", "nginx", true}, - {"nginx", "nginx", "nginx", true}, - {"custom", "custom", "nginx", true}, - {"", "killer", "nginx", false}, - {"custom", "nginx", "nginx", false}, - } - - crd := &consumerv1.KongConsumer{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "foo", - Namespace: api.NamespaceDefault, - }, - } - - data := map[string]string{} - crd.SetAnnotations(data) - for _, test := range tests { - crd.Annotations[IngressKey] = test.ingress - - IngressClass = test.controller - DefaultClass = test.defClass - - b, _, _ := CanUpdateResource(crd) - if b != test.isValid { - t.Errorf("test %v - expected %v but %v was returned", test, test.isValid, b) - } - } -} diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index 21f9bdc9b6..bc911122da 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -358,9 +358,16 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, store.informers.Secret.AddEventHandler(secrEventHandler) store.informers.Service.AddEventHandler(serviceEventHandler) - annotatedCrdEventHandler := cache.ResourceEventHandlerFuncs{ + pluginEventHandler := cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - if ok := class.CanAddResource(obj); !ok { + res, ok := obj.(*pluginv1.KongPlugin) + if !ok { + return + } + objectMeta := &res.ObjectMeta + if !class.IsValid(objectMeta) { + a, _ := parser.GetStringAnnotation(class.IngressKey, objectMeta) + glog.Infof("ignoring add for plugin %v based on annotation %v with value %v", res.Name, class.IngressKey, a) return } @@ -370,7 +377,149 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, } }, DeleteFunc: func(obj interface{}) { - if ok := class.CanDeleteResource(obj); !ok { + res, ok := obj.(*pluginv1.KongPlugin) + if !ok { + tombstone, ok := obj.(cache.DeletedFinalStateUnknown) + if !ok { + glog.Errorf("couldn't get object from tombstone %#v", obj) + return + } + res, ok = tombstone.Obj.(*pluginv1.KongPlugin) + if !ok { + glog.Errorf("Tombstone contained object that is not a plugin: %#v", obj) + return + } + } + objectMeta := &res.ObjectMeta + if !class.IsValid(objectMeta) { + a, _ := parser.GetStringAnnotation(class.IngressKey, objectMeta) + glog.Infof("ignoring delete for plugin %v based on annotation %v with value %v", res.Name, class.IngressKey, a) + return + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: obj, + } + }, + UpdateFunc: func(old, cur interface{}) { + oldRes := old.(*pluginv1.KongPlugin) + curRes := cur.(*pluginv1.KongPlugin) + validOld := class.IsValid(&oldRes.ObjectMeta) + validCur := class.IsValid(&curRes.ObjectMeta) + + if !validOld && validCur { + glog.Infof("creating plugin %v based on annotation %v", curRes.Name, class.IngressKey) + } else if validOld && !validCur { + glog.Infof("removing plugin %v based on annotation %v", curRes.Name, class.IngressKey) + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: cur, + } + }, + } + + consumerEventHandler := cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj interface{}) { + res, ok := obj.(*consumerv1.KongConsumer) + if !ok { + return + } + objectMeta := &res.ObjectMeta + if !class.IsValid(objectMeta) { + a, _ := parser.GetStringAnnotation(class.IngressKey, objectMeta) + glog.Infof("ignoring add for consumer %v based on annotation %v with value %v", res.Name, class.IngressKey, a) + return + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: obj, + } + }, + DeleteFunc: func(obj interface{}) { + res, ok := obj.(*consumerv1.KongConsumer) + if !ok { + tombstone, ok := obj.(cache.DeletedFinalStateUnknown) + if !ok { + glog.Errorf("couldn't get object from tombstone %#v", obj) + return + } + res, ok = tombstone.Obj.(*consumerv1.KongConsumer) + if !ok { + glog.Errorf("Tombstone contained object that is not a consumer: %#v", obj) + return + } + } + objectMeta := &res.ObjectMeta + if !class.IsValid(objectMeta) { + a, _ := parser.GetStringAnnotation(class.IngressKey, objectMeta) + glog.Infof("ignoring delete for consumer %v based on annotation %v with value %v", res.Name, class.IngressKey, a) + return + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: obj, + } + }, + UpdateFunc: func(old, cur interface{}) { + oldRes := old.(*consumerv1.KongConsumer) + curRes := cur.(*consumerv1.KongConsumer) + validOld := class.IsValid(&oldRes.ObjectMeta) + validCur := class.IsValid(&curRes.ObjectMeta) + + if !validOld && validCur { + glog.Infof("creating consumer %v based on annotation %v", curRes.Name, class.IngressKey) + } else if validOld && !validCur { + glog.Infof("removing consumer %v based on annotation %v", curRes.Name, class.IngressKey) + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: cur, + } + }, + } + + credentialEventHandler := cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj interface{}) { + res, ok := obj.(*credentialv1.KongCredential) + if !ok { + return + } + objectMeta := &res.ObjectMeta + if !class.IsValid(objectMeta) { + a, _ := parser.GetStringAnnotation(class.IngressKey, objectMeta) + glog.Infof("ignoring add for credential %v based on annotation %v with value %v", res.Name, class.IngressKey, a) + return + } + + updateCh.In() <- Event{ + Type: ConfigurationEvent, + Obj: obj, + } + }, + DeleteFunc: func(obj interface{}) { + res, ok := obj.(*credentialv1.KongCredential) + if !ok { + tombstone, ok := obj.(cache.DeletedFinalStateUnknown) + if !ok { + glog.Errorf("couldn't get object from tombstone %#v", obj) + return + } + res, ok = tombstone.Obj.(*credentialv1.KongCredential) + if !ok { + glog.Errorf("Tombstone contained object that is not a credential: %#v", obj) + return + } + } + objectMeta := &res.ObjectMeta + if !class.IsValid(objectMeta) { + a, _ := parser.GetStringAnnotation(class.IngressKey, objectMeta) + glog.Infof("ignoring delete for credential %v based on annotation %v with value %v", res.Name, class.IngressKey, a) return } @@ -380,13 +529,15 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, } }, UpdateFunc: func(old, cur interface{}) { - validOld, _, _ := class.CanUpdateResource(old) - validCur, curType, curName := class.CanUpdateResource(cur) + oldRes := old.(*credentialv1.KongCredential) + curRes := cur.(*credentialv1.KongCredential) + validOld := class.IsValid(&oldRes.ObjectMeta) + validCur := class.IsValid(&curRes.ObjectMeta) if !validOld && validCur { - glog.Infof("creating %v %v based on annotation %v", curType, curName, class.IngressKey) + glog.Infof("creating credential %v based on annotation %v", curRes.Name, class.IngressKey) } else if validOld && !validCur { - glog.Infof("removing %v %v based on annotation %v", curType, curName, class.IngressKey) + glog.Infof("removing credential %v based on annotation %v", curRes.Name, class.IngressKey) } updateCh.In() <- Event{ @@ -422,21 +573,21 @@ func New(namespace string, configmap, tcp, udp, defaultSSLCertificate string, store.informers.Kong.Plugin = pluginFactory.Configuration().V1().KongPlugins().Informer() store.listers.Kong.Plugin = store.informers.Kong.Plugin.GetStore() - store.informers.Kong.Plugin.AddEventHandler(annotatedCrdEventHandler) + store.informers.Kong.Plugin.AddEventHandler(pluginEventHandler) consumerClient, _ := consumerclientv1.NewForConfig(clientConf) consumerFactory := consumerinformer.NewFilteredSharedInformerFactory(consumerClient, resyncPeriod, namespace, func(*metav1.ListOptions) {}) store.informers.Kong.Consumer = consumerFactory.Configuration().V1().KongConsumers().Informer() store.listers.Kong.Consumer = store.informers.Kong.Consumer.GetStore() - store.informers.Kong.Consumer.AddEventHandler(annotatedCrdEventHandler) + store.informers.Kong.Consumer.AddEventHandler(consumerEventHandler) credClient, _ := credentialclientv1.NewForConfig(clientConf) credentialFactory := credentialinformer.NewFilteredSharedInformerFactory(credClient, resyncPeriod, namespace, func(*metav1.ListOptions) {}) store.informers.Kong.Credential = credentialFactory.Configuration().V1().KongCredentials().Informer() store.listers.Kong.Credential = store.informers.Kong.Credential.GetStore() - store.informers.Kong.Credential.AddEventHandler(annotatedCrdEventHandler) + store.informers.Kong.Credential.AddEventHandler(credentialEventHandler) confClient, _ := configurationclientv1.NewForConfig(clientConf) configFactory := configurationinformer.NewFilteredSharedInformerFactory(confClient, resyncPeriod, namespace, func(*metav1.ListOptions) {})