Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport of GH-3406 - Only error for config entries from different datacenters when the config entries are different into release/1.1.x #3929

Merged
merged 1 commit into from
Apr 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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/controller/configentry_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,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 @@ -221,37 +221,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 @@ -261,7 +255,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 @@ -275,7 +269,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
55 changes: 45 additions & 10 deletions control-plane/controller/configentry_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package controller

import (
"context"
"errors"
"fmt"
"testing"
"time"
Expand Down Expand Up @@ -1494,16 +1495,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 @@ -1525,6 +1547,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 @@ -1560,7 +1587,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 @@ -1572,9 +1599,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
Loading