Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add template to enable log forwarding for the clouds #109

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

edwardsitj-stratascale
Copy link

@edwardsitj-stratascale edwardsitj-stratascale commented Jun 29, 2023

Background

Found a gap in the init script where it would be beneficial to have log forwarding in the user_data for certain clouds. This is especially helpful if it is an autoscale group or scale set.

Relates OR Closes #0000

How has this been tested?

Validated that it generated the script template correctly and did not affect existing templates. Have not tested the log forwarding settings in each of the clouds.

TFE Modules

Did you add a new setting?

If no, you may delete these tasks.
If yes, please check each box after you have have added an issue in the TFE modules to add this setting:

This PR makes me feel

optional gif describing your feelings about this pr

@edwardsitj-stratascale edwardsitj-stratascale requested a review from a team as a code owner June 29, 2023 19:03
@anniehedgpeth
Copy link
Contributor

Thank you so much for your contribution! This is definitely something we needed to add, so your PR is much appreciated. I will review and test this next week.

Copy link
Contributor

@anniehedgpeth anniehedgpeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @edwardsitj-stratascale, I had a chance to look at your PR, and we so appreciate all the work you put into it!

I have a comment below about the overall design that I'm hoping isn't too much work to move around. If you don't have time to address it, then we can certainly put it on our backlog for some point soon.

And please let me know if you understand what I'm saying. It was a little difficult to describe. :D

Thanks!

Comment on lines 67 to 69
log_settings = var.log_settings != null ? var.log_settings : {
log_destination = null
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the one thing to know about the design we chose for these modules is that we feed all of the settings through the settings module. So, log_forwarding is admittedly a bit annoying in this regard because you'd have to set up the config there in the settings module (probably in its own tfe_log_forwarding_config.tf file like this one for external vault). And then feed it to the tfe_init module, where you would leave the setup_log_forwarding function as is, but instead of having the variables in the tfe_init module, you'd call the values off of the tfe_configuration output of the settings module.

Then the setup_log_forwarding.log_settings value would probably be:

     log_settings  = try(var.tfe_configuration.log_forwarding_config.value, null)

Also, I might change it to log_forwarding_config just to be a little clearer.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @anniehedgpeth , I made the recommended changes. I found that log_forwarding_config was already listed in the settings module although it looks incomplete. I added the additional variable object and passed it as a new local called log_forwarding_cloud_config to be merged in to the tfe_configuration.

I also added the log_forwarding_config to the setup_log_forwarding script template so that users could still pass the fluentbit config as a string if needed.

@edwardsitj-stratascale edwardsitj-stratascale changed the title Add template to enable log forwarding for the clouds - 2 Add template to enable log forwarding for the clouds Jul 10, 2023
base_external_configs = local.pg_optional_configs != null && (var.enable_active_active || var.production_type == "external") ? (merge(local.pg_configs, local.pg_optional_configs)) : (merge(local.pg_configs, {
pg_extra_params = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept getting an error here when I would plan that false did not have the pg_extra_params attribute. Not sure if it broke in a later version of terraform.

@@ -27,6 +27,10 @@ locals {
value = var.log_forwarding_config
}

log_forwarding_cloud_config = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These setting names need to be exactly what we have set in Replicated, so you would put this config in the log_fowarding_config block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants