To fix the issue when it use assume role to create EC2 instance

[changyong]

What does this PR do?

To fix the issue when it use assume role to create AWS resources in another AWS account. The issue in #52501 is because when the variable __Expiration__ isn't equal to '', it will always return from the function directly. It won't have chance to execute provider.get('role_arn') and then get credential from assume role again.

What issues does this PR fix or reference?

Fix #52501

Tests written?

No

Commits signed with GPG?

No

[dwoz]

@changyong Is there test coverage for this change?

[changyong]

@dwoz I didn't write test code but tested it manually:

I executed following three test cases to verify the code changes and all the tests pass, it can create EC2 instances successfully:

1. Run salt-cloud to create EC2 through AWS access id and key.

provider settings:

test52501-id-key-provider:
  id: 'XXXXXXXXXXXXXXXXXXXX'
  key: 'XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX'
  driver: ec2
  private_key: /home/ychang/.ssh/dummy.pem
  keyname: dummy
  location: eu-central-1
  del_all_vols_on_destroy: True

output logs:

test52501-id-key-provider.log

2. Run salt-cloud to create EC2 through instance role in the same account.

provider settings:

test52501-instance-role-provider:
  id: 'use-instance-role-credentials'
  key: 'use-instance-role-credentials'
  driver: ec2
  private_key: /home/ychang/.ssh/dummy.pem
  keyname: dummy
  location: eu-central-1
  del_all_vols_on_destroy: True

output logs:

test52501-instance-role-provider.log

3. Run salt-cloud to create EC2 through assume role in another account.

provider settings:

test52501-assume-role-provider:
  id: 'use-instance-role-credentials'
  key: 'use-instance-role-credentials'
  role_arn: 'arn:aws:iam::111111111111:role/salt-cloud-cross-account'
  driver: ec2
  private_key: /home/ychang/.ssh/dummy.pem
  keyname: dummy
  location: eu-central-1
  del_all_vols_on_destroy: True

output logs:

test52501-assume-role-provider.log

The test-profile.conf profile file context is:

test52501-id-key-profile:
  provider: test52501-id-key-provider
  image: ami-0d60b7af1abf4434d
  ssh_username: ec2-user
  ssh_interface: private_ips
  location: eu-central-1
  subnetid: subnet-41b7d529
  securitygroupid:
    - sg-933ef6fb
  minion:
    master: 10.115.64.253
    hash_type: sha512
    file_roots:
      base:
        - /opt/salt/base
    pillar_roots:
      base:
        - /opt/salt/pillar
  script_args: -C
  size: t3.small
test52501-instance-role-profile:
  provider: test52501-instance-role-provider
  image: ami-0d60b7af1abf4434d
  ssh_username: ec2-user
  ssh_interface: private_ips
  location: eu-central-1
  subnetid: subnet-41b7d529
  securitygroupid:
    - sg-933ef6fb
  minion:
    master: 10.115.64.253
    hash_type: sha512
    file_roots:
      base:
        - /opt/salt/base
    pillar_roots:
      base:
        - /opt/salt/pillar
  script_args: -C
  size: t3.small
test52501-assume-role-profile:
  provider: test52501-assume-role-provider
  image: ami-09def150731bdbcc2
  ssh_username: ec2-user
  ssh_interface: private_ips
  location: eu-central-1
  subnetid: subnet-0f4e4dc3b1dfab30a
  securitygroupid:
    - sg-05949256929b1ab99
  minion:
    master: 10.115.64.253
    hash_type: sha512
    file_roots:
      base:
        - /opt/salt/base
    pillar_roots:
      base:
        - /opt/salt/pillar
  script_args: -C
  size: t3.small

BTW, I replaced some sensitive info in the settings and logs.

[major0]

Issue #52501 is still broken (not certain why it was closed) and this fix is still applicable. Was there any particular approach to a unit test that was needed to get this merged? I am currently impacted by this across 5 AWS accounts.

[thebluesnevrdie]

I am also interested in seeing this moving forward as it will affect me soon.

[twangboy]

@changyong In order for this to get in it needs to be rebased against the master branch and it needs to have tests written for it.

[sagetherage]

@major0 let's rebase this against master and write some tests, are you willing to help?

[sagetherage]

Discussed in Grooming on 2021-APR-05 and 2021-APR-19 and followed up with @major0 about closing this PR. I will wait 2 more weeks and if no response or work I will close.

[sagetherage]

Reviewed in Grooming 2021-MAY-03 keeping it open for @major0, needs test coverage and rebased against master branch, removing from grooming project

[tyhunt99]

Has anyone had a chance on getting the test required to merge this in? It would be a super nice fix to have.

[Ch3LL]

Anyone willing to push this PR to completion?

[Ch3LL]

Closing due to inactivity. Please let me know if you want me to re-open this PR or open a new PR against the master branch including the feedback in the PR