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

pkg/snet: ignore decoding/parsing reading errors on SCIONPacketConn #4670

Merged
merged 2 commits into from
Dec 23, 2024

Conversation

JordiSubira
Copy link
Contributor

So far, the snet/SCIONPacketConn was bubbling up decoding/parsing errors to the calling applications. In principle, these errors are not recoverable by the application. The current solution simulates that the network stack simply discards incorrect/malformated packets. Otherwise,some libraries/applications would misbehave (similar to what would happen if were to receive SCMP errors). If we believe that SCION-aware applications using the low-level SCIONPacketConn should receive the error, we can move this up to Conn.

@JordiSubira JordiSubira requested a review from a team as a code owner December 19, 2024 13:10
@jiceatscion
Copy link
Contributor

This change is Reviewable

Copy link
Contributor

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

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

I think this isn't too high. Someone has even argued that broken packets should die earlier than that: #4264

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JordiSubira)


pkg/snet/packet_conn.go line 218 at r1 (raw file):

	if err := pkt.Decode(); err != nil {
		metrics.CounterInc(c.Metrics.ParseErrors)
		// XXX(JordiSubira): We avoid bubbling up parsing errors to the

I don't think you need so much in-line explanations as a future reader can git blame, if curious.


pkg/snet/packet_conn.go line 240 at r1 (raw file):

		lastHop, err = c.lastHop(pkt)
		if err != nil {
			// XXX(JordiSubira): We avoid bubbling up parsing errors to the

Ditto

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @JordiSubira)


pkg/snet/packet_conn.go line 227 at r1 (raw file):

		// SCIONPacketConn should receive the error, we can move this up to
		// Conn.
		log.Error("decoding packet", "error", err)

I would log at most at debug level here.

Otherwise, there is a lot of garbage logs someone can induce.


pkg/snet/packet_conn.go line 249 at r1 (raw file):

			// SCIONPacketConn should receive the error, we can move this up to
			// Conn.
			log.Error("extracting last hop based on packet path", "error", err)

ditto

Copy link
Contributor Author

@JordiSubira JordiSubira left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jiceatscion and @oncilla)


pkg/snet/packet_conn.go line 218 at r1 (raw file):

Previously, jiceatscion wrote…

I don't think you need so much in-line explanations as a future reader can git blame, if curious.

Done.


pkg/snet/packet_conn.go line 227 at r1 (raw file):

Previously, oncilla (Dominik Roos) wrote…

I would log at most at debug level here.

Otherwise, there is a lot of garbage logs someone can induce.

Done.


pkg/snet/packet_conn.go line 240 at r1 (raw file):

Previously, jiceatscion wrote…

Ditto

Done.


pkg/snet/packet_conn.go line 249 at r1 (raw file):

Previously, oncilla (Dominik Roos) wrote…

ditto

Done.

Copy link
Contributor

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @JordiSubira)

@JordiSubira JordiSubira merged commit 0b42cbc into scionproto:master Dec 23, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants