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

3.0.0 #311

Merged
merged 72 commits into from
Sep 29, 2020
Merged

3.0.0 #311

merged 72 commits into from
Sep 29, 2020

Conversation

dinosaure
Copy link
Member

This is the main PR where I replaced all conduit by tuyau. Currently, the lwt-unix implementation is missing and should be provided as before. Otherwise, it seems good to prepare a plan about a possible release of 3.0.0.

@dinosaure
Copy link
Member Author

It seems that the CI is happy with my new test: a ping-pong server without and with TLS. I did not yet implement the async support. Only the lwt-unix support is available - about mirage, I don't know the way to compile/run properly a mirage unikernel.

I currently on a reverse-dependency work to smoothly move to the new interface.

@dinosaure
Copy link
Member Author

dinosaure commented Apr 22, 2020

Considering this TODO before any merge:

  • cohttp
  • datakit
  • git
  • hiredis
  • jitsu
  • ketrew
  • websocket
  • yurt

@nickbetteridge
Copy link

I would find cohttp, git and websocket highly useful :)

  • datakit has been archived

@samoht
Copy link
Member

samoht commented Apr 22, 2020

datakit has been archived

it's not anymore.

@nickbetteridge
Copy link

Ahah, then datakit would be a welcome inclusion also! :)

@dinosaure dinosaure mentioned this pull request Apr 22, 2020
3 tasks
lib/conduit.ml Outdated Show resolved Hide resolved
@dinosaure
Copy link
Member Author

TCP async support was done to be able to provide cohttp-async as usual. The design is the same than conduit-mirage.tcp where we need to unlock the internal loop provided by Async.Tcp.Server to be able to provide accept as expected.

@dinosaure
Copy link
Member Author

dinosaure commented Apr 23, 2020

We re-implement the support of SSL on conduit-async. I did not test the code but I did it as usual. However, we should add some tests about it:

  • test conduit-async and conduit-async.ssl

conduit-tls.opam Show resolved Hide resolved
lib/README.md Outdated Show resolved Hide resolved
@dinosaure
Copy link
Member Author

Support of lwt_ssl was done and it was tested. Currently, we already try TLSv1_2, it seems that in GitHub Actions (and on my computer), we don't have the support of TLSv1_3.

@dinosaure
Copy link
Member Author

For more context and about produced implementation of protocols, I stress-test conduit with httpaf to ensure any details.

@hannesm
Copy link
Member

hannesm commented Apr 27, 2020

what happened to vchan? I'm not sure whether vchan support is used by anyone (or needed - and for sure can be re-added later if desired).

@dinosaure
Copy link
Member Author

what happened to vchan? I'm not sure whether vchan support is used by anyone (or needed - and for sure can be re-added later if desired).

As a new principle of conduit, extension of protocols are not limited by the new conduit implementation. If someone wants to extend conduit with VCHAN, he can outside the scope of conduit - of course, he must implement the protocol and register it but it's not necessary to update the conduit library to provide a new protocol.

It's the same for any others protocols such as TCP, TLS or SSL even if the distribution implements them - I want to re-implement them to avoid a regression on the cohttp stack used by many but, again, such a work still is outside the core library. In others words, conduit can be used as is but we must still provide what we provided before 👍.

@dinosaure
Copy link
Member Author

I finished to support SSL and TLS on lwt and async with examples and tests.

  • windows support

@dinosaure
Copy link
Member Author

Ok, ocaml-ci is happy (including ocamlformat), I still have some troubles about tests only on ocaml-ci but I can not introspect it as a black-box. I removed all optional dependencies, so conduit-async and conduit-lwt-unix have an explosion of dependencies due to TLS and SSL layers provided.

conduit still is light (and conduit-lwt) and conduit-mirage needs conduit-tls to provide a TLS layer on top of the TCP/IP stack.

@talex5
Copy link
Contributor

talex5 commented May 5, 2020

I still have some troubles about tests only on ocaml-ci but I can not introspect it as a black-box.

What is the problem? Each platform's log page displays a shell script you can run to reproduce the build on your local machine.

@dinosaure
Copy link
Member Author

dinosaure commented May 5, 2020

What is the problem? Each platform's log page displays a shell script you can run to reproduce the build on your local machine.

The problem is a bit hard to highlight. Currently tests want to start several ping-pong servers and clients (with several protocols, TCP, TCP + SSL and TCP + TLS) and it seems that sometimes, ocaml-ci stuck on one of them (but logs can not tell me more about which one and why).

I added (locks ...) to serialize tests and avoid an EADDRINUSE but I don't really know what happens for 116a42d, 2d615e5 or 64ef587.

EDIT: a such situation seems more reproducible with mirage/ocaml-cohttp#692 where we launch tests of ocaml-cohttp and ocaml-conduit.

@dinosaure
Copy link
Member Author

cohttp is done, tests are good, CI is green, let's go to others projects.

@dinosaure dinosaure force-pushed the 3.0.0 branch 3 times, most recently from 1bde31c to 6b73b9b Compare May 5, 2020 18:50
@dinosaure
Copy link
Member Author

The PR is cleaned. The support of Windows is a bit tricky. We currently can build and install conduit-lwt-unix (conduit-async can not be installed due to core/async_ssl) but tests fails because a out of scope error (ssleay32.dll not found).

I consider the PR as done and ready for reviews. cohttp works on top of that. I will re-evaluate the list of revdeps projects because some of them are olds.

@dinosaure
Copy link
Member Author

A high usage of ocaml-conduit (with unikernels) show to us (specially @hannesm and me) that [ host ] Domain_name.t as the concrete value to resolve an endpoint is probably not the best choice when you should not require a /domain-name resolver/ at this point - not that it's not a real domain-name resolver but a simple function which should resolve a domain-name.

In the case of where we need to be connected to a peer which does not have a proper domain-name but only an IP address, it's difficult to tell conduit to try a connection to 10.0.0.1 for example. A hack is to decide arbitrary about a fake domain-name which is translated then to our IP address such as:

let resolvers =
  Conduit.empty
  |> Conduit.add TCP.protocol @@ fun domain_name -> match Domain_name.to_string domain_name with
  | "localhost" -> return (Some Ipaddr.V4.localhost)
  | "any" -> return (Some Ipaddr.V4.any)
  | _ -> return None

A solution is to replace [ host ] Domain_name.t by a type such as:

