From 340462dd725c3a2fcbf4f589d341f610bdea44f9 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 16 Mar 2021 14:53:56 -0400 Subject: [PATCH 1/2] state: use constants and add tests for acl-policies table --- agent/consul/state/acl.go | 6 ++--- agent/consul/state/acl_events.go | 5 +++-- agent/consul/state/acl_oss.go | 16 +++++++------- agent/consul/state/acl_oss_test.go | 35 ++++++++++++++++++++++++++++++ agent/consul/state/acl_test.go | 9 ++++---- agent/consul/state/schema_test.go | 1 + 6 files changed, 55 insertions(+), 17 deletions(-) create mode 100644 agent/consul/state/acl_oss_test.go diff --git a/agent/consul/state/acl.go b/agent/consul/state/acl.go index bb0b15755e..5401d733e3 100644 --- a/agent/consul/state/acl.go +++ b/agent/consul/state/acl.go @@ -228,7 +228,7 @@ func (s *Restore) ACLToken(token *structs.ACLToken) error { // ACLPolicies is used when saving a snapshot func (s *Snapshot) ACLPolicies() (memdb.ResultIterator, error) { - iter, err := s.tx.Get("acl-policies", "id") + iter, err := s.tx.Get(tableACLPolicies, indexID) if err != nil { return nil, err } @@ -1212,8 +1212,8 @@ func (s *Store) ACLPolicyBatchGet(ws memdb.WatchSet, ids []string) (uint64, stru } // We are specifically not wanting to call aclPolicyMaxIndex here as we always want the - // index entry for the "acl-policies" table. - idx := maxIndexTxn(tx, "acl-policies") + // index entry for the tableACLPolicies table. + idx := maxIndexTxn(tx, tableACLPolicies) return idx, policies, nil } diff --git a/agent/consul/state/acl_events.go b/agent/consul/state/acl_events.go index 7b20c9fd46..f6a959a972 100644 --- a/agent/consul/state/acl_events.go +++ b/agent/consul/state/acl_events.go @@ -1,9 +1,10 @@ package state import ( + memdb "github.com/hashicorp/go-memdb" + "github.com/hashicorp/consul/agent/consul/stream" "github.com/hashicorp/consul/agent/structs" - memdb "github.com/hashicorp/go-memdb" ) // aclChangeUnsubscribeEvent creates and returns stream.UnsubscribeEvents that @@ -27,7 +28,7 @@ func aclChangeUnsubscribeEvent(tx ReadTxn, changes Changes) ([]stream.Event, err } secretIDs = appendSecretIDsFromTokenIterator(secretIDs, tokens) - case "acl-policies": + case tableACLPolicies: policy := changeObject(change).(*structs.ACLPolicy) tokens, err := aclTokenListByPolicy(tx, policy.ID, &policy.EnterpriseMeta) if err != nil { diff --git a/agent/consul/state/acl_oss.go b/agent/consul/state/acl_oss.go index ea4e473430..730cd0892b 100644 --- a/agent/consul/state/acl_oss.go +++ b/agent/consul/state/acl_oss.go @@ -11,11 +11,11 @@ import ( ) func aclPolicyInsert(tx *txn, policy *structs.ACLPolicy) error { - if err := tx.Insert("acl-policies", policy); err != nil { + if err := tx.Insert(tableACLPolicies, policy); err != nil { return fmt.Errorf("failed inserting acl policy: %v", err) } - if err := indexUpdateMaxTxn(tx, policy.ModifyIndex, "acl-policies"); err != nil { + if err := indexUpdateMaxTxn(tx, policy.ModifyIndex, tableACLPolicies); err != nil { return fmt.Errorf("failed updating acl policies index: %v", err) } @@ -23,32 +23,32 @@ func aclPolicyInsert(tx *txn, policy *structs.ACLPolicy) error { } func aclPolicyGetByID(tx ReadTxn, id string, _ *structs.EnterpriseMeta) (<-chan struct{}, interface{}, error) { - return tx.FirstWatch("acl-policies", "id", id) + return tx.FirstWatch(tableACLPolicies, indexID, id) } func aclPolicyGetByName(tx ReadTxn, name string, _ *structs.EnterpriseMeta) (<-chan struct{}, interface{}, error) { - return tx.FirstWatch("acl-policies", "name", name) + return tx.FirstWatch(tableACLPolicies, indexName, name) } func aclPolicyList(tx ReadTxn, _ *structs.EnterpriseMeta) (memdb.ResultIterator, error) { - return tx.Get("acl-policies", "id") + return tx.Get(tableACLPolicies, indexID) } func aclPolicyDeleteWithPolicy(tx *txn, policy *structs.ACLPolicy, idx uint64) error { // remove the policy - if err := tx.Delete("acl-policies", policy); err != nil { + if err := tx.Delete(tableACLPolicies, policy); err != nil { return fmt.Errorf("failed deleting acl policy: %v", err) } // update the overall acl-policies index - if err := indexUpdateMaxTxn(tx, idx, "acl-policies"); err != nil { + if err := indexUpdateMaxTxn(tx, idx, tableACLPolicies); err != nil { return fmt.Errorf("failed updating acl policies index: %v", err) } return nil } func aclPolicyMaxIndex(tx ReadTxn, _ *structs.ACLPolicy, _ *structs.EnterpriseMeta) uint64 { - return maxIndexTxn(tx, "acl-policies") + return maxIndexTxn(tx, tableACLPolicies) } func aclPolicyUpsertValidateEnterprise(*txn, *structs.ACLPolicy, *structs.ACLPolicy) error { diff --git a/agent/consul/state/acl_oss_test.go b/agent/consul/state/acl_oss_test.go new file mode 100644 index 0000000000..2979e13b69 --- /dev/null +++ b/agent/consul/state/acl_oss_test.go @@ -0,0 +1,35 @@ +// +build !consulent + +package state + +import "github.com/hashicorp/consul/agent/structs" + +func testIndexerTableACLPolicies() map[string]indexerTestCase { + obj := &structs.ACLPolicy{ + ID: "123e4567-e89b-12d3-a456-426614174abc", + Name: "PoLiCyNaMe", + } + encodedID := []byte{0x12, 0x3e, 0x45, 0x67, 0xe8, 0x9b, 0x12, 0xd3, 0xa4, 0x56, 0x42, 0x66, 0x14, 0x17, 0x4a, 0xbc} + return map[string]indexerTestCase{ + indexID: { + read: indexValue{ + source: obj.ID, + expected: encodedID, + }, + write: indexValue{ + source: obj, + expected: encodedID, + }, + }, + indexName: { + read: indexValue{ + source: []interface{}{"PolicyName"}, + expected: []byte("policyname\x00"), + }, + write: indexValue{ + source: obj, + expected: []byte("policyname\x00"), + }, + }, + } +} diff --git a/agent/consul/state/acl_test.go b/agent/consul/state/acl_test.go index 441ddfbbb2..3f3f21019c 100644 --- a/agent/consul/state/acl_test.go +++ b/agent/consul/state/acl_test.go @@ -7,13 +7,14 @@ import ( "testing" "time" + memdb "github.com/hashicorp/go-memdb" + "github.com/hashicorp/go-uuid" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/lib" pbacl "github.com/hashicorp/consul/proto/pbacl" - memdb "github.com/hashicorp/go-memdb" - "github.com/hashicorp/go-uuid" - "github.com/stretchr/testify/require" ) const ( @@ -3801,7 +3802,7 @@ func TestStateStore_ACLPolicies_Snapshot_Restore(t *testing.T) { require.NoError(t, err) require.Equal(t, uint64(2), idx) require.ElementsMatch(t, policies, res) - require.Equal(t, uint64(2), s.maxIndex("acl-policies")) + require.Equal(t, uint64(2), s.maxIndex(tableACLPolicies)) }() } diff --git a/agent/consul/state/schema_test.go b/agent/consul/state/schema_test.go index 4851a314d8..0992ded938 100644 --- a/agent/consul/state/schema_test.go +++ b/agent/consul/state/schema_test.go @@ -128,6 +128,7 @@ func TestNewDBSchema_Indexers(t *testing.T) { require.NoError(t, schema.Validate()) var testcases = map[string]func() map[string]indexerTestCase{ + tableACLPolicies: testIndexerTableACLPolicies, tableChecks: testIndexerTableChecks, tableServices: testIndexerTableServices, tableNodes: testIndexerTableNodes, From eb6769ccc6c73e9115392aa46c4b5b146c193cb5 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 16 Mar 2021 15:29:30 -0400 Subject: [PATCH 2/2] state: convert acl-policies table to new pattern --- agent/consul/state/acl.go | 20 ++++++---- agent/consul/state/acl_oss.go | 24 ++++++++---- agent/consul/state/acl_oss_test.go | 2 +- agent/consul/state/acl_schema.go | 8 ++-- agent/consul/state/catalog_oss.go | 15 -------- agent/consul/state/query_oss.go | 38 +++++++++++++++++++ .../testdata/TestStateStoreSchema.golden | 2 +- 7 files changed, 73 insertions(+), 36 deletions(-) create mode 100644 agent/consul/state/query_oss.go diff --git a/agent/consul/state/acl.go b/agent/consul/state/acl.go index 5401d733e3..811bfe3ff7 100644 --- a/agent/consul/state/acl.go +++ b/agent/consul/state/acl.go @@ -228,11 +228,7 @@ func (s *Restore) ACLToken(token *structs.ACLToken) error { // ACLPolicies is used when saving a snapshot func (s *Snapshot) ACLPolicies() (memdb.ResultIterator, error) { - iter, err := s.tx.Get(tableACLPolicies, indexID) - if err != nil { - return nil, err - } - return iter, nil + return s.tx.Get(tableACLPolicies, indexID) } func (s *Restore) ACLPolicy(policy *structs.ACLPolicy) error { @@ -1162,7 +1158,8 @@ func aclPolicySetTxn(tx *txn, idx uint64, policy *structs.ACLPolicy) error { } // ensure the name is unique (cannot conflict with another policy with a different ID) - _, nameMatch, err := aclPolicyGetByName(tx, policy.Name, &policy.EnterpriseMeta) + q := Query{Value: policy.Name, EnterpriseMeta: policy.EnterpriseMeta} + nameMatch, err := tx.First(tableACLPolicies, indexName, q) if err != nil { return err } @@ -1195,6 +1192,15 @@ func (s *Store) ACLPolicyGetByName(ws memdb.WatchSet, name string, entMeta *stru return s.aclPolicyGet(ws, name, aclPolicyGetByName, entMeta) } +func aclPolicyGetByName(tx ReadTxn, name string, entMeta *structs.EnterpriseMeta) (<-chan struct{}, interface{}, error) { + // todo: accept non-pointer value + if entMeta == nil { + entMeta = structs.DefaultEnterpriseMeta() + } + q := Query{Value: name, EnterpriseMeta: *entMeta} + return tx.FirstWatch(tableACLPolicies, indexName, q) +} + func (s *Store) ACLPolicyBatchGet(ws memdb.WatchSet, ids []string) (uint64, structs.ACLPolicies, error) { tx := s.db.Txn(false) defer tx.Abort() @@ -1252,7 +1258,7 @@ func (s *Store) ACLPolicyList(ws memdb.WatchSet, entMeta *structs.EnterpriseMeta tx := s.db.Txn(false) defer tx.Abort() - iter, err := aclPolicyList(tx, entMeta) + iter, err := tx.Get(tableACLPolicies, indexName+"_prefix", entMeta) if err != nil { return 0, nil, fmt.Errorf("failed acl policy lookup: %v", err) } diff --git a/agent/consul/state/acl_oss.go b/agent/consul/state/acl_oss.go index 730cd0892b..b4a63e6b50 100644 --- a/agent/consul/state/acl_oss.go +++ b/agent/consul/state/acl_oss.go @@ -4,6 +4,7 @@ package state import ( "fmt" + "strings" memdb "github.com/hashicorp/go-memdb" @@ -22,18 +23,25 @@ func aclPolicyInsert(tx *txn, policy *structs.ACLPolicy) error { return nil } +func indexNameFromACLPolicy(raw interface{}) ([]byte, error) { + p, ok := raw.(*structs.ACLPolicy) + if !ok { + return nil, fmt.Errorf("unexpected type %T for structs.ACLPolicy index", raw) + } + + if p.Name == "" { + return nil, errMissingValueForIndex + } + + var b indexBuilder + b.String(strings.ToLower(p.Name)) + return b.Bytes(), nil +} + func aclPolicyGetByID(tx ReadTxn, id string, _ *structs.EnterpriseMeta) (<-chan struct{}, interface{}, error) { return tx.FirstWatch(tableACLPolicies, indexID, id) } -func aclPolicyGetByName(tx ReadTxn, name string, _ *structs.EnterpriseMeta) (<-chan struct{}, interface{}, error) { - return tx.FirstWatch(tableACLPolicies, indexName, name) -} - -func aclPolicyList(tx ReadTxn, _ *structs.EnterpriseMeta) (memdb.ResultIterator, error) { - return tx.Get(tableACLPolicies, indexID) -} - func aclPolicyDeleteWithPolicy(tx *txn, policy *structs.ACLPolicy, idx uint64) error { // remove the policy if err := tx.Delete(tableACLPolicies, policy); err != nil { diff --git a/agent/consul/state/acl_oss_test.go b/agent/consul/state/acl_oss_test.go index 2979e13b69..fab7197819 100644 --- a/agent/consul/state/acl_oss_test.go +++ b/agent/consul/state/acl_oss_test.go @@ -23,7 +23,7 @@ func testIndexerTableACLPolicies() map[string]indexerTestCase { }, indexName: { read: indexValue{ - source: []interface{}{"PolicyName"}, + source: Query{Value: "PolicyName"}, expected: []byte("policyname\x00"), }, write: indexValue{ diff --git a/agent/consul/state/acl_schema.go b/agent/consul/state/acl_schema.go index 88cd1b4d61..0126b5b90b 100644 --- a/agent/consul/state/acl_schema.go +++ b/agent/consul/state/acl_schema.go @@ -125,10 +125,10 @@ func policiesTableSchema() *memdb.TableSchema { Name: indexName, AllowMissing: false, Unique: true, - Indexer: &memdb.StringFieldIndex{ - Field: "Name", - // TODO (ACL-V2) - should we coerce to lowercase? - Lowercase: true, + Indexer: indexerSingleWithPrefix{ + readIndex: readIndex(indexFromQuery), + writeIndex: writeIndex(indexNameFromACLPolicy), + prefixIndex: prefixIndex(prefixIndexFromQuery), }, }, }, diff --git a/agent/consul/state/catalog_oss.go b/agent/consul/state/catalog_oss.go index 547ea89b11..090422603c 100644 --- a/agent/consul/state/catalog_oss.go +++ b/agent/consul/state/catalog_oss.go @@ -103,21 +103,6 @@ func indexFromServiceNode(raw interface{}) ([]byte, error) { return b.Bytes(), nil } -func prefixIndexFromQuery(arg interface{}) ([]byte, error) { - var b indexBuilder - switch v := arg.(type) { - case *structs.EnterpriseMeta: - return nil, nil - case structs.EnterpriseMeta: - return nil, nil - case Query: - b.String(strings.ToLower(v.Value)) - return b.Bytes(), nil - } - - return nil, fmt.Errorf("unexpected type %T for NodeServiceQuery prefix index", arg) -} - func serviceIndexName(name string, _ *structs.EnterpriseMeta) string { return fmt.Sprintf("service.%s", name) } diff --git a/agent/consul/state/query_oss.go b/agent/consul/state/query_oss.go new file mode 100644 index 0000000000..79f45095be --- /dev/null +++ b/agent/consul/state/query_oss.go @@ -0,0 +1,38 @@ +// +build !consulent + +package state + +import ( + "fmt" + "strings" + + "github.com/hashicorp/consul/agent/structs" +) + +// indexFromQuery builds an index key where Query.Value is lowercase, and is +// a required value. +func indexFromQuery(arg interface{}) ([]byte, error) { + q, ok := arg.(Query) + if !ok { + return nil, fmt.Errorf("unexpected type %T for Query index", arg) + } + + var b indexBuilder + b.String(strings.ToLower(q.Value)) + return b.Bytes(), nil +} + +func prefixIndexFromQuery(arg interface{}) ([]byte, error) { + var b indexBuilder + switch v := arg.(type) { + case *structs.EnterpriseMeta: + return nil, nil + case structs.EnterpriseMeta: + return nil, nil + case Query: + b.String(strings.ToLower(v.Value)) + return b.Bytes(), nil + } + + return nil, fmt.Errorf("unexpected type %T for Query prefix index", arg) +} diff --git a/agent/consul/state/testdata/TestStateStoreSchema.golden b/agent/consul/state/testdata/TestStateStoreSchema.golden index 7f74557800..4a6b752557 100644 --- a/agent/consul/state/testdata/TestStateStoreSchema.golden +++ b/agent/consul/state/testdata/TestStateStoreSchema.golden @@ -12,7 +12,7 @@ table=acl-policies index=id unique indexer=github.com/hashicorp/go-memdb.UUIDFieldIndex Field=ID index=name unique - indexer=github.com/hashicorp/go-memdb.StringFieldIndex Field=Name Lowercase=true + indexer=github.com/hashicorp/consul/agent/consul/state.indexerSingleWithPrefix readIndex=github.com/hashicorp/consul/agent/consul/state.indexFromQuery writeIndex=github.com/hashicorp/consul/agent/consul/state.indexNameFromACLPolicy prefixIndex=github.com/hashicorp/consul/agent/consul/state.prefixIndexFromQuery table=acl-roles index=id unique