Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provider produced inconsistent result after apply #10193

Closed
sethvargo opened this issue Sep 29, 2021 · 14 comments · Fixed by GoogleCloudPlatform/magic-modules#9727, hashicorp/terraform-provider-google-beta#6818 or #16927

Comments

@sethvargo
Copy link
Contributor

sethvargo commented Sep 29, 2021

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request.
  • Please do not leave +1 or me too comments, they generate extra noise for issue followers and do not help prioritize the request.
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment.
  • If an issue is assigned to the modular-magician user, it is either in the process of being autogenerated, or is planned to be autogenerated soon. If an issue is assigned to a user, that user is claiming responsibility for the issue. If an issue is assigned to hashibot, a community member has claimed the issue already.

Terraform Version

Terraform v1.0.7
on darwin_amd64
+ provider registry.terraform.io/hashicorp/google v3.86.0
+ provider registry.terraform.io/hashicorp/google-beta v3.86.0

Affected Resource(s)

  • google_service_account

Terraform Configuration Files

resource "google_service_account" "runner" {
  project = var.project_id

  account_id   = "runner"
  display_name = "GitHub Actions runner"

  depends_on = [
    google_project_service.services["iam.googleapis.com"],
  ]
}

resource "google_project_iam_member" "runner" {
  project = var.project_id

  for_each = toset([
    "roles/logging.logWriter",
    "roles/monitoring.metricWriter",
    "roles/stackdriver.resourceMetadata.writer",
  ])

  role   = each.key
  member = "serviceAccountId:${google_service_account.runner.unique_id}"
}

Debug Output

https://gist.github.com/sethvargo/6d940d2d95bf373f2a1d1b2f279ac5b9

Expected Behavior

Apply should work.

Actual Behavior

│ Error: Provider produced inconsistent result after apply
│
│ When applying changes to google_project_iam_member.runner["roles/logging.logWriter"], provider "provider[\"registry.terraform.io/hashicorp/google\"]" produced an unexpected new value: Root resource was present, but now
│ absent.
│
│ This is a bug in the provider, which should be reported in the provider's own issue tracker.

Steps to Reproduce

  1. terraform apply

Important Factoids

None

References

b/304724862

@sethvargo sethvargo added the bug label Sep 29, 2021
@sethvargo
Copy link
Contributor Author

The issue appears to be related to use serviceAccountId instead of serviceAccount.

@edwardmedia edwardmedia self-assigned this Sep 30, 2021
@edwardmedia
Copy link
Contributor

edwardmedia commented Oct 1, 2021

@sethvargo yes. Below are the member formats that are supported. Closing this issue then

https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/google_project_iam#member/members

  • user:{emailid}: An email address that represents a specific Google account. For example, alice@gmail.com or joe@example.com.
  • serviceAccount:{emailid}: An email address that represents a service account. For example, my-other-app@appspot.gserviceaccount.com.
  • group:{emailid}: An email address that represents a Google group. For example, admins@example.com.
  • domain:{domain}: A G Suite domain (primary, instead of alias) name that represents all the users of that domain. For example, google.com or example.com.

@sethvargo
Copy link
Contributor Author

@edwardmedia okay, but that doesn't match the list of allowed values IAM accepts for principals. Service account emails are re-usable, but service account IDs are unique and are not subject to eventual consistency challenges like emails.

@sethvargo
Copy link
Contributor Author

At the very least, if you don't want to support this, Terraform should catch this and raise a more specific error. The current error specifically notes that this is a bug in the provider and should be reported upstream. /cc @rileykarson

@rileykarson
Copy link
Collaborator

We'll probably have to validate away serviceAccountId given that the API turns it into the service account email format in the response. Looking at your debug logs, here's the POST request and response snippet:

   {
    "members": [
     "serviceAccount:runner@sv-gh-patproxy.iam.gserviceaccount.com",
     "serviceAccountId:117089295354523313186"
    ],
    "role": "roles/stackdriver.resourceMetadata.writer"
   }
  {
   "role": "roles/stackdriver.resourceMetadata.writer",
   "members": [
    "serviceAccount:runner@sv-gh-patproxy.iam.gserviceaccount.com",
    "serviceAccount:runner@sv-gh-patproxy.iam.gserviceaccount.com"
   ]
  }

We could probably figure it out and handle this correctly in a CustomizeDiff if we needed to, but could be pretty annoying to get right so disallowing the format is probably how we want to start out. The Terraform runner needs https://cloud.google.com/iam/docs/reference/rest/v1/projects.serviceAccounts/get permissions to turn the unique id into the email, and may not have them granted, for one.

Duplicate entries showing up is, uh, some extra fun. It should work w/o issues, but it may be worth having the onduty trace through the IAM code to see if it'll cause any issues.

@sethvargo: Do you happen to know where we'd find an updated list of member types offhand? That's a new one to me, and doesn't show up at https://cloud.google.com/resource-manager/reference/rest/Shared.Types/Binding.

@rileykarson rileykarson reopened this Oct 1, 2021
@sethvargo
Copy link
Contributor Author

Do you happen to know where we'd find an updated list of member types offhand?

I'll ping you a link privately.

@rileykarson
Copy link
Collaborator

Thanks! For Googlers: go/iam-types

@nat-henderson
Copy link
Contributor

Yeah - that's going to be pretty challenging to work out, I'm not sure if an API endpoint exists that we could use to check that the ID is equal to the email address that gets returned. Hm.

I'll investigate to see what can be done, but I suspect this is going to get "persistent bug" treatment - this whole area could use a cleanup, since I notice many items on go/iam-types that we don't support..

@sethvargo
Copy link
Contributor Author

@ndmckinley - could we just reject these types entirely at graph time? Basically, we can create an allowlist of valid prefixes that are known to work (or a denylist of those that fail) and bail the Terraform run early. That would avoid the "this is always a bug in the provider" error. You could also ping cache to see if he knows of an endpoint you could query.

@rileykarson
Copy link
Collaborator

@ndmckinley: See #10193 (comment), an endpoint to get the email exists but is gonna need extra permissions so we probably don't want to use it. Using validation to reject the value will stop users hitting permadiffs, at least.

I think a broader IAM fix is going to need a breaking change in order to completely remove the case-insensitivity behaviour in the provider / lock it behind certain principal types, definitely a larger change than the bug-at-hand.

@nat-henderson
Copy link
Contributor

Ah, of course - I forgot that CustomizeDiff actually got an authenticated Config object and didn't think we'd be able to use it for that, but just checked, reminded myself that it does.

Okay, acknowledged, this bug is exclusively to reject the types we can't currently handle.

@benhxy
Copy link

benhxy commented Dec 12, 2023

@edwardmedia okay, but that doesn't match the list of allowed values IAM accepts for principals. Service account emails are re-usable, but service account IDs are unique and are not subject to eventual consistency challenges like emails.

Hi, service account team here.

Even though service account emails are reusable, in IAM policy under the hood, we differentiate service accounts with the same email with unique IDs. If a service account is deleted, purged, and recreated again using the same email, a policy binding created for the old service account won't work on the new service account. The old account binding will be marked as "deleted:serviceaccount:" in IAM policy before it's eventually purged, and you can add a new policy binding for the new account.

Hence you can safely use service account email in TF IAM policy module.

@benhxy
Copy link

benhxy commented Dec 12, 2023

We can consider adding 409 handling (if 409, do GetServiceAccount) in https://github.com/hashicorp/terraform-provider-google/blob/main/google/services/resourcemanager/resource_google_service_account.go#L117-L122 to adapt to the eventual consistency of account creation.

Copy link

github-actions bot commented Feb 8, 2024

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.