Can we get some other test cases? E.g.

• IP addresses
• including parts of a URL - http://foo.example.com, lavalamp@foo.example.com, foo.example.com/mypage, foo.example.com#myanchor

I assume all of these should be illegal?

I've added some more thorough validation that follows the pattern of other apimachinery validation tests, along with some small additions to the corresponding EndpointSlice validation tests. Amusingly, IPv4 addresses actually pass this validation, and that actually seems reasonable. Since this is validating that there are at least 3 DNS labels separated by ., and DNS labels can contain purely numbers, an IP address is considered valid. Though of course it would be interpreted as a domain name here. A common example of a numeric domain is 911.gov, though of course the TLD is not numeric, I don't think we can safely assume that all valid FQDNs will never have a numeric TLD.

Since this is validating that there are at least 3 DNS labels separated by .

Is that the behavior you want? example.com isn't valid?

It seems like it's relatively difficult to nail down a precise spec for FQDNs. Most examples do include 3 segments, including all examples found in relevant RFCs that I can find, but there's no evidence that it's actually necessary. It's entirely possible that example.com could be a FQDN in certain use cases. Given that along with the current IsFullyQualifiedName function failing on a FQDN with a trailing ., it seemed worthwhile to create a new validation function here. I've added a IsFullyQualifiedDomainName right below IsFullyQualifiedName and added comments to differentiate the validations used here.

If people use this and roll back to a prior version, the objects will not pass validation any longer.

@lavalamp Good catch, what's the best way to add a type like this? Is it better to add a change like this along with a new API version like betav1?

Or in this case, v1alpha2. But things still have to be round-trippable, so that doesn't really help. The actual way to make this safe is to release a version that understands the new address type, but doesn't permit new instances of it, followed by a next version that both understands and permits usages.

We generally permit alpha APIs to break things, so it might be OK to not do that dance. (@liggitt can confirm?)

If people use this and roll back to a prior version

The primary concern with alpha-level APIs (and fields) is around schema type compatibility, not API server validation. If this was changing the type of the field, it should do so in a new version to ensure no client written against the v1alpha1 API got a type error trying to decode an API server response. That API server with that new version should either round-trip it v1alpha1 or drop v1alpha1 entirely (which is allowed on a release boundary) and release note that existing persisted v1alpha1 endpointslices must be deleted before upgrading.

If the question is just about adding an allowed enum value to validation, that's worth a release note, but not a version bump in my opinion. Full support for upgrade/rollback of alpha/experimental features is not claimed.

The actual way to make this safe is to release a version that understands the new address type, but doesn't permit new instances of it, followed by a next version that both understands and permits usages.

That is the dance we would require for a field/resource that was beta level or above, correct.

If the question is just about adding an allowed enum value to validation

It's worse than that, the different enum effectively changes the behavior of a string field; the old system only understands IP addresses, now the new one will also permit FQDNs to be stored there.

It's worse than that, the different enum effectively changes the behavior of a string field; the old system only understands IP addresses, now the new one will also permit FQDNs to be stored there.

If it's still type-compatible (and especially if any old data is still valid and behaves the same), that seems like an in-bounds change for an alpha-level API.

Yeah, my original note was mostly meant to be informational.

Thanks for the clarification @liggitt and @lavalamp! The plan is for this API to graduate to beta in 1.17 but we'd still like to leave the option open of adding additional address types in the future. Would the best approach here be to not prevent the creation of EndpointSlices with unrecognized address types here and maybe add some kind of soft error like an event? Consumers of this API (primarily kube-proxy at this point) would then just have to filter out address types they could not handle (already necessary with this FQDN update).

No, that lets people put in land mines (e.g., it's unrecognized today, but tomorrow it's recognized and also invalid because it was entered incorrectly).

Adding new address types in a beta or GA API will require the laborious dance I described.

I recommend that you figure out the types you will eventually want, and put recognition and validation code in place, and just prevent folks from making new instances of the ones you're not ready for. Then you'll have pre-done the first step of the dance and you can enable them in one release.

