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

Implement audit backend for TimescaleDB. #151

Merged
merged 20 commits into from
Feb 11, 2025
Merged

Implement audit backend for TimescaleDB. #151

merged 20 commits into from
Feb 11, 2025

Conversation

go.mod Outdated Show resolved Hide resolved
@Gerrit91 Gerrit91 marked this pull request as ready for review January 24, 2025 12:22
@Gerrit91 Gerrit91 requested a review from a team as a code owner January 24, 2025 12:22
auditing/timescaledb.go Outdated Show resolved Hide resolved
auditing/timescaledb.go Outdated Show resolved Hide resolved
Comment on lines 256 to 295
if err := addFilter("body", filter.Body, phrase); err != nil {
return nil, err
}
if err := addFilter("component", filter.Component, equals); err != nil {
return nil, err
}
if err := addFilter("detail", filter.Detail, equals); err != nil {
return nil, err
}
if err := addFilter("error", filter.Error, equals); err != nil {
return nil, err
}
if err := addFilter("forwardedfor", filter.ForwardedFor, equals); err != nil {
return nil, err
}
if err := addFilter("path", filter.Path, equals); err != nil {
return nil, err
}
if err := addFilter("phase", filter.Phase, equals); err != nil {
return nil, err
}
if err := addFilter("remoteaddr", filter.RemoteAddr, equals); err != nil {
return nil, err
}
if err := addFilter("rqid", filter.RequestId, equals); err != nil {
return nil, err
}
if err := addFilter("statuscode", filter.StatusCode, equals); err != nil {
return nil, err
}
if err := addFilter("tenant", filter.Tenant, equals); err != nil {
return nil, err
}
if err := addFilter("type", filter.Type, equals); err != nil {
return nil, err
}
if err := addFilter("user", filter.User, equals); err != nil {
return nil, err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

this must all match to the Entry json annotations, easy to make mistakes here or miss some of them. There might be a smarter way, but unsure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only other approach that comes to my mind is to put the field names into constants and build a generic map[string]any with them, which will be JSON-serialized instead of using the struct with JSON annotations. Not sure if it's better.

auditing/timescaledb.go Show resolved Hide resolved
@Gerrit91 Gerrit91 requested a review from vknabel February 4, 2025 13:27
Copy link
Contributor

@vknabel vknabel left a comment

Choose a reason for hiding this comment

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

Looks really promising! Please resolve the conflicts and run go mod tidy

auditing/auditing-interceptor.go Show resolved Hide resolved
auditing/auditing.go Show resolved Hide resolved
auditing/timescaledb.go Outdated Show resolved Hide resolved
@Gerrit91
Copy link
Contributor Author

So, how about this pull request? I think the implementation here should work?

Copy link
Contributor

@majst01 majst01 left a comment

Choose a reason for hiding this comment

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

One small nit, but if fixed we can merge

auditing/timescaledb.go Show resolved Hide resolved
@vknabel
Copy link
Contributor

vknabel commented Feb 11, 2025

Yup, this implementation is fine. I just tend to review the whole bunch of PRs at once. I only had comments on the other two

@Gerrit91 Gerrit91 merged commit a0370f0 into master Feb 11, 2025
2 checks passed
@Gerrit91 Gerrit91 deleted the audit-timescaledb branch February 11, 2025 13:29
@Gerrit91
Copy link
Contributor Author

Thanks for the reviews, I will adapt the related PRs.

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.

3 participants