Skip to content

Fail the AWS Kinesis input on unrecoverable AWS authorization denials (7.1) - #27261

Open
patrickmann wants to merge 3 commits into
7.1from
backport-7.1/fix/kinesis-input-fail-on-aws-authorization-denial
Open

Fail the AWS Kinesis input on unrecoverable AWS authorization denials (7.1)#27261
patrickmann wants to merge 3 commits into
7.1from
backport-7.1/fix/kinesis-input-fail-on-aws-authorization-denial

Conversation

@patrickmann

@patrickmann patrickmann commented Sep 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note: This is a backport of #26898 to 7.1, but not a clean cherry-pick.

Why 7.1 differs from 7.2

7.1 runs the same KCL 3.x and therefore has the same bug, but its Kinesis consumer is an older shape: 7.2 has reworked how the AWS clients are built, added the single-table migration, and gained an integration-test harness. The behavioural core, the denial detector and the failure recording, is byte-identical to master. Only the wiring around it is new work.

If the detector were attached incorrectly, the input would simply run without it and behave exactly as it does today, with nothing failing loudly to say so. A test for this is added.

The upgrade guidance is reworded for 7.1's feature set.

What Circle will see

Their denial is AccessDeniedException on DynamoDB Query, a watched operation.

  • After about two minutes of continuous denial, each affected input moves to FAILING with a message naming dynamodb:Query and the LeaseOwnerToLeaseKeyIndex ARN, and raises a system notification.
  • The KCL scheduler stops, so the ~200/sec Failed to execute lease discovery loop ends. Bounded, not eliminated: roughly two minutes of denials per input per node, plus up to 20s of graceful-shutdown drain, then silence.
  • Ingestion is not restored. That requires dynamodb:Query on table/graylog-aws-plugin-*/index/* granted on their role.
  • After granting it, the input needs a manual stop and start. The terminal latch does not clear itself.
  • Shards that never checkpointed resume at the stream tip, so the outage backlog is skipped for those. KinesisConsumer does not set initialPositionInStream, so KCL's LATEST default applies. Checkpointed shards replay from their checkpoint, within stream retention.

7.1 is the branch that reaches them. 7.0 ships KCL 2.6.1, which discovers leases by Scan and has no GSI, so this denial cannot occur there.

Risk of blocking an input on a transient error

Once tripped, terminallyFailed latches: The consumer stays stopped until an operator restarts the input. Previously such a condition self-healed, at the cost of log spam. The exposure is therefore a correctly configured input stopping permanently over a temporary condition.

What makes that hard to hit: the streak needs at least two minutes between its first and last denial with zero successful calls in between. Expired session credentials, throttling and the recoverable KMS states are excluded from the allowlist for exactly this reason.

What remained: KMSAccessDeniedException and KMSDisabledException were terminal, and both are routine temporary states on a healthy SSE-KMS stream. Disabling a CMK is a standard rotation step, and a grant expiry or key-policy change during a deploy can exceed two minutes. They are therefore excluded, see below. KMSNotFoundException and KMSOptInRequired are genuinely unrecoverable, so terminal is correct for those. Harm is bounded by stream retention: a restart inside the retention window replays checkpointed shards.

Excluding the recoverable codes

Fixed on master first as #27263 via #27265, now cherry-picked here: KMSAccessDeniedException and KMSDisabledException are out of the terminal list, KMSNotFoundException and KMSOptInRequired stay.

No effect on #15073. That denial is AccessDeniedException on DynamoDB Query, which stays terminal either way.

Resolves Graylog2/graylog-plugin-enterprise#15073

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have requested a documentation update.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@patrickmann @claude
Fail the AWS Kinesis input on unrecoverable AWS authorization denials (…
0398e18 
…#26898)

* Fail the AWS Kinesis input on unrecoverable AWS authorization denials

The Kinesis Client Library retries its own DynamoDB calls on a fixed
schedule for as long as the input runs and only logs the failure, so a
missing IAM permission produces an endless ERROR loop while the input
consumes no records and still reports RUNNING. KCL offers no hook to
observe or stop that, but we build the AWS clients it uses, so an
ExecutionInterceptor on those clients can see every denial.

After three consecutive denials the input is set to FAILING with the
denied action and resource, and the KCL scheduler is stopped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Add changelog for #26898

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Only stop the input when a denial has also stopped ingestion

Review found that a duration-only threshold stops inputs that are working.
KCL absorbs several denials and keeps delivering records from the leases it
already holds: a stalled single-table migration retries TransactWriteItems
about twice a second, the lease-assignment Scan runs every 20s, and the
DescribeTable used only for scan sizing never caches on failure. Each of
those has no permitted sibling call to clear its streak, so each matured at
two minutes and killed a healthy, ingesting input. The previous
three-consecutive-denials rule could not reach any of them because
interleaved successes reset it.

Terminality now needs both conditions: one operation denied for two minutes
and no record-processing task completed in that window. The reported case is
unaffected, since a worker denied lease discovery holds no leases and
processes nothing.

Also from the review:

- Decouple the streak-reset gap from the reporting threshold. They were the
  same constant, so an operation retried at just over two minutes restarted
  its streak on every attempt and could never be reported.
- Publish an IOStateChangedEvent when the detailed message changes while the
  state does not. The notification, the system message and the persisted
  runtime state are all written by subscribers, so the actionable message
  was reaching only callers reading the input state directly.
- Move terminality into InputFailureRecorder under its own lock.
  KinesisConsumer read its flag before writing the failure, so a task
  completing in between could report the input healthy again.
- Distinguish rejected credentials from a missing permission in the failure
  message. A rotated key is terminal, but "grant it" is the wrong remedy.
- Add the three unrecoverable KMS error codes; a kms:Decrypt gap on an
  encrypted stream produced the same endless loop undetected.
- Fail closed when the SDK reports no operation name, rather than sharing one
  bucket across unrelated calls.
- Log a terminal failure at ERROR, name the shutdown thread per stream, and
  stop asserting shutdown failed when KCL initialization still holds its lock.
- Drop the redundant reported latch and the unreachable self-cause guard.

Tests: drive a denial through the interceptor actually installed on the
client, so the feature cannot be left unwired; cover both halves of the new
rule, the threshold boundaries, exact error-code matching, and the terminal
message replacing a transient one. All four new properties confirmed red
against the corresponding mutation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Document the DynamoDB permissions KCL 3.5 added

The 7.2 upgrade note deferred required permissions to AWS's documentation and
named none, which is what made the reported incident possible: a policy
written for KCL 2.x looks correct and still denies the input.

Names the three actions that are new or newly scoped - Query on the lease
table's index, UpdateTable to create that index, and DescribeTable on the
legacy CoordinatorState and WorkerMetricStats tables even for inputs that
never had them - plus the item-level actions the single-table migration needs
inside its transaction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Report only denials of operations the consumer cannot work without

Review found the no-progress condition unsound in both directions, because it
measures completed KCL record-processing tasks and those exist only for leases
the worker already holds.

False negative: DynamoDBLeaseCoordinator.start() calls leaseRenewer.initialize()
unconditionally, which Scans the lease table and adopts every row whose
leaseOwner matches this worker. The worker id is a SHA-256 of the persisted node
id, so it is stable across restarts, and nothing clears leaseOwner on shutdown.
A worker denied lease discovery therefore keeps processing the leases it owned
before, stamps progress every ~1.5s and is never reported - including on the 7.1
to 7.2 upgrade path that produced the incident, and on every restart after the
fix has fired once.

False positive: a worker holding no leases never stamps progress at all, so the
gate is permanently open there and the rule degrades to the duration-only one it
replaced. AWS inputs are created global, and KCL leadership is a DynamoDB lock
with no lease affinity, so on a cluster with more nodes than shards the
leader-only schedules the gate was added to protect - the single-table migration
TransactWriteItems, the lease-assignment Scan, the scan-sizing DescribeTable -
still stop consumers one node at a time until only the leaseholder is left, with
no failover capacity behind it.

Terminality now keys on the operation instead. A denial is reported only for the
calls KCL retries forever while surfacing nothing and the consumer cannot work
without: DynamoDB Query for lease discovery, and Kinesis GetRecords for the read
path, which is also how the KMS failures of an encrypted stream arrive.
Everything KCL absorbs is logged and left alone, on every node and at every
stage of a worker's life, so detection no longer depends on lease ownership.

ListShards and GetShardIterator are deliberately out: ListShards runs at the
120s periodic-sync cadence, which would mature a streak on two samples, and a
denied GetShardIterator already surfaces through TaskExecutionListener.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Test the notification retire-and-re-raise branch

The branch that gets the actionable message onto the INPUT_FAILING notification
had no test anywhere: deleting it left the whole suite green while restoring the
defect it was added for, and the notification is the only surface a Cloud tenant
can see. There was no InputStateListenerTest at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Correct and rescope the DynamoDB permissions documentation

Review found three defects in the note added by ea838b3.

It was filed as a subsection of the single-table migration topic, which its own
text scopes to inputs created before 7.2, so an operator running only new inputs
skips the section and never sees the dynamodb:Query requirement that caused the
incident. It also displaced that topic's closing paragraph, which defers
required permissions to AWS's documentation - the very thing the note exists to
replace. It is now its own section, ahead of the migration topic.

The migration transaction also writes a conditional Put to the CoordinatorState
table, so it needs PutItem there, not only DeleteItem and ConditionCheckItem.
Without it the transaction is rejected, KCL logs "Will retry next cycle" and
swallows the failure, and the migration never reaches COMPLETE while the UI
reports nothing - which is the state the migration steps tell the operator to
verify.

DescribeTable alone on the legacy tables is enough only for an input created on
7.2. Until an older input completes the migration, KCL routes its leader lock
and its 30s worker-metrics writes to those tables, so they need the same
item-level actions as the lease table. A policy scoped precisely to
DescribeTable breaks every upgraded input; what hides this today is that the
wildcard in the example resource happens to match both suffixed table names.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Address the smaller review items

- Treat KMSDisabledException as terminal. The comment claimed it recovers on
  its own, but AWS's own model says the key "isn't enabled", which stays true
  until an operator re-enables it. With GetRecords watched, a disabled CMK
  otherwise reproduces this bug exactly: PrefetchRecordsPublisher swallows the
  exception and re-polls every 1.5s while the input reports RUNNING and consumes
  nothing. KMSInvalidStateException stays excluded, but on honest grounds - its
  documentation does not say which key states produce it, so an allowlist has to
  fail safe.
- Derive TERMINAL_ERROR_CODES from CREDENTIAL_ERROR_CODES with Sets.union
  instead of repeating its four literals. A code in only one of the two sets
  would either never be reported or be reported with the wrong remedy, and no
  test could have caught either.
- Make setTerminallyFailing self-bounding. At-most-once is the caller's to
  enforce, but a repeated report should not become a stream of state writes:
  each published event costs a system message, a notification rebuild and a
  Mongo upsert.
- Drop applyFailure's boolean parameter, which duplicated the terminallyFailed
  field it is always equal to. What silently diverges otherwise is the log
  level, which is the one signal ERROR logging was added to guarantee.
- Name the stream, not the input, in the terminal message: the value
  interpolated there is the Kinesis stream name, and two inputs can share one
  stream. Also present tense, since stop() is dispatched afterwards.
- Have the test call shutdownThreadName() rather than repeat its format string,
  so @VisibleForTesting is true and the two cannot drift.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Drop redundant tests and trim parameters

Remove four cases that duplicated coverage or exercised pre-existing,
unchanged behavior, and narrow the absorbed-operation parameters to two
representatives (the property holds for one):

- InputFailureRecorderTest.setRunningClearsAFailure (pre-existing behavior)
- InputStateListenerTest.persistsTheCurrentStateAndMessage (pre-existing persistence)
- KinesisConsumerTest.successfulTaskClearsAnOrdinaryFailure (one-line delegate)
- AWSAuthorizationFailureDetectorTest.ignoresDenialsBuriedDeeperThanTheCauseChainLimit
  (cause-chain unwrap already covered)
- neverReportsAnOperationKclAbsorbs: 6 -> 2 params

52 tests, all green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Avoid "null" string in warning message

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* Address review: volatile scheduler + UPGRADING.md caveats

- Make kinesisScheduler volatile so the async stop() handoff sees the
  write from the KCL runner thread (danotorrey).
- UPGRADING.md: note the new failure detection does not cover the denied
  UpdateTable / missing-index case, which surfaces as a not-found error
  rather than a denial (danotorrey, kodjo-anipah).
- UPGRADING.md: grammar fix ("actions that KCL 2.x did not").

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Address review: DI cleanup, drop @VisibleForTesting seams

- AWSAuthorizationFailureDetector: recordFailure is now private and
  recordSuccess is inlined into afterExecution; tests drive the detector
  through its public onExecutionFailure/afterExecution hooks instead
  (kodjo-anipah). Removed the now-redundant explicit-hooks test.
- KinesisConsumer: collapse the two constructors into one taking nanoClock
  as a normal dependency; KinesisTransport passes System::nanoTime.
  recordTaskSuccess is now private (kodjo-anipah).
- changelog details.ops: note the fail-fast does not cover a denied
  UpdateTable / missing-index, which surfaces as a not-found error rather
  than a denial (kodjo-anipah).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Broaden not-found caveat to any missing index or lease table

The fail-fast fires only on authorization denials of watched operations,
not on not-found errors. State the general rule rather than just the
denied UpdateTable case, in UPGRADING.md and the changelog (kodjo-anipah).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Trim repetition in UPGRADING.md Kinesis sections

- Drop the duplicated "In Graylog 7.2 ... upgraded to KCL 3.5" preamble
  from the state-tracking section; the permissions section above already
  states it.
- Remove the repeated "consumes nothing" in the UpdateTable caveat.
- Make the ConditionCheckItem paragraph a clear forward-reference to the
  migration section and stop duplicating its TABLE_MIGRATION_STATUS_COMPLETE
  terminology.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Address review: init-failure docs, null error code, node-scoped clear

- Guard denialWithCodeIn against a null AWS error code so a body with no
  parseable code no longer NPEs into a new stack-trace loop; add a test.
- Correct UPGRADING.md, the changelog and PR step 8: a denied UpdateTable
  (or DescribeTable) aborts KCL startup, so the input fails initialization
  with a generic error rather than starting and consuming nothing.
- Document that KMS access failures on an encrypted stream are terminal.
- Node-scope the RUNNING-branch notification clears so a recovering node no
  longer deletes a still-failing peer's notification for a global input,
  which the terminal-failure latch would never re-raise.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Default the node-scoped fixed() so implementations outside core still compile

NotificationService is on the plugin API surface, so adding an abstract
method breaks every implementation outside this repository, including
NullNotificationServiceImpl in graylog-plugin-enterprise. This repo's CI
does not compile enterprise, so it would have landed as a red enterprise
master.

The default delegates to the two-argument form, which clears cluster wide.
That is the behaviour callers had before this method existed, so an
implementation that does not override it is coarser rather than broken, and
the javadoc says so to stop callers relying on node scoping.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Pin the watched operation names to what the SDK and KCL actually emit

ESSENTIAL_OPERATIONS holds two operation-name literals that were only ever
compared against literals the tests themselves supplied, so the constant and
the tests could agree with each other and both be wrong relative to the SDK.
A KCL upgrade that moved the read path off GetRecords would have gone
unnoticed with every test green.

KinesisConsumerIT now overrides createClientBuilders to record the operation
names KCL asks the Kinesis client for, and the record round trip asserts
GetRecords is among them. A new test asks the SDK what it names
DynamoDbClient.query, because this IT runs KCL in 2.x-compatible assignment
mode and so never issues the lease discovery query itself. Both route through
the real onExecutionFailure hook against a controllable clock.

Kept in the IT rather than in the unit test so the unit class stays at its
sub-second baseline. Constructing an SDK client there cost 4.8s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Clear input failure notifications cluster wide again

Notification identity is (type, key) with no node dimension: node_id is
stamped by whichever node wins publishIfFirst and only feeds the template.
fixed(Type, Node) has existed all along with zero production callers, so a
node-scoped clear would have been the only one in the codebase.

It also only half worked. The document belongs to the node that raised it
first, so scoping the clear protects a still-failing peer only when the
recovering node is not the owner. When the owner is the node that recovers,
the notification is deleted either way.

This reverts the node-scoped clear and the NotificationService overload
added for it, including the default added so implementations outside this
repository would still compile. All three notification files are byte
identical to master again.

The terminal failure stays visible per node in input_runtime_states and the
inputs list, and on Enterprise in the health panel, so what is given up is
the system notification in one interleaving.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
(cherry picked from commit 009e2cd)
@patrickmann @claude
Pin that KCL does not drop the Kinesis authorization-failure detector
36d7b1c 
This branch registers the interceptor on the Kinesis builder and then hands
that builder to KinesisClientUtil.createKinesisAsyncClient(), which applies
its own HTTP client builder before building. Master builds the client itself,
so it has no equivalent seam, and the KinesisConsumerIT case that would have
caught a regression here is not portable to 7.1.

Verified genuinely red by resetting the override configuration before the
adjust call, which drops the detector and fails the assertion.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 3, 2026
Closed
Merged
@patrickmann @claude @danotorrey
Do not fail a Kinesis input on a recoverable KMS failure (#27265)
a88f358 
* Do not fail a Kinesis input on a recoverable KMS failure

KMSDisabledException and KMSAccessDeniedException were terminal, but each
names a state that routinely clears without any change in Graylog: a CMK
disabled while rotating to a new key, an expired grant, an edited key policy.
Because the terminal state latches, a stream that recovered on its own was
left stopped until an operator restarted the input.

KMSNotFoundException and KMSOptInRequired stay terminal; neither resolves
without operator action.

The cost is that AWS documents KMSAccessDeniedException as covering both a
key that does not exist and one you cannot access, so a deleted key reported
under that code is no longer caught. KMSNotFoundException covers the usual
spelling.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Add PR number to changelog

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Dan Torrey <dan.torrey@graylog.com>
(cherry picked from commit 07f8b3b)
@patrickmann
patrickmann marked this pull request as ready for review September 7, 2026 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danotorrey danotorrey Awaiting requested review from danotorrey
@kodjo-anipah kodjo-anipah Awaiting requested review from kodjo-anipah

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

Assignees

No one assigned

Labels

backport

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@patrickmann