Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: allow to use environment variables for openid-connect plugin #11451

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

feat: allow to use environment variables for openid-connect plugin #11451

wants to merge 8 commits into from
+110 −2

Conversation

darkSheep404
Copy link
Contributor

Description

feat: allow to use environment variables for openid-connect plugin

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@darkSheep404 darkSheep404 mentioned this pull request Aug 9, 2024
Open
@juzhiyuan juzhiyuan mentioned this pull request Aug 13, 2024
Open
@darkSheep404
Copy link
Contributor Author

darkSheep404 commented Sep 24, 2024 •
edited
Loading

hi good morning @shreemaan-abhishek clould you kindly review this PR for me ?

shreemaan-abhishek
shreemaan-abhishek reviewed Sep 25, 2024
View reviewed changes
Copy link
Contributor

@shreemaan-abhishek shreemaan-abhishek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please write a test case that uses vault as well.

apisix/plugins/openid-connect.lua Show resolved Hide resolved
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

shreemaan-abhishek
shreemaan-abhishek reviewed Sep 25, 2024
View reviewed changes
Copy link
Contributor

@shreemaan-abhishek shreemaan-abhishek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please write a test case that uses vault as well.

t/fips/openid-connect.t Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add new test cases in t/plugin/openid-connect

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 25, 2024
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 26, 2024
@darkSheep404
Copy link
Contributor Author

darkSheep404 commented Sep 26, 2024 •
edited
Loading

please write a test case that uses vault as well.↳
hi @shreemaan-abhishek
I tried to add a test for valut but I am not sure if it is correct, may need your help to have a look

shreemaan-abhishek
shreemaan-abhishek reviewed Oct 3, 2024
View reviewed changes
Copy link
Contributor

@shreemaan-abhishek shreemaan-abhishek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test cases for secret resource seems correct to me, please resolve the conflicts with master so that the tests can run

@alberto-corrales
Copy link

Great contribution!! I'm looking forward to this fix, as we are using AWS Secrets Manager as a secrets provider and we need to configure the secret as an environment variable. I hope this fix is merged and released soon 🙏

@darkSheep404
Copy link
Contributor Author

the test cases for secret resource seems correct to me, please resolve the conflicts with master so that the tests can run↳

hi @shreemaan-abhishek
The conflict has been resolved. Please help approve it when you are free so that can run the test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shreemaan-abhishek shreemaan-abhishek shreemaan-abhishek left review comments

At least 3 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@darkSheep404 @alberto-corrales @shreemaan-abhishek