fix: consumer key duplication check #12040

Baoyuantop · 2025-03-11T15:25:19Z

Description

This PR adds a duplicate consumer key check when creating consumers and credentials using the admin api to prevent different consumers from having the same key.

Fixes #11197

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Baoyuantop · 2025-03-14T06:20:23Z

t/admin/credentials.t

@@ -110,7 +110,7 @@ passed
                     "desc": "basic-auth for jack",
                     "plugins": {
                         "basic-auth": {
-                             "username": "the-user",
+                             "username": "the-new-user",


After adding the duplicate judgment, it cannot be added here again, so I made a modification.

Baoyuantop · 2025-03-14T06:23:53Z

t/secret/aws.t

@@ -314,3 +314,5 @@ GET /t
    }
 --- response_body
 all done
+--- error_log


The testing of these changes is an expected change. These tests have an admin api call to the consumer and use a non-existent secret reference, which in the new logic triggers a secret lookup failure and an error log. It didn't break the original test path.

shreemaan-abhishek · 2025-03-17T06:43:52Z

apisix/admin/consumers.lua

@@ -32,7 +34,50 @@ local function check_conf(username, conf, need_username, schema)
    if conf.plugins then
        ok, err = plugins.check_schema(conf.plugins, core.schema.TYPE_CONSUMER)
        if not ok then
-            return nil, {error_msg = "invalid plugins configuration: " .. err}
+            return nil, {
+                error_msg = "invalid plugins configuration: " .. err


we can keep this line unchanged.

shreemaan-abhishek · 2025-03-17T06:48:10Z

apisix/admin/consumers.lua

+                        local consumer, _, err = consumer
+                            .find_consumer(plugin_name, key_field, key_value)
+                        if err then
+                            core.log.warn("failed to find consumer: ", err)


I don't understand the logic here. In the find_consumer() method call above. We are checking if the given consumer key already exists, no? If it doesn't exist then the duplication check should be successful (no duplicate found), right?

If yes, then there is no point in printing a warn log.

When fixing the failure test, I found that when calling the find_consumer() method, its internal call will do a secret find and replace, and when given a non-existing secret, there will be an internal error log, where the err data is captured and printed.
It is also possible to remove this log.

I think you can add this info as a comment in the code.

shreemaan-abhishek · 2025-03-17T06:51:21Z

apisix/admin/consumers.lua

+                            core.log.warn("failed to find consumer: ", err)
+                        end
+
+                        if consumer and consumer.username ~= conf.username then


Is it guaranteed that the returned consumer will have the field username in it? Different consumers have different key fields right? Take your own code snippet for example:

This block will execute only if a consumer with given key_field already exists. So, if it does, we should directly raise an error, no?

The purpose of this is to allow the user to edit a pre-existing consumer. If the consuner being edited matches the username of the consumer being looked up, then the same consumer is being edited, and this behavior should be allowed.

got it. What about my first question?

Is it guaranteed that the returned consumer will have the field username in it? Different consumers have different key fields right? Take your own code snippet for example:

username is a unique field for the consumer and must exist. https://apisix.apache.org/docs/apisix/terminology/consumer/#configuration-options

AlinsRan

Do we not need to check for duplicate keys when watching etcd data?

apisix/admin/consumers.lua

Baoyuantop · 2025-03-27T09:39:36Z

Do we not need to check for duplicate keys when watching etcd data?

Do you mean that etcd can have duplicate keys written to it by other components ? @AlinsRan

Copilot

Copilot wasn't able to review any files in this pull request.

Files not reviewed (8)

apisix/admin/consumers.lua: Language not supported
apisix/admin/credentials.lua: Language not supported
apisix/admin/utils.lua: Language not supported
t/admin/consumers2.t: Language not supported
t/admin/credentials.t: Language not supported
t/secret/aws.t: Language not supported
t/secret/gcp.t: Language not supported
t/secret/secret_lru.t: Language not supported

AlinsRan · 2025-03-28T04:03:30Z

apisix/admin/credentials.lua

            if plugin_obj.type ~= "auth" then
                return nil, {error_msg = "only supports auth type plugins in consumer credential"}
            end
+
+            -- check duplicate key
+            local decrypted_conf = core.table.deepcopy(plugin_conf)


There are too many nested judgments inside, but I think we can extract this logic into a universal function that can be used by both consumers and credentials.

AlinsRan · 2025-03-28T04:05:49Z

Do we not need to check for duplicate keys when watching etcd data?

Do you mean that etcd can have duplicate keys written to it by other components ? @AlinsRan

Components that do not use admin API, such as dashboard.

apisix/admin/consumers.lua

apisix/admin/credentials.lua

t/admin/credentials.t

apisix/admin/consumers.lua

Baoyuantop · 2025-04-03T06:37:12Z

Do we not need to check for duplicate keys when watching etcd data?

Do you mean that etcd can have duplicate keys written to it by other components ? @AlinsRan

Components that do not use admin API, such as dashboard.

I think it can be discussed in a new issue, and this PR only deals with the admin api section

AlinsRan · 2025-04-03T09:43:33Z

apisix/admin/utils.lua

+}
+
+
+function _M.check_duplicate_key(plugins_conf, username, credential_id)


I suggest putting the check_deuplicate_key function in apisix/consumer.lua

Baoyuantop added 6 commits March 11, 2025 23:24

feat: consumer key duplication check

ce80722

fix: improve code logic

489a985

fix: lint

a3a4ac2

test: add test case

fa3caa8

fix: format code

ef0a8ea

fix: format code

80ce431

Baoyuantop marked this pull request as ready for review March 12, 2025 09:51

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request labels Mar 12, 2025

Baoyuantop requested review from nic-6443 and shreemaan-abhishek March 12, 2025 09:51

Baoyuantop added 4 commits March 12, 2025 21:46

fix: test

93a4b63

fix: lint code

5e0fe0f

fix: test

5660ad2

fix: add log

6cb6b78

Baoyuantop commented Mar 14, 2025

View reviewed changes

Baoyuantop requested a review from membphis March 17, 2025 01:31

shreemaan-abhishek reviewed Mar 17, 2025

View reviewed changes

fix: format code

02daa2c

Baoyuantop requested a review from shreemaan-abhishek March 17, 2025 07:44

Baoyuantop changed the title ~~feat: consumer key duplication check~~ fix: consumer key duplication check Mar 21, 2025

Baoyuantop added 3 commits March 21, 2025 17:20

update

3b2aa93

fix: test

7d2d930

fix: lint code

7fe1827

Baoyuantop requested a review from AlinsRan March 25, 2025 08:32

AlinsRan reviewed Mar 26, 2025

View reviewed changes

apisix/admin/consumers.lua Outdated Show resolved Hide resolved

fix: remove error log

38430a0

Baoyuantop requested a review from AlinsRan March 27, 2025 09:52

nic-6443 requested a review from Copilot March 27, 2025 10:02

Copilot AI reviewed Mar 27, 2025

View reviewed changes

AlinsRan reviewed Mar 28, 2025

View reviewed changes

Baoyuantop added this to Apache APISIX backlog Mar 28, 2025

Baoyuantop moved this to 🏗 In progress in Apache APISIX backlog Mar 28, 2025

Baoyuantop moved this from 🏗 In progress to 👀 In review in Apache APISIX backlog Mar 28, 2025

Baoyuantop added 2 commits March 28, 2025 16:42

fix: update code

95c2a92

fix: lint

de68197

nic-6443 reviewed Apr 1, 2025

View reviewed changes

apisix/admin/consumers.lua Outdated Show resolved Hide resolved

nic-6443 reviewed Apr 1, 2025

View reviewed changes

apisix/admin/credentials.lua Outdated Show resolved Hide resolved

nic-6443 reviewed Apr 1, 2025

View reviewed changes

t/admin/credentials.t Outdated Show resolved Hide resolved

fix: update code

b5efe91

dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 2, 2025

fix: update

f5f883d

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Apr 2, 2025

fix: update

e3af265

Baoyuantop requested a review from nic-6443 April 2, 2025 03:29

nic-6443 reviewed Apr 2, 2025

View reviewed changes

apisix/admin/consumers.lua Outdated Show resolved Hide resolved

nic-6443 previously approved these changes Apr 3, 2025

View reviewed changes

update

3717039

Baoyuantop dismissed nic-6443’s stale review via 3717039 April 3, 2025 02:57

nic-6443 previously approved these changes Apr 3, 2025

View reviewed changes

fix: merge function

07b2eca

Baoyuantop dismissed nic-6443’s stale review via 07b2eca April 3, 2025 08:42

fix: lint code

9caab8f

AlinsRan reviewed Apr 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: consumer key duplication check #12040

fix: consumer key duplication check #12040

Baoyuantop commented Mar 11, 2025 •

edited

Loading

Baoyuantop Mar 14, 2025

Baoyuantop Mar 14, 2025

shreemaan-abhishek Mar 17, 2025

shreemaan-abhishek Mar 17, 2025

Baoyuantop Mar 17, 2025

shreemaan-abhishek Mar 17, 2025

shreemaan-abhishek Mar 17, 2025

Baoyuantop Mar 17, 2025

shreemaan-abhishek Mar 17, 2025

Baoyuantop Mar 18, 2025

AlinsRan left a comment

Baoyuantop commented Mar 27, 2025

Copilot AI left a comment

AlinsRan Mar 28, 2025

AlinsRan commented Mar 28, 2025

Baoyuantop commented Apr 3, 2025

AlinsRan Apr 3, 2025

		}


		function _M.check_duplicate_key(plugins_conf, username, credential_id)

fix: consumer key duplication check #12040

Are you sure you want to change the base?

fix: consumer key duplication check #12040

Conversation

Baoyuantop commented Mar 11, 2025 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlinsRan left a comment

Choose a reason for hiding this comment

Baoyuantop commented Mar 27, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlinsRan commented Mar 28, 2025

Baoyuantop commented Apr 3, 2025

Choose a reason for hiding this comment

Baoyuantop commented Mar 11, 2025 •

edited

Loading