KEP-5311 Relaxed validation for Services names #132339

adrianmoisey · 2025-06-16T20:22:48Z

What this PR does / why we need it:

Implements the new relaxed variation for Service names

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

When `RelaxedServiceNameValidation` feature gate is enabled, the 
names of new Services names are validation with `NameIsDNSLabel()`,
relaxing the  pre-existing validation.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/issues/5311

/kind feature
/sig network
/assign @thockin @aojea @danwinship
/priority important-soon
/triage accepted

k8s-ci-robot · 2025-06-16T20:41:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: adrianmoisey
Once this PR has been reviewed and has the lgtm label, please ask for approval from thockin. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/apis/OWNERS
pkg/features/OWNERS
test/OWNERS
test/compatibility_lifecycle/reference/OWNERS

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

danwinship · 2025-06-18T15:55:29Z

pkg/features/kube_features.go

@@ -1641,6 +1647,10 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate
 		{Version: version.MustParse("1.34"), Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.37
 	},

+	RelaxedServiceNameValidation: {
+		{Version: version.MustParse("1.33"), Default: false, PreRelease: featuregate.Alpha},


danwinship · 2025-06-18T15:56:42Z

pkg/apis/core/validation/validation.go

@@ -5924,9 +5929,16 @@ var supportedServiceIPFamilyPolicy = sets.New(
 	core.IPFamilyPolicyRequireDualStack)

 // ValidateService tests if required fields/annotations of a Service are valid.
-func ValidateService(service, oldService *core.Service) field.ErrorList {
+func ValidateService(service, oldService *core.Service, isUpdate bool) field.ErrorList {


you don't need isUpdate; it's an update if oldService != nil

danwinship · 2025-06-18T15:58:27Z

pkg/apis/core/validation/validation.go

 	metaPath := field.NewPath("metadata")
-	allErrs := ValidateObjectMeta(&service.ObjectMeta, true, ValidateServiceName, metaPath)
+
+	// Avoid validating the name if this is an update, since the name is immutable.


"Avoid re-validating the name on update, to avoid problems when rolling back RelaxedServiceNameValidation"

danwinship · 2025-06-18T16:00:28Z

pkg/apis/networking/validation/validation.go

-		AllowInvalidWildcardHostRule: allowInvalidWildcardHostRule(oldIngress),
+		AllowInvalidSecretName:              allowInvalidSecretName(oldIngress),
+		AllowInvalidWildcardHostRule:        allowInvalidWildcardHostRule(oldIngress),
+		AllowedRelaxedServiceNameValidation: allowRelaxedServiceNameValidation(oldIngress),


I would just set this always true for Updates and false for Creates.

Also, the name is misleading because you obey the feature gate too... I'd make it something like SkipServiceNameValidation (and then just skip validation)

I would just set this always true for Updates and false for Creates.

Could you explain the logic behind this? Not sure I understand

ah, I forgot there can be multiple Services referred to in the Ingress. So what you have isn't right. The KEP says:

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

But what this implements is "if any service name in oldIngress requires NameIsDNSLabel, then validate every service name in ingress with NameIsDNSLabel.

To do what the KEP says, you'd have to pass oldIngress all the way down through the validation... a simpler approach might be to do it like what I did with AllowCIDRsEvenIfInvalid for NetworkPolicy validation in the same file: at the start of Update validation, make a list of all of the service names in oldIngress that don't validate with NameIsDNS1035Label, and then in validateIngressBackend, allow any of those names to be used in addition to valid names.

This still doesn't implement quite what the KEP says, but it's much closer, and a lot simpler to implement...

Given this discussion, I had assumed that what I have implemented in this PR was enough.

I'm happy to change it to be what you've suggested here, that doesn't seem too difficult.

ah... hm, ok (though when you update the KEP for beta, you should probably fix the text to match that better)

danwinship · 2025-06-18T16:03:24Z

pkg/apis/core/validation/validation_test.go

@@ -15477,6 +15478,7 @@ func TestValidateServiceCreate(t *testing.T) {
 		name           string
 		tweakSvc       func(svc *core.Service) // given a basic valid service, each test case can customize it
 		numErrs        int
+		featureGates   []featuregate.Feature


You probably started writing this before I pushed the PreferSameTrafficDistribution update that removed this field, right?

The problem is that having featureGates works during alpha when the feature is off by default and you're specifying gates that should be enabled, but then it breaks in beta when the feature is on by default. It works better to just have a bool here for whether to enable this feature gate, like we do with the other two.

You probably started writing this before I pushed the PreferSameTrafficDistribution update that removed this field, right?

Yup!

The problem is that having featureGates works during alpha when the feature is off by default and you're specifying gates that should be enabled

Ah thanks, that makes sense.

danwinship · 2025-06-18T16:05:06Z

pkg/apis/core/validation/validation_test.go

 		},
 	}

 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PreferSameTrafficDistribution, tc.newTrafficDist)
 			featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !tc.legacyIPs)
