feat: skip name prefix/suffix by kind

[sethp-nr]

Introduces a new config field, namePrefixSuffixSkip, that takes a list
of gvk arguments. For example:

namePrefixSuffixSkip:
- apiVersion: storage.k8s.io/v1
  kind: StorageClass

For any matching group/version and kind, kustomize will preserve the
original resource name even if namePrefix and nameSuffix are
defined.

Fixes #519

[k8s-ci-robot]

Welcome @sethp-nr!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sethp-nr
To complete the pull request process, please assign pwittrock
You can assign the PR to them by writing /assign @pwittrock in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sethp-nr]

I'm not sure if this was intentionally omitted or not, but I noticed it was absent as part of this change. Happy to pull this into a separate PR if you'd rather.

[richardmarshall]

When the sorting was added the name suffix transformation hadn't been implemented yet. Almost certain it being missing here was just an oversight when the suffix support was added later.

[richardmarshall]

This will be great to have thanks for working on it!

In my opinion using the fsSlice type instead of adding a new gvkSlice would make this simpler. The fieldSpec type already embeds gvk.Gvk and could be used for this config as well. The path and create fields may be unnecessary for this case but doesn't seem like enough of a problem to need a more specialized config type just for this transformer.

[richardmarshall]

When the sorting was added the name suffix transformation hadn't been implemented yet. Almost certain it being missing here was just an oversight when the suffix support was added later.

[sethp-nr]

Thanks for the feedback @richardmarshall! I went with a new type because I couldn't see a way to have the path and create fields make sense, and so someone might find themself in the position where they got an error about being unable to merge differing create fields despite that field having no effect.

That said, it is quite a narrow possibility. I'll remove the gvkSlice and switch over to fsSlice in the morning.

[jbrette]

@sethp-nr @richardmarshall @Liujingfang1 @monopole The current PR seems to introduce a special case for suffix/prefix for an issue which much more global as described here: #896. We will have a to double the number of fields in the transformation config for all the other

If one the otherside we add a "Skip" field to FieldSpec

type FieldSpec struct {
        gvk.Gvk            `json:",inline,omitempty" yaml:",inline,omitempty"`
        Path               string `json:"path,omitempty" yaml:"path,omitempty"`
        CreateIfNotPresent bool   `json:"create,omitempty" yaml:"create,omitempty"`
        Skip bool   `json:"skip,omitempty" yaml:"skip,omitempty"`
}

we would be able to we would be able to:

• use the new option field (default value false) for suffix/prefix
• use for the other transformers configurations
• not change the syntax of the kustomization.yaml.

[richardmarshall]

@jbrette Ah, good call out for the general case. Had a can't see forrest for the trees moment there.

That said I'm conflicted on the idea of combining general field specs with the set to skip. I definitely see the Skip field in FieldSpec working but have a couple concerns about the approach.

Primarily it would make the case where a user wants to skip all transformers for a specific FieldSpec challenging. They would need to add an entry to every transformers configuration that might end up touching what they would like to be ignored. Also in the future if we add additional transformers they would then need to update their configuration to account for it as well or potentially face unexpected side effects.

An alternative would be to define a new configuration key for the sole purpose of defining what should be skipped by transformers.

type SkipSpec struct {
        FieldSpec             `json:",inline,omitempty" yaml:",inline,omitempty"`
        Transformers []string `json:"transformers,omitempty" yaml:"transformers,omitempty"`
}

Where the behavior of the Transformers slice would be if empty skip all transformers similarly to how the gvk fields in FieldSpec act as wildcard for selection when not provided. This would enable a user to define the skip behavior once for all current or future builtin transformers.

The default transformer config for this to account for the current skipping for CustomResourceDefinitions in the name prefixsuffix transformer could then be something along the lines of:

skipTransformation:
- kind: CustomResourceDefinitions
  transformers:
  - prefixsuffix

This pattern would also make adding additional skipping behavior not require adjustments to the FieldSpec type. An example being being able to skip based on the name of an object.

[Liujingfang1]

Thank you for putting thoughts on this. I agree being able to skip is something we should consider to support.

If one the otherside we add a "Skip" field to FieldSpec

type FieldSpec struct {
        gvk.Gvk            `json:",inline,omitempty" yaml:",inline,omitempty"`
        Path               string `json:"path,omitempty" yaml:"path,omitempty"`
       CreateIfNotPresent bool   `json:"create,omitempty" yaml:"create,omitempty"`
       Skip bool   `json:"skip,omitempty" yaml:"skip,omitempty"`
}

we would be able to we would be able to:

use the new option field (default value false) for suffix/prefix
use for the other transformers configurations
not change the syntax of the kustomization.yaml.

I did consider add the Skip field previously, it has the strong points as you listed. It can solve some certain skips but not all. If you consider skipping nameprefix, suffix or namespace by this format, it's not as easy as you expected.

type SkipSpec struct {
        FieldSpec             `json:",inline,omitempty" yaml:",inline,omitempty"`
        Transformers []string `json:"transformers,omitempty" yaml:"transformers,omitempty"`
}

For this one, I don't think we need to add an extra field in kustomization.yaml to support this. The skipping should be part of the transformation config.

I'd like the solutions to be generic, backward compatible with previous transformer configs and easy to solve all kinds of Skips. Considering the various transformers and the type of skipping users may want them to do, there are several kinds of Skips:

• Skip a transformer which only mutates one field of the resource, such as skipping prefix/suffix by kind
• Skip a transformer which mutates multiple fields of the resource
• Skip a transformer for a subset of mutated fields while not skip the rest subset. For example, skip adding labels to label selector field, but not label field.

How about we keep using current FieldSpec, but add a skip field spec to each transformer.

type ATransformer struct {
  FieldSpecs []config.FieldSpec
  SkipFieldSpecs []config.FieldSpec
}

The configuration file for transformers can be

skipCommonLabels:
  - path: spec/selector
     kind: Service
skipNamePrefix:
  - kind: CustomResourceDefinition
  - kind: MyKind
skipNameSpace:
  - kind: MyKind
  - kind: ClusterLeveledCRDs 

[jbrette]

@Liujingfang1 @richardmarshall @sethp-nr @monopole I started to prototype an idea based on patch to change the configurations of transformer themselves.

Indeed sometimes a user wants add to transformer config, sometime he wants to add entries, sometimes he wants to completely replace the list of entries. This is basically the JSON patch operations that kustomize can already apply on a resmap. In on word, if the TransformerConfig object could implement the ResMap api, this would address a lot of those issue because the user could change the configuration of transformer by adding apply a JSONPatch to it (The current JSONPath operation is add).

Without going this far (aka TransfomerConfig implements ResMap API), I started with adding a JSONOp field to the FieldSpec instead of simple Skip boolean: See here

Even for the simple skip boolean, the hardest part of the implementation is detecting conflicts between skip and add: The code has to be able to understand what is the intersection of the fieldspec between the add and the skip.

For instance if you have:

applytransformation: fieldspec:metadata/namespace
skiptransformation:    fieldspec:metadata/namespace and gvk:(mycrd, groupa, version22).

it means that the transformer has to be able to understand that it needs to apply the transformation to mycrd,groupa,version10 but not to mycrd,groupa,version22.

[Liujingfang1]

@Liujingfang1 @richardmarshall @sethp-nr @monopole I started to prototype an idea based on patch to change the configurations of transformer themselves.

What is the purpose of that? The custom configurations of transformers should from the same set of files. Users can directly edit those files, they don't need to patch them.

**Indeed sometimes a user wants add to transformer config, sometime he wants to add entries, sometimes he wants to completely replace the list of entries. This is basically the JSON patch operations that kustomize can already apply on a resmap.

This is already supported in current transformer configuration that you can specify in kustomization.yaml. They are merged into the default one.

[jbrette]

@Liujingfang1 @richardmarshall @sethp-nr @monopole Sorry I probably did not describe properly what I saw.
Please correct if I'm wrong, I had the impression that a user could only add the configurations by using the configurations field of the kustomization.yaml, but that he could not completely removed the existing default config or remove or modify an existing item. The tranformerconfig and mergeall seems to come up often in issues encountered by users.

If it is possible to completely override the default configuration, I'm not sure that we have to do any code change to kustomize. In the current example of name prefix/suffix we just have to tell the user to replace

nameprefix:
- path: metadata/name

by

nameprefix:
- path: metadata/name
  kind: kind1
- path: metadata/name
  kind: kind2

Same thing would be applicable for commonLabel, selector.... when a user has something specific to do, he just override the default config completly.

[richardmarshall]

For this one, I don't think we need to add an extra field in kustomization.yaml to support this. The skipping should be part of the transformation config.

I agree and that's what I was trying to convey, sorry if that wasn't clear. My thought was instead of adding unique transformation configs for every transformer we instead add a global transformation skip config. Would still be part of the tranformerconfig (managed through configurations: in the kustomization file.) but would allow for not having to replicate rules numerous times if a user wants to skip several transformers (8 times if they want to skip all current transformers) for a specific Kind.

To use the examples you gave for skipCommonLabels, skipNamePrefix, and skipNameSpace. This approach could be mapped in a transformer config file like this:

skipTransformation:
  - path: spec/selector
     kind: Service
     transformers:
     - commonlabels
  - kind: CustomResourceDefinition
     transformers:
     - nameprefix
  - kind: MyKind
     transformers:
     - nameprefix
     - namespace
  - kind: ClusterLeveledCRDs
     transformers:
     - namespace

If it is possible to completely override the default configuration, I'm not sure that we have to do any code change to kustomize. In the current example of name prefix/suffix we just have to tell the user to replace

nameprefix:
- path: metadata/name

by

nameprefix:
- path: metadata/name
  kind: kind1
- path: metadata/name
  kind: kind2

Same thing would be applicable for commonLabel, selector.... when a user has something specific to do, he just override the default config completly.

In my opinion this would end up being rather cumbersome for users, especially for transformers such as prefixsuffix. Having to explicitly opt-in every kind that is desired for transformation would be very challenging for larger kustomizations and prone to errors as new resources are added.

[sethp-nr]

Thanks so much for all the discussion! It sounds like there's an open question on how best to configure the "skip" behavior. So far I've seen options:

A. For each plugin, add a specific skip configuration field (roughly as implemented for PrefixSuffixTransformer here)
B. A list of (transformer, field spec) tuples that indicate what to skip
C. Introduce a json patch mechanism that allows arbitrary configuration modification, including the existing default config

While I appreciate the flexibility option C offers, I have to say that as an end-user I would find it fairly unapproachable – how would I discover what patch to use to turn off the NamePrefixSuffixTransformer on just one Kind? I am also fairly concerned about poking a hole in the "additive-only" mental model I've developed about Kustomize – right or wrong, I would not expect a higher layer to un-do some configuration I did in my base.

Regarding option B, it sounds like there's a sub-question around whether that list is specified in plugin-major order (For plugin A, skip field specs S) or in field-major order (For field spec S, skip plugins A and B). In my use-case, I'm publishing a StorageClass for consumption outside my Kustomize directory (i.e. outside the purview of namereference), so there's not a whole lot of difference to me as I'm only specifying a single pair. So far I don't see any way that we can avoid writing plugin-specific config and skip code, so either shape seems about as easy to implement.

I’m happy to pursue any one of those strategies for this PR, but regardless of which you all would prefer, I would hope we can pick something that can be done incrementally – meaning we could agree on the format here, but not necessarily gate this change on an implementation for every plugin.

Let me know how you think is best to proceed! I really appreciate everyone's time and thoughtfulness 😄

[jbrette]

@sethp-nr @richardmarshall @Liujingfang1 Hi all. I merged what we spoke about in the comments and the work that @sethp-nr had already done into one pull request: #1491 so that we can see what is user friendly and what is not.

From a pure technical standpoint, the PrefixSuffixTransformer and NamespaceTransformer are now leveraging the new feature.

It is still really rough and test coverage is really limited. Would be nice if we could find a way to share the workload, test it really well before asking to merge it into master.

[richardmarshall]

I am also fairly concerned about poking a hole in the "additive-only" mental model I've developed about Kustomize – right or wrong, I would not expect a higher layer to un-do some configuration I did in my base.

I second this concern.

Regarding option B, it sounds like there's a sub-question around whether that list is specified in plugin-major order (For plugin A, skip field specs S) or in field-major order (For field spec S, skip plugins A and B).

The way I was thinking about the idea for option B was field-major order. A list of FieldSpecs paired with the transformers that should ignore them.

@sethp-nr @richardmarshall @Liujingfang1 Hi all. I merged what we spoke about in the comments and the work that @sethp-nr had already done into one pull request: #1491 so that we can see what is user friendly and what is not.

I agree with @Liujingfang1 that this has big enough ramifications that we should drive consensus on the implementation through a KEP.

[jbrette]

@Liujingfang1 @richardmarshall @sethp-nr Here is the corresponding KEP

[monopole]

It seems most of the context is already here, rather
than on the new KEP from @jbrette, so i'll just some
add some opinion here. Sorry - that seems to happen in
kubernetes (long thread, ask for KEP, best context gets
lost)

tl;dr

For the case described here, it's possible to get the
desired result by providing a custom config to
a builtin plugin - no need for new code or new fields
in the kustomization file.

This unit test provides a custom config to
the builtin name prefix transformer such that it only
only applies to instances of Deployment and Service,
"skipping" all other kinds (in the test, the Role
resource is skipped).

The downside is having to explicitly list all the
resources to touch (see below for comments on this).

Longer opinion

Recommendation

If tempted to add new transformation-related fields to
the kustomization file, try these steps

• try writing your own config file for one of the
  builtin plugins (that works for the problem that
  kicked off this thread),

• if that seems verbose or requires repetition,
  consider using a plugin to transform (i.e. patch common
  data into) the config objects for other plugins,

• failing that, consider updating how that plugin is
  configured - i.e. add backward compatible fields to
  the plugin config (this is a code change in the plugin),

• failing that, increment the version API in the plugin
  config and add new code to the plugin to handle the
  new config,

• failing that, write an entirely new plugin.

In all cases, you get reusable custom
transformations (i.e. you and others can use the
plugin via a base-like mechanism), the config is
obvious and isolated, and the resulting plugin can be
applied in order relative to other plugins that's under
your control, and we don't add more complexity to the
old code paths.

We should deprecate the configuration: field in the
kustomization file, and describe more clearly how the
plugin approach already provides a better, completely
customizable extension mechanism.

Background (for those interested)

The v1, v2 code path

• A kustomization file represented a list of reserved
  keywords like namePrefix, commonLabels, etc.

• These would trigger execution of blocks of builtin
  transformation functions in the kustomize codebase,
  in the same order for everyone. It was a series of
  if(field1)then{doThing1}, if(field2)then{doThing2}.

• The values coming from the kustomization file
  (e.g. the value of the namePrefix: field) was only
  part of what the transforming functions needed to do
  their jobs.

• The rest of the transforming function input data came
  from a common object called TransformerConfig. The
  object represented configuration for all known
  transformations, containing fields like

  • namePrefix []FieldSpec
  • commonLabels []FieldSpec,

  etc. explaining where to apply transformations.

• The default values for this object are hard coded in
  pkg/transformers/config/defaultconfig, and can be
  overridden by content from files pointed to by the
  configuration: field in a kustomization file. This
  process raises questions - are the defaults
  overridden or merged with new data? Are the new
  values applied to upstream overlays or just to the
  containing kustomization? One can figure it out, but
  it's not obvious.

The post v3 code path

• Every builtin transformation is now implemented as a
  plugin.

• A plugin is independently configured from other
  plugins.

• There are now two ways to trigger use of a builtin plugin,
  via legacy fields or the new transformers: field.

  E.g., to use the builtin.PrefixSuffixTransformer one can
  either

  • Use the keyword namePrefix: {arg} as before in
    the kustomization file. Since everything is now a
    plugin, a plugin is triggered - in this case
    builtin.PrefixSuffixTransformer. The plugin's
    config comes in via the old v1, v2 code path using
    an instance of TransformerConfig, adaptive code
    in kusttarget_configplugin.go, and may be
    influenced by the configuration: field.

  • Omit the keyword, and instead add a plugin config
    file, specifying the
    builtin.PrefixSuffixTransformer, to the
    transformers: list. Behavior is completely
    defined by that file.

• Independently configured plugins can be put in a base
  and reused, by name, in any number of kustomizations.
  There's no way to do this in the v1, v2 world.

• Transformers (plugins) are executed in the order
  specifed in the transformers: field. Non-default
  transformer order is impossible in the v1, v2 world.

[richardmarshall]

@monopole thanks for the lengthy bit of history, context, and thoughts on this subject.

• try writing your own config file for one of the
  builtin plugins (that works for the problem that
  kicked off this thread),

As @jbrette stated in the KEP comments, this currently doesn't work with official kustomize releases as the transformers and generators fields are only used to configure external plugins. Making this work for the builtins would be pretty straight forward but will require some code changes.

It also doesn't work for the namereference and refvar transformers which are still special cases that haven't been moved to the plugin model. If they were usable in this model they would be cases where the user experience would be pretty poor given the significant number of FieldSpecs in their respective default configurations.

In general I worry about this approach being a pretty big hurdle for new users. While the current system is somewhat arcane (especially considering the rather limited documentation we have in place). It at least is a pretty small ask to have to add a single FieldSpec to a file and include that in the kustomization to enable a new field for a transformer. Having to now move the transformer bits out of the kustomization file entirely and recreate the whole plugin config externally feels like something that would be not only be difficult for new comers but also difficult to maintain. Users explicitly configuring a transformer would not be able to take advantage of updates to the default config as new versions of kustomize are released.

• if that seems verbose or requires repetition,
  consider using a plugin to transform (i.e. patch common
  data into) the config objects for other plugins,

Unless I'm missing something I don't see how we would do this in one step with the current plugin config system. Are you thinking that this would be achieved by performing two kustomize builds?

phase 1: A kustomization that includes the plugin configs as resources and manipulates them as needed.
phase 2: A second kustomization that includes the actual k8s resources as well as the output from phase 1 as generators or transformers?

• Independently configured plugins can be put in a base
  and reused, by name, in any number of kustomizations.
  There's no way to do this in the v1, v2 world.

Do you have an example of what this setup would look like?

[monopole]

@richardmarshall @jbrette Right you are. Somehow b4 vacation i'd set out to get the builtins to run from the statically compiled versions, and then came back thinking i'd already done the work - my apologies. Fixed by #1532, PTAL.

The unit tests confirming that builtin plugins could accept custom config were actually loading and running .so files (i wrote the test framework, but forgot this). Parenthetically, this is why the best test in kustomize are the full feature tests in the examples.

Unless I'm missing something...require two kustomize builds.

Just one build. In the "lowest" base, there's YAML that happens to be plugin configuration. It's specified in the resources: field of the local kustomization file (not the transformers field). Higher level overlays (used in one build) can modify that base any number of times via overlays, treating the YAML as a resource. Standard stuff. A plugin config can take patches for example.

At the highest level, the YAML coming from that particular kustomzation sequence is interpreted as a transformer config, simply by specifying it in the transformers field. Sounds worse than it is. Here's a contrived example. We need more non-toy examples of this, to evaluate it as something useful - or as something that's just a bizarre thing allowed by how kustomize works.

W/r to your other comment about the relative difficult of explaining how to customize kustomize - i.e. continuing to evolve the global TransformerConfig and add more complexity to fields in the kustomization file, vs. encouraging plugin use - and the problem of where to put all those FieldSpecs (especially for NameReference)... I'm concerned too.

working on an example to inform this some more.

[richardmarshall]

@monopole

Fixed by #1532, PTAL.

👍

that particular kustomzation sequence is interpreted as a transformer config, simply by specifying it in the transformers field.

Ah! yes got it now. Completely overlooked the implications of leveraging the resource accumulator to load the transformer/generator configs.

Sounds worse than it is.

😆

to evaluate it as something useful - or as something that's just a bizarre thing allowed by how kustomize works.

After the "oh, of course you can do that" realization it seems useful, though a bit bizarre, will play with this model a bit to form a more concrete opinion.

[monopole]

The following test is an example of custom config for the PrefixSuffixTransformer.
The test programs it to add prefixes only to Deplyment and Service, and shows how a Role is left unmodified.
https://github.com/kubernetes-sigs/kustomize/blob/master/api/internal/target/customconfigofbuiltinplugin_test.go

the plugin's config is currently oriented towards specifying which kinds to modify and ignoring others.
the plugin and it's config could be modified to do the opposite - add prefixes to everything except specific items on a skip list.

The following test has custom configuration for the LabelTransformer, showing
how to use it to add labels to specific kinds, and specific paths in those kinds:
https://github.com/kubernetes-sigs/kustomize/blob/master/api/internal/target/customconfigreusable_test.go

Moreover, the test shows how to bundle custom config for transformers into a reusable base.

[dvaldivia]

@monopole I think those two files are gone from master, could you care to point out a solution to skip a single resource by name?

[k8s-ci-robot]

@sethp-nr: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[hasueki]

@dvaldivia I was looking for the linked example as well. Here they are, https://github.com/kubernetes-sigs/kustomize/tree/bf119bf5b7ab3b51cba5845832ae1cc9807d553e/api/internal/target.