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

Add path selector context and explicit path query #260

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

lschulz
Copy link

@lschulz lschulz commented Nov 24, 2024

Adds a context pointer to the path selector's Path() method that allows applications to pass arbitrary context data to their custom path selector. Currently Path() has to select a path without knowing anything about the packet it picks a path for. The context can provide additional information, e.g., whether the packet is urgent or not.

Adds an exported function for querying paths to a particular destination AS. This function was missing before, making it very hard to send a packet to a new destination on a ListenConn.

Adds a getter for the size of a path in the dataplane. The path size can be used with the MTU metadata to approximate the maximum payload size per packet.

Other changes:

  • Return an error if SCION host context initialization fails (e.g., because one forgot to set SCION_DAEMON_ADDRESS). Previously PAN terminated the application in this case even though a connection error is very likely recoverable. Returning an error to the application is especially important for GUI apps that do not have a terminal PAN could print anything to.

This change is Reviewable

Return an error from SCION host context singleton initialization instead
of terminating the application outright.
Extend path selectors with a context passed down from calls to
Write-type functions on PAN UDP connections.
@JordiSubira JordiSubira self-requested a review November 25, 2024 08:35
Copy link
Contributor

@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 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained


pkg/pan/udp_listen.go line 57 at r1 (raw file):

	ReadFromVia(b []byte) (int, UDPAddr, *Path, error)
	// WriteToWithCtx writes a message to the remote address using a path from
	// the path selector. ctx is passed to the path selector.

Similar comment to the counterpart function in udp_dial.go


pkg/pan/udp_listen.go line 180 at r1 (raw file):

}

func (s *DefaultReplySelector) Path(ctx interface{}, remote UDPAddr) *Path {

_ (unused parameter)

Code quote:

ctx

pkg/pan/sciond.go line 52 at r1 (raw file):

func (e HostContextError) Error() string {
	return fmt.Sprintf("error initializing SCION host context: '%s'", e.Cause)

%v

Code quote:

%s

pkg/pan/path.go line 47 at r1 (raw file):

}

// DataplaneLen returns the legnth of the path in the data plane.

length

Code quote:

legnth

pkg/pan/path.go line 48 at r1 (raw file):

// DataplaneLen returns the legnth of the path in the data plane.
func (p *Path) DataplaneLen() (int, error) {

Currently this function isn't used anywhere, could we add some small UT?


ssh/client/ssh/selector.go line 58 at r1 (raw file):

}

func (s *roundRobinSelector) Path(cts interface{}) *pan.Path {

_ (unused parameter)

Code quote:

cts

ssh/client/ssh/selector.go line 97 at r1 (raw file):

}

func (s *randomSelector) Path(ctx interface{}) *pan.Path {

_ (unused parameter)

Code quote:

ctx

pkg/pan/selector.go line 34 at r1 (raw file):

	// Path selects the path for the next packet.
	// Invoked for each packet sent with Write.
	Path(ctx interface{}) *Path

Is there a reason for not making this "context.Context"

Code quote:

interface{}

pkg/pan/selector.go line 67 at r1 (raw file):

}

func (s *DefaultSelector) Path(ctx interface{}) *Path {

_ (unused parameter)

Code quote:

ctx

pkg/pan/selector.go line 146 at r1 (raw file):

}

func (s *PingingSelector) Path(ctx interface{}) *Path {

_ (unused parameter)

Code quote:

ctx

pkg/pan/udp_dial.go line 32 at r1 (raw file):

	SetPolicy(policy Policy)
	// WriteWithCtx writes a message to the remote address using a path from the
	// path policy and selector. ctx is passed to the path selector.

It may be useful to extend this comment with the brief explanation you have added to this PR,i.e.,: "he context can provide additional information, e.g., whether the packet is urgent or not.", or a similar one.

Copy link
Author

@lschulz lschulz 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: 4 of 11 files reviewed, 11 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained


pkg/pan/path.go line 47 at r1 (raw file):

Previously, JordiSubira wrote…

length

Done.


pkg/pan/path.go line 48 at r1 (raw file):

Previously, JordiSubira wrote…

Currently this function isn't used anywhere, could we add some small UT?

Done.


pkg/pan/sciond.go line 52 at r1 (raw file):

Previously, JordiSubira wrote…

%v

Done.


pkg/pan/selector.go line 34 at r1 (raw file):

Previously, JordiSubira wrote…

Is there a reason for not making this "context.Context"

I thought about making it a context.Context, but I didn't see a reason to restrict user code to a specific type or interface. That being said, I'm not against using context.Context if this is more idiomatic, or if PAN would need to interact with this context in the future.


pkg/pan/selector.go line 67 at r1 (raw file):

Previously, JordiSubira wrote…

_ (unused parameter)

Done.


pkg/pan/selector.go line 146 at r1 (raw file):

Previously, JordiSubira wrote…

_ (unused parameter)

Done.


pkg/pan/udp_dial.go line 32 at r1 (raw file):

Previously, JordiSubira wrote…

It may be useful to extend this comment with the brief explanation you have added to this PR,i.e.,: "he context can provide additional information, e.g., whether the packet is urgent or not.", or a similar one.

Done.


pkg/pan/udp_listen.go line 57 at r1 (raw file):

Previously, JordiSubira wrote…

Similar comment to the counterpart function in udp_dial.go

Done.


pkg/pan/udp_listen.go line 180 at r1 (raw file):

Previously, JordiSubira wrote…

_ (unused parameter)

Done.


ssh/client/ssh/selector.go line 58 at r1 (raw file):

Previously, JordiSubira wrote…

_ (unused parameter)

Done.


ssh/client/ssh/selector.go line 97 at r1 (raw file):

Previously, JordiSubira wrote…

_ (unused parameter)

Done.

Copy link
Contributor

@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 3 of 7 files at r2, all commit messages.
Reviewable status: 7 of 11 files reviewed, 1 unresolved discussion / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained


pkg/pan/selector.go line 34 at r1 (raw file):

Previously, lschulz (Lars-Christian Schulz) wrote…

I thought about making it a context.Context, but I didn't see a reason to restrict user code to a specific type or interface. That being said, I'm not against using context.Context if this is more idiomatic, or if PAN would need to interact with this context in the future.

I do believe it is more idiomatic and more consistent with the existing code which uses context.Context, so if there's no strong reason, I'd opt for using it here as well.

Copy link
Author

@lschulz lschulz 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: 5 of 11 files reviewed, 1 unresolved discussion / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained


pkg/pan/selector.go line 34 at r1 (raw file):

Previously, JordiSubira wrote…

I do believe it is more idiomatic and more consistent with the existing code which uses context.Context, so if there's no strong reason, I'd opt for using it here as well.

Ok, I switched to context.Context.

Copy link
Contributor

@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.

:lgtm:

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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.

2 participants