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

GetFieldValue doesn't work with fields that contain dots(.) #4487

Closed
Goodwine opened this issue Feb 24, 2022 · 6 comments · Fixed by #4591
Closed

GetFieldValue doesn't work with fields that contain dots(.) #4487

Goodwine opened this issue Feb 24, 2022 · 6 comments · Fixed by #4591
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@Goodwine
Copy link
Contributor

Describe the bug

rnode.GetFieldValue doesn't work correctly when the target field contains a .

For example when I call rnode.GetFieldValue("foo.bar") this function will try to find field "bar" within field "foo" instead of finding the field "foo.bar"

Files that can reproduce the issue

package v1alpha1

import (
	"testing"

	"github.com/stretchr/testify/require"
	"sigs.k8s.io/kustomize/kyaml/kio"
	"sigs.k8s.io/kustomize/kyaml/yaml"
)

const input = `
kind: Pod
metadata:
  name: hello-world
  labels:
    app: hello-world-app
    foo.appname: hello-world-foo
`

func TestGetFieldValue(t *testing.T) {
	data, err := kio.ParseAll(input)
	require.NoError(t, err)

	labelRNode, err := data[0].Pipe(yaml.Lookup("metadata", "labels"))
	require.NoError(t, err)

	app, err := labelRNode.GetFieldValue("app")
	require.NoError(t, err)
	require.Equal(t, "hello-world-app", app)

	fooAppName, err := labelRNode.GetFieldValue("foo.appname")
	require.NoError(t, err)
	require.Equal(t, "hello-world-foo", fooAppName) // no field named 'foo.appname'
}

Expected output

labelRNode.GetFieldValue("foo.appname") should return hello-world-app

Actual output

labelRNode.GetFieldValue("foo.appname") returns an error because there is no field foo

Kustomize version

require sigs.k8s.io/kustomize/kyaml v0.13.1

Additional context

If my pod was defined as:

kind: Pod
metadata:
  name: hello-world
  labels:
    app: hello-world-app
    foo.appname: hello-world-foo
    foo:
      appname: wtf       # <--------

then it would have "worked" but it would have returned wtf instead of hello-world-app.

Workaround

I can still use rnode.Pipe(yaml.Lookup("foo.appname")) and I can also use m, err := rnode.Map() followed by m["foo.appname"], it just caught me offguard

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

natasha41575 commented Feb 24, 2022

/triage accepted

There are these path splitters in api that allow escaping delimiters, with a TODO to move them to kyaml. Part of this issue would to be to finally do that TODO, and use it in rnode.GetFieldValue, so that you can do something like rnode.GetFieldValue("foo\.bar").

Edit: Forgot to link the path splitters: https://github.com/kubernetes-sigs/kustomize/blob/master/api/internal/utils/pathsplitter.go#L8

@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 Feb 24, 2022
@natasha41575 natasha41575 added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Feb 24, 2022
@Goodwine
Copy link
Contributor Author

Is there an example of something that was migrated over from strings.Split to the better path splitters in case this is something I could try to work on?

In hindsight I guess it makes sense obj.foo.bar doesn't work because in regular JSON I would do obj["foo.bar"], I guess what was surprising to me is that I expected GetFieldValue not to recursively go down the path, the name of the function made me think I was extracting the value of the key/field

@natasha41575
Copy link
Contributor

It's used in the replacements filter here:

fieldPath := utils.SmarterPathSplitter(fp, ".")

The purpose is to traverse a path that looks like metadata.annotations.config\.kubernetes\.io/local-config. This lets you target something like

metadata:
  annotations:
    config.kubernetes.io/local-config: # this is the target field here

@r0ushann
Copy link

hello! I'm interested in this issue. please guide me.

@NikhilSharmaWe
Copy link
Member

@natasha41575 So I think there is no need to make any changes in the repo since it is not a bug, am I right ?
Could you please inform if we need some improvements, I will look forward to work on it.

@natasha41575
Copy link
Contributor

See my comment above: #4487 (comment). This explains the work that needs to be done to resolve this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
5 participants