s3_input: handle s3:TestEvent messages to prevent redelivery - #4667
s3_input: handle s3:TestEvent messages to prevent redelivery#4667josephwoodward wants to merge 6 commits into
Conversation
| addDudFn(sqsMsg) | ||
| s.log.Debug("Extracted zero target keys from SQS message") | ||
| s.log.Warnf( | ||
| "Extracted zero target keys from SQS message using key_path %q (bucket_path %q) - this likely indicates a misconfigured key_path/bucket_path, or an unrecognised notification event type: %s", | ||
| s.conf.SQS.KeyPath, s.conf.SQS.BucketPath, *sqsMsg.Body, | ||
| ) |
There was a problem hiding this comment.
Promoting this from Debug to Warnf (including the full message body) turns a benign trace into a log flood, because the surrounding code deliberately redelivers the message forever.
addDudFn on the line above queues a ChangeMessageVisibility with VisibilityTimeout: 0 (input.go#L534-L540), so the same message becomes visible immediately and is re-received on the next poll — and the empty-read path only backs off 500ms (input.go#L442-L445). A single permanently-misconfigured key_path therefore emits this warning with the entire SQS body roughly twice a second, indefinitely, per message in the queue. The PR's own integration test asserts exactly this steady state (message stays on the queue), which is the flood condition.
CONTRIBUTING.md §1.2.2 — "Provides relevant logging to support troubleshooting" — is not served by an unbounded repeat; it drowns the surrounding logs.
Suggested fix: emit the diagnostic warning at most once per message (e.g. track seen MessageIds, or a sync.Once/rate-limited warn for the misconfiguration hint) and keep the per-occurrence detail at debug level.
There was a problem hiding this comment.
Yeah, this part of the acceptance criteria concerns me too. I'm not convinced of your suggested fix but I'm going to clarify this is really what we want.
S3 publishes an s3:TestEvent to the configured SQS queue whenever bucket notifications are (re)configured, to verify the queue exists and the bucket owner has permission to publish to it.
Before this change, regular messages got deleted via sqs.DeleteMessage whereas zero-target messages (messages with no bucket+key pair, such as s3:TestEvent) went a different code path to addDudFn which would set the visibility to 0 and make it immediately available, so the same test event gets redelivered and reprocessed indefinitely, spamming Extracted zero target keys from SQS message at DEBUG.
This change ensures we handle s3:TestEvent messages explicitly, acking/deleting them so they don't get redelivered.
Proof of Work
Run 1: First run shows the delivery of s3:TestEvent resulting in repeated
Extracted zero target keys from SQS messagedebug logs.Run 2: Switch to this branch where we explicitly check for the test events and handle them
Run 3: Revert back to main to validate the s3:TestEvents have been acked.