Skip to content

Commit

Permalink
Improve variable conflict detection. Fix bug 506
Browse files Browse the repository at this point in the history
  • Loading branch information
@jbrette
jbrette committed Oct 4, 2019
1 parent d2b9b7a commit a6e2850
Show file tree
Hide file tree
Showing 7 changed files with 160 additions and 67 deletions.
81 changes: 47 additions & 34 deletions pkg/accumulator/resaccumulator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,19 +172,22 @@ func (ra *ResAccumulator) AppendResolvedVars(otherSet types.VarSet) error {
"var '%s' already encountered", v.Name)
}

matched := ra.resMap.GetMatchingResourcesByOriginalId(
resid.NewResId(v.ObjRef.GVK(), v.ObjRef.Name).GvknEquals)
if len(matched) > 1 {
// We detected a diamond import of kustomization
// context where one variable pointing at one resource
// in each context is now pointing at two resources
// (different CurrId) because the two contexts have
// been merged.
return fmt.Errorf(
"found %d resId matches for var %s "+
"(unable to disambiguate)",
len(matched), v)
}
// Behavior has been modified.
// Conflict detection moved to VarMap object
// =============================================
// matched := ra.resMap.GetMatchingResourcesByOriginalId(
// resid.NewResId(v.ObjRef.GVK(), v.ObjRef.Name).GvknEquals)
// if len(matched) > 1 {
// // We detected a diamond import of kustomization
// // context where one variable pointing at one resource
// // in each context is now pointing at two resources
// // (different CurrId) because the two contexts have
// // been merged.
// return fmt.Errorf(
// "found %d resId matches for var %s "+
// "(unable to disambiguate)",
// len(matched), v)
// }
}
return ra.varSet.MergeSlice(mergeableVars)
}
Expand All @@ -207,12 +210,16 @@ func (ra *ResAccumulator) MergeVars(incoming []types.Var) error {
idMatcher = targetId.Equals
}
matched := ra.resMap.GetMatchingResourcesByOriginalId(idMatcher)
if len(matched) > 1 {
return fmt.Errorf(
"found %d resId matches for var %s "+
"(unable to disambiguate)",
len(matched), v)
}

// Behavior has been modified.
// Conflict detection moved to VarMap object
// =============================================
// if len(matched) > 1 {
// return fmt.Errorf(
// "found %d resId matches for var %s "+
// "(unable to disambiguate)",
// len(matched), v)
// }

if len(matched) == 0 {
// no associated resources yet.
Expand All @@ -222,11 +229,13 @@ func (ra *ResAccumulator) MergeVars(incoming []types.Var) error {
continue
}

// Found one unique associated resource.
// Found one or more associated resource.
if err := ra.varSet.Absorb(v); err != nil {
return err
}
matched[0].AppendRefVarName(v)
for _, match := range matched {
match.AppendRefVarName(v)
}
}

return nil
Expand All @@ -248,39 +257,43 @@ func (ra *ResAccumulator) MergeAccumulator(other *ResAccumulator) (err error) {
return ra.AppendResolvedVars(other.varSet)
}

