fluent / fluent-plugin-opensearch

OpenSearch Plugin for Fluentd
Apache License 2.0
57 stars 20 forks source link

Prevent AWS credentials refresh from stopping on exception #142

Closed aYukiSekiguchi closed 4 days ago

aYukiSekiguchi commented 1 month ago

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 commented 1 month ago

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.

akhil31415 commented 1 month ago

@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.

aYukiSekiguchi commented 1 month ago

@akhil31415

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

Ruby 3.2 Unit Testing on ubuntu-latest

Coverage/Coveralls

ashie commented 1 month ago

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

aYukiSekiguchi commented 1 month ago

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.

cosmo0920 commented 1 month ago

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

aYukiSekiguchi commented 1 month ago

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

ntopee commented 1 week ago

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

cosmo0920 commented 1 week ago

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

aYukiSekiguchi commented 6 days ago

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.

kenhys commented 5 days ago

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 commented 5 days ago

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