KEP-5311: Relaxed validation for Services names

[adrianmoisey]

• One-line PR description: Relaxed validation for Services names

• Issue link: Relaxed validation for Services names #5311

• Other comments:

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[danwinship]

Suggested change

	with the validation requirements of other resources in Kubernetes.
	with the validation requirements of names of other resources in Kubernetes.

[danwinship]

should talk more here about the validation you talked about under "Risks and Mitigations". At what point will that happen?

[adrianmoisey]

Um, I don't know. I may need some guidance here.
The risks internal to k/k will be handled with integration and e2e tests (ie: the Ingress resource)

As for the risks external to k/k (consumers of Services and Ingress resources), I'm not really sure. Is there something this KEP should call out?
I remember seeing another KEP mentioning waiting for enough clouds to get onboard with the idea before graduating to GA, so I assume something similar needs to happen here, but I'm not really sure how to reasonability approach this.

[adrianmoisey]

How should this risk be mitigated?
For ingress-nginx I'm happy to make a PR to change its validation, but that assumes it will one day bump to using k8s.io/apimachinery 0.34.0 (or higher).

What about the unknowns? There could be other service meshes, Ingress controllers, etc that are doing their own validation. Is it in scope of this KEP to identify those and get them fixed?

[danwinship]

If I'm understanding the code, it says that the value of the default-backend annotation has to be a DNS1035Label. So that just means that if you create a Service with a DNS1123Label name, you won't be able to refer to that Service from an ingress-nginx default-backend annotation (until they change the ingress-nginx validation). So this isn't really a big deal; you can just not use a 1123 name in that case.

doing their own validation. Is it in scope of this KEP to identify those and get them fixed?

If there are problems, they're probably not going to be because of people doing conflicting validation, they're going to be because of people putting Service names into contexts where an initial digit would screw things up.

Like, for instance, if you write out a YAML file by hand, and put the service name into a field, currently it is guaranteed that that field would get parsed as a string. But with this KEP, it would be possible to have a service whose name was entirely numeric, so if you wrote that out, the consumer of the YAML would find an integer value there rather than a string value and probably break.

Yeah, yeah, you could break it before with a service named "true" too. Yay YAML. Anyway, you get the idea though. People might use service names in contexts where the newly-allowed characters could cause problems.

And there's no way you could find all such places, since this could just be in a script that some random k8s admin somewhere wrote.

So, this is definitely something to worry about, but in the end, we have to just be like "eh... probably not going to happen".

[adrianmoisey]

If I'm understanding the code, it says that the value of the default-backend annotation has to be a DNS1035Label. So that just means that if you create a Service with a DNS1123Label name, you won't be able to refer to that Service from an ingress-nginx default-backend annotation (until they change the ingress-nginx validation). So this isn't really a big deal; you can just not use a 1123 name in that case.

🤦 You're right. I found the function and just assumed how it would be used, without actually reading the context. Thanks for making up for my laziness :/

So, this is definitely something to worry about, but in the end, we have to just be like "eh... probably not going to happen".

If I'm hearing you right, while a concern, it's really out of scope of this KEP?

[adrianmoisey]

I think this is in a good place for another review.
I struggle a little with the order of operation of things like the PRR and all that.

[danwinship]

Suggested change

	- Updates of Ingress will use the new `NameIsDNSLabel()` validation for `.spec.[]rules.http.[]paths.backend.service.name` if that field changes, otherwise, there is no validation
	- Updates of Ingress will use the previous `NameIsDNS1035Label()` validation for `.spec.[]rules.http.[]paths.backend.service.name` if that field changes, otherwise, there is no validation

What this is talking about is:

• User upgrades to 1.34 and enables the feature gate.
• User creates a new Service 5311-kep-example and an new Ingress kep-example that points to that Service via .spec.[]rules.http.[]paths.backend.service.name
• User disables the feature gate
• At this point, Services can't have non-1035 names, and Ingresses aren't allowed to refer to non-1035 names, but the user already has a non-1035-named Service and an Ingress that refers to it, and we don't want them to just break. So:
  • We let the user make updates to the 5311-kep-example Service, even though that Service would be considered invalid if they tried to create it now.
  • We let the user make updates to the kep-example Ingress as long as they don't modify the rule referring to 5311-kep-example.
  • We let the user update the kep-example Ingress to fix the 5311-kep-example rule to refer to kep-example instead (because we would validate the modified rule with NameIsDNS1035Label, and find it valid).
  • We don't let the user update the kep-example Ingress to add a new rule referring to 5311-kep-example (because we would validate the new rule with NameIsDNS1035Label and find it invalid).

[thockin]

The usual pattern is to check the gate in pkg/registry/.../strategy.go and pass a flag or "opts" struct into the Validation function. Then validation checks the flag/opts and changes its behavior based on that. Then you can test both paths easily.

[adrianmoisey]

Great, thanks. I think I had assumed that once a field was using the new validation, it should continue to use that validation, but ye, I see what you're saying.

[thockin]

To clarify - Service name is immutable so you can just skip validation of it on updates. The field referenced here SHOULD continue to use the original validation. We don't want saved objects to suddenly become invalid.

I said "check the gate in pkg/registry/.../strategy.go" what I really meant was to apply the usual logic of "if the gate is set OR the field is already using the gated functionality". Put another way: if the gate is off, the door is shut, but anyone who is already inside is allowed to remain inside. :)

In declarative validation this is being implemented as "if newVal == oldVal, skip validation entirely". But that requires plumbing oldVal into every validation function. In most handwritten code it becomes somethign like:

opts.ServiceNameRelaxed = <read feature gate>
if isValidDNSLabel(field) && !isValidDNS1035Label) {
    opts.ServiceNameRelaxed = true
}

