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 505afbf39..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 @@ -186,6 +186,228 @@ spec: 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.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' + io.k8s.cli.setters.my-other-setter: + x-k8s-cli: + setter: + name: my-other-setter + value: nginxotherthing + `, + 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"} + `, + }, + { + 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/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..3ddd53017 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,32 @@ 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 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 - 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,8 +142,74 @@ 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) { +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 @@ -200,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 e654c7037..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{ @@ -88,19 +134,25 @@ func (c SubstitutionCreator) CreateSettersForSubstitution(openAPIPath string) er return err } - // for each ref in values, check if the setter already exists, if not create them + // for each ref in values, check if the setter or substitution already exists, if not create setter for _, value := range c.Values { - obj, err := y.Pipe(yaml.Lookup( + // 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 - "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.") + 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) sd := setters2.SetterDefinition{ @@ -118,6 +170,49 @@ func (c SubstitutionCreator) CreateSettersForSubstitution(openAPIPath string) er 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) }