Skip to content

Move proxy values balancing to the beginning of parsing #8133

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 32 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
252ee58
add helper function to enable proxy_buffers if proxy_buffer_size is e…
AlexFenlon Jul 24, 2025
4d5a6d3
add a safe way to enable proxy_buffers and proxy_buffer_size
AlexFenlon Jul 24, 2025
36ad41a
add `nginx.org/proxy-busy-buffers-size` annotation to ingress
AlexFenlon Jul 25, 2025
0952f77
add `proxy-busy-buffer-size` to configmap, improve validation
AlexFenlon Jul 28, 2025
5bea282
Merge remote-tracking branch 'refs/remotes/origin/main' into feat/saf…
AlexFenlon Jul 28, 2025
de8c064
rename ProxyBusyBufferSize to ProxyBusyBuffersSize to be more accurate
AlexFenlon Jul 28, 2025
ca2e16b
Add ProxyBusyBuffersSize to VirtualServer as `busy-buffers-size`
AlexFenlon Jul 28, 2025
4162d5d
make validation better
AlexFenlon Jul 30, 2025
7457a49
Merge branch 'main' into feat/safe-buffer-config
AlexFenlon Jul 30, 2025
1bf7b14
make validation better
AlexFenlon Jul 30, 2025
9cffde2
Merge remote-tracking branch 'origin/feat/safe-buffer-config' into fe…
AlexFenlon Jul 30, 2025
10999ff
Update tests
AlexFenlon Jul 31, 2025
b9394c0
Update tests, improve validation
AlexFenlon Jul 31, 2025
58fe7cd
fix validation
AlexFenlon Jul 31, 2025
942f17d
Put back the proxy max temp filesize data
javorszky Aug 8, 2025
bd2cd8e
Create a new typed unit to store sizes and offsets
javorszky Aug 8, 2025
e2a9072
Move new units to internal configs
javorszky Aug 8, 2025
22187df
Move data types to their own file in internal/validation
javorszky Aug 11, 2025
eb0098d
Move proxy units into common package
javorszky Aug 11, 2025
4abb3cf
Adjust templates to make them pass
javorszky Aug 11, 2025
64a401c
Normalise parsing of bad / unknown to mb
javorszky Aug 12, 2025
d8998cd
Return emptystring if numbersize is zero
javorszky Aug 12, 2025
cd5587b
Create BalanceProxy function and tests
javorszky Aug 13, 2025
af0aba1
Reconcile the proxy values after parsing them
javorszky Aug 13, 2025
aee9418
Fix logic in clamping behaviour
javorszky Aug 13, 2025
ed52d20
Fix tests, make another test parallel
javorszky Aug 13, 2025
2a8287f
Account for proxies not being present if invalid
javorszky Aug 13, 2025
254c261
Simplify tests for data types
javorszky Aug 13, 2025
ba838f0
Fix tests
javorszky Aug 13, 2025
128e97a
Fix more tests
javorszky Aug 13, 2025
0186bb5
Merge branch 'main' into feat/fixed-safe-buffer-config
javorszky Aug 14, 2025
c55f83d
Merge branch 'main' into feat/fixed-safe-buffer-config
javorszky Aug 14, 2025
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.' |
| `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
41 changes: 39 additions & 2 deletions 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 @@ -296,12 +297,48 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool
}
}

// Proxy Buffers uses number + size format, like "8 4k".
if proxyBuffers, exists := ingEx.Ingress.Annotations["nginx.org/proxy-buffers"]; exists {
cfgParams.ProxyBuffers = proxyBuffers
proxyBufferUnits, err := validation.NewNumberSizeConfig(proxyBuffers)
if err != nil {
nl.Errorf(l, "error parsing nginx.org/proxy-buffers: %s", err)
} else {
cfgParams.ProxyBuffers = proxyBufferUnits
}
}

// Proxy Buffer Size uses only size format, like "4k".
if proxyBufferSize, exists := ingEx.Ingress.Annotations["nginx.org/proxy-buffer-size"]; exists {
cfgParams.ProxyBufferSize = proxyBufferSize
proxyBufferSizeUnit, err := validation.NewSizeWithUnit(proxyBufferSize)
if err != nil {
nl.Errorf(l, "error parsing nginx.org/proxy-buffer-size: %s", err)
} else {
cfgParams.ProxyBufferSize = proxyBufferSizeUnit
}
}

// Proxy Busy Buffers Size uses only size format, like "8k".
if proxyBusyBuffersSize, exists := ingEx.Ingress.Annotations["nginx.org/proxy-busy-buffers-size"]; exists {
proxyBusyBufferSizeUnit, err := validation.NewSizeWithUnit(proxyBusyBuffersSize)
if err != nil {
nl.Errorf(l, "error parsing nginx.org/proxy-busy-buffers-size: %s", err)
} else {
cfgParams.ProxyBusyBuffersSize = proxyBusyBufferSizeUnit
}
}

balancedProxyBuffers, balancedProxyBufferSize, balancedProxyBusyBufferSize, modifications, err := validation.BalanceProxyValues(cfgParams.ProxyBuffers, cfgParams.ProxyBufferSize, cfgParams.ProxyBusyBuffersSize)
if err != nil {
nl.Errorf(l, "error reconciling proxy_buffers, proxy_buffer_size, and proxy_busy_buffers_size values: %s", err.Error())
}
cfgParams.ProxyBuffers = balancedProxyBuffers
cfgParams.ProxyBufferSize = balancedProxyBufferSize
cfgParams.ProxyBusyBuffersSize = balancedProxyBusyBufferSize

if len(modifications) > 0 {
for _, modification := range modifications {
nl.Infof(l, "Changes made to proxy values: %s", modification)
}
}

if upstreamZoneSize, exists := ingEx.Ingress.Annotations["nginx.org/upstream-zone-size"]; exists {
Expand Down
6 changes: 4 additions & 2 deletions internal/configs/config_params.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

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

// ConfigParams holds NGINX configuration parameters that affect the main NGINX config
Expand Down Expand Up @@ -69,8 +70,9 @@ type ConfigParams struct {
MainAppProtectDosLogFormatEscaping string
MainAppProtectDosArbFqdn string
ProxyBuffering bool
ProxyBuffers string
ProxyBufferSize string
ProxyBuffers validation.NumberSizeConfig
ProxyBufferSize validation.SizeWithUnit
ProxyBusyBuffersSize validation.SizeWithUnit
ProxyConnectTimeout string
ProxyHideHeaders []string
ProxyMaxTempFileSize string
Expand Down
47 changes: 44 additions & 3 deletions internal/configs/configmaps.go
Original file line number Diff line number Diff line change
Expand Up @@ -335,12 +335,53 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has
}

if proxyBuffers, exists := cfgm.Data["proxy-buffers"]; exists {
cfgParams.ProxyBuffers = proxyBuffers
proxyBuffersData, err := validation.NewNumberSizeConfig(proxyBuffers)
if err != nil {
wrappedError := fmt.Errorf("ConfigMap %s/%s: invalid value for 'proxy-buffers': %w", cfgm.GetNamespace(), cfgm.GetName(), err)

nl.Errorf(l, "%s", wrappedError.Error())
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, wrappedError.Error())
configOk = false
} else {
cfgParams.ProxyBuffers = proxyBuffersData
}
}

if proxyBufferSize, exists := cfgm.Data["proxy-buffer-size"]; exists {
cfgParams.ProxyBufferSize = proxyBufferSize
proxyBufferSizeData, err := validation.NewSizeWithUnit(proxyBufferSize)
if err != nil {
nl.Errorf(l, "error parsing nginx.org/proxy-buffer-size: %s", err)
} else {
cfgParams.ProxyBufferSize = proxyBufferSizeData
}
// normalizedProxyBufferSize := validation.NormalizeBufferSize(proxyBufferSize)
Copy link
Preview

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

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

This commented out code should be removed as it's not being used and adds clutter to the codebase.

Suggested change
// normalizedProxyBufferSize := validation.NormalizeBufferSize(proxyBufferSize)

Copilot uses AI. Check for mistakes.

}

// Proxy Busy Buffers Size uses only size format, like "8k".
if proxyBusyBuffersSize, exists := cfgm.Data["proxy-busy-buffers-size"]; exists {
proxyBusyBufferSizeUnit, err := validation.NewSizeWithUnit(proxyBusyBuffersSize)
if err != nil {
nl.Errorf(l, "error parsing nginx.org/proxy-busy-buffers-size: %s", err)
} else {
cfgParams.ProxyBusyBuffersSize = proxyBusyBufferSizeUnit
}
}

balancedProxyBuffers, balancedProxyBufferSize, balancedProxyBusyBufferSize, modifications, err := validation.BalanceProxyValues(cfgParams.ProxyBuffers, cfgParams.ProxyBufferSize, cfgParams.ProxyBusyBuffersSize)
if err != nil {
nl.Errorf(l, "error reconciling proxy_buffers, proxy_buffer_size, and proxy_busy_buffers_size values: %s", err.Error())
}
cfgParams.ProxyBuffers = balancedProxyBuffers
cfgParams.ProxyBufferSize = balancedProxyBufferSize
cfgParams.ProxyBusyBuffersSize = balancedProxyBusyBufferSize

if len(modifications) > 0 {
for _, modification := range modifications {
nl.Infof(l, "Changes made to proxy values: %s", modification)
}
}

// Normalise the three proxy buffer values across each other.
Copy link
Preview

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

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

This comment doesn't add value as it just restates what the code above does. It should be removed or made more descriptive.

Suggested change
// Normalise the three proxy buffer values across each other.
// Ensure proxy_buffers, proxy_buffer_size, and proxy_busy_buffers_size are consistent.
// The BalanceProxyValues function reconciles these values to prevent misconfiguration,
// applying necessary adjustments and logging any changes made.

Copilot uses AI. Check for mistakes.


if proxyMaxTempFileSize, exists := cfgm.Data["proxy-max-temp-file-size"]; exists {
cfgParams.ProxyMaxTempFileSize = proxyMaxTempFileSize
Expand Down Expand Up @@ -408,7 +449,7 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has
}
}

_, err := parseConfigMapZoneSync(l, cfgm, cfgParams, eventLog, nginxPlus)
_, err = parseConfigMapZoneSync(l, cfgm, cfgParams, eventLog, nginxPlus)
if err != nil {
configOk = false
}
Expand Down
Loading
Loading