feat(admin): add kms admin api by kingluo · Pull Request #8394 · apache/apisix

kingluo · 2022-11-24T11:36:20Z

Description

As part of #8319:

Support adding KMS resources through Admin API.

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)

spacewander · 2022-11-25T06:21:29Z

+        $block->set_value("request", "GET /t");
+    }
+
+    if ((!defined $block->error_log) && (!defined $block->no_error_log)) {


We don't need this now?

spacewander · 2022-11-25T06:26:03Z

+
+
+function _M.delete(id, conf, sub_path)
+    local uri_segs = core.utils.split_uri(sub_path)


Can we refactor the code to get type & id from sub_path in a function?

PATCH requires a different number of segments than GET and PUT, then it makes function wrap a bit complicated.
And the goal is to replace id with real id, and the original id is actually kms type, which is different from other admin resources. And the replacement happens after the invocation, so I think it's trivial to wrap them into function.

But we can share the same code for GET and PUT and DELETE right? This is enough to simplify the implementation instead of copying the code three times.

spacewander · 2022-11-25T06:26:47Z

+
+    local res, err = core.etcd.atomic_set(key, node_value, nil, modified_index)
+    if not res then
+        core.log.error("failed to set new consumer group[", key, "]: ", err)


spacewander · 2022-11-25T06:28:32Z

+function _M.put(id, conf, sub_path)
+    local uri_segs = core.utils.split_uri(sub_path)
+    if #uri_segs ~= 1 then
+        return 400, "no kms id in uri"


Better to keep the style of error message. We should use {error_msg = xxx} like other places

This reverts commit 512c7f4.

This reverts commit a7c86d1.

spacewander

Need to address #8394 (comment) first

spacewander · 2022-11-28T11:58:48Z

+| prefix    | True    | string        | key prefix
+| token     | True    | string      | vault token. |                                                  |
+| desc        | False    | Auxiliary   | Description of usage scenarios.                                                                                    | kms xxxx                                    |
+| labels      | False    | Match Rules | Attributes of the kms specified as key-value pairs.                                                           | {"version":"v2","build":"16","env":"production"} |


Does kms have these fields?

spacewander · 2022-11-29T05:32:48Z

+| uri    | True     | URI        | URI of the vault server.                                                                                              |                                                  |
+| prefix    | True    | string        | key prefix
+| token     | True    | string      | vault token. |                                                  |
+| desc        | False    | Auxiliary   | Description of usage scenarios.                                                                                    | kms xxxx                                    |


The kms_vault doesn't have desc field

* upstream/master: (48 commits) fix(ai): remove BUILD_ROUTER event when ai module is unloaded (apache#8184) chore: add some comment for make_request_to_vault function (apache#8420) docs: update admin api English doc (apache#8227) ci: use fixed os version of ubuntu (apache#8438) feat: Support store secrets in secrets manager for auth plugin via kms components (apache#8421) feat: interact via gRPC in APISIX Admin API (apache#8411) fix: last_err can be nil when the reconnection is successful (apache#8377) feat: support global data encryption of secret information (apache#8403) refactor(env): rename funtion name (apache#8426) feat(admin): add kms admin api (apache#8394) docs: update consumer and upstream docs (apache#8223) ci: add cron job for GM (apache#8398) docs: add kms env doc (apache#8419) feat: Added log format support in syslog plugin. (apache#8279) feat: add vault common components (apache#8412) docs: update global-rule/plugin-config/plugin/ docs (apache#8262) docs: update consumer-group/router/service/script doc (apache#8332) feat: support store secret in env for auth plugin (apache#8390) docs: update Upgrade Guide CN version (apache#8392) docs: add GM plugin EN doc to make website display normally (apache#8393) ...

shreemaan-abhishek

I was trying to use this method and found these difficulties. I am new to Lua so my suggestions may not be of use.

shreemaan-abhishek · 2023-01-11T14:00:30Z

+end
+
+
+function _M.get(id, conf, sub_path)


@kingluo, @soulbird conf is unused.

This is to be consistent with the operation functions of other resources, and they will be called uniformly here https://github.com/apache/apisix/blob/master/apisix/admin/init.lua#L201
In fact, we are refactoring this part, you are welcome to participate in the review #8611

shreemaan-abhishek · 2023-01-11T14:01:47Z

+    end
+
+    utils.fix_count(res.body, id)
+    return res.status, res.body


What do you think about return res, nil? This way the second return variable is always of type error and first one will always be response

kingluo added 3 commits November 24, 2022 19:35

feat(admin): add kms admin api

0df2f44

add code

140cd09

fix lint

8f38567

soulbird reviewed Nov 25, 2022

View reviewed changes

Comment thread apisix/schema_def.lua Outdated

soulbird reviewed Nov 25, 2022

View reviewed changes

Comment thread apisix/schema_def.lua Outdated

soulbird reviewed Nov 25, 2022

View reviewed changes

Comment thread t/admin/kms.t

kingluo added 2 commits November 25, 2022 11:11

fix PR

5141f6f

fix PR

e87027b

spacewander reviewed Nov 25, 2022

View reviewed changes

fix PR

128b88d

kingluo marked this pull request as draft November 25, 2022 09:37

soulbird reviewed Nov 28, 2022

View reviewed changes

Comment thread apisix/constants.lua Outdated

Comment thread t/admin/kms.t

kingluo added 8 commits November 28, 2022 13:59

use /kms, read all without dir indicator

512c7f4

add test case for schema check

2619b8d

Revert "use /kms, read all without dir indicator"

3074631

This reverts commit 512c7f4.

add both /kms and /kms/vault in constants.lua

a7c86d1

add doc

48d762d

Revert "add both /kms and /kms/vault in constants.lua"

b9c458d

This reverts commit a7c86d1.

refactoring

49f374c

fix kms id as {secretmanager}/{id}

b711b03

soulbird marked this pull request as ready for review November 28, 2022 09:42

spacewander reviewed Nov 28, 2022

View reviewed changes

fix PR

ab9f642

spacewander reviewed Nov 28, 2022

View reviewed changes

fix PR

a7564f2

spacewander reviewed Nov 29, 2022

View reviewed changes

fix PR

d9dae5e

spacewander approved these changes Nov 29, 2022

View reviewed changes

soulbird approved these changes Nov 29, 2022

View reviewed changes

tzssangglass approved these changes Nov 29, 2022

View reviewed changes

spacewander merged commit de069aa into apache:master Nov 29, 2022

shreemaan-abhishek reviewed Jan 11, 2023

View reviewed changes



		function _M.delete(id, conf, sub_path)
		local uri_segs = core.utils.split_uri(sub_path)

Uh oh!

Conversation

kingluo commented Nov 24, 2022 • edited by soulbird Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

spacewander left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shreemaan-abhishek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kingluo commented Nov 24, 2022 •

edited by soulbird

Loading