hubble: Use netip.Addr instead of net.IP in getter functions #23143

lambdanis · 2023-01-17T21:13:17Z

Hubble getters now take netip.Addr as an argument instead of net.IP. All of them handle invalid addresses, and Hubble parsers just pass addresses without checking if they're valid.

I also migrated endpointmanager.LookupIP to take netip.Addr, as it's called by GetEndpointInfo getter.

All conversions net.IP -> netip.Addr are handled by pkg/ip, with a hidden unmapping of IPv6-mapped IPv4 addresses. Conversions happen in Hubble parsers, DNS proxy (it uses LookupSecIDByIP getter), and wherever endpointmanager.LookupIP is called.

lambdanis · 2023-01-18T12:19:56Z

cc @gandro @tklauser

lambdanis · 2023-01-18T12:20:08Z

/test

gandro

Awesome, thanks a lot for fixing this!

gandro · 2023-01-18T13:50:24Z

pkg/hubble/parser/seven/parser.go

-	sourceIP := net.ParseIP(ip.Source)
-	destinationIP := net.ParseIP(ip.Destination)
+	sourceIP, _ := netip.ParseAddr(ip.Source)
+	destinationIP, _ := netip.ParseAddr(ip.Destination)


Last time I was looking at this, I was considering just rejecting flows with invalid IPs (i.e. return an error here, if ParseAddr fails). #22949 indicates that flows with invalid addresses can occur, but it's not yet clear why.

I agree and think we should check the , ok value here and return an error.

@gandro @tklauser You mean only here or in all decoders? I did that in all decoders, but then looked at failing tests and reverted the change.

Currently L7 parsers don't require log records to include SourceEndpoint or DestinationEndpoint at all. Are there possible cases when they'd be entirely empty?

For L3/4 parser it's also a behaviour change - at the moment it doesn't error if it receives e.g. an Ethernet packet, without IP layer. I don't know how intentional it is, do you think the parser should return an error in such case? If so, I'd leave this change for a separate PR.

Ah, good point regarding L3/L4. I think we can observe packets that are Ethernet-only (e.g. ARP) so we probably should not reject flows that do not have any IP address.

For L7, we don't know. Issue #22949 seems to indicate that those flows seem to be able to occur in practice, but it's not yet clear what kind of flows those are.

So I guess we probably should keep the existing behavior (and ignore ok) for now.

Agree, let's keep the existing behavior (i.e. ignore , ok) for now and follow up in a separate PR if needed.

Reading this, it seems like we should explicitly add a comment to why we're ignoring , ok, since multiple people were confused about it.

ok, I added some comments

daemon/cmd/fqdn.go

tklauser · 2023-01-18T16:06:34Z

pkg/hubble/parser/seven/parser.go

-	sourceIP := net.ParseIP(ip.Source)
-	destinationIP := net.ParseIP(ip.Destination)
+	sourceIP, _ := netip.ParseAddr(ip.Source)
+	destinationIP, _ := netip.ParseAddr(ip.Destination)


I agree and think we should check the , ok value here and return an error.

chancez

Is there a motivation for this change, I can't quite tell from the PR description or commit messages? Might be useful to provide such details in the commit message.

lambdanis · 2023-01-18T22:09:01Z

/test

lambdanis · 2023-01-18T22:59:52Z

Need to update tests, converting to draft.

gandro · 2023-01-23T10:52:30Z

Is there a motivation for this change, I can't quite tell from the PR description or commit messages? Might be useful to provide such details in the commit message.

Cilium is slowly converting it's codebase to use netip.Addr instead of net.IP, because it uses less memory and allocations.

While there are still some netip.Addr <-> net.IP conversions left in this PR, ultimately they will likely be removed once other parts of Cilium do the migration too.

#21445 has a bit more on the rationale

lambdanis · 2023-01-23T18:31:48Z

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed:

Click to show.

Test Name

K8sKafkaPolicyTest Kafka Policy Tests KafkaPolicies

Failure Output

FAIL: Unable to restart unmanaged pods with 'kubectl -n kube-system delete pods coredns-8cfc78c54-jqmzl': Exitcode: 1

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.9 so I can create one.

lambdanis · 2023-01-24T10:56:46Z

/test-1.16-4.9

lambdanis · 2023-01-30T11:42:05Z

The CI failure in ingress conformance test:

Error: waiting for service (foo-exact) endpoints available: timed out waiting for the condition

doesn't seem to be related. Can someone rerun it maybe?

christarazi · 2023-02-02T23:06:11Z

@chancez ping.

pkg/hubble/parser/threefour/parser.go

chancez

Changes LGTM. I think since a few reviewers had questions about why we're ignoring errors, we should add some comments in the areas, so future readers will know why we've chosen to ignore them.

lambdanis · 2023-02-06T14:08:56Z

/test

aanm · 2023-03-08T14:38:03Z

related to #24246

aanm · 2023-03-08T14:38:13Z

@lambdanis ping?

It's preparing to migrate Hubble getters (calling LookupIP) from net.IP to netip.Addr. Signed-off-by: Anna Kapuscinska <anna@isovalent.com>

Only renaming a variable, to avoid confusion with netip.Addr. It's preparing to migrate from net.IP to netip.Addr. Signed-off-by: Anna Kapuscinska <anna@isovalent.com>

Various parts of Cilium now use netip.Addr instead of net.IP or custom IP types internally, and other parts will likely be migrated soon. This patch updates Hubble getters to take netip.Addr as an argument instead of net.IP. That will reduce type conversions needed in Hubble. All getters handle invalid addresses. Signed-off-by: Anna Kapuscinska <anna@isovalent.com>

