Repository navigation
AWS SDK for Go v2 migration followups - #2309
Merged
Merged
Conversation
The pusher only retried PutLogEvents failures that satisfied smithy.APIError and dropped every other error after a single attempt, advancing the state-file offset so the events were never resent. With AWS SDK for Go v1, every SDK error implemented awserr.Error, so network failures (RequestError), credential failures (NoCredentialProviders) and (de)serialization failures all passed that gate and were retried for up to maxRetryTimeout. With SDK v2 these surface as *smithy.OperationError wrapping *smithyhttp.RequestSendError, an untyped credential error, or *smithy.DeserializationError, none of which implement smithy.APIError. Connection refused/reset, DNS and TLS failures, client timeouts, credential refresh failures and malformed 5xx bodies therefore caused permanent log loss after roughly one to three seconds. Invert the decision: retry every error except those that can never succeed. Terminal errors are InvalidParameterException, DataAlreadyAcceptedException, smithy.InvalidParamsError (client-side validation) and smithy.SerializationError (request marshalling), all matched by type. Everything else falls through to the existing bounded retry loop, which also makes the long backoff for connection errors in retry.go reachable again. Add sender tests that feed realistic SDK v2 error chains for transport, credential and deserialization failures and assert they are retried, and tests that the two client-side terminal errors are dropped. Retarget the queue no-resend test at a terminal error; it previously asserted the buggy behaviour with a generic error.
SDK v1 prepended "https://" to a custom endpoint that had no URL scheme (endpoints.AddScheme), so the documented form "vpce-xxx.logs.us-east-1.vpce.amazonaws.com" worked. SDK v2 assigns the value to BaseEndpoint and parses it as-is; a bare hostname becomes a URL path and every request fails with `unsupported protocol scheme ""`. Normalize the value once in the translator, at every site that reads endpoint_override from the JSON config: the two TOML rules that feed plugins/outputs/cloudwatch and cloudwatchlogs, and the OTel translators for awscloudwatch, awscloudwatchlogs, awsemf, awsxray, journald, the awsproxy extension and the awsxray receiver. This also fixes the forked exporters, which set BaseEndpoint from the same string without normalization, without needing a change in the fork. A value that already carries a scheme is preserved, so an explicit http:// endpoint keeps working. Whitespace is trimmed; ports, paths and trailing slashes are left as-is. Add unit tests for the helper and the OTel translators, and switch the log_ecs_metric_only golden config to the documented scheme-less form so the TOML and YAML outputs lock in the behavior.
SDK v1 tolerated a named shared-config profile that did not exist and still resolved the region from AWS_REGION/AWS_DEFAULT_REGION, and it accepted a credentials-style "[name]" section from any shared file. SDK v2 differs on both: naming a profile selects a strict loader whose missing-profile error makes LoadDefaultConfig return a zero config, and a file passed via WithSharedConfigFiles has its bare "[name]" sections dropped. SDKRegionWithCredsMap passed the operator's credentials file in the config slot and returned "" on any error, so on-prem hosts failed with "Region info is missing" and EC2 hosts using a named profile silently fell back to the instance region. - Add cfg/aws.SharedConfigRegion: environment region first, then the profile's region read with LoadSharedConfigProfile from explicit credentials/config files. A missing profile yields "" instead of an error. Used by the translator and the wizard's SDKRegionWithProfile. - SDKRegionWithCredsMap passes the credentials file in the credentials slot and only the sibling config in the config slot. This also stops the SDK from filling the credentials slot with ~/.aws/credentials, a file the operator did not configure. - DefaultEC2Region (wizard) builds an IMDS-only client instead of a credential-backed config, removing an IMDS credential probe and a 15s retry sleep that could misreport an EC2 host as on-premises. - Add tests against the real resolution path, which every existing test stubbed out via util.DetectRegion. Deliberate differences from SDK v1: an environment region is returned even when the shared file is broken; when a profile sets a region in both files, the credentials file wins (SDK v2 merge order); a profile with partial static credentials yields no region and a warning.
CheckAndSetHomeDir sets HOME/USERPROFILE so the shared credentials and config files resolve under the intended home (C:\Users\Administrator on Windows, the real user's home elsewhere) rather than the service account's. SDK v1 computed the default file paths when a session was created and saw the change. SDK v2 computes DefaultSharedCredentialsFiles and DefaultSharedConfigFiles once at package init, before main runs, so the change was ignored and a profile-only common-config on such a host could not find its region. Resolve the default paths in SharedConfigRegion at call time using the SDK's DefaultShared*Filename functions, which read the environment when called. Explicit file lists and the AWS_SHARED_CREDENTIALS_FILE / AWS_CONFIG_FILE variables keep precedence.
SDK v1's MaxRetries counted retries after the first attempt; SDK v2's MaxAttempts counts every attempt. The value 3 was carried over unchanged, so the CloudWatch metrics output, CloudWatch Logs clients, and ECS service discovery clients made 3 HTTP attempts per call instead of 4. Apply the same +1 the forked contrib already uses in awsutil and the IMDS retryer so both halves keep the v1 contract.
sky333999
changed the title
sky333999
added
the
ready for testing
The lint job runs golangci-lint with new-from-rev, so it flagged four issues in lines this branch added: - cfg/aws/region_test.go built the shared-credentials path by reading the env var back out of the environment, which gosec flags as a tainted path (G703). The path now comes from new testutil.SharedCredentialsPath/SharedConfigPath helpers, which also removes the duplicated file names and the test's dependence on the isolation helper's internals. - The awscloudwatchlogs test fixture map contains a shared_credentials_file path, which gosec reads as a hardcoded credential (G101). Suppressed with a reason; the value is a path. - Two awsemf selectors spelled out the embedded AWSSessionSettings field, which staticcheck flags as removable (QF1008). golangci-lint run ./... reports 0 issues.
lorespiz
approved these changes
Sep 28, 2026
Contributor
Binary Size Reportlinux/amd64
Notable changes:
linux/arm64
windows/amd64
Investigating size changesUse go-size-analyzer to compare binaries: GOEXPERIMENT=jsonv2 go install github.com/Zxilly/go-size-analyzer/cmd/gsa@latest
gsa diff --old <baseline-binary> --new <new-binary> |
EduVencovsky
approved these changes
Sep 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of the issue
Followups from the AWS SDK for Go v2 migration (#2222).
Log batches are discarded on transport and credential errors. The log sender retried only errors
it recognized as CloudWatch Logs API errors. Under SDK v2 a connection failure, DNS failure, TLS error,
client timeout, or credential refresh failure is not an API error, so the batch was dropped and the file
offset advanced past it. SDK v1 retried these.
endpoint_overriderequires an explicit scheme. SDK v1 accepted a bare host such aslogs.us-east-1.amazonaws.com; SDK v2 fails endpoint resolution withunsupported protocol scheme "".Documented examples are scheme-less.
On-premises region resolution fails. The shared credentials file was passed in the shared config
file slot, and a named-but-missing profile was treated as fatal. A profile-only on-premises setup
resolved no region, and the agent exited with
Region info is missing.EC2 hosts can publish to the wrong region. With the same resolution failure, the EC2 path fell
through to instance metadata, so a host configured for one region published to the region it was running
in.
CheckAndSetHomeDirdoes not affect SDK default file paths. SDK v2 computes its default sharedcredentials and config paths at package initialization; the agent sets
HOMEafterwards. Reachable onWindows running as
SYSTEMwith a profile-only configuration.Configuration wizard selects the wrong default region and delays ~15 seconds. Same resolution
root cause, plus a credential probe and sleep on the EC2 path.
Retry-configured clients make one fewer attempt. SDK v1's
MaxRetriescounted retries after thefirst attempt; SDK v2's
MaxAttemptscounts every attempt. The value3was carried over unchanged,reducing the CloudWatch metrics output, the CloudWatch Logs clients, and the ECS service discovery
clients from 4 attempts to 3.
Description of changes
One commit per issue.
fix(cloudwatchlogs): Retry log batches on non-API errors(1) —plugins/outputs/cloudwatchlogs/internal/pusher/sender.go: replaced the allowlist of retryable errorswith a denylist of terminal ones (
InvalidParameterException,DataAlreadyAcceptedException,client-side parameter validation, serialization failures). All other errors retry through the sender's
existing bounded retry loop.
fix(translator): Add https:// to scheme-less endpoint_override(2) — newNormalizeEndpointURLand
GetEndpointOverrideintranslator/translate/otel/common/endpoints.go, applied at all nine sitesthat read
endpoint_override. An explicitly specified scheme is preserved; otherwisehttps://isprepended. Normalizing at the read site means the forked exporter components receive a valid value and
need no change. One golden sample config was switched to the scheme-less form to exercise the path.
fix(region): Restore env and shared-file region resolution(3, 4, 6) — newcfg/aws/region.gowith
SharedConfigRegion, which resolves the region from the environment first, then from the namedprofile with credentials and config files in their correct slots. A missing profile yields an empty
region instead of an error, so callers can still fall back to instance metadata.
translator/utilandtool/utilnow use it, andDefaultEC2Regionqueries instance metadata directly, removing thecredential probe and 15-second sleep.
fix(region): Honor CheckAndSetHomeDir for SDK default shared files(5) — when no explicit filelist is supplied, resolve defaults through
config.DefaultSharedCredentialsFilename()andconfig.DefaultSharedConfigFilename(), which read the environment at call time. Explicit file lists andAWS_SHARED_CREDENTIALS_FILE/AWS_CONFIG_FILEkeep precedence.fix(retryer): Restore SDK v1 attempt count for agent-built clients(7) —internal/retryer/logthrottle.goandinternal/ecsservicediscovery/servicediscovery.goadd one whensetting
MaxAttempts, and the constants document their unit. This matches the conversion the forkedcontrib already applies in
awsutiland its IMDS retryer.License
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this
contribution, under the terms of your choice.
Tests
Each new test was confirmed to fail against the unfixed code by reverting only the source file.
sender_test.gousing realistic SDK v2 error chains (connection refused,DNS failure, TLS error, client timeout, credential refresh failure) plus the terminal cases;
queue_test.goasserts only terminal errors stop the resend. Verified under-race.endpoints_test.gocovers empty, scheme-less,https://,http://, other schemes, andsurrounding whitespace. Each of the nine call sites asserts the resulting endpoint value.
cfg/aws/region_test.go, plus tests intranslator/utilandtool/utilthat exercise the real resolution path rather than the stub that previously replaced it. Ashared helper isolates the AWS shared-config environment, since an existing helper overwrites
HOME.clients.
Requirements
make fmt/make fmt-sh—gofmt -lreports nothing on the changed files.make lint—golangci-lint run ./...reports 0 issues.