Fix whole upstream group in nginx1.27.3 #322
Conversation
u5surf
force-pushed
the
fix_whole_ug
branch
2 times, most recently
from
e75ef65 to
4866f2c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…checking - Replace hardcoded constants 3 and 4 with named constants - Add NGX_HTTP_VHOST_TRAFFIC_STATUS_UPSTREAM_PREFIX_LEN and NGX_HTTP_VHOST_TRAFFIC_STATUS_UPSTREAM_KEY_LEN - Add bounds checking before accessing vtsn->data to prevent buffer overflow - Fix indentation and syntax issues in ngx_http_vhost_traffic_status_display_ug_host function 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes upstream dynamic DNS resolution metrics for nginx version 1.27.3 and later, ensuring that metrics for previously resolved peers are properly output when using the NGX_HTTP_UPSTREAM_MODIFY flag.
- Adds support for handling upstream groups with dynamic DNS resolution capabilities
- Implements tree traversal to collect metrics from previously resolved peers
- Adds conditional compilation for nginx 1.27.3+ with upstream modification support
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/ngx_http_vhost_traffic_status_display_json.h | Adds constants for upstream prefix and key length calculations |
| src/ngx_http_vhost_traffic_status_display_json.c | Implements new logic for handling dynamic DNS upstream groups and adds recursive tree traversal function |
| "}" \ | ||
| "}," | ||
|
|
||
| #if (NGX_HTTP_CACHE) |
There was a problem hiding this comment.
Magic number 3 should be documented or replaced with a more descriptive calculation to explain what this prefix length represents.
| #if (NGX_HTTP_CACHE) | |
| #if (NGX_HTTP_CACHE) | |
| // The prefix length for upstream traffic status keys. This value is derived | |
| // from the fixed format of upstream keys, which use a 3-character prefix. |
Copilot uses AI. Check for mistakes.
| #define NGX_HTTP_VHOST_TRAFFIC_STATUS_UPSTREAM_KEY_LEN 4 | ||
|
|
There was a problem hiding this comment.
Magic number 4 should be documented or replaced with a more descriptive calculation to explain what this key length represents.
| #define NGX_HTTP_VHOST_TRAFFIC_STATUS_UPSTREAM_KEY_LEN 4 | |
| // The length of the upstream key used in traffic status calculations. | |
| #define NGX_HTTP_VHOST_TRAFFIC_STATUS_UPSTREAM_KEY_LENGTH 4 |
Copilot uses AI. Check for mistakes.
| ngx_http_upstream_rr_peers_unlock(peers); | ||
| } | ||
| } | ||
| goto last; |
There was a problem hiding this comment.
[nitpick] Using goto statements can make code harder to follow. Consider restructuring the logic to avoid the goto by using early returns or restructuring the conditional blocks.
Copilot uses AI. Check for mistakes.
| usn.fail_timeout = 0; | ||
| usn.backup = 0; | ||
| usn.down = 0; | ||
| while (peers != NULL) { |
There was a problem hiding this comment.
The nested loop structure with peers traversal inside the tree traversal could lead to O(n²) complexity. Consider optimizing by caching peer lookups or restructuring the algorithm.
Copilot uses AI. Check for mistakes.
| rc = ngx_memn2cmp(peer->name.data, usn.name.data, peer->name.len, (size_t) usn.name.len); | ||
| if (rc == 0) { | ||
| usn.weight = peer->weight; | ||
| usn.max_fails = peer->max_fails; | ||
| usn.fail_timeout = peer->fail_timeout; | ||
| usn.backup = 0; | ||
| usn.down = (peer->fails >= peer->max_fails || peer->down); | ||
| } | ||
| } |
There was a problem hiding this comment.
The memory comparison uses different lengths (peer->name.len vs usn.name.len) which could lead to incorrect matches when one name is a prefix of another. Both lengths should be equal for a valid match.
| rc = ngx_memn2cmp(peer->name.data, usn.name.data, peer->name.len, (size_t) usn.name.len); | |
| if (rc == 0) { | |
| usn.weight = peer->weight; | |
| usn.max_fails = peer->max_fails; | |
| usn.fail_timeout = peer->fail_timeout; | |
| usn.backup = 0; | |
| usn.down = (peer->fails >= peer->max_fails || peer->down); | |
| } | |
| } | |
| if (peer->name.len == usn.name.len) { | |
| rc = ngx_memn2cmp(peer->name.data, usn.name.data, peer->name.len, (size_t) usn.name.len); | |
| if (rc == 0) { | |
| usn.weight = peer->weight; | |
| usn.max_fails = peer->max_fails; | |
| usn.fail_timeout = peer->fail_timeout; | |
| usn.backup = 0; | |
| usn.down = (peer->fails >= peer->max_fails || peer->down); | |
| } | |
| } // End of length check |
Copilot uses AI. Check for mistakes.
#318
Fix upstream dynamic DNS revolution which has supported from nginx 1.27.3 cannot output in the past resolved peer’s metrics