func (ra *ResAccumulator) findVarValueFromResources(v types.Var) (interface{}, error) {
func (ra *ResAccumulator) findVarValueFromResources(v types.Var, varMap *resmap.VarMap) error {
foundCount := 0
for _, res := range ra.resMap.Resources() {
for _, varName := range res.GetRefVarNames() {
if varName == v.Name {
s, err := res.GetFieldValue(v.FieldRef.FieldPath)
if err != nil {
return "", fmt.Errorf(
return fmt.Errorf(
"field specified in var '%v' "+
"not found in corresponding resource", v)
}

return s, nil
foundCount++
varMap.Append(v.Name, res, s)
}
}
}

return "", fmt.Errorf(
"var '%v' cannot be mapped to a field "+
"in the set of known resources", v)
if foundCount == 0 {
return fmt.Errorf(
"var '%v' cannot be mapped to a field "+
"in the set of known resources", v)
}

return nil
}

// makeVarReplacementMap returns a map of Var names to
// their final values. The values are strings intended
// for substitution wherever the $(var.Name) occurs.
func (ra *ResAccumulator) makeVarReplacementMap() (map[string]interface{}, error) {
result := map[string]interface{}{}
func (ra *ResAccumulator) makeVarReplacementMap() (resmap.VarMap, error) {
result := resmap.NewEmptyVarMap()
for _, v := range ra.Vars() {
s, err := ra.findVarValueFromResources(v)
err := ra.findVarValueFromResources(v, &result)
if err != nil {
return nil, err
return result, err
}

result[v.Name] = s
}

return result, nil
Expand All @@ -295,7 +308,7 @@ func (ra *ResAccumulator) ResolveVars() error {
if err != nil {
return err
}
if len(replacementMap) == 0 {
if len(replacementMap.VarNames()) == 0 {
return nil
}
t := transformers.NewRefVarTransformer(
Expand Down
47 changes: 28 additions & 19 deletions pkg/accumulator/resaccumulator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package accumulator_test

import (
"bytes"
"fmt"
"log"
"os"
"strings"
Expand Down Expand Up @@ -171,13 +170,20 @@ func TestResolveVarsVarNeedsDisambiguation(t *testing.T) {
},
},
})
if err == nil {
if err != nil {
t.Fatalf("expected error")
}
if !strings.Contains(
err.Error(), "unable to disambiguate") {
t.Fatalf("unexpected err: %v", err)
}

// Behavior has been modified.
// Conflict detection moved to VarMap object
// =============================================
// if err == nil {
// t.Fatalf("expected error")
// }
// if !strings.Contains(
// err.Error(), "unable to disambiguate") {
// t.Fatalf("unexpected err: %v", err)
// }
}

func TestResolveVarsGoodResIdBadField(t *testing.T) {
Expand Down Expand Up @@ -482,19 +488,22 @@ func TestAppendResolvedVarsDiamondWithConflicts(t *testing.T) {
t.Fatalf("unexpected err: %v", err)
}

// A second call will result in a conflicting variable name. Further,
// it will be unclear whether this variable refers to app1-backend or
// app2-backend. This is an error
expectedErrMsg := fmt.Sprintf(
"found %d resId matches for var %s "+
"(unable to disambiguate)",
ra.ResMap().Size(), serviceVar)
if err := ra.AppendResolvedVars(vars); err == nil {
t.Fatalf("expected error")
} else if err.Error() != expectedErrMsg {
t.Errorf("received the wrong error, expected `%s`, got `%s`",
expectedErrMsg, err.Error())
}
// Behavior has been modified.
// Conflict detection moved to VarMap object
// =============================================
// // A second call will result in a conflicting variable name. Further,
// // it will be unclear whether this variable refers to app1-backend or
// // app2-backend. This is an error
// expectedErrMsg := fmt.Sprintf(
// "found %d resId matches for var %s "+
// "(unable to disambiguate)",
// ra.ResMap().Size(), serviceVar)
// if err := ra.AppendResolvedVars(vars); err == nil {
// t.Fatalf("expected error")
// } else if err.Error() != expectedErrMsg {
// t.Errorf("received the wrong error, expected `%s`, got `%s`",
// expectedErrMsg, err.Error())
// }
}

func find(name string, resMap resmap.ResMap) *resource.Resource {
Expand Down
69 changes: 69 additions & 0 deletions pkg/resmap/varmap.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// Copyright 2019 The Kubernetes Authors.
// SPDX-License-Identifier: Apache-2.0

// Package resmap implements a map from ResId to Resource that
// tracks all resources in a kustomization.
package resmap

import (
"fmt"
"sigs.k8s.io/kustomize/v3/pkg/resource"
)

type varValue struct {
value interface{}
ctx *resource.Resource
}

type VarMap struct {
varmap map[string][]varValue
}

func NewEmptyVarMap() VarMap {
return VarMap{varmap: make(map[string][]varValue)}
}

func NewVarMap(simple map[string]interface{}) VarMap {
varmap := make(map[string][]varValue)
for key, value := range simple {
oneVal := varValue{value: value, ctx: nil}
varmap[key] = []varValue{oneVal}
}
return VarMap{varmap: varmap}
}

func (vm *VarMap) Append(name string, ctx *resource.Resource, value interface{}) {
if _, found := vm.varmap[name]; !found {
vm.varmap[name] = []varValue{}
}

vm.varmap[name] = append(vm.varmap[name], varValue{value: value, ctx: ctx})
}

func (vm *VarMap) VarNames() []string {
var names []string
for k := range vm.varmap {
names = append(names, k)
}
return names
}

func (vm *VarMap) SubsetThatCouldBeReferencedByResource(
inputRes *resource.Resource) (map[string]interface{}, error) {
result := map[string]interface{}{}
for varName, varValueSlice := range vm.varmap {
for _, r := range varValueSlice {
rctxm := inputRes.PrefixesSuffixesEquals
if (r.ctx == nil) || (r.ctx.InSameKustomizeCtx(rctxm)) {
if _, found := result[varName]; found {
return result, fmt.Errorf(
"found %d resId matches for var %s "+
"(unable to disambiguate)",
2, varName)
}
result[varName] = r.value
}
}
}
return result, nil
}
4 changes: 2 additions & 2 deletions pkg/target/kusttarget_configplugin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@
package target

import (
"strings"
"sigs.k8s.io/kustomize/v3/pkg/image"
"sigs.k8s.io/kustomize/v3/pkg/plugins"
"sigs.k8s.io/kustomize/v3/pkg/resmap"
"sigs.k8s.io/kustomize/v3/pkg/transformers/config"
"sigs.k8s.io/kustomize/v3/pkg/types"
"strings"
)

// Functions dedicated to configuring the builtin
Expand Down Expand Up @@ -175,7 +175,7 @@ var transformerConfigurators = map[plugins.BuiltinPluginType]func(
plugins.PatchStrategicMergeTransformer: func(
kt *KustTarget, bpt plugins.BuiltinPluginType, f tFactory, _ *config.TransformerConfig) (
result []resmap.Transformer, err error) {
if len(kt.kustomization.PatchesStrategicMerge) == 0 && len(kt.dynamic.Patches) == 0 {
if len(kt.kustomization.PatchesStrategicMerge) == 0 && len(kt.dynamic.Patches) == 0 {
return
}
var c struct {
Expand Down
10 changes: 4 additions & 6 deletions pkg/target/variableref_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -502,13 +502,11 @@ spec:
image: whatever
name: myServer
`
_, err := th.MakeKustTarget().MakeCustomizedResMap()
if err == nil {
t.Fatalf("should have an error")
}
if !strings.Contains(err.Error(), "(unable to disambiguate)") {
t.Fatalf("unexpected err: %v", err)
m, err := th.MakeKustTarget().MakeCustomizedResMap()
if err != nil {
t.Fatalf("Err: %v", err)
}
th.AssertActualEqualsExpected(m, presumablyDesired)
}

func TestVarRefBig(t *testing.T) {
Expand Down
14 changes: 9 additions & 5 deletions pkg/transformers/refvars.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
)

type RefVarTransformer struct {
varMap map[string]interface{}
varMap resmap.VarMap
replacementCounts map[string]int
fieldSpecs []config.FieldSpec
mappingFunc func(string) interface{}
Expand All @@ -34,7 +34,7 @@ type RefVarTransformer struct {
// that replaces $(VAR) style variables with values.
// The fieldSpecs are the places to look for occurrences of $(VAR).
func NewRefVarTransformer(
varMap map[string]interface{}, fs []config.FieldSpec) *RefVarTransformer {
varMap resmap.VarMap, fs []config.FieldSpec) *RefVarTransformer {
return &RefVarTransformer{
varMap: varMap,
fieldSpecs: fs,
Expand Down Expand Up @@ -91,7 +91,7 @@ func (rv *RefVarTransformer) replaceVars(in interface{}) (interface{}, error) {
// after a Transform run.
func (rv *RefVarTransformer) UnusedVars() []string {
var unused []string
for k := range rv.varMap {
for _, k := range rv.varMap.VarNames() {
_, ok := rv.replacementCounts[k]
if !ok {
unused = append(unused, k)
Expand All @@ -103,9 +103,13 @@ func (rv *RefVarTransformer) UnusedVars() []string {
// Transform replaces $(VAR) style variables with values.
func (rv *RefVarTransformer) Transform(m resmap.ResMap) error {
rv.replacementCounts = make(map[string]int)
rv.mappingFunc = expansion.MappingFuncFor(
rv.replacementCounts, rv.varMap)
for _, res := range m.Resources() {
varSubset, err := rv.varMap.SubsetThatCouldBeReferencedByResource(res)
if err != nil {
return err
}
rv.mappingFunc = expansion.MappingFuncFor(
rv.replacementCounts, varSubset)
for _, fieldSpec := range rv.fieldSpecs {
if res.OrgId().IsSelected(&fieldSpec.Gvk) {
if err := MutateField(
Expand Down
2 changes: 1 addition & 1 deletion pkg/transformers/refvars_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ func TestVarRef(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
// arrange
tr := NewRefVarTransformer(tc.given.varMap, tc.given.fs)
tr := NewRefVarTransformer(resmap.NewVarMap(tc.given.varMap), tc.given.fs)

// act
err := tr.Transform(tc.given.res)
Expand Down

0 comments on commit a6e2850

Please sign in to comment.