bad String() in istio.io/client-go/pkg/apis/networking/v1beta1.OutboundTrafficPolicy

[jmazzitelli]

Bug Description

Kiali uses the Istio client. We are finding the Istio client cannot generate proper yaml/json for Sidecar objects for display to the user because the value of OutboundTrafficPolicy is coming back empty when it should not be.

Here's simple replication procedures to see the problem.

1. Create this 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() {
	sc := &networking_v1beta1.Sidecar{
		Spec: api_networking_v1beta1.Sidecar{
			WorkloadSelector: &api_networking_v1beta1.WorkloadSelector{
				Labels: map[string]string{
					"foo": "bar",
				},
			},
			OutboundTrafficPolicy: &api_networking_v1beta1.OutboundTrafficPolicy{
				Mode: api_networking_v1beta1.OutboundTrafficPolicy_REGISTRY_ONLY,
			},
		},
	}
	fmt.Printf("sc: %v\n", sc.Spec.String())
}

2. Create this go.mod

module main

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

3. Run the following commands:

go mod tidy
go build main.go
./main

4. See the results:

sc: workload_selector:{labels:{key:"foo" value:"bar"}} outbound_traffic_policy:{}

Notice outbound_traffic_policy:{} -- why is this empty? The code set it to include mode set to REGISTRY_ONLY:

			OutboundTrafficPolicy: &api_networking_v1beta1.OutboundTrafficPolicy{
				Mode: api_networking_v1beta1.OutboundTrafficPolicy_REGISTRY_ONLY,
			},

This manifests itself in the Kiali UI when you look at Sidecar objects in the Kiali YAML editor - notice the empty outbound traffic policy:

Yet, I know the outbound traffic policy is set by examining the object directly:

$ kubectl get sidecars.networking.istio.io default -n mazz -o jsonpath='{.spec.outboundTrafficPolicy}{"\n"}'
{"mode":"REGISTRY_ONLY"}

The entire spec looks like:

$ kubectl get sidecars.networking.istio.io default -n mazz -o jsonpath='{.spec}{"\n"}'
{"egress":[{"hosts":["./*","istio-system/*"]}],"outboundTrafficPolicy":{"mode":"REGISTRY_ONLY"}}

Version

This is not a runtime Istio issue. See the code that replicates the problem to see the versions involved.

Additional Information

No response

[howardjohn]

this is how protobuf works afaik. It doesn't actually know if you set the enum or not since the registry only is the default value. Cannot tell zero vs unset in Go

[jmazzitelli]

Wow, OK, I just got a history lesson on nullable/unset fields in protobuf by reading:

• C#: Consider exposing protobuf optional fields as nullable properties protocolbuffers/protobuf#9083
• default values and testing if a field is set in v3 protocolbuffers/protobuf#359

I don't think there is an easy solution here for Kiali to be able to use and display the correct YAML (at least when going through the protobuf encoding).

[jmazzitelli]

https://protobuf.dev/programming-guides/proto3/#default

Note that for scalar message fields, once a message is parsed there’s no way of telling
whether a field was explicitly set to the default value (for example whether a boolean
was set to false) or just not set at all: you should bear this in mind when defining your
message types. For example, don’t have a boolean that switches on some behavior
when set to false if you don’t want that behavior to also happen by default.
Also note that if a scalar message field is set to its default, the value will not be serialized
on the wire.

Note the last sentence, thar's our problem:

Also note that if a scalar message field is set to its default, the value will not be serialized on the wire.

So when we deserialize it, it seems to not be explicitly set to the default value again (it remains unset).

I'm closing this issue. There is nothing for the Istio client to do here. I'll have to see what hacks Kiali can do to work around this.

[jmazzitelli]

See this followup issue that discusses this protobuf default handling and how it doesn't match that of the documentation (or, at least some of the documentation): #43657