Conversation
…o match chart name. added an option for defining an IP range whitelist to the helm chart
|
Hi @Rigdon. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| # cpu: 100m | ||
| # memory: 128Mi | ||
| nodeSelector: {} | ||
| includeIPRanges: {} |
There was a problem hiding this comment.
Docs please: what is the syntax, example (with 2 ranges to see the delim ), what is the effect of setting it up ( disable egress rules )
There was a problem hiding this comment.
Also - not sure if the naming is right, it may be quite confusing.
iptablesCaptureWhitelist ?
There was a problem hiding this comment.
@costinm I added a description there in the values.yaml.tmpl with a link to the istioctl docs. I can change the name if we want but I think it makes sense for it to match the corresponding argument to istioctl kube-inject.
|
I added a description there in the values.yaml.tmpl with a link to the istioctl docs. I can change the name if we want but I think it makes sense for it to match the corresponding argument to |
costinm
left a comment
There was a problem hiding this comment.
/lgtm
Thanks for the change - maybe you can file a bug to discuss a better name for kube-inject, and adding a field to the mesh config ( I don't believe we have one ).
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: costinm The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
@Rigdon did you test this change? We had a prior problem where sidecarInjection did not work with a - in it because of a flaw in Helm 2.7.2+ which is as yet unresolved. |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
|
Once again it is pretty critical to get the helm templates included in the automated tests ( in particular using 'helm template' or istioctl ). I hope this was tested manually and the - doesn't break stuff, easy to |
I added an option to specify
includeIPRangesin the helm chart in order to match the corresponding argument toistioctlwhen using automatic sidecar injection.I also fixed an issue with the value name for the sidecar injector where it didn't match the name of the sidecar injector subchart, and thus the values weren't available to it.