Add support for Cloud Asset feeds #3750

apsureda · 2020-07-16T17:12:18Z

Added resources for the Cloud Asset Inventory feeds. The feeds requires the billing project to be passed on via the X-Goog-User-Project HTTP header. In order to accomplish this without modifying the current transport logic, I have added a pre_create custom code hook right before the sendRequestWithTimeout function. It would be preferable to provide a proper option to do this without abusing the UserProjectOverride field.

This API is weird in the sense that the feed payload needs to be sent under a feed attribute, but in the HTTP response the payload is directly in the body. I solved this by moving the feed under a feed attribute in the pre_commit and pre_update hooks.

Release Note Template for Downstream PRs (will be copied)

`google_cloud_asset_organization_feed`

`google_cloud_asset_folder_feed`

`google_cloud_asset_project_feed`

… be available by then.

modular-magician · 2020-07-16T17:13:24Z

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.

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

modular-magician · 2020-07-16T17:19:05Z

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

Diff report:

Terraform GA: Diff ( 154 files changed, 2829 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 184 files changed, 2859 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 4 files changed, 369 insertions(+))

modular-magician · 2020-07-16T17:41:38Z

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

Diff report:

Terraform GA: Diff ( 15 files changed, 2684 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 15 files changed, 2684 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 4 files changed, 369 insertions(+))

… as required.

modular-magician · 2020-07-16T18:03:35Z

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

Diff report:

Terraform GA: Diff ( 15 files changed, 2684 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 15 files changed, 2684 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 4 files changed, 369 insertions(+))

modular-magician · 2020-07-16T19:09:20Z

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

Diff report:

Terraform GA: Diff ( 15 files changed, 2696 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 15 files changed, 2696 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 4 files changed, 369 insertions(+))

megan07

Thanks for the contribution! It looks great! Just a few comments - I had to learn about the resources while I looked through it, so please let me know if you think I misunderstood any of it.

megan07 · 2020-07-16T17:32:36Z

products/cloudasset/api.yaml

@@ -0,0 +1,252 @@
+# Copyright 2019 Google Inc.


Would you mind changing the copyright to 2020, please? This is applicable to the other files as well. Thanks!

products/cloudasset/api.yaml

megan07 · 2020-07-16T17:56:09Z

products/cloudasset/api.yaml

+          the feed. For example: "compute.googleapis.com/Disk"
+          See https://cloud.google.com/asset-inventory/docs/supported-asset-types for a list of all
+          supported asset types.
+      - !ruby/object:Api::Type::String


This can be Type::Enum and we can list the values here. This is applicable for all the different types of feeds.

Ah! Good to know. I did not know how to indicate that. The contentType is indeed an Enum. I'll change that.

megan07 · 2020-07-16T18:14:15Z

products/cloudasset/api.yaml

+                  Destination on Cloud Pubsub topic.
+  - !ruby/object:Api::Resource
+    name: FolderFeed
+    base_url: "{{folder}}/feeds"


Do we want the prefix folders in the base_url and create_url? I assume it's not here because the name returned from the google_folder resource/data source already has the prefix, but as I'm looking over it, I wonder if maybe we want to add a computed attribute, folder_id on the folder, that would be just the folder id rather than the full name. It should be a quick add. What do you think?

Yes, you are right. I did not like it either. I'll try to change this, and come back if I have any question on how to do it.

I didn't like this either. I have added the folder_id computed attribute as suggested.

megan07 · 2020-07-16T18:14:55Z

products/cloudasset/api.yaml

+          'https://cloud.google.com/asset-inventory/docs'
+      api: 'https://cloud.google.com/asset-inventory/docs/reference/rest/'
+    properties:
+      - !ruby/object:Api::Type::String


I think we might want to put this under a parameters block rather than properties

I'm not sure I understand the difference between the two but, if it seems right, Why not?

My understanding is there's not really a technical difference between the two (the technical importance is url_param_only), but more of an organizational difference. Thanks!

megan07 · 2020-07-16T18:16:20Z

products/cloudasset/api.yaml

+        url_param_only: true
+        description: |
+          The organization this feed should be created in.
+      - !ruby/object:Api::Type::String


Is this necessary for organization feeds? I think this would be handled with supports_indirect_user_project_override

Yes. Just like in the case of the FolderFeed, I am using this flag only to get the "project" var declared before the sendRequestWithTimeout function is called. Not the way I'd like to do it, but currently the only way to set the billing project header.

megan07 · 2020-07-16T18:17:42Z

products/cloudasset/api.yaml

+          'https://cloud.google.com/asset-inventory/docs'
+      api: 'https://cloud.google.com/asset-inventory/docs/reference/rest/'
+    properties:
+      - !ruby/object:Api::Type::String


I think we want to put this under a parameters block as well.

As mentioned earlier, seems right.

megan07 · 2020-07-16T18:47:50Z

templates/terraform/pre_create/cloud_asset_feed.go.erb

@@ -0,0 +1,16 @@
+// The feed object must be under the "feed" attribute on the request.


Would we be able to achieve this with an encoder rather than requiring a pre-create and pre-update. It looks as though the functionality is a little different among create, update and delete. I'm not sure if this is something we could capture in an encoder or not, but wanted to suggest it if it's possible.

I didn't know about encoders! This is definitely better suited for an encoder. At least, the part where I rearrange the obj structure. The billing project part, however, still requires a custom code hook.

megan07 · 2020-07-17T15:18:23Z

products/cloudasset/api.yaml

+      api: 'https://cloud.google.com/asset-inventory/docs/reference/rest/'
+    properties:
+      - !ruby/object:Api::Type::String
+        name: billing_project


Would you mind explaining what billing_project is used for here? I think if you're looking to override the project, you should be able to just call it project. Is that the intent? It also would not need to be url_param_only.

See an explanation for this in this comment: #3750 (comment)

megan07 · 2020-07-17T16:24:09Z

products/cloudasset/terraform.yaml

+--- !ruby/object:Provider::Terraform::Config
+overrides: !ruby/object:Overrides::ResourceOverrides
+  ProjectFeed: !ruby/object:Overrides::Terraform::ResourceOverride
+    autogen_async: false


I don't think this is necessary, I think it is the default

Right! I'm removing these.

apsureda · 2020-07-18T21:18:13Z

I forgot to mention. This pull request addresses the following feature request: hashicorp/terraform-provider-google#6549

modular-magician · 2020-07-18T21:22:30Z

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

Diff report:

Terraform GA: Diff ( 15 files changed, 2777 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 15 files changed, 2777 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 4 files changed, 411 insertions(+))

megan07 · 2020-07-20T21:15:30Z

templates/terraform/encoders/cloud_asset_feed.go.erb

@@ -0,0 +1,8 @@
+// Remove the "folders/" prefix from the folder ID


This is one way to do it, and I'm not totally opposed. My suggestion was more along the line of editing the actual folder resource and adding a read-only attribute there for folder_id, which would essentially do this logic in that resource so that if we were ever to need this value again, as input into another resource, we'd have this value available to us. What do you think of that? Or would you prefer to keep the logic here?

I initially tried to modify the folder attribute in the custom encoder. But the automated test failed due to a diff being found (the folder parameter was different than expected). So I ended up creating a computed folder_id attribute.
Did I miss something?

Sorry! I was likely confusing - I meant adding folder_id to the google_folder resource schema and then doing a d.Set() around here in the actual resource.
In this way, instead of expecting the user to pass in the name folders/{{folder_id}} we'd just expect the {{folder_id}}. If we add it to the google_folder resource, it will make it easier on the user to get it from the Terraform resource. We should probably add it on the data source as well, but I think the two share some code, so that shouldn't be too bad.
Let me know if that makes sense to you!

The way I have implemented it now accepts both folders/12345 and 12345. So I think we are good.
Still, I think it would make sense to add the folder_id attribute to the google_folder resource. But I would not want this to be dependent on that.

megan07 · 2020-07-20T21:23:26Z

products/cloudasset/api.yaml

+        url_param_only: true
+        description: |
+          The folder this feed should be created in.
+      - !ruby/object:Api::Type::String


Ok, I'm starting to understand this a bit better I think. Will the billing_project always be the project in feed_ouput_config.pubsub_destination.topic? I'm curious if we can make this a smoother, reusable solution for in the future. Something similar to supports_indirect_user_project_override, but a way to point to a resource's url that has the project we want. If that doesn't seem plausible, or the work seems too much, I'm not against what you have, it seems to work fine!

apsureda · 2020-07-21T07:40:20Z

For some reason, I cannot reply directly to this comment, so I add my reply here.

You can actually have three different projects involved in a google_cloud_asset_project_feed:

The project for which we want to receive notifications whenever a resource is modified in this project.
The project where the PubSub topic will be located.
The billing_project, which is the project whose cloud asset service account will be used as identity when writing messages to the PubSub topic (service-[PROJECT_NUMBER]@gcp-sa-cloudasset.iam.gserviceaccount.com).

When using the gcloud command, you indicate 1 using the --project parameter and 3 using the --billing-project parameter. 2 is implicit in the PubSub topic name. If you do not add the --billing-project parameter, --project will be used. I have replicated this same behaviour for consistency and flexibility.

In the case of the google_cloud_asset_organization_feed and google_cloud_asset_folder_feed, the billing_project is required. Otherwise, no X-Goog-User-Project header will be sent, and the request will result in an error:

Fail to use projects/PROJECT/topics/TOPIC
      as feed output destination. Error: User not authorized to perform this action.

