Would the “permissions” field be better titled as “localizationMethods” or “localizationPolicies"? Permissions typically relates to security and access control, so it seems a bit out of place here
Also, should we consider adding the fallback list to the allowed localization methods/policies? This might allow us to remove the “fallbackToGeo” flag If a backup list is specified and geo-location is not allowed, that can be managed through this table rather than the one-off flag Definitely +1 to keep this extensible, as there will be more localization types in the future (Anycast, etc,… We at Cisco also have a network hops localization policy engine too, although its not open-sourced) —Eric > On Jul 5, 2018, at 11:24 AM, Robert Butts <[email protected]> wrote: > > +1 on listing them as an array. JSON arrays are defined to be ordered, so > that lets us use the array order as the lookup order, making it easy to add > that as a feature in the future. > > -1 on array types in the database. It's unnecessary denormalization, and > will be harder to deal with in the future. We can do the same thing with a > normal join table: > > cg | localization | priority > ---------------------------- > foo | coverage_zone | 1 > foo | maxmind | 2 > foo | deep | 0 > bar | coverage_zone | 0 > > Which maps to the config `"permissions": [ "deep", "coverage-zone", > "geo"],` and also addresses your points on future types (we just add a new > enum value to the localization type). The additional `priority` column lets > us order it in the JSON array via SELECT ORDER BY. > > +1 on rethinking the cachegroup fallback API, too. We should have more join > tables, and better data normalization, but it's painful to use HTTP/JSON > APIs like that. Our APIs will be much easier to use, if we can stop > exposing join tables to the API altogether (e.g. cachegroup_fallback, > deliveryservice_regex), and only insert them into their proper objects > (cachegroup, deliveryservice), and then manipulate them within the object > via POST/PUT/PATCH on the larger object. > > > On Thu, Jul 5, 2018 at 9:23 AM, Mark Torluemke <[email protected]> > wrote: > >> Inline. >> >> On Thu, Jul 5, 2018 at 9:00 AM, Rawlin Peters <[email protected]> >> wrote: >> >>> You're not mistaken, Jonathan, that is how the logic basically flows >>> today; however, with cachegroup fallback configuration now it can be >>> deep[if enabled] -> cz -> geo[if fallback config allows]. >>> >>> Marking a cachegroup as "CZ-only" is really about filtering possible >>> cachegroups in selection *after* a client's location is discovered (by >>> geoIP DB). So it can really be described as "given the method of >>> client localization, filter out certain cachegroup targets". >>> >>> "Deep-only": filtered out after the "CZ localization" and "post-Geo >>> localization" steps >>> "CZ-only": filtered out after the "Geo localization" step >>> "Geo-only": filtered out after the "deep/regular-CZ" steps >>> >>> Right now, those all seem somewhat mutually exclusive (except CZ kind >>> of includes both deep and regular CZ), unless you wanted to do >>> something like "Deep and Geo only". I don't know if that's a valid use >>> case, but maybe "CZ and Future-thing only" would make sense. So maybe >>> cachegroups should really be described as: >>> >>> "Deep-enabled": allowed after "deep-CZ localization" step >>> "CZ-enabled": allowed after the "regular-CZ localization" step >>> "Geo-enabled": allowed after the "geo localization" step >>> "Future-thing-enabled": allowed after the "future-thing" step >>> >> >> +1. Well said. >> >> >>> >>> Then we could add multiple permissions to a cachegroup rather than >>> making them mutually exclusive. For the initial use case I proposed >>> this for, I would just be marking the cachegroups that aren't directly >>> connected to the internet as "Deep-enabled, CZ-enabled, >>> future-thing-enabled" rather than "CZ-only". "Deep-only" cachegroups >>> would be marked "deep-enabled". >>> >>> If no permissions are configured, all permissions are given. Enabling >>> one permission means all other permissions are disabled. >>> >>> I think this could be done by adding an array-type column to the >>> cachegroup table/API like so: >>> >>> { >>> "name": "cache_group_edge", >>> "shortName": "cg_edge", >>> "latitude": 12, >>> "longitude": 45, >>> "permissions": [ >>> "DEEP", >>> "CZ", >>> "GEO" >>> ], >>> "typeId": 6 >>> } >>> >> >> The array feels a little fragile, perhaps. Should we decorate each entry >> with an "order", for completeness? (It's obvious to us that the order is >> DEEP->CZ->GEO, but might not be for new users.) >> >> >>> The "permissions" field would remain optional, meaning that no >>> permissions chosen assumes *all* permissions are enabled (giving you >>> the existing default behavior). Postgres has array-type columns which >>> would map to this API quite nicely. In fact, I'm also kind of >>> rethinking the "Cachegroup Fallback" API after going over this, >>> because listing a cachegroup's fallbacks in an array like this could >>> make sense and remove the need for the new cachegroup_fallbacks API... >> >> >> +1 here as well (without knowing the ins-and-outs of the Cachegroup >> Fallback API). >> >> >> >>> On Wed, Jul 4, 2018 at 3:47 PM Gray, Jonathan <[email protected] >>> >>> wrote: >>>> >>>> If I'm not mistaken, today the logic is fall through on miss (deep[if >>> enabled] -> cz -> geo). The suggestion to make a 1:* relationship moves >>> that from being implicit in the code to explicit choice by the DS owner. >>> Changing the database/apis is expensive enough that maybe we can't make >>> that jump near term. If you do only add a single field for this using an >>> enum, there should be a default 'fallthrough' or 'native' option to >>> replicate the old behavior. Also, I misspoke earlier, this isn't about >> the >>> routing methods it's about localization methods. >>>> >>>> On 7/4/18, 11:18 AM, "Jeremy Mitchell" <[email protected]> wrote: >>>> >>>> Can a cache group have many "routing methods". If not, would one >>> column >>>> (cachegroup.routing_method) suffice where acceptable values are an >>> enum? ( >>>> CZ, DEEP, GEO) >>>> >>>> On Tue, Jul 3, 2018 at 7:05 PM, Mark Torluemke < >>> [email protected]> >>>> wrote: >>>> >>>>> Feels like columns in the cachegroup table could go a long way in >>> the short >>>>> term -- routing_cz, routing_geo, routing_deep? >>>>> >>>>> On Tue, Jul 3, 2018 at 1:04 PM, Rawlin Peters < >>> [email protected]> >>>>> wrote: >>>>> >>>>>> Thanks for the suggestion, Jonathan. I wasn't even thinking >>> about the >>>>>> possibility of "deep-only" cachegroups, but I could definitely >>> see >>>>>> that as a future use case. >>>>>> >>>>>> Do you envision something like a table of cachegroup >> permissions >>>>>> (cachegroup_id, cachegroup_permission enum type), where the >> enums >>>>>> would be DEEP_ONLY and CZ_ONLY to start with? That would >>> definitely >>>>>> increase initial dev time a bit and might require some new API >>>>>> endpoints to add/remove permissions unless we can make the new >>> API >>>>>> field use an array value. >>>>>> On Tue, Jul 3, 2018 at 12:23 PM Gray, Jonathan >>>>>> <[email protected]> wrote: >>>>>>> >>>>>>> Rather than a cz_only flag, it might be more powerful to have >>> a join >>>>>> table with allowed routing methods. You could cover the same >>> use case, >>>>> but >>>>>> it would also allow you to do certain things like deep_only or >>>>> effectively >>>>>> not-geo. >>>>>>> >>>>>>> - Jonathan >>>>>>> >>>>>>> On 7/3/18, 12:03 PM, "Rawlin Peters" < >> [email protected]> >>> wrote: >>>>>>> >>>>>>> Hey Traffic Controllers, >>>>>>> >>>>>>> I'd like to be able to mark a cachegroup as "CZ-only" via >>> the API >>>>> so >>>>>>> that off-net clients (i.e. IPs that aren't included in >> the >>> Coverage >>>>>>> Zone File) don't get routed to "CZ-only" cachegroups. >> This >>> is >>>>> because >>>>>>> some cachegroups aren't directly connected to the >> internet >>> and >>>>>> routing >>>>>>> off-net clients to those cachegroups is expensive. This >>> proposal >>>>>> would >>>>>>> change Traffic Router behavior to route these clients to >>> the >>>>> closest >>>>>>> available cachegroup that's NOT marked as "CZ-only". >>>>>>> >>>>>>> My proposal is to add a boolean column to the cachegroup >>> table - >>>>>>> cz_only - which is then included in the CRConfig's >>> "edgeLocations" >>>>>> for >>>>>>> Traffic Router to parse and look up when routing off-net >>> clients. >>>>> By >>>>>>> default it will be false to keep the existing behavior. >>> The new >>>>> field >>>>>>> will be optional in the API but NOT NULL in the DB, so >>> null values >>>>> in >>>>>>> requests will explicitly be set to false in the DB. >>>>>>> >>>>>>> Questions/concerns? >>>>>>> >>>>>>> - Rawlin >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>>> >>> >>
