Skip to content

Commit

Permalink
fix: update consumers when custom_id is also set
Browse files Browse the repository at this point in the history
Right now decK only uses ID and Username for Consumers lookup,
which is problematic because CustomID is also unique and
modifying a Username causes a Consumer re-creation, which
triggers a collision for the CustomID field.

This PR makes sure that CustomID is included in the lookup
loop, so that such Consumers changes can be made in-place.
  • Loading branch information
GGabriele committed Jun 29, 2022
1 parent 5df93b6 commit c27845a
Show file tree
Hide file tree
Showing 9 changed files with 134 additions and 3 deletions.
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# Table of Contents

- [Unreleased](#Unreleased)
- [v1.12.2](#v1122)
- [v1.12.1](#v1121)
- [v1.12.0](#v1120)
Expand Down Expand Up @@ -39,6 +40,16 @@
- [v0.2.0](#v020)
- [v0.1.0](#v010)

## [Unreleased]

> Release date: TBD
### Fixes

- Make sure decK can update in place consumers' username when
`custom_id` is also set.
[#707](https://github.com/Kong/deck/pull/707)

## [v1.12.2]

> Release date: 2022/06/06
Expand Down Expand Up @@ -947,6 +958,7 @@ No breaking changes have been introduced in this release.

Debut release of decK

[Unreleased]: https://github.com/kong/deck/compare/v1.12.2...Unreleased
[v1.12.2]: https://github.com/kong/deck/compare/v1.12.1...v1.12.2
[v1.12.1]: https://github.com/kong/deck/compare/v1.12.0...v1.12.1
[v1.12.0]: https://github.com/kong/deck/compare/v1.11.0...v1.12.0
Expand Down
10 changes: 9 additions & 1 deletion file/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,15 @@ func (b *stateBuilder) consumers() {
if utils.Empty(c.ID) {
consumer, err := b.currentState.Consumers.Get(*c.Username)
if err == state.ErrNotFound {
c.ID = uuid()
if c.CustomID != nil {
consumer, err = b.currentState.Consumers.Get(*c.CustomID)
if err == nil {
c.ID = kong.String(*consumer.ID)
}
}
if c.ID == nil {
c.ID = uuid()
}
} else if err != nil {
b.err = err
return
Expand Down
11 changes: 10 additions & 1 deletion state/consumer.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ var consumerTableSchema = &memdb.TableSchema{
Indexer: &memdb.StringFieldIndex{Field: "Username"},
AllowMissing: true,
},
"CustomID": {
Name: "CustomID",
Unique: true,
Indexer: &memdb.StringFieldIndex{Field: "CustomID"},
AllowMissing: true,
},
all: allIndex,
},
}
Expand All @@ -47,6 +53,9 @@ func (k *ConsumersCollection) Add(consumer Consumer) error {
if !utils.Empty(consumer.Username) {
searchBy = append(searchBy, *consumer.Username)
}
if !utils.Empty(consumer.CustomID) {
searchBy = append(searchBy, *consumer.CustomID)
}
_, err := getConsumer(txn, searchBy...)
if err == nil {
return fmt.Errorf("inserting consumer %v: %w", consumer.Console(), ErrAlreadyExists)
Expand All @@ -65,7 +74,7 @@ func (k *ConsumersCollection) Add(consumer Consumer) error {
func getConsumer(txn *memdb.Txn, IDs ...string) (*Consumer, error) {
for _, id := range IDs {
res, err := multiIndexLookupUsingTxn(txn, consumerTableName,
[]string{"Username", "id"}, id)
[]string{"Username", "id", "CustomID"}, id)
if err == ErrNotFound {
continue
}
Expand Down
86 changes: 86 additions & 0 deletions tests/integration/sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1277,3 +1277,89 @@ func Test_Sync_PluginsOnEntities(t *testing.T) {
})
}
}

func Test_Sync_UpdateUsernameInConsumerWithCustomID(t *testing.T) {
// setup stage
client, err := getTestClient()
if err != nil {
t.Errorf(err.Error())
}

tests := []struct {
name string
kongFile string
kongFileInitial string
expectedState utils.KongRawState
}{
{
name: "update username on a consumer with custom_id",
kongFile: "testdata/sync/011-update-username-consumer-with-custom-id/kong.yaml",
kongFileInitial: "testdata/sync/011-update-username-consumer-with-custom-id/kong-initial.yaml",
expectedState: utils.KongRawState{
Consumers: []*kong.Consumer{
{
Username: kong.String("test_new"),
CustomID: kong.String("custom_test"),
},
},
},
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
kong.RunWhenKong(t, ">=2.8.0")
teardown := setup(t)
defer teardown(t)

// set up initial state
sync(tc.kongFileInitial)
// update with desired final state
sync(tc.kongFile)
testKongState(t, client, tc.expectedState, nil)
})
}
}

func Test_Sync_UpdateConsumerWithCustomID(t *testing.T) {
// setup stage
client, err := getTestClient()
if err != nil {
t.Errorf(err.Error())
}

tests := []struct {
name string
kongFile string
kongFileInitial string
expectedState utils.KongRawState
}{
{
name: "update username on a consumer with custom_id",
kongFile: "testdata/sync/012-update-consumer-with-custom-id/kong.yaml",
kongFileInitial: "testdata/sync/012-update-consumer-with-custom-id/kong-initial.yaml",
expectedState: utils.KongRawState{
Consumers: []*kong.Consumer{
{
Username: kong.String("test"),
CustomID: kong.String("new_custom_test"),
},
},
},
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
kong.RunWhenKong(t, ">=2.8.0")
teardown := setup(t)
defer teardown(t)

// set up initial state
sync(tc.kongFileInitial)
// update with desired final state
sync(tc.kongFile)
testKongState(t, client, tc.expectedState, nil)
})
}
}
2 changes: 1 addition & 1 deletion tests/integration/test_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func testKongState(t *testing.T, client *kong.Client,
cmpopts.IgnoreFields(kong.CACertificate{}, "ID", "CreatedAt"),
cmpopts.IgnoreFields(kong.RBACEndpointPermission{}, "Role", "CreatedAt"),
cmpopts.IgnoreFields(kong.RBACRole{}, "ID", "CreatedAt"),
cmpopts.IgnoreFields(kong.Consumer{}, "CreatedAt"),
cmpopts.IgnoreFields(kong.Consumer{}, "ID", "CreatedAt"),
cmpopts.SortSlices(sortSlices),
cmpopts.SortSlices(func(a, b *string) bool { return *a < *b }),
cmpopts.EquateEmpty(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
_format_version: "1.1"
consumers:
- custom_id: custom_test
username: test
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
_format_version: "1.1"
consumers:
- custom_id: custom_test
username: test_new
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
_format_version: "1.1"
consumers:
- custom_id: custom_test
username: test
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
_format_version: "1.1"
consumers:
- custom_id: new_custom_test
username: test

0 comments on commit c27845a

Please sign in to comment.