consul-k8s icon indicating copy to clipboard operation
consul-k8s copied to clipboard

Only error for config entries from different datacenters when the config entries are different

Open komapa opened this issue 1 year ago • 1 comments

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request. Searching for pre-existing feature requests helps us consolidate datapoints for identical requirements into a single place, thank you!
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment.

Is your feature request related to a problem? Please describe.

We get a lot of reconciliation errors right now because we have ServiceRouter (for example) with the same content for the same app in two different datacenters where that app is deployed.

Use Case(s)

We apply the same config entries to different datacenters because of the way our automation works and we would like to not error when the end result in Consul is the same.

Contributions

Index: control-plane/controllers/configentry_controller.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/control-plane/controllers/configentry_controller.go b/control-plane/controllers/configentry_controller.go
--- a/control-plane/controllers/configentry_controller.go	(revision 0bd21a7814d69f39f82abd24db59bc22d2a46d47)
+++ b/control-plane/controllers/configentry_controller.go	(date 1703008805097)
@@ -213,25 +213,25 @@
 	requiresMigration := false
 	sourceDatacenter := entry.GetMeta()[common.DatacenterKey]
 
-	// Check if the config entry is managed by our datacenter.
-	// Do not process resource if the entry was not created within our datacenter
-	// as it was created in a different cluster which will be managing that config entry.
-	if sourceDatacenter != r.DatacenterName {
+	if !configEntry.MatchesConsul(entry) {
+		// Check if the config entry is managed by our datacenter.
+		// Do not process resource if the entry was not created within our datacenter
+		// as it was created in a different cluster which will be managing that config entry.
+		if sourceDatacenter != r.DatacenterName {
 
-		// Note that there is a special case where we will migrate a config entry
-		// that wasn't created by the controller if it has the migrate-entry annotation set to true.
-		// This functionality exists to help folks who are upgrading from older helm
-		// chart versions where they had previously created config entries themselves but
-		// now want to manage them through custom resources.
-		if configEntry.GetObjectMeta().Annotations[common.MigrateEntryKey] != common.MigrateEntryTrue {
-			return r.syncFailed(ctx, logger, crdCtrl, configEntry, ExternallyManagedConfigError,
-				sourceDatacenterMismatchErr(sourceDatacenter))
-		}
+			// Note that there is a special case where we will migrate a config entry
+			// that wasn't created by the controller if it has the migrate-entry annotation set to true.
+			// This functionality exists to help folks who are upgrading from older helm
+			// chart versions where they had previously created config entries themselves but
+			// now want to manage them through custom resources.
+			if configEntry.GetObjectMeta().Annotations[common.MigrateEntryKey] != common.MigrateEntryTrue {
+				return r.syncFailed(ctx, logger, crdCtrl, configEntry, ExternallyManagedConfigError,
+					sourceDatacenterMismatchErr(sourceDatacenter))
+			}
 
-		requiresMigration = true
-	}
+			requiresMigration = true
+		}
 
-	if !configEntry.MatchesConsul(entry) {
 		if requiresMigration {
 			// If we're migrating this config entry but the custom resource
 			// doesn't match what's in Consul currently we error out so that

komapa avatar Dec 19 '23 19:12 komapa

This might be a better patch/diff:

Index: control-plane/controller/configentry_controller.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/control-plane/controller/configentry_controller.go b/control-plane/controller/configentry_controller.go
--- a/control-plane/controller/configentry_controller.go	(revision bd9720f3c0a0246bcd9bdccc45e3e1c0a8704a74)
+++ b/control-plane/controller/configentry_controller.go	(date 1706581064135)
@@ -220,18 +220,18 @@
 
 	requiresMigration := false
 	sourceDatacenter := entry.GetMeta()[common.DatacenterKey]
+	matchesConsul := configEntry.MatchesConsul(entry)
 
 	// Check if the config entry is managed by our datacenter.
 	// Do not process resource if the entry was not created within our datacenter
 	// as it was created in a different cluster which will be managing that config entry.
 	if sourceDatacenter != r.DatacenterName {
-
 		// Note that there is a special case where we will migrate a config entry
 		// that wasn't created by the controller if it has the migrate-entry annotation set to true.
 		// This functionality exists to help folks who are upgrading from older helm
 		// chart versions where they had previously created config entries themselves but
 		// now want to manage them through custom resources.
-		if configEntry.GetObjectMeta().Annotations[common.MigrateEntryKey] != common.MigrateEntryTrue {
+		if !matchesConsul && configEntry.GetObjectMeta().Annotations[common.MigrateEntryKey] != common.MigrateEntryTrue {
 			return r.syncFailed(ctx, logger, crdCtrl, configEntry, ExternallyManagedConfigError,
 				sourceDatacenterMismatchErr(sourceDatacenter))
 		}
@@ -239,7 +239,7 @@
 		requiresMigration = true
 	}
 
-	if !configEntry.MatchesConsul(entry) {
+	if !matchesConsul {
 		if requiresMigration {
 			// If we're migrating this config entry but the custom resource
 			// doesn't match what's in Consul currently we error out so that

komapa avatar Jan 30 '24 02:01 komapa