And actually, in the gcloud command, it is the --billing-project parameter that must be used, since you must use only one among the project|folder|organization parameters. Maybe I should rename the project attribute in the google_cloud_asset_organization_feed and google_cloud_asset_folder_feed resources and call them billing_project instead?

megan07 · 2020-07-21T15:27:00Z

@apsureda - sure, I like the idea of using billing_project across all three resources for consistency. thanks!

megan07

If you wouldn't mind updating project to billing_project for consistency across the 3 feeds, I'll get this merged in. Thanks!

…ency across resources and with gcloud command

modular-magician · 2020-07-21T16:55:28Z

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

Diff report:

Terraform GA: Diff ( 15 files changed, 2793 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 15 files changed, 2793 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 4 files changed, 411 insertions(+))

apsureda · 2020-07-21T16:59:45Z

I renamed the project fields. Automated tests passed, and my own manual tests did too. I think we are good for the merge.
Thanks a lot for your patience. I have learnt a lot. My next PR will require less back and forth :-)

megan07 · 2020-07-21T17:58:51Z

Fixes hashicorp/terraform-provider-google#6549

@apsureda of course! thanks so much for your contribution - and for helping me learn more about the cloud asset feeds!

apsureda added 7 commits July 9, 2020 22:46

Added pre_create custom code hook

b3f16a5

Adding the google_cloud_asset_project_feed resource type

b838d5a

Move the pre_create override further down. We need the provect var to…

8443f98

… be available by then.

Get the variables from the test environment

c23226d

Just update the project variable, not the whole data structure.

421721a

Add the google_cloud_asset_folder_feed resource type

6cfc6c2

Adding the google_cloud_asset_organization_feed resource type

5550fc9

googlebot added the cla: yes label Jul 16, 2020

modular-magician requested a review from megan07 July 16, 2020 17:13

Remove blank lines to avoid unecessary diffs

29ddccb

apsureda changed the title ~~Clooud asset feeds~~ Add support for Cloud Asset feeds Jul 16, 2020

Just noticed the attributes in the output destination were not marked…

f819831

… as required.

Fix linter error

d5824d2

megan07 suggested changes Jul 17, 2020

View reviewed changes

apsureda added 6 commits July 17, 2020 19:32

Update copyright

8cf4992

Add enum for contentType parameter

5cc5756

Moving folder and org id to params block

3d197fd

Removing unnecessary autogen_async flag

888fd3c

Moving the obj structure code to an encoder

614d192

Added folder_id computed attribute

f957ad6

apsureda requested a review from megan07 July 18, 2020 21:23

megan07 reviewed Jul 20, 2020

View reviewed changes

apsureda requested a review from megan07 July 21, 2020 07:40

megan07 approved these changes Jul 21, 2020

View reviewed changes

Rename project to billing_project in folder and org feeds for consist…

81eff90

…ency across resources and with gcloud command

apsureda requested a review from megan07 July 21, 2020 16:59

megan07 merged commit f9a00a9 into GoogleCloudPlatform:master Jul 21, 2020

This was referenced Jul 21, 2020

Add support for Cloud Asset feeds GoogleCloudPlatform/terraform-google-conversion#480

Merged

Add support for Cloud Asset feeds hashicorp/terraform-provider-google#6821

Merged

Add support for Cloud Asset feeds hashicorp/terraform-provider-google-beta#2284

Merged

This was referenced Jul 21, 2020

Cloud Asset Feeds hashicorp/terraform-provider-google#6549

Closed

add folder_id as output to folder datasource and resource #3763

Merged

This was referenced Jul 21, 2020

add folder_id as output to folder datasource and resource hashicorp/terraform-provider-google#6823

Merged

add folder_id as output to folder datasource and resource hashicorp/terraform-provider-google-beta#2287

Merged

		@@ -0,0 +1,16 @@
		// The feed object must be under the "feed" attribute on the request.

		@@ -0,0 +1,8 @@
		// Remove the "folders/" prefix from the folder ID

Add support for Cloud Asset feeds #3750

Add support for Cloud Asset feeds #3750

Conversation

apsureda commented Jul 16, 2020

modular-magician commented Jul 16, 2020

modular-magician commented Jul 16, 2020

Diff report:

modular-magician commented Jul 16, 2020

Diff report:

modular-magician commented Jul 16, 2020

Diff report:

modular-magician commented Jul 16, 2020

Diff report:

megan07 left a 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

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

apsureda commented Jul 18, 2020

modular-magician commented Jul 18, 2020

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

apsureda commented Jul 21, 2020

megan07 commented Jul 21, 2020 • edited Loading

megan07 left a comment

Choose a reason for hiding this comment

modular-magician commented Jul 21, 2020

Diff report:

apsureda commented Jul 21, 2020

megan07 commented Jul 21, 2020

megan07 commented Jul 21, 2020 •

edited

Loading