Unfortunately I missed your graphQL presentation at the summit, but I do think that warrants some serious consideration as well. I don't know if we'd be able to do _only_ graphQL for the TO API at this point, but we should consider running an experimental graphQL server side-by-side to vet it. Once vetted, maybe it could replace TO as we know it today.
- Rawlin On Thu, Feb 14, 2019 at 9:58 AM Gray, Jonathan <[email protected]> wrote: > > I'm still digesting this for a better response. If you're considering what > both the big next thing for the API is and at the same time are considering > linking it to the ATC release, I'll just put this back on the radar > https://github.com/apache/trafficcontrol/issues/2630. There was good > discussion after the presentation I gave on it at the last TrafficControl > Summit as a potential augment/replacement someday. Now that the TO Golang > plugin system is in place, it wouldn't be too bad to have a plugin that > served as an authentication proxy to an instance hanging out there. I still > very much like the solution as a whole because it helps encourage better sql > data modeling practices (admittedly that we're not quite ready for today > probably). > > Jonathan G > > > On 2/14/19, 9:39 AM, "Moltzau, Matthew" <[email protected]> wrote: > > > The "CRUDER" is a great innovation, to be sure, as it saves us quite a > bit of time and prevents > duplicating code, but the problem it solves is an emergent > property of of API problem #1 above. > > With fewer endpoints we'd have much more specific handling, and the > need for and advantages of > the "CRUDER" will vanish. > > FYI: a restructure of our CRUDer interfaces currently has a PR here that > is in review: > > > https://protect2.fireeye.com/url?k=ef31a313a5498710.ef3184a7-8d8f491145442f8b&u=https://github.com/apache/trafficcontrol/pull/2886 > > It allows the user to implement a single C, R, U, or D operation for > better flexibility. > > On 2/14/19, 8:21 AM, "Fieck, Brennan" <[email protected]> wrote: > > I'd take it further, though in my opinion it shouldn't be done until > we're ready to do away with Perl entirely. > I've been working on this draft for a while now, and I sort of wanted > to wait until I made a wiki page for a proposed API v2 spec before sending > it, but what the heck. The conversation's here now. > > I don't like the way our API is versioned. Let me be clear, it > _should_ be versioned, and the > versions _should_ be semantic, but it is my opinion that the API > should be versioned alongside > Traffic Ops rather than at its own pace. I've written essentially an > article on why this should be > the case, and it follows: > > ** TIMING ** > The first issue I should address is timing - this needn't happen > immediately. For the time being, I > think a radical change would be much more harmful than continuing > along our current versioning > scheme. This change should be implemented with the advent of what we > currently refer to as "API v2". > Presumably, at this time all of the Perl code has been removed and we > are looking to re-tool the API > to be more sensible and standards-conforming. It's a time when we'll > already be making big, breaking > changes. I personally would love for this to be ATC 4.0, but that may > not be a realistic goal. > > ** REDUNDANCY ** > The easiest to see - albiet simultaneously most trivial - issue with > the current API versioning > scheme is how every request path must start with '/api/1.x/'. This is > annoying, to be frank; > especially when one considers that the current plan for Traffic Ops > is to reduce it entirely to an > API, so literally every endpoint will have '/api/' in common. We > _know_ the endpoint we are trying > to query is an API endpoint because _all_ endpoints are API > endpoints. When we reach "API v2" the > '/api/' part of request paths will have lost all value entirely. > Even with that gone we are left with '/1.' (or '/2.' as the case may > become) as a common prefix, > again annoying although not totally useless in this case. However, > the vast majority of API > endpoints see no changes between minor versions, so really '/1.x' > just becomes a static, constant > prefix where 'x' is the latest version of the API. > In any case, versioning the API alongside Traffic Ops solves this > problem because our Traffic Ops > server(s) emit HTTP headers that name the server. Once Perl is gone, > we'll be free to use the HTTP > `Server:` header to name the server e.g. `Server: Traffic Ops/3.2.1`. > At this point, we could > either implement an `OPTIONS` method request at the server's root > that would just return some > headers - including `Server:` or just let people to a `GET` (or > better yet `HEAD`) request to > `/ping` and pull the server version out of the headers. The client > then has all the information it > needs to communicate effectively with the server. The alternative to > this within our current > versioning schema is to implement an unversioned API endpoint such > that we have a hundred `/api/1.x` > endpoints and one that has some other - or possibly no - prefix, or > have a versioned API version API > endpoint, which is confusing even just to say. > > ** TRAFFIC OPS _IS_ THE API ** > As mentioned previously, the endgame of our transition from Perl to > Go is that Traffic Portal is the > only UI for ATC - and in fact that's already somewhat true. That > leaves Traffic Ops as a database > and a REST API for interacting with said database. In fact, the > database is often referred to as its > own service: "ToDb", "Traffic Ops DB" etc. That means that we have > the single most important Traffic > Control component split in two - half of its functionality is > versioned sanely with Traffic Control > while the other half is versioned separately from anything else in > the world. That's crazy, because > if you have a program that only does two things, then surely a > breaking change to one of those > things means increasing its major version? If that's the case, then > why are we not versioning the > API alongside Traffic Ops? > It may be argued (incorrectly, in my opinion) that Traffic Ops does > more than serve an API to > interact with a database. It generates configuration files and system > images, it combines data and > does heavy processing on it. But really those things shouldn't be > taken into account in a versioning > scheme except insofar as they affect the experience of some user, > administrator, or application > interfacing with Traffic Ops. If the API responses don't change in > form or content, and if the > process of setting up or maintaining the application haven't changed, > then any code changes you've > made are a patch, not a version change. Traffic Ops does big things, > but at the end of the day it > all just boils down to API inputs and outputs as far as anything and > anyone else is concerned. > > ** CONFUSION ** > We currently live in a world where I can run a script using the > Traffic Ops API that works perfectly > fine against Traffic Ops version 3.2.1, but then if I again test it > against version 3.2.1 at some > point in the future it breaks because breaking changes were made in > the API. It's easy to say, "oh, > that just means that when we make breaking API changes we should > increment the version > appropriately," but realize that this _is versioning the API > alongside Traffic Ops_. If we're not > doing that, we're saying there is unpredictability with the behavior > of our system within releases, > and if we _are_ doing that then the only difference between the API > version and the Traffic Ops > version is that the API version is confusingly behind by about 2 > major revisions. It should just be > the same for simplicity's sake. > > ** THE API "PROMISE" ** > The single most common argument I hear in favor of our current API > versioning scheme is "well we've > said an API version 1.x behaves in this way, and so we must uphold > that promise to the user base". > Not only do we routinely break that promise already, > (e.g. PR #3110 [https://github.com/apache/trafficcontrol/pull/3110]) > but I'm certainly not > suggesting that we don't uphold this promise. Yes, this does mean > that making breaking API changes > in TO would require a new major release and adding features/fields to > the API would require a new > minor release. I don't see that as a big deal, especially if > implemented at the time I'm suggesting > when we'd be re-designing the API - and it does sorely need to be > redesigned. > > * The API Needs to be Redesigned * > I'm going to go down this rabbit hole for a second, if you're already > convinced the TO API needs a > re-design then feel free to skip this section. I'm not going to touch > on any problems caused in the > API as currently implemented by the use of a standalone API version - > that's what the entire article > is for. > Currently, our API currently has three huge problems: > > 1. Rampant oversaturation of endpoints > We have a systemic issue of re-implementing behaviour in > multiple endpoints. This is due in part > to a lack of good documentation - so developers aren't aware > of the endpoints available to them > - and partly because of the "Mojolicious Mindset" that > plagues our oldest endpoints. The > "Mojolicious Mindset" refers to the use of URL path fragments > as request parameters, e.g. > '/users/{{ID}}' instead of/in addition to '/users?id={{ID}}'. > From the perspective of someone > who is just writing the back-end for these endpoints, there's > no clear advantage to one over the > other except that the former seems to more clearly reflect > the intent of requesting a specific > object whereas the latter could be seen as more of a "filter" > on a larger collection. That's not > incorrect, necessarily, but the two are totally separate > request paths, so having '/users' and > '/users/{{ID}}' means documenting two endpoints instead of > one, and it means two lines in the > route definitions, and it means two seperate handlers instead > of one (albiet a more complex > one). > Consider also that we have all of the following endpoints for > manipulating Cache Groups: > > * /api/1.x/cachegroup/{{parameter ID}}/parameter > * /api/1.x/cachegroup_fallbacks > * /api/1.x/cachegroupparameters > * /api/1.x/cachegroupparameters/{{ID}}/{{parameter ID}} > * /api/1.x/cachegroups > * /api/1.x/cachegroups/{{ID}} > * /api/1.x/cachegroups/{{ID}}/deliveryservices > * /api/1.x/cachegroups/{{ID}}/parameters > * /api/1.x/cachegroups/{{ID}}/queue_updates > * /api/1.x/cachegroups/{{ID}}/unassigned_parameters > * /api/1.x/cachegroups/{{parameter ID}}/parameter_available > * /api/1.x/cachegroups_trimmed > > These could all be collapsed neatly into one or two endpoints, > but were instead implemented > separately for whatever reason(s) > (see Issue #2934 > [https://github.com/apache/trafficcontrol/issues/2934] for details). > > 2. Improper/Non-existent standards conformity > We have requests that should be read-only that make > server-side state changes (Issue #3054 > [https://github.com/apache/trafficcontrol/issues/3054]), we > have endpoints returning success > responses on failure (Issue #3003 > [https://github.com/apache/trafficcontrol/issues/3003]) and > our "CRUDER" has failed us. `PUT` should be used to _create_ > objects (or possibly update them by > creating an alternative representation with the same > identifiying information) but is instead > used as the primary "edit this" method. `POST` is for > processing entities in a data payload, but > is instead used for object creation. `PATCH` languishes, > totally unimplemented. These problems > are systemic and stem partially from the "Mojolicious > Mindset" whereby new functionality is > introduced into the API by first considering what request > method is appropriate and then > deciding on a request path that names the operation being > done. Request methods are meant to be > the different ways in which a client interacts with a > resource on the server, and thus the > resources themselves should be considered primary. The > "CRUDER" hampers this mindset, because it > makes treating payloads and query parameters generic and > isn't receptive to injection of new > behaviour. > The "CRUDER" is a great innovation, to be sure, as it saves > us quite a bit of time and prevents > duplicating code, but the problem it solves is an emergent > property of of API problem #1 above. > With fewer endpoints we'd have much more specific handling, > and the need for and advantages of > the "CRUDER" will vanish. > > 3. Needing multiple queries to obtain a single piece of information > This issue is pretty deeply rooted, and is related to the way > our database is structured. But > that's part of the problem - an API needn't replicate the > database, and is therefore free from > some of the constraints that bind a database. > The way things are now, in order to e.g. create a server > definition I must make several > preliminary requests to determine the integral, unique, > non-deterministic identifiers of other > objects. I think we all agree that ideally these integral IDs > would have no part in the output > of the API, and many people I've spoken to would support > wiping them from the database entirely > given enough time and manpower. > > > >
