[draft] Fix keepalive initialization in awss3 input

faec commented 4 days ago

Proposed commit message

The visibility_timeout flag in the SQS input isn't initialized correctly. It is used when extending visibility timeouts, but not when initially fetching the message, so the first timeout defaults to the global queue setting (which is configured by the user when creating the SQS queue). If the client-side timeout is higher than the AWS-side timeout, then Agent will not extend the visibility in time, and any objects that aren't processed by the AWS-side timeout will be returned to the queue.

The intention is for the visibility timeout to be applied regardless of the SQS-side defaults. The solution is to call the SQS ChangeMessageVisibility API at the beginning of the keepalive helper, rather than waiting until the first timeout expires.

Checklist

[ ] My code follows the style guidelines of this project
[ ] I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation
[ ] I have made corresponding change to the default configuration files
[ ] I have added tests that prove my fix is effective or that my feature works
[ ] I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Author's Checklist

[ ]

How to test this PR locally

Related issues

-

Use cases

Screenshots

Logs

mergify[bot] commented 4 days ago

This pull request does not have a backport label. If this is a bug or security fix, could you label this PR @faec? 🙏. For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

andrewkroh commented 4 days ago

The visibility_timeout flag in the SQS input isn't initialized correctly. It is used when extending visibility timeouts, but not when initially fetching the message

https://github.com/elastic/beats/blob/e3a8223d43bd3af01183c4f559280f1da72007de/x-pack/filebeat/input/awss3/interfaces.go#L141-L144

I thought that's what this code was doing during the initial ReceiveMessage. Or are you saying that a.visibilityTimeout isn't initialized?

elastic / beats