hashicorp / terraform-provider-google

Terraform Provider for Google Cloud Platform
https://registry.terraform.io/providers/hashicorp/google/latest/docs
Mozilla Public License 2.0
2.28k stars 1.72k forks source link

google_bigquery_dataset_access disappears after editing google_bigquery_dataset_iam_* #12278

Open N214 opened 2 years ago

N214 commented 2 years ago

Hello,

I want to discuss the issue #10156 that has been locked.

I have had exactly the same problem and it has been tedious to find the culprit since the Terraform config is maintained by different people in the Team. I was not aware that google_bigquery_datasetiam would delete all the access created by google_bigquery_dataset_access. Indeed, there is a warning on the official documentation page but maybe it's worth considering to integrate that warning in terraform code, while deploying google_bigquery_datasetiam ?

Because, having a resource (dataset_iam) deleting another resource (dataset_access) without any warning on the CLI is not what people would expect as behaviour ?

b/321924266

jamiet-msm commented 1 year ago

This is tricky because its the underlying API that (IMO) is at fault here. Nevertheless, it would be handy if the TF provider could mitigate this.

We first mitigated this by putting a big warning at the top of any TF files that manipulate BigQuery dataset IAM:

#_________ _______  _______  _______  _______ _________ _______  _       _________
#\__   __/(       )(  ____ )(  ___  )(  ____ )\__   __/(  ___  )( (    /|\__   __/
#   ) (   | () () || (    )|| (   ) || (    )|   ) (   | (   ) ||  \  ( |   ) (
#   | |   | || || || (____)|| |   | || (____)|   | |   | (___) ||   \ | |   | |
#   | |   | |(_)| ||  _____)| |   | ||     __)   | |   |  ___  || (\ \) |   | |
#   | |   | |   | || (      | |   | || (\ (      | |   | (   ) || | \   |   | |
#___) (___| )   ( || )      | (___) || ) \ \__   | |   | )   ( || )  \  |   | |
#\_______/|/     \||/       (_______)|/   \__/   )_(   |/     \||/    )_)   )_(
# Please never use
#  google_bigquery_dataset_iam_member
#  google_bigquery_dataset_iam_policy
#  google_bigquery_dataset_iam_binding
# https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/bigquery_dataset_iam
# to grant access to this dataset. We create authorised views on top of that dataset and any use of
# those resources will clobber those authorised views.

This obviously isn't failsafe though so we now have a check in our CI pipeline that greps the codebase and throws an error if it finds "google_bigquery_dataset_iam_member". We use GitHub Actions so we wrote our own action to do this:

name: 'prevent use of BigQuery dataset IAM'
description: 'We make extensive use of authorised views and authorised datasets hence we want to avoid the use of the terraform resources that can cause them to be removed.'
runs:
  using: "composite"
  steps:
    - name: Checkout code
      uses: actions/checkout@v3
    - name: Check no google_bigquery_dataset_iam_* resources are being used
      shell: bash
      run: |
        RED_BACKGROUND='\033[41m'
        NC='\033[0m'       # No colour
        re='\"google_bigquery_dataset_iam_member\"|\"google_bigquery_dataset_iam_binding\"|\"google_bigquery_dataset_iam_policy\"'
        if [ "$(grep -rnI -E ${re} . | wc -l)" -gt 0 ]; then
          # grep returns non-zero exit code if it finds zero matches so the next line would cause CI job failure. Hence I'm running it once its already been decided some matches have been found
          INVALID_RESOURCES=$(grep -rnI -E ${re} .)
          echo "$INVALID_RESOURCES"
          echo -e "${RED_BACKGROUND}"There are "$(echo "$INVALID_RESOURCES" | wc -l)" occurrences of google_bigquery_dataset_iam_member, google_bigquery_dataset_iam_binding or google_bigquery_dataset_iam_policy"${NC}"
          echo -e "Please read https://link/to/internal/jira/JIRA-X to understand why this is not allowed"
          exit 1
        fi

Its a bit makeshift, but it works

killthekitten commented 1 year ago

@jamiet-msm thanks for posting your workaround! Even though this doesn't solve the underlying issue, it is useful for debugging purposes.

@edwardmedia I believe assigning the "question" label to this issue has been a mistake, and this is actually a bug on the providers' side. I can see that Terraform sends an ACL update request every time it applies the google_bigquery_dataset_iam_member policy.

Looks like the ACL policy is being copied from some source (be it the API or the terraform state), and then re-applied, omitting a certain type of ACL entries, perhaps the ones containing viewName or missing the email or role fields. It could be this bit of code:

https://github.com/hashicorp/terraform-provider-google/blob/96af27eecd98c6fdcce16bbe83dffd2a42d55e1b/google/iam_bigquery_dataset.go#L129-L135

Here's how the full request looks when the entry is being deleted:

