I agree with being vigilant with regards to the Perl. I am still on board with forcing the Perl route handlers to return a 501 after being rewritten, but we shouldn't do that in the same release as its corresponding Go-rewritten route so that we'd still be able to fallback easily to the Perl handler. That, and we should have a really good reason to deviate the Go-rewritten handler's behavior from the Perl in such a way that they wouldn't remain interchangeable. That should be a conscious decision made by the contributor/community, and I'd hope that we'd be well aware at that point to know not to keep it "Perl-routable".
- Rawlin On Fri, Nov 1, 2019 at 11:45 AM Robert Butts <[email protected]> wrote: > > > I would propose that newly rewritten routes be added to this hardcoded > list in TO-Go. Then after being in one release as "Perl-routable", they > would become "non-Perl-routable" in the following release. > > I'm a big +1, as long as we're extra-vigilant about this. More than > unroutable, we need to delete the code. The old Perl which has diverged > from Go is extremely dangerous and can result in catastrophic data loss and > unpredictable behavior. We got consensus to delete it and change the > endpoints (which shouldn't be callable) to return a 501, like a year ago, > but never did it. Which is my fault as much as anyone; but we need to do it. > > Also, if we ever change behavior at the same time as a rewrite, we need to > be careful _not_ to add it to the whitelist, for the reason above, because > then calling the Perl can result in unexpected behavior and data loss. > > But as long as we're careful about that, and don't allow falling back to > Perl where it's changed, and start deleting the dangerous old code; this is > a great feature, to not have to roll back a deploy for a single broken > endpoint. +1 > > > On Fri, Nov 1, 2019 at 11:41 AM Jeremy Mitchell <[email protected]> > wrote: > > > I'm +1 on this idea as well. Not too concerned about the implementation but > > just having an easy way to "turn off" rewritten Go api routes and fall back > > to the proven perl routes seems like a nice safety valve. In the past, > > we've seen bugs creep in to the api rewrite and a quick off switch seems > > very valuable to have > > > > On Fri, Nov 1, 2019 at 11:03 AM Dave Neuman <[email protected]> wrote: > > > > > I am +1 on this idea. > > > > > > On Fri, Nov 1, 2019 at 09:26 Gray, Jonathan <[email protected]> > > > wrote: > > > > > > > The available routes and what associated backend they go to are > > operator+ > > > > sensitive information. I'd also lean towards a stronger degree of > > safety > > > > and only make it read-only. > > > > > > > > Jonathan G > > > > > > > > On 11/1/19, 8:41 AM, "Hoppal, Michael" <[email protected]> > > > > wrote: > > > > > > > > I would be +1 on TO API routing blacklist. > > > > > > > > I think it is a great idea to allow us to continue to move fast > > while > > > > minimizing risk. > > > > > > > > I do agree we should expose the current state of the blacklist as > > an > > > > API endpoint as to easily get insight into the current configuration. > > We > > > > could even make the blacklist driven by the API instead of a JSON > > config > > > > file. > > > > > > > > Michael > > > > > > > > On 10/31/19, 4:18 PM, "Gray, Jonathan" <[email protected]> > > > > wrote: > > > > > > > > Not trying to sideswipe, but could we expose that as an > > endpoint > > > > with a Golang list as well to solve: > > > > > > > > > https://protect2.fireeye.com/url?k=1978833f-459c8df4-1978a48b-000babff3540-c717b11942d8382e&u=https://github.com/apache/trafficcontrol/issues/2872 > > > > > > > > Jonathan G > > > > > > > > On 10/31/19, 3:51 PM, "Rawlin Peters" <[email protected]> > > wrote: > > > > > > > > Hey all, > > > > > > > > We've been picking up momentum in the TO Perl -> Go rewrite > > > > recently, > > > > which has gotten me thinking of ways to reduce the risk of > > > > possible > > > > regressions in the rewritten APIs. I'd like to propose that > > > we > > > > implement a sort of "routing blacklist" for the TO API > > > routes. > > > > > > > > At a high level, this blacklist would be a simple JSON > > config > > > > file like this: > > > > { > > > > "perlRoutes": [ > > > > {"method": "GET", "path": "/api/1.1/foos"}, > > > > {"method": "POST", "path": "/api/1.1/foos"} > > > > ], > > > > "disabledRoutes": [ > > > > {"method": "GET", "path": "/api/1.1/foos"}, > > > > {"method": "POST", "path": "/api/1.1/foos"} > > > > ] > > > > } > > > > > > > > APIs in "perlRoutes" would be explicitly routed to the > > Perl, > > > > even if > > > > the Go route exists and could handle the request. The > > primary > > > > use case > > > > for this feature would be to deploy an upgraded version of > > > > TO-Go with > > > > rewritten APIs without having TO-Go handle the requests > > yet. > > > > Post-upgrade, you could remove a rewritten route from the > > > > blacklist on > > > > a single TO instance, validate it for some period of time, > > > > then roll > > > > out the rewritten API to other TO instances by removing the > > > > API from > > > > their blacklists. > > > > > > > > APIs in "disabledRoutes" would be explicitly disabled. The > > > use > > > > cases > > > > for this field would include: > > > > - disabling endpoints that are part of an incomplete > > feature > > > > and don't > > > > really make sense to use on their own yet > > > > - disabling endpoints that have known, serious issues that > > > > should be > > > > disabled immediately > > > > This would make it easier to plug holes in TO without > > having > > > to > > > > rebuild and redeploy. > > > > > > > > Ideally, this config file would be SIGHUP-able so that it > > can > > > > be > > > > reconfigured without having to restart TO. Also, there > > should > > > > be a > > > > hardcoded list of "Perl-routable" routes within TO-Go, so > > > that > > > > you > > > > can't just depend on Perl forever if you wanted. I would > > > > propose that > > > > newly rewritten routes be added to this hardcoded list in > > > > TO-Go. Then > > > > after being in one release as "Perl-routable", they would > > > > become > > > > "non-Perl-routable" in the following release. > > > > > > > > IMO we could've used something like this since the > > beginning > > > > of the TO > > > > Perl -> Go rewrite effort, but we still do have a decent > > > > amount of > > > > routes left that this could be useful for. > > > > > > > > Please let me know what you think. If we're generally +1 on > > > > this idea, > > > > I'll throw together a blueprint with more details, then > > maybe > > > > I can > > > > convince my boss to let me work on it ;) > > > > > > > > - Rawlin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
