You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "shreemaan-abhishek (via GitHub)" <gi...@apache.org> on 2023/07/17 11:41:16 UTC
[GitHub] [apisix] shreemaan-abhishek opened a new pull request, #9855: feat: support pulling env vars from yaml keys
shreemaan-abhishek opened a new pull request, #9855:
URL: https://github.com/apache/apisix/pull/9855
### Description
Fixes #9397
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] 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] monkeyDluffy6017 commented on a diff in pull request #9855: feat: support pulling env vars from yaml keys
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9855:
URL: https://github.com/apache/apisix/pull/9855#discussion_r1271637351
##########
apisix/cli/file.lua:
##########
@@ -53,44 +53,60 @@ local function tab_is_array(t)
return #t == count
end
+local function var_sub(val)
Review Comment:
Two blanks are needed
--
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 #9855: feat: support pulling env vars from yaml keys
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #9855:
URL: https://github.com/apache/apisix/pull/9855#discussion_r1273192469
##########
apisix/cli/file.lua:
##########
@@ -54,43 +54,61 @@ local function tab_is_array(t)
end
+local function var_sub(val)
+ local err
+ local var_used = false
+ -- we use '${{var}}' because '$var' and '${var}' are taken
+ -- by Nginx
+ local new_val = val:gsub("%$%{%{%s*([%w_]+[%:%=]?.-)%s*%}%}", function(var)
+ local i, j = var:find("%:%=")
+ local default
+ if i and j then
+ default = var:sub(i + 2, #var)
+ default = default:gsub('^%s*(.-)%s*$', '%1')
+ var = var:sub(1, i - 1)
+ end
+
+ local v = getenv(var) or default
+ if v then
+ if not exported_vars then
+ exported_vars = {}
+ end
+
+ exported_vars[var] = v
+ var_used = true
+ return v
+ end
+
+ err = "failed to handle configuration: " ..
+ "can't find environment variable " .. var
+ return ""
+ end)
+ return new_val, var_used, err
+end
+
+
local function resolve_conf_var(conf)
for key, val in pairs(conf) do
+ -- substitute environment variables from conf keys
+ if type(key) == "string" then
+ local new_key, _, err = var_sub(key)
+ if err then
+ return nil, err
+ end
+ if new_key ~= key then
+ conf.key = nil
+ conf[new_key] = val
Review Comment:
I think it's okay to do so for the sake of readability and simplicity we are not dealing with a huge table here, what do you think?
--
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] monkeyDluffy6017 commented on a diff in pull request #9855: feat: support pulling env vars from yaml keys
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9855:
URL: https://github.com/apache/apisix/pull/9855#discussion_r1274335438
##########
apisix/cli/file.lua:
##########
@@ -54,43 +54,61 @@ local function tab_is_array(t)
end
+local function var_sub(val)
+ local err
+ local var_used = false
+ -- we use '${{var}}' because '$var' and '${var}' are taken
+ -- by Nginx
+ local new_val = val:gsub("%$%{%{%s*([%w_]+[%:%=]?.-)%s*%}%}", function(var)
+ local i, j = var:find("%:%=")
+ local default
+ if i and j then
+ default = var:sub(i + 2, #var)
+ default = default:gsub('^%s*(.-)%s*$', '%1')
+ var = var:sub(1, i - 1)
+ end
+
+ local v = getenv(var) or default
+ if v then
+ if not exported_vars then
+ exported_vars = {}
+ end
+
+ exported_vars[var] = v
+ var_used = true
+ return v
+ end
+
+ err = "failed to handle configuration: " ..
+ "can't find environment variable " .. var
+ return ""
+ end)
+ return new_val, var_used, err
+end
+
+
local function resolve_conf_var(conf)
for key, val in pairs(conf) do
+ -- substitute environment variables from conf keys
+ if type(key) == "string" then
+ local new_key, _, err = var_sub(key)
+ if err then
+ return nil, err
+ end
+ if new_key ~= key then
+ conf.key = nil
+ conf[new_key] = val
Review Comment:
It's a hidden bug here, no one knows it will be iterated two 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] monkeyDluffy6017 commented on a diff in pull request #9855: feat: support pulling env vars from yaml keys
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9855:
URL: https://github.com/apache/apisix/pull/9855#discussion_r1271966620
##########
apisix/cli/file.lua:
##########
@@ -54,43 +54,61 @@ local function tab_is_array(t)
end
+local function var_sub(val)
+ local err
+ local var_used = false
+ -- we use '${{var}}' because '$var' and '${var}' are taken
+ -- by Nginx
+ local new_val = val:gsub("%$%{%{%s*([%w_]+[%:%=]?.-)%s*%}%}", function(var)
+ local i, j = var:find("%:%=")
+ local default
+ if i and j then
+ default = var:sub(i + 2, #var)
+ default = default:gsub('^%s*(.-)%s*$', '%1')
+ var = var:sub(1, i - 1)
+ end
+
+ local v = getenv(var) or default
+ if v then
+ if not exported_vars then
+ exported_vars = {}
+ end
+
+ exported_vars[var] = v
+ var_used = true
+ return v
+ end
+
+ err = "failed to handle configuration: " ..
+ "can't find environment variable " .. var
+ return ""
+ end)
+ return new_val, var_used, err
+end
+
+
local function resolve_conf_var(conf)
for key, val in pairs(conf) do
+ -- substitute environment variables from conf keys
+ if type(key) == "string" then
+ local new_key, _, err = var_sub(key)
+ if err then
+ return nil, err
+ end
+ if new_key ~= key then
+ conf.key = nil
+ conf[new_key] = val
+ key = new_key
+ end
+ end
Review Comment:
Better to add a blank below
--
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] monkeyDluffy6017 commented on a diff in pull request #9855: feat: support pulling env vars from yaml keys
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9855:
URL: https://github.com/apache/apisix/pull/9855#discussion_r1271970588
##########
apisix/cli/file.lua:
##########
@@ -54,43 +54,61 @@ local function tab_is_array(t)
end
+local function var_sub(val)
+ local err
+ local var_used = false
+ -- we use '${{var}}' because '$var' and '${var}' are taken
+ -- by Nginx
+ local new_val = val:gsub("%$%{%{%s*([%w_]+[%:%=]?.-)%s*%}%}", function(var)
+ local i, j = var:find("%:%=")
+ local default
+ if i and j then
+ default = var:sub(i + 2, #var)
+ default = default:gsub('^%s*(.-)%s*$', '%1')
+ var = var:sub(1, i - 1)
+ end
+
+ local v = getenv(var) or default
+ if v then
+ if not exported_vars then
+ exported_vars = {}
+ end
+
+ exported_vars[var] = v
+ var_used = true
+ return v
+ end
+
+ err = "failed to handle configuration: " ..
+ "can't find environment variable " .. var
+ return ""
+ end)
+ return new_val, var_used, err
+end
+
+
local function resolve_conf_var(conf)
for key, val in pairs(conf) do
+ -- substitute environment variables from conf keys
+ if type(key) == "string" then
+ local new_key, _, err = var_sub(key)
+ if err then
+ return nil, err
+ end
+ if new_key ~= key then
+ conf.key = nil
+ conf[new_key] = val
Review Comment:
It's better not to change the key in the original table, it will be iterated again
--
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] monkeyDluffy6017 commented on a diff in pull request #9855: feat: support pulling env vars from yaml keys
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9855:
URL: https://github.com/apache/apisix/pull/9855#discussion_r1271643997
##########
apisix/cli/file.lua:
##########
@@ -53,44 +53,60 @@ local function tab_is_array(t)
return #t == count
end
+local function var_sub(val)
+ local err
+ local var_used = false
+ -- we use '${{var}}' because '$var' and '${var}' are taken
+ -- by Nginx
+ local new_val = val:gsub("%$%{%{%s*([%w_]+[%:%=]?.-)%s*%}%}", function(var)
+ local i, j = var:find("%:%=")
+ local default
+ if i and j then
+ default = var:sub(i + 2, #var)
+ default = default:gsub('^%s*(.-)%s*$', '%1')
+ var = var:sub(1, i - 1)
+ end
+
+ local v = getenv(var) or default
+ if v then
+ if not exported_vars then
+ exported_vars = {}
+ end
+
+ exported_vars[var] = v
+ var_used = true
+ return v
+ end
+
+ err = "failed to handle configuration: " ..
+ "can't find environment variable " .. var
+ return ""
+ end)
+ return new_val, var_used, err
+end
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] shreemaan-abhishek commented on a diff in pull request #9855: feat: support pulling env vars from yaml keys
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #9855:
URL: https://github.com/apache/apisix/pull/9855#discussion_r1273200647
##########
apisix/cli/file.lua:
##########
@@ -54,43 +54,61 @@ local function tab_is_array(t)
end
+local function var_sub(val)
+ local err
+ local var_used = false
+ -- we use '${{var}}' because '$var' and '${var}' are taken
+ -- by Nginx
+ local new_val = val:gsub("%$%{%{%s*([%w_]+[%:%=]?.-)%s*%}%}", function(var)
+ local i, j = var:find("%:%=")
+ local default
+ if i and j then
+ default = var:sub(i + 2, #var)
+ default = default:gsub('^%s*(.-)%s*$', '%1')
+ var = var:sub(1, i - 1)
+ end
+
+ local v = getenv(var) or default
+ if v then
+ if not exported_vars then
+ exported_vars = {}
+ end
+
+ exported_vars[var] = v
+ var_used = true
+ return v
+ end
+
+ err = "failed to handle configuration: " ..
+ "can't find environment variable " .. var
+ return ""
+ end)
+ return new_val, var_used, err
+end
+
+
local function resolve_conf_var(conf)
for key, val in pairs(conf) do
+ -- substitute environment variables from conf keys
+ if type(key) == "string" then
+ local new_key, _, err = var_sub(key)
+ if err then
+ return nil, err
+ end
+ if new_key ~= key then
+ conf.key = nil
+ conf[new_key] = val
Review Comment:
Because to avoid modifying the original table we should create a copy of the original table which will again consume CPU cycles.
--
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] monkeyDluffy6017 commented on a diff in pull request #9855: feat: support pulling env vars from yaml keys
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9855:
URL: https://github.com/apache/apisix/pull/9855#discussion_r1278954475
##########
apisix/cli/file.lua:
##########
@@ -32,6 +32,7 @@ local print = print
local _M = {}
local exported_vars
+local new_keys = {}
Review Comment:
It should be declared in 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] monkeyDluffy6017 merged pull request #9855: feat: support pulling env vars from yaml keys
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 merged PR #9855:
URL: https://github.com/apache/apisix/pull/9855
--
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