[datadog_integration_gcp_sts] Add Account tags, ResourceCollectionEnabled and IsSecurityCommandCenterEnabled support to gcp tf module #2134

dtru-ddog · 2023-10-04T13:20:10Z

Purpose
Attempting to add new fields for gcp sts tf module to utilize and set account tags for customer accounts.

ash-ddog

Nothing really blocking, but we should talk about some of these comments. 👍

Also, are there no tests in the terraform stuff? 🤔

datadog/fwprovider/resource_datadog_integration_gcp_sts.go

ash-ddog · 2023-10-04T18:22:15Z

datadog/fwprovider/resource_datadog_integration_gcp_sts.go

@@ -240,6 +249,12 @@ func (r *integrationGcpStsResource) buildIntegrationGcpStsRequestBody(ctx contex
 	diags := diag.Diagnostics{}
 	attributes := datadogV2.NewGCPSTSServiceAccountAttributesWithDefaults()

+	accountTags := make([]string, 0)


This is not idiomatic Go. Typically, you initialize slices to nil. nil slices are valid for just about anything (i.e. iteration, appending, etc).
var accountTags []string

But not familiar enough with this terraform stuff to know if that'll make a difference.

Guess this is a copy paste from the code for host_filters (same type)

ash-ddog · 2023-10-04T18:22:34Z

datadog/fwprovider/resource_datadog_integration_gcp_sts.go

+	if !state.AccountTags.IsNull() {
+		diags.Append(state.AccountTags.ElementsAs(ctx, &accountTags, false)...)
+	}
+	attributes.SetAccountTags(accountTags)


Feels weird to include account_tags=[] in the REST call when it's an optional field.

ash-ddog · 2023-10-04T18:22:51Z

datadog/fwprovider/resource_datadog_integration_gcp_sts.go

+	if !state.AccountTags.IsNull() {
+		diags.Append(state.AccountTags.ElementsAs(ctx, &accountTags, false)...)
+	}
+	attributes.SetAccountTags(accountTags)


Same comments apply here.

Should we be doing something like the IsUnknown() for AccountTags (like we're doing below) since it's also a new field? Not familiar enough with terraform to know what's right here.

ash-ddog · 2023-10-04T18:35:07Z

datadog/fwprovider/resource_datadog_integration_gcp_sts.go

+		diags.Append(state.AccountTags.ElementsAs(ctx, &accountTags, false)...)
+	}
+	attributes.SetAccountTags(accountTags)
+
 	if !state.Automute.IsNull() {
 		attributes.SetAutomute(state.Automute.ValueBool())
 	}


All this stuff is super duplicative. Is it worth refactoring like this?

func (r *integrationGcpStsResource) buildIntegrationGcpStsCreateRequest(ctx context.Context, state *integrationGcpStsModel) (*datadogV2.GCPSTSServiceAccountCreateRequest, diag.Diagnostics) { attrs, diags := r.buildIntegrationGcpStsAccountAttributes(ctx, state) req := datadogV2.NewGCPSTSServiceAccountCreateRequestWithDefaults() req.Data = datadogV2.NewGCPSTSServiceAccountDataWithDefaults() req.Data.SetAttributes(*attrs) return req, diags } func (r *integrationGcpStsResource) buildIntegrationGcpStsUpdateRequest(ctx context.Context, state *integrationGcpStsModel) (*datadogV2.GCPSTSServiceAccountUpdateRequest, diag.Diagnostics) { attrs, diags := r.buildIntegrationGcpStsAccountAttributes(ctx, state) req := datadogV2.NewGCPSTSServiceAccountUpdateRequestWithDefaults() req.Data = datadogV2.NewGCPSTSServiceAccountUpdateRequestDataWithDefaults() req.Data.SetAttributes(*attrs) return req, diags } func (r *integrationGcpStsResource) buildIntegrationGcpStsAccountAttributes(ctx context.Context, state *integrationGcpStsModel) (*datadogV2.GCPSTSServiceAccountAttributes, diag.Diagnostics) { diags := diag.Diagnostics{} attributes := datadogV2.NewGCPSTSServiceAccountAttributesWithDefaults() accountTags := make([]string, 0) if !state.AccountTags.IsNull() { diags.Append(state.AccountTags.ElementsAs(ctx, &accountTags, false)...) } attributes.SetAccountTags(accountTags) if !state.Automute.IsNull() { attributes.SetAutomute(state.Automute.ValueBool()) } if !state.ClientEmail.IsNull() { attributes.SetClientEmail(state.ClientEmail.ValueString()) } if !state.IsCspmEnabled.IsNull() { attributes.SetIsCspmEnabled(state.IsCspmEnabled.ValueBool()) } hostFilters := make([]string, 0) if !state.HostFilters.IsNull() { diags.Append(state.HostFilters.ElementsAs(ctx, &hostFilters, false)...) } attributes.SetHostFilters(hostFilters) return attributes, diags }

Weird thing is that I'm pretty sure that the update API does not support updating ClientEmail, but it was already in the update body before these changes 🥴 .

IMO that's pertinent but maybe an a separated PR?

Looks like we mostly took this (thanks), but still we have ClientEmail in the update API when that's not supported. Not blocking, but could confuse someone in the future.

Agreed.. But didn't want to make functionally different changes (removal of ClientEmail in the Update call) in a PR that is essentially additive changes.

mariuskiessling · 2023-12-19T16:08:00Z

Hey @ash-ddog and @dtru-ddog, do you have any updates on this PR? I was about to hand in my own PR for this missing field when I saw that this one already exists. We rely on setting the account tags in one of our configurations that we provision from Terraform. Anything I can do to help this thing be merged?

NitriKx · 2024-01-18T12:44:10Z

I would also like to see that merged, may I help? For the automated tests we can recycle my closed PR #2248

…urityCommandCenterEnabled fields, add tests, update docs

skarimo · 2024-02-23T18:04:38Z

datadog/fwprovider/resource_datadog_integration_gcp_sts.go

 			"automute": schema.BoolAttribute{
 				Optional:    true,
 				Computed:    true,
 				Description: "Silence monitors for expected GCE instance shutdowns.",
+				Default:     booldefault.StaticBool(false),


Whats the reasoning for this change? This can potentially break existing customers if they set automute to true in the UI.

Fair enough. Hadn't really considered the case where customers were using both terraform and the UI to manage their configuration. Will remove.

skarimo · 2024-02-23T18:06:13Z

datadog/fwprovider/resource_datadog_integration_gcp_sts.go

 			"is_cspm_enabled": schema.BoolAttribute{
 				Optional:    true,
 				Computed:    true,
-				Description: "When enabled, Datadog performs configuration checks across your Google Cloud environment by continuously scanning every resource, which may incur additional charges.",
+				Description: "Whether Datadog collects cloud security posture management resources from your GCP project. If enabled, requires `resource_collection_enabled` to also be enabled.",
+				Default:     booldefault.StaticBool(false),


Same question here about new default value on TF side. Can we rely on the backend to handle defaults properly since it is already computed-optional field on terraform side?

Same as above

skarimo · 2024-02-23T18:08:59Z

datadog/fwprovider/resource_datadog_integration_gcp_sts.go

-	req := datadogV2.NewGCPSTSServiceAccountUpdateRequestWithDefaults()
-	req.Data = datadogV2.NewGCPSTSServiceAccountUpdateRequestDataWithDefaults()
-	req.Data.SetAttributes(attributes)
+	if !state.IsSecurityCommandCenterEnabled.IsNull() {


Suggested change

if !state.IsSecurityCommandCenterEnabled.IsNull() {

if !state.IsSecurityCommandCenterEnabled.IsUnknown() {

ash-ddog

Nothing blocking but most notable comments

Do we want to add CloudRunRevisionFilters ?
Do we need to do IsUnknown() for AccountTags

ash-ddog · 2024-03-01T17:13:43Z

datadog/fwprovider/resource_datadog_integration_gcp_sts.go

+	HostFilters                    types.Set    `tfsdk:"host_filters"`
+	IsCspmEnabled                  types.Bool   `tfsdk:"is_cspm_enabled"`
+	IsSecurityCommandCenterEnabled types.Bool   `tfsdk:"is_security_command_center_enabled"`
+	ResourceCollectionEnabled      types.Bool   `tfsdk:"resource_collection_enabled"`


What is DelegateAccountEmail vs ClientEmail ? My guess is DelegateAccountEmail == client_id and ClientEmail == client_email

Update: read the descriptions below, but still, not sure how DelegateAccountEmail can have any affect on the API call we make since it's not a request parameter?

We're missing cloud_run_revision_filters here, not sure if that's on purpose or not.

Oh I'm sorry, thought this PR was for our V1 API but it's V2.

But still the comments above are valid.

DelegateAccountEmail is the STS delegate. (Basically, the principal that needs the token creator role on the client Email).

In this schema, it's a computed value that this provider returns (and not as input), mainly to allow setup purely through terraform.

Basically:

terraform to create GCP service account (including permissions)

terraform using this module to configure the datadog gcp sts integration (using output from previous)

terraform to give output of integration (second bullet) the token creator role to the service account in first bullet

Let me know if you have questions

We have an upcoming task to add cloud_run_revision_filters.

datadog/fwprovider/resource_datadog_integration_gcp_sts.go

ash-ddog · 2024-03-01T17:19:06Z

datadog/fwprovider/resource_datadog_integration_gcp_sts.go

@@ -71,17 +81,27 @@ func (r *integrationGcpStsResource) Schema(_ context.Context, _ resource.SchemaR
 					stringplanmodifier.UseStateForUnknown(),
 				},
 			},
+			"host_filters": schema.SetAttribute{
+				Optional:    true,
+				Description: "Your Host Filters.",


Kinda nit: might want to be more specific about what this actually is? maybe Filters to be applied for GCE instance resources or something?

I just shuffled it (to be ordered alphabetically).. I didn't actually add this description (It exists currently: https://registry.terraform.io/providers/DataDog/datadog/latest/docs/resources/integration_gcp_sts#host_filters)

We can update it, but can tackle in a separate PR

ash-ddog · 2024-03-01T17:28:36Z

datadog/fwprovider/resource_datadog_integration_gcp_sts.go

+		diags.Append(state.AccountTags.ElementsAs(ctx, &accountTags, false)...)
+	}
+	attributes.SetAccountTags(accountTags)
+
 	if !state.Automute.IsNull() {
 		attributes.SetAutomute(state.Automute.ValueBool())
 	}


Looks like we mostly took this (thanks), but still we have ClientEmail in the update API when that's not supported. Not blocking, but could confuse someone in the future.

ash-ddog · 2024-03-01T17:30:20Z

datadog/fwprovider/resource_datadog_integration_gcp_sts.go

+	if !state.AccountTags.IsNull() {
+		diags.Append(state.AccountTags.ElementsAs(ctx, &accountTags, false)...)
+	}
+	attributes.SetAccountTags(accountTags)


Should we be doing something like the IsUnknown() for AccountTags (like we're doing below) since it's also a new field? Not familiar enough with terraform to know what's right here.

dtru-ddog requested review from a team as code owners October 4, 2023 13:20

dtru-ddog requested a review from mitchellzehr October 4, 2023 13:20

dtru-ddog added the changelog/feature label Oct 4, 2023

dtru-ddog changed the title ~~Add Account tags support to gcp tf module~~ [datadog_integration_gcp_sts] Add Account tags support to gcp tf module Oct 4, 2023

ash-ddog previously approved these changes Oct 4, 2023

View reviewed changes

jhgilbert previously approved these changes Oct 4, 2023

View reviewed changes

Add Account tags support to gcp tf module

9186396

dtru-ddog dismissed stale reviews from jhgilbert and ash-ddog via 9186396 October 23, 2023 00:34

dtru-ddog force-pushed the dan.trujillo/gcp-account-tags-support branch from ba594e9 to 9186396 Compare October 23, 2023 00:34

dtru-ddog removed the request for review from mitchellzehr October 30, 2023 13:09

NitriKx mentioned this pull request Jan 18, 2024

[datadog_integration_gcp_sts] Add account_tags parameter #2248

Closed

smuhit added 2 commits February 22, 2024 20:53

Merge branch 'master' into dan.trujillo/gcp-account-tags-support

e78aa29

[datadog_integration_gcp_sts] Add ResourceCollectionEnabled and IsSec…

ca882c1

…urityCommandCenterEnabled fields, add tests, update docs

smuhit added the gcp-integrations label Feb 23, 2024

smuhit changed the title ~~[datadog_integration_gcp_sts] Add Account tags support to gcp tf module~~ [datadog_integration_gcp_sts] Add Account tags, ResourceCollectionEnabled and IsSecurityCommandCenterEnabled support to gcp tf module Feb 23, 2024

Forgot to update state

0f42469

smuhit requested a review from ash-ddog February 23, 2024 14:49

skarimo reviewed Feb 23, 2024

View reviewed changes

Addressing PR Comments

09d1443

smuhit requested a review from skarimo February 23, 2024 22:12

ash-ddog approved these changes Mar 1, 2024

View reviewed changes

skarimo approved these changes Mar 1, 2024

View reviewed changes

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

skarimo deleted the dan.trujillo/gcp-account-tags-support branch March 12, 2024 14:54

Januznl mentioned this pull request Apr 2, 2024

Added cloud_run_revision_filters option to the GCP STS integration #2344

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[datadog_integration_gcp_sts] Add Account tags, ResourceCollectionEnabled and IsSecurityCommandCenterEnabled support to gcp tf module #2134

[datadog_integration_gcp_sts] Add Account tags, ResourceCollectionEnabled and IsSecurityCommandCenterEnabled support to gcp tf module #2134

dtru-ddog commented Oct 4, 2023 •

edited by smuhit

Loading

ash-ddog left a comment •

edited

Loading

ash-ddog Oct 4, 2023 •

edited

Loading

NitriKx Jan 18, 2024

ash-ddog Oct 4, 2023

ash-ddog Oct 4, 2023

ash-ddog Mar 1, 2024

ash-ddog Oct 4, 2023

NitriKx Jan 18, 2024

ash-ddog Mar 1, 2024

smuhit Mar 1, 2024 •

edited

Loading

mariuskiessling commented Dec 19, 2023

NitriKx commented Jan 18, 2024

skarimo Feb 23, 2024

smuhit Feb 23, 2024

skarimo Feb 23, 2024

smuhit Feb 23, 2024

skarimo Feb 23, 2024

ash-ddog left a comment

ash-ddog Mar 1, 2024 •

edited

Loading

ash-ddog Mar 1, 2024 •

edited

Loading

smuhit Mar 1, 2024 •

edited

Loading

smuhit Mar 1, 2024

ash-ddog Mar 1, 2024

smuhit Mar 1, 2024

ash-ddog Mar 1, 2024

ash-ddog Mar 1, 2024

	if !state.IsSecurityCommandCenterEnabled.IsNull() {
	if !state.IsSecurityCommandCenterEnabled.IsUnknown() {

[datadog_integration_gcp_sts] Add Account tags, ResourceCollectionEnabled and IsSecurityCommandCenterEnabled support to gcp tf module #2134

[datadog_integration_gcp_sts] Add Account tags, ResourceCollectionEnabled and IsSecurityCommandCenterEnabled support to gcp tf module #2134

Conversation

dtru-ddog commented Oct 4, 2023 • edited by smuhit Loading

ash-ddog left a comment • edited Loading

Choose a reason for hiding this comment

ash-ddog Oct 4, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smuhit Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

mariuskiessling commented Dec 19, 2023

NitriKx commented Jan 18, 2024

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

Choose a reason for hiding this comment

ash-ddog Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

ash-ddog Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

smuhit Mar 1, 2024 • 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

Choose a reason for hiding this comment

dtru-ddog commented Oct 4, 2023 •

edited by smuhit

Loading

ash-ddog left a comment •

edited

Loading

ash-ddog Oct 4, 2023 •

edited

Loading

smuhit Mar 1, 2024 •

edited

Loading

ash-ddog Mar 1, 2024 •

edited

Loading

ash-ddog Mar 1, 2024 •

edited

Loading

smuhit Mar 1, 2024 •

edited

Loading