octodns / octodns-ns1

Ns1Provider provider for octoDNS
MIT License
4 stars 13 forks source link

Geo rule for Asia pulls in Oceania also #38

Closed viranch closed 1 year ago

viranch commented 1 year ago

Something that crossed my mind while testing #37: the mapping of AS continent to NS1's ASIAPAC region inadvertently pulls in all non-Asian (ie, Oceania) countries also to that rule. Example:

www:
  ttl: 300
  type: CNAME
  value: def.odns.io.
  dynamic:
    pools:
      as:
        values:
        - value: as.odns.io.
          status: up
      world:
        values:
        - value: any.odns.io.
          status: up
    rules:
    - geos: [AS]
      pool: as
    - pool: world

The above record should hand out the world pool for clients from Oceania. But due to the above mapping, NS1 will hand out as pool for them, which can be against user's expectations since octodns offers another OC continent separate from AS.

One solution can be to duplicate the catch-all rule and put Oceania in there. If one doesn't exist, point OC to the top-level default.

istr commented 1 year ago

@viranch good that you found this as well. Maybe just don't use the built-in ASIAPAC ns1 region at all (because in reality it maps octo(AS + OC) <-> ns1(ASIAPAC)) and use a synthetic region for AS in ns1 as well (just like for OC)?

viranch commented 1 year ago

@istr AS has the largest list of countries, but yes, not impossible to do. I would want to re-use octo's geo_data.py though instead of hard-coding the list of countries in Ns1Provider like it does today for OC.

ross commented 1 year ago

@viranch good that you found this as well. Maybe just don't use the built-in ASIAPAC ns1 region at all (because in reality it maps octo(AS + OC) <-> ns1(ASIAPAC)) and use a synthetic region for AS in ns1 as well (just like for OC)?

That makes sense/seems preferable to me.

@istr AS has the largest list of countries, but yes, not impossible to do. I would want to re-use octo's geo_data.py though instead of hard-coding the list of countries in Ns1Provider like it does today for OC.

Good call. Just checked and there's actually one missing from the NS1 list, UM that's in geo_data.

ross commented 1 year ago

Fixed by https://github.com/octodns/octodns-ns1/pull/41.