Repository: octodns/octodns

Issue:
Deletion when syncing from a remote provider to the local Yaml provider.
From my testing and operations it seems that when syncing a zone (e.g. clowns.org) from a remote provider (e.g. DigitalOcean) to the local Yaml Provider,  if the local yaml file e.g. clowns.org.yaml contains:

---
bozo:
    type A:
    value: 10.11.12.13

And said record is NO found in the remote DigitalOcean zone:  clowns.org
then when a sync is done from DO ----> localYAML the above record stays unchanged. 

I suppose this is by design? I.e. it's not possible to treat the DO data in some sense as authoritative
and then sync to localYaml and then from there to other remote locales.

Is there a way to perform  a sync from a remote provider that will do a true sync, which may include adds, mods and deletions?

Discussion:
@ross: Going to need more details about what you're running. If you only plan (no `--doit`) then the file would be left untouched, but if you apply the changes the local yaml file will be overwritten with the values pulled from DO.

What's you're octoDNS config file look like an what command line and args are you running?@quistian: Ah... That's what I expected --doit to do.  How about in the case (which is what I was testing) where the DO zone has no RRs, .i.e. empty and the local yaml file for that zone say has only one A record. Will the sync with --doit leave the local yaml file empty as well?@ross: `octodns-sync --doit` makes whatever exists in the `sources` exist in the `targets`, if that's nothing then the targets will plan and apply their way to nothing.@ross: Actually there might be an edge case there if the `sources` and empty, the `YamlProvider` target may think there's nothing to do and thus not apply. Hadn't thought about that before since that's not really a common/expected case to have nothing in sources. It should work for everything else except `YamlProvider`. Not sure if that could be cleanly addressed.@quistian: Ok..that's exactly what I though should happen. I will have to dig deeper into the provider I've written for the BlueCat v1 API to see why the yaml RRs are not being overwritten in my test case. Thanks.'@quistian: Here's what I get when my BC zone is empty and there is indeed one local yaml record:


```
$ cat leaf-zones/234.privatelink.openai.azure.com.yaml
---
n234_test:
  ttl: 4800
  type: A
  value: 10.140.94.10

And the sync from an empty BC zone:

....

$ octodns-sync --debug --config config/bcv1_to_yaml.yaml 234.privatelink.openai.azure.com.
....
2024-10-16T17:25:32  [48383631960] INFO  BlueCatProvider[bluecat-v1] populate: found 0 records, exists=True
2024-10-16T17:25:32  [48383631960] DEBUG Manager sync:   planning, zone=234.privatelink.openai.azure.com.
2024-10-16T17:25:32  [48383631960] INFO  YamlProvider[yaml] plan: desired=234.privatelink.openai.azure.com.
2024-10-16T17:25:32  [48383631960] DEBUG Zone __init__: zone=Zone<234.privatelink.openai.azure.com.>, sub_zones=set()
2024-10-16T17:25:32  [48383631960] DEBUG YamlProvider[yaml] populate: name=234.privatelink.openai.azure.com., target=True, lenient=True
2024-10-16T17:25:32  [48383631960] DEBUG Zone __init__: zone=Zone<234.privatelink.openai.azure.com.>, sub_zones=set()
2024-10-16T17:25:32  [48383631960] WARNING YamlProvider[yaml] root NS record supported, but no record is configured for 234.privatelink.openai.azure.com.
2024-10-16T17:25:32  [48383631960] DEBUG Zone changes: zone=Zone<234.privatelink.openai.azure.com.>, target=YamlProvider
2024-10-16T17:25:32  [48383631960] INFO  YamlProvider[yaml] plan:   No changes
2024-10-16T17:25:32  [48383631960] INFO  Plan
********************************************************************************
No changes were planned
********************************************************************************

Here's the config file:


providers:
  bluecat-v1:
    class: octodns_bluecat_v1.BlueCatProvider
    endpoint: env/BAM_ENDPOINT
    username: env/BAM_USER
    password: env/BAM_PW
    confname: env/BAM_CONF
    viewname: env/BAM_VIEW
  yaml:
    class: octodns.provider.yaml.YamlProvider
    directory: ./leaf-zones
    default_ttl: 3600
    enforce_order: True

processors:
  only-a-and-aaaa:
    class: octodns.processor.filter.TypeAllowlistFilter
    allowlist:
      - A
      - AAAA
  no-root-ns:
    class: octodns.processor.filter.IgnoreRootNsFilter

zones:
  '*':
    sources:
      - bluecat-v1
    processors:
      - no-root-ns
      - only-a-and-aaaa
    targets:
      - yaml
```



