Add cloud identity group #3696

megan07 · 2020-06-22T20:26:01Z

Fixes hashicorp/terraform-provider-google#3479

I've commented out a few un-implemented things (per email thread), but will update/include them as I hear back from the rest of the team.

Release Note Template for Downstream PRs (will be copied)

`google_cloud_identity_group` (TPGB-only)

added the `https://www.googleapis.com/auth/cloud-identity` scope to the provider by default

modular-magician · 2020-06-22T20:33:16Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 13 files changed, 129 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 21 files changed, 971 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2020-06-22T20:33:22Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=122165"

modular-magician · 2020-06-22T20:44:25Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 13 files changed, 60 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 21 files changed, 971 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2020-06-22T20:44:36Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=122166"

modular-magician · 2020-06-22T20:57:57Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 12 files changed, 52 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 21 files changed, 971 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2020-06-22T20:58:03Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=122167"

products/cloudidentity/api.yaml

products/cloudidentity/terraform.yaml

danawillow · 2020-06-22T21:20:36Z

templates/terraform/post_create/set_computed_name.erb

@@ -1,7 +1,11 @@
 // `name` is autogenerated from the api so needs to be set post-create
 name, ok := res["name"]
 if !ok {
-  return fmt.Errorf("Create response didn't contain critical fields. Create may not have succeeded.")
+	name, ok = res["response"].(map[string]interface{})["name"]


If an api response doesn't have "name" or "response" for whatever reason, wouldn't this be a nil ptr?

danawillow · 2020-06-22T21:26:39Z

products/cloudidentity/api.yaml

+      - !ruby/object:Api::Type::NestedObject
+        name: 'groupKey'
+        required: true
+        input: true


Should id and namespace also be marked input: true?

danawillow · 2020-06-22T21:29:32Z

third_party/terraform/tests/resource_cloud_identity_group_test.go.erb

+	})
+}
+
+func testAccCloudIdentityGroup_basic(context map[string]interface{}) string {


This is the same as testAccCloudIdentityGroup_cloudIdentityGroupsBasicExample, right? want to just use that?

Ah, yes! I thought the random suffixes would be different between the two, but just tried it and it worked. Thanks!

danawillow · 2020-06-22T21:32:02Z

templates/terraform/env_var_context.go.erb

@@ -17,5 +17,7 @@
 		"<%= var_name -%>": getTestProjectFromEnv(),
 	<% elsif var_type == :FIRESTORE_PROJECT_NAME -%>
 		"<%= var_name -%>": getTestFirestoreProjectFromEnv(t),
+	<% elsif var_type == :CUST_ID  -%>


I think you also need to add something to provider/terraform/examples.rb to get the cust_id to appear in the docs example

danawillow · 2020-06-22T21:32:40Z

products/cloudidentity/api.yaml

+
+              If specified, the EntityKey represents an external-identity-mapped group.
+              The namespace must correspond to an identity source created in Admin Console
+              and must be in the form of `identitysources/{identity_source_id}.


Suggested change

and must be in the form of `identitysources/{identity_source_id}.

and must be in the form of `identitysources/{identity_source_id}`.

danawillow · 2020-06-22T21:43:19Z

third_party/terraform/utils/config.go.erb

@@ -190,6 +190,7 @@ var <%= product[:definitions].name -%>DefaultBasePath = "<%= product[:definition
 var defaultClientScopes = []string{
 	"https://www.googleapis.com/auth/compute",
 	"https://www.googleapis.com/auth/cloud-platform",
+	"https://www.googleapis.com/auth/cloud-identity",


It's probably worth adding a note to the changelog about this (we did last time: #2473)

modular-magician · 2020-06-23T13:51:51Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 12 files changed, 52 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 21 files changed, 971 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2020-06-23T13:51:58Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=122393"

modular-magician · 2020-06-23T13:52:09Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 12 files changed, 52 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 21 files changed, 971 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2020-06-23T13:52:16Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=122393"

googlebot · 2020-06-23T14:24:55Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Co-authored-by: Dana Hoffman <[email protected]>

googlebot · 2020-06-23T14:25:44Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

modular-magician · 2020-06-23T14:30:43Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 12 files changed, 92 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 21 files changed, 1006 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2020-06-23T15:15:41Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 12 files changed, 92 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 21 files changed, 1006 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2020-06-23T15:15:51Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=122399"

modular-magician · 2020-06-23T16:39:20Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 12 files changed, 92 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 25 files changed, 1978 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2020-06-23T16:39:26Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=122407"

modular-magician · 2020-06-23T16:48:09Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 12 files changed, 92 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 25 files changed, 1978 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2020-06-23T16:48:17Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=122411"

danawillow · 2020-06-23T18:03:09Z

products/cloudidentity/api.yaml

+        resource: 'Group'
+        imports: 'name'
+        description: |
+          The name of the Group to create this membership.


Suggested change

The name of the Group to create this membership.

The name of the Group to create this membership in.

danawillow · 2020-06-23T18:04:12Z

products/cloudidentity/api.yaml

+          The resource name of the Membership, of the form groups/{group_id}/memberships/{membership_id}.
+      - !ruby/object:Api::Type::NestedObject
+        name: 'memberKey'
+        input: true


Same thing here wrt input: true on id/namespace

danawillow · 2020-06-23T18:04:38Z

products/cloudidentity/api.yaml

+              and must be in the form of `identitysources/{identity_source_id}.
+      - !ruby/object:Api::Type::NestedObject
+        name: 'preferredMemberKey'
+        input: true


danawillow · 2020-06-23T18:05:18Z

products/cloudidentity/api.yaml

+        name: 'roles'
+        description: |
+          The MembershipRoles that apply to the Membership.
+          If unspecified, defaults to a single MembershipRole with name MEMBER.


Is this true? It's not marked as computed. Or is it just not returned from the API?

After double-checking, this appears to not be true? I tried not including it and got an error, so I'm just going to mark this as required.

danawillow · 2020-06-23T18:05:28Z

products/cloudidentity/api.yaml

+        name: 'type'
+        output: true
+        description: |
+          # The type of the membership.


Suggested change

# The type of the membership.

The type of the membership.

danawillow · 2020-06-23T18:07:22Z

templates/terraform/examples/cloud_identity_group_membership.tf.erb

+
+resource "google_cloud_identity_group_membership" "<%= ctx[:primary_resource_id] %>" {
+  provider = google-beta
+  group    = google_cloud_identity_group.group.name


Can this be .id instead of .name? https://github.com/terraform-providers/terraform-provider-google/wiki/Developer-Best-Practices#self-links-names-and-ids

danawillow · 2020-06-23T18:17:06Z

products/cloudidentity/api.yaml

+
+              If specified, the EntityKey represents an external-identity-mapped group.
+              The namespace must correspond to an identity source created in Admin Console
+              and must be in the form of `identitysources/{identity_source_id}.


Suggested change

and must be in the form of `identitysources/{identity_source_id}.

and must be in the form of `identitysources/{identity_source_id}`.

danawillow · 2020-06-23T18:18:03Z

products/cloudidentity/terraform.yaml

+      post_create: templates/terraform/post_create/set_computed_name.erb
+  GroupMembership: !ruby/object:Overrides::Terraform::ResourceOverride
+    import_format: ["{{name}}"]
+    examples:


Even though it's really similar, I think people would probably appreciate an example showing how to add a user in addition to the existing one for adding a child group.

I agree, I started with that, but the user has to exist, and I can't create that with terraform, I don't believe. Do you want me to put an example in that is skip_test: true and hard-code an example email?

Does the user have to be in the org in question? If not I think we have a bunch of tests that use various emails belonging to people (like Paddy)

Yea, it has to end with the org_domain.

I put the example in with skip_test, I tested it first with a user I created in our org, and it worked, so I'm confident it will work.

modular-magician · 2020-06-23T20:23:36Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 12 files changed, 92 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 25 files changed, 2016 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2020-06-23T20:23:44Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=122425"

modular-magician · 2020-06-23T21:25:52Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 12 files changed, 92 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 25 files changed, 2087 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2020-06-23T21:25:58Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=122432"

danawillow · 2020-06-23T22:30:16Z

products/cloudidentity/terraform.yaml

+        test_env_vars:
+          org_domain: :ORG_DOMAIN
+          cust_id: :CUST_ID
+          admin_user: :ADMIN_USER


do they have to be an admin?

i don't believe so. i did this to separate that it has to be a user in admin console rather than a user in a project. maybe IDENTITY_USER might be more descriptive? i was trying to think on it last night, that's the best i could come up with.

danawillow · 2020-06-24T00:05:06Z

templates/terraform/examples/cloud_identity_group_membership.tf.erb

+  parent = "customers/<%= ctx[:test_env_vars]['cust_id'] %>"
+
+  group_key {
+  	id = "<%= ctx[:vars]['id_group'] %>-child@<%= ctx[:test_env_vars]['org_domain'] %>"


watch out for tabs vs spaces

modular-magician · 2020-06-24T16:04:20Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 12 files changed, 92 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 25 files changed, 2087 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+))

modular-magician · 2020-06-24T16:04:30Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=122650"

googlebot added the cla: yes label Jun 22, 2020

megan07 requested a review from danawillow June 22, 2020 21:02

danawillow suggested changes Jun 22, 2020

View reviewed changes

googlebot added cla: no and removed cla: yes labels Jun 23, 2020

megan07 and others added 9 commits June 23, 2020 09:25

add cloud identity to mm

2b9307d

make labels key value pair

75ba4bc

add cloud identity group

63e5c32

fix white spacing, add description to update test

614e990

make tests beta-only

74537e0

make customer id var beta-only

d5c66bc

Update products/cloudidentity/api.yaml

e923146

Co-authored-by: Dana Hoffman <[email protected]>

Update products/cloudidentity/terraform.yaml

6c83bb8

Co-authored-by: Dana Hoffman <[email protected]>

update code review comments

fbe00bc

megan07 force-pushed the megan_cloudidentity branch from 8f26ad5 to fbe00bc Compare June 23, 2020 14:25

googlebot added cla: yes and removed cla: no labels Jun 23, 2020

megan07 added 2 commits June 23, 2020 09:45

move unless ga to top of file for test

e465ac3

move unless ga back

6692f25

add group membership back in

a88bc9f

remove last comma

a52c6e6

danawillow suggested changes Jun 23, 2020

View reviewed changes

update per review comments

8a05f26

add group membership test for user

17717b5

megan07 requested a review from danawillow June 23, 2020 21:28

danawillow approved these changes Jun 24, 2020

View reviewed changes

update admin -> identity, tabs -> spaces

a2d3618

megan07 merged commit f78a0a9 into GoogleCloudPlatform:master Jun 24, 2020

This was referenced Jun 24, 2020

Add cloud identity group GoogleCloudPlatform/terraform-google-conversion#472

Merged

Add cloud identity group hashicorp/terraform-provider-google#6681

Merged

Add cloud identity group hashicorp/terraform-provider-google-beta#2224

Merged

rileykarson mentioned this pull request Mar 9, 2021

Add instructions on Cloud Identity values #4577

Merged

5 tasks

This was referenced Mar 10, 2021

Add instructions on Cloud Identity values hashicorp/terraform-provider-google#8648

Merged

Add instructions on Cloud Identity values hashicorp/terraform-provider-google-beta#3044

Merged

salrashid123 mentioned this pull request Oct 18, 2021

Add support for ExirationDate value for groups.memberships.ExpiryDetail hashicorp/terraform-provider-google#10343

Closed

	and must be in the form of `identitysources/{identity_source_id}.
	and must be in the form of `identitysources/{identity_source_id}`.

	The name of the Group to create this membership.
	The name of the Group to create this membership in.

Add cloud identity group #3696

Add cloud identity group #3696

Conversation

megan07 commented Jun 22, 2020 • edited Loading

modular-magician commented Jun 22, 2020

Diff report:

modular-magician commented Jun 22, 2020

modular-magician commented Jun 22, 2020

Diff report:

modular-magician commented Jun 22, 2020

modular-magician commented Jun 22, 2020

Diff report:

modular-magician commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Jun 23, 2020

Diff report:

modular-magician commented Jun 23, 2020

modular-magician commented Jun 23, 2020

Diff report:

modular-magician commented Jun 23, 2020

googlebot commented Jun 23, 2020

googlebot commented Jun 23, 2020

modular-magician commented Jun 23, 2020

Diff report:

modular-magician commented Jun 23, 2020

Diff report:

modular-magician commented Jun 23, 2020

modular-magician commented Jun 23, 2020

Diff report:

modular-magician commented Jun 23, 2020

modular-magician commented Jun 23, 2020

Diff report:

modular-magician commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Jun 23, 2020

Diff report:

modular-magician commented Jun 23, 2020

modular-magician commented Jun 23, 2020

Diff report:

modular-magician commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Jun 24, 2020

Diff report:

modular-magician commented Jun 24, 2020

megan07 commented Jun 22, 2020 •

edited

Loading