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.

Sign up for GitHub

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

Jump to bottom

feat: add functionality to forward Cloudwatch Events #48

Merged
merged 10 commits into from
Mar 8, 2023
Merged

feat: add functionality to forward Cloudwatch Events #48

merged 10 commits into from
Mar 8, 2023

Conversation

kevcube
Copy link
Contributor

@kevcube kevcube commented Feb 28, 2023

what

  • Enables forwarding of CloudWatch Events to Datadog Logs
  • Uses Cloudposse Cloudwatch Events module
  • Upgrades tf to >=1.3.0 to enable optionals

why

  • Enables us to get GuardDuty events into DD Logs

@kevcube kevcube requested review from a team as code owners February 28, 2023 16:46
@Gowiem
Copy link
Member

Gowiem commented Mar 2, 2023

/test all

@Gowiem
Copy link
Member

Gowiem commented Mar 3, 2023

/test all

@Gowiem
Copy link
Member

Gowiem commented Mar 3, 2023

@kevcube those tests really don't like this PR 😅. Can you access them?

@kevcube
Copy link
Contributor Author

kevcube commented Mar 3, 2023 •
edited
Loading

@Gowiem I think it may be that there's some terratest updates or something that the bot is not able to commit to my fork, but I don't know why that's the case.. looking

@kevcube
Copy link
Contributor Author

kevcube commented Mar 3, 2023 •
edited
Loading

fixed some issues by running some of the bot workflows manually. @Gowiem can you kick off tests again

@Gowiem
Copy link
Member

Gowiem commented Mar 3, 2023

/test all

@Gowiem
Copy link
Member

Gowiem commented Mar 3, 2023

@kevcube :

    	Error Trace:	apply.go:15
    	            				examples_complete_test.go:38
    	Error:      	Received unexpected error:
    	            	FatalError{Underlying: error while running command: exit status 1; ╷
    	            	│ Error: creating EventBridge Rule (eg-ue2-test-datadog-lambda-forwarder-96834-logs): ConcurrentModificationException: com.amazon.aws.platform.tagris.client.exception.TagrisClientException: A resource with the same resourceName but a different internalId already exists: 8a494429-297d-3b47-840f-af8392eeacfc
    	            	│ 
    	            	│   with module.datadog_lambda_log_forwarder.module.cloudwatch_event["cloudtrail"].aws_cloudwatch_event_rule.this,
    	            	│   on .terraform/modules/datadog_lambda_log_forwarder.cloudwatch_event/main.tf line 10, in resource "aws_cloudwatch_event_rule" "this":
    	            	│   10: resource "aws_cloudwatch_event_rule" "this" {
    	            	│ 
    	            	╵}
    	Test:       	TestExamplesComplete

@kevcube
Copy link
Contributor Author

kevcube commented Mar 3, 2023

@Gowiem fixed

kevcube
kevcube commented Mar 3, 2023
View reviewed changes
lambda-log.tf Outdated

for_each = local.lambda_enabled && var.forwarder_log_enabled ? var.cloudwatch_forwarder_event_patterns : {}

name = "${module.forwarder_log_label.id}-${each.key}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is probably a better way I should be naming these.. should I be making a new label module for each cloudwatch_event??

I may update this child module to expose the cloudwatch property name_prefix instead of only name.

Copy link
Member

Choose a reason for hiding this comment

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

Aha - you should be passing context (e.g. context = module.forwarder_log_label.context) and then adding a specific name for the key (like you're doing) or adding attributes as well if there are going to be conflicts with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it - did that and it looks a lot better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-03-06 at 11 17 56

though it does override var.name..

and if I use attributes, then not everyone is using attributes. I think I'm still gonna expose name_prefix in the child module'

@kevcube kevcube requested review from Gowiem and removed request for dotCipher and florian0410 March 7, 2023 15:46
@Gowiem
Copy link
Member

Gowiem commented Mar 7, 2023

/test all

Gowiem
Gowiem reviewed Mar 7, 2023
View reviewed changes
versions.tf
@@ -1,5 +1,5 @@
terraform {
required_version = ">= 0.13"
required_version = ">= 1.3.0"
Copy link
Member

Choose a reason for hiding this comment

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

@kevcube this is the only thing holding us up. I'm still discussing with the maintainer team on this.

Copy link
Member

Choose a reason for hiding this comment

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

Got the general 👍 on this after some back and forth discussion. 🎉

@Gowiem Gowiem added the minor New features that do not break anything label Mar 8, 2023
@Gowiem Gowiem changed the title add functionality to forward Cloudwatch Events feat: add functionality to forward Cloudwatch Events Mar 8, 2023
@Gowiem Gowiem merged commit 4337de0 into cloudposse:master Mar 8, 2023
This was referenced Mar 20, 2023
Closed
Merged
@kevcube kevcube deleted the master branch March 21, 2023 17:27
@kevcube kevcube mentioned this pull request Mar 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gowiem Gowiem Gowiem approved these changes

Assignees
No one assigned
Labels
minor New features that do not break anything
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kevcube @Gowiem @cloudpossebot