feat: support gcp secret manager #11436
Conversation
dosubot
bot
added
the
size:XXL
|
Hi @kayx23 Please take a look at the documentation when you are free to see if there are any problems. 🫡 |
| - secret_name: the secret name in the secrets management service | ||
| - key: get the value of a property when the value of the secret is a JSON string | ||
|
|
||
| ### Required Parameters |
There was a problem hiding this comment.
This portion of the doc perhaps should also be added to Admin API doc given the current structure of the apache/apisix doc since that is where the vault parameters are also documented: https://apisix.apache.org/docs/apisix/admin-api/#request-body-parameters-11
cc @pottekkat for inputs as well.
There was a problem hiding this comment.
Maybe I can do it in the new PR, because I didn't add the AWS part before.
#11417
There was a problem hiding this comment.
OK, I have completed：#11569
t/secret/gcp.t
Outdated
|
|
||
| __DATA__ | ||
|
|
||
| === TEST 1: sanity |
There was a problem hiding this comment.
change sanity to a meaningful title, and it should contain schema and validate
There was a problem hiding this comment.
All Resolved.
I have fixed these issues, please review them again.
| local res, err = make_request_to_gcp(conf, main_key) | ||
| if not res then | ||
| return nil, "failed to retrtive data from gcp secret manager: " .. err | ||
| end |
There was a problem hiding this comment.
This is consistent with any of the remaining implementations, both hc vault and aws. aws follows the style of vault, so I think it's fine to keep it the same.
If you think it's critical, you can naturally change all three secret providers at once after that PR merge, but unfortunately I don't see the need to do that necessarily.
There was a problem hiding this comment.
I agree, I also think there's no need to modify it.
apisix/secret/gcp.lua
Outdated
|
|
||
| local data, err = core.json.decode(res) | ||
| if not data then | ||
| return nil, "failed to decode result, res: " .. res .. ", err: " .. err |
There was a problem hiding this comment.
I think we need to remove res, it is dangerous to contain res.body in error message
| aud = self.token_uri, | ||
| scope = self.scope, | ||
| iat = get_timestamp(), | ||
| exp = get_timestamp() + (60 * 60) |
There was a problem hiding this comment.
60 * 60, it is a magic number, pls add a comment
There was a problem hiding this comment.
I don't think we know exactly what it means because the code is a simple copy of the apisix/plugins/google-cloud-logging/oauth.lua file.
There was a problem hiding this comment.
we need to create an issue to make apisix/plugins/google-cloud-logging/oauth.lua use this new file?
There was a problem hiding this comment.
Yes, when this PR is merged, I will create a new PR to use this new file.
|
@HuanXin-Chen There were some failures in CI involving traffic-split3.t, no worries, just merge the master branch into your dev branch |
Description
Many enterprises are utilizing cloud services from AWS and GCP, relying on the secret manager provided by these platforms to handle sensitive information. Integrating Apache APISIX with these secret managers can streamline the process of using sensitive information within APISIX, enabling users to manage and utilize cloud-stored sensitive data more conveniently, thus enhancing the overall security and usability of the system.
This PR has completed the support for GCP. It added the gcp.lua file to the original secret module, allowing users to store their secret information on GCP using the same reference method as before.
Checklist