Merge pull request #7900 from hashicorp/dnephin/add-linter-staticcheck-2

intentions: fix a bug in Intention.SetHash

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
+	// DefaultAddr is not used.
-	// make this connection.
+	// Deprecated: DefaultAddr is not used and may be removed in a future version.
-	DefaultAddr string
+	DefaultAddr string `bexpr:"-" codec:",omitempty"`
-	DefaultPort int
+	// 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
+// SetHash calculates Intention.Hash from any mutable "content" fields.
-func (x *Intention) SetHash(force bool) []byte {
+//
-	if force || x.Hash == nil {
+// The Hash is primarily used for replication to determine if a token
-		hash, err := blake2b.New256(nil)
+// has changed and should be updated locally.
-		if err != nil {
+//
-			panic(err)
+// TODO: move to agent/consul where it is called
-		}
+func (x *Intention) SetHash() {
-
+	hash, err := blake2b.New256(nil)
-		// Any non-immutable "content" fields should be involved with the
+	if err != nil {
-		// overall hash. The IDs are immutable which is why they aren't here.
+		panic(err)
-		// 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
+
+	// 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)
+	// 56 = 36 (uuid) + 16 (RaftIndex) + 4 (Precedence)
-	size := 60 + len(x.Description) + len(x.SourceNS) + len(x.SourceName) + len(x.DestinationNS) +
+	size := 56 + len(x.Description) + len(x.SourceNS) + len(x.SourceName) + len(x.DestinationNS) +
-		len(x.DestinationName) + len(x.SourceType) + len(x.Action) + len(x.DefaultAddr)
+		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
+	// DefaultAddr is not used.
-	// make this connection.
+	// Deprecated: DefaultAddr is not used and may be removed in a future version.
-	DefaultAddr string
+	DefaultAddr string `json:",omitempty"`
-	DefaultPort int
+	// 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",