GCP logging/monitoring configurable #116
Conversation
There was a problem hiding this comment.
LGTM, but since it introduces a new metadata attribute, this becomes a breaking change as it will re-create the instance.
Maybe we can use this opportunity and flip oslogin to false by default, what do you think, @bschaatsbergen?
|
@kpocius I see oslogin being set to false already by default in the variable file .... where do you see it set to true? |
|
@contil, sorry for the confusion. My memory tricked me -- I thought it was set to true in order not to become a breaking change. Turns out it wasn't. |
|
@contil thanks for raising this PR, I'll go through it tomorrow 👍🏼 |
There was a problem hiding this comment.
Thanks for this PR, it looks ok - though I'm curious what it exactly enables as I already see logs and metrics.. can you be a bit more specific on the metadata attributes please? :)
And regarding the examples that you've updated, I would prefer to have them not in there as it adds extra noise to the example code as they're simply optional variables anyways.
| image = "<your-image>" | ||
| domain = "<example.com>" | ||
| managed_zone = "<your-managed-zone>" | ||
| enable_oslogin = false |
There was a problem hiding this comment.
Can we remove the locals from all the examples, as these are optional variables anyways and now add extra entries to the list in the example.
There was a problem hiding this comment.
the main reason I created this PR was to allow to make these settings configurable ... i.e. logging is being transitioned from old to new ( using now fluentbit etc ). The main idea was to make sure we have terraform clearly state the requirements and do not depend on defaults as well for different versions of COS etc.
| managed_zone = "<your-managed-zone>" | ||
| enable_oslogin = false | ||
| google_logging_enabled = true | ||
| google_logging_use_fluentbit = false |
There was a problem hiding this comment.
Can we remove the locals from all the examples, as these are optional variables anyways and now add extra entries to the list in the example.
There was a problem hiding this comment.
I added the locals to make sure the user understand the choices are deliberate and can be changed ONLY looking at the locals section without altering the bulk of the code past the locals.
| managed_zone = "<your-managed-zone>" | ||
| enable_oslogin = false | ||
| google_logging_enabled = true | ||
| google_logging_use_fluentbit = false |
There was a problem hiding this comment.
Can we remove the locals from all the examples, as these are optional variables anyways and now add extra entries to the list in the example.
|
|
||
| variable "google_logging_enabled" { | ||
| type = bool | ||
| description = "Enable Google Cloud Logging" |
There was a problem hiding this comment.
What does it exactly enable? As logging already works, if I look a Cloud Logging I can already see container logs being written there.
There was a problem hiding this comment.
Logging is being changed and these settings should allow to make some choices that will enforce settings even when defaults will change and/or to disable some features
|
|
||
| variable "google_monitoring_enabled" { | ||
| type = bool | ||
| description = "Enable Google Cloud Monitoring" |
There was a problem hiding this comment.
What does it exactly enable?
There was a problem hiding this comment.
metadata settings and their mapping to COS milestones are described in https://cloud.google.com/container-optimized-os/docs/how-to/logging
|
Hi @contil, I was on holiday for a long period - I'm addressing some changes this week and will look into yours! |
what
why