@ross: So yeah, that's the behavior mentioned in https://github.com/octodns/octodns/issues/1222#issuecomment-2417946734, working as expected, if not ideally would.@quistian: What I have found, is if I create an A record, in what was an empty zone, and do the sync (on the above mentioned case), the new A record gets created in the yaml file and the old record does get deleted. So the sync happens... But... octodns-sync says nothing about doing the delete. It says Creates=1, Deletes=0... 

```
$ cat leaf-zones/234.privatelink.openai.azure.com.yaml
---
n234_test:
  ttl: 4800
  type: A
  value: 10.140.94.10
```

```
$ octodns-sync --doit --config config/bcv1_to_yaml.yaml 234.privatelink.openai.azure.com.
....
2024-10-16T21:03:51  [16374987407960] WARNING YamlProvider[yaml] root NS record supported, but no record is configured for 234.privatelink.openai.azure.com.
2024-10-16T21:03:51  [16374987407960] INFO  YamlProvider[yaml] plan:   Creates=1, Updates=0, Deletes=0, Existing Records=0
2024-10-16T21:03:51  [16374987407960] INFO  Plan
********************************************************************************
* 234.privatelink.openai.azure.com.
********************************************************************************
* yaml (YamlProvider)
*   Create Zone<234.privatelink.openai.azure.com.>
*   Create <ARecord A 3600, z234_do_not_remove.234.privatelink.openai.azure.com., ['10.0.0.1']> (bluecat-v1)
*   Summary: Creates=1, Updates=0, Deletes=0, Existing Records=0
********************************************************************************


2024-10-16T21:03:51  [16374987407960] INFO  YamlProvider[yaml] apply: making 1 changes to 234.privatelink.openai.azure.com.
2024-10-16T21:03:51  [16374987407960] INFO  Manager sync:   1 total changes
```

```
$ cat leaf-zones/234.privatelink.openai.azure.com.yaml
---
z234_do_not_remove:
  type: A
  value: 10.0.0.1
```

