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

ValueAddTransformer crashes w/ stack overflow under Windows if target value contains / as path separator #5078

Open
Unveraenderbar opened this issue Mar 7, 2023 · 4 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@Unveraenderbar
Copy link

What happened?

When setting up a ValueAddTransfomer using kustomize Windows executable that access the kustomization directory basename as value, a Go stack overflow is triggered if the target value contains a forward slash as path separator

What did you expect to happen?

kustomize should not crash, but instead be able to process Unix-like paths seamlessly also if running under Windows.
Still better would if kustomize would perform a "normalization" of paths to forward slash Unix notation -- IME, Windows can cope with such paths w/o problems, while generating paths containing backslashes as separators makes it impossible to do trans-platform development (i.e. develop kustomizations under Windows and use them under Unix)

How can we reproduce it (as minimally and precisely as possible)?

# kustomization.yaml
---
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- application.yaml

transformers:
- application-transformer.yaml
---
apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: prod-application
spec:
  source:
    repoURL: https://git.int.co/gitops-kustomize
  targetRevision: prod
  #path: resources\prod # works if kustomize executable for Windows is used
  path: resources/prod  # crashes w/ golang stack overflow if kustomize executable for Windows is used
---
apiVersion: builtin
kind: ValueAddTransformer
metadata:
  name: application-transformer

# Omitting the 'value:' field means that the current kustomization root directory name will be used as the value.
# value: not specified!

targets:
- selector:
    kind: Application
  fieldPath: spec/path
  filePathPosition: 99

Expected output

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: prod-application
spec:
  path: resources/prod/value-add-transformer-crash
  source:
    repoURL: https://git.int.co/gitops-kustomize
  targetRevision: prod

Actual output

kustomize Windows executables crash w/ go stack overflow

Kustomize version

v4.5.7 and v5.0.0

Operating system

Windows

@Unveraenderbar Unveraenderbar added the kind/bug Categorizes issue or PR as related to a bug. label Mar 7, 2023
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Mar 7, 2023
@natasha41575
Copy link
Contributor

There appears to be an issue in the ValueAddTransformer code, @ephesused will provide some more code details here later. Some relevant code snippets:
https://github.com/kubernetes-sigs/kustomize/blob/master/api/filters/valueadd/valueadd.go#L128
https://github.com/kubernetes-sigs/kustomize/blob/master/kyaml/filesys/util.go#L40

While we would accept a fix to prevent the stack overflow error from happening, we would like to encourage you to switch over to using the ReplacementTransformer instead. The ReplacementTransformer provides a superset of features over the ValueAddTransformer, and are considering deprecating ValueAddTransformer. The related issue is here: #4520

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 15, 2023
@ephesused
Copy link
Contributor

The crash comes from infinite recursion in filesys.PathSplit. The call to filepath.Split honors both / and \ as path separators on windows. However, filesys.PathSplit expects only os.PathSeparator (both here and here).

When "resources/prod" is processed, the first PathSplit execution breaks it into "resources/" and "prod". It then recurses with a value of "resources/". The TrimSuffix at line 51 does nothing, which means it recurses again with a value of "resources/". This continues until the stack overflow.

@Unveraenderbar
Copy link
Author

While we would accept a fix to prevent the stack overflow error from happening, we would like to encourage you to switch over to using the ReplacementTransformer instead. The ReplacementTransformer provides a superset of features over the ValueAddTransformer, and are considering deprecating ValueAddTransformer. The related issue is here: #4520

OK... but, may I ask what you think about my comment on #4520?

@k8s-triage-robot
Copy link

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
None yet
Development

No branches or pull requests

5 participants