From f001f9e1dbce644a1b7d22b370ab37fc7d770c7e Mon Sep 17 00:00:00 2001 From: Chao Xu Date: Mon, 4 Feb 2019 11:15:16 -0800 Subject: [PATCH] Set the maximum size increase the copy operations in a json patch can cause --- .../app/options/options_test.go | 1 + staging/src/k8s.io/apiserver/pkg/server/BUILD | 1 + .../src/k8s.io/apiserver/pkg/server/config.go | 53 ++++++++++++++----- .../pkg/server/options/server_run_options.go | 13 ++++- .../server/options/server_run_options_test.go | 20 +++++++ 5 files changed, 72 insertions(+), 16 deletions(-) diff --git a/cmd/kube-apiserver/app/options/options_test.go b/cmd/kube-apiserver/app/options/options_test.go index e4173130a3b..3f7984e85aa 100644 --- a/cmd/kube-apiserver/app/options/options_test.go +++ b/cmd/kube-apiserver/app/options/options_test.go @@ -129,6 +129,7 @@ func TestAddFlags(t *testing.T) { MaxMutatingRequestsInFlight: 200, RequestTimeout: time.Duration(2) * time.Minute, MinRequestTimeout: 1800, + JSONPatchMaxCopyBytes: int64(10 * 1024 * 1024), }, Admission: &kubeoptions.AdmissionOptions{ GenericAdmission: &apiserveroptions.AdmissionOptions{ diff --git a/staging/src/k8s.io/apiserver/pkg/server/BUILD b/staging/src/k8s.io/apiserver/pkg/server/BUILD index 18f319fd31a..058f8d37a25 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/server/BUILD @@ -108,6 +108,7 @@ go_library( "//staging/src/k8s.io/component-base/logs:go_default_library", "//vendor/github.com/coreos/go-systemd/daemon:go_default_library", "//vendor/github.com/emicklei/go-restful:go_default_library", + "//vendor/github.com/evanphx/json-patch:go_default_library", "//vendor/github.com/go-openapi/spec:go_default_library", "//vendor/github.com/pborman/uuid:go_default_library", "//vendor/golang.org/x/net/http2:go_default_library", diff --git a/staging/src/k8s.io/apiserver/pkg/server/config.go b/staging/src/k8s.io/apiserver/pkg/server/config.go index da5b28a30e2..0ae03047023 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/config.go +++ b/staging/src/k8s.io/apiserver/pkg/server/config.go @@ -26,8 +26,10 @@ import ( "sort" "strconv" "strings" + "sync/atomic" "time" + jsonpatch "github.com/evanphx/json-patch" "github.com/go-openapi/spec" "github.com/pborman/uuid" "k8s.io/klog" @@ -153,6 +155,10 @@ type Config struct { // If specified, long running requests such as watch will be allocated a random timeout between this value, and // twice this value. Note that it is up to the request handlers to ignore or honor this timeout. In seconds. MinRequestTimeout int + // The limit on the total size increase all "copy" operations in a json + // patch may cause. + // This affects all places that applies json patch in the binary. + JSONPatchMaxCopyBytes int64 // MaxRequestsInFlight is the maximum number of parallel non-long-running requests. Every further // request has to wait. Applies only to non-mutating requests. MaxRequestsInFlight int @@ -243,20 +249,26 @@ type AuthorizationInfo struct { // NewConfig returns a Config struct with the default values func NewConfig(codecs serializer.CodecFactory) *Config { return &Config{ - Serializer: codecs, - BuildHandlerChainFunc: DefaultBuildHandlerChain, - HandlerChainWaitGroup: new(utilwaitgroup.SafeWaitGroup), - LegacyAPIGroupPrefixes: sets.NewString(DefaultLegacyAPIPrefix), - DisabledPostStartHooks: sets.NewString(), - HealthzChecks: []healthz.HealthzChecker{healthz.PingHealthz, healthz.LogHealthz}, - EnableIndex: true, - EnableDiscovery: true, - EnableProfiling: true, - EnableMetrics: true, - MaxRequestsInFlight: 400, - MaxMutatingRequestsInFlight: 200, - RequestTimeout: time.Duration(60) * time.Second, - MinRequestTimeout: 1800, + Serializer: codecs, + BuildHandlerChainFunc: DefaultBuildHandlerChain, + HandlerChainWaitGroup: new(utilwaitgroup.SafeWaitGroup), + LegacyAPIGroupPrefixes: sets.NewString(DefaultLegacyAPIPrefix), + DisabledPostStartHooks: sets.NewString(), + HealthzChecks: []healthz.HealthzChecker{healthz.PingHealthz, healthz.LogHealthz}, + EnableIndex: true, + EnableDiscovery: true, + EnableProfiling: true, + EnableMetrics: true, + MaxRequestsInFlight: 400, + MaxMutatingRequestsInFlight: 200, + RequestTimeout: time.Duration(60) * time.Second, + MinRequestTimeout: 1800, + // 10MB is the recommended maximum client request size in bytes + // the etcd server should accept. Thus, we set it as the limit + // on the size increase the "copy" operations in a json patch + // can cause. See + // https://github.com/etcd-io/etcd/blob/release-3.3/etcdserver/server.go#L90. + JSONPatchMaxCopyBytes: int64(10 * 1024 * 1024), EnableAPIResponseCompression: utilfeature.DefaultFeatureGate.Enabled(features.APIResponseCompression), // Default to treating watch as a long-running operation @@ -451,6 +463,19 @@ func (c completedConfig) New(name string, delegationTarget DelegationTarget) (*G enableAPIResponseCompression: c.EnableAPIResponseCompression, } + for { + if c.JSONPatchMaxCopyBytes <= 0 { + break + } + existing := atomic.LoadInt64(&jsonpatch.AccumulatedCopySizeLimit) + if existing > 0 && existing < c.JSONPatchMaxCopyBytes { + break + } + if atomic.CompareAndSwapInt64(&jsonpatch.AccumulatedCopySizeLimit, existing, c.JSONPatchMaxCopyBytes) { + break + } + } + for k, v := range delegationTarget.PostStartHooks() { s.postStartHooks[k] = v } diff --git a/staging/src/k8s.io/apiserver/pkg/server/options/server_run_options.go b/staging/src/k8s.io/apiserver/pkg/server/options/server_run_options.go index 2884f853ff6..78a4ddcf9c9 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/options/server_run_options.go +++ b/staging/src/k8s.io/apiserver/pkg/server/options/server_run_options.go @@ -42,7 +42,10 @@ type ServerRunOptions struct { MaxMutatingRequestsInFlight int RequestTimeout time.Duration MinRequestTimeout int - TargetRAMMB int + // We intentionally did not add a flag for this option. Users of the + // apiserver library can wire it to a flag. + JSONPatchMaxCopyBytes int64 + TargetRAMMB int } func NewServerRunOptions() *ServerRunOptions { @@ -52,6 +55,7 @@ func NewServerRunOptions() *ServerRunOptions { MaxMutatingRequestsInFlight: defaults.MaxMutatingRequestsInFlight, RequestTimeout: defaults.RequestTimeout, MinRequestTimeout: defaults.MinRequestTimeout, + JSONPatchMaxCopyBytes: defaults.JSONPatchMaxCopyBytes, } } @@ -63,6 +67,7 @@ func (s *ServerRunOptions) ApplyTo(c *server.Config) error { c.MaxMutatingRequestsInFlight = s.MaxMutatingRequestsInFlight c.RequestTimeout = s.RequestTimeout c.MinRequestTimeout = s.MinRequestTimeout + c.JSONPatchMaxCopyBytes = s.JSONPatchMaxCopyBytes c.PublicAddress = s.AdvertiseAddress return nil @@ -107,10 +112,14 @@ func (s *ServerRunOptions) Validate() []error { errors = append(errors, fmt.Errorf("--min-request-timeout can not be negative value")) } + if s.JSONPatchMaxCopyBytes < 0 { + errors = append(errors, fmt.Errorf("--json-patch-max-copy-bytes can not be negative value")) + } + return errors } -// AddFlags adds flags for a specific APIServer to the specified FlagSet +// AddUniversalFlags adds flags for a specific APIServer to the specified FlagSet func (s *ServerRunOptions) AddUniversalFlags(fs *pflag.FlagSet) { // Note: the weird ""+ in below lines seems to be the only way to get gofmt to // arrange these text blocks sensibly. Grrr. diff --git a/staging/src/k8s.io/apiserver/pkg/server/options/server_run_options_test.go b/staging/src/k8s.io/apiserver/pkg/server/options/server_run_options_test.go index 0fd69b4eb01..bd00eb84c4d 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/options/server_run_options_test.go +++ b/staging/src/k8s.io/apiserver/pkg/server/options/server_run_options_test.go @@ -40,6 +40,7 @@ func TestServerRunOptionsValidate(t *testing.T) { MaxMutatingRequestsInFlight: 200, RequestTimeout: time.Duration(2) * time.Minute, MinRequestTimeout: 1800, + JSONPatchMaxCopyBytes: 10 * 1024 * 1024, TargetRAMMB: -65536, }, expectErr: "--target-ram-mb can not be negative value", @@ -53,6 +54,7 @@ func TestServerRunOptionsValidate(t *testing.T) { MaxMutatingRequestsInFlight: 200, RequestTimeout: time.Duration(2) * time.Minute, MinRequestTimeout: 1800, + JSONPatchMaxCopyBytes: 10 * 1024 * 1024, TargetRAMMB: 65536, }, expectErr: "--max-requests-inflight can not be negative value", @@ -66,6 +68,7 @@ func TestServerRunOptionsValidate(t *testing.T) { MaxMutatingRequestsInFlight: -200, RequestTimeout: time.Duration(2) * time.Minute, MinRequestTimeout: 1800, + JSONPatchMaxCopyBytes: 10 * 1024 * 1024, TargetRAMMB: 65536, }, expectErr: "--max-mutating-requests-inflight can not be negative value", @@ -79,6 +82,7 @@ func TestServerRunOptionsValidate(t *testing.T) { MaxMutatingRequestsInFlight: 200, RequestTimeout: -time.Duration(2) * time.Minute, MinRequestTimeout: 1800, + JSONPatchMaxCopyBytes: 10 * 1024 * 1024, TargetRAMMB: 65536, }, expectErr: "--request-timeout can not be negative value", @@ -92,10 +96,25 @@ func TestServerRunOptionsValidate(t *testing.T) { MaxMutatingRequestsInFlight: 200, RequestTimeout: time.Duration(2) * time.Minute, MinRequestTimeout: -1800, + JSONPatchMaxCopyBytes: 10 * 1024 * 1024, TargetRAMMB: 65536, }, expectErr: "--min-request-timeout can not be negative value", }, + { + name: "Test when JSONPatchMaxCopyBytes is negative value", + testOptions: &ServerRunOptions{ + AdvertiseAddress: net.ParseIP("192.168.10.10"), + CorsAllowedOriginList: []string{"10.10.10.100", "10.10.10.200"}, + MaxRequestsInFlight: 400, + MaxMutatingRequestsInFlight: 200, + RequestTimeout: time.Duration(2) * time.Minute, + MinRequestTimeout: 1800, + JSONPatchMaxCopyBytes: -10 * 1024 * 1024, + TargetRAMMB: 65536, + }, + expectErr: "--json-patch-max-copy-bytes can not be negative value", + }, { name: "Test when ServerRunOptions is valid", testOptions: &ServerRunOptions{ @@ -105,6 +124,7 @@ func TestServerRunOptionsValidate(t *testing.T) { MaxMutatingRequestsInFlight: 200, RequestTimeout: time.Duration(2) * time.Minute, MinRequestTimeout: 1800, + JSONPatchMaxCopyBytes: 10 * 1024 * 1024, TargetRAMMB: 65536, }, },