From 5faa4e08e8783f8f08c5a96833d0b9bf8f452889 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Tue, 16 Aug 2022 14:07:37 -0500 Subject: [PATCH] cleanup: cleanup more slice-set comparisons --- go.mod | 2 +- go.sum | 4 +- helper/funcs.go | 29 ++++++++ helper/funcs_test.go | 34 +++++++++ nomad/structs/services.go | 144 +++++--------------------------------- 5 files changed, 82 insertions(+), 131 deletions(-) diff --git a/go.mod b/go.mod index 4be6cee99..952342216 100644 --- a/go.mod +++ b/go.mod @@ -64,7 +64,7 @@ require ( github.com/hashicorp/go-plugin v1.4.3 github.com/hashicorp/go-secure-stdlib/listenerutil v0.1.4 github.com/hashicorp/go-secure-stdlib/strutil v0.1.2 - github.com/hashicorp/go-set v0.1.2 + github.com/hashicorp/go-set v0.1.4 github.com/hashicorp/go-sockaddr v1.0.2 github.com/hashicorp/go-syslog v1.0.0 github.com/hashicorp/go-uuid v1.0.2 diff --git a/go.sum b/go.sum index 8fd1e2ed1..cdb023554 100644 --- a/go.sum +++ b/go.sum @@ -760,8 +760,8 @@ github.com/hashicorp/go-secure-stdlib/strutil v0.1.2 h1:kes8mmyCpxJsI7FTwtzRqEy9 github.com/hashicorp/go-secure-stdlib/strutil v0.1.2/go.mod h1:Gou2R9+il93BqX25LAKCLuM+y9U2T4hlwvT1yprcna4= github.com/hashicorp/go-secure-stdlib/tlsutil v0.1.1 h1:Yc026VyMyIpq1UWRnakHRG01U8fJm+nEfEmjoAb00n8= github.com/hashicorp/go-secure-stdlib/tlsutil v0.1.1/go.mod h1:l8slYwnJA26yBz+ErHpp2IRCLr0vuOMGBORIz4rRiAs= -github.com/hashicorp/go-set v0.1.2 h1:WqFkeT32zKiD/l7zwO1RLF4YwctJwp6IByML0LLa0os= -github.com/hashicorp/go-set v0.1.2/go.mod h1:0jTQeDo6GKX0WMFUV4IicFkxXo9DuoRnUODngpsoYCk= +github.com/hashicorp/go-set v0.1.4 h1:LADVZzgPFLNI20TII1l75bnBI0LTnxpoCOPNjozNpi8= +github.com/hashicorp/go-set v0.1.4/go.mod h1:XFMEKCP3rGoZUBvdYwC9k2YVDj8PsMU/B0ITuYkl8IA= github.com/hashicorp/go-sockaddr v1.0.0/go.mod h1:7Xibr9yA9JjQq1JpNB2Vw7kxv8xerXegt+ozgdvDeDU= github.com/hashicorp/go-sockaddr v1.0.2 h1:ztczhD1jLxIRjVejw8gFomI1BQZOe2WoVOu0SyteCQc= github.com/hashicorp/go-sockaddr v1.0.2/go.mod h1:rB4wwRAUzs07qva3c5SdrY/NEtAUjGlgmH/UkBUC97A= diff --git a/helper/funcs.go b/helper/funcs.go index 1ab2792d1..9269bb9b9 100644 --- a/helper/funcs.go +++ b/helper/funcs.go @@ -11,6 +11,7 @@ import ( "time" multierror "github.com/hashicorp/go-multierror" + "github.com/hashicorp/go-set" "github.com/hashicorp/hcl/hcl/ast" "golang.org/x/exp/constraints" ) @@ -683,3 +684,31 @@ OUTER: } return true } + +// SliceSetEq returns true if slices a and b contain the same elements (in no +// particular order), using '==' for comparison. +// +// Note: for pointers, consider implementing an Equals method and using +// ElementsEquals instead. +func SliceSetEq[T comparable](a, b []T) bool { + lenA, lenB := len(a), len(b) + if lenA != lenB { + return false + } + + if lenA > 10 { + // avoid quadratic comparisons over large input + return set.From(a).EqualSlice(b) + } + +OUTER: + for _, item := range a { + for _, other := range b { + if item == other { + continue OUTER + } + } + return false + } + return true +} diff --git a/helper/funcs_test.go b/helper/funcs_test.go index 792bd85ea..c4d1b7b94 100644 --- a/helper/funcs_test.go +++ b/helper/funcs_test.go @@ -593,3 +593,37 @@ func Test_ElementsEquals(t *testing.T) { must.False(t, ElementsEquals(b, a)) }) } + +func Test_SliceSetEq(t *testing.T) { + t.Run("empty", func(t *testing.T) { + a := make([]int, 0) + b := make([]int, 0) + must.True(t, SliceSetEq(a, b)) + }) + + t.Run("subset small", func(t *testing.T) { + a := []int{1, 2, 3, 4, 5} + b := []int{1, 2, 3} + must.False(t, SliceSetEq(a, b)) + must.False(t, SliceSetEq(b, a)) + }) + + t.Run("subset large", func(t *testing.T) { + a := []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12} + b := []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11} + must.False(t, SliceSetEq(a, b)) + must.False(t, SliceSetEq(b, a)) + }) + + t.Run("same small", func(t *testing.T) { + a := []int{1, 2, 3, 4, 5} + b := []int{1, 2, 3, 4, 5} + must.True(t, SliceSetEq(a, b)) + }) + + t.Run("same large", func(t *testing.T) { + a := []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12} + b := []int{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12} + must.True(t, SliceSetEq(a, b)) + }) +} diff --git a/nomad/structs/services.go b/nomad/structs/services.go index 632391361..284c88319 100644 --- a/nomad/structs/services.go +++ b/nomad/structs/services.go @@ -17,6 +17,7 @@ import ( "github.com/hashicorp/consul/api" "github.com/hashicorp/go-multierror" + "github.com/hashicorp/go-set" "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/args" "github.com/hashicorp/nomad/helper/pointer" @@ -1363,29 +1364,13 @@ func (p *ConsulProxy) Copy() *ConsulProxy { return nil } - newP := &ConsulProxy{ + return &ConsulProxy{ LocalServiceAddress: p.LocalServiceAddress, LocalServicePort: p.LocalServicePort, Expose: p.Expose.Copy(), + Upstreams: slices.Clone(p.Upstreams), + Config: helper.CopyMap(p.Config), } - - if n := len(p.Upstreams); n > 0 { - newP.Upstreams = make([]ConsulUpstream, n) - - for i := range p.Upstreams { - newP.Upstreams[i] = *p.Upstreams[i].Copy() - } - } - - if n := len(p.Config); n > 0 { - newP.Config = make(map[string]interface{}, n) - - for k, v := range p.Config { - newP.Config[k] = v - } - } - - return newP } // opaqueMapsEqual compares map[string]interface{} commonly used for opaque @@ -1492,61 +1477,16 @@ type ConsulUpstream struct { MeshGateway ConsulMeshGateway } -func upstreamsEquals(a, b []ConsulUpstream) bool { - if len(a) != len(b) { - return false - } - -LOOP: // order does not matter - for _, upA := range a { - for _, upB := range b { - if upA.Equals(&upB) { - continue LOOP - } - } - return false - } - return true -} - -// Copy the stanza recursively. Returns nil if u is nil. -func (u *ConsulUpstream) Copy() *ConsulUpstream { - if u == nil { - return nil - } - - return &ConsulUpstream{ - DestinationName: u.DestinationName, - DestinationNamespace: u.DestinationNamespace, - LocalBindPort: u.LocalBindPort, - Datacenter: u.Datacenter, - LocalBindAddress: u.LocalBindAddress, - MeshGateway: u.MeshGateway.Copy(), - } -} - // Equals returns true if the structs are recursively equal. func (u *ConsulUpstream) Equals(o *ConsulUpstream) bool { if u == nil || o == nil { return u == o } + return *u == *o +} - switch { - case u.DestinationName != o.DestinationName: - return false - case u.DestinationNamespace != o.DestinationNamespace: - return false - case u.LocalBindPort != o.LocalBindPort: - return false - case u.Datacenter != o.Datacenter: - return false - case u.LocalBindAddress != o.LocalBindAddress: - return false - case !u.MeshGateway.Equals(o.MeshGateway): - return false - } - - return true +func upstreamsEquals(a, b []ConsulUpstream) bool { + return set.From(a).Equal(set.From(b)) } // ConsulExposeConfig represents a Consul Connect expose jobspec stanza. @@ -1562,21 +1502,8 @@ type ConsulExposePath struct { ListenerPort string } -func exposePathsEqual(pathsA, pathsB []ConsulExposePath) bool { - if len(pathsA) != len(pathsB) { - return false - } - -LOOP: // order does not matter - for _, pathA := range pathsA { - for _, pathB := range pathsB { - if pathA == pathB { - continue LOOP - } - } - return false - } - return true +func exposePathsEqual(a, b []ConsulExposePath) bool { + return helper.SliceSetEq(a, b) } // Copy the stanza. Returns nil if e is nil. @@ -2041,21 +1968,8 @@ func (l *ConsulIngressListener) Validate() error { return nil } -func ingressServicesEqual(servicesA, servicesB []*ConsulIngressService) bool { - if len(servicesA) != len(servicesB) { - return false - } - -COMPARE: // order does not matter - for _, serviceA := range servicesA { - for _, serviceB := range servicesB { - if serviceA.Equals(serviceB) { - continue COMPARE - } - } - return false - } - return true +func ingressServicesEqual(a, b []*ConsulIngressService) bool { + return helper.ElementsEquals(a, b) } // ConsulIngressConfigEntry represents the Consul Configuration Entry type for @@ -2116,21 +2030,8 @@ func (e *ConsulIngressConfigEntry) Validate() error { return nil } -func ingressListenersEqual(listenersA, listenersB []*ConsulIngressListener) bool { - if len(listenersA) != len(listenersB) { - return false - } - -COMPARE: // order does not matter - for _, listenerA := range listenersA { - for _, listenerB := range listenersB { - if listenerA.Equals(listenerB) { - continue COMPARE - } - } - return false - } - return true +func ingressListenersEqual(a, b []*ConsulIngressListener) bool { + return helper.ElementsEquals(a, b) } type ConsulLinkedService struct { @@ -2205,21 +2106,8 @@ func (s *ConsulLinkedService) Validate() error { return nil } -func linkedServicesEqual(servicesA, servicesB []*ConsulLinkedService) bool { - if len(servicesA) != len(servicesB) { - return false - } - -COMPARE: // order does not matter - for _, serviceA := range servicesA { - for _, serviceB := range servicesB { - if serviceA.Equals(serviceB) { - continue COMPARE - } - } - return false - } - return true +func linkedServicesEqual(a, b []*ConsulLinkedService) bool { + return helper.ElementsEquals(a, b) } type ConsulTerminatingConfigEntry struct {