8000 [v1.16] hubble: fix flowfilter flag parsing allowing only one filter by devodev · Pull Request #38794 · cilium/cilium · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[v1.16] hubble: fix flowfilter flag parsing allowing only one filter #38794

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 8, 2025
Merged

[v1.16] hubble: fix flowfilter flag parsing allowing only one filter #38794

merged 1 commit into from
Apr 8, 2025

Conversation

devodev
Copy link
Contributor
@devodev devodev commented Apr 7, 2025 •
edited
Loading

Description

This PR fixes the flag parsing of hubble-export-allowlist and hubble-export-denylist that currently only considers one JSON object from the Helm chart allowList and denyList options.

Details

The parsing logic for the Hubble flowfilter flags does not exhaust the JSON decoder, resulting in only the first filter to be considered.

Using the following Helm values:

  export:
    static:
      enabled: true
      filePath: /var/run/cilium/hubble/events.log
      allowList: []
      denyList:
        - '{"source_pod":["kube-system/"],"destination_ip":["1.1.1.1/32"]}'
        - '{"destination_pod":["kube-system/"],"source_ip":["1.1.1.1/32"]}'

Which will be used generate the cilium configmap:

{{- if .Values.hubble.export.static.enabled }}
  ...
  hubble-export-allowlist: {{ .Values.hubble.export.static.allowList | join "," | quote }}
  hubble-export-denylist: {{ .Values.hubble.export.static.denyList | join "," | quote }}
{{- end }}

It seems we assume that the flag parsing done by viper when using stringSlice will return a list of JSON objects, but it actually returns one string with all JSON objects joined with a comma. Adding a log statement confirms the latter:

time="2025-04-07T19:43:20Z" level=info msg="decoding denylist argument from flag: hubble-export-denylist" filter-string="{\"source_pod\":[\"kube-system/\"],\"destination_ip\":[\"1.1.1.1/32\"]},{\"destination_pod\":[\"kube-system/\"],\"source_ip\":[\"1.1.1.1/32\"]}" idx=0 subsys=config type=deny

The JSON decoder is able to decode the first object, and on the next iteration would error out on the comma which it does not understand when outside an object, but since we don't continue iterating after the first result, no errors are raised, and consequently we also silently ignore the remaining objects.

To fix, join with an empty separator and wrap the decoding in a for loop to ensure we exhaust the content of the reader passed in the decoder.

This has already been fixed in 1.17+ following the migration of the exporter codebase to a hive cell and using a different flag parsing method.

Testing

Add a log statement when building the filter list and notice that Hubble only parses the first filter it sees:

time="2025-04-07T19:27:47Z" level=info msg="  --hubble-export-allowlist=''" subsys=daemon
time="2025-04-07T19:27:47Z" level=info msg="  --hubble-export-denylist='{\"source_pod\":[\"kube-system/\"],\"destination_ip\":[\"1.1.1.1/32\"]}{\"destination_pod\":[\"kube-system/\"],\"source_ip\":[\"1.1.1.1/32\"]}'" subsys=daemon
time="2025-04-07T19:27:52Z" level=info msg="building filter list for allowlist" filters="[]" subsys=daemon
time="2025-04-07T19:27:52Z" level=info msg="building filter list for denylist" filters="[source_pod:\"kube-system/\" destination_ip:\"1.1.1.1/32\"]" subsys=daemon

Then apply the fix described above and confirm we see both filters in the filter list:

time="2025-04-07T19:30:13Z" level=info msg="  --hubble-export-allowlist=''" subsys=daemon
time="2025-04-07T19:30:13Z" level=info msg="  --hubble-export-denylist='{\"source_pod\":[\"kube-system/\"],\"destination_ip\":[\"1.1.1.1/32\"]}{\"destination_pod\":[\"kube-system/\"],\"source_ip\":[\"1.1.1.1/32\"]}'" subsys=daemon
time="2025-04-07T19:30:18Z" level=info msg="building filter list for allowlist" filters="[]" subsys=daemon
time="2025-04-07T19:30:18Z" level=info msg="building filter list for denylist" filters="[source_pod:\"kube-system/\" destination_ip:\"1.1.1.1/32\" source_ip:\"1.1.1.1/32\" destination_pod:\"kube-system/\"]" subsys=daemon

@devodev
hubble: fix flowfilter flag parsing allowing only one filter
4c74741 
It seems we assume that the flag parsing done by viper when using
stringSlice will return a list of JSON objects, but it actually returns
one string with all JSON objects joined with a comma.

The JSON decoder is able to decode the first object, and on the next
iteration would error out on the comma which it does not understand
when outside an object, but since we don't continue iterating after
the first result, no errors are raised, and consequently we also silently
ignore the remaining objects.

To fix, join with an empty separator and wrap the decoding in a for loop.
This has also been fixed in 1.17+ following the migration of the exporter
codebase to a hive cell and using a different flag parsing method.

Signed-off-by: Alexandre Barone <abalexandrebarone@gmail.com>
@devodev devodev requested a review from a team as a code owner April 7, 2025 20:13
@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.16 This PR represents a backport for Cilium 1.16.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. labels Apr 7, 2025
@devodev
Copy link
Contributor Author
devodev commented Apr 7, 2025

/test

@maintainer-s-little-help 8000 er maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 8, 2025
kaworu
kaworu approved these changes Apr 8, 2025
View reviewed changes
Copy link
Member
@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Thank you @devodev !

@kaworu kaworu added this pull request to the merge queue Apr 8, 2025
Merged via the queue into cilium:v1.16 with commit 0d077b7 Apr 8, 2025
60 of 62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@kaworu kaworu kaworu approved these changes

Assignees
No one assigned
Labels
backport/1.16 This PR represents a backport for Cilium 1.16.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@devodev @gandro @kaworu
0