Bug 1684951: pkg/destroy/aws: Use GetSession() to prompt when missing #1365

wking · 2019-03-05T08:25:17Z

Avoid looping forever with:

INFO get tagged resources: NoCredentialProviders: no valid providers in chain. Deprecated.
    For verbose messaging see aws.Config.CredentialsChainVerboseErrors

and so on for all the other calls. With this commit, I'm allowing callers to provide their own session, which they can pre-check as they like.

I'm not adjusting pkg/destroy/aws to use GetSession internally, because it would drag in a bunch of mostly-unrelated dependencies that pkg/destroy/aws consumers like Hive won't want. And Hive's consumer isn't interactive anyway, so our prompting logic would not be a good fit for them. With this commit, we can use our prompting GetSession without forcing it onto Hive.

staebler · 2019-03-05T14:35:04Z

I get this output when I run destroy cluster with no profile.

? AWS Access Key ID AKsomethingsomething
? AWS Secret Access Key [? for help] ****************************************
INFO Writing AWS credentials to "/home/staebler/.aws/credentials" (https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html) 
INFO Writing AWS credentials to "/home/staebler/.aws/credentials" (https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html) 
DEBUG search for and delete matching resources by tag in us-east-2 matching aws.Filter{"kubernetes.io/cluster/mstaeble-zfgzp":"owned"} 
INFO get tagged resources: MissingRegion: could not find region configuration 
DEBUG search for and delete matching resources by tag in us-east-2 matching aws.Filter{"openshiftClusterID":"04fc8b89-6895-4693-b257-a01170b65f2f"} 
INFO get tagged resources: MissingRegion: could not find region configuration 
DEBUG search for and delete matching resources by tag in us-east-1 matching aws.Filter{"kubernetes.io/cluster/mstaeble-zfgzp":"owned"} 
DEBUG search for and delete matching resources by tag in us-east-1 matching aws.Filter{"openshiftClusterID":"04fc8b89-6895-4693-b257-a01170b65f2f"} 
DEBUG no deletions from us-east-1, removing client 
DEBUG search for IAM roles                         
DEBUG search for IAM users                         
DEBUG search for and delete matching resources by tag in us-east-2 matching aws.Filter{"kubernetes.io/cluster/mstaeble-zfgzp":"owned"} 
INFO get tagged resources: MissingRegion: could not find region configuration 
DEBUG search for and delete matching resources by tag in us-east-2 matching aws.Filter{"openshiftClusterID":"04fc8b89-6895-4693-b257-a01170b65f2f"} 
INFO get tagged resources: MissingRegion: could not find region configuration 
DEBUG search for IAM roles                         
DEBUG search for IAM users                         
DEBUG search for and delete matching resources by tag in us-east-2 matching aws.Filter{"kubernetes.io/cluster/mstaeble-zfgzp":"owned"} 
INFO get tagged resources: MissingRegion: could not find region configuration 
DEBUG search for and delete matching resources by tag in us-east-2 matching aws.Filter{"openshiftClusterID":"04fc8b89-6895-4693-b257-a01170b65f2f"} 
INFO get tagged resources: MissingRegion: could not find region configuration 
DEBUG search for IAM roles                         
DEBUG search for IAM users                         
...

This has two issue that I see.

It creates the AWS credentials flie when we may not want it to.
It does not work, and still loops endlessly, because there is no region specified.

wking · 2019-03-05T15:36:02Z

It creates the AWS credentials flie when we may not want it to.

Why would this be different from the create case? I'm fine with "hold creds in memory" or "write creds to disk", but don't see a reason to use different approaches for create and destroy.

It does not work, and still loops endlessly, because there is no region specified.

I thought clients were regioned, but the session was global? But yeah, obviously something going on there...

wking · 2019-03-07T05:35:54Z

DEBUG search for and delete matching resources by tag in us-east-2 matching aws.Filter{"kubernetes.io/cluster/mstaeble-zfgzp":"owned"} 
INFO get tagged resources: MissingRegion: could not find region configuration

Ok, I've rebased onto master with 1c25463d2 -> 72319c7f4, which also adds a Session.Copy(...) call which I think will address this.

pkg/destroy/aws.go

pkg/destroy/aws/aws.go

Avoid looping forever with [1]: INFO get tagged resources: NoCredentialProviders: no valid providers in chain. Deprecated. For verbose messaging see aws.Config.CredentialsChainVerboseErrors and so on for all the other calls. With this commit, I'm allowing callers to provide their own session, which they can pre-check as they like. I'm not adjusting pkg/destroy/aws to use GetSession internally, because it would drag in a bunch of mostly-unrelated dependencies that pkg/destroy/aws consumers like Hive [2] won't want. And Hive's consumer isn't interactive anyway, so our prompting logic would not be a good fit for them. With this commit, *we* can use our prompting GetSession without forcing it onto Hive. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1684951 [2]: https://github.com/openshift/hive/blob/a9ffb5af700bdc46dab2a880ec8414cd9e99242e/contrib/cmd/hiveutil/awstagdeprovision.go#L24 Issue: https://bugzilla.redhat.com/show_bug.cgi?id=1684951

wking · 2019-04-05T17:13:59Z

Rebased around #1268 with bb4396613 -> 419c5a7.

wking · 2019-04-05T20:57:04Z

All green. @abhinavdahiya, @staebler, can you take another look?

pkg/destroy/aws/aws.go

abhinavdahiya · 2019-04-15T23:27:42Z

/lgtm

openshift-ci-robot · 2019-04-15T23:27:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2019-04-16T10:26:41Z

/retest