Merge pull request #63533 from sttts/sttts-required-with-status-subresource

Automatic merge from submit-queue (batch tested with PRs 59034, 63565, 63533). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

apiextensions: allow "required" at root with status subresource

In the subresources alpha we intentionally disallowed anything than `properties` at the root of the validation schema in order to allow us to project it to the .status subtree. By doing this we also disallowed `required` at the root which is necessary to enforce e.g. a spec to be set. This PR fixes this.

Moreover, it fixes that the restriction is only enforced when the status subresource is actually enabled. Before this PR we were enforcing the restriction as soon as the feature gate was enabled, leading to a backwards incompatible change.

```release-note
Allow "required" to be used at the CRD OpenAPI validation schema when the /status subresource is enabled.
```

There was an issue reporting the bug. But cannot find it.

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/BUILD

@@ -27,8 +27,11 @@ go_test(
     embed = [":go_default_library"],
     deps = [
         "//vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library",
+        "//vendor/k8s.io/apiextensions-apiserver/pkg/features:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/util/feature/testing:go_default_library",
     ],
 )
 

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

@@ -106,7 +106,11 @@ func ValidateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi
 	allErrs = append(allErrs, ValidateCustomResourceDefinitionNames(&spec.Names, fldPath.Child("names"))...)
 
 	if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceValidation) {
-		allErrs = append(allErrs, ValidateCustomResourceDefinitionValidation(spec.Validation, fldPath.Child("validation"))...)
+		statusEnabled := false
+		if spec.Subresources != nil && spec.Subresources.Status != nil {
+			statusEnabled = true
+		}
+		allErrs = append(allErrs, ValidateCustomResourceDefinitionValidation(spec.Validation, statusEnabled, fldPath.Child("validation"))...)
 	} else if spec.Validation != nil {
 		allErrs = append(allErrs, field.Forbidden(fldPath.Child("validation"), "disabled by feature-gate CustomResourceValidation"))
 	}
@@ -187,7 +191,7 @@ type specStandardValidator interface {
 }
 
 // ValidateCustomResourceDefinitionValidation statically validates
