Consolidate on YAML library usage (istio#52445)

* Consolidate on YAML library usage * fixes
MorrisLaw · Aug 1, 2024 · d12a0ff · d12a0ff
1 parent d66028e
commit d12a0ff
Show file tree

Hide file tree

Showing 21 changed files with 73 additions and 427 deletions.
diff --git a/common/config/.golangci.yml b/common/config/.golangci.yml
@@ -226,6 +226,12 @@ linters-settings:
             desc: "do not use golang.org/x/exp/slices; use istio.io/istio/pkg/slices instead"
           - pkg: slices
             desc: "do not use slices; use istio.io/istio/pkg/slices instead"
+          - pkg: gopkg.in/yaml.v2
+            desc: "do not use gopkg.in/yaml.v2; use sigs.k8s.io/yaml instead"
+          - pkg: gopkg.in/yaml.v3
+            desc: "do not use gopkg.in/yaml.v3; use sigs.k8s.io/yaml instead"
+          - pkg: github.com/ghodss/yaml
+            desc: "do not use github.com/ghodss/yaml; use sigs.k8s.io/yaml instead"
       DenyOperatorAndIstioctl:
         files:
           # Tests can do anything

diff --git a/istioctl/pkg/precheck/precheck.go b/istioctl/pkg/precheck/precheck.go
@@ -23,11 +23,11 @@ import (
 
 	"github.com/fatih/color"
 	"github.com/spf13/cobra"
-	"gopkg.in/yaml.v2"
 	authorizationapi "k8s.io/api/authorization/v1"
 	crd "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/yaml"
 
 	"istio.io/api/label"
 	networking "istio.io/api/networking/v1alpha3"

diff --git a/istioctl/pkg/validate/validate.go b/istioctl/pkg/validate/validate.go
@@ -15,6 +15,7 @@
 package validate
 
 import (
+	"bufio"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -25,10 +26,11 @@ import (
 
 	"github.com/hashicorp/go-multierror"
 	"github.com/spf13/cobra"
-	"gopkg.in/yaml.v2"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	kubeyaml "k8s.io/apimachinery/pkg/util/yaml"
+	"sigs.k8s.io/yaml"
 
 	"istio.io/istio/istioctl/pkg/cli"
 	operatoristio "istio.io/istio/operator/pkg/apis/istio"
@@ -236,25 +238,26 @@ func GetTemplateLabels(u *unstructured.Unstructured) (map[string]string, error)
 
 func (v *validator) validateFile(path string, istioNamespace *string, defaultNamespace string, reader io.Reader, writer io.Writer,
 ) (validation.Warning, error) {
-	decoder := yaml.NewDecoder(reader)
-	decoder.SetStrict(true)
+	yamlReader := kubeyaml.NewYAMLReader(bufio.NewReader(reader))
 	var errs error
 	var warnings validation.Warning
 	for {
-		// YAML allows non-string keys and the produces generic keys for nested fields
-		raw := make(map[any]any)
-		err := decoder.Decode(&raw)
+		doc, err := yamlReader.Read()
 		if err == io.EOF {
 			return warnings, errs
 		}
 		if err != nil {
 			errs = multierror.Append(errs, multierror.Prefix(err, fmt.Sprintf("failed to decode file %s: ", path)))
 			return warnings, errs
 		}
-		if len(raw) == 0 {
+		if len(doc) == 0 {
 			continue
 		}
-		out := transformInterfaceMap(raw)
+		out := map[string]any{}
+		if err := yaml.UnmarshalStrict(doc, &out); err != nil {
+			errs = multierror.Append(errs, multierror.Prefix(err, fmt.Sprintf("failed to decode file %s: ", path)))
+			return warnings, errs
+		}
 		un := unstructured.Unstructured{Object: out}
 		warning, err := v.validateResource(*istioNamespace, defaultNamespace, &un, writer)
 		if err != nil {
@@ -445,33 +448,6 @@ func warningToString(w validation.Warning) string {
 	return w.Error()
 }
 
-func transformInterfaceArray(in []any) []any {
-	out := make([]any, len(in))
-	for i, v := range in {
-		out[i] = transformMapValue(v)
-	}
-	return out
-}
-
-func transformInterfaceMap(in map[any]any) map[string]any {
-	out := make(map[string]any, len(in))
-	for k, v := range in {
-		out[fmt.Sprintf("%v", k)] = transformMapValue(v)
-	}
-	return out
-}
-
-func transformMapValue(in any) any {
-	switch v := in.(type) {
-	case []any:
-		return transformInterfaceArray(v)
-	case map[any]any:
-		return transformInterfaceMap(v)
-	default:
-		return v
-	}
-}
-
 func servicePortPrefixed(n string) bool {
 	i := strings.IndexByte(n, '-')
 	if i >= 0 {

diff --git a/operator/pkg/patch/patch.go b/operator/pkg/patch/patch.go
@@ -99,7 +99,7 @@ import (
 	"fmt"
 	"strings"
 
-	yaml2 "gopkg.in/yaml.v2"
+	yaml2 "gopkg.in/yaml.v2" // nolint: depguard // needed for weird tricks
 
 	"istio.io/api/operator/v1alpha1"
 	"istio.io/istio/operator/pkg/helm"

diff --git a/operator/pkg/tpath/tree.go b/operator/pkg/tpath/tree.go
@@ -30,8 +30,7 @@ import (
 	"strconv"
 	"strings"
 
-	"gopkg.in/yaml.v2"
-	yaml2 "sigs.k8s.io/yaml"
+	"sigs.k8s.io/yaml"
 
 	"istio.io/istio/operator/pkg/util"
 	"istio.io/istio/pkg/log"
@@ -556,7 +555,7 @@ func tryToUnmarshalStringToYAML(s any) (any, bool) {
 				// treat JSON as string
 				return vv, false
 			}
-			if err := yaml2.Unmarshal([]byte(vv.(string)), &nv); err == nil {
+			if err := yaml.Unmarshal([]byte(vv.(string)), &nv); err == nil {
 				return nv, true
 			}
 		}

diff --git a/operator/pkg/tpath/util.go b/operator/pkg/tpath/util.go
@@ -19,8 +19,7 @@ util.go contains utility function for dealing with trees.
 package tpath
 
 import (
-	"gopkg.in/yaml.v2"
-	yaml2 "sigs.k8s.io/yaml"
+	"sigs.k8s.io/yaml"
 
 	"istio.io/istio/operator/pkg/util"
 )
@@ -42,15 +41,15 @@ func AddSpecRoot(tree string) (string, error) {
 // GetConfigSubtree returns the subtree at the given path.
 func GetConfigSubtree(manifest, path string) (string, error) {
 	root := make(map[string]any)
-	if err := yaml2.Unmarshal([]byte(manifest), &root); err != nil {
+	if err := yaml.Unmarshal([]byte(manifest), &root); err != nil {
 		return "", err
 	}
 
 	nc, _, err := GetPathContext(root, util.PathFromString(path), false)
 	if err != nil {
 		return "", err
 	}
-	out, err := yaml2.Marshal(nc.Node)
+	out, err := yaml.Marshal(nc.Node)
 	if err != nil {
 		return "", err
 	}

diff --git a/operator/pkg/translate/yaml_tree.go b/operator/pkg/translate/yaml_tree.go
@@ -15,7 +15,7 @@
 package translate
 
 import (
-	"gopkg.in/yaml.v2"
+	"sigs.k8s.io/yaml"
 
 	"istio.io/istio/operator/pkg/tpath"
 	"istio.io/istio/operator/pkg/util"

diff --git a/operator/pkg/version/version.go b/operator/pkg/version/version.go
@@ -20,7 +20,6 @@ import (
 	"strings"
 
 	goversion "github.com/hashicorp/go-version"
-	"gopkg.in/yaml.v2"
 )
 
 const (
@@ -57,12 +56,11 @@ func NewVersionFromString(s string) (*Version, error) {
 
 // IsVersionString checks whether the given string is a version string
 func IsVersionString(path string) bool {
-	_, err := goversion.NewSemver(path)
-	if err != nil {
+	if _, err := goversion.NewSemver(path); err != nil {
 		return false
 	}
-	vs := Version{}
-	return yaml.Unmarshal([]byte(path), &vs) == nil
+	_, err := NewVersionFromString(path)
+	return err == nil
 }
 
 // TagToVersionString converts an istio container tag into a version string
@@ -167,17 +165,3 @@ func (v *Version) String() string {
 	}
 	return fmt.Sprintf("%s-%s", v.PatchVersion, v.Suffix)
 }
-
-// UnmarshalYAML implements the Unmarshaler interface.
-func (v *Version) UnmarshalYAML(unmarshal func(any) error) error {
-	s := ""
-	if err := unmarshal(&s); err != nil {
-		return err
-	}
-	out, err := NewVersionFromString(s)
-	if err != nil {
-		return err
-	}
-	*v = *out
-	return nil
-}
diff --git a/operator/pkg/version/version_test.go b/operator/pkg/version/version_test.go
@@ -15,85 +15,80 @@
 package version
 
 import (
-	"fmt"
+	"strings"
 	"testing"
 
-	"gopkg.in/yaml.v2"
-
 	"istio.io/istio/pkg/test/util/assert"
 )
 
 func TestVersion(t *testing.T) {
 	tests := []struct {
 		desc    string
-		yamlStr string
+		input   string
 		want    Version
 		wantErr string
 	}{
 		{
-			desc: "nil success",
-		},
-		{
-			desc:    "major success",
-			yamlStr: "1",
-			want:    NewVersion(1, 0, 0, ""),
+			desc:  "major success",
+			input: "1",
+			want:  NewVersion(1, 0, 0, ""),
 		},
 		{
 			desc:    "major fail",
-			yamlStr: "1..",
+			input:   "1..",
 			wantErr: `Malformed version: 1..`,
 		},
 		{
 			desc:    "major fail prefix",
-			yamlStr: ".1",
+			input:   ".1",
 			wantErr: `Malformed version: .1`,
 		},
 		{
-			desc:    "minor success",
-			yamlStr: "1.2",
-			want:    NewVersion(1, 2, 0, ""),
+			desc:  "minor success",
+			input: "1.2",
+			want:  NewVersion(1, 2, 0, ""),
 		},
 		{
 			desc:    "minor fail",
-			yamlStr: "1.1..",
+			input:   "1.1..",
 			wantErr: `Malformed version: 1.1..`,
 		},
 		{
-			desc:    "patch success",
-			yamlStr: "1.2.3",
-			want:    NewVersion(1, 2, 3, ""),
+			desc:  "patch success",
+			input: "1.2.3",
+			want:  NewVersion(1, 2, 3, ""),
 		},
 		{
 			desc:    "patch fail",
-			yamlStr: "1.1.-1",
+			input:   "1.1.-1",
 			wantErr: `Malformed version: 1.1.-1`,
 		},
 		{
-			desc:    "suffix success",
-			yamlStr: "1.2.3-istio-test",
-			want:    NewVersion(1, 2, 3, "istio-test"),
+			desc:  "suffix success",
+			input: "1.2.3-istio-test",
+			want:  NewVersion(1, 2, 3, "istio-test"),
 		},
 		{
 			desc:    "suffix fail",
-			yamlStr: ".1.1.1-something",
+			input:   ".1.1.1-something",
 			wantErr: `Malformed version: .1.1.1-something`,
 		},
 		{
 			desc:    "Malformed version fail",
-			yamlStr: "istio-testing-distroless",
+			input:   "istio-testing-distroless",
 			wantErr: `Malformed version: istio-testing-distroless`,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
-			got := Version{}
-			err := yaml.Unmarshal([]byte(tt.yamlStr), &got)
-			if gotErr, wantErr := errToString(err), tt.wantErr; gotErr != wantErr {
-				t.Fatalf("yaml.Unmarshal(%s): got error: %s, want error: %s", tt.desc, gotErr, wantErr)
+			got, err := NewVersionFromString(tt.input)
+			if gotErr, wantErr := errToString(err), tt.wantErr; !strings.Contains(gotErr, wantErr) {
+				t.Fatalf("got error: %s, want error: %s", gotErr, wantErr)
 			}
 			if tt.wantErr == "" {
-				assert.Equal(t, got, tt.want)
+				assert.Equal(t, true, got != nil)
+				assert.Equal(t, *got, tt.want)
 			}
 		})
 	}
@@ -302,16 +297,3 @@ func TestVersionString(t *testing.T) {
 		})
 	}
 }
-
-func TestUnmarshalYAML(t *testing.T) {
-	v := &Version{}
-	expectedErr := fmt.Errorf("test error")
-	errReturn := func(any) error { return expectedErr }
-	gotErr := v.UnmarshalYAML(errReturn)
-	if gotErr == nil {
-		t.Errorf("expected error but got nil")
-	}
-	if gotErr != expectedErr {
-		t.Errorf("error mismatch")
-	}
-}
diff --git a/pilot/pkg/config/file/store.go b/pilot/pkg/config/file/store.go
@@ -28,7 +28,7 @@ import (
 	"sync"
 
 	"github.com/hashicorp/go-multierror"
-	yamlv3 "gopkg.in/yaml.v3"
+	yamlv3 "gopkg.in/yaml.v3" // nolint: depguard // needed for line numbers
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"

diff --git a/pilot/pkg/config/kube/crd/conversion.go b/pilot/pkg/config/kube/crd/conversion.go
@@ -22,9 +22,9 @@ import (
 	"reflect"
 
 	"github.com/hashicorp/go-multierror"
-	"gopkg.in/yaml.v2"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	kubeyaml "k8s.io/apimachinery/pkg/util/yaml"
+	"sigs.k8s.io/yaml"
 
 	"istio.io/istio/pkg/config"
 	"istio.io/istio/pkg/config/schema/collections"

diff --git a/pkg/test/framework/components/cluster/config.go b/pkg/test/framework/components/cluster/config.go
@@ -19,11 +19,11 @@ import (
 )
 
 type Config struct {
-	Name               string     `yaml:"clusterName,omitempty"`
-	Network            string     `yaml:"network,omitempty"`
-	HTTPProxy          string     `yaml:"httpProxy,omitempty"`
-	ProxyKubectlOnly   bool       `yaml:"proxyKubectlOnly,omitempty"`
-	PrimaryClusterName string     `yaml:"primaryClusterName,omitempty"`
-	ConfigClusterName  string     `yaml:"configClusterName,omitempty"`
-	Meta               config.Map `yaml:"meta,omitempty"`
+	Name               string     `json:"clusterName,omitempty"`
+	Network            string     `json:"network,omitempty"`
+	HTTPProxy          string     `json:"httpProxy,omitempty"`
+	ProxyKubectlOnly   bool       `json:"proxyKubectlOnly,omitempty"`
+	PrimaryClusterName string     `json:"primaryClusterName,omitempty"`
+	ConfigClusterName  string     `json:"configClusterName,omitempty"`
+	Meta               config.Map `json:"meta,omitempty"`
 }
diff --git a/pkg/test/framework/components/echo/config.go b/pkg/test/framework/components/echo/config.go
@@ -21,7 +21,7 @@ import (
 	"time"
 
 	"github.com/mitchellh/copystructure"
-	"gopkg.in/yaml.v3"
+	"sigs.k8s.io/yaml"
 
 	"istio.io/api/annotation"
 	"istio.io/istio/pkg/config/constants"