{
  "protoPayload": {
    "@type": "type.googleapis.com/google.cloud.audit.AuditLog",
    "status": {},
    "authenticationInfo": {
      "principalEmail": "nikolay@example.com"
    },
    "requestMetadata": {
      "callerIp": "-----------",
      "callerSuppliedUserAgent": "Terraform/1.3.6 (+https://www.terraform.io) Terraform-Plugin-SDK/2.10.1 terraform-provider-google/dev,gzip(gfe)",
      "requestAttributes": {},
      "destinationAttributes": {}
    },
    "serviceName": "bigquery.googleapis.com",
    "methodName": "datasetservice.update",
    "authorizationInfo": [
      {
        "resource": "projects/example/datasets/some_dataset",
        "permission": "bigquery.datasets.update",
        "granted": true,
        "resourceAttributes": {}
      }
    ],
    "resourceName": "projects/example/datasets/some_dataset",
    "serviceData": {
      "@type": "type.googleapis.com/google.cloud.bigquery.logging.v1.AuditData",
      "datasetUpdateRequest": {
        "resource": {
          "datasetName": {
            "projectId": "example",
            "datasetId": "some_dataset"
          },
          "info": {},
          "acl": {
            "entries": [
              {
                "role": "roles/bigquery.dataEditor",
                "specialGroup": "PROJECT_WRITERS",
                "viewName": {}
              },
              {
                "role": "roles/bigquery.dataOwner",
                "specialGroup": "PROJECT_OWNERS",
                "viewName": {}
              },
              {
                "role": "roles/bigquery.dataOwner",
                "userEmail": "user@example.com",
                "specialGroup": "PROJECT_OWNERS",
                "viewName": {}
              },
              {
                "role": "roles/bigquery.dataViewer",
                "userEmail": "some-dataset@example.iam.gserviceaccount.com",
                "specialGroup": "PROJECT_OWNERS",
                "viewName": {}
              },
              {
                "role": "roles/bigquery.dataViewer",
                "userEmail": "some-service@example.iam.gserviceaccount.com",
                "specialGroup": "PROJECT_OWNERS",
                "viewName": {}
              }
            ]
          }
        }
      },
      "datasetUpdateResponse": {
        "resource": {
          "datasetName": {
            "projectId": "example",
            "datasetId": "some_dataset"
          },
          "info": {},
          "createTime": "2022-11-25T09:31:41.611Z",
          "updateTime": "2023-01-04T17:31:56.725Z",
          "acl": {
            "entries": [
              {
                "role": "WRITER",
                "specialGroup": "PROJECT_WRITERS",
                "viewName": {}
              },
              {
                "role": "OWNER",
                "specialGroup": "PROJECT_OWNERS",
                "viewName": {}
              },
              {
                "role": "OWNER",
                "userEmail": "user@example.com",
                "specialGroup": "PROJECT_OWNERS",
                "viewName": {}
              },
              {
                "role": "READER",
                "userEmail": "some-dataset@example.iam.gserviceaccount.com",
                "specialGroup": "PROJECT_OWNERS",
                "viewName": {}
              },
              {
                "role": "READER",
                "userEmail": "some-service@example.iam.gserviceaccount.com",
                "specialGroup": "PROJECT_OWNERS",
                "viewName": {}
              }
            ]
          }
        }
      }
    }
  },
  "insertId": "1e3oejdm60c",
  "resource": {
    "type": "bigquery_resource",
    "labels": {
      "project_id": "example"
    }
  },
  "timestamp": "2023-01-04T17:31:57.921838Z",
  "severity": "NOTICE",
  "logName": "projects/example/logs/cloudaudit.googleapis.com%2Factivity",
  "receiveTimestamp": "2023-01-04T17:31:58.685137767Z"
}

And here is the missing entry:

{
  "specialGroup": "PROJECT_OWNERS",
  "viewName": {
    "projectId": "example",
    "datasetId": "some_dataset",
    "tableId": "some_table"
  }
}

This is pretty serious, as it caused us a downtime multiple times over the recent months. Let me know if I can help in any way to resolve this issue!

rileykarson commented 11 months ago

IMO the issue here is not really the API, it's that we added these resources to Terraform in the first place. These are IAM-like resources and not IAM resources that are redundant with resource-level settings and with https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/bigquery_dataset_access. They should have had different names to disambiguate them from real IAM resources or not been added, since unless you're reading the warnings on the page you'll easily miss that they will fight with each other.

melinath commented 7 months ago

Note from triage: One potential option here would be to deprecate the fake iam resources in favor of dataset access, but there could also be other paths forward. Unclear what is best / what the impact on users would be.

walterjking commented 7 months ago

the google_bigquery_dataset_access docs mention the issues with google_bigquery_dataset, but not google_bigquery_datasetiam*. i know the iam page mentions it but we missed thinking we were ok from reading the access doc since were werent using the dataset's access block, and we werent duplicating any roles

personally id rather work with the iam_ resources since that's how you interact with iam for most/all other resource types and so i think its easier to look at them and know how they work. whether theyre iam-like or iam isnt something as an end user i really see as a difference, aside from views/functions being different, as its still just assigning permissions to an object. in particular we like google_bigquery_dataset_iam_binding as a good mix of authoritativeness(which is a goal and bq_dataset_access doesnt support) and readability/conciseness(vs 50 data_access resources or a 200 line gogole_bigquery_dataset object)