Handle role removal correctly in IAM instance profiles.

[pgavlin]

If an aws_iam_instance_profile resource is created with role set (vs.
roles, which is deprecated) then the resulting resource is written
to the state file with only role populated.

An IAM instance profile cannot be deleted as long as there is a role
attached.

If an attempt is made to delete the instance profile resource without an
intervening refresh, the provider will read the (empty) roles property,
detach no roles, then try to delete the resource. The delete will fail.
Note that most users won't see this because apply and plan refresh first,
and the code for Get always sets roles.

These changes consider the value of role in
instanceProfileRemoveAllRoles before considering roles. If role is
populated, the role named therein is removed. The removed role is
tracked s.t. if it is also present in roles no attempt is made to
remove it twice.

[pgavlin]

Fixes #5.

[lukehoban]

Fixes pulumi/pulumi-aws#41, right?

[lukehoban]

And also fixes hashicorp/terraform-provider-aws#1777?

[pgavlin]

Fixed pulumi/pulumi-aws#41, right?

Sort of? It fixes the underlying issue, but I think that issue effectively tracks updating our examples and pulumi/cloud to use role.

And also fixes hashicorp/terraform-provider-aws#1777?

Yes.

[lukehoban]

lgtm

Would be great if we could add a test for this condition. As I noted in pulumi/pulumi-aws#41, it's not entirely clear why existing tests don't hit this.

It may be that this case is unreachable via terraform and only reachable via how we invoke Terraform Providers. If so, we should understand that and whether that is something we need to fix more generally.

[joeduffy]

I am still not following one thing: Are we invoking the Terraform provider "incorrectly"?

[pgavlin]

Given my limited understanding of Terraform, I suspect that this is not something that is reachable via the terraform CLI or tests, but I do believe that we are invoking the APIs correctly. At a cursory glance I was unable to find any documentation that implies that a refresh is necessary before a delete.

[lukehoban]

Are we invoking the Terraform provider "incorrectly"?

That's my question exactly. Or more specifically - why is terraform never appearing to hit this issue, but pulumi does?

If we could write a terraform test that exercised this issue, that would prove that it's an issue that is also an observable bug via terraform. Without that, it may be hard to upstream this.

And if it's not observable from terraform, then presumably it is part of a class of bugs we're going to have when providers make certain terraform-compatible assumptions - and we should try to understand that class of bugs and identify if there is anything we could/should do to address it.

At a cursory glance I was unable to find any documentation that implies that a refresh is necessary before a delete.

Unfortunately we can't really rely just on documentation - we have to generally support whatever providers assume and do in practice - as our goal is to be compatible with (effectively) all of them.

[pgavlin]

If we could write a terraform test that exercised this issue, that would prove that it's an issue that is also an observable bug via terraform. Without that, it may be hard to upstream this.

I think we might actually be able to write a test for this: IIUC we can set the PreventPostDestroyRefresh field on resource.TestCase which will keep terraform from performing a resource before destruction.

FWIW, this might actually be observable from terraform: it turns out that there is another potential hit in hashicorp/terraform-provider-aws#2241.

[pgavlin]

IIUC we can set the PreventPostDestroyRefresh field on resource.TestCase which will keep terraform from performing a resource before destruction.

Never mind; I did not understand correctly. The Terraform tests always perform a Refresh as the first part of every test step, including the terminal destroy.

[pgavlin]

Luke and I discussed offline what we think the path forward is here. For now, we've decided to take and upstream this change in order to unblock ourselves. We will also separately experiment with performing Refresh operations before each Apply.

[pgavlin]

(the latter is tracked by https://github.com/pulumi/pulumi-terraform/issues/57)