lambdanis · 2023-03-09T12:04:04Z

/test

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 Jan 17, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jan 17, 2023

lambdanis force-pushed the pr/hubble-netip branch 2 times, most recently from 7893839 to 97ea578 Compare January 18, 2023 11:27

lambdanis marked this pull request as ready for review January 18, 2023 12:16

lambdanis requested review from a team as code owners January 18, 2023 12:16

lambdanis requested review from jrajahalme, chancez and thorn3r January 18, 2023 12:16

gandro approved these changes Jan 18, 2023

View reviewed changes

tklauser added the release-note/misc This PR makes changes that have no direct user impact. label Jan 18, 2023

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 Jan 18, 2023

tklauser removed the kind/community-contribution This was a contribution made by a community member. label Jan 18, 2023

tklauser approved these changes Jan 18, 2023

View reviewed changes

chancez reviewed Jan 18, 2023

View reviewed changes

lambdanis force-pushed the pr/hubble-netip branch from 97ea578 to ea01003 Compare January 18, 2023 22:06

lambdanis requested review from chancez and removed request for jrajahalme and thorn3r January 18, 2023 22:07

lambdanis marked this pull request as draft January 18, 2023 22:59

lambdanis force-pushed the pr/hubble-netip branch from ea01003 to 77c03a4 Compare January 23, 2023 13:38

lambdanis marked this pull request as ready for review January 23, 2023 13:56

christarazi added kind/enhancement This would improve or streamline existing functionality. area/daemon Impacts operation of the Cilium daemon. sig/hubble Impacts hubble server or relay labels Feb 2, 2023

chancez reviewed Feb 2, 2023

View reviewed changes

pkg/hubble/parser/threefour/parser.go Show resolved Hide resolved

chancez approved these changes Feb 2, 2023

View reviewed changes

christarazi approved these changes Feb 3, 2023

View reviewed changes

lambdanis force-pushed the pr/hubble-netip branch from 77c03a4 to f27bc13 Compare February 6, 2023 14:07

lambdanis force-pushed the pr/hubble-netip branch from f27bc13 to 7395a75 Compare February 6, 2023 14:20

lambdanis added 3 commits March 9, 2023 11:18

endpointmanager: Use netip.Addr instead of net.IP in LookupIP

b5adf18

It's preparing to migrate Hubble getters (calling LookupIP) from net.IP to netip.Addr. Signed-off-by: Anna Kapuscinska <anna@isovalent.com>

dnsproxy: Rename targetServerAddr -> targetServerAddrStr

9422f28

Only renaming a variable, to avoid confusion with netip.Addr. It's preparing to migrate from net.IP to netip.Addr. Signed-off-by: Anna Kapuscinska <anna@isovalent.com>

lambdanis force-pushed the pr/hubble-netip branch from 7395a75 to e56cfc2 Compare March 9, 2023 11:19

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 Mar 9, 2023

christarazi merged commit 75c9ce9 into cilium:master Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hubble: Use netip.Addr instead of net.IP in getter functions #23143

hubble: Use netip.Addr instead of net.IP in getter functions #23143

lambdanis commented Jan 17, 2023 •

edited

lambdanis commented Jan 18, 2023

lambdanis commented Jan 18, 2023

gandro left a comment

gandro Jan 18, 2023

tklauser Jan 18, 2023

lambdanis Jan 23, 2023 •

edited

gandro Jan 23, 2023

tklauser Jan 24, 2023

chancez Feb 2, 2023

lambdanis Feb 6, 2023

tklauser Jan 18, 2023

chancez left a comment

lambdanis commented Jan 18, 2023

lambdanis commented Jan 18, 2023

gandro commented Jan 23, 2023

lambdanis commented Jan 23, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

lambdanis commented Jan 24, 2023

lambdanis commented Jan 30, 2023

christarazi commented Feb 2, 2023

chancez left a comment

lambdanis commented Feb 6, 2023

aanm commented Mar 8, 2023

aanm commented Mar 8, 2023

lambdanis commented Mar 9, 2023

hubble: Use netip.Addr instead of net.IP in getter functions #23143

hubble: Use netip.Addr instead of net.IP in getter functions #23143

Conversation

lambdanis commented Jan 17, 2023 • edited

lambdanis commented Jan 18, 2023

lambdanis commented Jan 18, 2023

gandro left a comment

Choose a reason for hiding this comment

gandro Jan 18, 2023

Choose a reason for hiding this comment

tklauser Jan 18, 2023

Choose a reason for hiding this comment

lambdanis Jan 23, 2023 • edited

Choose a reason for hiding this comment

gandro Jan 23, 2023

Choose a reason for hiding this comment

tklauser Jan 24, 2023

Choose a reason for hiding this comment

chancez Feb 2, 2023

Choose a reason for hiding this comment

lambdanis Feb 6, 2023

Choose a reason for hiding this comment

tklauser Jan 18, 2023

Choose a reason for hiding this comment

chancez left a comment

Choose a reason for hiding this comment

lambdanis commented Jan 18, 2023

lambdanis commented Jan 18, 2023

gandro commented Jan 23, 2023

lambdanis commented Jan 23, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

lambdanis commented Jan 24, 2023

lambdanis commented Jan 30, 2023

christarazi commented Feb 2, 2023

chancez left a comment

Choose a reason for hiding this comment

lambdanis commented Feb 6, 2023

aanm commented Mar 8, 2023

aanm commented Mar 8, 2023

lambdanis commented Mar 9, 2023

lambdanis commented Jan 17, 2023 •

edited

lambdanis Jan 23, 2023 •

edited

lambdanis commented Jan 23, 2023 •

edited by maintainer-s-little-helper bot