From 45f0afcbf4e53b58da38c4933431c5c4fb41ce63 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 11 Dec 2020 16:10:00 -0500 Subject: [PATCH 1/2] structs: Fix printing of IDs These types are used as values (not pointers) in other structs. Using a pointer receiver causes problems when the value is printed. fmt will not call the String method if it is passed a value and the String method has a pointer receiver. By using a value receiver the correct string is printed. Also remove some unused methods. --- agent/agent_test.go | 3 +- agent/checks/alias.go | 6 ++-- agent/consul/discoverychain/compile.go | 7 ++-- agent/consul/state/config_entry.go | 5 +-- agent/local/state.go | 7 ++-- agent/structs/config_entry.go | 11 +++--- agent/structs/config_entry_gateways.go | 7 ++-- agent/structs/structs.go | 46 ++++---------------------- agent/structs/structs_oss.go | 6 ++-- 9 files changed, 33 insertions(+), 65 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index 1fe00626f..2a6c42df3 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -742,8 +742,7 @@ func TestAgent_CheckAliasRPC(t *testing.T) { foundService := false lookup := structs.NewServiceID("svcid1", structs.WildcardEnterpriseMeta()) for _, srv := range out.NodeServices.Services { - sid := srv.CompoundServiceID() - if lookup.Matches(&sid) { + if lookup.Matches(srv.CompoundServiceID()) { foundService = true } } diff --git a/agent/checks/alias.go b/agent/checks/alias.go index cb41a586c..af6155517 100644 --- a/agent/checks/alias.go +++ b/agent/checks/alias.go @@ -156,8 +156,7 @@ RETRY_CALL: return false, err } for _, srv := range out.NodeServices.Services { - sid := srv.CompoundServiceID() - if serviceID.Matches(&sid) { + if serviceID.Matches(srv.CompoundServiceID()) { return true, nil } } @@ -250,8 +249,7 @@ func (c *CheckAlias) processChecks(checks []*structs.HealthCheck, CheckIfService if c.Node != "" && c.Node != chk.Node { continue } - sid := chk.CompoundServiceID() - serviceMatch := c.ServiceID.Matches(&sid) + serviceMatch := c.ServiceID.Matches(chk.CompoundServiceID()) if chk.ServiceID != "" && !serviceMatch { continue } diff --git a/agent/consul/discoverychain/compile.go b/agent/consul/discoverychain/compile.go index d305e5da4..4c5f20278 100644 --- a/agent/consul/discoverychain/compile.go +++ b/agent/consul/discoverychain/compile.go @@ -5,10 +5,11 @@ import ( "strings" "time" - "github.com/hashicorp/consul/agent/connect" - "github.com/hashicorp/consul/agent/structs" "github.com/mitchellh/hashstructure" "github.com/mitchellh/mapstructure" + + "github.com/hashicorp/consul/agent/connect" + "github.com/hashicorp/consul/agent/structs" ) type CompileRequest struct { @@ -721,7 +722,7 @@ func (c *compiler) getSplitterNode(sid structs.ServiceID) (*structs.DiscoveryGra } // Check to see if the split is eligible for additional splitting. - if !splitID.Matches(&sid) && split.ServiceSubset == "" { + if !splitID.Matches(sid) && split.ServiceSubset == "" { nextNode, err := c.getSplitterNode(splitID) if err != nil { return nil, err diff --git a/agent/consul/state/config_entry.go b/agent/consul/state/config_entry.go index 8df8f1876..68cbbd73c 100644 --- a/agent/consul/state/config_entry.go +++ b/agent/consul/state/config_entry.go @@ -4,11 +4,12 @@ import ( "errors" "fmt" + memdb "github.com/hashicorp/go-memdb" + "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/consul/discoverychain" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/lib" - memdb "github.com/hashicorp/go-memdb" ) const ( @@ -473,7 +474,7 @@ func (s *Store) discoveryChainSourcesTxn(tx ReadTxn, ws memdb.WatchSet, dc strin em := structs.EnterpriseMetaInitializer(t.Namespace) candidate := structs.NewServiceName(t.Service, &em) - if !candidate.Matches(&destination) { + if !candidate.Matches(destination) { continue } if idx > maxIdx { diff --git a/agent/local/state.go b/agent/local/state.go index 5851648f9..6f0b779fc 100644 --- a/agent/local/state.go +++ b/agent/local/state.go @@ -687,7 +687,7 @@ func (l *State) ChecksForService(serviceID structs.ServiceID, includeNodeChecks if c.Check.ServiceID != "" { sid := c.Check.CompoundServiceID() - if !serviceID.Matches(&sid) { + if !serviceID.Matches(sid) { continue } } else if !includeNodeChecks { @@ -1150,7 +1150,7 @@ func (l *State) deleteService(key structs.ServiceID) error { for _, c := range l.checks { if c.Deleted && c.Check != nil { sid := c.Check.CompoundServiceID() - if sid.Matches(&key) { + if sid.Matches(key) { l.pruneCheck(c.Check.CompoundCheckID()) } } @@ -1239,8 +1239,7 @@ func (l *State) syncService(key structs.ServiceID) error { if c.Deleted || c.InSync { continue } - sid := c.Check.CompoundServiceID() - if !key.Matches(&sid) { + if !key.Matches(c.Check.CompoundServiceID()) { continue } if st != l.checkToken(checkKey) { diff --git a/agent/structs/config_entry.go b/agent/structs/config_entry.go index 8a343ca48..268c50405 100644 --- a/agent/structs/config_entry.go +++ b/agent/structs/config_entry.go @@ -6,14 +6,15 @@ import ( "strings" "time" - "github.com/hashicorp/consul/acl" - "github.com/hashicorp/consul/agent/cache" - "github.com/hashicorp/consul/lib" - "github.com/hashicorp/consul/lib/decode" "github.com/hashicorp/go-msgpack/codec" "github.com/hashicorp/go-multierror" "github.com/mitchellh/hashstructure" "github.com/mitchellh/mapstructure" + + "github.com/hashicorp/consul/acl" + "github.com/hashicorp/consul/agent/cache" + "github.com/hashicorp/consul/lib" + "github.com/hashicorp/consul/lib/decode" ) const ( @@ -599,7 +600,7 @@ type UpstreamConfigs []UpstreamConfig func (configs UpstreamConfigs) GetUpstreamConfig(sid ServiceID) (config map[string]interface{}, found bool) { for _, usconf := range configs { - if usconf.Upstream.Matches(&sid) { + if usconf.Upstream.Matches(sid) { return usconf.Config, true } } diff --git a/agent/structs/config_entry_gateways.go b/agent/structs/config_entry_gateways.go index 61b993082..d3b9e302a 100644 --- a/agent/structs/config_entry_gateways.go +++ b/agent/structs/config_entry_gateways.go @@ -5,9 +5,10 @@ import ( "sort" "strings" + "github.com/miekg/dns" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/lib/stringslice" - "github.com/miekg/dns" ) // IngressGatewayConfigEntry manages the configuration for an ingress service @@ -460,8 +461,8 @@ func (g *GatewayService) Addresses(defaultHosts []string) []string { } func (g *GatewayService) IsSame(o *GatewayService) bool { - return g.Gateway.Matches(&o.Gateway) && - g.Service.Matches(&o.Service) && + return g.Gateway.Matches(o.Gateway) && + g.Service.Matches(o.Service) && g.GatewayKind == o.GatewayKind && g.Port == o.Port && g.Protocol == o.Protocol && diff --git a/agent/structs/structs.go b/agent/structs/structs.go index 5e0956a06..389ee8153 100644 --- a/agent/structs/structs.go +++ b/agent/structs/structs.go @@ -1754,7 +1754,7 @@ func NewCheckID(id types.CheckID, entMeta *EnterpriseMeta) CheckID { // StringHash is used mainly to populate part of the filename of a check // definition persisted on the local agent -func (cid *CheckID) StringHash() string { +func (cid CheckID) StringHash() string { hasher := md5.New() hasher.Write([]byte(cid.ID)) cid.EnterpriseMeta.addToHash(hasher, true) @@ -1778,35 +1778,19 @@ func NewServiceID(id string, entMeta *EnterpriseMeta) ServiceID { return sid } -func (sid *ServiceID) Matches(other *ServiceID) bool { - if sid == nil && other == nil { - return true - } - - if sid == nil || other == nil || sid.ID != other.ID || !sid.EnterpriseMeta.Matches(&other.EnterpriseMeta) { - return false - } - - return true +func (sid ServiceID) Matches(other ServiceID) bool { + return sid.ID == other.ID && sid.EnterpriseMeta.Matches(&other.EnterpriseMeta) } // StringHash is used mainly to populate part of the filename of a service // definition persisted on the local agent -func (sid *ServiceID) StringHash() string { +func (sid ServiceID) StringHash() string { hasher := md5.New() hasher.Write([]byte(sid.ID)) sid.EnterpriseMeta.addToHash(hasher, true) return fmt.Sprintf("%x", hasher.Sum(nil)) } -func (sid *ServiceID) LessThan(other *ServiceID) bool { - if sid.EnterpriseMeta.LessThan(&other.EnterpriseMeta) { - return true - } - - return sid.ID < other.ID -} - type IndexedNodes struct { Nodes Nodes QueryMeta @@ -1837,30 +1821,14 @@ func NewServiceName(name string, entMeta *EnterpriseMeta) ServiceName { return ret } -func (n *ServiceName) Matches(o *ServiceName) bool { - if n == nil && o == nil { - return true - } - - if n == nil || o == nil || n.Name != o.Name || !n.EnterpriseMeta.Matches(&o.EnterpriseMeta) { - return false - } - - return true +func (n ServiceName) Matches(o ServiceName) bool { + return n.Name == o.Name && n.EnterpriseMeta.Matches(&o.EnterpriseMeta) } -func (n *ServiceName) ToServiceID() ServiceID { +func (n ServiceName) ToServiceID() ServiceID { return ServiceID{ID: n.Name, EnterpriseMeta: n.EnterpriseMeta} } -func (n *ServiceName) LessThan(other *ServiceName) bool { - if n.EnterpriseMeta.LessThan(&other.EnterpriseMeta) { - return true - } - - return n.Name < other.Name -} - type ServiceList []ServiceName type IndexedServiceList struct { diff --git a/agent/structs/structs_oss.go b/agent/structs/structs_oss.go index 29a45520b..3e972bae2 100644 --- a/agent/structs/structs_oss.go +++ b/agent/structs/structs_oss.go @@ -106,7 +106,7 @@ func ParseServiceIDString(input string) (string, *EnterpriseMeta) { return input, DefaultEnterpriseMeta() } -func (sid *ServiceID) String() string { +func (sid ServiceID) String() string { return sid.ID } @@ -119,7 +119,7 @@ func ParseServiceNameString(input string) (string, *EnterpriseMeta) { return input, DefaultEnterpriseMeta() } -func (n *ServiceName) String() string { +func (n ServiceName) String() string { return n.Name } @@ -128,7 +128,7 @@ func ServiceNameFromString(input string) ServiceName { return ServiceName{Name: id} } -func (cid *CheckID) String() string { +func (cid CheckID) String() string { return string(cid.ID) } From 31b1addd9ebe0a4d84fdfb6b5fd5902be44463a0 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 7 Jan 2021 18:47:25 -0500 Subject: [PATCH 2/2] structs: add tests for String() methods To show that printing one of these IDs works properly now that the String() method receiver is no longer a pointer. --- agent/structs/structs_oss_test.go | 41 +++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 agent/structs/structs_oss_test.go diff --git a/agent/structs/structs_oss_test.go b/agent/structs/structs_oss_test.go new file mode 100644 index 000000000..402122994 --- /dev/null +++ b/agent/structs/structs_oss_test.go @@ -0,0 +1,41 @@ +package structs + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestServiceID_String(t *testing.T) { + t.Run("value", func(t *testing.T) { + sid := NewServiceID("the-id", &EnterpriseMeta{}) + require.Equal(t, "the-id", fmt.Sprintf("%v", sid)) + }) + t.Run("pointer", func(t *testing.T) { + sid := NewServiceID("the-id", &EnterpriseMeta{}) + require.Equal(t, "the-id", fmt.Sprintf("%v", &sid)) + }) +} + +func TestCheckID_String(t *testing.T) { + t.Run("value", func(t *testing.T) { + cid := NewCheckID("the-id", &EnterpriseMeta{}) + require.Equal(t, "the-id", fmt.Sprintf("%v", cid)) + }) + t.Run("pointer", func(t *testing.T) { + cid := NewCheckID("the-id", &EnterpriseMeta{}) + require.Equal(t, "the-id", fmt.Sprintf("%v", &cid)) + }) +} + +func TestServiceName_String(t *testing.T) { + t.Run("value", func(t *testing.T) { + sn := NewServiceName("the-id", &EnterpriseMeta{}) + require.Equal(t, "the-id", fmt.Sprintf("%v", sn)) + }) + t.Run("pointer", func(t *testing.T) { + sn := NewServiceName("the-id", &EnterpriseMeta{}) + require.Equal(t, "the-id", fmt.Sprintf("%v", &sn)) + }) +}