Support organization iam conditions (#4749) (#9047)

* Split out organization iam tests * Added failing tests for organization iam binding and member with conditions * Made organization iam policy requests include requested policy version Signed-off-by: Modular Magician <[email protected]>
hashicorp · Apr 30, 2021 · 77eae86 · 77eae86
1 parent 20b4794
commit 77eae86
Show file tree

Hide file tree

Showing 6 changed files with 178 additions and 17 deletions.
diff --git a/.changelog/4749.txt b/.changelog/4749.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+resourcemanager: fixed broken handling of IAM conditions for `google_organization_iam_member`, `google_organization_iam_binding`, and `google_organization_iam_policy`
+```
diff --git a/google/iam.go b/google/iam.go
@@ -10,6 +10,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/davecgh/go-spew/spew"
 	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"google.golang.org/api/cloudresourcemanager/v1"
@@ -69,7 +70,7 @@ func iamPolicyReadWithRetry(updater ResourceIamUpdater) (*cloudresourcemanager.P
 	if err != nil {
 		return nil, err
 	}
-	log.Printf("[DEBUG] Retrieved policy for %s: %+v\n", updater.DescribeResource(), policy)
+	log.Print(spew.Sprintf("[DEBUG] Retrieved policy for %s: %#v\n", updater.DescribeResource(), policy))
 	return policy, nil
 }
 

diff --git a/google/iam_organization.go b/google/iam_organization.go
@@ -43,7 +43,14 @@ func (u *OrganizationIamUpdater) GetResourceIamPolicy() (*cloudresourcemanager.P
 		return nil, err
 	}
 
-	p, err := u.Config.NewResourceManagerClient(userAgent).Organizations.GetIamPolicy("organizations/"+u.resourceId, &cloudresourcemanager.GetIamPolicyRequest{}).Do()
+	p, err := u.Config.NewResourceManagerClient(userAgent).Organizations.GetIamPolicy(
+		"organizations/"+u.resourceId,
+		&cloudresourcemanager.GetIamPolicyRequest{
+			Options: &cloudresourcemanager.GetPolicyOptions{
+				RequestedPolicyVersion: iamPolicyVersion,
+			},
+		},
+	).Do()
 	if err != nil {
 		return nil, errwrap.Wrapf(fmt.Sprintf("Error retrieving IAM policy for %s: {{err}}", u.DescribeResource()), err)
 	}

diff --git a/google/resource_google_organization_iam_test.go b/google/resource_google_organization_iam_test.go
@@ -18,13 +18,32 @@ import (
 // behavior however induces flakiness in our acceptance tests, hence the need for running them
 // serially.
 // Policies are *not tested*, because testing them will ruin changes made to the test org.
-func TestAccOrganizationIam(t *testing.T) {
+func TestAccOrganizationIamMembersAndBindings(t *testing.T) {
 	if os.Getenv("TF_RUN_ORG_IAM") != "true" {
 		t.Skip("Environment variable TF_RUN_ORG_IAM is not set, skipping.")
 	}
-
 	t.Parallel()
 
+	testCases := map[string]func(t *testing.T){
+		"bindingBasic":     testAccOrganizationIamBinding_basic,
+		"bindingCondition": testAccOrganizationIamBinding_condition,
+		"memberBasic":      testAccOrganizationIamMember_basic,
+		"memberCondition":  testAccOrganizationIamMember_condition,
+	}
+
+	for name, tc := range testCases {
+		// shadow the tc variable into scope so that when
+		// the loop continues, if t.Run hasn't executed tc(t)
+		// yet, we don't have a race condition
+		// see https://github.com/golang/go/wiki/CommonMistakes#using-goroutines-on-loop-iterator-variables
+		tc := tc
+		t.Run(name, func(t *testing.T) {
+			tc(t)
+		})
+	}
+}
+
+func testAccOrganizationIamBinding_basic(t *testing.T) {
 	org := getTestOrgFromEnv(t)
 	account := fmt.Sprintf("tf-test-%d", randInt(t))
 	roleId := "tfIamTest" + randString(t, 10)
@@ -34,7 +53,7 @@ func TestAccOrganizationIam(t *testing.T) {
 		Steps: []resource.TestStep{
 			{
 				// Test Iam Binding creation
-				Config: testAccOrganizationIamBinding_basic(account, roleId, org),
+				Config: testAccOrganizationIamBinding_basicConfig(account, roleId, org),
 				Check: testAccCheckGoogleOrganizationIamBindingExists(t, "foo", "test-role", []string{
 					fmt.Sprintf("serviceAccount:%s@%s.iam.gserviceaccount.com", account, getTestProjectFromEnv()),
 				}),
@@ -59,9 +78,46 @@ func TestAccOrganizationIam(t *testing.T) {
 				ImportState:       true,
 				ImportStateVerify: true,
 			},
+		},
+	})
+}
+
+func testAccOrganizationIamBinding_condition(t *testing.T) {
+	org := getTestOrgFromEnv(t)
+	account := fmt.Sprintf("tf-test-%d", randInt(t))
+	roleId := "tfIamTest" + randString(t, 10)
+	conditionTitle := "expires_after_2019_12_31"
+	vcrTest(t, resource.TestCase{
+		PreCheck:  func() { testAccPreCheck(t) },
+		Providers: testAccProviders,
+		Steps: []resource.TestStep{
+			{
+				// Test Iam Binding creation
+				Config: testAccOrganizationIamBinding_conditionConfig(account, roleId, org, conditionTitle),
+				Check: testAccCheckGoogleOrganizationIamBindingExists(t, "foo", "test-role", []string{
+					fmt.Sprintf("serviceAccount:%s@%s.iam.gserviceaccount.com", account, getTestProjectFromEnv()),
+				}),
+			},
+			{
+				ResourceName:      "google_organization_iam_binding.foo",
+				ImportStateId:     fmt.Sprintf("%s organizations/%s/roles/%s %s", org, org, roleId, conditionTitle),
+				ImportState:       true,
+				ImportStateVerify: true,
+			},
+		},
+	})
+}
+
+func testAccOrganizationIamMember_basic(t *testing.T) {
+	org := getTestOrgFromEnv(t)
+	account := fmt.Sprintf("tf-test-%d", randInt(t))
+	vcrTest(t, resource.TestCase{
+		PreCheck:  func() { testAccPreCheck(t) },
+		Providers: testAccProviders,
+		Steps: []resource.TestStep{
 			{
 				// Test Iam Member creation (no update for member, no need to test)
-				Config: testAccOrganizationIamMember_basic(account, org),
+				Config: testAccOrganizationIamMember_basicConfig(account, org),
 				Check: testAccCheckGoogleOrganizationIamMemberExists(t, "foo", "roles/browser",
 					fmt.Sprintf("serviceAccount:%s@%s.iam.gserviceaccount.com", account, getTestProjectFromEnv()),
 				),
@@ -76,6 +132,37 @@ func TestAccOrganizationIam(t *testing.T) {
 	})
 }
 
+func testAccOrganizationIamMember_condition(t *testing.T) {
+	org := getTestOrgFromEnv(t)
+	account := fmt.Sprintf("tf-test-%d", randInt(t))
+	conditionTitle := "expires_after_2019_12_31"
+	vcrTest(t, resource.TestCase{
+		PreCheck:  func() { testAccPreCheck(t) },
+		Providers: testAccProviders,
+		Steps: []resource.TestStep{
+			{
+				// Test Iam Member creation (no update for member, no need to test)
+				Config: testAccOrganizationIamMember_conditionConfig(account, org, conditionTitle),
+				Check: testAccCheckGoogleOrganizationIamMemberExists(t, "foo", "roles/browser",
+					fmt.Sprintf("serviceAccount:%s@%s.iam.gserviceaccount.com", account, getTestProjectFromEnv()),
+				),
+			},
+			{
+				ResourceName: "google_organization_iam_member.foo",
+				ImportStateId: fmt.Sprintf(
+					"%s roles/browser serviceAccount:%s@%s.iam.gserviceaccount.com %s",
+					org,
+					account,
+					getTestProjectFromEnv(),
+					conditionTitle,
+				),
+				ImportState:       true,
+				ImportStateVerify: true,
+			},
+		},
+	})
+}
+
 func testAccCheckGoogleOrganizationIamBindingExists(t *testing.T, bindingResourceName, roleResourceName string, members []string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		bindingRs, ok := s.RootModule().Resources["google_organization_iam_binding."+bindingResourceName]
@@ -89,7 +176,14 @@ func testAccCheckGoogleOrganizationIamBindingExists(t *testing.T, bindingResourc
 		}
 
 		config := googleProviderConfig(t)
-		p, err := config.NewResourceManagerClient(config.userAgent).Organizations.GetIamPolicy("organizations/"+bindingRs.Primary.Attributes["org_id"], &cloudresourcemanager.GetIamPolicyRequest{}).Do()
+		p, err := config.NewResourceManagerClient(config.userAgent).Organizations.GetIamPolicy(
+			"organizations/"+bindingRs.Primary.Attributes["org_id"],
+			&cloudresourcemanager.GetIamPolicyRequest{
+				Options: &cloudresourcemanager.GetPolicyOptions{
+					RequestedPolicyVersion: iamPolicyVersion,
+				},
+			},
+		).Do()
 		if err != nil {
 			return err
 		}
@@ -119,7 +213,14 @@ func testAccCheckGoogleOrganizationIamMemberExists(t *testing.T, n, role, member
 		}
 
 		config := googleProviderConfig(t)
-		p, err := config.NewResourceManagerClient(config.userAgent).Organizations.GetIamPolicy("organizations/"+rs.Primary.Attributes["org_id"], &cloudresourcemanager.GetIamPolicyRequest{}).Do()
+		p, err := config.NewResourceManagerClient(config.userAgent).Organizations.GetIamPolicy(
+			"organizations/"+rs.Primary.Attributes["org_id"],
+			&cloudresourcemanager.GetIamPolicyRequest{
+				Options: &cloudresourcemanager.GetPolicyOptions{
+					RequestedPolicyVersion: iamPolicyVersion,
+				},
+			},
+		).Do()
 		if err != nil {
 			return err
 		}
@@ -142,7 +243,7 @@ func testAccCheckGoogleOrganizationIamMemberExists(t *testing.T, n, role, member
 
 // We are using a custom role since iam_binding is authoritative on the member list and
 // we want to avoid removing members from an existing role to prevent unwanted side effects.
-func testAccOrganizationIamBinding_basic(account, role, org string) string {
+func testAccOrganizationIamBinding_basicConfig(account, role, org string) string {
 	return fmt.Sprintf(`
 resource "google_service_account" "test-account" {
   account_id   = "%s"
@@ -194,7 +295,34 @@ resource "google_organization_iam_binding" "foo" {
 `, account, role, org, account, org)
 }
 
-func testAccOrganizationIamMember_basic(account, org string) string {
+func testAccOrganizationIamBinding_conditionConfig(account, role, org, conditionTitle string) string {
+	return fmt.Sprintf(`
+resource "google_service_account" "test-account" {
+  account_id   = "%s"
+  display_name = "Organization Iam Testing Account"
+}
+
+resource "google_organization_iam_custom_role" "test-role" {
+  role_id     = "%s"
+  org_id      = "%s"
+  title       = "Iam Testing Role"
+  permissions = ["genomics.datasets.get"]
+}
+
+resource "google_organization_iam_binding" "foo" {
+  org_id  = "%s"
+  role    = google_organization_iam_custom_role.test-role.id
+  members = ["serviceAccount:${google_service_account.test-account.email}"]
+  condition {
+    title       = "%s"
+    description = "Expiring at midnight of 2019-12-31"
+    expression  = "request.time < timestamp(\"2020-01-01T00:00:00Z\")"
+  }
+}
+`, account, role, org, org, conditionTitle)
+}
+
+func testAccOrganizationIamMember_basicConfig(account, org string) string {
 	return fmt.Sprintf(`
 resource "google_service_account" "test-account" {
   account_id   = "%s"
@@ -208,3 +336,23 @@ resource "google_organization_iam_member" "foo" {
 }
 `, account, org)
 }
+
+func testAccOrganizationIamMember_conditionConfig(account, org, conditionTitle string) string {
+	return fmt.Sprintf(`
+resource "google_service_account" "test-account" {
+  account_id   = "%s"
+  display_name = "Organization Iam Testing Account"
+}
+
+resource "google_organization_iam_member" "foo" {
+  org_id = "%s"
+  role   = "roles/browser"
+  member = "serviceAccount:${google_service_account.test-account.email}"
+  condition {
+    title       = "%s"
+    description = "Expiring at midnight of 2019-12-31"
+    expression  = "request.time < timestamp(\"2020-01-01T00:00:00Z\")"
+  }
+}
+`, account, org, conditionTitle)
+}
diff --git a/google/resource_iam_binding.go b/google/resource_iam_binding.go
@@ -7,6 +7,7 @@ import (
 	"regexp"
 	"strings"
 
+	"github.com/davecgh/go-spew/spew"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
 	"google.golang.org/api/cloudresourcemanager/v1"
@@ -129,8 +130,8 @@ func resourceIamBindingRead(newUpdaterFunc newResourceIamUpdaterFunc) schema.Rea
 		if err != nil {
 			return handleNotFoundError(err, d, fmt.Sprintf("Resource %q with IAM Binding (Role %q)", updater.DescribeResource(), eBinding.Role))
 		}
-		log.Printf("[DEBUG] Retrieved policy for %s: %+v", updater.DescribeResource(), p)
-		log.Printf("[DEBUG] Looking for binding with role %q and condition %+v", eBinding.Role, eCondition)
+		log.Print(spew.Sprintf("[DEBUG] Retrieved policy for %s: %#v", updater.DescribeResource(), p))
+		log.Printf("[DEBUG] Looking for binding with role %q and condition %#v", eBinding.Role, eCondition)
 
 		var binding *cloudresourcemanager.Binding
 		for _, b := range p.Bindings {
@@ -142,7 +143,7 @@ func resourceIamBindingRead(newUpdaterFunc newResourceIamUpdaterFunc) schema.Rea
 
 		if binding == nil {
 			log.Printf("[WARNING] Binding for role %q not found, assuming it has no members. If you expected existing members bound for this role, make sure your role is correctly formatted.", eBinding.Role)
-			log.Printf("[DEBUG] Binding for role %q and condition %+v not found in policy for %s, assuming it has no members.", eBinding.Role, eCondition, updater.DescribeResource())
+			log.Printf("[DEBUG] Binding for role %q and condition %#v not found in policy for %s, assuming it has no members.", eBinding.Role, eCondition, updater.DescribeResource())
 			if err := d.Set("role", eBinding.Role); err != nil {
 				return fmt.Errorf("Error setting role: %s", err)
 			}

diff --git a/google/resource_iam_member.go b/google/resource_iam_member.go
@@ -7,6 +7,7 @@ import (
 	"regexp"
 	"strings"
 
+	"github.com/davecgh/go-spew/spew"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
 	"google.golang.org/api/cloudresourcemanager/v1"
@@ -222,8 +223,8 @@ func resourceIamMemberRead(newUpdaterFunc newResourceIamUpdaterFunc) schema.Read
 		if err != nil {
 			return handleNotFoundError(err, d, fmt.Sprintf("Resource %q with IAM Member: Role %q Member %q", updater.DescribeResource(), eMember.Role, eMember.Members[0]))
 		}
-		log.Printf("[DEBUG]: Retrieved policy for %s: %+v\n", updater.DescribeResource(), p)
-		log.Printf("[DEBUG]: Looking for binding with role %q and condition %+v", eMember.Role, eCondition)
+		log.Print(spew.Sprintf("[DEBUG]: Retrieved policy for %s: %#v\n", updater.DescribeResource(), p))
+		log.Printf("[DEBUG]: Looking for binding with role %q and condition %#v", eMember.Role, eCondition)
 
 		var binding *cloudresourcemanager.Binding
 		for _, b := range p.Bindings {
@@ -234,7 +235,7 @@ func resourceIamMemberRead(newUpdaterFunc newResourceIamUpdaterFunc) schema.Read
 		}
 
 		if binding == nil {
-			log.Printf("[DEBUG]: Binding for role %q with condition %+v does not exist in policy of %s, removing member %q from state.", eMember.Role, eCondition, updater.DescribeResource(), eMember.Members[0])
+			log.Printf("[DEBUG]: Binding for role %q with condition %#v does not exist in policy of %s, removing member %q from state.", eMember.Role, eCondition, updater.DescribeResource(), eMember.Members[0])
 			d.SetId("")
 			return nil
 		}
@@ -248,7 +249,7 @@ func resourceIamMemberRead(newUpdaterFunc newResourceIamUpdaterFunc) schema.Read
 		}
 
 		if member == "" {
-			log.Printf("[DEBUG]: Member %q for binding for role %q with condition %+v does not exist in policy of %s, removing from state.", eMember.Members[0], eMember.Role, eCondition, updater.DescribeResource())
+			log.Printf("[DEBUG]: Member %q for binding for role %q with condition %#v does not exist in policy of %s, removing from state.", eMember.Members[0], eMember.Role, eCondition, updater.DescribeResource())
 			d.SetId("")
 			return nil
 		}