From c47550c247b1cd2a564b83cb10fcc7fb49d993f5 Mon Sep 17 00:00:00 2001 From: Nick Young Date: Mon, 6 Apr 2020 10:42:27 +1000 Subject: [PATCH] Add ingress class filtering to ingress status updating Fixes #2388 Updates #403 Move annotations to use upstream `ObjectMetaAccessor` instead of `k8s.Object`, removes an import cycle. Copy `KindOf` from k8s to `annotations_test.go` for use in the tests only, also prevents and import cycle. Move `ParseTimeout` from `k8s` to `annotation` for now as well, more import cycles. Move ingress class matching code from `dag` to `annotation`, so it's only in one place. Add ingress class filtering to `k8s.IngressStatusUpdater` `OnAdd` and `OnUpdate`. Signed-off-by: Nick Young --- internal/annotation/annotations.go | 38 +++++-- internal/annotation/annotations_test.go | 125 ++++++++++++++++++++++-- internal/{k8s => annotation}/timeout.go | 5 +- internal/dag/cache.go | 34 +++---- internal/dag/cache_test.go | 13 +-- internal/dag/policy.go | 7 +- internal/dag/policy_test.go | 4 +- internal/k8s/ingressstatus.go | 50 +++++++--- internal/k8s/objectmeta.go | 2 + 9 files changed, 214 insertions(+), 64 deletions(-) rename internal/{k8s => annotation}/timeout.go (90%) diff --git a/internal/annotation/annotations.go b/internal/annotation/annotations.go index 07aed5f859c..18e6907ce7f 100644 --- a/internal/annotation/annotations.go +++ b/internal/annotation/annotations.go @@ -20,10 +20,13 @@ import ( "time" envoy_api_v2_auth "github.com/envoyproxy/go-control-plane/envoy/api/v2/auth" - "github.com/projectcontour/contour/internal/k8s" "k8s.io/api/networking/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// DEFAULT_INGRESS_CLASS is the Contour default. +const DEFAULT_INGRESS_CLASS = "contour" + // IsKnown checks if an annotation is one Contour knows about. func IsKnown(key string) bool { // We should know about everything with a Contour prefix. @@ -101,7 +104,7 @@ func ValidForKind(kind string, key string) bool { // CompatAnnotation checks the Object for the given annotation, first with the // "projectcontour.io/" prefix, and then with the "contour.heptio.com/" prefix // if that is not found. -func CompatAnnotation(o k8s.Object, key string) string { +func CompatAnnotation(o metav1.ObjectMetaAccessor, key string) string { a := o.GetObjectMeta().GetAnnotations() if val, ok := a["projectcontour.io/"+key]; ok { @@ -184,7 +187,7 @@ func NumRetries(i *v1beta1.Ingress) uint32 { // PerTryTimeout returns the duration envoy will wait per retry cycle. func PerTryTimeout(i *v1beta1.Ingress) time.Duration { - return k8s.ParseTimeout(CompatAnnotation(i, "per-try-timeout")) + return ParseTimeout(CompatAnnotation(i, "per-try-timeout")) } // IngressClass returns the first matching ingress class for the following @@ -192,7 +195,7 @@ func PerTryTimeout(i *v1beta1.Ingress) time.Duration { // 1. projectcontour.io/ingress.class // 2. contour.heptio.com/ingress.class // 3. kubernetes.io/ingress.class -func IngressClass(o k8s.Object) string { +func IngressClass(o metav1.ObjectMetaAccessor) string { a := o.GetObjectMeta().GetAnnotations() if class, ok := a["projectcontour.io/ingress.class"]; ok { return class @@ -206,6 +209,25 @@ func IngressClass(o k8s.Object) string { return "" } +// MatchesIngressClass checks that the passed object has an ingress class that matches +// either the passed ingress-class string, or DEFAULT_INGRESS_CLASS if it's empty. +func MatchesIngressClass(o metav1.ObjectMetaAccessor, ic string) bool { + + switch IngressClass(o) { + case ic: + // Handles ic == "" and ic == "custom". + return true + case DEFAULT_INGRESS_CLASS: + // ic == "" implicitly matches the default too. + if ic == "" { + return true + } + } + + return false + +} + // MinProtoVersion returns the TLS protocol version specified by an ingress annotation // or default if non present. func MinProtoVersion(version string) envoy_api_v2_auth.TlsParameters_TlsProtocol { @@ -226,7 +248,7 @@ func MinProtoVersion(version string) envoy_api_v2_auth.TlsParameters_TlsProtocol // 2. contour.heptio.com/max-connections // // '0' is returned if the annotation is absent or unparseable. -func MaxConnections(o k8s.Object) uint32 { +func MaxConnections(o metav1.ObjectMetaAccessor) uint32 { return parseUInt32(CompatAnnotation(o, "max-connections")) } @@ -236,7 +258,7 @@ func MaxConnections(o k8s.Object) uint32 { // 2. contour.heptio.com/max-pending-requests // // '0' is returned if the annotation is absent or unparseable. -func MaxPendingRequests(o k8s.Object) uint32 { +func MaxPendingRequests(o metav1.ObjectMetaAccessor) uint32 { return parseUInt32(CompatAnnotation(o, "max-pending-requests")) } @@ -246,7 +268,7 @@ func MaxPendingRequests(o k8s.Object) uint32 { // 2. contour.heptio.com/max-requests // // '0' is returned if the annotation is absent or unparseable. -func MaxRequests(o k8s.Object) uint32 { +func MaxRequests(o metav1.ObjectMetaAccessor) uint32 { return parseUInt32(CompatAnnotation(o, "max-requests")) } @@ -256,6 +278,6 @@ func MaxRequests(o k8s.Object) uint32 { // 2. contour.heptio.com/max-retries // // '0' is returned if the annotation is absent or unparseable. -func MaxRetries(o k8s.Object) uint32 { +func MaxRetries(o metav1.ObjectMetaAccessor) uint32 { return parseUInt32(CompatAnnotation(o, "max-retries")) } diff --git a/internal/annotation/annotations_test.go b/internal/annotation/annotations_test.go index f1e49639be4..a4f1292600f 100644 --- a/internal/annotation/annotations_test.go +++ b/internal/annotation/annotations_test.go @@ -20,7 +20,6 @@ import ( ingressroutev1 "github.com/projectcontour/contour/apis/contour/v1beta1" projectcontour "github.com/projectcontour/contour/apis/projectcontour/v1" "github.com/projectcontour/contour/internal/assert" - "github.com/projectcontour/contour/internal/k8s" v1 "k8s.io/api/core/v1" "k8s.io/api/networking/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -388,7 +387,7 @@ func TestAnnotationKindValidation(t *testing.T) { valid bool } tests := map[string]struct { - obj k8s.Object + obj metav1.ObjectMetaAccessor annotations map[string]status }{ "service": { @@ -436,10 +435,10 @@ func TestAnnotationKindValidation(t *testing.T) { // Trivially check that everything specified in the global // table is valid. for _, kind := range []string{ - k8s.KindOf(&v1.Service{}), - k8s.KindOf(&v1beta1.Ingress{}), - k8s.KindOf(&ingressroutev1.IngressRoute{}), - k8s.KindOf(&projectcontour.HTTPProxy{}), + kindOf(&v1.Service{}), + kindOf(&v1beta1.Ingress{}), + kindOf(&ingressroutev1.IngressRoute{}), + kindOf(&projectcontour.HTTPProxy{}), } { for key := range annotationsByKind[kind] { t.Run(fmt.Sprintf("%s is known and valid for %s", key, kind), @@ -455,15 +454,127 @@ func TestAnnotationKindValidation(t *testing.T) { t.Run(name, func(t *testing.T) { for k, s := range tc.annotations { assert.Equal(t, s.known, IsKnown(k)) - assert.Equal(t, s.valid, ValidForKind(k8s.KindOf(tc.obj), k)) + assert.Equal(t, s.valid, ValidForKind(kindOf(tc.obj), k)) } }) } } +func TestMatchIngressClass(t *testing.T) { + + // This is a matrix test, we are testing the annotation parser + // across various annotations, with two options: + // ingress class is empty + // ingress class is not empty. + tests := map[string]struct { + fixture metav1.ObjectMetaAccessor + // these are results for empty and "contour" ingress class + // respectively. + want []bool + }{ + "ingress nginx kubernetes.io/ingress.class": { + fixture: &v1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "incorrect", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "nginx", + }, + }, + }, + want: []bool{false, false}, + }, + "ingress nginx contour.heptio.com/ingress.class": { + fixture: &v1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "incorrect", + Namespace: "default", + Annotations: map[string]string{ + "contour.heptio.com/ingress.class": "nginx", + }, + }, + }, + want: []bool{false, false}, + }, + "ingress contour kubernetes.io/ingress.class": { + fixture: &v1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "incorrect", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": DEFAULT_INGRESS_CLASS, + }, + }, + }, + want: []bool{true, true}, + }, + "ingress contour contour.heptio.com/ingress.class": { + fixture: &v1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "incorrect", + Namespace: "default", + Annotations: map[string]string{ + "contour.heptio.com/ingress.class": DEFAULT_INGRESS_CLASS, + }, + }, + }, + want: []bool{true, true}, + }, + "no annotation": { + fixture: &v1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "noannotation", + Namespace: "default", + }, + }, + want: []bool{true, false}, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + cases := []string{"", DEFAULT_INGRESS_CLASS} + for i := 0; i < len(cases); i++ { + got := MatchesIngressClass(tc.fixture, cases[i]) + if tc.want[i] != got { + t.Errorf("IngressClass match(%v): expected %v, got %v", tc.fixture, tc.want[i], got) + } + } + + }) + } +} func backend(name string, port intstr.IntOrString) *v1beta1.IngressBackend { return &v1beta1.IngressBackend{ ServiceName: name, ServicePort: port, } } + +// kindOf returns the kind string for the given Kubernetes object. +// +// The API machinery doesn't populate the metav1.TypeMeta field for +// objects, so we have to use a type assertion to detect kinds that +// we care about. +// TODO(youngnick): This is a straight copy from internal/k8s/kind.go +// Needs to be moved to a separate module somewhere. +func kindOf(obj interface{}) string { + switch obj.(type) { + case *v1.Secret: + return "Secret" + case *v1.Service: + return "Service" + case *v1beta1.Ingress: + return "Ingress" + case *ingressroutev1.IngressRoute: + return "IngressRoute" + case *projectcontour.HTTPProxy: + return "HTTPProxy" + case *ingressroutev1.TLSCertificateDelegation: + return "TLSCertificateDelegation" + case *projectcontour.TLSCertificateDelegation: + return "TLSCertificateDelegation" + default: + return "" + } +} diff --git a/internal/k8s/timeout.go b/internal/annotation/timeout.go similarity index 90% rename from internal/k8s/timeout.go rename to internal/annotation/timeout.go index 752085855db..3fc3efc38eb 100644 --- a/internal/k8s/timeout.go +++ b/internal/annotation/timeout.go @@ -11,10 +11,13 @@ // See the License for the specific language governing permissions and // limitations under the License. -package k8s +package annotation import "time" +// TODO(youngnick): This needs to move to another package, but we need to be careful +// about the import graph, this must stay a leaf node. + // ParseTimeout parses timeouts we pass in various places in a standard way. func ParseTimeout(timeout string) time.Duration { if timeout == "" { diff --git a/internal/dag/cache.go b/internal/dag/cache.go index 7f2740f8cf6..a75da2d50fa 100644 --- a/internal/dag/cache.go +++ b/internal/dag/cache.go @@ -27,9 +27,6 @@ import ( serviceapis "sigs.k8s.io/service-apis/api/v1alpha1" ) -// DEFAULT_INGRESS_CLASS is the Contour default. -const DEFAULT_INGRESS_CLASS = "contour" - // A KubernetesCache holds Kubernetes objects and associated configuration and produces // DAG values. type KubernetesCache struct { @@ -60,30 +57,21 @@ type KubernetesCache struct { // matchesIngressClass returns true if the given Kubernetes object // belongs to the Ingress class that this cache is using. func (kc *KubernetesCache) matchesIngressClass(obj k8s.Object) bool { - objectClass := annotation.IngressClass(obj) - switch objectClass { - case kc.IngressClass: - // Handles kc.IngressClass == "" and kc.IngressClass == "custom". - return true - case DEFAULT_INGRESS_CLASS: - // kc.IngressClass == "" implicitly matches the default too. - if kc.IngressClass == "" { - return true - } - } + if !annotation.MatchesIngressClass(obj, kc.IngressClass) { + kind := k8s.KindOf(obj) + om := obj.GetObjectMeta() - // Any other ingress class fails to match. - kind := k8s.KindOf(obj) - om := obj.GetObjectMeta() + kc.WithField("name", om.GetName()). + WithField("namespace", om.GetNamespace()). + WithField("kind", kind). + WithField("ingress.class", annotation.IngressClass(obj)). + Debug("ignoring object with unmatched ingress class") + return false + } - kc.WithField("name", om.GetName()). - WithField("namespace", om.GetNamespace()). - WithField("kind", kind). - WithField("ingress.class", objectClass). - Debug("ignoring object with unmatched ingress class") + return true - return false } // Insert inserts obj into the KubernetesCache. diff --git a/internal/dag/cache_test.go b/internal/dag/cache_test.go index 0f7abb640b2..10e7d08307c 100644 --- a/internal/dag/cache_test.go +++ b/internal/dag/cache_test.go @@ -18,6 +18,7 @@ import ( ingressroutev1 "github.com/projectcontour/contour/apis/contour/v1beta1" projcontour "github.com/projectcontour/contour/apis/projectcontour/v1" + "github.com/projectcontour/contour/internal/annotation" "github.com/sirupsen/logrus" v1 "k8s.io/api/core/v1" "k8s.io/api/networking/v1beta1" @@ -585,7 +586,7 @@ func TestKubernetesCacheInsert(t *testing.T) { Name: "incorrect", Namespace: "default", Annotations: map[string]string{ - "kubernetes.io/ingress.class": DEFAULT_INGRESS_CLASS, + "kubernetes.io/ingress.class": annotation.DEFAULT_INGRESS_CLASS, }, }, }, @@ -597,7 +598,7 @@ func TestKubernetesCacheInsert(t *testing.T) { Name: "incorrect", Namespace: "default", Annotations: map[string]string{ - "contour.heptio.com/ingress.class": DEFAULT_INGRESS_CLASS, + "contour.heptio.com/ingress.class": annotation.DEFAULT_INGRESS_CLASS, }, }, }, @@ -642,7 +643,7 @@ func TestKubernetesCacheInsert(t *testing.T) { Name: "kuard", Namespace: "default", Annotations: map[string]string{ - "contour.heptio.com/ingress.class": DEFAULT_INGRESS_CLASS, + "contour.heptio.com/ingress.class": annotation.DEFAULT_INGRESS_CLASS, }, }, }, @@ -654,7 +655,7 @@ func TestKubernetesCacheInsert(t *testing.T) { Name: "kuard", Namespace: "default", Annotations: map[string]string{ - "kubernetes.io/ingress.class": DEFAULT_INGRESS_CLASS, + "kubernetes.io/ingress.class": annotation.DEFAULT_INGRESS_CLASS, }, }, }, @@ -699,7 +700,7 @@ func TestKubernetesCacheInsert(t *testing.T) { Name: "kuard", Namespace: "default", Annotations: map[string]string{ - "contour.heptio.com/ingress.class": DEFAULT_INGRESS_CLASS, + "contour.heptio.com/ingress.class": annotation.DEFAULT_INGRESS_CLASS, }, }, }, @@ -711,7 +712,7 @@ func TestKubernetesCacheInsert(t *testing.T) { Name: "kuard", Namespace: "default", Annotations: map[string]string{ - "kubernetes.io/ingress.class": DEFAULT_INGRESS_CLASS, + "kubernetes.io/ingress.class": annotation.DEFAULT_INGRESS_CLASS, }, }, }, diff --git a/internal/dag/policy.go b/internal/dag/policy.go index f7e968fc1b7..d6a0b166630 100644 --- a/internal/dag/policy.go +++ b/internal/dag/policy.go @@ -21,7 +21,6 @@ import ( ingressroutev1 "github.com/projectcontour/contour/apis/contour/v1beta1" projcontour "github.com/projectcontour/contour/apis/projectcontour/v1" "github.com/projectcontour/contour/internal/annotation" - "github.com/projectcontour/contour/internal/k8s" "k8s.io/api/networking/v1beta1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" @@ -136,7 +135,7 @@ func ingressrouteTimeoutPolicy(tp *ingressroutev1.TimeoutPolicy) *TimeoutPolicy // due to a misunderstanding the name of the field ingressroute is // Request, however the timeout applies to the response resulting from // a request. - ResponseTimeout: k8s.ParseTimeout(tp.Request), + ResponseTimeout: annotation.ParseTimeout(tp.Request), } } @@ -145,8 +144,8 @@ func timeoutPolicy(tp *projcontour.TimeoutPolicy) *TimeoutPolicy { return nil } return &TimeoutPolicy{ - ResponseTimeout: k8s.ParseTimeout(tp.Response), - IdleTimeout: k8s.ParseTimeout(tp.Idle), + ResponseTimeout: annotation.ParseTimeout(tp.Response), + IdleTimeout: annotation.ParseTimeout(tp.Idle), } } func ingressrouteHealthCheckPolicy(hc *ingressroutev1.HealthCheck) *HTTPHealthCheckPolicy { diff --git a/internal/dag/policy_test.go b/internal/dag/policy_test.go index 9f6e79b8bcc..ab499f7ec23 100644 --- a/internal/dag/policy_test.go +++ b/internal/dag/policy_test.go @@ -19,8 +19,8 @@ import ( ingressroutev1 "github.com/projectcontour/contour/apis/contour/v1beta1" projcontour "github.com/projectcontour/contour/apis/projectcontour/v1" + "github.com/projectcontour/contour/internal/annotation" "github.com/projectcontour/contour/internal/assert" - "github.com/projectcontour/contour/internal/k8s" "k8s.io/api/networking/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -413,7 +413,7 @@ func TestParseTimeout(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { - got := k8s.ParseTimeout(tc.duration) + got := annotation.ParseTimeout(tc.duration) assert.Equal(t, tc.want, got) }) } diff --git a/internal/k8s/ingressstatus.go b/internal/k8s/ingressstatus.go index 4108cf69e4c..43dac1d0f06 100644 --- a/internal/k8s/ingressstatus.go +++ b/internal/k8s/ingressstatus.go @@ -14,25 +14,34 @@ package k8s import ( + "github.com/projectcontour/contour/internal/annotation" "github.com/sirupsen/logrus" v1 "k8s.io/api/core/v1" "k8s.io/api/networking/v1beta1" clientset "k8s.io/client-go/kubernetes" ) -// StatusLoadbalancerUpdater observes informer OnAdd events and +// IngressStatusUpdater observes informer OnAdd events and // updates the ingress.status.loadBalancer field on all Ingress // objects that match the ingress class (if used). type IngressStatusUpdater struct { - Client clientset.Interface - Logger logrus.FieldLogger - Status v1.LoadBalancerStatus + Client clientset.Interface + Logger logrus.FieldLogger + Status v1.LoadBalancerStatus + IngressClass string } func (s *IngressStatusUpdater) OnAdd(obj interface{}) { - ing := obj.(*v1beta1.Ingress).DeepCopy() - // TODO(dfc) check ingress class + ing := obj.(*v1beta1.Ingress).DeepCopy() + if !annotation.MatchesIngressClass(ing, s.IngressClass) { + s.Logger. + WithField("name", ing.GetName()). + WithField("namespace", ing.GetNamespace()). + WithField("ingress-class", annotation.IngressClass(ing)). + Debug("unmatched ingress class, skip status update") + return + } ing.Status.LoadBalancer = s.Status _, err := s.Client.NetworkingV1beta1().Ingresses(ing.GetNamespace()).UpdateStatus(ing) @@ -45,14 +54,29 @@ func (s *IngressStatusUpdater) OnAdd(obj interface{}) { } func (s *IngressStatusUpdater) OnUpdate(oldObj, newObj interface{}) { - // Ignoring OnUpdate allows us to avoid the message generated - // from the status update. - // TODO(dfc) handle these cases: - // - OnUpdate transitions from an ingress class which is out of scope - // to one in scope. - // - OnUpdate transitions from an ingress class in scope to one out - // of scope. + oldIng := oldObj.(*v1beta1.Ingress).DeepCopy() + newIng := newObj.(*v1beta1.Ingress).DeepCopy() + + // We need to only act when things come *into* our ingressclass scope. When they fall out, we don't care about them any + // more, and it's the new controller's job to fix things. + // Note that this also handles the case where someone deletes the annotation + if !annotation.MatchesIngressClass(oldIng, s.IngressClass) && annotation.MatchesIngressClass(newIng, s.IngressClass) { + // Add status because we started matching ingress-class. + s.Logger. + WithField("name", newIng.GetName()). + WithField("namespace", newIng.GetNamespace()). + WithField("ingress-class", annotation.IngressClass(newIng)). + Debug("Updated Ingress is in scope, updating") + newIng.Status.LoadBalancer = s.Status + _, err := s.Client.NetworkingV1beta1().Ingresses(newIng.GetNamespace()).UpdateStatus(newIng) + if err != nil { + s.Logger. + WithField("name", newIng.GetName()). + WithField("namespace", newIng.GetNamespace()). + WithError(err).Error("unable to update status") + } + } } func (s *IngressStatusUpdater) OnDelete(obj interface{}) { diff --git a/internal/k8s/objectmeta.go b/internal/k8s/objectmeta.go index 59e1f2fdd46..459065b724f 100644 --- a/internal/k8s/objectmeta.go +++ b/internal/k8s/objectmeta.go @@ -18,6 +18,8 @@ import ( ) // Object is any Kubernetes object that has an ObjectMeta. +// TODO(youngnick): Review references to this and replace them +// with straight metav1.ObjectMetaAccessor calls if we can. type Object interface { metav1.ObjectMetaAccessor }