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

API refactor process #358

Open
yuvipanda opened this issue Dec 17, 2017 · 14 comments · May be fixed by #1048
Open

API refactor process #358

yuvipanda opened this issue Dec 17, 2017 · 14 comments · May be fixed by #1048

Comments

@yuvipanda
Copy link
Collaborator

Our current API has a bunch of problems:

  1. Not versioned (see Version our API too! #304)
  2. EventSource is maybe not the best fit (see eventsource continues launching more containers if left open #317)
  3. Hard to extend for finding logs after building (see Allow copying build logs easily #75 and friends)

This issue would be to gather constraints and feature requests various audiences want so we can come up with an evolved new design for our API that enables the various use cases we want to support.

Some use cases off the top of my head:

  1. The mybinder.org home page as it is now
  2. Possible different loading page for when users just click a link from outside to a binder
  3. External web applications using our API like nteract/play or thebelab
  4. External non-web applications (like the iOS app, possible commandline applications) that use our API

/me puts operations hat on

From an operational perspective, I think the only constraint is:

  1. Do not introduce state that needs to be shared across multiple running instances of binderhub. BinderHub should continue to be as stateless as possible

Others, feel free to chime in with workflows you would like to enable, current problems with the API and constraints you want to put on the API.

/cc @rgbkrk @minrk @betatim since I've heard them talk about this before. Feel free to tag others too.

Note that we should continue to support the current API while building the new one, and make the new one the 'v2' API :)

@yuvipanda
Copy link
Collaborator Author

yuvipanda commented Dec 19, 2017

From @rgbkrk:

I would love if the initial request was a GET or POST because I can do that while server side rendering, making it so that the binder build happens in the background. I can't pass an EventSource over from server side rendering but if I have a resource by URL / id, even if ephemeral, I can connect again on the frontend. It's too bad we don't have the path for the user in advance because the logs could get streamed from there instead. If we know https://hub.mybinder.org/user/nteract-vdom-qnka42p5 in advance that would stream logs before it provides the jupyterhub.

@minrk
Copy link
Member

minrk commented Dec 20, 2017

I agree that launching with an explicit POST is best, with the eventstream only retrieving logs, never triggering a new build. This would solve several of the unused container issues we have had. The only wrinkle I see is how to handle that resource id without violating the shared state question. In memory would obviously be the easiest solution, but if we are sharding Binders that will be an impediment. Maybe we can encode all of that info in the evenstream URL:

  1. POST v2/build/gh/... initiates build and:
    1. generates build slug
    2. generates username
    3. responds with URL containing build info and username
  2. GET v2/events/build-slug/username
    1. hooks up build logs
    2. after build, creates and launches user

but now I've introduced the same problem, because to fix our eventstream problem, the eventstream URLs must be consumable. Maybe that info could be in the username? I'm not sure how to do that reliably without races, though.

We handle this pretty well right now with builds, just not launches. This is thanks to builds being run in deterministic pod IDs, to which we hook up logs. I guess if the launch itself were in a similarly specified pod, it would also work?

It's too bad we don't have the path for the user in advance because the logs could get streamed from there instead

This sounds really attractive! Binder is responsible for picking the username, so we can know this at any point we want. The harder part would be registering the handler for the URL. This is where building on top of JupyterHub gets in the way a bit, because doing this would mean Binder handling some Hub URLs. This could mean talking to the Hub proxy directly, or it could mean routing based on path in the ingress. Either way, it's potentially fiddly.

@yuvipanda
Copy link
Collaborator Author

yuvipanda commented Dec 20, 2017

So the state that needs to be persisted is:

  1. Repo info (provider + spec)
  2. Username

I've been thinking we could use encrypted JWT for this.

  1. POST /v2/build/<provider>/<spec> initiates creates a jupyterhub user, build if needed and responds with an encrypted Build-JWT that has:
    i. build-slug
    ii. hub username
  2. EventStream connection on /v2/events/<Build-JWT>
    i. Log stream if build happened (in the future we can stream back saved logs too if we want)
    ii. Ends with a different Launch-JWT that has hub username & the image name
  3. POST /v2/launch/<Launch-JWT>
    i. Launches server for user on the hub

This has the following nice properties:

  1. The tokens are opaque and extensible, which I quite like. We can put more or less fields in it without breaking clients
  2. All actions that cause changes are done via POST. EventStream doesn't trigger any launches
  3. There's an easy extension point here, where we can stream logs from Launch-JWT for logs about the launch process and possibly notebook logs too.
  4. It is totally stateless everywhere, except in the one place we already keep state - the hub! The JWT stuff allows us to also shard across multiple hubs in the future if we need to.

@yuvipanda
Copy link
Collaborator Author

I should note that @willingc @betatim and I discussed this yesterday and figured the first steps are:

  1. Document the current API (Add api doc #366)
  2. Make sure we cancel our launch or build when the eventstream is disconnected.

This is probably easy quick first fixes.

@yuvipanda
Copy link
Collaborator Author

I realized we don't have to encrypt our JWTs, just using simple signed ones is good enough for all the things we need to do.

@choldgraf
Copy link
Member

So the state that needs to be persisted is:

  • Repo info (provider + spec)
  • Username

Would this include parameters like urlpath etc?

JWT

in general can you try remembering to spell out acronyms for us less technically-savvy? :-) I assume you're talking about https://jwt.io/ yeah?

Documenting the current API I think would be a good start either way so 👍 from me on that point

@yuvipanda
Copy link
Collaborator Author

Whoops, I'll try to remember to link acronyms first time I use them.

We merged documentation for the API at #366 now.

@choldgraf
Copy link
Member

choldgraf commented Dec 21, 2017 via email

@betatim
Copy link
Member

betatim commented Dec 22, 2017

Some what out of context comment: why do we need to store the username? The logs only depend on the repository so we can share them with all users.

@minrk
Copy link
Member

minrk commented Jan 4, 2018

why do we need to store the username?

With the current API, we need this because it's not a build request, it's a build & launch request on a single event stream. We consolidate the build logs, which are per-repo, but the eventstream is specific to each launch request, which is per-user.

Since @yuvipanda's proposal explicitly splits build from launch, I don't think we do anymore. We don't need to create the user until build has completed and launch is requested (step 3), unless I'm missing something.

@minrk
Copy link
Member

minrk commented Jan 5, 2018

Adding a second API version is probably also going to reveal that we want less logic in the Handlers and more in objects, because incrementing the version means leaving the old handlers in place and creating new ones. Any logic shared between the v1 and v2 build handlers will likely be duplicated unless it resides in another object. This points to keeping Handler code small going forward.

@minrk
Copy link
Member

minrk commented Jan 5, 2018

One question about jwt: Is there a reason for signing at all? What's the benefit of jwt over, say, b64-encoded JSON? From the API perspective, it's an opaque blob, so encoding doesn't matter and can change (we could happily switch from b64 to jwt without an API change), I was just curious about choosing an additional technology vs simple existing standards.

@yuvipanda
Copy link
Collaborator Author

@minrk the reason for JWT is mostly so we can trust the tokens coming in, so users aren't gaining access to other people's running servers.

@choldgraf
Copy link
Member

Has there been any movement on this? We just got burned by what sounds like an open eventsource on the nbinteract page

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 a pull request may close this issue.

4 participants