Move injection settings from .Values. into ProxyConfig #1478
Conversation
istio-testing
added
the
do-not-merge/work-in-progress
|
😊 Welcome @dgn! This is either your first contribution to the Istio api repo, or it's been You can learn more about the Istio working groups, code of conduct, and contributing guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
|
Skipping CI for Draft Pull Request. |
googlebot
added
the
cla: yes
istio-testing
added
the
size/L
|
/test all |
|
/retest |
There was a problem hiding this comment.
Overall in favor of this type of move, thanks for working on this.
cc @costinm
| string proxy_init_image = 33; | ||
| string proxy_init_image_pull_policy = 39; | ||
|
|
||
| repeated string include_outbound_ip_ranges = 34; |
There was a problem hiding this comment.
Do we want to nest this? Either one level (capture.include_outbound_ip_ranges) or two levels (capture.include.inbound_ip_ranges) or I suppose we could do more?
There was a problem hiding this comment.
We have existing settings - let's keep consistent and minimize cosmetic change.
| // multicluster scenarios). | ||
| repeated string pod_dns_search_namespaces = 31; | ||
|
|
||
| bool rewrite_app_http_probe = 32; |
There was a problem hiding this comment.
Not sure how WIP this is meant to be, but we should add some comments prior to merge
There was a problem hiding this comment.
+1, this is just in draft state
There was a problem hiding this comment.
What is the status of the rewrite_app_http_probe ? Was a feature flag - did we make it default ? Do we still have a use case to turn it off/on ?
There was a problem hiding this comment.
It is default now. I wouldn't be against removing it as as option? @incfly any thoughts?
There was a problem hiding this comment.
We can keep it as an option, for safety - but not as API in ProxyConfig.
If we are sure it can never fail - we can also remove it as an option.
I think it is reasonable to have a number of env variables for infrequent/unlikely cases - just to be sure, for safety.
But if there is no good use case for normal user to interact with it - should not be in the API.
|
|
||
| bool rewrite_app_http_probe = 32; | ||
|
|
||
| string proxy_init_image = 33; |
There was a problem hiding this comment.
I suggest we remove these entirely, and just have a single proxy image option
There was a problem hiding this comment.
+1 - we're not planning separate image for init.
There was a problem hiding this comment.
good point, will remove
| @@ -302,6 +302,8 @@ message ProxyConfig { | |||
| // Port on which Envoy should listen for administrative commands. | |||
| int32 proxy_admin_port = 11; | |||
|
|
|||
| int32 proxy_status_port = 36; | |||
There was a problem hiding this comment.
This one already exists, option 26
There was a problem hiding this comment.
Not sure if sorting fields by number will affect generated protobuff ?, but I think that will help to avoid numbering confusion in the future.
There was a problem hiding this comment.
What do you mean ? Leave the original as it is, don't add the new one.
| repeated string include_outbound_ip_ranges = 34; | ||
| repeated string exclude_outbound_ip_ranges = 35; | ||
|
|
||
| repeated string include_inbound_ports = 37; |
There was a problem hiding this comment.
I think the ProxyConfig docs are generated based on these API definitions, so it will be good to add a one-liner description about each field just to avoid blank line in description column. :)
| // The mode used to redirect inbound traffic to Envoy. | ||
| InboundInterceptionMode interception_mode = 18; | ||
|
|
||
| InboundInterceptionTechnique interception_technique = 30; |
There was a problem hiding this comment.
I don't think this is a setting we want here: it is not actionable. The proxy can't decide to use CNI - either it is installed and used, or not.
Besides, the 'technique' name is very strange - and we already have interception mode.
We could add an extra setting to the interception mode enum if we really find a use case that is
possible to implement.
There was a problem hiding this comment.
It is acted upon in the injection template - it checks whether .Values.istio_cni.enabled == true, so obviously it is of interest to the control plane whether we're using CNI or not
There was a problem hiding this comment.
That doesn't mean it should be part of the API exposed to user.
ProxyConfig is an API used to define how a proxy behave - if the user sets it to CNI, the expectation is that somehow CNI will be used - which we obviously can't unless CNI is installed in the cluster. And if the CNI is installed - not clear we want to allow users to 'opt out' using ProxyConfig. There is an annotation we can continue to support.
I think it may be fine to have 'cni enabled' in MeshConfig - as a Istiod/injector setting. But probably the semantic wouldn't be if CNI is enabled - Istiod can just check if the CNI deployment is there without user having to specify. We may want to allow the admin to control if pods can opt out or not.
Either way - bulk adding APIs is problematic, we should review each API carefully - we don't add dozen unrelated
fields to Gateway or VirtualService, we shouldn't do it for ProxyConfig either. A RFC on how to expose the settings for CNI would be great, meanwhile please keep it out.
There was a problem hiding this comment.
Yeah, I would have seen this as the default value for whether or not to use CNI during injection. You could override this per-proxy to use proxy_init for example. Of course it would only make sense if CNI is installed, I agree. That should be checked during validaton
There was a problem hiding this comment.
And, again, all names are tbd, please don't assume I wanted to merge this as-is, just suggest better names
| // This does not apply to gateway pods as they typically need a different | ||
| // set of DNS settings than the normal application pods (e.g., in | ||
| // multicluster scenarios). | ||
| repeated string pod_dns_search_namespaces = 31; |
There was a problem hiding this comment.
Not familiar with this - doesn't k8s already have this in the pod spec ? What is the purpose and use here ?
There was a problem hiding this comment.
Maybe add a comment on how it is used in Istio - and we should confirm that
- we can't use the setting from the pod ( injection should have access to that )
- we want to use a random name instead of using the same name as k8s. In general we want to use the same syntax when possible - and minimize the 'istio-invented names'
There was a problem hiding this comment.
Good point actually I was thinking this was a container setting which sort of makes sense. For a pod - they should just add this to their own spec..
There was a problem hiding this comment.
@costinm this is a draft that just blindly moves settings from .Values.global. to ProxyConfig, nothing was invented here
There was a problem hiding this comment.
I meant: values.yaml and global.yaml (which this PR 'blindly moves' ) invented a lot of inconsistent names - not matching what K8S is using or even our own Istio APIs.
We do not want 'blind move' of settings - what's the point ? We need to do what we failed to do when we let
everything in values/global - and consider the use cases, UX, consistency, etc - on each feature. It is an API.
| // multicluster scenarios). | ||
| repeated string pod_dns_search_namespaces = 31; | ||
|
|
||
| bool rewrite_app_http_probe = 32; |
There was a problem hiding this comment.
What is the status of the rewrite_app_http_probe ? Was a feature flag - did we make it default ? Do we still have a use case to turn it off/on ?
|
|
||
| bool rewrite_app_http_probe = 32; | ||
|
|
||
| string proxy_init_image = 33; |
There was a problem hiding this comment.
+1 - we're not planning separate image for init.
|
|
||
| repeated string include_inbound_ports = 37; | ||
| repeated string exclude_inbound_ports = 38; | ||
| repeated string include_outbound_ports = 40; |
There was a problem hiding this comment.
Pretty sure we already have some of those - excludeOutboundPorts for sure.
Please keep consistent with the current API.
There was a problem hiding this comment.
Those exist only as annotations, not in the API yet
There was a problem hiding this comment.
Sorry, was looking at the wrong proto. For some reason there is another ProxyConfig with different content in operator, wrong autocomplete.
@howardjohn was right in the comment above - it may make sense to nest.
Please add the comments from the annotations.yaml - and include the equivalent annotation name.
Personally I'm not convinced that duplicating annotations in ProxyConfig is useful for the existing annotations.
Pain for the user to move - we should first focus on things that are not already supported. When we're done we
can come back for annotations. Or if we do want to move the annotations in ProxyConfig - we should maybe
do it consistently for all of them ( that we want to be exposed as API ).
Another aspect to keep in mind is that some of the annotations that are commonly used may also be promoted
to Sidecar.
There was a problem hiding this comment.
Yeah, I agree that duplicating the annotations is strange. However, we already had them duplicated, because the default values could be set by users in .Values.global, I just moved that over to here. I think if we want to allow users to set defaults, it should be in MeshConfig
|
Thanks for the comments @costinm @howardjohn, please note that this is an early draft, with the intention of moving us away from the spaghetti in |
|
@dgn I am very happy to remove the spaghetti, but this is not removing it its just moving it. We can use this as an opportunity to make it a better API. |
Well said ! I don't mind removing things from ProxyConfig that no longer make sense or belong to MeshConfig or are no longer used. |
That's exactly what I meant by asking for suggestions for better names and structure 🙂 I understand the urge to re-design the API but tbh that should have happened when these fields were first added. IMO, they are now API, and changing them should be treated as API changes. So I'd suggest us to carefully move settings, possibly with new name and structure, but without removing any at this point. If we then want to remove settings later on, let's do that in a follow-up PR. If you make it a condition to have a perfect API before formalizing it, I'm afraid we'll simply never formalize it. Please don't say we should 'use the opportunity' to change it now, because that to me sounds like "I don't want to look at this again tomorrow" - this will be an ongoing process. So my proposal would be: move everything we have today into MeshConfig v1alpha, e.g. into an |
No, the helm options were never considered an API - just install options for helm. There was no API review.
I don't think this is good for our users or for us. Values.yaml is a mess - I hope nobody disagree - and moving it to
We already have them in values.yaml - users are not blocked in any way. The idea that 'later we'll clean up' was also proposed for global.yaml and values.yaml - and we ended up with the |
|
To be clear: for proper inject config settings, please write a RFC. People working on CNI must also comment, and we need to be sure we want to support long term all the options. A lot of the implementation was based on the limitations of the injector - which was pretty dumb. Now in istiod we have access It is a pretty critical aspect of Istio covering multiple WG - it's a big difference between experiments and hacks in the install |
I am trying to clean up the mess here Costin, and the only thing I'm looking for is constructive feedback to move things forward, and I'm afraid what you're suggesting is going to stall this effort forever, basically making your statement here a self-fulfilling prophecy. |
|
On Tue, Jun 23, 2020 at 11:05 PM Daniel Grimm ***@***.***> wrote:
@dgn <https://github.com/dgn> I am very happy to remove the spaghetti,
but this is not removing it its just moving it. We can use this as an
opportunity to make it a better API.
That's exactly what I meant by asking for suggestions for better names and
structure 🙂 my ask is just to keep bigger refactorings and removing
graduated fields out of this PR, as they should be done in isolation,
otherwise this will never be finished.
+1
And usually API changes should have an RFC and relevant WGs involved. It is
slow - but that's a feature.
|
|
On Wed, Jun 24, 2020 at 12:19 AM Daniel Grimm ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In mesh/v1alpha1/proxy.proto
<#1478 (comment)>:
> // The mode used to redirect inbound traffic to Envoy.
InboundInterceptionMode interception_mode = 18;
+ InboundInterceptionTechnique interception_technique = 30;
Yeah, I would have seen this as the default value for whether or not to
use CNI during injection. You could override this per-proxy to use
proxy_init for example. Of course it would only make sense if CNI is
installed, I agree. That should be checked during validaton
If CNI is installed - do we even want to allow bypassing it ? In most cases
- it will not work ( PSP will prevent proxy_init ).
|
|
On Wed, Jun 24, 2020 at 10:34 AM Daniel Grimm ***@***.***> wrote:
The idea that 'later we'll clean up' was also proposed for global.yaml and
values.yaml - and we ended up with the
mess we have. If we 'move the mess' - next time the argument will be 'it's
in broad use, cost to create tooling to convert is too high'. And users
will correctly complain that Istio APIs are a mess and too complicated.
I am trying to clean up the mess here Costin, and the only thing I'm
looking for is constructive feedback to move things forward, and I'm afraid
what you're suggesting is going to stall this effort forever, basically
making your statement here a self-fulfilling prophecy.
The feedback is to do it in small chunks, with RFC for things like
interception.
There are quite a few small targeted PRs in progress moving things to
ProxyConfig and MeshConfig - meshID for example ( also actively discussed).
APIs last a long time and have a big impact on users - the 'Istio is
complex' problem is quite important. Not everything in values.yaml
needs to become an API - some of them are experimental or corner cases that
we may not need to support long term.
|
|
@dgn: PR needs rebase. Instructions 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. |
istio-testing
added
the
needs-rebase
|
#1488 is also related to injection settings - please sync on the same RFC. |
API changes for istio/istio#24898