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

feat(lambda-promtail): Adding S3 log parser support for AWS GuardDuty #13148

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

samuelebstein
Copy link
Contributor

@samuelebstein samuelebstein commented Jun 5, 2024

This pull request introduces the following changes to the lambda-promtail module:

  1. Adding Terraform Variable:

    • Introduces a new Terraform variable to allow a different filter suffix for the S3 bucket notification resource. This enhancement provides more flexibility in configuring S3 bucket notifications.
  2. Adding GuardDuty Log Type:

    • Adds support for parsing GuardDuty log types in the S3 log parser. This ensures that Promtail can push GuardDuty findings logs to Loki for monitoring and analysis.

Which issue(s) this PR fixes:
Fixes # #13129

Why this PR is needed

  • The addition of the filter suffix variable allows users to customize the suffix used for S3 bucket notifications, which is particularly useful for different logging requirements and setups.
  • Including GuardDuty log types in the S3 log parser expands Promtail's capabilities, enabling it to handle and forward GuardDuty findings logs, which are crucial for security monitoring.

Checklist

  • Added the new Terraform variable to the lambda-promtail module.
  • Updated the S3 log parser to include GuardDuty log types.
  • Tested the changes to ensure they work as expected.
  • Updated documentation.

Upgrading Steps

If these changes affect the default configuration, metrics names, log lines used in dashboards or alerts, configuration parameters, or API endpoints, please document what has changed and what needs to be done in the upgrade guide.

  • Default configuration values: None affected.
  • Metric names or label names: None affected.
  • Changes to existing log lines: None affected.
  • Configuration parameters: Added new variable for filter suffix with default value that is the same as the previously hardcoded value.
  • Breaking changes to HTTP or gRPC API endpoints: None.

Please review the changes and let me know if there are any questions or concerns. Thank you!

@CLAassistant
Copy link

CLAassistant commented Jun 5, 2024

CLA assistant check
All committers have signed the CLA.

@samuelebstein samuelebstein force-pushed the sam/lambda-promtail-guardduty branch from c11321f to 7d810f4 Compare June 7, 2024 21:36
samuelebstein added a commit to samuelebstein/tkhq-loki that referenced this pull request Jun 7, 2024
…na#13148)

Co-authored-by: James Callahan <https://github.com/james-callahan>ase enter the commit message for your changes. Lines starting
samuelebstein added a commit to samuelebstein/tkhq-loki that referenced this pull request Jun 7, 2024
…na#13148)

Co-authored-by: James Callahan <https://github.com/james-callahan>ase enter the commit message for your changes. Lines starting
@samuelebstein samuelebstein force-pushed the sam/lambda-promtail-guardduty branch from 7d810f4 to f26c354 Compare June 7, 2024 21:37
@samuelebstein samuelebstein changed the title tools/lambda-promtail: feat(promtail): Adding S3 log parser support for AWS GuardDuty Jun 7, 2024
@samuelebstein samuelebstein marked this pull request as ready for review June 7, 2024 21:38
@samuelebstein samuelebstein requested a review from a team as a code owner June 7, 2024 21:38
samuelebstein added a commit to tkhq/loki that referenced this pull request Jun 10, 2024
…na#13148)

Co-authored-by: James Callahan <https://github.com/james-callahan>ase enter the commit message for your changes. Lines starting
tools/lambda-promtail/lambda-promtail/s3.go Outdated Show resolved Hide resolved
tools/lambda-promtail/lambda-promtail/s3.go Outdated Show resolved Hide resolved
tools/lambda-promtail/variables.tf Outdated Show resolved Hide resolved
@evilr00t
Copy link

evilr00t commented Jul 10, 2024

Bump, any update on this? GuardDuty findings support would be great to have!

edit: I've made a fork and applied this patch, so far it looks good and guardduty logs in loki seems to be fine! Ruler also does not complain.

I can step in and fix issues in the comments - would that be ok and have chances to be merged?

@cstyan cstyan changed the title feat(promtail): Adding S3 log parser support for AWS GuardDuty feat(lambda-promtail): Adding S3 log parser support for AWS GuardDuty Jul 16, 2024
@evilr00t
Copy link

I've noticed that for some logs, wrong timestamp is being used - in our case launchTime from Instance resource.

Reading the code I see promtail is already aware of such behaviour but for CloudTrail logs, I've added this parser for GuardDuty log types and so far it looks ok.

diff --git a/tools/lambda-promtail/lambda-promtail/s3.go b/tools/lambda-promtail/lambda-promtail/s3.go
index 1ddac3500..c4fdd21ca 100644
--- a/tools/lambda-promtail/lambda-promtail/s3.go
+++ b/tools/lambda-promtail/lambda-promtail/s3.go
@@ -179,7 +179,7 @@ func parseS3Log(ctx context.Context, b *batch, labels map[string]string, obj io.
        ls = applyLabels(ls)

        // extract the timestamp of the nested event and sends the rest as raw json
-       if labels["type"] == CLOUDTRAIL_LOG_TYPE {
+       if labels["type"] == CLOUDTRAIL_LOG_TYPE || labels["type"] == GUARDDUTY_LOG_TYPE {
                records := make(chan Record)
                jsonStream := NewJSONStream(records)
                go jsonStream.Start(gzreader, parser.skipHeaderCount)
(END)

looks like quick fix for now - FYI.

@samuelebstein
Copy link
Contributor Author

samuelebstein commented Jul 23, 2024

@evilr00t

Bump, any update on this? GuardDuty findings support would be great to have!

edit: I've made a fork and applied this patch, so far it looks good and guardduty logs in loki seems to be fine! Ruler also does not complain.

I can step in and fix issues in the comments - would that be ok and have chances to be merged?

That would be great. Please do. I created a fork and used it and then had to prioritize other work. But would love this to get merged

@cstyan
Copy link
Contributor

cstyan commented Jul 23, 2024

@samuelebstein it looks like @evilr00t has an additional fix we should add in as well? after that I think we're probably good to go

@cstyan
Copy link
Contributor

cstyan commented Jul 23, 2024

I think you might need to also rebase on main to get CI to run properly

samuelebstein added a commit to tkhq/loki that referenced this pull request Jul 23, 2024
…na#13148)

Co-authored-by: James Callahan <https://github.com/james-callahan>ase enter the commit message for your changes. Lines starting
samuelebstein added a commit to samuelebstein/tkhq-loki that referenced this pull request Jul 23, 2024
…na#13148)

Co-authored-by: James Callahan <https://github.com/james-callahan>ase enter the commit message for your changes. Lines starting
samuelebstein added a commit to samuelebstein/tkhq-loki that referenced this pull request Jul 23, 2024
…na#13148)

Co-authored-by: James Callahan <https://github.com/james-callahan>ase enter the commit message for your changes. Lines starting
samuelebstein added a commit to samuelebstein/tkhq-loki that referenced this pull request Jul 23, 2024
…na#13148)

Co-authored-by: James Callahan <https://github.com/james-callahan>ase enter the commit message for your changes. Lines starting
@samuelebstein
Copy link
Contributor Author

@cstyan all comments resolved. Feel free to merge now!

@cstyan
Copy link
Contributor

cstyan commented Jul 24, 2024

@samuelebstein in the future please try and avoid force pushing during reviews, it makes it harder to see what has changed since the last review

@cstyan cstyan merged commit 2d92fff into grafana:main Jul 24, 2024
61 checks passed
@evilr00t
Copy link

Sorry guys for being silent, didn't notice your conversation! Kudos on getting this merged, will check that soon!

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

Successfully merging this pull request may close these issues.

5 participants