diff --git a/v1/bundle/store.go b/v1/bundle/store.go index f9576c13f8..f203f7086b 100644 --- a/v1/bundle/store.go +++ b/v1/bundle/store.go @@ -12,6 +12,8 @@ import ( "fmt" "maps" "path/filepath" + "slices" + "sort" "strings" "sync" @@ -1059,32 +1061,40 @@ func lookup(path storage.Path, data map[string]any) (any, bool) { return value, ok } -func hasRootsOverlap(ctx context.Context, store storage.Store, txn storage.Transaction, bundles map[string]*Bundle) error { - collisions := map[string][]string{} - allBundles, err := ReadBundleNamesFromStore(ctx, store, txn) +func hasRootsOverlap(ctx context.Context, store storage.Store, txn storage.Transaction, newBundles map[string]*Bundle) error { + storeBundles, err := ReadBundleNamesFromStore(ctx, store, txn) if suppressNotFound(err) != nil { return err } allRoots := map[string][]string{} + bundlesWithEmptyRoots := map[string]bool{} // Build a map of roots for existing bundles already in the system - for _, name := range allBundles { + for _, name := range storeBundles { roots, err := ReadBundleRootsFromStore(ctx, store, txn, name) if suppressNotFound(err) != nil { return err } allRoots[name] = roots + if slices.Contains(roots, "") { + bundlesWithEmptyRoots[name] = true + } } // Add in any bundles that are being activated, overwrite existing roots // with new ones where bundles are in both groups. - for name, bundle := range bundles { + for name, bundle := range newBundles { allRoots[name] = *bundle.Manifest.Roots + if slices.Contains(*bundle.Manifest.Roots, "") { + bundlesWithEmptyRoots[name] = true + } } // Now check for each new bundle if it conflicts with any of the others - for name, bundle := range bundles { + collidingBundles := map[string]bool{} + conflictSet := map[string]bool{} + for name, bundle := range newBundles { for otherBundle, otherRoots := range allRoots { if name == otherBundle { // Skip the current bundle being checked @@ -1094,22 +1104,41 @@ func hasRootsOverlap(ctx context.Context, store storage.Store, txn storage.Trans // Compare the "new" roots with other existing (or a different bundles new roots) for _, newRoot := range *bundle.Manifest.Roots { for _, otherRoot := range otherRoots { - if RootPathsOverlap(newRoot, otherRoot) { - collisions[otherBundle] = append(collisions[otherBundle], newRoot) + if !RootPathsOverlap(newRoot, otherRoot) { + continue + } + + collidingBundles[name] = true + collidingBundles[otherBundle] = true + + // Different message required if the roots are same + if newRoot == otherRoot { + conflictSet[fmt.Sprintf("root %s is in multiple bundles", newRoot)] = true + } else { + paths := []string{newRoot, otherRoot} + sort.Strings(paths) + conflictSet[fmt.Sprintf("%s overlaps %s", paths[0], paths[1])] = true } } } } } - if len(collisions) > 0 { - var bundleNames []string - for name := range collisions { - bundleNames = append(bundleNames, name) - } - return fmt.Errorf("detected overlapping roots in bundle manifest with: %s", bundleNames) + if len(collidingBundles) == 0 { + return nil } - return nil + + bundleNames := strings.Join(util.KeysSorted(collidingBundles), ", ") + + if len(bundlesWithEmptyRoots) > 0 { + return fmt.Errorf( + "bundles [%s] have overlapping roots and cannot be activated simultaneously because bundle(s) [%s] specify empty root paths ('') which overlap with any other bundle root", + bundleNames, + strings.Join(util.KeysSorted(bundlesWithEmptyRoots), ", "), + ) + } + + return fmt.Errorf("detected overlapping roots in manifests for these bundles: [%s] (%s)", bundleNames, strings.Join(util.KeysSorted(conflictSet), ", ")) } func applyPatches(ctx context.Context, store storage.Store, txn storage.Transaction, patches []PatchOperation) error { diff --git a/v1/bundle/store_test.go b/v1/bundle/store_test.go index c04b445824..517e4d0106 100644 --- a/v1/bundle/store_test.go +++ b/v1/bundle/store_test.go @@ -6621,52 +6621,78 @@ func TestHasRootsOverlap(t *testing.T) { ctx := context.Background() cases := []struct { - note string - storeRoots map[string]*[]string - bundleRoots map[string]*[]string - overlaps bool + note string + storeRoots map[string]*[]string + newBundleRoots map[string]*[]string + expectedError string }{ { - note: "no overlap with existing roots", - storeRoots: map[string]*[]string{"bundle1": {"a", "b"}}, - bundleRoots: map[string]*[]string{"bundle2": {"c"}}, - overlaps: false, + note: "no overlap between store and new bundles", + storeRoots: map[string]*[]string{"bundle1": {"a", "b"}}, + newBundleRoots: map[string]*[]string{"bundle2": {"c"}}, + }, + { + note: "no overlap between store and multiple new bundles", + storeRoots: map[string]*[]string{"bundle1": {"a", "b"}}, + newBundleRoots: map[string]*[]string{"bundle2": {"c"}, "bundle3": {"d"}}, + }, + { + note: "no overlap with empty store", + storeRoots: map[string]*[]string{}, + newBundleRoots: map[string]*[]string{"bundle1": {"a", "b"}}, + }, + { + note: "no overlap between multiple new bundles with empty store", + storeRoots: map[string]*[]string{}, + newBundleRoots: map[string]*[]string{"bundle1": {"a", "b"}, "bundle2": {"c"}}, + }, + { + note: "overlap between multiple new bundles with empty store", + storeRoots: map[string]*[]string{}, + newBundleRoots: map[string]*[]string{"bundle1": {"a", "b"}, "bundle2": {"a", "c"}}, + expectedError: "detected overlapping roots in manifests for these bundles: [bundle1, bundle2] (root a is in multiple bundles)", }, { - note: "no overlap with existing roots multiple bundles", - storeRoots: map[string]*[]string{"bundle1": {"a", "b"}}, - bundleRoots: map[string]*[]string{"bundle2": {"c"}, "bundle3": {"d"}}, - overlaps: false, + note: "overlap between store and new bundle", + storeRoots: map[string]*[]string{"bundle1": {"a", "b"}}, + newBundleRoots: map[string]*[]string{"bundle2": {"c", "a"}}, + expectedError: "detected overlapping roots in manifests for these bundles: [bundle1, bundle2] (root a is in multiple bundles)", }, { - note: "no overlap no existing roots", - storeRoots: map[string]*[]string{}, - bundleRoots: map[string]*[]string{"bundle1": {"a", "b"}}, - overlaps: false, + note: "overlap between store and multiple new bundles", + storeRoots: map[string]*[]string{"bundle1": {"a", "b"}}, + newBundleRoots: map[string]*[]string{"bundle2": {"c", "a"}, "bundle3": {"a"}}, + expectedError: "detected overlapping roots in manifests for these bundles: [bundle1, bundle2, bundle3] (root a is in multiple bundles)", }, { - note: "no overlap without existing roots multiple bundles", - storeRoots: map[string]*[]string{}, - bundleRoots: map[string]*[]string{"bundle1": {"a", "b"}, "bundle2": {"c"}}, - overlaps: false, + note: "overlap between store bundle and new empty root bundle", + storeRoots: map[string]*[]string{"bundle1": {"a", "b"}}, + newBundleRoots: map[string]*[]string{"bundle2": {""}}, + expectedError: "bundles [bundle1, bundle2] have overlapping roots and cannot be activated simultaneously because bundle(s) [bundle2] specify empty root paths ('') which overlap with any other bundle root", }, { - note: "overlap without existing roots multiple bundles", - storeRoots: map[string]*[]string{}, - bundleRoots: map[string]*[]string{"bundle1": {"a", "b"}, "bundle2": {"a", "c"}}, - overlaps: true, + note: "overlap between multiple new empty root bundles", + storeRoots: map[string]*[]string{}, + newBundleRoots: map[string]*[]string{"bundle1": {""}, "bundle2": {""}}, + expectedError: "bundles [bundle1, bundle2] have overlapping roots and cannot be activated simultaneously because bundle(s) [bundle1, bundle2] specify empty root paths ('') which overlap with any other bundle root", }, { - note: "overlap with existing roots", - storeRoots: map[string]*[]string{"bundle1": {"a", "b"}}, - bundleRoots: map[string]*[]string{"bundle2": {"c", "a"}}, - overlaps: true, + note: "overlap between new empty root and new regular root bundles", + storeRoots: map[string]*[]string{}, + newBundleRoots: map[string]*[]string{"bundle1": {"a"}, "bundle2": {""}}, + expectedError: "bundles [bundle1, bundle2] have overlapping roots and cannot be activated simultaneously because bundle(s) [bundle2] specify empty root paths ('') which overlap with any other bundle root", }, { - note: "overlap with existing roots multiple bundles", - storeRoots: map[string]*[]string{"bundle1": {"a", "b"}}, - bundleRoots: map[string]*[]string{"bundle2": {"c", "a"}, "bundle3": {"a"}}, - overlaps: true, + note: "overlap between nested paths", + storeRoots: map[string]*[]string{}, + newBundleRoots: map[string]*[]string{"bundle1": {"a"}, "bundle2": {"a/b"}}, + expectedError: "detected overlapping roots in manifests for these bundles: [bundle1, bundle2] (a overlaps a/b)", + }, + { + note: "overlap between store nested path and new bundle path", + storeRoots: map[string]*[]string{"bundle1": {"a/b"}}, + newBundleRoots: map[string]*[]string{"bundle2": {"a"}}, + expectedError: "detected overlapping roots in manifests for these bundles: [bundle1, bundle2] (a overlaps a/b)", }, } @@ -6683,7 +6709,7 @@ func TestHasRootsOverlap(t *testing.T) { } bundles := map[string]*Bundle{} - for name, roots := range tc.bundleRoots { + for name, roots := range tc.newBundleRoots { bundles[name] = &Bundle{ Manifest: Manifest{ Roots: roots, @@ -6692,10 +6718,15 @@ func TestHasRootsOverlap(t *testing.T) { } err := hasRootsOverlap(ctx, mockStore, txn, bundles) - if !tc.overlaps && err != nil { - t.Fatalf("unepected error: %s", err) - } else if tc.overlaps && (err == nil || !strings.Contains(err.Error(), "detected overlapping roots in bundle manifest")) { - t.Fatalf("expected overlapping roots error, got: %s", err) + if tc.expectedError != "" { + if err == nil { + t.Fatalf("expected error %q, got nil", tc.expectedError) + } + if err.Error() != tc.expectedError { + t.Fatalf("expected error message %q, got %q", tc.expectedError, err.Error()) + } + } else if err != nil { + t.Fatalf("unexpected error: %s", err) } err = mockStore.Commit(ctx, txn) diff --git a/v1/plugins/bundle/plugin_test.go b/v1/plugins/bundle/plugin_test.go index 78a3afa15b..9d19613d4c 100644 --- a/v1/plugins/bundle/plugin_test.go +++ b/v1/plugins/bundle/plugin_test.go @@ -3282,7 +3282,7 @@ func ensureBundleOverlapStatus(t *testing.T, p *Plugin, bundleNames []string, ex t.Fatalf("expected bundle %s to be in an error state", name) } else if !expectedErrs[i] && hasErr { t.Fatalf("unexpected error state for bundle %s", name) - } else if hasErr && expectedErrs[i] && !strings.Contains(p.status[name].Message, "detected overlapping roots") { + } else if hasErr && expectedErrs[i] && !strings.Contains(p.status[name].Message, "overlapping roots") { t.Fatalf("expected bundle overlap error for bundle %s, got: %s", name, p.status[name].Message) } } @@ -7209,8 +7209,8 @@ result := true`, if status.Code != errCode { t.Fatalf("Expected status code to be %s, found %s", errCode, status.Code) } - if !strings.Contains(status.Message, "detected overlapping") { - t.Fatalf(`Expected status message to contain "detected overlapping roots", found %s`, status.Message) + if !strings.Contains(status.Message, "specify empty root paths") { + t.Fatalf(`Expected status message to contain "specify empty root paths", found %s`, status.Message) } }