feat!: Support lifecycle rules and labels for GCS submodule #96
Conversation
|
Thanks for the PR! 🚀 |
…nation, updated relevant examples. 2. Added opinion based service account creation for subscriber, not always a service account would be required
sumeetoc
changed the title
|
@bharathkkb could you please comment on this PR, I would be glad to make necessary updates to atleast get storage labels and lifecycle rules implemented for aggregated log gcs bucket |
bharathkkb
left a comment
bharathkkb
left a comment
There was a problem hiding this comment.
Thanks for the PR @sumeetoc
It would be great to put some more details into the PR description which currently states only bucket label changes.
| for_each = var.expiration_days == null ? [] : [var.expiration_days] | ||
| for_each = var.lifecycle_rules |
There was a problem hiding this comment.
This is breaking change and we will need an upgrade guide. Something similar to the diff you have in the readme should work. Example https://github.com/terraform-google-modules/terraform-google-log-export/blob/master/docs/upgrading_to_v5.0.md
modules/pubsub/main.tf
Outdated
| count = var.create_subscriber ? 1 : 0 | ||
| count = var.create_subscriber_sa ? 1 : 0 | ||
| account_id = local.subscriber_id | ||
| display_name = "${local.topic_name} Topic Subscriber" | ||
| project = var.project_id | ||
| } | ||
|
|
||
| resource "google_pubsub_subscription_iam_member" "pubsub_subscriber_role" { | ||
| count = var.create_subscriber ? 1 : 0 | ||
| count = var.create_subscriber_sa ? 1 : 0 | ||
| role = "roles/pubsub.subscriber" | ||
| project = var.project_id | ||
| subscription = local.pubsub_subscription | ||
| member = "serviceAccount:${google_service_account.pubsub_subscriber[0].email}" | ||
| } | ||
|
|
||
| resource "google_pubsub_topic_iam_member" "pubsub_viewer_role" { | ||
| count = var.create_subscriber ? 1 : 0 | ||
| count = var.create_subscriber_sa ? 1 : 0 |
There was a problem hiding this comment.
Could you elaborate on why you need to break this functionality into two flags?
There was a problem hiding this comment.
Sure, so not all the SIEM tool requires service account to authenticate to the Pub/Sub, in few tools it uses users oauth tokens to authenticate. Though the existing SA creation is helpful, it might be additional/unnecessary resource for few deployments, hence the additional flag
There was a problem hiding this comment.
@sumeetoc in those cases I think the best approach would be to create the subscription outside of the module
There was a problem hiding this comment.
@bharathkkb are you suggesting to revert back the changes on pub/sub and if such requirement comes in then create subscription separately?
There was a problem hiding this comment.
@sumeetoc yes correct since I think its easy enough to create just the subscription use this module topic output for your usecase.
There was a problem hiding this comment.
@bharathkkb I've pushed the reverted changes for pub/sub, thanks for looking into this, please let me know if anything else is required
Thanks, done |
|
@bharathkkb the upgrade document is pushed in latest commit, please review and let me know if anything else is required. Would be glad to use this directly in approaching deployment :) |
bharathkkb
left a comment
bharathkkb
left a comment
There was a problem hiding this comment.
Guide LGTM, thanks for adding.
modules/pubsub/main.tf
Outdated
| count = var.create_subscriber ? 1 : 0 | ||
| count = var.create_subscriber_sa ? 1 : 0 | ||
| account_id = local.subscriber_id | ||
| display_name = "${local.topic_name} Topic Subscriber" | ||
| project = var.project_id | ||
| } | ||
|
|
||
| resource "google_pubsub_subscription_iam_member" "pubsub_subscriber_role" { | ||
| count = var.create_subscriber ? 1 : 0 | ||
| count = var.create_subscriber_sa ? 1 : 0 | ||
| role = "roles/pubsub.subscriber" | ||
| project = var.project_id | ||
| subscription = local.pubsub_subscription | ||
| member = "serviceAccount:${google_service_account.pubsub_subscriber[0].email}" | ||
| } | ||
|
|
||
| resource "google_pubsub_topic_iam_member" "pubsub_viewer_role" { | ||
| count = var.create_subscriber ? 1 : 0 | ||
| count = var.create_subscriber_sa ? 1 : 0 |
There was a problem hiding this comment.
@sumeetoc in those cases I think the best approach would be to create the subscription outside of the module
bharathkkb
changed the title
3 participants
This PR provides following updates: