-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Rewrite lambda-promtail to use subscription filters. #4315
Conversation
Signed-off-by: Callum Styan <callumstyan@gmail.com>
XXL tag is due to vendoring changes. |
@julienduchesne let me know if you have any comments on the lambda/terraform here :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only looked at Terraform stuff. LGTM other than some nits
Signed-off-by: Callum Styan <callumstyan@gmail.com>
@julienduchesne thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tools/lambda-promtail/main.tf
Outdated
memory_size = 128 | ||
package_type = "Image" | ||
|
||
# vpc_config { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we expose this in a conditional configuration rather than via comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a look at terraform docs for doing this.
Properties: | ||
DestinationArn: !GetAtt LambdaPromtailFunction.Arn | ||
FilterPattern: "" | ||
LogGroupName: "/aws/lambda/some-lamda-log-group" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, let's use parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one I'd prefer to leave as is, since it's more likely than not that users will have multiple log groups that they want to subscribe to, with different FilterPattern
s per group.
However, I'll have a look at if there's a nice way to duplicate a terraform config block with an array as an input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately there isn't a nice built in way to repeat a resource block in CloudFormation based on a list of variables, but I have made that change to the Terraform file. IMO it's best if users copy paste this section for CloudFormation until we find a better option here.
vars. Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Callum Styan <callumstyan@gmail.com>
the write endpoint and log groups to subscribe to. Signed-off-by: Callum Styan <callumstyan@gmail.com>
Signed-off-by: Callum Styan <callumstyan@gmail.com>
commands in readme Signed-off-by: Callum Styan <callumstyan@gmail.com>
VPC config for promtail-lambda deployment. Signed-off-by: Callum Styan <callumstyan@gmail.com>
Minor code changes to
main.go
, the only code config value needed is the write address. Configuration of CloudWatch subscription filters (previously just configuration of Log Groups to make API calls for) and other AWS side config is done via either the Terraform or CloudFormation files. We also now build the Go code into a docker image; this image needs to be uploaded to an ECR repo to be used by end users in their Lambda deployments.Probably still some README improvements/initial details that can be added but this works ™️
Signed-off-by: Callum Styan callumstyan@gmail.com