fix: correct consumers handling when custom_id is used

CHANGELOG.md

@@ -67,10 +67,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

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)

file/writer.go

@@ -382,7 +382,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)
 			}

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

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
 	}

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)

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)
+}

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

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
 	}