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/08/24 01:19:09 UTC
[GitHub] [apisix] soulbird opened a new pull request, #7775: feat(admin-api): support check conf only, don't write etcd
soulbird opened a new pull request, #7775:
URL: https://github.com/apache/apisix/pull/7775
### 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] soulbird commented on pull request #7775: feat(admin-api): support check conf only, don't write etcd
Posted by GitBox <gi...@apache.org>.
soulbird commented on PR #7775:
URL: https://github.com/apache/apisix/pull/7775#issuecomment-1225204415
> Description
> I don't understand what problem this PR solves, where is the documentation?
The title already describes the function of this PR. In some cases, such as the API design stage, the user just wants to verify whether the configuration is correct, and does not want to actually submit the configuration to APISIX. This PR provides an API to help users do just that.
--
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] bzp2010 commented on a diff in pull request #7775: feat(admin-api): support check conf only, don't write etcd
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7775:
URL: https://github.com/apache/apisix/pull/7775#discussion_r953260562
##########
apisix/admin/init.lua:
##########
@@ -130,13 +130,18 @@ local function run()
local uri_segs = core.utils.split_uri(ngx.var.uri)
core.log.info("uri: ", core.json.delay_encode(uri_segs))
+ local check_only = false
-- /apisix/admin/schema/route
local seg_res, seg_id = uri_segs[4], uri_segs[5]
local seg_sub_path = core.table.concat(uri_segs, "/", 6)
if seg_res == "schema" and seg_id == "plugins" then
-- /apisix/admin/schema/plugins/limit-count
seg_res, seg_id = uri_segs[5], uri_segs[6]
seg_sub_path = core.table.concat(uri_segs, "/", 7)
+
+ elseif seg_res == "check_conf" then
Review Comment:
I think some comments are needed here to indicate what the check conf interface uri of the Admin API is.
like `-- /apisix/admin/schema/route`
--
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 closed pull request #7775: feat(admin-api): support check conf only, don't write etcd
Posted by GitBox <gi...@apache.org>.
soulbird closed pull request #7775: feat(admin-api): support check conf only, don't write etcd
URL: https://github.com/apache/apisix/pull/7775
--
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 pull request #7775: feat(admin-api): support check conf only, don't write etcd
Posted by GitBox <gi...@apache.org>.
soulbird commented on PR #7775:
URL: https://github.com/apache/apisix/pull/7775#issuecomment-1226729376
Introducing a new API to support is not a good method. We will initiate a discussion in the #7790 and reopen it after the solution is confirmed.
--
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 pull request #7775: feat(admin-api): support check conf only, don't write etcd
Posted by GitBox <gi...@apache.org>.
soulbird commented on PR #7775:
URL: https://github.com/apache/apisix/pull/7775#issuecomment-1225217369
> You should use the CLI instead of the admin API to solve this problem. Using the admin API to do the test work, is a very weird thing
It should be noted that this is not only to verify the schema, but also includes some reference verification between objects, SSL certificate verification, etc. Even if the CLI is used, APISIX also needs to provide the ability first.
--
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] bzp2010 commented on pull request #7775: feat(admin-api): support check conf only, don't write etcd
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on PR #7775:
URL: https://github.com/apache/apisix/pull/7775#issuecomment-1225096094
Is it necessary to add the documentation for these 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] starsz commented on a diff in pull request #7775: feat(admin-api): support check conf only, don't write etcd
Posted by GitBox <gi...@apache.org>.
starsz commented on code in PR #7775:
URL: https://github.com/apache/apisix/pull/7775#discussion_r954426498
##########
t/admin/check_conf.t:
##########
@@ -0,0 +1,457 @@
+#
+# 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 (!defined $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]");
+ }
+
+ $block;
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: check routes conf
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code = t('/apisix/admin/check_conf/routes/1',
+ ngx.HTTP_POST,
+ [[{
+ "methods": ["GET"],
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:8080": 1
+ },
+ "type": "roundrobin"
+ },
+ "desc": "new route",
+ "uri": "/index.html"
+ }]]
+ )
+
+ ngx.status = code
+ }
+ }
+
+
+
+=== TEST 2: route conf don't write etcd
+--- request
+GET /index.html
+--- error_code: 404
+--- response_body
+{"error_msg":"404 Route Not Found"}
+
+
+
+=== TEST 3: check routes conf: wrong method
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/check_conf/routes/1',
Review Comment:
Add `check_conf` into path.
Maybe a param like `dry-run` would be better?
--
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 pull request #7775: feat(admin-api): support check conf only, don't write etcd
Posted by GitBox <gi...@apache.org>.
soulbird commented on PR #7775:
URL: https://github.com/apache/apisix/pull/7775#issuecomment-1225204652
> I don't understand what problem this PR solves, where is the documentation?
The title already describes the function of this PR. In some cases, such as the API design stage, the user just wants to verify whether the configuration is correct, and does not want to actually submit the configuration to APISIX. This PR provides an API to help users do just that.
--
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 pull request #7775: feat(admin-api): support check conf only, don't write etcd
Posted by GitBox <gi...@apache.org>.
soulbird commented on PR #7775:
URL: https://github.com/apache/apisix/pull/7775#issuecomment-1225096978
> Is it necessary to add the documentation for these Admin API?
Yes, the documentation is done in another PR
--
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