You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/11/24 11:36:22 UTC
[GitHub] [apisix] kingluo opened a new pull request, #8394: feat(admin): add kms admin api
kingluo opened a new pull request, #8394:
URL: https://github.com/apache/apisix/pull/8394
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes # (issue)
### 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](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1033343799
##########
apisix/admin/kms.lua:
##########
@@ -0,0 +1,207 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local core = require("apisix.core")
+local utils = require("apisix.admin.utils")
+local type = type
+local tostring = tostring
+
+
+local _M = {
+ need_v3_filter = true,
+}
+
+
+local function check_conf(id, conf, need_id, typ)
+ if not conf then
+ return nil, {error_msg = "missing configurations"}
+ end
+
+ id = id or conf.id
+ if need_id and not id then
+ return nil, {error_msg = "missing id"}
+ end
+
+ if not need_id and id then
+ return nil, {error_msg = "wrong id, do not need it"}
+ end
+
+ if need_id and conf.id and tostring(conf.id) ~= tostring(id) then
+ return nil, {error_msg = "wrong id"}
+ end
+
+ conf.id = id
+
+ core.log.info("conf: ", core.json.delay_encode(conf))
+ local ok, err = core.schema.check(core.schema["kms_" .. typ], conf)
+ if not ok then
+ return nil, {error_msg = "invalid configuration: " .. err}
+ end
+
+ return true
+end
+
+
+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"
+ end
+ local typ = id
+ id = uri_segs[1]
+
+ local ok, err = check_conf(id, conf, true, typ)
+ if not ok then
+ return 400, err
+ end
+
+ local key = "/kms/" .. typ .. "/" .. id
+
+ local ok, err = utils.inject_conf_with_prev_conf("kms", key, conf)
+ if not ok then
+ return 503, {error_msg = err}
+ end
+
+ local res, err = core.etcd.set(key, conf)
+ if not res then
+ core.log.error("failed to put kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ return res.status, res.body
+end
+
+
+function _M.get(id, conf, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
+ local typ = id
+ if typ == nil then
+ return 404, '{"error_msg":"not found"}\n'
+ end
+ id = nil
+ if #uri_segs > 0 then
+ id = uri_segs[1]
+ end
+
+ local key = "/kms/" .. typ
+ if id then
+ key = key .. "/" .. id
+ end
+
+ local res, err = core.etcd.get(key, not id)
+ if not res then
+ core.log.error("failed to get kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ utils.fix_count(res.body, id)
+ return res.status, res.body
+end
+
+
+function _M.delete(id, conf, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1031940451
##########
apisix/schema_def.lua:
##########
@@ -692,6 +692,23 @@ _M.service = {
}
+_M.vault = {
Review Comment:
Use `kms_vault`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] shreemaan-abhishek commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1067026201
##########
apisix/admin/kms.lua:
##########
@@ -0,0 +1,203 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local core = require("apisix.core")
+local utils = require("apisix.admin.utils")
+local type = type
+local tostring = tostring
+
+
+local _M = {
+ need_v3_filter = true,
+}
+
+
+local function check_conf(id, conf, need_id, typ)
+ if not conf then
+ return nil, {error_msg = "missing configurations"}
+ end
+
+ id = id or conf.id
+ if need_id and not id then
+ return nil, {error_msg = "missing id"}
+ end
+
+ if not need_id and id then
+ return nil, {error_msg = "wrong id, do not need it"}
+ end
+
+ if need_id and conf.id and tostring(conf.id) ~= tostring(id) then
+ return nil, {error_msg = "wrong id"}
+ end
+
+ conf.id = id
+
+ core.log.info("conf: ", core.json.delay_encode(conf))
+ local ok, err = core.schema.check(core.schema["kms_" .. typ], conf)
+ if not ok then
+ return nil, {error_msg = "invalid configuration: " .. err}
+ end
+
+ return true
+end
+
+
+local function split_typ_and_id(id, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
+ local typ = id
+ local id = nil
+ if #uri_segs > 0 then
+ id = uri_segs[1]
+ end
+ return typ, id
+end
+
+
+function _M.put(id, conf, sub_path)
+ local typ, id = split_typ_and_id(id, sub_path)
+ if not id then
+ return 400, {error_msg = "no kms id in uri"}
+ end
+
+ local ok, err = check_conf(typ .. "/" .. id, conf, true, typ)
+ if not ok then
+ return 400, err
+ end
+
+ local key = "/kms/" .. typ .. "/" .. id
+
+ local ok, err = utils.inject_conf_with_prev_conf("kms", key, conf)
+ if not ok then
+ return 503, {error_msg = err}
+ end
+
+ local res, err = core.etcd.set(key, conf)
+ if not res then
+ core.log.error("failed to put kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ return res.status, res.body
+end
+
+
+function _M.get(id, conf, sub_path)
Review Comment:
@kingluo, @soulbird `conf` is unused.
##########
apisix/admin/kms.lua:
##########
@@ -0,0 +1,203 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local core = require("apisix.core")
+local utils = require("apisix.admin.utils")
+local type = type
+local tostring = tostring
+
+
+local _M = {
+ need_v3_filter = true,
+}
+
+
+local function check_conf(id, conf, need_id, typ)
+ if not conf then
+ return nil, {error_msg = "missing configurations"}
+ end
+
+ id = id or conf.id
+ if need_id and not id then
+ return nil, {error_msg = "missing id"}
+ end
+
+ if not need_id and id then
+ return nil, {error_msg = "wrong id, do not need it"}
+ end
+
+ if need_id and conf.id and tostring(conf.id) ~= tostring(id) then
+ return nil, {error_msg = "wrong id"}
+ end
+
+ conf.id = id
+
+ core.log.info("conf: ", core.json.delay_encode(conf))
+ local ok, err = core.schema.check(core.schema["kms_" .. typ], conf)
+ if not ok then
+ return nil, {error_msg = "invalid configuration: " .. err}
+ end
+
+ return true
+end
+
+
+local function split_typ_and_id(id, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
+ local typ = id
+ local id = nil
+ if #uri_segs > 0 then
+ id = uri_segs[1]
+ end
+ return typ, id
+end
+
+
+function _M.put(id, conf, sub_path)
+ local typ, id = split_typ_and_id(id, sub_path)
+ if not id then
+ return 400, {error_msg = "no kms id in uri"}
+ end
+
+ local ok, err = check_conf(typ .. "/" .. id, conf, true, typ)
+ if not ok then
+ return 400, err
+ end
+
+ local key = "/kms/" .. typ .. "/" .. id
+
+ local ok, err = utils.inject_conf_with_prev_conf("kms", key, conf)
+ if not ok then
+ return 503, {error_msg = err}
+ end
+
+ local res, err = core.etcd.set(key, conf)
+ if not res then
+ core.log.error("failed to put kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ return res.status, res.body
+end
+
+
+function _M.get(id, conf, sub_path)
+ local typ, id = split_typ_and_id(id, sub_path)
+
+ local key = "/kms/"
+ if id then
+ key = key .. typ
+ key = key .. "/" .. id
+ end
+
+ local res, err = core.etcd.get(key, not id)
+ if not res then
+ core.log.error("failed to get kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ utils.fix_count(res.body, id)
+ return res.status, res.body
Review Comment:
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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1031938164
##########
apisix/schema_def.lua:
##########
@@ -692,6 +692,23 @@ _M.service = {
}
+_M.vault = {
+ type = "object",
+ properties = {
+ uri = {
+ type = "string",
Review Comment:
Should use `uri_def`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1033175904
##########
apisix/constants.lua:
##########
@@ -33,6 +33,7 @@ return {
["/protos"] = true,
["/plugin_configs"] = true,
["/consumer_groups"] = true,
+ ["/kms/vault"] = true,
Review Comment:
Add both `/kms` and `/kms/vault` makes it work.
Because we need to get all resources both for `/kms` and `/kms/vault` prefix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #8394:
URL: https://github.com/apache/apisix/pull/8394
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1067038996
##########
apisix/admin/kms.lua:
##########
@@ -0,0 +1,203 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local core = require("apisix.core")
+local utils = require("apisix.admin.utils")
+local type = type
+local tostring = tostring
+
+
+local _M = {
+ need_v3_filter = true,
+}
+
+
+local function check_conf(id, conf, need_id, typ)
+ if not conf then
+ return nil, {error_msg = "missing configurations"}
+ end
+
+ id = id or conf.id
+ if need_id and not id then
+ return nil, {error_msg = "missing id"}
+ end
+
+ if not need_id and id then
+ return nil, {error_msg = "wrong id, do not need it"}
+ end
+
+ if need_id and conf.id and tostring(conf.id) ~= tostring(id) then
+ return nil, {error_msg = "wrong id"}
+ end
+
+ conf.id = id
+
+ core.log.info("conf: ", core.json.delay_encode(conf))
+ local ok, err = core.schema.check(core.schema["kms_" .. typ], conf)
+ if not ok then
+ return nil, {error_msg = "invalid configuration: " .. err}
+ end
+
+ return true
+end
+
+
+local function split_typ_and_id(id, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
+ local typ = id
+ local id = nil
+ if #uri_segs > 0 then
+ id = uri_segs[1]
+ end
+ return typ, id
+end
+
+
+function _M.put(id, conf, sub_path)
+ local typ, id = split_typ_and_id(id, sub_path)
+ if not id then
+ return 400, {error_msg = "no kms id in uri"}
+ end
+
+ local ok, err = check_conf(typ .. "/" .. id, conf, true, typ)
+ if not ok then
+ return 400, err
+ end
+
+ local key = "/kms/" .. typ .. "/" .. id
+
+ local ok, err = utils.inject_conf_with_prev_conf("kms", key, conf)
+ if not ok then
+ return 503, {error_msg = err}
+ end
+
+ local res, err = core.etcd.set(key, conf)
+ if not res then
+ core.log.error("failed to put kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ return res.status, res.body
+end
+
+
+function _M.get(id, conf, sub_path)
Review Comment:
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 https://github.com/apache/apisix/pull/8611
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1067040193
##########
apisix/admin/kms.lua:
##########
@@ -0,0 +1,203 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local core = require("apisix.core")
+local utils = require("apisix.admin.utils")
+local type = type
+local tostring = tostring
+
+
+local _M = {
+ need_v3_filter = true,
+}
+
+
+local function check_conf(id, conf, need_id, typ)
+ if not conf then
+ return nil, {error_msg = "missing configurations"}
+ end
+
+ id = id or conf.id
+ if need_id and not id then
+ return nil, {error_msg = "missing id"}
+ end
+
+ if not need_id and id then
+ return nil, {error_msg = "wrong id, do not need it"}
+ end
+
+ if need_id and conf.id and tostring(conf.id) ~= tostring(id) then
+ return nil, {error_msg = "wrong id"}
+ end
+
+ conf.id = id
+
+ core.log.info("conf: ", core.json.delay_encode(conf))
+ local ok, err = core.schema.check(core.schema["kms_" .. typ], conf)
+ if not ok then
+ return nil, {error_msg = "invalid configuration: " .. err}
+ end
+
+ return true
+end
+
+
+local function split_typ_and_id(id, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
+ local typ = id
+ local id = nil
+ if #uri_segs > 0 then
+ id = uri_segs[1]
+ end
+ return typ, id
+end
+
+
+function _M.put(id, conf, sub_path)
+ local typ, id = split_typ_and_id(id, sub_path)
+ if not id then
+ return 400, {error_msg = "no kms id in uri"}
+ end
+
+ local ok, err = check_conf(typ .. "/" .. id, conf, true, typ)
+ if not ok then
+ return 400, err
+ end
+
+ local key = "/kms/" .. typ .. "/" .. id
+
+ local ok, err = utils.inject_conf_with_prev_conf("kms", key, conf)
+ if not ok then
+ return 503, {error_msg = err}
+ end
+
+ local res, err = core.etcd.set(key, conf)
+ if not res then
+ core.log.error("failed to put kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ return res.status, res.body
+end
+
+
+function _M.get(id, conf, sub_path)
+ local typ, id = split_typ_and_id(id, sub_path)
+
+ local key = "/kms/"
+ if id then
+ key = key .. typ
+ key = key .. "/" .. id
+ end
+
+ local res, err = core.etcd.get(key, not id)
+ if not res then
+ core.log.error("failed to get kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ utils.fix_count(res.body, id)
+ return res.status, res.body
Review Comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1033055981
##########
apisix/constants.lua:
##########
@@ -33,6 +33,7 @@ return {
["/protos"] = true,
["/plugin_configs"] = true,
["/consumer_groups"] = true,
+ ["/kms/vault"] = true,
Review Comment:
`["/kms"] = true` is enough. We need to load all kms configs on init_worker, not just vault.
##########
t/admin/kms.t:
##########
@@ -0,0 +1,188 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+log_level("info");
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (!$block->request) {
+ $block->set_value("request", "GET /t");
+ }
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: PUT
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local etcd = require("apisix.core.etcd")
+ local code, body = t('/apisix/admin/kms/vault/test1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "http://127.0.0.1:12800/get",
+ "prefix" : "apisix",
+ "token" : "apisix"
+ }]],
+ [[{
+ "value": {
+ "uri": "http://127.0.0.1:12800/get",
+ "prefix" : "apisix",
+ "token" : "apisix"
+ },
+ "key": "/apisix/kms/vault/test1"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+
+ local res = assert(etcd.get('/kms/vault/test1'))
+ local create_time = res.body.node.value.create_time
+ assert(create_time ~= nil, "create_time is nil")
+ local update_time = res.body.node.value.update_time
+ assert(update_time ~= nil, "update_time is nil")
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 2: GET
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/kms/vault/test1',
+ ngx.HTTP_GET,
+ nil,
+ [[{
+ "value": {
+ "uri": "http://127.0.0.1:12800/get",
+ "prefix" : "apisix",
+ "token" : "apisix"
+ },
+ "key": "/apisix/kms/vault/test1"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 3: GET all
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/kms/vault',
+ ngx.HTTP_GET,
+ nil,
+ [[{
+ "total": 1,
+ "list": [
+ {
+ "key": "/apisix/kms/vault/test1",
+ "value": {
+ "uri": "http://127.0.0.1:12800/get",
+ "prefix" : "apisix",
+ "token" : "apisix"
+ }
+ }
+ ]
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 4: PATCH
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local etcd = require("apisix.core.etcd")
+ local res = assert(etcd.get('/kms/vault/test1'))
+ local prev_create_time = res.body.node.value.create_time
+ assert(prev_create_time ~= nil, "create_time is nil")
+ local prev_update_time = res.body.node.value.update_time
+ assert(prev_update_time ~= nil, "update_time is nil")
+ ngx.sleep(1)
+
+ local code, body = t('/apisix/admin/kms/vault/test1/token',
+ ngx.HTTP_PATCH,
+ [["unknown"]],
+ [[{
+ "value": {
+ "uri": "http://127.0.0.1:12800/get",
+ "prefix" : "apisix",
+ "token" : "unknown"
+ },
+ "key": "/apisix/kms/vault/test1"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+
+ local res = assert(etcd.get('/kms/vault/test1'))
+ local create_time = res.body.node.value.create_time
+ assert(prev_create_time == create_time, "create_time mismatched")
+ local update_time = res.body.node.value.update_time
+ assert(update_time ~= nil, "update_time is nil")
+ assert(prev_update_time ~= update_time, "update_time should be changed")
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 5: DELETE
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/kms/vault/test1',
+ ngx.HTTP_DELETE
+ )
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
Review Comment:
Add test case for schema.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1031940650
##########
t/admin/kms.t:
##########
@@ -0,0 +1,192 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+log_level("info");
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (!$block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
+ $block->set_value("no_error_log", "[error]\n[alert]");
+ }
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: PUT
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local etcd = require("apisix.core.etcd")
+ local code, body = t('/apisix/admin/kms/vault/test1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/get",
+ "prefix" : "apisix",
+ "token" : "apisix"
+ }]],
+ [[{
+ "value": {
+ "uri": "/get",
+ "prefix" : "apisix",
+ "token" : "apisix"
+ },
+ "key": "/apisix/kms/vault/test1"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+
+ local res = assert(etcd.get('/kms/vault/test1'))
+ local create_time = res.body.node.value.create_time
+ assert(create_time ~= nil, "create_time is nil")
+ local update_time = res.body.node.value.update_time
+ assert(update_time ~= nil, "update_time is nil")
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 2: GET
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/kms/vault/test1',
+ ngx.HTTP_GET,
+ nil,
+ [[{
+ "value": {
+ "uri": "/get",
+ "prefix" : "apisix",
+ "token" : "apisix"
+ },
+ "key": "/apisix/kms/vault/test1"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 3: GET all
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/kms/vault',
+ ngx.HTTP_GET,
+ nil,
+ [[{
+ "total": 1,
+ "list": [
+ {
+ "key": "/apisix/kms/vault/test1",
+ "value": {
+ "uri": "/get",
+ "prefix" : "apisix",
+ "token" : "apisix"
+ }
+ }
+ ]
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 4: PATCH
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local etcd = require("apisix.core.etcd")
+ local res = assert(etcd.get('/kms/vault/test1'))
+ local prev_create_time = res.body.node.value.create_time
+ assert(prev_create_time ~= nil, "create_time is nil")
+ local prev_update_time = res.body.node.value.update_time
+ assert(prev_update_time ~= nil, "update_time is nil")
+ ngx.sleep(1)
+
+ local code, body = t('/apisix/admin/kms/vault/test1/token',
+ ngx.HTTP_PATCH,
+ [["unknown"]],
+ [[{
+ "value": {
+ "uri": "/get",
+ "prefix" : "apisix",
+ "token" : "unknown"
+ },
+ "key": "/apisix/kms/vault/test1"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+
+ local res = assert(etcd.get('/kms/vault/test1'))
+ local create_time = res.body.node.value.create_time
+ assert(prev_create_time == create_time, "create_time mismatched")
+ local update_time = res.body.node.value.update_time
+ assert(update_time ~= nil, "update_time is nil")
+ assert(prev_update_time ~= update_time, "update_time should be changed")
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 5: delete consumer-group
+--- config
Review Comment:
typo?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1032064959
##########
apisix/admin/kms.lua:
##########
@@ -0,0 +1,207 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local core = require("apisix.core")
+local utils = require("apisix.admin.utils")
+local type = type
+local tostring = tostring
+
+
+local _M = {
+ need_v3_filter = true,
+}
+
+
+local function check_conf(id, conf, need_id, typ)
+ if not conf then
+ return nil, {error_msg = "missing configurations"}
+ end
+
+ id = id or conf.id
+ if need_id and not id then
+ return nil, {error_msg = "missing id"}
+ end
+
+ if not need_id and id then
+ return nil, {error_msg = "wrong id, do not need it"}
+ end
+
+ if need_id and conf.id and tostring(conf.id) ~= tostring(id) then
+ return nil, {error_msg = "wrong id"}
+ end
+
+ conf.id = id
+
+ core.log.info("conf: ", core.json.delay_encode(conf))
+ local ok, err = core.schema.check(core.schema["kms_" .. typ], conf)
+ if not ok then
+ return nil, {error_msg = "invalid configuration: " .. err}
+ end
+
+ return true
+end
+
+
+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"
+ end
+ local typ = id
+ id = uri_segs[1]
+
+ local ok, err = check_conf(id, conf, true, typ)
+ if not ok then
+ return 400, err
+ end
+
+ local key = "/kms/" .. typ .. "/" .. id
+
+ local ok, err = utils.inject_conf_with_prev_conf("kms", key, conf)
+ if not ok then
+ return 503, {error_msg = err}
+ end
+
+ local res, err = core.etcd.set(key, conf)
+ if not res then
+ core.log.error("failed to put kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ return res.status, res.body
+end
+
+
+function _M.get(id, conf, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
+ local typ = id
+ if typ == nil then
+ return 404, '{"error_msg":"not found"}\n'
+ end
+ id = nil
+ if #uri_segs > 0 then
+ id = uri_segs[1]
+ end
+
+ local key = "/kms/" .. typ
+ if id then
+ key = key .. "/" .. id
+ end
+
+ local res, err = core.etcd.get(key, not id)
+ if not res then
+ core.log.error("failed to get kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ utils.fix_count(res.body, id)
+ return res.status, res.body
+end
+
+
+function _M.delete(id, conf, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1033450996
##########
apisix/admin/kms.lua:
##########
@@ -0,0 +1,207 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local core = require("apisix.core")
+local utils = require("apisix.admin.utils")
+local type = type
+local tostring = tostring
+
+
+local _M = {
+ need_v3_filter = true,
+}
+
+
+local function check_conf(id, conf, need_id, typ)
+ if not conf then
+ return nil, {error_msg = "missing configurations"}
+ end
+
+ id = id or conf.id
+ if need_id and not id then
+ return nil, {error_msg = "missing id"}
+ end
+
+ if not need_id and id then
+ return nil, {error_msg = "wrong id, do not need it"}
+ end
+
+ if need_id and conf.id and tostring(conf.id) ~= tostring(id) then
+ return nil, {error_msg = "wrong id"}
+ end
+
+ conf.id = id
+
+ core.log.info("conf: ", core.json.delay_encode(conf))
+ local ok, err = core.schema.check(core.schema["kms_" .. typ], conf)
+ if not ok then
+ return nil, {error_msg = "invalid configuration: " .. err}
+ end
+
+ return true
+end
+
+
+local function split_typ_and_id(id, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
+ local typ = id
+ local id = nil
+ if #uri_segs > 0 then
+ id = uri_segs[1]
+ end
+ return typ, id
+end
+
+
+function _M.put(id, conf, sub_path)
+ local typ, id = split_typ_and_id(id, sub_path)
+ if not id then
+ return 400, {error_msg = "no kms id in uri"}
+ end
+
+ local ok, err = check_conf(typ .. "/" .. id, conf, true, typ)
+ if not ok then
+ return 400, {error_msg = err}
+ end
+
+ local key = "/kms/" .. typ .. "/" .. id
+
+ local ok, err = utils.inject_conf_with_prev_conf("kms", key, conf)
+ if not ok then
+ return 503, {error_msg = err}
+ end
+
+ local res, err = core.etcd.set(key, conf)
+ if not res then
+ core.log.error("failed to put kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ return res.status, res.body
+end
+
+
+function _M.get(id, conf, sub_path)
+ local typ, id = split_typ_and_id(id, sub_path)
+
+ local key = "/kms/"
+ if id then
+ key = key .. typ
+ key = key .. "/" .. id
+ end
+
+ local res, err = core.etcd.get(key, not id)
+ if not res then
+ core.log.error("failed to get kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ utils.fix_count(res.body, id)
+ return res.status, res.body
+end
+
+
+function _M.delete(id, conf, sub_path)
+ local typ, id = split_typ_and_id(id, sub_path)
+ if not id then
+ return 400, {error_msg = "no kms id in uri"}
+ end
+
+ if not id then
Review Comment:
Why check the id twice?
##########
docs/en/latest/admin-api.md:
##########
@@ -1119,3 +1120,65 @@ Route used in the [Stream Proxy](./stream-proxy.md).
To learn more about filtering in stream proxies, check [this](./stream-proxy.md#more-route-match-options) document.
[Back to TOC](#table-of-contents)
+
+## kms
+
+**API**: /apisix/admin/kms/{secretmanager}/{id}
+
+kms means `Secrets Management`, which could use any secret manager supported, e.g. `vault`.
+
+### Request Methods
+
+| Method | Request URI | Request Body | Description |
+| ------ | ---------------------------------- | ------------ | ------------------------------------------------- |
+| GET | /apisix/admin/kms | NULL | Fetches a list of all kms. |
+| GET | /apisix/admin/kms/{secretmanager}/{id} | NULL | Fetches specified kms by id. |
+| PUT | /apisix/admin/kms/{secretmanager} | {...} | Create new kms configuration. |
+| DELETE | /apisix/admin/kms/{secretmanager}/{id} | NULL | Removes the kms with the specified id. |
+| PATCH | /apisix/admin/kms/{secretmanager}/{id} | {...} | Updates the selected attributes of the specified, existing kms. To delete an attribute, set value of attribute set to null. |
+| PATCH | /apisix/admin/kms/{secretmanager}/{id}/{path} | {...} | Updates the attribute specified in the path. The values of other attributes remain unchanged. |
+
+### Request Body Parameters
+
+When `{secretmanager}` is `vault`:
+
+| Parameter | Required | Type | Description | Example |
+| ----------- | -------- | ----------- | ------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------ |
+| 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 |
+| labels | False | Match Rules | Attributes of the kms specified as key-value pairs. | {"version":"v2","build":"16","env":"production"} |
Review Comment:
Does kms have these fields?
##########
apisix/admin/kms.lua:
##########
@@ -0,0 +1,207 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local core = require("apisix.core")
+local utils = require("apisix.admin.utils")
+local type = type
+local tostring = tostring
+
+
+local _M = {
+ need_v3_filter = true,
+}
+
+
+local function check_conf(id, conf, need_id, typ)
+ if not conf then
+ return nil, {error_msg = "missing configurations"}
+ end
+
+ id = id or conf.id
+ if need_id and not id then
+ return nil, {error_msg = "missing id"}
+ end
+
+ if not need_id and id then
+ return nil, {error_msg = "wrong id, do not need it"}
+ end
+
+ if need_id and conf.id and tostring(conf.id) ~= tostring(id) then
+ return nil, {error_msg = "wrong id"}
+ end
+
+ conf.id = id
+
+ core.log.info("conf: ", core.json.delay_encode(conf))
+ local ok, err = core.schema.check(core.schema["kms_" .. typ], conf)
+ if not ok then
+ return nil, {error_msg = "invalid configuration: " .. err}
+ end
+
+ return true
+end
+
+
+local function split_typ_and_id(id, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
+ local typ = id
+ local id = nil
+ if #uri_segs > 0 then
+ id = uri_segs[1]
+ end
+ return typ, id
+end
+
+
+function _M.put(id, conf, sub_path)
+ local typ, id = split_typ_and_id(id, sub_path)
+ if not id then
+ return 400, {error_msg = "no kms id in uri"}
+ end
+
+ local ok, err = check_conf(typ .. "/" .. id, conf, true, typ)
+ if not ok then
+ return 400, {error_msg = err}
Review Comment:
The err from check_conf already contains error_msg field.
##########
docs/en/latest/admin-api.md:
##########
@@ -1119,3 +1120,65 @@ Route used in the [Stream Proxy](./stream-proxy.md).
To learn more about filtering in stream proxies, check [this](./stream-proxy.md#more-route-match-options) document.
[Back to TOC](#table-of-contents)
+
+## kms
+
+**API**: /apisix/admin/kms/{secretmanager}/{id}
+
+kms means `Secrets Management`, which could use any secret manager supported, e.g. `vault`.
Review Comment:
I am curious why kms means `Secrets Management`. `kms` is not the abbreviation of Secrets Management.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1034323376
##########
t/admin/kms.t:
##########
@@ -0,0 +1,221 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+log_level("info");
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (!$block->request) {
+ $block->set_value("request", "GET /t");
+ }
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: PUT
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local etcd = require("apisix.core.etcd")
+ local code, body = t('/apisix/admin/kms/vault/test1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "http://127.0.0.1:12800/get",
+ "prefix" : "apisix",
+ "token" : "apisix"
+ }]],
+ [[{
+ "value": {
+ "uri": "http://127.0.0.1:12800/get",
+ "prefix" : "apisix",
+ "token" : "apisix"
+ },
+ "key": "/apisix/kms/vault/test1"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+
+ local res = assert(etcd.get('/kms/vault/test1'))
+ local create_time = res.body.node.value.create_time
Review Comment:
My bad. These timestamps are automatically injected in the Admin API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
kingluo commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1033513977
##########
docs/en/latest/admin-api.md:
##########
@@ -1119,3 +1120,65 @@ Route used in the [Stream Proxy](./stream-proxy.md).
To learn more about filtering in stream proxies, check [this](./stream-proxy.md#more-route-match-options) document.
[Back to TOC](#table-of-contents)
+
+## kms
+
+**API**: /apisix/admin/kms/{secretmanager}/{id}
+
+kms means `Secrets Management`, which could use any secret manager supported, e.g. `vault`.
Review Comment:
@soulbird Please help here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1032039956
##########
apisix/admin/kms.lua:
##########
@@ -0,0 +1,207 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local core = require("apisix.core")
+local utils = require("apisix.admin.utils")
+local type = type
+local tostring = tostring
+
+
+local _M = {
+ need_v3_filter = true,
+}
+
+
+local function check_conf(id, conf, need_id, typ)
+ if not conf then
+ return nil, {error_msg = "missing configurations"}
+ end
+
+ id = id or conf.id
+ if need_id and not id then
+ return nil, {error_msg = "missing id"}
+ end
+
+ if not need_id and id then
+ return nil, {error_msg = "wrong id, do not need it"}
+ end
+
+ if need_id and conf.id and tostring(conf.id) ~= tostring(id) then
+ return nil, {error_msg = "wrong id"}
+ end
+
+ conf.id = id
+
+ core.log.info("conf: ", core.json.delay_encode(conf))
+ local ok, err = core.schema.check(core.schema["kms_" .. typ], conf)
+ if not ok then
+ return nil, {error_msg = "invalid configuration: " .. err}
+ end
+
+ return true
+end
+
+
+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"
+ end
+ local typ = id
+ id = uri_segs[1]
+
+ local ok, err = check_conf(id, conf, true, typ)
+ if not ok then
+ return 400, err
+ end
+
+ local key = "/kms/" .. typ .. "/" .. id
+
+ local ok, err = utils.inject_conf_with_prev_conf("kms", key, conf)
+ if not ok then
+ return 503, {error_msg = err}
+ end
+
+ local res, err = core.etcd.set(key, conf)
+ if not res then
+ core.log.error("failed to put kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ return res.status, res.body
+end
+
+
+function _M.get(id, conf, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
+ local typ = id
+ if typ == nil then
+ return 404, '{"error_msg":"not found"}\n'
+ end
+ id = nil
+ if #uri_segs > 0 then
+ id = uri_segs[1]
+ end
+
+ local key = "/kms/" .. typ
+ if id then
+ key = key .. "/" .. id
+ end
+
+ local res, err = core.etcd.get(key, not id)
+ if not res then
+ core.log.error("failed to get kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ utils.fix_count(res.body, id)
+ return res.status, res.body
+end
+
+
+function _M.delete(id, conf, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
Review Comment:
Can we refactor the code to get type & id from sub_path in a function?
##########
apisix/admin/kms.lua:
##########
@@ -0,0 +1,207 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local core = require("apisix.core")
+local utils = require("apisix.admin.utils")
+local type = type
+local tostring = tostring
+
+
+local _M = {
+ need_v3_filter = true,
+}
+
+
+local function check_conf(id, conf, need_id, typ)
+ if not conf then
+ return nil, {error_msg = "missing configurations"}
+ end
+
+ id = id or conf.id
+ if need_id and not id then
+ return nil, {error_msg = "missing id"}
+ end
+
+ if not need_id and id then
+ return nil, {error_msg = "wrong id, do not need it"}
+ end
+
+ if need_id and conf.id and tostring(conf.id) ~= tostring(id) then
+ return nil, {error_msg = "wrong id"}
+ end
+
+ conf.id = id
+
+ core.log.info("conf: ", core.json.delay_encode(conf))
+ local ok, err = core.schema.check(core.schema["kms_" .. typ], conf)
+ if not ok then
+ return nil, {error_msg = "invalid configuration: " .. err}
+ end
+
+ return true
+end
+
+
+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"
Review Comment:
Better to keep the style of error message. We should use `{error_msg = xxx}` like other places
##########
apisix/admin/kms.lua:
##########
@@ -0,0 +1,207 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+local core = require("apisix.core")
+local utils = require("apisix.admin.utils")
+local type = type
+local tostring = tostring
+
+
+local _M = {
+ need_v3_filter = true,
+}
+
+
+local function check_conf(id, conf, need_id, typ)
+ if not conf then
+ return nil, {error_msg = "missing configurations"}
+ end
+
+ id = id or conf.id
+ if need_id and not id then
+ return nil, {error_msg = "missing id"}
+ end
+
+ if not need_id and id then
+ return nil, {error_msg = "wrong id, do not need it"}
+ end
+
+ if need_id and conf.id and tostring(conf.id) ~= tostring(id) then
+ return nil, {error_msg = "wrong id"}
+ end
+
+ conf.id = id
+
+ core.log.info("conf: ", core.json.delay_encode(conf))
+ local ok, err = core.schema.check(core.schema["kms_" .. typ], conf)
+ if not ok then
+ return nil, {error_msg = "invalid configuration: " .. err}
+ end
+
+ return true
+end
+
+
+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"
+ end
+ local typ = id
+ id = uri_segs[1]
+
+ local ok, err = check_conf(id, conf, true, typ)
+ if not ok then
+ return 400, err
+ end
+
+ local key = "/kms/" .. typ .. "/" .. id
+
+ local ok, err = utils.inject_conf_with_prev_conf("kms", key, conf)
+ if not ok then
+ return 503, {error_msg = err}
+ end
+
+ local res, err = core.etcd.set(key, conf)
+ if not res then
+ core.log.error("failed to put kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ return res.status, res.body
+end
+
+
+function _M.get(id, conf, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
+ local typ = id
+ if typ == nil then
+ return 404, '{"error_msg":"not found"}\n'
+ end
+ id = nil
+ if #uri_segs > 0 then
+ id = uri_segs[1]
+ end
+
+ local key = "/kms/" .. typ
+ if id then
+ key = key .. "/" .. id
+ end
+
+ local res, err = core.etcd.get(key, not id)
+ if not res then
+ core.log.error("failed to get kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ utils.fix_count(res.body, id)
+ return res.status, res.body
+end
+
+
+function _M.delete(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"
+ end
+ local typ = id
+ id = uri_segs[1]
+
+ if not id then
+ return 400, {error_msg = "missing kms id"}
+ end
+
+ local key = "/kms/" .. typ .. "/" .. id
+
+ local res, err = core.etcd.delete(key)
+ if not res then
+ core.log.error("failed to delete kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ return res.status, res.body
+end
+
+
+function _M.patch(id, conf, sub_path)
+ local uri_segs = core.utils.split_uri(sub_path)
+ if #uri_segs < 2 then
+ return 400, "no kms id and/or sub path in uri"
+ end
+ local typ = id
+ id = uri_segs[1]
+ sub_path = core.table.concat(uri_segs, "/", 2)
+
+ if not id then
+ return 400, {error_msg = "missing kms id"}
+ end
+
+ if not conf then
+ return 400, {error_msg = "missing new configuration"}
+ end
+
+ if not sub_path or sub_path == "" then
+ if type(conf) ~= "table" then
+ return 400, {error_msg = "invalid configuration"}
+ end
+ end
+
+ local key = "/kms/" .. typ .. "/" .. id
+ local res_old, err = core.etcd.get(key)
+ if not res_old then
+ core.log.error("failed to get kms [", key, "]: ", err)
+ return 503, {error_msg = err}
+ end
+
+ if res_old.status ~= 200 then
+ return res_old.status, res_old.body
+ end
+ core.log.info("key: ", key, " old value: ",
+ core.json.delay_encode(res_old, true))
+
+ local node_value = res_old.body.node.value
+ local modified_index = res_old.body.node.modifiedIndex
+
+ if sub_path and sub_path ~= "" then
+ local code, err, node_val = core.table.patch(node_value, sub_path, conf)
+ node_value = node_val
+ if code then
+ return code, err
+ end
+ utils.inject_timestamp(node_value, nil, true)
+ else
+ node_value = core.table.merge(node_value, conf)
+ utils.inject_timestamp(node_value, nil, conf)
+ end
+
+ core.log.info("new conf: ", core.json.delay_encode(node_value, true))
+
+ local ok, err = check_conf(id, node_value, true, typ)
+ if not ok then
+ return 400, err
+ end
+
+ 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)
Review Comment:
Typo?
##########
t/admin/kms.t:
##########
@@ -0,0 +1,192 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+log_level("info");
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (!$block->request) {
+ $block->set_value("request", "GET /t");
+ }
+
+ if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
Review Comment:
We don't need this now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1034263159
##########
docs/en/latest/admin-api.md:
##########
@@ -1119,3 +1120,65 @@ Route used in the [Stream Proxy](./stream-proxy.md).
To learn more about filtering in stream proxies, check [this](./stream-proxy.md#more-route-match-options) document.
[Back to TOC](#table-of-contents)
+
+## kms
+
+**API**: /apisix/admin/kms/{secretmanager}/{id}
+
+kms means `Secrets Management`, which could use any secret manager supported, e.g. `vault`.
Review Comment:
There is currently no good acronym to summarize `secrets management`, kms seems relatively appropriate. I think we can weaken kms first without explaining it. So is it better to change it to something like this:
With the kms resource, you can use any secrets management service already supported in APISIX. @spacewander
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8394: feat(admin): add kms admin api
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8394:
URL: https://github.com/apache/apisix/pull/8394#discussion_r1034316393
##########
docs/en/latest/admin-api.md:
##########
@@ -1119,3 +1120,62 @@ Route used in the [Stream Proxy](./stream-proxy.md).
To learn more about filtering in stream proxies, check [this](./stream-proxy.md#more-route-match-options) document.
[Back to TOC](#table-of-contents)
+
+## kms
+
+**API**: /apisix/admin/kms/{secretmanager}/{id}
+
+kms means `Secrets Management`, which could use any secret manager supported, e.g. `vault`.
+
+### Request Methods
+
+| Method | Request URI | Request Body | Description |
+| ------ | ---------------------------------- | ------------ | ------------------------------------------------- |
+| GET | /apisix/admin/kms | NULL | Fetches a list of all kms. |
+| GET | /apisix/admin/kms/{secretmanager}/{id} | NULL | Fetches specified kms by id. |
+| PUT | /apisix/admin/kms/{secretmanager} | {...} | Create new kms configuration. |
+| DELETE | /apisix/admin/kms/{secretmanager}/{id} | NULL | Removes the kms with the specified id. |
+| PATCH | /apisix/admin/kms/{secretmanager}/{id} | {...} | Updates the selected attributes of the specified, existing kms. To delete an attribute, set value of attribute set to null. |
+| PATCH | /apisix/admin/kms/{secretmanager}/{id}/{path} | {...} | Updates the attribute specified in the path. The values of other attributes remain unchanged. |
+
+### Request Body Parameters
+
+When `{secretmanager}` is `vault`:
+
+| Parameter | Required | Type | Description | Example |
+| ----------- | -------- | ----------- | ------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------ |
+| 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 |
Review Comment:
The kms_vault doesn't have desc field
##########
t/admin/kms.t:
##########
@@ -0,0 +1,221 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+log_level("info");
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (!$block->request) {
+ $block->set_value("request", "GET /t");
+ }
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: PUT
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local etcd = require("apisix.core.etcd")
+ local code, body = t('/apisix/admin/kms/vault/test1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "http://127.0.0.1:12800/get",
+ "prefix" : "apisix",
+ "token" : "apisix"
+ }]],
+ [[{
+ "value": {
+ "uri": "http://127.0.0.1:12800/get",
+ "prefix" : "apisix",
+ "token" : "apisix"
+ },
+ "key": "/apisix/kms/vault/test1"
+ }]]
+ )
+
+ ngx.status = code
+ ngx.say(body)
+
+ local res = assert(etcd.get('/kms/vault/test1'))
+ local create_time = res.body.node.value.create_time
Review Comment:
Look like we need to add the xxx_time fields, otherwise, the test cases need to be changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org