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

google_bigquery_dataset_iam_member doesn't handle deleted members #7896

Closed
joerayme opened this issue Nov 26, 2020 · 18 comments
Closed

google_bigquery_dataset_iam_member doesn't handle deleted members #7896

joerayme opened this issue Nov 26, 2020 · 18 comments
Assignees
Labels

Comments

@joerayme
Copy link

joerayme commented Nov 26, 2020

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 v0.13.5
+ provider registry.terraform.io/hashicorp/google v3.48.0
+ provider registry.terraform.io/hashicorp/google-beta v3.48.0

Affected Resource(s)

  • google_bigquery_dataset_iam_member

Debug Output

gist showing the output after the apply confirmation has been given: https://gist.github.com/joerayme/0f9ec66b4db461eece6690ceee0879e5

Panic Output

Expected Behavior

It should have ignored the deleted user

Actual Behavior

It failed to correctly manage the BigQuery dataset IAM policy with the following error:

Error: Error applying IAM policy for Bigquery Dataset <account>/<dataset>: Failed to parse BigQuery Dataset IAM member type: deleted:serviceAccount:<service-account>@<account>.iam.gserviceaccount.com?uid=100543859842954111727

Steps to Reproduce

provider "google" {
  version = "~> 3.49.0"
}

resource "google_bigquery_dataset" "test_dataset" {
  dataset_id    = "test_dataset"
  friendly_name = "test_dataset"
  description   = "this is a test dataset"
  location      = "europe-west2"
}

resource "google_service_account" "test_service_account" {
  account_id = "test-bigquery"
}

