istio
Fix merge trafficpolicy
type TrafficPolicy struct {
...
Tls *ClientTLSSettings `protobuf:"bytes,4,opt,name=tls,proto3" json:"tls,omitempty"`
// Traffic policies specific to individual ports. Note that port level
// settings will override the destination-level settings. Traffic
// settings specified at the destination-level will not be inherited when
// overridden by port-level settings, i.e. default values will be applied
// to fields omitted in port-level traffic policies.
PortLevelSettings []*TrafficPolicy_PortTrafficPolicy `protobuf:"bytes,5,rep,name=port_level_settings,json=portLevelSettings,proto3" json:"port_level_settings,omitempty"`
The api says we should do what this patch does
Another thing is current documented behaviour is different from what subset level traffic policy does? I do not know if there are any specific reasons for port level policies to not inherit parent -One reason I could think is, if it is a different port in a service, it is considered different set of APIs exposed by same service and hence should not inherit from main policy unlike subsets.
I think we should avoid breaking users - if we have to change may be some flag to control the new behaviour may be better.
One reason I could thin is, if it is a different port in a service, it is considered different set of APIs exposed by same service and hence should not inherit from main policy unlike subsets.
I do believe this was the original design reasoning, as well as indecision on how main, subset, and port policies should interact with one another. Based on the API, the port level policies should be getting the default values specified in MeshConfig: h2UpgradePolicy, tcpKeepalive, localityLbSetting, timeouts, etc but NOT any of the main or subset trafficPolicy settings. The DestinationRule API was written before MeshConfig, and so I assume as more settings got added to MeshConfig that implementation detail was overlooked.
I am a bit more inclined to just change the API docs to match the real behavior. WDYT?
TBH, I donot think this is a good way. It will make users feel istio community would respect a mistake they made yesterday rather than fixing it.
TBH, I donot think this is a good way. It will make users feel istio community would respect a mistake they made yesterday rather than fixing it.
I agree. If the original design was to have different port level settings to cater to different set of APIs that @GregHanson seems to also agree with - I think we can introduce a flag to change to new behaviour (documented behaviour) and add a release note to flip it back if needed and we can remove the flag few releases down the line (Similar to how Envoy handles breaking changes with runtime configuration). May be we need some thing similar in Istio rather than adding too many env var flags. But we can do that later
TBH, I donot think this is a good way. It will make users feel istio community would respect a mistake they made yesterday rather than fixing it.
Yes, this is exactly what I want. Our users have given us feedback over and over again that the most painful part of Istio is making backwards incompatible changes. Very few things are more important than preserving compatibility IMO. This doesn't seem like one of them.
Similar to how Envoy handles breaking changes with runtime configuration
Envoy's API is largely an implementation detail of products, so I wouldn't use it as a rubrik for a good way to make breaking changes, as these are often hidden by higher level APIs like Istio
@howardjohn What do you mean? Should i continue fixing in this PR or not?
// Traffic policies specific to individual ports. Note that port level
// settings will override the destination-level settings. Traffic
// settings specified at the destination-level will not be inherited when
// overridden by port-level settings, i.e. default values will be applied
// to fields omitted in port-level traffic policies.
PortLevelSettings []*TrafficPolicy_PortTrafficPolicy `protobuf:"bytes,5,rep,name=port_level_settings,json=portLevelSettings,proto3" json:"port_level_settings,omitempty"`
This is not changing the API, on the opposite, I think the current implementation is not right.
This is not changing the API, on the opposite, I think the current implementation is not right.
It is changing the behavior of existing APIs. Meaning if a user upgrades, they will immediately get different behavior and likely cause outages, etc. The implementation is more important than the docs unfortunately, as that is what actually impacts users.
Personally, i think it is bad, this means we donot want to admit a mistake made before, but would rather say hey, this is not my fault, this is the rule's fault
It is bad. But making breaking API changes is the absolute worst thing we can do based on user feedback research
🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2022-07-08. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.
Created by the issue and PR lifecycle manager.
