Yeah, I was gonna say it sounds like Rawlin's already thought about this from every angle, and I'm certainly not about to jump and change the routing in the space of an hour so he can move forward with it. I don't mean to stand in the way of progress, it's just scary to think of things that are meant to be short-lived and used for a specific purpose winding up long-standing things that everyone rolls theirs eyes about but can't/won't change because there are bigger fires and whatnot (e.g. "RASCAL_" mandatory prefix). Hopefully Perl won't survive the version roll to 5.x.
On Thu, Nov 21, 2019 at 8:20 AM Dave Neuman <[email protected]> wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