resource "google_bigquery_dataset_iam_member" "service_account" {
  dataset_id = google_bigquery_dataset.test_dataset.dataset_id
  role       = "roles/bigquery.dataOwner"
  member     = "user:${google_service_account.test_service_account.email}"
}
  1. terraform apply (ignore the error you get from the google_bigquery_dataset_iam_member, that's because we're using user instead of serviceAccount which is incorrect TF code, but it's useful to demonstrate this bug)
  2. Change the account_id attribute of google_service_account.test_service_account to something else (e.g. test-bigquery-2)
  3. terraform apply again

Important Factoids

References

@ghost ghost added bug labels Nov 26, 2020
@edwardmedia edwardmedia self-assigned this Nov 26, 2020
@edwardmedia
Copy link
Contributor

@joerayme can you provide the config along with the repro steps?

@ralbertazzi
Copy link

ralbertazzi commented Dec 1, 2020

Hi @joerayme, here are the steps to reproduce it:

  1. Create a dataset and two service accounts. Assign them roles on the dataset through google_bigquery_dataset_iam_member (in the example below I had to create the google_bigquery_dataset_iam_member resources after applying other resources to avoid the Terraform error: The "for_each" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created.)
terraform {
  required_version = ">= 0.13"

  required_providers {
    google      = "~> 3.49.0"
    google-beta = "~> 3.49.0"
  }
}

resource "google_service_account" a_service_account {
  account_id = "a-service-account"
}

resource "google_service_account" another_service_account {
  account_id = "another-service-account"
}

resource "google_bigquery_dataset" a_dataset {
  dataset_id = "a_dataset"
  location   = "US"
}

locals {
  service_accounts = [
    google_service_account.a_service_account.email,
    google_service_account.another_service_account.email,
  ]
}

resource google_bigquery_dataset_iam_member "reader" {
  for_each   = toset(local.service_accounts)
  dataset_id = google_bigquery_dataset.a_dataset.dataset_id
  member     = "serviceAccount:${each.key}"
  role       = "roles/bigquery.dataViewer"
}
  1. Manually deleted one of the two service accounts and perform an additional change on the IAM permissions of the dataset. In my example, I'm trying to delete the IAM role of the still existing service account:
terraform {
  required_version = ">= 0.13"

  required_providers {
    google      = "~> 3.49.0"
    google-beta = "~> 3.49.0"
  }
}

resource "google_service_account" a_service_account {
  account_id = "a-service-account"
}

// I DELETED THIS MANUALLY
//resource "google_service_account" another_service_account {
//  account_id = "another-service-account"
//}

resource "google_bigquery_dataset" a_dataset {
  dataset_id = "a_dataset"
  location   = "US"
}

locals {
  service_accounts = [
//    google_service_account.a_service_account.email,
//    google_service_account.another_service_account.email,
  ]
}

resource google_bigquery_dataset_iam_member "reader" {
  for_each   = toset(local.service_accounts)
  dataset_id = google_bigquery_dataset.a_dataset.dataset_id
  member     = "serviceAccount:${each.key}"
  role       = "roles/bigquery.dataViewer"
}

The diff of terraform apply is correct:

Terraform will perform the following actions:

  # google_bigquery_dataset_iam_member.reader["[email protected]"] will be destroyed
  - resource "google_bigquery_dataset_iam_member" "reader" {
      - dataset_id = "a_dataset" -> null
      - id         = "projects/ra-testing-42/datasets/a_dataset/roles/bigquery.dataViewer/serviceaccount:[email protected]" -> null
      - member     = "serviceAccount:[email protected]" -> null
      - project    = "ra-testing-42" -> null
      - role       = "roles/bigquery.dataViewer" -> null
    }

Plan: 0 to add, 0 to change, 1 to destroy.

However, when applying changes I get this error:

google_bigquery_dataset_iam_member.reader["[email protected]"]: Destroying... [id=projects/ra-testing-42/datasets/a_dataset/roles/bigquery.dataViewer/serviceaccount:[email protected]]

Error: Error when reading or editing Resource projects/ra-testing-42/datasets/a_dataset for IAM Member (role "serviceAccount:[email protected]", "roles/bigquery.dataViewer"): Error applying IAM policy for Bigquery Dataset ra-testing-42/a_dataset: Failed to parse BigQuery Dataset IAM member type: deleted:serviceAccount:[email protected]?uid=115133331797674156835

Interesting enough, the bug doesn't seem to happen if using two separate google_bigquery_dataset_iam_member resources (i.e. not using the for_each)

@ghost ghost removed waiting-response labels Dec 1, 2020
@edwardmedia
Copy link
Contributor

edwardmedia commented Dec 1, 2020

@ralbertazzi is this the same issue as @joerayme originally reported?

As you mentioned, this can't be repro by using two separate resources. The issue is likely beyond the provider plugin, which is in the area about how to use the for_each

By introducing the dynamic code & variables, basically you break the relationship between resources. Have you tried by adding depend_on in the google_bigquery_dataset_iam_member?

@ralbertazzi
Copy link

ralbertazzi commented Dec 2, 2020

@edwardmedia which explicit dependency should I define in google_bigquery_dataset_iam_member? The service account?

By the way, we have this problem in a project in which we assign permissions to many service accounts from other projects. This means that if somebody in project B deletes a service account, our Terraform definition in project A (the one with google_bigquery_dataset_iam_member) breakes and we have to run some manual scripts to clean it. This also means that we cannot define explicit dependencies as you were suggesting.

Could the issue be related to the Terraform provider not being able to correctly parse deleted: entries in the IAM policies? Seems like GCP automatically converts IAM policy entries for deleted entities (users, service accounts, ..) by adding this deleted: prefix to them. This not only applies to BigQuery datasets but also to any other type of Google resource (a GCS bucket, a Pub/Sub resource, a project, ...)

@ghost ghost removed the waiting-response label Dec 2, 2020
@joerayme
Copy link
Author

joerayme commented Dec 2, 2020

I've added more detailed steps to reproduce the problem I encountered in the original issue above ☝️ I think it's probably a simpler version of @ralbertazzi 's.

The issue here is when changes are made that are outside of perfectly-written Terraform which is managing both the service account and the full IAM policy of the bigquery (and possibly other types) resource.

Let me know if that makes sense.

@edwardmedia
Copy link
Contributor

@joerayme I have just run your code. After updating the account_id, applying the new change is fine with me. I see both resources (google_service_account and google_bigquery_dataset_iam_member) are successfully recreated. Can you share your debug log so I can take a close look?

@joerayme
Copy link
Author

joerayme commented Dec 3, 2020

Here's the gist for when I follow the above steps to reproduce after having changed the name of the service account.

Looking at the code it appears that it's not expecting to find deleted: at the start of the IAM member.

@ghost ghost removed waiting-response labels Dec 3, 2020
@ralbertazzi
Copy link

ralbertazzi commented Dec 3, 2020

@joerayme on a side note, I also think that this will fail in the context of this toy example, but that could be a separate issue:

  1. Create a service account A
  2. Assign IAM permission to it
  3. Delete the service account A
  4. Re-create the service account A with the same name (aka email)
  5. Re-apply the IAM permission to it <- FAIL

The reason why you want to perform step 5 (i.e. reassign the permission) is that GCP internally trackes IAM using service account IDs, which are unique even across re-creation of a seervice account with the same name. But, when applying, I think that GCP will complain about the unfeasible existence of both the deleted and the new member in the IAM policy, which is refused. To make it work you first have to remove the deleted: entry from the IAM policy than apply the new one.

@edwardmedia
Copy link
Contributor

edwardmedia commented Dec 7, 2020

@ralbertazzi this is not the use case for Terraform. You should try to avoid manual updating resources. If you have to, you may run import to sync after that. If you allow different groups to manage same resources, does remote state help?

@ralbertazzi
Copy link

@edwardmedia unfortunately this happens in our organization. We usually have one Terraform configuration per project, but in some projects we assign IAM roles to service accounts from other projects (i.e. managed in another Terraform configuration), which means that we don't have full visibility of what happens in other projects.

@edwardmedia
Copy link
Contributor

edwardmedia commented Dec 7, 2020

@joerayme reviewing your comments, changes applied outside Terraform might not be a good Terraform use case either.

@edwardmedia
Copy link
Contributor

@joerayme @ralbertazzi up to now, I don't see anything breaks in the provider plugin. Closing the issue then. Feel free to reopen the issue if you need further conversation. Thank you

@pietrodn
Copy link

pietrodn commented Dec 9, 2020

@joerayme what if we are tracking some dataset permissions with google_bigquery_dataset_iam_member and a service account, whose permissions we don't track in the Terraform state, is deleted?
It would give a parsing error.

google_bigquery_dataset_iam_member is non-authoritative, therefore it should handle gracefully (i.e., ignore) the changes in the permissions that are not tracked in Terraform.

@joerayme
Copy link
Author

joerayme commented Dec 9, 2020

Exactly. This exact issue has cropped up for us; it's why I opened the ticket. Things happen and my Terraform shouldn't break because someone accidentally deleted a service account or even offboarded a member of staff that's managed by some other bit of Terraform. Deleted members are handled elsewhere and a bug related to this was even opened pre-emptively (#7278) so I don't see why this should be any different.

@pietrodn
Copy link

pietrodn commented Dec 9, 2020

Oops, I meant to tag @edwardmedia instead 😄 but yes, that's my concern.

@ralbertazzi
Copy link

ralbertazzi commented Dec 14, 2020

@edwardmedia GCP added the deleted: prefix after a change in September 14, 2020. This was quite an important change as people received information about it by email. These are some interesting parts from the email that I received at that time:

Roles that are granted to deleted accounts will be clearly labeled as deleted accounts, allowing you to differentiate between roles granted to active or deleted accounts with a particular given email.

[...]

If you manage IAM Policies with infrastructure as code tools such as Terraform:
* When a user, group, or service account is labeled as deleted on IAM policy, you should remove the deleted member from your master copy of the policy (this step is already required after accounts are irrevocably purged). Once an account is purged, setting IAM Policy with references to the account is not supported.
* You may also need to update configuration-as-code tools to recognize the new prefix. For example, if you use the Google provider for Terraform, be sure you have updated to at least version 3.3.0.

You may also need to update configuration-as-code tools to recognize the new prefix.

I think that the error Failed to parse BigQuery Dataset IAM member type: deleted:serviceAccount:<service-account>@<account>.iam.gserviceaccount.com is a clear indication that action needs to be taken in the provider to be compliant with Google's guidelines

@ghost
Copy link

ghost commented Jan 7, 2021

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Jan 7, 2021
@edwardmedia
Copy link
Contributor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants