Allow duplicate source or destination, but enforce uniqueness across all four.

agent/consul/state/intention.go

@@ -29,7 +29,9 @@ func intentionsTableSchema() *memdb.TableSchema {
 			"destination": &memdb.IndexSchema{
 				Name:         "destination",
 				AllowMissing: true,
-				Unique:       true,
+				// This index is not unique since we need uniqueness across the whole
+				// 4-tuple.
+				Unique: false,
 				Indexer: &memdb.CompoundIndex{
 					Indexes: []memdb.Indexer{
 						&memdb.StringFieldIndex{
@@ -46,6 +48,25 @@ func intentionsTableSchema() *memdb.TableSchema {
 			"source": &memdb.IndexSchema{
 				Name:         "source",
 				AllowMissing: true,
+				// This index is not unique since we need uniqueness across the whole
+				// 4-tuple.
+				Unique: false,
+				Indexer: &memdb.CompoundIndex{
+					Indexes: []memdb.Indexer{
+						&memdb.StringFieldIndex{
+							Field:     "SourceNS",
+							Lowercase: true,
+						},
+						&memdb.StringFieldIndex{
+							Field:     "SourceName",
+							Lowercase: true,
+						},
+					},
+				},
+			},
+			"source_destination": &memdb.IndexSchema{
+				Name:         "source_destination",
+				AllowMissing: true,
 				Unique:       true,
 				Indexer: &memdb.CompoundIndex{
 					Indexes: []memdb.Indexer{
@@ -57,6 +78,14 @@ func intentionsTableSchema() *memdb.TableSchema {
 							Field:     "SourceName",
 							Lowercase: true,
 						},
+						&memdb.StringFieldIndex{
+							Field:     "DestinationNS",
+							Lowercase: true,
+						},
+						&memdb.StringFieldIndex{
+							Field:     "DestinationName",
+							Lowercase: true,
+						},
 					},
 				},
 			},
@@ -142,7 +171,7 @@ func (s *Store) intentionSetTxn(tx *memdb.Txn, idx uint64, ixn *structs.Intentio
 	// Check for an existing intention
 	existing, err := tx.First(intentionsTableName, "id", ixn.ID)
 	if err != nil {
-		return fmt.Errorf("failed intention looup: %s", err)
+		return fmt.Errorf("failed intention lookup: %s", err)
 	}
 	if existing != nil {
 		oldIxn := existing.(*structs.Intention)
@@ -153,6 +182,17 @@ func (s *Store) intentionSetTxn(tx *memdb.Txn, idx uint64, ixn *structs.Intentio
 	}
 	ixn.ModifyIndex = idx
 
+	// Check for duplicates on the 4-tuple.
+	duplicate, err := tx.First(intentionsTableName, "source_destination",
+		ixn.SourceNS, ixn.SourceName, ixn.DestinationNS, ixn.DestinationName)
+	if err != nil {
+		return fmt.Errorf("failed intention lookup: %s", err)
+	}
+	if duplicate != nil {
+		dupIxn := duplicate.(*structs.Intention)
+		return fmt.Errorf("duplicate intention found: %s", dupIxn.String())
+	}
+
 	// We always force meta to be non-nil so that we its an empty map.
 	// This makes it easy for API responses to not nil-check this everywhere.
 	if ixn.Meta == nil {

agent/consul/state/intention_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/go-memdb"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestStore_IntentionGet_none(t *testing.T) {
@@ -32,21 +33,29 @@ func TestStore_IntentionSetGet_basic(t *testing.T) {
 
 	// Build a valid intention
 	ixn := &structs.Intention{
-		ID:   testUUID(),
-		Meta: map[string]string{},
+		ID:              testUUID(),
+		SourceNS:        "default",
+		SourceName:      "*",
+		DestinationNS:   "default",
+		DestinationName: "web",
+		Meta:            map[string]string{},
 	}
 
 	// Inserting a with empty ID is disallowed.
 	assert.Nil(s.IntentionSet(1, ixn))
 
 	// Make sure the index got updated.
-	assert.Equal(s.maxIndex(intentionsTableName), uint64(1))
+	assert.Equal(uint64(1), s.maxIndex(intentionsTableName))
 	assert.True(watchFired(ws), "watch fired")
 
 	// Read it back out and verify it.
 	expected := &structs.Intention{
-		ID:   ixn.ID,
-		Meta: map[string]string{},
+		ID:              ixn.ID,
+		SourceNS:        "default",
+		SourceName:      "*",
+		DestinationNS:   "default",
+		DestinationName: "web",
+		Meta:            map[string]string{},
 		RaftIndex: structs.RaftIndex{
 			CreateIndex: 1,
 			ModifyIndex: 1,
@@ -64,7 +73,7 @@ func TestStore_IntentionSetGet_basic(t *testing.T) {
 	assert.Nil(s.IntentionSet(2, ixn))
 
 	// Make sure the index got updated.
-	assert.Equal(s.maxIndex(intentionsTableName), uint64(2))
+	assert.Equal(uint64(2), s.maxIndex(intentionsTableName))
 	assert.True(watchFired(ws), "watch fired")
 
 	// Read it back and verify the data was updated
@@ -75,6 +84,24 @@ func TestStore_IntentionSetGet_basic(t *testing.T) {
 	assert.Nil(err)
 	assert.Equal(expected.ModifyIndex, idx)
 	assert.Equal(expected, actual)
+
+	// Attempt to insert another intention with duplicate 4-tuple
+	ixn = &structs.Intention{
+		ID:              testUUID(),
+		SourceNS:        "default",
+		SourceName:      "*",
+		DestinationNS:   "default",
+		DestinationName: "web",
+		Meta:            map[string]string{},
+	}
+
+	// Duplicate 4-tuple should cause an error
+	ws = memdb.NewWatchSet()
+	assert.NotNil(s.IntentionSet(3, ixn))
+
+	// Make sure the index did NOT get updated.
+	assert.Equal(uint64(2), s.maxIndex(intentionsTableName))
+	assert.False(watchFired(ws), "watch not fired")
 }
 
 func TestStore_IntentionSet_emptyId(t *testing.T) {
@@ -305,6 +332,31 @@ func TestStore_IntentionMatch_table(t *testing.T) {
 				},
 			},
 		},
+
+		{
+			"single exact namespace/name with duplicate destinations",
+			[][]string{
+				// 4-tuple specifies src and destination to test duplicate destinations
+				// with different sources. We flip them around to test in both
+				// directions. The first pair are the ones searched on in both cases so
+				// the duplicates need to be there.
+				{"foo", "bar", "foo", "*"},
+				{"foo", "bar", "bar", "*"},
+				{"*", "*", "*", "*"},
+			},
+			[][]string{
+				{"foo", "bar"},
+			},
+			[][][]string{
+				{
+					// Note the first two have the same precedence so we rely on arbitrary
+					// lexicographical tie-break behaviour.
+					{"foo", "bar", "bar", "*"},
+					{"foo", "bar", "foo", "*"},
+					{"*", "*", "*", "*"},
+				},
+			},
+		},
 	}
 
 	// testRunner implements the test for a single case, but can be
@@ -321,9 +373,17 @@ func TestStore_IntentionMatch_table(t *testing.T) {
 			case structs.IntentionMatchDestination:
 				ixn.DestinationNS = v[0]
 				ixn.DestinationName = v[1]
+				if len(v) == 4 {
+					ixn.SourceNS = v[2]
+					ixn.SourceName = v[3]
+				}
 			case structs.IntentionMatchSource:
 				ixn.SourceNS = v[0]
 				ixn.SourceName = v[1]
+				if len(v) == 4 {
+					ixn.DestinationNS = v[2]
+					ixn.DestinationName = v[3]
+				}
 			}
 
 			assert.Nil(s.IntentionSet(idx, ixn))
@@ -345,7 +405,7 @@ func TestStore_IntentionMatch_table(t *testing.T) {
 		assert.Nil(err)
 
 		// Should have equal lengths
-		assert.Len(matches, len(tc.Expected))
+		require.Len(t, matches, len(tc.Expected))
 
 		// Verify matches
 		for i, expected := range tc.Expected {
@@ -353,9 +413,27 @@ func TestStore_IntentionMatch_table(t *testing.T) {
 			for _, ixn := range matches[i] {
 				switch typ {
 				case structs.IntentionMatchDestination:
-					actual = append(actual, []string{ixn.DestinationNS, ixn.DestinationName})
+					if len(expected) > 1 && len(expected[0]) == 4 {
+						actual = append(actual, []string{
+							ixn.DestinationNS,
+							ixn.DestinationName,
+							ixn.SourceNS,
+							ixn.SourceName,
+						})
+					} else {
+						actual = append(actual, []string{ixn.DestinationNS, ixn.DestinationName})
+					}
 				case structs.IntentionMatchSource:
-					actual = append(actual, []string{ixn.SourceNS, ixn.SourceName})
+					if len(expected) > 1 && len(expected[0]) == 4 {
+						actual = append(actual, []string{
+							ixn.SourceNS,
+							ixn.SourceName,
+							ixn.DestinationNS,
+							ixn.DestinationName,
+						})
+					} else {
+						actual = append(actual, []string{ixn.SourceNS, ixn.SourceName})
+					}
 				}
 			}
 

agent/structs/intention.go

@@ -166,7 +166,7 @@ func (x *Intention) GetACLPrefix() (string, bool) {
 
 // String returns a human-friendly string for this intention.
 func (x *Intention) String() string {
-	return fmt.Sprintf("%s %s/%s => %s/%s (ID: %s",
+	return fmt.Sprintf("%s %s/%s => %s/%s (ID: %s)",
 		strings.ToUpper(string(x.Action)),
 		x.SourceNS, x.SourceName,
 		x.DestinationNS, x.DestinationName,
@@ -305,7 +305,26 @@ func (s IntentionPrecedenceSorter) Less(i, j int) bool {
 	// Next test the # of exact values in source
 	aExact = s.countExact(a.SourceNS, a.SourceName)
 	bExact = s.countExact(b.SourceNS, b.SourceName)
-	return aExact > bExact
+	if aExact != bExact {
+		return aExact > bExact
+	}
+
+	// Tie break on lexicographic order of the 4-tuple in canonical form (SrcNS,
+	// Src, DstNS, Dst). This is arbitrary but it keeps sorting deterministic
+	// which is a nice property for consistency. It is arguably open to abuse if
+	// implementations rely on this however by definition the order among
+	// same-precedence rules is arbitrary and doesn't affect whether an allow or
+	// deny rule is acted on since all applicable rules are checked.
+	if a.SourceNS != b.SourceNS {
+		return a.SourceNS < b.SourceNS
+	}
+	if a.SourceName != b.SourceName {
+		return a.SourceName < b.SourceName
+	}
+	if a.DestinationNS != b.DestinationNS {
+		return a.DestinationNS < b.DestinationNS
+	}
+	return a.DestinationName < b.DestinationName
 }
 
 // countExact counts the number of exact values (not wildcards) in

agent/structs/intention_test.go

@@ -192,6 +192,30 @@ func TestIntentionPrecedenceSorter(t *testing.T) {
 				{"*", "*", "*", "*"},
 			},
 		},
+		{
+			"tiebreak deterministically",
+			[][]string{
+				{"a", "*", "a", "b"},
+				{"a", "*", "a", "a"},
+				{"b", "a", "a", "a"},
+				{"a", "b", "a", "a"},
+				{"a", "a", "b", "a"},
+				{"a", "a", "a", "b"},
+				{"a", "a", "a", "a"},
+			},
+			[][]string{
+				// Exact matches first in lexicographical order (arbitrary but
+				// deterministic)
+				{"a", "a", "a", "a"},
+				{"a", "a", "a", "b"},
+				{"a", "a", "b", "a"},
+				{"a", "b", "a", "a"},
+				{"b", "a", "a", "a"},
+				// Wildcards next, lexicographical
+				{"a", "*", "a", "a"},
+				{"a", "*", "a", "b"},
+			},
+		},
 	}
 
 	for _, tc := range cases {