Istio Config doesn't show correct yaml for Sidecar OutboundTrafficPolicy.Mode

[jmazzitelli]

For background, see these:

• confusion around OutboundTrafficPolicy.Mode default; protobuf enum default makes this difficult istio/istio#43657
• bad String() in istio.io/client-go/pkg/apis/networking/v1beta1.OutboundTrafficPolicy istio/istio#43643

From one of the comments in that first issue:

outboundTrafficPolicy: {}: REGISTRY_ONLY (due to proto)
outboundTrafficPolicy: null (or more likely, not present at all): ALLOW_ANY, due to Istio specific code

The problem is that proto doesn't give us any way that I can see to determine if the value was explicitly specified but stripped from the object because it was the default value (that's the {}) OR if the value was simply not defined (that's the null). So... there is really no way to know how to display this to the user once proto has serialized/deserialized the object.

Here's a quick way to see the problem.

1. Create main.go:

package main

import (
	"fmt"

	api_networking_v1beta1 "istio.io/api/networking/v1beta1"
	networking_v1beta1 "istio.io/client-go/pkg/apis/networking/v1beta1"
)

func main() {
	sc1 := &networking_v1beta1.Sidecar{
		Spec: api_networking_v1beta1.Sidecar{
			WorkloadSelector: &api_networking_v1beta1.WorkloadSelector{
				Labels: map[string]string{
					"modeSpecified": "yes",
				},
			},
			OutboundTrafficPolicy: &api_networking_v1beta1.OutboundTrafficPolicy{
				Mode: api_networking_v1beta1.OutboundTrafficPolicy_REGISTRY_ONLY,
			},
		},
	}
	sc2 := &networking_v1beta1.Sidecar{
		Spec: api_networking_v1beta1.Sidecar{
			WorkloadSelector: &api_networking_v1beta1.WorkloadSelector{
				Labels: map[string]string{
					"modeSpecified": "no",
				},
			},
			OutboundTrafficPolicy: &api_networking_v1beta1.OutboundTrafficPolicy{
			},
		},
	}
	b, _ := sc1.Spec.MarshalJSON()
	fmt.Printf("sc1 json  --> %v\n", string(b))
	b, _ = sc1.Spec.GetOutboundTrafficPolicy().MarshalJSON()
	fmt.Printf("otp1 json --> %v\n", string(b))
	fmt.Printf("sc1.Mode  --> %v\n", sc1.Spec.GetOutboundTrafficPolicy().Mode)

	b, _ = sc2.Spec.MarshalJSON()
	fmt.Printf("sc2 json  --> %v\n", string(b))
	b, _ = sc2.Spec.GetOutboundTrafficPolicy().MarshalJSON()
	fmt.Printf("otp2 json --> %v\n", string(b))
	fmt.Printf("sc2.Mode  --> %v\n", sc2.Spec.GetOutboundTrafficPolicy().Mode)
}

2. Create go.mod:

module main

require (
	istio.io/api v0.0.0-20230217221049-9d422bf48675
	istio.io/client-go v1.17.1
)

3. Run go mod tidy
4. Build with go build
5. Run the program ./main

Now notice the output:

sc1 json  --> {"workloadSelector":{"labels":{"modeSpecified":"yes"}},"outboundTrafficPolicy":{}}
otp1 json --> {}
sc1.Mode  --> REGISTRY_ONLY
sc2 json  --> {"workloadSelector":{"labels":{"modeSpecified":"no"}},"outboundTrafficPolicy":{}}
otp2 json --> {}
sc2.Mode  --> REGISTRY_ONLY

Notice in both cases (where the mode was explicitly set to the default of REGISTRY_ONLY and where mode was not set at all), there is no way to discern whether mode was explicitly set or not. The JSON gives no indication (both mode JSON representations are {}) and directly accessing the Mode field are both REGISTRY_ONLY.

The confusion lies in the fact that Istio behavior is different if Mode is explicitly set to REGISTRY_ONLY or if it is left undefined (in that case, Istio will default to ALLOW_ALL). But Kiali has no way of knowing which one to show the user because it can't tell if Mode is explicitly set to REGISTRY_ONLY or if it was left unset. Read this issue for further explanation on the confusion.

It is unfortunate that the Istio client has coded up the Mode enum's default value to be REGISTRY_ONLY when the Istio default behavior is ALLOW_ANY. But, alas, that is the state of the code and it doesn't seem like it will change in the near future.

So... this issue is to determine what the Kiali UI should do. This may involve simply documentation (perhaps a Release Notes or FAQ "known issues" blurb).

[jshaughn]

I think I'm in favor of adding some sort of "Help" information in the side-panel. FAQs are good but I don't think people usually think, or spend the time, to look. Known problems is fine but also not going to help the active user.

[jmazzitelli]

adding some sort of "Help" information in the side-panel.

I'm OK with that. We could have a small blurb in the Help text, but in there we also should point them to istio/istio#43657 so they can read about the details. I don't think we'll be able to fully explain this in a Help text. We'll want to think about what we want to say in the Help text (which is only going to be a few sentences at most).

[jshaughn]

How about:

The YAML outboundTrafficPolicy of {} is ambiguous. It can not be properly displayed due to an Istio limitation. {} indicates ALLOW_ANY when the setting is left unset. {} indicates REGISTRY_ONLY when the setting is set to REGISTRY_ONLY. To inspect the value you must use other means.

[jmazzitelli]

If possible, we should only show that when the value is explicitly showing as {} (because if it shows some actual value, we know it is correct - no ambiguity).

So we should show a little "i" information bubble here on this line when the value is {} (and only when it is that value). I assume that is possible.