Adds tests exhibiting possible bug in NamespaceTransformer

[naanselmo]

This PR creates a pair of tests, one with expected results and one with unexpected results (both tests pass), to exhibit a possible bug.

The tests create the following structure:

 .
 ┣ 📜kustomization.yaml
 ┣ 📂a
 ┃ ┗ 📜kustomization.yaml
 ┃ ┗ 📜a.yaml
 ┣ 📂b
 ┃ ┗ 📜kustomization.yaml
 ┃ ┗ 📜b.yaml

The root kustomization.yaml only has resources set to the two subdirectories (a and then b), and the kustomization.yaml on each of those subdirectories sets its namespace to a or b (same as its subdir), and includes either a.yaml or b.yaml as a resource.

The content of a.yaml and b.yaml is identical, only having different names (one uses -a suffix, the other -b), and is as follows:

apiVersion: v1
kind: ServiceAccount
metadata:
  name: sa-a
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: crb-a
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: cr-a
subjects:
  - kind: ServiceAccount
    name: sa-a

And in this case produces the following expected result:

apiVersion: v1
kind: ServiceAccount
metadata:
  name: sa-a
  namespace: a
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: crb-a
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: cr-a
subjects:
- kind: ServiceAccount
  name: sa-a
  namespace: a
---
apiVersion: v1
kind: ServiceAccount
metadata:
  name: sa-b
  namespace: b
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: crb-b
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: cr-b
subjects:
- kind: ServiceAccount
  name: sa-b
  namespace: b

However, with a small change to each file, renaming sa-a and sa-b to sa (including in the ClusterRoleBinding), the output unexpectedly becomes:

apiVersion: v1
kind: ServiceAccount
metadata:
  name: sa
  namespace: a
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: crb-a
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: cr-a
subjects:
- kind: ServiceAccount
  name: sa
  namespace: a
---
apiVersion: v1
kind: ServiceAccount
metadata:
  name: sa
  namespace: b
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: crb-b
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: cr-b
subjects:
- kind: ServiceAccount
  name: sa
  namespace: a # <-------

With the unexpected result being that crb-b's subject's namespace became a, while I would've expected it to remain b as that would match the namespace of the ServiceAccount defined within its structure.

If the order is changed in the root kustomization.yaml so that the resource b comes before the resource a, then the opposite will happen, and crb-a will have the b namespace in its subject, which to me seems even more unexpected.

If this is indeed unexpected, then I'm guessing it comes from how the NamespaceTransformer applies its changes to a ClusterRoleBinding considering that a CRB has no namespace, possibly in:

kustomize/api/filters/namespace/namespace.go

Lines 109 to 145 in 7e0fd02

	func (ns Filter) roleBindingHack(obj *yaml.RNode, gvk resid.Gvk) error {
	if gvk.Kind != roleBindingKind && gvk.Kind != clusterRoleBindingKind {
	return nil
	}
	// Lookup the namespace field on all elements.
	// We should change the fieldspec so this isn't necessary.
	obj, err := obj.Pipe(yaml.Lookup(subjectsField))
	if err != nil || yaml.IsMissingOrNull(obj) {
	return err
	}
	// add the namespace to each "subject" with name: default
	err = obj.VisitElements(func(o *yaml.RNode) error {
	// The only case we need to force the namespace
	// if for the "service account". "default" is
	// kind of hardcoded here for right now.
	name, err := o.Pipe(
	yaml.Lookup("name"), yaml.Match("default"),
	)
	if err != nil || yaml.IsMissingOrNull(name) {
	return err
	}
	// set the namespace for the default account
	node, err := o.Pipe(
	yaml.LookupCreate(yaml.ScalarNode, "namespace"),
	)
	if err != nil {
	return err
	}
	return ns.trackableSetter.SetEntry("", ns.Namespace, yaml.NodeTagString)(node)
	})
	return err
	}

[k8s-ci-robot]

@naanselmo: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: naanselmo / name: Nuno Anselmo (86d48b2, d8efc15)

[k8s-ci-robot]

Welcome @naanselmo!

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]

Hi @naanselmo. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: naanselmo
To complete the pull request process, please assign natasha41575 after the PR has been reviewed.
You can assign the PR to them by writing /assign @natasha41575 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

[KnVerey]

Thank you for the tests--this is very helpful. I dug into the problem case on this branch, and here's what I learned.

1. The NamespaceTransformer does not handle RB/CRB subject namespaces, other than in the narrow case of the hack that you found. That hack specifically adds the namespace if and only if the subject name is "default", so it does not do anything in your example.
2. The thing that is transforming the RB/CRB namespaces correctly in one case and incorrectly in the other is actually the NameReferenceTransformer. This is a special transformer that runs only at the end of the build--not at each layer--to resolve name references globally. This transformer looks at all names a given object has had over the history of the build, finds references to any of those names across other objects (including RB/CRB), and updates those references to the final name.
3. What's happening in this case is that, since both SAs originally started off with the same name and namespace (sa/<blank>), they both have that name in their history. When NameReferenceTranformer does its work, it sees that old name in the CRB and updates it to the latest name. Unfortunately it doesn't detect the conflict, and one of the two instead silently "wins".

Solutions

The ambiguity needs to be resolved so that Kustomize can do the right thing, and there are at least two possible approaches to do this. One is to tell the NamespaceTransformer to target the subjects in at least one of the bases so that they aren't ambiguous by the time NameReferenceTransformer sees them. This is very verbose (#4404 would help), but it works. Here's the transformer config (goes in a file referenced by the transformers field.

apiVersion: builtin
kind: NamespaceTransformer
metadata:
  namespace: b
  name: include-binding-subjects
fieldSpecs:
- path: metadata/namespace
  create: true
- path: metadata/name
  kind: Namespace
  create: true
- path: subjects/namespace # not included by default
  kind: RoleBinding
  create: true
- path: subjects/namespace # not included by default
  kind: ClusterRoleBinding
  create: true
- path: spec/service/namespace
  group: apiregistration.k8s.io
  kind: APIService
  create: true
- path: spec/conversion/webhook/clientConfig/service/namespace
  group: apiextensions.k8s.io
  kind: CustomResourceDefinition

We can't make this change to the built-in defaults because it is valid/common for subjects to refer to objects in other namespaces, which we shouldn't touch. But in your case, it is what you want.

The second is to disambiguate the SAs (and the references to them) directly in the bases. Changing the name does this, as you've discovered. Including a distinct namespace (any value will do, since they're being overridden) in the bases will also work.

Possible changes to Kustomize

• I think it is a bug in NameReferenceTransformer that this case doesn't result in an error about the ambiguity (it is supposed to, and does in other cases). I found the cause and will get a PR up soon. EDIT: Correctly detect ambiguity between potential referrers when targeting a name+namespace reference #4700
• Finish Enable explicitly invoked transformers to use default fieldSpecs #4404 to make it easier for folks who want to target all subject namespaces to configure Kustomize to do so