Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 HTTP spec #508
add HTTP spec #508
Changes from 14 commits
bc1aa59
1f075f6
12f86b8
146c09a
5398f5d
b6c1bc2
8a57943
d506145
946f516
3681472
dd5d07c
46d1857
ebe612c
7e5a077
db2b3b5
6319458
c7c9c43
454e25c
a25267b
3014b22
f96359b
1e87960
d0f0d93
8fbd64a
71415b0
4a03bb0
877899d
dc71f2c
d8850aa
78e8ca1
d30efda
8628b5a
3c0ac40
75bc635
f95e4db
e3eb9dc
95ffe6d
8f44d00
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if there is precedent for this in libp2p specs, but I thought the visual diagram you had created for earlier HTTP discussions was useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hesitant to include that diagram since it's a png and hard to update. I've tried replicating with Mermaid so that it's text based. wdyt? I don't think it's a nice and maybe a bit more confusing than the text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've attached the image I was referring to so others are aware.
It seems like that diagram was particularly useful when it came to talking about how "vanilla/simple request response" maps. That's not in scope for this spec. I'm still not opposed to having the diagram you have, although I do think it would be useful to color code the lines like you did in the diagram below to cover the "HTTP over libp2p" and "Just HTTP" cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a bit confusing to me. Above you are saying the you generally refer to HTTP semantics and the next sentence says that a goal is to interoperate with existing HTTP servers and clients which refers to the transport, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refers to both actually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 so.. this exists:
but #550 does not mention
/https
notation at all.@MarcoPolo Should we explicitly state in that spec that
/https
is an alias for/tls/http
+ do the below here?Or was this discussed already elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/https
is an alias for/tls/http
: multiformats/multiaddr#109 and multiformats/multicodec#145https://github.com/multiformats/multiaddr/blob/master/protocols.csv?plain=1#L31
I'm fine to add a note about
/https
somewhere thoughThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not the biggest fan of the term "services" here. I think I would prefer "protocols", "applications", or maybe something else. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe "endpoints"? (since that is what we define here, HTTP API endpoints for specific service/protocol)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The endpoints are the values though, really what we define is metadata for all supported protocols. We might extend this metadata with additional information later (see @BigLep's comment below). My vote goes down for
protocols
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So today we have a pair of information: <libp2p protocol name, URL path>
I agree maps make it easy for encoding a collection of pairs.
Do we ever expect to need more information (tuple)?
Maybe encode like:
?
This makes it self documenting and allows expansion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that protocol IDs have to be unique anyway, how about a map of string to object?
Perhaps we can define that implementations must accept both representations and the "string:string" representation is a short-form of the "string:object" notation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more thoughts on this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit hesitant to add the ability to put more metadata without a use case behind it, but making it a map with path seems easy enough, and would allow adding new fields in a backwards compatible way, which is nice.
This is up to the application protocol. The application protocol defines how it works and what HTTP methods it uses for what. This metadata only describes where the application protocol is mounted at. It doesn't describe the application protocol.
Yes. That's up to the application protocol. Kademlia could make use of the simple request-response abstraction in #561 to define this, but ultimately the application protocol decides tis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this to be a map from protocol name to metadata that includes a path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for explaining. Can we make this clearer? Something like:
This mapping only defines a URL namespace for certain applications protocols. It is entirely up to the application protocol (like kademlia) to define how it can be interacted with over HTTP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from @marten-seemann #556 (comment)
I believe most HTTP path routing libraries do this already. This is RECOMMENDED because it would be confusing to a client if
/kademlia
(in this example) was routed one way with one server but a different way for another server with the same "services" map.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcoPolo perhaps we could rephrase this to be more actionable for implementers.
How about:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me, but I would change this to a "SHOULD" rather than a "MUST". Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about
Authentication-Info
!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How and where is this token included in the response? In the noise extensions? In an HTTP header?
This is essentially a cookie, right? Would it make sense to actually use the
Set-Cookie
header?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cookie headers have a bit of baggage with them. For example they are a Forbidden header name, and cannot be used with a service worker proxy.
The token would be included in the Noise extensions along with the SNI used. That wasn't clear, thanks for pointing that out!