Add values validation (istio#227)

* Initial commit of values validation

* Initial commit of values validation

* Fix validation

* Add profile check, update function name

* Address comment

cmd/mesh/profile-diff.go

@@ -41,13 +41,13 @@ func profileDiffCmd(rootArgs *rootArgs) *cobra.Command {
 func profileDiff(rootArgs *rootArgs, args []string) {
 	initLogsOrExit(rootArgs)
 
-	a, err := helm.ReadValuesYAML(args[0])
+	a, err := helm.ReadProfileYAML(args[0])
 	if err != nil {
 		log.Errorf("could not read the profile values from %s: %s", args[0], err)
 		os.Exit(1)
 	}
 
-	b, err := helm.ReadValuesYAML(args[1])
+	b, err := helm.ReadProfileYAML(args[1])
 	if err != nil {
 		log.Errorf("could not read the profile values from %s: %s", args[1], err)
 		os.Exit(1)

cmd/mesh/profile-dump.go

@@ -97,7 +97,7 @@ func genProfile(helmValues bool, inFilename, profile, setOverlayYAML, configPath
 	}
 
 	// This contains the IstioControlPlane CR.
-	baseCRYAML, err := helm.ReadValuesYAML(profile)
+	baseCRYAML, err := helm.ReadProfileYAML(profile)
 	if err != nil {
 		return "", fmt.Errorf("could not read the profile values for %s: %s", profile, err)
 	}
@@ -108,7 +108,7 @@ func genProfile(helmValues bool, inFilename, profile, setOverlayYAML, configPath
 		if err != nil {
 			return "", err
 		}
-		defaultYAML, err := helm.ReadValuesYAML(dfn)
+		defaultYAML, err := helm.ReadProfileYAML(dfn)
 		if err != nil {
 			return "", fmt.Errorf("could not read the default profile values for %s: %s", dfn, err)
 		}

pkg/helm/helm.go

@@ -64,15 +64,15 @@ func NewHelmRenderer(chartsRootDir, helmBaseDir, componentName, namespace string
 	}
 }
 
-// ReadValuesYAML reads the values YAML associated with the given profile. It uses an appropriate reader for the
+// ReadProfileYAML reads the YAML values associated with the given profile. It uses an appropriate reader for the
 // profile format (compiled-in, file, HTTP, etc.).
-func ReadValuesYAML(profile string) (string, error) {
+func ReadProfileYAML(profile string) (string, error) {
 	var err error
 	var globalValues string
 	if profile == "" {
-		log.Infof("ReadValuesYAML for profile name: [Empty]")
+		log.Infof("ReadProfileYAML for profile name: [Empty]")
 	} else {
-		log.Infof("ReadValuesYAML for profile name: %s", profile)
+		log.Infof("ReadProfileYAML for profile name: %s", profile)
 	}
 
 	// Get global values from profile.

pkg/manifest/installer.go

@@ -133,7 +133,7 @@ func init() {
 
 }
 
-// ParseK8SYAMLToIstioControlPlaneSpec parses a YAML string IstioControlPlane CustomResource and unmarshals in into
+// ParseK8SYAMLToIstioControlPlaneSpec parses a IstioControlPlane CustomResource YAML string and unmarshals in into
 // an IstioControlPlaneSpec object. It returns the object and an API group/version with it.
 func ParseK8SYAMLToIstioControlPlaneSpec(yml string) (*v1alpha2.IstioControlPlaneSpec, *schema.GroupVersionKind, error) {
 	o, err := object.ParseYAMLToK8sObject([]byte(yml))

pkg/validate/common.go

@@ -126,6 +126,9 @@ func validatePortNumberString(path util.Path, val interface{}) util.Errors {
 	if !util.IsString(val) {
 		return util.NewErrs(fmt.Errorf("validatePortNumberString(%s) bad type %T, want string", path, val))
 	}
+	if val.(string) == "*" || val.(string) == "" {
+		return nil
+	}
 	intV, err := strconv.ParseInt(val.(string), 10, 32)
 	if err != nil {
 		return util.NewErrs(fmt.Errorf("%s : %s", path, err))
@@ -148,7 +151,7 @@ func validateIPRangesOrStar(path util.Path, val interface{}) (errs util.Errors)
 		return util.NewErrs(err)
 	}
 
-	if val.(string) == "*" {
+	if val.(string) == "*" || val.(string) == "" {
 		return errs
 	}
 

pkg/validate/validate.go

@@ -32,7 +32,6 @@ var (
 		"CustomPackagePath": validateInstallPackagePath,
 		"DefaultNamespace":  validateDefaultNamespace,
 	}
-
 	// requiredValues lists all the values that must be non-empty.
 	requiredValues = map[string]bool{
 		"DefaultNamespace": true,
@@ -41,8 +40,9 @@ var (
 
 // CheckIstioControlPlaneSpec validates the values in the given Installer spec, using the field map defaultValidations to
 // call the appropriate validation function.
-func CheckIstioControlPlaneSpec(is *v1alpha2.IstioControlPlaneSpec, checkRequired bool) util.Errors {
-	return validate(defaultValidations, is, nil, checkRequired)
+func CheckIstioControlPlaneSpec(is *v1alpha2.IstioControlPlaneSpec, checkRequired bool) (errs util.Errors) {
+	errs = util.AppendErrs(errs, CheckValues(is.Values))
+	return util.AppendErrs(errs, validate(defaultValidations, is, nil, checkRequired))
 }
 
 func validate(validations map[string]ValidatorFunc, structPtr interface{}, path util.Path, checkRequired bool) (errs util.Errors) {

pkg/validate/validate_test.go

@@ -15,13 +15,9 @@
 package validate
 
 import (
-	"bytes"
 	"strings"
 	"testing"
 
-	"github.com/ghodss/yaml"
-	"github.com/gogo/protobuf/jsonpb"
-	"github.com/gogo/protobuf/proto"
 	"github.com/kylelemons/godebug/diff"
 
 	"istio.io/operator/pkg/apis/istio/v1alpha2"
@@ -134,11 +130,11 @@ affinity:
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
 			tk := &v1alpha2.TestKube{}
-			err := unmarshalWithJSONPB(tt.yamlStr, tk)
+			err := util.UnmarshalWithJSONPB(tt.yamlStr, tk)
 			if err != nil {
 				t.Fatalf("unmarshalWithJSONPB(%s): got error %s", tt.desc, err)
 			}
-			s, err := marshalWithJSONPB(tk)
+			s, err := util.MarshalWithJSONPB(tk)
 			if err != nil {
 				t.Fatalf("unmarshalWithJSONPB(%s): got error %s", tt.desc, err)
 			}
@@ -222,6 +218,11 @@ trafficManagement:
                 targetAverageUtilization: 80
         nodeSelector:
           disktype: ssd
+values:
+  global:
+    proxy:
+      includeIpRanges: "1.1.0.0/16,2.2.0.0/16"
+      excludeIpRanges: "3.3.0.0/16,4.4.0.0/16"
 
 `,
 		},
@@ -243,14 +244,37 @@ hub: docker.io:tag/istio
 			desc: "GoodURL",
 			yamlStr: `
 installPackagePath: /local/file/path
+`,
+		},
+		{
+			desc: "BadValuesIP",
+			yamlStr: `
+values:
+  global:
+    proxy:
+      includeIpRanges: "1.1.0.300/16,2.2.0.0/16"
+`,
+			wantErrs: makeErrors([]string{`global.proxy.includeIpRanges invalid CIDR address: 1.1.0.300/16`}),
+		},
+		{
+			desc: "EmptyValuesIP",
+			yamlStr: `
+values:
+  global:
+    proxy:
+      includeIpRanges: ""
+      includeInboundPorts: "*"
 `,
 		},
 	}
 
 	for _, tt := range tests {
+		if tt.desc != "EmptyValuesIP" {
+			continue
+		}
 		t.Run(tt.desc, func(t *testing.T) {
 			ispec := &v1alpha2.IstioControlPlaneSpec{}
-			err := unmarshalWithJSONPB(tt.yamlStr, ispec)
+			err := util.UnmarshalWithJSONPB(tt.yamlStr, ispec)
 			if err != nil {
 				t.Fatalf("unmarshalWithJSONPB(%s): got error %s", tt.desc, err)
 			}
@@ -262,33 +286,6 @@ installPackagePath: /local/file/path
 	}
 }
 
-func unmarshalWithJSONPB(y string, out proto.Message) error {
-	jb, err := yaml.YAMLToJSON([]byte(y))
-	if err != nil {
-		return err
-	}
-
-	u := jsonpb.Unmarshaler{}
-	err = u.Unmarshal(bytes.NewReader(jb), out)
-	if err != nil {
-		return err
-	}
-	return nil
-}
-
-func marshalWithJSONPB(in proto.Message) (string, error) {
-	m := jsonpb.Marshaler{}
-	js, err := m.MarshalToString(in)
-	if err != nil {
-		return "", err
-	}
-	yb, err := yaml.JSONToYAML([]byte(js))
-	if err != nil {
-		return "", err
-	}
-	return string(yb), nil
-}
-
 func stripNL(s string) string {
 	return strings.Trim(s, "\n")
 }

pkg/validate/validate_values.go

@@ -15,6 +15,9 @@
 package validate
 
 import (
+	"github.com/ghodss/yaml"
+
+	"istio.io/operator/pkg/apis/istio/v1alpha2"
 	"istio.io/operator/pkg/util"
 )
 
@@ -30,6 +33,14 @@ var (
 
 // CheckValues validates the values in the given tree, which follows the Istio values.yaml schema.
 func CheckValues(root map[string]interface{}) util.Errors {
+	vs, err := yaml.Marshal(root)
+	if err != nil {
+		return util.Errors{err}
+	}
+	val := &v1alpha2.Values{}
+	if err := yaml.Unmarshal(vs, val); err != nil {
+		return util.Errors{err}
+	}
 	return validateValues(defaultValuesValidations, root, nil)
 }
 

pkg/validate/validate_values_test.go

@@ -20,6 +20,8 @@ import (
 
 	"github.com/ghodss/yaml"
 
+	"istio.io/operator/pkg/helm"
+	"istio.io/operator/pkg/manifest"
 	"istio.io/operator/pkg/util"
 )
 
@@ -129,6 +131,46 @@ global:
 	}
 }
 
+func TestValidateValuesFromProfile(t *testing.T) {
+	tests := []struct {
+		desc     string
+		profile  string
+		wantErrs util.Errors
+	}{
+		{
+			profile: "default",
+		},
+		{
+			profile: "demo",
+		},
+		{
+			profile: "demo-auth",
+		},
+		{
+			profile: "minimal",
+		},
+		{
+			profile: "sds",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			pf, err := helm.ReadProfileYAML(tt.profile)
+			if err != nil {
+				t.Fatalf("fail to read profile: %s", tt.profile)
+			}
+			val, _, err := manifest.ParseK8SYAMLToIstioControlPlaneSpec(pf)
+			if err != nil {
+				t.Fatalf(" fail to parse profile to ISCP: (%s), got error %s", tt.profile, err)
+			}
+			errs := CheckValues(val.Values)
+			if gotErr, wantErr := errs, tt.wantErrs; !util.EqualErrors(gotErr, wantErr) {
+				t.Errorf("CheckValues of (%v): gotErr:%s, wantErr:%s", tt.profile, gotErr, wantErr)
+			}
+		})
+	}
+}
+
 func makeErrors(estr []string) util.Errors {
 	var errs util.Errors
 	for _, s := range estr {