Thanks, Neuman. I agree with the points made by Jonathan and ocket8888, and if the ID-based solution ends up being a bit of a pain, we can always add support for a purely path-based solution later, once all the routes are no longer regexes. I just don't want this to be too hard for a layman to configure and to avoid the risk of messing up regular expressions. Thinking about the startup log issue a bit more, it shouldn't be too hard for me to add a CLI option to traffic_ops_golang to just print out the route + ID table in an easily parseable manner. Hopefully that would alleviate that concern a bit.
- Rawlin 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
