search

nginxinc / nginx-openid-connect

Reference implementation of OpenID Connect integration for NGINX Plus
https://www.nginx.com/products/nginx/
Other
196 stars 92 forks source link

Emphasis the sync for NGINXaaS OIDC #99

Closed happyhd closed 1 month ago

happyhd commented 1 month ago

NGINXaaS for Azure customers uses this repo to configure OIDC for NGINXaaS which is a NGINX Plus cluster, create a config template folder nginxaas_oidc for them to make the configuration easier.

route443 commented 1 month ago

Thank you for the pull request, however, I have some doubts/questions regarding the appropriateness of adding a new folder to the repository that will contain a configuration template for NGINXaaS.

Why should this template, which essentially contains only runtime state sharing, be part of this repository?

  1. What advantages will this template provide for regular users? Who is the target audience?
  2. What will the deployment or usage process of this template look like? Simple file copying or is there some hidden machinery on your side?
  3. Why can't this template be part of the GitHub wiki?
  4. If there are multiple configuration templates in the future, should they all be located at the root of the repository?
  5. The current folder name nginxaas_oidc is not informative at all, meaning I can't personally imagine what it contains. What is the purpose of the oidc suffix if the entire repository is about OIDC?
happyhd commented 1 month ago

Thank you for the comment. Please kindly see my reply as below.

Why should this template, which essentially contains only runtime state sharing, be part of this repository? -> The NGINXaaS customer is always refer this repo for configuring OIDC and it is not convenient for them to go back and forth to check the NGINXaaS official doc and README here. And if they only follow the doc here (sync is optional, also /api endpoint should not be in the NGINXaaS config), the OIDC won't work well for NGINXaaS. We are trying to make them do the configuration in an easier way in one place. That's why we would like to mention NGINXaaS here and this template is one way for them to use directly instead of the general template.

  1. What advantages will this template provide for regular users? Who is the target audience? -> It is not for regular users, but for the users who wants to configure NGINXaaS for Azure.
  2. What will the deployment or usage process of this template look like? Simple file copying or is there some hidden machinery on your side? -> It is simple file copying then the same process as usual.
  3. Why can't this template be part of the GitHub wiki? -> It could be, just need to find how and where to insert them properly.
  4. If there are multiple configuration templates in the future, should they all be located at the root of the repository? -> Good question, I guess we don't want to create multiple configuration templates, it is not easy to maintain, if only the common part of the config is changed, we need to change all the config templates.
  5. The current folder name nginxaas_oidc is not informative at all, meaning I can't personally imagine what it contains. What is the purpose of the oidc suffix if the entire repository is about OIDC? -> If we don't use the subfolder way, do you have any suggestion? Maybe a separate page including the special config for NGINXaaS and refer it from the README in different configuration section?
route443 commented 1 month ago

Hi @happyhd, Thank you for your responses. In this case, I don't see any necessity to make this template (or any other) part of this repository:

  1. They introduce some chaos and inconvenience into the overall structure. Regular users will need to question what it is and whether they need it...
  2. All these duplicates of the main configuration files require maintenance. If someone makes changes to openid_connect_configuration.conf or openid_connect.server_conf (which happens frequently), we need to constantly ensure these templates are also updated. This is the maintainer's responsibility, and they must not forget to do this.
  3. This template copies 95% of the existing code, adding only the shared zone sync and removing the API endpoint.

As a result, I don't see any practicality in adding such templates to this repository, as there is already a proper tool for this task - the GitHub wiki. Let me know if the proposed solution works for you.

happyhd commented 1 month ago

@route443 Hello Ivan, could you please have a review for this small change? And let me know if it is proper? I think "optional" in "sync" here makes user confusion, for deploying in cluster, it should be a must? I added some additional NGINXaaS link for clarifying that sync is a must.

route443 commented 1 month ago

Since this information relates to the "Configuring the Key-Value Store" section, it implies that sync in this case is a parameter for the keyval_zone directive, which is optional. As for updating the readme and adding information about NGINXaaS for Azure, these changes look good to me. Let’s hope users actually read the documentation...