Feat sink billing account #129

renato-rudnicki · 2024-02-28T18:30:18Z

No description provided.

This reverts commit ee3554c.

…dule" This reverts commit 14a3a99.

This reverts commit f4f2eb8.

This reverts commit bebcbb2.

This reverts commit 85d6844.

This reverts commit 57ec9f6.

1-org/envs/shared/org_policy.tf

1-org/modules/centralized-logging/variables.tf

1-org/modules/centralized-logging/main.tf

daniel-cit · 2024-02-28T20:41:56Z

1-org/modules/centralized-logging/README.md

 | logging\_project\_key | (Optional) The key of logging destination project if it is inside resources map. It is mandatory when resource\_type = project and logging\_target\_type = logbucket. | `string` | `""` | no |
 | pubsub\_options | Destination Pubsub options:<br>- topic\_name: The name of the pubsub topic to be created and used for log entries matching the filter.<br>- logging\_sink\_name: The name of the log sink to be created.<br>- logging\_sink\_filter: The filter to apply when exporting logs. Only log entries that match the filter are exported. Default is "" which exports all logs.<br>- create\_subscriber: Whether to create a subscription to the topic that was created and used for log entries matching the filter. If 'true', a pull subscription is created along with a service account that is granted roles/pubsub.subscriber and roles/pubsub.viewer to the topic. | <pre>object({<br>    topic_name          = optional(string, null)<br>    logging_sink_name   = optional(string, null)<br>    logging_sink_filter = optional(string, "")<br>    create_subscriber   = optional(bool, true)<br>  })</pre> | `null` | no |
-| resource\_type | Resource type of the resource that will export logs to destination. Must be: project, organization, or folder. | `string` | n/a | yes |
+| resource\_type | Resource type of the resource that will export logs to destination. Must be: project, organization or folder. | `string` | n/a | yes |


daniel-cit · 2024-02-28T20:42:31Z

1-org/modules/centralized-logging/main.tf

+  topic   = module.destination_pubsub[0].resource_name
+  role    = "roles/pubsub.publisher"
+  member  = module.log_export_billing["${each.value}_pub"].writer_identity
+}


missing new line at the end of file

daniel-cit · 2024-02-28T20:58:21Z

