Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2297013252 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,7 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf, true, plugin_conf, "") Review Comment: Merged, thanks for your contribution. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop merged PR #11451: URL: https://github.com/apache/apisix/pull/11451 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3218706562 Merge first, the failed CI will be fixed at https://github.com/apache/apisix/pull/12530#discussion_r2293332483 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2297003716 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,7 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf, true, plugin_conf, "") Review Comment: > more than 3 approvals and manually click merge button. > > There is also a CI error, but it doesn't seem to be related to this PR. I've seen similar errors in other PRs. then could you help to merge it 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2297001753 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,7 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf, true, plugin_conf, "") Review Comment: more than 3 approvals and manually click merge button. There is also a CI error, but it doesn't seem to be related to this PR. I've seen similar errors in other PRs. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2292781376 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,7 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf, true, plugin_conf, "") Review Comment: @bzp2010 may i ask what is the apisix PR merge flow? more than 3 approvals will merge it automatic or need someone manually click merge button,or else ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2292740786 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,7 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf, true, plugin_conf, "") Review Comment: hi @bzp2010 just add a commit to clone plugin_conf before fetch_secrets -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
bzp2010 commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2292693948 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,7 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf, true, plugin_conf, "") Review Comment: > I previously referred to the implementation of other plugin codes: > - https://github.com/apache/apisix/blob/master/apisix/plugins/authz-keycloak.lua#L767 > - https://github.com/apache/apisix/blob/master/apisix/plugins/limit-count.lua#L37 > >There are many other locations, so I think there is no problem with the current PR from the implementation point of view. > > As soon as someone modifies fetch_secrets in the future and doesn't clone there anymore, something will go wrong with your code. > > If this happens, many plugin codes will need to be adjusted. If my understanding is wrong, please correct me. I do not agree with this conclusion. The examples you listed are insufficient to prove this point. The reason we need to clone conf is that this plugin may temporarily and locally modify fields in conf. Lua tables use reference passing (pointer), and modifying the conf within the plugin would have side effects on the global configuration cache, which is unacceptable. You can check the two examples you provided; they do not involve any scenarios where the conf is modified, so there is no need to explicitly clone the conf. Whether fetch_secrets clones the conf internally has no impact on this. Looking at the openid-connect plugin, there are modifications to conf everywhere, so it undoubtedly needs to know whether conf has been cloned. This is a necessary measure to ensure that the global cache is not polluted. However, we cannot rely on external behavior (`fetch_secrets`) to guarantee this, as it violates the principle of low coupling. https://github.com/darkSheep404/apisix/blob/ec2974aa3e1d486b8416e5010f3d1046db093f37/apisix/plugins/openid-connect.lua#L562-L585 --- I still stand by this view, and unless this is modified, I will not merge this PR. 1. I do not believe that `fetch_secrets` implies that it will perform a clone and guarantee that this behavior will always be valid. 2. Performing an explicit clone (which is only a shallow clone) is inexpensive. At the same time, it ensures that it will conform to the logic before this modification rather than relying on external conditions for assurance. 3. Even if anybody does modify `fetch_secrets` in the future, the fact that conf is not cloned cannot be easily detected. **This will lead to unexpected behavior rather than an error.** I do not have the time to track all code changes in this area, and I do not want to wait until a bad situation occurs to discover it. If this is still a matter of controversy, then let more maintainers express their opinions. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
bzp2010 commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2292693948 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,7 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf, true, plugin_conf, "") Review Comment: > If this happens, many plugin codes will need to be adjusted. If my understanding is wrong, please correct me. I do not agree with this conclusion. The examples you listed are insufficient to prove this point. The reason we need to clone conf is that this plugin may temporarily and locally modify fields in conf. Lua tables use reference passing (pointer), and modifying the conf within the plugin would have side effects on the global configuration cache, which is unacceptable. You can check the two examples you provided; they do not involve any scenarios where the conf is modified, so there is no need to explicitly clone the conf. Whether fetch_secrets clones the conf internally has no impact on this. Looking at the openid-connect plugin, there are modifications to conf everywhere, so it undoubtedly needs to know whether conf has been cloned. This is a necessary measure to ensure that the global cache is not polluted. However, we cannot rely on external behavior (`fetch_secrets`) to guarantee this, as it violates the principle of low coupling. https://github.com/darkSheep404/apisix/blob/ec2974aa3e1d486b8416e5010f3d1046db093f37/apisix/plugins/openid-connect.lua#L562-L585 --- I still stand by this view, and unless this is modified, I will not merge this PR. 1. I do not believe that `fetch_secrets` implies that it will perform a clone and guarantee that this behavior will always be valid. 2. Performing an explicit clone (which is only a shallow clone) is inexpensive. At the same time, it ensures that it will conform to the logic before this modification rather than relying on external conditions for assurance. 3. Even if anybody does modify `fetch_secrets` in the future, the fact that conf is not cloned cannot be easily detected. **This will lead to unexpected behavior rather than an error.** I do not have the time to track all code changes in this area, and I do not want to wait until a bad situation occurs to discover it. If this is still a matter of controversy, then let more maintainers express their opinions. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
bzp2010 commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2292693948 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,7 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf, true, plugin_conf, "") Review Comment: > If this happens, many plugin codes will need to be adjusted. If my understanding is wrong, please correct me. I do not agree with this conclusion. The examples you listed are insufficient to prove this point. The reason we need to clone conf is that this plugin may temporarily and locally modify fields in conf. Lua tables use reference passing, and modifying the conf within the plugin would have side effects on the global configuration cache, which is unacceptable. You can check the two examples you provided; they do not involve any scenarios where the conf is modified, so there is no need to explicitly clone the conf. Whether fetch_secrets clones the conf internally has no impact on this. Looking at the openid-connect plugin, there are modifications to conf everywhere, so it undoubtedly needs to know whether conf has been cloned. This is a necessary measure to ensure that the global cache is not polluted. However, we cannot rely on external behavior (`fetch_secrets`) to guarantee this, as it violates the principle of low coupling. https://github.com/darkSheep404/apisix/blob/ec2974aa3e1d486b8416e5010f3d1046db093f37/apisix/plugins/openid-connect.lua#L562-L585 --- I still stand by this view, and unless this is modified, I will not merge this PR. 1. I do not believe that `fetch_secrets` implies that it will perform a clone and guarantee that this behavior will always be valid. 2. Performing an explicit clone (which is only a shallow clone) is inexpensive. At the same time, it ensures that it will conform to the logic before this modification rather than relying on external conditions for assurance. 3. Even if anybody does modify `fetch_secrets` in the future, the fact that conf is not cloned cannot be easily detected. **This will lead to unexpected behavior rather than an error.** I do not have the time to track all code changes in this area, and I do not want to wait until a bad situation occurs to discover it. If this is still a matter of controversy, then let more maintainers express their opinions. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2292489944 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,7 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf, true, plugin_conf, "") Review Comment: hi @Baoyuantop already 5 approval more than 3 approval required now, cloud you help to merge this 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2253885348 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,7 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf, true, plugin_conf, "") Review Comment: I previously referred to the implementation of other plugin codes: - https://github.com/apache/apisix/blob/master/apisix/plugins/authz-keycloak.lua#L767 - https://github.com/apache/apisix/blob/master/apisix/plugins/limit-count.lua#L37 There are many other locations, so I think there is no problem with the current PR from the implementation point of view. > As soon as someone modifies fetch_secrets in the future and doesn't clone there anymore, something will go wrong with your code. If this happens, many plugin codes will need to be adjusted. If my understanding is wrong, please correct me. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2253344630 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,7 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf, true, plugin_conf, "") Review Comment: > Please still clone the table. While it is true that `fetch_secrets` does imply this fact, it cannot be inferred from the name and is not a feature of any mandatory guarantee. As soon as someone modifies `fetch_secrets` in the future and doesn't clone there anymore, something will go wrong with your code. > > Therefore, it's best to explicitly copy this table here anyway to ensure that, no matter how much external conditions change, the logic of this plugin doesn't break. hi @Baoyuantop please advice -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
bzp2010 commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2253328769 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,7 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf, true, plugin_conf, "") Review Comment: Please still clone the table. While it is true that `fetch_secrets` does imply this fact, it cannot be inferred from the name and is not a feature of any mandatory guarantee. As soon as someone modifies `fetch_secrets` in the future and doesn't clone there anymore, something will go wrong with your code. Therefore, it's best to explicitly copy this table here anyway to ensure that, no matter how much external conditions change, the logic of this plugin doesn't break. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3153147299 > @nic-6443 @bzp2010 @Revolyssup @AlinsRan pls review this PR. https://github.com/user-attachments/assets/9da1dddb-590e-42de-85c2-0ee98c4dac34"; /> seems can merge now, need someone help to click merge button or wait those members approve it, then it will be merged automatic ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
SkyeYoung commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3149490517 @nic-6443 @bzp2010 @Revolyssup @AlinsRan pls review this 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3130836535 > Hi @darkSheep404, please merge the master branch code, and the failed test will be fixed. merged -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3130801821 Hi @darkSheep404, please merge the master branch code, and the failed test will be fixed. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2235247905 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,7 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf,true,plugin_conf,"") Review Comment: ```suggestion local conf = fetch_secrets(plugin_conf, true, plugin_conf, "") ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3125933557 > Hi @darkSheep404, please fix the doc lint error. ![Uploading image.png…]() seems job issue ,canot see what lint not pass -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3125909954 Hi @darkSheep404, please fix the doc lint error. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2221813253 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,8 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf_clone = core.table.clone(plugin_conf) Review Comment: > Referring to the code of other plugins, I think it can be modified with > > ``` > conf = fetch_secrets(conf, true, conf, "") > ``` > > The fetch_secrets function already handles deepcopy and cache. done -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r2221734435 ## apisix/plugins/openid-connect.lua: ## @@ -554,7 +555,8 @@ local function validate_claims_in_oidcauth_response(resp, conf) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf_clone = core.table.clone(plugin_conf) Review Comment: Referring to the code of other plugins, I think it can be modified with ``` conf = fetch_secrets(conf, true, conf, "") ``` The fetch_secrets function already handles deepcopy and cache. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3100440050 > Hi @darkSheep404, there are still some lint errors that need to be fixed. fixed -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3096028514 Hi @darkSheep404, there are still some lint errors that need to be fixed. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451:
URL: https://github.com/apache/apisix/pull/11451#discussion_r2215194939
##
t/plugin/openid-connect9.t:
##
@@ -0,0 +1,140 @@
+#
+# 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';
+
+log_level('debug');
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+my ($block) = @_;
+
+if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
+$block->set_value("no_error_log", "[error]");
+}
+
+if (!defined $block->request) {
+$block->set_value("request", "GET /t");
+}
+});
+
+BEGIN {
+$ENV{CLIENT_SECRET_ENV} =
"60Op4HFM0I8ajz0WdiStAbziZ-VFQttXuxixHHs2R7r7-CW8GR79l-mmLqMhc-Sa";
+$ENV{VAULT_TOKEN} = "root";
+}
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: configure oidc plugin with small public key using environment
variable
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, body = t('/apisix/admin/routes/1',
+ngx.HTTP_PUT,
+[[{ "plugins": {
+"openid-connect": {
+"client_id": "kbyuFDidLLm280LIwVFiazOqjO3ty8KH",
+"client_secret": "$ENV://CLIENT_SECRET_ENV",
+"discovery":
"https://samples.auth0.com/.well-known/openid-configuration";,
+"redirect_uri": "https://iresty.com";,
+"ssl_verify": false,
+"timeout": 10,
+"bearer_only": true,
+"scope": "apisix",
+"public_key": "-BEGIN PUBLIC KEY-\n]] ..
+
[[MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANW16kX5SMrMa2t7F2R1w6Bk/qpjS4QQ\n]] ..
+
[[hnrbED3Dpsl9JXAx90MYsIWp51hBxJSE/EPVK8WF/sjHK1xQbEuDfEECAwEAAQ==\n]] ..
+[[-END PUBLIC KEY-",
+"token_signing_alg_values_expected": "RS256"
+}
+},
+"upstream": {
+"nodes": {
+"127.0.0.1:1980": 1
+},
+"type": "roundrobin"
+},
+"uri": "/hello"
+}]]
+)
+
+if code >= 300 then
+ngx.status = code
+end
+ngx.say(body)
+}
+}
+--- response_body
+passed
+
+
+
+=== TEST 2: store secret into vault
+--- exec
+VAULT_TOKEN='root' VAULT_ADDR='http://0.0.0.0:8200' vault kv put kv/apisix/foo
client_secret=60Op4HFM0I8ajz0WdiStAbziZ-VFQttXuxixHHs2R7r7-CW8GR79l-mmLqMhc-Sa
+--- response_body
+Success! Data written to: kv/apisix/foo
+
+
+
+=== TEST 3: configure oidc plugin with small public key using vault
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{ "plugins": {
+"openid-connect": {
+"client_id":
"kbyuFDidLLm280LIwVFiazOqjO3ty8KH",
+"client_secret":
"$secret://vault/test1/foo/client_secret",
+"discovery":
"https://samples.auth0.com/.well-known/openid-configuration";,
+"redirect_uri": "https://iresty.com";,
+"ssl_verify": false,
+"timeout": 10,
+"bearer_only": true,
+"scope": "apisix",
+"public_key": "-BEGIN PUBLIC KEY-\n]]
..
+
[[MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANW16kX5SMrMa2t7F2R1w6Bk/qpjS4QQ\n]] ..
+
[[hnrbED3Dpsl9JXAx90MYsIWp51hBxJSE/EPVK8WF/sjHK1xQbEuDfEECAwEAAQ==\n]] ..
+[[-END PUBLIC KEY-"
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451:
URL: https://github.com/apache/apisix/pull/11451#discussion_r2215188294
##
t/plugin/openid-connect9.t:
##
@@ -0,0 +1,140 @@
+#
+# 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';
+
+log_level('debug');
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+my ($block) = @_;
+
+if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
+$block->set_value("no_error_log", "[error]");
+}
+
+if (!defined $block->request) {
+$block->set_value("request", "GET /t");
+}
+});
+
+BEGIN {
+$ENV{CLIENT_SECRET_ENV} =
"60Op4HFM0I8ajz0WdiStAbziZ-VFQttXuxixHHs2R7r7-CW8GR79l-mmLqMhc-Sa";
+$ENV{VAULT_TOKEN} = "root";
+}
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: configure oidc plugin with small public key using environment
variable
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, body = t('/apisix/admin/routes/1',
+ngx.HTTP_PUT,
+[[{ "plugins": {
+"openid-connect": {
+"client_id": "kbyuFDidLLm280LIwVFiazOqjO3ty8KH",
+"client_secret": "$ENV://CLIENT_SECRET_ENV",
+"discovery":
"https://samples.auth0.com/.well-known/openid-configuration";,
+"redirect_uri": "https://iresty.com";,
+"ssl_verify": false,
+"timeout": 10,
+"bearer_only": true,
+"scope": "apisix",
+"public_key": "-BEGIN PUBLIC KEY-\n]] ..
+
[[MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANW16kX5SMrMa2t7F2R1w6Bk/qpjS4QQ\n]] ..
+
[[hnrbED3Dpsl9JXAx90MYsIWp51hBxJSE/EPVK8WF/sjHK1xQbEuDfEECAwEAAQ==\n]] ..
+[[-END PUBLIC KEY-",
+"token_signing_alg_values_expected": "RS256"
+}
+},
+"upstream": {
+"nodes": {
+"127.0.0.1:1980": 1
+},
+"type": "roundrobin"
+},
+"uri": "/hello"
+}]]
+)
+
+if code >= 300 then
+ngx.status = code
+end
+ngx.say(body)
+}
+}
+--- response_body
+passed
+
+
+
+=== TEST 2: store secret into vault
+--- exec
+VAULT_TOKEN='root' VAULT_ADDR='http://0.0.0.0:8200' vault kv put kv/apisix/foo
client_secret=60Op4HFM0I8ajz0WdiStAbziZ-VFQttXuxixHHs2R7r7-CW8GR79l-mmLqMhc-Sa
+--- response_body
+Success! Data written to: kv/apisix/foo
+
+
+
+=== TEST 3: configure oidc plugin with small public key using vault
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{ "plugins": {
+"openid-connect": {
+"client_id":
"kbyuFDidLLm280LIwVFiazOqjO3ty8KH",
+"client_secret":
"$secret://vault/test1/foo/client_secret",
+"discovery":
"https://samples.auth0.com/.well-known/openid-configuration";,
+"redirect_uri": "https://iresty.com";,
+"ssl_verify": false,
+"timeout": 10,
+"bearer_only": true,
+"scope": "apisix",
+"public_key": "-BEGIN PUBLIC KEY-\n]]
..
+
[[MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANW16kX5SMrMa2t7F2R1w6Bk/qpjS4QQ\n]] ..
+
[[hnrbED3Dpsl9JXAx90MYsIWp51hBxJSE/EPVK8WF/sjHK1xQbEuDfEECAwEAAQ==\n]] ..
+[[-END PUBLIC KEY-"
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3071745108 > > Merging the latest master branch can make the failing test pass. > > merged by the way,i merged at last week and seems mcp-bridge test still fail after i merged lateset master branch -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451:
URL: https://github.com/apache/apisix/pull/11451#discussion_r2206187820
##
t/plugin/openid-connect.t:
##
@@ -33,7 +33,6 @@ add_block_preprocessor(sub {
$block->set_value("request", "GET /t");
}
});
-
Review Comment:
not, just split test to another openid-connect9.t cause this
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3071740452 > Merging the latest master branch can make the failing test pass. merged -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3071736243 Merging the latest master branch can make the failing test pass. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on code in PR #11451:
URL: https://github.com/apache/apisix/pull/11451#discussion_r2206170596
##
t/plugin/openid-connect.t:
##
@@ -33,7 +33,6 @@ add_block_preprocessor(sub {
$block->set_value("request", "GET /t");
}
});
-
Review Comment:
Is this change necessary?
##
docs/en/latest/plugins/openid-connect.md:
##
@@ -102,6 +102,14 @@ The `openid-connect` Plugin supports the integration with
[OpenID Connect (OIDC)
| claim_validator.issuer.valid_issuers | string[] | False | | | Whitelist
the vetted issuers of the jwt. When not passed by the user, the issuer returned
by discovery endpoint will be used. In case both are missing, the issuer will
not be validated. |
NOTE: `encrypt_fields = {"client_secret"}` is also defined in the schema,
which means that the field will be stored encrypted in etcd. See [encrypted
storage fields](../plugin-develop.md#encrypted-storage-fields).
+In addition, you can use Environment Variables or APISIX secret to store and
reference plugin attributes. APISIX currently supports storing secrets in two
ways - [Environment Variables and HashiCorp Vault](../terminology/secret.md).
Review Comment:
Please add zh docs
##
t/plugin/openid-connect9.t:
##
@@ -0,0 +1,140 @@
+#
+# 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';
+
+log_level('debug');
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+my ($block) = @_;
+
+if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
+$block->set_value("no_error_log", "[error]");
+}
+
+if (!defined $block->request) {
+$block->set_value("request", "GET /t");
+}
+});
+
+BEGIN {
+$ENV{CLIENT_SECRET_ENV} =
"60Op4HFM0I8ajz0WdiStAbziZ-VFQttXuxixHHs2R7r7-CW8GR79l-mmLqMhc-Sa";
+$ENV{VAULT_TOKEN} = "root";
+}
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: configure oidc plugin with small public key using environment
variable
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, body = t('/apisix/admin/routes/1',
+ngx.HTTP_PUT,
+[[{ "plugins": {
+"openid-connect": {
+"client_id": "kbyuFDidLLm280LIwVFiazOqjO3ty8KH",
+"client_secret": "$ENV://CLIENT_SECRET_ENV",
+"discovery":
"https://samples.auth0.com/.well-known/openid-configuration";,
+"redirect_uri": "https://iresty.com";,
+"ssl_verify": false,
+"timeout": 10,
+"bearer_only": true,
+"scope": "apisix",
+"public_key": "-BEGIN PUBLIC KEY-\n]] ..
+
[[MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANW16kX5SMrMa2t7F2R1w6Bk/qpjS4QQ\n]] ..
+
[[hnrbED3Dpsl9JXAx90MYsIWp51hBxJSE/EPVK8WF/sjHK1xQbEuDfEECAwEAAQ==\n]] ..
+[[-END PUBLIC KEY-",
+"token_signing_alg_values_expected": "RS256"
+}
+},
+"upstream": {
+"nodes": {
+"127.0.0.1:1980": 1
+},
+"type": "roundrobin"
+},
+"uri": "/hello"
+}]]
+)
+
+if code >= 300 then
+ngx.status = code
+end
+ngx.say(body)
+}
+}
+--- response_body
+passed
+
+
+
+=== TEST 2: store secret into vault
+--- exec
+VAULT_TOKEN='root' VAULT_ADDR='http://0.0.0.0:8200' vault kv put kv/apisix/foo
client_secret=60Op4HFM0I8ajz0WdiStAbziZ-VFQttXuxixHHs2R7r7-CW8GR79l-mmLqMhc-Sa
+--- response_body
+Success! Data written to: kv/apisix/foo
+
+
+
+=== TEST 3: configure oidc plugin with small public key using vault
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3067644165 > > Hi @darkSheep404, after re-run CI, it still fails. Can you merge the latest code from the master branch? > > hi @Baoyuantop updated hi > > Hi @darkSheep404, after re-run CI, it still fails. Can you merge the latest code from the master branch? > > hi @Baoyuantop updated hi @Baoyuantop pls help to review or approve at your convinence -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3043277440 > Hi @darkSheep404, after re-run CI, it still fails. Can you merge the latest code from the master branch? hi @Baoyuantop updated -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3043255055 Hi @darkSheep404, after re-run CI, it still fails. Can you merge the latest code from the master branch? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3034560182 > seems `./utils/reindex t/plugin/openid-connect9.t` works hi @Baoyuantop test lint issue fixed. Could help to review and approve PR ? dead ci is about mcp bridge test,this not related to this 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
fengbr1 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3034218139 seems `./utils/reindex t/plugin/openid-connect9.t` works -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3030517306 > openid-connect.t @Baoyuantop tried to fix the lint,but i'm not sure will it works .. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3030386964 > Hi @darkSheep404, any updates? cloud you help to fix the lint issue for me? my local pc no apisix runtime 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-3030370363 Hi @darkSheep404, any updates? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2835475369 Hi @darkSheep404, please fix failed ci -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2749870920 > Hi @darkSheep404, can you merge the latest master branch to trigger all the tests? hi @Baoyuantop just merged -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
Baoyuantop commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2749830992 Hi @darkSheep404, can you merge the latest master branch to trigger all the tests? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
github-actions[bot] commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2747557669 This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
membphis commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r1926688577 ## apisix/plugins/openid-connect.lua: ## @@ -471,7 +472,8 @@ local function required_scopes_present(required_scopes, http_scopes) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf_clone = core.table.clone(plugin_conf) Review Comment: https://github.com/apache/apisix/blob/0cb7d9ac8def4a490c8b49b7755a9484f3018eb2/apisix/plugins/basic-auth.lua#L116 here is an example in basic-auth plugin, it can help you -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2576945274 > ## 1th tip > =bad style= reindex: t/plugin/openid-connect.t: done.↳ > > * echo 'you need to run '''reindex''' to fix them. Read CONTRIBUTING.md for more details.' > you need to run 'reindex' to fix them. Read CONTRIBUTING.md for more details. > * exit 1 > make: *** [Makefile:173: lint] Error 1 > > ## 2th tip > you can merge the latest master branch↳ > ## 1th tip > =bad style= reindex: t/plugin/openid-connect.t: done.↳ > > * echo 'you need to run '''reindex''' to fix them. Read CONTRIBUTING.md for more details.' > you need to run 'reindex' to fix them. Read CONTRIBUTING.md for more details. > * exit 1 > make: *** [Makefile:173: lint] Error 1 > > ## 2th tip > you can merge the latest master branch↳ merged latest master branch for 1th tip, cloud you kindly help to run reindex for me ? my local windows pc no apisix source code env, we do development related to apisix use apisix offical docker image -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
membphis commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2576593792 ## 1th tip =bad style= reindex: t/plugin/openid-connect.t: done. + echo 'you need to run '\''reindex'\'' to fix them. Read CONTRIBUTING.md for more details.' you need to run 'reindex' to fix them. Read CONTRIBUTING.md for more details. + exit 1 make: *** [Makefile:173: lint] Error 1 ## 2th tip you can merge the latest master branch -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2568857811 In addition, I noticed that two tests failed, but I don't know how to fix them. Is there anyone who is kind enough to help 😢 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r1901567247 ## apisix/plugins/openid-connect.lua: ## @@ -471,7 +472,8 @@ local function required_scopes_present(required_scopes, http_scopes) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf_clone = core.table.clone(plugin_conf) Review Comment: > performance tips: > > we can use a lrucache, avoid repeated rendering of plugin_conf configuration Thank you, but I don't fully understand your suggestion The original way, I just used `fetch_secret` without clone the config first , but with [this suggestion](https://github.com/apache/apisix/pull/11451#issuecomment-2561283094), I add `table.clone` before `fetch_secret` I think `fetch_secret` itself will return a clone of conf, and if you want to optimize it, maybe can just keep using fetch_secret directly, instead of add another extra lrucache -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
membphis commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r1899963373 ## apisix/plugins/openid-connect.lua: ## @@ -471,7 +472,8 @@ local function required_scopes_present(required_scopes, http_scopes) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf_clone = core.table.clone(plugin_conf) Review Comment: performance tips: we can use a lrucache, avoid repeated rendering of plugin_conf configuration -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2561315392 > I have left a comment: https://github.com/apache/apisix/pull/11451/files#r1896868940 i think `fetch_sercets` should already return a clone conf instead of modify origianl plugin_conf anyway, i just adopted your suggestion and committed it to clone `plugin_conf` before `fetch_sercets` to avoid possible problems -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
shreemaan-abhishek commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r1896868940 ## apisix/plugins/openid-connect.lua: ## @@ -471,7 +473,7 @@ local function required_scopes_present(required_scopes, http_scopes) end function _M.rewrite(plugin_conf, ctx) -local conf = core.table.clone(plugin_conf) +local conf = fetch_secrets(plugin_conf) Review Comment: @darkSheep404, why did you resolve this conversation by yourself? You must not remove `core.table.clone`. As it is a bugfix: https://github.com/apache/apisix/issues/1956 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
shreemaan-abhishek commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2561283094 I have left a comment: https://github.com/apache/apisix/pull/11451/files#r1896868940 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2561190905 > > > Are there any updates on this? At the moment we have to keep a secret in our repo because of the bug that it is fixing this PR, and we need to move it to environment variables. So it would be awesome if we could have this fixed in a patch soon > > > > > > We need another good Samaritan to approve this change, then this PR will have three approve and be merged > > @shreemaan-abhishek @nic-6443 @membphis > > Could any of you help approve this PR at your convenience? It has been pending here for a long time and only needs the last approver @kayx23 @membphis @Revolyssup Could any of you help? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2548568702 > > Are there any updates on this? At the moment we have to keep a secret in our repo because of the bug that it is fixing this PR, and we need to move it to environment variables. So it would be awesome if we could have this fixed in a patch soon > > We need another good Samaritan to approve this change, then this PR will have three approve and be merged @shreemaan-abhishek @nic-6443 @membphis Could any of you help approve this PR at your convenience? It has been pending here for a long time and only needs the last approver -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2548497369 > Are there any updates on this? At the moment we have to keep a secret in our repo because of the bug that it is fixing this PR, and we need to move it to environment variables. So it would be awesome if we could have this fixed in a patch soon We need another good Samaritan to approve this change, and this PR will have three approve and be merged -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
alberto-corrales commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2548486959 Are there any updates on this? At the moment we have to keep a secret in our repo because of the bug that it is fixing this PR, and we need to move it to environment variables. So it would be awesome if we could have this fixed in a patch soon -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451:
URL: https://github.com/apache/apisix/pull/11451#discussion_r1843134874
##
apisix/plugins/openid-connect.lua:
##
@@ -290,7 +291,8 @@ local _M = {
}
-function _M.check_schema(conf)
+function _M.check_schema(plugin_conf)
+local conf = fetch_secrets(plugin_conf)
Review Comment:
pushed this change
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451:
URL: https://github.com/apache/apisix/pull/11451#discussion_r1843132891
##
apisix/plugins/openid-connect.lua:
##
@@ -290,7 +291,8 @@ local _M = {
}
-function _M.check_schema(conf)
+function _M.check_schema(plugin_conf)
+local conf = fetch_secrets(plugin_conf)
Review Comment:
So should we remove fetch_secrets from check_schema to avoid this?
By contrast, putting a Boolean value in a secret is not a particularly
common case in this plugin. Typically, only string urls and secret keys will be
placed in valut
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
brmejia commented on code in PR #11451:
URL: https://github.com/apache/apisix/pull/11451#discussion_r1838493955
##
apisix/plugins/openid-connect.lua:
##
@@ -290,7 +291,8 @@ local _M = {
}
-function _M.check_schema(conf)
+function _M.check_schema(plugin_conf)
+local conf = fetch_secrets(plugin_conf)
Review Comment:
This line is giving me some issues after apisix reload.
If line fetch_secrets is done in check_schema, the route fails showing me
the following error:
```
2024/11/12 17:12:51 [error] 240#240: *13728 lua entry thread aborted:
runtime error: ...isix/custom-plugins/apisix/plugins/openid-connect.lua:478:
attempt to compare nil with number
stack traceback:
coroutine 0:
...isix/custom-plugins/apisix/plugins/openid-connect.lua: in function
'phase_func'
/usr/local/apisix/apisix/plugin.lua:1166: in function 'run_plugin'
/usr/local/apisix/apisix/init.lua:689: in function 'http_access_phase'
access_by_lua(nginx.conf:310):2: in main chunk, client: 10.89.2.37,
server: _, request: "GET /private/anything HTTP/2.0", host: ""
```
If I remove fetch_secrets from check_schema, the route work as expected but
the following warning is shown at startup:
`[warn] 187#187: *8391 [lua] utils.lua:418: find_and_log(): Using
openid-connect discovery with no TLS is a security risk, context:
init_worker_by_lua*`
I assume that without the fetch_secrets the value of `discovery` is not
resolved and openid checks https for it.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2375986584 > please write a test case that uses vault as well.↳ hi @shreemaan-abhishek I tried to add one but I'm not sure if it's correct -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r1776414271 ## t/fips/openid-connect.t: ## Review Comment: > this is not needed.↳ hi @shreemaan-abhishek I tried to add one but I'm not sure if it's correct -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r1776414271 ## t/fips/openid-connect.t: ## Review Comment: > this is not needed.↳ hi @shreemaan-abhishek I tried to add one but I'm not sure if it's correct -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on code in PR #11451:
URL: https://github.com/apache/apisix/pull/11451#discussion_r1776378533
##
apisix/plugins/openid-connect.lua:
##
@@ -290,7 +291,8 @@ local _M = {
}
-function _M.check_schema(conf)
+function _M.check_schema(plugin_conf)
+local conf = fetch_secrets(plugin_conf)
Review Comment:
> this is not needed.↳
This is needed when someone puts a non-string value such as a Boolean into
env var, otherwise the type inconsistency will fail the check
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
shreemaan-abhishek commented on code in PR #11451: URL: https://github.com/apache/apisix/pull/11451#discussion_r1774788563 ## t/fips/openid-connect.t: ## Review Comment: please add new test cases in t/plugin/openid-connect -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
shreemaan-abhishek commented on code in PR #11451:
URL: https://github.com/apache/apisix/pull/11451#discussion_r1774785818
##
apisix/plugins/openid-connect.lua:
##
@@ -290,7 +291,8 @@ local _M = {
}
-function _M.check_schema(conf)
+function _M.check_schema(plugin_conf)
+local conf = fetch_secrets(plugin_conf)
Review Comment:
this is not needed.
##
apisix/plugins/openid-connect.lua:
##
@@ -471,7 +473,7 @@ local function required_scopes_present(required_scopes,
http_scopes)
end
function _M.rewrite(plugin_conf, ctx)
-local conf = core.table.clone(plugin_conf)
+local conf = fetch_secrets(plugin_conf)
Review Comment:
```suggestion
local conf = core.table.clone(plugin_conf)
local conf = fetch_secrets(plugin_conf)
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2369911272 hi @shreemaan-abhishek clould you kindly review it for me ? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: allow to use environment variables for openid-connect plugin [apisix]
darkSheep404 commented on PR #11451: URL: https://github.com/apache/apisix/pull/11451#issuecomment-2270531643 hi @shreemaan-abhishek cloud you kindly review it for me -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
