[datadog_integration_gcp] Migrate to FW Provider, Add ResourceCollectionEnabled and IsSecurityCommandCenterEnabled fields #2230

smuhit · 2024-01-05T19:55:01Z

Added a couple of new fields to the (legacy) gcp integration resource. (NOTE: Despite being legacy, a couple of fields were added to the legacy APIs and hence needed to be added here.)

Tested locally and verified the following:

A state from pre-mirgration works post migration
Changes to new and old fields are picked up
- Host Filters were strings pre-migration. For backwards compatibility, kept them as strings
Creation and deletion also work post-migration.

NOTE: Waiting on a new release of the datadog go client library to be released.

docs/resources/integration_gcp.md

brett0000FF · 2024-01-08T21:10:48Z

docs/resources/integration_gcp.md

@@ -8,7 +8,7 @@ description: |-

 # datadog_integration_gcp (Resource)

-This resource is deprecated — use the `datadog_integration_gcp_sts resource` instead. Provides a Datadog - Google Cloud Platform integration resource. This can be used to create and manage Datadog - Google Cloud Platform integration.
+This resource is deprecated — use the `datadog_integration_gcp_sts` resource instead. Provides a Datadog - Google Cloud Platform integration resource. This can be used to create and manage Datadog - Google Cloud Platform integration.


Suggested change

This resource is deprecated — use the `datadog_integration_gcp_sts` resource instead. Provides a Datadog - Google Cloud Platform integration resource. This can be used to create and manage Datadog - Google Cloud Platform integration.

This resource is deprecated—use the `datadog_integration_gcp_sts` resource instead. Provides a Datadog - Google Cloud Platform integration resource. This can be used to create and manage Datadog - Google Cloud Platform integration.

@brett0000FF per line 6 should we just update both to stay consistent?

Yeah, we generally advise not to put spaces around hyphens, so that would be a good edit. I wasn't sure if the integration's name already contained the spacing, which is why I didn't edit it before.

ash-ddog

Couple things to change and/or discuss, lmk if you have questions!

ash-ddog · 2024-01-17T14:18:57Z

datadog/fwprovider/resource_datadog_integration_gcp.go

+var (
+	_ resource.ResourceWithConfigure   = &integrationGcpResource{}
+	_ resource.ResourceWithImportState = &integrationGcpResource{}
+)


Two nits:

I'd move this right above the definition of integrationGcpResource, since these lines are just trying to prove that it implements certain interfaces.

This is typically written as

var ( _ resource.ResourceWithConfigure = (*integrationGcpResource)(nil) _ resource.ResourceWithImportState = (*integrationGcpResource)(nil) )

i.e. no point in actually building structs since they are assigned to _ and so are not accessible.

ash-ddog · 2024-01-17T14:20:54Z

datadog/fwprovider/resource_datadog_integration_gcp.go

+	_ resource.ResourceWithImportState = &integrationGcpResource{}
+)
+
+var integrationGcpMutex = sync.Mutex{}


Nit: I'd move this to the top and write this as

var ( integrationGcpMutex sync.Mutex )

i.e. the zero-value mutex is valid for use

ash-ddog · 2024-01-17T14:23:02Z

datadog/fwprovider/resource_datadog_integration_gcp.go

+
+type integrationGcpResource struct {
+	Api  *datadogV1.GCPIntegrationApi
+	Auth context.Context


Any reason these are exported fields? Feels weird to have exported fields on an unexported type. That's usually only useful for struct tags (i.e. `json:"blah"`)

Switched to unexported

ash-ddog · 2024-01-17T14:30:57Z

datadog/fwprovider/resource_datadog_integration_gcp.go

+	Automute                       types.Bool   `tfsdk:"automute"`
+	HostFilters                    types.String `tfsdk:"host_filters"`
+	ResourceCollectionEnabled      types.Bool   `tfsdk:"resource_collection_enabled"`
+	CspmResourceCollectionEnabled  types.Bool   `tfsdk:"cspm_resource_collection_enabled"`


Isn't this typically called is_cspm_enabled?

Even if we have to leave the `tfsdk:"cspm_resource_collection_enabled"` , we could rename the field?

We could, but keeping it named as is for backwards compatibility (and the fact that this is a public repo) with https://registry.terraform.io/providers/DataDog/datadog/latest/docs/resources/integration_gcp#optional

ash-ddog · 2024-01-17T14:33:34Z

datadog/fwprovider/resource_datadog_integration_gcp.go

+	var state integrationGcpModel
+
+	integrationGcpMutex.Lock()
+	defer integrationGcpMutex.Unlock()


Is this meant to be a global lock? Any reason to not move it to being defined on integrationGcpResource so it's scoped to the instance? Even if integrationGcpResource is a singleton, an instance lock will in that case act like a global lock.

Terraform will do some automated parallelism behind the scenes. This is to ensure that the API calls aren't clobbering each other.

ash-ddog · 2024-01-17T14:44:50Z

datadog/fwprovider/resource_datadog_integration_gcp.go

+}
+
+func (r *integrationGcpResource) getGCPIntegration(state integrationGcpModel) (*datadogV1.GCPAccount, diag.Diagnostics) {
+	diags := diag.Diagnostics{}


Nit: prefer var diags diag.Diagnostics

ash-ddog · 2024-01-17T14:52:41Z

datadog/fwprovider/resource_datadog_integration_gcp.go

+	state.ID = projectId
+}
+
+func (r *integrationGcpResource) getGCPIntegration(state integrationGcpModel) (*datadogV1.GCPAccount, diag.Diagnostics) {


Kinda nit:

IMO all of this code would be cleaner if we left out terraform-specific stuff until the last moment. i.e. make the return signature of this method (datadogV1.GCPAccount, error), and add the error to the diag. That way we don't have to rely on pointers to tell if something worked or not, and we don't have to switch back and forth btw pointers and values.

ash-ddog · 2024-01-17T14:54:49Z

datadog/fwprovider/resource_datadog_integration_gcp.go

+				diags.AddError("response contains unparsedObject", err.Error())
+				return nil, diags
+			}
+			account = &integration


