Skip to content

Migrate to aws-sdk-go-v2 and drop aws-sdk-go v1 - #710

Draft
moustafab wants to merge 16 commits into
mainfrom
moustafab/aws-sdk-v2-stacked
Draft

Migrate to aws-sdk-go-v2 and drop aws-sdk-go v1#710
moustafab wants to merge 16 commits into
mainfrom
moustafab/aws-sdk-v2-stacked

Conversation

@moustafab

@moustafab moustafab commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Note

Stacked on #696 (which is stacked on #695). The base branch is yuri-tseretyan/slog-adapter, so this diff shows only the two commits below. It needs grafana/prometheus-alertmanager#168.

This removes github.com/aws/aws-sdk-go (v1, end of support) from grafana/alerting entirely. v1 carries CVE-2020-8911 and CVE-2020-8912 (GO-2022-0646 / GO-2022-0635, in s3crypto, no fix available). Tracking issue: grafana/alerting-squad#2185.

Commits

  1. Migrate SNS receivers to aws-sdk-go-v2. Ports receivers/sns/v1 (Grafana) and receivers/sns/v0mimir1 (Mimir) from session/credentials/stscreds/sns v1 to config.LoadDefaultConfig, credentials, stscreds.NewAssumeRoleProvider and sns.NewFromConfig. The goal is to keep existing behaviour for credentials, profile, region, assume-role, api_url and the HTTP client:
    • api_url semantics are unchanged. v2 requires a full URL, so withDefaultScheme adds https:// when the scheme is missing, the same way v1's endpoints.AddScheme did.
    • In v0mimir1, the user's http_config client is set only on the SNS client, as before. It's set per service rather than through config.WithHTTPClient, which also avoids the v2 AWS_CA_BUNDLE failure that upstream fixed in alertmanager#5178.
    • RoleSessionName keeps the v1 format.
    • In v0mimir1, HTTP errors still map to the retrier and ErrorWithReason through smithyhttp.ResponseError.
    • Publish now honours the notification context.
    • aws/arn moves to aws-sdk-go-v2/aws/arn.
    • New httptest-based tests cover a signed publish through api_url, a 400 error mapping to not-retryable plus ClientErrorReason, a missing region, and the scheme default.
  2. Bump the alertmanager fork to drop aws-sdk-go v1. Moves the fork to 8b0db09d6c8a (Upgrade SNS notifier to aws-sdk-go-v2 and prometheus/sigv4 prometheus-alertmanager#168) and switches definition/compat to github.com/prometheus/sigv4. The new fork brings some indirect upgrades, e.g. sprig 3.3.0, go-openapi/strfmt 0.26.3 and snappy 1.0.0.

⚠️ Open question: assume-role credential sourcing

This PR deliberately keeps how the existing v1 code chooses credentials for the STS AssumeRole call when sigv4.role_arn is set. That choice depends on whether api_url is set and whether static keys are configured; see the comment in createSNSClient. Upstream alertmanager changed this logic when it migrated to v2 (prometheus/alertmanager#4482), and the fork now follows upstream (#168).

Decision needed before this leaves draft: keep the current behaviour for Grafana-managed SNS contact points, or align with upstream? Either way, it should be its own reviewed change, not something that slips in with the SDK migration. Related: there's no external_id support today (upstream added it in prometheus/alertmanager#5157).

Other behaviour differences in v2 (worth a release note)

  • STS endpoint: v2 always calls the regional sts.<region>.amazonaws.com endpoint. v1 defaulted to the global sts.amazonaws.com.
  • Shared config: v2 always reads ~/.aws/config as well as the credentials file. That means profile region, SSO and credential_process settings now apply. A named profile that doesn't exist now fails when the client is built, not at publish.
  • Environment: AWS_DEFAULT_REGION, AWS_ENDPOINT_URL and AWS_ENDPOINT_URL_SNS are now honoured when the corresponding setting is empty.
  • Error text: errors use the v2/smithy format (operation error SNS: Publish, https response error StatusCode: …). They show up in contact-point test results and notification history.
  • Retries: v2's standard retryer makes 3 attempts in total. v1 made 3 retries, so 4 attempts.

Not done here

use_aws_http_client, which the fork's SNSConfig gained in #168, is not mirrored in v0mimir1. Our notifier sets the HTTP client per service, so the CA-bundle problem it works around doesn't come up. We could still accept the field for Mimir config parity; that's open for discussion.

Verification

  • go build ./... and go vet ./... pass.
  • go test -tags netgo -timeout 30m -race -count 1 ./... passes (58 packages).
  • make lint reports 0 issues, and make mod-check passes.
  • go mod why -m github.com/aws/aws-sdk-go and go mod why -m github.com/prometheus/common/sigv4 both report "main module does not need module".
  • govulncheck no longer reports GO-2022-0646 or GO-2022-0635.

yuri-tceretian and others added 13 commits September 25, 2026 12:45
@yuri-tceretian
Bump grafana/prometheus-alertmanager fork to 27a6d009
d1865e2 
Picks up grafana/prometheus-alertmanager#165, which mirrors upstream's
config package layout (shared types moved to config/common, several
notifier configs moved to notify/<channel>). This module will not
compile until the fallout is fixed in a follow-up commit.

Ref: #694
@yuri-tceretian
Fix fallout from fork's config layout change
732bcf7 
Update qualifiers for types that moved with grafana/prometheus-alertmanager#165:
NotifierConfig, URL, SecretURL, MatchRegexps, Matchers and InhibitRule now
live in config/common; DiscordConfig, WebhookConfig, MSTeamsConfig,
MSTeamsV2Config, JiraConfig, PagerdutyConfig (and PagerdutyImage/Link),
OpsGenieConfig (and OpsGenieConfigResponder) and TelegramConfig now live in
their respective notify/<channel> packages. PagerDuty ServiceKey/RoutingKey,
OpsGenie APIKey and Telegram BotToken are now commoncfg.Secret instead of
the fork's own config.Secret.

No behavior change: same fields, same JSON/YAML output, only import paths
and type qualifiers differ.

Ref: #694
@yuri-tceretian
Add go-kit -> slog logging adapter for fork boundary calls
79004f4 
Bridges a go-kit/log.Logger to *slog.Logger for the boundary where
grafana/alerting will call into the (slog-based) prometheus-alertmanager
fork, without touching the go-kit log lines grafana/grafana and
grafana-alertmanager emit today.

Golden-line tests cover level mapping, Debug filtering, WithAttrs/WithGroup
flattening, and the two known github.com/tjhop/slog-gokit defects (an extra
'time' key and an extra, wrongly-depthed 'caller' key) across three consumer
decoration styles.

Caller parity is not fully solvable from within grafana/alerting: consumers
that bake a fixed-stack-depth caller Valuer into their go-kit logger before
handing it to us (e.g. grafana-alertmanager's InitLogger,
spanlogger.Caller(6|7)) get a wrong caller through this adapter, because the
extra slog call frames shift the depth the Valuer walks, and the same shared
logger instance is also used for direct (non-slog) call sites where the
original depth is correct. TestCallerParity documents the measured gap.
@yuri-tceretian
Bump grafana/prometheus-alertmanager fork to 605b1ec9
1283350 
Fork branch refactor/adopt-slog (grafana/prometheus-alertmanager#167),
which adopts log/slog and drops go-kit/log throughout. Breaks every
exported call into the fork that used to take a go-kit log.Logger;
fixed in the following commit.
@yuri-tceretian
Fix Phase A review findings in the go-kit -> slog adapter
438b93c 
- appendAttr forwarded the slog.Value wrapper instead of its resolved
  underlying value: typed attrs (int, bool, nil, time.Time, structs) came
  out wrong (e.g. quoted strings) through the adapter but not through a
  direct go-kit call. Use a.Value.Any() after resolving.
- Enabled(Debug) only detected DebugEnabled() on the exact logger instance
  passed to NewSlogLogger; wrapping it first with log.With(...) (this
  package's own scoping pattern) hides the method behind an opaque
  *log.context, silently losing detection. Document which of
  grafana-alertmanager and grafana/grafana's logger constructions can be
  detected at all, and add WithDebugEnabled so a caller that wraps before
  handing off can supply the answer explicitly.
- Rebuilt the golden tests: real ordered logfmt decoding (go-logfmt/logfmt)
  instead of an unordered map, type-aware JSON assertions, every level
  (not just Info), typed values, slog.LogValuer, nested/inline groups,
  independent With branches, and fixtures that mirror each consumer's
  actual construction (including grafana-alertmanager's own level filter).
  TestCallerParity now pins the actual observed caller value per path
  instead of only asserting the two differ, which could pass for the
  wrong reason.
@yuri-tceretian
Wrap loggers at every grafana/alerting call into the fork
0f18d2c 
The fork (bumped in the previous commit) now takes *slog.Logger at its
public boundaries. grafana/alerting's own public API stays on go-kit's
log.Logger (GrafanaAlertmanagerOpts.Logger, v1 receiver constructors,
TestTemplate, NewGrafanaAlertmanagerMetrics); every call site that reaches
into the fork now wraps with logging.NewSlogLogger before crossing that
boundary. Found by the compiler, not by grepping:

- notify/grafana_alertmanager.go: compat.InitFromFlags, nflog.New,
  flushlog.New, mem.NewAlerts, inhibit.NewInhibitor, silence.NewSilencer,
  dispatch.NewDispatcher, notify.GetTemplateData (in TestTemplate), via two
  new helpers, forkLogger/forkLoggerRaw, matching the two existing
  raw/scoped go-kit logger shapes this file already used.
- notify/grafana_alertmanager_metrics.go: metrics.NewAlerts.
- notify/stages/wait_stage.go: WaitStage.Exec, which implements
  notify.Stage -- its logger parameter is now *slog.Logger like every
  other Stage implementation upstream converted.
- templates/template_data.go and every v0mimir1/v0mimir2 receiver's
  Notify(): notify.GetTemplateData. The receivers themselves (New(),
  n.logger) stay on go-kit -- only this one call crosses into the fork.
- cluster/cluster.go: Create was a bare re-export, so its signature would
  have silently changed along with the fork's; grafana/grafana calls this
  with a go-kit logger (pkg/services/ngalert/notifier/multiorg_alertmanager.go),
  so it's now a real wrapper function that keeps the go-kit signature and
  adapts internally. Audited every other var/type alias in cluster/ and
  the rest of the repo for the same problem -- nothing else re-exports a
  logger-carrying fork signature. config.NewCoordinator is unused in this
  repo, so it needed no change.

Checked with the fork's compiler errors, not by inspection: after this
commit, go build/vet is clean (root module and apps/historian, in and out
of workspace mode) and go test -race passes, except the pre-existing
apps/historian/cmd/historian-operator TestMain_OperatorComesUp race that
also fails on main.
@yuri-tceretian
Add opt-in caller derived from slog.Record.PC (option b+d)
5a6ddf7 
Commander decision, 2026-09-25: for the caller-parity gap documented in
slog-caller-report.md, add GrafanaAlertmanagerOpts.LoggerWithoutCaller, an
optional go-kit logger equivalent to Logger but without its own "caller"
field, used only for the loggers handed into the fork. The
grafana-alertmanager side (building and passing that logger) is a later PR,
not in this repo.

- logging.WithCaller(): when set, the adapter adds exactly one "caller"
  field per record, in go-kit's own basename:line format, derived from
  slog.Record.PC -- the call site slog itself captured, correct regardless
  of how many extra frames the adapter or slog add on top (unlike a go-kit
  Valuer's fixed-skip stack walk). Must only be paired with a logger that
  doesn't already carry a "caller" field; the adapter cannot detect or
  strip one already baked into an opaque go-kit log.Logger, and combining
  the two produces a visible duplicate (logfmt) or a silent overwrite
  (JSON, which deduplicates keys, last write wins) -- documented on the
  option and exercised by TestOptInCaller_NeverDoubledWithExistingCaller.
- GrafanaAlertmanager.forkLogger/forkLoggerRaw (added wrapping the fork
  boundary in the previous commit) now check LoggerWithoutCaller: set, they
  wrap it with WithCaller(); unset, behavior is unchanged from the previous
  commit -- am.logger/opts.Logger wrapped with no caller added, so
  grafana/grafana's output (which never opts in) stays byte-identical.
- Golden tests: a grafana-alertmanager-style logger (ts baked in) minus its
  own caller field, with the option set, gets the real call line exactly
  once; a grafana/grafana-style logger, option unset, gets no caller key at
  all; and the same pair through GrafanaAlertmanager.forkLogger/
  forkLoggerRaw directly.
@yuri-tceretian
Add adapter benchmark
de7cc00 
Per log line, ns/op and allocs/op for: direct go-kit, the adapter with no
options, the adapter with WithCaller() (option b+d, replaces what would
otherwise have been a prototype -- the real option landed in the previous
commit), and the adapter with debug disabled while logging at debug (using
a grafana-alertmanager-shaped logger, i.e. one DebugEnabled() can actually
be detected on, so this measures the Enabled() fast path rather than
building-then-dropping). Realistic call: a message plus 6 key/values,
logfmt writer to io.Discard.

linux/amd64, Intel Core Ultra 9 185H, go1.26.4:

  BenchmarkLogLine_DirectGoKit-6            988.8 ns/op    372 B/op    4 allocs/op
  BenchmarkLogLine_Adapter-6                1923 ns/op     865 B/op   18 allocs/op
  BenchmarkLogLine_AdapterWithCaller-6      2202 ns/op    1185 B/op   22 allocs/op
  BenchmarkLogLine_AdapterDebugDisabled-6   6.196 ns/op      0 B/op    0 allocs/op

The adapter costs roughly 2x direct go-kit for the enabled-level path
(slog's own Record/Attr machinery plus our flattening), and WithCaller()
adds a further ~15% for the runtime.CallersFrames lookup. When the level
is actually disabled and DebugEnabled() can be detected, Enabled() skips
building the record entirely -- effectively free, several orders of
magnitude cheaper than either enabled path.
@yuri-tceretian
Fix review findings on the go-kit -> slog adapter and opt-in caller
54f738f 
- Handle() emitted msg before preformatted (WithAttrs-bound) attrs, the
  reverse of go-kit's own log.With ordering ("With returns a new
  contextual logger with keyvals prepended to those passed to calls to
  Log"). A real fork call shaped as
  log.With(am.logger, "aggrGroup", g, "group_fingerprint", f).Log("msg", m, "alerts", a)
  emits aggrGroup/group_fingerprint before msg; the adapter emitted msg
  first. Swap the order.
- forkLogger/forkLoggerRaw built a scoped go-kit logger via log.With(...)
  and adapted that, losing DebugEnabled() detection on the way (the same
  opaque-wrapping problem NewSlogLogger's own doc comment describes) even
  though the pre-scoping logger (opts.Logger / opts.LoggerWithoutCaller)
  supports it. Probe DebugEnabled() there, before scoping, and forward it
  via WithDebugEnabled.
- Golden-test fixture gaLevelFilter only embedded log.Logger without
  overriding Log(), so it was one stack frame shallower than
  grafana-alertmanager's real levelFilter (which overrides Log to
  intercept a private probe value) -- not faithful for caller-depth
  measurements. Added the override.
- TestCallerParity_GrafanaAlertmanagerStyle: rebuilt using the corrected
  fixture plus the "component"/tenant log.With scoping grafana/alerting's
  own am.logger applies in production, and replaced every hardcoded
  expected-line-number constant (here and in
  TestOptInCaller_GrafanaAlertmanagerStyleMinusCaller and
  TestGrafanaAlertmanager_forkLogger) with a nextLine(t) helper
  (runtime.Caller-based) so edits can't silently go stale. With the
  faithful fixture shape, the direct path now resolves to the real call
  site (it didn't before); the adapter path is asserted to land inside our
  own adapter file, not pinned to a specific internal file:line (compiler-
  and Go-version-sensitive, and not the point being tested).
- Golden line comparisons stripped ts/t/caller without checking they were
  even there: added assertTimeAndCallerShape, checked for every
  fixture/level/path, asserting presence exactly once and RFC3339Nano
  format for the timestamp key each consumer actually binds (ts or t), and
  that plain-logfmt/grafana-grafana (which never bind a caller in these
  tests) don't have one.

Same checks as before: go build/vet clean (root module and apps/historian),
make lint 0 issues, make mod-check clean, go test -tags netgo -race
-count 1 ./... passes in both modules except the pre-existing
apps/historian/cmd/historian-operator TestMain_OperatorComesUp race.
@yuri-tceretian
Add permanent regression tests for bound-attribute field order
2e92d38 
Requested by review: the committed suite as of 54f738f could not actually
catch the field-order bug it fixed -- every committed test still passed
after temporarily reverting Handle() to put "msg" before h.preformatted
again (verified locally, then restored; not a permanent change).

- TestGoldenLines_BoundAttributeOrder_Logfmt: two binding layers -- a
  go-kit-level one (component/orgID, on the underlying logger itself,
  applied identically on both paths and not exercising the bug) and a
  slog-level one via our own WithAttrs (aggrGroup/group_fingerprint,
  mirroring dispatch.go's aggrGroup flush line), then a message plus
  event attrs (alerts). Compares direct vs adapter output as an ordered
  slice, which catches a transposed msg/aggrGroup pair, not just a
  missing or extra field. Verified this fails on the reverted code and
  passes on the fix.
- TestGoldenLines_BoundMsgOverride_JSON: a bound "msg" (log.With(l, "msg",
  ...)) combined with the event's own msg at call time. go-kit's JSON
  logger dedupes by last-write-wins, so the event message must win in
  both paths; asserts that directly instead of relying on a duplicate-key
  count, which JSON's dedup would hide.

Same checks: go build/vet clean, make lint 0 issues, make mod-check
clean, go test -tags netgo -race -count 1 ./logging/... passes (only
logging/ touched).
@yuri-tceretian
Fix TestGoldenLines_BoundMsgOverride_JSON to bind msg at the slog level
df00d1c 
Review found it bound "msg" on the underlying go-kit logger
(log.With(newJSONLogger(...), "msg", "bound-msg")), leaving
h.preformatted empty -- the test still passed with Handle() reverted to
put "msg" before h.preformatted, same as the logfmt test's original gap.
Bind it via our own WithAttrs instead
(NewSlogLogger(...).With("msg", "bound-msg")), matching
TestGoldenLines_BoundAttributeOrder_Logfmt's two-binding-layer approach.

Verified (then restored) that this version does fail under the reverted
ordering: bound-msg wins instead of event-msg.
@moustafab
Migrate SNS receivers to aws-sdk-go-v2
8d4fd25 
Port receivers/sns/v1 and receivers/sns/v0mimir1 from aws-sdk-go v1 to
aws-sdk-go-v2, keeping the existing credential, assume-role, endpoint and
HTTP client behaviour. Publish now honours the notification context.

aws-sdk-go v1 remains an indirect dependency through
prometheus/common/sigv4, which the alertmanager fork's SNSConfig uses.
@moustafab
Bump alertmanager fork to drop aws-sdk-go v1
98564a9 
Bump grafana/prometheus-alertmanager to 8b0db09d6c8a (branch
moustafab/aws-sdk-go-v2), which moves the SNS notifier to
aws-sdk-go-v2 and github.com/prometheus/sigv4. Switch the SNS compat
conversion to github.com/prometheus/sigv4 accordingly.

github.com/aws/aws-sdk-go and github.com/prometheus/common/sigv4 are no
longer in the module graph.
@moustafab moustafab mentioned this pull request Oct 7, 2026
Merged
@yuri-tceretian
yuri-tceretian force-pushed the yuri-tseretyan/slog-adapter branch 2 times, most recently from d30e09b to de78442 Compare October 7, 2026 18:43
@yuri-tceretian
chore(logging): adapt go-kit loggers to Alertmanager slog API
28c6a84 
Bridge go-kit logging at fork call sites. Add adapter tests and benchmarks, and pin both modules to the fork main revision that provides the slog API. Keep caller reporting available to users of logging.WithCaller().
@yuri-tceretian
yuri-tceretian force-pushed the yuri-tseretyan/slog-adapter branch from de78442 to 28c6a84 Compare October 7, 2026 19:17
@moustafab
Merge remote-tracking branch 'origin/main' into moustafab/aws-sdk-v2-…
5ef54b9 
…stacked

# Conflicts:
#	apps/historian/go.mod
#	apps/historian/go.sum
#	go.mod
#	go.sum
@moustafab
Merge remote-tracking branch 'origin/yuri-tseretyan/slog-adapter' int…
7ba89f7 
…o moustafab/aws-sdk-v2-stacked

# Conflicts:
#	apps/historian/go.mod
#	apps/historian/go.sum
#	go.mod
#	go.sum
#	go.work.sum
#	logging/slog_test.go
#	notify/grafana_alertmanager.go
#	notify/grafana_alertmanager_test.go
@titolins
titolins marked this pull request as ready for review October 14, 2026 08:30
@titolins
titolins requested a review from a team as a code owner October 14, 2026 08:30
konstantinmv
konstantinmv previously approved these changes Oct 14, 2026
View reviewed changes
@titolins
titolins marked this pull request as draft October 14, 2026 13:59
@titolins

titolins commented Oct 14, 2026

Copy link
Copy Markdown
Collaborator

I was planning to try and deliver this but as talked about directly we need to discuss a few things. Moved back to draft for now

Base automatically changed from yuri-tseretyan/slog-adapter to main October 14, 2026 14:45
@yuri-tceretian
yuri-tceretian dismissed konstantinmv’s stale review October 14, 2026 14:45

The base branch was changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@konstantinmv konstantinmv konstantinmv left review comments
@yuri-tceretian yuri-tceretian Awaiting requested review from yuri-tceretian

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@moustafab @titolins @konstantinmv @yuri-tceretian