intentions: fix a bug in Intention.SetHash

Found using staticcheck.

binary.Write does not accept int types without a size. The error from binary.Write was ignored, so we never saw this error. Casting the data to uint64 produces a correct hash.

Also deprecate the Default{Addr,Port} fields, and prevent them from being encoded. These fields will always be empty and are not used.
Removing these would break backwards compatibility, so they are left in place for now.

Co-authored-by: Hans Hasselberg <[email protected]>

agent/consul/intention_endpoint.go

@@ -86,7 +86,7 @@ func (s *Intention) prepareApplyCreate(ident structs.ACLIdentity, authz acl.Auth
 	}
 
 	// make sure we set the hash prior to raft application
-	args.Intention.SetHash(true)
+	args.Intention.SetHash()
 
 	return nil
 }
@@ -145,7 +145,7 @@ func (s *Intention) prepareApplyUpdate(ident structs.ACLIdentity, authz acl.Auth
 	}
 
 	// make sure we set the hash prior to raft application
-	args.Intention.SetHash(true)
+	args.Intention.SetHash()
 
 	return nil
 }

agent/http_decode_test.go

@@ -1996,8 +1996,6 @@ func TestDecodeCatalogRegister(t *testing.T) {
 //     DestinationName	string
 //     SourceType	structs.IntentionSourceType
 //     Action	structs.IntentionAction
-//     DefaultAddr	string
-//     DefaultPort	int
 //     Meta	map[string]string
 //     Precedence	int
 //     CreatedAt	time.Time	mapstructure:'-'

agent/structs/intention.go

@@ -55,10 +55,12 @@ type Intention struct {
 	// Action is whether this is an allowlist or denylist intention.
 	Action IntentionAction
 
-	// DefaultAddr, DefaultPort of the local listening proxy (if any) to
-	// make this connection.
-	DefaultAddr string
-	DefaultPort int
+	// DefaultAddr is not used.
+	// Deprecated: DefaultAddr is not used and may be removed in a future version.
+	DefaultAddr string `bexpr:"-" codec:",omitempty"`
+	// DefaultPort is not used.
+	// Deprecated: DefaultPort is not used and may be removed in a future version.
+	DefaultPort int `bexpr:"-" codec:",omitempty"`
 
 	// Meta is arbitrary metadata associated with the intention. This is
 	// opaque to Consul but is served in API responses.
@@ -103,55 +105,47 @@ func (t *Intention) UnmarshalJSON(data []byte) (err error) {
 	return nil
 }
 
-// nolint: staticcheck // should be fixed in https://github.com/hashicorp/consul/pull/7900
-func (x *Intention) SetHash(force bool) []byte {
-	if force || x.Hash == nil {
-		hash, err := blake2b.New256(nil)
-		if err != nil {
-			panic(err)
-		}
-
-		// Any non-immutable "content" fields should be involved with the
-		// overall hash. The IDs are immutable which is why they aren't here.
-		// The raft indices are metadata similar to the hash which is why they
-		// aren't incorporated. CreateTime is similarly immutable
-		//
-		// The Hash is really only used for replication to determine if a token
-		// has changed and should be updated locally.
-
-		// Write all the user set fields
-		hash.Write([]byte(x.ID))
-		hash.Write([]byte(x.Description))
-		hash.Write([]byte(x.SourceNS))
-		hash.Write([]byte(x.SourceName))
-		hash.Write([]byte(x.DestinationNS))
-		hash.Write([]byte(x.DestinationName))
-		hash.Write([]byte(x.SourceType))
-		hash.Write([]byte(x.Action))
-		hash.Write([]byte(x.DefaultAddr))
-		binary.Write(hash, binary.LittleEndian, x.DefaultPort)
-		binary.Write(hash, binary.LittleEndian, x.Precedence)
-
-		// hashing the metadata
-		var keys []string
-		for k := range x.Meta {
-			keys = append(keys, k)
-		}
-
-		// keep them sorted to ensure hash stability
-		sort.Strings(keys)
-
-		for _, k := range keys {
-			hash.Write([]byte(k))
-			hash.Write([]byte(x.Meta[k]))
-		}
-
-		// Finalize the hash
-		hashVal := hash.Sum(nil)
-
-		x.Hash = hashVal
-	}
-	return x.Hash
+// SetHash calculates Intention.Hash from any mutable "content" fields.
+//
+// The Hash is primarily used for replication to determine if a token
+// has changed and should be updated locally.
+//
+// TODO: move to agent/consul where it is called
+func (x *Intention) SetHash() {
+	hash, err := blake2b.New256(nil)
+	if err != nil {
+		panic(err)
+	}
+
+	// Write all the user set fields
+	hash.Write([]byte(x.ID))
+	hash.Write([]byte(x.Description))
+	hash.Write([]byte(x.SourceNS))
+	hash.Write([]byte(x.SourceName))
+	hash.Write([]byte(x.DestinationNS))
+	hash.Write([]byte(x.DestinationName))
+	hash.Write([]byte(x.SourceType))
+	hash.Write([]byte(x.Action))
+	// hash.Write can not return an error, so the only way for binary.Write to
+	// error is to pass it data with an invalid data type. Doing so would be a
+	// programming error, so panic in that case.
+	if err := binary.Write(hash, binary.LittleEndian, uint64(x.Precedence)); err != nil {
+		panic(err)
+	}
+
+	// sort keys to ensure hash stability when meta is stored later
+	var keys []string
+	for k := range x.Meta {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+
+	for _, k := range keys {
+		hash.Write([]byte(k))
+		hash.Write([]byte(x.Meta[k]))
+	}
+
+	x.Hash = hash.Sum(nil)
 }
 
 // Validate returns an error if the intention is invalid for inserting
@@ -337,9 +331,9 @@ func (x *Intention) String() string {
 
 // EstimateSize returns an estimate (in bytes) of the size of this structure when encoded.
 func (x *Intention) EstimateSize() int {
-	// 60 = 36 (uuid) + 16 (RaftIndex) + 4 (Precedence) + 4 (DefaultPort)
-	size := 60 + len(x.Description) + len(x.SourceNS) + len(x.SourceName) + len(x.DestinationNS) +
-		len(x.DestinationName) + len(x.SourceType) + len(x.Action) + len(x.DefaultAddr)
+	// 56 = 36 (uuid) + 16 (RaftIndex) + 4 (Precedence)
+	size := 56 + len(x.Description) + len(x.SourceNS) + len(x.SourceName) + len(x.DestinationNS) +
+		len(x.DestinationName) + len(x.SourceType) + len(x.Action)
 
 	for k, v := range x.Meta {
 		size += len(k) + len(v)

agent/structs/intention_test.go

@@ -334,3 +334,28 @@ func TestIntentionPrecedenceSorter(t *testing.T) {
 		})
 	}
 }
+
+func TestIntention_SetHash(t *testing.T) {
+	i := Intention{
+		ID:              "the-id",
+		Description:     "the-description",
+		SourceNS:        "source-ns",
+		SourceName:      "source-name",
+		DestinationNS:   "dest-ns",
+		DestinationName: "dest-name",
+		SourceType:      "source-type",
+		Action:          "action",
+		Precedence:      123,
+		Meta: map[string]string{
+			"meta1": "one",
+			"meta2": "two",
+		},
+	}
+	i.SetHash()
+	expected := []byte{
+		0x20, 0x89, 0x55, 0xdb, 0x69, 0x34, 0xce, 0x89, 0xd8, 0xb9, 0x2e, 0x3a,
+		0x85, 0xb6, 0xea, 0x43, 0xb2, 0x23, 0x16, 0x93, 0x94, 0x13, 0x2a, 0xe4,
+		0x81, 0xfe, 0xe, 0x34, 0x91, 0x99, 0xe9, 0x8d,
+	}
+	require.Equal(t, expected, i.Hash)
+}

agent/structs/structs_filtering_test.go

@@ -566,16 +566,6 @@ var expectedFieldConfigIntention bexpr.FieldConfigurations = bexpr.FieldConfigur
 		CoerceFn:            bexpr.CoerceString,
 		SupportedOperations: []bexpr.MatchOperator{bexpr.MatchEqual, bexpr.MatchNotEqual, bexpr.MatchIn, bexpr.MatchNotIn, bexpr.MatchMatches, bexpr.MatchNotMatches},
 	},
-	"DefaultAddr": &bexpr.FieldConfiguration{
-		StructFieldName:     "DefaultAddr",
-		CoerceFn:            bexpr.CoerceString,
-		SupportedOperations: []bexpr.MatchOperator{bexpr.MatchEqual, bexpr.MatchNotEqual, bexpr.MatchIn, bexpr.MatchNotIn, bexpr.MatchMatches, bexpr.MatchNotMatches},
-	},
-	"DefaultPort": &bexpr.FieldConfiguration{
-		StructFieldName:     "DefaultPort",
-		CoerceFn:            bexpr.CoerceInt,
-		SupportedOperations: []bexpr.MatchOperator{bexpr.MatchEqual, bexpr.MatchNotEqual},
-	},
 	"Precedence": &bexpr.FieldConfiguration{
 		StructFieldName:     "Precedence",
 		CoerceFn:            bexpr.CoerceInt,

api/connect_intention.go

@@ -36,10 +36,12 @@ type Intention struct {
 	// Action is whether this is an allowlist or denylist intention.
 	Action IntentionAction
 
-	// DefaultAddr, DefaultPort of the local listening proxy (if any) to
-	// make this connection.
-	DefaultAddr string
-	DefaultPort int
+	// DefaultAddr is not used.
+	// Deprecated: DefaultAddr is not used and may be removed in a future version.
+	DefaultAddr string `json:",omitempty"`
+	// DefaultPort is not used.
+	// Deprecated: DefaultPort is not used and may be removed in a future version.
+	DefaultPort int `json:",omitempty"`
 
 	// Meta is arbitrary metadata associated with the intention. This is
 	// opaque to Consul but is served in API responses.

ui-v2/app/models/intention.js

@@ -14,11 +14,6 @@ export default Model.extend({
   Precedence: attr('number'),
   SourceType: attr('string', { defaultValue: 'consul' }),
   Action: attr('string', { defaultValue: 'deny' }),
-  // These are in the API response but up until now
-  // aren't used for anything
-  DefaultAddr: attr('string'),
-  DefaultPort: attr('number'),
-  //
   Meta: attr(),
   SyncTime: attr('number'),
   Datacenter: attr('string'),

website/pages/api-docs/connect/intentions.mdx

@@ -145,8 +145,6 @@ $ curl \
   "DestinationName": "db",
   "SourceType": "consul",
   "Action": "allow",
-  "DefaultAddr": "",
-  "DefaultPort": 0,
   "Meta": {},
   "Precedence": 9,
   "CreatedAt": "2018-05-21T16:41:27.977155457Z",
@@ -208,8 +206,6 @@ $ curl \
     "DestinationName": "db",
     "SourceType": "consul",
     "Action": "allow",
-    "DefaultAddr": "",
-    "DefaultPort": 0,
     "Meta": {},
     "Precedence": 9,
     "CreatedAt": "2018-05-21T16:41:27.977155457Z",
@@ -228,8 +224,6 @@ the following selectors and filter operations being supported:
 | Selector          | Supported Operations                               |
 | ----------------- | -------------------------------------------------- |
 | `Action`          | Equal, Not Equal, In, Not In, Matches, Not Matches |
-| `DefaultAddr`     | Equal, Not Equal, In, Not In, Matches, Not Matches |
-| `DefaultPort`     | Equal, Not Equal                                   |
 | `Description`     | Equal, Not Equal, In, Not In, Matches, Not Matches |
 | `DestinationNS`   | Equal, Not Equal, In, Not In, Matches, Not Matches |
 | `DestinationName` | Equal, Not Equal, In, Not In, Matches, Not Matches |
@@ -450,8 +444,6 @@ $ curl \
       "DestinationName": "db",
       "SourceType": "consul",
       "Action": "deny",
-      "DefaultAddr": "",
-      "DefaultPort": 0,
       "Meta": {},
       "CreatedAt": "2018-05-21T16:41:33.296693825Z",
       "UpdatedAt": "2018-05-21T16:41:33.296694288Z",
@@ -467,8 +459,6 @@ $ curl \
       "DestinationName": "*",
       "SourceType": "consul",
       "Action": "allow",
-      "DefaultAddr": "",
-      "DefaultPort": 0,
       "Meta": {},
       "CreatedAt": "2018-05-21T16:41:27.977155457Z",
       "UpdatedAt": "2018-05-21T16:41:27.977157724Z",