From 10250286c7798eefc3ca435fb28338f3902bf78a Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Sat, 30 May 2020 18:48:03 -0700 Subject: [PATCH] Check for cycles during create-subst --- .../commands/cmdcreatesubstitution.go | 23 ++- .../commands/cmdcreatesubstitution_test.go | 152 ++++++++++++++++-- kyaml/setters2/set.go | 15 +- .../settersutil/substitutioncreator.go | 121 +++++++++++--- kyaml/setters2/types.go | 12 +- 5 files changed, 270 insertions(+), 53 deletions(-) diff --git a/cmd/config/internal/commands/cmdcreatesubstitution.go b/cmd/config/internal/commands/cmdcreatesubstitution.go index e0a12ae24..46422ce5a 100644 --- a/cmd/config/internal/commands/cmdcreatesubstitution.go +++ b/cmd/config/internal/commands/cmdcreatesubstitution.go @@ -88,16 +88,31 @@ func (r *CreateSubstitutionRunner) preRunE(c *cobra.Command, args []string) erro re := regexp.MustCompile(`\$\{([^}]*)\}`) markers := re.FindAll([]byte(r.CreateSubstitution.Pattern), -1) if len(markers) == 0 { - return errors.Errorf("unable to find setter names in pattern, " + + return errors.Errorf("unable to find setter or substitution names in pattern, " + "setter names must be enclosed in ${}") } for _, marker := range markers { - ref := fieldmeta.DefinitionsPrefix + fieldmeta.SetterDefinitionPrefix + - strings.TrimSuffix(strings.TrimPrefix(string(marker), "${"), "}") + name := strings.TrimSuffix(strings.TrimPrefix(string(marker), "${"), "}") + + ref, err := spec.NewRef(fieldmeta.DefinitionsPrefix + fieldmeta.SubstitutionDefinitionPrefix + name) + if err != nil { + return err + } + + var markerRef string + subst, _ := openapi.Resolve(&ref) + // check if the substitution exists with the marker name or fall back to creating setter + // ref with the name + if subst != nil { + markerRef = fieldmeta.DefinitionsPrefix + fieldmeta.SubstitutionDefinitionPrefix + name + } else { + markerRef = fieldmeta.DefinitionsPrefix + fieldmeta.SetterDefinitionPrefix + name + } + r.CreateSubstitution.Values = append( r.CreateSubstitution.Values, - setters2.Value{Marker: string(marker), Ref: ref}, + setters2.Value{Marker: string(marker), Ref: markerRef}, ) } diff --git a/cmd/config/internal/commands/cmdcreatesubstitution_test.go b/cmd/config/internal/commands/cmdcreatesubstitution_test.go index e670d5bee..b10f41c92 100644 --- a/cmd/config/internal/commands/cmdcreatesubstitution_test.go +++ b/cmd/config/internal/commands/cmdcreatesubstitution_test.go @@ -149,16 +149,6 @@ apiVersion: v1alpha1 kind: Example openAPI: definitions: - io.k8s.cli.setters.my-image-setter: - x-k8s-cli: - setter: - name: my-image-setter - value: nginx - io.k8s.cli.setters.my-tag-setter: - x-k8s-cli: - setter: - name: my-tag-setter - value: 1.7.9 io.k8s.cli.substitutions.my-image-subst: x-k8s-cli: substitution: @@ -169,6 +159,16 @@ openAPI: ref: '#/definitions/io.k8s.cli.setters.my-image-setter' - marker: ${my-tag-setter} ref: '#/definitions/io.k8s.cli.setters.my-tag-setter' + io.k8s.cli.setters.my-image-setter: + x-k8s-cli: + setter: + name: my-image-setter + value: nginx + io.k8s.cli.setters.my-tag-setter: + x-k8s-cli: + setter: + name: my-tag-setter + value: 1.7.9 `, expectedResources: ` apiVersion: apps/v1 @@ -189,7 +189,8 @@ spec: { name: "nested substitution", args: []string{ - "my-nested-subst", "--field-value", "something/nginx::1.7.9/nginxotherthing", "--pattern", "something/${my-image-subst}/${my-other-setter}"}, + "my-nested-subst", "--field-value", "something/nginx::1.7.9/nginxotherthing", + "--pattern", "something/${my-image-subst}/${my-other-setter}"}, input: ` apiVersion: apps/v1 kind: Deployment @@ -256,11 +257,6 @@ openAPI: ref: '#/definitions/io.k8s.cli.setters.my-image-setter' - marker: ${my-tag-setter} ref: '#/definitions/io.k8s.cli.setters.my-tag-setter' - io.k8s.cli.setters.my-other-setter: - x-k8s-cli: - setter: - name: my-other-setter - value: nginxotherthing io.k8s.cli.substitutions.my-nested-subst: x-k8s-cli: substitution: @@ -271,6 +267,11 @@ openAPI: ref: '#/definitions/io.k8s.cli.substitutions.my-image-subst' - marker: ${my-other-setter} ref: '#/definitions/io.k8s.cli.setters.my-other-setter' + io.k8s.cli.setters.my-other-setter: + x-k8s-cli: + setter: + name: my-other-setter + value: nginxotherthing `, expectedResources: ` apiVersion: apps/v1 @@ -288,6 +289,125 @@ spec: image: nginx::1.7.9 # {"$openapi":"my-image-subst"} `, }, + { + name: "nested cyclic substitution", + args: []string{"my-nested-subst", "--field-value", "something/nginx::1.7.9/nginxotherthing", + "--pattern", "something/${my-image-subst}/${my-other-setter}"}, + inputOpenAPI: ` +apiVersion: v1alpha1 +kind: Example +openAPI: + definitions: + io.k8s.cli.setters.my-image-setter: + x-k8s-cli: + setter: + name: my-image-setter + value: nginx + io.k8s.cli.setters.my-tag-setter: + x-k8s-cli: + setter: + name: my-tag-setter + value: 1.7.9 + io.k8s.cli.substitutions.my-image-subst: + x-k8s-cli: + substitution: + name: my-image-subst + pattern: ${my-nested-subst}::${my-tag-setter} + values: + - marker: ${my-nested-subst} + ref: '#/definitions/io.k8s.cli.substitutions.my-nested-subst' + - marker: ${my-tag-setter} + ref: '#/definitions/io.k8s.cli.setters.my-tag-setter' + io.k8s.cli.setters.my-other-setter: + x-k8s-cli: + setter: + name: my-other-setter + value: nginxotherthing + io.k8s.cli.substitutions.my-nested-subst: + x-k8s-cli: + substitution: + name: my-nested-subst + pattern: something/${my-image-subst}/${my-other-setter} + values: + - marker: ${my-image-subst} + ref: '#/definitions/io.k8s.cli.substitutions.my-image-subst' + - marker: ${my-other-setter} + ref: '#/definitions/io.k8s.cli.setters.my-other-setter' + `, + input: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + replicas: 3 + template: + spec: + containers: + - name: nginx + image: something/nginx::1.7.9/nginxotherthing # {"$openapi":"my-nested-subst"} + - name: sidecar + image: nginx::1.7.9 # {"$openapi":"my-image-subst"} + `, + expectedOpenAPI: ` +apiVersion: v1alpha1 +kind: Example +openAPI: + definitions: + io.k8s.cli.setters.my-image-setter: + x-k8s-cli: + setter: + name: my-image-setter + value: nginx + io.k8s.cli.setters.my-tag-setter: + x-k8s-cli: + setter: + name: my-tag-setter + value: 1.7.9 + io.k8s.cli.substitutions.my-image-subst: + x-k8s-cli: + substitution: + name: my-image-subst + pattern: ${my-nested-subst}::${my-tag-setter} + values: + - marker: ${my-nested-subst} + ref: '#/definitions/io.k8s.cli.substitutions.my-nested-subst' + - marker: ${my-tag-setter} + ref: '#/definitions/io.k8s.cli.setters.my-tag-setter' + io.k8s.cli.setters.my-other-setter: + x-k8s-cli: + setter: + name: my-other-setter + value: nginxotherthing + io.k8s.cli.substitutions.my-nested-subst: + x-k8s-cli: + substitution: + name: my-nested-subst + pattern: something/${my-image-subst}/${my-other-setter} + values: + - marker: ${my-image-subst} + ref: '#/definitions/io.k8s.cli.substitutions.my-image-subst' + - marker: ${my-other-setter} + ref: '#/definitions/io.k8s.cli.setters.my-other-setter' + + `, + expectedResources: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + replicas: 3 + template: + spec: + containers: + - name: nginx + image: something/nginx::1.7.9/nginxotherthing # {"$openapi":"my-nested-subst"} + - name: sidecar + image: nginx::1.7.9 # {"$openapi":"my-image-subst"} +`, + err: "cyclic substitution detected with name my-nested-subst", + }, } for i := range tests { test := tests[i] diff --git a/kyaml/setters2/set.go b/kyaml/setters2/set.go index 6dae0bb5c..3ddd53017 100644 --- a/kyaml/setters2/set.go +++ b/kyaml/setters2/set.go @@ -109,7 +109,7 @@ func (s *Set) visitScalar(object *yaml.RNode, p string, schema *openapi.Resource // substitute updates the value of field from ext if ext contains a substitution that // depends on a setter whose name matches s.Name. -func (s *Set) substitute(field *yaml.RNode, ext *cliExtension) (bool, error) { +func (s *Set) substitute(field *yaml.RNode, ext *CliExtension) (bool, error) { // check partial setters to see if they contain the setter as part of a // substitution if ext.Substitution == nil { @@ -119,9 +119,12 @@ func (s *Set) substitute(field *yaml.RNode, ext *cliExtension) (bool, error) { // track the visited nodes to detect cycles in nested substitutions visited := sets.String{} + // nameMatch indicates if the input substitution depends on the specified setter, + // the substitution in ext is parsed recursively and if the setter in Set is hit while + // parsing, it indicates the match nameMatch := false - res, err := s.substituteUtil(ext, &visited, &nameMatch) + res, err := s.substituteUtil(ext, visited, &nameMatch) if err != nil { return false, err } @@ -141,7 +144,7 @@ func (s *Set) substitute(field *yaml.RNode, ext *cliExtension) (bool, error) { // substituteUtil recursively parses nested substitutions in ext and sets the setter value // returns error if cyclic substitution is detected or any other unexpected errors -func (s *Set) substituteUtil(ext *cliExtension, visited *sets.String, nameMatch *bool) (string, error) { +func (s *Set) substituteUtil(ext *CliExtension, visited sets.String, nameMatch *bool) (string, error) { // check if the substitution has already been visited and throw error as cycles // are not allowed in nested substitutions if visited.Has(ext.Substitution.Name) { @@ -168,7 +171,7 @@ func (s *Set) substituteUtil(ext *cliExtension, visited *sets.String, nameMatch if err != nil { return "", errors.Wrap(err) } - defExt, err := getExtFromSchema(def) // parse the extension out of the openAPI + defExt, err := GetExtFromSchema(def) // parse the extension out of the openAPI if err != nil { return "", errors.Wrap(err) } @@ -206,7 +209,7 @@ func (s *Set) substituteUtil(ext *cliExtension, visited *sets.String, nameMatch } // set applies the value from ext to field if its name matches s.Name -func (s *Set) set(field *yaml.RNode, ext *cliExtension, sch *spec.Schema) (bool, error) { +func (s *Set) set(field *yaml.RNode, ext *CliExtension, sch *spec.Schema) (bool, error) { // check full setter if ext.Setter == nil || !s.isMatch(ext.Setter.Name) { return false, nil @@ -233,7 +236,7 @@ func (s *Set) set(field *yaml.RNode, ext *cliExtension, sch *spec.Schema) (bool, // validateAgainstSchema validates the input setter value against user provided // openAI schema -func validateAgainstSchema(ext *cliExtension, sch *spec.Schema) error { +func validateAgainstSchema(ext *CliExtension, sch *spec.Schema) error { sc := spec.Schema{} sc.Properties = map[string]spec.Schema{} sc.Properties[ext.Setter.Name] = *sch diff --git a/kyaml/setters2/settersutil/substitutioncreator.go b/kyaml/setters2/settersutil/substitutioncreator.go index 3e8e9ec68..a48905ebd 100644 --- a/kyaml/setters2/settersutil/substitutioncreator.go +++ b/kyaml/setters2/settersutil/substitutioncreator.go @@ -5,12 +5,16 @@ package settersutil import ( "fmt" + "io/ioutil" + "os" "strings" + "github.com/go-openapi/spec" "sigs.k8s.io/kustomize/kyaml/errors" "sigs.k8s.io/kustomize/kyaml/fieldmeta" "sigs.k8s.io/kustomize/kyaml/kio" "sigs.k8s.io/kustomize/kyaml/openapi" + "sigs.k8s.io/kustomize/kyaml/sets" "sigs.k8s.io/kustomize/kyaml/setters2" "sigs.k8s.io/kustomize/kyaml/yaml" ) @@ -47,7 +51,15 @@ func (c SubstitutionCreator) Create(openAPIPath, resourcesPath string) error { Pattern: c.Pattern, } - err := c.CreateSettersForSubstitution(openAPIPath) + // the input substitution definition is updated in the openAPI file and then parsed + // to check if there are any cycles in nested substitutions, if there are + // any, the openAPI file will be reverted to current state and error is thrown + stat, err := os.Stat(openAPIPath) + if err != nil { + return err + } + + curOpenAPI, err := ioutil.ReadFile(openAPIPath) if err != nil { return err } @@ -61,6 +73,40 @@ func (c SubstitutionCreator) Create(openAPIPath, resourcesPath string) error { return err } + visited := sets.String{} + ref, err := spec.NewRef(fieldmeta.DefinitionsPrefix + fieldmeta.SubstitutionDefinitionPrefix + c.Name) + if err != nil { + return err + } + + schema, err := openapi.Resolve(&ref) + if err != nil { + return err + } + + ext, err := setters2.GetExtFromSchema(schema) + if err != nil { + return err + } + + err = c.CreateSettersForSubstitution(openAPIPath) + if err != nil { + return err + } + + // Load the updated definitions after setters are created + if err := openapi.AddSchemaFromFile(openAPIPath); err != nil { + return err + } + + // revert openAPI file if there are cycles detected in created input substitution + if err := checkForCycles(ext, visited); err != nil { + if writeErr := ioutil.WriteFile(openAPIPath, curOpenAPI, stat.Mode().Perm()); writeErr != nil { + return writeErr + } + return err + } + // Update the resources with the setter reference inout := &kio.LocalPackageReadWriter{PackagePath: resourcesPath} return kio.Pipeline{ @@ -77,7 +123,7 @@ func (c SubstitutionCreator) Create(openAPIPath, resourcesPath string) error { // CreateSettersForSubstitution creates the setters for all the references in the substitution // values if they don't already exist in openAPIPath file. -func (c *SubstitutionCreator) CreateSettersForSubstitution(openAPIPath string) error { +func (c SubstitutionCreator) CreateSettersForSubstitution(openAPIPath string) error { y, err := yaml.ReadFile(openAPIPath) if err != nil { return err @@ -88,9 +134,14 @@ func (c *SubstitutionCreator) CreateSettersForSubstitution(openAPIPath string) e return err } - // for each ref in values, check if the setter already exists, if not create them - for i := range c.Values { - value := c.Values[i] + // for each ref in values, check if the setter or substitution already exists, if not create setter + for _, value := range c.Values { + // continue if ref is a substitution, as it has already been checked if it exists + // as part of preRunE + if strings.Contains(value.Ref, fieldmeta.SubstitutionDefinitionPrefix) { + fmt.Printf("found a substitution with name %s\n", value.Marker) + continue + } setterObj, err := y.Pipe(yaml.Lookup( // get the setter key from ref. Ex: from #/definitions/io.k8s.cli.setters.image_setter // extract io.k8s.cli.setters.image_setter @@ -100,22 +151,7 @@ func (c *SubstitutionCreator) CreateSettersForSubstitution(openAPIPath string) e return err } - arr := strings.Split(value.Ref, ".") - name := arr[len(arr)-1] - // lookup if there is a substitution with name as it could be a nested substitution - substRef := fieldmeta.SubstitutionDefinitionPrefix + name - substObj, err := y.Pipe(yaml.Lookup("openAPI", "definitions", substRef)) - - if err != nil { - return err - } - - if setterObj == nil && substObj != nil { - fmt.Printf("found a substitution with name %s\n", name) - c.Values[i].Ref = fieldmeta.DefinitionsPrefix + substRef - } - - if setterObj == nil && substObj == nil { + if setterObj == nil { name := strings.TrimPrefix(value.Ref, fieldmeta.DefinitionsPrefix+fieldmeta.SetterDefinitionPrefix) value := m[value.Marker] fmt.Printf("unable to find setter with name %s, creating new setter with value %s\n", name, value) @@ -134,6 +170,49 @@ func (c *SubstitutionCreator) CreateSettersForSubstitution(openAPIPath string) e return nil } +func checkForCycles(ext *setters2.CliExtension, visited sets.String) error { + // check if the substitution has already been visited and throw error as cycles + // are not allowed in nested substitutions + if visited.Has(ext.Substitution.Name) { + return errors.Errorf( + "cyclic substitution detected with name " + ext.Substitution.Name) + } + + visited.Insert(ext.Substitution.Name) + + // substitute each setter into the pattern to get the new value + // if substitution references to another substitution, recursively + // process the nested substitutions to replace the pattern with setter values + for _, v := range ext.Substitution.Values { + if v.Ref == "" { + return errors.Errorf( + "missing reference on substitution " + ext.Substitution.Name) + } + ref, err := spec.NewRef(v.Ref) + if err != nil { + return errors.Wrap(err) + } + def, err := openapi.Resolve(&ref) // resolve the def to its openAPI def + if err != nil { + return errors.Wrap(err) + } + defExt, err := setters2.GetExtFromSchema(def) // parse the extension out of the openAPI + if err != nil { + return errors.Wrap(err) + } + + if defExt.Substitution != nil { + // parse recursively if it reference is substitution + err := checkForCycles(defExt, visited) + if err != nil { + return err + } + } + } + + return nil +} + // GetValuesForMarkers parses the pattern and field value to derive values for the // markers in the pattern string. Returns error if the marker values can't be derived func (c SubstitutionCreator) GetValuesForMarkers() (map[string]string, error) { diff --git a/kyaml/setters2/types.go b/kyaml/setters2/types.go index 5b489bcb1..5c1bc1407 100644 --- a/kyaml/setters2/types.go +++ b/kyaml/setters2/types.go @@ -11,7 +11,7 @@ import ( "sigs.k8s.io/kustomize/kyaml/openapi" ) -type cliExtension struct { +type CliExtension struct { Setter *setter `yaml:"setter,omitempty" json:"setter,omitempty"` Substitution *substitution `yaml:"substitution,omitempty" json:"substitution,omitempty"` } @@ -37,8 +37,8 @@ type substitutionSetterReference struct { //K8sCliExtensionKey is the name of the OpenAPI field containing the setter extensions const K8sCliExtensionKey = "x-k8s-cli" -// getExtFromSchema returns the cliExtension openAPI extension if it is present in schema -func getExtFromSchema(schema *spec.Schema) (*cliExtension, error) { +// GetExtFromSchema returns the cliExtension openAPI extension if it is present in schema +func GetExtFromSchema(schema *spec.Schema) (*CliExtension, error) { cep := schema.VendorExtensible.Extensions[K8sCliExtensionKey] if cep == nil { return nil, nil @@ -47,7 +47,7 @@ func getExtFromSchema(schema *spec.Schema) (*cliExtension, error) { if err != nil { return nil, err } - val := &cliExtension{} + val := &CliExtension{} if err := json.Unmarshal(b, val); err != nil { return nil, err } @@ -56,7 +56,7 @@ func getExtFromSchema(schema *spec.Schema) (*cliExtension, error) { // getExtFromComment returns the cliExtension openAPI extension if it is present as // a comment on the field. -func getExtFromComment(schema *openapi.ResourceSchema) (*cliExtension, error) { +func getExtFromComment(schema *openapi.ResourceSchema) (*CliExtension, error) { if schema == nil { // no schema found // TODO(pwittrock): should this be an error if it doesn't resolve? @@ -64,7 +64,7 @@ func getExtFromComment(schema *openapi.ResourceSchema) (*cliExtension, error) { } // get the cli extension from the openapi (contains setter information) - ext, err := getExtFromSchema(schema.Schema) + ext, err := GetExtFromSchema(schema.Schema) if err != nil { return nil, errors.Wrap(err) }