aws: update support for additional regions#11733
aws: update support for additional regions#11733ShelbyZ wants to merge 2 commits intofluent:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaced bespoke region checks with a static prefix→suffix lookup table to determine AWS endpoint domain suffixes; Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb4c04712e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aws/flb_aws_util.c (1)
573-590:⚠️ Potential issue | 🟠 MajorDetach or destroy the HTTP client before releasing its connection.
Line 578 releases
u_connwhile returningcwithout first detaching it, and the error path releasesu_connbefore destroyingc. Sinceflb_http_client_detach_connection()restores the connection’s original network setup, keep that cleanup while the request still owns the connection.Proposed lifecycle-ordering fix
if (ret != 0 && c != NULL) { flb_http_client_destroy(c); c = NULL; } + if (c != NULL) { + flb_http_client_detach_connection(c); + } + flb_upstream_conn_release(u_conn); flb_sds_destroy(signature); return c; error: - if (u_conn) { - flb_upstream_conn_release(u_conn); - } if (signature) { flb_sds_destroy(signature); } if (c) { flb_http_client_destroy(c); } + if (u_conn) { + flb_upstream_conn_release(u_conn); + } return NULL; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aws/flb_aws_util.c` around lines 573 - 590, Return and error paths currently release the upstream connection (u_conn) before detaching or destroying the HTTP client (c); call flb_http_client_detach_connection(c) if available or flb_http_client_destroy(c) while the client still owns the connection, then flb_upstream_conn_release(u_conn), and finally clean up signature (flb_sds_destroy(signature)) before returning; ensure both the normal-return branch (when ret != 0) and the error: branch follow this order and null out c/u_conn after release to avoid double-free/use-after-free.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/aws/flb_aws_util.c`:
- Around line 573-590: Return and error paths currently release the upstream
connection (u_conn) before detaching or destroying the HTTP client (c); call
flb_http_client_detach_connection(c) if available or flb_http_client_destroy(c)
while the client still owns the connection, then
flb_upstream_conn_release(u_conn), and finally clean up signature
(flb_sds_destroy(signature)) before returning; ensure both the normal-return
branch (when ret != 0) and the error: branch follow this order and null out
c/u_conn after release to avoid double-free/use-after-free.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0978ee13-fb38-4b71-b541-2a7b5efabe01
📒 Files selected for processing (2)
src/aws/flb_aws_util.ctests/internal/aws_util.c
- add new endpoint suffixes - use mapping between region prefix to endpoint suffix - update flb_aws_endpoint to use new mapping Signed-off-by: Shelby Hagman <shelbyzh@amazon.com>
- update test_flb_aws_endpoint to test additional regions Signed-off-by: Shelby Hagman <shelbyzh@amazon.com>
1 participant
Summary
AWS region mapping handles existing regions, China, GovCloud, and recently was updated to support EU sovereign cloud, but lacks dedicated cloud. This set of changes aims to update the existing regions supported to cover a wider selection by adding missing mappings. Customers can currently override the
ENDPOINTparameter to work around this limitation, but it would be nicer to just handle it based on the suppliedREGIONparameter.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
fluent/fluent-bit-docs#2552
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
Bug Fixes
New Features
Tests