type endpoint =
  | Dns of [ `host ] Domain_name.t
  | Ip of Ipaddr.t

And provide finally:

val resolve : endpoint -> resolvers -> (flow, [> error ]) result io

It does not change too much the core but it's an other indirection about what the user need to give to us to start a connection (note that Uri.t is able to have an IP or a simple hostname). So, from my perspective, I agree with such change but I'm more disposal to delay this update after a merge of this (huge) PR. (@mirage/core?)

@dinosaure
Copy link
Member Author

So I think it's time to merge this PR, waiting the CI (which should be green). We reach the point where I can not spend much more time to maintain all PRs according this new version and @mirage/core agreed with this new version. The next real plan is to propose an other PR which will implement an endpoint (with domain-name and IP as explained here)), fallback this change into PRs (such as cohttp and ocaml-git) and cut a release!

The merge will invalidate #319 (which requires an new version of lwt_ssl), #316 & #312 (move to OCaml CI), #296 (which is done by mirage/awa-ssh#19), #139 and #133 which are pretty old (4 years ago). Issues will be close step by step with an explanation about what it should be a fix (according to this comment).

Copy link
Member

@avsm avsm left a comment

Choose a reason for hiding this comment

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

Lgtm. In terms of next steps, lets:

  • merge to master (I will do so now)
  • add the endpoint/ip functionality in a separate PR as you suggest
  • prepare a migration guide from conduit 2, which @hannesm had some ideas for as he has ported his live unikernels
  • post this on discuss.ocaml.org and add to the CHANGES file to this repo
  • merge the various cohttp changes (cc @mseri) into cohttp master
  • then cut releases into opam of conduit and cohttp at the same time

@avsm avsm merged commit 66fd34a into mirage:master Sep 29, 2020
@mseri
Copy link

mseri commented Sep 29, 2020

Awesome job!

dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Oct 15, 2020
…wt, conduit-async-tls, conduit-async, conduit, conduit-async-ssl and conduit-lwt-ssl (3.0.0)

CHANGES:

* **breaking change** - New version of `conduit`. Most of the new API has a
  description on the pull-request
  [mirage/ocaml-conduit#311](mirage/ocaml-conduit#311). Documentation is
  updated as well with a small HOW-TO (available
  [here](https://mirage.github.io/ocaml-conduit/conduit/howto.html)) which
  describes the new API. An other document exists to understand the goal
  of Conduit [here](https://mirage.github.io/ocaml-conduit/conduit/readme.html).
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Oct 16, 2020
…wt, conduit-async-tls, conduit-async, conduit, conduit-async-ssl and conduit-lwt-ssl (3.0.0)

CHANGES:

* **breaking change** - New version of `conduit`. Most of the new API has a
  description on the pull-request
  [mirage/ocaml-conduit#311](mirage/ocaml-conduit#311). Documentation is
  updated as well with a small HOW-TO (available
  [here](https://mirage.github.io/ocaml-conduit/conduit/howto.html)) which
  describes the new API. An other document exists to understand the goal
  of Conduit [here](https://mirage.github.io/ocaml-conduit/conduit/readme.html).
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Oct 16, 2020
…wt, conduit-async-tls, conduit-async, conduit, conduit-async-ssl and conduit-lwt-ssl (3.0.0)

CHANGES:

* **breaking change** - New version of `conduit`. Most of the new API has a
  description on the pull-request
  [mirage/ocaml-conduit#311](mirage/ocaml-conduit#311). Documentation is
  updated as well with a small HOW-TO (available
  [here](https://mirage.github.io/ocaml-conduit/conduit/howto.html)) which
  describes the new API. An other document exists to understand the goal
  of Conduit [here](https://mirage.github.io/ocaml-conduit/conduit/readme.html).
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Oct 16, 2020
…wt, conduit-async-tls, conduit-async, conduit, conduit-async-ssl and conduit-lwt-ssl (3.0.0)

CHANGES:

* **breaking change** - New version of `conduit`. Most of the new API has a
  description on the pull-request
  [mirage/ocaml-conduit#311](mirage/ocaml-conduit#311). Documentation is
  updated as well with a small HOW-TO (available
  [here](https://mirage.github.io/ocaml-conduit/conduit/howto.html)) which
  describes the new API. An other document exists to understand the goal
  of Conduit [here](https://mirage.github.io/ocaml-conduit/conduit/readme.html).
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Oct 16, 2020
…wt, conduit-async-tls, conduit-async, conduit, conduit-async-ssl and conduit-lwt-ssl (3.0.0)

CHANGES:

* **breaking change** - New version of `conduit`. Most of the new API has a
  description on the pull-request
  [mirage/ocaml-conduit#311](mirage/ocaml-conduit#311). Documentation is
  updated as well with a small HOW-TO (available
  [here](https://mirage.github.io/ocaml-conduit/conduit/howto.html)) which
  describes the new API.
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Oct 16, 2020
…wt, conduit-async-tls, conduit-async, conduit, conduit-async-ssl and conduit-lwt-ssl (3.0.0)

CHANGES:

* **breaking change** - New version of `conduit`. Most of the new API has a
  description on the pull-request
  [mirage/ocaml-conduit#311](mirage/ocaml-conduit#311). Documentation is
  updated as well with a small HOW-TO (available
  [here](https://mirage.github.io/ocaml-conduit/conduit/howto.html)) which
  describes the new API.
kit-ty-kate pushed a commit to dinosaure/opam-repository that referenced this pull request Oct 18, 2020
…wt, conduit-async-tls, conduit-async, conduit, conduit-async-ssl and conduit-lwt-ssl (3.0.0)

CHANGES:

* **breaking change** - New version of `conduit`. Most of the new API has a
  description on the pull-request
  [mirage/ocaml-conduit#311](mirage/ocaml-conduit#311). Documentation is
  updated as well with a small HOW-TO (available
  [here](https://mirage.github.io/ocaml-conduit/conduit/howto.html)) which
  describes the new API.
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Oct 19, 2020
…wt, conduit-async-tls, conduit-async, conduit, conduit-async-ssl and conduit-lwt-ssl (3.0.0)

CHANGES:

* **breaking change** - New version of `conduit`. Most of the new API has a
  description on the pull-request
  [mirage/ocaml-conduit#311](mirage/ocaml-conduit#311). Documentation is
  updated as well with a small HOW-TO (available
  [here](https://mirage.github.io/ocaml-conduit/conduit/howto.html)) which
  describes the new API.
@@ -48,11 +48,11 @@ val protocol_with_ssl :
(context * 'edn, 'flow with_ssl) Client.protocol

val service_with_ssl :
('cfg, 't * 'flow) Service.service ->
('cfg, 't, 'flow) Service.service ->
reader:('flow -> Reader.t) ->
writer:('flow -> Writer.t) ->
('edn, 'flow with_ssl) Client.protocol ->
Copy link
Member

Choose a reason for hiding this comment

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

is that protocol still needed here?

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.

9 participants