Org Security Policies (Hierarchical Firewalls) #3626

drebes · 2020-06-10T23:00:23Z

Release Note Template for Downstream PRs (will be copied)

`google_compute_compute_organization_security_policy` (beta-only)

`google_compute_compute_organization_security_policy_association` (beta-only)

`google_compute_compute_organization_security_policy_rule` (beta-only)

Fixes hashicorp/terraform-provider-google#6535

drebes · 2020-06-10T23:04:53Z

This requires update of the client libraries (go get -u google.golang.org/api/compute/v0.beta) to get the new org. level operations. I wanted to cleanly implement async op support but couldn't find a way to have the operation customized based on resource type, so hacked around using "post" custom_code and some updates to the resource template which end up affecting all resources that can be updated (but it's just a minor logging change).

It's still pending update support and more proper testing. If you have some ideas on how to handle those ops more cleanly let me know.

danawillow · 2020-06-11T23:43:08Z

Error from cloud build:

F, [2020-06-10T23:02:45.610853 #22] FATAL -- : Error compiling /mm-inspec-beta-head/templates/inspec/examples/google_compute_organization_security_policy/google_compute_organization_security_policy.erb
Error compiling file: /mm-inspec-beta-head/templates/inspec/doc_template.md.erb
bundler: failed to load command: compiler (compiler)
Errno::ENOENT: No such file or directory @ rb_sysopen - /mm-inspec-beta-head/templates/inspec/examples/google_compute_organization_security_policy/google_compute_organization_security_policy.erb
  /mm-inspec-beta-head/compile/core.rb:263:in `read'
  /mm-inspec-beta-head/compile/core.rb:263:in `get_helper_file'
  /mm-inspec-beta-head/compile/core.rb:126:in `compile'
  (erb):20:in `provider_binding'
  /root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/erb.rb:901:in `eval'
  /root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/erb.rb:901:in `result'
  /mm-inspec-beta-head/compile/core.rb:219:in `compile_string'
  /mm-inspec-beta-head/compile/core.rb:154:in `compile_file'
  /mm-inspec-beta-head/provider/file_template.rb:63:in `block in generate'
  /mm-inspec-beta-head/provider/file_template.rb:63:in `open'
  /mm-inspec-beta-head/provider/file_template.rb:63:in `generate'
  /mm-inspec-beta-head/provider/inspec.rb:187:in `generate_documentation'
  /mm-inspec-beta-head/provider/inspec.rb:74:in `generate_resource'
  /mm-inspec-beta-head/provider/core.rb:230:in `generate_object'
  /mm-inspec-beta-head/provider/core.rb:218:in `block in generate_objects'
  /mm-inspec-beta-head/provider/core.rb:201:in `each'
  /mm-inspec-beta-head/provider/core.rb:201:in `generate_objects'
  /mm-inspec-beta-head/provider/core.rb:80:in `generate'
  compiler:219:in `block in <top (required)>'
  compiler:134:in `each'
  compiler:134:in `<top (required)>'

Unless you'd like to look into adding inspec support, I'd recommend adding an exclude for this resource in the relevant inspec.yaml file.

danawillow · 2020-06-12T16:53:32Z

New error from cloud build:

#<Thread:0x000056342653b370@/mm-tf-conversion-ga-head/provider/core.rb:137 run> terminated with exception (report_on_exception is true):
/root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/fileutils.rb:1307:in `lstat': No such file or directory @ rb_file_s_lstat - third_party/terraform/utils/compute_operation.go (Errno::ENOENT)
	from /root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/fileutils.rb:1307:in `lstat'
	from /root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/fileutils.rb:1350:in `copy'
	from /root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/fileutils.rb:478:in `block in copy_entry'
	from /root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/fileutils.rb:1484:in `wrap_traverse'
	from /root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/fileutils.rb:475:in `copy_entry'
	from /mm-tf-conversion-ga-head/provider/core.rb:149:in `block (2 levels) in copy_file_list'
Errno::ENOENT: No such file or directory @ rb_file_s_lstat - third_party/terraform/utils/compute_operation.go
  /root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/fileutils.rb:1307:in `lstat'
  /root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/fileutils.rb:1307:in `lstat'
  /root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/fileutils.rb:1350:in `copy'
  /root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/fileutils.rb:478:in `block in copy_entry'
  /root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/fileutils.rb:1484:in `wrap_traverse'
  /root/.rbenv/versions/2.6.0/lib/ruby/2.6.0/fileutils.rb:475:in `copy_entry'
  /mm-tf-conversion-ga-head/provider/core.rb:149:in `block (2 levels) in copy_file_list'
bundler: failed to load command: compiler (compiler)

modular-magician · 2020-06-13T13:07:42Z

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

Diff report:

Terraform GA: Diff ( 100 files changed, 281 insertions(+), 51 deletions(-))
Terraform Beta: Diff ( 136 files changed, 2460 insertions(+), 73 deletions(-))

modular-magician · 2020-06-13T13:07:50Z

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

danawillow

Did a first pass, have a few comments. I think the way you handled operations is totally fine- I can't think of anything obviously cleaner.

products/compute/api.yaml

products/compute/terraform.yaml

danawillow · 2020-06-17T22:59:36Z

templates/terraform/examples/organization_security_policy_association_basic.tf.erb

+  parent       = "organizations/<%= ctx[:test_env_vars]['org_id'] %>"
+}
+
+resource "google_compute_organization_security_policy_rule" "<%= ctx[:primary_resource_id] %>" {


Since this is the test for the association resource, is the rule necessary to have?

Technically this is an (end-to-end) example, not a test. ;) But I don't mind removing it if it's clear for customers on the relationship of the 3 resources.

templates/terraform/post_create/org_security_policy.go.erb

modular-magician · 2020-06-24T23:25:31Z

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.

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

modular-magician · 2020-06-24T23:30:57Z

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

Diff report:

Terraform GA: Diff ( 100 files changed, 281 insertions(+), 51 deletions(-))
Terraform Beta: Diff ( 141 files changed, 2470 insertions(+), 75 deletions(-))

modular-magician · 2020-06-24T23:31:07Z

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

modular-magician · 2020-06-25T11:30:36Z

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.

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

modular-magician · 2020-06-25T11:35:57Z

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

Diff report:

Terraform GA: Diff ( 100 files changed, 281 insertions(+), 51 deletions(-))
Terraform Beta: Diff ( 141 files changed, 2470 insertions(+), 75 deletions(-))

modular-magician · 2020-06-25T11:36:05Z

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

products/compute/api.yaml

danawillow · 2020-06-25T21:50:50Z

products/compute/api.yaml

+            - !ruby/object:Api::Type::Array
+              name: 'srcIpRanges'
+              description: |
+                Source IP address range in CIDR format.


In validation.go, we have validateIpCidrRange. Does that make sense to apply to these attributes?

Seems like validateFunc can't be used on arrays:
resource google_compute_organization_security_policy_rule: src_ip_ranges: ValidateFunc is not yet supported on lists or sets.

danawillow · 2020-06-25T21:52:45Z

products/compute/api.yaml

+        description: |
+          A textual description for the organization security policy.
+      - !ruby/object:Api::Type::String
+        name: id


I think we normally just don't bother including server-defined ids unless they're actually necessary, like for referencing resources from others. Is this that sort of situation, or can this be removed?

policy_id is needed to build the self_link for subsequent reads. Any way I can build the URL without the ID?

danawillow · 2020-06-25T21:54:40Z

products/compute/api.yaml

+          Format: organizations/{organization_id} or folders/{folder_id}
+        required: true
+        input: true
+    properties:


I could see name and labels being useful fields here. Any reason they're missing?

name is the same as id, not user defined:

$ gcloud beta compute org-security-policies describe test --organization 928204348109 --- creationTimestamp: '2020-07-28T15:47:24.153-07:00' description: '' displayName: test fingerprint: Mj-VjR8Uz6g= id: '819155619164' kind: compute#securityPolicy labelFingerprint: 42WmSpB8rSM= name: '819155619164' parent: organizations/928204348109 ruleTupleCount: 2 rules: - action: goto_next ...

so not much value in including it IMO. labels do not seem to be supported for OrganizationSecurityPolicies and this also seems to be a left over from Cloud Armor policies. They are not documented in the gcloud docs and when I try to create one with labels via API, the labels are not returned on a read call.

danawillow · 2020-06-25T21:57:28Z

products/compute/api.yaml

+    name: 'OrganizationSecurityPolicy'
+    min_version: beta
+    base_url: 'locations/global/securityPolicies?parentId={{parent}}'
+    self_link: 'locations/global/securityPolicies/{{id}}'


https://cloud.google.com/compute/docs/reference/rest/beta/organizationSecurityPolicies/get implies these can be read by name. Is that not true?

Yes, but name is the same as id.

…n code is fixed

modular-magician · 2020-07-28T22:54:18Z

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

Diff report:

Terraform GA: Diff ( 107 files changed, 299 insertions(+), 56 deletions(-))
Terraform Beta: Diff ( 149 files changed, 2599 insertions(+), 81 deletions(-))
Inspec: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2020-07-28T22:54:25Z

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

modular-magician · 2020-07-28T23:56:15Z

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

Diff report:

Terraform GA: Diff ( 107 files changed, 299 insertions(+), 56 deletions(-))
Terraform Beta: Diff ( 149 files changed, 2599 insertions(+), 81 deletions(-))
Inspec: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2020-07-28T23:56:24Z

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

modular-magician · 2020-07-29T21:14:22Z

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

Diff report:

Terraform GA: Diff ( 108 files changed, 300 insertions(+), 56 deletions(-))
Terraform Beta: Diff ( 150 files changed, 2710 insertions(+), 81 deletions(-))
Inspec: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2020-07-29T21:14:30Z

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

drebes · 2020-07-29T21:21:00Z

I don't have any further pending changes. Let me know on what you think about keeping policy_id vs replacing it by the name which is actually the same numeric ID. I can't see why the build is failing. Also, if you'd like me to break the change that adds the log entry in every resource file into its own PR to make it easier to review what's specific about hierarchical firewalls only, let me know.

danawillow

Thanks for the info- almost there!

third_party/terraform/utils/compute_operation.go.erb

products/compute/api.yaml

modular-magician · 2020-08-03T21:23:30Z

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

Diff report:

Terraform GA: Diff ( 108 files changed, 300 insertions(+), 56 deletions(-))
Terraform Beta: Diff ( 150 files changed, 2710 insertions(+), 81 deletions(-))
Inspec: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2020-08-03T21:23:37Z

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

modular-magician · 2020-08-03T21:43:28Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccEndpointsService_basic|TestAccFolderIamAuditConfig_multiple|TestAccRedisInstance_redisInstanceFullExample|TestAccRedisInstance_redisInstancePrivateServiceExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=137493"

modular-magician · 2020-08-04T17:25:38Z

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

Diff report:

Terraform GA: Diff ( 108 files changed, 300 insertions(+), 56 deletions(-))
Terraform Beta: Diff ( 150 files changed, 2710 insertions(+), 81 deletions(-))
Inspec: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2020-08-04T17:25:47Z

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

modular-magician · 2020-08-04T17:30:24Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceComputeNetworkEndpointGroup|TestAccDataSourceDnsManagedZone_basic|TestAccDataSourceDNSKeys_noDnsSec|TestAccDataSourceGoogleActiveFolder_space|TestAccDataSourceDNSKeys_basic|TestAccDataSourceGoogleActiveFolder_default|TestAccDataSourceGoogleBillingAccount_byDisplayName|TestAccDataSourceGoogleCloudFunctionsFunction_basic|TestAccDataSourceComputeBackendBucket_basic|TestAccDataSourceComputeBackendService_basic|TestAccDataSourceGoogleForwardingRule|TestAccDataSourceComputeImage|TestAccDataSourceGoogleComputeInstanceGroup_basic|TestAccDataSourceGoogleComputeInstanceGroup_withNamedPort|TestAccDataSourceGoogleComputeInstanceGroup_fromIGM|TestAccDataSourceComputeInstanceSerialPort_basic|TestAccDataSourceComputeInstance_basic|TestAccDataSourceGoogleNetwork|TestAccDataSourceComputeRouter|TestAccDataSourceComputeSslCertificate|TestAccDataSourceGoogleSslPolicy|TestAccDataSourceGoogleSubnetwork|TestAccDataSourceGoogleVpnGateway|TestAccContainerClusterDatasource_zonal|TestAccContainerClusterDatasource_regional|TestAccDataSourceGoogleFolderOrganizationPolicy_basic|TestAccDataSourceGoogleFolder_byFullName|TestAccDataSourceGoogleFolder_byShortName|TestAccDataSourceGoogleFolder_lookupOrganization|TestAccDataSourceGoogleFolder_byFullNameNotFound|TestAccDataKmsSecretCiphertext_basic|TestAccDataSourceGoogleOrganization_byDomain|TestAccDataSourceGoogleProject_basic|TestAccRedisInstanceDatasource_basic|TestAccDatasourceGoogleServiceAccountKey_basic|TestAccDatasourceGoogleServiceAccount_basic|TestAccDataSourceGoogleSQLCaCerts_basic|TestAccDataSourceGoogleMonitoringNotificationChannel_byDisplayName|TestAccDataSourceGoogleMonitoringNotificationChannel_byTypeAndLabel|TestAccDataSourceGoogleMonitoringNotificationChannel_UserLabel|TestAccDataSourceGoogleMonitoringNotificationChannel_byDisplayNameAndType|TestAccDataSourceGoogleMonitoringNotificationChannel_ErrorNotFound|TestAccDataSourceGoogleMonitoringNotificationChannel_ErrorNotUnique|TestAccDatasourceSecretManagerSecretVersion_basic|TestAccDatasourceSecretManagerSecretVersion_latest|TestAccDataSourceSqlDatabaseInstance_basic|TestAccBinaryAuthorizationAttestorIamBindingGenerated|TestAccBinaryAuthorizationAttestorIamMemberGenerated|TestAccBinaryAuthorizationAttestorIamPolicyGenerated|TestAccCloudFunctionsCloudFunctionIamBindingGenerated|TestAccCloudFunctionsCloudFunctionIamMemberGenerated|TestAccCloudFunctionsCloudFunctionIamPolicyGenerated|TestAccCloudRunServiceIamBindingGenerated|TestAccCloudRunServiceIamMemberGenerated|TestAccCloudRunServiceIamPolicyGenerated|TestAccComputeInstanceIamBindingGenerated|TestAccComputeInstanceIamMemberGenerated|TestAccComputeInstanceIamPolicyGenerated|TestAccComputeSubnetworkIamBindingGenerated|TestAccComputeSubnetworkIamMemberGenerated|TestAccDataCatalogEntryGroupIamBindingGenerated|TestAccComputeSubnetworkIamPolicyGenerated|TestAccDataCatalogEntryGroupIamMemberGenerated|TestAccIapAppEngineServiceIamBindingGenerated|TestAccDataCatalogEntryGroupIamPolicyGenerated|TestAccIapAppEngineServiceIamMemberGenerated|TestAccIapAppEngineServiceIamPolicyGenerated|TestAccIapAppEngineVersionIamBindingGenerated|TestAccIapAppEngineVersionIamMemberGenerated|TestAccIapAppEngineVersionIamPolicyGenerated|TestAccIapTunnelInstanceIamMemberGenerated|TestAccIapTunnelInstanceIamPolicyGenerated|TestAccIapWebBackendServiceIamBindingGenerated|TestAccIapTunnelInstanceIamBindingGenerated|TestAccIapWebBackendServiceIamPolicyGenerated|TestAccIapWebBackendServiceIamMemberGenerated|TestAccIapWebIamBindingGenerated|TestAccIapWebIamMemberGenerated|TestAccIapWebTypeAppEngineIamBindingGenerated|TestAccIapWebIamPolicyGenerated|TestAccIapWebTypeComputeIamBindingGenerated|TestAccIapWebTypeComputeIamMemberGenerated|TestAccPubsubTopicIamBindingGenerated|TestAccIapWebTypeComputeIamPolicyGenerated|TestAccIapWebTypeAppEngineIamMemberGenerated|TestAccPubsubTopicIamMemberGenerated|TestAccRuntimeConfigConfigIamBindingGenerated|TestAccPubsubTopicIamPolicyGenerated|TestAccIapWebTypeAppEngineIamPolicyGenerated|TestAccSecretManagerSecretIamMemberGenerated|TestAccRuntimeConfigConfigIamMemberGenerated|TestAccSecretManagerSecretIamBindingGenerated|TestAccRuntimeConfigConfigIamPolicyGenerated|TestAccSecretManagerSecretIamPolicyGenerated|TestAccServiceManagementServiceIamPolicyGenerated|TestAccSourceRepoRepositoryIamBindingGenerated|TestAccSourceRepoRepositoryIamMemberGenerated|TestAccServiceManagementServiceIamBindingGenerated|TestAccServiceManagementServiceIamMemberGenerated|TestAccSourceRepoRepositoryIamPolicyGenerated You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=137624"

drebes added the enhancement label Jun 10, 2020

drebes requested a review from danawillow June 10, 2020 23:00

googlebot added the cla: yes label Jun 12, 2020

danawillow suggested changes Jun 17, 2020

View reviewed changes

drebes force-pushed the org-sec-policies branch from eef0e3d to 0355216 Compare June 24, 2020 23:24

modular-magician requested a review from nat-henderson June 24, 2020 23:25

danawillow removed the request for review from nat-henderson June 25, 2020 00:26

modular-magician requested a review from danawillow June 25, 2020 11:30

danawillow suggested changes Jun 26, 2020

View reviewed changes

drebes added 9 commits July 28, 2020 15:08

Org Security Policies

40a2bc8

Exclude inspec

4a1de1b

compite compute_operation.go

2f2cc05

OperationWaitTimeWithResponse

d979285

use ids as policy_ids

2b6978d

standardize on policy_id attribute

7467d28

use singular form for layer4_config block

e367106

add note about cleaning up skip_delete_during_test.go.erb after retur…

c1886b5

…n code is fixed

add reference to docs and include policy update test

5ff1022

drebes force-pushed the org-sec-policies branch from 0bf131d to 5ff1022 Compare July 28, 2020 22:46

make config required

ac7c663

update and import rule

6b7760f

danawillow reviewed Aug 3, 2020

View reviewed changes

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

products/compute/api.yaml Show resolved Hide resolved

fix lint error and improve rule validation

b916ff4

fix exactly_one_ofs

cfbebe3

danawillow approved these changes Aug 4, 2020

View reviewed changes

danawillow merged commit 3c53f83 into GoogleCloudPlatform:master Aug 4, 2020

Org Security Policies (Hierarchical Firewalls) #3626

Org Security Policies (Hierarchical Firewalls) #3626

Conversation

drebes commented Jun 10, 2020 • edited by danawillow Loading

drebes commented Jun 10, 2020

danawillow commented Jun 11, 2020

danawillow commented Jun 12, 2020

modular-magician commented Jun 13, 2020

Diff report:

modular-magician commented Jun 13, 2020

danawillow left a 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 24, 2020

modular-magician commented Jun 24, 2020

Diff report:

modular-magician commented Jun 24, 2020

modular-magician commented Jun 25, 2020

modular-magician commented Jun 25, 2020

Diff report:

modular-magician commented Jun 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drebes Jul 28, 2020 • edited Loading

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 Jul 28, 2020

Diff report:

modular-magician commented Jul 28, 2020

modular-magician commented Jul 28, 2020

Diff report:

modular-magician commented Jul 28, 2020

modular-magician commented Jul 29, 2020

Diff report:

modular-magician commented Jul 29, 2020

drebes commented Jul 29, 2020

danawillow left a comment

Choose a reason for hiding this comment

modular-magician commented Aug 3, 2020

Diff report:

modular-magician commented Aug 3, 2020

modular-magician commented Aug 3, 2020

modular-magician commented Aug 4, 2020

Diff report:

modular-magician commented Aug 4, 2020

modular-magician commented Aug 4, 2020

drebes commented Jun 10, 2020 •

edited by danawillow

Loading

drebes Jul 28, 2020 •

edited

Loading