Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: timeout risk in usages of lua-resty-aws #12070

Merged
merged 1 commit into from
Mar 20, 2025

Conversation

shreemaan-abhishek
Copy link
Contributor

Description

When initializing the lua-resty-aws module, it may send requests to IDMS endpoint which may timeout in non-aws environment or just introduce delays in some cases. Thus it is good to remove the initialisation from the module level.

Also, it is unnecessary to initialise the module for each request. This problem has also been fixed which ensures that if any there are any delays in initialising the module, such delay would occur only once.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Mar 19, 2025
@bzp2010 bzp2010 self-requested a review March 20, 2025 04:20
@@ -14,8 +14,12 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
require("resty.aws.config") -- to read env vars before initing aws module
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want the default value of false for AWS_EC2_METADATA_DISABLED to cause access to IDMS to be turned off by default unless the user explicitly turns it on?

Can you give a more detailed argument for its use? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this line mainly as a standard practice. (To read AWS specific ENV vars when APISIX is deployed in AWS environment)

Do you want the default value of false for AWS_EC2_METADATA_DISABLED to cause access to IDMS to be turned off by default unless the user explicitly turns it on?

No, I added this to ensure the IDMS query can be disabled if need be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore, it obeys those predefined environment variables anyway, and the initialization process for aws instance is moved to when the AWS API request actually occurs.
IDMS requests don't happen if the plugin configuration contains user-set AK/SK?

Copy link
Contributor Author

@shreemaan-abhishek shreemaan-abhishek Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDMS requests don't happen if the plugin configuration contains user-set AK/SK?

as per my last test, IDMS request happened anyway. Even if configuration contained user-set AK/SK.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this will block requests when API calls are incoming, or block the secret manager.
Unless the user explicitly specifies AWS_EC2_METADATA_DISABLED as true to disable IDMS request?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this will block requests when API calls are incoming, or block the secret manager.

yes, but only for the first time.

@bzp2010 bzp2010 mentioned this pull request Mar 20, 2025
5 tasks
@Revolyssup Revolyssup merged commit 55b1dd2 into apache:master Mar 20, 2025
60 checks passed
@shreemaan-abhishek shreemaan-abhishek deleted the timeout-risk-2 branch March 20, 2025 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants