From 521392694ed9d07a17fa6b803112e8f6413cfcba Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Wed, 6 Jan 2021 13:56:26 -0800 Subject: [PATCH] Ensure ServiceIntentions aren't resynced Consul will internally sort the array of ServiceIntention sources by precedence. For example, if it receives: ``` sources: - name: "*" action: allow - name: bar action: allow ``` It will return it in precedence sorted order: ``` sources: - name: bar action: allow - name: "*" action: allow ``` This sorting breaks our MatchesConsul function because we think the resource has changed and so we will update it continually since it will never match. This change uses gocmp's SortSlices to re-sort the sources array so that both the Kube resource and the Consul resource can be compared despite Consul's sorting. We use a JSON encoded representation of the source element with the fields that Consul set automatically zero'd out. --- api/v1alpha1/serviceintentions_types.go | 30 ++++++++++++++ api/v1alpha1/serviceintentions_types_test.go | 43 ++++++++++++++++++++ controller/configentry_controller.go | 17 +++++++- 3 files changed, 89 insertions(+), 1 deletion(-) diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go index 8ec94cd456..5d9f03dfe3 100644 --- a/api/v1alpha1/serviceintentions_types.go +++ b/api/v1alpha1/serviceintentions_types.go @@ -3,6 +3,7 @@ package v1alpha1 import ( "encoding/json" "net/http" + "strings" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -222,6 +223,28 @@ func (in *ServiceIntentions) MatchesConsul(candidate api.ConfigEntry) bool { if !ok { return false } + + // sourceSortKey returns a string that can be used to sort intention + // sources. + sourceSortKey := func(ixn *capi.SourceIntention) string { + if ixn == nil { + return "" + } + + // Zero out fields Consul sets automatically because the Kube resource + // won't have them set. + ixn.LegacyCreateTime = nil + ixn.LegacyUpdateTime = nil + ixn.LegacyID = "" + ixn.LegacyMeta = nil + ixn.Precedence = 0 + + // It's okay to swallow this error because we know the intention is JSON + // marshal-able since it was ingested as JSON. + asJSON, _ := json.Marshal(ixn) + return string(asJSON) + } + // No datacenter is passed to ToConsul as we ignore the Meta field when checking for equality. return cmp.Equal( in.ToConsul(""), @@ -230,6 +253,13 @@ func (in *ServiceIntentions) MatchesConsul(candidate api.ConfigEntry) bool { cmpopts.IgnoreFields(capi.SourceIntention{}, "LegacyID", "LegacyMeta", "LegacyCreateTime", "LegacyUpdateTime", "Precedence", "Type"), cmpopts.IgnoreUnexported(), cmpopts.EquateEmpty(), + // Consul will sort the sources by precedence when returning the resource + // so we need to re-sort the sources to ensure our comparison is accurate. + cmpopts.SortSlices(func(a *capi.SourceIntention, b *capi.SourceIntention) bool { + // SortSlices expects a "less than" comparator function so we can + // piggyback on strings.Compare that returns -1 if a < b. + return strings.Compare(sourceSortKey(a), sourceSortKey(b)) == -1 + }), ) } diff --git a/api/v1alpha1/serviceintentions_types_test.go b/api/v1alpha1/serviceintentions_types_test.go index a37d848f3b..fc563a9c38 100644 --- a/api/v1alpha1/serviceintentions_types_test.go +++ b/api/v1alpha1/serviceintentions_types_test.go @@ -162,6 +162,49 @@ func TestServiceIntentions_MatchesConsul(t *testing.T) { }, Matches: false, }, + "different order of sources matches": { + Ours: ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "bar", + }, + Sources: SourceIntentions{ + { + Name: "*", + Action: "allow", + }, + { + Name: "foo", + Action: "allow", + }, + }, + }, + }, + Theirs: &capi.ServiceIntentionsConfigEntry{ + Name: "bar", + Kind: capi.ServiceIntentions, + CreateIndex: 1, + ModifyIndex: 2, + Meta: map[string]string{ + common.SourceKey: common.SourceValue, + common.DatacenterKey: "datacenter", + }, + Sources: []*capi.SourceIntention{ + { + Name: "foo", + Action: "allow", + }, + { + Name: "*", + Action: "allow", + }, + }, + }, + Matches: true, + }, } for name, c := range cases { t.Run(name, func(t *testing.T) { diff --git a/controller/configentry_controller.go b/controller/configentry_controller.go index 6298099b5c..65277b5221 100644 --- a/controller/configentry_controller.go +++ b/controller/configentry_controller.go @@ -2,6 +2,7 @@ package controller import ( "context" + "encoding/json" "fmt" "strings" @@ -21,6 +22,7 @@ const ( FinalizerName = "finalizers.consul.hashicorp.com" ConsulAgentError = "ConsulAgentError" ExternallyManagedConfigError = "ExternallyManagedConfigError" + MigrationFailedError = "MigrationFailedError" ) // Controller is implemented by CRD-specific controllers. It is used by @@ -191,11 +193,24 @@ func (r *ConfigEntryController) ReconcileEntry( // 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 entry.GetMeta()[common.DatacenterKey] != r.DatacenterName { + // Note that there is a special case where we will "adopt" a config entry + // that wasn't created by the controller if it has the adopt 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 entry.GetMeta()[common.DatacenterKey] != r.DatacenterName && configEntry.GetObjectMeta().Annotations[common.MigrateExistingKey] != "true" { return r.syncFailed(ctx, logger, crdCtrl, configEntry, ExternallyManagedConfigError, fmt.Errorf("config entry managed in different datacenter: %q", entry.GetMeta()[common.DatacenterKey])) } if !configEntry.MatchesConsul(entry) { + // Check if we are adopting this config entry. If so, we want to ensure + // that the custom resource matches what's in Consul currently. + if entry.GetMeta()[common.DatacenterKey] != r.DatacenterName && configEntry.GetObjectMeta().Annotations[common.MigrateExistingKey] == "true" { + kubeJSON, _ := json.Marshal(configEntry.ToConsul(r.DatacenterName)) + consulJSON, _ := json.Marshal(entry) + return r.syncFailed(ctx, logger, crdCtrl, configEntry, MigrationFailedError, fmt.Errorf("migration failed because spec of this resource does not match existing Consul config entry: consul=%s, resource=%s", consulJSON, kubeJSON)) + } + logger.Info("config entry does not match consul", "modify-index", entry.GetModifyIndex()) _, writeMeta, err := r.ConsulClient.ConfigEntries().Set(consulEntry, &capi.WriteOptions{ Namespace: r.consulNamespace(consulEntry, configEntry.ConsulMirroringNS(), configEntry.ConsulGlobalResource()),