-func ValidateCustomResourceDefinitionValidation(customResourceValidation *apiextensions.CustomResourceValidation, fldPath *field.Path) field.ErrorList {
+func ValidateCustomResourceDefinitionValidation(customResourceValidation *apiextensions.CustomResourceValidation, statusSubresourceEnabled bool, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 
 	if customResourceValidation == nil {
@@ -195,21 +199,19 @@ func ValidateCustomResourceDefinitionValidation(customResourceValidation *apiext
 	}
 
 	if schema := customResourceValidation.OpenAPIV3Schema; schema != nil {
-		// if subresources are enabled, only properties is allowed inside the root schema
+		// if subresources are enabled, only "properties" and "required" is allowed inside the root schema
-		if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceSubresources) {
+		if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceSubresources) && statusSubresourceEnabled {
 			v := reflect.ValueOf(schema).Elem()
-			fieldsPresent := 0
-
 			for i := 0; i < v.NumField(); i++ {
-				field := v.Field(i).Interface()
+				// skip zero values
-				if !reflect.DeepEqual(field, reflect.Zero(reflect.TypeOf(field)).Interface()) {
+				if value := v.Field(i).Interface(); reflect.DeepEqual(value, reflect.Zero(reflect.TypeOf(value)).Interface()) {
-					fieldsPresent++
+					continue
 				}
-			}
 
-			if fieldsPresent > 1 || (fieldsPresent == 1 && v.FieldByName("Properties").IsNil()) {
+				if name := v.Type().Field(i).Name; name != "Properties" && name != "Required" {
-				allErrs = append(allErrs, field.Invalid(fldPath.Child("openAPIV3Schema"), *schema, fmt.Sprintf("if subresources for custom resources are enabled, only properties can be used at the root of the schema")))
+					allErrs = append(allErrs, field.Invalid(fldPath.Child("openAPIV3Schema"), *schema, fmt.Sprintf(`must only have "properties" or "required" at the root if the status subresource is enabled`)))
-				return allErrs
+					break
+				}
 			}
 		}
 

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go

@@ -19,9 +19,13 @@ package validation
 import (
 	"testing"
 
-	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	utilfeaturetesting "k8s.io/apiserver/pkg/util/feature/testing"
+
+	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
+	"k8s.io/apiextensions-apiserver/pkg/features"
 )
 
 type validationMatch struct {
@@ -517,3 +521,70 @@ func TestValidateCustomResourceDefinitionUpdate(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateCustomResourceDefinitionValidation(t *testing.T) {
+	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CustomResourceSubresources, true)()
+
+	tests := []struct {
+		name          string
+		input         apiextensions.CustomResourceValidation
+		statusEnabled bool
+		wantError     bool
+	}{
+		{
+			name:      "empty",
+			input:     apiextensions.CustomResourceValidation{},
+			wantError: false,
+		},
+		{
+			name:          "empty with status",
+			input:         apiextensions.CustomResourceValidation{},
+			statusEnabled: true,
+			wantError:     false,
+		},
+		{
+			name: "root type without status",
+			input: apiextensions.CustomResourceValidation{
+				OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
+					Type: "object",
+				},
+			},
+			statusEnabled: false,
+			wantError:     false,
+		},
+		{
+			name: "root type with status",
+			input: apiextensions.CustomResourceValidation{
+				OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
+					Type: "object",
+				},
+			},
+			statusEnabled: true,
+			wantError:     true,
+		},
+		{
+			name: "properties and required with status",
+			input: apiextensions.CustomResourceValidation{
+				OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
+					Properties: map[string]apiextensions.JSONSchemaProps{
+						"spec":   {},
+						"status": {},
+					},
+					Required: []string{"spec", "status"},
+				},
+			},
+			statusEnabled: true,
+			wantError:     false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := ValidateCustomResourceDefinitionValidation(&tt.input, tt.statusEnabled, field.NewPath("spec", "validation"))
+			if !tt.wantError && len(got) > 0 {
+				t.Errorf("Expected no error, but got: %v", got)
+			} else if tt.wantError && len(got) == 0 {
+				t.Error("Expected error, but got none")
+			}
+		})
+	}
+}

staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go

@@ -355,9 +355,12 @@ func TestValidationSchema(t *testing.T) {
 
 	// fields other than properties in root schema are not allowed
 	noxuDefinition := newNoxuValidationCRD(apiextensionsv1beta1.NamespaceScoped)
+	noxuDefinition.Spec.Subresources = &apiextensionsv1beta1.CustomResourceSubresources{
+		Status: &apiextensionsv1beta1.CustomResourceSubresourceStatus{},
+	}
 	err = testserver.CreateNewCustomResourceDefinition(noxuDefinition, apiExtensionClient, dynamicClient)
 	if err == nil {
-		t.Fatalf("unexpected non-error: if subresources for custom resources are enabled, only properties can be used at the root of the schema")
+		t.Fatalf(`unexpected non-error, expected: must only have "properties" or "required" at the root if the status subresource is enabled`)
 	}
 
 	// make sure we are not restricting fields to properties even in subschemas
@@ -372,6 +375,7 @@ func TestValidationSchema(t *testing.T) {
 				},
 			},
 		},
+		Required: []string{"spec"},
 	}
 	err = testserver.CreateNewCustomResourceDefinition(noxuDefinition, apiExtensionClient, dynamicClient)
 	if err != nil {

staging/src/k8s.io/apiextensions-apiserver/test/integration/testserver/resources.go

@@ -246,6 +246,7 @@ func checkForWatchCachePrimed(crd *apiextensionsv1beta1.CustomResourceDefinition
 			"gamma":   "bar",
 			"delta":   "hello",
 			"epsilon": "foobar",
+			"spec":    map[string]interface{}{},
 		},
 	}
 	if _, err := resourceClient.Create(instance); err != nil {