First of all I will admit that I did not read every email in this chain. That being said, based on the fact that A) Rawlin is doing this work, B) I would like to get this feature into 4.x, and C) I would like 4.x to be cut in the next week, I will defer to Rawlin's idea and +1 that. We don't need to let perfect be the enemy of good and if something doesn't quite work right and we come up with a better solution later, then we can implement it later. This is something that is meant to be short-lived and used for a specific purpose.
--Dave On Wed, Nov 20, 2019 at 4:55 PM Rawlin Peters <[email protected]> wrote: > I already have a regex-based implementation working; that is why I am > proposing the ID-based implementation. I saw the light. I looked at > what I had done and thought better. I really think this is a much > safer and easier way to accomplish the same goal. > > While I'm totally on board with making the API routes not regexes, I'm > not going to rewrite all of that existing API route regex > functionality just to be able to use non-regex paths in cdn.conf, for > a feature that is meant to be rarely used in the first place and > already provides the ID -> route mapping in an easy and readily > available manner. We should definitely do that whenever we get around > to API 2.0 (whether it's a _true_ API 2.0 or just a thinly veiled > guise of 1.x without some routes). > > This is feature creep and bike-shedding to the max. We could really > use this feature for 4.0, given that about 70 routes have been > rewritten from Perl to Go and not vetted in a production environment > yet (that I know of... please speak up if you're running master in > Prod). > > - Rawlin > > On Wed, Nov 20, 2019 at 4:48 PM ocket 8888 <[email protected]> wrote: > > > > The routes don't need to be regular expressions though. That's a change > > that still needs to be made, and while it would have other benefits > beyond > > this whitelist/blacklist, it is a significant amount of work that may or > > may not be more than the work of getting regular expressions to work, so > > idk if you wanna do that but it's true. Then, as far as path parameters, > > you can just strip them before comparison by doing something like > > > > regex.MustCompile(`\{[^\}]+\}`).Replace(path, "") > > > > not actually familiar with that api, but the point is that comparisons > can > > be made to ignore path parameters probably pretty easily - as long as > they > > are just strings. > > > > On Wed, Nov 20, 2019 at 4:31 PM Rawlin Peters <[email protected]> wrote: > > > > > That's an interesting idea, but I think that might make this feature > > > too complicated, especially with Perl going away. Ultimately, clients > > > shouldn't have to worry about which backend is actually handling their > > > request, and this feature is mainly about only falling back to the > > > Perl handler in case serious regressions are discovered in the > > > Go-rewritten handler. > > > > > > - Rawlin > > > > > > On Wed, Nov 20, 2019 at 4:10 PM Williams, Adam > > > <[email protected]> wrote: > > > > > > > > An alternative implementation could be for the TrafficOps client to > be > > > able to specify the backend, on a per request basis (via HTTP header, > > > params, or some other mechanism), instead of a whitelist in > TrafficOps. In > > > this case, TrafficOps would still ultimately decide if a route could be > > > serviced by multiple backends or not. > > > > > > > > One nice property is that tests, automated or otherwise, could easily > > > compare behavior from multiple backends. For example, a test case in > the > > > traffic_ops/testing/api/v14 directory could use the same logic to test > both > > > the Perl and Go implementations. > > > > > > > > From: ocket 8888 <[email protected]> > > > > Reply-To: "[email protected]" < > [email protected] > > > > > > > > Date: Wednesday, November 20, 2019 at 15:25 > > > > To: "[email protected]" <[email protected]> > > > > Subject: Re: [EXTERNAL] TO API routing blacklist > > > > > > > > I think documenting all of those is going to be more work than using > the > > > > actual paths. I'm also not a fan of numeric IDs in most cases > because it > > > > means > > > > > > > > you wouldn't be able to tell which routes are "Perl'd" or disabled > just > > > > by looking at the config file > > > > > > > > > > > > > > > > On Wed, Nov 20, 2019 at 3:16 PM Rawlin Peters <[email protected] > <mailto: > > > [email protected]>> wrote: > > > > > > > > No, operators wouldn't need to read the source code, they would just > > > > have to read something like this in the log (printed on startup): > > > > 1 GET /api/1.1/cdns > > > > 2 PUT /api/1.1/cdns > > > > ... > > > > and so on. Then in cdn.conf you would put: > > > > { > > > > .... > > > > "routing_blacklist": { > > > > "perl_routes": [1, 2, 3, 4], > > > > "disabled_routes": [5, 6, 7, 8] > > > > }, > > > > .... > > > > } > > > > > > > > The obvious disadvantage would be that you wouldn't be able to tell > > > > which routes are "Perl'd" or disabled just by looking at the config > > > > file (although you could add comments in ignored json fields if you > > > > wanted to), but in practice I don't think this will be much of an > > > > issue. > > > > > > > > Yes, the idea is that the IDs would be statically maintained and not > > > > auto-generated on startup (as that could invalidate existing configs > > > > as you've said). Basically, we would just have to pick the next > unused > > > > integer whenever adding a new route. Maintenance cost would be > > > > basically zero, since TO would fail to start if you've added a route > > > > with an ID that is already taken. > > > > > > > > - Rawlin > > > > > > > > On Wed, Nov 20, 2019 at 3:00 PM ocket 8888 <[email protected] > <mailto: > > > [email protected]>> wrote: > > > > > > > > > > If you make the configuration ID-based you're telling operators > they > > > need > > > > > to read the source code to be able to configure TO. Plus if those > IDs > > > are > > > > > generated sequentially and we need to insert a route in the middle > so > > > > that > > > > > a later rule that would match it doesn't override the route then > > > suddenly > > > > > everyone's configuration file is broken. Well, either that or we > need > > > to > > > > > statically maintain and document a magic number for every API > route. > > > > > > > > > > Idk if this helps at all, but as has been pointed out before the > routes > > > > > don't actually need to be regular expressions. > > > > > > > > > > On Wed, Nov 20, 2019 at 2:55 PM Rawlin Peters <[email protected] > > > <mailto:[email protected]>> wrote: > > > > > > > > > > > Hey folks, > > > > > > > > > > > > I'm currently working on this TO API routing blacklist feature, > and > > > > > > while I've identified which routes should be on the whitelist of > > > > > > "routes that have been rewritten to Go but that are still safe to > > > fall > > > > > > back to Perl for", the tediousness of copying route regular > > > > > > expressions for the whitelist has given me another idea. > > > > > > > > > > > > Rather than have the configuration be based on regular > expressions > > > > > > that are meant to match the actual routes in the code, I was > thinking > > > > > > of giving each route an ID, including the ID as part of the Route > > > > > > struct, and making the configuration based on these IDs instead > of > > > > > > trying to mirror the regex. > > > > > > > > > > > > That way, you can't accidentally disable routes or have Perl > handle > > > > > > routes you didn't mean to from writing a bad regular expression. > > > > > > Essentially, every route would get a unique ID that can be > referenced > > > > > > in the config for either disabling it or routing it to Perl. > Whether > > > > > > or not a Go route would be routable to Perl would also become > part of > > > > > > the Route struct. TO would print the route IDs on startup (so > you can > > > > > > easily find them and match them to the routes you're trying to > > > disable > > > > > > or fall back to Perl) and verify that the actual given route IDs > are > > > > > > unique (to ensure that IDs stay unique as routes are moved > around or > > > > > > new routes are added). > > > > > > > > > > > > What do you think? Stick to regular expressions, or go with this > IDea > > > > > > instead (see what I did there)? > > > > > > > > > > > > - Rawlin > > > > > > > > > > > > On Fri, Nov 1, 2019 at 4:01 PM Gray, Jonathan < > > > > [email protected]<mailto:[email protected]>> > > > > > > wrote: > > > > > > > > > > > > > > I largely don't care about the blacklisted routes for this > purpose. > > > > I > > > > > > really care about a conclusive list of whitelisted routes (for > which > > > > the > > > > > > example json payload could be expanded to carry). It seems like > > > we're > > > > > > solving the exact same issue from two directions. It permits > each > > > > native > > > > > > client library to assert that the routes it expects and needs to > > > exist, > > > > > > exist on the other side. I have no desire to actively modify the > > > > runtime > > > > > > routes (for security I don't think we every should), just to get > the > > > > list > > > > > > of what it had at startup. Having the override config file on > disk > > > to > > > > > > switch on/off independent route/methods is something I'd expect > to > > > > have to > > > > > > restart TO for (no different than changes in the cdn.conf). I do > > > also > > > > > > agree with proper 503 handling, but it allows us to perform a > basic > > > > sanity > > > > > > check to prevent half-completed workflows necessitating complex > > > > recovery > > > > > > paths. For applications that use the client SDK, it gives an > easy > > > > handle > > > > > > to know if every single upgrade necessitates recompiling and > > > deploying > > > > 3rd > > > > > > party applications, such as a CZF File generator. > > > > > > > > > > > > > > Jonathan G > > > > > > > > > > > > > > > > > > > > > On 11/1/19, 1:49 PM, "Rawlin Peters" <[email protected] > <mailto: > > > [email protected]>> wrote: > > > > > > > > > > > > > > > Not trying to sideswipe, but could we expose that as an > > > > endpoint > > > > > > with a Golang list as well to solve: > > > > > > > > > > > > > > https://urldefense.com/v3/__https://protect2.fireeye.com/url?k=5d57b4c9-01b3ba02-5d57937d-000babff3540-17d7cedf2908de8b&u=https:**Agithub.com*apache*trafficcontrol*issues*2872__;Ly8vLy8v!rx_L75ITgOQ!VMUtDsupfJ7TCk0L1Blt_1O4ovxM1nalp21_Fpmbp1Htn9o2oWOC83kc0nWYfOWdDzCc$ > > > < > > > > https://urldefense.com/v3/__https:/protect2.fireeye.com/url?k=5d57b4c9-01b3ba02-5d57937d-000babff3540-17d7cedf2908de8b&u=https:**Agithub.com*apache*trafficcontrol*issues*2872__;Ly8vLy8v!rx_L75ITgOQ!VMUtDsupfJ7TCk0L1Blt_1O4ovxM1nalp21_Fpmbp1Htn9o2oWOC83kc0nWYfOWdDzCc$ > > > > > > > > > > > > > > > > > > While I do agree with the request for an API endpoint that > > > tells > > > > the > > > > > > > client what API versions are supported, I wouldn't want to > > > > > > > overcomplicate *this* particular feature with an API > endpoint > > > to > > > > > > > expose the information that is in the config file. > > > > > > > > > > > > > > If we implement that kind of "API information" API > endpoint, I > > > > > > > wouldn't be opposed to including the currently blacklisted > > > > routes in > > > > > > > its response as a minor goal, but I don't really think it's > > > > warranted > > > > > > > by this routing blacklist feature alone. You should have a > > > > really, > > > > > > > really good reason to blacklist a route or bypass a TO-Go > route > > > > for > > > > > > > the Perl, so this should be a (hopefully) relatively rare > > > > operation > > > > > > to > > > > > > > begin with. I don't think it would be all that useful for > API > > > > clients > > > > > > > to be able to see the list of currently blacklisted APIs. > The > > > API > > > > > > > client should be written to properly handle 503s whenever > they > > > > occur, > > > > > > > and to the client it shouldn't matter if the 503 is from > the > > > > database > > > > > > > being overloaded at the time or if the route is > blacklisted. > > > > > > > > > > > > > > - Rawlin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
