tracing tune up #880

daniel-noland · 2025-09-29T23:10:10Z

~~On top of #879~~

^^ rebased

mvachhar

The commits that are in addition to #879 are approved, but I'll withhold final approval so that we don't accidentally merge this before #879 is ready.

Fredi-raspall

I like some of the changes in this PR (even though I'm unsure about the urgency of them compared to other stuff). The only objection I have has to do with changing the log-levels in the traces. Happy to discuss in any case.

Fredi-raspall · 2025-09-30T12:10:27Z

dataplane/src/packet_processor/ipforward.rs

        /* get destination ip address */
        let Some(dst) = packet.ip_destination() else {
-            error!("{nfi}: Failed to get destination ip address for packet");
+            debug!("{nfi}: Failed to get destination ip address for packet");


I see the intent here (reducing verbosity), but to me the changes in this commit will harm more than anything.
If something is an ERROR or a warning, we should log it as such, because when displayed the severity/loglevel tells you what it is. We now have the option to completely disable traces in the pipeline. We may want to adjust the levels in the source code if some errors get very frequently logged; and we still have the option to rate-limit them for instance. However, in this particular example, I'd say you'd want to see the log as an ERROR in neon-lights since failing to get the destination ip address should be the symptom of something going really wrong.

If this can be triggered by a carefully crafted user packet, then this should not be error level as then malicious senders can cause logs to fill up and DOS the gateway.

if the user can drive us into any situation where ip address is missing then this message is a dos vector

Well, those packets (a packet with ethertype=IP and no IP header) should not even make it here, right?

sure, but error! is intended for serious application level errors. This is just what is happening if we happen to get invalid data.

Or are you concerned that we might manipulate the data into an invalid shape by accident (which, I suppose, would promote this to error! in truth)

As discussed, I believe that to address your concerns (DoS) we should clearly delimit where it is safe to log "issues" and where not. In my mind, if I have an Ip packet and can't retrieve the ip address:

either there is a bug in the getter (unlikely), and that should be error

or the packet is malformed, in which case it should not be a valid packet and make it here (this is the IP-forward stage).

I agree that the parser may not know up to which "layer" it should validate a packet. But we should decide at which point packets can be considered malformed/well-formed so that we can "reasonably" tag logs with the right severity and at the same time safely be protected against DoS.

fair enough. It is left at error for now

dataplane/src/packet_processor/ingress.rs

daniel-noland · 2025-09-30T20:37:22Z

dataplane/src/packet_processor/ipforward.rs


        /* Read-only access to the fib table */
        let Some(fibtr) = self.fibtr.enter() else {
-            error!("{nfi}: Unable to lookup fib for vrf {vrfid}");


this can likely remain an error. If the user is able to give us messages for vrfs which don't exist then we have a more fundamental problem than DoS

I think warn is fair

I'd leave this one as error. That's not the meaning of the error. It's failing to "enter" in the reader.

dataplane/src/packet_processor/ipforward.rs

Fredi-raspall

Daniel, thanks for addressing my comments. There's one change, though, that I believe is a regression. See #880 (comment)

This path is much too hot to be error tracing on malformed packets. It is a major denial of service vector prior to this commit. Signed-off-by: Daniel Noland <[email protected]>

This is just less complex. Signed-off-by: Daniel Noland <[email protected]>

Some very significant setup functions should appear in telemetry traces. Signed-off-by: Daniel Noland <[email protected]>

Signed-off-by: Daniel Noland <[email protected]>

sorting and such Signed-off-by: Daniel Noland <[email protected]>

daniel-noland · 2025-10-07T03:59:13Z

Daniel, thanks for addressing my comments. There's one change, though, that I believe is a regression. See #880 (comment)

I think this is all sorted now

qmonnet

Looks good from my side

daniel-noland force-pushed the pr/daniel-noland/tracing-tune-up branch 3 times, most recently from 31e2e8b to 17e3c72 Compare September 29, 2025 23:57

daniel-noland changed the title ~~Pr/daniel noland/tracing tune up~~ tracing tune up Sep 30, 2025

daniel-noland self-assigned this Sep 30, 2025

daniel-noland added the clean-up Code base clean-up, no functional change label Sep 30, 2025

daniel-noland marked this pull request as ready for review September 30, 2025 00:19

daniel-noland requested a review from a team as a code owner September 30, 2025 00:19

daniel-noland requested review from Fredi-raspall and removed request for a team September 30, 2025 00:19

mvachhar reviewed Sep 30, 2025

View reviewed changes

Fredi-raspall requested changes Sep 30, 2025

View reviewed changes

daniel-noland commented Sep 30, 2025

View reviewed changes

dataplane/src/packet_processor/ipforward.rs Show resolved Hide resolved

daniel-noland force-pushed the pr/daniel-noland/tracing-tune-up branch from 17e3c72 to cf60f5f Compare September 30, 2025 20:45

daniel-noland requested a review from Fredi-raspall September 30, 2025 21:17

Fredi-raspall requested changes Oct 1, 2025

View reviewed changes

daniel-noland added 5 commits October 6, 2025 23:53

fix: reduce tracing on hot path

abefe4d

This path is much too hot to be error tracing on malformed packets. It is a major denial of service vector prior to this commit. Signed-off-by: Daniel Noland <[email protected]>

fix: trivial logic cleanup

d9fb606

This is just less complex. Signed-off-by: Daniel Noland <[email protected]>

fix(dataplane): tune up instrumentation

50ed649

Some very significant setup functions should appear in telemetry traces. Signed-off-by: Daniel Noland <[email protected]>

fix(pipeline): remove unused annotations

0d73060

Signed-off-by: Daniel Noland <[email protected]>

fix(dataplane): Cargo.toml cleanup

0e07973

sorting and such Signed-off-by: Daniel Noland <[email protected]>

daniel-noland force-pushed the pr/daniel-noland/tracing-tune-up branch from cf60f5f to 0e07973 Compare October 6, 2025 23:54

daniel-noland requested a review from Fredi-raspall October 6, 2025 23:54

qmonnet approved these changes Oct 7, 2025

View reviewed changes

tracing tune up #880

Are you sure you want to change the base?

tracing tune up #880

Conversation

daniel-noland commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mvachhar left a comment

Choose a reason for hiding this comment

Uh oh!

Fredi-raspall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fredi-raspall Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fredi-raspall left a comment

Choose a reason for hiding this comment

Uh oh!

daniel-noland commented Oct 7, 2025

Uh oh!

qmonnet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daniel-noland commented Sep 29, 2025 •

edited

Loading

Fredi-raspall Oct 1, 2025 •

edited

Loading