From 0cd0585b644e7dcd087648afa9b44d5ca3c9e2c6 Mon Sep 17 00:00:00 2001 From: Mike Nguyen Date: Fri, 21 Feb 2025 16:11:11 +0000 Subject: [PATCH] fix: arguments accept units (#1490) * fix: arguments accept units `max-body-size` and `read-buffer-size` now accept units as defined in the docs. Fixes #1489 Signed-off-by: Mike Nguyen * chore: gofumpt Signed-off-by: Mike Nguyen * refactor: modify logic to comply with vetting Signed-off-by: Mike Nguyen * chore: gofumpt -w . Signed-off-by: Mike Nguyen * refactor: set defaults `max-body-size` is defaulted to 4Mi `request-buffer-size` is defaulted to 4Ki This is inline with the runtime. Signed-off-by: Mike Nguyen * fix: set defaults in run and annotate Signed-off-by: Mike Nguyen * chore: gofumpt Signed-off-by: Mike Nguyen * refactor: exit with error rather than panic Co-authored-by: Anton Troshin Signed-off-by: Mike Nguyen --------- Signed-off-by: Mike Nguyen Co-authored-by: Anton Troshin --- cmd/annotate.go | 32 +++++++++++++++------ cmd/run.go | 10 ++++--- pkg/runexec/runexec_test.go | 56 +++++++++++++++++++++++++++++++------ pkg/standalone/run.go | 52 +++++++++++++++++++++++++++------- 4 files changed, 120 insertions(+), 30 deletions(-) diff --git a/cmd/annotate.go b/cmd/annotate.go index 6dd573738..c68c17232 100644 --- a/cmd/annotate.go +++ b/cmd/annotate.go @@ -21,6 +21,11 @@ import ( "net/url" "os" "path/filepath" + "strconv" + + "github.com/dapr/dapr/pkg/runtime" + + "k8s.io/apimachinery/pkg/api/resource" "github.com/spf13/cobra" @@ -60,8 +65,8 @@ var ( annotateReadinessProbeThreshold int annotateDaprImage string annotateAppSSL bool - annotateMaxRequestBodySize int - annotateReadBufferSize int + annotateMaxRequestBodySize string + annotateReadBufferSize string annotateHTTPStreamRequestBody bool annotateGracefulShutdownSeconds int annotateEnableAPILogging bool @@ -318,12 +323,23 @@ func getOptionsFromFlags() kubernetes.AnnotateOptions { if annotateAppSSL { o = append(o, kubernetes.WithAppSSL()) } - if annotateMaxRequestBodySize != -1 { - o = append(o, kubernetes.WithMaxRequestBodySize(annotateMaxRequestBodySize)) + if annotateMaxRequestBodySize != "-1" { + if q, err := resource.ParseQuantity(annotateMaxRequestBodySize); err != nil { + print.FailureStatusEvent(os.Stderr, "error parsing value of max-body-size: %s, error: %s", annotateMaxRequestBodySize, err.Error()) + os.Exit(1) + } else { + o = append(o, kubernetes.WithMaxRequestBodySize(int(q.Value()))) + } } - if annotateReadBufferSize != -1 { - o = append(o, kubernetes.WithReadBufferSize(annotateReadBufferSize)) + if annotateReadBufferSize != "-1" { + if q, err := resource.ParseQuantity(annotateReadBufferSize); err != nil { + print.FailureStatusEvent(os.Stderr, "error parsing value of read-buffer-size: %s, error: %s", annotateMaxRequestBodySize, err.Error()) + os.Exit(1) + } else { + o = append(o, kubernetes.WithReadBufferSize(int(q.Value()))) + } } + if annotateHTTPStreamRequestBody { o = append(o, kubernetes.WithHTTPStreamRequestBody()) } @@ -385,8 +401,8 @@ func init() { AnnotateCmd.Flags().StringVar(&annotateDaprImage, "dapr-image", "", "The image to use for the dapr sidecar container") AnnotateCmd.Flags().BoolVar(&annotateAppSSL, "app-ssl", false, "Enable SSL for the app") AnnotateCmd.Flags().MarkDeprecated("app-ssl", "This flag is deprecated and will be removed in a future release. Use \"app-protocol\" flag with https or grpcs instead") - AnnotateCmd.Flags().IntVar(&annotateMaxRequestBodySize, "max-body-size", -1, "The maximum request body size to use") - AnnotateCmd.Flags().IntVar(&annotateReadBufferSize, "read-buffer-size", -1, "The maximum size of HTTP header read buffer in kilobytes") + AnnotateCmd.Flags().StringVar(&annotateMaxRequestBodySize, "max-body-size", strconv.Itoa(runtime.DefaultMaxRequestBodySize>>20)+"Mi", "The maximum request body size to use") + AnnotateCmd.Flags().StringVar(&annotateReadBufferSize, "read-buffer-size", strconv.Itoa(runtime.DefaultReadBufferSize>>10)+"Ki", "The maximum size of HTTP header read buffer in kilobytes") AnnotateCmd.Flags().BoolVar(&annotateHTTPStreamRequestBody, "http-stream-request-body", false, "Enable streaming request body for HTTP") AnnotateCmd.Flags().IntVar(&annotateGracefulShutdownSeconds, "graceful-shutdown-seconds", -1, "The number of seconds to wait for the app to shutdown") AnnotateCmd.Flags().BoolVar(&annotateEnableAPILogging, "enable-api-logging", false, "Enable API logging for the Dapr sidecar") diff --git a/cmd/run.go b/cmd/run.go index c6dfdd93d..31c4f6fac 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -29,6 +29,8 @@ import ( "github.com/spf13/cobra" "github.com/spf13/viper" + daprRuntime "github.com/dapr/dapr/pkg/runtime" + "github.com/dapr/cli/pkg/kubernetes" "github.com/dapr/cli/pkg/metadata" "github.com/dapr/cli/pkg/print" @@ -55,8 +57,8 @@ var ( resourcesPaths []string appSSL bool metricsPort int - maxRequestBodySize int - readBufferSize int + maxRequestBodySize string + readBufferSize string unixDomainSocket string enableAppHealth bool appHealthPath string @@ -473,8 +475,8 @@ func init() { RunCmd.Flags().MarkDeprecated("app-ssl", "This flag is deprecated and will be removed in the future releases. Use \"app-protocol\" flag with https or grpcs values instead") RunCmd.Flags().IntVarP(&metricsPort, "metrics-port", "M", -1, "The port of metrics on dapr") RunCmd.Flags().BoolP("help", "h", false, "Print this help message") - RunCmd.Flags().IntVarP(&maxRequestBodySize, "max-body-size", "", -1, "Max size of request body in MB") - RunCmd.Flags().IntVarP(&readBufferSize, "read-buffer-size", "", -1, "HTTP header read buffer in KB") + RunCmd.Flags().StringVarP(&maxRequestBodySize, "max-body-size", "", strconv.Itoa(daprRuntime.DefaultMaxRequestBodySize>>20)+"Mi", "Max size of request body in MB") + RunCmd.Flags().StringVarP(&readBufferSize, "read-buffer-size", "", strconv.Itoa(daprRuntime.DefaultReadBufferSize>>10)+"Ki", "HTTP header read buffer in KB") RunCmd.Flags().StringVarP(&unixDomainSocket, "unix-domain-socket", "u", "", "Path to a unix domain socket dir. If specified, Dapr API servers will use Unix Domain Sockets") RunCmd.Flags().BoolVar(&enableAppHealth, "enable-app-health-check", false, "Enable health checks for the application using the protocol defined with app-protocol") RunCmd.Flags().StringVar(&appHealthPath, "app-health-check-path", "", "Path used for health checks; HTTP only") diff --git a/pkg/runexec/runexec_test.go b/pkg/runexec/runexec_test.go index 14ff619cf..740cdb159 100644 --- a/pkg/runexec/runexec_test.go +++ b/pkg/runexec/runexec_test.go @@ -20,6 +20,8 @@ import ( "strings" "testing" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" "github.com/dapr/cli/pkg/standalone" @@ -180,8 +182,8 @@ func TestRun(t *testing.T) { AppProtocol: "http", ComponentsPath: componentsDir, AppSSL: true, - MaxRequestBodySize: -1, - HTTPReadBufferSize: -1, + MaxRequestBodySize: "-1", + HTTPReadBufferSize: "-1", EnableAPILogging: true, APIListenAddresses: "127.0.0.1", } @@ -294,8 +296,8 @@ func TestRun(t *testing.T) { basicConfig.ProfilePort = 0 basicConfig.EnableProfiling = true basicConfig.MaxConcurrency = 0 - basicConfig.MaxRequestBodySize = 0 - basicConfig.HTTPReadBufferSize = 0 + basicConfig.MaxRequestBodySize = "" + basicConfig.HTTPReadBufferSize = "" basicConfig.AppProtocol = "" basicConfig.SetDefaultFromSchema() @@ -307,8 +309,8 @@ func TestRun(t *testing.T) { assert.Equal(t, -1, basicConfig.ProfilePort) assert.True(t, basicConfig.EnableProfiling) assert.Equal(t, -1, basicConfig.MaxConcurrency) - assert.Equal(t, -1, basicConfig.MaxRequestBodySize) - assert.Equal(t, -1, basicConfig.HTTPReadBufferSize) + assert.Equal(t, "4Mi", basicConfig.MaxRequestBodySize) + assert.Equal(t, "4Ki", basicConfig.HTTPReadBufferSize) assert.Equal(t, "http", basicConfig.AppProtocol) // Test after Validate gets called. @@ -322,8 +324,46 @@ func TestRun(t *testing.T) { assert.Positive(t, basicConfig.ProfilePort) assert.True(t, basicConfig.EnableProfiling) assert.Equal(t, -1, basicConfig.MaxConcurrency) - assert.Equal(t, -1, basicConfig.MaxRequestBodySize) - assert.Equal(t, -1, basicConfig.HTTPReadBufferSize) + assert.Equal(t, "4Mi", basicConfig.MaxRequestBodySize) + assert.Equal(t, "4Ki", basicConfig.HTTPReadBufferSize) assert.Equal(t, "http", basicConfig.AppProtocol) }) + + t.Run("run with max body size without units", func(t *testing.T) { + basicConfig.MaxRequestBodySize = "4000000" + + output, err := NewOutput(basicConfig) + require.NoError(t, err) + assertArgumentEqual(t, "max-body-size", "4M", output.DaprCMD.Args) + }) + + t.Run("run with max body size with units", func(t *testing.T) { + basicConfig.MaxRequestBodySize = "4Mi" + + output, err := NewOutput(basicConfig) + require.NoError(t, err) + assertArgumentEqual(t, "max-body-size", "4Mi", output.DaprCMD.Args) + + basicConfig.MaxRequestBodySize = "5M" + + output, err = NewOutput(basicConfig) + require.NoError(t, err) + assertArgumentEqual(t, "max-body-size", "5M", output.DaprCMD.Args) + }) + + t.Run("run with read buffer size set without units", func(t *testing.T) { + basicConfig.HTTPReadBufferSize = "16001" + + output, err := NewOutput(basicConfig) + require.NoError(t, err) + assertArgumentEqual(t, "read-buffer-size", "16001", output.DaprCMD.Args) + }) + + t.Run("run with read buffer size set with units", func(t *testing.T) { + basicConfig.HTTPReadBufferSize = "4Ki" + + output, err := NewOutput(basicConfig) + require.NoError(t, err) + assertArgumentEqual(t, "read-buffer-size", "4Ki", output.DaprCMD.Args) + }) } diff --git a/pkg/standalone/run.go b/pkg/standalone/run.go index 2752edab2..fddfa6bf4 100644 --- a/pkg/standalone/run.go +++ b/pkg/standalone/run.go @@ -24,6 +24,8 @@ import ( "strconv" "strings" + "k8s.io/apimachinery/pkg/api/resource" + "github.com/Pallinder/sillyname-go" "github.com/phayes/freeport" "gopkg.in/yaml.v2" @@ -79,8 +81,8 @@ type SharedRunConfig struct { ResourcesPaths []string `arg:"resources-path" yaml:"resourcesPaths"` // Speicifcally omitted from annotations as appSSL is deprecated. AppSSL bool `arg:"app-ssl" yaml:"appSSL"` - MaxRequestBodySize int `arg:"max-body-size" annotation:"dapr.io/max-body-size" yaml:"maxBodySize" default:"-1"` - HTTPReadBufferSize int `arg:"read-buffer-size" annotation:"dapr.io/read-buffer-size" yaml:"readBufferSize" default:"-1"` + MaxRequestBodySize string `arg:"max-body-size" annotation:"dapr.io/max-body-size" yaml:"maxBodySize" default:"4Mi"` + HTTPReadBufferSize string `arg:"read-buffer-size" annotation:"dapr.io/read-buffer-size" yaml:"readBufferSize" default:"4Ki"` EnableAppHealth bool `arg:"enable-app-health-check" annotation:"dapr.io/enable-app-health-check" yaml:"enableAppHealthCheck"` AppHealthPath string `arg:"app-health-check-path" annotation:"dapr.io/app-health-check-path" yaml:"appHealthCheckPath"` AppHealthInterval int `arg:"app-health-probe-interval" annotation:"dapr.io/app-health-probe-interval" ifneq:"0" yaml:"appHealthProbeInterval"` @@ -226,12 +228,27 @@ func (config *RunConfig) Validate() error { if config.MaxConcurrency < 1 { config.MaxConcurrency = -1 } - if config.MaxRequestBodySize < 0 { - config.MaxRequestBodySize = -1 + + qBody, err := resource.ParseQuantity(config.MaxRequestBodySize) + if err != nil { + return fmt.Errorf("invalid max request body size: %w", err) + } + + if qBody.Value() < 0 { + config.MaxRequestBodySize = "-1" + } else { + config.MaxRequestBodySize = qBody.String() + } + + qBuffer, err := resource.ParseQuantity(config.HTTPReadBufferSize) + if err != nil { + return fmt.Errorf("invalid http read buffer size: %w", err) } - if config.HTTPReadBufferSize < 0 { - config.HTTPReadBufferSize = -1 + if qBuffer.Value() < 0 { + config.HTTPReadBufferSize = "-1" + } else { + config.HTTPReadBufferSize = qBuffer.String() } err = config.validatePlacementHostAddr() @@ -265,12 +282,27 @@ func (config *RunConfig) ValidateK8s() error { if config.MaxConcurrency < 1 { config.MaxConcurrency = -1 } - if config.MaxRequestBodySize < 0 { - config.MaxRequestBodySize = -1 + + qBody, err := resource.ParseQuantity(config.MaxRequestBodySize) + if err != nil { + return fmt.Errorf("invalid max request body size: %w", err) + } + + if qBody.Value() < 0 { + config.MaxRequestBodySize = "-1" + } else { + config.MaxRequestBodySize = qBody.String() + } + + qBuffer, err := resource.ParseQuantity(config.HTTPReadBufferSize) + if err != nil { + return fmt.Errorf("invalid http read buffer size: %w", err) } - if config.HTTPReadBufferSize < 0 { - config.HTTPReadBufferSize = -1 + if qBuffer.Value() < 0 { + config.HTTPReadBufferSize = "-1" + } else { + config.HTTPReadBufferSize = qBuffer.String() } return nil