diff --git a/config/crd/bases/k8s.nginx.org_virtualserverroutes.yaml b/config/crd/bases/k8s.nginx.org_virtualserverroutes.yaml index 9b3afbe076..22cc7c221a 100644 --- a/config/crd/bases/k8s.nginx.org_virtualserverroutes.yaml +++ b/config/crd/bases/k8s.nginx.org_virtualserverroutes.yaml @@ -872,6 +872,12 @@ spec: is set in the proxy-buffers ConfigMap key. type: string type: object + busy-buffers-size: + description: Sets the size of the buffers used for reading a + response from the upstream server when the proxy_buffering + is enabled. The default is set in the proxy-busy-buffers-size + ConfigMap key.' + type: string client-max-body-size: description: Sets the maximum allowed size of the client request body. The default is set in the client-max-body-size ConfigMap diff --git a/config/crd/bases/k8s.nginx.org_virtualservers.yaml b/config/crd/bases/k8s.nginx.org_virtualservers.yaml index 8e4cafe4e3..995786bfce 100644 --- a/config/crd/bases/k8s.nginx.org_virtualservers.yaml +++ b/config/crd/bases/k8s.nginx.org_virtualservers.yaml @@ -1061,6 +1061,12 @@ spec: is set in the proxy-buffers ConfigMap key. type: string type: object + busy-buffers-size: + description: Sets the size of the buffers used for reading a + response from the upstream server when the proxy_buffering + is enabled. The default is set in the proxy-busy-buffers-size + ConfigMap key.' + type: string client-max-body-size: description: Sets the maximum allowed size of the client request body. The default is set in the client-max-body-size ConfigMap diff --git a/deploy/crds.yaml b/deploy/crds.yaml index 5b80d9dff9..d1db1df275 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -1805,6 +1805,12 @@ spec: is set in the proxy-buffers ConfigMap key. type: string type: object + busy-buffers-size: + description: Sets the size of the buffers used for reading a + response from the upstream server when the proxy_buffering + is enabled. The default is set in the proxy-busy-buffers-size + ConfigMap key.' + type: string client-max-body-size: description: Sets the maximum allowed size of the client request body. The default is set in the client-max-body-size ConfigMap @@ -3217,6 +3223,12 @@ spec: is set in the proxy-buffers ConfigMap key. type: string type: object + busy-buffers-size: + description: Sets the size of the buffers used for reading a + response from the upstream server when the proxy_buffering + is enabled. The default is set in the proxy-busy-buffers-size + ConfigMap key.' + type: string client-max-body-size: description: Sets the maximum allowed size of the client request body. The default is set in the client-max-body-size ConfigMap diff --git a/docs/crd/k8s.nginx.org_virtualserverroutes.md b/docs/crd/k8s.nginx.org_virtualserverroutes.md index 93bc49f191..bcf4c2a925 100644 --- a/docs/crd/k8s.nginx.org_virtualserverroutes.md +++ b/docs/crd/k8s.nginx.org_virtualserverroutes.md @@ -168,6 +168,7 @@ The `.spec` object supports the following fields: | `upstreams[].buffers` | `object` | Configures the buffers used for reading a response from the upstream server for a single connection. | | `upstreams[].buffers.number` | `integer` | Configures the number of buffers. The default is set in the proxy-buffers ConfigMap key. | | `upstreams[].buffers.size` | `string` | Configures the size of a buffer. The default is set in the proxy-buffers ConfigMap key. | +| `upstreams[].busy-buffers-size` | `string` | Sets the size of the buffers used for reading a response from the upstream server when the proxy_buffering is enabled. The default is set in the proxy-busy-buffers-size ConfigMap key.' | | `upstreams[].client-max-body-size` | `string` | Sets the maximum allowed size of the client request body. The default is set in the client-max-body-size ConfigMap key. | | `upstreams[].connect-timeout` | `string` | The timeout for establishing a connection with an upstream server. The default is specified in the proxy-connect-timeout ConfigMap key. | | `upstreams[].fail-timeout` | `string` | The time during which the specified number of unsuccessful attempts to communicate with an upstream server should happen to consider the server unavailable. The default is set in the fail-timeout ConfigMap key. | diff --git a/docs/crd/k8s.nginx.org_virtualservers.md b/docs/crd/k8s.nginx.org_virtualservers.md index 154584e564..cc0268c773 100644 --- a/docs/crd/k8s.nginx.org_virtualservers.md +++ b/docs/crd/k8s.nginx.org_virtualservers.md @@ -203,6 +203,7 @@ The `.spec` object supports the following fields: | `upstreams[].buffers` | `object` | Configures the buffers used for reading a response from the upstream server for a single connection. | | `upstreams[].buffers.number` | `integer` | Configures the number of buffers. The default is set in the proxy-buffers ConfigMap key. | | `upstreams[].buffers.size` | `string` | Configures the size of a buffer. The default is set in the proxy-buffers ConfigMap key. | +| `upstreams[].busy-buffers-size` | `string` | Sets the size of the buffers used for reading a response from the upstream server when the proxy_buffering is enabled. The default is set in the proxy-busy-buffers-size ConfigMap key.' | | `upstreams[].client-max-body-size` | `string` | Sets the maximum allowed size of the client request body. The default is set in the client-max-body-size ConfigMap key. | | `upstreams[].connect-timeout` | `string` | The timeout for establishing a connection with an upstream server. The default is specified in the proxy-connect-timeout ConfigMap key. | | `upstreams[].fail-timeout` | `string` | The time during which the specified number of unsuccessful attempts to communicate with an upstream server should happen to consider the server unavailable. The default is set in the fail-timeout ConfigMap key. | diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index 8718dd151e..7756d56e56 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -6,6 +6,7 @@ import ( "slices" nl "github.com/nginx/kubernetes-ingress/internal/logger" + "github.com/nginx/kubernetes-ingress/internal/validation" ) // JWTKeyAnnotation is the annotation where the Secret with a JWK is specified. @@ -296,12 +297,48 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool } } + // Proxy Buffers uses number + size format, like "8 4k". if proxyBuffers, exists := ingEx.Ingress.Annotations["nginx.org/proxy-buffers"]; exists { - cfgParams.ProxyBuffers = proxyBuffers + proxyBufferUnits, err := validation.NewNumberSizeConfig(proxyBuffers) + if err != nil { + nl.Errorf(l, "error parsing nginx.org/proxy-buffers: %s", err) + } else { + cfgParams.ProxyBuffers = proxyBufferUnits + } } + // Proxy Buffer Size uses only size format, like "4k". if proxyBufferSize, exists := ingEx.Ingress.Annotations["nginx.org/proxy-buffer-size"]; exists { - cfgParams.ProxyBufferSize = proxyBufferSize + proxyBufferSizeUnit, err := validation.NewSizeWithUnit(proxyBufferSize) + if err != nil { + nl.Errorf(l, "error parsing nginx.org/proxy-buffer-size: %s", err) + } else { + cfgParams.ProxyBufferSize = proxyBufferSizeUnit + } + } + + // Proxy Busy Buffers Size uses only size format, like "8k". + if proxyBusyBuffersSize, exists := ingEx.Ingress.Annotations["nginx.org/proxy-busy-buffers-size"]; exists { + proxyBusyBufferSizeUnit, err := validation.NewSizeWithUnit(proxyBusyBuffersSize) + if err != nil { + nl.Errorf(l, "error parsing nginx.org/proxy-busy-buffers-size: %s", err) + } else { + cfgParams.ProxyBusyBuffersSize = proxyBusyBufferSizeUnit + } + } + + balancedProxyBuffers, balancedProxyBufferSize, balancedProxyBusyBufferSize, modifications, err := validation.BalanceProxyValues(cfgParams.ProxyBuffers, cfgParams.ProxyBufferSize, cfgParams.ProxyBusyBuffersSize) + if err != nil { + nl.Errorf(l, "error reconciling proxy_buffers, proxy_buffer_size, and proxy_busy_buffers_size values: %s", err.Error()) + } + cfgParams.ProxyBuffers = balancedProxyBuffers + cfgParams.ProxyBufferSize = balancedProxyBufferSize + cfgParams.ProxyBusyBuffersSize = balancedProxyBusyBufferSize + + if len(modifications) > 0 { + for _, modification := range modifications { + nl.Infof(l, "Changes made to proxy values: %s", modification) + } } if upstreamZoneSize, exists := ingEx.Ingress.Annotations["nginx.org/upstream-zone-size"]; exists { diff --git a/internal/configs/config_params.go b/internal/configs/config_params.go index c73310056d..4197a2f976 100644 --- a/internal/configs/config_params.go +++ b/internal/configs/config_params.go @@ -5,6 +5,7 @@ import ( "github.com/nginx/kubernetes-ingress/internal/configs/version2" "github.com/nginx/kubernetes-ingress/internal/nginx" + "github.com/nginx/kubernetes-ingress/internal/validation" ) // ConfigParams holds NGINX configuration parameters that affect the main NGINX config @@ -69,8 +70,9 @@ type ConfigParams struct { MainAppProtectDosLogFormatEscaping string MainAppProtectDosArbFqdn string ProxyBuffering bool - ProxyBuffers string - ProxyBufferSize string + ProxyBuffers validation.NumberSizeConfig + ProxyBufferSize validation.SizeWithUnit + ProxyBusyBuffersSize validation.SizeWithUnit ProxyConnectTimeout string ProxyHideHeaders []string ProxyMaxTempFileSize string diff --git a/internal/configs/configmaps.go b/internal/configs/configmaps.go index 7a1c7cd001..97812da100 100644 --- a/internal/configs/configmaps.go +++ b/internal/configs/configmaps.go @@ -335,12 +335,53 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has } if proxyBuffers, exists := cfgm.Data["proxy-buffers"]; exists { - cfgParams.ProxyBuffers = proxyBuffers + proxyBuffersData, err := validation.NewNumberSizeConfig(proxyBuffers) + if err != nil { + wrappedError := fmt.Errorf("ConfigMap %s/%s: invalid value for 'proxy-buffers': %w", cfgm.GetNamespace(), cfgm.GetName(), err) + + nl.Errorf(l, "%s", wrappedError.Error()) + eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, wrappedError.Error()) + configOk = false + } else { + cfgParams.ProxyBuffers = proxyBuffersData + } } if proxyBufferSize, exists := cfgm.Data["proxy-buffer-size"]; exists { - cfgParams.ProxyBufferSize = proxyBufferSize + proxyBufferSizeData, err := validation.NewSizeWithUnit(proxyBufferSize) + if err != nil { + nl.Errorf(l, "error parsing nginx.org/proxy-buffer-size: %s", err) + } else { + cfgParams.ProxyBufferSize = proxyBufferSizeData + } + // normalizedProxyBufferSize := validation.NormalizeBufferSize(proxyBufferSize) + } + + // Proxy Busy Buffers Size uses only size format, like "8k". + if proxyBusyBuffersSize, exists := cfgm.Data["proxy-busy-buffers-size"]; exists { + proxyBusyBufferSizeUnit, err := validation.NewSizeWithUnit(proxyBusyBuffersSize) + if err != nil { + nl.Errorf(l, "error parsing nginx.org/proxy-busy-buffers-size: %s", err) + } else { + cfgParams.ProxyBusyBuffersSize = proxyBusyBufferSizeUnit + } + } + + balancedProxyBuffers, balancedProxyBufferSize, balancedProxyBusyBufferSize, modifications, err := validation.BalanceProxyValues(cfgParams.ProxyBuffers, cfgParams.ProxyBufferSize, cfgParams.ProxyBusyBuffersSize) + if err != nil { + nl.Errorf(l, "error reconciling proxy_buffers, proxy_buffer_size, and proxy_busy_buffers_size values: %s", err.Error()) } + cfgParams.ProxyBuffers = balancedProxyBuffers + cfgParams.ProxyBufferSize = balancedProxyBufferSize + cfgParams.ProxyBusyBuffersSize = balancedProxyBusyBufferSize + + if len(modifications) > 0 { + for _, modification := range modifications { + nl.Infof(l, "Changes made to proxy values: %s", modification) + } + } + + // Normalise the three proxy buffer values across each other. if proxyMaxTempFileSize, exists := cfgm.Data["proxy-max-temp-file-size"]; exists { cfgParams.ProxyMaxTempFileSize = proxyMaxTempFileSize @@ -408,7 +449,7 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has } } - _, err := parseConfigMapZoneSync(l, cfgm, cfgParams, eventLog, nginxPlus) + _, err = parseConfigMapZoneSync(l, cfgm, cfgParams, eventLog, nginxPlus) if err != nil { configOk = false } diff --git a/internal/configs/configmaps_test.go b/internal/configs/configmaps_test.go index b4afbd6613..ad3e2174ed 100644 --- a/internal/configs/configmaps_test.go +++ b/internal/configs/configmaps_test.go @@ -7,8 +7,10 @@ import ( "testing" "github.com/nginx/kubernetes-ingress/internal/configs/commonhelpers" + "github.com/nginx/kubernetes-ingress/internal/validation" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" ) @@ -1924,6 +1926,280 @@ func TestOpenTelemetryConfigurationInvalid(t *testing.T) { } } +func TestParseProxyBuffers(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + configMap *v1.ConfigMap + expectedProxyBuffers validation.NumberSizeConfig + expectedProxyBufferSize validation.SizeWithUnit + expectedProxyBusyBuffersSize validation.SizeWithUnit + description string + }{ + { + name: "all proxy buffer settings provided", + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "proxy-buffers": "8 4k", + "proxy-buffer-size": "8k", + "proxy-busy-buffers-size": "16k", + }, + }, + expectedProxyBuffers: validation.NumberSizeConfig{ + Number: 8, + Size: validation.SizeWithUnit{ + Size: 4, + Unit: validation.SizeKB, + }, + }, + expectedProxyBufferSize: validation.SizeWithUnit{ + Size: 8, + Unit: validation.SizeKB, + }, + expectedProxyBusyBuffersSize: validation.SizeWithUnit{ + Size: 16, + Unit: validation.SizeKB, + }, + description: "should parse all proxy buffer settings correctly", + }, + { + name: "only proxy-buffers provided", + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "proxy-buffers": "16 8k", + }, + }, + expectedProxyBuffers: validation.NumberSizeConfig{ + Number: 16, + Size: validation.SizeWithUnit{ + Size: 8, + Unit: validation.SizeKB, + }, + }, + expectedProxyBufferSize: validation.SizeWithUnit{ + Size: 8, + Unit: validation.SizeKB, + }, + expectedProxyBusyBuffersSize: validation.SizeWithUnit{ + Size: 8, + Unit: validation.SizeKB, + }, + description: "should parse proxy-buffers only", + }, + { + name: "only proxy-buffer-size provided", + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "proxy-buffer-size": "16k", + }, + }, + expectedProxyBuffers: validation.NumberSizeConfig{ + Number: 2, + Size: validation.SizeWithUnit{ + Size: 4, + Unit: validation.SizeKB, + }, + }, + expectedProxyBufferSize: validation.SizeWithUnit{ + Size: 4, + Unit: validation.SizeKB, + }, + expectedProxyBusyBuffersSize: validation.SizeWithUnit{ + Size: 4, + Unit: validation.SizeKB, + }, + description: "should parse proxy-buffer-size only", + }, + { + name: "case insensitive units get normalized", + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "proxy-buffers": "8 4K", + "proxy-buffer-size": "8K", + "proxy-busy-buffers-size": "16K", + }, + }, + expectedProxyBuffers: validation.NumberSizeConfig{ + Number: 8, + Size: validation.SizeWithUnit{ + Size: 4, + Unit: validation.SizeKB, + }, + }, + expectedProxyBufferSize: validation.SizeWithUnit{ + Size: 8, + Unit: validation.SizeKB, + }, + expectedProxyBusyBuffersSize: validation.SizeWithUnit{ + Size: 16, + Unit: validation.SizeKB, + }, + description: "should normalize case insensitive units", + }, + { + name: "invalid units get normalized", + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "proxy-buffers": "8 4g", + "proxy-buffer-size": "8x", + "proxy-busy-buffers-size": "16z", + }, + }, + expectedProxyBuffers: validation.NumberSizeConfig{ + Number: 8, + Size: validation.SizeWithUnit{ + Size: 4, + Unit: validation.SizeMB, + }, + }, + expectedProxyBufferSize: validation.SizeWithUnit{ + Size: 8, + Unit: validation.SizeMB, + }, + expectedProxyBusyBuffersSize: validation.SizeWithUnit{ + Size: 16, + Unit: validation.SizeMB, + }, + description: "should normalize invalid units to 'm'", + }, + { + name: "empty configmap", + configMap: &v1.ConfigMap{ + Data: map[string]string{}, + }, + expectedProxyBuffers: validation.NumberSizeConfig{}, + expectedProxyBufferSize: validation.SizeWithUnit{}, + expectedProxyBusyBuffersSize: validation.SizeWithUnit{}, + description: "should handle empty configmap gracefully", + }, + } + + nginxPlus := true + hasAppProtect := false + hasAppProtectDos := false + hasTLSPassthrough := false + + for _, test := range tests { + test := test // capture range variable + + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + eventRecorder := makeEventLogger() + result, configOk := ParseConfigMap(context.Background(), test.configMap, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, eventRecorder) + + if !configOk { + t.Errorf("%s: expected config to be valid but got invalid", test.description) + } + + if result.ProxyBuffers != test.expectedProxyBuffers { + t.Errorf("%s: ProxyBuffers = %q, want %q", test.description, result.ProxyBuffers, test.expectedProxyBuffers) + } + + if result.ProxyBufferSize != test.expectedProxyBufferSize { + t.Errorf("%s: ProxyBufferSize = %q, want %q", test.description, result.ProxyBufferSize, test.expectedProxyBufferSize) + } + + if result.ProxyBusyBuffersSize != test.expectedProxyBusyBuffersSize { + t.Errorf("%s: ProxyBusyBuffersSize = %q, want %q", test.description, result.ProxyBusyBuffersSize, test.expectedProxyBusyBuffersSize) + } + + fakeRecorder := eventRecorder.(*record.FakeRecorder) + if len(fakeRecorder.Events) > 0 { + t.Errorf("%s: unexpected warnings generated: %d events", test.description, len(fakeRecorder.Events)) + } + }) + } +} + +func TestParseProxyBuffersInvalidFormat(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + proxyBuffers string + expectValid bool + description string + }{ + { + name: "valid format", + proxyBuffers: "4 8k", + expectValid: true, + description: "should accept valid 'count size' format", + }, + { + name: "invalid - only size", + proxyBuffers: "1k", + expectValid: false, + description: "should reject format with only size", + }, + { + name: "invalid - only count", + proxyBuffers: "4", + expectValid: false, + description: "should reject format with only count", + }, + { + name: "invalid - three parts", + proxyBuffers: "4 8k extra", + expectValid: false, + description: "should reject format with too many parts", + }, + { + name: "invalid - empty", + proxyBuffers: "", + expectValid: true, + description: "should accept empty string (will get corrected)", + }, + } + + nginxPlus := true + hasAppProtect := false + hasAppProtectDos := false + hasTLSPassthrough := false + + for _, test := range tests { + test := test // capture range variable + + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + cm := &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-configmap", + Namespace: "default", + }, + Data: map[string]string{ + "proxy-buffers": test.proxyBuffers, + }, + } + + eventRecorder := makeEventLogger() + result, configOk := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, eventRecorder) + + if configOk != test.expectValid { + t.Errorf("%s: expected configOk=%v, got configOk=%v", test.description, test.expectValid, configOk) + } + + if test.expectValid { + if result.ProxyBuffers.String() != test.proxyBuffers { + t.Errorf("%s: expected ProxyBuffers=%q, got %q", test.description, test.proxyBuffers, result.ProxyBuffers) + } + } else { + if result.ProxyBuffers.String() != "" { + t.Errorf("%s: expected ProxyBuffers to be empty for invalid config, got %q", test.description, result.ProxyBuffers) + } + + fakeRecorder := eventRecorder.(*record.FakeRecorder) + if len(fakeRecorder.Events) == 0 { + t.Errorf("%s: expected error event to be generated for invalid config", test.description) + } + } + }) + } +} + func makeEventLogger() record.EventRecorder { return record.NewFakeRecorder(1024) } diff --git a/internal/configs/ingress.go b/internal/configs/ingress.go index 123bf02afa..f74f70dde1 100644 --- a/internal/configs/ingress.go +++ b/internal/configs/ingress.go @@ -499,8 +499,9 @@ func createLocation(path string, upstream version1.Upstream, cfg *ConfigParams, SSL: ssl, GRPC: grpc, ProxyBuffering: cfg.ProxyBuffering, - ProxyBuffers: cfg.ProxyBuffers, - ProxyBufferSize: cfg.ProxyBufferSize, + ProxyBuffers: cfg.ProxyBuffers.String(), + ProxyBufferSize: cfg.ProxyBufferSize.String(), + ProxyBusyBuffersSize: cfg.ProxyBusyBuffersSize.String(), ProxyMaxTempFileSize: cfg.ProxyMaxTempFileSize, ProxySSLName: proxySSLName, LocationSnippets: cfg.LocationSnippets, diff --git a/internal/configs/version1/config.go b/internal/configs/version1/config.go index 106470b865..9091036dae 100644 --- a/internal/configs/version1/config.go +++ b/internal/configs/version1/config.go @@ -180,6 +180,7 @@ type Location struct { ProxyBuffering bool ProxyBuffers string ProxyBufferSize string + ProxyBusyBuffersSize string ProxyMaxTempFileSize string ProxySSLName string JWTAuth *JWTAuth diff --git a/internal/configs/version1/nginx-plus.ingress.tmpl b/internal/configs/version1/nginx-plus.ingress.tmpl index 94f30de87f..6bce2163c1 100644 --- a/internal/configs/version1/nginx-plus.ingress.tmpl +++ b/internal/configs/version1/nginx-plus.ingress.tmpl @@ -301,6 +301,9 @@ server { {{- if $location.ProxyBufferSize}} proxy_buffer_size {{$location.ProxyBufferSize}}; {{- end}} + {{- if $location.ProxyBusyBuffersSize}} + proxy_busy_buffers_size {{$location.ProxyBusyBuffersSize}}; + {{- end}} {{- if $location.ProxyMaxTempFileSize}} proxy_max_temp_file_size {{$location.ProxyMaxTempFileSize}}; {{- end}} diff --git a/internal/configs/version1/nginx.ingress.tmpl b/internal/configs/version1/nginx.ingress.tmpl index 1d7ca8cd87..4b10efa90b 100644 --- a/internal/configs/version1/nginx.ingress.tmpl +++ b/internal/configs/version1/nginx.ingress.tmpl @@ -204,13 +204,9 @@ server { proxy_set_header X-Forwarded-Port $server_port; proxy_set_header X-Forwarded-Proto {{if $server.RedirectToHTTPS}}https{{else}}$scheme{{end}}; proxy_buffering {{if $location.ProxyBuffering}}on{{else}}off{{end}}; - - {{- if $location.ProxyBuffers}} - proxy_buffers {{$location.ProxyBuffers}}; - {{- end}} - {{- if $location.ProxyBufferSize}} - proxy_buffer_size {{$location.ProxyBufferSize}}; - {{- end}} + {{- if $location.ProxyBuffers}}proxy_buffers {{$location.ProxyBuffers}};{{end}} + {{- if $location.ProxyBufferSize}}proxy_buffer_size {{$location.ProxyBufferSize}};{{end}} + {{- if $location.ProxyBusyBuffersSize}}proxy_busy_buffers_size {{$location.ProxyBusyBuffersSize}};{{end}} {{- if $location.ProxyMaxTempFileSize}} proxy_max_temp_file_size {{$location.ProxyMaxTempFileSize}}; {{- end}} diff --git a/internal/configs/version2/__snapshots__/templates_test.snap b/internal/configs/version2/__snapshots__/templates_test.snap index c7dc116be4..4b74074a93 100644 --- a/internal/configs/version2/__snapshots__/templates_test.snap +++ b/internal/configs/version2/__snapshots__/templates_test.snap @@ -413,6 +413,7 @@ server { proxy_buffering on; proxy_buffers 8 4k; proxy_buffer_size 4k; + proxy_busy_buffers_size 8k; proxy_http_version 1.1; proxy_set_header Upgrade $http_upgrade; proxy_set_header Connection $vs_connection_header; @@ -838,6 +839,7 @@ server { proxy_buffering on; proxy_buffers 8 4k; proxy_buffer_size 4k; + proxy_busy_buffers_size 8k; proxy_http_version 1.1; proxy_set_header Upgrade $http_upgrade; proxy_set_header Connection $vs_connection_header; @@ -2818,6 +2820,7 @@ server { proxy_buffering on; proxy_buffers 8 4k; proxy_buffer_size 4k; + proxy_busy_buffers_size 8k; proxy_http_version 1.1; proxy_set_header Upgrade $http_upgrade; proxy_set_header Connection $vs_connection_header; @@ -3220,6 +3223,7 @@ server { proxy_buffering on; proxy_buffers 8 4k; proxy_buffer_size 4k; + proxy_busy_buffers_size 8k; proxy_http_version 1.1; proxy_set_header Upgrade $http_upgrade; proxy_set_header Connection $vs_connection_header; diff --git a/internal/configs/version2/http.go b/internal/configs/version2/http.go index f835656fb9..aa09b4fdc4 100644 --- a/internal/configs/version2/http.go +++ b/internal/configs/version2/http.go @@ -199,6 +199,7 @@ type Location struct { ProxyBuffering bool ProxyBuffers string ProxyBufferSize string + ProxyBusyBuffersSize string ProxyPass string ProxyNextUpstream string ProxyNextUpstreamTimeout string diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 90f264ded7..ac287dd18a 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -612,11 +612,20 @@ server { {{- end }} proxy_buffering {{ if $l.ProxyBuffering }}on{{ else }}off{{ end }}; - {{- if $l.ProxyBuffers }} - proxy_buffers {{ $l.ProxyBuffers }}; - {{- end }} - {{- if $l.ProxyBufferSize }} - {{ $proxyOrGRPC }}_buffer_size {{ $l.ProxyBufferSize }}; + {{- if not $l.GRPCPass }} + {{- if $l.ProxyBuffers}} + proxy_buffers {{$l.ProxyBuffers}}; + {{- end}} + {{- if $l.ProxyBufferSize}} + proxy_buffer_size {{$l.ProxyBufferSize}}; + {{- end}} + {{- if $l.ProxyBusyBuffersSize}} + proxy_busy_buffers_size {{$l.ProxyBusyBuffersSize}}; + {{- end}} + {{- else }} + {{- if $l.ProxyBufferSize }} + grpc_buffer_size {{ $l.ProxyBufferSize }}; + {{- end }} {{- end }} {{- if not $l.GRPCPass }} proxy_http_version 1.1; diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index 4721b3c879..4ba5a72b58 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -352,11 +352,20 @@ server { {{- end }} proxy_buffering {{ if $l.ProxyBuffering }}on{{ else }}off{{ end }}; - {{- if $l.ProxyBuffers }} - proxy_buffers {{ $l.ProxyBuffers }}; - {{- end }} - {{- if $l.ProxyBufferSize }} - {{ $proxyOrGRPC }}_buffer_size {{ $l.ProxyBufferSize }}; + {{- if not $l.GRPCPass }} + {{- if $l.ProxyBuffers}} + proxy_buffers {{$l.ProxyBuffers}}; + {{- end}} + {{- if $l.ProxyBufferSize}} + proxy_buffer_size {{$l.ProxyBufferSize}}; + {{- end}} + {{- if $l.ProxyBusyBuffersSize}} + proxy_busy_buffers_size {{$l.ProxyBusyBuffersSize}}; + {{- end}} + {{- else }} + {{- if $l.ProxyBufferSize }} + grpc_buffer_size {{ $l.ProxyBufferSize }}; + {{- end }} {{- end }} {{- if not $l.GRPCPass }} proxy_http_version 1.1; diff --git a/internal/configs/version2/templates_test.go b/internal/configs/version2/templates_test.go index beaa511dfa..fce352eaa9 100644 --- a/internal/configs/version2/templates_test.go +++ b/internal/configs/version2/templates_test.go @@ -1088,6 +1088,7 @@ func vsConfig() VirtualServerConfig { ProxyBuffering: true, ProxyBuffers: "8 4k", ProxyBufferSize: "4k", + ProxyBusyBuffersSize: "8k", ProxyMaxTempFileSize: "1024m", ProxyPass: "http://test-upstream", ProxyNextUpstream: "error timeout", @@ -1451,6 +1452,7 @@ var ( ProxyBuffering: true, ProxyBuffers: "8 4k", ProxyBufferSize: "4k", + ProxyBusyBuffersSize: "8k", ProxyMaxTempFileSize: "1024m", ProxyPass: "http://test-upstream", ProxyNextUpstream: "error timeout", @@ -1800,6 +1802,7 @@ var ( ProxyBuffering: true, ProxyBuffers: "8 4k", ProxyBufferSize: "4k", + ProxyBusyBuffersSize: "8k", ProxyMaxTempFileSize: "1024m", ProxyPass: "http://test-upstream", ProxyNextUpstream: "error timeout", diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 9cc56a1942..1f5526bf5d 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -2493,8 +2493,9 @@ func generateLocationForProxying(path string, upstreamName string, upstream conf ClientMaxBodySize: generateString(upstream.ClientMaxBodySize, cfgParams.ClientMaxBodySize), ProxyMaxTempFileSize: cfgParams.ProxyMaxTempFileSize, ProxyBuffering: generateBool(upstream.ProxyBuffering, cfgParams.ProxyBuffering), - ProxyBuffers: generateBuffers(upstream.ProxyBuffers, cfgParams.ProxyBuffers), - ProxyBufferSize: generateString(upstream.ProxyBufferSize, cfgParams.ProxyBufferSize), + ProxyBuffers: generateBuffers(upstream.ProxyBuffers, cfgParams.ProxyBuffers.String()), + ProxyBufferSize: generateString(upstream.ProxyBufferSize, cfgParams.ProxyBufferSize.String()), + ProxyBusyBuffersSize: generateString(upstream.ProxyBusyBuffersSize, cfgParams.ProxyBusyBuffersSize.String()), ProxyPass: generateProxyPass(upstream.TLS.Enable, upstreamName, internal, proxy), ProxyNextUpstream: generateString(upstream.ProxyNextUpstream, "error timeout"), ProxyNextUpstreamTimeout: generateTimeWithDefault(upstream.ProxyNextUpstreamTimeout, "0s"), diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 106e4162e2..88c02f893a 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -18,6 +18,7 @@ import ( nic_glog "github.com/nginx/kubernetes-ingress/internal/logger/glog" "github.com/nginx/kubernetes-ingress/internal/logger/levels" "github.com/nginx/kubernetes-ingress/internal/nginx" + "github.com/nginx/kubernetes-ingress/internal/validation" conf_v1 "github.com/nginx/kubernetes-ingress/pkg/apis/configuration/v1" api_v1 "k8s.io/api/core/v1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -15221,9 +15222,22 @@ func TestGenerateLocationForProxying(t *testing.T) { ClientMaxBodySize: "1m", ProxyMaxTempFileSize: "1024m", ProxyBuffering: true, - ProxyBuffers: "8 4k", - ProxyBufferSize: "4k", - LocationSnippets: []string{"# location snippet"}, + ProxyBuffers: validation.NumberSizeConfig{ + Number: 8, + Size: validation.SizeWithUnit{ + Size: 4, + Unit: validation.SizeKB, + }, + }, + ProxyBufferSize: validation.SizeWithUnit{ + Size: 4, + Unit: validation.SizeKB, + }, + ProxyBusyBuffersSize: validation.SizeWithUnit{ + Size: 8, + Unit: validation.SizeKB, + }, + LocationSnippets: []string{"# location snippet"}, } path := "/" upstreamName := "test-upstream" @@ -15240,6 +15254,7 @@ func TestGenerateLocationForProxying(t *testing.T) { ProxyBuffering: true, ProxyBuffers: "8 4k", ProxyBufferSize: "4k", + ProxyBusyBuffersSize: "8k", ProxyPass: "http://test-upstream", ProxyNextUpstream: "error timeout", ProxyNextUpstreamTimeout: "0s", @@ -15268,10 +15283,23 @@ func TestGenerateLocationForGrpcProxying(t *testing.T) { ClientMaxBodySize: "1m", ProxyMaxTempFileSize: "1024m", ProxyBuffering: true, - ProxyBuffers: "8 4k", - ProxyBufferSize: "4k", - LocationSnippets: []string{"# location snippet"}, - HTTP2: true, + ProxyBuffers: validation.NumberSizeConfig{ + Number: 8, + Size: validation.SizeWithUnit{ + Size: 4, + Unit: validation.SizeKB, + }, + }, + ProxyBufferSize: validation.SizeWithUnit{ + Size: 4, + Unit: validation.SizeKB, + }, + ProxyBusyBuffersSize: validation.SizeWithUnit{ + Size: 8, + Unit: validation.SizeKB, + }, + LocationSnippets: []string{"# location snippet"}, + HTTP2: true, } path := "/" upstreamName := "test-upstream" @@ -15288,6 +15316,7 @@ func TestGenerateLocationForGrpcProxying(t *testing.T) { ProxyBuffering: true, ProxyBuffers: "8 4k", ProxyBufferSize: "4k", + ProxyBusyBuffersSize: "8k", ProxyPass: "http://test-upstream", ProxyNextUpstream: "error timeout", ProxyNextUpstreamTimeout: "0s", diff --git a/internal/validation/data_types.go b/internal/validation/data_types.go new file mode 100644 index 0000000000..6048bd0f79 --- /dev/null +++ b/internal/validation/data_types.go @@ -0,0 +1,259 @@ +package validation + +import ( + "fmt" + "strconv" + "strings" +) + +const ( + // DefaultPageSize is one page size to be used for default values in NGINX. + // 4k page size is fairly + DefaultPageSize = "4k" +) + +// SizeUnit moves validation and normalisation of incoming string into a custom +// type so we can pass that one around. Source for the size unit is from nginx +// documentation. @see https://nginx.org/en/docs/syntax.html +// +// This is also used for offsets like buffer sizes with badUnit. +type SizeUnit uint64 + +// SizeUnit represents the size unit used in NGINX configuration. It can be +// one of KB, MB, GB, or BadUnit for invalid sizes. +const ( + BadUnit SizeUnit = 1 << (10 * iota) + SizeKB + SizeMB + SizeGB +) + +// String returns the string representation of the SizeUnit in lowercase. +func (s SizeUnit) String() string { + switch s { + case SizeKB: + return "k" + case SizeMB: + return "m" + case SizeGB: + return "g" + default: + return "" + } +} + +// SizeWithUnit represents a size value with a unit. It's used for handling any +// NGINX configuration values that have a size type. All the size values need to +// be non-negative, hence the use of uint64 for the size. +// +// Example: "4k" represents 4 kilobytes. +type SizeWithUnit struct { + Size uint64 + Unit SizeUnit +} + +func (s SizeWithUnit) String() string { + if s.Size == 0 { + return "" + } + + return fmt.Sprintf("%d%s", s.Size, s.Unit) +} + +// SizeBytes returns the size in bytes based on the size and unit to make it +// easier to compare sizes and use them in calculations. +func (s SizeWithUnit) SizeBytes() uint64 { + return s.Size * uint64(s.Unit) +} + +// NewSizeWithUnit creates a SizeWithUnit from a string representation. +func NewSizeWithUnit(sizeStr string) (SizeWithUnit, error) { + sizeStr = strings.ToLower(strings.TrimSpace(sizeStr)) + if sizeStr == "" { + return SizeWithUnit{}, nil + } + + var unit SizeUnit + lastChar := sizeStr[len(sizeStr)-1] + numStr := sizeStr[:len(sizeStr)-1] + + switch lastChar { + case 'k': + unit = SizeKB + case 'm': + unit = SizeMB + case '0', '1', '2', '3', '4', '5', '6', '7', '8', '9': + unit = SizeMB // Default to MB if no unit is specified + numStr = sizeStr // If the last character is a digit, treat the whole string as a number + default: + unit = SizeMB + } + + num, err := strconv.ParseUint(numStr, 10, 64) + if err != nil || num < 1 { + return SizeWithUnit{}, fmt.Errorf("invalid size value, must be an integer larger than 0: %s", sizeStr) + } + + ret := SizeWithUnit{ + Size: num, + Unit: unit, + } + + if num > 1024 { + ret.Size = 1024 + } + + return ret, nil +} + +// NumberSizeConfig is a configuration that combines a number with a size. Used +// for directives that require a number and a size, like `proxy_buffer_size` or +// `client_max_body_size`. +// +// Example: "8 4k" represents 8 buffers of size 4 kilobytes. +type NumberSizeConfig struct { + Number uint64 + Size SizeWithUnit +} + +func (nsc NumberSizeConfig) String() string { + if nsc.Number == 0 || nsc.Size.Size == 0 { + return "" + } + + return fmt.Sprintf("%d %s", nsc.Number, nsc.Size) +} + +// NewNumberSizeConfig creates a NumberSizeConfig from a string representation. +func NewNumberSizeConfig(sizeStr string) (NumberSizeConfig, error) { + sizeStr = strings.ToLower(strings.TrimSpace(sizeStr)) + if sizeStr == "" { + return NumberSizeConfig{}, nil + } + + parts := strings.Fields(sizeStr) + if len(parts) != 2 { + return NumberSizeConfig{}, fmt.Errorf("invalid size format, expected ' ', got: %s", sizeStr) + } + + num, err := strconv.ParseUint(parts[0], 10, 64) + if err != nil { + return NumberSizeConfig{}, fmt.Errorf("invalid number value, could not parse into unsigned integer: %s", parts[0]) + } + + if num < 2 { + num = 2 + } + + size, err := NewSizeWithUnit(parts[1]) + if err != nil { + return NumberSizeConfig{}, fmt.Errorf("could not parse size with unit: %s", parts[1]) + } + + return NumberSizeConfig{ + Number: num, + Size: size, + }, nil +} + +// BalanceProxyValues normalises and validates the values for the proxy buffer +// configuration options and their defaults: +// * proxy_buffers 8 4k|8k (one memory page size) +// * proxy_buffer_size 4k|8k (one memory page size) +// * proxy_busy_buffers_size 8k|16k (two memory page sizes) +// +// These requirements are based on the NGINX source code. The rules and their +// priorities are: +// +// 1. there must be at least 2 proxy buffers +// 2. proxy_busy_buffers_size must be equal to or greater than the max of +// proxy_buffer_size and one of proxy_buffers +// 3. proxy_busy_buffers_size must be less than or equal to the size of all +// proxy_buffers minus one proxy_buffer +// +// The above also means that: +// 4. proxy_buffer_size must be less than or equal to the size of all +// proxy_buffers minus one proxy_buffer +// +// This function returns new values and an error. The returns in order are: +// proxy_buffers, proxy_buffer_size, proxy_busy_buffers_size, error. +func BalanceProxyValues(proxyBuffers NumberSizeConfig, proxyBufferSize, proxyBusyBuffers SizeWithUnit) (NumberSizeConfig, SizeWithUnit, SizeWithUnit, []string, error) { + modifications := make([]string, 0) + + if proxyBuffers.String() == "" && proxyBufferSize.String() == "" && proxyBusyBuffers.String() == "" { + return proxyBuffers, proxyBufferSize, proxyBusyBuffers, modifications, nil + } + + // If any of them are defined, we'll align them. + + // Create a default size so we can use it in case the values are not set. + defaultSize, err := NewSizeWithUnit(DefaultPageSize) + if err != nil { + return NumberSizeConfig{}, SizeWithUnit{}, SizeWithUnit{}, modifications, fmt.Errorf("could not create default size: %w", err) + } + + // 1.a there must be at least 2 proxy buffers + if proxyBuffers.Number < 2 { + modifications = append(modifications, fmt.Sprintf("adjusted proxy_buffers size from %d to 2", proxyBuffers.Number)) + proxyBuffers.Number = 2 + } + + // 1.b there must be at most 1024 proxy buffers + if proxyBuffers.Number > 1024 { + modifications = append(modifications, fmt.Sprintf("adjusted proxy_buffers number from %d to 1024", proxyBuffers.Number)) + proxyBuffers.Number = 1024 + } + + // 2.a proxy_buffers size must be greater than 0 + if proxyBuffers.Size.Size == 0 || proxyBuffers.Size.Unit == BadUnit { + modifications = append(modifications, fmt.Sprintf("proxy_buffers had an empty size, set it to [%s]", defaultSize)) + proxyBuffers.Size = defaultSize + } + + maxProxyBusyBuffersSize := SizeWithUnit{ + Size: proxyBuffers.Size.Size * (proxyBuffers.Number - 1), + Unit: proxyBuffers.Size.Unit, + } + + // check if proxy_buffer_size is empty, and set it to one of proxy_buffers + if proxyBufferSize.String() == "" { + modifications = append(modifications, fmt.Sprintf("proxy_buffer_size was empty, set it to one of proxy_buffers: %s", proxyBuffers.Size)) + proxyBufferSize = proxyBuffers.Size + } + + // 3. clamp proxy_buffer_size to be at most all of proxy_buffers minus one + // proxy buffer. + // + // This is needed in order to be conservative with memory (rather shrink + // than grow so we don't run into resource issues), and also to avoid + // undoing work in the last step when adjusting proxy_busy_buffers_size. + if proxyBufferSize.SizeBytes() > (proxyBuffers.Size.SizeBytes() * (proxyBuffers.Number - 1)) { + newSize := maxProxyBusyBuffersSize + + modifications = append(modifications, fmt.Sprintf("adjusted proxy_buffer_size from %s to %s because it was too big for proxy_buffers (%s)", proxyBufferSize, newSize, proxyBuffers)) + proxyBufferSize = newSize + } + + // 4. grab the max of proxy_buffer_size and one of proxy_buffers + var greaterSize SizeWithUnit + if proxyBuffers.Size.SizeBytes() > proxyBufferSize.SizeBytes() { + greaterSize = proxyBuffers.Size + } else { + greaterSize = proxyBufferSize + } + + // 4. proxy_busy_buffers_size must be equal to or greater than the max of + // proxy_buffer_size and one of proxy_buffers (greater size from above) + if proxyBusyBuffers.SizeBytes() < greaterSize.SizeBytes() { + modifications = append(modifications, fmt.Sprintf("adjusted proxy_busy_buffers_size from %s to %s because it was too small", proxyBusyBuffers, greaterSize)) + proxyBusyBuffers = greaterSize + } + + if proxyBusyBuffers.SizeBytes() > maxProxyBusyBuffersSize.SizeBytes() { + modifications = append(modifications, fmt.Sprintf("adjusted proxy_busy_buffers_size from %s to %s because it was too large", proxyBusyBuffers, maxProxyBusyBuffersSize)) + + proxyBusyBuffers = maxProxyBusyBuffersSize + } + + return proxyBuffers, proxyBufferSize, proxyBusyBuffers, modifications, nil +} diff --git a/internal/validation/data_types_test.go b/internal/validation/data_types_test.go new file mode 100644 index 0000000000..da8b59578c --- /dev/null +++ b/internal/validation/data_types_test.go @@ -0,0 +1,527 @@ +package validation_test + +import ( + "testing" + + "github.com/nginx/kubernetes-ingress/internal/validation" + "github.com/stretchr/testify/assert" +) + +func TestNewSizeWithUnit(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + sizeStr string + want string + wantErr bool + }{ + { + name: "empty string gets an empty response", + sizeStr: "", + want: "", + wantErr: false, + }, + { + name: "invalid non-numeric string", + sizeStr: "invalid", + want: "", + wantErr: true, + }, + { + name: "invalid non-numeric string with whitespace", + sizeStr: " invalid value ", + want: "", + wantErr: true, + }, + { + name: "size without unit will be assumed to be mb", + sizeStr: "1024", + want: "1024m", + wantErr: false, + }, + { + name: "valid size with k unit", + sizeStr: "4k", + want: "4k", + wantErr: false, + }, + { + name: "valid size with m unit", + sizeStr: "2m", + want: "2m", + wantErr: false, + }, + { + name: "invalid size with g unit to be replaced with m", + sizeStr: "1g", + want: "1m", + wantErr: false, + }, + { + name: "valid size with uppercase unit", + sizeStr: "8K", + want: "8k", + wantErr: false, + }, + { + name: "valid size with whitespace", + sizeStr: " 16m ", + want: "16m", + wantErr: false, + }, + { + name: "valid size with invalid unit replaced with m", + sizeStr: "32x", + want: "32m", + wantErr: false, + }, + { + name: "invalid negative size", + sizeStr: "-4k", + want: "", + wantErr: true, + }, + { + name: "invalid non-integer size", + sizeStr: "4.5m", + want: "", + wantErr: true, + }, + { + name: "invalid size exceeding uint64", + sizeStr: "18446744073709551616k", // 1 more than max uint64 + want: "", + wantErr: true, + }, + { + name: "invalid size with unit because zero", + sizeStr: "0k", + want: "", + wantErr: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got, err := validation.NewSizeWithUnit(tt.sizeStr) + if (err != nil) != tt.wantErr { + t.Errorf("Newvalidation.SizeWithUnit() error = %v, wantErr %v", err, tt.wantErr) + return + } + + if got.String() != tt.want { + t.Errorf("Newvalidation.SizeWithUnit() got = %v, want %v", got, tt.want) + } + }) + } +} + +func TestNewNumberSizeConfig(t *testing.T) { + tests := []struct { + name string + sizeStr string + want validation.NumberSizeConfig + wantErr bool + }{ + { + name: "valid number and size with k unit", + sizeStr: "8 4k", + want: validation.NumberSizeConfig{ + Number: 8, + Size: validation.SizeWithUnit{Size: 4, Unit: validation.SizeKB}, + }, + wantErr: false, + }, + { + name: "valid number and size with m unit", + sizeStr: "10 2m", + want: validation.NumberSizeConfig{ + Number: 10, + Size: validation.SizeWithUnit{Size: 2, Unit: validation.SizeMB}, + }, + wantErr: false, + }, + { + name: "valid number and size with g unit, replaced with m", + sizeStr: "3 1g", + want: validation.NumberSizeConfig{ + Number: 3, + Size: validation.SizeWithUnit{Size: 1, Unit: validation.SizeMB}, + }, + wantErr: false, + }, + { + name: "invalid zero number with valid size", + sizeStr: "0 4k", + want: validation.NumberSizeConfig{ + Number: 2, + Size: validation.SizeWithUnit{Size: 4, Unit: validation.SizeKB}, + }, + wantErr: false, + }, + { + name: "valid number with invalid size unit, replaced with m", + sizeStr: "5 4x", + want: validation.NumberSizeConfig{ + Number: 5, + Size: validation.SizeWithUnit{ + Size: 4, + Unit: validation.SizeMB, + }, + }, + wantErr: false, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got, err := validation.NewNumberSizeConfig(tt.sizeStr) + if (err != nil) != tt.wantErr { + t.Errorf("Newvalidation.NumberSizeConfig() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("Newvalidation.NumberSizeConfig() got = %v, want %v", got, tt.want) + } + }) + } +} + +func TestBalanceProxyValues(t *testing.T) { + type args struct { + proxyBuffers string + proxyBufferSize string + proxyBusyBuffersSize string + } + tests := []struct { + name string + args args + wantProxyBuffers string + wantProxyBufferSize string + wantProxyBusyBufferSize string + wantErr bool + }{ + { + name: "All empty", + wantErr: false, + }, + + { + name: "only proxy_buffer_size is defined", + args: args{ + proxyBufferSize: "4k", + }, + wantProxyBuffers: "2 4k", + wantProxyBufferSize: "4k", + wantProxyBusyBufferSize: "4k", + wantErr: false, + }, + + { + name: "only proxy_buffers is defined", + args: args{ + proxyBuffers: "4 16k", + }, + wantProxyBuffers: "4 16k", + wantProxyBufferSize: "16k", + wantProxyBusyBufferSize: "16k", + wantErr: false, + }, + + { + name: "Invalid combination that should correct itself", + args: args{ + proxyBuffers: "8 1m", + proxyBufferSize: "5m", + }, + wantProxyBuffers: "8 1m", + wantProxyBufferSize: "5m", + wantProxyBusyBufferSize: "5m", + wantErr: false, + }, + + { + name: "Buffer-size smaller than individual buffer size", + args: args{ + proxyBuffers: "4 1m", + proxyBufferSize: "512k", + }, + wantProxyBuffers: "4 1m", + wantProxyBufferSize: "512k", + wantProxyBusyBufferSize: "1m", + }, + + { + name: "Minimum buffers configuration", + args: args{ + proxyBuffers: "2 4k", + proxyBufferSize: "4k", + }, + wantProxyBuffers: "2 4k", + wantProxyBufferSize: "4k", + wantProxyBusyBufferSize: "4k", + wantErr: false, + }, + + { + name: "All three parameters set", + args: args{ + proxyBuffers: "8 4k", + proxyBufferSize: "4k", + proxyBusyBuffersSize: "16k", + }, + wantProxyBuffers: "8 4k", + wantProxyBufferSize: "4k", + wantProxyBusyBufferSize: "16k", + wantErr: false, + }, + + { + name: "Busy buffer too large - reduces in size", + args: args{ + proxyBuffers: "4 8k", + proxyBufferSize: "8k", + proxyBusyBuffersSize: "40k", + }, + wantProxyBuffers: "4 8k", + wantProxyBufferSize: "8k", + wantProxyBusyBufferSize: "24k", + wantErr: false, + }, + + { + name: "Empty/zero values - corrected to minimum", + args: args{ + proxyBuffers: "0 4k", + }, + wantProxyBuffers: "2 4k", + wantProxyBufferSize: "4k", + wantProxyBusyBufferSize: "4k", + wantErr: false, + }, + + { + name: "Extreme values - autocorrect", + args: args{ + proxyBuffers: "1000000 1k", + proxyBufferSize: "999m", + }, + wantProxyBuffers: "1024 1k", + wantProxyBufferSize: "1023k", + wantProxyBusyBufferSize: "1023k", + wantErr: false, + }, + + { + name: "Autocorrect buffer size and buffers", + args: args{ + proxyBuffers: "8 4k", + proxyBufferSize: "64k", + }, + wantProxyBuffers: "8 4k", + wantProxyBufferSize: "28k", + wantProxyBusyBufferSize: "28k", + wantErr: false, + }, + + { + name: "Buffer size with busy buffer calculates minimum buffers", + args: args{ + proxyBufferSize: "4k", + proxyBusyBuffersSize: "20k", + }, + wantProxyBuffers: "2 4k", + wantProxyBufferSize: "4k", + wantProxyBusyBufferSize: "4k", + wantErr: false, + }, + + { + name: "Single buffer corrected to minimum count", + args: args{ + proxyBuffers: "1 2k", + }, + wantProxyBuffers: "2 2k", + wantProxyBufferSize: "2k", + wantProxyBusyBufferSize: "2k", + wantErr: false, + }, + + { + name: "Single buffer with larger buffer size gets corrected", + args: args{ + proxyBuffers: "1 2k", + proxyBufferSize: "8k", + }, + wantProxyBuffers: "2 2k", + wantProxyBufferSize: "2k", + wantProxyBusyBufferSize: "2k", + wantErr: false, + }, + + { + name: "Zero buffers corrected to minimum 2", + args: args{ + proxyBuffers: "0 4k", + }, + wantProxyBuffers: "2 4k", + wantProxyBufferSize: "4k", + wantProxyBusyBufferSize: "4k", + wantErr: false, + }, + + { + name: "Large buffer count unchanged", + args: args{ + proxyBuffers: "16 1k", + }, + wantProxyBuffers: "16 1k", + wantProxyBufferSize: "1k", + wantProxyBusyBufferSize: "1k", + wantErr: false, + }, + + { + name: "Only busy buffer size set", + args: args{ + proxyBusyBuffersSize: "8k", + }, + wantProxyBuffers: "2 4k", + wantProxyBufferSize: "4k", + wantProxyBusyBufferSize: "4k", + wantErr: false, + }, + + { + name: "Very small buffers with large buffer size", + args: args{ + proxyBuffers: "2 1k", + proxyBufferSize: "2k", + }, + wantProxyBuffers: "2 1k", + wantProxyBufferSize: "1k", + wantProxyBusyBufferSize: "1k", + wantErr: false, + }, + + { + name: "Busy buffer exactly at limit", + args: args{ + proxyBuffers: "4 4k", + proxyBusyBuffersSize: "12k", + }, + wantProxyBuffers: "4 4k", + wantProxyBufferSize: "4k", + wantProxyBusyBufferSize: "12k", + wantErr: false, + }, + + { + name: "Busy buffer too small - gets adjusted", + args: args{ + proxyBuffers: "4 8k", + proxyBufferSize: "16k", + proxyBusyBuffersSize: "4k", + }, + wantProxyBuffers: "4 8k", + wantProxyBufferSize: "16k", + wantProxyBusyBufferSize: "16k", + wantErr: false, + }, + // no no no no + { + name: "Both buffers and buffer-size set", + args: args{ + proxyBuffers: "4 16k", + proxyBufferSize: "8k", + }, + wantProxyBuffers: "4 16k", + wantProxyBufferSize: "8k", + wantProxyBusyBufferSize: "16k", + wantErr: false, + }, + + { + name: "proxy_buffers empty, others aren't, fix proxy_buffers, adjust everything too", + args: args{ + proxyBufferSize: "8k", + proxyBusyBuffersSize: "16k", + }, + wantProxyBuffers: "2 4k", + wantProxyBufferSize: "4k", + wantProxyBusyBufferSize: "4k", + wantErr: false, + }, + { + name: "proxy_buffers is too small, but valid", + args: args{ + proxyBuffers: "24 1k", + proxyBufferSize: "32k", + proxyBusyBuffersSize: "64k", + }, + wantProxyBuffers: "24 1k", + wantProxyBufferSize: "23k", + wantProxyBusyBufferSize: "23k", + wantErr: false, + }, + { + name: "trio should pass unchanged", + args: args{ + proxyBuffers: "8 4k", + proxyBufferSize: "8k", + proxyBusyBuffersSize: "16k", + }, + wantProxyBuffers: "8 4k", + wantProxyBufferSize: "8k", + wantProxyBusyBufferSize: "16k", + wantErr: false, + }, + { + name: "proxy_busy_buffers is in MB", + args: args{ + proxyBuffers: "8 4k", + proxyBufferSize: "4k", + proxyBusyBuffersSize: "1m", + }, + wantProxyBuffers: "8 4k", + wantProxyBufferSize: "4k", + wantProxyBusyBufferSize: "28k", + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pb, err := validation.NewNumberSizeConfig(tt.args.proxyBuffers) + if err != nil { + t.Fatalf("Failed to parse proxyBuffers: %v", err) + } + + pbs, err := validation.NewSizeWithUnit(tt.args.proxyBufferSize) + if err != nil { + t.Fatalf("Failed to parse proxyBufferSize: %v", err) + } + + pbbs, err := validation.NewSizeWithUnit(tt.args.proxyBusyBuffersSize) + if err != nil { + t.Fatalf("Failed to parse proxyBusyBuffers: %v", err) + } + + gotProxyBuffers, gotProxyBufferSize, gotProxyBusyBufferSize, m, err := validation.BalanceProxyValues(pb, pbs, pbbs) + + assert.NoError(t, err) + + for _, mm := range m { + t.Logf("Modification: %s", mm) + } + + assert.Equalf(t, tt.wantProxyBuffers, gotProxyBuffers.String(), "proxy buffers, want: %s, got: %s", tt.wantProxyBuffers, gotProxyBuffers.String()) + assert.Equalf(t, tt.wantProxyBufferSize, gotProxyBufferSize.String(), "proxy_buffer_size, want: %s, got: %s", tt.wantProxyBufferSize, gotProxyBufferSize.String()) + assert.Equalf(t, tt.wantProxyBusyBufferSize, gotProxyBusyBufferSize.String(), "proxy_busy_buffers_size, want: %s, got: %s", tt.wantProxyBusyBufferSize, gotProxyBusyBufferSize.String()) + }) + } +} diff --git a/internal/validation/validation.go b/internal/validation/validation.go index e62086e469..f7171cdd48 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -178,3 +178,80 @@ func ValidateURI(uri string, options ...URIValidationOption) error { return nil } + +// NormalizeSize converts size strings to valid format +func NormalizeSize(sizeStr string) string { + bytes := ParseSize(sizeStr) + if bytes <= 0 { + return "" + } + return FormatSize(bytes) +} + +// ParseSize converts size strings to bytes, autocorrecting invalid units to 'm' +func ParseSize(sizeStr string) int64 { + sizeStr = strings.ToLower(strings.TrimSpace(sizeStr)) + if sizeStr == "" { + return 0 + } + + if num, err := strconv.ParseInt(sizeStr, 10, 64); err == nil { + if num <= 0 { + return 0 + } + return num + } + + if len(sizeStr) < 2 { + return 0 + } + + numStr := sizeStr[:len(sizeStr)-1] + unit := sizeStr[len(sizeStr)-1] + num, err := strconv.ParseInt(numStr, 10, 64) + if err != nil || num <= 0 { + return 0 + } + + // Autocorrect invalid units to 'm' + if unit != 'k' && unit != 'm' { + unit = 'm' + } + + switch unit { + case 'k': + return num << 10 + case 'm': + return num << 20 + default: + return num << 20 // Treat as MB + } +} + +// FormatSize converts bytes to human-readable size string +func FormatSize(bytes int64) string { + if bytes == 0 { + return "0" + } + + if bytes >= (1 << 20) { + return fmt.Sprintf("%dm", bytes/(1<<20)) + } + + if bytes >= (1 << 10) { + return fmt.Sprintf("%dk", bytes/(1<<10)) + } + + return fmt.Sprintf("%d", bytes) +} + +// NormalizeBufferSize handles buffer size values has the wrong format eg input "2 1k", returns "1k" +func NormalizeBufferSize(sizeStr string) string { + fields := strings.Fields(strings.TrimSpace(sizeStr)) + if len(fields) == 2 { + if _, err := strconv.Atoi(fields[0]); err == nil { + sizeStr = fields[1] + } + } + return NormalizeSize(sizeStr) +} diff --git a/internal/validation/validation_test.go b/internal/validation/validation_test.go index ac2869651d..e5eae8dcf0 100644 --- a/internal/validation/validation_test.go +++ b/internal/validation/validation_test.go @@ -205,3 +205,64 @@ func TestValidateURI(t *testing.T) { }) } } + +func TestParseSize(t *testing.T) { + t.Parallel() + + testCases := []struct { + input string + expected int64 + }{ + {"", 0}, + {"1024", 1024}, + {"4k", 4096}, + {"2m", 2097152}, + {"1g", 1048576}, // Now returns 1MB fallback instead of 1GB + {"4K", 4096}, // case insensitive + {"invalid", 0}, + {" 8k ", 8192}, // with whitespace + {"4kb", 0}, + {"8x", 8388608}, // Invalid unit returns same value as MB + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.input, func(t *testing.T) { + t.Parallel() + + got := ParseSize(tc.input) + if got != tc.expected { + t.Errorf("ParseSize(%q) = %d, expected %d", tc.input, got, tc.expected) + } + }) + } +} + +func TestFormatSize(t *testing.T) { + t.Parallel() + + testCases := []struct { + input int64 + expected string + }{ + {0, "0"}, + {1024, "1k"}, + {4096, "4k"}, + {2097152, "2m"}, + {1073741824, "1024m"}, // Now formats as 1024m instead of 1g (no g support) + {1536, "1k"}, // rounds down + {500, "500"}, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.expected, func(t *testing.T) { + t.Parallel() + + got := FormatSize(tc.input) + if got != tc.expected { + t.Errorf("FormatSize(%d) = %q, expected %q", tc.input, got, tc.expected) + } + }) + } +} diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index df32e367dc..26773cf696 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -152,6 +152,8 @@ type Upstream struct { ProxyBuffers *UpstreamBuffers `json:"buffers"` // Sets the size of the buffer used for reading the first part of a response received from the upstream server. The default is set in the proxy-buffer-size ConfigMap key. ProxyBufferSize string `json:"buffer-size"` + // Sets the size of the buffers used for reading a response from the upstream server when the proxy_buffering is enabled. The default is set in the proxy-busy-buffers-size ConfigMap key.' + ProxyBusyBuffersSize string `json:"busy-buffers-size"` // Sets the maximum allowed size of the client request body. The default is set in the client-max-body-size ConfigMap key. ClientMaxBodySize string `json:"client-max-body-size"` // The TLS configuration for the Upstream. diff --git a/pkg/apis/configuration/validation/virtualserver.go b/pkg/apis/configuration/validation/virtualserver.go index aa13c5061c..1c102fccd7 100644 --- a/pkg/apis/configuration/validation/virtualserver.go +++ b/pkg/apis/configuration/validation/virtualserver.go @@ -606,6 +606,7 @@ func (vsv *VirtualServerValidator) validateUpstreams(upstreams []v1.Upstream, fi allErrs = append(allErrs, validateTime(u.SlowStart, idxPath.Child("slow-start"))...) allErrs = append(allErrs, validateBuffer(u.ProxyBuffers, idxPath.Child("buffers"))...) allErrs = append(allErrs, validateSize(u.ProxyBufferSize, idxPath.Child("buffer-size"))...) + allErrs = append(allErrs, validateSize(u.ProxyBusyBuffersSize, idxPath.Child("busy-buffers-size"))...) allErrs = append(allErrs, validateQueue(u.Queue, idxPath.Child("queue"))...) allErrs = append(allErrs, validateSessionCookie(u.SessionCookie, idxPath.Child("sessionCookie"))...) allErrs = append(allErrs, validateUpstreamType(u.Type, idxPath.Child("type"))...) diff --git a/tests/data/virtual-server-configmap-keys/configmap-no-validation-keys-invalid-no-proxies.yaml b/tests/data/virtual-server-configmap-keys/configmap-no-validation-keys-invalid-no-proxies.yaml new file mode 100644 index 0000000000..a228ad3a56 --- /dev/null +++ b/tests/data/virtual-server-configmap-keys/configmap-no-validation-keys-invalid-no-proxies.yaml @@ -0,0 +1,17 @@ +kind: ConfigMap +apiVersion: v1 +metadata: + name: nginx-config + namespace: nginx-ingress +data: + proxy-connect-timeout: "something invalid" + proxy-read-timeout: "something invalid" + client-max-body-size: "something invalid" + proxy-max-temp-file-size: "something invalid" + set-real-ip-from: "something invalid" + real-ip-header: "something invalid" + location-snippets: "something invalid" + server-snippets: "something invalid" + fail-timeout: "something invalid" + proxy-send-timeout: "something invalid" + upstream-zone-size: "something invalid" diff --git a/tests/suite/test_virtual_server_configmap_keys.py b/tests/suite/test_virtual_server_configmap_keys.py index 3fd4c6345d..429021cea6 100644 --- a/tests/suite/test_virtual_server_configmap_keys.py +++ b/tests/suite/test_virtual_server_configmap_keys.py @@ -55,6 +55,20 @@ def assert_keys_without_validation(config, expected_values): assert f" {expected_values['upstream-zone-size']};" in config +def assert_keys_without_validation_or_proxies(config, expected_values): + assert f"proxy_connect_timeout {expected_values['proxy-connect-timeout']};" in config + assert f"proxy_read_timeout {expected_values['proxy-read-timeout']};" in config + assert f"client_max_body_size {expected_values['client-max-body-size']};" in config + assert f"proxy_max_temp_file_size {expected_values['proxy-max-temp-file-size']};" in config + assert f"set_real_ip_from {expected_values['set-real-ip-from']};" in config + assert f"real_ip_header {expected_values['real-ip-header']};" in config + assert f"{expected_values['location-snippets']}" in config + assert f"{expected_values['server-snippets']}" in config + assert f"fail_timeout={expected_values['fail-timeout']}" in config + assert f"proxy_send_timeout {expected_values['proxy-send-timeout']};" in config + assert f" {expected_values['upstream-zone-size']};" in config + + def assert_keys_with_validation(config, expected_values): # based on f"{TEST_DATA}/virtual-server-configmap-keys/configmap-validation-keys.yaml" assert "proxy_buffering off;" in config @@ -223,7 +237,7 @@ def test_keys( cm_src, ) expected_values = get_configmap_fields_from_yaml( - f"{TEST_DATA}/virtual-server-configmap-keys/configmap-no-validation-keys-invalid.yaml" + f"{TEST_DATA}/virtual-server-configmap-keys/configmap-no-validation-keys-invalid-no-proxies.yaml" ) wait_before_test(1) step_2_events = get_events(kube_apis.v1, virtual_server_setup.namespace) @@ -235,7 +249,7 @@ def test_keys( ingress_controller_prerequisites.namespace, ) assert_not_applied_events_emitted(virtual_server_setup, step_2_events, step_1_events, ic_pods_amount) - assert_keys_without_validation(step_2_config, expected_values) + assert_keys_without_validation_or_proxies(step_2_config, expected_values) # to cover the OSS case, this will be changed in the future if cli_arguments["ic-type"] == "nginx-ingress":