Hello,

please review attached patch set. It fixes
https://fedorahosted.org/bind-dyndb-ldap/ticket/167

The code is also available on Github:
https://github.com/pspacek/bind-dyndb-ldap/tree/fix_root_zone_removal

Patched SRPM:
https://pspacek.fedorapeople.org/bind-dyndb-ldap/bind-dyndb-ldap-10.0-3.fc24.src.rpm

COPR build:
https://copr.fedorainfracloud.org/coprs/pspacek/bind-dyndb-ldap/build/440841/

Martin Basti, please build it also in @freeipa/freeipa-master COPR so CI can
pick it up. Thank you!


Patch set description:
Fix zone removal to respect forward configuration inheritance.

Ad-hoc fwd_delete_table() calls did not respect inheritance hierarchy
in forwarding configuration. Now all manipulation with forward table
is done in fwd_configure_zone() and fully respects configuration inheritance.

There is a trick: When removing or deactivating a zone, fwd_configure_zone()
is called with empty configuration set to simulate that the zone does
not have any explicit configuration. This triggers the inheritance
logic when necessary (i.e. for the root zone).

https://fedorahosted.org/bind-dyndb-ldap/ticket/167
https://github.com/pspacek/bind-dyndb-ldap/commit/d6e413c4cc88101b902d73e05e1ce35e2fe4aedd



Remove preserve_forwarding parameter from ldap_delete_zone2().

The parameter was TRUE only when called from zone_security_change().
zone_security_change() is calling ldap_delete_zone2() in exclusive mode
anyway so there is no need to optimize this.

Removal of the parameter will make easier to centralize forwarding
configuration on one place.

https://fedorahosted.org/bind-dyndb-ldap/ticket/167
https://github.com/pspacek/bind-dyndb-ldap/commit/b40976263460d8f4aeeec2a2a8f41cc54dcd0b28

-- 
Petr^2 Spacek
From 3b44a3d73a0d1980b43e9022d9249c4c19ba56d2 Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Thu, 11 Aug 2016 12:40:39 +0200
Subject: [PATCH] Remove preserve_forwarding parameter from
 ldap_delete_zone2().

The parameter was TRUE only when called from zone_security_change().
zone_security_change() is calling ldap_delete_zone2() in exclusive mode
anyway so there is no need to optimize this.

Removal of the parameter will make easier to centralize forwarding
configuration on one place.

https://fedorahosted.org/bind-dyndb-ldap/ticket/167
---
 src/ldap_helper.c   | 26 ++++++++++----------------
 src/ldap_helper.h   |  3 +--
 src/zone_register.c |  2 +-
 3 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 12a743b095ba400373cb87653d26af82cc95c2ea..696a755fb8001993ff1a16fa034a9286cbb5ad89 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -1285,8 +1285,7 @@ configure_zone_ssutable(dns_zone_t *zone, const char *update_str)
 
 /* Delete zone by dns zone name */
 isc_result_t
