Fail the AWS Kinesis input on unrecoverable AWS authorization denials - #26898
Draft
patrickmann wants to merge 8 commits into
Draft
Fail the AWS Kinesis input on unrecoverable AWS authorization denials#26898patrickmann wants to merge 8 commits into
patrickmann wants to merge 8 commits into
Conversation
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>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
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>
This was referenced Aug 5, 2026
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>
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>
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>
- 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>
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.
Closes Graylog2/graylog-plugin-enterprise#15073
Description
When the IAM role an AWS Kinesis input uses is missing a DynamoDB permission, the Kinesis Client Library retries the denied call on a fixed schedule for as long as the input runs and only logs the failure. The result is an endless
ERRORloop with a full stack trace, while the input consumes no records and still reportsRUNNING.The concrete case that prompted this is KCL 3.x lease discovery, which queries a global secondary index on the lease table.
dynamodb:Queryontable/<app>/index/*is a new requirement in KCL 3.x, and an index is a separate IAM resource, so a policy that grantsQueryon the table alone still denies it:KCL exposes no callback for lease-management failures and no setting to bound them. It does, however, use AWS clients that we build ourselves (we bypass
KinesisClientUtilto keep proxy support), so anExecutionInterceptoron those clients observes every denial the service returns.AWSAuthorizationFailureDetectorreports a failure and stops the KCL scheduler once a call the consumer cannot work without has been denied for two minutes.Notes:
TransactWriteItemsabout twice a second, the lease-assignmentScanruns every 20s, and theDescribeTableused only for scan sizing never caches on failure. None of those has a permitted sibling call to clear its streak, so reporting on any denial stops inputs that are ingesting normally. Two operations qualify, both of which KCL retries forever while surfacing nothing: DynamoDBQuery, which is lease discovery and the call denied in the reported case, and KinesisGetRecords, which is the read path itself and how an encrypted stream's KMS failures arrive. Everything else is logged and left alone.DynamoDBLeaseRenewer.initialize(), which is a permittedScan, so it keeps processing and a denied lease discovery is never reported - including on the 7.1 to 7.2 upgrade path that produced this incident. Conversely a worker holding no leases never reports progress at all, and since AWS inputs are global and KCL leadership is a DynamoDB lock with no lease affinity, a cluster with more nodes than shards would still lose one consumer at a time to the leader-only schedules above.GetRecordsare included where they need an operator to act -KMSAccessDeniedException,KMSNotFoundException,KMSOptInRequiredandKMSDisabledException, the last because AWS's model says the key "isn't enabled", which stays true until someone re-enables it.PrefetchRecordsPublisherswallows all of them and re-polls every 1.5s forever, so each produces this same bug.KMSInvalidStateExceptionis excluded because its documentation does not say which key states produce it, and an allowlist has to fail safe. Matching is exact equality, which a test pins, because the allowlist carries the short codeAccessDeniedand that is a substring of unrelated error codes.DynamoDbAsyncClientcarries around a dozen KCL schedules at very different rates: the denied lease-discoveryQueryruns every ~10s, while lease renewal writes every ~3.3s and the leader polls the migration state every ~1s. Any state shared across operations is cleared by that healthy traffic long before a threshold is reached. Note this keys on the operation name while IAM authorizes on (action, resource), so a permitted call to a different table under the same operation name does clear the streak. That does not affect the reported case:Queryhas exactly one call site in KCL 3.5.0 and it always targets the index.cloudwatch:PutMetricDatadenial is non-fatal and must not fail an input that is otherwise ingesting.stop()blocks for up to 20s, and the callback runs on the client's shared SDK response-completion pool (sdk-async-response), or on that pool's rejection path directly on the HTTP thread, so occupying one for that long would stall unrelated completions on the same client. The thread is named per stream so concurrent failures are distinguishable in a dump.InputFailureRecorder.setFailingkeeps the first message once an input is alreadyFAILING, so a precedingTaskOutcome.FAILUREwould hide the denied action and resource permanently. A newsetTerminallyFailingreplaces it and blocks any latersetRunning().IOState.setStatealso had to publish its event when only the message changes: the notification, the system message and the persisted runtime state are all written byIOStateChangedEventsubscribers, so the actionable message was otherwise reaching only callers reading the input state directly.InputStateListeneris the only subscriber in core or enterprise.InputFailureRecorder, under the same lock as every state write.KinesisConsumerpreviously read its own flag before the failure was written, so a KCL task completing in between could revert the input toRUNNING.UPGRADING.mdnow names the DynamoDB permissions KCL 3.5 added, in its own section rather than inside the single-table migration topic, becauseQueryon the index andUpdateTableto create it are required by every 7.2 input including new ones. It also splits the legacy-CoordinatorStateand-WorkerMetricStatstables by input age:DescribeTablealone is enough only for an input created on 7.2, while an input that has not completed the migration keeps its leader lock and worker metrics there and needs the same item-level actions as the lease table. The 7.2 note deferred all of this to AWS's documentation and listed none of it, which is what let the incident happen.Known limits, stated rather than fixed:
server.logand from AWS.FAILINGper node as each node's own streak matures. Neither watched operation is leader-gated and neither needs a held lease, so unlike the earlier revision detection no longer depends on which node holds what.dynamodb:UpdateTableis not detectable through this seam. If it is denied the index is never created, and a KCL idempotence bug means the second initialization attempt skips index creation entirely, so the input starts and the discoverer then fails withResourceNotFoundExceptionrather thanAccessDenied.UPGRADING.mdcovers it instead.ListShardsandGetShardIteratorare deliberately not watched.ListShardspost-init runs at the 120s periodic-sync cadence, close enough to the threshold that a streak could mature on two samples, and a deniedGetShardIteratoraborts its shard consumer and already reaches the input throughTaskExecutionListener, though only with the generic message.How Tested
AWSAuthorizationFailureDetectorTest(40 cases): reports once a denial run spans two minutes at the real 9975ms lease-discovery cadence; a success on a different operation on the same client does not clear the run, which is the property that makes the fix work; twenty minutes of continuous denial of an operation KCL absorbs changes nothing, overScan,DescribeTable,TransactWriteItems,UpdateItem,GetItemandPutItem; a denied Kinesis record fetch is reported; a success on the denied operation itself clears the run; the threshold boundaries either side of two minutes; an operation retried more slowly than the threshold is still reported; a gap longer than the reset window starts a new run; unwraps a denial nested in a cause chain but not one buried deeper than the limit; treats all nine terminal codes as terminal and matches them exactly; never reports self-healing codes, non-AWS exceptions, or AWS exceptions without error details; drives the realonExecutionFailure/afterExecutionhooks; and fails closed when the SDK reports no operation name.KinesisConsumerTest(7 cases): the headline one drives a denial through the interceptor actually installed on the DynamoDB client, so the feature cannot be left unwired; plus the terminal failure and off-thread stop with the per-stream thread name, at-most-once handling, the credential-specific message, the per-stream shutdown thread name taken from production code, one detector each on DynamoDB and Kinesis and none on CloudWatch, distinct instances, and a successful task clearing an ordinary failure.InputFailureRecorderTest(9 cases):setFailingkeeps the first message,setTerminallyFailingreplaces it, keeps its own first message, and survives a latersetRunning()orsetFailing(), an event is published when only the message changes and not when nothing changes.InputStateListenerTest(4 cases): the same-state failure retires the stale notification before re-raising it, for bothINPUT_FAILINGandINPUT_FAILED_TO_START; a real state transition retires nothing, since that would discard a notification another node may have raised; and the current state and message are persisted. This branch is what gets the actionable message onto the notification, which is the only surface a Cloud tenant can see, and nothing covered it before.KinesisConsumerITnow also capturessetTerminallyFailing, or a terminal failure inside the IT would be swallowed until its deadline.org.graylog.integrations.awssuite plusInputFailureRecorderTest,InputStateListenerTestandIOStateTest: 116 tests green, includingKinesisConsumerIT.forbiddenapisclean on both source and test scans.InputStateListenerfails exactly its two notification tests and leaves the other two green. Earlier in the branch, reverting theIOStatemessage check, dropping the terminal flag fromsetRunning()and replacing the detector callback with a no-op each failed exactly one corresponding test.KinesisConsumerITruns against an emulator with no IAM enforcement, so it cannot produce a real 403.Manual testing against real AWS, which is the only way to produce a genuine denial:
CreateTable,DescribeTable,GetItem,PutItem,Scan,UpdateItem,UpdateTable) but notdynamodb:Queryonarn:aws:dynamodb:<region>:<account>:table/graylog-aws-plugin-*/index/*.UpdateTablematters: without it the index is never created and you reproduce a different failure.dynamodb:Queryand theLeaseOwnerToLeaseKeyIndexARN, and thatFailed to execute lease discoverystops recurring. This no longer depends on the state of the lease table, so it reproduces on a first start and on any later start alike.cloudwatch:PutMetricDataand confirm the input keeps running.dynamodb:DescribeTableon the lease table. KCL falls back to a default scan parallelism and keeps delivering records, andDescribeTableis not a watched operation, so the input must not be stopped. Worth checking on a node that holds no leases as well as on the leaseholder, since that is where a progress-based rule went wrong.Also validated end to end against a local emulator driving the real KCL 3.x lease discoverer, with a proxy in front of DynamoDB that denies only
Query(the GSI call) and forwards everything else, mirroring the customer's policy.Queryis exactly the operation the shipped rule watches, so that run still describes the shipped behaviour. Note the harness ran withfailoverTimeMillis=500, which compresses KCL's coordination timings by about 21x, so it validates the window and the interleaving but not the production cadence: it recorded 254 denied queries at ~473ms apart against 1,086 permitted calls on the same client (UpdateItem872,Scan197,DescribeTable10,GetItem4,PutItem3, and no permittedQuery). That interleaving is precisely what a counter shared across operations cannot survive. At production defaults the equivalent run is 13 denials against roughly 145 permitted calls, and the input is reported at 129.7s. The reported cause was the SDK-unmarshalledAccessDeniedExceptionnamingdynamodb:Queryand theLeaseOwnerToLeaseKeyIndexARN, andstop()ran on the auth-failure shutdown thread. The harness is not committed: it needs Docker, waits out the real two-minute window, and its in-process HTTP proxy trips thejdk-non-portableforbidden-API check.Note this behaviour also reaches the Cloud Forwarder, since
KinesisTransportis bound inconfigureUniversalBindings()and the input is forwarder- and Cloud-compatible. The forwarder UI does renderFAILINGand offer Stop, so the "stop and start the input" remedy is actionable there too.Types of changes
Checklist: