Option to customize NamespaceTransformer role binding subject handling #4704

KnVerey · 2022-07-11T17:09:01Z

Resolves #1599
Closes #4301 and #4302, which are alternative solutions to the same issue.

We can't change the default, since that would be enormously disruptive to our users. Instead, this PR proposes a new option for the transformer to enable it to behave in the desired manner. This option are exposed explicitly via the transformer's configuration resource, which can be used in the transformers field. While using resource-specific annotations to configure transformers is an interesting idea that may be appropriate in some cases, my opinion is that transformer-level is better granularity for these particular options. I also considered a selection-base solution as suggested here, but at that point I think folks should reach for replacements instead, now that they exist.

apiVersion: builtin
kind: NamespaceTransformer
metadata:
  name: notImportantHere
  namespace: test
setRoleBindingSubjects: defaultOnly|allServiceAccounts|none # new
fieldSpecs:
- path: metadata/name
  kind: Namespace
  create: true

Originally, this PR had all instead of allServiceAccounts. However, the other two documented kinds are not namespaced, so setting all of them / using fieldspecs for these will never make sense. I'm not sure from the docs whether it is possible to have auth plugins with their own bespoke kinds. If that is possible, and those kinds are namespaced, they will not be handled by this transformer.

k8s-ci-robot · 2022-07-11T17:09:04Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

natasha41575 · 2022-07-11T20:43:42Z

api/filters/namespace/namespace.go

+	// objects will have their namespace fields set. Overrides field specs provided for these types, if any.
+	// - defaultOnly (default): namespace will be set only on subjects named "default".
+	// - all: namespace will be set on all subjects
+	// - none: all subjects will be skipped.


This is an interesting idea, and I really like it!

One complication I just realized is the meaning of "all". Some options:

Support the value, and drive it with create: false fieldspecs. This means all subject namespaces with any value will be overwritten, allowing the user to effectively tell us which subjects are namespaced by setting dummy values in them. Has a weird interaction where skipExisting: true, setRoleBindingSubjects: all sets nothing.

Support the value, and drive it with create: true fieldspecs. I think this is unacceptable, because it will add namespaces to subjects with cluster-scoped kinds.

Don't support that value, but stop stripping the role binding fieldspecs so that users can do one of the above themselves (not any more elegantly).

Don't support that value, and continue stripping the role binding fieldspecs.

Support the value, and drive it with openAPI-aware and/or bespoke code (I could be wrong don't think User and Group are part of openapi, since they're not real kinds). Much the same as we do for metadata.namespace, except only do it if the "all" option is specified or the fieldspecs are given.

This PR currently does the first one. But I'm leaning towards one of the last two after typing this out.

I update the PR to do option 5. Confirmed that the OpenAPI does not identify User or Group as cluster-scoped, so was not usable for this purpose.

Sorry for the late response. I agree that the last two options are best, and am happy with the choice of option 5.

k8s-ci-robot · 2022-07-11T21:45:43Z

@KnVerey: 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.

KnVerey · 2022-07-11T21:25:24Z

api/filters/filtersutil/setters.go

