Add IAM Conditions support; enable it in service account IAM #2372

danawillow · 2019-09-25T00:48:33Z

Part of hashicorp/terraform-provider-google#2909.

This adds the conditions properties to all IAM resources (except, I think, google_project_iam_policy because that one does its own thing). To keep this PR from getting too unwieldy (and because certain resources seem to be broken), I've only enabled and documented it in the service account IAM resources. Technically this means that if this change is released on its own, people will be able to set the conditions field in other IAM resources, but it would fail at apply-time because we didn't set the version to 3. I think this is fine, but if we don't want that to happen we can time this PR with any follow-ups so they appear in a release together.

This change also adds a bunch of the scaffolding to the GA provider without adding full support for the feature, mainly to limit the number of erb tags in the code. Let me know if that's confusing and I can adjust.

The other confusing behavior is around users who have changed IAM conditions out-of-band. We
have no way to know whether a given binding with no condition (and no equivalent binding upstream but one with a condition upstream) had a condition added out-of-band (and thus TF should clobber the condition) or if it is intentionally being created as one with no conditions (and thus TF should add a new binding with no conditions). That, combined with the fact that we allow bindings with no members, leads to some weird diffs:
Config:

resource "google_service_account_iam_binding" "foo" {
  provider = "google-beta"

  service_account_id = "${google_service_account.test_account.name}"
  role        = "roles/iam.serviceAccountUser"
  members     = ["user:[email protected]"]
}

If I add a condition out of band, I get this diff:

  # google_service_account_iam_binding.foo will be updated in-place
  ~ resource "google_service_account_iam_binding" "foo" {
        etag               = "BwWTVHk6K4g="
        id                 = "projects/graphite-test-danahoffman-tf/serviceAccounts/[email protected] roles/iam.serviceAccountUser"
      ~ members            = [
          + "user:[email protected]",
        ]
        role               = "roles/iam.serviceAccountUser"
        service_account_id = "projects/graphite-test-danahoffman-tf/serviceAccounts/[email protected]"
    }

If applied, this would add a new binding with this member list and no condition, and leave the one with the condition no longer managed by TF.

If I add a condition out of band, then add it to my config as such:

resource "google_service_account_iam_binding" "foo" {
  provider = "google-beta"

  service_account_id = "${google_service_account.test_account.name}"
  role        = "roles/iam.serviceAccountUser"
  members     = ["user:[email protected]"]
  condition {
    title       = "expires_after_2019_12_31"
    description = "Expiring at midnight of 2019-12-31"
    expression  = "request.time < timestamp(\"2020-01-01T00:00:00Z\")"
  }
}

Then the diff looks like this:

  # google_service_account_iam_binding.foo will be updated in-place
  ~ resource "google_service_account_iam_binding" "foo" {
        etag               = "BwWTVHk6K4g="
        id                 = "projects/graphite-test-danahoffman-tf/serviceAccounts/[email protected] roles/iam.serviceAccountUser"
      ~ members            = [
          + "user:[email protected]",
        ]
        role               = "roles/iam.serviceAccountUser"
        service_account_id = "projects/graphite-test-danahoffman-tf/serviceAccounts/[email protected]"

      + condition {
          + description = "Expiring at midnight of 2019-12-31"
          + expression  = "request.time < timestamp(\"2020-01-01T00:00:00Z\")"
          + title       = "expires_after_2019_12_31"
        }
    }

And applying it is effectively a no-op.

I've been staring at this for too long and could use some fresh eyes to see if there's a way to improve this UX. @emilymye, any chance you'll have time before your break to do a thorough review?

Release Note Template for Downstream PRs (will be copied)

iam: added support for IAM Conditions to the `google_service_account_iam_*` resources (beta provider only)

`google_service_account_iam_*` resources now support IAM Conditions. If any conditions had been created out of band before this release, take extra care to ensure they are present in your Terraform config so the provider doesn't try to create new bindings with no conditions. Terraform will show a diff that it is adding the condition to the resource, which is safe to apply.

modular-magician · 2019-09-25T00:55:46Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 97c97a6.

Pull request statuses

No diff detected in terraform-google-conversion.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#1188
depends: hashicorp/terraform-provider-google#4541

dancmeyers · 2019-09-25T08:24:23Z

@danawillow Can I ask which resources are broken? Our primary use case for wanting IAM conditions is to be able to set access permissions on GCS buckets that apply to entire prefixes rather than either entire buckets or only specific objects (e.g. everything under bucket-name/prefixA accessible by groupA, bucket-name/prefixB by groupB, and groupC can control everything (regular bucket level IAM). It'd be useful to know if that is going to take signifiant time/work to achieve even after this PR.

Apologies if this would be better on hashicorp/terraform-provider-google#2909, let me know if so and I'll move it there.

danawillow · 2019-09-25T15:51:54Z

@dancmeyers, the only resources I've played around with so far are service accounts (which work) and pubsub subscriptions (which don't seem to).

modular-magician · 2019-10-15T23:05:51Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, e943dc2.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

third_party/terraform/resources/resource_iam_binding.go.erb

third_party/terraform/resources/resource_iam_member.go.erb

third_party/terraform/utils/iam.go.erb

modular-magician · 2019-10-24T00:55:23Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 96e7ce3.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

third_party/terraform/resources/resource_iam_binding.go.erb

third_party/terraform/utils/iam.go.erb

modular-magician · 2019-10-24T23:29:34Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 1a028ac.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

modular-magician · 2019-10-25T00:02:20Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 894a9b9.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

third_party/terraform/resources/resource_iam_binding.go.erb

