feat: Add the Google PubSub terraform script.

[Marco-Premier]

No description provided.

[wfa-reviewable]

This change is 

[SanjayVas]

PR titles should be imperative sentences (e.g. "Add" not "Adds"). I took the liberty of fixing this one for you. See https://github.com/world-federation-of-advertisers/cross-media-measurement/blob/main/docs/dev-standards.md#pull-request-description

Reviewed 2 of 6 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kungfucraig, @Marco-Premier, and @stevenwarejones)

---

src/main/terraform/gcloud/examples/control-plane/main.tf line 1 at r2 (raw file):

# Copyright 2024 The Cross-Media Measurement Authors

It's premature to add this example, as the example should be for a specific usage/component such as the EDP aggregator or TEE Duchy. Meaning that we'd e.g. have another module for the EDP aggregator that uses the TEE control plane module, and the example would be for that.

---

src/main/terraform/gcloud/control-plane/main.tf line 1 at r2 (raw file):

# Copyright 2023 The Cross-Media Measurement Authors

This should be part of the cmms root module, which covers everything we put into Halo test environments.

---

src/main/terraform/gcloud/modules/control-plane/main.tf line 1 at r2 (raw file):

# Copyright 2024 The Cross-Media Measurement Authors

This module name is too generic. Control plane for what?

[Marco-Premier]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kungfucraig, @SanjayVas, and @stevenwarejones)

---

src/main/terraform/gcloud/control-plane/main.tf line 1 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This should be part of the cmms root module, which covers everything we put into Halo test environments.

I created a secure_computation.tf file where I will add in following PRs all infra needed for the secure computation kotlin package.

i.e.

• storages
• pubsub
• cloud functions
• mig

I thought also to edp_aggregator as a name but I think that mig for instance is not really part of that, wyt?

---

src/main/terraform/gcloud/examples/control-plane/main.tf line 1 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

It's premature to add this example, as the example should be for a specific usage/component such as the EDP aggregator or TEE Duchy. Meaning that we'd e.g. have another module for the EDP aggregator that uses the TEE control plane module, and the example would be for that.

Okay

---

src/main/terraform/gcloud/modules/control-plane/main.tf line 1 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This module name is too generic. Control plane for what?

Renamed it to pubsub, I'll create other modules for:

• mig
• cloud functions

[SanjayVas]

Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @kungfucraig, @Marco-Premier, and @stevenwarejones)

---

src/main/terraform/gcloud/control-plane/main.tf line 1 at r2 (raw file):
This is fine for now, but I do actually think we'll end up with separate modules or examples for the EDP aggregator vs. the TEE-based Duchy. While we'll want to exercise everything in Halo test environments, different entities may want to deploy these separately. For example, an EDP hosting their own private aggregator instance.

I thought also to edp_aggregator as a name but I think that mig for instance is not really part of that, wyt?

I haven't kept up on the scaling discussion in the design doc, but I thought that one of the options was to have each queue responsible for spinning up its own MIGs dynamically. At the very least, I assumed there would be a separate MIG for each queue since each queue processes a different type of workload and therefore has its own VM requirements.

---

src/main/terraform/gcloud/modules/pubsub/main.tf line 1 at r3 (raw file):

# Copyright 2024 The Cross-Media Measurement Authors

This looks like it's intended to be a generic reusable module for pubsub subscriptions, in which case it should have a README.md file. By Terraform convention, externally reusable modules have READMEs. This is also noted in the README for the modules directory.

---

src/main/terraform/gcloud/cmms/secure_computation.tf line 15 at r3 (raw file):

# limitations under the License.

