Add TO API Routing Blacklist via cdn.conf#4145
Conversation
rawlinp
added
new feature
|
Refer to this link for build results (access rights to CI server needed): |
rawlinp
force-pushed
the
to-api-routing-blacklist-IDs
branch
from
ce4e759 to
3977df3
Compare
|
Refer to this link for build results (access rights to CI server needed): |
Hm, I see Chris's point here, numbers seem a bit more difficult for people to use. But I also see your point on the list about regexes being a pain. Question: is there a reason the route ID needs to be a number? What if every Route had a "Route ID" that was an arbitrary string, e.g. Would it be difficult to change the existing code to that? If not, is there an advantage to numbers over strings? |
I considered that as well, but what would you name the 10 different variations of the |
Well, the whole endeavor is kinda a short term patch. But I think that "short term" means "at least a year" here. So it's worth at least a little to consider the ops costs folks will incur over the year. Also, I'm a bit concerned about the opportunity for merge conflicts. Any time you create a registry of ids, you're going to get conflicts, especially at the rate we're developing in this area.
I really like this suggestion. It also solves the merge conflict issue.
The name is mostly arbitrary. I'd do something fairly regular, though, like: get-federations A route-name like that would also be handy for recording which handler was invoked for logging purposes if we wanted later. |
|
We should keep in mind, not only is at least |
I'm less concerned about the "adding a route" case. Numbers work fine for that, imo. (But your random number suggestion works better in that case, too.) I'm more concerned about the "trying to figure out why TO isn't behaving the way I expect it to" debugging case. For that, the engineer doing the debugging would need to look up each route id to see which thing it refers to. And while they might have to check the docs to see exactly which of the dozen or so federation routes it refers to, you won't need to check all of them, because you can rule out all the routes that obviously don't apply by name. With few exceptions, config files are for humans and it's ideal to let them be readable for humans. You changed the regex precisely because of this. It was terrible for humans. This is a step up, but I think we can do slightly better.
Writing features that aren't meant to be used is a path that often leads to long-term suffering. perl_routes should definitely go away (although I'm not sure quite how "soon".) disabled_routes will probably live forever, though. |
|
One thing I was thinking about is maybe we could rework this into some kind of Go plugin at some point. Once request paths are actually strings, we could configure a plugin to accept a blacklist of endpoints to disable and then we can remove all this ID stuff from |
|
To be clear, I'm -0 on numeric IDs, +0 on string IDs. I don't agree numbers are easier for Dev or Ops, but I don't feel strongly enough to fight for it. |
That's a great idea. I agree 100%, this is an excellent candidate for a Plugin. But I wouldn't hold up this PR for it, it can be converted later.
That's the intention. There are very few Plugin Hooks at the moment, the idea was that we'd add them as we need them. They're easy to add, and I'm happy to help anyone adding one (I wrote the Plugin framework, maybe it's not as obvious to others as me; maybe we can improve the docs at the same time). |
|
And since we're being clear... I'm -2 on consecutive IDs (gotta make up numbers here :) ), -1 on random IDs, and +1 on string ids, especially if we can leverage those strings into the future for logging and plugin purposes. But the world won't end if we decide that naming all the routes is more work than we're willing to allocate at the moment and just go with random IDs. |
|
I guess I'd need to know more about the "random ID" implementation to comment on it. Seems like it'd just make things more confusing and make it even harder to add new routes than sequential IDs. |
|
All these suggestions are great, but we've successfully migrated the bike-shedding off of the mailing list into this PR. I can re-generate the route IDs to make them random instead of sequential, in order to reduce potential dev headaches due to merge conflicts, but I will leave the rest of the comments that require completely redoing this PR and/or the TO routing code to be done in the future, once our routes are simple paths and not regexes. If anyone feels strongly enough to go through and uniquely name all of our existing routes, replace the IDs, and open a PR before I cut 4.x, I would be open to that. But we are looking to cut 4.x within a week. |
|
I get the concern and will defer to the implementer. But it's worth noting that if we don't do it now, it's going to be much, much harder to do in the future because we'll have backward compatibility concerns. |
|
I don't think it would be that much harder in terms of backwards-compatibility. We would just have to keep numeric IDs and name-based IDs (or non-regex paths) on the route objects for a release, allowing both integer-based and name-based route IDs in the config (but separate fields). Then we'd tell everyone they have to switch to name-based route IDs before the next release, and rip out the route IDs in the next release. I know this PR seems a little big at ~800 LOC changed, but ~600 of those is just updating the existing Route objects to add IDs and a boolean field. The code responsible for actually bypassing to Perl and disabling routes was only ~200 LOC changed and are fairly straightforward IMO. So adding support for name-based IDs on top of integer-based IDs would basically be copy/paste of the ID-based code, except checking for string matches instead of integer matches. It would take longer to come up with a unique name for all of our existing endpoints than to add support for disabling/Perling routes on those name-based IDs. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
ocket8888
left a comment
ocket8888
left a comment
There was a problem hiding this comment.
both command line options work, verified that blacklist and perllist both work, all tests still pass.
|
Refer to this link for build results (access rights to CI server needed): |
|
@alficles if you're good with this the way it is now, can you submit an approving review? I don't wanna merge something out from under an active reviewer. |
rawlinp
force-pushed
the
to-api-routing-blacklist-IDs
branch
from
4710aa8 to
4485970
Compare
|
Rebased to resolve merge conflict due to the newly-rewritten |
Add a "routing_blacklist" field to cdn.conf's "traffic_ops_golang"
section, consisting of two array fields:
- "perl_routes": routes that will be handled by TO-Perl instead of Go
- "disabled_routes": routes that will be disabled by immediately
returning a 503
These fields are arrays of route IDs (integers). As part of this change,
routes in traffic_ops_golang get a unique ID now, as well as a boolean
field indicating whether or not the route can be bypassed to TO-Perl.
Add a "--api-routes" flag to traffic_ops_golang that prints out all the
route information (ID, version, path, etc) and exits.
Also, add option to ignore unknown route IDs on startup.
rawlinp
force-pushed
the
to-api-routing-blacklist-IDs
branch
from
4485970 to
f64b71d
Compare
|
Rebased to resolve merge conflict due to |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
alficles
left a comment
alficles
left a comment
There was a problem hiding this comment.
New stuff looks good. Thanks!
6 participants
What does this PR (Pull Request) do?
Add a
"routing_blacklist"field to cdn.conf's "traffic_ops_golang"section, consisting of two array fields:
"perl_routes": routes that will be handled by TO-Perl instead of Go"disabled_routes": routes that will be disabled by immediatelyreturning a 503
These fields are arrays of route IDs (integers). As part of this change,
routes in
traffic_ops_golangget a unique ID now, as well as a booleanfield indicating whether or not the route can be bypassed to TO-Perl.
Add a
--api-routesflag totraffic_ops_golangthat prints out all theroute information (ID, version, path, etc) and exits.
Which Traffic Control components are affected by this PR?
What is the best way to verify this PR?
traffic_ops_golangunit testscdn.confto disable a certain route, make a request to that route, and verify that a 503 response is returned.cdn.confto have Perl handle a certain route, make a request to that route, and verify that the route was handled by TO-Perl and not Go.The following criteria are ALL met by this PR