Added org policy policy resource. #5199

trodge · 2021-09-14T18:49:42Z

Added org policy policy resource to tpgtools. This is a new, schema-incompatible version of google_organization_policy, google_folder_organization_policy, and google_project_organization_policy which can have any of the three parent resources.

Added cloudresourcemanager folder and project resources with serialization only override for use in testing. These resources are still not implemented in terraform through the DCL.

fixes hashicorp/terraform-provider-google#2605

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

`google_org_policy_policy`

modular-magician · 2021-09-14T18:49:58Z

Oops! It looks like you're using an unknown release-note type in your changelog entries:

REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

modular-magician · 2021-09-14T18:51:03Z

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@c2thorn, please review this PR or find an appropriate assignee.

modular-magician · 2021-09-14T19:00:53Z

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

Diff report:

Terraform GA: Diff ( 29 files changed, 1356 insertions(+), 9574 deletions(-))
Terraform Beta: Diff ( 33 files changed, 1621 insertions(+), 11308 deletions(-))
TF Conversion: Diff ( 1 file changed, 5 insertions(+), 3 deletions(-))

nat-henderson · 2021-09-14T19:15:22Z

tpgtools/handwritten/orgpolicy_utils.go

+
+func resourceOrgPolicyPolicyCustomImport(d *schema.ResourceData, meta interface{}) error {
+	config := meta.(*Config)
+	if err := parseImportId([]string{


I would add a comment which says "this differs from the normal import method because it permits slashes in the first element and requires the parent to start at the beginning of the string.

I can't think of any off the top of my head, but do we see this import pattern in other resources? Any merit to a generic solution?

There are other resources in the DCL that allow a forward slashes in the values for certain fields. Usually, as it is here, this is because the field is a parent field and there can be more than one type of parent for the resource.

nat-henderson · 2021-09-14T19:15:54Z

tpgtools/overrides/cloudresourcemanager/beta/folder.yaml

+- type: SERIALIZATION_ONLY
+- type: CUSTOM_TERRAFORM_PRODUCT_NAME
+  details:
+    product: ""


Maybe a comment that says "this produces the correct name google_folder"?

nat-henderson · 2021-09-14T19:17:12Z

tpgtools/resource.go

+// of the DCL resource with a different location type. All references in samples
+// to a resource with an alternate location will point to the main version.
+func (r Resource) IsAlternateLocation() bool {
+	// For now, we consider non-regional resources to be alternate.


I'd extend this comment to say "non-locational resources have the empty string for their location."

nat-henderson · 2021-09-14T19:18:53Z

tpgtools/serialization_helpers.go

+	return serializeProjectToHCL(m)
+}
+
+func serializeProjectToHCL(m map[string]interface{}) (string, error) {


I notice this doesn't have folder, is that a git history mishap or is that on purpose?

Folder does not need a custom serializer because it has the same schema in DCL and terraform.

nat-henderson · 2021-09-14T19:19:16Z

tpgtools/templates/resource.go.tmpl

@@ -235,9 +235,9 @@ func resource{{$.PathType}}Create(d *schema.ResourceData, meta interface{}) erro
 	}
 {{ end }}

-	id, err := {{ $.IdFunction }}(d, config, "{{$.ID}}")


Does this mean we can remove IdFunction and ID now that the DCL exports an ID method?

I believe so. Should I do that in this PR?

Hmm... leave a TODO, maybe? This does already include an awful lot of changes that are required-but-tangentially-related.

I believe this would be better tracked if there were a GH issue that this TODO could point to

nat-henderson · 2021-09-14T19:20:57Z

@c2thorn - Thomas and I pair programmed most of this, so I'll take a first pass on review but we'll ping you when we need a fresh pair of eyes. :)

modular-magician · 2021-09-14T19:38:02Z

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

Diff report:

Terraform GA: Diff ( 29 files changed, 1356 insertions(+), 9574 deletions(-))
Terraform Beta: Diff ( 34 files changed, 1622 insertions(+), 11309 deletions(-))
TF Conversion: Diff ( 1 file changed, 5 insertions(+), 3 deletions(-))

nat-henderson · 2021-09-14T23:33:52Z

go.sum

@@ -0,0 +1 @@
+3644975SUM_LINE


This file should be removed - some kind of issue copying makefile commands I assume?

nat-henderson · 2021-09-14T23:34:06Z

mmv1/third_party/terraform/go.sum

@@ -1577,3 +1579,6 @@ rsc.io/sampler v1.3.0 h1:7uVkIFmeBqHfdjD+gZwtXXI+RODJ2Wc4O7MPEh/QiW4=
 rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA=
 sigs.k8s.io/yaml v1.1.0 h1:4A07+ZFc2wgJwo8YNlQpr1rVlgUDlxXHhPJciaPY5gs=
 sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o=
+3644975SUM_LINE


This line should be removed.

I've amended the commit that added this line. Let me know if it's still incorrect.

nat-henderson · 2021-09-14T23:35:25Z

You'll need to rebase + update those go.mod-related files again.

modular-magician · 2021-09-15T00:34:28Z

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

Diff report:

Terraform GA: Diff ( 21 files changed, 1333 insertions(+), 24 deletions(-))
Terraform Beta: Diff ( 25 files changed, 1343 insertions(+), 31 deletions(-))
TF Conversion: Diff ( 1 file changed, 5 insertions(+), 3 deletions(-))

nat-henderson · 2021-09-15T00:47:21Z

Awesome, looks like this all compiles and passes lints. /gcbrun to get the full test suite running. It'll take til tomorrow, we'll look at it in the morning.

trodge · 2021-09-15T00:52:27Z

/gcbrun

nat-henderson · 2021-09-15T00:54:52Z

Ah, sorry, it needed me to do it. It's going to complain you're not on the allowlist. But it did trigger, we can check in at https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstreamVcr/205879? in the morning - although I suspect permissions there will be a hassle. Something else to get sorted before the e2e experiment.

modular-magician · 2021-09-15T01:02:39Z

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

Diff report:

Terraform GA: Diff ( 21 files changed, 1333 insertions(+), 24 deletions(-))
Terraform Beta: Diff ( 24 files changed, 1342 insertions(+), 30 deletions(-))
TF Conversion: Diff ( 1 file changed, 5 insertions(+), 3 deletions(-))

nat-henderson · 2021-09-15T18:16:58Z

There's an issue in a DCL-based resource which is likely related to DCL-side handling of the .ID() change - we are probably not correctly normalizing everything that goes in that field. Once we fix that, should be good for review.

nat-henderson · 2021-09-15T21:20:42Z

Okay, I pushed a change that should fix that test failure. We'll have to see after the diff generation.

modular-magician · 2021-09-15T21:31:55Z

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

Diff report:

Terraform GA: Diff ( 21 files changed, 1324 insertions(+), 15 deletions(-))
Terraform Beta: Diff ( 25 files changed, 1331 insertions(+), 19 deletions(-))
TF Conversion: Diff ( 1 file changed, 5 insertions(+), 3 deletions(-))

nat-henderson · 2021-09-15T22:33:50Z

/gcbrun

modular-magician · 2021-09-15T22:44:43Z

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

Diff report:

Terraform GA: Diff ( 21 files changed, 1324 insertions(+), 15 deletions(-))
Terraform Beta: Diff ( 25 files changed, 1331 insertions(+), 19 deletions(-))
TF Conversion: Diff ( 1 file changed, 5 insertions(+), 3 deletions(-))

modular-magician · 2021-09-16T00:21:15Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccOrgPolicyPolicy_EnforcePolicy|TestAccOrgPolicyPolicy_FolderPolicy|TestAccOrgPolicyPolicy_OrganizationPolicy|TestAccOrgPolicyPolicy_ProjectPolicy You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=206047

nat-henderson · 2021-09-16T00:23:40Z

Great. Now only the new tests are failing and we have gotten all of them to pass on our side already so we have a good reason to think they will pass when they run.

modular-magician · 2021-09-16T19:08:17Z

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

Diff report:

Terraform GA: Diff ( 31 files changed, 1377 insertions(+), 9575 deletions(-))
Terraform Beta: Diff ( 35 files changed, 1642 insertions(+), 11309 deletions(-))
TF Conversion: Diff ( 1 file changed, 5 insertions(+), 3 deletions(-))

modular-magician · 2021-09-16T22:44:27Z

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

Diff report:

Terraform GA: Diff ( 23 files changed, 1356 insertions(+), 15 deletions(-))
Terraform Beta: Diff ( 28 files changed, 1364 insertions(+), 21 deletions(-))
TF Conversion: Diff ( 1 file changed, 5 insertions(+), 3 deletions(-))

nat-henderson · 2021-09-16T23:01:36Z

/gcbrun

nat-henderson · 2021-09-16T23:36:39Z

/gcbrun

…ooleans.

modular-magician · 2021-09-20T16:06:14Z

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

Diff report:

Terraform GA: Diff ( 23 files changed, 1354 insertions(+), 13 deletions(-))
Terraform Beta: Diff ( 27 files changed, 1361 insertions(+), 17 deletions(-))
TF Conversion: Diff ( 1 file changed, 5 insertions(+), 3 deletions(-))

nat-henderson · 2021-09-20T16:35:53Z

/gcbrun

modular-magician · 2021-09-20T16:46:58Z

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

Diff report:

Terraform GA: Diff ( 23 files changed, 1354 insertions(+), 13 deletions(-))
Terraform Beta: Diff ( 26 files changed, 1360 insertions(+), 16 deletions(-))
TF Conversion: Diff ( 1 file changed, 5 insertions(+), 3 deletions(-))

modular-magician · 2021-09-20T18:23:16Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccComputeManagedSslCertificate_managedSslCertificateBasicExample|TestAccComputeServiceAttachment_serviceAttachmentBasicExample|TestAccOrgPolicyPolicy_EnforcePolicy|TestAccOrgPolicyPolicy_FolderPolicy|TestAccOrgPolicyPolicy_OrganizationPolicy|TestAccOrgPolicyPolicy_ProjectPolicy You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=206583

tpgtools/overrides/cloudresourcemanager/project.yaml

modular-magician · 2021-09-20T18:41:31Z

Tests failed during RECORDING mode: TestAccOrgPolicyPolicy_OrganizationPolicy|TestAccOrgPolicyPolicy_FolderPolicy|TestAccOrgPolicyPolicy_ProjectPolicy|TestAccOrgPolicyPolicy_EnforcePolicy|TestAccComputeServiceAttachment_serviceAttachmentBasicExample Please fix these to complete your PR

c2thorn · 2021-09-20T18:42:12Z

tpgtools/templates/resource.go.tmpl

 	if err != nil {
-		return fmt.Errorf("Error constructing id: %s", err)


Why remove this extra formatting? The message still seems appropriate to me for the UseDCLID case, but if we need to modify it I think that's better than fully removing. Just makes it easier to read in the logs.

I've put the extra formatting back, lowercased in case the error isn't at the start of a sentence.

c2thorn · 2021-09-20T18:51:03Z

tpgtools/overrides/orgpolicy/beta/policy.yaml

+  details:
+    function: resourceOrgPolicyPolicyCustomImport
+- type: USE_DCL_ID
+- type: ENUM_BOOL


Non-review question: how would contributors be able to to recognize that a boolean should have this override applied to it?

If the field is a boolean in the DCL and it's necessary to be able to distinguish between true, false, and nil, then it needs to be an enum/string in terraform which can be "TRUE", "FALSE", or "".
In this case the distinction is important because these fields conflict with each other and setting enforce to false is different than leaving all of them unset.

If it needs to have an unset case which is distinct from false. This would be a good place to insist we write those override docs!

Yes if we could add a one-pager doc per override added, that would help. I don't quite remember if we had a specific place where we've kept them all, @ndmckinley do you remember? Maybe we can chat in an internal chat about it.

nat-henderson · 2021-09-20T18:54:47Z

FYI with respect to tests the issue was "api not enabled", enabled and rerunning.
https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstream/206594

c2thorn · 2021-09-20T18:55:33Z

tpgtools/overrides/orgpolicy/beta/policy.yaml

@@ -0,0 +1,10 @@
+- type: CUSTOM_IMPORT_FUNCTION


The generated sweeper does not seem to work correctly likely tied to the need for the custom import. We should probably add a NO_SWEEPER override for now.

Ah, yes, thank you!

nat-henderson · 2021-09-20T19:03:32Z

FYI those tests all passed - this is a fully green run.

…construction errors.

modular-magician · 2021-09-20T19:27:49Z

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

Diff report:

Terraform GA: Diff ( 23 files changed, 1354 insertions(+), 13 deletions(-))
Terraform Beta: Diff ( 25 files changed, 1288 insertions(+), 16 deletions(-))
TF Conversion: Diff ( 1 file changed, 5 insertions(+), 3 deletions(-))

suckowbiz · 2021-09-21T05:43:00Z

Hi @trodge ,

this mr closes #2605. Could you please point me to the new functionality that now enables to manage a monitoring workspace in an automated way?

trodge · 2021-09-21T16:07:37Z

Hi @trodge ,

this mr closes #2605. Could you please point me to the new functionality that now enables to manage a monitoring workspace in an automated way?

I seem to have linked this issue in the wrong PR.
#5218 will implement monitoring metrics scope and monitored project resources when it merges. These resources allow users to monitor metrics from multiple projects from the scope of a single project.

rileykarson · 2021-09-21T16:47:10Z

@trodge @ndmckinley this is hashicorp/terraform-provider-google#9341, right?

nat-henderson · 2021-09-21T18:10:10Z

Thanks, yes - I got the internal bugs and missed the external one. Appreciate the pointer.

* Added org policy policy resource. * Added additional comments. * Allow resources to use the normal terraform ID process by default - DCL by override. * Added a way to expand and flatten between terraform strings and dcl booleans. * Updated GA version of policy.yaml. * Ran make upgrade-dcl (and added missing tab to tf go.mod). * Added NO_SWEEPER override for orgpolicy policy and formatting for id construction errors. Co-authored-by: Nathan Mckinley <nmckinley@google.com>

google-cla bot added the cla: yes label Sep 14, 2021

modular-magician requested a review from c2thorn September 14, 2021 18:51

nat-henderson reviewed Sep 14, 2021

View reviewed changes

trodge force-pushed the orgpolicy-policy-dcl branch from 70ed626 to d474c59 Compare September 14, 2021 19:26

nat-henderson reviewed Sep 14, 2021

View reviewed changes

trodge force-pushed the orgpolicy-policy-dcl branch 2 times, most recently from 5c995c5 to 01b7437 Compare September 15, 2021 00:23

trodge force-pushed the orgpolicy-policy-dcl branch from 01b7437 to 5e4d5e5 Compare September 15, 2021 00:51

trodge added 2 commits September 20, 2021 08:34

Added a way to expand and flatten between terraform strings and dcl b…

18b6d6b

…ooleans.

Updated GA version of policy.yaml.

fccc730

trodge force-pushed the orgpolicy-policy-dcl branch 2 times, most recently from 8f63d84 to af6f45a Compare September 20, 2021 15:53

Ran make upgrade-dcl (and added missing tab to tf go.mod).

9edff17

trodge force-pushed the orgpolicy-policy-dcl branch from af6f45a to 9edff17 Compare September 20, 2021 15:55

c2thorn reviewed Sep 20, 2021

View reviewed changes

tpgtools/overrides/cloudresourcemanager/project.yaml Show resolved Hide resolved

c2thorn reviewed Sep 20, 2021

View reviewed changes

c2thorn requested changes Sep 20, 2021

View reviewed changes

Added NO_SWEEPER override for orgpolicy policy and formatting for id …

27e6838

…construction errors.

c2thorn approved these changes Sep 20, 2021

View reviewed changes

nat-henderson merged commit ef776cd into GoogleCloudPlatform:master Sep 20, 2021

trodge deleted the orgpolicy-policy-dcl branch September 21, 2021 16:23

		if err != nil {
		return fmt.Errorf("Error constructing id: %s", err)

		@@ -0,0 +1 @@
		3644975SUM_LINE

Added org policy policy resource. #5199

Added org policy policy resource. #5199

Conversation

trodge commented Sep 14, 2021 • edited Loading

modular-magician commented Sep 14, 2021

modular-magician commented Sep 14, 2021

modular-magician commented Sep 14, 2021

Diff report:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nat-henderson commented Sep 14, 2021

modular-magician commented Sep 14, 2021

Diff report:

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

nat-henderson commented Sep 14, 2021

modular-magician commented Sep 15, 2021

Diff report:

nat-henderson commented Sep 15, 2021

trodge commented Sep 15, 2021

nat-henderson commented Sep 15, 2021

modular-magician commented Sep 15, 2021

Diff report:

nat-henderson commented Sep 15, 2021

nat-henderson commented Sep 15, 2021

modular-magician commented Sep 15, 2021

Diff report:

nat-henderson commented Sep 15, 2021

modular-magician commented Sep 15, 2021

Diff report:

modular-magician commented Sep 16, 2021

nat-henderson commented Sep 16, 2021

modular-magician commented Sep 16, 2021

Diff report:

modular-magician commented Sep 16, 2021

Diff report:

nat-henderson commented Sep 16, 2021

nat-henderson commented Sep 16, 2021

modular-magician commented Sep 20, 2021

Diff report:

nat-henderson commented Sep 20, 2021

modular-magician commented Sep 20, 2021

Diff report:

modular-magician commented Sep 20, 2021

modular-magician commented Sep 20, 2021

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

nat-henderson commented Sep 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nat-henderson commented Sep 20, 2021

modular-magician commented Sep 20, 2021

Diff report:

suckowbiz commented Sep 21, 2021

trodge commented Sep 21, 2021

rileykarson commented Sep 21, 2021

nat-henderson commented Sep 21, 2021

trodge commented Sep 14, 2021 •

edited

Loading