terraform {

Leave out the backend and provider since these are already defined in main.tf. In particular, this uses the same TF state as the rest of the root module.

---

src/main/terraform/gcloud/modules/pubsub/variables.tf line 17 at r3 (raw file):

variable "topic_name" {
  description = "Name of the Pub/Sub topic"
  type        = string

I assume you want nullable = false here to indicate that it's required and non-null.

---

src/main/terraform/gcloud/modules/pubsub/variables.tf line 34 at r3 (raw file):

  description = "The duration (in seconds) for which Pub/Sub retains unacknowledged messages."
  type        = string
  default     = "86400s"

Why do we have our own defaults for these optional fields? Meaning is there a reason that Halo Pub/Sub topic/subscription pairs want these specific values, or can we instead just rely on the defaults from the resource type (i.e. make these default to null)?

Note that this module name indicates that it's generically for Pub/Sub, so it should only have defaults that differ from those in the resource if they can apply generally to most Pub/Sub subscriptions that Halo would use (including those not for TEE).

[Marco-Premier]

Reviewable status: 4 of 8 files reviewed, 5 unresolved discussions (waiting on @kungfucraig, @SanjayVas, and @stevenwarejones)

---

src/main/terraform/gcloud/modules/pubsub/main.tf line 1 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This looks like it's intended to be a generic reusable module for pubsub subscriptions, in which case it should have a README.md file. By Terraform convention, externally reusable modules have READMEs. This is also noted in the README for the modules directory.

Done.

---

src/main/terraform/gcloud/cmms/secure_computation.tf line 15 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Leave out the backend and provider since these are already defined in main.tf. In particular, this uses the same TF state as the rest of the root module.

Done.

---

src/main/terraform/gcloud/control-plane/main.tf line 1 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This is fine for now, but I do actually think we'll end up with separate modules or examples for the EDP aggregator vs. the TEE-based Duchy. While we'll want to exercise everything in Halo test environments, different entities may want to deploy these separately. For example, an EDP hosting their own private aggregator instance.

I thought also to edp_aggregator as a name but I think that mig for instance is not really part of that, wyt?

I haven't kept up on the scaling discussion in the design doc, but I thought that one of the options was to have each queue responsible for spinning up its own MIGs dynamically. At the very least, I assumed there would be a separate MIG for each queue since each queue processes a different type of workload and therefore has its own VM requirements.

that's is what we agreed upon yes, 1 mig per queue/app-version.

---

src/main/terraform/gcloud/modules/pubsub/variables.tf line 17 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I assume you want nullable = false here to indicate that it's required and non-null.

Done.

---

src/main/terraform/gcloud/modules/pubsub/variables.tf line 34 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Why do we have our own defaults for these optional fields? Meaning is there a reason that Halo Pub/Sub topic/subscription pairs want these specific values, or can we instead just rely on the defaults from the resource type (i.e. make these default to null)?

Note that this module name indicates that it's generically for Pub/Sub, so it should only have defaults that differ from those in the resource if they can apply generally to most Pub/Sub subscriptions that Halo would use (including those not for TEE).

Done.

[kungfucraig]

Reviewed 2 of 7 files at r3, all commit messages.
Reviewable status: 4 of 8 files reviewed, 6 unresolved discussions (waiting on @Marco-Premier, @SanjayVas, and @stevenwarejones)

---

src/main/terraform/gcloud/control-plane/main.tf line 1 at r2 (raw file):

Previously, Marco-Premier (marcopremier) wrote…

that's is what we agreed upon yes, 1 mig per queue/app-version.

Given this is empty, maybe drop it for now and come back later and add an application specific TF file(s)?

It's kind of to Sanjay's point, the items in this directory are components of the system (e.g. kingdom) not horizontal functions (e.g. secure computation).

---

src/main/terraform/gcloud/modules/pubsub/variables.tf line 27 at r4 (raw file):

}

variable "message_retention_duration" {

It looks like you having this applying to the subscription, but the field could also apply to the topic. Want to qualify it?
https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/pubsub_topic

Or maybe we should have a separate modules for subscription and topic registration?

They seem fairly independent, and this is how TF has it anyway.

[Marco-Premier]

Reviewable status: 4 of 8 files reviewed, 5 unresolved discussions (waiting on @kungfucraig, @SanjayVas, and @stevenwarejones)

---

src/main/terraform/gcloud/modules/pubsub/variables.tf line 27 at r4 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

It looks like you having this applying to the subscription, but the field could also apply to the topic. Want to qualify it?
https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/pubsub_topic

Or maybe we should have a separate modules for subscription and topic registration?

They seem fairly independent, and this is how TF has it anyway.

I'm open to this but the 2 message_retention_duration works at different level. The one for the sub indicates for how long a message is kept in the queue waiting for a subscriber to process it (7days as default).

The one at topic level is used to enable the "seek feature" (https://cloud.google.com/pubsub/docs/replay-overview) which enables messages to be stored and let subscription to seek back for a particular message even after it was acknowledge.

Since we don't have this feature enable, shall we use a single module for now? I'd like to minimize fragmentation and have a single module for pubsub as long as it covers our use cases.

[kungfucraig]

Reviewable status: 4 of 8 files reviewed, 5 unresolved discussions (waiting on @SanjayVas and @stevenwarejones)

---

src/main/terraform/gcloud/modules/pubsub/variables.tf line 27 at r4 (raw file):

Previously, Marco-Premier (marcopremier) wrote…

I'm open to this but the 2 message_retention_duration works at different level. The one for the sub indicates for how long a message is kept in the queue waiting for a subscriber to process it (7days as default).

The one at topic level is used to enable the "seek feature" (https://cloud.google.com/pubsub/docs/replay-overview) which enables messages to be stored and let subscription to seek back for a particular message even after it was acknowledge.

Since we don't have this feature enable, shall we use a single module for now? I'd like to minimize fragmentation and have a single module for pubsub as long as it covers our use cases.

I don't have a deep understanding of TF best practices and practical implications of this decision. However, convoluting things tends to lead to problems. @SanjayVas wdyt?

[SanjayVas]

Reviewed 1 of 4 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @Marco-Premier and @stevenwarejones)

---

src/main/terraform/gcloud/modules/pubsub/variables.tf line 34 at r3 (raw file):

Previously, Marco-Premier (marcopremier) wrote…

Done.

Not having our own defaults doesn't mean copying other default values, but rather having these be nullable and defaulting to null so we get the default from the underlying resource type.

At that point, I'm not terribly certain I'm seeing the usefulness of this module over just using the underlying resources. Our other modules do something, such as combining resources that are used together (e.g. service account and membership for workload identity users) or setting our opinionated values (e.g. settings we want on every cluster or node pool)

[Marco-Premier]

Reviewable status: 5 of 8 files reviewed, 2 unresolved discussions (waiting on @SanjayVas and @stevenwarejones)

---

src/main/terraform/gcloud/modules/pubsub/variables.tf line 34 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Not having our own defaults doesn't mean copying other default values, but rather having these be nullable and defaulting to null so we get the default from the underlying resource type.

At that point, I'm not terribly certain I'm seeing the usefulness of this module over just using the underlying resources. Our other modules do something, such as combining resources that are used together (e.g. service account and membership for workload identity users) or setting our opinionated values (e.g. settings we want on every cluster or node pool)

my bad! I wasn't sure that null was treated as missing value in order to fallback to terraform default values

[SanjayVas]

Reviewed 1 of 4 files at r4, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)

[stevenwarejones]

Reviewed 1 of 6 files at r1, 2 of 4 files at r2, 4 of 7 files at r3, 1 of 4 files at r4, 1 of 2 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)

---

src/main/terraform/gcloud/modules/pubsub/variables.tf line 27 at r4 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

I don't have a deep understanding of TF best practices and practical implications of this decision. However, convoluting things tends to lead to problems. @SanjayVas wdyt?

so, this is for subscription right? just rename to subscription_queue_retention_period or `subscription_message_retention_period1?

[Marco-Premier]

Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion (waiting on @SanjayVas and @stevenwarejones)

---

src/main/terraform/gcloud/modules/pubsub/variables.tf line 27 at r4 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

so, this is for subscription right? just rename to subscription_queue_retention_period or `subscription_message_retention_period1?

Exactly, this indicates for how long a message (default 7 days) is retained in the subscription queue waiting to be acked.
If after this period the message still has o receive an acknowledge it is removed from the queue (and so not deliverable anymore).