1-org/modules/centralized-logging/variables.tf


 variable "resource_type" {
-  description = "Resource type of the resource that will export logs to destination. Must be: project, organization, or folder."
+  description = "Resource type of the resource that will export logs to destination. Must be: project, organization or folder."


restore serial comma
https://developers.google.com/style/commas#serial-commas

daniel-cit · 2024-02-28T20:58:37Z

1-org/modules/centralized-logging/variables.tf

  validation {
    condition     = contains(["project", "folder", "organization"], var.resource_type)
-    error_message = "The resource_type value must be: project, organization, or folder."
+    error_message = "The resource_type value must be: project, organization or folder."


serial comma
https://developers.google.com/style/commas#serial-commas

romanini-ciandt · 2024-03-04T13:09:27Z

1-org/envs/shared/variables.tf

 * limitations under the License.
 */

+variable "enable_billing_account_sink" {


I'm not sure if we want to do that, but a possibility would be to add this variable into the .tfvars example file too.
That way it would be more clear for the user about how to enable the billing account sink.

this variable should not exist at the env/shared level, only in the module, and the foundation should always set it to true when calling the module

romanini-ciandt · 2024-03-04T13:15:59Z

1-org/modules/centralized-logging/main.tf

    lbk = try(module.destination_logbucket[0].destination_uri, "")
  }

+  destination_resource_uri = {


I can be wrong, but I believe that destination_resource_uri map shouldn't have empty string ("") values for certain keys, otherwise the for_each we are consuming this variable will raise an error.

A possible approach to avoid that could be something like that:

filtered_destination_resource_uri = { for key, value in destination_resource_uri : key => value if value != "" }

yes, the values need to be filtered before usage

daniel-cit · 2024-03-04T14:02:57Z

1-org/modules/centralized-logging/main.tf

+
+  # Set permission only on sinks for this destination using
+  # module.log_export_billing key "<resource>_<dest>"
+  member = module.log_export_billing["_lbk"].writer_identity


is the value "_lbk" correct?

the local var keys do not have _

destination_resource_uri = { pub = try(module.destination_pubsub[0].destination_name, "") sto = try(module.destination_storage[0].destination_name, "") lbk = try(module.destination_logbucket[0].destination_name, "") }

@renato-rudnicki

daniel-cit · 2024-03-04T14:17:57Z

1-org/modules/centralized-logging/main.tf

    lbk = try(module.destination_logbucket[0].destination_uri, "")
  }

+  destination_resource_uri = {


It should be called destination_resource_name not destination_resource_uri
or destination_name_map

romanini-ciandt

Just a small suggestion. Overall LGTM!

(Assuming that the test is passing locally)

romanini-ciandt · 2024-03-12T18:30:00Z

test/integration/org/org_test.go

+					destination: fmt.Sprintf("pubsub.googleapis.com/projects/%s/topics/%s", auditLogsProjectID, logsExportTopicName),
+				},
+			} {
+				logSinkBilling := gcloud.Runf(t, "logging sinks describe %s ----billing-account %s", sinkBilling.name, billingAccount)


Suggested change

logSinkBilling := gcloud.Runf(t, "logging sinks describe %s ----billing-account %s", sinkBilling.name, billingAccount)

logSinkBilling := gcloud.Runf(t, "logging sinks describe %s --billing-account %s", sinkBilling.name, billingAccount)

romanini-ciandt · 2024-03-12T18:45:33Z

test/integration/org/org_test.go

+			billingAccount := org.GetStringOutput("billing_account")
+			billingLBKSinkName := org.GetStringOutput("billing_sink_names")[0]
+			billingPUBSinkName := org.GetStringOutput("billing_sink_names")[1]
+			billingSTOSinkName := org.GetStringOutput("billing_sink_names")[2]


A possible improvement that I see here would be to change these outputs to return a map instead of a list, so we can access the values like org.GetStringOutput("billing_sink_names")["lbk"] instead of the index [0].

If we do that, this test would be less fragile and if for some reason the order of this output is changed, the test will continue to work as expected.

It is just an improvement suggestion, not a blocker. Feel free to evaluate if it is worthy :)

daniel-cit

some changes regarding the outputs

daniel-cit · 2024-03-12T19:06:27Z

1-org/modules/centralized-logging/outputs.tf

+output "enable_billing_account_sink" {
+  description = "If true, a log router sink will be created for the billing account. The billing_account variable cannot be null."
+  value       = var.enable_billing_account_sink
+}


this output should be removed since it is an input for the module

daniel-cit · 2024-03-12T19:07:13Z

1-org/modules/centralized-logging/outputs.tf

+  value = [
+    for key, options in local.destinations_options : "${coalesce(options.logging_sink_name, local.logging_sink_name_map[key])}-billing-${random_string.suffix.result}"
+  ]


this should be a map like
https://github.com/terraform-google-modules/terraform-example-foundation/blob/master/0-bootstrap/outputs.tf#L96C1-L99C2

daniel-cit · 2024-03-12T19:07:34Z

1-org/modules/centralized-logging/outputs.tf

+output "billing_account" {
+  description = "Billing Account ID used in case sinks are under billing account level."
+  value       = var.billing_account
+}


The billing account should not be an output since it is an input for the stage.

If it is needed for the integration test the value should be retrieve form the output of the test setup

https://github.com/terraform-google-modules/terraform-example-foundation/blob/master/test/integration/bootstrap/bootstrap_test.go#L54

https://github.com/terraform-google-modules/terraform-example-foundation/blob/master/test/integration/bootstrap/bootstrap_test.go#L62C58-L62C99

using the billing account output

https://github.com/renato-rudnicki/terraform-example-foundation/blob/master/test/setup/outputs.tf#L43C1-L45C2

daniel-cit · 2024-03-12T19:08:24Z

1-org/envs/shared/outputs.tf

+output "billing_account" {
+  value       = module.logs_export.billing_account
+  description = "Billing Account ID used in case sinks are under billing account level. "
+}


The billing account should not be an output since it is an input for the stage.

If it is needed for the integration test the value should be retrieve form the output of the test setup

https://github.com/terraform-google-modules/terraform-example-foundation/blob/master/test/integration/bootstrap/bootstrap_test.go#L54

https://github.com/terraform-google-modules/terraform-example-foundation/blob/master/test/integration/bootstrap/bootstrap_test.go#L62C58-L62C99

using the billing account output

https://github.com/renato-rudnicki/terraform-example-foundation/blob/master/test/setup/outputs.tf#L43C1-L45C2

daniel-cit · 2024-03-12T19:11:26Z

test/integration/org/org_test.go

+			billingAccount := org.GetStringOutput("billing_account")
+			billingLBKSinkName := org.GetStringOutput("billing_sink_names")[0]
+			billingPUBSinkName := org.GetStringOutput("billing_sink_names")[1]
+			billingSTOSinkName := org.GetStringOutput("billing_sink_names")[2]


daniel-cit · 2024-03-12T19:13:29Z

test/integration/org/org_test.go

+				destination string
+			}{
+				{
+					name:        string(billingLBKSinkName),


this is already a string

daniel-cit · 2024-03-12T19:13:36Z

test/integration/org/org_test.go

+					destination: fmt.Sprintf("storage.googleapis.com/%s", logsExportStorageBucketName),
+				},
+				{
+					name:        string(billingSTOSinkName),


this is already a string

daniel-cit · 2024-03-12T19:13:46Z

test/integration/org/org_test.go

+					destination: fmt.Sprintf("logging.googleapis.com/%s", logBktFullName),
+				},
+				{
+					name:        string(billingPUBSinkName),


this is already a string

… /test/integration (terraform-google-modules#1162) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

daniel-cit · 2024-03-14T17:09:27Z

1-org/modules/centralized-logging/main.tf

+#---------------------------------------------------------------#
+resource "google_storage_bucket_iam_member" "storage_sink_member_billing" {
+  count = var.enable_billing_account_sink == true && var.storage_options != null ? length(var.resources) : 0
+  #count = var.enable_billing_account_sink && var.storage_options != null ? length(var.resources) : 0


remove this commented line

daniel-cit · 2024-03-14T17:10:20Z

test/integration/org/org_test.go

 			assert.Equal(logsExportTopicFullName, logsExportTopic.Get("name").String(), fmt.Sprintf("topic %s should have been created", logsExportTopicName))

-			// logging sinks
+			// // logging sinks


Suggested change

// // logging sinks

// logging sinks

daniel-cit · 2024-03-14T17:11:09Z

test/integration/org/org_test.go

 			}

+			// Log Sink billing
+			billingAccount := bootstrap.GetTFSetupStringOutput("billing_account")


Suggested change

billingAccount := bootstrap.GetTFSetupStringOutput("billing_account")

billingAccount := org.GetTFSetupStringOutput("billing_account")

daniel-cit · 2024-03-14T17:44:23Z

1-org/modules/centralized-logging/variables.tf

+variable "enable_billing_account_sink" {
+  description = "If true, a log router sink will be created for the billing account. The billing_account variable cannot be null."
+  type        = bool
+  default     = true


I think the default should be false because the billing account default value is null, which would make the module if the user did not change one of the values.

changing the default to false will also require that the call to the module uses

billing_account = local.billing_account enable_billing_account_sink = true

renato-rudnicki added 17 commits February 14, 2024 18:51

add suport to billing account logs

6d26c13

small improvements

690f6e7

create custom log bucket and sink for project destination

57ec9f6

fix lint

85d6844

Fix duplicated sink name

bebcbb2

fix integration tests

f4f2eb8

adds unique_writer_identity in internal_project_log_export module

14a3a99

update code

ee3554c

Revert "update code"

5530ed9

This reverts commit ee3554c.

Revert "adds unique_writer_identity in internal_project_log_export mo…

4775ef3

…dule" This reverts commit 14a3a99.

Revert "fix integration tests"

df8dc0c

This reverts commit f4f2eb8.

Revert "Fix duplicated sink name"

47418a3

This reverts commit bebcbb2.

Revert "fix lint"

06ec1c1

This reverts commit 85d6844.

Revert "create custom log bucket and sink for project destination"

5c34cd5

This reverts commit 57ec9f6.

add randon for billing sink

fa23017

fix for billing account sink

46e44cb

update README

648ed1a

romanini-ciandt reviewed Feb 28, 2024

View reviewed changes

1-org/envs/shared/org_policy.tf Outdated Show resolved Hide resolved

1-org/modules/centralized-logging/variables.tf Show resolved Hide resolved

1-org/modules/centralized-logging/main.tf Outdated Show resolved Hide resolved

daniel-cit reviewed Feb 28, 2024

View reviewed changes

renato-rudnicki added 2 commits February 28, 2024 18:11

fix lint

f3c7fbc

fixes for module log_export_billing

db9f4e1

romanini-ciandt reviewed Mar 4, 2024

View reviewed changes

daniel-cit reviewed Mar 4, 2024

View reviewed changes

renato-rudnicki added 3 commits March 4, 2024 16:51

add PR suggestions

ea4e2c0

update

b6bf537

fixes for log_export_billing

07df788

add integration tests for sink under billing account level

e759cf9

romanini-ciandt reviewed Mar 12, 2024

View reviewed changes

daniel-cit requested changes Mar 12, 2024

View reviewed changes

renato-rudnicki and others added 3 commits March 12, 2024 19:15

code review improvements

b41b322

fix lint

459fc90

chore(deps): bump google.golang.org/protobuf from 1.31.0 to 1.33.0 in…

32fb194

… /test/integration (terraform-google-modules#1162) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

daniel-cit reviewed Mar 14, 2024

View reviewed changes

renato-rudnicki added 2 commits March 14, 2024 14:17

Merge branch 'master' into feat_sink_billing_account

8b17343

removing comments

1fe50db

daniel-cit reviewed Mar 14, 2024

View reviewed changes

renato-rudnicki and others added 7 commits March 14, 2024 15:46

change default value for enable_billing_account_sink variable

a8b11b8

adds get sink permission on CICD SA

bc5c46d

update resource name for billign account sink

e1274a6

fix!: Groups creation and permissions (terraform-google-modules#1110)

f57a47c

Merge branch 'master' into feat_sink_billing_account

27807fd

chore: update CODEOWNERS

be6b808

Merge branch 'master' into feat_sink_billing_account

a950798

	\| resource\_type \| Resource type of the resource that will export logs to destination. Must be: project, organization or folder. \| `string` \| n/a \| yes \|
	\| resource\_type \| Resource type of the resource that will export logs to destination. Must be: project, organization, or folder. \| `string` \| n/a \| yes \|

	logSinkBilling := gcloud.Runf(t, "logging sinks describe %s ----billing-account %s", sinkBilling.name, billingAccount)
	logSinkBilling := gcloud.Runf(t, "logging sinks describe %s --billing-account %s", sinkBilling.name, billingAccount)

	billingAccount := bootstrap.GetTFSetupStringOutput("billing_account")
	billingAccount := org.GetTFSetupStringOutput("billing_account")

Feat sink billing account #129

Are you sure you want to change the base?

Feat sink billing account #129

Uh oh!

Conversation

renato-rudnicki commented Feb 28, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-cit Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-cit Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romanini-ciandt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-cit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

daniel-cit Mar 4, 2024 •

edited

Loading

daniel-cit Mar 4, 2024 •

edited

Loading