From 252ee58abaec2b9cffba42650c845c1e5ff18431 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Thu, 24 Jul 2025 13:35:58 +0100 Subject: [PATCH 01/11] add helper function to enable proxy_buffers if proxy_buffer_size is enabled and vise versa --- .../configs/version1/nginx-plus.ingress.tmpl | 7 +-- internal/configs/version1/nginx.ingress.tmpl | 8 +-- internal/configs/version1/template_helper.go | 49 +++++++++++++------ 3 files changed, 39 insertions(+), 25 deletions(-) diff --git a/internal/configs/version1/nginx-plus.ingress.tmpl b/internal/configs/version1/nginx-plus.ingress.tmpl index 94f30de87f..4353cdea86 100644 --- a/internal/configs/version1/nginx-plus.ingress.tmpl +++ b/internal/configs/version1/nginx-plus.ingress.tmpl @@ -295,11 +295,8 @@ 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}}; + {{- if or $location.ProxyBuffers $location.ProxyBufferSize}} + {{makeProxyBuffersAndProxyBufferSize $location.ProxyBuffers $location.ProxyBufferSize}} {{- end}} {{- if $location.ProxyMaxTempFileSize}} proxy_max_temp_file_size {{$location.ProxyMaxTempFileSize}}; diff --git a/internal/configs/version1/nginx.ingress.tmpl b/internal/configs/version1/nginx.ingress.tmpl index 1d7ca8cd87..4c922e550e 100644 --- a/internal/configs/version1/nginx.ingress.tmpl +++ b/internal/configs/version1/nginx.ingress.tmpl @@ -204,12 +204,8 @@ 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}}; + {{- if or $location.ProxyBuffers $location.ProxyBufferSize}} + {{makeProxyBuffersAndProxyBufferSize $location.ProxyBuffers $location.ProxyBufferSize}} {{- end}} {{- if $location.ProxyMaxTempFileSize}} proxy_max_temp_file_size {{$location.ProxyMaxTempFileSize}}; diff --git a/internal/configs/version1/template_helper.go b/internal/configs/version1/template_helper.go index 203f2b4a2e..4901866a25 100644 --- a/internal/configs/version1/template_helper.go +++ b/internal/configs/version1/template_helper.go @@ -202,19 +202,40 @@ func makeResolver(resolverAddresses []string, resolverValid string, resolverIPV6 return builder.String() } +func makeProxyBuffersAndProxyBufferSize(proxyBuffers, proxyBufferSize string) string { + if proxyBufferSize != "" && proxyBuffers == "" { + // Only proxy-buffer-size is set - use it for both directives + return "proxy_buffer_size " + proxyBufferSize + ";\n\t\tproxy_buffers 4 " + proxyBufferSize + ";" + } else if proxyBuffers != "" && proxyBufferSize == "" { + // Only proxy-buffers is set - extract size for proxy_buffer_size + result := "proxy_buffers " + proxyBuffers + ";" + if parts := strings.Fields(proxyBuffers); len(parts) >= 2 { + bufferSize := parts[len(parts)-1] + result += "\n\t\tproxy_buffer_size " + bufferSize + ";" + } + return result + } else if proxyBuffers != "" && proxyBufferSize != "" { + // Both are set - use as specified + return "proxy_buffers " + proxyBuffers + ";\n\t\tproxy_buffer_size " + proxyBufferSize + ";" + } + // If neither is set, return empty string + return "" +} + var helperFunctions = template.FuncMap{ - "split": split, - "trim": trim, - "contains": strings.Contains, - "hasPrefix": strings.HasPrefix, - "hasSuffix": strings.HasSuffix, - "toLower": strings.ToLower, - "toUpper": strings.ToUpper, - "replaceAll": strings.ReplaceAll, - "makeLocationPath": makeLocationPath, - "makeSecretPath": commonhelpers.MakeSecretPath, - "makeOnOffFromBool": commonhelpers.MakeOnOffFromBool, - "generateProxySetHeaders": generateProxySetHeaders, - "boolToPointerBool": commonhelpers.BoolToPointerBool, - "makeResolver": makeResolver, + "split": split, + "trim": trim, + "contains": strings.Contains, + "hasPrefix": strings.HasPrefix, + "hasSuffix": strings.HasSuffix, + "toLower": strings.ToLower, + "toUpper": strings.ToUpper, + "replaceAll": strings.ReplaceAll, + "makeLocationPath": makeLocationPath, + "makeSecretPath": commonhelpers.MakeSecretPath, + "makeOnOffFromBool": commonhelpers.MakeOnOffFromBool, + "generateProxySetHeaders": generateProxySetHeaders, + "boolToPointerBool": commonhelpers.BoolToPointerBool, + "makeResolver": makeResolver, + "makeProxyBuffersAndProxyBufferSize": makeProxyBuffersAndProxyBufferSize, } From 4d5a6d3b32845bd99d2a56fc05e2b650f6b8ca19 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Thu, 24 Jul 2025 16:31:29 +0100 Subject: [PATCH 02/11] add a safe way to enable proxy_buffers and proxy_buffer_size --- internal/configs/version1/template_helper.go | 58 +++++++++ .../configs/version1/template_helper_test.go | 123 ++++++++++++++++++ 2 files changed, 181 insertions(+) diff --git a/internal/configs/version1/template_helper.go b/internal/configs/version1/template_helper.go index 4901866a25..2c64966923 100644 --- a/internal/configs/version1/template_helper.go +++ b/internal/configs/version1/template_helper.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "regexp" + "strconv" "strings" "text/template" @@ -203,6 +204,8 @@ func makeResolver(resolverAddresses []string, resolverValid string, resolverIPV6 } func makeProxyBuffersAndProxyBufferSize(proxyBuffers, proxyBufferSize string) string { + proxyBuffers, proxyBufferSize = correctProxyBuffers(proxyBuffers, proxyBufferSize) + if proxyBufferSize != "" && proxyBuffers == "" { // Only proxy-buffer-size is set - use it for both directives return "proxy_buffer_size " + proxyBufferSize + ";\n\t\tproxy_buffers 4 " + proxyBufferSize + ";" @@ -222,6 +225,61 @@ func makeProxyBuffersAndProxyBufferSize(proxyBuffers, proxyBufferSize string) st return "" } +// correctProxyBuffers detects and corrects problematic proxy buffer configurations. +// If proxy_buffer_size is larger than individual buffer size in proxy_buffers, +// it adjusts the configuration to use proxy_buffer_size for both settings. +func correctProxyBuffers(proxyBuffers, proxyBufferSize string) (string, string) { + if proxyBuffers == "" || proxyBufferSize == "" { + return proxyBuffers, proxyBufferSize + } + + bufferSizeParts := strings.Split(strings.TrimSpace(proxyBufferSize), " ") + buffersParts := strings.Split(strings.TrimSpace(proxyBuffers), " ") + + // Check if proxy_buffer_size > individual buffer size in proxy_buffers + if len(bufferSizeParts) == 1 && len(buffersParts) >= 2 { + individualBufferSize := buffersParts[1] // Extract buffer size from "count size" format + + // Compare sizes in bytes + bufferSizeBytes := parseSize(proxyBufferSize) + individualSizeBytes := parseSize(individualBufferSize) + + // If proxy_buffer_size is larger, use it for both + if bufferSizeBytes > individualSizeBytes { + return "4 " + proxyBufferSize, proxyBufferSize + } + } + + return proxyBuffers, proxyBufferSize +} + +// parseSize converts size strings like "5m", "8k", "1g" to bytes for comparison +func parseSize(sizeStr string) int64 { + sizeStr = strings.ToLower(strings.TrimSpace(sizeStr)) + if sizeStr == "" { + return 0 + } + + // Use regex to extract number and units + re := regexp.MustCompile(`^(\d+)([kmg])$`) + matches := re.FindStringSubmatch(sizeStr) + if matches == nil { + return 0 + } + + num, _ := strconv.Atoi(matches[1]) + switch matches[2] { + case "k": + return int64(num * 1024) + case "m": + return int64(num * 1024 * 1024) + case "g": + return int64(num * 1024 * 1024 * 1024) + default: + return int64(num) + } +} + var helperFunctions = template.FuncMap{ "split": split, "trim": trim, diff --git a/internal/configs/version1/template_helper_test.go b/internal/configs/version1/template_helper_test.go index 69f04a0c69..4412de798a 100644 --- a/internal/configs/version1/template_helper_test.go +++ b/internal/configs/version1/template_helper_test.go @@ -926,3 +926,126 @@ func TestMakeResolver(t *testing.T) { }) } } + +func TestMakeProxyBuffersAndProxyBufferSize(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + proxyBuffers string + proxyBufferSize string + expectedOutput string + }{ + { + name: "Only buffer-size set", + proxyBuffers: "", + proxyBufferSize: "4k", + expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 4 4k;", + }, + { + name: "Only buffers set", + proxyBuffers: "4 16k", + proxyBufferSize: "", + expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;", + }, + { + name: "Both set correctly", + proxyBuffers: "4 16k", + proxyBufferSize: "8k", + expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 8k;", + }, + { + name: "Large buffer-size corrected", + proxyBuffers: "8 1k", + proxyBufferSize: "2m", + expectedOutput: "proxy_buffers 4 2m;\n\t\tproxy_buffer_size 2m;", + }, + { + name: "Correct configuration unchanged", + proxyBuffers: "8 4k", + proxyBufferSize: "4k", + expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 4k;", + }, + { + name: "Both empty", + proxyBuffers: "", + proxyBufferSize: "", + expectedOutput: "", + }, + { + name: "Buffer-size smaller than individual buffer size", + proxyBuffers: "4 1m", + proxyBufferSize: "512k", + expectedOutput: "proxy_buffers 4 1m;\n\t\tproxy_buffer_size 512k;", + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got := makeProxyBuffersAndProxyBufferSize(tc.proxyBuffers, tc.proxyBufferSize) + + if got != tc.expectedOutput { + t.Errorf("Input: buffers=%q, bufferSize=%q\nGot: %q\nExpected: %q", + tc.proxyBuffers, tc.proxyBufferSize, got, tc.expectedOutput) + } + }) + } +} + +func TestCorrectProxyBuffers(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + inputBuffers string + inputBufferSize string + expectedBuffers string + expectedBufferSize string + }{ + { + name: "Large buffer-size corrected", + inputBuffers: "8 1k", + inputBufferSize: "2m", + expectedBuffers: "4 2m", + expectedBufferSize: "2m", + }, + { + name: "Correct configuration unchanged", + inputBuffers: "8 4k", + inputBufferSize: "4k", + expectedBuffers: "8 4k", + expectedBufferSize: "4k", + }, + { + name: "Empty inputs", + inputBuffers: "", + inputBufferSize: "", + expectedBuffers: "", + expectedBufferSize: "", + }, + { + name: "One empty input", + inputBuffers: "4 8k", + inputBufferSize: "", + expectedBuffers: "4 8k", + expectedBufferSize: "", + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + gotBuffers, gotBufferSize := correctProxyBuffers(tc.inputBuffers, tc.inputBufferSize) + + if gotBuffers != tc.expectedBuffers || gotBufferSize != tc.expectedBufferSize { + t.Errorf("Input: buffers=%q, bufferSize=%q\nGot: buffers=%q, bufferSize=%q\nExpected: buffers=%q, bufferSize=%q", + tc.inputBuffers, tc.inputBufferSize, gotBuffers, gotBufferSize, tc.expectedBuffers, tc.expectedBufferSize) + } + }) + } +} From 36ad41a818f8c95dcc88551ca304bafc3c92a6a6 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Fri, 25 Jul 2025 16:00:25 +0100 Subject: [PATCH 03/11] add `nginx.org/proxy-busy-buffers-size` annotation to ingress --- internal/configs/annotations.go | 4 + .../commonhelpers/common_template_helpers.go | 152 ++++++++++ .../common_template_helpers_test.go | 274 ++++++++++++++++++ internal/configs/config_params.go | 1 + internal/configs/ingress.go | 1 + internal/configs/version1/config.go | 1 + .../configs/version1/nginx-plus.ingress.tmpl | 4 +- internal/configs/version1/nginx.ingress.tmpl | 4 +- internal/configs/version1/template_helper.go | 108 ++----- .../configs/version1/template_helper_test.go | 123 -------- 10 files changed, 454 insertions(+), 218 deletions(-) diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index 8718dd151e..866b056310 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -304,6 +304,10 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool cfgParams.ProxyBufferSize = proxyBufferSize } + if proxyBusyBuffersSize, exists := ingEx.Ingress.Annotations["nginx.org/proxy-busy-buffers-size"]; exists { + cfgParams.ProxyBusyBuffersSize = proxyBusyBuffersSize + } + if upstreamZoneSize, exists := ingEx.Ingress.Annotations["nginx.org/upstream-zone-size"]; exists { cfgParams.UpstreamZoneSize = upstreamZoneSize } diff --git a/internal/configs/commonhelpers/common_template_helpers.go b/internal/configs/commonhelpers/common_template_helpers.go index feb0e70e54..76656e7e8e 100644 --- a/internal/configs/commonhelpers/common_template_helpers.go +++ b/internal/configs/commonhelpers/common_template_helpers.go @@ -2,6 +2,8 @@ package commonhelpers import ( + "fmt" + "strconv" "strings" ) @@ -27,3 +29,153 @@ func MakeOnOffFromBool(b *bool) string { func BoolToPointerBool(b bool) *bool { return &b } + +// MakeProxyBuffers generates nginx proxy buffer configuration with safety validations +func MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBufferSize string) string { + var parts []string + + if proxyBufferSize != "" && proxyBuffers == "" { + count := 4 + if proxyBusyBufferSize != "" { + if minBuffers := int((ParseSize(proxyBusyBufferSize) + ParseSize(proxyBufferSize)) / ParseSize(proxyBufferSize)); minBuffers > count { + count = minBuffers + } + } + proxyBuffers = fmt.Sprintf("%d %s", count, proxyBufferSize) + parts = append(parts, fmt.Sprintf("proxy_buffer_size %s", proxyBufferSize), fmt.Sprintf("proxy_buffers %s", proxyBuffers)) + } else if proxyBuffers != "" { + originalProxyBuffers := proxyBuffers + proxyBuffers, proxyBufferSize = applySafetyCorrections(proxyBuffers, proxyBufferSize) + parts = append(parts, fmt.Sprintf("proxy_buffers %s", proxyBuffers)) + if proxyBufferSize != "" { + parts = append(parts, fmt.Sprintf("proxy_buffer_size %s", proxyBufferSize)) + } + + // If proxy_buffers was corrected and no explicit busy buffer size is set, + // we need to set a safe default to prevent nginx conflicts + if proxyBusyBufferSize == "" && originalProxyBuffers != proxyBuffers { + proxyBusyBufferSize = calculateSafeBusyBufferSize(proxyBuffers) + } + } + + // Add busy buffers with validation + // proxy_busy_buffers_size must be equal to or greater than the maximum of the value of proxy_buffer_size and one of the poxy_buffers + if proxyBusyBufferSize != "" { + validatedSize := proxyBusyBufferSize + if len(parts) > 0 && proxyBuffers != "" && proxyBufferSize != "" { + validatedSize = validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBufferSize) + } + parts = append(parts, fmt.Sprintf("proxy_busy_buffers_size %s", validatedSize)) + } + + if len(parts) == 0 { + return "" + } + return strings.Join(parts, ";\n\t\t") + ";" +} + +// calculateSafeBusyBufferSize calculates a safe default busy buffer size when proxy_buffers was corrected +func calculateSafeBusyBufferSize(proxyBuffers string) string { + fields := strings.Fields(proxyBuffers) + if len(fields) >= 2 { + count, _ := strconv.Atoi(fields[0]) + individualSize := ParseSize(fields[1]) + // Set busy buffer size to a safe value: max allowed is (count * size - size) + safeSize := int64(count-1) * individualSize + if safeSize > 0 { + return FormatSize(safeSize) + } + } + return "" +} + +// applySafetyCorrections applies safety corrections to proxy buffer configuration +func applySafetyCorrections(proxyBuffers, proxyBufferSize string) (string, string) { + fields := strings.Fields(strings.TrimSpace(proxyBuffers)) + if len(fields) < 2 { + return proxyBuffers, proxyBufferSize + } + + count, _ := strconv.Atoi(fields[0]) + if count < 2 { + count = 2 + proxyBuffers = fmt.Sprintf("2 %s", fields[1]) + } + if proxyBufferSize == "" { + proxyBufferSize = fields[1] + } else if ParseSize(proxyBufferSize) > ParseSize(fields[1]) { + proxyBuffers = fmt.Sprintf("%d %s", count, proxyBufferSize) + } + + return proxyBuffers, proxyBufferSize +} + +// validateBusyBufferSize ensures proxy_busy_buffers_size meets nginx requirements +func validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBufferSize string) string { + if proxyBusyBufferSize == "" { + return "" + } + + fields := strings.Fields(proxyBuffers) + if len(fields) < 2 { + return proxyBusyBufferSize + } + + count, _ := strconv.Atoi(fields[0]) + busySize, bufferSize, individualSize := ParseSize(proxyBusyBufferSize), ParseSize(proxyBufferSize), ParseSize(fields[1]) + + minSize := max(bufferSize, individualSize) + maxSize := int64(count)*individualSize - individualSize + + if busySize < minSize { + return FormatSize(minSize) + } + if maxSize > 0 && busySize >= maxSize { + return FormatSize(maxSize) + } + return proxyBusyBufferSize +} + +// ParseSize converts size strings to bytes +func ParseSize(sizeStr string) int64 { + sizeStr = strings.ToLower(strings.TrimSpace(sizeStr)) + if sizeStr == "" { + return 0 + } + + // Handle plain numbers + if num, err := strconv.ParseInt(sizeStr, 10, 64); err == nil { + return num + } + + // Parse with units + if len(sizeStr) < 2 { + return 0 + } + + unit := sizeStr[len(sizeStr)-1] + if num, err := strconv.ParseInt(sizeStr[:len(sizeStr)-1], 10, 64); err == nil { + switch unit { + case 'k': + return num << 10 + case 'm': + return num << 20 + case 'g': + return num << 30 + } + } + return 0 +} + +// FormatSize converts bytes to appropriate size string +func FormatSize(bytes int64) string { + for _, unit := range []struct { + size int64 + suffix string + }{{1 << 30, "g"}, {1 << 20, "m"}, {1 << 10, "k"}} { + if bytes >= unit.size { + return fmt.Sprintf("%d%s", bytes/unit.size, unit.suffix) + } + } + return fmt.Sprintf("%d", bytes) +} diff --git a/internal/configs/commonhelpers/common_template_helpers_test.go b/internal/configs/commonhelpers/common_template_helpers_test.go index 3d6c06cea9..9e2a4cd1af 100644 --- a/internal/configs/commonhelpers/common_template_helpers_test.go +++ b/internal/configs/commonhelpers/common_template_helpers_test.go @@ -61,3 +61,277 @@ func newMakeSecretPathTemplate(t *testing.T) *template.Template { } return tmpl } + +func TestMakeProxyBuffers(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + proxyBuffers string + proxyBufferSize string + proxyBusyBufferSize string + expectedOutput string + }{ + { + name: "Only buffer-size set", + proxyBuffers: "", + proxyBufferSize: "4k", + proxyBusyBufferSize: "", + expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 4 4k;", + }, + { + name: "Only buffers set", + proxyBuffers: "4 16k", + proxyBufferSize: "", + proxyBusyBufferSize: "", + expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;", + }, + { + name: "Both set correctly", + proxyBuffers: "4 16k", + proxyBufferSize: "8k", + proxyBusyBufferSize: "", + expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 8k;", + }, + { + name: "Correct configuration unchanged", + proxyBuffers: "8 4k", + proxyBufferSize: "4k", + proxyBusyBufferSize: "", + expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 4k;", + }, + { + name: "All empty", + proxyBuffers: "", + proxyBufferSize: "", + proxyBusyBufferSize: "", + expectedOutput: "", + }, + { + name: "Buffer-size smaller than individual buffer size", + proxyBuffers: "4 1m", + proxyBufferSize: "512k", + proxyBusyBufferSize: "", + expectedOutput: "proxy_buffers 4 1m;\n\t\tproxy_buffer_size 512k;", + }, + { + name: "Only busy buffer size set", + proxyBuffers: "", + proxyBufferSize: "", + proxyBusyBufferSize: "8k", + expectedOutput: "proxy_busy_buffers_size 8k;", + }, + { + name: "All three parameters set", + proxyBuffers: "8 4k", + proxyBufferSize: "4k", + proxyBusyBufferSize: "16k", + expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 16k;", + }, + { + name: "Busy buffer size too large gets corrected", + proxyBuffers: "4 4k", + proxyBufferSize: "4k", + proxyBusyBufferSize: "20k", // larger than (4*4k - 4k = 12k) + expectedOutput: "proxy_buffers 4 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 12k;", + }, + { + name: "Buffer size with busy buffer calculates minimum buffers", + proxyBuffers: "", + proxyBufferSize: "4k", + proxyBusyBufferSize: "20k", // needs (20k + 4k) / 4k = 6 buffers + expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 6 4k;\n\t\tproxy_busy_buffers_size 20k;", + }, + { + name: "Buffer size with busy buffer calculates minimum buffers with existing buffers", + proxyBuffers: "1 2k", + proxyBufferSize: "", + proxyBusyBufferSize: "", + expectedOutput: "proxy_buffers 2 2k;\n\t\tproxy_buffer_size 2k;\n\t\tproxy_busy_buffers_size 2k;", + }, + { + name: "Single buffer corrected to minimum 2 buffers", + proxyBuffers: "1 4k", + proxyBufferSize: "", + proxyBusyBufferSize: "", + expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", + }, + { + name: "Single buffer with explicit buffer size corrected", + proxyBuffers: "1 8k", + proxyBufferSize: "4k", + proxyBusyBufferSize: "", + expectedOutput: "proxy_buffers 2 8k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 8k;", + }, + { + name: "Single buffer with larger buffer size gets corrected", + proxyBuffers: "1 2k", + proxyBufferSize: "8k", + proxyBusyBufferSize: "", + expectedOutput: "proxy_buffers 2 8k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 8k;", + }, + { + name: "Zero buffers corrected to minimum 2", + proxyBuffers: "0 4k", + proxyBufferSize: "", + proxyBusyBufferSize: "", + expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", + }, + { + name: "Single buffer with explicit busy buffer size", + proxyBuffers: "1 4k", + proxyBufferSize: "", + proxyBusyBufferSize: "6k", + expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", + }, + { + name: "Valid configuration with minimum buffers unchanged", + proxyBuffers: "2 4k", + proxyBufferSize: "", + proxyBusyBufferSize: "", + expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;", + }, + { + name: "Large buffer count unchanged", + proxyBuffers: "16 1k", + proxyBufferSize: "", + proxyBusyBufferSize: "", + expectedOutput: "proxy_buffers 16 1k;\n\t\tproxy_buffer_size 1k;", + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got := MakeProxyBuffers(tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBufferSize) + + if got != tc.expectedOutput { + t.Errorf("Input: buffers=%q, bufferSize=%q, busyBufferSize=%q\nGot: %q\nExpected: %q", + tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBufferSize, got, tc.expectedOutput) + } + }) + } +} + +func TestValidateProxyBusyBufferSize(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + proxyBuffers string + proxyBufferSize string + proxyBusyBufferSize string + expected string + }{ + { + name: "Valid busy buffer size unchanged", + proxyBuffers: "8 4k", + proxyBufferSize: "4k", + proxyBusyBufferSize: "16k", + expected: "16k", + }, + { + name: "Busy buffer too small gets corrected", + proxyBuffers: "8 4k", + proxyBufferSize: "8k", + proxyBusyBufferSize: "2k", // less than max(4k, 8k) = 8k + expected: "8k", + }, + { + name: "Busy buffer too large gets corrected", + proxyBuffers: "4 4k", + proxyBufferSize: "4k", + proxyBusyBufferSize: "20k", // larger than (4*4k - 4k = 12k) + expected: "12k", + }, + { + name: "Empty busy buffer size returns empty", + proxyBuffers: "8 4k", + proxyBufferSize: "4k", + proxyBusyBufferSize: "", + expected: "", + }, + { + name: "Invalid proxy buffers format returns original", + proxyBuffers: "invalid", + proxyBufferSize: "4k", + proxyBusyBufferSize: "8k", + expected: "8k", + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got := validateBusyBufferSize(tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBufferSize) + + if got != tc.expected { + t.Errorf("Input: buffers=%q, bufferSize=%q, busyBufferSize=%q\nGot: %q\nExpected: %q", + tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBufferSize, got, tc.expected) + } + }) + } +} + +func TestParseSize(t *testing.T) { + t.Parallel() + + testCases := []struct { + input string + expected int64 + }{ + {"", 0}, + {"1024", 1024}, + {"4k", 4096}, + {"2m", 2097152}, + {"1g", 1073741824}, + {"4K", 4096}, // case insensitive + {"invalid", 0}, + {" 8k ", 8192}, // with whitespace + } + + 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, "1g"}, + {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/internal/configs/config_params.go b/internal/configs/config_params.go index c73310056d..0e31282251 100644 --- a/internal/configs/config_params.go +++ b/internal/configs/config_params.go @@ -71,6 +71,7 @@ type ConfigParams struct { ProxyBuffering bool ProxyBuffers string ProxyBufferSize string + ProxyBusyBuffersSize string ProxyConnectTimeout string ProxyHideHeaders []string ProxyMaxTempFileSize string diff --git a/internal/configs/ingress.go b/internal/configs/ingress.go index 123bf02afa..f5bf9e6087 100644 --- a/internal/configs/ingress.go +++ b/internal/configs/ingress.go @@ -501,6 +501,7 @@ func createLocation(path string, upstream version1.Upstream, cfg *ConfigParams, ProxyBuffering: cfg.ProxyBuffering, ProxyBuffers: cfg.ProxyBuffers, ProxyBufferSize: cfg.ProxyBufferSize, + ProxyBusyBuffersSize: cfg.ProxyBusyBuffersSize, 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 4353cdea86..be9737d366 100644 --- a/internal/configs/version1/nginx-plus.ingress.tmpl +++ b/internal/configs/version1/nginx-plus.ingress.tmpl @@ -295,8 +295,8 @@ 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 or $location.ProxyBuffers $location.ProxyBufferSize}} - {{makeProxyBuffersAndProxyBufferSize $location.ProxyBuffers $location.ProxyBufferSize}} + {{- if or $location.ProxyBuffers $location.ProxyBufferSize $location.ProxyBusyBuffersSize}} + {{makeProxyBuffers $location.ProxyBuffers $location.ProxyBufferSize $location.ProxyBusyBuffersSize}} {{- end}} {{- if $location.ProxyMaxTempFileSize}} proxy_max_temp_file_size {{$location.ProxyMaxTempFileSize}}; diff --git a/internal/configs/version1/nginx.ingress.tmpl b/internal/configs/version1/nginx.ingress.tmpl index 4c922e550e..95c0456038 100644 --- a/internal/configs/version1/nginx.ingress.tmpl +++ b/internal/configs/version1/nginx.ingress.tmpl @@ -204,8 +204,8 @@ 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 or $location.ProxyBuffers $location.ProxyBufferSize}} - {{makeProxyBuffersAndProxyBufferSize $location.ProxyBuffers $location.ProxyBufferSize}} + {{- if or $location.ProxyBuffers $location.ProxyBufferSize $location.ProxyBusyBuffersSize}} + {{makeProxyBuffers $location.ProxyBuffers $location.ProxyBufferSize $location.ProxyBusyBuffersSize}} {{- end}} {{- if $location.ProxyMaxTempFileSize}} proxy_max_temp_file_size {{$location.ProxyMaxTempFileSize}}; diff --git a/internal/configs/version1/template_helper.go b/internal/configs/version1/template_helper.go index 2c64966923..69f41e07b7 100644 --- a/internal/configs/version1/template_helper.go +++ b/internal/configs/version1/template_helper.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "regexp" - "strconv" "strings" "text/template" @@ -203,97 +202,24 @@ func makeResolver(resolverAddresses []string, resolverValid string, resolverIPV6 return builder.String() } -func makeProxyBuffersAndProxyBufferSize(proxyBuffers, proxyBufferSize string) string { - proxyBuffers, proxyBufferSize = correctProxyBuffers(proxyBuffers, proxyBufferSize) - - if proxyBufferSize != "" && proxyBuffers == "" { - // Only proxy-buffer-size is set - use it for both directives - return "proxy_buffer_size " + proxyBufferSize + ";\n\t\tproxy_buffers 4 " + proxyBufferSize + ";" - } else if proxyBuffers != "" && proxyBufferSize == "" { - // Only proxy-buffers is set - extract size for proxy_buffer_size - result := "proxy_buffers " + proxyBuffers + ";" - if parts := strings.Fields(proxyBuffers); len(parts) >= 2 { - bufferSize := parts[len(parts)-1] - result += "\n\t\tproxy_buffer_size " + bufferSize + ";" - } - return result - } else if proxyBuffers != "" && proxyBufferSize != "" { - // Both are set - use as specified - return "proxy_buffers " + proxyBuffers + ";\n\t\tproxy_buffer_size " + proxyBufferSize + ";" - } - // If neither is set, return empty string - return "" -} - -// correctProxyBuffers detects and corrects problematic proxy buffer configurations. -// If proxy_buffer_size is larger than individual buffer size in proxy_buffers, -// it adjusts the configuration to use proxy_buffer_size for both settings. -func correctProxyBuffers(proxyBuffers, proxyBufferSize string) (string, string) { - if proxyBuffers == "" || proxyBufferSize == "" { - return proxyBuffers, proxyBufferSize - } - - bufferSizeParts := strings.Split(strings.TrimSpace(proxyBufferSize), " ") - buffersParts := strings.Split(strings.TrimSpace(proxyBuffers), " ") - - // Check if proxy_buffer_size > individual buffer size in proxy_buffers - if len(bufferSizeParts) == 1 && len(buffersParts) >= 2 { - individualBufferSize := buffersParts[1] // Extract buffer size from "count size" format - - // Compare sizes in bytes - bufferSizeBytes := parseSize(proxyBufferSize) - individualSizeBytes := parseSize(individualBufferSize) - - // If proxy_buffer_size is larger, use it for both - if bufferSizeBytes > individualSizeBytes { - return "4 " + proxyBufferSize, proxyBufferSize - } - } - - return proxyBuffers, proxyBufferSize -} - -// parseSize converts size strings like "5m", "8k", "1g" to bytes for comparison -func parseSize(sizeStr string) int64 { - sizeStr = strings.ToLower(strings.TrimSpace(sizeStr)) - if sizeStr == "" { - return 0 - } - - // Use regex to extract number and units - re := regexp.MustCompile(`^(\d+)([kmg])$`) - matches := re.FindStringSubmatch(sizeStr) - if matches == nil { - return 0 - } - - num, _ := strconv.Atoi(matches[1]) - switch matches[2] { - case "k": - return int64(num * 1024) - case "m": - return int64(num * 1024 * 1024) - case "g": - return int64(num * 1024 * 1024 * 1024) - default: - return int64(num) - } +func makeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBufferSize string) string { + return commonhelpers.MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBufferSize) } var helperFunctions = template.FuncMap{ - "split": split, - "trim": trim, - "contains": strings.Contains, - "hasPrefix": strings.HasPrefix, - "hasSuffix": strings.HasSuffix, - "toLower": strings.ToLower, - "toUpper": strings.ToUpper, - "replaceAll": strings.ReplaceAll, - "makeLocationPath": makeLocationPath, - "makeSecretPath": commonhelpers.MakeSecretPath, - "makeOnOffFromBool": commonhelpers.MakeOnOffFromBool, - "generateProxySetHeaders": generateProxySetHeaders, - "boolToPointerBool": commonhelpers.BoolToPointerBool, - "makeResolver": makeResolver, - "makeProxyBuffersAndProxyBufferSize": makeProxyBuffersAndProxyBufferSize, + "split": split, + "trim": trim, + "contains": strings.Contains, + "hasPrefix": strings.HasPrefix, + "hasSuffix": strings.HasSuffix, + "toLower": strings.ToLower, + "toUpper": strings.ToUpper, + "replaceAll": strings.ReplaceAll, + "makeLocationPath": makeLocationPath, + "makeSecretPath": commonhelpers.MakeSecretPath, + "makeOnOffFromBool": commonhelpers.MakeOnOffFromBool, + "generateProxySetHeaders": generateProxySetHeaders, + "boolToPointerBool": commonhelpers.BoolToPointerBool, + "makeResolver": makeResolver, + "makeProxyBuffers": makeProxyBuffers, } diff --git a/internal/configs/version1/template_helper_test.go b/internal/configs/version1/template_helper_test.go index 4412de798a..69f04a0c69 100644 --- a/internal/configs/version1/template_helper_test.go +++ b/internal/configs/version1/template_helper_test.go @@ -926,126 +926,3 @@ func TestMakeResolver(t *testing.T) { }) } } - -func TestMakeProxyBuffersAndProxyBufferSize(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - proxyBuffers string - proxyBufferSize string - expectedOutput string - }{ - { - name: "Only buffer-size set", - proxyBuffers: "", - proxyBufferSize: "4k", - expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 4 4k;", - }, - { - name: "Only buffers set", - proxyBuffers: "4 16k", - proxyBufferSize: "", - expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;", - }, - { - name: "Both set correctly", - proxyBuffers: "4 16k", - proxyBufferSize: "8k", - expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 8k;", - }, - { - name: "Large buffer-size corrected", - proxyBuffers: "8 1k", - proxyBufferSize: "2m", - expectedOutput: "proxy_buffers 4 2m;\n\t\tproxy_buffer_size 2m;", - }, - { - name: "Correct configuration unchanged", - proxyBuffers: "8 4k", - proxyBufferSize: "4k", - expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 4k;", - }, - { - name: "Both empty", - proxyBuffers: "", - proxyBufferSize: "", - expectedOutput: "", - }, - { - name: "Buffer-size smaller than individual buffer size", - proxyBuffers: "4 1m", - proxyBufferSize: "512k", - expectedOutput: "proxy_buffers 4 1m;\n\t\tproxy_buffer_size 512k;", - }, - } - - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - got := makeProxyBuffersAndProxyBufferSize(tc.proxyBuffers, tc.proxyBufferSize) - - if got != tc.expectedOutput { - t.Errorf("Input: buffers=%q, bufferSize=%q\nGot: %q\nExpected: %q", - tc.proxyBuffers, tc.proxyBufferSize, got, tc.expectedOutput) - } - }) - } -} - -func TestCorrectProxyBuffers(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - inputBuffers string - inputBufferSize string - expectedBuffers string - expectedBufferSize string - }{ - { - name: "Large buffer-size corrected", - inputBuffers: "8 1k", - inputBufferSize: "2m", - expectedBuffers: "4 2m", - expectedBufferSize: "2m", - }, - { - name: "Correct configuration unchanged", - inputBuffers: "8 4k", - inputBufferSize: "4k", - expectedBuffers: "8 4k", - expectedBufferSize: "4k", - }, - { - name: "Empty inputs", - inputBuffers: "", - inputBufferSize: "", - expectedBuffers: "", - expectedBufferSize: "", - }, - { - name: "One empty input", - inputBuffers: "4 8k", - inputBufferSize: "", - expectedBuffers: "4 8k", - expectedBufferSize: "", - }, - } - - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - gotBuffers, gotBufferSize := correctProxyBuffers(tc.inputBuffers, tc.inputBufferSize) - - if gotBuffers != tc.expectedBuffers || gotBufferSize != tc.expectedBufferSize { - t.Errorf("Input: buffers=%q, bufferSize=%q\nGot: buffers=%q, bufferSize=%q\nExpected: buffers=%q, bufferSize=%q", - tc.inputBuffers, tc.inputBufferSize, gotBuffers, gotBufferSize, tc.expectedBuffers, tc.expectedBufferSize) - } - }) - } -} From 0952f77d223e03ba54fc025e9065aefa42dc6537 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Mon, 28 Jul 2025 13:23:51 +0100 Subject: [PATCH 04/11] add `proxy-busy-buffer-size` to configmap, improve validation --- .../commonhelpers/common_template_helpers.go | 81 +++--- .../common_template_helpers_test.go | 175 ++++--------- internal/configs/configmaps.go | 22 +- internal/configs/configmaps_test.go | 236 ++++++++++++++++++ .../version2/nginx-plus.virtualserver.tmpl | 14 +- .../configs/version2/nginx.virtualserver.tmpl | 14 +- internal/configs/version2/template_helper.go | 5 + internal/validation/validation.go | 122 +++++++++ internal/validation/validation_test.go | 208 +++++++++++++++ 9 files changed, 685 insertions(+), 192 deletions(-) diff --git a/internal/configs/commonhelpers/common_template_helpers.go b/internal/configs/commonhelpers/common_template_helpers.go index 76656e7e8e..f10834c378 100644 --- a/internal/configs/commonhelpers/common_template_helpers.go +++ b/internal/configs/commonhelpers/common_template_helpers.go @@ -5,6 +5,8 @@ import ( "fmt" "strconv" "strings" + + "github.com/nginx/kubernetes-ingress/internal/validation" ) // MakeSecretPath will return the path to the secret with the base secrets @@ -34,11 +36,18 @@ func BoolToPointerBool(b bool) *bool { func MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBufferSize string) string { var parts []string + // Validate and normalize size inputs to prevent invalid nginx configs + proxyBufferSize = validation.NormalizeSize(proxyBufferSize) + proxyBusyBufferSize = validation.NormalizeSize(proxyBusyBufferSize) + if proxyBufferSize != "" && proxyBuffers == "" { count := 4 if proxyBusyBufferSize != "" { - if minBuffers := int((ParseSize(proxyBusyBufferSize) + ParseSize(proxyBufferSize)) / ParseSize(proxyBufferSize)); minBuffers > count { - count = minBuffers + bufferSizeBytes := validation.ParseSize(proxyBufferSize) + if bufferSizeBytes > 0 { // Prevent division by zero + if minBuffers := int((validation.ParseSize(proxyBusyBufferSize) + bufferSizeBytes) / bufferSizeBytes); minBuffers > count { + count = minBuffers + } } } proxyBuffers = fmt.Sprintf("%d %s", count, proxyBufferSize) @@ -59,7 +68,6 @@ func MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBufferSize string) } // Add busy buffers with validation - // proxy_busy_buffers_size must be equal to or greater than the maximum of the value of proxy_buffer_size and one of the poxy_buffers if proxyBusyBufferSize != "" { validatedSize := proxyBusyBufferSize if len(parts) > 0 && proxyBuffers != "" && proxyBufferSize != "" { @@ -79,11 +87,11 @@ func calculateSafeBusyBufferSize(proxyBuffers string) string { fields := strings.Fields(proxyBuffers) if len(fields) >= 2 { count, _ := strconv.Atoi(fields[0]) - individualSize := ParseSize(fields[1]) + individualSize := validation.ParseSize(fields[1]) // Set busy buffer size to a safe value: max allowed is (count * size - size) safeSize := int64(count-1) * individualSize if safeSize > 0 { - return FormatSize(safeSize) + return validation.FormatSize(safeSize) } } return "" @@ -103,7 +111,12 @@ func applySafetyCorrections(proxyBuffers, proxyBufferSize string) (string, strin } if proxyBufferSize == "" { proxyBufferSize = fields[1] - } else if ParseSize(proxyBufferSize) > ParseSize(fields[1]) { + } else if validation.ParseSize(proxyBufferSize) > validation.ParseSize(fields[1]) { + // Don't allow individual buffers larger than 1m to prevent nginx issues + bufferSizeBytes := validation.ParseSize(proxyBufferSize) + if bufferSizeBytes > (1 << 20) { // 1MB limit + return proxyBuffers, proxyBufferSize + } proxyBuffers = fmt.Sprintf("%d %s", count, proxyBufferSize) } @@ -111,6 +124,7 @@ func applySafetyCorrections(proxyBuffers, proxyBufferSize string) (string, strin } // validateBusyBufferSize ensures proxy_busy_buffers_size meets nginx requirements +// and gives precedence to proxy_buffer_size when determining the minimum func validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBufferSize string) string { if proxyBusyBufferSize == "" { return "" @@ -122,60 +136,21 @@ func validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBufferSize s } count, _ := strconv.Atoi(fields[0]) - busySize, bufferSize, individualSize := ParseSize(proxyBusyBufferSize), ParseSize(proxyBufferSize), ParseSize(fields[1]) + busySize, bufferSize, individualSize := validation.ParseSize(proxyBusyBufferSize), validation.ParseSize(proxyBufferSize), validation.ParseSize(fields[1]) + // Give precedence to proxy_buffer_size - if it's larger, use it as the minimum minSize := max(bufferSize, individualSize) maxSize := int64(count)*individualSize - individualSize + // If proxy_buffer_size is significantly larger, prefer to align busy buffer with it + if bufferSize > individualSize && busySize < bufferSize && bufferSize <= maxSize { + return validation.FormatSize(bufferSize) + } if busySize < minSize { - return FormatSize(minSize) + return validation.FormatSize(minSize) } if maxSize > 0 && busySize >= maxSize { - return FormatSize(maxSize) + return validation.FormatSize(maxSize) } return proxyBusyBufferSize } - -// ParseSize converts size strings to bytes -func ParseSize(sizeStr string) int64 { - sizeStr = strings.ToLower(strings.TrimSpace(sizeStr)) - if sizeStr == "" { - return 0 - } - - // Handle plain numbers - if num, err := strconv.ParseInt(sizeStr, 10, 64); err == nil { - return num - } - - // Parse with units - if len(sizeStr) < 2 { - return 0 - } - - unit := sizeStr[len(sizeStr)-1] - if num, err := strconv.ParseInt(sizeStr[:len(sizeStr)-1], 10, 64); err == nil { - switch unit { - case 'k': - return num << 10 - case 'm': - return num << 20 - case 'g': - return num << 30 - } - } - return 0 -} - -// FormatSize converts bytes to appropriate size string -func FormatSize(bytes int64) string { - for _, unit := range []struct { - size int64 - suffix string - }{{1 << 30, "g"}, {1 << 20, "m"}, {1 << 10, "k"}} { - if bytes >= unit.size { - return fmt.Sprintf("%d%s", bytes/unit.size, unit.suffix) - } - } - return fmt.Sprintf("%d", bytes) -} diff --git a/internal/configs/commonhelpers/common_template_helpers_test.go b/internal/configs/commonhelpers/common_template_helpers_test.go index 9e2a4cd1af..a132dea6f4 100644 --- a/internal/configs/commonhelpers/common_template_helpers_test.go +++ b/internal/configs/commonhelpers/common_template_helpers_test.go @@ -72,6 +72,13 @@ func TestMakeProxyBuffers(t *testing.T) { proxyBusyBufferSize string expectedOutput string }{ + { + name: "All empty", + proxyBuffers: "", + proxyBufferSize: "", + proxyBusyBufferSize: "", + expectedOutput: "", + }, { name: "Only buffer-size set", proxyBuffers: "", @@ -87,26 +94,12 @@ func TestMakeProxyBuffers(t *testing.T) { expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;", }, { - name: "Both set correctly", + name: "Both buffers and buffer-size set correctly", proxyBuffers: "4 16k", proxyBufferSize: "8k", proxyBusyBufferSize: "", expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 8k;", }, - { - name: "Correct configuration unchanged", - proxyBuffers: "8 4k", - proxyBufferSize: "4k", - proxyBusyBufferSize: "", - expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 4k;", - }, - { - name: "All empty", - proxyBuffers: "", - proxyBufferSize: "", - proxyBusyBufferSize: "", - expectedOutput: "", - }, { name: "Buffer-size smaller than individual buffer size", proxyBuffers: "4 1m", @@ -128,13 +121,6 @@ func TestMakeProxyBuffers(t *testing.T) { proxyBusyBufferSize: "16k", expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 16k;", }, - { - name: "Busy buffer size too large gets corrected", - proxyBuffers: "4 4k", - proxyBufferSize: "4k", - proxyBusyBufferSize: "20k", // larger than (4*4k - 4k = 12k) - expectedOutput: "proxy_buffers 4 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 12k;", - }, { name: "Buffer size with busy buffer calculates minimum buffers", proxyBuffers: "", @@ -143,26 +129,12 @@ func TestMakeProxyBuffers(t *testing.T) { expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 6 4k;\n\t\tproxy_busy_buffers_size 20k;", }, { - name: "Buffer size with busy buffer calculates minimum buffers with existing buffers", + name: "Single buffer corrected to minimum count", proxyBuffers: "1 2k", proxyBufferSize: "", proxyBusyBufferSize: "", expectedOutput: "proxy_buffers 2 2k;\n\t\tproxy_buffer_size 2k;\n\t\tproxy_busy_buffers_size 2k;", }, - { - name: "Single buffer corrected to minimum 2 buffers", - proxyBuffers: "1 4k", - proxyBufferSize: "", - proxyBusyBufferSize: "", - expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", - }, - { - name: "Single buffer with explicit buffer size corrected", - proxyBuffers: "1 8k", - proxyBufferSize: "4k", - proxyBusyBufferSize: "", - expectedOutput: "proxy_buffers 2 8k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 8k;", - }, { name: "Single buffer with larger buffer size gets corrected", proxyBuffers: "1 2k", @@ -177,13 +149,6 @@ func TestMakeProxyBuffers(t *testing.T) { proxyBusyBufferSize: "", expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", }, - { - name: "Single buffer with explicit busy buffer size", - proxyBuffers: "1 4k", - proxyBufferSize: "", - proxyBusyBufferSize: "6k", - expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", - }, { name: "Valid configuration with minimum buffers unchanged", proxyBuffers: "2 4k", @@ -215,7 +180,7 @@ func TestMakeProxyBuffers(t *testing.T) { } } -func TestValidateProxyBusyBufferSize(t *testing.T) { +func TestValidateBusyBufferSize(t *testing.T) { t.Parallel() testCases := []struct { @@ -226,39 +191,58 @@ func TestValidateProxyBusyBufferSize(t *testing.T) { expected string }{ { - name: "Valid busy buffer size unchanged", - proxyBuffers: "8 4k", - proxyBufferSize: "4k", - proxyBusyBufferSize: "16k", - expected: "16k", + proxyBuffers: "", + proxyBufferSize: "", + proxyBusyBufferSize: "", + expected: "", }, { - name: "Busy buffer too small gets corrected", - proxyBuffers: "8 4k", + proxyBuffers: "4 16k", + proxyBufferSize: "", + proxyBusyBufferSize: "", + expected: "4 16k", + }, + { + proxyBuffers: "4 16k", proxyBufferSize: "8k", - proxyBusyBufferSize: "2k", // less than max(4k, 8k) = 8k - expected: "8k", + proxyBusyBufferSize: "", + expected: "4 16k", }, { - name: "Busy buffer too large gets corrected", - proxyBuffers: "4 4k", - proxyBufferSize: "4k", - proxyBusyBufferSize: "20k", // larger than (4*4k - 4k = 12k) - expected: "12k", + proxyBuffers: "4 16k", + proxyBufferSize: "", + proxyBusyBufferSize: "8k", + expected: "4 16k", + }, + { + proxyBuffers: "4 16k", + proxyBufferSize: "8k", + proxyBusyBufferSize: "12k", + expected: "4 16k", }, { - name: "Empty busy buffer size returns empty", proxyBuffers: "8 4k", proxyBufferSize: "4k", + proxyBusyBufferSize: "16k", + expected: "8 4k", + }, + { + proxyBuffers: "1 2k", + proxyBufferSize: "", + proxyBusyBufferSize: "20k", // larger than (4*4k - 4k = 12k) + expected: "4 4k", + }, + { + proxyBuffers: "1 2k", + proxyBufferSize: "8k", proxyBusyBufferSize: "", - expected: "", + expected: "2 8k", }, { - name: "Invalid proxy buffers format returns original", - proxyBuffers: "invalid", - proxyBufferSize: "4k", - proxyBusyBufferSize: "8k", - expected: "8k", + proxyBuffers: "1 2k", + proxyBufferSize: "8k", + proxyBusyBufferSize: "16k", + expected: "2 8k", }, } @@ -276,62 +260,3 @@ func TestValidateProxyBusyBufferSize(t *testing.T) { }) } } - -func TestParseSize(t *testing.T) { - t.Parallel() - - testCases := []struct { - input string - expected int64 - }{ - {"", 0}, - {"1024", 1024}, - {"4k", 4096}, - {"2m", 2097152}, - {"1g", 1073741824}, - {"4K", 4096}, // case insensitive - {"invalid", 0}, - {" 8k ", 8192}, // with whitespace - } - - 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, "1g"}, - {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/internal/configs/configmaps.go b/internal/configs/configmaps.go index 7a1c7cd001..f05ddb0f50 100644 --- a/internal/configs/configmaps.go +++ b/internal/configs/configmaps.go @@ -335,15 +335,29 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has } if proxyBuffers, exists := cfgm.Data["proxy-buffers"]; exists { - cfgParams.ProxyBuffers = proxyBuffers + fields := strings.Fields(strings.TrimSpace(proxyBuffers)) + if len(fields) == 2 { + normalizedSize := validation.NormalizeSize(fields[1]) + if normalizedSize != fields[1] { + correctedBuffers := fields[0] + " " + normalizedSize + nl.Info(l, fmt.Sprintf("Auto-corrected proxy-buffers from '%s' to '%s'", proxyBuffers, correctedBuffers)) + cfgParams.ProxyBuffers = correctedBuffers + } else { + cfgParams.ProxyBuffers = proxyBuffers + } + } else { + cfgParams.ProxyBuffers = proxyBuffers + } } if proxyBufferSize, exists := cfgm.Data["proxy-buffer-size"]; exists { - cfgParams.ProxyBufferSize = proxyBufferSize + normalizedProxyBufferSize := validation.NormalizeSize(proxyBufferSize) + cfgParams.ProxyBufferSize = normalizedProxyBufferSize } - if proxyMaxTempFileSize, exists := cfgm.Data["proxy-max-temp-file-size"]; exists { - cfgParams.ProxyMaxTempFileSize = proxyMaxTempFileSize + if proxyBusyBuffersSize, exists := cfgm.Data["proxy-busy-buffers-size"]; exists { + normalizedProxyBusyBufferSize := validation.NormalizeSize(proxyBusyBuffersSize) + cfgParams.ProxyBusyBuffersSize = normalizedProxyBusyBufferSize } if mainMainSnippets, exists := GetMapKeyAsStringSlice(cfgm.Data, "main-snippets", cfgm, "\n"); exists { diff --git a/internal/configs/configmaps_test.go b/internal/configs/configmaps_test.go index b4afbd6613..3f57b5863a 100644 --- a/internal/configs/configmaps_test.go +++ b/internal/configs/configmaps_test.go @@ -1924,6 +1924,242 @@ func TestOpenTelemetryConfigurationInvalid(t *testing.T) { } } +func TestParseProxyBuffers(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + configMap *v1.ConfigMap + expectedProxyBuffers string + expectedProxyBufferSize string + expectedProxyBusyBuffersSize string + 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: "8 4k", + expectedProxyBufferSize: "8k", + expectedProxyBusyBuffersSize: "16k", + 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: "16 8k", + expectedProxyBufferSize: "", + expectedProxyBusyBuffersSize: "", + description: "should parse proxy-buffers only", + }, + { + name: "only proxy-buffer-size provided", + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "proxy-buffer-size": "16k", + }, + }, + expectedProxyBuffers: "", + expectedProxyBufferSize: "16k", + expectedProxyBusyBuffersSize: "", + 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: "8 4k", + expectedProxyBufferSize: "8k", + expectedProxyBusyBuffersSize: "16k", + 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: "8 4m", + expectedProxyBufferSize: "8m", + expectedProxyBusyBuffersSize: "16m", + description: "should normalize invalid units to 'm'", + }, + { + name: "empty configmap", + configMap: &v1.ConfigMap{ + Data: map[string]string{}, + }, + expectedProxyBuffers: "", + expectedProxyBufferSize: "", + expectedProxyBusyBuffersSize: "", + description: "should handle empty configmap gracefully", + }, + } + + nginxPlus := true + hasAppProtect := false + hasAppProtectDos := false + hasTLSPassthrough := false + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + 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) + } + + // Should not generate warnings for valid cases + fakeRecorder := eventRecorder.(*record.FakeRecorder) + if len(fakeRecorder.Events) > 0 { + t.Errorf("%s: unexpected warnings generated: %d events", test.description, len(fakeRecorder.Events)) + } + }) + } +} + +func TestParseProxyBuffersFails(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + configMap *v1.ConfigMap + expectedProxyBuffers string + expectedProxyBufferSize string + expectedProxyBusyBuffersSize string + description string + }{ + { + name: "invalid proxy-buffers format stored as-is", + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "proxy-buffers": "invalid format here", + }, + }, + expectedProxyBuffers: "invalid format here", + expectedProxyBufferSize: "", + expectedProxyBusyBuffersSize: "", + description: "should store invalid proxy-buffers format as-is", + }, + { + name: "invalid proxy-buffer-size stored as-is", + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "proxy-buffer-size": "invalid-size", + }, + }, + expectedProxyBuffers: "", + expectedProxyBufferSize: "invalid-size", + expectedProxyBusyBuffersSize: "", + description: "should store invalid proxy-buffer-size as-is", + }, + { + name: "invalid proxy-busy-buffers-size stored as-is", + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "proxy-busy-buffers-size": "not-a-size", + }, + }, + expectedProxyBuffers: "", + expectedProxyBufferSize: "", + expectedProxyBusyBuffersSize: "not-a-size", + description: "should store invalid proxy-busy-buffers-size as-is", + }, + { + name: "zero values stored as-is", + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "proxy-buffers": "0 4k", + "proxy-buffer-size": "0k", + "proxy-busy-buffers-size": "0m", + }, + }, + expectedProxyBuffers: "0 4k", + expectedProxyBufferSize: "0k", + expectedProxyBusyBuffersSize: "0m", + description: "should store zero values as-is", + }, + { + name: "mixed valid and invalid values", + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "proxy-buffers": "8 4k", + "proxy-buffer-size": "invalid", + "proxy-busy-buffers-size": "16k", + }, + }, + expectedProxyBuffers: "8 4k", + expectedProxyBufferSize: "invalid", + expectedProxyBusyBuffersSize: "16k", + description: "should store all values as-is regardless of validity", + }, + } + + nginxPlus := true + hasAppProtect := false + hasAppProtectDos := false + hasTLSPassthrough := false + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + 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) + } + + // ConfigMap parser should not generate warnings - validation happens later + fakeRecorder := eventRecorder.(*record.FakeRecorder) + if len(fakeRecorder.Events) > 0 { + t.Errorf("%s: unexpected warnings generated: %d events", test.description, len(fakeRecorder.Events)) + } + }) + } +} + func makeEventLogger() record.EventRecorder { return record.NewFakeRecorder(1024) } diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 91c592cb79..770eb7bbac 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -610,11 +610,15 @@ 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 }} + {{- $proxyBuffersConfig := makeProxyBuffers $l.ProxyBuffers $l.ProxyBufferSize $l.ProxyBusyBuffersSize }} + {{- if $proxyBuffersConfig }} + {{ $proxyBuffersConfig }} + {{- 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..8ac56938ee 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -352,11 +352,15 @@ 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 }} + {{- $proxyBuffersConfig := makeProxyBuffers $l.ProxyBuffers $l.ProxyBufferSize $l.ProxyBusyBuffersSize }} + {{- if $proxyBuffersConfig }} + {{ $proxyBuffersConfig }} + {{- 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/template_helper.go b/internal/configs/version2/template_helper.go index a31003e928..ceef2f4d85 100644 --- a/internal/configs/version2/template_helper.go +++ b/internal/configs/version2/template_helper.go @@ -248,6 +248,10 @@ func boolToInteger(b bool) int { return i } +func makeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBufferSize string) string { + return commonhelpers.MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBufferSize) +} + var helperFunctions = template.FuncMap{ "headerListToCIMap": headerListToCIMap, "hasCIKey": hasCIKey, @@ -264,4 +268,5 @@ var helperFunctions = template.FuncMap{ "makeTransportListener": makeTransportListener, "makeServerName": makeServerName, "boolToInteger": boolToInteger, + "makeProxyBuffers": makeProxyBuffers, } diff --git a/internal/validation/validation.go b/internal/validation/validation.go index e62086e469..cb8c317276 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -178,3 +178,125 @@ 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 + } + + // Handle plain numbers + 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' + } + + // Convert based on unit + switch unit { + case 'k': + return num << 10 + case 'm': + return num << 20 + default: + return num << 20 // Treat as MB + } +} + +// FormatSize converts bytes to a human-readable size string with appropriate units +func FormatSize(bytes int64) string { + if bytes == 0 { + return "0" + } + + // If it's >= 1MB, format as MB (round down) + if bytes >= (1 << 20) { + return fmt.Sprintf("%dm", bytes/(1<<20)) + } + + // If it's >= 1KB, format as KB (round down) + if bytes >= (1 << 10) { + return fmt.Sprintf("%dk", bytes/(1<<10)) + } + + // Otherwise return as plain bytes + return fmt.Sprintf("%d", bytes) +} + +// ValidateNginxSize validates and normalizes nginx size format, auto-correcting invalid units to 'm' +func ValidateNginxSize(sizeStr string) error { + sizeStr = strings.ToLower(strings.TrimSpace(sizeStr)) + if sizeStr == "" { + return fmt.Errorf("size cannot be empty") + } + + // Allow plain numbers + if _, err := strconv.ParseInt(sizeStr, 10, 64); err == nil { + return nil + } + + if len(sizeStr) < 2 { + return fmt.Errorf("invalid size format: %s", sizeStr) + } + + numStr, unit := sizeStr[:len(sizeStr)-1], sizeStr[len(sizeStr)-1] + + // Validate the numeric part + if _, err := strconv.ParseInt(numStr, 10, 64); err != nil { + return fmt.Errorf("invalid size format: %s", sizeStr) + } + + // Only allow k, m units - reject g and other invalid units + if unit != 'k' && unit != 'm' { + return fmt.Errorf("invalid size unit '%c': only 'k' and 'm' are allowed", unit) + } + + return nil +} + +// ValidateProxyBuffers validates and normalizes "count size" format, auto-correcting invalid units +func ValidateProxyBuffers(proxyBuffers string) error { + if proxyBuffers == "" { + return fmt.Errorf("proxy_buffers cannot be empty") + } + + fields := strings.Fields(strings.TrimSpace(proxyBuffers)) + if len(fields) != 2 { + return fmt.Errorf("proxy_buffers must be in format 'count size', got: %s", proxyBuffers) + } + + // Validate count + if count, err := strconv.Atoi(fields[0]); err != nil || count <= 0 { + return fmt.Errorf("invalid buffer count '%s': must be positive integer", fields[0]) + } + + // Validate size - now allows auto-correction of invalid units + return ValidateNginxSize(fields[1]) +} diff --git a/internal/validation/validation_test.go b/internal/validation/validation_test.go index ac2869651d..520b391237 100644 --- a/internal/validation/validation_test.go +++ b/internal/validation/validation_test.go @@ -205,3 +205,211 @@ 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) + } + }) + } +} + +func TestValidateNginxSize(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + input string + wantErr bool + }{ + { + name: "valid plain number", + input: "1024", + wantErr: false, + }, + { + name: "valid k unit", + input: "4k", + wantErr: false, + }, + { + name: "valid m unit", + input: "2m", + wantErr: false, + }, + { + name: "invalid g unit", + input: "1g", + wantErr: true, + }, + { + name: "invalid multi-letter unit", + input: "4kb", + wantErr: true, + }, + { + name: "empty string", + input: "", + wantErr: true, + }, + { + name: "invalid format", + input: "invalid", + wantErr: true, + }, + { + name: "case insensitive", + input: "4K", + wantErr: false, + }, + { + name: "with whitespace", + input: " 8k ", + wantErr: false, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + err := ValidateNginxSize(tc.input) + if tc.wantErr && err == nil { + t.Errorf("ValidateNginxSize(%q) expected error, got nil", tc.input) + } + if !tc.wantErr && err != nil { + t.Errorf("ValidateNginxSize(%q) expected no error, got %v", tc.input, err) + } + }) + } +} + +func TestValidateProxyBuffers(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + input string + wantErr bool + }{ + { + name: "valid format", + input: "8 4k", + wantErr: false, + }, + { + name: "valid with m unit", + input: "4 2m", + wantErr: false, + }, + { + name: "invalid format - missing size", + input: "8", + wantErr: true, + }, + { + name: "invalid format - too many parts", + input: "8 4k extra", + wantErr: true, + }, + { + name: "invalid count - zero", + input: "0 4k", + wantErr: true, + }, + { + name: "invalid count - negative", + input: "-1 4k", + wantErr: true, + }, + { + name: "invalid count - non-numeric", + input: "abc 4k", + wantErr: true, + }, + { + name: "invalid size - g unit", + input: "8 1g", + wantErr: true, + }, + { + name: "empty string", + input: "", + wantErr: true, + }, + { + name: "with whitespace", + input: " 8 4k ", + wantErr: false, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + err := ValidateProxyBuffers(tc.input) + if tc.wantErr && err == nil { + t.Errorf("ValidateProxyBuffers(%q) expected error, got nil", tc.input) + } + if !tc.wantErr && err != nil { + t.Errorf("ValidateProxyBuffers(%q) expected no error, got %v", tc.input, err) + } + }) + } +} From de8c0641ee940ee3820c6a0ede2c9e1c39510603 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Mon, 28 Jul 2025 13:32:25 +0100 Subject: [PATCH 05/11] rename ProxyBusyBufferSize to ProxyBusyBuffersSize to be more accurate --- .../commonhelpers/common_template_helpers.go | 28 +-- .../common_template_helpers_test.go | 230 +++++++++--------- internal/configs/configmaps.go | 4 +- internal/configs/version1/template_helper.go | 4 +- internal/configs/version2/template_helper.go | 4 +- 5 files changed, 135 insertions(+), 135 deletions(-) diff --git a/internal/configs/commonhelpers/common_template_helpers.go b/internal/configs/commonhelpers/common_template_helpers.go index f10834c378..84aef22d4f 100644 --- a/internal/configs/commonhelpers/common_template_helpers.go +++ b/internal/configs/commonhelpers/common_template_helpers.go @@ -33,19 +33,19 @@ func BoolToPointerBool(b bool) *bool { } // MakeProxyBuffers generates nginx proxy buffer configuration with safety validations -func MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBufferSize string) string { +func MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) string { var parts []string // Validate and normalize size inputs to prevent invalid nginx configs proxyBufferSize = validation.NormalizeSize(proxyBufferSize) - proxyBusyBufferSize = validation.NormalizeSize(proxyBusyBufferSize) + proxyBusyBuffersSize = validation.NormalizeSize(proxyBusyBuffersSize) if proxyBufferSize != "" && proxyBuffers == "" { count := 4 - if proxyBusyBufferSize != "" { + if proxyBusyBuffersSize != "" { bufferSizeBytes := validation.ParseSize(proxyBufferSize) if bufferSizeBytes > 0 { // Prevent division by zero - if minBuffers := int((validation.ParseSize(proxyBusyBufferSize) + bufferSizeBytes) / bufferSizeBytes); minBuffers > count { + if minBuffers := int((validation.ParseSize(proxyBusyBuffersSize) + bufferSizeBytes) / bufferSizeBytes); minBuffers > count { count = minBuffers } } @@ -62,16 +62,16 @@ func MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBufferSize string) // If proxy_buffers was corrected and no explicit busy buffer size is set, // we need to set a safe default to prevent nginx conflicts - if proxyBusyBufferSize == "" && originalProxyBuffers != proxyBuffers { - proxyBusyBufferSize = calculateSafeBusyBufferSize(proxyBuffers) + if proxyBusyBuffersSize == "" && originalProxyBuffers != proxyBuffers { + proxyBusyBuffersSize = calculateSafeBusyBufferSize(proxyBuffers) } } // Add busy buffers with validation - if proxyBusyBufferSize != "" { - validatedSize := proxyBusyBufferSize + if proxyBusyBuffersSize != "" { + validatedSize := proxyBusyBuffersSize if len(parts) > 0 && proxyBuffers != "" && proxyBufferSize != "" { - validatedSize = validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBufferSize) + validatedSize = validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize) } parts = append(parts, fmt.Sprintf("proxy_busy_buffers_size %s", validatedSize)) } @@ -125,18 +125,18 @@ func applySafetyCorrections(proxyBuffers, proxyBufferSize string) (string, strin // validateBusyBufferSize ensures proxy_busy_buffers_size meets nginx requirements // and gives precedence to proxy_buffer_size when determining the minimum -func validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBufferSize string) string { - if proxyBusyBufferSize == "" { +func validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) string { + if proxyBusyBuffersSize == "" { return "" } fields := strings.Fields(proxyBuffers) if len(fields) < 2 { - return proxyBusyBufferSize + return proxyBusyBuffersSize } count, _ := strconv.Atoi(fields[0]) - busySize, bufferSize, individualSize := validation.ParseSize(proxyBusyBufferSize), validation.ParseSize(proxyBufferSize), validation.ParseSize(fields[1]) + busySize, bufferSize, individualSize := validation.ParseSize(proxyBusyBuffersSize), validation.ParseSize(proxyBufferSize), validation.ParseSize(fields[1]) // Give precedence to proxy_buffer_size - if it's larger, use it as the minimum minSize := max(bufferSize, individualSize) @@ -152,5 +152,5 @@ func validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBufferSize s if maxSize > 0 && busySize >= maxSize { return validation.FormatSize(maxSize) } - return proxyBusyBufferSize + return proxyBusyBuffersSize } diff --git a/internal/configs/commonhelpers/common_template_helpers_test.go b/internal/configs/commonhelpers/common_template_helpers_test.go index a132dea6f4..8b0b92477f 100644 --- a/internal/configs/commonhelpers/common_template_helpers_test.go +++ b/internal/configs/commonhelpers/common_template_helpers_test.go @@ -66,102 +66,102 @@ func TestMakeProxyBuffers(t *testing.T) { t.Parallel() testCases := []struct { - name string - proxyBuffers string - proxyBufferSize string - proxyBusyBufferSize string - expectedOutput string + name string + proxyBuffers string + proxyBufferSize string + proxyBusyBuffersSize string + expectedOutput string }{ { - name: "All empty", - proxyBuffers: "", - proxyBufferSize: "", - proxyBusyBufferSize: "", - expectedOutput: "", + name: "All empty", + proxyBuffers: "", + proxyBufferSize: "", + proxyBusyBuffersSize: "", + expectedOutput: "", }, { - name: "Only buffer-size set", - proxyBuffers: "", - proxyBufferSize: "4k", - proxyBusyBufferSize: "", - expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 4 4k;", + name: "Only buffer-size set", + proxyBuffers: "", + proxyBufferSize: "4k", + proxyBusyBuffersSize: "", + expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 4 4k;", }, { - name: "Only buffers set", - proxyBuffers: "4 16k", - proxyBufferSize: "", - proxyBusyBufferSize: "", - expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;", + name: "Only buffers set", + proxyBuffers: "4 16k", + proxyBufferSize: "", + proxyBusyBuffersSize: "", + expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;", }, { - name: "Both buffers and buffer-size set correctly", - proxyBuffers: "4 16k", - proxyBufferSize: "8k", - proxyBusyBufferSize: "", - expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 8k;", + name: "Both buffers and buffer-size set correctly", + proxyBuffers: "4 16k", + proxyBufferSize: "8k", + proxyBusyBuffersSize: "", + expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 8k;", }, { - name: "Buffer-size smaller than individual buffer size", - proxyBuffers: "4 1m", - proxyBufferSize: "512k", - proxyBusyBufferSize: "", - expectedOutput: "proxy_buffers 4 1m;\n\t\tproxy_buffer_size 512k;", + name: "Buffer-size smaller than individual buffer size", + proxyBuffers: "4 1m", + proxyBufferSize: "512k", + proxyBusyBuffersSize: "", + expectedOutput: "proxy_buffers 4 1m;\n\t\tproxy_buffer_size 512k;", }, { - name: "Only busy buffer size set", - proxyBuffers: "", - proxyBufferSize: "", - proxyBusyBufferSize: "8k", - expectedOutput: "proxy_busy_buffers_size 8k;", + name: "Only busy buffer size set", + proxyBuffers: "", + proxyBufferSize: "", + proxyBusyBuffersSize: "8k", + expectedOutput: "proxy_busy_buffers_size 8k;", }, { - name: "All three parameters set", - proxyBuffers: "8 4k", - proxyBufferSize: "4k", - proxyBusyBufferSize: "16k", - expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 16k;", + name: "All three parameters set", + proxyBuffers: "8 4k", + proxyBufferSize: "4k", + proxyBusyBuffersSize: "16k", + expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 16k;", }, { - name: "Buffer size with busy buffer calculates minimum buffers", - proxyBuffers: "", - proxyBufferSize: "4k", - proxyBusyBufferSize: "20k", // needs (20k + 4k) / 4k = 6 buffers - expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 6 4k;\n\t\tproxy_busy_buffers_size 20k;", + name: "Buffer size with busy buffer calculates minimum buffers", + proxyBuffers: "", + proxyBufferSize: "4k", + proxyBusyBuffersSize: "20k", // needs (20k + 4k) / 4k = 6 buffers + expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 6 4k;\n\t\tproxy_busy_buffers_size 20k;", }, { - name: "Single buffer corrected to minimum count", - proxyBuffers: "1 2k", - proxyBufferSize: "", - proxyBusyBufferSize: "", - expectedOutput: "proxy_buffers 2 2k;\n\t\tproxy_buffer_size 2k;\n\t\tproxy_busy_buffers_size 2k;", + name: "Single buffer corrected to minimum count", + proxyBuffers: "1 2k", + proxyBufferSize: "", + proxyBusyBuffersSize: "", + expectedOutput: "proxy_buffers 2 2k;\n\t\tproxy_buffer_size 2k;\n\t\tproxy_busy_buffers_size 2k;", }, { - name: "Single buffer with larger buffer size gets corrected", - proxyBuffers: "1 2k", - proxyBufferSize: "8k", - proxyBusyBufferSize: "", - expectedOutput: "proxy_buffers 2 8k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 8k;", + name: "Single buffer with larger buffer size gets corrected", + proxyBuffers: "1 2k", + proxyBufferSize: "8k", + proxyBusyBuffersSize: "", + expectedOutput: "proxy_buffers 2 8k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 8k;", }, { - name: "Zero buffers corrected to minimum 2", - proxyBuffers: "0 4k", - proxyBufferSize: "", - proxyBusyBufferSize: "", - expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", + name: "Zero buffers corrected to minimum 2", + proxyBuffers: "0 4k", + proxyBufferSize: "", + proxyBusyBuffersSize: "", + expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", }, { - name: "Valid configuration with minimum buffers unchanged", - proxyBuffers: "2 4k", - proxyBufferSize: "", - proxyBusyBufferSize: "", - expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;", + name: "Valid configuration with minimum buffers unchanged", + proxyBuffers: "2 4k", + proxyBufferSize: "", + proxyBusyBuffersSize: "", + expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;", }, { - name: "Large buffer count unchanged", - proxyBuffers: "16 1k", - proxyBufferSize: "", - proxyBusyBufferSize: "", - expectedOutput: "proxy_buffers 16 1k;\n\t\tproxy_buffer_size 1k;", + name: "Large buffer count unchanged", + proxyBuffers: "16 1k", + proxyBufferSize: "", + proxyBusyBuffersSize: "", + expectedOutput: "proxy_buffers 16 1k;\n\t\tproxy_buffer_size 1k;", }, } @@ -170,11 +170,11 @@ func TestMakeProxyBuffers(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - got := MakeProxyBuffers(tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBufferSize) + got := MakeProxyBuffers(tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBuffersSize) if got != tc.expectedOutput { t.Errorf("Input: buffers=%q, bufferSize=%q, busyBufferSize=%q\nGot: %q\nExpected: %q", - tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBufferSize, got, tc.expectedOutput) + tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBuffersSize, got, tc.expectedOutput) } }) } @@ -184,65 +184,65 @@ func TestValidateBusyBufferSize(t *testing.T) { t.Parallel() testCases := []struct { - name string - proxyBuffers string - proxyBufferSize string - proxyBusyBufferSize string - expected string + name string + proxyBuffers string + proxyBufferSize string + proxyBusyBuffersSize string + expected string }{ { - proxyBuffers: "", - proxyBufferSize: "", - proxyBusyBufferSize: "", - expected: "", + proxyBuffers: "", + proxyBufferSize: "", + proxyBusyBuffersSize: "", + expected: "", }, { - proxyBuffers: "4 16k", - proxyBufferSize: "", - proxyBusyBufferSize: "", - expected: "4 16k", + proxyBuffers: "4 16k", + proxyBufferSize: "", + proxyBusyBuffersSize: "", + expected: "4 16k", }, { - proxyBuffers: "4 16k", - proxyBufferSize: "8k", - proxyBusyBufferSize: "", - expected: "4 16k", + proxyBuffers: "4 16k", + proxyBufferSize: "8k", + proxyBusyBuffersSize: "", + expected: "4 16k", }, { - proxyBuffers: "4 16k", - proxyBufferSize: "", - proxyBusyBufferSize: "8k", - expected: "4 16k", + proxyBuffers: "4 16k", + proxyBufferSize: "", + proxyBusyBuffersSize: "8k", + expected: "4 16k", }, { - proxyBuffers: "4 16k", - proxyBufferSize: "8k", - proxyBusyBufferSize: "12k", - expected: "4 16k", + proxyBuffers: "4 16k", + proxyBufferSize: "8k", + proxyBusyBuffersSize: "12k", + expected: "4 16k", }, { - proxyBuffers: "8 4k", - proxyBufferSize: "4k", - proxyBusyBufferSize: "16k", - expected: "8 4k", + proxyBuffers: "8 4k", + proxyBufferSize: "4k", + proxyBusyBuffersSize: "16k", + expected: "8 4k", }, { - proxyBuffers: "1 2k", - proxyBufferSize: "", - proxyBusyBufferSize: "20k", // larger than (4*4k - 4k = 12k) - expected: "4 4k", + proxyBuffers: "1 2k", + proxyBufferSize: "", + proxyBusyBuffersSize: "20k", // larger than (4*4k - 4k = 12k) + expected: "4 4k", }, { - proxyBuffers: "1 2k", - proxyBufferSize: "8k", - proxyBusyBufferSize: "", - expected: "2 8k", + proxyBuffers: "1 2k", + proxyBufferSize: "8k", + proxyBusyBuffersSize: "", + expected: "2 8k", }, { - proxyBuffers: "1 2k", - proxyBufferSize: "8k", - proxyBusyBufferSize: "16k", - expected: "2 8k", + proxyBuffers: "1 2k", + proxyBufferSize: "8k", + proxyBusyBuffersSize: "16k", + expected: "2 8k", }, } @@ -251,11 +251,11 @@ func TestValidateBusyBufferSize(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - got := validateBusyBufferSize(tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBufferSize) + got := validateBusyBufferSize(tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBuffersSize) if got != tc.expected { t.Errorf("Input: buffers=%q, bufferSize=%q, busyBufferSize=%q\nGot: %q\nExpected: %q", - tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBufferSize, got, tc.expected) + tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBuffersSize, got, tc.expected) } }) } diff --git a/internal/configs/configmaps.go b/internal/configs/configmaps.go index f05ddb0f50..946ad6204c 100644 --- a/internal/configs/configmaps.go +++ b/internal/configs/configmaps.go @@ -356,8 +356,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has } if proxyBusyBuffersSize, exists := cfgm.Data["proxy-busy-buffers-size"]; exists { - normalizedProxyBusyBufferSize := validation.NormalizeSize(proxyBusyBuffersSize) - cfgParams.ProxyBusyBuffersSize = normalizedProxyBusyBufferSize + normalizedProxyBusyBuffersSize := validation.NormalizeSize(proxyBusyBuffersSize) + cfgParams.ProxyBusyBuffersSize = normalizedProxyBusyBuffersSize } if mainMainSnippets, exists := GetMapKeyAsStringSlice(cfgm.Data, "main-snippets", cfgm, "\n"); exists { diff --git a/internal/configs/version1/template_helper.go b/internal/configs/version1/template_helper.go index 69f41e07b7..43959aaac9 100644 --- a/internal/configs/version1/template_helper.go +++ b/internal/configs/version1/template_helper.go @@ -202,8 +202,8 @@ func makeResolver(resolverAddresses []string, resolverValid string, resolverIPV6 return builder.String() } -func makeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBufferSize string) string { - return commonhelpers.MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBufferSize) +func makeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) string { + return commonhelpers.MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize) } var helperFunctions = template.FuncMap{ diff --git a/internal/configs/version2/template_helper.go b/internal/configs/version2/template_helper.go index ceef2f4d85..393de1e7af 100644 --- a/internal/configs/version2/template_helper.go +++ b/internal/configs/version2/template_helper.go @@ -248,8 +248,8 @@ func boolToInteger(b bool) int { return i } -func makeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBufferSize string) string { - return commonhelpers.MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBufferSize) +func makeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) string { + return commonhelpers.MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize) } var helperFunctions = template.FuncMap{ From ca2e16b9fce020c7b797a86591b52138e801c111 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Mon, 28 Jul 2025 16:35:19 +0100 Subject: [PATCH 06/11] Add ProxyBusyBuffersSize to VirtualServer as `busy-buffers-size` --- .../k8s.nginx.org_virtualserverroutes.yaml | 6 + .../bases/k8s.nginx.org_virtualservers.yaml | 6 + deploy/crds.yaml | 12 ++ docs/crd/k8s.nginx.org_virtualserverroutes.md | 1 + docs/crd/k8s.nginx.org_virtualservers.md | 1 + .../common_template_helpers_test.go | 43 ++++--- internal/configs/configmaps_test.go | 113 ------------------ .../__snapshots__/templates_test.snap | 4 + internal/configs/version2/http.go | 1 + internal/configs/version2/templates_test.go | 3 + internal/configs/virtualserver.go | 1 + internal/configs/virtualserver_test.go | 4 + pkg/apis/configuration/v1/types.go | 2 + .../configuration/validation/virtualserver.go | 1 + 14 files changed, 68 insertions(+), 130 deletions(-) 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/commonhelpers/common_template_helpers_test.go b/internal/configs/commonhelpers/common_template_helpers_test.go index 8b0b92477f..5d2932b4ee 100644 --- a/internal/configs/commonhelpers/common_template_helpers_test.go +++ b/internal/configs/commonhelpers/common_template_helpers_test.go @@ -191,58 +191,67 @@ func TestValidateBusyBufferSize(t *testing.T) { expected string }{ { + name: "All empty", proxyBuffers: "", proxyBufferSize: "", proxyBusyBuffersSize: "", expected: "", }, { + name: "No busy buffer size set", proxyBuffers: "4 16k", proxyBufferSize: "", proxyBusyBuffersSize: "", - expected: "4 16k", + expected: "", }, { + name: "No busy buffer size with buffer size set", proxyBuffers: "4 16k", proxyBufferSize: "8k", proxyBusyBuffersSize: "", - expected: "4 16k", + expected: "", }, { + name: "Valid busy buffer size within limits", proxyBuffers: "4 16k", proxyBufferSize: "", - proxyBusyBuffersSize: "8k", - expected: "4 16k", + proxyBusyBuffersSize: "32k", // within (4*16k - 16k = 48k) + expected: "32k", }, { + name: "Valid busy buffer size with buffer size", proxyBuffers: "4 16k", proxyBufferSize: "8k", - proxyBusyBuffersSize: "12k", - expected: "4 16k", + proxyBusyBuffersSize: "32k", + expected: "32k", }, { + name: "Valid configuration", proxyBuffers: "8 4k", proxyBufferSize: "4k", proxyBusyBuffersSize: "16k", - expected: "8 4k", + expected: "16k", }, { - proxyBuffers: "1 2k", + name: "Busy buffer too large, gets clamped to max", + proxyBuffers: "4 4k", proxyBufferSize: "", proxyBusyBuffersSize: "20k", // larger than (4*4k - 4k = 12k) - expected: "4 4k", + expected: "12k", }, { - proxyBuffers: "1 2k", - proxyBufferSize: "8k", - proxyBusyBuffersSize: "", - expected: "2 8k", + name: "Busy buffer too small, gets adjusted to minimum", + proxyBuffers: "4 8k", + proxyBufferSize: "16k", // larger than individual buffer + proxyBusyBuffersSize: "4k", // smaller than 16k minimum + expected: "16k", }, { - proxyBuffers: "1 2k", - proxyBufferSize: "8k", - proxyBusyBuffersSize: "16k", - expected: "2 8k", + name: "Buffer size larger than individual, busy buffer gets aligned", + proxyBuffers: "4 8k", + proxyBufferSize: "16k", + proxyBusyBuffersSize: "12k", // less than buffer size but within range + expected: "16k", }, } diff --git a/internal/configs/configmaps_test.go b/internal/configs/configmaps_test.go index 3f57b5863a..22559b1823 100644 --- a/internal/configs/configmaps_test.go +++ b/internal/configs/configmaps_test.go @@ -2039,119 +2039,6 @@ func TestParseProxyBuffers(t *testing.T) { t.Errorf("%s: ProxyBusyBuffersSize = %q, want %q", test.description, result.ProxyBusyBuffersSize, test.expectedProxyBusyBuffersSize) } - // Should not generate warnings for valid cases - fakeRecorder := eventRecorder.(*record.FakeRecorder) - if len(fakeRecorder.Events) > 0 { - t.Errorf("%s: unexpected warnings generated: %d events", test.description, len(fakeRecorder.Events)) - } - }) - } -} - -func TestParseProxyBuffersFails(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - configMap *v1.ConfigMap - expectedProxyBuffers string - expectedProxyBufferSize string - expectedProxyBusyBuffersSize string - description string - }{ - { - name: "invalid proxy-buffers format stored as-is", - configMap: &v1.ConfigMap{ - Data: map[string]string{ - "proxy-buffers": "invalid format here", - }, - }, - expectedProxyBuffers: "invalid format here", - expectedProxyBufferSize: "", - expectedProxyBusyBuffersSize: "", - description: "should store invalid proxy-buffers format as-is", - }, - { - name: "invalid proxy-buffer-size stored as-is", - configMap: &v1.ConfigMap{ - Data: map[string]string{ - "proxy-buffer-size": "invalid-size", - }, - }, - expectedProxyBuffers: "", - expectedProxyBufferSize: "invalid-size", - expectedProxyBusyBuffersSize: "", - description: "should store invalid proxy-buffer-size as-is", - }, - { - name: "invalid proxy-busy-buffers-size stored as-is", - configMap: &v1.ConfigMap{ - Data: map[string]string{ - "proxy-busy-buffers-size": "not-a-size", - }, - }, - expectedProxyBuffers: "", - expectedProxyBufferSize: "", - expectedProxyBusyBuffersSize: "not-a-size", - description: "should store invalid proxy-busy-buffers-size as-is", - }, - { - name: "zero values stored as-is", - configMap: &v1.ConfigMap{ - Data: map[string]string{ - "proxy-buffers": "0 4k", - "proxy-buffer-size": "0k", - "proxy-busy-buffers-size": "0m", - }, - }, - expectedProxyBuffers: "0 4k", - expectedProxyBufferSize: "0k", - expectedProxyBusyBuffersSize: "0m", - description: "should store zero values as-is", - }, - { - name: "mixed valid and invalid values", - configMap: &v1.ConfigMap{ - Data: map[string]string{ - "proxy-buffers": "8 4k", - "proxy-buffer-size": "invalid", - "proxy-busy-buffers-size": "16k", - }, - }, - expectedProxyBuffers: "8 4k", - expectedProxyBufferSize: "invalid", - expectedProxyBusyBuffersSize: "16k", - description: "should store all values as-is regardless of validity", - }, - } - - nginxPlus := true - hasAppProtect := false - hasAppProtectDos := false - hasTLSPassthrough := false - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - 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) - } - - // ConfigMap parser should not generate warnings - validation happens later fakeRecorder := eventRecorder.(*record.FakeRecorder) if len(fakeRecorder.Events) > 0 { t.Errorf("%s: unexpected warnings generated: %d events", test.description, len(fakeRecorder.Events)) diff --git a/internal/configs/version2/__snapshots__/templates_test.snap b/internal/configs/version2/__snapshots__/templates_test.snap index 581fe23d00..2326c8aeb1 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; @@ -2810,6 +2812,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; @@ -3212,6 +3215,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/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..454a2642c9 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -2495,6 +2495,7 @@ func generateLocationForProxying(path string, upstreamName string, upstream conf ProxyBuffering: generateBool(upstream.ProxyBuffering, cfgParams.ProxyBuffering), ProxyBuffers: generateBuffers(upstream.ProxyBuffers, cfgParams.ProxyBuffers), ProxyBufferSize: generateString(upstream.ProxyBufferSize, cfgParams.ProxyBufferSize), + ProxyBusyBuffersSize: generateString(upstream.ProxyBusyBuffersSize, cfgParams.ProxyBusyBuffersSize), 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..1ceaf46c01 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -15223,6 +15223,7 @@ func TestGenerateLocationForProxying(t *testing.T) { ProxyBuffering: true, ProxyBuffers: "8 4k", ProxyBufferSize: "4k", + ProxyBusyBuffersSize: "8k", LocationSnippets: []string{"# location snippet"}, } path := "/" @@ -15240,6 +15241,7 @@ func TestGenerateLocationForProxying(t *testing.T) { ProxyBuffering: true, ProxyBuffers: "8 4k", ProxyBufferSize: "4k", + ProxyBusyBuffersSize: "8k", ProxyPass: "http://test-upstream", ProxyNextUpstream: "error timeout", ProxyNextUpstreamTimeout: "0s", @@ -15270,6 +15272,7 @@ func TestGenerateLocationForGrpcProxying(t *testing.T) { ProxyBuffering: true, ProxyBuffers: "8 4k", ProxyBufferSize: "4k", + ProxyBusyBuffersSize: "8k", LocationSnippets: []string{"# location snippet"}, HTTP2: true, } @@ -15288,6 +15291,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/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"))...) From 4162d5dfa9ba1ffeac6094a5334ea495516b63af Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Wed, 30 Jul 2025 12:52:58 +0100 Subject: [PATCH 07/11] make validation better --- internal/configs/annotations.go | 5 +- .../commonhelpers/common_template_helpers.go | 201 ++++++++++++---- .../common_template_helpers_test.go | 214 +++++++++++------- internal/configs/configmaps.go | 9 +- internal/configs/configmaps_test.go | 84 +++++++ internal/validation/validation.go | 63 +----- internal/validation/validation_test.go | 147 ------------ 7 files changed, 382 insertions(+), 341 deletions(-) diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index 866b056310..44f8f313a5 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. @@ -301,11 +302,11 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool } if proxyBufferSize, exists := ingEx.Ingress.Annotations["nginx.org/proxy-buffer-size"]; exists { - cfgParams.ProxyBufferSize = proxyBufferSize + cfgParams.ProxyBufferSize = validation.NormalizeBufferSize(proxyBufferSize) } if proxyBusyBuffersSize, exists := ingEx.Ingress.Annotations["nginx.org/proxy-busy-buffers-size"]; exists { - cfgParams.ProxyBusyBuffersSize = proxyBusyBuffersSize + cfgParams.ProxyBusyBuffersSize = validation.NormalizeBufferSize(proxyBusyBuffersSize) } if upstreamZoneSize, exists := ingEx.Ingress.Annotations["nginx.org/upstream-zone-size"]; exists { diff --git a/internal/configs/commonhelpers/common_template_helpers.go b/internal/configs/commonhelpers/common_template_helpers.go index 84aef22d4f..18cd85ddbc 100644 --- a/internal/configs/commonhelpers/common_template_helpers.go +++ b/internal/configs/commonhelpers/common_template_helpers.go @@ -32,20 +32,71 @@ func BoolToPointerBool(b bool) *bool { return &b } -// MakeProxyBuffers generates nginx proxy buffer configuration with safety validations +// capBufferLimits applies reasonable limits to buffer configurations to prevent impossible nginx configurations +func capBufferLimits(bufferSize string, bufferCount int) (string, int) { + const maxReasonableBufferSize = 1000 * 1024 * 1024 // 1000MB in bytes + const maxReasonableBufferCount = 1024 + const minReasonableBufferCount = 2 + + cappedSize := bufferSize + cappedCount := bufferCount + + // Cap buffer size + if bufferSize != "" { + bufferSizeBytes := validation.ParseSize(bufferSize) + if bufferSizeBytes > maxReasonableBufferSize { + cappedSize = validation.FormatSize(maxReasonableBufferSize) + } + } + + // Cap buffer count + if bufferCount > maxReasonableBufferCount { + cappedCount = maxReasonableBufferCount + } else if bufferCount < minReasonableBufferCount { + cappedCount = minReasonableBufferCount + } + + return cappedSize, cappedCount +} + +// addBusyBufferSizeConfig manages busy buffer size configuration and adds it to the parts slice +func addBusyBufferSizeConfig(parts []string, proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) []string { + // Always ensure we have a valid busy buffer size when we have any buffer configuration + if len(parts) > 0 && proxyBuffers != "" { + if proxyBusyBuffersSize == "" { + proxyBusyBuffersSize = calculateSafeBusyBufferSize(proxyBuffers, proxyBufferSize) + } else { + proxyBusyBuffersSize = validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize) + } + + if proxyBusyBuffersSize != "" { + parts = append(parts, fmt.Sprintf("proxy_busy_buffers_size %s", proxyBusyBuffersSize)) + } + } else if proxyBusyBuffersSize != "" { + // Handle case where only busy buffer size is provided without buffer configuration + parts = append(parts, fmt.Sprintf("proxy_busy_buffers_size %s", proxyBusyBuffersSize)) + } + + return parts +} + +// MakeProxyBuffers generates nginx proxy buffer configuration with validation func MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) string { var parts []string - // Validate and normalize size inputs to prevent invalid nginx configs - proxyBufferSize = validation.NormalizeSize(proxyBufferSize) - proxyBusyBuffersSize = validation.NormalizeSize(proxyBusyBuffersSize) + proxyBufferSize = validation.NormalizeBufferSize(proxyBufferSize) + proxyBusyBuffersSize = validation.NormalizeBufferSize(proxyBusyBuffersSize) + + // Apply capping limits to buffer size + proxyBufferSize, _ = capBufferLimits(proxyBufferSize, 0) if proxyBufferSize != "" && proxyBuffers == "" { count := 4 if proxyBusyBuffersSize != "" { bufferSizeBytes := validation.ParseSize(proxyBufferSize) - if bufferSizeBytes > 0 { // Prevent division by zero - if minBuffers := int((validation.ParseSize(proxyBusyBuffersSize) + bufferSizeBytes) / bufferSizeBytes); minBuffers > count { + if bufferSizeBytes > 0 { + minBuffers := int((validation.ParseSize(proxyBusyBuffersSize) + bufferSizeBytes) / bufferSizeBytes) + if minBuffers > count { count = minBuffers } } @@ -53,28 +104,19 @@ func MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string proxyBuffers = fmt.Sprintf("%d %s", count, proxyBufferSize) parts = append(parts, fmt.Sprintf("proxy_buffer_size %s", proxyBufferSize), fmt.Sprintf("proxy_buffers %s", proxyBuffers)) } else if proxyBuffers != "" { - originalProxyBuffers := proxyBuffers - proxyBuffers, proxyBufferSize = applySafetyCorrections(proxyBuffers, proxyBufferSize) + originalBuffers := proxyBuffers + proxyBuffers, proxyBufferSize = correctBufferConfig(proxyBuffers, proxyBufferSize) parts = append(parts, fmt.Sprintf("proxy_buffers %s", proxyBuffers)) if proxyBufferSize != "" { parts = append(parts, fmt.Sprintf("proxy_buffer_size %s", proxyBufferSize)) } - // If proxy_buffers was corrected and no explicit busy buffer size is set, - // we need to set a safe default to prevent nginx conflicts - if proxyBusyBuffersSize == "" && originalProxyBuffers != proxyBuffers { - proxyBusyBuffersSize = calculateSafeBusyBufferSize(proxyBuffers) + if proxyBusyBuffersSize == "" && originalBuffers != proxyBuffers { + proxyBusyBuffersSize = defaultBusyBufferSize(proxyBuffers) } } - // Add busy buffers with validation - if proxyBusyBuffersSize != "" { - validatedSize := proxyBusyBuffersSize - if len(parts) > 0 && proxyBuffers != "" && proxyBufferSize != "" { - validatedSize = validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize) - } - parts = append(parts, fmt.Sprintf("proxy_busy_buffers_size %s", validatedSize)) - } + parts = addBusyBufferSizeConfig(parts, proxyBuffers, proxyBufferSize, proxyBusyBuffersSize) if len(parts) == 0 { return "" @@ -82,47 +124,92 @@ func MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string return strings.Join(parts, ";\n\t\t") + ";" } -// calculateSafeBusyBufferSize calculates a safe default busy buffer size when proxy_buffers was corrected -func calculateSafeBusyBufferSize(proxyBuffers string) string { +// defaultBusyBufferSize calculates a default busy buffer size when proxy_buffers is set +func defaultBusyBufferSize(proxyBuffers string) string { fields := strings.Fields(proxyBuffers) if len(fields) >= 2 { - count, _ := strconv.Atoi(fields[0]) - individualSize := validation.ParseSize(fields[1]) - // Set busy buffer size to a safe value: max allowed is (count * size - size) - safeSize := int64(count-1) * individualSize - if safeSize > 0 { - return validation.FormatSize(safeSize) - } + // Return the individual buffer size as the default busy buffer size + return fields[1] } return "" } -// applySafetyCorrections applies safety corrections to proxy buffer configuration -func applySafetyCorrections(proxyBuffers, proxyBufferSize string) (string, string) { +// correctBufferConfig applies corrections to proxy buffer configuration +func correctBufferConfig(proxyBuffers, proxyBufferSize string) (string, string) { fields := strings.Fields(strings.TrimSpace(proxyBuffers)) if len(fields) < 2 { return proxyBuffers, proxyBufferSize } count, _ := strconv.Atoi(fields[0]) - if count < 2 { - count = 2 - proxyBuffers = fmt.Sprintf("2 %s", fields[1]) - } + + // Apply capping limits to buffer count and individual buffer size + fields[1], count = capBufferLimits(fields[1], count) + proxyBuffers = fmt.Sprintf("%d %s", count, fields[1]) + if proxyBufferSize == "" { proxyBufferSize = fields[1] } else if validation.ParseSize(proxyBufferSize) > validation.ParseSize(fields[1]) { - // Don't allow individual buffers larger than 1m to prevent nginx issues bufferSizeBytes := validation.ParseSize(proxyBufferSize) - if bufferSizeBytes > (1 << 20) { // 1MB limit - return proxyBuffers, proxyBufferSize + individualSizeBytes := validation.ParseSize(fields[1]) + + maxPossibleBusySize := int64(count)*individualSizeBytes - individualSizeBytes + if bufferSizeBytes >= maxPossibleBusySize { + // Cap to half of max possible to ensure valid configuration + cappedSize := maxPossibleBusySize / 2 + if cappedSize < individualSizeBytes { + cappedSize = individualSizeBytes + } + proxyBufferSize = validation.FormatSize(cappedSize) + } else if bufferSizeBytes <= (1 << 20) { // 1MB limit for changing buffer sizes + proxyBuffers = fmt.Sprintf("%d %s", count, proxyBufferSize) } - proxyBuffers = fmt.Sprintf("%d %s", count, proxyBufferSize) } return proxyBuffers, proxyBufferSize } +// calculateSafeBusyBufferSize calculates a safe busy buffer size based on proxy_buffers and proxy_buffer_size +func calculateSafeBusyBufferSize(proxyBuffers, proxyBufferSize string) string { + fields := strings.Fields(proxyBuffers) + if len(fields) < 2 { + return "" + } + + count, _ := strconv.Atoi(fields[0]) + individualSize := validation.ParseSize(fields[1]) + proxyBufferSize, _ = capBufferLimits(proxyBufferSize, 0) + bufferSize := validation.ParseSize(proxyBufferSize) + + maxSize := int64(count)*individualSize - individualSize + + minSize := individualSize + if bufferSize > minSize { + minSize = bufferSize + } + + // If the configuration is impossible (minSize >= maxSize), we need to fix it + if maxSize <= 0 || minSize >= maxSize { + if maxSize > individualSize { + safeSize := maxSize - 1024 // Leave 1k margin for safety + if safeSize < individualSize { + safeSize = individualSize + } + return validation.FormatSize(safeSize) + } + return validation.FormatSize(individualSize) + } + + // proxy_buffer_size + (2 * individual_buffer_size) + recommendedSize := minSize + 2*individualSize + + if recommendedSize >= maxSize { + return validation.FormatSize(minSize) + } + + return validation.FormatSize(recommendedSize) +} + // validateBusyBufferSize ensures proxy_busy_buffers_size meets nginx requirements // and gives precedence to proxy_buffer_size when determining the minimum func validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) string { @@ -136,21 +223,39 @@ func validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize } count, _ := strconv.Atoi(fields[0]) - busySize, bufferSize, individualSize := validation.ParseSize(proxyBusyBuffersSize), validation.ParseSize(proxyBufferSize), validation.ParseSize(fields[1]) + busySize := validation.ParseSize(proxyBusyBuffersSize) + bufferSize := validation.ParseSize(proxyBufferSize) + individualSize := validation.ParseSize(fields[1]) - // Give precedence to proxy_buffer_size - if it's larger, use it as the minimum - minSize := max(bufferSize, individualSize) maxSize := int64(count)*individualSize - individualSize - // If proxy_buffer_size is significantly larger, prefer to align busy buffer with it - if bufferSize > individualSize && busySize < bufferSize && bufferSize <= maxSize { - return validation.FormatSize(bufferSize) + minSize := individualSize + if bufferSize > minSize { + minSize = bufferSize } - if busySize < minSize { - return validation.FormatSize(minSize) + + // If the configuration is impossible (minSize >= maxSize), we need to fix it + if maxSize <= 0 || minSize >= maxSize { + if maxSize > individualSize { + safeSize := maxSize - 1024 // Leave 1k margin for safety + if safeSize < individualSize { + safeSize = individualSize + } + return validation.FormatSize(safeSize) + } + return validation.FormatSize(individualSize) } - if maxSize > 0 && busySize >= maxSize { + + // If busy buffer is too large, cap it at the exact maximum allowed value + if busySize >= maxSize { return validation.FormatSize(maxSize) } + + if busySize < minSize { + if minSize < maxSize { + return validation.FormatSize(minSize) + } + return validation.FormatSize(minSize) + } return proxyBusyBuffersSize } diff --git a/internal/configs/commonhelpers/common_template_helpers_test.go b/internal/configs/commonhelpers/common_template_helpers_test.go index 5d2932b4ee..a6b78300c9 100644 --- a/internal/configs/commonhelpers/common_template_helpers_test.go +++ b/internal/configs/commonhelpers/common_template_helpers_test.go @@ -65,7 +65,7 @@ func newMakeSecretPathTemplate(t *testing.T) *template.Template { func TestMakeProxyBuffers(t *testing.T) { t.Parallel() - testCases := []struct { + tests := []struct { name string proxyBuffers string proxyBufferSize string @@ -73,99 +73,147 @@ func TestMakeProxyBuffers(t *testing.T) { expectedOutput string }{ { - name: "All empty", - proxyBuffers: "", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expectedOutput: "", + name: "All empty", + expectedOutput: "", }, { - name: "Only buffer-size set", - proxyBuffers: "", - proxyBufferSize: "4k", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 4 4k;", + name: "Case 1: Only buffer-size set (buffer-size only)", + proxyBufferSize: "4k", + expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 4 4k;\n\t\tproxy_busy_buffers_size 4k;", }, { - name: "Only buffers set", - proxyBuffers: "4 16k", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;", + name: "Case 2: Only buffers set (buffers only)", + proxyBuffers: "4 16k", + expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;\n\t\tproxy_busy_buffers_size 16k;", }, { - name: "Both buffers and buffer-size set correctly", - proxyBuffers: "4 16k", - proxyBufferSize: "8k", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 8k;", + name: "Case 3: Both buffers and buffer-size set (both explicit)", + proxyBuffers: "4 16k", + proxyBufferSize: "8k", + expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 16k;", }, { - name: "Buffer-size smaller than individual buffer size", - proxyBuffers: "4 1m", - proxyBufferSize: "512k", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 4 1m;\n\t\tproxy_buffer_size 512k;", + name: "Case 4: Invalid combination that should self-heal", + proxyBuffers: "8 1m", + proxyBufferSize: "5m", + expectedOutput: "proxy_buffers 8 1m;\n\t\tproxy_buffer_size 5m;\n\t\tproxy_busy_buffers_size 5m;", }, { - name: "Only busy buffer size set", - proxyBuffers: "", - proxyBufferSize: "", - proxyBusyBuffersSize: "8k", - expectedOutput: "proxy_busy_buffers_size 8k;", + name: "Case 5: Buffer-size smaller than individual buffer size", + proxyBuffers: "4 1m", + proxyBufferSize: "512k", + expectedOutput: "proxy_buffers 4 1m;\n\t\tproxy_buffer_size 512k;\n\t\tproxy_busy_buffers_size 1m;", + }, + { + name: "Case 6: Minimum buffers configuration", + proxyBuffers: "2 4k", + proxyBufferSize: "4k", + expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", }, { - name: "All three parameters set", + name: "Case 7: All three parameters set (all three valid)", proxyBuffers: "8 4k", proxyBufferSize: "4k", proxyBusyBuffersSize: "16k", expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 16k;", }, + { + name: "Case 8: Busy buffer too large - gets clamped", + proxyBuffers: "4 8k", + proxyBufferSize: "8k", + proxyBusyBuffersSize: "40k", + expectedOutput: "proxy_buffers 4 8k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 24k;", + }, + { + name: "Case 9: Busy buffer wrong format - should be validated", + proxyBuffers: "4 4k", + proxyBusyBuffersSize: "invalid", + expectedOutput: "proxy_buffers 4 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", + }, + { + name: "Case 10: Empty/zero values - corrected to minimum", + proxyBuffers: "0 4k", + expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", + }, + { + name: "Case 11: Invalid units - should be handled gracefully", + proxyBuffers: "4 4invalid", + proxyBufferSize: "8invalid", + expectedOutput: "proxy_buffers 4 4invalid;\n\t\tproxy_buffer_size 4invalid;\n\t\tproxy_busy_buffers_size 0;", + }, + { + name: "Case 12: Extreme values - should handle gracefully", + proxyBuffers: "1000000 1k", + proxyBufferSize: "999m", + expectedOutput: "proxy_buffers 1024 1k;\n\t\tproxy_buffer_size 511k;\n\t\tproxy_busy_buffers_size 511k;", + }, + { + name: "Case 13: Boundary conditions", + proxyBuffers: "2 1k", + proxyBufferSize: "1k", + expectedOutput: "proxy_buffers 2 1k;\n\t\tproxy_buffer_size 1k;\n\t\tproxy_busy_buffers_size 1k;", + }, + { + name: "Case 14: Real-world problematic configuration", + proxyBuffers: "8 4k", + proxyBufferSize: "64k", + expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 14k;\n\t\tproxy_busy_buffers_size 22k;", + }, { name: "Buffer size with busy buffer calculates minimum buffers", - proxyBuffers: "", proxyBufferSize: "4k", - proxyBusyBuffersSize: "20k", // needs (20k + 4k) / 4k = 6 buffers + proxyBusyBuffersSize: "20k", expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 6 4k;\n\t\tproxy_busy_buffers_size 20k;", }, { - name: "Single buffer corrected to minimum count", - proxyBuffers: "1 2k", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 2 2k;\n\t\tproxy_buffer_size 2k;\n\t\tproxy_busy_buffers_size 2k;", + name: "Single buffer corrected to minimum count", + proxyBuffers: "1 2k", + expectedOutput: "proxy_buffers 2 2k;\n\t\tproxy_buffer_size 2k;\n\t\tproxy_busy_buffers_size 2k;", }, { - name: "Single buffer with larger buffer size gets corrected", - proxyBuffers: "1 2k", - proxyBufferSize: "8k", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 2 8k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 8k;", + name: "Single buffer with larger buffer size gets corrected", + proxyBuffers: "1 2k", + proxyBufferSize: "8k", + expectedOutput: "proxy_buffers 2 2k;\n\t\tproxy_buffer_size 2k;\n\t\tproxy_busy_buffers_size 2k;", + }, + { + name: "Zero buffers corrected to minimum 2", + proxyBuffers: "0 4k", + expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", + }, + { + name: "Large buffer count unchanged", + proxyBuffers: "16 1k", + expectedOutput: "proxy_buffers 16 1k;\n\t\tproxy_buffer_size 1k;\n\t\tproxy_busy_buffers_size 3k;", + }, + { + name: "Only busy buffer size set", + proxyBusyBuffersSize: "8k", + expectedOutput: "proxy_busy_buffers_size 8k;", }, + // Additional edge cases based on nginx validation requirements { - name: "Zero buffers corrected to minimum 2", - proxyBuffers: "0 4k", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", + name: "Very small buffers with large buffer size", + proxyBuffers: "2 1k", + proxyBufferSize: "2k", + expectedOutput: "proxy_buffers 2 1k;\n\t\tproxy_buffer_size 1k;\n\t\tproxy_busy_buffers_size 1k;", }, { - name: "Valid configuration with minimum buffers unchanged", - proxyBuffers: "2 4k", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;", + name: "Busy buffer exactly at limit", + proxyBuffers: "4 4k", + proxyBusyBuffersSize: "12k", + expectedOutput: "proxy_buffers 4 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 12k;", }, { - name: "Large buffer count unchanged", - proxyBuffers: "16 1k", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 16 1k;\n\t\tproxy_buffer_size 1k;", + name: "Busy buffer too small - gets adjusted", + proxyBuffers: "4 8k", + proxyBufferSize: "16k", + proxyBusyBuffersSize: "4k", + expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;\n\t\tproxy_busy_buffers_size 16k;", }, } - for _, tc := range testCases { + for _, tc := range tests { tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() @@ -183,7 +231,7 @@ func TestMakeProxyBuffers(t *testing.T) { func TestValidateBusyBufferSize(t *testing.T) { t.Parallel() - testCases := []struct { + tests := []struct { name string proxyBuffers string proxyBufferSize string @@ -191,31 +239,24 @@ func TestValidateBusyBufferSize(t *testing.T) { expected string }{ { - name: "All empty", - proxyBuffers: "", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expected: "", + name: "All empty", + expected: "", }, { - name: "No busy buffer size set", - proxyBuffers: "4 16k", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expected: "", + name: "No busy buffer size set", + proxyBuffers: "4 16k", + expected: "", }, { - name: "No busy buffer size with buffer size set", - proxyBuffers: "4 16k", - proxyBufferSize: "8k", - proxyBusyBuffersSize: "", - expected: "", + name: "No busy buffer size with buffer size set", + proxyBuffers: "4 16k", + proxyBufferSize: "8k", + expected: "", }, { name: "Valid busy buffer size within limits", proxyBuffers: "4 16k", - proxyBufferSize: "", - proxyBusyBuffersSize: "32k", // within (4*16k - 16k = 48k) + proxyBusyBuffersSize: "32k", expected: "32k", }, { @@ -233,29 +274,28 @@ func TestValidateBusyBufferSize(t *testing.T) { expected: "16k", }, { - name: "Busy buffer too large, gets clamped to max", + name: "Busy buffer too large, gets clamped", proxyBuffers: "4 4k", - proxyBufferSize: "", - proxyBusyBuffersSize: "20k", // larger than (4*4k - 4k = 12k) + proxyBusyBuffersSize: "20k", expected: "12k", }, { - name: "Busy buffer too small, gets adjusted to minimum", + name: "Busy buffer too small, gets adjusted", proxyBuffers: "4 8k", - proxyBufferSize: "16k", // larger than individual buffer - proxyBusyBuffersSize: "4k", // smaller than 16k minimum + proxyBufferSize: "16k", + proxyBusyBuffersSize: "4k", expected: "16k", }, { - name: "Buffer size larger than individual, busy buffer gets aligned", + name: "Buffer size larger than individual", proxyBuffers: "4 8k", proxyBufferSize: "16k", - proxyBusyBuffersSize: "12k", // less than buffer size but within range + proxyBusyBuffersSize: "12k", expected: "16k", }, } - for _, tc := range testCases { + for _, tc := range tests { tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() diff --git a/internal/configs/configmaps.go b/internal/configs/configmaps.go index 946ad6204c..1bc5415792 100644 --- a/internal/configs/configmaps.go +++ b/internal/configs/configmaps.go @@ -346,17 +346,20 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has cfgParams.ProxyBuffers = proxyBuffers } } else { - cfgParams.ProxyBuffers = proxyBuffers + errorText := fmt.Sprintf("ConfigMap %s/%s: invalid value for 'proxy-buffers': %q, must be in format 'count size' (e.g. '4 8k'), ignoring", cfgm.GetNamespace(), cfgm.GetName(), proxyBuffers) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText) + configOk = false } } if proxyBufferSize, exists := cfgm.Data["proxy-buffer-size"]; exists { - normalizedProxyBufferSize := validation.NormalizeSize(proxyBufferSize) + normalizedProxyBufferSize := validation.NormalizeBufferSize(proxyBufferSize) cfgParams.ProxyBufferSize = normalizedProxyBufferSize } if proxyBusyBuffersSize, exists := cfgm.Data["proxy-busy-buffers-size"]; exists { - normalizedProxyBusyBuffersSize := validation.NormalizeSize(proxyBusyBuffersSize) + normalizedProxyBusyBuffersSize := validation.NormalizeBufferSize(proxyBusyBuffersSize) cfgParams.ProxyBusyBuffersSize = normalizedProxyBusyBuffersSize } diff --git a/internal/configs/configmaps_test.go b/internal/configs/configmaps_test.go index 22559b1823..220f0755f9 100644 --- a/internal/configs/configmaps_test.go +++ b/internal/configs/configmaps_test.go @@ -9,6 +9,7 @@ import ( "github.com/nginx/kubernetes-ingress/internal/configs/commonhelpers" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" ) @@ -2047,6 +2048,89 @@ func TestParseProxyBuffers(t *testing.T) { } } +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: false, + description: "should reject empty string", + }, + } + + nginxPlus := true + hasAppProtect := false + hasAppProtectDos := false + hasTLSPassthrough := false + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + 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 != test.proxyBuffers { + t.Errorf("%s: expected ProxyBuffers=%q, got %q", test.description, test.proxyBuffers, result.ProxyBuffers) + } + } else { + if result.ProxyBuffers != "" { + 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/validation/validation.go b/internal/validation/validation.go index cb8c317276..f7171cdd48 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -195,7 +195,6 @@ func ParseSize(sizeStr string) int64 { return 0 } - // Handle plain numbers if num, err := strconv.ParseInt(sizeStr, 10, 64); err == nil { if num <= 0 { return 0 @@ -219,7 +218,6 @@ func ParseSize(sizeStr string) int64 { unit = 'm' } - // Convert based on unit switch unit { case 'k': return num << 10 @@ -230,73 +228,30 @@ func ParseSize(sizeStr string) int64 { } } -// FormatSize converts bytes to a human-readable size string with appropriate units +// FormatSize converts bytes to human-readable size string func FormatSize(bytes int64) string { if bytes == 0 { return "0" } - // If it's >= 1MB, format as MB (round down) if bytes >= (1 << 20) { return fmt.Sprintf("%dm", bytes/(1<<20)) } - // If it's >= 1KB, format as KB (round down) if bytes >= (1 << 10) { return fmt.Sprintf("%dk", bytes/(1<<10)) } - // Otherwise return as plain bytes return fmt.Sprintf("%d", bytes) } -// ValidateNginxSize validates and normalizes nginx size format, auto-correcting invalid units to 'm' -func ValidateNginxSize(sizeStr string) error { - sizeStr = strings.ToLower(strings.TrimSpace(sizeStr)) - if sizeStr == "" { - return fmt.Errorf("size cannot be empty") - } - - // Allow plain numbers - if _, err := strconv.ParseInt(sizeStr, 10, 64); err == nil { - return nil - } - - if len(sizeStr) < 2 { - return fmt.Errorf("invalid size format: %s", sizeStr) - } - - numStr, unit := sizeStr[:len(sizeStr)-1], sizeStr[len(sizeStr)-1] - - // Validate the numeric part - if _, err := strconv.ParseInt(numStr, 10, 64); err != nil { - return fmt.Errorf("invalid size format: %s", sizeStr) - } - - // Only allow k, m units - reject g and other invalid units - if unit != 'k' && unit != 'm' { - return fmt.Errorf("invalid size unit '%c': only 'k' and 'm' are allowed", unit) - } - - return nil -} - -// ValidateProxyBuffers validates and normalizes "count size" format, auto-correcting invalid units -func ValidateProxyBuffers(proxyBuffers string) error { - if proxyBuffers == "" { - return fmt.Errorf("proxy_buffers cannot be empty") - } - - fields := strings.Fields(strings.TrimSpace(proxyBuffers)) - if len(fields) != 2 { - return fmt.Errorf("proxy_buffers must be in format 'count size', got: %s", proxyBuffers) - } - - // Validate count - if count, err := strconv.Atoi(fields[0]); err != nil || count <= 0 { - return fmt.Errorf("invalid buffer count '%s': must be positive integer", fields[0]) +// 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] + } } - - // Validate size - now allows auto-correction of invalid units - return ValidateNginxSize(fields[1]) + return NormalizeSize(sizeStr) } diff --git a/internal/validation/validation_test.go b/internal/validation/validation_test.go index 520b391237..e5eae8dcf0 100644 --- a/internal/validation/validation_test.go +++ b/internal/validation/validation_test.go @@ -266,150 +266,3 @@ func TestFormatSize(t *testing.T) { }) } } - -func TestValidateNginxSize(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - input string - wantErr bool - }{ - { - name: "valid plain number", - input: "1024", - wantErr: false, - }, - { - name: "valid k unit", - input: "4k", - wantErr: false, - }, - { - name: "valid m unit", - input: "2m", - wantErr: false, - }, - { - name: "invalid g unit", - input: "1g", - wantErr: true, - }, - { - name: "invalid multi-letter unit", - input: "4kb", - wantErr: true, - }, - { - name: "empty string", - input: "", - wantErr: true, - }, - { - name: "invalid format", - input: "invalid", - wantErr: true, - }, - { - name: "case insensitive", - input: "4K", - wantErr: false, - }, - { - name: "with whitespace", - input: " 8k ", - wantErr: false, - }, - } - - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - err := ValidateNginxSize(tc.input) - if tc.wantErr && err == nil { - t.Errorf("ValidateNginxSize(%q) expected error, got nil", tc.input) - } - if !tc.wantErr && err != nil { - t.Errorf("ValidateNginxSize(%q) expected no error, got %v", tc.input, err) - } - }) - } -} - -func TestValidateProxyBuffers(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - input string - wantErr bool - }{ - { - name: "valid format", - input: "8 4k", - wantErr: false, - }, - { - name: "valid with m unit", - input: "4 2m", - wantErr: false, - }, - { - name: "invalid format - missing size", - input: "8", - wantErr: true, - }, - { - name: "invalid format - too many parts", - input: "8 4k extra", - wantErr: true, - }, - { - name: "invalid count - zero", - input: "0 4k", - wantErr: true, - }, - { - name: "invalid count - negative", - input: "-1 4k", - wantErr: true, - }, - { - name: "invalid count - non-numeric", - input: "abc 4k", - wantErr: true, - }, - { - name: "invalid size - g unit", - input: "8 1g", - wantErr: true, - }, - { - name: "empty string", - input: "", - wantErr: true, - }, - { - name: "with whitespace", - input: " 8 4k ", - wantErr: false, - }, - } - - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - err := ValidateProxyBuffers(tc.input) - if tc.wantErr && err == nil { - t.Errorf("ValidateProxyBuffers(%q) expected error, got nil", tc.input) - } - if !tc.wantErr && err != nil { - t.Errorf("ValidateProxyBuffers(%q) expected no error, got %v", tc.input, err) - } - }) - } -} From 1bf7b14b50e750498547494ee1a4574d27ceee50 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Wed, 30 Jul 2025 12:52:58 +0100 Subject: [PATCH 08/11] make validation better --- internal/configs/annotations.go | 5 +- .../commonhelpers/common_template_helpers.go | 177 +++++++++----- .../common_template_helpers_test.go | 216 +++++++++++------- internal/configs/configmaps.go | 9 +- internal/configs/configmaps_test.go | 84 +++++++ internal/validation/validation.go | 63 +---- internal/validation/validation_test.go | 147 ------------ 7 files changed, 354 insertions(+), 347 deletions(-) diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index 866b056310..44f8f313a5 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. @@ -301,11 +302,11 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool } if proxyBufferSize, exists := ingEx.Ingress.Annotations["nginx.org/proxy-buffer-size"]; exists { - cfgParams.ProxyBufferSize = proxyBufferSize + cfgParams.ProxyBufferSize = validation.NormalizeBufferSize(proxyBufferSize) } if proxyBusyBuffersSize, exists := ingEx.Ingress.Annotations["nginx.org/proxy-busy-buffers-size"]; exists { - cfgParams.ProxyBusyBuffersSize = proxyBusyBuffersSize + cfgParams.ProxyBusyBuffersSize = validation.NormalizeBufferSize(proxyBusyBuffersSize) } if upstreamZoneSize, exists := ingEx.Ingress.Annotations["nginx.org/upstream-zone-size"]; exists { diff --git a/internal/configs/commonhelpers/common_template_helpers.go b/internal/configs/commonhelpers/common_template_helpers.go index 84aef22d4f..b7e66ae943 100644 --- a/internal/configs/commonhelpers/common_template_helpers.go +++ b/internal/configs/commonhelpers/common_template_helpers.go @@ -32,20 +32,22 @@ func BoolToPointerBool(b bool) *bool { return &b } -// MakeProxyBuffers generates nginx proxy buffer configuration with safety validations +// MakeProxyBuffers generates nginx proxy buffer configuration with validation func MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) string { var parts []string - // Validate and normalize size inputs to prevent invalid nginx configs - proxyBufferSize = validation.NormalizeSize(proxyBufferSize) - proxyBusyBuffersSize = validation.NormalizeSize(proxyBusyBuffersSize) + proxyBufferSize = validation.NormalizeBufferSize(proxyBufferSize) + proxyBusyBuffersSize = validation.NormalizeBufferSize(proxyBusyBuffersSize) + + proxyBufferSize, _ = capBufferLimits(proxyBufferSize, 0) if proxyBufferSize != "" && proxyBuffers == "" { count := 4 if proxyBusyBuffersSize != "" { bufferSizeBytes := validation.ParseSize(proxyBufferSize) - if bufferSizeBytes > 0 { // Prevent division by zero - if minBuffers := int((validation.ParseSize(proxyBusyBuffersSize) + bufferSizeBytes) / bufferSizeBytes); minBuffers > count { + if bufferSizeBytes > 0 { + minBuffers := int((validation.ParseSize(proxyBusyBuffersSize) + bufferSizeBytes) / bufferSizeBytes) + if minBuffers > count { count = minBuffers } } @@ -53,28 +55,19 @@ func MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string proxyBuffers = fmt.Sprintf("%d %s", count, proxyBufferSize) parts = append(parts, fmt.Sprintf("proxy_buffer_size %s", proxyBufferSize), fmt.Sprintf("proxy_buffers %s", proxyBuffers)) } else if proxyBuffers != "" { - originalProxyBuffers := proxyBuffers - proxyBuffers, proxyBufferSize = applySafetyCorrections(proxyBuffers, proxyBufferSize) + originalBuffers := proxyBuffers + proxyBuffers, proxyBufferSize = correctBufferConfig(proxyBuffers, proxyBufferSize) parts = append(parts, fmt.Sprintf("proxy_buffers %s", proxyBuffers)) if proxyBufferSize != "" { parts = append(parts, fmt.Sprintf("proxy_buffer_size %s", proxyBufferSize)) } - // If proxy_buffers was corrected and no explicit busy buffer size is set, - // we need to set a safe default to prevent nginx conflicts - if proxyBusyBuffersSize == "" && originalProxyBuffers != proxyBuffers { - proxyBusyBuffersSize = calculateSafeBusyBufferSize(proxyBuffers) + if proxyBusyBuffersSize == "" && originalBuffers != proxyBuffers { + proxyBusyBuffersSize = defaultBusyBufferSize(proxyBuffers) } } - // Add busy buffers with validation - if proxyBusyBuffersSize != "" { - validatedSize := proxyBusyBuffersSize - if len(parts) > 0 && proxyBuffers != "" && proxyBufferSize != "" { - validatedSize = validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize) - } - parts = append(parts, fmt.Sprintf("proxy_busy_buffers_size %s", validatedSize)) - } + parts = addBusyBufferSizeConfig(parts, proxyBuffers, proxyBufferSize, proxyBusyBuffersSize) if len(parts) == 0 { return "" @@ -82,75 +75,153 @@ func MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string return strings.Join(parts, ";\n\t\t") + ";" } -// calculateSafeBusyBufferSize calculates a safe default busy buffer size when proxy_buffers was corrected -func calculateSafeBusyBufferSize(proxyBuffers string) string { +func defaultBusyBufferSize(proxyBuffers string) string { fields := strings.Fields(proxyBuffers) if len(fields) >= 2 { - count, _ := strconv.Atoi(fields[0]) - individualSize := validation.ParseSize(fields[1]) - // Set busy buffer size to a safe value: max allowed is (count * size - size) - safeSize := int64(count-1) * individualSize - if safeSize > 0 { - return validation.FormatSize(safeSize) - } + // Return the individual buffer size as the default busy buffer size + return fields[1] } return "" } -// applySafetyCorrections applies safety corrections to proxy buffer configuration -func applySafetyCorrections(proxyBuffers, proxyBufferSize string) (string, string) { +// correctBufferConfig applies corrections to proxy buffer configuration +func correctBufferConfig(proxyBuffers, proxyBufferSize string) (string, string) { fields := strings.Fields(strings.TrimSpace(proxyBuffers)) if len(fields) < 2 { return proxyBuffers, proxyBufferSize } count, _ := strconv.Atoi(fields[0]) - if count < 2 { - count = 2 - proxyBuffers = fmt.Sprintf("2 %s", fields[1]) - } + + // Capping buffer count and individual buffer size + fields[1], count = capBufferLimits(fields[1], count) + proxyBuffers = fmt.Sprintf("%d %s", count, fields[1]) + if proxyBufferSize == "" { proxyBufferSize = fields[1] } else if validation.ParseSize(proxyBufferSize) > validation.ParseSize(fields[1]) { - // Don't allow individual buffers larger than 1m to prevent nginx issues bufferSizeBytes := validation.ParseSize(proxyBufferSize) - if bufferSizeBytes > (1 << 20) { // 1MB limit - return proxyBuffers, proxyBufferSize + individualSizeBytes := validation.ParseSize(fields[1]) + + maxPossibleBusySize := int64(count)*individualSizeBytes - individualSizeBytes + if bufferSizeBytes >= maxPossibleBusySize { + cappedSize := maxPossibleBusySize / 2 + if cappedSize < individualSizeBytes { + cappedSize = individualSizeBytes + } + proxyBufferSize = validation.FormatSize(cappedSize) + } else if bufferSizeBytes <= (1 << 20) { // 1MB limit for changing buffer sizes + proxyBuffers = fmt.Sprintf("%d %s", count, proxyBufferSize) } - proxyBuffers = fmt.Sprintf("%d %s", count, proxyBufferSize) } return proxyBuffers, proxyBufferSize } -// validateBusyBufferSize ensures proxy_busy_buffers_size meets nginx requirements -// and gives precedence to proxy_buffer_size when determining the minimum -func validateBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) string { - if proxyBusyBuffersSize == "" { - return "" +// capBufferLimits applies limits to buffer configurations to prevent issues +func capBufferLimits(bufferSize string, bufferCount int) (string, int) { + const maxReasonableBufferSize = 1000 * 1024 * 1024 // 1000MB in bytes + const maxReasonableBufferCount = 1024 + const minReasonableBufferCount = 2 + + cappedSize := bufferSize + cappedCount := bufferCount + + // Cap buffer size + if bufferSize != "" { + bufferSizeBytes := validation.ParseSize(bufferSize) + if bufferSizeBytes > maxReasonableBufferSize { + cappedSize = validation.FormatSize(maxReasonableBufferSize) + } + } + + // Cap buffer count + if bufferCount > maxReasonableBufferCount { + cappedCount = maxReasonableBufferCount + } else if bufferCount < minReasonableBufferCount { + cappedCount = minReasonableBufferCount + } + + return cappedSize, cappedCount +} + +// addBusyBufferSizeConfig manages busy buffer size configuration and adds it to the parts slice +func addBusyBufferSizeConfig(parts []string, proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) []string { + // Always ensure we have a valid busy buffer size when we have any buffer configuration + if len(parts) > 0 && proxyBuffers != "" { + if proxyBusyBuffersSize == "" { + proxyBusyBuffersSize = processBusyBufferSize(proxyBuffers, proxyBufferSize, "") + } else { + proxyBusyBuffersSize = processBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize) + } + + if proxyBusyBuffersSize != "" { + parts = append(parts, fmt.Sprintf("proxy_busy_buffers_size %s", proxyBusyBuffersSize)) + } + } else if proxyBusyBuffersSize != "" { + // Handle case where only busy buffer size is provided without buffer configuration + parts = append(parts, fmt.Sprintf("proxy_busy_buffers_size %s", proxyBusyBuffersSize)) } + return parts +} + +// processBusyBufferSize handles both validation of busy buffer size +// If proxyBusyBuffersSize is empty, it calculates a safe value +// If proxyBusyBuffersSize is provided, it validates and corrects it if needed +func processBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) string { fields := strings.Fields(proxyBuffers) if len(fields) < 2 { + if proxyBusyBuffersSize == "" { + return "" + } return proxyBusyBuffersSize } count, _ := strconv.Atoi(fields[0]) - busySize, bufferSize, individualSize := validation.ParseSize(proxyBusyBuffersSize), validation.ParseSize(proxyBufferSize), validation.ParseSize(fields[1]) + individualSize := validation.ParseSize(fields[1]) + + if proxyBusyBuffersSize == "" { + proxyBufferSize, _ = capBufferLimits(proxyBufferSize, 0) + } + bufferSize := validation.ParseSize(proxyBufferSize) - // Give precedence to proxy_buffer_size - if it's larger, use it as the minimum - minSize := max(bufferSize, individualSize) maxSize := int64(count)*individualSize - individualSize + minSize := individualSize + if bufferSize > minSize { + minSize = bufferSize + } - // If proxy_buffer_size is significantly larger, prefer to align busy buffer with it - if bufferSize > individualSize && busySize < bufferSize && bufferSize <= maxSize { - return validation.FormatSize(bufferSize) + if maxSize <= 0 || minSize >= maxSize { + safeSize := individualSize + if maxSize > individualSize { + safeSize = maxSize - 1024 // Leave 1k margin for safety + if safeSize < individualSize { + safeSize = individualSize + } + } + return validation.FormatSize(safeSize) } - if busySize < minSize { - return validation.FormatSize(minSize) + + // If no busy buffer size provided, calculate a safe one + if proxyBusyBuffersSize == "" { + // proxy_buffer_size + (2 * individual_buffer_size) + recommendedSize := minSize + 2*individualSize + if recommendedSize >= maxSize { + return validation.FormatSize(minSize) + } + return validation.FormatSize(recommendedSize) } - if maxSize > 0 && busySize >= maxSize { + + busySize := validation.ParseSize(proxyBusyBuffersSize) + + if busySize >= maxSize { return validation.FormatSize(maxSize) } + + if busySize < minSize { + return validation.FormatSize(minSize) + } + return proxyBusyBuffersSize } diff --git a/internal/configs/commonhelpers/common_template_helpers_test.go b/internal/configs/commonhelpers/common_template_helpers_test.go index 5d2932b4ee..ffe7449282 100644 --- a/internal/configs/commonhelpers/common_template_helpers_test.go +++ b/internal/configs/commonhelpers/common_template_helpers_test.go @@ -65,7 +65,7 @@ func newMakeSecretPathTemplate(t *testing.T) *template.Template { func TestMakeProxyBuffers(t *testing.T) { t.Parallel() - testCases := []struct { + tests := []struct { name string proxyBuffers string proxyBufferSize string @@ -73,99 +73,147 @@ func TestMakeProxyBuffers(t *testing.T) { expectedOutput string }{ { - name: "All empty", - proxyBuffers: "", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expectedOutput: "", + name: "All empty", + expectedOutput: "", }, { - name: "Only buffer-size set", - proxyBuffers: "", - proxyBufferSize: "4k", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 4 4k;", + name: "Case 1: Only buffer-size set (buffer-size only)", + proxyBufferSize: "4k", + expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 4 4k;\n\t\tproxy_busy_buffers_size 4k;", }, { - name: "Only buffers set", - proxyBuffers: "4 16k", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;", + name: "Case 2: Only buffers set (buffers only)", + proxyBuffers: "4 16k", + expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;\n\t\tproxy_busy_buffers_size 16k;", }, { - name: "Both buffers and buffer-size set correctly", - proxyBuffers: "4 16k", - proxyBufferSize: "8k", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 8k;", + name: "Case 3: Both buffers and buffer-size set (both explicit)", + proxyBuffers: "4 16k", + proxyBufferSize: "8k", + expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 16k;", }, { - name: "Buffer-size smaller than individual buffer size", - proxyBuffers: "4 1m", - proxyBufferSize: "512k", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 4 1m;\n\t\tproxy_buffer_size 512k;", + name: "Case 4: Invalid combination that should self-heal", + proxyBuffers: "8 1m", + proxyBufferSize: "5m", + expectedOutput: "proxy_buffers 8 1m;\n\t\tproxy_buffer_size 5m;\n\t\tproxy_busy_buffers_size 5m;", }, { - name: "Only busy buffer size set", - proxyBuffers: "", - proxyBufferSize: "", - proxyBusyBuffersSize: "8k", - expectedOutput: "proxy_busy_buffers_size 8k;", + name: "Case 5: Buffer-size smaller than individual buffer size", + proxyBuffers: "4 1m", + proxyBufferSize: "512k", + expectedOutput: "proxy_buffers 4 1m;\n\t\tproxy_buffer_size 512k;\n\t\tproxy_busy_buffers_size 1m;", + }, + { + name: "Case 6: Minimum buffers configuration", + proxyBuffers: "2 4k", + proxyBufferSize: "4k", + expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", }, { - name: "All three parameters set", + name: "Case 7: All three parameters set (all three valid)", proxyBuffers: "8 4k", proxyBufferSize: "4k", proxyBusyBuffersSize: "16k", expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 16k;", }, + { + name: "Case 8: Busy buffer too large - gets clamped", + proxyBuffers: "4 8k", + proxyBufferSize: "8k", + proxyBusyBuffersSize: "40k", + expectedOutput: "proxy_buffers 4 8k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 24k;", + }, + { + name: "Case 9: Busy buffer wrong format - should be validated", + proxyBuffers: "4 4k", + proxyBusyBuffersSize: "invalid", + expectedOutput: "proxy_buffers 4 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", + }, + { + name: "Case 10: Empty/zero values - corrected to minimum", + proxyBuffers: "0 4k", + expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", + }, + { + name: "Case 11: Invalid units - should be handled gracefully", + proxyBuffers: "4 4invalid", + proxyBufferSize: "8invalid", + expectedOutput: "proxy_buffers 4 4invalid;\n\t\tproxy_buffer_size 4invalid;\n\t\tproxy_busy_buffers_size 0;", + }, + { + name: "Case 12: Extreme values - should handle gracefully", + proxyBuffers: "1000000 1k", + proxyBufferSize: "999m", + expectedOutput: "proxy_buffers 1024 1k;\n\t\tproxy_buffer_size 511k;\n\t\tproxy_busy_buffers_size 511k;", + }, + { + name: "Case 13: Boundary conditions", + proxyBuffers: "2 1k", + proxyBufferSize: "1k", + expectedOutput: "proxy_buffers 2 1k;\n\t\tproxy_buffer_size 1k;\n\t\tproxy_busy_buffers_size 1k;", + }, + { + name: "Case 14: Real-world problematic configuration", + proxyBuffers: "8 4k", + proxyBufferSize: "64k", + expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 14k;\n\t\tproxy_busy_buffers_size 22k;", + }, { name: "Buffer size with busy buffer calculates minimum buffers", - proxyBuffers: "", proxyBufferSize: "4k", - proxyBusyBuffersSize: "20k", // needs (20k + 4k) / 4k = 6 buffers + proxyBusyBuffersSize: "20k", expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 6 4k;\n\t\tproxy_busy_buffers_size 20k;", }, { - name: "Single buffer corrected to minimum count", - proxyBuffers: "1 2k", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 2 2k;\n\t\tproxy_buffer_size 2k;\n\t\tproxy_busy_buffers_size 2k;", + name: "Single buffer corrected to minimum count", + proxyBuffers: "1 2k", + expectedOutput: "proxy_buffers 2 2k;\n\t\tproxy_buffer_size 2k;\n\t\tproxy_busy_buffers_size 2k;", }, { - name: "Single buffer with larger buffer size gets corrected", - proxyBuffers: "1 2k", - proxyBufferSize: "8k", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 2 8k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 8k;", + name: "Single buffer with larger buffer size gets corrected", + proxyBuffers: "1 2k", + proxyBufferSize: "8k", + expectedOutput: "proxy_buffers 2 2k;\n\t\tproxy_buffer_size 2k;\n\t\tproxy_busy_buffers_size 2k;", + }, + { + name: "Zero buffers corrected to minimum 2", + proxyBuffers: "0 4k", + expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", + }, + { + name: "Large buffer count unchanged", + proxyBuffers: "16 1k", + expectedOutput: "proxy_buffers 16 1k;\n\t\tproxy_buffer_size 1k;\n\t\tproxy_busy_buffers_size 3k;", + }, + { + name: "Only busy buffer size set", + proxyBusyBuffersSize: "8k", + expectedOutput: "proxy_busy_buffers_size 8k;", }, + // Additional edge cases based on nginx validation requirements { - name: "Zero buffers corrected to minimum 2", - proxyBuffers: "0 4k", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", + name: "Very small buffers with large buffer size", + proxyBuffers: "2 1k", + proxyBufferSize: "2k", + expectedOutput: "proxy_buffers 2 1k;\n\t\tproxy_buffer_size 1k;\n\t\tproxy_busy_buffers_size 1k;", }, { - name: "Valid configuration with minimum buffers unchanged", - proxyBuffers: "2 4k", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;", + name: "Busy buffer exactly at limit", + proxyBuffers: "4 4k", + proxyBusyBuffersSize: "12k", + expectedOutput: "proxy_buffers 4 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 12k;", }, { - name: "Large buffer count unchanged", - proxyBuffers: "16 1k", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expectedOutput: "proxy_buffers 16 1k;\n\t\tproxy_buffer_size 1k;", + name: "Busy buffer too small - gets adjusted", + proxyBuffers: "4 8k", + proxyBufferSize: "16k", + proxyBusyBuffersSize: "4k", + expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;\n\t\tproxy_busy_buffers_size 16k;", }, } - for _, tc := range testCases { + for _, tc := range tests { tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() @@ -183,7 +231,7 @@ func TestMakeProxyBuffers(t *testing.T) { func TestValidateBusyBufferSize(t *testing.T) { t.Parallel() - testCases := []struct { + tests := []struct { name string proxyBuffers string proxyBufferSize string @@ -191,31 +239,24 @@ func TestValidateBusyBufferSize(t *testing.T) { expected string }{ { - name: "All empty", - proxyBuffers: "", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expected: "", + name: "All empty", + expected: "", }, { - name: "No busy buffer size set", - proxyBuffers: "4 16k", - proxyBufferSize: "", - proxyBusyBuffersSize: "", - expected: "", + name: "No busy buffer size set", + proxyBuffers: "4 16k", + expected: "16k", }, { - name: "No busy buffer size with buffer size set", - proxyBuffers: "4 16k", - proxyBufferSize: "8k", - proxyBusyBuffersSize: "", - expected: "", + name: "No busy buffer size with buffer size set", + proxyBuffers: "4 16k", + proxyBufferSize: "8k", + expected: "16k", }, { name: "Valid busy buffer size within limits", proxyBuffers: "4 16k", - proxyBufferSize: "", - proxyBusyBuffersSize: "32k", // within (4*16k - 16k = 48k) + proxyBusyBuffersSize: "32k", expected: "32k", }, { @@ -233,34 +274,33 @@ func TestValidateBusyBufferSize(t *testing.T) { expected: "16k", }, { - name: "Busy buffer too large, gets clamped to max", + name: "Busy buffer too large, gets clamped", proxyBuffers: "4 4k", - proxyBufferSize: "", - proxyBusyBuffersSize: "20k", // larger than (4*4k - 4k = 12k) + proxyBusyBuffersSize: "20k", expected: "12k", }, { - name: "Busy buffer too small, gets adjusted to minimum", + name: "Busy buffer too small, gets adjusted", proxyBuffers: "4 8k", - proxyBufferSize: "16k", // larger than individual buffer - proxyBusyBuffersSize: "4k", // smaller than 16k minimum + proxyBufferSize: "16k", + proxyBusyBuffersSize: "4k", expected: "16k", }, { - name: "Buffer size larger than individual, busy buffer gets aligned", + name: "Buffer size larger than individual", proxyBuffers: "4 8k", proxyBufferSize: "16k", - proxyBusyBuffersSize: "12k", // less than buffer size but within range + proxyBusyBuffersSize: "12k", expected: "16k", }, } - for _, tc := range testCases { + for _, tc := range tests { tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() - got := validateBusyBufferSize(tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBuffersSize) + got := processBusyBufferSize(tc.proxyBuffers, tc.proxyBufferSize, tc.proxyBusyBuffersSize) if got != tc.expected { t.Errorf("Input: buffers=%q, bufferSize=%q, busyBufferSize=%q\nGot: %q\nExpected: %q", diff --git a/internal/configs/configmaps.go b/internal/configs/configmaps.go index 946ad6204c..1bc5415792 100644 --- a/internal/configs/configmaps.go +++ b/internal/configs/configmaps.go @@ -346,17 +346,20 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has cfgParams.ProxyBuffers = proxyBuffers } } else { - cfgParams.ProxyBuffers = proxyBuffers + errorText := fmt.Sprintf("ConfigMap %s/%s: invalid value for 'proxy-buffers': %q, must be in format 'count size' (e.g. '4 8k'), ignoring", cfgm.GetNamespace(), cfgm.GetName(), proxyBuffers) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText) + configOk = false } } if proxyBufferSize, exists := cfgm.Data["proxy-buffer-size"]; exists { - normalizedProxyBufferSize := validation.NormalizeSize(proxyBufferSize) + normalizedProxyBufferSize := validation.NormalizeBufferSize(proxyBufferSize) cfgParams.ProxyBufferSize = normalizedProxyBufferSize } if proxyBusyBuffersSize, exists := cfgm.Data["proxy-busy-buffers-size"]; exists { - normalizedProxyBusyBuffersSize := validation.NormalizeSize(proxyBusyBuffersSize) + normalizedProxyBusyBuffersSize := validation.NormalizeBufferSize(proxyBusyBuffersSize) cfgParams.ProxyBusyBuffersSize = normalizedProxyBusyBuffersSize } diff --git a/internal/configs/configmaps_test.go b/internal/configs/configmaps_test.go index 22559b1823..220f0755f9 100644 --- a/internal/configs/configmaps_test.go +++ b/internal/configs/configmaps_test.go @@ -9,6 +9,7 @@ import ( "github.com/nginx/kubernetes-ingress/internal/configs/commonhelpers" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" ) @@ -2047,6 +2048,89 @@ func TestParseProxyBuffers(t *testing.T) { } } +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: false, + description: "should reject empty string", + }, + } + + nginxPlus := true + hasAppProtect := false + hasAppProtectDos := false + hasTLSPassthrough := false + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + 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 != test.proxyBuffers { + t.Errorf("%s: expected ProxyBuffers=%q, got %q", test.description, test.proxyBuffers, result.ProxyBuffers) + } + } else { + if result.ProxyBuffers != "" { + 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/validation/validation.go b/internal/validation/validation.go index cb8c317276..f7171cdd48 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -195,7 +195,6 @@ func ParseSize(sizeStr string) int64 { return 0 } - // Handle plain numbers if num, err := strconv.ParseInt(sizeStr, 10, 64); err == nil { if num <= 0 { return 0 @@ -219,7 +218,6 @@ func ParseSize(sizeStr string) int64 { unit = 'm' } - // Convert based on unit switch unit { case 'k': return num << 10 @@ -230,73 +228,30 @@ func ParseSize(sizeStr string) int64 { } } -// FormatSize converts bytes to a human-readable size string with appropriate units +// FormatSize converts bytes to human-readable size string func FormatSize(bytes int64) string { if bytes == 0 { return "0" } - // If it's >= 1MB, format as MB (round down) if bytes >= (1 << 20) { return fmt.Sprintf("%dm", bytes/(1<<20)) } - // If it's >= 1KB, format as KB (round down) if bytes >= (1 << 10) { return fmt.Sprintf("%dk", bytes/(1<<10)) } - // Otherwise return as plain bytes return fmt.Sprintf("%d", bytes) } -// ValidateNginxSize validates and normalizes nginx size format, auto-correcting invalid units to 'm' -func ValidateNginxSize(sizeStr string) error { - sizeStr = strings.ToLower(strings.TrimSpace(sizeStr)) - if sizeStr == "" { - return fmt.Errorf("size cannot be empty") - } - - // Allow plain numbers - if _, err := strconv.ParseInt(sizeStr, 10, 64); err == nil { - return nil - } - - if len(sizeStr) < 2 { - return fmt.Errorf("invalid size format: %s", sizeStr) - } - - numStr, unit := sizeStr[:len(sizeStr)-1], sizeStr[len(sizeStr)-1] - - // Validate the numeric part - if _, err := strconv.ParseInt(numStr, 10, 64); err != nil { - return fmt.Errorf("invalid size format: %s", sizeStr) - } - - // Only allow k, m units - reject g and other invalid units - if unit != 'k' && unit != 'm' { - return fmt.Errorf("invalid size unit '%c': only 'k' and 'm' are allowed", unit) - } - - return nil -} - -// ValidateProxyBuffers validates and normalizes "count size" format, auto-correcting invalid units -func ValidateProxyBuffers(proxyBuffers string) error { - if proxyBuffers == "" { - return fmt.Errorf("proxy_buffers cannot be empty") - } - - fields := strings.Fields(strings.TrimSpace(proxyBuffers)) - if len(fields) != 2 { - return fmt.Errorf("proxy_buffers must be in format 'count size', got: %s", proxyBuffers) - } - - // Validate count - if count, err := strconv.Atoi(fields[0]); err != nil || count <= 0 { - return fmt.Errorf("invalid buffer count '%s': must be positive integer", fields[0]) +// 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] + } } - - // Validate size - now allows auto-correction of invalid units - return ValidateNginxSize(fields[1]) + return NormalizeSize(sizeStr) } diff --git a/internal/validation/validation_test.go b/internal/validation/validation_test.go index 520b391237..e5eae8dcf0 100644 --- a/internal/validation/validation_test.go +++ b/internal/validation/validation_test.go @@ -266,150 +266,3 @@ func TestFormatSize(t *testing.T) { }) } } - -func TestValidateNginxSize(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - input string - wantErr bool - }{ - { - name: "valid plain number", - input: "1024", - wantErr: false, - }, - { - name: "valid k unit", - input: "4k", - wantErr: false, - }, - { - name: "valid m unit", - input: "2m", - wantErr: false, - }, - { - name: "invalid g unit", - input: "1g", - wantErr: true, - }, - { - name: "invalid multi-letter unit", - input: "4kb", - wantErr: true, - }, - { - name: "empty string", - input: "", - wantErr: true, - }, - { - name: "invalid format", - input: "invalid", - wantErr: true, - }, - { - name: "case insensitive", - input: "4K", - wantErr: false, - }, - { - name: "with whitespace", - input: " 8k ", - wantErr: false, - }, - } - - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - err := ValidateNginxSize(tc.input) - if tc.wantErr && err == nil { - t.Errorf("ValidateNginxSize(%q) expected error, got nil", tc.input) - } - if !tc.wantErr && err != nil { - t.Errorf("ValidateNginxSize(%q) expected no error, got %v", tc.input, err) - } - }) - } -} - -func TestValidateProxyBuffers(t *testing.T) { - t.Parallel() - - testCases := []struct { - name string - input string - wantErr bool - }{ - { - name: "valid format", - input: "8 4k", - wantErr: false, - }, - { - name: "valid with m unit", - input: "4 2m", - wantErr: false, - }, - { - name: "invalid format - missing size", - input: "8", - wantErr: true, - }, - { - name: "invalid format - too many parts", - input: "8 4k extra", - wantErr: true, - }, - { - name: "invalid count - zero", - input: "0 4k", - wantErr: true, - }, - { - name: "invalid count - negative", - input: "-1 4k", - wantErr: true, - }, - { - name: "invalid count - non-numeric", - input: "abc 4k", - wantErr: true, - }, - { - name: "invalid size - g unit", - input: "8 1g", - wantErr: true, - }, - { - name: "empty string", - input: "", - wantErr: true, - }, - { - name: "with whitespace", - input: " 8 4k ", - wantErr: false, - }, - } - - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - err := ValidateProxyBuffers(tc.input) - if tc.wantErr && err == nil { - t.Errorf("ValidateProxyBuffers(%q) expected error, got nil", tc.input) - } - if !tc.wantErr && err != nil { - t.Errorf("ValidateProxyBuffers(%q) expected no error, got %v", tc.input, err) - } - }) - } -} From 10999ff02aa025f93394990c2ef879d5eb759b9f Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Thu, 31 Jul 2025 10:28:02 +0100 Subject: [PATCH 09/11] Update tests --- .../commonhelpers/common_template_helpers.go | 3 +- .../common_template_helpers_test.go | 37 ++++++------------- 2 files changed, 13 insertions(+), 27 deletions(-) diff --git a/internal/configs/commonhelpers/common_template_helpers.go b/internal/configs/commonhelpers/common_template_helpers.go index b7e66ae943..283338ee48 100644 --- a/internal/configs/commonhelpers/common_template_helpers.go +++ b/internal/configs/commonhelpers/common_template_helpers.go @@ -166,9 +166,8 @@ func addBusyBufferSizeConfig(parts []string, proxyBuffers, proxyBufferSize, prox return parts } -// processBusyBufferSize handles both validation of busy buffer size // If proxyBusyBuffersSize is empty, it calculates a safe value -// If proxyBusyBuffersSize is provided, it validates and corrects it if needed +// If proxyBusyBuffersSize is provided, it validates and corrects it func processBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) string { fields := strings.Fields(proxyBuffers) if len(fields) < 2 { diff --git a/internal/configs/commonhelpers/common_template_helpers_test.go b/internal/configs/commonhelpers/common_template_helpers_test.go index ffe7449282..132d614a48 100644 --- a/internal/configs/commonhelpers/common_template_helpers_test.go +++ b/internal/configs/commonhelpers/common_template_helpers_test.go @@ -77,84 +77,72 @@ func TestMakeProxyBuffers(t *testing.T) { expectedOutput: "", }, { - name: "Case 1: Only buffer-size set (buffer-size only)", + name: "buffer-size only", proxyBufferSize: "4k", expectedOutput: "proxy_buffer_size 4k;\n\t\tproxy_buffers 4 4k;\n\t\tproxy_busy_buffers_size 4k;", }, { - name: "Case 2: Only buffers set (buffers only)", + name: "buffers only", proxyBuffers: "4 16k", expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;\n\t\tproxy_busy_buffers_size 16k;", }, { - name: "Case 3: Both buffers and buffer-size set (both explicit)", + name: "Both buffers and buffer-size set ", proxyBuffers: "4 16k", proxyBufferSize: "8k", expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 16k;", }, { - name: "Case 4: Invalid combination that should self-heal", + name: "Invalid combination that should correct itself", proxyBuffers: "8 1m", proxyBufferSize: "5m", expectedOutput: "proxy_buffers 8 1m;\n\t\tproxy_buffer_size 5m;\n\t\tproxy_busy_buffers_size 5m;", }, { - name: "Case 5: Buffer-size smaller than individual buffer size", + name: "Buffer-size smaller than individual buffer size", proxyBuffers: "4 1m", proxyBufferSize: "512k", expectedOutput: "proxy_buffers 4 1m;\n\t\tproxy_buffer_size 512k;\n\t\tproxy_busy_buffers_size 1m;", }, { - name: "Case 6: Minimum buffers configuration", + name: "Minimum buffers configuration", proxyBuffers: "2 4k", proxyBufferSize: "4k", expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", }, { - name: "Case 7: All three parameters set (all three valid)", + name: "All three parameters set", proxyBuffers: "8 4k", proxyBufferSize: "4k", proxyBusyBuffersSize: "16k", expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 16k;", }, { - name: "Case 8: Busy buffer too large - gets clamped", + name: "Busy buffer too large - reduces in size", proxyBuffers: "4 8k", proxyBufferSize: "8k", proxyBusyBuffersSize: "40k", expectedOutput: "proxy_buffers 4 8k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 24k;", }, { - name: "Case 9: Busy buffer wrong format - should be validated", + name: "Busy buffer wrong format", proxyBuffers: "4 4k", proxyBusyBuffersSize: "invalid", expectedOutput: "proxy_buffers 4 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", }, { - name: "Case 10: Empty/zero values - corrected to minimum", + name: "Empty/zero values - corrected to minimum", proxyBuffers: "0 4k", expectedOutput: "proxy_buffers 2 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 4k;", }, { - name: "Case 11: Invalid units - should be handled gracefully", - proxyBuffers: "4 4invalid", - proxyBufferSize: "8invalid", - expectedOutput: "proxy_buffers 4 4invalid;\n\t\tproxy_buffer_size 4invalid;\n\t\tproxy_busy_buffers_size 0;", - }, - { - name: "Case 12: Extreme values - should handle gracefully", + name: "Extreme values - autocorrect", proxyBuffers: "1000000 1k", proxyBufferSize: "999m", expectedOutput: "proxy_buffers 1024 1k;\n\t\tproxy_buffer_size 511k;\n\t\tproxy_busy_buffers_size 511k;", }, { - name: "Case 13: Boundary conditions", - proxyBuffers: "2 1k", - proxyBufferSize: "1k", - expectedOutput: "proxy_buffers 2 1k;\n\t\tproxy_buffer_size 1k;\n\t\tproxy_busy_buffers_size 1k;", - }, - { - name: "Case 14: Real-world problematic configuration", + name: "Autocorrect buffer size and buffers", proxyBuffers: "8 4k", proxyBufferSize: "64k", expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 14k;\n\t\tproxy_busy_buffers_size 22k;", @@ -191,7 +179,6 @@ func TestMakeProxyBuffers(t *testing.T) { proxyBusyBuffersSize: "8k", expectedOutput: "proxy_busy_buffers_size 8k;", }, - // Additional edge cases based on nginx validation requirements { name: "Very small buffers with large buffer size", proxyBuffers: "2 1k", From b9394c0d52bc5e7edf0dc26544897f2d1cdb0747 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Thu, 31 Jul 2025 14:07:09 +0100 Subject: [PATCH 10/11] Update tests, improve validation --- .../commonhelpers/common_template_helpers.go | 18 +++++++----------- .../common_template_helpers_test.go | 8 ++++---- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/internal/configs/commonhelpers/common_template_helpers.go b/internal/configs/commonhelpers/common_template_helpers.go index 283338ee48..121f16b3c5 100644 --- a/internal/configs/commonhelpers/common_template_helpers.go +++ b/internal/configs/commonhelpers/common_template_helpers.go @@ -99,20 +99,16 @@ func correctBufferConfig(proxyBuffers, proxyBufferSize string) (string, string) if proxyBufferSize == "" { proxyBufferSize = fields[1] - } else if validation.ParseSize(proxyBufferSize) > validation.ParseSize(fields[1]) { + } else { + // When proxyBufferSize is explicitly set, keep it to the individual buffer size from proxy_buffers if it's larger + individualBufferSize := validation.ParseSize(fields[1]) bufferSizeBytes := validation.ParseSize(proxyBufferSize) - individualSizeBytes := validation.ParseSize(fields[1]) - maxPossibleBusySize := int64(count)*individualSizeBytes - individualSizeBytes - if bufferSizeBytes >= maxPossibleBusySize { - cappedSize := maxPossibleBusySize / 2 - if cappedSize < individualSizeBytes { - cappedSize = individualSizeBytes - } - proxyBufferSize = validation.FormatSize(cappedSize) - } else if bufferSizeBytes <= (1 << 20) { // 1MB limit for changing buffer sizes - proxyBuffers = fmt.Sprintf("%d %s", count, proxyBufferSize) + if bufferSizeBytes > individualBufferSize { + // proxy_buffer_size to not exceed individual buffer size + proxyBufferSize = fields[1] } + proxyBufferSize, _ = capBufferLimits(proxyBufferSize, 0) } return proxyBuffers, proxyBufferSize diff --git a/internal/configs/commonhelpers/common_template_helpers_test.go b/internal/configs/commonhelpers/common_template_helpers_test.go index 132d614a48..71e89c7172 100644 --- a/internal/configs/commonhelpers/common_template_helpers_test.go +++ b/internal/configs/commonhelpers/common_template_helpers_test.go @@ -96,7 +96,7 @@ func TestMakeProxyBuffers(t *testing.T) { name: "Invalid combination that should correct itself", proxyBuffers: "8 1m", proxyBufferSize: "5m", - expectedOutput: "proxy_buffers 8 1m;\n\t\tproxy_buffer_size 5m;\n\t\tproxy_busy_buffers_size 5m;", + expectedOutput: "proxy_buffers 8 1m;\n\t\tproxy_buffer_size 1m;\n\t\tproxy_busy_buffers_size 3m;", }, { name: "Buffer-size smaller than individual buffer size", @@ -139,13 +139,13 @@ func TestMakeProxyBuffers(t *testing.T) { name: "Extreme values - autocorrect", proxyBuffers: "1000000 1k", proxyBufferSize: "999m", - expectedOutput: "proxy_buffers 1024 1k;\n\t\tproxy_buffer_size 511k;\n\t\tproxy_busy_buffers_size 511k;", + expectedOutput: "proxy_buffers 1024 1k;\n\t\tproxy_buffer_size 1k;\n\t\tproxy_busy_buffers_size 1k;", }, { name: "Autocorrect buffer size and buffers", proxyBuffers: "8 4k", proxyBufferSize: "64k", - expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 14k;\n\t\tproxy_busy_buffers_size 22k;", + expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 12k;", }, { name: "Buffer size with busy buffer calculates minimum buffers", @@ -196,7 +196,7 @@ func TestMakeProxyBuffers(t *testing.T) { proxyBuffers: "4 8k", proxyBufferSize: "16k", proxyBusyBuffersSize: "4k", - expectedOutput: "proxy_buffers 4 16k;\n\t\tproxy_buffer_size 16k;\n\t\tproxy_busy_buffers_size 16k;", + expectedOutput: "proxy_buffers 4 8k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 8k;", }, } From 58fe7cd26833008ae3d1840bbf57cdebcdf0feae Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Thu, 31 Jul 2025 16:05:38 +0100 Subject: [PATCH 11/11] fix validation --- .../configs/commonhelpers/common_template_helpers.go | 12 ++++++------ .../commonhelpers/common_template_helpers_test.go | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/configs/commonhelpers/common_template_helpers.go b/internal/configs/commonhelpers/common_template_helpers.go index 121f16b3c5..27dd570cfb 100644 --- a/internal/configs/commonhelpers/common_template_helpers.go +++ b/internal/configs/commonhelpers/common_template_helpers.go @@ -100,15 +100,15 @@ func correctBufferConfig(proxyBuffers, proxyBufferSize string) (string, string) if proxyBufferSize == "" { proxyBufferSize = fields[1] } else { - // When proxyBufferSize is explicitly set, keep it to the individual buffer size from proxy_buffers if it's larger - individualBufferSize := validation.ParseSize(fields[1]) + // Validate user-defined proxy_buffer_size bufferSizeBytes := validation.ParseSize(proxyBufferSize) - - if bufferSizeBytes > individualBufferSize { - // proxy_buffer_size to not exceed individual buffer size + if bufferSizeBytes <= 0 { + // Invalid → fallback to individual buffer size proxyBufferSize = fields[1] + } else { + // Valid → cap it if needed, but don't downgrade + proxyBufferSize, _ = capBufferLimits(proxyBufferSize, 0) } - proxyBufferSize, _ = capBufferLimits(proxyBufferSize, 0) } return proxyBuffers, proxyBufferSize diff --git a/internal/configs/commonhelpers/common_template_helpers_test.go b/internal/configs/commonhelpers/common_template_helpers_test.go index 71e89c7172..7cf66ec4e9 100644 --- a/internal/configs/commonhelpers/common_template_helpers_test.go +++ b/internal/configs/commonhelpers/common_template_helpers_test.go @@ -96,7 +96,7 @@ func TestMakeProxyBuffers(t *testing.T) { name: "Invalid combination that should correct itself", proxyBuffers: "8 1m", proxyBufferSize: "5m", - expectedOutput: "proxy_buffers 8 1m;\n\t\tproxy_buffer_size 1m;\n\t\tproxy_busy_buffers_size 3m;", + expectedOutput: "proxy_buffers 8 1m;\n\t\tproxy_buffer_size 5m;\n\t\tproxy_busy_buffers_size 5m;", }, { name: "Buffer-size smaller than individual buffer size", @@ -139,13 +139,13 @@ func TestMakeProxyBuffers(t *testing.T) { name: "Extreme values - autocorrect", proxyBuffers: "1000000 1k", proxyBufferSize: "999m", - expectedOutput: "proxy_buffers 1024 1k;\n\t\tproxy_buffer_size 1k;\n\t\tproxy_busy_buffers_size 1k;", + expectedOutput: "proxy_buffers 1024 1k;\n\t\tproxy_buffer_size 999m;\n\t\tproxy_busy_buffers_size 1022k;", }, { name: "Autocorrect buffer size and buffers", proxyBuffers: "8 4k", proxyBufferSize: "64k", - expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 4k;\n\t\tproxy_busy_buffers_size 12k;", + expectedOutput: "proxy_buffers 8 4k;\n\t\tproxy_buffer_size 64k;\n\t\tproxy_busy_buffers_size 27k;", }, { name: "Buffer size with busy buffer calculates minimum buffers", @@ -162,7 +162,7 @@ func TestMakeProxyBuffers(t *testing.T) { name: "Single buffer with larger buffer size gets corrected", proxyBuffers: "1 2k", proxyBufferSize: "8k", - expectedOutput: "proxy_buffers 2 2k;\n\t\tproxy_buffer_size 2k;\n\t\tproxy_busy_buffers_size 2k;", + expectedOutput: "proxy_buffers 2 2k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 2k;", }, { name: "Zero buffers corrected to minimum 2", @@ -183,7 +183,7 @@ func TestMakeProxyBuffers(t *testing.T) { name: "Very small buffers with large buffer size", proxyBuffers: "2 1k", proxyBufferSize: "2k", - expectedOutput: "proxy_buffers 2 1k;\n\t\tproxy_buffer_size 1k;\n\t\tproxy_busy_buffers_size 1k;", + expectedOutput: "proxy_buffers 2 1k;\n\t\tproxy_buffer_size 2k;\n\t\tproxy_busy_buffers_size 1k;", }, { name: "Busy buffer exactly at limit", @@ -196,7 +196,7 @@ func TestMakeProxyBuffers(t *testing.T) { proxyBuffers: "4 8k", proxyBufferSize: "16k", proxyBusyBuffersSize: "4k", - expectedOutput: "proxy_buffers 4 8k;\n\t\tproxy_buffer_size 8k;\n\t\tproxy_busy_buffers_size 8k;", + expectedOutput: "proxy_buffers 4 8k;\n\t\tproxy_buffer_size 16k;\n\t\tproxy_busy_buffers_size 16k;", }, }