Add proper validation to mesh config (istio#22951)

* Add proper validation to mesh config

WIP until istio/api#1384 lands

* lint

* Update CRDs

* update

go.mod

@@ -153,7 +153,7 @@ require (
 	gopkg.in/d4l3k/messagediff.v1 v1.2.1
 	gopkg.in/square/go-jose.v2 v2.3.1
 	gopkg.in/yaml.v2 v2.2.8
-	istio.io/api v0.0.0-20200416152425-5eb0c08ebd2b
+	istio.io/api v0.0.0-20200416233651-7dddf3e33eca
 	istio.io/gogo-genproto v0.0.0-20200326154102-997c228eecef
 	istio.io/pkg v0.0.0-20200327214633-ce134a9bd104
 	k8s.io/api v0.18.1

go.sum

@@ -972,8 +972,8 @@ honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWh
 honnef.co/go/tools v0.0.1-2019.2.3 h1:3JgtbtFHMiCmsznwGVTUWbgGov+pVqnlf1dEJTNAXeM=
 honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt0JzvZhAg=
 istio.io/api v0.0.0-20190515205759-982e5c3888c6/go.mod h1:hhLFQmpHia8zgaM37vb2ml9iS5NfNfqZGRt1pS9aVEo=
-istio.io/api v0.0.0-20200416152425-5eb0c08ebd2b h1:VQIylP5lEF4z4kmy/ZxcpXMwKpKYYdYqLAwahAlcdAU=
-istio.io/api v0.0.0-20200416152425-5eb0c08ebd2b/go.mod h1:kyq3g5w42zl/AKlbzDGppYpGMQYMYMyZKeq0/eexML8=
+istio.io/api v0.0.0-20200416233651-7dddf3e33eca h1:FUQBHdkSnZ3Yx/StoboZdovPpDeYhZ8Z/NKcL69Q6jA=
+istio.io/api v0.0.0-20200416233651-7dddf3e33eca/go.mod h1:kyq3g5w42zl/AKlbzDGppYpGMQYMYMyZKeq0/eexML8=
 istio.io/gogo-genproto v0.0.0-20190930162913-45029607206a/go.mod h1:OzpAts7jljZceG4Vqi5/zXy/pOg1b209T3jb7Nv5wIs=
 istio.io/gogo-genproto v0.0.0-20200326154102-997c228eecef h1:jVlVwrFW1LIm9XyPnvthNEvLjALn1zsTysGRKctl4Lc=
 istio.io/gogo-genproto v0.0.0-20200326154102-997c228eecef/go.mod h1:OzpAts7jljZceG4Vqi5/zXy/pOg1b209T3jb7Nv5wIs=

operator/cmd/mesh/testdata/manifest-generate/input/pilot_merge_meshconfig.yaml

@@ -6,11 +6,13 @@ spec:
   tag: 1.1.4
   meshConfig:
     rootNamespace: istio-control
-    mixerCheckServer: foo
+    mixerCheckServer: foo:1234
     outboundTrafficPolicy:
       mode: REGISTRY_ONLY
     defaultConfig:
+      discoveryAddress: my-discovery:123
       drainDuration: 12s
+      controlPlaneAuthPolicy: NONE
       proxyMetadata:
         TERMINATION_DRAIN_DURATION_SECONDS: "120"
   components:

operator/cmd/mesh/testdata/manifest-generate/output/pilot_merge_meshconfig.golden.yaml

@@ -29,7 +29,7 @@ data:
       configPath: /etc/istio/proxy
       connectTimeout: 10s
       controlPlaneAuthPolicy: NONE
-      discoveryAddress: istiod.istio-system.svc:15012
+      discoveryAddress: my-discovery:123
       drainDuration: 12s
       parentShutdownDuration: 1m0s
       proxyAdminPort: 15000
@@ -49,9 +49,9 @@ data:
     ingressService: istio-ingressgateway
     localityLbSetting:
       enabled: true
-    mixerCheckServer: foo
+    mixerCheckServer: foo:1234
     outboundTrafficPolicy:
-      mode: ALLOW_ANY
+      mode: REGISTRY_ONLY
     protocolDetectionTimeout: 100ms
     reportBatchMaxEntries: 100
     reportBatchMaxTime: 1s

operator/pkg/component/component.go

@@ -27,15 +27,15 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 	"istio.io/api/operator/v1alpha1"
+	"istio.io/pkg/log"
+
 	"istio.io/istio/operator/pkg/helm"
 	"istio.io/istio/operator/pkg/name"
 	"istio.io/istio/operator/pkg/object"
 	"istio.io/istio/operator/pkg/patch"
 	"istio.io/istio/operator/pkg/tpath"
 	"istio.io/istio/operator/pkg/translate"
 	"istio.io/istio/operator/pkg/util"
-	"istio.io/istio/pkg/util/gogoprotomarshal"
-	"istio.io/pkg/log"
 )
 
 const (
@@ -217,13 +217,13 @@ func (c *PilotComponent) overlayMeshConfig(baseYAML string) (string, error) {
 			continue
 		}
 
-		meshOverride, err := gogoprotomarshal.ToYAML(c.CommonComponentFields.InstallSpec.MeshConfig)
+		meshOverride, err := yaml.Marshal(c.CommonComponentFields.InstallSpec.MeshConfig)
 		if err != nil {
 			return "", err
 		}
 
 		// Merge the MeshConfig yaml on top of whatever is in the configMap already
-		meshStr, err = util.OverlayYAML(meshStr, meshOverride)
+		meshStr, err = util.OverlayYAML(meshStr, string(meshOverride))
 		if err != nil {
 			return "", err
 		}

operator/pkg/controller/istiocontrolplane/istiocontrolplane_controller_test.go

@@ -28,8 +28,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
-	mesh "istio.io/api/mesh/v1alpha1"
 	"istio.io/api/operator/v1alpha1"
+
 	iop "istio.io/istio/operator/pkg/apis/istio/v1alpha1"
 	"istio.io/istio/operator/pkg/helmreconciler"
 	"istio.io/istio/operator/pkg/name"
@@ -112,8 +112,8 @@ func testSwitchProfile(t *testing.T, c testCase) {
 		Spec: &v1alpha1.IstioOperatorSpec{
 			Profile:            c.initialProfile,
 			InstallPackagePath: installPackagePath,
-			MeshConfig: &mesh.MeshConfig{
-				RootNamespace: "istio-system",
+			MeshConfig: map[string]interface{}{
+				"rootNamespace": "istio-system",
 			},
 		},
 	}

operator/pkg/validate/validate.go

@@ -18,15 +18,23 @@ import (
 	"fmt"
 	"reflect"
 
+	"github.com/ghodss/yaml"
+
 	"istio.io/api/operator/v1alpha1"
+
 	operator_v1alpha1 "istio.io/istio/operator/pkg/apis/istio/v1alpha1"
 	"istio.io/istio/operator/pkg/name"
 	"istio.io/istio/operator/pkg/util"
+	"istio.io/istio/pkg/config/mesh"
 )
 
 var (
 	// DefaultValidations maps a data path to a validation function.
 	DefaultValidations = map[string]ValidatorFunc{
+		"Values": func(path util.Path, i interface{}) util.Errors {
+			return CheckValues(i)
+		},
+		"MeshConfig":                         validateMeshConfig,
 		"Hub":                                validateHub,
 		"Tag":                                validateTag,
 		"AddonComponents":                    validateAddonComponents,
@@ -55,7 +63,6 @@ func CheckIstioOperatorSpec(is *v1alpha1.IstioOperatorSpec, checkRequiredFields
 		return util.Errors{}
 	}
 
-	errs = CheckValues(is.Values)
 	return util.AppendErrs(errs, Validate(DefaultValidations, is, nil, checkRequiredFields))
 }
 
@@ -154,6 +161,18 @@ func validateLeaf(validations map[string]ValidatorFunc, path util.Path, val inte
 	return vf(path, val)
 }
 
+func validateMeshConfig(path util.Path, root interface{}) util.Errors {
+	vs, err := yaml.Marshal(root)
+	if err != nil {
+		return util.Errors{err}
+	}
+	// This method will also perform validation automatically
+	if _, validErr := mesh.ApplyMeshConfigDefaults(string(vs)); validErr != nil {
+		return util.Errors{validErr}
+	}
+	return nil
+}
+
 func validateHub(path util.Path, val interface{}) util.Errors {
 	return validateWithRegex(path, val, ReferenceRegexp)
 }

operator/pkg/validate/validate_test.go

@@ -167,6 +167,26 @@ values:
   global:
     proxy:
       includeIPRanges: ""
+`,
+		},
+		{
+			desc: "Bad mesh config",
+			yamlStr: `
+meshConfig:
+  defaultConfig:
+    discoveryAddress: missingport
+`,
+			wantErrs: makeErrors([]string{`1 error occurred:
+	* invalid discovery address: unable to split "missingport": address missingport: missing port in address
+
+`}),
+		},
+		{
+			desc: "Good mesh config",
+			yamlStr: `
+meshConfig:
+  defaultConfig:
+    discoveryAddress: istiod:15012
 `,
 		},
 	}

operator/pkg/validate/validate_values.go

@@ -36,7 +36,7 @@ var (
 )
 
 // CheckValues validates the values in the given tree, which follows the Istio values.yaml schema.
-func CheckValues(root map[string]interface{}) util.Errors {
+func CheckValues(root interface{}) util.Errors {
 	vs, err := yaml.Marshal(root)
 	if err != nil {
 		return util.Errors{err}