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 ILP Over HTTP #504

Merged
merged 1 commit into from Mar 5, 2019
Merged

Add ILP Over HTTP #504

merged 1 commit into from Mar 5, 2019

Conversation

@emschwartz
Copy link
Member

emschwartz commented Jan 24, 2019

Here is a minimal spec for the protocol proposed in this blog post.

### Request

```http
POST /ilp HTTP/2.0

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jan 24, 2019

Author Member

Is there a canonical way of writing out HTTP2 requests? It's a binary format so maybe not...

This comment has been minimized.

Copy link
@sharafian

sharafian Jan 24, 2019

Contributor

I think that we should specify this protocol without saying a specific HTTP version. Even if HTTP2 is used on the outgoing side, it's likely that the HTTP2 connection will be made to a load balancer and then a regular HTTP connection will be made to the application that actually implements this protocol. A client might also use a proxy like envoy (https://www.envoyproxy.io/) to upgrade outgoing requests to HTTP2 while using an ordinary HTTP client themselves.

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jan 24, 2019

Author Member

I'll change the example, but I think it's important to mention what versions implementations SHOULD support or default to. If you're using this internally, all bets are off, but the purpose of the spec is to make sure that messages that cross administrative boundaries work.

This comment has been minimized.

Copy link
@adrianhopebailie

adrianhopebailie Jan 24, 2019

Member

As discussed on the community group call, using HTTP pre-2 introduces HOL blocking that makes this likely to be significantly worse than BTP.

I don't think using this protocol is worth it unless you use HTTP2.

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jan 24, 2019

Author Member

Quite possibly, and I think in most cases the libraries people would use will support HTTP2. Do you think the language around its use should be stronger?

This comment has been minimized.

Copy link
@sharafian

sharafian Jan 25, 2019

Contributor

As discussed on the community group call, using HTTP pre-2 introduces HOL blocking that makes this likely to be significantly worse than BTP.

HTTP with keepalive has head of line blocking, but you can just put each request on a different connection. This adds round trips, especially if TLS is used, which is why I would only imagine it inside of a datacenter/cluster or even between processes on a single machine. But I don't want to deal with the headache of certs and HTTP2 support everywhere inside of our cluster when we could just have the load balancer deal with HTTP2 and do HTTP/1.1 inside.

This comment has been minimized.

Copy link
@adrianhopebailie

adrianhopebailie Jan 29, 2019

Member

put each request on a different connection

Connection establishment is the biggest culprit in most network perf issues. The TCP connection establishment handshake is one of the main drivers behind QUIC.

I don't want to deal with the headache of certs and HTTP2 support everywhere inside of our cluster when we could just have the load balancer deal with HTTP2 and do HTTP/1.1 inside.

What you do internally is up to you. It definitely shouldn't drive what's in the standard

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jan 29, 2019

Author Member

The current draft says:

HTTP/2 is HIGHLY RECOMMENDED for performance reasons, although HTTP/1.1 MAY also be used. Implementations SHOULD support HTTP version negotiation via Application Protocol Negotiation (ALPN).

@adrianhopebailie Is that okay with you?

0000-ilp-over-http.md Outdated Show resolved Hide resolved
Copy link
Contributor

sharafian left a comment

LGTM other than some naming/phrasing things

0000-ilp-over-http.md Outdated Show resolved Hide resolved
0000-ilp-over-http.md Outdated Show resolved Hide resolved
### Request

```http
POST /ilp HTTP/2.0

This comment has been minimized.

Copy link
@sharafian

sharafian Jan 24, 2019

Contributor

I think that we should specify this protocol without saying a specific HTTP version. Even if HTTP2 is used on the outgoing side, it's likely that the HTTP2 connection will be made to a load balancer and then a regular HTTP connection will be made to the application that actually implements this protocol. A client might also use a proxy like envoy (https://www.envoyproxy.io/) to upgrade outgoing requests to HTTP2 while using an ordinary HTTP client themselves.

0000-ilp-over-http.md Outdated Show resolved Hide resolved
0000-ilp-over-http.md Outdated Show resolved Hide resolved
0000-ilp-over-http.md Outdated Show resolved Hide resolved
0000-ilp-over-http.md Outdated Show resolved Hide resolved
0000-ilp-over-http.md Outdated Show resolved Hide resolved
Copy link
Member

