Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a ReplacementTransformer to replace the vars feature. #3492

Closed
monopole opened this issue Jan 20, 2021 · 11 comments
Closed

Introduce a ReplacementTransformer to replace the vars feature. #3492

monopole opened this issue Jan 20, 2021 · 11 comments
Assignees

Comments

@monopole
Copy link
Contributor

Let's replace the vars feature (see discussion in #2052), then deprecate vars.

Here's an untested, unused replacement transformer which could be used as raw material.

It refers to a replacement object, currently unused and mutable.

To close this issue, we'd need a new transformer plugin (any kind - go, executable, krm function). If go plugin, stick it under examples.

The plugin would accept a source (object and field spec) and, say, N targets (also object and field specs), then look for targets and replace whatever's found in those fields with whatever's found in the source.

  • New code should use kyaml (no apimachinery).
  • Unedited source material would be valid YAML that could be applied to a cluster (no dollar vars).
  • Type information should be retained, so if the source material is, say, an integer port number (e.g. an unquoted number like 8080) or a boolean (e.g. an unquoted true or false ), and has an RNode yaml tag indicating as much, then this type should be respected when written into the target (and not get quoted - see use of suspend: $(SUSPENDED) in [Question] Setting 'suspend' parameter in Argo Cron workflow based on overlay #3449)

A follow on issue (not filed at time of writing) would modify the kustomize fix command to examine someone's kustomization file and related files and convert any var use to use of a replacement transformer.

@monopole monopole changed the title Introduce a ReplacementTransformer as a replacement for the vars feature. Introduce a ReplacementTransformer to replacement the vars feature. Jan 20, 2021
@monopole
Copy link
Contributor Author

Yes. 90% of partial replacements are swapping out some clearly defined subelement of a path, URL domain spec, IP number or something that looks like one of those things.

For this, we just need an integer field in the source and target spec called positionInField, and maybe a single character field delimiter spec. A smart transformer would take that, try to split the field contents on the delimiter (likely / or .), do the swap, and then re-join and write into the target field. You can specify an index of -1 to get prefixing, or some large number to get postfixing.

The ValueAddTransformer works this way. One can change the nth element of a file path in any field through a simple mechanism (in the example, search for filePathPosition).

I'm not enthusiastic about offering general regexp replacement, as it's too unstructured - it will be used as a means to introduce $variables through the side door and thus miss the point of having a usable base.

@yanniszark
Copy link
Contributor

@monopole sorry, I moved my message to #2052 (comment) as I thought it made more sense there. But the email still reached you. This was my original comment:

@monopole this is great! I have to ask though, does this solution (replacement transformer) support partial substitution?
Partial substitution is one of the main uses of vars today. For example, to substitute a service name and namespace in a Certificate object: https://github.com/kubeflow/kfserving/blob/master/config/certmanager/certificate.yaml#L20

As for the solution described here:

For this, we just need an integer field in the source and target spec called positionInField, and maybe a single character field delimiter spec. A smart transformer would take that, try to split the field contents on the delimiter (likely / or .), do the swap, and then re-join and write into the target field. You can specify an index of -1 to get prefixing, or some large number to get postfixing.

Yes, this seems powerful enough to get the job done for the use-case I have in mind.

I'm not enthusiastic about offering general regexp replacement, as it's too unstructured - it will be used as a means to introduce $variables through the side door and thus miss the point of having a usable base.

How would that happen? With the replacement transformer as described, there is no need for any $(VAR) notation in the YAML.

@monopole
Copy link
Contributor Author

How would that happen?

Right, it intentionally cannot happen under the described behavior.

@aodinokov
Copy link
Contributor

aodinokov commented Jan 21, 2021

@monopole JFYI,
may be it will be interesting for you from features/use-cases perspective...
there are at least 2 replacement transformer implementations available in form of krm-functions:

  1. https://github.com/aodinokov/noctl-airship-poc/tree/master/kpt-functions/replacement
  2. https://github.com/airshipit/airshipctl/tree/master/krm-functions/replacement-transformer

New code should use kyaml (no apimachinery).

both based on kyaml, but they still may depend on apimachinery in some ways, e.g. for label filtering [1]
I think if it's krm-function there shouldn't be any dependencies issues anyway.

Speaking about different features like 'partial replacements':
The first one also supports yaml in yaml replacements (path is separated with "|" ) , e.g. [2]
partial replacements can be done on go-template level, e.g.
https://github.com/aodinokov/noctl-airship-poc/blob/master/kpt-functions/replacement/function_test.go#L525
and
https://github.com/aodinokov/noctl-airship-poc/blob/master/kpt-functions/replacement/function_test.go#L584
It was needed to introduce because partial replacements and replacements where you need to combine several fields in one string - that cases were pretty common.

The second one uses jsonpath to reference part of docs, which is pretty cool.

[1]
https://github.com/aodinokov/noctl-airship-poc/blob/master/kpt-functions/replacement/labelflt.go#L4
[2]
https://github.com/aodinokov/noctl-airship-poc/blob/master/kpt-functions/replacement/function_test.go#L466

@monopole monopole changed the title Introduce a ReplacementTransformer to replacement the vars feature. Introduce a ReplacementTransformer to replace the vars feature. Feb 2, 2021
@natasha41575 natasha41575 self-assigned this Feb 17, 2021
@natasha41575
Copy link
Contributor

natasha41575 commented Feb 17, 2021

Planning to pick this up after completion of kubernetes/enhancements#2206
If someone else wants to pick this up before I get to it, feel free.

@monopole
Copy link
Contributor Author

Considering the existing transformer and the two suggested by @aodinokov above,
and given the existing Selector, how about this for the transformer's config:

type Kustomization struct {
  ...
  Replacments []*Replacement
  ...
}

type Replacement struct {
  SourceRaw    string           // Raw value of replacement OR
  SourceField  *SelectedField   // a specific field to read it from.
  Targets      []*SelectedField // The N fields to write the value to.
}

// SelectedField specifies one field in one or more objects.
// Context determines if multiple objects are allowed.
type SelectedField struct {
  Allow     *Selector          // Allow objects that match this.
  Deny      *Selector          // From the allowed set, remove objects that match this.
  FieldPath string             // Structured field path expected in each allowed object.
  Options   *FieldOptions      // Options that refine interpretation of the field.
}

// FieldOptions lets one consider part of a string field rather than the whole field.
type FieldOptions struct {
  Delimiter  string            // Used to split/join the field.
  Index      int               // Which position in the split to consider.
}

The FieldOptions lets one pull part of a field and stick into a different part of another field.

Most of the time I've seen people use vars to modify a file path, either in a ConfigMap or in the command line sent to a container image.

E.g. people might organize cluster configuration data on disk in directories named to match cluster namespaces. So one might want to replace parts of file path in various places with the value found in the metadata/name field of a Namespace object.

@monopole
Copy link
Contributor Author

monopole commented Mar 22, 2021

  • If SourceField matches more than one object, it's an error that fails the build.
  • One could defer doing Deny and FieldOptions. They are just refinements on the basics of replacing a whole field.

@monopole
Copy link
Contributor Author

closed by #3737

@joebowbeer
Copy link
Contributor

@monopole @natasha41575

Was SourceRaw dropped? I don't see it in the tests but I also can't find if/when it was considered.

... how about this for the transformer's config:

type Kustomization struct {
  ...
  Replacments []*Replacement
  ...
}

type Replacement struct {
  SourceRaw    string           // Raw value of replacement OR
  SourceField  *SelectedField   // a specific field to read it from.
  Targets      []*SelectedField // The N fields to write the value to.
}

@Juzov
Copy link

Juzov commented Aug 3, 2023

Hi, I think it would be more useful if -1, didn't prepend, and would instead target the last element of the field, and -2 the next to last, and so forth.

Append, prepend would instead be options if that is wanted.

This is usually how programming languages work anyway.

Either this, or allow referencing the length.

@naveencloud
Copy link

Hi.., I can see the comment mentioned in the beginning of this issue that the suggestion to move the VARS to Replacements with option handle Integer Value.,
"Type information should be retained, so if the source material is, say, an integer port number (e.g. an unquoted number like 8080) or a boolean (e.g. an unquoted true or false ), and has an RNode yaml tag indicating as much, then this type should be respected when written into the target"

I'm stuck with replacement that while referring PORT from configMap it gives String instead of Integer but the POD definition expect Integer. Is there any workaround for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants