Adds Contour IngressClass Name Support #239

danehans · 2021-03-04T01:44:41Z

Adds support for specifying Contour's IngressClass Name, i.e. the --ingress-class-name flag. If unspecified, Contour processes all ingress objects without an ingress-class annotation or with an annotation matching ingress-class=contour.

Fixes: #228

Signed-off-by: Daneyon Hansen daneyonhansen@gmail.com

youngnick

I think we need to be really careful about this, because it can interact with the cluster default IngressClass in ways that can be surprising.

youngnick · 2021-03-04T02:55:49Z

api/v1alpha1/contour_types.go

@@ -89,6 +89,17 @@ type ContourSpec struct {
 	// +kubebuilder:validation:MaxLength=253
 	// +optional
 	GatewayClassRef *string `json:"gatewayClassRef,omitempty"`
+
+	// IngressClassName is the name of the IngressClass used by Contour. If unset,


I think there needs to be a bit more subtlety here, sadly. The Contour controller currently defaults to assuming it's the default in the cluster, indicated by an unset or "" value for the --ingress-class-name argument/field.

I think the contract for this field needs to be:

unset: default for cluster. The operator sets everything up so that Contour is the default (either no IngressClass created or create one and set it as the default, I can't remember which is preferred for IngressClass)

set: the flag is set to the given value, and an IngressClass is created that fits that contract. It is not set as the cluster default.

The value can't default to contour because that will imply the second behavior when it's passed to the controller, sadly.

The Contour controller currently defaults to assuming it's the default in the cluster, indicated by an unset or "" value for the --ingress-class-name argument/field.

@youngnick according to the contour ingressclass docs, "contour" is the default value of --ingress-class-name. Therefore, I'm having the operator provide the same default behavior, using api defaulting instead of the ingressClassName field being a pointer. Are the contour ingressclass docs incorrect regarding "--ingress-class-name=contour" or am I misunderstanding the docs?

If unset Contour processes ALL ingress objects without an ingress-class annotation or with an annotation matching ingress-class=contour.

When you set the ingress-class explicitly, the Contour ONLY processes ingress objects that match that class.

So the default should be no ingress class, then is someone wants to configure one, they can.

So now that this is a point, we have a clear distinction between "unset" and "set", and we can replicate the Contour behavior? If so, looks good to me.

@youngnick yes, the latest commit makes contour.spec.IngressClassName optional to replicate Contour's default behavior: If unspecified, Contour processes all ingress objects without an ingress-class annotation or with an annotation matching ingress-class=contour. If specified, Contour processes ingress objects with an annotation matching the value of contour.spec.IngressClassName. I've updated the description of the PR accordingly.

youngnick · 2021-03-04T02:56:52Z

internal/objects/deployment/deployment.go

@@ -132,6 +134,10 @@ func DesiredDeployment(contour *operatorv1alpha1.Contour, image string) *appsv1.
 			args = append(args, fmt.Sprintf("--envoy-service-https-port=%d", port.PortNumber))
 		}
 	}
+	// Pass the IngressClass name if it's not the default.
+	if contour.Spec.IngressClassName != ContourIngressClassName {


Ah, I said earlier that it couldn't have a default, if we do it like this, we can have a contour default, I suppose.

Yeah, I prefer being explicit with fields over using pointers.

If someone explicitly sets the field in YAML to "" it won't be overriden by the default in the kubebuilder validation setup right? Should we also check the value isn't "" here?

@sunjayBhatia a Contour with ingressClassName: "" would fail api validation due to +kubebuilder:validation:MinLength=1.

Example:

$ cat <<EOF | kubectl apply -f - > apiVersion: operator.projectcontour.io/v1alpha1 > kind: Contour > metadata: > name: contour-sample > spec: > ingressClassName: "" > networkPublishing: > envoy: > type: NodePortService > EOF The Contour "contour-sample" is invalid: spec.ingressClassName: Invalid value: "": spec.ingressClassName in body should be at least 1 chars long

Ha! good call

danehans · 2021-03-11T22:41:36Z

@stevesloka commit ca392be resolves your comments above.

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

youngnick

LGTM assuming I'm right about how everything will flow?

youngnick · 2021-03-16T00:27:45Z

api/v1alpha1/contour_types.go

@@ -89,6 +89,17 @@ type ContourSpec struct {
 	// +kubebuilder:validation:MaxLength=253
 	// +optional
 	GatewayClassRef *string `json:"gatewayClassRef,omitempty"`
+
+	// IngressClassName is the name of the IngressClass used by Contour. If unset,


So now that this is a point, we have a clear distinction between "unset" and "set", and we can replicate the Contour behavior? If so, looks good to me.

danehans · 2021-03-17T03:28:41Z

LGTM assuming I'm right about how everything will flow?

@youngnick thanks for the review. Yes, I describe the details in #239 (comment).

sunjayBhatia

LGTM, looks like it follows the outline set up in the controller repo for the flag 👍🏽

stevesloka · 2021-03-17T16:07:36Z

api/v1alpha1/contour_types.go

+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=253
+	// +optional
+	IngressClassName *string `json:"ingressClassName,omitempty"`


Are there any more "ingress-y" things vs gatewayapi things we'd want to split apart into structs? We've done this with the "policy" stuff in Contour to allow ways to grow values and put then in categories.

Just trying to think ahead before we get too far down the road.

+1 thats a good point

@stevesloka I can't think of anything from an Ingress standpoint. Doing a quick review of the Contour ConfigMap, I don't see anything Ingress-specific that may be exposed through the Contour API. I see Contour has a ingress-status-address flag. Is this a commonly used flag?

As for Gateway API, Contour references a GatewayClass and vice versa. A GatewayClass is pretty minimal. Are there any other config knobs that you expect to expose from Contour to support the Gateway API?

👍 in general that adding some nesting to the spec here will probably be helpful long-term as we add more and more toggles. I guess we still have room to move things around given that the API is alpha right now, though.

skriss

LGTM

skriss · 2021-03-18T15:27:27Z

api/v1alpha1/contour_types.go

+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=253
+	// +optional
+	IngressClassName *string `json:"ingressClassName,omitempty"`


👍 in general that adding some nesting to the spec here will probably be helpful long-term as we add more and more toggles. I guess we still have room to move things around given that the API is alpha right now, though.

danehans requested a review from a team as a code owner March 4, 2021 01:44

danehans requested review from stevesloka, skriss and youngnick and removed request for a team March 4, 2021 01:44

danehans self-assigned this Mar 4, 2021

danehans added this to 1.14 release in Contour Project Board Mar 4, 2021

danehans added this to the 1.14.0 milestone Mar 4, 2021

danehans mentioned this pull request Mar 4, 2021

Validate IngressClassName #240

Closed

youngnick suggested changes Mar 4, 2021

View reviewed changes

danehans force-pushed the ingressclass branch 2 times, most recently from f0a180d to ca392be Compare March 11, 2021 22:40

Adds Support for Contour's IngressClass Name

6708de3

Signed-off-by: Daneyon Hansen <daneyonhansen@gmail.com>

danehans force-pushed the ingressclass branch from ca392be to 6708de3 Compare March 11, 2021 23:19

youngnick approved these changes Mar 16, 2021

View reviewed changes

sunjayBhatia approved these changes Mar 17, 2021

View reviewed changes

stevesloka reviewed Mar 17, 2021

View reviewed changes

skriss approved these changes Mar 18, 2021

View reviewed changes

danehans merged commit 1bebb2c into projectcontour:main Mar 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Contour IngressClass Name Support #239

Adds Contour IngressClass Name Support #239

danehans commented Mar 4, 2021 •

edited

youngnick left a comment

youngnick Mar 4, 2021

danehans Mar 4, 2021

stevesloka Mar 9, 2021

stevesloka Mar 9, 2021

youngnick Mar 16, 2021

danehans Mar 16, 2021

youngnick Mar 4, 2021

danehans Mar 4, 2021

sunjayBhatia Mar 8, 2021

danehans Mar 8, 2021 •

edited

sunjayBhatia Mar 9, 2021

danehans commented Mar 11, 2021

youngnick left a comment

youngnick Mar 16, 2021

danehans commented Mar 17, 2021

sunjayBhatia left a comment

stevesloka Mar 17, 2021

sunjayBhatia Mar 17, 2021

danehans Mar 17, 2021

skriss Mar 18, 2021

skriss left a comment

skriss Mar 18, 2021

Adds Contour IngressClass Name Support #239

Adds Contour IngressClass Name Support #239

Conversation

danehans commented Mar 4, 2021 • edited

youngnick 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

danehans Mar 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans commented Mar 11, 2021

youngnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans commented Mar 17, 2021

sunjayBhatia 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

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans commented Mar 4, 2021 •

edited

danehans Mar 8, 2021 •

edited