Check for cycles during create-subst

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},
 		)
 	}
 

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]

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

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) {

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)
 	}