octodns / octodns-ns1

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

'A' Record with tier > 1 not populating answers #52

Closed tatnat closed 1 year ago

tatnat commented 1 year ago

Hello!

I'm noticing that for 'A' records that are tier > 1 and contain the meta field that the answers field is not being populated. This results in the plan containing an A record with no values and typically results in a KeyError down the road.

Is it by design that the values field is not extended with the answers and only done in geo? What about if the meta fields are something else (in my case I have up)?


Reference function: https://github.com/octodns/octodns-ns1/blame/main/octodns_ns1/__init__.py#L565-L601

def _data_for_geo_A(self, _type, record):
        # record meta (which would include geo information is only
        # returned when getting a record's detail, not from zone detail
        geo = defaultdict(list)
        data = {'ttl': record['ttl'], 'type': _type}
        values, codes = [], []
        for answer in record.get('answers', []):
            meta = answer.get('meta', {})
            if meta:
                # country + state and country + province are allowed
                # in that case though, supplying a state/province would
                # be redundant since the country would supercede in when
                # resolving the record.  it is syntactically valid, however.
                country = meta.get('country', [])
                us_state = meta.get('us_state', [])
                ca_province = meta.get('ca_province', [])
                for cntry in country:
                    con = country_alpha2_to_continent_code(cntry)
                    key = f'{con}-{cntry}'
                    geo[key].extend(answer['answer'])
                for state in us_state:
                    key = f'NA-US-{state}'
                    geo[key].extend(answer['answer'])
                for province in ca_province:
                    key = f'NA-CA-{province}'
                    geo[key].extend(answer['answer'])
                for code in meta.get('iso_region_code', []):
                    key = code
                    geo[key].extend(answer['answer'])
            else:
                values.extend(answer['answer'])
                codes.append([])
        values = [str(x) for x in values]
        geo = OrderedDict({str(k): [str(x) for x in v] for k, v in geo.items()})
        data['values'] = values
        data['geo'] = geo
        return data

This line needs to be unindented so that it runs for each answer as it gets skipped for branch if meta.

values.extend(answer['answer'])

Sample geo record:

[{'domain': 'ntp.test.ntat.io', 'zone': 'test.ntat.io', 'id': '6466b20632fe8805a06f5a84', 'use_client_subnet': True, 'answers': [{'answer': ['1.1.1.1'], 'meta': {'up': True}, 'id': '6466b20632fe8805a06f5a82'}, {'answer': ['2.2.2.2'], 'meta': {'up': True}, 'id': '6466b20632fe8805a06f5a83'}], 'networks': [0], 'regions': {}, 'meta': {}, 'link': None, 'filters': [{'filter': 'up', 'config': {}}, {'filter': 'shuffle', 'config': {}}], 'ttl': 3600, 'tier': 2, 'type': 'A', 'override_ttl': False}]

Results in Record:

name=ntp, data={'ttl': 3600, 'type': 'A', 'values': [], 'geo': OrderedDict()}, data_for=<bound method Ns1Provider._data_for_A of Ns1Provider>

And dumped in yaml as:

ntp:
  type: A

Also another side observation is that it seems codes isn't used for anything but I don't have enough context on how that should be implemented.

tatnat commented 1 year ago

Upon further investigation, is this by design? https://github.com/octodns/octodns-ns1/issues/17

viranch commented 1 year ago

This is because octodns-ns1 can't parse geo or dynamic records not originally created by itself. This could be handled better with more appropriate messaging in the log output.

Also, geo records are an old variant of the more modern and feature-rich dynamic records. @ross any plans to deprecate support for geo records across octodns?

ross commented 1 year ago

@ross any plans to deprecate support for geo records across octodns?

We probably should. I guess 1.0 is the point at which they should either deprecate or just go away. I don't have strong feelings as to which should happen now.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 7 days.