From 2d5ceb9b15347b8bb5af360470b6346370157042 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Tue, 24 Jan 2023 17:48:31 +0000 Subject: [PATCH] drop Enabled() checks for ServerSideFieldValidation feature gate --- .../pkg/controller/openapi/builder/builder.go | 10 ++----- .../apimachinery/pkg/apis/meta/v1/types.go | 27 +++++++------------ .../apiserver/pkg/endpoints/apiserver_test.go | 5 ---- .../apiserver/pkg/endpoints/handlers/rest.go | 9 ++----- .../apiserver/pkg/endpoints/installer.go | 11 +++----- .../pkg/endpoints/metrics/metrics.go | 9 +++---- .../apiserver/field_validation_test.go | 6 ----- 7 files changed, 19 insertions(+), 58 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go index 2323b542466..ecb7fbbf330 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go @@ -39,8 +39,6 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/endpoints" "k8s.io/apiserver/pkg/endpoints/openapi" - "k8s.io/apiserver/pkg/features" - utilfeature "k8s.io/apiserver/pkg/util/feature" utilopenapi "k8s.io/apiserver/pkg/util/openapi" openapibuilder "k8s.io/kube-openapi/pkg/builder" "k8s.io/kube-openapi/pkg/builder3" @@ -347,10 +345,6 @@ func (b *builder) buildRoute(root, path, httpMethod, actionVerb, operationVerb s route.Consumes(runtime.ContentTypeJSON, runtime.ContentTypeYAML) } - var disabledParams []string - if !utilfeature.DefaultFeatureGate.Enabled(features.ServerSideFieldValidation) { - disabledParams = []string{"fieldValidation"} - } // Build option parameters switch actionVerb { case "get": @@ -360,9 +354,9 @@ func (b *builder) buildRoute(root, path, httpMethod, actionVerb, operationVerb s endpoints.AddObjectParams(b.ws, route, &metav1.ListOptions{}) case "put", "patch": // TODO: PatchOption added in feature branch but not in master yet - endpoints.AddObjectParams(b.ws, route, &metav1.UpdateOptions{}, disabledParams...) + endpoints.AddObjectParams(b.ws, route, &metav1.UpdateOptions{}) case "post": - endpoints.AddObjectParams(b.ws, route, &metav1.CreateOptions{}, disabledParams...) + endpoints.AddObjectParams(b.ws, route, &metav1.CreateOptions{}) case "delete": endpoints.AddObjectParams(b.ws, route, &metav1.DeleteOptions{}) route.Reads(&metav1.DeleteOptions{}).ParameterNamed("body").Required(false) diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go index 152f99296ca..2a6fd74ca00 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go @@ -542,19 +542,16 @@ type CreateOptions struct { // fieldValidation instructs the server on how to handle // objects in the request (POST/PUT/PATCH) containing unknown - // or duplicate fields, provided that the `ServerSideFieldValidation` - // feature gate is also enabled. Valid values are: + // or duplicate fields. Valid values are: // - Ignore: This will ignore any unknown fields that are silently // dropped from the object, and will ignore all but the last duplicate // field that the decoder encounters. This is the default behavior - // prior to v1.23 and is the default behavior when the - // `ServerSideFieldValidation` feature gate is disabled. + // prior to v1.23. // - Warn: This will send a warning via the standard warning response // header for each unknown field that is dropped from the object, and // for each duplicate field that is encountered. The request will // still succeed if there are no other errors, and will only persist - // the last of any duplicate fields. This is the default when the - // `ServerSideFieldValidation` feature gate is enabled. + // the last of any duplicate fields. This is the default in v1.23+ // - Strict: This will fail the request with a BadRequest error if // any unknown fields would be dropped from the object, or if any // duplicate fields are present. The error returned from the server @@ -597,19 +594,16 @@ type PatchOptions struct { // fieldValidation instructs the server on how to handle // objects in the request (POST/PUT/PATCH) containing unknown - // or duplicate fields, provided that the `ServerSideFieldValidation` - // feature gate is also enabled. Valid values are: + // or duplicate fields. Valid values are: // - Ignore: This will ignore any unknown fields that are silently // dropped from the object, and will ignore all but the last duplicate // field that the decoder encounters. This is the default behavior - // prior to v1.23 and is the default behavior when the - // `ServerSideFieldValidation` feature gate is disabled. + // prior to v1.23. // - Warn: This will send a warning via the standard warning response // header for each unknown field that is dropped from the object, and // for each duplicate field that is encountered. The request will // still succeed if there are no other errors, and will only persist - // the last of any duplicate fields. This is the default when the - // `ServerSideFieldValidation` feature gate is enabled. + // the last of any duplicate fields. This is the default in v1.23+ // - Strict: This will fail the request with a BadRequest error if // any unknown fields would be dropped from the object, or if any // duplicate fields are present. The error returned from the server @@ -674,19 +668,16 @@ type UpdateOptions struct { // fieldValidation instructs the server on how to handle // objects in the request (POST/PUT/PATCH) containing unknown - // or duplicate fields, provided that the `ServerSideFieldValidation` - // feature gate is also enabled. Valid values are: + // or duplicate fields. Valid values are: // - Ignore: This will ignore any unknown fields that are silently // dropped from the object, and will ignore all but the last duplicate // field that the decoder encounters. This is the default behavior - // prior to v1.23 and is the default behavior when the - // `ServerSideFieldValidation` feature gate is disabled. + // prior to v1.23. // - Warn: This will send a warning via the standard warning response // header for each unknown field that is dropped from the object, and // for each duplicate field that is encountered. The request will // still succeed if there are no other errors, and will only persist - // the last of any duplicate fields. This is the default when the - // `ServerSideFieldValidation` feature gate is enabled. + // the last of any duplicate fields. This is the default in v1.23+ // - Strict: This will fail the request with a BadRequest error if // any unknown fields would be dropped from the object, or if any // duplicate fields are present. The error returned from the server diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go index b803377631d..c51e8906008 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go @@ -72,10 +72,7 @@ import ( "k8s.io/apiserver/pkg/endpoints/handlers/responsewriters" "k8s.io/apiserver/pkg/endpoints/request" genericapitesting "k8s.io/apiserver/pkg/endpoints/testing" - "k8s.io/apiserver/pkg/features" "k8s.io/apiserver/pkg/registry/rest" - utilfeature "k8s.io/apiserver/pkg/util/feature" - featuregatetesting "k8s.io/component-base/featuregate/testing" ) type alwaysMutatingDeny struct{} @@ -4041,7 +4038,6 @@ var ( // TestFieldValidation tests the create, update, and patch handlers for correctness when faced with field validation errors. func TestFieldValidation(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ServerSideFieldValidation, true)() var ( strictDecodingErr = `strict decoding error: duplicate field \"other\", unknown field \"unknown\"` strictDecodingWarns = []string{`duplicate field "other"`, `unknown field "unknown"`} @@ -4180,7 +4176,6 @@ unknown: baz`) // BenchmarkFieldValidation benchmarks the create, update, and patch handlers for performance distinctions between // strict, warn, and ignore field validation handling. func BenchmarkFieldValidation(b *testing.B) { - defer featuregatetesting.SetFeatureGateDuringTest(b, utilfeature.DefaultFeatureGate, features.ServerSideFieldValidation, true)() var ( validJSONDataPost = []byte(`{"kind":"Simple", "apiVersion":"test.group/version", "metadata":{"creationTimestamp":null}, "other":"foo"}`) validYAMLDataPost = []byte(`apiVersion: test.group/version diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go index f582c668ff7..1a9f7791901 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go @@ -45,9 +45,7 @@ import ( "k8s.io/apiserver/pkg/endpoints/handlers/responsewriters" "k8s.io/apiserver/pkg/endpoints/metrics" "k8s.io/apiserver/pkg/endpoints/request" - "k8s.io/apiserver/pkg/features" "k8s.io/apiserver/pkg/registry/rest" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/apiserver/pkg/warning" ) @@ -406,13 +404,10 @@ func isDryRun(url *url.URL) bool { // fieldValidation checks that the field validation feature is enabled // and returns a valid directive of either -// - Ignore (default when feature is disabled) -// - Warn (default when feature is enabled) +// - Ignore +// - Warn (default) // - Strict func fieldValidation(directive string) string { - if !utilfeature.DefaultFeatureGate.Enabled(features.ServerSideFieldValidation) { - return metav1.FieldValidationIgnore - } if directive == "" { return metav1.FieldValidationWarn } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go b/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go index 2c4d7b95793..0b8f6ef509b 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go @@ -622,11 +622,6 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag } } - var disabledParams []string - if !utilfeature.DefaultFeatureGate.Enabled(features.ServerSideFieldValidation) { - disabledParams = []string{"fieldValidation"} - } - // Create Routes for the actions. // TODO: Add status documentation using Returns() // Errors (see api/errors/errors.go as well as go-restful router): @@ -857,7 +852,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag Returns(http.StatusCreated, "Created", producedObject). Reads(defaultVersionedObject). Writes(producedObject) - if err := AddObjectParams(ws, route, versionedUpdateOptions, disabledParams...); err != nil { + if err := AddObjectParams(ws, route, versionedUpdateOptions); err != nil { return nil, nil, err } addParams(route, action.Params) @@ -886,7 +881,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag Returns(http.StatusCreated, "Created", producedObject). Reads(metav1.Patch{}). Writes(producedObject) - if err := AddObjectParams(ws, route, versionedPatchOptions, disabledParams...); err != nil { + if err := AddObjectParams(ws, route, versionedPatchOptions); err != nil { return nil, nil, err } addParams(route, action.Params) @@ -917,7 +912,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag Returns(http.StatusAccepted, "Accepted", producedObject). Reads(defaultVersionedObject). Writes(producedObject) - if err := AddObjectParams(ws, route, versionedCreateOptions, disabledParams...); err != nil { + if err := AddObjectParams(ws, route, versionedCreateOptions); err != nil { return nil, nil, err } addParams(route, action.Params) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go index 4c2abfaedce..61f53c18eab 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go @@ -33,8 +33,6 @@ import ( "k8s.io/apiserver/pkg/authentication/user" "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/endpoints/responsewriter" - "k8s.io/apiserver/pkg/features" - utilfeature "k8s.io/apiserver/pkg/util/feature" compbasemetrics "k8s.io/component-base/metrics" "k8s.io/component-base/metrics/legacyregistry" ) @@ -136,7 +134,7 @@ var ( fieldValidationRequestLatencies = compbasemetrics.NewHistogramVec( &compbasemetrics.HistogramOpts{ Name: "field_validation_request_duration_seconds", - Help: "Response latency distribution in seconds for each field validation value and whether field validation is enabled or not", + Help: "Response latency distribution in seconds for each field validation value", // This metric is supplementary to the requestLatencies metric. // It measures request durations for the various field validation // values. @@ -144,7 +142,7 @@ var ( 4, 5, 6, 8, 10, 15, 20, 30, 45, 60}, StabilityLevel: compbasemetrics.ALPHA, }, - []string{"field_validation", "enabled"}, + []string{"field_validation"}, ) responseSizes = compbasemetrics.NewHistogramVec( &compbasemetrics.HistogramOpts{ @@ -543,8 +541,7 @@ func MonitorRequest(req *http.Request, verb, group, version, resource, subresour } requestLatencies.WithContext(req.Context()).WithLabelValues(reportedVerb, dryRun, group, version, resource, subresource, scope, component).Observe(elapsedSeconds) fieldValidation := cleanFieldValidation(req.URL) - fieldValidationEnabled := strconv.FormatBool(utilfeature.DefaultFeatureGate.Enabled(features.ServerSideFieldValidation)) - fieldValidationRequestLatencies.WithContext(req.Context()).WithLabelValues(fieldValidation, fieldValidationEnabled) + fieldValidationRequestLatencies.WithContext(req.Context()).WithLabelValues(fieldValidation) if wd, ok := request.LatencyTrackersFrom(req.Context()); ok { sliLatency := elapsedSeconds - (wd.MutatingWebhookTracker.GetLatency() + wd.ValidatingWebhookTracker.GetLatency()).Seconds() diff --git a/test/integration/apiserver/field_validation_test.go b/test/integration/apiserver/field_validation_test.go index bdf2bd352ec..c7c32a14519 100644 --- a/test/integration/apiserver/field_validation_test.go +++ b/test/integration/apiserver/field_validation_test.go @@ -32,12 +32,9 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" - "k8s.io/apiserver/pkg/features" - utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/dynamic" clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" - featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/klog/v2" kubeapiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing" @@ -517,8 +514,6 @@ spec: ) func TestFieldValidation(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ServerSideFieldValidation, true)() - server, err := kubeapiservertesting.StartTestServer(t, kubeapiservertesting.NewDefaultTestServerOptions(), nil, framework.SharedEtcd()) if err != nil { t.Fatal(err) @@ -2934,7 +2929,6 @@ func setupCRD(t testing.TB, config *rest.Config, apiGroup string, schemaless boo } func BenchmarkFieldValidation(b *testing.B) { - defer featuregatetesting.SetFeatureGateDuringTest(b, utilfeature.DefaultFeatureGate, features.ServerSideFieldValidation, true)() flag.Lookup("v").Value.Set("0") server, err := kubeapiservertesting.StartTestServer(b, kubeapiservertesting.NewDefaultTestServerOptions(), nil, framework.SharedEtcd()) if err != nil {