-
Notifications
You must be signed in to change notification settings - Fork 264
Add trusted_addresses option to apisix configmap template #909
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.
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
Add trusted_addresses option to apisix configmap template #909
Conversation
charts/apisix/values.yaml
Outdated
|
|
||
| trusted_addresses: | ||
| - 127.0.0.1 | ||
| - 172.18.0.0/16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 172.18.0.0/16 does not appear suitable as one of the default values. I believe we can keep only 127.0.0.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need defaults? Should we just use empty list as default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe since this is an opt-out feature, some default values may make it more visible for the end user. So I'd leave some default values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's potentially a breaking change to users, who previously didn't want to set trusted addresses
|
BTW, you'll also need to run the |
charts/apisix/README.md
Outdated
| | apisix.status.ip | string | `"0.0.0.0"` | | | ||
| | apisix.status.port | int | `7085` | | | ||
| | apisix.stream_plugins | list | `[]` | Customize the list of APISIX stream_plugins to enable. By default, APISIX's [default stream_plugins](https://github.com/apache/apisix/blob/master/apisix/cli/config.lua#L294) are automatically used. | | ||
| | apisix.trustedAddresses[0] | string | `"127.0.0.1"` | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | apisix.trustedAddresses[0] | string | `"127.0.0.1"` | | | |
| | apisix.trustedAddresses | list | `["127.0.0.1"]` | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this auto generated or manually edited?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://github.com/norwoodj/helm-docs, I think it was auto-generated. Adding a comment as I suggested will resolve this.
Documentation quote:
The following rules are used to determine which values will be added to the values table in the README:
- By default, only leaf nodes, that is, fields of type int, string, float, bool, empty lists, and empty maps are added as rows in the values table. These fields will be added even if they do not have a description comment
- Lists and maps which contain elements will not be added as rows in the values table unless they have a description comment which refers to them
- Adding a description comment for a non-empty list or map in this way makes it so that leaf nodes underneath the described field will not be automatically added to the values table. In order to document both a non-empty list/map and a leaf node within that field, description comments must be added for both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, will adapt the changes in a sec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ngclinflows as you said, after the comment, the docs show the correct type
Co-authored-by: Nicolas Gagnière <[email protected]>
This PR builds on top of #900 to enable trusted_addresses option and bump chart version for automatic deployment