GH-3406 - Only error for config entries from different datacenters wh…

…en the config entries are different (#3873) * GH-3406 - Only error for config entries from different datacenters when the config entries are different * add changelog * fixing tests and logic * refactoring code to make tests pass and also use a switch statement for readability and also get rid of intermediate state flag of requireMigration in a long iterative section of code.
hashicorp · Apr 19, 2024 · 40a7fad · 40a7fad
1 parent c29effd
commit 40a7fad
Show file tree

Hide file tree

Showing 3 changed files with 73 additions and 41 deletions.
diff --git a/.changelog/3873.txt b/.changelog/3873.txt
@@ -0,0 +1,3 @@
+```release-note:improvement
+ConfigEntries controller: Only error for config entries from different datacenters when the config entries are different
+```
diff --git a/control-plane/controllers/configentries/configentry_controller.go b/control-plane/controllers/configentries/configentry_controller.go
@@ -183,7 +183,7 @@ func (r *ConfigEntryController) ReconcileEntry(ctx context.Context, crdCtrl Cont
 	}
 
 	// Check to see if consul has config entry with the same name
-	entry, _, err := consulClient.ConfigEntries().Get(configEntry.ConsulKind(), configEntry.ConsulName(), &capi.QueryOptions{
+	entryFromConsul, _, err := consulClient.ConfigEntries().Get(configEntry.ConsulKind(), configEntry.ConsulName(), &capi.QueryOptions{
 		Namespace: r.consulNamespace(consulEntry, configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()),
 	})
 	// If a config entry with this name does not exist
@@ -223,37 +223,31 @@ func (r *ConfigEntryController) ReconcileEntry(ctx context.Context, crdCtrl Cont
 		return r.syncFailed(ctx, logger, crdCtrl, configEntry, ConsulAgentError, err)
 	}
 
-	requiresMigration := false
-	sourceDatacenter := entry.GetMeta()[common.DatacenterKey]
-
+	sourceDatacenter := entryFromConsul.GetMeta()[common.DatacenterKey]
+	managedByThisDC := sourceDatacenter == r.DatacenterName
 	// 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))
-		}
-
-		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
-			// it doesn't overwrite something accidentally.
-			return r.syncFailed(ctx, logger, crdCtrl, configEntry, MigrationFailedError,
-				r.nonMatchingMigrationError(configEntry, entry))
-		}
-
-		logger.Info("config entry does not match consul", "modify-index", entry.GetModifyIndex())
+	matchesConsul := configEntry.MatchesConsul(entryFromConsul)
+	// 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.
+	hasMigrationKey := configEntry.GetObjectMeta().Annotations[common.MigrateEntryKey] == common.MigrateEntryTrue
+
+	switch {
+	case !matchesConsul && !managedByThisDC && !hasMigrationKey:
+		return r.syncFailed(ctx, logger, crdCtrl, configEntry, ExternallyManagedConfigError,
+			sourceDatacenterMismatchErr(sourceDatacenter))
+	case !matchesConsul && hasMigrationKey:
+		// If we're migrating this config entry but the custom resource
+		// doesn't match what's in Consul currently we error out so that
+		// it doesn't overwrite something accidentally.
+		return r.syncFailed(ctx, logger, crdCtrl, configEntry, MigrationFailedError,
+			r.nonMatchingMigrationError(configEntry, entryFromConsul))
+	case !matchesConsul:
+		logger.Info("config entry does not match consul", "modify-index", entryFromConsul.GetModifyIndex())
 		_, writeMeta, err := consulClient.ConfigEntries().Set(consulEntry, &capi.WriteOptions{
 			Namespace: r.consulNamespace(consulEntry, configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()),
 		})
@@ -263,7 +257,7 @@ func (r *ConfigEntryController) ReconcileEntry(ctx context.Context, crdCtrl Cont
 		}
 		logger.Info("config entry updated", "request-time", writeMeta.RequestTime)
 		return r.syncSuccessful(ctx, crdCtrl, configEntry)
-	} else if requiresMigration && entry.GetMeta()[common.DatacenterKey] != r.DatacenterName {
+	case hasMigrationKey && !managedByThisDC:
 		// If we get here then we're doing a migration and the entry in Consul
 		// matches the entry in Kubernetes. We just need to update the metadata
 		// of the entry in Consul to say that it's now managed by Kubernetes.
@@ -277,7 +271,7 @@ func (r *ConfigEntryController) ReconcileEntry(ctx context.Context, crdCtrl Cont
 		}
 		logger.Info("config entry migrated", "request-time", writeMeta.RequestTime)
 		return r.syncSuccessful(ctx, crdCtrl, configEntry)
-	} else if configEntry.SyncedConditionStatus() != corev1.ConditionTrue {
+	case configEntry.SyncedConditionStatus() != corev1.ConditionTrue:
 		return r.syncSuccessful(ctx, crdCtrl, configEntry)
 	}
 

diff --git a/control-plane/controllers/configentries/configentry_controller_test.go b/control-plane/controllers/configentries/configentry_controller_test.go
@@ -5,6 +5,7 @@ package configentries
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"testing"
 	"time"
@@ -1683,16 +1684,37 @@ func TestConfigEntryControllers_doesNotCreateUnownedConfigEntry(t *testing.T) {
 	kubeNS := "default"
 
 	cases := []struct {
-		datacenterAnnotation string
-		expErr               string
+		name                    string
+		datacenterAnnotation    string
+		expErr                  error
+		expReason               string
+		makeDifferentFromConsul bool
 	}{
 		{
-			datacenterAnnotation: "",
-			expErr:               "config entry already exists in Consul",
+			name:                    "when dc annotation is blank and the config entry does not match consul, then error is thrown, entry is not synced and reason is it is externally managed.",
+			datacenterAnnotation:    "",
+			makeDifferentFromConsul: true,
+			expErr:                  errors.New("config entry already exists in Consul"),
+			expReason:               "ExternallyManagedConfigError",
 		},
 		{
-			datacenterAnnotation: "other-datacenter",
-			expErr:               "config entry managed in different datacenter: \"other-datacenter\"",
+			name:                    "when dc annotation is not blank and the config entry matches consul, then error is not thrown and it is marked as synced",
+			datacenterAnnotation:    "",
+			makeDifferentFromConsul: false,
+			expErr:                  nil,
+		},
+		{
+			name:                    "when dc annotation is not blank and the config entry does not match consul, then error is thrown, entry is not synced and reason is it is externally managed.",
+			datacenterAnnotation:    "other-datacenter",
+			makeDifferentFromConsul: true,
+			expErr:                  errors.New("config entry managed in different datacenter: \"other-datacenter\""),
+			expReason:               "ExternallyManagedConfigError",
+		},
+		{
+			name:                    "when dc annotation is not blank and the config entry matches consul, then error is not thrown and it is marked as synced",
+			datacenterAnnotation:    "other-datacenter",
+			makeDifferentFromConsul: false,
+			expErr:                  nil,
 		},
 	}
 
@@ -1714,6 +1736,11 @@ func TestConfigEntryControllers_doesNotCreateUnownedConfigEntry(t *testing.T) {
 			s.AddKnownTypes(v1alpha1.GroupVersion, svcDefaults)
 			fakeClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(svcDefaults).Build()
 
+			// Change the config entry so protocol is https instead of http if test case says to
+			if c.makeDifferentFromConsul {
+				svcDefaults.Spec.Protocol = "https"
+			}
+
 			testClient := test.TestServerWithMockConnMgrWatcher(t, nil)
 			testClient.TestServer.WaitForServiceIntentions(t)
 			consulClient := testClient.APIClient
@@ -1749,7 +1776,7 @@ func TestConfigEntryControllers_doesNotCreateUnownedConfigEntry(t *testing.T) {
 				resp, err := reconciler.Reconcile(ctx, ctrl.Request{
 					NamespacedName: namespacedName,
 				})
-				req.EqualError(err, c.expErr)
+				req.Equal(err, c.expErr)
 				req.False(resp.Requeue)
 
 				// Now check that the object in Consul is as expected.
@@ -1761,9 +1788,17 @@ func TestConfigEntryControllers_doesNotCreateUnownedConfigEntry(t *testing.T) {
 				err = fakeClient.Get(ctx, namespacedName, svcDefaults)
 				req.NoError(err)
 				status, reason, errMsg := svcDefaults.SyncedCondition()
-				req.Equal(corev1.ConditionFalse, status)
-				req.Equal("ExternallyManagedConfigError", reason)
-				req.Equal(errMsg, c.expErr)
+				expectedStatus := corev1.ConditionFalse
+				if !c.makeDifferentFromConsul {
+					expectedStatus = corev1.ConditionTrue
+				}
+				req.Equal(expectedStatus, status)
+				if !c.makeDifferentFromConsul {
+					req.Equal(c.expReason, reason)
+				}
+				if c.expErr != nil {
+					req.Equal(errMsg, c.expErr.Error())
+				}
 			}
 		})
 	}