add support for ingressv1.Spec.IngressClassName #2548

stevesloka · 2020-05-21T19:56:07Z

Fixes #2146 by adding support for ingressv1.Spec.IngressClassName.

Signed-off-by: Steve Sloka [email protected]

stevesloka · 2020-05-21T19:56:47Z

I'm not in love with the package name annotations, but couldn't think up a better name. Up for discussion of ideas on that. =)

codecov · 2020-05-21T20:00:04Z

Codecov Report

Merging #2548 into master will decrease coverage by 0.09%.
The diff coverage is 79.03%.

@@            Coverage Diff             @@
##           master    #2548      +/-   ##
==========================================
- Coverage   77.04%   76.94%   -0.10%     
==========================================
  Files          72       72              
  Lines        5723     5757      +34     
==========================================
+ Hits         4409     4430      +21     
- Misses       1226     1238      +12     
- Partials       88       89       +1

Impacted Files	Coverage Δ
cmd/contour/serve.go	`2.55% <0.00%> (-0.04%)`	⬇️
internal/k8s/clients.go	`11.62% <ø> (ø)`
internal/k8s/informers.go	`0.00% <0.00%> (ø)`
internal/dag/cache.go	`95.93% <84.61%> (-2.24%)`	⬇️
internal/annotation/annotations.go	`100.00% <100.00%> (ø)`
internal/dag/builder.go	`91.93% <100.00%> (ø)`
internal/k8s/statusaddress.go	`86.95% <100.00%> (+0.14%)`	⬆️

stevesloka · 2020-05-22T17:14:50Z

I took off the 1.15 milestone since this is not critical to land in that release.

jpeach

If I' reading this correctly, this change looks for an Ingress class string in the spec.ingressclassname field. However, in the v1 API, this string is the name of an object of type IngressClass which represents a deployed Contour control plane.

stevesloka · 2020-06-09T19:32:57Z

ha you're right @jpeach, I misread that spec. I'll get this fixed up. =)

github-actions · 2020-06-24T00:24:37Z

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 90 days.

stevesloka · 2020-06-30T19:26:36Z

I'm not happy with the version check logic, going to look at implementing: #2219

stevesloka · 2020-07-08T17:56:11Z

#2668 adds a dynamic lookup for resources. Once that merges, I'll rebase this PR to include that portion.

stevesloka · 2020-07-15T15:28:44Z

The current implementation checks if the IngressClass api type exists, but doesn't set an error on the status of an object if used. The only feedback is a warning emitted when Contour starts.

This is for the use-case where a cluster doesn't have ingressClasses but a user tries to use them, which I think is an edge case but still valid.

jpeach

I made a first pass over this. I think it would really help to sketch out how we think about IngressClass deployments and how we want them to evolve over time. That larger context would help illuminate the decisions in this PR.

internal/dag/cache.go

jpeach · 2020-07-29T06:27:05Z

internal/annotation/annotations.go

-func MatchesIngressClass(o metav1.ObjectMetaAccessor, ic string) bool {
-
-	switch IngressClass(o) {
+func MatchesIngressClass(objectClass, ic string) bool {


The purpose of this function is to capture the complexity around matching resources to ingress classes. Now, that is distributed across here, (*KubernetesCache) matchesIngressClass() and (*StatusAddressUpdater) OnAdd(). Can we bring is all back so that there is exactly one place that does the ingress class match?

jpeach · 2020-07-29T06:33:36Z

internal/dag/cache.go

+				return *obj.Spec.IngressClassName
+			}
+			return ""
+		}


This is what annotation.IngressClass() should do - given a generic resource, figure out and return the ingress class that it belongs to.

The problem is that we need to know if the annotation is defined at all, if it is then check if it matches. If it's not defined, then we can look at ingress class. That's why it's changed.

I agree that having one function that returns an object's ingress class would be really helpful since the logic is spread around a bunch now. It may need to also return a bool indicating if the class came from Spec.IngressClassName or not.

The problem is that we need to know if the annotation is defined at all, if it is then check if it matches. If it's not defined, then we can look at ingress class. That's why it's changed.

I think that we can still have it in one place though. The logic for IngressClass would be:

func IngressClass(o runtime.Object) string { a := o.GetObjectMeta().GetAnnotations() if class, ok := a["projectcontour.io/ingress.class"]; ok { return class } if class, ok := a["contour.heptio.com/ingress.class"]; ok { return class } if class, ok := a["kubernetes.io/ingress.class"]; ok { return class } switch obj := obj.(type) { case *v1beta1.Ingress: return obj.Spec.IngressClassName } return "" }

This treats the ingress class the same no matter how it is specified. Then we can treat the issue of whether the ingress class matches our configuration separately. We may not want to propagate the default Contour matching behaviour across the the IngressClass resource.

jpeach · 2020-07-29T06:37:27Z

internal/annotation/annotations.go

 	}
-	if class, ok := a["kubernetes.io/ingress.class"]; ok {
-		return class
+	if class, ok := annotations["kubernetes.io/ingress.class"]; ok {


Since you are working in the area, suggest adopting the v1beta1.AnnotationIngressClass constant.

jpeach · 2020-07-29T06:43:12Z

internal/dag/cache.go

+			}
+			if obj.Spec.IngressClassName != nil {
+				return *obj.Spec.IngressClassName
+			}


IMHO the Spec.IngressClassName should take precedence since it's not deprecated.

The docs used to say that annotations were to take precedence over the ingressclass, but now I can't find that reference. I know @youngnick mentioned the same on a previous call.

https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20190125-ingress-api-group.md#interoperability-with-previous-annotation

Yup you are right. From the spec:

// IngressClassName is the name of the IngressClass cluster resource. The
// associated IngressClass defines which controller will implement the
// resource. This replaces the deprecated kubernetes.io/ingress.class
// annotation. For backwards compatibility, when that annotation is set, it
// must be given precedence over this field. The controller may emit a
// warning if the field and annotation have different values.
// Implementations of this API should ignore Ingresses without a class
// specified.

Can we add a comment with the rationale and background?

jpeach · 2020-07-29T06:47:44Z

internal/dag/cache.go

@@ -139,13 +157,39 @@ func (kc *KubernetesCache) Insert(obj interface{}) bool {
 	case *v1.Service:
 		kc.services[k8s.NamespacedNameOf(obj)] = obj
 		return kc.serviceTriggersRebuild(obj)
+	case *v1beta1.IngressClass:
+		// Match only ingress classes that Contour should be able to own.
+		if obj.Spec.Controller == "projectcontour.io/ingress-controller" {


"projectcontour.io/ingress-controller" should be a named constant somewhere.

Maybe this should be "projectcontour.io/contour" 🤷

The docs show it as ingress-controller and I've seen other folks do the same: https://kubernetes.io/docs/concepts/services-networking/ingress/#ingress-class

…gress objects Add support for IngressClass objects which match the controller name of 'projectcontour.io/ingress-controller'. Any Ingress object which defines an 'Spec.IngressClassName', Contour will validate that a corresponding IngressClass exists. Current ingress class logic for Contour is not affected and still take priority over any IngressClass logic. Signed-off-by: Steve Sloka <[email protected]>

skriss · 2020-07-29T16:21:18Z

👀

skriss · 2020-07-29T16:55:10Z

cmd/contour/serve.go

@@ -258,6 +258,13 @@ func doServe(log logrus.FieldLogger, ctx *serveContext) error {
 		}
 	}

+	// Check if the resource exists in the API server before setting up the informer.
+	if !clients.ResourceExists(k8s.IngressClassResources()...) {
+		log.WithField("InformOnResources", "Ingressclass Resources").Warnf("resources %v not found in api server", k8s.IngressClassResources())


this should probably be info-level since it's not required / the user hasn't explicitly opted into using it (unlike the service API types just above)

skriss · 2020-07-29T17:15:16Z

internal/dag/cache.go

+			}
+			if obj.Spec.IngressClassName != nil {
+				return *obj.Spec.IngressClassName
+			}


https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20190125-ingress-api-group.md#interoperability-with-previous-annotation

skriss · 2020-07-29T17:39:46Z

internal/k8s/statusaddress.go

-	if !annotation.MatchesIngressClass(typed, s.IngressClass) {
+	// check if the object has an annotation, which take precedence over spec.ingressclass
+	_, classAnnotation := annotation.IngressClass(typed.GetObjectMeta().GetAnnotations())
+	if !annotation.MatchesIngressClass(classAnnotation, s.IngressClass) {


This doesn't seem like it'll correctly handle Ingresses with Spec.IngressClassName specified -- for example, if s.IngressClass != "" then won't all Ingresses that only have Spec.IngressClassName (no annotation) be filtered out here?

skriss · 2020-07-29T17:59:11Z

internal/annotation/annotations.go

-
-	switch IngressClass(o) {
+func MatchesIngressClass(objectClass, ic string) bool {
+	switch objectClass {


Not directly related to your PR, but I think this block of code would be easier to understand if we sort of inverted the logic, something like:

// if ic is specified, then the object's ingress class must match it if ic != "" { return objectClass == ic } // if ic is empty, then the object's ingress class can either be empty // or DEFAULT_INGRESS_CLASS return objectClass == "" || objectClass == DEFAULT_INGRESS_CLASS

May be a matter of personal preference though, so take it or leave it.

skriss · 2020-07-29T18:01:26Z

internal/dag/cache.go

+		kc.WithField("name", om.GetName()).
+			WithField("namespace", om.GetNamespace()).
+			WithField("kind", kind).
+			WithField("ingress-class", ic).


I think you want ic() here

skriss · 2020-07-29T18:04:32Z

internal/dag/cache.go

+				return *obj.Spec.IngressClassName
+			}
+			return ""
+		}


I agree that having one function that returns an object's ingress class would be really helpful since the logic is spread around a bunch now. It may need to also return a bool indicating if the class came from Spec.IngressClassName or not.

jpeach · 2020-07-30T02:44:52Z

internal/dag/cache.go

+		// Check if this class matches a existing IngressClass.
+		_, ok := kc.ingressclasses[*ingressClassName]
+		return ok
+	}


I thought I left a comment before, but I can't find it now :-/

IIUC, Contour will cache all IngressClass resources that match the controller name. Here, we will match any ingress class whose name matches the Spec.IngressClassName. So the result of this is that Contour will accept all resources which map to a Contour IngressClass regardless of the class name.

Did I get that right?

github-actions · 2020-08-19T00:39:40Z

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 90 days.

youngnick · 2020-08-20T06:38:17Z

I think this one won't be implementable until we've settled on a decision in #2809.

stevesloka added this to the 1.5.0 milestone May 21, 2020

stevesloka removed this from the 1.5.0 milestone May 22, 2020

jpeach self-requested a review May 23, 2020 00:28

jpeach suggested changes Jun 2, 2020

View reviewed changes

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 24, 2020

stevesloka force-pushed the ingressv1ingressclass branch from e90e82a to 4d8c977 Compare June 26, 2020 16:45

stevesloka removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 26, 2020

stevesloka added this to the 1.6.0 milestone Jun 26, 2020

stevesloka added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 26, 2020

stevesloka force-pushed the ingressv1ingressclass branch from 4d8c977 to 102de6f Compare June 26, 2020 16:54

stevesloka requested review from jpeach, youngnick and skriss June 26, 2020 16:54

stevesloka force-pushed the ingressv1ingressclass branch 4 times, most recently from 177b3b1 to 8b74bfb Compare June 26, 2020 18:04

stevesloka modified the milestones: 1.6.0, 1.7.0 Jun 29, 2020

stevesloka force-pushed the ingressv1ingressclass branch 2 times, most recently from 6d1d7bb to 9443d11 Compare June 30, 2020 17:26

stevesloka marked this pull request as draft June 30, 2020 19:27

stevesloka marked this pull request as ready for review July 15, 2020 15:26

stevesloka force-pushed the ingressv1ingressclass branch from 9443d11 to d98dc6c Compare July 15, 2020 16:29

stevesloka force-pushed the ingressv1ingressclass branch 3 times, most recently from 388cc95 to f7f7e4a Compare July 28, 2020 15:21

jpeach suggested changes Jul 29, 2020

View reviewed changes

stevesloka force-pushed the ingressv1ingressclass branch from f7f7e4a to f8a0325 Compare July 29, 2020 14:08

skriss requested changes Jul 29, 2020

View reviewed changes

jpeach reviewed Jul 30, 2020

View reviewed changes

youngnick removed this from the 1.7.0 milestone Aug 4, 2020

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 19, 2020

jpeach closed this Aug 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for ingressv1.Spec.IngressClassName #2548

add support for ingressv1.Spec.IngressClassName #2548

stevesloka commented May 21, 2020

stevesloka commented May 21, 2020

codecov bot commented May 21, 2020 •

edited

Loading

stevesloka commented May 22, 2020

jpeach left a comment

stevesloka commented Jun 9, 2020

github-actions bot commented Jun 24, 2020

stevesloka commented Jun 30, 2020

stevesloka commented Jul 8, 2020

stevesloka commented Jul 15, 2020

jpeach left a comment

jpeach Jul 29, 2020

jpeach Jul 29, 2020

stevesloka Jul 29, 2020

skriss Jul 29, 2020

jpeach Jul 30, 2020

jpeach Jul 29, 2020

jpeach Jul 29, 2020

stevesloka Jul 29, 2020

skriss Jul 29, 2020

jpeach Jul 30, 2020

jpeach Jul 29, 2020

jpeach Jul 29, 2020

stevesloka Jul 29, 2020

skriss commented Jul 29, 2020

skriss Jul 29, 2020

skriss Jul 29, 2020

skriss Jul 29, 2020

skriss Jul 29, 2020

skriss Jul 29, 2020

skriss Jul 29, 2020

jpeach Jul 30, 2020

github-actions bot commented Aug 19, 2020

youngnick commented Aug 20, 2020

add support for ingressv1.Spec.IngressClassName #2548

add support for ingressv1.Spec.IngressClassName #2548

Conversation

stevesloka commented May 21, 2020

stevesloka commented May 21, 2020

codecov bot commented May 21, 2020 • edited Loading

Codecov Report

stevesloka commented May 22, 2020

jpeach left a comment

Choose a reason for hiding this comment

stevesloka commented Jun 9, 2020

github-actions bot commented Jun 24, 2020

stevesloka commented Jun 30, 2020

stevesloka commented Jul 8, 2020

stevesloka commented Jul 15, 2020

jpeach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Jul 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 19, 2020

youngnick commented Aug 20, 2020

codecov bot commented May 21, 2020 •

edited

Loading