kpt fn render recursive setters malformed

[paulwoelfel]

Expected behavior

Test git repo: https://github.com/cloudpilots/kpt-recurison-bug

Given the following folder structure:

• main package
  • gcr-io sub-package
  • googleapis sub-package

The purpose of the package is to setup googleapis access for a project with config connector. It creates a ComputeRoute and 2 DNS zones (the sub packages).

We want to be able to render recursively the package and the two sub-packages. With kpt fn render . all resources should look the same same as if we run kpt fn render gcr-io, if all the setters are the same.

The file gcr-io/dns-record-private.yaml should have this metadata name

metadata:
  name: my-project-123-gcr-io-private # kpt-set: ${project-id}-${zone-name}-private

Actual behavior

If we run render in the main folder, the apply-setters kpt function does break some values, if they contain a - in the value.

The file gcr-io/dns-record-private.yaml has this metadata name after a render of the main folder:

metadata:
  name: my-project-123-io-private # kpt-set: ${project-id}-${zone-name}-private

The metadata.name changes from my-project-123-gcr-io-private to my-project-123-io-private.

This is the output of a git diff which shows the changed resources in the sub package gcr-io:

diff --git a/gcr-io/dns-record-private.yaml b/gcr-io/dns-record-private.yaml
index c49f4f5..84bf6f3 100644
--- a/gcr-io/dns-record-private.yaml
+++ b/gcr-io/dns-record-private.yaml
@@ -1,7 +1,7 @@
 apiVersion: dns.cnrm.cloud.google.com/v1beta1
 kind: DNSRecordSet
 metadata:
-  name: my-project-123-gcr-io-private # kpt-set: ${project-id}-${zone-name}-private
+  name: my-project-123-io-private # kpt-set: ${project-id}-${zone-name}-private

The setters in the main folder are:

data:
  network-name: vpc-01
  network-namespace: networking
  project-id: my-project-123
  projects-namespace: projects

In the subfolder the setters add the required values for the DNS Zone:

data:
  network-name: vpc-01
  network-namespace: networking
  project-id: my-project-123
  projects-namespace: projects
  dns-name: gcr.io.
  zone-name: gcr-io
  ttl: "600"

So it seems like the bug is related to having a - in the name. kpt fn render does render the sub files, but it removes some parts of the value: io instead of gcr-io.

Information

Kpt version: 1.0.0-beta.17 Example Repo: https://github.com/cloudpilots/kpt-recurison-bug

The issue seems to relate to #2474 from @RolandOtta

Steps to reproduce the behavior

git clone https://github.com/cloudpilots/kpt-recurison-bug
kpt fn render googleapis --truncate-output=false
kpt fn render gcr-io --truncate-output=false
# Everything fine here, do a commit just in case something has changed
git add .; git commit -m "Commit diff"
# This breaks the resources in gcr-io and googleapis
kpt fn render . --truncate-output=false
# show the differences, which should not be there
git diff

[paulwoelfel]

@RolandOtta: As the other issue has been closed, I've opened a new issue.

Your supposed change does fix it for me too.

[laureen71]

@paulwoelfel: it looks like the mentioned test repo is not publically available

[paulwoelfel]

@lauren71 Repo is now public: https://github.com/cloudpilots/kpt-recurison-bug

[yuwenma]

Thank you for catching it and filing the issue, @paulwoelfel. I can reproduce the problems and agree this is a bug in apply-setter around the dash - between two setters.

We plan not to make active development to apply-setters because it will increase the setter usage and having the users being less KRM focused. But I can see this issue has many up-votes. Thus, we may want to make this an exception and accept fixes in apply-setter (more likely the fix is in kustomize/kyaml)

[yuwenma]

I triaged the issue and found the root cause. For @paulwoelfel A quick fix on your side is to simply remove the project-id setter in the root kpt-recursion-bug setters.yaml file.

Root cause

apply-setters uses wild-card pattern (?P<x>.*) to match and replace the setters. When two setters have dash - used as both a value substring (like gcr-io my-project-123) and the concatenation syntax (like ${project-id}-${zone-name}), the wildcard can match the wrong string and re-group the default setter values due to the lookahead matching for nested pkg hydration.

