Refactor endpoint resolver for AWS services

[sayden]

What does this PR do?

Custom endpoint resolver in AWS services initialisation was broken after the aws sdk upgrade.
This PR remove the custom endpoint resolver, but for the case of non aws bucket in direct aws s3 input

• Closes #32888

[mergify]

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @sayden? 🙏.
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

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b aws-fix-region-not-being-used upstream/aws-fix-region-not-being-used
git merge upstream/main
git push upstream aws-fix-region-not-being-used

[tommyers-elastic]

nice - does this resolve the reported issue in SQS ?

do we have an understanding of the difference between this and the built-in default resolver? i.e. can we switch over to that instead of maintaining this one?

[sayden]

This is the least invasive way to update the current library without proper testing. We can use the default in a follow up, once we understand how it works and why AWS introduced a breaking change there.

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-09-06T00:24:25.954+0000

• Duration: 136 min 46 sec

Test stats 🧪

Test	Results
Failed	0
Passed	8717
Skipped	503
Total	9220

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• /package : Generate the packages and run the E2E tests.

• /beats-tester : Run the installation tests with beats-tester.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[tommyers-elastic]

without proper testing

could you expand on this a little? 😛

[sayden]

could you expand on this a little?

There are no tests, at all, that actually checks this across all Beats. I like to understand side effects of changing something that affects 5-6 packages.

Hey, but I disagree and commit. Seriously, no prob. 🙂

[ruflin]

Is there a way that the user can change this manually in config or environment variables without having to change it in the code? I'm asking to see if for users that are hit by this issue, if there is also currently a manual workaround?

[sayden]

Is there a way that the user can change this manually in config or environment variables without having to change it in the code? I'm asking to see if for users that are hit by this issue, if there is also currently a manual workaround?

Good question, I have looked at the code and I don't see a workaround atm. The only way to avoid the issue will be to not use a customized endpoint as far as I can see.

This function has 22 references in other parts of the code: billing, cloudwatch, rds, sqs... so it's complicated to predict side effects.

Looking at the old and new code, this change looks like the missing piece. But I think we should consider to decouple those 22 functions to alleviate the side effects produced by small changes like this in the near future.

This issue comes from a breaking change in the AWS library where they removed the generic resolver and introduced specific resolvers for each service: ec2 resolver, sqs resolver, etc. I have some degree of confidence, after looking at alternatives and other solutions in the AWS SDK, that this should solve the issue. But as I said before, we should look at those specific resolvers because they probably do much more for users than the custom generic solution we have now.

[aspacca]

Hey, but disagree and commit. Seriously, no prob.

I disagreed and committed :)

Given other issues the problem does not seem to be limited to the missing SigningRegion, but to the overall behaviour of the endpoint resolver (see elastic/integrations#3990 for example).

My point to the change I made is that I expect the internal endpoint resolver of the packages in the aws sdk to be much more tested than any coverage and manual testing we can do for an hot fix.

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b aws-fix-region-not-being-used upstream/aws-fix-region-not-being-used
git merge upstream/main
git push upstream aws-fix-region-not-being-used

[aspacca]

after discussing with @sayden we agreed on reducing the surface of the change

we need proper manual testing of different scenarios

[tommyers-elastic]

thanks for getting deeper into this @sayden and @aspacca.

i think it would be have been best to merge the small fix as originally reviewed, then approach the larger refactor in a new PR. this way the short term fix for SQS could be merged, whilst the larger PR goes through review + test. i have removed my original ✅ for now.

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b aws-fix-region-not-being-used upstream/aws-fix-region-not-being-used
git merge upstream/main
git push upstream aws-fix-region-not-being-used

[legoguy1000]

I'm looking at it now, but there may be an issue with removing the endpoint config option for metrics/sqs/... is for non regular AWS, to include the China AWS and US gov isolated (not govcloud) environments which all have different domains than amazonaws.com

Looks like that has been handled via https://github.com/aws/aws-sdk-go-v2/blob/main/service/s3/internal/endpoints/endpoints.go. It appears as long as the region is set it will autoconfigure the proper endpoint. I would say that the docs should be updated to enforce the importance of setting the default_region when using the non-regular AWS domain (amazonaws.com)

[tlee-elastic]

Quick question, will we expose a UI option for setting the default_region for fleet managed agents as I dont think there is a way to set this currently?

[aspacca]

@tlee-elastic

Quick question, will we expose a UI option for setting the default_region for fleet managed agents as I dont think there is a way to set this currently?

@legoguy1000 is taking care of it at elastic/integrations#4158 👍