From a43d43f2c48d430e2ca0eb3d5a089e82aaef84f0 Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Tue, 26 May 2020 13:25:21 -0700 Subject: [PATCH] Nested substitutions --- .../commands/cmdcreatesubstitution_test.go | 102 ++++++++ cmd/config/internal/commands/cmdset_test.go | 238 +++++++++++++++++- kyaml/setters2/set.go | 119 +++++---- .../settersutil/substitutioncreator.go | 28 ++- 4 files changed, 435 insertions(+), 52 deletions(-) diff --git a/cmd/config/internal/commands/cmdcreatesubstitution_test.go b/cmd/config/internal/commands/cmdcreatesubstitution_test.go index 505afbf39..e670d5bee 100644 --- a/cmd/config/internal/commands/cmdcreatesubstitution_test.go +++ b/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"} `, }, } diff --git a/cmd/config/internal/commands/cmdset_test.go b/cmd/config/internal/commands/cmdset_test.go index 427a0c557..38f64df4a 100644 --- a/cmd/config/internal/commands/cmdset_test.go +++ b/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`, } }) } -} +} \ No newline at end of file diff --git a/kyaml/setters2/set.go b/kyaml/setters2/set.go index 39a63a5a5..6dae0bb5c 100644 --- a/kyaml/setters2/set.go +++ b/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) { - nameMatch := false - +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 { 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 - 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) - } + nameMatch := false - if err := validateAgainstSchema(subSetter, setter); 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 - } + res, err := s.substituteUtil(ext, &visited, &nameMatch) + if err != nil { + return false, err } + 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 = p + field.YNode().Value = res // 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 diff --git a/kyaml/setters2/settersutil/substitutioncreator.go b/kyaml/setters2/settersutil/substitutioncreator.go index e654c7037..3e8e9ec68 100644 --- a/kyaml/setters2/settersutil/substitutioncreator.go +++ b/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 { - obj, err := y.Pipe(yaml.Lookup( + for i := range c.Values { + 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 { - name := strings.TrimPrefix(value.Ref, "#/definitions/io.k8s.cli.setters.") + 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 { + 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{