diff --git a/.golangci.yml b/.golangci.yml index 7a8af03765..cf8047031e 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -37,7 +37,7 @@ linters: - gocyclo # - godot # - godox - - goerr113 +# - goerr113 - gofmt # - gofumpt - goheader diff --git a/Makefile-tools.mk b/Makefile-tools.mk index d795062489..351f0465c3 100644 --- a/Makefile-tools.mk +++ b/Makefile-tools.mk @@ -28,7 +28,7 @@ uninstall-out-of-tree-tools: rm -f $(MYGOBIN)/stringer $(MYGOBIN)/golangci-lint: - go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.45.2 + go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.46.2 $(MYGOBIN)/mdrip: go install github.com/monopole/mdrip@v1.0.2 diff --git a/api/filters/filtersutil/setters.go b/api/filters/filtersutil/setters.go index 1ab19e8e1a..f776667856 100644 --- a/api/filters/filtersutil/setters.go +++ b/api/filters/filtersutil/setters.go @@ -12,13 +12,13 @@ type SetFn func(*yaml.RNode) error // SetScalar returns a SetFn to set a scalar value func SetScalar(value string) SetFn { - return func(node *yaml.RNode) error { - return node.PipeE(yaml.FieldSetter{StringValue: value}) - } + return SetEntry("", value, yaml.NodeTagEmpty) } -// SetEntry returns a SetFn to set an entry in a map -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. +func SetEntry(name, value, tag string) SetFn { n := &yaml.Node{ Kind: yaml.ScalarNode, Value: value, @@ -26,7 +26,7 @@ func SetEntry(key, value, tag string) SetFn { } return func(node *yaml.RNode) error { return node.PipeE(yaml.FieldSetter{ - Name: key, + Name: name, Value: yaml.NewRNode(n), }) } @@ -34,36 +34,72 @@ func SetEntry(key, value, tag string) SetFn { type TrackableSetter struct { // SetValueCallback will be invoked each time a field is set - setValueCallback func(key, value, tag string, node *yaml.RNode) + setValueCallback func(name, value, tag string, node *yaml.RNode) } // WithMutationTracker registers a callback which will be invoked each time a field is mutated -func (s *TrackableSetter) WithMutationTracker(callback func(key, value, tag string, node *yaml.RNode)) { +func (s *TrackableSetter) WithMutationTracker(callback func(key, value, tag string, node *yaml.RNode)) *TrackableSetter { s.setValueCallback = callback + return s } -// SetScalar returns a SetFn to set a scalar value +// SetScalar returns a SetFn to set a scalar value. // if a mutation tracker has been registered, the tracker will be invoked each // time a scalar is set func (s TrackableSetter) SetScalar(value string) SetFn { - origSetScalar := SetScalar(value) + return s.SetEntry("", value, yaml.NodeTagEmpty) +} + +// SetScalarIfEmpty returns a SetFn to set a scalar value only if it isn't already set. +// If a mutation tracker has been registered, the tracker will be invoked each +// time a scalar is actually set. +func (s TrackableSetter) SetScalarIfEmpty(value string) SetFn { + return s.SetEntryIfEmpty("", value, yaml.NodeTagEmpty) +} + +// 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. +// If a mutation tracker has been registered, the tracker will be invoked each +// time an entry is set. +func (s TrackableSetter) SetEntry(name, value, tag string) SetFn { + origSetEntry := SetEntry(name, value, tag) return func(node *yaml.RNode) error { if s.setValueCallback != nil { - s.setValueCallback("", value, "", node) + s.setValueCallback(name, value, tag, node) } - return origSetScalar(node) + return origSetEntry(node) } } -// SetEntry returns a SetFn to set an entry in a map -// if a mutation tracker has been registered, the tracker will be invoked each -// time an entry is set -func (s TrackableSetter) SetEntry(key, value, tag string) SetFn { +// SetEntryIfEmpty returns a SetFn to set a field or a map entry to a value only if it isn't already set. +// 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. +// If a mutation tracker has been registered, the tracker will be invoked each +// time an entry is actually set. +func (s TrackableSetter) SetEntryIfEmpty(key, value, tag string) SetFn { origSetEntry := SetEntry(key, value, tag) return func(node *yaml.RNode) error { + if hasExistingValue(node, key) { + return nil + } if s.setValueCallback != nil { s.setValueCallback(key, value, tag, node) } return origSetEntry(node) } } + +func hasExistingValue(node *yaml.RNode, key string) bool { + if node.IsNilOrEmpty() { + return false + } + if err := yaml.ErrorIfInvalid(node, yaml.ScalarNode); err == nil { + return yaml.GetValue(node) != "" + } + entry := node.Field(key) + if entry.IsNilOrEmpty() { + return false + } + return yaml.GetValue(entry.Value) != "" +} diff --git a/api/filters/filtersutil/setters_test.go b/api/filters/filtersutil/setters_test.go new file mode 100644 index 0000000000..862a968b36 --- /dev/null +++ b/api/filters/filtersutil/setters_test.go @@ -0,0 +1,106 @@ +// Copyright 2022 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package filtersutil_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sigs.k8s.io/kustomize/api/filters/filtersutil" + "sigs.k8s.io/kustomize/kyaml/yaml" +) + +func TestTrackableSetter_SetScalarIfEmpty(t *testing.T) { + tests := []struct { + name string + input *yaml.RNode + value string + want string + }{ + { + name: "sets null values", + input: yaml.MakeNullNode(), + value: "foo", + want: "foo", + }, + { + name: "sets empty values", + input: yaml.NewScalarRNode(""), + value: "foo", + want: "foo", + }, + { + name: "does not overwrite values", + input: yaml.NewStringRNode("a"), + value: "foo", + want: "a", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + wasSet := false + s := (&filtersutil.TrackableSetter{}).WithMutationTracker(func(_, _, _ string, _ *yaml.RNode) { + wasSet = true + }) + wantSet := tt.value == tt.want + fn := s.SetScalarIfEmpty(tt.value) + require.NoError(t, fn(tt.input)) + assert.Equal(t, tt.want, yaml.GetValue(tt.input)) + assert.Equal(t, wantSet, wasSet, "tracker invoked even though value was not changed") + }) + } +} + +func TestTrackableSetter_SetEntryIfEmpty(t *testing.T) { + tests := []struct { + name string + input *yaml.RNode + key string + value string + want string + }{ + { + name: "sets empty values", + input: yaml.NewMapRNode(&map[string]string{"setMe": ""}), + key: "setMe", + value: "foo", + want: "foo", + }, + { + name: "sets missing keys", + input: yaml.NewMapRNode(&map[string]string{}), + key: "setMe", + value: "foo", + want: "foo", + }, + { + name: "does not overwrite values", + input: yaml.NewMapRNode(&map[string]string{"existing": "original"}), + key: "existing", + value: "foo", + want: "original", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + wasSet := false + s := (&filtersutil.TrackableSetter{}).WithMutationTracker(func(_, _, _ string, _ *yaml.RNode) { + wasSet = true + }) + wantSet := tt.value == tt.want + fn := s.SetEntryIfEmpty(tt.key, tt.value, "") + require.NoError(t, fn(tt.input)) + assert.Equal(t, tt.want, yaml.GetValue(tt.input.Field(tt.key).Value)) + assert.Equal(t, wantSet, wasSet, "tracker invoked even though value was not changed") + }) + } +} + +func TestTrackableSetter_SetEntryIfEmpty_BadInputNodeKind(t *testing.T) { + fn := filtersutil.TrackableSetter{}.SetEntryIfEmpty("foo", "false", yaml.NodeTagBool) + rn := yaml.NewListRNode("nope") + rn.AppendToFieldPath("dummy", "path") + assert.EqualError(t, fn(rn), "wrong Node Kind for dummy.path expected: MappingNode was SequenceNode: value: {- nope}") +} diff --git a/api/filters/nameref/nameref.go b/api/filters/nameref/nameref.go index 860ff09736..7549ab7001 100644 --- a/api/filters/nameref/nameref.go +++ b/api/filters/nameref/nameref.go @@ -337,19 +337,16 @@ func (f Filter) selectReferral( return candidates[0], nil } ids := getIds(candidates) - f.failureDetails(candidates) - return nil, fmt.Errorf(" found multiple possible referrals: %s", ids) + return nil, fmt.Errorf("found multiple possible referrals: %s\n%s", ids, f.failureDetails(candidates)) } -func (f Filter) failureDetails(resources []*resource.Resource) { - fmt.Printf( - "\n**** Too many possible referral targets to referrer:\n%s\n", - f.Referrer.MustYaml()) +func (f Filter) failureDetails(resources []*resource.Resource) string { + msg := strings.Builder{} + msg.WriteString(fmt.Sprintf("\n**** Too many possible referral targets to referrer:\n%s\n", f.Referrer.MustYaml())) for i, r := range resources { - fmt.Printf( - "--- possible referral %d:\n%s", i, r.MustYaml()) - fmt.Println("------") + msg.WriteString(fmt.Sprintf("--- possible referral %d:\n%s\n", i, r.MustYaml())) } + return msg.String() } func allNamesAreTheSame(resources []*resource.Resource) bool { diff --git a/api/filters/namespace/namespace.go b/api/filters/namespace/namespace.go index 8d0f54ea28..1b28d5c347 100644 --- a/api/filters/namespace/namespace.go +++ b/api/filters/namespace/namespace.go @@ -19,6 +19,9 @@ type Filter struct { // FsSlice contains the FieldSpecs to locate the namespace field FsSlice types.FsSlice `json:"fieldSpecs,omitempty" yaml:"fieldSpecs,omitempty"` + // UnsetOnly means only blank namespace fields will be set + UnsetOnly bool `json:"unsetOnly" yaml:"unsetOnly"` + trackableSetter filtersutil.TrackableSetter } @@ -36,47 +39,34 @@ func (ns Filter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) { // Run runs the filter on a single node rather than a slice func (ns Filter) run(node *yaml.RNode) (*yaml.RNode, error) { - // hacks for hardcoded types -- :( - if err := ns.hacks(node); err != nil { + // Special handling for metadata.namespace -- :( + // never let SetEntry handle metadata.namespace--it will incorrectly include cluster-scoped resources + ns.FsSlice = ns.removeMetaNamespaceFieldSpecs(ns.FsSlice) + gvk := resid.GvkFromNode(node) + if err := ns.metaNamespaceHack(node, gvk); err != nil { return nil, err } - // Remove the fieldspecs that are for hardcoded fields. The fieldspecs - // exist for backwards compatibility with other implementations - // of this transformation. - // This implementation of the namespace transformation - // Does not use the fieldspecs for implementing cases which - // require hardcoded logic. - ns.FsSlice = ns.removeFieldSpecsForHacks(ns.FsSlice) + // Special handling for (cluster) role binding -- :( + if isRoleBinding(gvk.Kind) { + ns.FsSlice = ns.removeRoleBindingFieldSpecs(ns.FsSlice) + if err := ns.roleBindingHack(node, gvk); err != nil { + return nil, err + } + } // transformations based on data -- :) err := node.PipeE(fsslice.Filter{ FsSlice: ns.FsSlice, - SetValue: ns.trackableSetter.SetEntry("", ns.Namespace, yaml.NodeTagString), + SetValue: ns.fieldSetter(), CreateKind: yaml.ScalarNode, // Namespace is a ScalarNode CreateTag: yaml.NodeTagString, }) return node, err } -// hacks applies the namespace transforms that are hardcoded rather -// than specified through FieldSpecs. -func (ns Filter) hacks(obj *yaml.RNode) error { - gvk := resid.GvkFromNode(obj) - if err := ns.metaNamespaceHack(obj, gvk); err != nil { - return err - } - return ns.roleBindingHack(obj, gvk) -} - // metaNamespaceHack is a hack for implementing the namespace transform // for the metadata.namespace field on namespace scoped resources. -// namespace scoped resources are determined by NOT being present -// in a hard-coded list of cluster-scoped resource types (by apiVersion and kind). -// -// This hack should be updated to allow individual resources to specify -// if they are cluster scoped through either an annotation on the resources, -// or through inlined OpenAPI on the resource as a YAML comment. func (ns Filter) metaNamespaceHack(obj *yaml.RNode, gvk resid.Gvk) error { if gvk.IsClusterScoped() { return nil @@ -85,16 +75,16 @@ func (ns Filter) metaNamespaceHack(obj *yaml.RNode, gvk resid.Gvk) error { FsSlice: []types.FieldSpec{ {Path: types.MetadataNamespacePath, CreateIfNotPresent: true}, }, - SetValue: ns.trackableSetter.SetEntry("", ns.Namespace, yaml.NodeTagString), + SetValue: ns.fieldSetter(), CreateKind: yaml.ScalarNode, // Namespace is a ScalarNode } _, err := f.Filter(obj) return err } -// roleBindingHack is a hack for implementing the namespace transform +// roleBindingHack is a hack for implementing the transformer's DefaultSubjectsOnly mode // for RoleBinding and ClusterRoleBinding resource types. -// RoleBinding and ClusterRoleBinding have namespace set on +// In this mode, RoleBinding and ClusterRoleBinding have namespace set on // elements of the "subjects" field if and only if the subject elements // "name" is "default". Otherwise the namespace is not set. // @@ -107,12 +97,11 @@ func (ns Filter) metaNamespaceHack(obj *yaml.RNode, gvk resid.Gvk) error { // - name: "something-else" # this will not have the namespace set // ... func (ns Filter) roleBindingHack(obj *yaml.RNode, gvk resid.Gvk) error { - if gvk.Kind != roleBindingKind && gvk.Kind != clusterRoleBindingKind { + if !isRoleBinding(gvk.Kind) { 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 @@ -138,27 +127,33 @@ func (ns Filter) roleBindingHack(obj *yaml.RNode, gvk resid.Gvk) error { return err } - return ns.trackableSetter.SetEntry("", ns.Namespace, yaml.NodeTagString)(node) + return ns.fieldSetter()(node) }) return err } -// removeFieldSpecsForHacks removes from the list fieldspecs that +func isRoleBinding(kind string) bool { + return kind == roleBindingKind || kind == clusterRoleBindingKind +} + +// removeRoleBindingFieldSpecs removes from the list fieldspecs that // have hardcoded implementations -func (ns Filter) removeFieldSpecsForHacks(fs types.FsSlice) types.FsSlice { +func (ns Filter) removeRoleBindingFieldSpecs(fs types.FsSlice) types.FsSlice { var val types.FsSlice for i := range fs { - // implemented by metaNamespaceHack - if fs[i].Path == types.MetadataNamespacePath { - continue - } - // implemented by roleBindingHack - if fs[i].Kind == roleBindingKind && fs[i].Path == subjectsField { + if isRoleBinding(fs[i].Kind) && fs[i].Path == subjectsField { continue } - // implemented by roleBindingHack - if fs[i].Kind == clusterRoleBindingKind && fs[i].Path == subjectsField { + val = append(val, fs[i]) + } + return val +} + +func (ns Filter) removeMetaNamespaceFieldSpecs(fs types.FsSlice) types.FsSlice { + var val types.FsSlice + for i := range fs { + if fs[i].Path == types.MetadataNamespacePath { continue } val = append(val, fs[i]) @@ -166,6 +161,13 @@ func (ns Filter) removeFieldSpecsForHacks(fs types.FsSlice) types.FsSlice { return val } +func (ns *Filter) fieldSetter() filtersutil.SetFn { + if ns.UnsetOnly { + return ns.trackableSetter.SetEntryIfEmpty("", ns.Namespace, yaml.NodeTagString) + } + return ns.trackableSetter.SetEntry("", ns.Namespace, yaml.NodeTagString) +} + const ( subjectsField = "subjects" roleBindingKind = "RoleBinding" diff --git a/api/internal/builtins/NamespaceTransformer.go b/api/internal/builtins/NamespaceTransformer.go index 39a514e8e5..03a664cff8 100644 --- a/api/internal/builtins/NamespaceTransformer.go +++ b/api/internal/builtins/NamespaceTransformer.go @@ -16,6 +16,7 @@ import ( type NamespaceTransformerPlugin struct { types.ObjectMeta `json:"metadata,omitempty" yaml:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` FieldSpecs []types.FieldSpec `json:"fieldSpecs,omitempty" yaml:"fieldSpecs,omitempty"` + UnsetOnly bool `json:"unsetOnly" yaml:"unsetOnly"` } func (p *NamespaceTransformerPlugin) Config( @@ -38,6 +39,7 @@ func (p *NamespaceTransformerPlugin) Transform(m resmap.ResMap) error { if err := r.ApplyFilter(namespace.Filter{ Namespace: p.Namespace, FsSlice: p.FieldSpecs, + UnsetOnly: p.UnsetOnly, }); err != nil { return err } diff --git a/plugin/builtin/namespacetransformer/NamespaceTransformer.go b/plugin/builtin/namespacetransformer/NamespaceTransformer.go index 807091bab7..249fdf1ea4 100644 --- a/plugin/builtin/namespacetransformer/NamespaceTransformer.go +++ b/plugin/builtin/namespacetransformer/NamespaceTransformer.go @@ -17,6 +17,7 @@ import ( type plugin struct { types.ObjectMeta `json:"metadata,omitempty" yaml:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` FieldSpecs []types.FieldSpec `json:"fieldSpecs,omitempty" yaml:"fieldSpecs,omitempty"` + UnsetOnly bool `json:"unsetOnly" yaml:"unsetOnly"` } //noinspection GoUnusedGlobalVariable @@ -42,6 +43,7 @@ func (p *plugin) Transform(m resmap.ResMap) error { if err := r.ApplyFilter(namespace.Filter{ Namespace: p.Namespace, FsSlice: p.FieldSpecs, + UnsetOnly: p.UnsetOnly, }); err != nil { return err } diff --git a/plugin/builtin/namespacetransformer/NamespaceTransformer_test.go b/plugin/builtin/namespacetransformer/NamespaceTransformer_test.go index 8e581a1942..efcc90c92a 100644 --- a/plugin/builtin/namespacetransformer/NamespaceTransformer_test.go +++ b/plugin/builtin/namespacetransformer/NamespaceTransformer_test.go @@ -10,6 +10,18 @@ import ( kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest" ) +const defaultFieldSpecs = ` +fieldSpecs: +- path: metadata/namespace + create: true +- path: subjects + kind: RoleBinding + group: rbac.authorization.k8s.io +- path: subjects + kind: ClusterRoleBinding + group: rbac.authorization.k8s.io +` + func TestNamespaceTransformer1(t *testing.T) { th := kusttest_test.MakeEnhancedHarness(t). PrepBuiltin("NamespaceTransformer") @@ -20,16 +32,7 @@ kind: NamespaceTransformer metadata: name: notImportantHere namespace: test -fieldSpecs: -- path: metadata/namespace - create: true -- path: subjects - kind: RoleBinding - group: rbac.authorization.k8s.io -- path: subjects - kind: ClusterRoleBinding - group: rbac.authorization.k8s.io -`, ` +`+defaultFieldSpecs, ` apiVersion: v1 kind: ConfigMap metadata: @@ -268,3 +271,151 @@ metadata: } }) } + +func TestNamespaceTransformer_UnsetOnlyTrue(t *testing.T) { + th := kusttest_test.MakeEnhancedHarness(t). + PrepBuiltin("NamespaceTransformer") + defer th.Reset() + th.RunTransformerAndCheckResult(` +apiVersion: builtin +kind: NamespaceTransformer +metadata: + name: notImportantHere + namespace: test +unsetOnly: true +fieldSpecs: +- path: metadata/namespace + create: true +- path: subjects/namespace + kind: RoleBinding + group: rbac.authorization.k8s.io +- path: subjects/namespace + kind: ClusterRoleBinding + group: rbac.authorization.k8s.io +- path: spec/template/namespace + kind: MyWeirdObject +- path: spec/template/altNamespace + kind: MyWeirdObject + create: true +`, ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm1 +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm2 + namespace: foo +--- +apiVersion: v1 +kind: Service +metadata: + name: svc1 +--- +apiVersion: v1 +kind: Service +metadata: + name: svc2 + namespace: "" +--- +apiVersion: v1 +kind: Namespace +metadata: + name: ns1 +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: manager-rolebinding +subjects: +- kind: ServiceAccount + name: default + namespace: other +- kind: ServiceAccount + name: default + namespace: "" +- kind: ServiceAccount + name: default +- kind: ServiceAccount + name: another + namespace: random +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: crd +--- +apiVersion: v1 +kind: MyWeirdObject +metadata: + name: custom +spec: + template: + namespace: original +`, + ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm1 + namespace: test +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm2 + namespace: foo +--- +apiVersion: v1 +kind: Service +metadata: + name: svc1 + namespace: test +--- +apiVersion: v1 +kind: Service +metadata: + name: svc2 + namespace: test +--- +apiVersion: v1 +kind: Namespace +metadata: + name: ns1 +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: manager-rolebinding +subjects: +- kind: ServiceAccount + name: default + namespace: other +- kind: ServiceAccount + name: default + namespace: test +- kind: ServiceAccount + name: default + namespace: test +- kind: ServiceAccount + name: another + namespace: random +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: crd +--- +apiVersion: v1 +kind: MyWeirdObject +metadata: + name: custom + namespace: test +spec: + template: + altNamespace: test + namespace: original +`) +} diff --git a/plugin/builtin/namespacetransformer/go.mod b/plugin/builtin/namespacetransformer/go.mod index 8b161fa9d6..daa24f62c7 100644 --- a/plugin/builtin/namespacetransformer/go.mod +++ b/plugin/builtin/namespacetransformer/go.mod @@ -4,6 +4,7 @@ go 1.18 require ( sigs.k8s.io/kustomize/api v0.11.5 + sigs.k8s.io/kustomize/kyaml v0.13.7 sigs.k8s.io/yaml v1.2.0 ) @@ -32,5 +33,4 @@ require ( gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect k8s.io/kube-openapi v0.0.0-20220401212409-b28bf2818661 // indirect - sigs.k8s.io/kustomize/kyaml v0.13.7 // indirect )