v1alpha2 feedback

[howardjohn]

I started working on implementing v1alpha2 and came across a few minor things that I think could use tweaks. Opening a single issue to capture them, and may use as a running log if I have any others.. we can split out larger ones if needed

• Clarify docs for Admitted status condition #821
• "If all hostnames do not match, then the HTTPRoute is not admitted". Should be "any", not "all", I think? In the same doc it says "Hostnames that do not match will be ignored." which implies its ok for only a subset to match.
• Scope in object references may be unnecessary #838
• ParentRefs - if the kind is unsupported, should we silently ignore it or report an error in status? It seems like maybe we shouldn't because it could be handled by another controller. What about invalid Scope/Namespace? That one we also probably can't tell we own it
• Listener.Name and corresponding status - should be +optional and a pointer? to be consistent with the docs. This impacts the YAML - name is required but docs say its optional.
• ListenerStatus.SupportedKinds - maybe nitpick, but "must represent the kinds an implementation supports for the specified protocol". Isn't it a property of protocol+tls settings?
• Document that Is HTTPS + Passthrough TLS is not supported #840
• multi crds: v1alpha1 "wins" on kubectl get gateway. We should have the kustomize only deploy v1alpha2 to the cluster. PR Versioning CRD yaml to simplify only installing v1alpha2 #789
• listener.Hostname ambiguity for protocols. "This field can be omitted for protocols that don't require hostname based matching." - what does it mean if its there for TCPRoute? Is it valid? Does it do anything? Since TLSRoute now has hostnames like HTTPRoute, it should IMO also have the same logic described for HTTPRoute there as well.
• ReferencePolicy.spec.from.group: When empty, the "core" API group is inferred.. But its required. so are users supposed to do group: ""? Is group: "core" legal? EDIT: read examples, looks like group: "" is the expectation. A bit odd but ok. I feel like default of core may be a good idea here - the most common usage will be Service and Secret probably (and routes I guess)

[robscott]

Thanks @howardjohn! This is a great list, I've added this issue to the v1alpha2 milestone.

[robscott]

Listener.Name and corresponding status - should be +optional and a pointer? to be consistent with the docs. This impacts the YAML - name is required but docs say its optional.

Looks like this got out of sync, but there was a relatively last minute change to make this field required. I missed updating the docs to correspond with that change.

[robscott]

ParentRefs - if the kind is unsupported, should we silently ignore it or report an error in status? It seems like maybe we shouldn't because it could be handled by another controller.

I think it should be ignored. Each implementation can't know what is or isn't valid for other implementations.

What about invalid Scope/Namespace? That one we also probably can't tell we own it.

If a controller can tell that a Route is trying to attach to a Gateway but not allowed to, I think that should be communicated in Route status at a minimum, maybe even Gateway status.

[youngnick]

Awesome list, thanks @howardjohn.

A couple thoughts:

Route: Admitted status docs say its only when Hostname does match. What about namespace, route type, etc not matching? Obviously we should not include them, but do we mark status as Admitted=false?
"If all hostnames do not match, then the HTTPRoute is not admitted". Should be "any", not "all", I think? In the same doc it says "Hostnames that do not match will be ignored." which implies its ok for only a subset to match.

Good catch, I think it should be something like:

• When a whole route is not admitted, then the Admitted Condition must be ensured as status: true
• If at least one hostname does not match, the HTTPRoute is not admitted.
• Hostnames that do not match are silently ignored (for now because I can't think of a way to do the status cleanly)

In the above, I'm using "ensured" to mean "must be present and". (I have on my list to do a pass over the status stuff to clarify what Conditions should be present and when, tl;dr I think all Conditions should always be present, but that's a discussion for another issue).

ParentRef.Scope is redundant. I know sig-apimachinery suggested it, but I disagree. Its inconsistent with Kubernetes - I don't run kubectl get clusterrole --scope Cluster, I run kubectl get clusterrole and the api-server knows it is cluster scoped. Why make users specify this boilerplate? It also gives users another way to create invalid config (ie Scope=Cluster Kind=Gateway); a great API makes it hard to create invalid configs.

I think this is a good point, and that if we are going to have Scope, it should default to namespaced, and only be required to be set in the event that scope is "Cluster". I feel like the scope requirement is mainly about ensuring that we don't end up with scope creep and inadvertent cross-namespace references, but I'm not sure.

ListenerStatus.SupportedKinds - maybe nitpick, but "must represent the kinds an implementation supports for the specified protocol". Isn't it a property of protocol+tls settings?

Yeah, it's really a mutli-value key of port, protocol, TLS settings. I think it's probably more correct to say "for the specified Listener", but that makes it a little redundant.

Is HTTPS + Passthrough TLS supported? Or do you have to declare the protocol as "TLS" in that case. I think its the latter but the docs could be more specific

I agree that Passthrough TLS should only be supported on TLSRoute objects, under a "TLS" protocol listener. Otherwise, it implies a level of visibility into the HTTP stream that TLS passthrough doesn't allow.

multi crds: v1alpha1 "wins" on kubectl get gateway. We should have the kustomize only deploy v1alpha2 to the cluster. PR Versioning CRD yaml to simplify only installing v1alpha2 #789

I think that we should also strongly recommend that people pull down their resources, remove v1alpha1, and then install v1alpha2 as the upgrade flow. I don't think that it's a good idea to have both versions in a cluster at once because of things like the above.

[hbagdi]

I think this is a good point, and that if we are going to have Scope, it should default to namespaced, and only be required to be set in the event that scope is "Cluster". I feel like the scope requirement is mainly about ensuring that we don't end up with scope creep and inadvertent cross-namespace references, but I'm not sure.

We already set the default to Namespace.
The same scope field is present in GatewayClass's ParametersRef as well so we must be careful.

Is HTTPS + Passthrough TLS supported?

I don't think we can even if we wanted to. So no.

ParentRefs - if the kind is unsupported, should we silently ignore it or report an error in status? It seems like maybe we shouldn't because it could be handled by another controller. What about invalid Scope/Namespace? That one we also probably can't tell we own it

If you (Gateway) are referenced in the ParentRefs, then it is your responsibility to report the status.
The larger problem is that the controller in question might not be even watching the Resource so it can't do anything to the resource.

[shaneutt]

@howardjohn / @robscott can we split these checkboxes off into issues? Github has a feature to do that and will let you convert a task to an issue, replacing the current text with a link to the new issue. I know its a lot of issues to create but from my perspective being able to just assign a couple issues to myself and get them done helps me organizationally.

[robscott]

I think everything is either already accounted for or being worked on. I checked off items that were either already fixed or will be by #839. The remaining 2 items have follow up issues assigned. I think we can close this one out once #839 merges and leave the other 2 issues open.

[robscott]

Closing this since the only remaining issue is tracked by #838

/close

[k8s-ci-robot]

@robscott: Closing this issue.

In response to this:

Closing this since the only remaining issue is tracked by #838

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.