consul-template icon indicating copy to clipboard operation
consul-template copied to clipboard

feat: add pkiSign function to use vault pki sign api endpoint

Open n-marton opened this issue 1 year ago • 5 comments

This pull requests intends to add a function to leverage the sign api endpoint in vault pki.

This can be really useful if one plans to use a large number of certificates with low TTL, where the usage of the issue endpoint can cause high load on the vault servers. By using the sign endpoint one can shift the heaviest part of the problem (the private key generation), from server side to client side.

n-marton avatar Apr 03 '24 12:04 n-marton

hey guys, any chance to have a look on that? appreciate.

eduardolmedeiros avatar Jul 15 '24 13:07 eduardolmedeiros

Hello team, could please someone have a look? It will be nice to have it soon. Thanks in advance!

alexandrenas avatar Sep 24 '24 21:09 alexandrenas

Hi @n-marton , thanks a lot for opening the PR and your contribution. Your changes look great! I left some feedback. Also, we need unit tests added in here to ensure your new functions are covered in unit tests (to test they work in different use cases and edge cases and in case someone changes it in the future, unit tests catch errors): https://github.com/hashicorp/consul-template/blob/main/template/funcs_test.go

aslamovamir avatar Feb 13 '25 15:02 aslamovamir

Hi @n-marton , thanks a lot for opening the PR and your contribution. Your changes look great! I left some feedback. Also, we need unit tests added in here to ensure your new functions are covered in unit tests (to test they work in different use cases and edge cases and in case someone changes it in the future, unit tests catch errors): https://github.com/hashicorp/consul-template/blob/main/template/funcs_test.go

hey @aslamovamir

thanks for getting back on this MR, I haven't wrote a test there because the cert issue func also did not have one, in fact that part is kind lacking heavily all together, however I am happy to write one for my new func, IF that is the only one thing we miss to merge this feature

n-marton avatar Feb 13 '25 16:02 n-marton

Hi @n-marton , thanks a lot for opening the PR and your contribution. Your changes look great! I left some feedback. Also, we need unit tests added in here to ensure your new functions are covered in unit tests (to test they work in different use cases and edge cases and in case someone changes it in the future, unit tests catch errors): https://github.com/hashicorp/consul-template/blob/main/template/funcs_test.go

hey @aslamovamir

thanks for getting back on this MR, I haven't wrote a test there because the cert issue func also did not have one, in fact that part is kind lacking heavily all together, however I am happy to write one for my new func, IF that is the only one thing we miss to merge this feature

Hi @n-marton, yup I see - I honestly am not part of the codeowners for this repo or never contributed to it so did not know other functions also lack unit tests here. But would be great to have it written for yours if thats okay. All the rest looks good to me in your PR and can approve it once we have unit test coverage.

aslamovamir avatar Feb 13 '25 16:02 aslamovamir