Nested substitutions

cmd/config/internal/commands/cmdcreatesubstitution_test.go

@@ -184,6 +184,108 @@ spec:
         image: something/nginx::1.7.9/nginxotherthing # {"$openapi":"my-image-subst"}
       - name: sidecar
         image: sidecar:1.7.9
+ `,
+		},
+		{
+			name: "nested substitution",
+			args: []string{
+				"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
+metadata:
+  name: nginx-deployment
+spec:
+  replicas: 3
+  template:
+    spec:
+      containers:
+      - name: nginx
+        image: something/nginx::1.7.9/nginxotherthing
+      - name: sidecar
+        image: nginx::1.7.9 # {"$openapi":"my-image-subst"}
+ `,
+			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-image-setter}::${my-tag-setter}
+          values:
+          - marker: ${my-image-setter}
+            ref: '#/definitions/io.k8s.cli.setters.my-image-setter'
+          - marker: ${my-tag-setter}
+            ref: '#/definitions/io.k8s.cli.setters.my-tag-setter'
+ `,
+			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-image-setter}::${my-tag-setter}
+          values:
+          - marker: ${my-image-setter}
+            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:
+          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"}
  `,
 		},
 	}

cmd/config/internal/commands/cmdset_test.go

@@ -238,7 +238,6 @@ openAPI:
             ref: '#/definitions/io.k8s.cli.setters.image-setter'
           - marker: TAG
             ref: '#/definitions/io.k8s.cli.setters.tag'
-
  `,
 			expectedResources: `
 apiVersion: apps/v1
@@ -432,7 +431,6 @@ openAPI:
             ref: '#/definitions/io.k8s.cli.setters.image'
           - marker: TAG
             ref: '#/definitions/io.k8s.cli.setters.tag'
-
  `,
 			expectedResources: `
 apiVersion: apps/v1
@@ -702,6 +700,240 @@ spec:
 list in body must be of type integer: "boolean"
 list in body should have at most 2 items`,
 		},
+		{
+			name: "nested substitution",
+			args: []string{"my-image-setter", "ubuntu"},
+			out:  "set 2 fields\n",
+			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-image-setter}::${my-tag-setter}
+          values:
+          - marker: ${my-image-setter}
+            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:
+          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: ubuntu
+    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-image-setter}::${my-tag-setter}
+          values:
+          - marker: ${my-image-setter}
+            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:
+          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/ubuntu::1.7.9/nginxotherthing # {"$openapi":"my-nested-subst"}
+      - name: sidecar
+        image: ubuntu::1.7.9 # {"$openapi":"my-image-subst"}
+`,
+		},
+		{
+			name: "nested cyclic substitution",
+			args: []string{"my-image-setter", "ubuntu"},
+			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"}
+`,
+			errMsg: "cyclic substitution detected with name my-nested-subst",
+		},
 	}
 	for i := range tests {
 		test := tests[i]
@@ -778,4 +1010,4 @@ list in body should have at most 2 items`,
 			}
 		})
 	}
-}
+}

kyaml/setters2/set.go

@@ -97,7 +97,7 @@ func (s *Set) visitScalar(object *yaml.RNode, p string, schema *openapi.Resource
 	}
 
 	// perform a substitution of the field if it matches
-	sub, err := s.substitute(object, ext, schema.Schema)
+	sub, err := s.substitute(object, ext)
 	if err != nil {
 		return err
 	}
