Rearrange security_token sources as AWS_SECURITY_TOKEN is deprecated (breaking change)

[nillovna]

SUMMARY

Rearranges security_token sources as AWS_SECURITY_TOKEN is deprecated.

As mentioned in boto documentation (and a bunch of other SDKs) AWS_SECURITY_TOKEN environment variable can be considered deprecated, it is only supported for backward-compatibility purposes. Only AWS_SESSION_TOKEN is mentioned as a session token environment variable in AWS documentation.
So it is more logical to check AWS_SESSION_TOKEN before AWS_SECURITY_TOKEN. Empty AWS_SECURITY_TOKEN could cause errors even if AWS_SESSION_TOKEN is defined.

Some tools already removed AWS_SECURITY_TOKEN support. For example, there is an issue with saml2aws.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

plugins.module_utils.ec2

ADDITIONAL INFORMATION

The problem is fully described in Versent/saml2aws#586. The issue is caused by an empty AWS_SECURITY_TOKEN. Despite AWS_SESSION_TOKEN is defined, ansible uses deprecated AWS_SECURITY_TOKEN in the first place.

[tremble]

I'm generally in favour of this, but definitely necessitates a changelog entry. I'd like to hear from @Jill / @goneri about when it would be appropriate to merge this.

[Akasurde]

Add a note in the description that this value is deprecated.

[jillr]

@tremble Agree the change looks sensible. We don't have unit test coverage for get_aws_connection_info() right now so we'll want to manually test the change to be safe, it would be great if we could get that covered at some point but that exceeds the scope of this change. I'm +1 once we have updated docs and a changelog.

Thanks for this patch @nillovna!

[tremble]

@jillr We might not have unit tests but we do have an integration test which tests the combination of get_aws_connection_info and connect_to_aws : https://github.com/ansible-collections/amazon.aws/blob/main/tests/integration/targets/module_utils_ec2/roles/connect_to_aws/tasks/credentials.yml what we don't test right now is the relative priority of EC2_SESSION_TOKEN and EC2_SECURITY_TOKEN

[tremble]

@nillovna Do you think you could tweak https://github.com/ansible-collections/amazon.aws/blob/main/tests/integration/targets/module_utils_ec2/roles/connect_to_aws/tasks/credentials.yml to add a test that AWS_SESSION_TOKEN takes priority over AWS_SECURITY_TOKEN ?

[tremble]

Digging into this a little further:
https://github.com/boto/botocore/blob/develop/botocore/credentials.py#L1051

It looks like our current behaviour mimics the botocore behaviour (wrt ordering)

[jillr]

The next collection release of amazon.aws will be 2.0 (late May ETA), which can contain breaking changes. Comments and test failures will need to be addressed for this to be able to merge.

[alinabuzachis]

@jillr Should we put this in the Jira backlog?

[tremble]

@jillr, I'm inclined to close this PR. While the change would make a deprecated env var lower priority, this would go against the order botocore evaluates, and I think we're better sticking to boto3's priorities.

[jillr]

@tremble I'm ok with that. It doesn't appear to be causing any notable problems for users and avoiding breaking changes is usually desirable.

It was good to have folks that a look at this and now we know about the deprecation, thanks for raising awareness about this @nillovna! If boto3 changes the processing order and we need to revisit this we can revive the PR.