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

Hubble: add possibility to export flows to container logs #31422

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

siegmund-heiss-ich
Copy link
Contributor

@siegmund-heiss-ich siegmund-heiss-ich commented Mar 15, 2024

This is a modification of the static exporter for flow logs which allows setting the hubble-export-file-path to stdout. This triggers not the creation of rotating files but the direct output to logs.

The benefit is the possibility to ship flow logs to a SIEM for continuous analysis without the deployment of a separate daemonset like used here: https://github.com/cilium/hubble-otel

I am happy for any suggestions.

@siegmund-heiss-ich siegmund-heiss-ich requested a review from a team as a code owner March 15, 2024 15:26
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 15, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Mar 15, 2024
@rolinh rolinh added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/hubble Impacts hubble server or relay labels Mar 15, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 15, 2024
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

lgtm 🚀

  • could you squash these 2 commits into 1 commit?
  • curious how it shows up in kubectl logs. could you post a sample output?

@michi-covalent
Copy link
Contributor

/test

@siegmund-heiss-ich
Copy link
Contributor Author

lgtm 🚀

* could you squash these 2 commits into 1 commit?

* curious how it shows up in kubectl logs. could you post a sample output?

Thanks!
The commits are merged now.

This is how one flow shows up in the log now:

level=info msg="{\"flow\":{\"time\":\"2024-03-15T22:23:36.124398461Z\",\"uuid\":\"a4100bd7-9732-442e-bbc8-beb5c7212f44\",\"verdict\":\"FORWARDED\",\"ethernet\":{\"source\":\"72:c0:06:45:0a:22\",\"destination\":\"76:a3:43:b7:98:5c\"},\"IP\":{\"source\":\"10.244.1.114\",\"destination\":\"10.244.1.113\",\"ipVersion\":\"IPv4\"},\"l4\":{\"TCP\":{\"source_port\":4222,\"destination_port\":33564,\"flags\":{\"FIN\":true,\"ACK\":true}}},\"source\":{\"ID\":2864,\"identity\":2542,\"namespace\":\"kube-system\",\"labels\":[\"k8s:app.kubernetes.io/name=hubble-relay\",\"k8s:app.kubernetes.io/part-of=cilium\",\"k8s:io.cilium.k8s.namespace.labels.kubernetes.io/metadata.name=kube-system\",\"k8s:io.cilium.k8s.policy.cluster=kind-kind\",\"k8s:io.cilium.k8s.policy.serviceaccount=hubble-relay\",\"k8s:io.kubernetes.pod.namespace=kube-system\",\"k8s:k8s-app=hubble-relay\"],\"pod_name\":\"hubble-relay-76cbb4895d-hd99d\",\"workloads\":[{\"name\":\"hubble-relay\",\"kind\":\"Deployment\"}]},\"destination\":{\"identity\":1,\"labels\":[\"reserved:host\"]},\"Type\":\"L3_L4\",\"node_name\":\"kind-kind/kind-worker\",\"reply\":true,\"event_type\":{\"type\":4,\"sub_type\":3},\"traffic_direction\":\"INGRESS\",\"trace_observation_point\":\"TO_STACK\",\"is_reply\":true,\"Summary\":\"TCP Flags: ACK, FIN\"},\"node_name\":\"kind-kind/kind-worker\",\"time\":\"2024-03-15T22:23:36.124398461Z\"}" subsys=hubble

@michi-covalent
Copy link
Contributor

/test

@michi-covalent
Copy link
Contributor

looks like https://github.com/cilium/cilium/actions/runs/8302891162/job/22726503509?pr=31422 is complaining about commit message format. there should an extra blank line between the title and body:

Add possibility to export flow logs to stdout

Signed-off-by: siegmund-heiss-ich <119589995+siegmund-heiss-ich@users.noreply.github.com>

Copy link
Contributor

@chancez chancez left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me. Overall, the code changes look good. Just a few doc updates needed:

@michi-covalent
Copy link
Contributor

/test

auto-merge was automatically disabled March 16, 2024 00:49

Head branch was pushed to by a user without write access

@michi-covalent
Copy link
Contributor

/test

@siegmund-heiss-ich
Copy link
Contributor Author

siegmund-heiss-ich commented Mar 16, 2024

looks like https://github.com/cilium/cilium/actions/runs/8302891162/job/22726503509?pr=31422 is complaining about commit message format. there should an extra blank line between the title and body:

Add possibility to export flow logs to stdout

Signed-off-by: siegmund-heiss-ich <119589995+siegmund-heiss-ich@users.noreply.github.com>

Sorry, I didn't see that message before...
Should be fixed now.

@qmonnet
Copy link
Member

qmonnet commented Mar 18, 2024

/test

@chancez
Copy link
Contributor

chancez commented Mar 19, 2024

/test

@qmonnet
Copy link
Member

qmonnet commented Mar 19, 2024

Please run make -C Documentation update-cmdref locally and add the changes to your commit, to update the command reference with the updated description for hubble-export-file-path. This should fix the failing documentation workflow.

@qmonnet
Copy link
Member

qmonnet commented Mar 19, 2024

/test

@chancez chancez requested a review from glibsm March 21, 2024 16:17
Copy link
Member

@glibsm glibsm left a comment

Choose a reason for hiding this comment

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

Requesting changes to halt the PR for a little bit so it's not merged accidentally.

pkg/hubble/exporter/exporter.go Outdated Show resolved Hide resolved
pkg/hubble/exporter/exporter.go Outdated Show resolved Hide resolved
@siegmund-heiss-ich
Copy link
Contributor Author

siegmund-heiss-ich commented Mar 22, 2024

According to @glibsm 's change request, the Hubble flows are now directly written to stdout instead of using logrus, because of performance concerns.

In addition, there are no more escaped characters in the output string, which makes it easier to use.
The drawback with this solution is, that there are no additional fields anymore, which shouldn't impose any problems.

Now flows are written to stdout like that:

{"flow":{"time":"2024-03-22T09:59:48.002076710Z","verdict":"FORWARDED","IP":{"source":"10.0.2.183","destination":"10.0.3.43","ipVersion":"IPv4"},"l4":{"TCP":{"source_port":80,"destination_port":46820}},"source":{"namespace":"default","pod_name":"deathstar-f449b9b55-hwz2h"},"destination":{"namespace":"default","pod_name":"tiefighter"},"node_name":"kind-worker3","l7":{"type":"RESPONSE","latency_ns":"1273541","http":{"code":403,"method":"PUT","url":"http://deathstar.default.svc.cluster.local/v1/exhaust-port","protocol":"HTTP/1.1","headers":[{"key":"Content-Length","value":"15"},{"key":"Content-Type","value":"text/plain"},{"key":"X-Request-Id","value":"30b0870c-cb91-44dd-a2ae-8f4ad88da582"}]}},"is_reply":true},"node_name":"kind-worker3","time":"2024-03-22T09:59:48.002076710Z"}

This is the most efficient solution possible. If additional fields are necessary, the data can be manipulated outside of cilium (fluent-bit, otel ...).

Hopefully, this PR can be merged soon.

@siegmund-heiss-ich
Copy link
Contributor Author

Requesting changes to halt the PR for a little bit so it's not merged accidentally.

@glibsm
Is there any news? You've been blocking this PR for over a month now without any further feedback.

Copy link
Member

@glibsm glibsm left a comment

Choose a reason for hiding this comment

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

Sorry about the long delay, this PR slipped through my review queue somehow

Documentation/cmdref/cilium-agent.md Show resolved Hide resolved
@chancez
Copy link
Contributor

chancez commented Apr 15, 2024

Check patch is failing: https://github.com/cilium/cilium/actions/runs/8388580874/job/23305658705?pr=31422

Error: ERROR:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'siegmund-heiss-ich 119589995+siegmund-heiss-ich@users.noreply.github.com'

Looks like something is off about your commit/sign-off.

This is a modification of the static exporter for flow logs, which allows setting the hubble-export-file-path to stdout. This triggers not the creation of rotating files but the direct output to logs.
The benefit is the possibility to ship flow logs to a SIEM for continuous analysis without the deployment of a separate daemonset.

Signed-off-by: Alois Petutschnig <alois@petutschnig.net>
@chancez
Copy link
Contributor

chancez commented Apr 17, 2024

/test

@pchaigno pchaigno enabled auto-merge April 22, 2024 19:58
@pchaigno pchaigno added this pull request to the merge queue Apr 22, 2024
Merged via the queue into cilium:main with commit aa02b74 Apr 22, 2024
62 checks passed
@siegmund-heiss-ich siegmund-heiss-ich deleted the feat/addStdoutExport branch April 22, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/community-contribution This was a contribution made by a community member. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/hubble Impacts hubble server or relay
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants