Add validation for Kubernetes object name fields and ProtocolType

[youngnick]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR ensures that there are no required fields that allow empty values, and that all required fields have consistent validation.

It does this by introducing a new string type alias, ObjectName, for Kubernetes object names, and by ensuring that the Listener ProtocolType has validation.

On the plus side, that was all the required types that could be zero I could find. The Port number field already has validation to ensure it's between 1 and 65535.

Which issue(s) this PR fixes:

Fixes #867

Does this PR introduce a user-facing change?:

Validation for Kubernetes object names has been updated
Listener ProtocolType now has validation

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: youngnick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [youngnick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[howardjohn]

Sorry if my regex knowledge is wrong here but I think this breaks the examples for ExternalService types where we have things like google.com?

Or I guess that is fine since there is no /. But a UDS would be broken. Or a GCS bucket. Object references need not be Kubernetes objects and therefore shouldn't be subject to validation in my opinion

[youngnick]

I have assumed that the ObjectReferences here must be to Kubernetes objects. I can see that it would be handy to be able to fudge a direct reference to a non-Kubernetes thing, but I guess I had assumed that in that case there would be some Kubernetes object to represent the external object first.

If we are not going to require that references are to Kubernetes objects, then we should record that fact alongside the all the object name references.

[hbagdi]

+1 to both points by Nick.

[youngnick]

Dang, I should have talked about this in the meeting today. I'll start a Slack thread so we can go async.

[jpeach]

I have assumed that the ObjectReferences here must be to Kubernetes objects.

Yeh, I had the same assumption.

[youngnick]

Okay, I'll leave this until we can come to some agreement about the project's stance on if the references must be to Kubernetes objects only, https://kubernetes.slack.com/archives/CR0H13KGA/p1632187311091400 is the Slack thread to discuss.

[robscott]

The Slack thread referenced by @youngnick above has gotten fairly long. Here's a summary of what I think the current state is for each of the two relevant questions.

1. Should we validate Name in ObjectRefs?
This is nuanced, and I don't think there's consensus here yet. My personal opinion is that we should not. Each Kubernetes resource can define its own name validation (example: EndpointSlice, others. Although the validation proposed here currently represents the least restrictive validation I'm aware of among current k8s objects, there is no guarantee that will be true in the future. The corresponding docs highlight the validation described here as the most common name validations, but do not provide any guarantees that resources will always fall into these buckets. The lack of upstream validation for ObjectRef names makes me think we should avoid this.

2. Should we be open to using ObjectRefs to reference non-k8s objects?
There are significant concerns raised in the Slack thread about using ObjectRefs to refer to non-k8s objects. @hbagdi summarizes it well with the following:

This API is complex enough making extension points more loosely defined and arbitrary could have effects that are hard for us to foresee today.

Although I'm personally open to the idea of referencing virtual or non-k8s resources, I don't feel as strongly about it. It seems mostly reasonable to start without this concept with the idea that we can add it later if necessary with a simple addition to docs and no change in validation.

[youngnick]

Okay, finishing up for the day but tomorrow after the meeting I will update this and remove the object name validation, and clarify that objects must be Kubernetes resources.

[youngnick]

In the latest commit, I:

• removed the validation on the ObjectName field
• Updated docs on all the reference fields with "must reference a valid Kubernetes object" language
• Added examples for ProtocolType
• Updated the regex for ProtocolType to match the given examples

[youngnick]

🤦 make generate, sigh.

[youngnick]

Well, the system works, turned out there were a couple of regex problems, which the tests picked up! Fixed now.

[robscott]

Thanks for the work on this @youngnick! I personally don't think we need regex validation for ObjectName, but also would be fine if it was added. I'm leaving a hold on here in case you want to add something based on the sig-api-machinery thread.

/lgtm
/hold

[robscott]

I followed up with sig-api-machinery and it sounds like there is a minimum set of validation we can provide here. It may be a bit difficult to translate into regex validation though, and others recommended against adding validation here. Although that minimum set of validation does not include a max length, 253 seems like a reasonable value to me based on the most common validation + ensure we don't end up with an unbounded list.

[youngnick]

I don't think that doing that level of checking is worthwhile when the apiserver will stop you creating objects with that name anyway; I think it's more important to get the "must refer to an existing type" clauses in.

/hold cancel

[robscott]

Thanks!

/lgtm