[adrianmoisey]

My response was to Dan, about Ingress.spec.rules[].http.paths[].backend.service.name.
I'm unsure if we're crossing threads here.

I guess what I'm now not clear about is the Ingress.spec.rules[].http.paths[].backend.service.name field.

Let's assume that a user turns the feature gate on, sets that field to "1-adrian", then turns the feature gate off. Now they edit it again, and change the field to "2-adrian", is that situation allowed here?

I think what I'm hearing Dan say is "During Ingress updates, only do validation if that field changes, otherwise do no validation", and what I'm hearing Tim say is "During Ingress updates , do validation on that field. If it matches the new validation, then validate using that".

Apologies if I'm not catching what you're saying!

[adrianmoisey]

Based on the existing code, the easiest path forward will be "If this Ingress resource currently uses the new validation on any of its service refs, allow that Ingress resource to continue validated using the new validation as a whole".
I think this is what Tim is saying?

[thockin]

Let's assume that a user turns the feature gate on, sets that field to "1-adrian", then turns the feature gate off. Now they edit it again, and change the field to "2-adrian", is that situation allowed here?

While we should say "no, only the existing value", it's just not worth the effort to prevent that, so we usually do not bother.

the easiest path forward will be "If this Ingress resource currently uses the new validation on any of its service refs, allow that Ingress resource to continue validated using the new validation...

Right - finer granularity than that is not super important. As declarative validdation comes on-line, we will get a lot of that for free.

[adrianmoisey]

Right, that makes sense. I've implemented it that was in the code I'm working on.

Should I update the KEP to also say that if an Ingress resource contains a new-validated Service name, then the entire Ingress resource is eligible to use the new-validated Service name?

[danwinship]

Suggested change

	- Updates of Services doesn't change, as the `metada.name` field is immutable
	- Updates of Services will no longer validate `metadata.name`, since the field is immutable, so the existing value can be assumed to be valid.

I said before that you "didn't need to worry about" this case, but that's not true; the current code always validates the name, and you need to change it so that it only validates the name on Creates, and skips re-validating it on Updates.

[thockin]

Yes, it looks like ValidateServiceUpdate() calls ValidateService(). You can pass a flag, e.g. isUpdate bool and change name validation to pass a do-nothing functionm, like:

nameFn := ValidateServiceName,
if isUpdate {
    nameFn = func(_ string, _ bool) []string { return nil }
}

[danwinship]

("No" is a legitimate answer here for simple KEPs.)

[adrianmoisey]

I left this one unanswered, as it was under the "Rollout, Upgrade and Rollback Planning" heading, which has the comment "This section must be completed when targeting beta to a release."

So I plan on doing the manual testing after writing the code.

[soltysh]

It's fine to leave it no for now, for beta you'll need to put here a description of upgrade->downgrade->upgrade manual testing.

[thockin]

Adding a few details to Dan's good comments.

[thockin]

Yes, it looks like ValidateServiceUpdate() calls ValidateService(). You can pass a flag, e.g. isUpdate bool and change name validation to pass a do-nothing functionm, like:

nameFn := ValidateServiceName,
if isUpdate {
    nameFn = func(_ string, _ bool) []string { return nil }
}

[thockin]

The usual pattern is to check the gate in pkg/registry/.../strategy.go and pass a flag or "opts" struct into the Validation function. Then validation checks the flag/opts and changes its behavior based on that. Then you can test both paths easily.

[thockin]

Do we really need integration AND validation for this? We don't generally exercise name validation in e2e (so expensive).

@aojea

[aojea]

I understand the sentence as it will be a single e2e test, that will create a Service with the new name format and perform a DNS lookup, so the validation will be tested implicitly not as a different test. We need this test to be promoted to conformance or we get the risk of new Service names will not be processed by DNS implementers.

[aojea]

This needs to be a Conformance test

[adrianmoisey]

Listed under e2e tests already: https://github.com/kubernetes/enhancements/pull/5315/files#diff-02440b3714bbc7b0b3116621ca36837b9b4a9289d483bf98de941679121077aaR237
And I've added this note under GA:
https://github.com/kubernetes/enhancements/pull/5315/files#diff-02440b3714bbc7b0b3116621ca36837b9b4a9289d483bf98de941679121077aaR253

[aojea]

/lgtm

only comments for clarifications and suggestions on the tests implementations

[danwinship]

lgtm... not sure if Antonio wanted changes now or later?

[thockin]

/approve for API - Dan and Antonio have the LGTM

[adrianmoisey]

/assign soltysh

(For PRR)

[danwinship]

/lgtm

[soltysh]

Several comments, but this is mostly ok

[soltysh]

This not entirely true, is it? We only validate service when creating, so theoretically they should function. One won't be able to create the services which rely on relaxed validation? Right?

[danwinship]

No, currently ValidateServiceCreate and ValidateServiceUpdate use a shared ValidateService function that mostly doesn't pay attention to the distinction between creates and updates.,

[soltysh]

Ok, that makes sense.

[soltysh]

I agree there's no explicit metric, but one might suggest looking at increased level of failed Service creations, which may signal a problem with mismatched versions, as described in the rollout/rollback failure above.

[adrianmoisey]

Great, thanks, I've updated this

[soltysh]

It's fine to leave it no for now, for beta you'll need to put here a description of upgrade->downgrade->upgrade manual testing.

[aojea]

/lgtm

[thockin]

/lgtm

[soltysh]

/approve
the PRR

[soltysh]

That's reasonable - thx.

[soltysh]

Ok, that makes sense.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianmoisey, soltysh, thockin

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:

• keps/prod-readiness/OWNERS [soltysh]
• keps/sig-network/OWNERS [thockin]

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