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

Prevent AWS credentials refresh from stopping on exception #142

Conversation

aYukiSekiguchi
Copy link
Contributor

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
Copy link
Contributor Author

This will fix #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
Copy link

@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
Copy link
Contributor Author

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

@ashie
Copy link
Member

ashie commented Sep 6, 2024

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

@aYukiSekiguchi
Copy link
Contributor Author

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
Copy link
Collaborator

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

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.

Signed-off-by: Yuki Sekiguchi <[email protected]>
@aYukiSekiguchi aYukiSekiguchi force-pushed the prevent_refresh_from_stopping_on_exception branch from c55c335 to a8e1639 Compare September 6, 2024 07:19
@aYukiSekiguchi
Copy link
Contributor Author

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
Copy link

ntopee commented Sep 27, 2024

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

@cosmo0920
Copy link
Collaborator

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

@aYukiSekiguchi
Copy link
Contributor Author

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
Copy link
Contributor

kenhys commented Oct 1, 2024

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
Copy link
Contributor

kenhys commented Oct 1, 2024

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

@kenhys kenhys requested a review from daipom October 2, 2024 07:22
Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

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

Thanks!

test/plugin/test_out_opensearch.rb Outdated Show resolved Hide resolved
test/plugin/test_out_opensearch.rb Outdated Show resolved Hide resolved
@kenhys kenhys force-pushed the prevent_refresh_from_stopping_on_exception branch from 9a5d2e9 to b877206 Compare October 2, 2024 07:52
Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

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

LGTM.

@cosmo0920 cosmo0920 merged commit e180c21 into fluent:main Oct 2, 2024
12 checks passed
kenhys added a commit that referenced this pull request Oct 2, 2024
Mainly improved retry logic and refresh aws credentials.

#116
#119
#131
#136
#142
#143
kenhys added a commit that referenced this pull request Oct 2, 2024
Mainly improved retry logic and refresh aws credentials.

#116
#119
#131
#136
#142
#143
Signed-off-by: Kentaro Hayashi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants