From 714ff85806076ce44a58095237f04b5ff5cb8dcd Mon Sep 17 00:00:00 2001 From: Morten Torkildsen Date: Sat, 13 Jun 2020 14:07:05 -0700 Subject: [PATCH] Fix validation logic to use yaml parsing instead of json --- kyaml/setters2/set.go | 50 ++++++-------- kyaml/setters2/set_test.go | 130 ++++++++++++++++++++++++++++++++++++- 2 files changed, 147 insertions(+), 33 deletions(-) diff --git a/kyaml/setters2/set.go b/kyaml/setters2/set.go index 3ddd53017..de8d85e74 100644 --- a/kyaml/setters2/set.go +++ b/kyaml/setters2/set.go @@ -4,13 +4,14 @@ package setters2 import ( - "encoding/json" "fmt" "strings" + "text/template" "github.com/go-openapi/spec" "github.com/go-openapi/strfmt" "github.com/go-openapi/validate" + goyaml "gopkg.in/yaml.v3" "sigs.k8s.io/kustomize/kyaml/errors" "sigs.k8s.io/kustomize/kyaml/fieldmeta" "sigs.k8s.io/kustomize/kyaml/kio" @@ -241,24 +242,28 @@ func validateAgainstSchema(ext *CliExtension, sch *spec.Schema) error { sc.Properties = map[string]spec.Schema{} sc.Properties[ext.Setter.Name] = *sch - input := map[string]interface{}{} - var inputJSON string - + var inputYAML string if len(ext.Setter.ListValues) > 0 { - format := `{"%s" : [%s]}` - inputJSON = fmt.Sprintf(format, ext.Setter.Name, JoinCompositeStrings(ext.Setter.ListValues)) - } else { - format := `{"%s" : "%s"}` - if yaml.IsValueNonString(ext.Setter.Value) { - format = `{"%s" : %s}` + tmpl, err := template.New("validator"). + Parse(`{{.key}}:{{block "list" .values}}{{"\n"}}{{range .}}{{println "-" .}}{{end}}{{end}}`) + if err != nil { + return err } - inputJSON = fmt.Sprintf(format, ext.Setter.Name, ext.Setter.Value) + var builder strings.Builder + err = tmpl.Execute(&builder, map[string]interface{}{ + "key": ext.Setter.Name, + "values": ext.Setter.ListValues, + }) + if err != nil { + return err + } + inputYAML = builder.String() + } else { + inputYAML = fmt.Sprintf("%s: %s", ext.Setter.Name, ext.Setter.Value) } - // leverage json.Unmarshal to parse the value type - // Ex: `{"setter" : "true"}` parses the value as string whereas - // `{"setter" : true}` parses as boolean - err := json.Unmarshal([]byte(inputJSON), &input) + input := map[string]interface{}{} + err := goyaml.Unmarshal([]byte(inputYAML), &input) if err != nil { return err } @@ -269,21 +274,6 @@ func validateAgainstSchema(ext *CliExtension, sch *spec.Schema) error { return nil } -// JoinCompositeStrings takes in strings whose values can be of different types and returns -// joined string with quotes for only string type values -// ex: ["10", "true", "hi", "1.1"] returns 10,true,"hi",1.1 -func JoinCompositeStrings(listValues []string) string { - res := "" - for _, val := range listValues { - if yaml.IsValueNonString(val) { - res += fmt.Sprintf(`%s,`, val) - } else { - res += fmt.Sprintf(`"%s",`, val) - } - } - return strings.TrimSuffix(res, ",") -} - // SetOpenAPI updates a setter value type SetOpenAPI struct { // Name is the name of the setter to add diff --git a/kyaml/setters2/set_test.go b/kyaml/setters2/set_test.go index bd56b2127..8b75aefc0 100644 --- a/kyaml/setters2/set_test.go +++ b/kyaml/setters2/set_test.go @@ -8,6 +8,7 @@ import ( "strings" "testing" + "github.com/go-openapi/spec" "github.com/stretchr/testify/assert" "sigs.k8s.io/kustomize/kyaml/openapi" "sigs.k8s.io/kustomize/kyaml/yaml" @@ -1294,7 +1295,130 @@ openAPI: } } -func TestJoinCompositeStrings(t *testing.T) { - input := []string{"10", "true", "hi", "1.1", "1.8.1"} - assert.Equal(t, `10,true,"hi",1.1,"1.8.1"`, JoinCompositeStrings(input)) +func TestValidateAgainstSchema(t *testing.T) { + testCases := []struct { + name string + setter *setter + schema spec.SchemaProps + shouldValidate bool + expectedErrorMsg string + }{ + { + name: "simple string value", + setter: &setter{ + Name: "foo", + Value: "bar", + }, + schema: spec.SchemaProps{ + Type: []string{"string"}, + }, + shouldValidate: true, + }, + { + name: "simple bool value", + setter: &setter{ + Name: "foo", + Value: "false", + }, + schema: spec.SchemaProps{ + Type: []string{"boolean"}, + }, + shouldValidate: true, + }, + { + name: "simple null value", + setter: &setter{ + Name: "foo", + Value: "null", + }, + schema: spec.SchemaProps{ + Type: []string{"null"}, + }, + shouldValidate: true, + }, + { + name: "bool value in yaml but not openapi", + setter: &setter{ + Name: "foo", + Value: "yes", + }, + schema: spec.SchemaProps{ + Type: []string{"string"}, + }, + shouldValidate: true, + }, + { + name: "number value should not be accepted as string", + setter: &setter{ + Name: "foo", + Value: "45", + }, + schema: spec.SchemaProps{ + Type: []string{"integer"}, + }, + shouldValidate: true, + }, + { + name: "list with int values", + setter: &setter{ + Name: "foo", + ListValues: []string{"123", "456"}, + }, + schema: spec.SchemaProps{ + Type: []string{"array"}, + Items: &spec.SchemaOrArray{ + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Type: []string{"integer"}, + }, + }, + }, + }, + shouldValidate: true, + }, + { + name: "list expecting int values, but with a string", + setter: &setter{ + Name: "foo", + ListValues: []string{"123", "456", "abc"}, + }, + schema: spec.SchemaProps{ + Type: []string{"array"}, + Items: &spec.SchemaOrArray{ + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Type: []string{"integer"}, + }, + }, + }, + }, + shouldValidate: false, + expectedErrorMsg: "foo in body must be of type integer", + }, + } + + for i := range testCases { + test := testCases[i] + t.Run(test.name, func(t *testing.T) { + ext := &CliExtension{ + Setter: test.setter, + } + + schema := &spec.Schema{ + SchemaProps: test.schema, + } + + err := validateAgainstSchema(ext, schema) + + if test.shouldValidate { + assert.NoError(t, err) + return + } + + if !assert.Error(t, err) { + t.FailNow() + } + assert.Contains(t, err.Error(), test.expectedErrorMsg) + }) + } }