adrianhopebailie left a comment

Would like to have some more data or evidence backed reasoning as to why we're using a less efficient serialization than possible but in principal think this is a good idea.

0000-ilp-over-http.md Outdated Show resolved Hide resolved

### Authentication

Peers MAY use any standard HTTP authentication mechanism to authenticate incoming requests. Bearer Tokens or TLS Client Certificates are RECOMMENDED.

This comment has been minimized.

Copy link
@adrianhopebailie

adrianhopebailie Jan 24, 2019

Member

Validating a bearer token on each request seems like a bad idea under scale. It also adds extra headers.

If the client is authenticated at the start of a session and the session can be bound to the connection then you're saving a lot of unnecessary bytes on the wire.

TL;DR: I'd simply recommend auth be done at the transport layer for efficiency and leave it at that.

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jan 24, 2019

Author Member

With HTTP2 header compression, it wouldn't actually send the full string over the wire each time.

Would using TLS client certs do what you're suggesting? If so, that sounds good to me. However, as far as I understand, client certs can be somewhat inconvenient to setup so I'd prefer to give options and let people determine what makes sense for their setup.

This comment has been minimized.

Copy link
@adrianhopebailie

adrianhopebailie Jan 26, 2019

Member

Client certs are definitely harder to setup but they'd be more efficient.

My point here is to avoid suggesting bearer tokens or using them in the example because the result will be that it becomes the de-facto standard for this protocol.

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jan 26, 2019

Author Member

the result will be that it becomes the de-facto standard for this protocol

I don't think that's true. As far as I know, most HTTP libraries support both methods and peers will use whatever setup makes sense for them.

This comment has been minimized.

Copy link
@adrianhopebailie

adrianhopebailie Jan 29, 2019

Member

Peers could have agreed to auth BTP using proper WebSocket handshakes but they all settled on credentials in the URL translated into an auth subprotocol because that's what was in the examples...

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jan 29, 2019

Author Member

Actually the BTP spec is pretty specific about how it expects peers to authenticate one another, and it doesn't make it sound like there are options:

Before anything else, when a client connects to a server, it sends a special Message request. Its primary protocolData entry MUST have name 'auth', content type MIME_APPLICATION_OCTET_STREAM, and empty data, and among the secondary entries, there MUST be a UTF-8 'auth_token' entry. The further secondary protocol data entries of this Message request MAY also be used to send additional information to the server. In situations where no authentication is needed, the 'auth_token' data can be set to the empty string, but it cannot be omitted.

If the client sends any BTP call that is not a Message, or sends a Message call whose primary sub-protocol is not auth, the server should respond with an Error and close the connection. [emphasis added]

This comment has been minimized.

Copy link
@adrianhopebailie

adrianhopebailie Jan 31, 2019

Member

True, my point was more to do with the "credentials in the URL" as a defacto-standard for configuration, especially of a server where a user DB is more common.

I don't think the available support in HTTP libraries is relevant. I'm more concerned with user behaviour. The way a standard is expressed in examples generally drives how users will implement it.

Not a big deal.

