Validate ReplicationController.metadata.name

This relies on `+k8s:subfield` and validation cohorts.  The
`k8s:optional` ensures that we don't run the name validation if name is
empty, because core apimachinery will already flag it as Required().

This demonstrates some of the DV value - docs and clients are now (in
theory) able to see what RC's name format is.

Co-Authored-by: Yongrui Lin <yongrlin@outlook.com>

pkg/apis/core/v1/zz_generated.validations.go

@@ -30,6 +30,7 @@ import (
 	operation "k8s.io/apimachinery/pkg/api/operation"
 	safe "k8s.io/apimachinery/pkg/api/safe"
 	validate "k8s.io/apimachinery/pkg/api/validate"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	runtime "k8s.io/apimachinery/pkg/runtime"
 	field "k8s.io/apimachinery/pkg/util/validation/field"
 )
@@ -62,7 +63,23 @@ func RegisterValidations(scheme *runtime.Scheme) error {
 // to declarative validation rules in the API schema.
 func Validate_ReplicationController(ctx context.Context, op operation.Operation, fldPath *field.Path, obj, oldObj *corev1.ReplicationController) (errs field.ErrorList) {
 	// field corev1.ReplicationController.TypeMeta has no validation
-	// field corev1.ReplicationController.ObjectMeta has no validation
+
+	// field corev1.ReplicationController.ObjectMeta
+	errs = append(errs,
+		func(fldPath *field.Path, obj, oldObj *metav1.ObjectMeta) (errs field.ErrorList) {
+			// don't revalidate unchanged data
+			if op.Type == operation.Update && equality.Semantic.DeepEqual(obj, oldObj) {
+				return nil
+			}
+			// call field-attached validations
+			func() { // cohort name
+				if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "name", func(o *metav1.ObjectMeta) *string { return &o.Name }, validate.OptionalValue); len(e) != 0 {
+					return // do not proceed
+				}
+				errs = append(errs, validate.Subfield(ctx, op, fldPath, obj, oldObj, "name", func(o *metav1.ObjectMeta) *string { return &o.Name }, validate.LongName)...)
+			}()
+			return
+		}(fldPath.Child("metadata"), &obj.ObjectMeta, safe.Field(oldObj, func(oldObj *corev1.ReplicationController) *metav1.ObjectMeta { return &oldObj.ObjectMeta }))...)
 
 	// field corev1.ReplicationController.Spec
 	errs = append(errs,

pkg/apis/core/validation/validation.go

@@ -17,6 +17,7 @@ limitations under the License.
 package validation
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"math"
@@ -35,7 +36,9 @@ import (
 
 	v1 "k8s.io/api/core/v1"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
+	"k8s.io/apimachinery/pkg/api/operation"
 	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/apimachinery/pkg/api/validate"
 	apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	unversionedvalidation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
@@ -253,6 +256,10 @@ func ValidateEndpointsSpecificAnnotations(annotations map[string]string, fldPath
 // value that were not valid.  Otherwise this returns an empty list or nil.
 type ValidateNameFunc apimachineryvalidation.ValidateNameFunc
 
+// ValidateNameFuncWithErrors validates that the provided name is valid for a
+// given resource type.
+type ValidateNameFuncWithErrors = apimachineryvalidation.ValidateNameFuncWithErrors
+
 // ValidatePodName can be used to check whether the given pod name is valid.
 // Prefix indicates this name will be used as part of generation, in which case
 // trailing dashes are allowed.
@@ -384,9 +391,19 @@ func ValidateImmutableAnnotation(newVal string, oldVal string, annotation string
 	return allErrs
 }
 
+// ValidateObjectMetaWithOpts validates an object's metadata on creation. It expects that name generation has already
+// been performed.
+func ValidateObjectMetaWithOpts(meta *metav1.ObjectMeta, isNamespaced bool, nameFn ValidateNameFuncWithErrors, fldPath *field.Path) field.ErrorList {
+	allErrs := apimachineryvalidation.ValidateObjectMetaWithOpts(meta, isNamespaced, nameFn, fldPath)
+	// run additional checks for the finalizer name
+	for i := range meta.Finalizers {
+		allErrs = append(allErrs, validateKubeFinalizerName(string(meta.Finalizers[i]), fldPath.Child("finalizers").Index(i))...)
+	}
+	return allErrs
+}
+
 // ValidateObjectMeta validates an object's metadata on creation. It expects that name generation has already
 // been performed.
-// It doesn't return an error for rootscoped resources with namespace, because namespace should already be cleared before.
 // TODO: Remove calls to this method scattered in validations of specific resources, e.g., ValidatePodUpdate.
 func ValidateObjectMeta(meta *metav1.ObjectMeta, requiresNamespace bool, nameFn ValidateNameFunc, fldPath *field.Path) field.ErrorList {
 	allErrs := apimachineryvalidation.ValidateObjectMeta(meta, requiresNamespace, apimachineryvalidation.ValidateNameFunc(nameFn), fldPath)
@@ -6727,7 +6744,10 @@ func ValidateServiceStatusUpdate(service, oldService *core.Service) field.ErrorL
 
 // ValidateReplicationController tests if required fields in the replication controller are set.
 func ValidateReplicationController(controller *core.ReplicationController, opts PodValidationOptions) field.ErrorList {
-	allErrs := ValidateObjectMeta(&controller.ObjectMeta, true, ValidateReplicationControllerName, field.NewPath("metadata"))
+	validateLongName := func(fldPath *field.Path, name string) field.ErrorList {
+		return validate.LongName(context.Background(), operation.Operation{}, fldPath, &name, nil).MarkCoveredByDeclarative()
+	}
+	allErrs := ValidateObjectMetaWithOpts(&controller.ObjectMeta, true, validateLongName, field.NewPath("metadata"))
 	allErrs = append(allErrs, ValidateReplicationControllerSpec(&controller.Spec, nil, field.NewPath("spec"), opts)...)
 	return allErrs
 }

pkg/registry/core/replicationcontroller/declarative_validation_test.go

@@ -17,6 +17,8 @@ limitations under the License.
 package replicationcontroller
 
 import (
+	"fmt"
+	"strings"
 	"testing"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -41,8 +43,117 @@ func TestDeclarativeValidateForDeclarative(t *testing.T) {
 		"empty resource": {
 			input: mkValidReplicationController(),
 		},
+		// metadata.name
+		"name: empty": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.Name = ""
+			}),
+			expectedErrs: field.ErrorList{
+				field.Required(field.NewPath("metadata.name"), ""),
+			},
+		},
+		"name: label format": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.Name = "this-is-a-label"
+			}),
+		},
+		"name: subdomain format": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.Name = "this.is.a.subdomain"
+			}),
+		},
+		"name: invalid label format": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.Name = "-this-is-not-a-label"
+			}),
+			expectedErrs: field.ErrorList{
+				field.Invalid(field.NewPath("metadata.name"), nil, "").WithOrigin("format=k8s-long-name"),
+			},
+		},
+		"name: invalid subdomain format": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.Name = ".this.is.not.a.subdomain"
+			}),
+			expectedErrs: field.ErrorList{
+				field.Invalid(field.NewPath("metadata.name"), nil, "").WithOrigin("format=k8s-long-name"),
+			},
+		},
+		"name: label format with trailing dash": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.Name = "this-is-a-label-"
+			}),
+			expectedErrs: field.ErrorList{
+				field.Invalid(field.NewPath("metadata.name"), nil, "").WithOrigin("format=k8s-long-name"),
+			},
+		},
+		"name: subdomain format with trailing dash": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.Name = "this.is.a.subdomain-"
+			}),
+			expectedErrs: field.ErrorList{
+				field.Invalid(field.NewPath("metadata.name"), nil, "").WithOrigin("format=k8s-long-name"),
+			},
+		},
+		"name: long label format": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.Name = strings.Repeat("x", 253)
+			}),
+		},
+		"name: long subdomain format": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.Name = strings.Repeat("x.", 126) + "x"
+			}),
+		},
+		"name: too long label format": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.Name = strings.Repeat("x", 254)
+			}),
+			expectedErrs: field.ErrorList{
+				field.Invalid(field.NewPath("metadata.name"), nil, "").WithOrigin("format=k8s-long-name"),
+			},
+		},
+		"name: too long subdomain format": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.Name = strings.Repeat("x.", 126) + "xx"
+			}),
+			expectedErrs: field.ErrorList{
+				field.Invalid(field.NewPath("metadata.name"), nil, "").WithOrigin("format=k8s-long-name"),
+			},
+		},
+		// metadata.generateName (note: it's is not really validated)
+		"generateName: valid with empty name": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.Name = ""
+				rc.GenerateName = "name"
+			}),
+			expectedErrs: field.ErrorList{
+				field.Required(field.NewPath("metadata.name"), ""),
+				field.InternalError(field.NewPath("metadata.name"), fmt.Errorf("")),
+			},
+		},
+
+		"generateName: label format": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.GenerateName = "this-is-a-label"
+			}),
+		},
+		"generateName: subdomain format": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.GenerateName = "this.is.a.subdomain"
+			}),
+		},
+		"generateName: invalid format": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.GenerateName = "Obviously not a valid generateName!!"
+			}),
+		},
+		"generateName: too long": {
+			input: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.GenerateName = strings.Repeat("x", 4096)
+			}),
+		},
 		// spec.replicas
