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

feature(terraform): init terraform module #83

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

Conversation

SollyzDev
Copy link
Contributor

@SollyzDev SollyzDev commented Aug 26, 2024

This PR creates a terraform module the creates the Forwarder lambda and allows the user to pass a list of log group names to create the needed Subscription filters.

@SollyzDev
Copy link
Contributor Author

SollyzDev commented Aug 26, 2024

hey @pecigonzalo regarding your comment previously:

Code wise/module wise, I dont think we should be doing the log-group subscriptions from Terraform. I think I mentioned the same in CF. Given we have Lambdas that can do the subscription backfill and dynamic addition of new log-groups, I don't think static resources (tf and cf) are suitable for handling dynamic resources like log-groups.

The problem with Subscriber/Unsubscriber module is that it created resources that Clouformation is not aware of. I wanted to avoid this pattern with terraform, and make it easier to find all the created subscription logs and easily maintain a list of log group names. I agree with you there should be a better way to handle the dynamic log groups. Wondering if current setup + listener only would fit, wdyt?

@pecigonzalo
Copy link

The problem with Subscriber/Unsubscriber module is that it created resources that Clouformation is not aware of. I wanted to avoid this pattern with terraform, and make it easier to find all the created subscription logs and easily maintain a list of log group names. I agree with you there should be a better way to handle the dynamic log groups. Wondering if current setup + listener only would fit, wdyt?

I understand the reasoning. The challenge is that the dynamic nature of resource like cloudwatch log groups does not fit well with Terraform rather static and stateful nature. If AWS supported a single global "subscription config" for all CWLG (CloudWatch Log Group) I would certainly prefer to configure this via Terraform.

For example, as a "Cloud Engineer", as soon as a new CWLG is created I want to ensure its logs are propagated the logging pipeline.
If this is done via a static terraform config for which I have to populate names, I have to either preemptively create the group and subscription or do it after it was created and I notice my logs are missing.
Furthermore, this is a challenge because its likely that the code that provisions the resource that creates the CWLG and the place in which I configure this are different.

I think the idea of having a CWLG controller, that takes care of configuring them all (kind of what we did with the lambda, and something I've implemented myself at other places) is more maintainable, fit for porpouse and works well in most environments.
The way I generally configure this is with a single controller lambda that has multiple triggers:

  • CWLG creation event -> Subscribe and set retention
  • Periodic -> Check all CWLG and reconcile any drift
  • Manual event -> Check a named CWLG and reconcile

So instead of having two ways of doing this, one that is a bit more subpar than the other (terraform), I would advocate for a single recommended approach. Particularly when anyone needing to configure this via Terraform only needs 1 or 2 resources at most, so its not something for which we add a ton of value in our module or they might get more value out of the settings of the module they might using already (be that a dedicated CWLG module or a Lambda or RDS module that already contains some CWLG settings).

.github/CODEOWNERS Outdated Show resolved Hide resolved
module/README.md Outdated Show resolved Hide resolved
@SollyzDev SollyzDev force-pushed the terraform-module branch 12 times, most recently from 443af34 to ef6f93e Compare September 30, 2024 15:10
@SollyzDev SollyzDev force-pushed the terraform-module branch 2 times, most recently from 24b253b to dcdd46d Compare October 3, 2024 12:23
@SollyzDev
Copy link
Contributor Author

SollyzDev commented Oct 4, 2024

@a-khaledf this is ready to be released. Let's make sure to squash those commits into one.

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