feat(alert-forwarder): replace Python with Go implementation#18
feat(alert-forwarder): replace Python with Go implementation#18calghar wants to merge 2 commits intodynatrace-oss:mainfrom
Conversation
blu3r4y
left a comment
There was a problem hiding this comment.
Thanks @f4r00q for the PR, looks like a great rewrite of the Python source. I worked through the code and made plenty of suggestions. Let me know if you find the time to integrate them, otherwise, I might also be able to take care of some of them.
There was a problem hiding this comment.
[ISSUE] Please don't delete README.md
There was a problem hiding this comment.
Not sure how this got deleted
alert-forwarder/Dockerfile
Outdated
| @@ -1,19 +1,32 @@ | |||
| FROM python:3.13-slim@sha256:21e39cf1815802d4c6f89a0d3a166cc67ce58f95b6d1639e68a394c99310d2e5 | |||
| # Copyright (c) 2025 Dynatrace LLC | |||
There was a problem hiding this comment.
[ISSUE] License header should only be added to source code files. Please remove it from the Dockerfile.
alert-forwarder/Dockerfile
Outdated
| # You should have received a copy of the GNU Affero General Public License | ||
| # along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| FROM --platform=$BUILDPLATFORM golang:1.21-alpine AS builder |
There was a problem hiding this comment.
[Q] What is --platform=$BUILDPLATFORM doing? If this is useful, should we also add it to the main Dockerfile?
There was a problem hiding this comment.
Its to enable easy cross-platform build. It would make sense to add it to the main file as well
alert-forwarder/Dockerfile
Outdated
| # You should have received a copy of the GNU Affero General Public License | ||
| # along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| FROM --platform=$BUILDPLATFORM golang:1.21-alpine AS builder |
There was a problem hiding this comment.
[ISSUE] Please use Go 1.24 as this is the most recent version
alert-forwarder/Dockerfile
Outdated
| # can be seen more easily (they are logged regardless) | ||
| ENV UVICORN_LOG_LEVEL=error | ||
|
|
||
| FROM alpine:latest |
There was a problem hiding this comment.
[Q] Could you use gcr.io/distroless/static:nonroot here?
alert-forwarder/tetragon.go
Outdated
| functionName, _ := processKprobe["function_name"].(string) | ||
| fileAccessFns := []string{"security_file_permission", "security_mmap_file"} | ||
|
|
||
| for _, fn := range fileAccessFns { |
There was a problem hiding this comment.
[NIT] Those two lines (the for and the if) could be simplified to the following:
if slices.Contains(fileAccessFns, functionName) {
trapType = TrapTypeFilesystemHoneytoken
// ...
}
alert-forwarder/tetragon.go
Outdated
| for _, fn := range fileAccessFns { | ||
| if functionName == fn { | ||
| trapType = TrapTypeFilesystemHoneytoken | ||
| if args, ok := processKprobe["args"].([]interface{}); ok && len(args) > 0 { |
There was a problem hiding this comment.
[NIT] That piece of code looks very hard to read - actually, also, in the Python version. However, in my recent PR I tried to extract this field resolution for honeytokens into their own method. Could you try to do the same for the Golang version here? For reference: https://github.com/dynatrace-oss/koney/blob/feat/TR-4934-dynatrace-ingest/alert-forwarder/forwarder/tetragon.py#L121
alert-forwarder/tetragon.go
Outdated
|
|
||
| timestamp, _ := event["time"].(string) | ||
| if timestamp == "" { | ||
| timestamp = time.Now().Format(time.RFC3339) |
There was a problem hiding this comment.
[ISSUE] No, please not default to the current time, let the field be empty if we cannot resolve it.
alert-forwarder/tetragon.go
Outdated
| func resolveDeceptionPolicyName(kubeClient *kubernetes.Clientset, tracingPolicyName string) (string, error) { | ||
| ctx := context.Background() | ||
|
|
||
| // Create a dynamic client for the custom resource |
There was a problem hiding this comment.
[ISSUE] Hmm, well, if we add github.com/cilium/tetragon as a dependency in go.mod we could use the typed client. If this code is integrated with the main controller, we would already have it.
Anyways, maybe we want to still stick with the untyped client here. But then, I think we shouldn't initialize the client here in tetragon.go but rather in main.go, and then pass the dynamic client. Reason being that client initialization should be decoupled from using them, I dislike that tetragon.go is reading the cluster config again and creating a new client here.
| } | ||
|
|
||
| // Helper functions | ||
| func getStringValue(m map[string]interface{}, key string) string { |
I can take care of this. Give me a few days to get to it. 😊 |
Description
This PR replaces the Python alert-forwarder with a high-performance Go implementation.
Type of Change
Motivation
The Python alert-forwarder works well but has some limitations:
The Go implementation addresses all these issues while maintaining 100% compatibility.
Changes
alert-forwarder/*.pywith Go implementationTesting
Tested on:
Compatibility
Checklist
Additional Notes
The Go implementation uses the official Kubernetes client-go library, providing better compatibility with future Kubernetes versions. The code structure follows Go best practices with clear separation of concerns.
Happy to make any adjustments based on review feedback.