-func SetEntry(key, value, tag string) SetFn {
+// SetEntry returns a SetFn to set a field or a map entry to a value.
+// It can be used with an empty name to set both a value and a tag on a scalar node.
+// When setting only a value on a scalar node, use SetScalar instead.


These new sentences were already true, but since the comment said it "set an entry in a map" I was initially confused, thinking NamespaceTransformer was doing the wrong thing / relying on an unintentional behaviour. But no, FieldSetter is documented to handle both scalars and maps, so SetEntry does too (even though "entry" sounds like it targets a sequence, which is not handled 😕 ).

.golangci.yml

Makefile-tools.mk

KnVerey · 2022-07-11T21:50:05Z

api/filters/filtersutil/setters.go

-	return func(node *yaml.RNode) error {
-		return node.PipeE(yaml.FieldSetter{StringValue: value})
-	}
+	return SetEntry("", value, yaml.NodeTagEmpty)


Since yaml.FieldSetter with an empty Name and populated StringValue creates a NewScalarRNode internally, this is exactly the same thing.

KnVerey · 2022-07-11T21:50:43Z

api/filters/filtersutil/setters.go

 	n := &yaml.Node{
 		Kind:  yaml.ScalarNode,
 		Value: value,
 		Tag:   tag,
 	}
 	return func(node *yaml.RNode) error {
 		return node.PipeE(yaml.FieldSetter{
-			Name:  key,
+			Name:  name,


as with the other changes, this is to make it more clear that it's perfectly fine to use SetEntry with scalars (which don't have keys)

KnVerey · 2022-07-11T22:04:07Z

api/konfig/builtinpluginconsts/namespace.go

  kind: RoleBinding
- path: subjects
+- path: subjects/namespace


These were incorrect and previously unused. While subjects is a valid fieldspec, it returns a map that includes the namespace field. The name reference transformer uses specs like as a signal to modify multiple fields at once (name and namespace), but this one expects scalars. I take it the pre-kyaml version of the transformer used these, and it must have been written differently.

The subjects is a slice of object which can have namespace field. Using subjects/namespace here seems to indicate kustomize deals with slice type in jsonpath this way (compared to the replacement transformer syntax [], like subjects[].namespace).

Correct, it isn't JsonPath syntax. For better or for worse, these older transformers use Kustomize's own field path syntax in their fieldSpec fields.

KnVerey · 2022-07-11T22:04:44Z

api/konfig/builtinpluginconsts/namespace.go

@@ -6,14 +6,12 @@ package builtinpluginconsts
 const (
 	namespaceFieldSpecs = `
 namespace:
- path: metadata/namespace
-  create: true


We could leave this in, or not. It's ignored either way... in fact it is actively stripped out when the defaults or user-provided specs include it.

api/krusty/namedspacedserviceaccounts_test.go

api/filters/namespace/namespace.go

KnVerey · 2022-07-11T23:47:31Z

api/filters/namespace/namespace.go

+	// objects will have their namespace fields set. Overrides field specs provided for these types, if any.
+	// - defaultOnly (default): namespace will be set only on subjects named "default".
+	// - all: namespace will be set on all subjects
+	// - none: all subjects will be skipped.


One complication I just realized is the meaning of "all". Some options:

Support the value, and drive it with create: false fieldspecs. This means all subject namespaces with any value will be overwritten, allowing the user to effectively tell us which subjects are namespaced by setting dummy values in them. Has a weird interaction where skipExisting: true, setRoleBindingSubjects: all sets nothing.

Support the value, and drive it with create: true fieldspecs. I think this is unacceptable, because it will add namespaces to subjects with cluster-scoped kinds.

Don't support that value, but stop stripping the role binding fieldspecs so that users can do one of the above themselves (not any more elegantly).

Don't support that value, and continue stripping the role binding fieldspecs.

Support the value, and drive it with openAPI-aware and/or bespoke code (I could be wrong don't think User and Group are part of openapi, since they're not real kinds). Much the same as we do for metadata.namespace, except only do it if the "all" option is specified or the fieldspecs are given.

This PR currently does the first one. But I'm leaning towards one of the last two after typing this out.

KnVerey · 2022-07-12T00:03:55Z

/test all

api/filters/namespace/namespace.go

KnVerey · 2022-07-15T20:07:53Z

api/konfig/builtinpluginconsts/namespace.go

 - path: metadata/name
  kind: Namespace
  create: true
- path: subjects
+- path: subjects/namespace


These are also stripped. Should we fix them (as this is doing) or just remove them?

Is there any purpose of keeping them in here? I see why we need to strip them for user provided fieldspecs but I'm not sure I understand the purpose of keeping stripped values in our builtin fieldspecs.

It would just be to show that the transformer affects these fields, I guess. In theory we could use it as a signal of whether or not we should (manually) modify them, but in practice that could be a breaking change for people who have existing manually configured transformers. I'm definitely fine with removing them.

I have a slight preference for removing them, but don't feel too strongly either way.

KnVerey · 2022-07-15T20:09:35Z

plugin/builtin/namespacetransformer/NamespaceTransformer_test.go

+  group: rbac.authorization.k8s.io
+- path: subjects/namespace
+  kind: ClusterRoleBinding
+  group: rbac.authorization.k8s.io


in the tests, I'm keeping both the old invalid paths and the valid paths to prove they all get stripped. If we stop stripping the old invalid ones, that could cause an error for someone who has customized their transformer by copying the default specs.

Can we emit a warning if we see an old invalid fieldspec? I imagine we'll eventually want to remove the code that handles the legacy fieldspecs

We can, but another option is to improve the error message for all cases where we're given a path that leads to a non-sequence node, and allow this case to fall through to that (hopefully helpful) error message. Plus make sure to have a release note stating that we fixed a bug where certain invalid fieldspecs were being ignored in the advanced configuration of the namespace transformer. #4727 takes that approach. It includes a dep change, so is a bigger risk for the kubectl update. I suggest we should merge this one (which continues to ignore the invalid fieldspec) and then make a decision on the invalid fieldspec in the other PR.

natasha41575

Apart from removing the unnecessary fieldspecs and a couple other minor comments, this LGTM

natasha41575 · 2022-07-26T18:53:54Z

plugin/builtin/namespacetransformer/NamespaceTransformer.go

+	case namespace.SubjectModeUnspecified:
+		p.SetRoleBindingSubjects = namespace.DefaultSubjectsOnly
+	default:
+		return errors.Errorf("invalid value %s for setRoleBindingSubjects", p.SetRoleBindingSubjects)


nit: suggest listing the valid values here to help users fix the issue (and quoting the values)

i.e. errors.Errorf("invalid value %q for setRoleBindingSubjects, must be one of %q or %q", p.SetRoleBindingSubjects, namespace.AllServiceAccountSubjects, namespace.DefaultSubjectsOnly)

natasha41575 · 2022-07-28T15:26:24Z

/lgtm
/approve

k8s-ci-robot · 2022-07-28T15:26:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KnVerey, natasha41575

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [KnVerey,natasha41575]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 11, 2022

k8s-ci-robot requested review from natasha41575 and yuwenma July 11, 2022 17:09

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 11, 2022

KnVerey force-pushed the ns_transformer_options branch from cecdf24 to 7e2312d Compare July 11, 2022 20:23

natasha41575 reviewed Jul 11, 2022

View reviewed changes

KnVerey force-pushed the ns_transformer_options branch from 7e2312d to df42626 Compare July 11, 2022 21:45

KnVerey force-pushed the ns_transformer_options branch 2 times, most recently from 6375d3b to feb93be Compare July 11, 2022 23:34

KnVerey commented Jul 11, 2022

View reviewed changes

yuwenma reviewed Jul 12, 2022

View reviewed changes

api/filters/namespace/namespace.go Outdated Show resolved Hide resolved

KnVerey force-pushed the ns_transformer_options branch from feb93be to 16f83e1 Compare July 12, 2022 23:42

KnVerey mentioned this pull request Jul 12, 2022

Option to customize NamespaceTransformer overwrite behaviour #4708

Merged

KnVerey changed the title ~~Options to customize NamespaceTransformer overwriting and role binding subject handling~~ Option to customize NamespaceTransformer role binding subject handling Jul 12, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2022

KnVerey force-pushed the ns_transformer_options branch from e9ea157 to 2091c22 Compare July 15, 2022 17:01

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 15, 2022

KnVerey force-pushed the ns_transformer_options branch from 2091c22 to fda85ca Compare July 15, 2022 19:58

KnVerey commented Jul 15, 2022

View reviewed changes

KnVerey marked this pull request as ready for review July 15, 2022 20:10

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2022

k8s-ci-robot requested a review from natasha41575 July 15, 2022 20:11

k8s-ci-robot requested a review from yuwenma July 15, 2022 20:11

natasha41575 reviewed Jul 26, 2022

View reviewed changes

KnVerey force-pushed the ns_transformer_options branch from fda85ca to 92e86ac Compare July 28, 2022 03:04

KnVerey mentioned this pull request Jul 28, 2022

Improve invalid node kind errors #4727

Merged

Option to customize NamespaceTransfomer role binding subject handling

0c37ee8

KnVerey force-pushed the ns_transformer_options branch from 92e86ac to 0c37ee8 Compare July 28, 2022 03:13

k8s-ci-robot assigned natasha41575 Jul 28, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2022

k8s-ci-robot merged commit c4b4a41 into kubernetes-sigs:master Jul 28, 2022

KnVerey mentioned this pull request Jul 29, 2022

Add docs for new advanced NamespaceTransformer config options kubernetes-sigs/cli-experimental#290

Merged

chobostar mentioned this pull request Aug 17, 2022

namespace set in an overlay’s kustomization.yaml does not override the namespace for the subjects of a cluster role binding #136

Closed

vishaltak mentioned this pull request Nov 4, 2022

kustomize 4.5.7 does not replace namespaces in subjects of rolebinding and clusterrolebinding devfile/devworkspace-operator#971

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to customize NamespaceTransformer role binding subject handling #4704

Option to customize NamespaceTransformer role binding subject handling #4704

KnVerey commented Jul 11, 2022 •

edited

k8s-ci-robot commented Jul 11, 2022

natasha41575 Jul 11, 2022

KnVerey Jul 11, 2022 •

edited

KnVerey Jul 15, 2022

natasha41575 Jul 18, 2022

k8s-ci-robot commented Jul 11, 2022

KnVerey Jul 11, 2022

KnVerey Jul 11, 2022

KnVerey Jul 11, 2022

KnVerey Jul 11, 2022

yuwenma Jul 12, 2022

KnVerey Jul 12, 2022

KnVerey Jul 11, 2022

KnVerey Jul 11, 2022 •

edited

KnVerey commented Jul 12, 2022

KnVerey Jul 15, 2022

natasha41575 Jul 18, 2022 •

edited

KnVerey Jul 19, 2022

natasha41575 Jul 26, 2022

KnVerey Jul 28, 2022

KnVerey Jul 15, 2022

natasha41575 Jul 18, 2022 •

edited

KnVerey Jul 28, 2022

natasha41575 left a comment •

edited

natasha41575 Jul 26, 2022 •

edited

natasha41575 commented Jul 28, 2022

k8s-ci-robot commented Jul 28, 2022

Option to customize NamespaceTransformer role binding subject handling #4704

Option to customize NamespaceTransformer role binding subject handling #4704

Conversation

KnVerey commented Jul 11, 2022 • edited

k8s-ci-robot commented Jul 11, 2022

Choose a reason for hiding this comment

KnVerey Jul 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnVerey Jul 11, 2022 • edited

Choose a reason for hiding this comment

KnVerey commented Jul 12, 2022

Choose a reason for hiding this comment

natasha41575 Jul 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 Jul 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 left a comment • edited

Choose a reason for hiding this comment

natasha41575 Jul 26, 2022 • edited

Choose a reason for hiding this comment

natasha41575 commented Jul 28, 2022

k8s-ci-robot commented Jul 28, 2022

KnVerey commented Jul 11, 2022 •

edited

KnVerey Jul 11, 2022 •

edited

KnVerey Jul 11, 2022 •

edited

natasha41575 Jul 18, 2022 •

edited

natasha41575 Jul 18, 2022 •

edited

natasha41575 left a comment •

edited

natasha41575 Jul 26, 2022 •

edited