However! Let me mention another potential failure mode: using these (e.g., via controller) in the same release that you enable them in apiserver. This means that the new controller, should it happen to talk to an old apiserver, will break. In theory, folks should upgrade apiserver first, but in practice, I'm not sure everyone does the upgrade in the right order (in fact, I am sure that some do not do so).

It is hard to design for hypothetical as we do not have concrete use cases for now. We entertain a few ideas, but there is no easy way to add more address types after beta. (assuming we can make breaking change in alpha) For the consumer/producer of the EndpointSlice we control, we can safe guard its internals by filtering specific types so that they are ignoring future types.

It looks inevitable to go thru the 2-release process for introducing new types in the future.

my reading of the caller is that it'd be possible to get here with an addrType of "", which sidesteps all this checking -- should probably either test or fix that.

We do have defaulting in place here that should default this to IP as an address type when it isn't specified. Above this we also have validation that ensures that the addressType is one of the supportedAddressTypes. So as long as every entry in supportedAddressTypes is also covered here, we should be covered. Though of course this could all change if it makes sense to make this validation less restrictive to simplify supporting additional address types in the future.

https://github.com/kubernetes/kubernetes/pull/84091/files#diff-d3dbc1e6a66cbe3fe6f1a843d112b96dL48-L50

I assume defaulting causes you to never take the first branch in the if statement, but if we're so confident in that, why would we even have it?

Yep, you're completely right. It just felt wrong to leave that unguarded without a nil check, but it is unnecessary so I've removed it.

Yeah, that feels pretty weird, I'd actually recommend keeping the nil check (and just setting addrType to something valid if it fails).

Sounds good. I've added the nil check back and added a comment with it to indicate that a default value should have already been set at that point.

Yeah, this function is probably just plain wrongly named. The purpose of this (IIRC) is to make people use meaningful group names. (not asking for a change unless you feel like adding a TODO)

I think it's actually got a really similar purpose to what we're doing with EndpointSlices. The only place I can find it used is for validating webhooks (https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/admissionregistration/validation/validation.go#L262 and https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/admissionregistration/validation/validation.go#L310). It's a shame to have 2 functions so similar but just slightly different, but I can't think of a great alternative.

Oh, this is probably some copy-and-paste inheritance, then. It probably should actually be the function you just wrote, but changing it is tricky. Can you add a "TODO: this function is deprecated and preserved until all callers migrate to IsFullyQualifiedDomainName; please don't add new callers"

👍 That makes sense, added that comment.

https://golang.org/pkg/strings/#Count might be a bit more natural?

I tried that but it ended up not testing precisely the same thing this does. Since the function is trying to validate a number of segments, not a number of . we get a different answer for a string like . that has one . but 0 segments.

What about just "com"?

Does the test verify that every name here is invalid? I'd expect the creation to fail if any name was invalid.

Actually, I see you have extensive testing of the relevant function (thanks!), so I'm not sure if exhaustively repeating that here is needed now.

This is a rather imprecise method of testing this validation, but it does ensure that the expected number of errors are returned. So in this case that is 4 since all addresses specified are invalid. Of course there is a chance that what is invalid changes just enough that it is missed here, but I think it's unlikely given the variety of tests. I added some additional test cases, including com to the tests covering the validation function as well.

Does IP mean just IPv4 or both that an v6? If just v4, maybe you should add a v6? If it means both, then the documentation below is confusing and should be explained:

The contents of this field are interpreted according to the corresponding EndpointSlice addressType field. This allows for cases like dual-stack (IPv4 and IPv6) networking.

This is currently meant to include both v4 and v6 addresses. When created by the EndpointSlice controller, this mirrors the value of PodIPs on the corresponding Pod (also containing v4 and v6 addresses). I'll update the comment to clarify that. It's also worth noting that there is some ongoing discussion around the merits of separating this into separate attributes, so there's a chance this could change before the next release.