feature: switch AWS Endpoints for European Souvereign Cloud #11356
Conversation
📝 WalkthroughWalkthroughEndpoint construction now selects a domain suffix dynamically: standard ".amazonaws.com", China ".amazonaws.com.cn" for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/aws/flb_aws_util.c (1)
🔇 Additional comments (5)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/aws/flb_aws_util.c (2)
86-93: Region-to-suffix mapping correctly implements EU Sovereign Cloud support.The logic properly routes:
- China regions (cn-north-1, cn-northwest-1) →
.amazonaws.com.cn- EU Sovereign Cloud (eusc-de-east-1) →
.amazonaws.eu- All other regions →
.amazonaws.com(default)💡 Optional: Consider a lookup table for future scalability
If additional EU Sovereign Cloud regions are added in the future, a lookup table or pattern-based approach could improve maintainability:
+static const char* get_domain_suffix(const char* region) { + if (strcmp(region, "cn-north-1") == 0 || strcmp(region, "cn-northwest-1") == 0) { + return AWS_SERVICE_ENDPOINT_SUFFIX_COM_CN; + } + if (strncmp(region, "eusc-", 5) == 0) { /* Match all eusc-* regions */ + return AWS_SERVICE_ENDPOINT_SUFFIX_EU; + } + return AWS_SERVICE_ENDPOINT_SUFFIX_COM; +}However, the current hardcoded approach is acceptable for the known regions.
107-112: Consider checking for snprintf truncation.The current error handling only checks for encoding errors (
bytes < 0). While truncation shouldn't occur due to the correct length calculation, defensive coding would also verify thatbytes < lento catch any unexpected truncation.🛡️ More robust snprintf error handling
- bytes = snprintf(endpoint, len, AWS_SERVICE_ENDPOINT_FORMAT, service, region, domain_suffix); - if (bytes < 0) { + bytes = snprintf(endpoint, len, AWS_SERVICE_ENDPOINT_FORMAT, service, region, domain_suffix); + if (bytes < 0 || bytes >= len) { flb_errno(); flb_free(endpoint); return NULL; }This guards against both encoding errors and unexpected truncation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/aws/flb_aws_util.ctests/internal/aws_util.c
🧰 Additional context used
🧬 Code graph analysis (2)
src/aws/flb_aws_util.c (1)
include/fluent-bit/flb_mem.h (1)
flb_calloc(84-96)
tests/internal/aws_util.c (2)
src/aws/flb_aws_util.c (1)
flb_aws_endpoint(78-116)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
🔇 Additional comments (3)
src/aws/flb_aws_util.c (2)
36-39: LGTM! Clear parameterization of endpoint format.The new constants cleanly separate the endpoint format from domain suffixes, enabling support for standard (.com), China (.com.cn), and EU Sovereign Cloud (.eu) regions.
95-99: LGTM! Length calculation is accurate.The calculation correctly accounts for all components in the format
"%s.%s%s":
- Service name length
- Dot separator (1 byte)
- Region name length
- Domain suffix length
- Null terminator (1 byte)
tests/internal/aws_util.c (1)
166-172: LGTM! Test case properly validates EU Sovereign Cloud endpoint.The test correctly verifies that region
eusc-de-east-1produces the endpointcloudwatch.eusc-de-east-1.amazonaws.eu, directly addressing the issue described in #1057. Memory management and test structure follow the established pattern.
Signed-off-by: Stephan <Stephan.Telahr@bertelsmann.de>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/aws/flb_aws_util.c (1)
91-93: Consider prefix-based matching for EU Sovereign Cloud regions.The current implementation correctly handles the
eusc-de-east-1region as specified in the issue. However, if AWS introduces additional EU Sovereign Cloud regions (which would likely follow theeusc-*naming pattern), each would require a code change.♻️ Proposed refactor for scalability
- else if (strcmp("eusc-de-east-1", region) == 0) { + else if (strncmp(region, "eusc-", 5) == 0) { domain_suffix = AWS_SERVICE_ENDPOINT_SUFFIX_EU; }This approach would automatically support any future EU Sovereign Cloud regions without requiring code changes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/aws/flb_aws_util.ctests/internal/aws_util.c
🧰 Additional context used
🧬 Code graph analysis (2)
src/aws/flb_aws_util.c (1)
include/fluent-bit/flb_mem.h (1)
flb_calloc(84-96)
tests/internal/aws_util.c (2)
src/aws/flb_aws_util.c (1)
flb_aws_endpoint(78-116)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
🔇 Additional comments (7)
src/aws/flb_aws_util.c (6)
36-39: LGTM! Clean constant definitions for domain suffixes.The introduction of separate domain suffix constants and the updated format string enables flexible endpoint construction for different AWS partitions (standard, China, EU Sovereign Cloud).
76-77: LGTM! Comment accurately reflects the updated endpoint format.
87-90: LGTM! China region handling is correct and complete.The code properly handles both China regions (cn-north-1 and cn-northwest-1) by using the
.amazonaws.com.cndomain suffix.
95-99: LGTM! Length calculation is correct and clear.The calculation properly accounts for all endpoint components (service, dot separator, region, domain suffix, and null terminator).
101-105: LGTM! Memory allocation and error handling are correct.
107-112: LGTM! Endpoint construction and error handling are correct.The
snprintfcall properly formats the endpoint using the selected domain suffix, and error handling ensures memory is freed on failure.tests/internal/aws_util.c (1)
166-171: LGTM! Test coverage for EU Sovereign Cloud endpoint is excellent.The new test case properly validates that the
eusc-de-east-1region generates the correct endpoint with the.amazonaws.eudomain suffix, as required by issue #1057. The test follows the established pattern and includes proper memory management.
Signed-off-by: Stephan <Stephan.Telahr@bertelsmann.de>
1 participant
Use European Souvereign Cloud specific Endpoints if neccessary
Fixes: aws/aws-for-fluent-bit#1057
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.
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
New Features
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.