From 211cda054ece2c3e97437af316b534ba876553af Mon Sep 17 00:00:00 2001 From: jregan Date: Sat, 9 Jun 2018 18:50:53 -0700 Subject: [PATCH] Some cleanup refactoring in app package. --- pkg/app/application.go | 280 +++++++++------------- pkg/app/application_test.go | 12 +- pkg/commands/build.go | 4 +- pkg/commands/diff.go | 8 +- pkg/resmap/resmap.go | 33 ++- pkg/resmap/resmap_test.go | 2 +- pkg/transformers/patch.go | 2 +- pkg/transformers/patchconflictdetector.go | 3 +- 8 files changed, 145 insertions(+), 199 deletions(-) diff --git a/pkg/app/application.go b/pkg/app/application.go index 26ab5fd0d..27bbf6ce5 100644 --- a/pkg/app/application.go +++ b/pkg/app/application.go @@ -36,31 +36,17 @@ import ( "github.com/pkg/errors" ) -// Application interface exposes methods to get resources of the application. -type Application interface { - // Resources computes and returns the resources for the app. - Resources() (resmap.ResMap, error) - // SemiResources computes and returns the resources without name hash and name reference for the app - SemiResources() (resmap.ResMap, error) - // RawResources computes and returns the raw resources from the kustomization file. - // It contains resources from - // 1) untransformed resources from current kustomization file - // 2) transformed resources from sub packages - RawResources() (resmap.ResMap, error) - // Vars returns all the variables defined by the app - Vars() ([]types.Var, error) -} - -var _ Application = &applicationImpl{} - -// Private implementation of the Application interface -type applicationImpl struct { +// Application implements the guts of the kustomize 'build' command. +// TODO: Change name, as "application" is overloaded and somewhat +// misleading (one can customize an RBAC policy). Perhaps "Target" +// https://github.com/kubernetes-sigs/kustomize/blob/master/docs/glossary.md#target +type Application struct { kustomization *types.Kustomization loader loader.Loader } -// New parses the kustomization file at the path using the loader and returns application. -func New(loader loader.Loader) (Application, error) { +// NewApplication returns a new instance of Application primed with a Loader. +func NewApplication(loader loader.Loader) (*Application, error) { content, err := loader.Load(constants.KustomizationFileName) if err != nil { return nil, err @@ -71,32 +57,50 @@ func New(loader loader.Loader) (Application, error) { if err != nil { return nil, err } - return &applicationImpl{kustomization: &m, loader: loader}, nil + return &Application{kustomization: &m, loader: loader}, nil } -// Resources computes and returns the resources from the kustomization file. -// The namehashing for configmap/secrets and resolving name reference is only done -// in the most top overlay once at the end of getting resources. -func (a *applicationImpl) Resources() (resmap.ResMap, error) { - res, err := a.SemiResources() +// MakeCustomizedResMap creates a ResMap per kustomization instructions. +// The Resources in the returned ResMap are fully customized. +func (a *Application) MakeCustomizedResMap() (resmap.ResMap, error) { + m, err := a.loadCustomizedResMap() if err != nil { return nil, err } - t, err := a.newHashAndReferenceTransformer(res) - if err != nil { - return nil, err - } - err = t.Transform(res) - if err != nil { - return nil, err - } - return res, nil + return a.resolveRefsToGeneratedResources(m) } -// SemiResources computes and returns the resources without name hash and name reference for the app -func (a *applicationImpl) SemiResources() (resmap.ResMap, error) { +// resolveRefsToGeneratedResources fixes all name references. +func (a *Application) resolveRefsToGeneratedResources(m resmap.ResMap) (resmap.ResMap, error) { + r := []transformers.Transformer{transformers.NewNameHashTransformer()} + + t, err := transformers.NewDefaultingNameReferenceTransformer() + if err != nil { + return nil, err + } + r = append(r, t) + + refVars, err := a.resolveRefVars(m) + if err != nil { + return nil, err + } + t, err = transformers.NewRefVarTransformer(refVars) + if err != nil { + return nil, err + } + r = append(r, t) + + err = transformers.NewMultiTransformer(r).Transform(m) + if err != nil { + return nil, err + } + return m, nil +} + +// loadCustomizedResMap loads and customizes resources to build a ResMap. +func (a *Application) loadCustomizedResMap() (resmap.ResMap, error) { errs := &interror.KustomizationErrors{} - raw, err := a.rawResources() + result, err := a.loadResMapFromBasesAndResources() if err != nil { errs.Append(errors.Wrap(err, "rawResources")) } @@ -109,12 +113,12 @@ func (a *applicationImpl) SemiResources() (resmap.ResMap, error) { if err != nil { errs.Append(errors.Wrap(err, "NewResMapFromSecretArgs")) } - res, err := resmap.Merge(cms, secrets) + res, err := resmap.MergeWithoutOverride(cms, secrets) if err != nil { return nil, errors.Wrap(err, "Merge") } - allRes, err := resmap.MergeWithOverride(raw, res) + result, err = resmap.MergeWithOverride(result, res) if err != nil { return nil, err } @@ -127,175 +131,120 @@ func (a *applicationImpl) SemiResources() (resmap.ResMap, error) { if len(errs.Get()) > 0 { return nil, errs } - t, err := a.newTransformer(patches) if err != nil { return nil, err } - err = t.Transform(allRes) + err = t.Transform(result) if err != nil { return nil, err } - return allRes, nil + return result, nil } -// RawResources computes and returns the raw resources from the kustomization file. -// The namehashing for configmap/secrets and resolving name reference is only done -// in the most top overlay once at the end of getting resources. -func (a *applicationImpl) RawResources() (resmap.ResMap, error) { - res, err := a.rawResources() - if err != nil { - return nil, errors.Wrap(err, "RawResources") - } - t, err := a.newHashAndReferenceTransformer(res) +// MakeUncustomizedResMap purports to create a ResMap without customization. +// The Resources in the returned ResMap include all resources mentioned +// in the kustomization file and transitively reachable via its Bases, +// and all generated secrets and configMaps. +// Meant for use in generating a diff against customized resources. +func (a *Application) MakeUncustomizedResMap() (resmap.ResMap, error) { + m, err := a.loadResMapFromBasesAndResources() if err != nil { return nil, err } - err = t.Transform(res) - if err != nil { - return nil, err - } - return res, nil + return a.resolveRefsToGeneratedResources(m) } -func (a *applicationImpl) rawResources() (resmap.ResMap, error) { - subAppResources, errs := a.subAppResources() +// Gets Bases and Resources as advertised. +func (a *Application) loadResMapFromBasesAndResources() (resmap.ResMap, error) { + bases, errs := a.loadCustomizedBases() resources, err := resmap.NewResMapFromFiles(a.loader, a.kustomization.Resources) if err != nil { errs.Append(errors.Wrap(err, "rawResources failed to read Resources")) } - if len(errs.Get()) > 0 { return nil, errs } - - return resmap.Merge(resources, subAppResources) + return resmap.MergeWithoutOverride(resources, bases) } -func (a *applicationImpl) subAppResources() (resmap.ResMap, *interror.KustomizationErrors) { - sliceOfSubAppResources := []resmap.ResMap{} +// Loop through the Bases of this kustomization recursively loading resources. +// Combine into one ResMap, demanding unique Ids for each resource. +func (a *Application) loadCustomizedBases() (resmap.ResMap, *interror.KustomizationErrors) { + list := []resmap.ResMap{} errs := &interror.KustomizationErrors{} - for _, pkgPath := range a.kustomization.Bases { - subloader, err := a.loader.New(pkgPath) + for _, path := range a.kustomization.Bases { + loader, err := a.loader.New(path) if err != nil { - errs.Append(errors.Wrap(err, "couldn't make loader for "+pkgPath)) + errs.Append(errors.Wrap(err, "couldn't make loader for "+path)) continue } - subapp, err := New(subloader) + app, err := NewApplication(loader) if err != nil { - errs.Append(errors.Wrap(err, "couldn't make app for "+pkgPath)) + errs.Append(errors.Wrap(err, "couldn't make app for "+path)) continue } - // Gather all transformed resources from subpackages. - subAppResources, err := subapp.SemiResources() + resMap, err := app.loadCustomizedResMap() if err != nil { errs.Append(errors.Wrap(err, "SemiResources")) continue } - sliceOfSubAppResources = append(sliceOfSubAppResources, subAppResources) + list = append(list, resMap) } - allResources, err := resmap.Merge(sliceOfSubAppResources...) + result, err := resmap.MergeWithoutOverride(list...) if err != nil { errs.Append(errors.Wrap(err, "Merge failed")) } - return allResources, errs + return result, errs } -func (a *applicationImpl) subApp() ([]Application, error) { - var apps []Application +func (a *Application) loadBasesAsFlatList() ([]*Application, error) { + var result []*Application errs := &interror.KustomizationErrors{} - for _, basePath := range a.kustomization.Bases { - subloader, err := a.loader.New(basePath) + for _, path := range a.kustomization.Bases { + loader, err := a.loader.New(path) if err != nil { errs.Append(err) continue } - subapp, err := New(subloader) + a, err := NewApplication(loader) if err != nil { errs.Append(err) continue } - apps = append(apps, subapp) + result = append(result, a) } if len(errs.Get()) > 0 { return nil, errs } - return apps, nil + return result, nil } -// newTransformer generates the following transformers: -// 1) apply overlay -// 2) name prefix -// 3) apply labels -// 4) apply annotations -func (a *applicationImpl) newTransformer(patches []*resource.Resource) (transformers.Transformer, error) { - ts := []transformers.Transformer{} - - ot, err := transformers.NewPatchTransformer(patches) +// newTransformer makes a Transformer that does everything except resolve generated names. +func (a *Application) newTransformer(patches []*resource.Resource) (transformers.Transformer, error) { + r := []transformers.Transformer{} + t, err := transformers.NewPatchTransformer(patches) if err != nil { return nil, err } - ts = append(ts, ot) - - ts = append(ts, transformers.NewNamespaceTransformer(string(a.kustomization.Namespace))) - - npt, err := transformers.NewDefaultingNamePrefixTransformer(string(a.kustomization.NamePrefix)) + r = append(r, t) + r = append(r, transformers.NewNamespaceTransformer(string(a.kustomization.Namespace))) + t, err = transformers.NewDefaultingNamePrefixTransformer(string(a.kustomization.NamePrefix)) if err != nil { return nil, err } - ts = append(ts, npt) - - lt, err := transformers.NewDefaultingLabelsMapTransformer(a.kustomization.CommonLabels) + r = append(r, t) + t, err = transformers.NewDefaultingLabelsMapTransformer(a.kustomization.CommonLabels) if err != nil { return nil, err } - ts = append(ts, lt) - - at, err := transformers.NewDefaultingAnnotationsMapTransformer(a.kustomization.CommonAnnotations) + r = append(r, t) + t, err = transformers.NewDefaultingAnnotationsMapTransformer(a.kustomization.CommonAnnotations) if err != nil { return nil, err } - ts = append(ts, at) - - return transformers.NewMultiTransformer(ts), nil -} - -// newHashAndReferenceTransformer generates the following transformers: -// 1) name hash for configmap and secrests -// 2) apply name reference -// 3) apply reference variables -func (a *applicationImpl) newHashAndReferenceTransformer(allRes resmap.ResMap) (transformers.Transformer, error) { - ts := []transformers.Transformer{} - nht := transformers.NewNameHashTransformer() - ts = append(ts, nht) - - nrt, err := transformers.NewDefaultingNameReferenceTransformer() - if err != nil { - return nil, err - } - ts = append(ts, nrt) - t, err := a.newVariableReferenceTransformer(allRes) - if err != nil { - return nil, err - } - ts = append(ts, t) - - return transformers.NewMultiTransformer(ts), nil -} - -func (a *applicationImpl) newVariableReferenceTransformer(allRes resmap.ResMap) (transformers.Transformer, error) { - refvars, err := a.resolveRefVars(allRes) - if err != nil { - return nil, err - } - - glog.Infof("found all the refvars: %+v", refvars) - - varExpander, err := transformers.NewRefVarTransformer(refvars) - if err != nil { - return nil, err - } - return varExpander, nil + r = append(r, t) + return transformers.NewMultiTransformer(r), nil } func unmarshal(y []byte, o interface{}) error { @@ -309,53 +258,52 @@ func unmarshal(y []byte, o interface{}) error { return dec.Decode(o) } -func (a *applicationImpl) resolveRefVars(resources resmap.ResMap) (map[string]string, error) { - refvars := map[string]string{} - vars, err := a.Vars() +func (a *Application) resolveRefVars(m resmap.ResMap) (map[string]string, error) { + result := map[string]string{} + vars, err := a.getAllVars() if err != nil { - return refvars, err + return result, err } - - for _, refvar := range vars { - id := resource.NewResId(refvar.ObjRef.GroupVersionKind(), refvar.ObjRef.Name) - if r, found := resources[id]; found { - s, err := r.GetFieldValue(refvar.FieldRef.FieldPath) + for _, v := range vars { + id := resource.NewResId(v.ObjRef.GroupVersionKind(), v.ObjRef.Name) + if r, found := m[id]; found { + s, err := r.GetFieldValue(v.FieldRef.FieldPath) if err != nil { - return nil, fmt.Errorf("failed to resolve referred var: %+v", refvar) + return nil, fmt.Errorf("failed to resolve referred var: %+v", v) } - refvars[refvar.Name] = s + result[v.Name] = s } else { - glog.Infof("couldn't resolve refvar: %v", refvar) + glog.Infof("couldn't resolve v: %v", v) } } - return refvars, nil + return result, nil } -// Vars returns all the variables defined at the app and subapps of the app -func (a *applicationImpl) Vars() ([]types.Var, error) { - vars := []types.Var{} +// getAllVars returns all the "environment" style Var instances defined in the app. +func (a *Application) getAllVars() ([]types.Var, error) { + result := []types.Var{} errs := &interror.KustomizationErrors{} - apps, err := a.subApp() + bases, err := a.loadBasesAsFlatList() if err != nil { return nil, err } - // TODO: computing vars and resources for subApps can be combined - for _, subApp := range apps { - subAppVars, err := subApp.Vars() + // TODO: computing vars and resources for bases can be combined + for _, b := range bases { + vars, err := b.getAllVars() if err != nil { errs.Append(err) continue } - vars = append(vars, subAppVars...) + result = append(result, vars...) } for _, v := range a.kustomization.Vars { v.Defaulting() - vars = append(vars, v) + result = append(result, v) } if len(errs.Get()) > 0 { return nil, errs } - return vars, nil + return result, nil } diff --git a/pkg/app/application_test.go b/pkg/app/application_test.go index d3a3c76be..3706c604b 100644 --- a/pkg/app/application_test.go +++ b/pkg/app/application_test.go @@ -181,11 +181,11 @@ func TestResources1(t *testing.T) { }), } l := makeLoader1(t) - app, err := New(l) + app, err := NewApplication(l) if err != nil { t.Fatalf("Unexpected construction error %v", err) } - actual, err := app.Resources() + actual, err := app.MakeCustomizedResMap() if err != nil { t.Fatalf("Unexpected Resources error %v", err) } @@ -216,11 +216,11 @@ func TestRawResources1(t *testing.T) { }), } l := makeLoader1(t) - app, err := New(l) + app, err := NewApplication(l) if err != nil { t.Fatalf("Unexpected construction error %v", err) } - actual, err := app.RawResources() + actual, err := app.MakeUncustomizedResMap() if err != nil { t.Fatalf("Unexpected RawResources error %v", err) } @@ -326,11 +326,11 @@ func TestRawResources2(t *testing.T) { }), } l := makeLoader2(t) - app, err := New(l) + app, err := NewApplication(l) if err != nil { t.Fatalf("Unexpected construction error %v", err) } - actual, err := app.RawResources() + actual, err := app.MakeUncustomizedResMap() if err != nil { t.Fatalf("Unexpected RawResources error %v", err) } diff --git a/pkg/commands/build.go b/pkg/commands/build.go index 3429516e2..bedfcb6a6 100644 --- a/pkg/commands/build.go +++ b/pkg/commands/build.go @@ -81,12 +81,12 @@ func (o *buildOptions) RunBuild(out, errOut io.Writer, fs fs.FileSystem) error { return err } - application, err := app.New(rootLoader) + application, err := app.NewApplication(rootLoader) if err != nil { return err } - allResources, err := application.Resources() + allResources, err := application.MakeCustomizedResMap() if err != nil { return err diff --git a/pkg/commands/diff.go b/pkg/commands/diff.go index 4ca349afc..8c41fc896 100644 --- a/pkg/commands/diff.go +++ b/pkg/commands/diff.go @@ -65,7 +65,7 @@ func (o *diffOptions) Validate(cmd *cobra.Command, args []string) error { return nil } -// RunDiff gets the differences between Application.Resources() and Application.RawResources(). +// RunDiff gets the differences between Application.MakeCustomizedResMap() and Application.MakeUncustomizedResMap(). func (o *diffOptions) RunDiff(out, errOut io.Writer, fs fs.FileSystem) error { l := loader.Init([]loader.SchemeLoader{loader.NewFileLoader(fs)}) @@ -80,15 +80,15 @@ func (o *diffOptions) RunDiff(out, errOut io.Writer, fs fs.FileSystem) error { return err } - application, err := app.New(rootLoader) + application, err := app.NewApplication(rootLoader) if err != nil { return err } - transformedResources, err := application.Resources() + transformedResources, err := application.MakeCustomizedResMap() if err != nil { return err } - rawResources, err := application.RawResources() + rawResources, err := application.MakeUncustomizedResMap() if err != nil { return err } diff --git a/pkg/resmap/resmap.go b/pkg/resmap/resmap.go index 9bc98ebd9..98b8629c1 100644 --- a/pkg/resmap/resmap.go +++ b/pkg/resmap/resmap.go @@ -107,7 +107,7 @@ func (m ResMap) insert(newName string, obj *unstructured.Unstructured) error { return nil } -// NewResourceSliceFromPatches returns a slice of Resources given a patch path slice from kustomization file. +// NewResourceSliceFromPatches returns a slice of resources given a patch path slice from a kustomization file. func NewResourceSliceFromPatches( loader loader.Loader, paths []string) ([]*resource.Resource, error) { result := []*resource.Resource{} @@ -140,7 +140,7 @@ func NewResMapFromFiles(loader loader.Loader, paths []string) (ResMap, error) { } result = append(result, res) } - return Merge(result...) + return MergeWithoutOverride(result...) } // newResMapFromBytes decodes a list of objects in byte array format. @@ -192,27 +192,26 @@ func newResourceSliceFromBytes(in []byte) ([]*resource.Resource, error) { return result, nil } -// Merge combines many maps to one. -func Merge(maps ...ResMap) (ResMap, error) { +// MergeWithoutOverride combines multiple ResMap instances, failing on key collision. +func MergeWithoutOverride(maps ...ResMap) (ResMap, error) { result := ResMap{} for _, m := range maps { - for id, obj := range m { + for id, resource := range m { if _, found := result[id]; found { - return nil, fmt.Errorf("there is already an entry: %q", id) + return nil, fmt.Errorf("id '%q' already used", id) } - result[id] = obj + result[id] = resource } } - return result, nil } -// MergeWithOverride merges the entries in the ResMap slice with Override. -// If there is already an entry with the same Id , different actions are performed -// according to value of behavior field: -// 'create': create a new one; -// 'replace': replace the data only; keep the labels and annotations -// 'merge': merge the data; keep the labels and annotations +// MergeWithOverride combines multiple ResMap instances, allowing and sometimes +// demanding certain collisions. +// When looping over the instances to combine them, if a resource id for resource X +// is found to be already in the combined map, then the behavior field for X +// must be BehaviorMerge or BehaviorReplace. If X is not in the map, then it's +// behavior cannot be merge or replace. func MergeWithOverride(maps ...ResMap) (ResMap, error) { result := ResMap{} for _, m := range maps { @@ -220,14 +219,14 @@ func MergeWithOverride(maps ...ResMap) (ResMap, error) { if _, found := result[id]; found { switch r.Behavior() { case resource.BehaviorReplace: - glog.V(4).Infof("Replace object %v by %v", result[id].Object, r.Object) + glog.V(4).Infof("Replace %v with %v", result[id].Object, r.Object) r.Replace(result[id]) result[id] = r case resource.BehaviorMerge: - glog.V(4).Infof("Merge object %v with %v", result[id].Object, r.Object) + glog.V(4).Infof("Merging %v with %v", result[id].Object, r.Object) r.Merge(result[id]) result[id] = r - glog.V(4).Infof("The merged object is %v", result[id].Object) + glog.V(4).Infof("Merged object is %v", result[id].Object) default: return nil, fmt.Errorf("Id %#v exists; must merge or replace.", id) } diff --git a/pkg/resmap/resmap_test.go b/pkg/resmap/resmap_test.go index 4d7041df7..187a7c21c 100644 --- a/pkg/resmap/resmap_test.go +++ b/pkg/resmap/resmap_test.go @@ -191,7 +191,7 @@ func TestMerge(t *testing.T) { }, }), } - merged, err := Merge(input...) + merged, err := MergeWithoutOverride(input...) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/pkg/transformers/patch.go b/pkg/transformers/patch.go index 3eb199e53..9b01988b0 100644 --- a/pkg/transformers/patch.go +++ b/pkg/transformers/patch.go @@ -85,7 +85,7 @@ func (pt *patchTransformer) Transform(baseResourceMap resmap.ResMap) error { case err != nil: return err default: - // Use Strategic Merge Patch to handle types w/ schema + // Use Strategic-Merge-Patch to handle types w/ schema // TODO: Change this to use the new Merge package. // Store the name of the base object, because this name may have been munged. // Apply this name to the StrategicMergePatched object. diff --git a/pkg/transformers/patchconflictdetector.go b/pkg/transformers/patchconflictdetector.go index 4c9dca7e2..cefc74d7b 100644 --- a/pkg/transformers/patchconflictdetector.go +++ b/pkg/transformers/patchconflictdetector.go @@ -19,8 +19,7 @@ package transformers import ( "encoding/json" - "github.com/evanphx/json-patch" - + jsonpatch "github.com/evanphx/json-patch" "github.com/kubernetes-sigs/kustomize/pkg/resource" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/mergepatch"