-
Notifications
You must be signed in to change notification settings - Fork 444
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
Add datastream to collect audit logs in kubernetes integration #2317
Add datastream to collect audit logs in kubernetes integration #2317
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
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.
Great addition!
- Could we also add sample logs along with a pipeline test?
- Also since I guess there is a specific pattern in these logs, will we be explicit in the fields we are storing here?
packages/kubernetes/changelog.yml
Outdated
@@ -1,4 +1,9 @@ | |||
# newer versions go on top | |||
- version: "1.5.1" |
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.
- version: "1.5.1" | |
- version: "1.6.0" |
New feature ;)
991373d
to
f03d57a
Compare
@ChrsMark thank you for the feedback!
I will have a look how to add it 👍
do you mean specifically |
Well if the possible fields are unknown then yes we can do something similar to what we do for |
@@ -0,0 +1,15 @@ | |||
title: "Kubernetes audit logs" | |||
type: logs | |||
release: experimental |
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.
@ChrsMark should I keep it as experimental
?
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 think it should be experimental
or beta
at least until we have tests+dashboards. @akshay-saraswat could confirm this too.
parsers: | ||
- ndjson: | ||
add_error_key: true | ||
target: "kubernetes_audit" |
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.
without defined target
- decoded json fields will be written under root
related issue - elastic/beats#29422
- rename: | ||
fields: | ||
- from: "kubernetes_audit" | ||
to: "kubernetes.audit" |
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.
it is not possible to use kubernetes.audit
as a target
, related issue: elastic/beats#29395
function process(event) { | ||
var audit = event.Get("kubernetes.audit"); | ||
var annotation; | ||
for (annotation in audit["annotations"]) { |
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.
You can remove declaration of annotation
and change the iteration to this
for (var annotation in audit["annotations"]) {
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.
done 6dad8d7
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.
Looks good! I left a few thoughts about the fields.
The way that we use the parser and the script processor is the best way possible at the moment I think so we are good here, good job!
Fyi we will need dashboard and tests for this at some point. Maybe a meta issue to keep track of this would help since it's not that much urgent to have them really soon :).
description: Time the request reached current audit stage | ||
- name: annotations.* | ||
type: object | ||
object_type: text |
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.
Any reason for not using keyword
here? In kubernetes it is defined as keyword: https://github.com/elastic/integrations/blob/master/packages/kubernetes/data_stream/container/fields/base-fields.yml#L57. In general we should use keyword when we want to make this field to be heavily searchable.
Also do you think we could move this field under kubernetes.annotations.*
that is already defined for kubernetes? In this way we might be able to corelate events with resources, however I'm not quite sure what values these annotations of the events could take and if related with the annotations of a resource. Let me know what you think.
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.
Any reason for not using keyword here? In kubernetes it is defined as keyword: https://github.com/elastic/integrations/blob/master/packages/kubernetes/data_stream/container/fields/base-fields.yml#L57. In general we should use keyword when we want to make this field to be heavily searchable.
I used text
, because one of the fields authorization_k8s_io/reason
seems to be rather an text according to the name and the example value below:
"annotations": {
"authorization_k8s_io/decision": "allow",
"authorization_k8s_io/reason": "RBAC: allowed by ClusterRoleBinding \"system:public-info-viewer\" of ClusterRole \"system:public-info-viewer\" to Group \"system:unauthenticated\""
},
Do you think it would be better to use keyword
instead
Also do you think we could move this field under kubernetes.annotations.* that is already defined for kubernetes? In this way we might be able to corelate events with resources, however I'm not quite sure what values these annotations of the events could take and if related with the annotations of a resource. Let me know what you think.
I think those annotations.*
are not related to the resource: from the doc:
Note that these annotations are for the audit event, and do not correspond to the metadata.annotations of the submitted object. Keys should uniquely identify the informing component to avoid name collisions (e.g. podsecuritypolicy.admission.k8s.io/policy). Values should be short. Annotations are included in the Metadata level.
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.
Ok, I think we can leave them as is for now. In any case this is still beta
so we will have the chance to iterate on this later if we have feedback.
"version": "8.0.0" | ||
}, | ||
"host": { | ||
"architecture": "x86_64", |
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.
For shake of simplicity we usually remove hosts's informations from sample events. A very baseline event with the actually interesting fields populated should be enough.
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.
thanks for letting me know, I think I've checked few sample_event files in kubernetes package (node
, pod
) and they actually contain all fields, including host
, so the idea is just to keep kubernetes.*
here, right?
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 would say that yes, the simple the better. I don't see any reason into having all these extra metadata for sample events that want to just show k8s relevant fields. Other data_streams might have been ported automatically and hence this is why they brought sample events from Beats repo that might container all those. I don't think it is documented somewhere however it's rather a "best-practice" convention :).
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.
done 74b92dc
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
…ssor; add script processor to dedoc annotations Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
74b92dc
to
7dd1f6a
Compare
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
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.
🚢 it!
…ic#2317) * add kubernetes audit logs Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * fix pr number; commit manifest changes * add empty line in the end; remove data_stream.namespace; fix readme Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * add sample_event; use json parser instead of decode_json_fields processor; add script processor to dedoc annotations Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * fix formatting error Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * remove declaration of annotation Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * keep only kubernetes.* fields in sample_event Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * remove only from sample_event Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * add new sample_event Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * keep only one ip and mac Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * run es-package format Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
Signed-off-by: Tetiana Kravchenko tetiana.kravchenko@elastic.co
What does this PR do?
This PR adds a new data stream in kubernetes integration to collect audit logs.
Checklist
changelog.yml
file.How to test this PR locally
Related issues
Screenshots