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

Adding a way of forcing the use of specific protocols #327

Closed
wants to merge 11 commits into from

Conversation

philsbln
Copy link
Contributor

@philsbln philsbln commented May 17, 2019

This fixes issue #324.

The solution involves specifying protocol sets on the local endpoint.
Note that the text is dependent the introduction of Transport Property Profiles.

This involes specifying protocol sets on the local endpoint.
Note that the text is dependent the introduction of Transport
Property Profiles.
Copy link
Collaborator

@MaxF12 MaxF12 left a comment

I like the fact that this gets added but I think there might be a change required in arch and/or impl that notes how racing behaves and what happens to specified transport properties if one or multiple protocols are forced.

LocalSpecifier.WithProtocols("tcp", "ipv4")
LocalSpecifier.WithProtocols("tcp", "ipv6")
~~~

Copy link
Collaborator

@MaxF12 MaxF12 May 17, 2019

Choose a reason for hiding this comment

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

Why is this done on the local endpoint object rather than on the transport properties object?

Copy link
Contributor Author

@philsbln philsbln May 18, 2019

Choose a reason for hiding this comment

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

The reason is that really forcing a protocol end-to-end is hard and requires crypto. Therefore, we only force the protocol at the local endpoint. On the path, there may be proxies or protocol converters, so that the Remote Endpoint may use another protocol. Adding it on the Local Endpoint object makes this explicit.

Copy link
Contributor

@abrunstrom abrunstrom May 24, 2019

Choose a reason for hiding this comment

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

I also find using the endpoint object confusing and agree with Max that it should be a transport property. It is used as a property for the selection and racing so we should keep all this together. (I am not convinced by the argument for why we should use the endpoint object. The path could violate other transport properties as well then.)

Copy link
Contributor Author

@philsbln philsbln Jul 5, 2019

Choose a reason for hiding this comment

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

There is another argument why realising this as Transport Property is a little painful: what would the following mean:

transportPorperties.require(protocol, UDP)
transportPorperties.require(protocol, TCP)
transportPorperties.require(protocol, IPv4)
transportPorperties.require(protocol, IPv6)

Does it mean what I intended (require to use TCP or UDP and prefer IPv4), or is this only satisfied by TCPoverUDPoverIPv6overIPv4 or TCPoverUDPoverIPv4overIPv6?

NewLocalEndpoint.WithProtocols("udp","tcp")
NewLocalEndpoint.WithProtocols("IPv4","IPv6"),

Is clear - I want either TCP or UDP over IPv4 or IPv6 - and does not allow to use other preferences and thus reduces complexity.

Copy link
Contributor

@mwelzl mwelzl Jul 7, 2019

Choose a reason for hiding this comment

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

I think the example that you give here makes the endpoint case clear because you assume an implicit "or" between parameters of one call. In the other case, you arrive at something ambiguous because you have only one flat property called "protocol".

The distinction that creates the problem here, to me, isn't about the local endpoint or use of a transport property; it's about semantics of the defined call that you use (or assume).

@GrumpyOldTroll
Copy link
Contributor

@GrumpyOldTroll GrumpyOldTroll commented May 20, 2019

I like this extension, and thanks for opening the issue.

As written, it leaves me guessing about at least one assumption, which I think would be:

If the constrained protocol choice conflicts with the default TransportProperties, the TransportProperties also must be set to something compatible, or you'd get an error when calling connect or listen, correct?

In particular, I think this would always fail with error:

   Preconnection := NewPreconnection(
           NewLocalEndpoint.WithProtocols("udp"),
           NewRemoteEndpoint.WithHostname("example.com").WithPort(9000),
           NewTransportProperties(),
           NewSecurityParameters())

Fixing it would require explicitly suppressing the several default-require transport properties that conflict with udp in order to have a chance at not erroring:

   Preconnection := NewPreconnection(
           NewLocalEndpoint.WithProtocols("udp"),
           NewRemoteEndpoint.WithHostname("example.com").WithPort(9000),
           NewTransportProperties()
                .Ignore(reliability)
                .Ignore(preserve-order)
                .Ignore(congestion-control),
           NewSecurityParameters())

Is that expected? Does it need to be made explicit?

And was there an intent for this to be made simpler with protocol-based profiles for TransportProperties?

@philsbln
Copy link
Contributor Author

@philsbln philsbln commented May 21, 2019

I suspect with the addition of profiles, the solution should rather look like:

   Preconnection := NewPreconnection(
           NewLocalEndpoint.WithProtocols("udp"),
           NewRemoteEndpoint.WithHostname("example.com").WithPort(9000),
           NewTransportProperties(unreliable-datagram)
           NewSecurityParameters())

with "unreliable datagram" implying

           NewTransportProperties()
                .avoid(reliability)
                .avoid(preserve-order)
                .avoid(congestion-control)
                .add(idempotent, true)
           }

@philsbln
Copy link
Contributor Author

@philsbln philsbln commented May 21, 2019

See PR #328 for a concrete proposal for profiles

gorryfair
Copy link
Contributor

@gorryfair gorryfair commented on e3b04fe May 21, 2019

Choose a reason for hiding this comment

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

Looks better, thanks for checking my inputs. Will comment on the integrated text, unless there is more debate

@britram
Copy link
Contributor

@britram britram commented Jul 3, 2019

I'm confused: after you have #328, why do you need this?

@philsbln
Copy link
Contributor Author

@philsbln philsbln commented Jul 4, 2019

@britram I share your confusion after re-reading it… there was something mixed up.

I see the following two cases:

  1. If you need a UDP like protocol #328 solves your problem:
   Preconnection := NewPreconnection(
           NewRemoteEndpoint.WithHostname("example.com").WithPort(9000),
           NewTransportProperties(unreliable-datagram)
           NewSecurityParameters())
  1. If you need exactly UDP because you defined a behaviour based on the protocol headers -- I don't find an example where this is useful, but I have several for TCP -- you need #327:
   Preconnection := NewPreconnection(
           NewLocalEndpoint.WithProtocols("udp"),
           NewRemoteEndpoint.WithHostname("example.com").WithPort(9000),
           NewTransportProperties(unreliable-datagram)
           NewSecurityParameters())

@GrumpyOldTroll
Copy link
Contributor

@GrumpyOldTroll GrumpyOldTroll commented Jul 8, 2019

It just occurred to me to wonder: Is there any reason we shouldn't do exact protocol selection as an inheriting subclass of a transport property profile?

So maybe "exact-udp" inherits from "unreliable-datagram", and app designer just picks what they need as a transport profile?

I usually don't much care for inheritance, but are there cases where this wouldn't be as good or better?
I agree with Anna I don't much like exact protocol selection in LocalEndpoint.

@philsbln
Copy link
Contributor Author

@philsbln philsbln commented Jul 8, 2019

As suggested several times, I changed the way of forcing specific protocol stacks into an ordinary Selection Property. This comes at the cost of introducing a list type for properties.

@GrumpyOldTroll I don't think inheritance solves the problem here, as one may want to give the transport system a choice between multiple sets of protocols. For example, SIP is defined to operate over SCTP, UDP or TCP – therefore, we would need a fairly generic set of Selection Properties that allows to use any of these and the newly introduced Property to force to us no other matching protocol.

@britram
Copy link
Contributor

@britram britram commented Jul 22, 2019

Okay. I'm still not a giant fan of this, because I'm pretty sure it'll get abused. Some extra text stating that this is basically only intended for special circumstances (e.g., implementing a layer-4+-topped stack over a specific layer-4-topped stack) would address that as best we can in an abstract API, though.

@mirjak
Copy link
Contributor

@mirjak mirjak commented Jul 22, 2019

I'm also not a fan. I think it should be a bit hard to force use of one specific protocol because the whole idea of this exercise is to have an interface that is NOT protocol-specific.

@philsbln
Copy link
Contributor Author

@philsbln philsbln commented Jul 23, 2019

I thought the text in this PR is already quite clear that this property is only for wired special cases.

@britram please make a suggestion how to make the text even more hostile for people that want to use the property.

@britram
Copy link
Contributor

@britram britram commented Jul 24, 2019

Per discussion on #324, we're going to do this another way.

@britram britram closed this Jul 24, 2019
@britram britram deleted the philsbln/explicit-protocols branch Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants