fluent-plugin-opensearch icon indicating copy to clipboard operation
fluent-plugin-opensearch copied to clipboard

Prevent AWS credentials refresh from stopping on exception

Open aYukiSekiguchi opened this issue 1 year ago • 7 comments

If aws_credentials() fails due to an unstable network or other issues, it throws an exception. This stops timer_execute() from repeating its block, preventing OpenSearchOutput from updating @_aws_credentials. As a result, @_aws_credentials will expire.

This commit catches the exception and prevents it from propagating to timer_execute(), ensuring continuous credential updates.

aYukiSekiguchi avatar Sep 02 '24 12:09 aYukiSekiguchi

This will fix https://github.com/fluent/fluent-plugin-opensearch/issues/129

It looks like there is no test case for @_aws_credentials and aws_credentials(). I don't create a testcase for the exception. Sorry.

aYukiSekiguchi avatar Sep 02 '24 12:09 aYukiSekiguchi

@aYukiSekiguchi san, I really appreciate the PR. Could you please check and fix the failing test cases? I kind of waiting for this to be merged.

akhil31415 avatar Sep 06 '24 03:09 akhil31415

@akhil31415

I'm not very familiar with GitHub Actions, so I might be mistaken.

Ruby 3.2 Unit Testing on ubuntu-latest

  • Failed Job Logs
    • The job failed with the following error:
      The installation path is insecure. Bundler cannot continue.
      /opt/hostedtoolcache/Ruby/3.2.5/x64/lib/ruby/gems/3.2.0/gems is world-writable (without sticky bit).
      Bundler cannot safely replace gems in world-writable directories due to potential vulnerabilities.
      Please change the permissions of this directory or choose a different install path.
      Error: Process completed with exit code 38.
      
    • It seems there is an issue with the GitHub Action settings.
    • This might be resolved by re-running the action, but it appears I don't have the permissions to do so.

Coverage/Coveralls

  • As mentioned in my previous comment, there is no test for @_aws_credentials.
  • Since I added begin / rescue to the @_aws_credentials code, this has decreased the branch coverage.
  • I'm unsure if this project allows for a decrease in coverage.
  • If decreasing coverage is not acceptable, I will try to write the necessary tests when I have some free time.

aYukiSekiguchi avatar Sep 06 '24 04:09 aYukiSekiguchi

Sorry for our less activity in this plugin. We are queuing this as our next task, will tackle on it next week.

ashie avatar Sep 06 '24 04:09 ashie

Thank you for retrying. It looks like there is a bug in .github/workflows/linux.yml. I have submitted a PR to fix the issue: PR #143.

aYukiSekiguchi avatar Sep 06 '24 06:09 aYukiSekiguchi

Hi, I merged you fixing Linux workflow PR. Could you rebase off master?

cosmo0920 avatar Sep 06 '24 07:09 cosmo0920

Thank you. I have rebased this PR. The error with Ruby 3.2 on Ubuntu has been fixed. Only the coverage/coveralls issue remains.

aYukiSekiguchi avatar Sep 06 '24 07:09 aYukiSekiguchi

Sorry to rush, but is there any ETA on merging/releasing this change? Thanks!

ntopee avatar Sep 27 '24 07:09 ntopee

@ashie @kenhys @daipom Could y'all take over this PR?

cosmo0920 avatar Sep 27 '24 11:09 cosmo0920

Sorry for any confusion.

I'm waiting for the committer to decide whether the Coveralls failure, which decreased coverage, is acceptable. Some recent commits to the main branch also failed Coveralls, so I think this is sometimes acceptable.

aYukiSekiguchi avatar Sep 30 '24 10:09 aYukiSekiguchi

It seems that PR itself is reasonable but it might be better to add test case for it in assured. I'll look into it.

kenhys avatar Oct 01 '24 01:10 kenhys

MEMO: it is enough that mock aws_credentials and raise RuntimeError in second call, then assert existence of error log message.

kenhys avatar Oct 01 '24 05:10 kenhys