```@ross: > What I have found, is if I create an A record, in what was an empty zone, and do the sync (on the above mentioned case), the new A record gets created in the yaml file and the old record does get deleted. So the sync happens... But... octodns-sync says nothing about doing the delete. It says Creates=1, Deletes=0...

Right. That's due to the same reasons as an empty sync doesn't delete anything. This is all expected behavior and I'm not sure if there's a clean way to do otherwise without digging into it and it's not a super high priority as it's behaving correctly and as expected, just the messaging isn't ideal.

https://github.com/octodns/octodns/blob/e30d91783eb0b85507605eb51fcfb75b0773fb26/octodns/provider/yaml.py#L345-L348@ross: Thought of a way to potentially pull off a fully spec/behavior compliant `YamlProvider` today on the :bus: ride home. Results are over in https://github.com/octodns/octodns/pull/1223#pullrequestreview-2376437939. Will have to think about and look over it more closely before it'd move forward. May even hold off for the 2.x timeline.@ross: https://github.com/octodns/octodns/pull/1223 has shipped and I believe that with those changes you should now get the desired behavior. Going to close this now. Feel free to comment/reopen if there's more.

Solution patch:
diff --git a/octodns/provider/yaml.py b/octodns/provider/yaml.py
index b5fd3d7..1a2a512 100644
--- a/octodns/provider/yaml.py
+++ b/octodns/provider/yaml.py
@@ -73,6 +73,12 @@ class YamlProvider(BaseProvider):
         # (optional, default False)
         disable_zonefile: false
 
+    Note
+    ----
+
+    When using this provider as a target any existing comments or formatting
+    in the zone files will be lost when changes are applyed.
+
     Split Details
     -------------
 
@@ -342,11 +348,6 @@ class YamlProvider(BaseProvider):
             lenient,
         )
 
-        if target:
-            # When acting as a target we ignore any existing records so that we
-            # create a completely new copy
-            return False
-
         before = len(zone.records)
 
         sources = []
@@ -363,33 +364,37 @@ class YamlProvider(BaseProvider):
         if self.shared_filename:
             sources.append(join(self.directory, self.shared_filename))
 
-        if not sources:
+        if not sources and not target:
             raise ProviderException(f'no YAMLs found for {zone.decoded_name}')
 
-        # determinstically order our sources
+        # deterministically order our sources
         sources.sort()
 
         for source in sources:
             self._populate_from_file(source, zone, lenient)
 
+        exists = len(sources) > 0
         self.log.info(
-            'populate:   found %s records, exists=False',
+            'populate:   found %s records, exists=%s',
             len(zone.records) - before,
+            exists,
         )
-        return False
+        return exists
 
     def _apply(self, plan):
-        desired = plan.desired
+        # make a copy of existing we can muck with
+        copy = plan.existing.copy()
         changes = plan.changes
         self.log.debug(
-            '_apply: zone=%s, len(changes)=%d',
-            desired.decoded_name,
-            len(changes),
+            '_apply: zone=%s, len(changes)=%d', copy.decoded_name, len(changes)
         )
-        # Since we don't have existing we'll only see creates
-        records = [c.new for c in changes]
-        # Order things alphabetically (records sort that way
-        records.sort()
+
+        # apply our pending changes to that copy
+        copy.apply(changes)
+
+        # we now have the records we need to write out, order things
+        # alphabetically (records sort that way
+        records = sorted(copy.records)
         data = defaultdict(list)
         for record in records:
             d = record.data
@@ -411,7 +416,7 @@ class YamlProvider(BaseProvider):
 
         if self.split_extension:
             # we're going to do split files
-            decoded_name = desired.decoded_name[:-1]
+            decoded_name = copy.decoded_name[:-1]
             directory = join(
                 self.directory, f'{decoded_name}{self.split_extension}'
             )
@@ -443,7 +448,7 @@ class YamlProvider(BaseProvider):
 
         else:
             # single large file
-            filename = join(self.directory, f'{desired.decoded_name}yaml')
+            filename = join(self.directory, f'{copy.decoded_name}yaml')
             self.log.debug('_apply:   writing filename=%s', filename)
             with open(filename, 'w') as fh:
                 safe_dump(
diff --git a/octodns/zone.py b/octodns/zone.py
index 5f23267..a94cda9 100644
--- a/octodns/zone.py
+++ b/octodns/zone.py
@@ -340,6 +340,16 @@ class Zone(object):
 
         return changes
 
+    def apply(self, changes):
+        '''
+        Apply the provided changes to the zone.
+        '''
+        for change in changes:
+            if isinstance(change, Delete):
+                self.remove_record(change.existing)
+            else:
+                self.add_record(change.new, replace=True, lenient=True)
+
     def hydrate(self):
         '''
         Take a shallow copy Zone and make it a deeper copy holding its own


Test patch:
diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py
index 271268d..76e5027 100644
--- a/tests/test_octodns_manager.py
+++ b/tests/test_octodns_manager.py
@@ -2,7 +2,7 @@
 #
 #
 
-from os import environ, listdir
+from os import environ, listdir, remove
 from os.path import dirname, isfile, join
 from unittest import TestCase
 from unittest.mock import MagicMock, patch
@@ -39,6 +39,13 @@ def get_config_filename(which):
     return join(config_dir, which)
 
 
+def reset(directory):
+    for filename in listdir(directory):
+        if filename.endswith('.yaml'):
+            filename = join(directory, filename)
+            remove(filename)
+
+
 class TestManager(TestCase):
     def test_missing_provider_class(self):
         with self.assertRaises(ManagerException) as ctx:
@@ -147,40 +154,47 @@ class TestManager(TestCase):
         with TemporaryDirectory() as tmpdir:
             environ['YAML_TMP_DIR'] = tmpdir.dirname
             environ['YAML_TMP_DIR2'] = tmpdir.dirname
+
             tc = Manager(get_config_filename('simple.yaml')).sync(dry_run=False)
             self.assertEqual(28, tc)
 
             # try with just one of the zones
+            reset(tmpdir.dirname)
             tc = Manager(get_config_filename('simple.yaml')).sync(
                 dry_run=False, eligible_zones=['unit.tests.']
             )
             self.assertEqual(22, tc)
 
             # the subzone, with 2 targets
+            reset(tmpdir.dirname)
             tc = Manager(get_config_filename('simple.yaml')).sync(
                 dry_run=False, eligible_zones=['subzone.unit.tests.']
             )
             self.assertEqual(6, tc)
 
             # and finally the empty zone
+            reset(tmpdir.dirname)
             tc = Manager(get_config_filename('simple.yaml')).sync(
                 dry_run=False, eligible_zones=['empty.']
             )
             self.assertEqual(0, tc)
 
             # Again with force
+            reset(tmpdir.dirname)
             tc = Manager(get_config_filename('simple.yaml')).sync(
                 dry_run=False, force=True
             )
             self.assertEqual(28, tc)
 
             # Again with max_workers = 1
+            reset(tmpdir.dirname)
             tc = Manager(
                 get_config_filename('simple.yaml'), max_workers=1
             ).sync(dry_run=False, force=True)
             self.assertEqual(28, tc)
 
             # Include meta
+            reset(tmpdir.dirname)
             tc = Manager(
                 get_config_filename('simple.yaml'),
                 max_workers=1,
@@ -1011,22 +1025,23 @@ class TestManager(TestCase):
         )
 
     def test_auto_arpa(self):
-        manager = Manager(get_config_filename('simple-arpa.yaml'))
+        with TemporaryDirectory() as tmpdir:
+            environ['YAML_TMP_DIR'] = tmpdir.dirname
 
-        # provider config
-        self.assertEqual(
-            True, manager.providers.get("auto-arpa").populate_should_replace
-        )
-        self.assertEqual(1800, manager.providers.get("auto-arpa").ttl)
+            manager = Manager(get_config_filename('simple-arpa.yaml'))
 
-        # processor config
-        self.assertEqual(
-            True, manager.processors.get("auto-arpa").populate_should_replace
-        )
-        self.assertEqual(1800, manager.processors.get("auto-arpa").ttl)
+            # provider config
+            self.assertEqual(
+                True, manager.providers.get("auto-arpa").populate_should_replace
+            )
+            self.assertEqual(1800, manager.providers.get("auto-arpa").ttl)
 
-        with TemporaryDirectory() as tmpdir:
-            environ['YAML_TMP_DIR'] = tmpdir.dirname
+            # processor config
+            self.assertEqual(
+                True,
+                manager.processors.get("auto-arpa").populate_should_replace,
+            )
+            self.assertEqual(1800, manager.processors.get("auto-arpa").ttl)
 
             # we can sync eligible_zones so long as they're not arpa
             tc = manager.sync(dry_run=False, eligible_zones=['unit.tests.'])
@@ -1043,6 +1058,7 @@ class TestManager(TestCase):
             )
 
             # same for eligible_sources
+            reset(tmpdir.dirname)
             tc = manager.sync(
                 dry_run=False,
                 eligible_zones=['unit.tests.'],
@@ -1058,6 +1074,7 @@ class TestManager(TestCase):
             )
 
             # same for eligible_targets
+            reset(tmpdir.dirname)
             tc = manager.sync(
                 dry_run=False,
                 eligible_zones=['unit.tests.'],
@@ -1073,10 +1090,11 @@ class TestManager(TestCase):
             )
 
             # full sync with arpa is fine, 2 extra records from it
+            reset(tmpdir.dirname)
             tc = manager.sync(dry_run=False)
             self.assertEqual(26, tc)
 
-    def test_dynamic_config(self):
+    def test_dynamic_config_targeted(self):
         with TemporaryDirectory() as tmpdir:
             environ['YAML_TMP_DIR'] = tmpdir.dirname
 
@@ -1100,6 +1118,12 @@ class TestManager(TestCase):
                 ),
             )
 
+    def test_dynamic_config_all(self):
+        with TemporaryDirectory() as tmpdir:
+            environ['YAML_TMP_DIR'] = tmpdir.dirname
+
+            manager = Manager(get_config_filename('dynamic-config.yaml'))
+
             # should sync everything across all zones, total of 32 records
             self.assertEqual(32, manager.sync(dry_run=False))
 
diff --git a/tests/test_octodns_provider_yaml.py b/tests/test_octodns_provider_yaml.py
index 06a6466..77a4f1e 100644
--- a/tests/test_octodns_provider_yaml.py
+++ b/tests/test_octodns_provider_yaml.py
@@ -29,11 +29,12 @@ class TestYamlProvider(TestCase):
         zone = Zone('unit.tests.', [])
         dynamic_zone = Zone('dynamic.tests.', [])
 
-        # With target we don't add anything
+        # With target we see everything
         source.populate(zone, target=source)
-        self.assertEqual(0, len(zone.records))
+        self.assertEqual(25, len(zone.records))
 
         # without it we see everything
+        zone = Zone('unit.tests.', [])
         source.populate(zone)
         self.assertEqual(25, len(zone.records))
 
@@ -95,11 +96,9 @@ class TestYamlProvider(TestCase):
 
             self.assertFalse(zone.changes(reloaded, target=source))
 
-            # A 2nd sync should still create everything
+            # A 2nd sync should result in no changes, thus no plan
             plan = target.plan(zone)
-            self.assertEqual(
-                22, len([c for c in plan.changes if isinstance(c, Create)])
-            )
+            self.assertFalse(plan)
 
             with open(yaml_file) as fh:
                 data = safe_load(fh.read())
@@ -177,7 +176,8 @@ class TestYamlProvider(TestCase):
             zone = Zone(idna_encode(name), [])
 
             # create a idna named file
-            with open(join(td.dirname, idna_encode(filename)), 'w') as fh:
+            idna_filename = join(td.dirname, idna_encode(filename))
+            with open(idna_filename, 'w') as fh:
                 fh.write(
                     '''---
 '':
@@ -204,7 +204,14 @@ xn--dj-kia8a:
             self.assertEqual(['2.3.4.5'], d['xn--dj-kia8a'].values)
             self.assertEqual(['3.4.5.6'], d['xn--28jm5b5a8k5k8cra'].values)
 
-            # create a utf8 named file (provider always writes utf-8 filenames
+            # if we plan there'll be nothing to do
+            plan = provider.plan(zone)
+            self.assertFalse(plan)
+
+            # get rid of the idna file
+            remove(idna_filename)
+            # create a utf8 named file (provider always writes utf-8 filenames,
+            # no file should have a plan now
             plan = provider.plan(zone)
             provider.apply(plan)
 
@@ -214,6 +221,9 @@ xn--dj-kia8a:
                 self.assertTrue('déjà:' in content)
                 self.assertTrue('これはテストです:' in content)
 
+            # recreate the idna version of the file
+            with open(idna_filename, 'w') as fh:
+                fh.write('')
             # does not allow both idna and utf8 named files
             with self.assertRaises(ProviderException) as ctx:
                 provider.populate(zone)
@@ -495,11 +505,12 @@ class TestSplitYamlProvider(TestCase):
         zone = Zone('unit.tests.', [])
         dynamic_zone = Zone('dynamic.tests.', [])
 
-        # With target we don't add anything
+        # With target we still see whatever is in the file
         source.populate(zone, target=source)
-        self.assertEqual(0, len(zone.records))
+        self.assertEqual(20, len(zone.records))
 
-        # without it we see everything
+        # without it we see everything too, doesn't make a difference
+        zone = Zone('unit.tests.', [])
         source.populate(zone)
         self.assertEqual(20, len(zone.records))
         self.assertFalse([r for r in zone.records if r.name.startswith('only')])
@@ -586,11 +597,9 @@ class TestSplitYamlProvider(TestCase):
 
             self.assertFalse(zone.changes(reloaded, target=source))
 
-            # A 2nd sync should still create everything
+            # A 2nd sync should have nothing to do
             plan = target.plan(zone)
-            self.assertEqual(
-                17, len([c for c in plan.changes if isinstance(c, Create)])
-            )
+            self.assertFalse(plan)
 
             yaml_file = join(zone_dir, '$unit.tests.yaml')
             self.assertTrue(isfile(yaml_file))
diff --git a/tests/test_octodns_zone.py b/tests/test_octodns_zone.py
index 4008210..16789d2 100644
--- a/tests/test_octodns_zone.py
+++ b/tests/test_octodns_zone.py
@@ -179,6 +179,9 @@ class TestZone(TestCase):
 
         # before == after -> no changes
         self.assertFalse(before.changes(after, target))
+        copy = before.copy()
+        copy.apply([])
+        self.assertEqual(copy.records, before.records)
 
         # add a record, delete a record -> [Delete, Create]
         c = ARecord(before, 'c', {'ttl': 42, 'value': '1.1.1.1'})
@@ -199,6 +202,15 @@ class TestZone(TestCase):
         delete.__repr__()
         create.__repr__()
 
+        # make a copy of before
+        copy = before.copy()
+        # apply the changes to it
+        copy.apply(changes)
+        # copy should not match it's origin any longer
+        self.assertNotEqual(copy.records, before.records)
+        # and it should now match the target
+        self.assertEqual(copy.records, after.records)
+
         after = Zone('unit.tests.', [])
         changed = ARecord(before, 'a', {'ttl': 42, 'value': '2.2.2.2'})
         after.add_record(changed)