Specifically, in @paulwoelfel example, after the child pkg gcr-io runs kpt-fn-render which updates the project-id and zone-name, the parent dir kpt-recursion-bug maps the pattern ${project-id}-${zone-name}-private (value my-project-123-gcr-io-private) to project-id: my-project-123-gcr and zone-name: io. The project-id value is overridden by the parent dir to "my-project-123" which now is correct, but the zone-name is still using the wrong value io.

Solutions

This is one of the caveats of using comment based wildcard matching (what apply-setters does mostly). There's no way to modify the wildcard matching inside the apply-setters because the wildcard cannot distinguish the right pattern if the same syntax is used (we may be able to cache the original setters in krm annotations like what we did for the first-class transformer identifier, but it is much more complicated than what setters suppose to be).

A note for other users: you can either choose to not use the same syntax in multi-setter value, or avoid using the same setters among child package and parent package.

[paulwoelfel]

@yuwenma you have just described the issue, but not the solution. The problem is that we have got a package structure with main and sub packages to setup projects. Therefore we need to selectively add kpt packages in the folder. With that project-id is then also provided to all sub-packages like we also expect. Just for special patterns it renders rubish.

So I don't get why this issue just got closed? This is a bug and you said a lot of other people are facing this issue.

The patch @RolandOtta suggested in #2474 works fine, why not apply this patch?

[yuwenma]

@paulwoelfel Thanks for linking the issue #2474. It's good to learn more context and the historical issues about apply-setters. I think what the previous author @phanimarupaka said aligned with my suggestion, but I'm sorry if I didn't make it clear:

First of all, the apply-setters supposes to have a simple logic as using wildcard match to parse comments and replace the matching values, and that's it. It has the caveats of not doing the expected parsing if the pattern is ambiguous. That's why both @phanimarupaka and I suggest using a different pattern to avoid the ambiguities (Like @phanimarupaka replied on #2474, apply-setters expect to derive the values of var1 and var2 from a pattern). Thus, a solution to your case is to avoid using ambiguous pattern. I also looked at the patch approach @RolandOtta suggested, I agree it will "fix" the current issue, but it also changes the overridden behaviors.

Secondly, the apply-setters has been spoiled and in many cases is used as an omnipotent function (bypassing the kpt config from Kptfile to setters.yaml). Thus, we have decided to discontinue accepting new changes to apply-setters. Instead, we encourage users to use more resource specific functions so they are aware of the KRM resource type they are dealing with.

My original suggestion is to remove the project-id setter in your parent package. Now I see that it is not feasible in your case because you want to extend the child packages. For a more generic solution, I'd suggest using resource specific functions like set-project-id, apply-replacements

Let me know if you have any other questions.

[paulwoelfel]

Hi @yuwenma!

Thanks for the detailed explanation. I still see this as a major bug for apply-setters and it is a pitty, that apply-setters is discontinued. This was one of the major features of kpt (besides packaging configs) we were using. We have also developed kpt fn for other usecases, but it looks like we need to change the tooling for future developments. This has really reduced a lot of trust in kpt and we will move to other tools like helm in future.

[yuwenma]

Thank you for sharing such honest feedback @paulwoelfel and it's really good to know your team has developed other Kpt functions. Our team took your feedback into consideration and here are the plans:

We understand many users have their Kpt packages tied closely with the "setter" features. Our concerns are mainly from the long term extension and config accuracy perspective. We truly hope that for whoever uses "setter" or Helm, you can be aware of these potential issues.

At the same time, we are very enthusiastic about the package management and Porch solutions. So we do not want to hold users back because of setters.

Right now, we are stretched thin and we would like to ask for other contributors' help to make “setter”/Kpt better. We are looking at ways to make it easier for people to contribute and for expanding the contributions and leadership to the broader community.

I'll reopen this issue. For everyone who is interested in the effort, I’ll keep you posted.

[RolandOtta]

imo the only solution for this issue is to get rid of the regex search for parsing values from child packages.

instead of this: why not build a map of all the parameters and their values from all setters of rendered kpt packages involved at first and then use this map for the search/replace mechanism instead of that regex search?