Preference for making this line return &integration, diags and making the last line return nil, diags, and removing the local var and break all together.

ash-ddog · 2024-01-17T15:08:12Z

datadog/fwprovider/resource_datadog_integration_gcp.go

+	return account, diags
+}
+
+func (r *integrationGcpResource) buildIntegrationGcpRequestBodyBase(state integrationGcpModel) (*datadogV1.GCPAccount, diag.Diagnostics) {


Why is diags.Diagnostics part of this return signature? It's always empty?

ash-ddog · 2024-01-17T15:10:58Z

datadog/fwprovider/resource_datadog_integration_gcp.go

+	body.SetHostFilters(state.HostFilters.ValueString())
+	if !state.ResourceCollectionEnabled.IsUnknown() {
+		body.SetResourceCollectionEnabled(state.ResourceCollectionEnabled.ValueBool())
+	}


Any reason this is special case? (i.e. I don't see IsUnknown() used anywhere else but ResourceCollectionEnabled)

Mainly for backwards compatibility. The API will return an error if the CSPM flag is on but the Resource Collection Flag is explicitly set to False. In this case, we don't want to set any value for the Resource Collection flag unless a value was explicitly set by the user.

dtru-ddog · 2024-02-14T15:10:41Z

Moving to draft until ready for re-review

…SecurityCommandCenterEnabled fields Update datadog client library Remove unused variable Fixes for migration to fwprovider More fixes Update codeowners for changes to gcp

… cassettes interacting with the GCP APIs

fix docs

ash-ddog

Couple of very small nits, thanks for addressing feedback!

I'd be curious about testing here. Seems we don't have any automated tests (bad). Is there usually a manual testing phase when we make large changes to our terraform stuff?

ash-ddog · 2024-02-27T14:52:03Z

datadog/fwprovider/resource_datadog_integration_gcp.go

+}
+
+func (r *integrationGcpResource) Configure(_ context.Context, request resource.ConfigureRequest, response *resource.ConfigureResponse) {
+	providerData, _ := request.ProviderData.(*FrameworkProvider)


Nit: I believe the , _ can be removed? It's a bool that makes this a safe cast, but we ignore the bool anyways. I think we'll get an NPE on the next two lines anyway if the cast fails.

ash-ddog · 2024-02-27T14:56:51Z

docs/resources/integration_gcp.md

 ---

 # datadog_integration_gcp (Resource)

-This resource is deprecated — use the `datadog_integration_gcp_sts resource` instead. Provides a Datadog - Google Cloud Platform integration resource. This can be used to create and manage Datadog - Google Cloud Platform integration.
+This resource is deprecated—use the `datadog_integration_gcp_sts` resource instead. Provides a Datadog - Google Cloud Platform integration resource. This can be used to create and manage Datadog - Google Cloud Platform integration.


Nit: why remove the spacing around the dash in these two?

Basically guidance from documentation: #2230 (comment)

ash-ddog · 2024-02-27T14:57:02Z

docs/resources/integration_gcp.md

@@ -3,12 +3,12 @@
 page_title: "datadog_integration_gcp Resource - terraform-provider-datadog"
 subcategory: ""
 description: |-
-  This resource is deprecated — use the datadog_integration_gcp_sts resource instead. Provides a Datadog - Google Cloud Platform integration resource. This can be used to create and manage Datadog - Google Cloud Platform integration.
+  This resource is deprecated—use the datadog_integration_gcp_sts resource instead. Provides a Datadog - Google Cloud Platform integration resource. This can be used to create and manage Datadog - Google Cloud Platform integration.


Nit: add backticks to datadog_integration_gcp_sts as you did below?

smuhit requested review from a team and nina9753 and removed request for a team January 5, 2024 19:55

smuhit requested review from a team as code owners January 5, 2024 19:55

smuhit added the gcp-integrations label Jan 5, 2024

smuhit marked this pull request as draft January 5, 2024 20:18

smuhit changed the title ~~[GCP V1 (Legacy)] Migrate to FW Provider, Add ResourceCollectionEnabled and IsSecurityCommandCenterEnabled fields~~ [datadog_integration_gcp] Migrate to FW Provider, Add ResourceCollectionEnabled and IsSecurityCommandCenterEnabled fields Jan 5, 2024

smuhit added the changelog/improvement label Jan 5, 2024

smuhit force-pushed the sabbir.muhit/gcp-1258/add-resource-collection-flag-to-gcp-resources branch from f51f638 to f758230 Compare January 8, 2024 19:27

smuhit marked this pull request as ready for review January 8, 2024 20:03

brett0000FF previously approved these changes Jan 8, 2024

View reviewed changes

smuhit requested a review from ash-ddog January 9, 2024 15:11

nina9753 previously approved these changes Jan 9, 2024

View reviewed changes

dtru-ddog self-requested a review January 10, 2024 15:12

dtru-ddog dismissed stale reviews from nina9753 and brett0000FF via 7f7ae53 January 11, 2024 21:19

ash-ddog requested changes Jan 17, 2024

View reviewed changes

dtru-ddog marked this pull request as draft February 14, 2024 15:10

smuhit added 7 commits February 20, 2024 19:54

[GCP V1] Migrate to FW Provider, Add ResourceCollectionEnabled and Is…

ae740d8

…SecurityCommandCenterEnabled fields Update datadog client library Remove unused variable Fixes for migration to fwprovider More fixes Update codeowners for changes to gcp

Updating docs

06486d3

Fixes for tests (recording now works properly)

0d4f9a6

Add new default value ("is_security_command_center_enabled":false) to…

4169e84

… cassettes interacting with the GCP APIs

Updates to docs

55757c0

Update docs to indicate that the resource is deprecated

b6fc6d0

fix docs

PR Comments

03e539a

smuhit force-pushed the sabbir.muhit/gcp-1258/add-resource-collection-flag-to-gcp-resources branch from 7f7ae53 to 03e539a Compare February 22, 2024 00:00

smuhit marked this pull request as ready for review February 22, 2024 14:44

Small doc upda†e

c7c8af3

dtru-ddog requested review from ash-ddog and nina9753 February 22, 2024 14:57

nina9753 approved these changes Feb 26, 2024

View reviewed changes

ash-ddog approved these changes Feb 27, 2024

View reviewed changes

dtru-ddog approved these changes Feb 27, 2024

View reviewed changes

skarimo approved these changes Mar 12, 2024

View reviewed changes

skarimo merged commit ab5bb04 into master Mar 12, 2024
10 checks passed

skarimo deleted the sabbir.muhit/gcp-1258/add-resource-collection-flag-to-gcp-resources branch March 12, 2024 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[datadog_integration_gcp] Migrate to FW Provider, Add ResourceCollectionEnabled and IsSecurityCommandCenterEnabled fields #2230

[datadog_integration_gcp] Migrate to FW Provider, Add ResourceCollectionEnabled and IsSecurityCommandCenterEnabled fields #2230

smuhit commented Jan 5, 2024 •

edited

brett0000FF Jan 8, 2024

dtru-ddog Jan 11, 2024

brett0000FF Jan 11, 2024

ash-ddog left a comment

ash-ddog Jan 17, 2024

ash-ddog Jan 17, 2024

ash-ddog Jan 17, 2024

smuhit Feb 23, 2024

ash-ddog Jan 17, 2024

ash-ddog Jan 17, 2024 •

edited

smuhit Feb 21, 2024

ash-ddog Jan 17, 2024

smuhit Feb 21, 2024

ash-ddog Jan 17, 2024

ash-ddog Jan 17, 2024

smuhit Feb 23, 2024

ash-ddog Jan 17, 2024

ash-ddog Jan 17, 2024

ash-ddog Jan 17, 2024

smuhit Jan 17, 2024

dtru-ddog commented Feb 14, 2024

ash-ddog left a comment •

edited

ash-ddog Feb 27, 2024

ash-ddog Feb 27, 2024

smuhit Feb 27, 2024

ash-ddog Feb 27, 2024 •

edited

	This resource is deprecated — use the `datadog_integration_gcp_sts` resource instead. Provides a Datadog - Google Cloud Platform integration resource. This can be used to create and manage Datadog - Google Cloud Platform integration.
	This resource is deprecated—use the `datadog_integration_gcp_sts` resource instead. Provides a Datadog - Google Cloud Platform integration resource. This can be used to create and manage Datadog - Google Cloud Platform integration.

[datadog_integration_gcp] Migrate to FW Provider, Add ResourceCollectionEnabled and IsSecurityCommandCenterEnabled fields #2230

[datadog_integration_gcp] Migrate to FW Provider, Add ResourceCollectionEnabled and IsSecurityCommandCenterEnabled fields #2230

Conversation

smuhit commented Jan 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ash-ddog 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

ash-ddog Jan 17, 2024 • edited

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

dtru-ddog commented Feb 14, 2024

ash-ddog left a comment • edited

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

ash-ddog Feb 27, 2024 • edited

Choose a reason for hiding this comment

smuhit commented Jan 5, 2024 •

edited

ash-ddog Jan 17, 2024 •

edited

ash-ddog left a comment •

edited

ash-ddog Feb 27, 2024 •

edited