@@ -109,62 +109,29 @@ 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, _ *spec.Schema) (bool, error) {
+func (s *Set) substitute(field *yaml.RNode, ext *cliExtension) (bool, error) {
-	nameMatch := false
-
 	// check partial setters to see if they contain the setter as part of a
 	// substitution
 	if ext.Substitution == nil {
 		return false, nil
 	}
 
-	p := ext.Substitution.Pattern
+	// track the visited nodes to detect cycles in nested substitutions
+	visited := sets.String{}
 
-	// substitute each setter into the pattern to get the new value
+	nameMatch := false
-	for _, v := range ext.Substitution.Values {
-		if v.Ref == "" {
-			return false, errors.Errorf(
-				"missing reference on substitution " + ext.Substitution.Name)
-		}
-		ref, err := spec.NewRef(v.Ref)
-		if err != nil {
-			return false, errors.Wrap(err)
-		}
-		setter, err := openapi.Resolve(&ref) // resolve the setter to its openAPI def
-		if err != nil {
-			return false, errors.Wrap(err)
-		}
-		subSetter, err := getExtFromSchema(setter) // parse the extension out of the openAPI
-		if err != nil {
-			return false, errors.Wrap(err)
-		}
 
-		if err := validateAgainstSchema(subSetter, setter); err != nil {
+	res, err := s.substituteUtil(ext, &visited, &nameMatch)
-			return false, err
+	if err != nil {
-		}
+		return false, err
-
-		if val, found := subSetter.Setter.EnumValues[subSetter.Setter.Value]; found {
-			// the setter has an enum-map.  we should replace the marker with the
-			// enum value looked up from the map rather than the enum key
-			p = strings.ReplaceAll(p, v.Marker, val)
-		} else {
-			// substitute the setters current value into the substitution pattern
-			p = strings.ReplaceAll(p, v.Marker, subSetter.Setter.Value)
-		}
-
-		if s.isMatch(subSetter.Setter.Name) {
-			// the substitution depends on the specified setter
-			nameMatch = true
-		}
 	}
+
 	if !nameMatch {
 		// doesn't depend on the setter, don't modify its value
 		return false, nil
 	}
 
-	// TODO(pwittrock): validate the field value
+	field.YNode().Value = res
-
-	field.YNode().Value = p
 
 	// substitutions are always strings
 	field.YNode().Tag = yaml.StringTag
@@ -172,6 +139,72 @@ func (s *Set) substitute(field *yaml.RNode, ext *cliExtension, _ *spec.Schema) (
 	return true, nil
 }
 
+// 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) {
+	// 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)
+	pattern := ext.Substitution.Pattern
+
+	// 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 := 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
+			substVal, err := s.substituteUtil(defExt, visited, nameMatch)
+			if err != nil {
+				return "", err
+			}
+			pattern = strings.ReplaceAll(pattern, v.Marker, substVal)
+			continue
+		}
+
+		// if code reaches this point, this is a setter, so validate the setter schema
+		if err := validateAgainstSchema(defExt, def); err != nil {
+			return "", err
+		}
+
+		if s.isMatch(defExt.Setter.Name) {
+			// the substitution depends on the specified setter
+			*nameMatch = true
+		}
+
+		if val, found := defExt.Setter.EnumValues[defExt.Setter.Value]; found {
+			// the setter has an enum-map.  we should replace the marker with the
+			// enum value looked up from the map rather than the enum key
+			pattern = strings.ReplaceAll(pattern, v.Marker, val)
+		} else {
+			pattern = strings.ReplaceAll(pattern, v.Marker, defExt.Setter.Value)
+		}
+	}
+
+	return pattern, nil
+}
+
 // 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) {
 	// check full setter

kyaml/setters2/settersutil/substitutioncreator.go

@@ -77,7 +77,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
@@ -89,18 +89,34 @@ func (c SubstitutionCreator) CreateSettersForSubstitution(openAPIPath string) er
 	}
 
 	// for each ref in values, check if the setter already exists, if not create them
-	for _, value := range c.Values {
+	for i := range c.Values {
-		obj, err := y.Pipe(yaml.Lookup(
+		value := c.Values[i]
+		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
-			"openAPI", "definitions", strings.TrimPrefix(value.Ref, "#/definitions/")))
+			"openAPI", "definitions", strings.TrimPrefix(value.Ref, fieldmeta.DefinitionsPrefix)))
 
 		if err != nil {
 			return err
 		}
 
-		if obj == nil {
+		arr := strings.Split(value.Ref, ".")
-			name := strings.TrimPrefix(value.Ref, "#/definitions/io.k8s.cli.setters.")
+		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 {
+			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)
 			sd := setters2.SetterDefinition{