From 97cf2aac343b418e1007bfc8466f3e504a452c3e Mon Sep 17 00:00:00 2001 From: Thibault NORMAND Date: Tue, 16 Feb 2021 14:26:22 +0100 Subject: [PATCH] fix(bundle): package concurrent iteration problem with duplicate names. --- pkg/bundle/patch/package.go | 21 ++- pkg/bundle/patch/package_test.go | 155 +++++++++++++++++++- test/fixtures/patch/valid/path-cleaner.yaml | 26 ++++ 3 files changed, 196 insertions(+), 6 deletions(-) create mode 100644 test/fixtures/patch/valid/path-cleaner.yaml diff --git a/pkg/bundle/patch/package.go b/pkg/bundle/patch/package.go index 92f3c7d9..1023f41e 100644 --- a/pkg/bundle/patch/package.go +++ b/pkg/bundle/patch/package.go @@ -93,21 +93,32 @@ func Apply(spec *bundlev1.Patch, b *bundlev1.Bundle, values map[string]interface // Copy bundle bCopy := proto.Clone(b).(*bundlev1.Bundle) + // Initialize empty package list + packageList := make([]*bundlev1.Package, 0) + // Process all rules - k := 0 for _, p := range bCopy.Packages { + lastAction := packageUnchanged + for i, r := range spec.Spec.Rules { action, err := executeRule(spec.Meta.Name, r, p, values) if err != nil { return b, fmt.Errorf("unable to execute rule index %d: %w", i, err) } - if action != packagedRemoved { - bCopy.Packages[k] = p - k++ + if action == packagedRemoved { + lastAction = action + break } } + + if lastAction != packagedRemoved { + // Assign package map + packageList = append(packageList, p) + } } - bCopy.Packages = bCopy.Packages[:k] + + // Reassign packages + bCopy.Packages = packageList // No error return bCopy, nil diff --git a/pkg/bundle/patch/package_test.go b/pkg/bundle/patch/package_test.go index e9a84469..31aeb3d7 100644 --- a/pkg/bundle/patch/package_test.go +++ b/pkg/bundle/patch/package_test.go @@ -18,12 +18,36 @@ package patch import ( + "os" "reflect" "testing" + bundlev1 "github.com/elastic/harp/api/gen/go/harp/bundle/v1" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" fuzz "github.com/google/gofuzz" +) - bundlev1 "github.com/elastic/harp/api/gen/go/harp/bundle/v1" +var ( + opt = cmp.FilterPath( + func(p cmp.Path) bool { + // Remove ignoring of the fields below once go-cmp is able to ignore generated fields. + // See https://github.com/google/go-cmp/issues/153 + ignoreXXXCache := + p.String() == "XXX_sizecache" || + p.String() == "Packages.XXX_sizecache" || + p.String() == "Packages.Secrets.XXX_sizecache" || + p.String() == "Packages.Secrets.Data.XXX_sizecache" + return ignoreXXXCache + }, cmp.Ignore()) + + ignoreOpts = []cmp.Option{ + cmpopts.IgnoreUnexported(bundlev1.Bundle{}), + cmpopts.IgnoreUnexported(bundlev1.Package{}), + cmpopts.IgnoreUnexported(bundlev1.SecretChain{}), + cmpopts.IgnoreUnexported(bundlev1.KV{}), + opt, + } ) func TestValidate(t *testing.T) { @@ -199,3 +223,132 @@ func TestApply_Fuzz(t *testing.T) { Apply(spec, &file, values) } } + +func mustLoadPatch(filePath string) *bundlev1.Patch { + f, err := os.Open(filePath) + if err != nil { + panic(err) + } + + p, err := YAML(f) + if err != nil { + panic(err) + } + + return p +} + +func TestApply(t *testing.T) { + type args struct { + spec *bundlev1.Patch + b *bundlev1.Bundle + values map[string]interface{} + } + tests := []struct { + name string + args args + want *bundlev1.Bundle + wantErr bool + }{ + { + name: "nil", + wantErr: true, + }, + { + name: "empty bundle", + args: args{ + spec: mustLoadPatch("../../../test/fixtures/patch/valid/path-cleaner.yaml"), + b: &bundlev1.Bundle{}, + values: map[string]interface{}{}, + }, + wantErr: false, + want: &bundlev1.Bundle{ + Packages: []*bundlev1.Package{}, + }, + }, + { + name: "modifiable bundle", + args: args{ + spec: mustLoadPatch("../../../test/fixtures/patch/valid/path-cleaner.yaml"), + b: &bundlev1.Bundle{ + Packages: []*bundlev1.Package{ + { + Name: "secrets/application/component-1.yaml", + }, + { + Name: "secrets/application/component-2.yaml", + }, + }, + }, + values: map[string]interface{}{}, + }, + wantErr: false, + want: &bundlev1.Bundle{ + Packages: []*bundlev1.Package{ + { + Annotations: map[string]string{ + "patched": "true", + "secret-path-cleaner": "true", + }, + Name: "application/component-1", + }, + { + Annotations: map[string]string{ + "patched": "true", + "secret-path-cleaner": "true", + }, + Name: "application/component-2", + }, + }, + }, + }, + { + name: "duplicate package paths", + args: args{ + spec: mustLoadPatch("../../../test/fixtures/patch/valid/path-cleaner.yaml"), + b: &bundlev1.Bundle{ + Packages: []*bundlev1.Package{ + { + Name: "secrets/application/component-1.yaml", + }, + { + Name: "secrets/application/component-1.yaml", + }, + }, + }, + values: map[string]interface{}{}, + }, + wantErr: false, + want: &bundlev1.Bundle{ + Packages: []*bundlev1.Package{ + { + Annotations: map[string]string{ + "patched": "true", + "secret-path-cleaner": "true", + }, + Name: "application/component-1", + }, + { + Annotations: map[string]string{ + "patched": "true", + "secret-path-cleaner": "true", + }, + Name: "application/component-1", + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := Apply(tt.args.spec, tt.args.b, tt.args.values) + if (err != nil) != tt.wantErr { + t.Errorf("Apply() error = %v, wantErr %v", err, tt.wantErr) + return + } + if diff := cmp.Diff(got, tt.want, ignoreOpts...); diff != "" { + t.Errorf("%q. Patch.Apply():\n-got/+want\ndiff %s", tt.name, diff) + } + }) + } +} diff --git a/test/fixtures/patch/valid/path-cleaner.yaml b/test/fixtures/patch/valid/path-cleaner.yaml new file mode 100644 index 00000000..b8af0808 --- /dev/null +++ b/test/fixtures/patch/valid/path-cleaner.yaml @@ -0,0 +1,26 @@ +apiVersion: harp.elastic.co/v1 +kind: BundlePatch +meta: + name: "secret-path-cleaner" + owner: security@elastic.co + description: "Remove 'secrets/' prefix of imported secrets" +spec: + rules: + - selector: + matchPath: + # All paths that starts with "secrets/" + regex: "^secrets/" + package: + path: + # Remove `secrets/` prefix + template: |- + {{ trimPrefix "secrets/" .Path }} + - selector: + matchPath: + # All paths that ends with ".yaml" + regex: ".yaml$" + package: + path: + # Remove '.yaml' suffix + template: |- + {{ trimSuffix ".yaml" .Path }}