Skip to content

Add Safer Proxy_Buffer Config #8088

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions config/crd/bases/k8s.nginx.org_virtualserverroutes.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions config/crd/bases/k8s.nginx.org_virtualservers.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions deploy/crds.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions docs/crd/k8s.nginx.org_virtualserverroutes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.' |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given the existing naming convension, shouldn't this be

upstreams[].buffers.busy-size

| `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. |
Expand Down
1 change: 1 addition & 0 deletions docs/crd/k8s.nginx.org_virtualservers.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. |
Expand Down
7 changes: 6 additions & 1 deletion internal/configs/annotations.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -301,7 +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 = validation.NormalizeBufferSize(proxyBusyBuffersSize)
}

if upstreamZoneSize, exists := ingEx.Ingress.Annotations["nginx.org/upstream-zone-size"]; exists {
Expand Down
193 changes: 193 additions & 0 deletions internal/configs/commonhelpers/common_template_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@
package commonhelpers

import (
"fmt"
"strconv"
"strings"

"github.com/nginx/kubernetes-ingress/internal/validation"
)

// MakeSecretPath will return the path to the secret with the base secrets
Expand All @@ -27,3 +31,192 @@ func MakeOnOffFromBool(b *bool) string {
func BoolToPointerBool(b bool) *bool {
return &b
}

// MakeProxyBuffers generates nginx proxy buffer configuration with validation
func MakeProxyBuffers(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) string {
var parts []string

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 {
minBuffers := int((validation.ParseSize(proxyBusyBuffersSize) + bufferSizeBytes) / bufferSizeBytes)
if 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))
Comment on lines +45 to +56
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be refactored to make the code more readable, i.e. a function named after what we want to do

} else if proxyBuffers != "" {
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 proxyBusyBuffersSize == "" && originalBuffers != proxyBuffers {
proxyBusyBuffersSize = defaultBusyBufferSize(proxyBuffers)
}
}

parts = addBusyBufferSizeConfig(parts, proxyBuffers, proxyBufferSize, proxyBusyBuffersSize)

if len(parts) == 0 {
return ""
}
return strings.Join(parts, ";\n\t\t") + ";"
}

func defaultBusyBufferSize(proxyBuffers string) string {
fields := strings.Fields(proxyBuffers)
if len(fields) >= 2 {
// Return the individual buffer size as the default busy buffer size
return fields[1]
}
return ""
}

// 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])

// 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 {
// Validate user-defined proxy_buffer_size
bufferSizeBytes := validation.ParseSize(proxyBufferSize)
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)
}
}

return proxyBuffers, proxyBufferSize
}

// 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
}

// If proxyBusyBuffersSize is empty, it calculates a safe value
// If proxyBusyBuffersSize is provided, it validates and corrects it
func processBusyBufferSize(proxyBuffers, proxyBufferSize, proxyBusyBuffersSize string) string {
fields := strings.Fields(proxyBuffers)
if len(fields) < 2 {
if proxyBusyBuffersSize == "" {
return ""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we not return an error here?

}
return proxyBusyBuffersSize
}

count, _ := strconv.Atoi(fields[0])
individualSize := validation.ParseSize(fields[1])

if proxyBusyBuffersSize == "" {
proxyBufferSize, _ = capBufferLimits(proxyBufferSize, 0)
}
bufferSize := validation.ParseSize(proxyBufferSize)

maxSize := int64(count)*individualSize - individualSize
minSize := individualSize
if bufferSize > minSize {
minSize = 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 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)
}

busySize := validation.ParseSize(proxyBusyBuffersSize)

if busySize >= maxSize {
return validation.FormatSize(maxSize)
}

if busySize < minSize {
return validation.FormatSize(minSize)
}

return proxyBusyBuffersSize
}
Loading
Loading