+			for i := range tc.featureGates {
+				featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, tc.featureGates[i], true)
+			}
 			svc := makeValidService()
 			tc.tweakSvc(&svc)
 			errs := ValidateServiceCreate(&svc)


you should add a test to ValidateServiceUpdate too, to ensure that you can update a service with a name that starts with a digit, even if the feature gate is disabled

danwinship · 2025-06-18T16:05:27Z

pkg/apis/networking/validation/validation_test.go

@@ -1018,6 +1019,7 @@ func TestValidateIngressCreate(t *testing.T) {
 	testCases := map[string]struct {
 		tweakIngress func(ingress *networking.Ingress)
 		expectedErrs field.ErrorList
+		featureGates []featuregate.Feature


(same comments as service test)

danwinship · 2025-06-18T16:06:13Z

test/e2e/network/dns.go

@@ -654,6 +655,36 @@ var _ = common.SIGDescribe("DNS", func() {
 		}
 		validateDNSResults(ctx, f, pod, append(agnhostFileNames, jessieFileNames...))
 	})
+
+	framework.It("should work with a host that starts with a digit", framework.WithFeatureGate(features.RelaxedServiceNameValidation), func(ctx context.Context) {


"should work with a service name that starts with a digit"

danwinship · 2025-06-18T16:06:45Z

test/e2e/network/dns.go

+			fmt.Sprintf("%s.%s.svc.%s", svcName, f.Namespace.Name, framework.TestContext.ClusterDNSDomain),
+		}
+
+		// TODO: Validate both IPv4 and IPv6 families for dual-stack


not really needed... there's no reason dual-stack in particular would break in this case

danwinship · 2025-06-19T15:32:08Z

/retitle KEP-5311 Relaxed validation for Services names

aojea · 2025-06-25T12:38:49Z

pkg/apis/core/validation/validation.go

+	nameFn := ValidateServiceName
+	if oldService != nil {
+		nameFn = func(_ string, _ bool) []string { return nil }
+	}
+
+	allErrs := ValidateObjectMeta(&service.ObjectMeta, true, nameFn, metaPath)


we already have ValidateObjectMetaUpdate that seems much better than creating the noop function

if oldService != nil {
ValidateObjectMetaUpdate
} else {
ValidateObjectMeta
}

I went with a different direction, but put it in its own commit: cf8e372 (#132339)
Let me know what you think

aojea · 2025-06-25T12:43:40Z

pkg/apis/networking/validation/validation.go

+	if oldIngress != nil {
+		for _, rule := range oldIngress.Spec.Rules {
+			for _, path := range rule.HTTP.Paths {
+				if path.Backend.Service == nil {
+					continue
+				}
+				serviceName := path.Backend.Service.Name
+				// If a name doesn't validate with NameIsDNS1035Label, but does validate with NameIsDNSLabel,
+				// then we allow it to be used as a Service name in an Ingress.
+				new := apimachineryvalidation.NameIsDNSLabel(serviceName, false)
+				old := apimachineryvalidation.NameIsDNS1035Label(serviceName, false)
+				if new == nil && old != nil {
+					return true
+				}
+			}
+		}
+	}
+	return false


nit, it is a common practice to avoid excessive indentantion, in this case you can just do

if oldIngress == nil { return false }

and you get the next section unindented

aojea · 2025-06-25T12:48:35Z

pkg/apis/networking/validation/validation.go

+				new := apimachineryvalidation.NameIsDNSLabel(serviceName, false)
+				old := apimachineryvalidation.NameIsDNS1035Label(serviceName, false)
+				if new == nil && old != nil {
+					return true
+				}


on slices you should try to avoid checking on nil, since the empty (0 elements) and nil slice has the same value for us here, use len(...) > 0 instead

https://go.dev/play/p/qabz4DFXjh-

Also give better names to the variables newValidationErrors, oldValidationErrors. ... I'm bad at this names but llms helpers use to help with this

Good callout, fixed

aojea · 2025-06-25T12:52:20Z

pkg/apis/networking/validation/validation.go

+				}
+			} else {
+				for _, msg := range apivalidation.ValidateServiceName(backend.Service.Name, false) {
+					allErrs = append(allErrs, field.Invalid(fldPath.Child("service", "name"), backend.Service.Name, msg))


this now is confusing, since this function is feature gated ... now we are introducing the feature gate logic into this function that IIRC is what we try to avoid by using the IngressValidationOptions structure

Right. I'm not too sure of a better path forward. Would it be acceptable to pass a function in via IngressValidationOptions rather than a boolean?

Oh wait, I see what you're saying. The decision needs to be set higher up.

🤦 Ok, I see what you're saying. apivalidation.ValidateServiceName is gated. Right.
I'm not sure on the best way forward. Some options I can think of:

Don't gate apivalidation.ValidateServiceName and handle the logic another way

Change validateIngressBackend to use either apimachineryvalidation.NameIsDNSLabel or apimachineryvalidation.NameIsDNS1035Label depending on the gate (leaving a comment to revert it back to apivalidation.ValidateServiceName when the feature is removed)

Some other way?

I settled on doing this: https://github.com/kubernetes/kubernetes/compare/3f0292d9d834ff1c008443e793da3d1582519833..ff34de2d9c709dfd7724506ddde46d4a15155e58

Let me know what you think

…sDNSLabel Only validate when feature gate RelaxedServiceNameValidation is enabled. Also remove name validation on Service updates, as the name is immutable.

…n.NameIsDNSLabel Only validate when feature gate RelaxedServiceNameValidation is enabled or when the Ingess resource contains a service ref that already validates with apimachineryvalidation.NameIsDNSLabel

Test the behaviour of feature gate RelaxedServiceNameValidation.

To test enabling and disabling of RelaxedServiceNameValidation feature gate

Put it into ValidateServiceCreate(), making the code path as such: ``` pkg/registry/core/service/strategy.go Validate -> validation.ValidateServiceCreate -> ValidateObjectMeta -> ValidateService ValidateUpdate -> validation.ValidateServiceUpdate -> ValidateObjectMetaUpdate -> ValidateService ``` Other resources I checked pass the update objects through ValidateObjectMeta and ValidateObjectMetaUpdate, so this breaks the pattern, but it seems to be how the ValidateObjectMeta/ValidateObjectMetaUpdate functions are designed to operate.

thockin · 2025-07-01T17:12:56Z

pkg/apis/core/validation/validation.go

@@ -5926,7 +5931,7 @@ var supportedServiceIPFamilyPolicy = sets.New(
 // ValidateService tests if required fields/annotations of a Service are valid.
 func ValidateService(service, oldService *core.Service) field.ErrorList {
 	metaPath := field.NewPath("metadata")
-	allErrs := ValidateObjectMeta(&service.ObjectMeta, true, ValidateServiceName, metaPath)


Leave a comment here to the effect of "Don't validate ObjectMeta here - that is handled in the Create/Update functions which call this."

thockin · 2025-07-01T17:13:41Z

pkg/apis/core/validation/validation.go

@@ -5926,7 +5931,7 @@ var supportedServiceIPFamilyPolicy = sets.New(
 // ValidateService tests if required fields/annotations of a Service are valid.
 func ValidateService(service, oldService *core.Service) field.ErrorList {


maybe rename to validateService to ensure no external callers get created?

thockin · 2025-07-01T17:15:42Z

pkg/apis/core/validation/validation_test.go

+			numErrs: 0,
+		}, {
+			name: "invalid: service name begins with a digit feature gate disabled",
+			tweakSvc: func(s *core.Service) {


set relaxedServiceNames: false so there's no ambiguity?

thockin · 2025-07-01T17:18:14Z

pkg/apis/core/validation/validation_test.go

@@ -19477,6 +19493,14 @@ func TestValidateServiceUpdate(t *testing.T) {
 				newSvc.Annotations[core.AnnotationLoadBalancerSourceRangesKey] = "010.0.0.0/8, 1.2.3.0/24"
 			},
 			numErrs: 1,
+		}, {
+			name: "can modify a pre-existing relaxed service name without error",


Doesn't this test need to at least assert the gate to false, to really prove it stays working?

thockin · 2025-07-01T17:57:53Z

pkg/apis/networking/validation/validation.go

+		if rule.HTTP == nil {
+			continue
+		}
+		for _, path := range rule.HTTP.Paths {


@aaron-prindle @yongruilin @jpbetz regarding declarative validation. Paths is "atomic" but a struct. It should PROBABLY have been a list-map but doesn't need multi-owner. I think we eventually need a listType which decouples the managed fields / SSA logic from the ability to do by-key ratcheting. Even the "identical value" ratcheting (which I think we should not do) would not help here -- the update validation would not get an "oldValue" for each Path in Paths.

Though for different purpose, we have added +k8s: prefix to listType and have been using it in DV. Switch, rename or reuse the same tagging should be relatively easy when we're ready in the migration process.

yongruilin · 2025-07-01T18:14:02Z

pkg/apis/networking/validation/validation.go

@@ -283,6 +285,9 @@ type IngressValidationOptions struct {

 	// AllowInvalidWildcardHostRule indicates whether invalid rule values are allowed in rules with wildcard hostnames
 	AllowInvalidWildcardHostRule bool
+
+	// AllowedRelaxedServiceNameValidation indicates if the backend service name can be validated with apimachineryvalidation.NameIsDNSLabel
+	AllowedRelaxedServiceNameValidation bool


AllowedRelax or AllowRelax... to align with previous naming?

yongruilin · 2025-07-01T18:26:40Z

pkg/apis/networking/validation/validation.go

+		if rule.HTTP == nil {
+			continue
+		}
+		for _, path := range rule.HTTP.Paths {


Though for different purpose, we have added +k8s: prefix to listType and have been using it in DV. Switch, rename or reuse the same tagging should be relatively easy when we're ready in the migration process.

k8s-ci-robot assigned aojea Jun 16, 2025

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 16, 2025

k8s-ci-robot assigned danwinship Jun 16, 2025

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 16, 2025

k8s-ci-robot assigned thockin Jun 16, 2025

k8s-ci-robot requested review from cici37 and danwinship June 16, 2025 20:23

k8s-ci-robot added area/test sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jun 16, 2025

github-project-automation bot added this to SIG Apps Jun 16, 2025

github-project-automation bot moved this to Needs Triage in SIG Apps Jun 16, 2025

adrianmoisey force-pushed the relaxed-validation-for-services-names branch from 9cf6d23 to c8b9edb Compare June 16, 2025 20:41

danwinship reviewed Jun 18, 2025

View reviewed changes

adrianmoisey force-pushed the relaxed-validation-for-services-names branch 2 times, most recently from fcd0d82 to 6e0df57 Compare June 19, 2025 15:12

k8s-ci-robot changed the title ~~KEP-3511 Relaxed validation for Services names~~ KEP-5311 Relaxed validation for Services names Jun 19, 2025

adrianmoisey force-pushed the relaxed-validation-for-services-names branch from 6e0df57 to e94834d Compare June 19, 2025 17:30

aojea reviewed Jun 25, 2025

View reviewed changes

Add RelaxedServiceNameValidation feature gate

1134220

Allow Service names to be validated with apimachineryvalidation.NameI…

ed2576a

…sDNSLabel Only validate when feature gate RelaxedServiceNameValidation is enabled. Also remove name validation on Service updates, as the name is immutable.

adrianmoisey force-pushed the relaxed-validation-for-services-names branch from e94834d to 3f0292d Compare June 28, 2025 12:07

adrianmoisey added 5 commits June 28, 2025 14:17

Allow Ingress service refs to be validated with apimachineryvalidatio…

0880b11

…n.NameIsDNSLabel Only validate when feature gate RelaxedServiceNameValidation is enabled or when the Ingess resource contains a service ref that already validates with apimachineryvalidation.NameIsDNSLabel

Add unit test for Service relaxed validation

795589c

Test the behaviour of feature gate RelaxedServiceNameValidation.

Add unit test for Ingress service ref relaxed validation

f24be25

Test the behaviour of feature gate RelaxedServiceNameValidation.

Add integration test for RelaxedServiceNameValidation.

9308498

To test enabling and disabling of RelaxedServiceNameValidation feature gate

Add DNS e2e test of NameIsDNSLabel validated Service names

4122c7b

adrianmoisey force-pushed the relaxed-validation-for-services-names branch 3 times, most recently from ba9d2c8 to a13900f Compare June 28, 2025 12:43

adrianmoisey force-pushed the relaxed-validation-for-services-names branch from a13900f to cf8e372 Compare June 28, 2025 12:44

thockin reviewed Jul 1, 2025

View reviewed changes

yongruilin reviewed Jul 1, 2025

View reviewed changes

		@@ -5926,7 +5931,7 @@ var supportedServiceIPFamilyPolicy = sets.New(
		// ValidateService tests if required fields/annotations of a Service are valid.
		func ValidateService(service, oldService *core.Service) field.ErrorList {

KEP-5311 Relaxed validation for Services names #132339

Are you sure you want to change the base?

KEP-5311 Relaxed validation for Services names #132339

Conversation

adrianmoisey commented Jun 16, 2025

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Jun 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danwinship commented Jun 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrianmoisey Jun 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrianmoisey Jun 28, 2025 •

edited

Loading