Skip to content

Commit

Permalink
GH-3406 - Only error for config entries from different datacenters wh…
Browse files Browse the repository at this point in the history
…en the config entries are different (#3873)
  • Loading branch information
jmurret committed Apr 19, 2024
1 parent 9b2f810 commit d051cfb
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 41 deletions.
3 changes: 3 additions & 0 deletions .changelog/3873.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
ConfigEntries controller: Only error for config entries from different datacenters when the config entries are different
```
56 changes: 25 additions & 31 deletions control-plane/config-entries/controllers/configentry_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()),
})
Expand All @@ -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.
Expand All @@ -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)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package controllers

import (
"context"
"errors"
"fmt"
"testing"
"time"
Expand Down Expand Up @@ -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,
},
}

Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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())
}
}
})
}
Expand Down

0 comments on commit d051cfb

Please sign in to comment.