Move proxy values balancing to the beginning of parsing #8133
Conversation
github-actions
bot
added
enhancement
afaik should only be used for CRD generation |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8133 +/- ##
==========================================
+ Coverage 53.35% 53.49% +0.13%
==========================================
Files 90 91 +1
Lines 21912 22237 +325
==========================================
+ Hits 11691 11895 +204
- Misses 9744 9851 +107
- Partials 477 491 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Docs PR: nginx/documentation#917 |
There was a problem hiding this comment.
Pull Request Overview
This PR moves proxy buffer value balancing from template helpers to the config/annotation parsing phase. It introduces custom data types for size and buffer configurations to ensure type safety and consistency, along with validation rules based on NGINX source code requirements.
Key changes:
- Introduces
SizeWithUnitandNumberSizeConfigstructs with type-safe parsing and validation - Implements
BalanceProxyValuesfunction to normalize proxy buffer relationships according to NGINX constraints - Updates test expectations to handle auto-correction of invalid proxy buffer configurations
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
internal/validation/data_types.go |
Defines new custom types for size and buffer configurations with validation logic |
internal/configs/configmaps.go |
Updates config parsing to use new types and balance proxy values |
internal/configs/annotations.go |
Updates annotation parsing to use new types and balance proxy values |
tests/suite/test_virtual_server_configmap_keys.py |
Updates test to handle corrected proxy buffer behavior |
internal/configs/virtualserver.go |
Updates template generation to use string representation of new types |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| // Autocorrect invalid units to 'm' | ||
| if unit != 'k' && unit != 'm' { |
There was a problem hiding this comment.
The autocorrection logic that changes invalid units to 'm' is not documented in the function comment. This behavior should be clearly documented as it could be unexpected for users.
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| if num > 1024 { | ||
| ret.Size = 1024 |
There was a problem hiding this comment.
The magic number 1024 should be defined as a named constant to improve code readability and maintainability.
| ret.Size = 1024 | |
| if num > MaxAllowedSize { | |
| ret.Size = MaxAllowedSize |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| if num < 2 { | ||
| num = 2 |
There was a problem hiding this comment.
The magic number 2 (minimum number of buffers) should be defined as a named constant to improve code readability and document the NGINX requirement.
| num = 2 | |
| if num < MinNginxBufferCount { | |
| num = MinNginxBufferCount |
Copilot uses AI. Check for mistakes.
| // 1.b there must be at most 1024 proxy buffers | ||
| if proxyBuffers.Number > 1024 { | ||
| modifications = append(modifications, fmt.Sprintf("adjusted proxy_buffers number from %d to 1024", proxyBuffers.Number)) | ||
| proxyBuffers.Number = 1024 |
There was a problem hiding this comment.
The magic number 1024 appears again and should be defined as a named constant to avoid duplication and improve maintainability.
| proxyBuffers.Number = 1024 | |
| // 1.b there must be at most MaxProxyBuffers proxy buffers | |
| if proxyBuffers.Number > MaxProxyBuffers { | |
| modifications = append(modifications, fmt.Sprintf("adjusted proxy_buffers number from %d to %d", proxyBuffers.Number, MaxProxyBuffers)) | |
| proxyBuffers.Number = MaxProxyBuffers |
Copilot uses AI. Check for mistakes.
| } else { | ||
| cfgParams.ProxyBufferSize = proxyBufferSizeData | ||
| } | ||
| // normalizedProxyBufferSize := validation.NormalizeBufferSize(proxyBufferSize) |
There was a problem hiding this comment.
This commented out code should be removed as it's not being used and adds clutter to the codebase.
| // normalizedProxyBufferSize := validation.NormalizeBufferSize(proxyBufferSize) |
Copilot uses AI. Check for mistakes.
| } | ||
| } | ||
|
|
||
| // Normalise the three proxy buffer values across each other. |
There was a problem hiding this comment.
This comment doesn't add value as it just restates what the code above does. It should be removed or made more descriptive.
| // 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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…nabled and vise versa
javorszky
force-pushed
the
feat/fixed-safe-buffer-config
branch
from
c6dd4db to
6f026c7
Compare
| } | ||
|
|
||
| in.ProxyBuffers = &conf_v1.UpstreamBuffers{ | ||
| Number: int(balancedPB.Number), |
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 9 hours ago
To fix the problem, we should ensure that before converting balancedPB.Number (a uint64) to int, we check that its value does not exceed math.MaxInt, which is the maximum value for the int type on the current platform. This is more robust than capping at math.MaxInt32, which is only correct for 32-bit systems. The fix involves:
- Replacing the check on line 312 from
math.MaxInt32tomath.MaxInt. - Updating the cap assignment accordingly.
- Ensuring that the
mathpackage is imported (already present). - No other changes are needed, as the lower bound is already enforced.
| @@ -309,8 +309,8 @@ | ||
| return fmt.Errorf("error balancing proxy values: %w", err) | ||
| } | ||
|
|
||
| if balancedPB.Number > uint64(math.MaxInt32) { | ||
| balancedPB.Number = uint64(math.MaxInt32) | ||
| if balancedPB.Number > uint64(math.MaxInt) { | ||
| balancedPB.Number = uint64(math.MaxInt) | ||
| } | ||
|
|
||
| in.ProxyBuffers = &conf_v1.UpstreamBuffers{ |
…nto feat/fixed-safe-buffer-config
Proposed changes
This PR moves the proxy values balancing from the template helpers to the time where the annotations and config maps are being parsed. There are a few notable changes:
4 8kand16k) are now custom structs with iota constants for the unit (k,m), anduint64for the numbers. This guarantees that once the values are parsed and corrected, they won't become invalid later through any changes, and we can offload a lot of the error checking to the compiler. One such is that we know none of the values will be negative, because auintcan't be negativeproxy_buffersvalue passed in via a configmap would generate a bad config, now it gets auto-corrected to the default2 4kvalue. This is the cost of moving the proxy balancing to be right after parsing the values2 4k, and everything else gets brought down to fit into that oneproxy_buffers: https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_proxy_module.c#L3763-L3767proxy_busy_buffers_size: https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_proxy_module.c#L3787-L3804proxy_buffers_sizeis at most all ofproxy_buffersminus one buffer sizeThis PR targets @AlexFenlon 's PR, as it's a modification of that one. Once merged, then we can bring the branch up to date with main.
Questions
Currently I put the custom data type and its tests into the
internal/validationpackage, however there's also thepkg/apis/configuration/v1/types.gofile which seems to do something similar. I have lukewarm feelings about where this should live, but the ambiguity around whatv1andv2mean means that I'm unsure whether I want to put the custom data type in this PR into thepkg/apis/configurationpkg.Namewise I should probably have named the
SizeWithUnitandNumberSizeConfigsomething more clever. Help there is appreciated.Tests
Because PRs to non-main and non-release branches don't run CI, I manually ran the unit tests with
make test, those passed.I also manually ran the one failing e2e test with the following after building a fresh image and loading that into minikube:
I'm currently running ALL of the e2e tests on a debian image. That might take a while, so I'll return to it once it finished and update this part of the PR message.
Checklist
Before creating a PR, run through this checklist and mark each as complete.