-		"nil replicas": {
+		"replicas: nil": {
 			input: mkValidReplicationController(func(rc *api.ReplicationController) {
 				rc.Spec.Replicas = nil
 			}),
@@ -50,32 +161,26 @@ func TestDeclarativeValidateForDeclarative(t *testing.T) {
 				field.Required(field.NewPath("spec.replicas"), ""),
 			},
 		},
-		"0 replicas": {
+		"replicas: 0": {
 			input: mkValidReplicationController(setSpecReplicas(0)),
 		},
-		"1 replicas": {
-			input: mkValidReplicationController(setSpecReplicas(1)),
-		},
-		"positive replicas": {
+		"replicas: positive": {
 			input: mkValidReplicationController(setSpecReplicas(100)),
 		},
-		"negative replicas": {
+		"replicas: negative": {
 			input: mkValidReplicationController(setSpecReplicas(-1)),
 			expectedErrs: field.ErrorList{
 				field.Invalid(field.NewPath("spec.replicas"), nil, "").WithOrigin("minimum"),
 			},
 		},
 		// spec.minReadySeconds
-		"0 minReadySeconds": {
+		"minReadySeconds: 0": {
 			input: mkValidReplicationController(setSpecMinReadySeconds(0)),
 		},
-		"1 minReadySeconds": {
-			input: mkValidReplicationController(setSpecMinReadySeconds(1)),
-		},
-		"positive minReadySeconds": {
+		"minReadySeconds: positive": {
 			input: mkValidReplicationController(setSpecMinReadySeconds(100)),
 		},
-		"negative minReadySeconds": {
+		"minReadySeconds: negative": {
 			input: mkValidReplicationController(setSpecMinReadySeconds(-1)),
 			expectedErrs: field.ErrorList{
 				field.Invalid(field.NewPath("spec.minReadySeconds"), nil, "").WithOrigin("minimum"),
@@ -100,12 +205,31 @@ func TestValidateUpdateForDeclarative(t *testing.T) {
 		expectedErrs field.ErrorList
 	}{
 		// baseline
-		"baseline": {
+		"no change": {
 			old:    mkValidReplicationController(),
 			update: mkValidReplicationController(),
 		},
+		// metadata.name
+		"name: changed": {
+			old: mkValidReplicationController(),
+			update: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.Name += "x"
+			}),
+			expectedErrs: field.ErrorList{
+				field.Invalid(field.NewPath("metadata.name"), nil, ""),
+			},
+		},
+		// metadata.generateName
+		"generateName: changed": {
+			old: mkValidReplicationController(),
+			update: mkValidReplicationController(func(rc *api.ReplicationController) {
+				rc.GenerateName += "x"
+			}),
+			// This is, oddly, mutable.  We should probably ratchet this off
+			// and make it immutable.
+		},
 		// spec.replicas
-		"nil replicas": {
+		"replicas: nil": {
 			old: mkValidReplicationController(),
 			update: mkValidReplicationController(func(rc *api.ReplicationController) {
 				rc.Spec.Replicas = nil
@@ -114,19 +238,15 @@ func TestValidateUpdateForDeclarative(t *testing.T) {
 				field.Required(field.NewPath("spec.replicas"), ""),
 			},
 		},
-		"0 replicas": {
+		"replicas: 0": {
 			old:    mkValidReplicationController(),
 			update: mkValidReplicationController(setSpecReplicas(0)),
 		},
-		"1 replicas": {
-			old:    mkValidReplicationController(),
-			update: mkValidReplicationController(setSpecReplicas(1)),
-		},
-		"positive replicas": {
+		"replicas: positive": {
 			old:    mkValidReplicationController(),
 			update: mkValidReplicationController(setSpecReplicas(100)),
 		},
-		"negative replicas": {
+		"replicas: negative": {
 			old:    mkValidReplicationController(),
 			update: mkValidReplicationController(setSpecReplicas(-1)),
 			expectedErrs: field.ErrorList{
@@ -134,19 +254,15 @@ func TestValidateUpdateForDeclarative(t *testing.T) {
 			},
 		},
 		// spec.minReadySeconds
-		"0 minReadySeconds": {
+		"minReadySeconds: 0": {
 			old:    mkValidReplicationController(),
 			update: mkValidReplicationController(setSpecMinReadySeconds(0)),
 		},
-		"1 minReadySeconds": {
-			old:    mkValidReplicationController(),
-			update: mkValidReplicationController(setSpecMinReadySeconds(1)),
-		},
-		"positive minReadySeconds": {
+		"minReadySeconds: positive": {
 			old:    mkValidReplicationController(),
 			update: mkValidReplicationController(setSpecMinReadySeconds(3)),
 		},
-		"negative minReadySeconds": {
+		"minReadySeconds: negative": {
 			old:    mkValidReplicationController(),
 			update: mkValidReplicationController(setSpecMinReadySeconds(-1)),
 			expectedErrs: field.ErrorList{

staging/src/k8s.io/api/core/v1/types.go

@@ -5566,6 +5566,8 @@ type ReplicationController struct {
 	// be the same as the Pod(s) that the replication controller manages.
 	// Standard object's metadata. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata
 	// +optional
+	// +k8s:subfield(name)=+k8s:optional
+	// +k8s:subfield(name)=+k8s:format=k8s-long-name
 	metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`
 
 	// Spec defines the specification of the desired behavior of the replication controller.