From adc5589329a8b3b1809208d18ce6e0b8c002641c Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Thu, 5 Apr 2018 12:41:49 +0100 Subject: [PATCH] Allow duplicate source or destination, but enforce uniqueness across all four. --- agent/consul/state/intention.go | 44 ++++++++++++- agent/consul/state/intention_test.go | 96 +++++++++++++++++++++++++--- agent/structs/intention.go | 23 ++++++- agent/structs/intention_test.go | 24 +++++++ 4 files changed, 174 insertions(+), 13 deletions(-) diff --git a/agent/consul/state/intention.go b/agent/consul/state/intention.go index bc8bb0213..907bdf1ab 100644 --- a/agent/consul/state/intention.go +++ b/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 { diff --git a/agent/consul/state/intention_test.go b/agent/consul/state/intention_test.go index d4c63647a..743f698af 100644 --- a/agent/consul/state/intention_test.go +++ b/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}) + } } } diff --git a/agent/structs/intention.go b/agent/structs/intention.go index d801635c9..316c9632b 100644 --- a/agent/structs/intention.go +++ b/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 diff --git a/agent/structs/intention_test.go b/agent/structs/intention_test.go index 948ae920e..cda88632f 100644 --- a/agent/structs/intention_test.go +++ b/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 {