third_party/terraform/resources/resource_iam_policy.go.erb

slevenick · 2019-10-25T16:57:51Z

third_party/terraform/website/docs/r/google_service_account_iam.html.markdown

+
+* `description` - (Optional) An optional description of the expression. This is a longer text which describes the expression, e.g. when hovered over it in a UI.
+
+~> **Warning:** Terraform considers the `role` and condition contents (`title`+`description`+`expression`) as the


Do we want description as part of the identifier for the binding?

Yes and no. It feels silly to do so, but it also makes the code much easier to deal with, and it aligns with what the IAM team has told us (since there can be two distinct conditions with the exact same title and expression but different descriptions). Since a ForceNew on an IAM resource isn't super destructive, I'm pretty ok with leaving it this way and seeing if it causes any issues.

modular-magician · 2019-10-25T17:34:04Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 0c0a13b.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

modular-magician · 2019-10-25T20:29:00Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, d5e5d40.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

modular-magician · 2019-10-25T23:23:08Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, e95eb91.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.

googlebot added the cla: yes label Sep 25, 2019

modular-magician added downstream-generated terraform-provider-google-beta terraform-provider-google labels Sep 25, 2019

emilymye self-requested a review September 25, 2019 20:57

danawillow force-pushed the iam-conditions-3 branch from 97c97a6 to e943dc2 Compare October 15, 2019 22:56

danawillow requested review from rileykarson and removed request for emilymye October 15, 2019 23:12

rileykarson reviewed Oct 16, 2019

View reviewed changes

danawillow requested a review from slevenick October 16, 2019 21:20

slevenick reviewed Oct 24, 2019

View reviewed changes

third_party/terraform/resources/resource_iam_binding.go.erb Outdated Show resolved Hide resolved

slevenick reviewed Oct 24, 2019

View reviewed changes

third_party/terraform/utils/iam.go.erb Outdated Show resolved Hide resolved

danawillow requested review from rileykarson and slevenick October 24, 2019 23:53

rileykarson approved these changes Oct 25, 2019

View reviewed changes

third_party/terraform/resources/resource_iam_binding.go.erb Outdated Show resolved Hide resolved

third_party/terraform/resources/resource_iam_binding.go.erb Outdated Show resolved Hide resolved

slevenick reviewed Oct 25, 2019

View reviewed changes

third_party/terraform/resources/resource_iam_policy.go.erb Outdated Show resolved Hide resolved

slevenick reviewed Oct 25, 2019

View reviewed changes

slevenick approved these changes Oct 25, 2019

View reviewed changes

modular-magician added the automerged label Oct 28, 2019

iam conditions

140a94d

danawillow added 6 commits October 28, 2019 10:13

remove drift detection and update ability

29b48f5

fix issues with import tests

d567986

fix some comments

060fd73

renames + comments

b1c7b30

don't send the condition unless it's set

427388a

lint fixes

2ceff33

danawillow force-pushed the iam-conditions-3 branch from e95eb91 to 2ceff33 Compare October 28, 2019 17:13

Update tracked submodules -> HEAD on Mon Oct 28 17:29:01 UTC 2019

e9704de

Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.

modular-magician merged commit 8ba5798 into GoogleCloudPlatform:master Oct 28, 2019

danawillow deleted the iam-conditions-3 branch October 28, 2019 17:31

danawillow mentioned this pull request Nov 4, 2019

allow importing bindings with no members #2582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IAM Conditions support; enable it in service account IAM #2372

Add IAM Conditions support; enable it in service account IAM #2372

danawillow commented Sep 25, 2019 •

edited

Loading

modular-magician commented Sep 25, 2019

dancmeyers commented Sep 25, 2019 •

edited

Loading

danawillow commented Sep 25, 2019

modular-magician commented Oct 15, 2019

modular-magician commented Oct 24, 2019

modular-magician commented Oct 24, 2019

modular-magician commented Oct 25, 2019

slevenick Oct 25, 2019

danawillow Oct 25, 2019

modular-magician commented Oct 25, 2019

modular-magician commented Oct 25, 2019

modular-magician commented Oct 25, 2019


		* `description` - (Optional) An optional description of the expression. This is a longer text which describes the expression, e.g. when hovered over it in a UI.

		~> Warning: Terraform considers the `role` and condition contents (`title`+`description`+`expression`) as the

Add IAM Conditions support; enable it in service account IAM #2372

Add IAM Conditions support; enable it in service account IAM #2372

Conversation

danawillow commented Sep 25, 2019 • edited Loading

modular-magician commented Sep 25, 2019

Pull request statuses

New Pull Requests

dancmeyers commented Sep 25, 2019 • edited Loading

danawillow commented Sep 25, 2019

modular-magician commented Oct 15, 2019

Pull request statuses

New Pull Requests

modular-magician commented Oct 24, 2019

Pull request statuses

New Pull Requests

modular-magician commented Oct 24, 2019

Pull request statuses

New Pull Requests

modular-magician commented Oct 25, 2019

Pull request statuses

New Pull Requests

slevenick Oct 25, 2019

Choose a reason for hiding this comment

danawillow Oct 25, 2019

Choose a reason for hiding this comment

modular-magician commented Oct 25, 2019

Pull request statuses

New Pull Requests

modular-magician commented Oct 25, 2019

Pull request statuses

New Pull Requests

modular-magician commented Oct 25, 2019

Pull request statuses

New Pull Requests

danawillow commented Sep 25, 2019 •

edited

Loading

dancmeyers commented Sep 25, 2019 •

edited

Loading