-ldap_delete_zone2(ldap_instance_t *inst, dns_name_t *name, isc_boolean_t lock,
-		  isc_boolean_t preserve_forwarding)
+ldap_delete_zone2(ldap_instance_t *inst, dns_name_t *name, isc_boolean_t lock)
 {
 	isc_result_t result;
 	isc_result_t isforward = ISC_R_NOTFOUND;
@@ -1302,13 +1301,11 @@ ldap_delete_zone2(ldap_instance_t *inst, dns_name_t *name, isc_boolean_t lock,
 	if (lock)
 		run_exclusive_enter(inst, &lock_state);
 
-	if (!preserve_forwarding) {
-		CHECK(fwd_delete_table(inst->view, name, "zone",
-				       zone_name_char));
-		isforward = fwdr_zone_ispresent(inst->fwd_register, name);
-		if (isforward == ISC_R_SUCCESS)
-			CHECK(fwdr_del_zone(inst->fwd_register, name));
-	}
+	CHECK(fwd_delete_table(inst->view, name, "zone",
+			       zone_name_char));
+	isforward = fwdr_zone_ispresent(inst->fwd_register, name);
+	if (isforward == ISC_R_SUCCESS)
+		CHECK(fwdr_del_zone(inst->fwd_register, name));
 
 	result = zr_get_zone_ptr(inst->zone_register, name, &raw, &secure);
 	if (result == ISC_R_NOTFOUND || result == DNS_R_PARTIALMATCH) {
@@ -1487,8 +1484,7 @@ ldap_parse_fwd_zoneentry(ldap_entry_t *entry, ldap_instance_t *inst)
 	if (HEAD(values) != NULL &&
 	    strcasecmp(HEAD(values)->value, "TRUE") != 0) {
 		/* Zone is not active */
-		result = ldap_delete_zone2(inst, &entry->fqdn,
-					   ISC_TRUE, ISC_FALSE);
+		result = ldap_delete_zone2(inst, &entry->fqdn, ISC_TRUE);
 		goto cleanup;
 	}
 
@@ -1990,7 +1986,7 @@ zone_security_change(ldap_entry_t * const entry, dns_name_t * const name,
 	 * in period where old zone was deleted but the new zone was not
 	 * created yet. */
 	run_exclusive_enter(inst, &lock_state);
-	CHECK(ldap_delete_zone2(inst, name, ISC_FALSE, ISC_TRUE));
+	CHECK(ldap_delete_zone2(inst, name, ISC_FALSE));
 	CHECK(ldap_parse_master_zoneentry(entry, olddb, inst, task));
 
 cleanup:
@@ -2173,8 +2169,7 @@ cleanup:
 		log_error_r("%s: publishing failed, rolling back due to",
 			    ldap_entry_logname(entry));
 		/* TODO: verify this */
-		result = ldap_delete_zone2(inst, &entry->fqdn,
-					   ISC_TRUE, ISC_FALSE);
+		result = ldap_delete_zone2(inst, &entry->fqdn, ISC_TRUE);
 		if (result != ISC_R_SUCCESS)
 			log_error_r("%s: rollback failed: ",
 				    ldap_entry_logname(entry));
@@ -3671,8 +3666,7 @@ update_zone(isc_task_t *task, isc_event_t *event)
 	INSIST(task == inst->task); /* For task-exclusive mode */
 
 	if (SYNCREPL_DEL(pevent->chgtype)) {
-		CHECK(ldap_delete_zone2(inst, &entry->fqdn,
-					ISC_TRUE, ISC_FALSE));
+		CHECK(ldap_delete_zone2(inst, &entry->fqdn, ISC_TRUE));
 	} else {
 		if (entry->class & LDAP_ENTRYCLASS_MASTER)
 			CHECK(ldap_parse_master_zoneentry(entry, NULL, inst,
diff --git a/src/ldap_helper.h b/src/ldap_helper.h
index 0368ec7343ef7b16e7afb25b17f3067bf7c09f76..a491baeb41105b9a352dbad6949c3fab008ab69b 100644
--- a/src/ldap_helper.h
+++ b/src/ldap_helper.h
@@ -46,8 +46,7 @@ new_ldap_instance(isc_mem_t *mctx, const char *db_name,
 void destroy_ldap_instance(ldap_instance_t **ldap_inst) ATTR_NONNULLS;
 
 isc_result_t
-ldap_delete_zone2(ldap_instance_t *inst, dns_name_t *name,
-		  isc_boolean_t lock, isc_boolean_t preserve_forwarding)
+ldap_delete_zone2(ldap_instance_t *inst, dns_name_t *name, isc_boolean_t lock)
 		  ATTR_NONNULLS;
 
 /* Functions for writing to LDAP. */
diff --git a/src/zone_register.c b/src/zone_register.c
index 3f8c070b3adfb0ecc5092eb1e84f3956ba3b4fb8..bde4a7c308a6a62ebe6b9123b3212a404603310a 100644
--- a/src/zone_register.c
+++ b/src/zone_register.c
@@ -163,7 +163,7 @@ zr_destroy(zone_register_t **zrp)
 		if (result == ISC_R_SUCCESS) {
 			rbt_iter_stop(&iter);
 			result = ldap_delete_zone2(zr->ldap_inst,
-						   &name, ISC_FALSE, ISC_FALSE);
+						   &name, ISC_FALSE);
 			RUNTIME_CHECK(result == ISC_R_SUCCESS);
 		}
 	} while (result == ISC_R_SUCCESS);
-- 
2.7.4

From c28f568bb546bb87adb1d250a479ff966de81a40 Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Fri, 12 Aug 2016 12:18:33 +0200
Subject: [PATCH] Fix zone removal to respect forward configuration
 inheritance.

Ad-hoc fwd_delete_table() calls did not respect inheritance hierarchy
in forwarding configuration. Now all manipulation with forward table
is done in fwd_configure_zone() and fully respects configuration inheritance.

There is a trick: When removing or deactivating a zone, fwd_configure_zone()
is called with empty configuration set to simulate that the zone does
not have any explicit configuration. This triggers the inheritance
logic when necessary (i.e. for the root zone).

https://fedorahosted.org/bind-dyndb-ldap/ticket/167
---
 src/ldap_helper.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 696a755fb8001993ff1a16fa034a9286cbb5ad89..ad6e41764740d18e9015b01d83e2634e0bcb9213 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -1301,19 +1301,18 @@ ldap_delete_zone2(ldap_instance_t *inst, dns_name_t *name, isc_boolean_t lock)
 	if (lock)
 		run_exclusive_enter(inst, &lock_state);
 
-	CHECK(fwd_delete_table(inst->view, name, "zone",
-			       zone_name_char));
+	/* simulate no explicit forwarding configuration */
+	CHECK(fwd_configure_zone(&inst->empty_fwdz_settings, inst, name));
 	isforward = fwdr_zone_ispresent(inst->fwd_register, name);
 	if (isforward == ISC_R_SUCCESS)
 		CHECK(fwdr_del_zone(inst->fwd_register, name));
 
 	result = zr_get_zone_ptr(inst->zone_register, name, &raw, &secure);
 	if (result == ISC_R_NOTFOUND || result == DNS_R_PARTIALMATCH) {
 		if (isforward == ISC_R_SUCCESS)
 			log_info("forward zone '%s': shutting down", zone_name_char);
 		log_debug(1, "zone '%s' not found in zone register", zone_name_char);
-		result = dns_view_flushcache(inst->view);
-		goto cleanup;
+		CLEANUP_WITH(ISC_R_SUCCESS);
 	} else if (result != ISC_R_SUCCESS)
 		goto cleanup;
 
@@ -1373,7 +1372,8 @@ unpublish_zone(ldap_instance_t *inst, dns_name_t *name, const char *logname) {
 	}
 	CHECK(dns_view_findzone(inst->view, name, &zone_in_view));
 	INSIST(zone_in_view == raw || zone_in_view == secure);
-	CHECK(fwd_delete_table(inst->view, name, "zone", logname));
+	/* simulate no explicit forwarding configuration */
+	CHECK(fwd_configure_zone(&inst->empty_fwdz_settings, inst, name));
 	CHECK(dns_zt_unmount(inst->view->zonetable, zone_in_view));
 
 cleanup:
-- 
2.7.4

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to