0000-ilp-over-http.md Outdated Show resolved Hide resolved
```http
POST /ilp HTTP/x.x
Accept: application/ilp+octet-stream
Content-Type: application/ilp+octet-stream

This comment has been minimized.

Copy link
@adrianhopebailie

adrianhopebailie Jan 24, 2019

Member

I don't think custom content types are necessary unless you plan on doing some kind of protocol negotiation or having other protocols served from the same endpoint.

This is just adding additional implementation burden on top of standard libraries IMO.

On the other hand, if you were sending the packet headers as HTTP headers you could use the types application/ilp-prepare+octet-stream, application/ilp-fulfill+octet-stream, and application/ilp-reject+octet-stream to indicate the type (possibly useful in the load balancer processing too to have this surfaced in the headers).

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jan 24, 2019

Author Member

I don't feel strongly about this. @sharafian want to weigh in?

This comment has been minimized.

Copy link
@sharafian

sharafian Jan 25, 2019

Contributor

I don't think we should be putting the internals of the packet into the headers. I guess custom content types may not be used but I can imagine some cases where you might want to use them (i.e. a middleware that catches all ILP packets but doesn't require a special pathname, just like we do for SPSP).

This comment has been minimized.

Copy link
@sappenin

sappenin Jan 26, 2019

Contributor

...unless you plan on doing some kind of protocol negotiation or having other protocols served from the same endpoint.

I prefer using custom content-types to make room for ideas like the quote cited above...for example, I plan to build both "packet serialization" variants of this proposal (HTTP headers vs OER Packet in the body) to better understand performance, and having a narrower content-type would be helpful here to allow room for other types (e.g., application/ilp-header+octet-stream).

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jan 28, 2019

Author Member

I'm eager to resolve this discussion because ultimately it doesn't make a big difference whether we use a custom content type or not and we could bikeshed this topic indefinitely.

If we want to use a custom type in the future for a different sort of protocol (maybe along the lines of the ILP packet headers in the HTTP headers), that would be enough to differentiate the OER-serialized version from the later version.

Does anyone feel very strongly that it should be application/ilp+octet-stream?

This comment has been minimized.

Copy link
@sappenin

sappenin Jan 28, 2019

Contributor

FWIW, I don't feel very strongly. My implementation supports all three content-types.

;)

This comment has been minimized.

Copy link
@adrianhopebailie

adrianhopebailie Jan 29, 2019

Member

@sappenin that's a good reason to use custom content types and actually means we could allow both serializations (OER in body AND packet headers in HTTP headers) because the Accept and Content-Type headers will tell the peer what is supported.

Can I suggest the following types:

  • application/ilp-prepare+octet-stream

  • application/ilp-fulfill+octet-stream

  • application/ilp-reject+octet-stream

  • application/ilp+octet-stream (where packet is OER encoded in body, therefor type is determined from type indicator byte in packet)

Given that most HTTP modules already have easy ways to bind application logic content types tit seems like supporting both serialisations would be pretty trivial. @sappenin, is that your experience?

This comment has been minimized.

Copy link
@emschwartz

emschwartz Jan 29, 2019

Author Member

I've thought about supporting both on the incoming side. The issue is what you send out and what you expect your peers to support as well.

I'd prefer to have fewer options, because that means less that implementers need to support in practice (I generally think that wherever there are options, implementers will probably need to implement all of the options rather than picking a set and risking incompatibilities with others)

This comment has been minimized.

Copy link
@sappenin

sappenin Jan 30, 2019

Contributor

@adrianhopebailie I agree with your type suggestions in principle, and also like the point by @emschwartz about keeping things simple.

To that end, let's discuss the details of the header-variant of this protocol in a different PR (I don't see anything in the current PR that would block any of that).

0000-ilp-over-http.md Outdated Show resolved Hide resolved
@sentientwaffle sentientwaffle mentioned this pull request Jan 24, 2019
0000-ilp-over-http.md Outdated Show resolved Hide resolved
0000-ilp-over-http.md Outdated Show resolved Hide resolved
0000-ilp-over-http.md Outdated Show resolved Hide resolved
sappenin added a commit to interledger4j/ilpv4-connector that referenced this pull request Mar 3, 2019
* Updates for ILPv4
* Implement [ILP-over-Http](interledger/rfcs#504)
* Support Shared-secret JWT (for now)
* Finish Ping protocol
* Update to strongly-typed ILP Address
* Extract InterledgerAddressPrefix to Quilt.
* Remove double-shutdown hook
* Ccp Routing Implementation
* Misc updates
* Cleanup RoutingTable
* Fix startup config.
* Replace Plugin with Link
* Enable server-based ITs
* Add outgoing balance tracking
* Update docs
* Fix dependencies

Signed-off-by: sappenin <sappenin@gmail.com>
@emschwartz emschwartz force-pushed the es-ilp-over-http branch from 26b7426 to f3c243d Mar 5, 2019
@emschwartz emschwartz merged commit ed1712b into master Mar 5, 2019
1 check passed
1 check passed
ci/circleci: test Your tests passed on CircleCI!
Details
@emschwartz emschwartz deleted the es-ilp-over-http branch Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.