istioctl: avoid parsing strings to ints for string fields

This uses reflection to check field types of our values. The performance
hit should be negligible as this only runs as part of istioctl. The
function turned out to be a bit more complex because we a) use a map
for values instead of the struct in our IstioOperator CRD and b)
protoc-gen-go does not generate proper json tags.

Fixes istio#50990

operator/pkg/apis/istio/v1alpha1/common.go

@@ -15,6 +15,12 @@
 package v1alpha1
 
 import (
+	"fmt"
+	"reflect"
+	"strings"
+
+	"golang.org/x/text/cases"
+	"golang.org/x/text/language"
 	"istio.io/api/operator/v1alpha1"
 )
 
@@ -50,3 +56,57 @@ func SetNamespace(iops *v1alpha1.IstioOperatorSpec, namespace string) {
 	}
 	// TODO implement
 }
+
+// GetFieldType uses reflection to retrieve the reflect.Type of a nested field
+// As the IstioOperator type only uses a map[string]any for values, it will
+// traverse either IstioOperatorSpec or Values depending on the given path
+func GetFieldType(path string) (reflect.Type, error) {
+	titleCaser := cases.Title(language.English, cases.NoLower)
+	iopSpec := v1alpha1.IstioOperatorSpec{}
+	values := Values{}
+	path = strings.TrimPrefix(path, "spec.")
+	pathParts := strings.Split(path, ".")
+	var currentNode reflect.Type
+	// traverse either values or iopSpec
+	currentPath := "spec"
+	if len(pathParts) > 0 && pathParts[0] == "values" {
+		currentNode = reflect.TypeOf(&values).Elem()
+		pathParts = pathParts[1:]
+		currentPath += ".values"
+	} else {
+		currentNode = reflect.TypeOf(&iopSpec).Elem()
+	}
+	for _, part := range pathParts {
+		// remove any slice brackets from the path
+		if sliceBrackets := strings.Index(part, "["); sliceBrackets > 0 {
+			part = part[:sliceBrackets]
+		}
+		currentPath += "." + part
+		nextNode, found := currentNode.FieldByName(titleCaser.String(part))
+		if !found {
+			// iterate over fields to compare against json tags
+			for i := 0; i < currentNode.NumField(); i++ {
+				f := currentNode.Field(i)
+				if strings.Contains(f.Tag.Get("protobuf"), "json="+part+",") {
+					nextNode = f
+					found = true
+					break
+				}
+			}
+			if !found {
+				return nil, fmt.Errorf("failed to identify type of %s: field %s does not exist", path, currentPath)
+			}
+		}
+		if nextNode.Type.Kind() == reflect.Slice || nextNode.Type.Kind() == reflect.Pointer {
+			currentNode = nextNode.Type.Elem()
+		} else if nextNode.Type.Kind() == reflect.Map {
+			currentNode = nextNode.Type.Elem()
+		} else {
+			currentNode = nextNode.Type
+		}
+		if currentNode.Kind() != reflect.Struct {
+			break
+		}
+	}
+	return currentNode, nil
+}

operator/pkg/manifest/shared.go

@@ -40,7 +40,6 @@ import (
 	"istio.io/istio/operator/pkg/validate"
 	"istio.io/istio/pkg/kube"
 	"istio.io/istio/pkg/log"
-	"istio.io/istio/pkg/util/sets"
 	pkgversion "istio.io/istio/pkg/version"
 )
 
@@ -470,10 +469,6 @@ func getInstallPackagePath(iopYAML string) (string, error) {
 	return iop.Spec.InstallPackagePath, nil
 }
 
-// alwaysString represents types that should always be decoded as strings
-// TODO: this could be automatically derived from the value_types.proto?
-var alwaysString = sets.New("values.compatibilityVersion", "compatibilityVersion")
-
 // overlaySetFlagValues overlays each of the setFlags on top of the passed in IOP YAML string.
 func overlaySetFlagValues(iopYAML string, setFlags []string) (string, error) {
 	iop := make(map[string]any)
@@ -494,7 +489,11 @@ func overlaySetFlagValues(iopYAML string, setFlags []string) (string, error) {
 		}
 		// input value type is always string, transform it to correct type before setting.
 		var val any = v
-		if !alwaysString.Contains(p) {
+		fieldType, err := iopv1alpha1.GetFieldType(p)
+		if err != nil {
+			return "", err
+		}
+		if fieldType.Kind() != reflect.String {
 			val = util.ParseValue(v)
 		}
 		if err := tpath.WritePathContext(inc, val, false); err != nil {

operator/pkg/manifest/shared_test.go

@@ -21,10 +21,14 @@ import (
 	"path/filepath"
 	"testing"
 
+	"istio.io/istio/operator/pkg/helm"
 	"istio.io/istio/operator/pkg/util"
+	"istio.io/istio/operator/pkg/util/clog"
 	"istio.io/istio/pkg/test/env"
 )
 
+const operatorSubdirFilePath = "manifests"
+
 func TestReadLayeredYAMLs(t *testing.T) {
 	testDataDir := filepath.Join(env.IstioSrc, "operator/pkg/util/testdata/yaml")
 	tests := []struct {
@@ -152,3 +156,53 @@ func TestConvertIOPMapValues(t *testing.T) {
 		})
 	}
 }
+
+func TestGenIOPFromProfile(t *testing.T) {
+	tests := []struct {
+		name        string
+		setFlags    []string
+		expectError bool
+	}{
+		{
+			name:     "cpu_limits",
+			setFlags: []string{"values.global.proxy.resources.limits.cpu=200"},
+		},
+		{
+			name:     "compatibilityVersion",
+			setFlags: []string{"values.compatibilityVersion=1.20", "compatibilityVersion=1.20"},
+		},
+		{
+			name:     "ingressGateways",
+			setFlags: []string{"components.ingressGateways[0].enabled=true", "components.ingressGateways[0].name=test"},
+		},
+		{
+			name:     "egressGatewayJsonNotation",
+			setFlags: []string{"values.gateways.istio-egressgateway.enabled=true"},
+		},
+		{
+			name:        "nonexistant_field",
+			setFlags:    []string{"values.global.proxy.unknown.image=test"},
+			expectError: true,
+		},
+	}
+	manifests := filepath.Join(env.IstioSrc, operatorSubdirFilePath)
+	profiles, err := helm.ListProfiles(manifests)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(profiles) < 2 {
+		// Just ensure we find some profiles, in case this code breaks
+		t.Fatalf("Maybe have failed getting profiles, got %v", profiles)
+	}
+	l := clog.NewConsoleLogger(os.Stdout, os.Stderr, nil)
+	for _, profile := range profiles {
+		for _, tc := range tests {
+			t.Run(profile+"_"+tc.name, func(t *testing.T) {
+				_, _, err := GenIOPFromProfile(profile, "", append([]string{"installPackagePath=" + manifests}, tc.setFlags...), false, false, nil, l)
+				if (err != nil) != tc.expectError {
+					t.Fatal(err)
+				}
+			})
+		}
+	}
+}

releasenotes/notes/50990.yaml

@@ -0,0 +1,8 @@
+apiVersion: release-notes/v2
+kind: bug-fix
+area: istioctl
+issue:
+- 50990
+releaseNotes:
+- |
+  **Fixed** a bug in istioctl where it would parse a field value into a non-string type which then failed to be assigned to the string field.