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. <p>**This will lead to unexpected behavior rather than an error.**</p> 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: notifications-unsubscr...@apisix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
