Keep previous credentials when refresh fails

[dw-kihara]

The current version of aws_credentials seems to have a bug around refreshing credentials.

When refreshing fails, the stored credentials value is overwritten with true.

Reproduction:

$ rebar3 shell
===> Verifying dependencies...
===> Analyzing applications...
===> Compiling aws_credentials
Erlang/OTP 26 [erts-14.1] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit:ns]

Eshell V14.1 (press Ctrl+G to abort, type help(). for help)
1> application:set_env(aws_credentials, provider_options, #{credential_path => "test/aws_credentials_providers_SUITE_data/"}). % ensures credentials to be loaded
ok
2> application:ensure_all_started(aws_credentials). % starts aws_credentials
{ok,[jsx,eini,iso8601,aws_credentials]}
3> application:set_env(aws_credentials, fail_if_unavailable, false). % overrides fail_if_unavailable (since it is set to true by rebar3 shell)
ok
4> aws_credentials:get_credentials(). % should show credentials
#{access_key_id => <<"dummy_access_key">>,
  credential_provider => aws_credentials_file,
  secret_access_key => <<"dummy_secret_access_key">>}
5> application:set_env(aws_credentials, provider_options, #{credential_path => "fail"}). % changes options to make refresh fail
ok
6> [{_, Pid, _, _}] = supervisor:which_children(aws_credentials_sup). % get pid
[{aws_credentials,<0.403.0>,worker,[aws_credentials]}]
7> Pid ! refresh_credentials. % manually cause refresh by sending message
refresh_credentials
8> aws_credentials:get_credentials(). % timeout.
** exception exit: {timeout,{gen_server,call,
                                        [aws_credentials,get_credentials]}}
     in function  gen_server:call/2 (gen_server.erl, line 386)
9> aws_credentials:get_credentials(). % again. credential becam `true'
true

I suggest keeping the previous credentials when automatic refresh fails.

[onno-vos-dev]

@dw-kihara Wouldn't this result in the inability to invalidate file credentials? 🤔

[dw-kihara]

Wouldn't this result in the inability to invalidate file credentials? 🤔

@onno-vos-dev Well, I overlooked that point.
However, I don't think it's a good idea to invalidate credentials, such as aws_credentials_ec2, just because an HTTP transport happens to fail once.

Maybe it whould be better to have a function to explicitly invalidate credentials, or an option to invalidate credentials for force_credentials_refresh.
What do you think?

[dw-kihara]

an option to invalidate credentials for force_credentials_refresh

Hmm, I think this does not need tobe an option but mandatory behavior.
Because it says "force" it seems to be natural to lose current credentials if it fails.

I will modify this PR again.

[dw-kihara]

@onno-vos-dev I have reverted behavior of force_credentials_refresh in order to invalidate credentials manually.

I wanted to prevent the automatic refresh from removing the credentials accidentaly, so the current pull request meets my requirements.

Thank you for suggestion.

[onno-vos-dev]

This makes sense to me 👍 Would like to wait a day or two to see if any of the other maintainers have a different opinion than mine 👍

[jadeallenx]

This is a good behavioral change, thanks

[onno-vos-dev]

Thank you @dw-kihara

Tagged and released as 0.2.4: https://hex.pm/packages/aws_credentials/0.2.4