Skip to content

Commit

Permalink
Ensure ServiceIntentions aren't resynced
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
lkysow committed Jan 6, 2021
1 parent f5c177e commit 5213926
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 1 deletion.
30 changes: 30 additions & 0 deletions api/v1alpha1/serviceintentions_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(""),
Expand All @@ -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
}),
)
}

Expand Down
43 changes: 43 additions & 0 deletions api/v1alpha1/serviceintentions_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
17 changes: 16 additions & 1 deletion controller/configentry_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package controller

import (
"context"
"encoding/json"
"fmt"
"strings"

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

0 comments on commit 5213926

Please sign in to comment.