Use a custom designate driver for FIP's #70

misamoylov · 2024-09-23T14:24:13Z

No description provided.

Use designate /v2/reverse/floatingips/ endpoint to create DNS zones and DNS entries during FIP creation. Current driver uses zones and recordset endpoints instead of specific designate endpoint.

neutron/db/dns_db.py

neutron/services/externaldns/drivers/designate/driver_ccloud.py

neutron/db/dns_db.py

neutron/services/externaldns/drivers/designate/driver_ccloud.py

neutron/db/dns_db.py

mutax

only some minor things

neutron/services/externaldns/driver.py

neutron/services/externaldns/drivers/designate/driver.py

neutron/db/dns_db.py

neutron/conf/services/extdns_designate_driver.py

neutron/db/dns_db.py

neutron/services/externaldns/driver.py

neutron/tests/unit/extensions/test_l3.py

neutron/db/dns_db.py

neutron/services/externaldns/drivers/designate/driver_ccloud.py

sebageek · 2025-02-19T11:09:31Z

neutron/services/externaldns/drivers/designate/driver_ccloud.py

+class DesignateCcloud(driver.ExternalDNSService):
+    """Driver for Designate."""


It looks like your code is almost identical to neutron.services.externaldns.drivers.designate.driver.Designate. You could inherit that class and then only use the methods that changed, i.e. create_record_set() and delete_record_set.

If I read the git log correctly, we already extensively modified that driver compared to upstream.
This means updating to a new release would mean patching two classes. If we want to avoid that we would need to compare the currently proposed new driver to upstream first and then overwrite the changed methods.

Yes, we already have a changes for driver.py.

Ah, I see. Would it then make sense to - in an extra commit - change the driver back to original? Then we have the usptream driver there and only our changes in our own driver.

I did a diff between the real upstream, our changes in yoga and the new ccloud driver, as well as the 2024.2 version.

Given that most of the code changes and only __init__ and 4 class-internal helper methods are not changed, inheriting from the upstream class seems to have no advantages, but would make the code hard to follow.

Reverting driver.py back to the upstream version would, in my opinion, defeat the purpose of adding a new ccloud driver instead of replacing driver.py. The original upstream driver will not work for us at all, and the reason to keep the our existing driver.py was, to be able to quickly switch back to that version by config, in case the new driver shows bad behavior.

If we replace our existing driver.py with upstream, we could also just replace driver.py by the ccloud_driver.py and be done.

neutron/services/externaldns/drivers/designate/driver_ccloud.py

sebageek · 2025-02-19T11:12:17Z

neutron/services/externaldns/drivers/designate/driver_ccloud.py

+        client, admin_client = get_all_projects_edit_managed_client(
+            context)


This seems to be the only change to the already existing Designate externaldns driver for this class. Why do we need this change here? It doesn't seem to be related to the FIP problem you set out to solve. If we don't need it we could just use the already existing delete_record_set() and would have less code duplication.

We need it to avoid leftover ptr's.

tools/check_unit_test_structure.sh

neutron/tests/unit/extensions/test_dns_ccloud.py

neutron/services/externaldns/drivers/designate/driver_ccloud.py

mutax · 2025-03-07T11:46:25Z

neutron/services/externaldns/drivers/designate/driver_ccloud.py

@@ -1,4 +1,4 @@
-# Copyright (c) 2016 IBM
+# Copyright (c) 2025 SAP SE


