diff --git a/CHANGELOG.md b/CHANGELOG.md index fae9654fc..838d096e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,10 +75,11 @@ ### Fixes +- Fix Consumers handling when a consumer's `custom_id` is equal to the `username` of another consumer. + [#986](https://github.com/Kong/deck/pull/986) - Avoid misleading diffs when configuration file has empty tags. [#985](https://github.com/Kong/deck/pull/985) - ## [v1.24.0] > Release date: 2023/07/24 diff --git a/file/builder.go b/file/builder.go index a0286e215..755ca5574 100644 --- a/file/builder.go +++ b/file/builder.go @@ -234,10 +234,10 @@ func (b *stateBuilder) consumers() { for _, c := range b.targetContent.Consumers { c := c if utils.Empty(c.ID) { - consumer, err := b.currentState.Consumers.Get(*c.Username) + consumer, err := b.currentState.Consumers.GetByIDOrUsername(*c.Username) if errors.Is(err, state.ErrNotFound) { if c.CustomID != nil { - consumer, err = b.currentState.Consumers.Get(*c.CustomID) + consumer, err = b.currentState.Consumers.GetByCustomID(*c.CustomID) if err == nil { c.ID = kong.String(*consumer.ID) } @@ -844,7 +844,7 @@ func (b *stateBuilder) plugins() { for _, p := range b.targetContent.Plugins { p := p if p.Consumer != nil && !utils.Empty(p.Consumer.ID) { - c, err := b.intermediate.Consumers.Get(*p.Consumer.ID) + c, err := b.intermediate.Consumers.GetByIDOrUsername(*p.Consumer.ID) if errors.Is(err, state.ErrNotFound) { b.err = fmt.Errorf("consumer %v for plugin %v: %w", p.Consumer.FriendlyName(), *p.Name, err) diff --git a/file/writer.go b/file/writer.go index 02529cce2..8d60385c3 100644 --- a/file/writer.go +++ b/file/writer.go @@ -391,7 +391,7 @@ func populatePlugins(kongState *state.KongState, file *Content, if p.Consumer != nil { associations++ cID := *p.Consumer.ID - consumer, err := kongState.Consumers.Get(cID) + consumer, err := kongState.Consumers.GetByIDOrUsername(cID) if err != nil { return fmt.Errorf("unable to get consumer %s for plugin %s [%s]: %w", cID, *p.Name, *p.ID, err) } diff --git a/state/builder.go b/state/builder.go index cf7e1c20a..d7200aa12 100644 --- a/state/builder.go +++ b/state/builder.go @@ -46,7 +46,7 @@ func ensureRoute(kongState *KongState, routeID string) (bool, *kong.Route, error } func ensureConsumer(kongState *KongState, consumerID string) (bool, *kong.Consumer, error) { - c, err := kongState.Consumers.Get(consumerID) + c, err := kongState.Consumers.GetByIDOrUsername(consumerID) if err != nil { if errors.Is(err, ErrNotFound) { return false, nil, nil diff --git a/state/consumer.go b/state/consumer.go index d2542c8ee..605311781 100644 --- a/state/consumer.go +++ b/state/consumer.go @@ -54,10 +54,23 @@ func (k *ConsumersCollection) Add(consumer Consumer) error { if !utils.Empty(consumer.Username) { searchBy = append(searchBy, *consumer.Username) } + + // search separately by id+username and by custom_id. + // + // This is because the custom_id is unique, but it may be equal to + // the username of another consumer. If we search by both id+username and + // custom_id, we may get a false positive. + _, err := getConsumer(txn, []string{"Username", "id"}, searchBy...) + if err == nil { + return fmt.Errorf("inserting consumer %v: %w", consumer.Console(), ErrAlreadyExists) + } else if !errors.Is(err, ErrNotFound) { + return err + } + if !utils.Empty(consumer.CustomID) { - searchBy = append(searchBy, *consumer.CustomID) + searchBy = []string{*consumer.CustomID} } - _, err := getConsumer(txn, searchBy...) + _, err = getConsumer(txn, []string{"CustomID"}, searchBy...) if err == nil { return fmt.Errorf("inserting consumer %v: %w", consumer.Console(), ErrAlreadyExists) } else if !errors.Is(err, ErrNotFound) { @@ -72,10 +85,9 @@ func (k *ConsumersCollection) Add(consumer Consumer) error { return nil } -func getConsumer(txn *memdb.Txn, IDs ...string) (*Consumer, error) { +func getConsumer(txn *memdb.Txn, indexes []string, IDs ...string) (*Consumer, error) { for _, id := range IDs { - res, err := multiIndexLookupUsingTxn(txn, consumerTableName, - []string{"Username", "id", "CustomID"}, id) + res, err := multiIndexLookupUsingTxn(txn, consumerTableName, indexes, id) if errors.Is(err, ErrNotFound) { continue } @@ -91,15 +103,26 @@ func getConsumer(txn *memdb.Txn, IDs ...string) (*Consumer, error) { return nil, ErrNotFound } -// Get gets a consumer by name or ID. -func (k *ConsumersCollection) Get(userNameOrID string) (*Consumer, error) { +// GetByIDOrUsername gets a consumer by name or ID. +func (k *ConsumersCollection) GetByIDOrUsername(userNameOrID string) (*Consumer, error) { if userNameOrID == "" { return nil, errIDRequired } txn := k.db.Txn(false) defer txn.Abort() - return getConsumer(txn, userNameOrID) + return getConsumer(txn, []string{"Username", "id"}, userNameOrID) +} + +// GetByCustomID gets a consumer by customID. +func (k *ConsumersCollection) GetByCustomID(customID string) (*Consumer, error) { + if customID == "" { + return nil, errIDRequired + } + + txn := k.db.Txn(false) + defer txn.Abort() + return getConsumer(txn, []string{"CustomID"}, customID) } // Update udpates an existing consumer. @@ -128,7 +151,7 @@ func (k *ConsumersCollection) Update(consumer Consumer) error { } func deleteConsumer(txn *memdb.Txn, userNameOrID string) error { - consumer, err := getConsumer(txn, userNameOrID) + consumer, err := getConsumer(txn, []string{"Username", "id"}, userNameOrID) if err != nil { return err } diff --git a/state/consumer_test.go b/state/consumer_test.go index 60922943e..35ecf5356 100644 --- a/state/consumer_test.go +++ b/state/consumer_test.go @@ -37,11 +37,11 @@ func TestConsumerGetUpdate(t *testing.T) { err := collection.Add(consumer) assert.Nil(err) - c, err := collection.Get("") + c, err := collection.GetByIDOrUsername("") assert.NotNil(err) assert.Nil(c) - c, err = collection.Get("first") + c, err = collection.GetByIDOrUsername("first") assert.Nil(err) assert.NotNil(c) @@ -56,11 +56,11 @@ func TestConsumerGetUpdate(t *testing.T) { c.ID = kong.String("first") assert.Nil(collection.Update(*c)) - c, err = collection.Get("my-name") + c, err = collection.GetByIDOrUsername("my-name") assert.NotNil(err) assert.Nil(c) - c, err = collection.Get("my-updated-name") + c, err = collection.GetByIDOrUsername("my-updated-name") assert.Nil(err) assert.NotNil(c) } @@ -77,12 +77,12 @@ func TestConsumerGetMemoryReference(t *testing.T) { err := collection.Add(consumer) assert.Nil(err) - c, err := collection.Get("first") + c, err := collection.GetByIDOrUsername("first") assert.Nil(err) assert.NotNil(c) c.Username = kong.String("update-should-not-reflect") - c, err = collection.Get("first") + c, err = collection.GetByIDOrUsername("first") assert.Nil(err) assert.Equal("my-name", *c.Username) } @@ -100,7 +100,7 @@ func TestConsumersInvalidType(t *testing.T) { txn.Commit() assert.Panics(func() { - collection.Get("my-name") + collection.GetByIDOrUsername("my-name") }) assert.Panics(func() { collection.GetAll() @@ -117,7 +117,7 @@ func TestConsumerDelete(t *testing.T) { err := collection.Add(consumer) assert.Nil(err) - c, err := collection.Get("my-consumer") + c, err := collection.GetByIDOrUsername("my-consumer") assert.Nil(err) assert.NotNil(c) assert.Equal("first", *c.ID) diff --git a/tests/integration/sync_test.go b/tests/integration/sync_test.go index 2c769953a..68fadc172 100644 --- a/tests/integration/sync_test.go +++ b/tests/integration/sync_test.go @@ -3519,3 +3519,33 @@ func Test_Sync_CreateCertificateWithSNIs(t *testing.T) { }, }, ignoredFields) } + +// test scope: +// - 3.0.0+ +// - konnect +func Test_Sync_ConsumersWithCustomIDAndUsername(t *testing.T) { + runWhenKongOrKonnect(t, ">=3.0.0") + + client, err := getTestClient() + if err != nil { + t.Errorf(err.Error()) + } + + err = sync("testdata/sync/024-consumers-with-custom_id-and-username/kong3x.yaml") + require.NoError(t, err) + + testKongState(t, client, false, utils.KongRawState{ + Consumers: []*kong.Consumer{ + { + ID: kong.String("ce49186d-7670-445d-a218-897631b29ada"), + Username: kong.String("Foo"), + CustomID: kong.String("foo"), + }, + { + ID: kong.String("7820f383-7b77-4fcc-af7f-14ff3e256693"), + Username: kong.String("foo"), + CustomID: kong.String("bar"), + }, + }, + }, nil) +} diff --git a/tests/integration/testdata/sync/024-consumers-with-custom_id-and-username/kong3x.yaml b/tests/integration/testdata/sync/024-consumers-with-custom_id-and-username/kong3x.yaml new file mode 100644 index 000000000..f20ee8080 --- /dev/null +++ b/tests/integration/testdata/sync/024-consumers-with-custom_id-and-username/kong3x.yaml @@ -0,0 +1,8 @@ +_format_version: "3.0" +consumers: +- custom_id: foo + id: ce49186d-7670-445d-a218-897631b29ada + username: Foo +- custom_id: bar + id: 7820f383-7b77-4fcc-af7f-14ff3e256693 + username: foo diff --git a/types/consumer.go b/types/consumer.go index 65a1092d2..6d8202ff5 100644 --- a/types/consumer.go +++ b/types/consumer.go @@ -95,7 +95,7 @@ func (d *consumerDiffer) Deletes(handler func(crud.Event) error) error { } func (d *consumerDiffer) deleteConsumer(consumer *state.Consumer) (*crud.Event, error) { - _, err := d.targetState.Consumers.Get(*consumer.ID) + _, err := d.targetState.Consumers.GetByIDOrUsername(*consumer.ID) if errors.Is(err, state.ErrNotFound) { return &crud.Event{ Op: crud.Delete, @@ -133,7 +133,7 @@ func (d *consumerDiffer) CreateAndUpdates(handler func(crud.Event) error) error func (d *consumerDiffer) createUpdateConsumer(consumer *state.Consumer) (*crud.Event, error) { consumerCopy := &state.Consumer{Consumer: *consumer.DeepCopy()} - currentConsumer, err := d.currentState.Consumers.Get(*consumer.ID) + currentConsumer, err := d.currentState.Consumers.GetByIDOrUsername(*consumer.ID) if errors.Is(err, state.ErrNotFound) { // consumer not present, create it @@ -181,7 +181,7 @@ func (d *consumerDiffer) DuplicatesDeletes() ([]crud.Event, error) { } func (d *consumerDiffer) deleteDuplicateConsumer(targetConsumer *state.Consumer) (*crud.Event, error) { - currentConsumer, err := d.currentState.Consumers.Get(*targetConsumer.Username) + currentConsumer, err := d.currentState.Consumers.GetByIDOrUsername(*targetConsumer.Username) if errors.Is(err, state.ErrNotFound) { return nil, nil }