Copyright is cumulative (i.e. as long as we keep some of the original code, the original's author still applies to that part of the code).
So please don't remove IBMs copyright, but instead add ours next to it, else this would be an IP infringement.
(Needed only for files that we actually change.)

mutax · 2025-03-07T11:50:52Z

neutron/tests/unit/extensions/test_dns_ccloud.py

@@ -1,4 +1,4 @@
-# Copyright 2012 VMware, Inc.
+# Copyright (c) 2025 SAP SE


Same issue with the copyright - we cannot remove other companies as long as there is upstream code left in the file.

mutax · 2025-03-07T12:39:32Z

neutron/services/externaldns/drivers/designate/driver_ccloud.py

+def get_clients(context, all_projects=False, edit_managed=False):
+    global _SESSION
+
+    if not _SESSION:
+        _SESSION = loading.load_session_from_conf_options(
+            CONF, 'designate')
+
+    auth = token_endpoint.Token(CONF.designate.url, context.auth_token)
+    client = d_client.Client(session=_SESSION, auth=auth)
+    if CONF.designate.auth_type:
+        admin_auth = loading.load_auth_from_conf_options(
+            CONF, 'designate')
+    else:
+        # TODO(tkajinam): Make this fail when admin_* parameters are removed.
+        admin_auth = password.Password(
+            auth_url=CONF.designate.admin_auth_url,
+            username=CONF.designate.admin_username,
+            password=CONF.designate.admin_password,
+            tenant_name=CONF.designate.admin_tenant_name,
+            tenant_id=CONF.designate.admin_tenant_id)
+    admin_client = d_client.Client(session=_SESSION, auth=admin_auth,
+                                   endpoint_override=CONF.designate.url,
+                                   all_projects=all_projects,
+                                   edit_managed=edit_managed)
+    return client, admin_client
+
+
+def get_all_projects_client(context):
+    auth = token_endpoint.Token(CONF.designate.url, context.auth_token)
+    return d_client.Client(session=_SESSION, auth=auth, all_projects=True)
+
+
+def get_all_projects_edit_managed_client(context):
+    return get_clients(context, all_projects=True, edit_managed=True)


get_clients is modified to support all_projects and edit_managed, which we need.
get_all_projects_client is not used and can be omitted, that is true
and get_all_projects_edit_managed_client is a local addition that is not present in the original upstream.
So there is nothing to import left I think.

mutax · 2025-03-07T12:50:30Z

neutron/services/externaldns/drivers/designate/driver_ccloud.py

+class DesignateCcloud(driver.ExternalDNSService):
+    """Driver for Designate."""


I did a diff between the real upstream, our changes in yoga and the new ccloud driver, as well as the 2024.2 version.

Given that most of the code changes and only __init__ and 4 class-internal helper methods are not changed, inheriting from the upstream class seems to have no advantages, but would make the code hard to follow.

Reverting driver.py back to the upstream version would, in my opinion, defeat the purpose of adding a new ccloud driver instead of replacing driver.py. The original upstream driver will not work for us at all, and the reason to keep the our existing driver.py was, to be able to quickly switch back to that version by config, in case the new driver shows bad behavior.

If we replace our existing driver.py with upstream, we could also just replace driver.py by the ccloud_driver.py and be done.

misamoylov force-pushed the fip_update branch from 47fec91 to 0cc8559 Compare October 31, 2024 13:55

misamoylov force-pushed the fip_update branch 5 times, most recently from 672dc6d to 1be302f Compare November 15, 2024 12:53

misamoylov marked this pull request as ready for review November 15, 2024 13:17

misamoylov force-pushed the fip_update branch from 1be302f to a5bd376 Compare December 16, 2024 11:51

misamoylov requested a review from galkindmitrii December 17, 2024 12:06

[designate] Add custom dns driver for FIP's

Loading
Loading status checks…

b44f3e7

Use designate /v2/reverse/floatingips/ endpoint to create DNS zones and DNS entries during FIP creation. Current driver uses zones and recordset endpoints instead of specific designate endpoint.

misamoylov force-pushed the fip_update branch from a5bd376 to b44f3e7 Compare January 13, 2025 10:19

mutax self-requested a review January 21, 2025 11:26

mutax requested changes Jan 22, 2025

View reviewed changes

mutax reviewed Jan 23, 2025

View reviewed changes

neutron/db/dns_db.py Show resolved Hide resolved

misamoylov marked this pull request as draft January 30, 2025 14:02

misamoylov added 2 commits January 30, 2025 18:34

Update after review

Loading
Loading status checks…

8534d81

Update after review

Loading
Loading status checks…

20d9ba7

misamoylov force-pushed the fip_update branch from fa3f049 to 20d9ba7 Compare February 2, 2025 03:15

misamoylov marked this pull request as ready for review February 5, 2025 10:09

mutax requested changes Feb 11, 2025

View reviewed changes

Add custom FIP DNS driver. Address review

Loading
Loading status checks…

63351f8

misamoylov requested a review from mutax February 12, 2025 10:13

sebageek requested changes Feb 19, 2025

View reviewed changes

mutax reviewed Feb 19, 2025

View reviewed changes

neutron/services/externaldns/drivers/designate/driver_ccloud.py Show resolved Hide resolved

misamoylov added 6 commits February 21, 2025 17:15

Add custom FIP DNS driver. Address review

Loading
Loading status checks…

27fce56

Add custom FIP DNS driver. Address review

Loading
Loading status checks…

e3650c8

Update

63382b9

Update

Loading
Loading status checks…

c0e587c

Update tests

f00f1eb

Update tests

Loading
Loading status checks…

63ec657

misamoylov added 4 commits February 26, 2025 15:44

Update tests

Loading
Loading status checks…

79d56ac

Update tests

Loading
Loading status checks…

65d1256

Update tests

Loading
Loading status checks…

ca23bc0

Update tests

Loading
Loading status checks…

a408d7c

mutax reviewed Mar 7, 2025

View reviewed changes

misamoylov added 2 commits March 10, 2025 13:45

Update copyrights and delete unused method in driver

577f3e0

Update copyrights and delete unused method in driver

Loading
Loading status checks…

e44bf90

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a custom designate driver for FIP's #70

Use a custom designate driver for FIP's #70

misamoylov commented Sep 23, 2024

mutax left a comment

sebageek Feb 19, 2025

mutax Feb 19, 2025

misamoylov Feb 21, 2025

sebageek Feb 26, 2025

mutax Mar 7, 2025

sebageek Feb 19, 2025

misamoylov Feb 24, 2025

mutax Mar 7, 2025

mutax Mar 7, 2025

mutax Mar 7, 2025

mutax Mar 7, 2025

		class DesignateCcloud(driver.ExternalDNSService):
		"""Driver for Designate."""

		client, admin_client = get_all_projects_edit_managed_client(
		context)

		@@ -1,4 +1,4 @@
		# Copyright (c) 2016 IBM
		# Copyright (c) 2025 SAP SE

		@@ -1,4 +1,4 @@
		# Copyright 2012 VMware, Inc.
		# Copyright (c) 2025 SAP SE

Use a custom designate driver for FIP's #70

Are you sure you want to change the base?

Use a custom designate driver for FIP's #70

Conversation

misamoylov commented Sep 23, 2024

mutax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment