Fix for policy manager not setting labels in some cases (#517)

* cli: fixed 'kopia policy rm' deleting global when passed policy ID

* policy: additional unit test coverage for policy manager

* fixed path parsing logic to avoid the use filepath package which is platform-dependent, added more tests
This commit is contained in:
Jarek Kowalski
2020-08-06 21:25:48 -07:00
committed by GitHub
parent 8532e843cc
commit 04fdd0105e
5 changed files with 480 additions and 34 deletions

View File

@@ -11,6 +11,7 @@
policyRemoveCommand = policyCommands.Command("remove", "Remove snapshot policy for a single directory, user@host or a global policy.").Alias("rm").Alias("delete")
policyRemoveTargets = policyRemoveCommand.Arg("target", "Target of a policy ('global','user@host','@host') or a path").Strings()
policyRemoveGlobal = policyRemoveCommand.Flag("global", "Set global policy").Bool()
policyRemoveDryRun = policyRemoveCommand.Flag("dry-run", "Do not remove").Short('n').Bool()
)
func init() {
@@ -26,6 +27,10 @@ func removePolicy(ctx context.Context, rep repo.Repository) error {
for _, target := range targets {
log(ctx).Infof("Removing policy on %q...", target)
if *policyRemoveDryRun {
continue
}
if err := policy.RemovePolicy(ctx, rep, target); err != nil {
return err
}

1
go.mod
View File

@@ -17,6 +17,7 @@ require (
github.com/gofrs/flock v0.7.1
github.com/golang/protobuf v1.4.2
github.com/google/fswalker v0.2.1-0.20200214223026-f0e929ba4126
github.com/google/go-cmp v0.4.0
github.com/google/readahead v0.0.0-20161222183148-eaceba169032 // indirect
github.com/google/wire v0.4.0 // indirect
github.com/gorilla/mux v1.7.4

View File

@@ -3,7 +3,6 @@
import (
"context"
"path/filepath"
"strings"
"github.com/pkg/errors"
@@ -36,7 +35,7 @@ func GetEffectivePolicy(ctx context.Context, rep repo.Repository, si snapshot.So
md = append(md, manifests...)
parentPath := filepath.Dir(tmp.Path)
parentPath := getParentPathOSIndependent(tmp.Path)
if parentPath == tmp.Path {
break
}
@@ -72,11 +71,10 @@ func GetEffectivePolicy(ctx context.Context, rep repo.Repository, si snapshot.So
for _, em := range md {
p := &Policy{}
if _, err := rep.GetManifest(ctx, em.ID, &p); err != nil {
if err := loadPolicyFromManifest(ctx, rep, em.ID, p); err != nil {
return nil, nil, errors.Wrapf(err, "got unexpected error when loading policy item %v", em.ID)
}
p.Labels = em.Labels
policies = append(policies, p)
log(ctx).Debugf("loaded parent policy for %v: %v", si, p.Target())
}
@@ -106,18 +104,10 @@ func GetDefinedPolicy(ctx context.Context, rep repo.Repository, si snapshot.Sour
p := &Policy{}
em, err := rep.GetManifest(ctx, manifestID, p)
if err != nil {
if err == manifest.ErrNotFound {
return nil, ErrPolicyNotFound
}
if err := loadPolicyFromManifest(ctx, rep, manifestID, p); err != nil {
return nil, err
}
p.Labels = em.Labels
return p, nil
}
@@ -160,10 +150,8 @@ func RemovePolicy(ctx context.Context, rep repo.Repository, si snapshot.SourceIn
// GetPolicyByID gets the policy for a given unique ID or ErrPolicyNotFound if not found.
func GetPolicyByID(ctx context.Context, rep repo.Repository, id manifest.ID) (*Policy, error) {
p := &Policy{}
if _, err := rep.GetManifest(ctx, id, &p); err != nil {
if err == manifest.ErrNotFound {
return nil, ErrPolicyNotFound
}
if err := loadPolicyFromManifest(ctx, rep, id, p); err != nil {
return nil, err
}
return p, nil
@@ -183,13 +171,10 @@ func ListPolicies(ctx context.Context, rep repo.Repository) ([]*Policy, error) {
for _, id := range ids {
pol := &Policy{}
md, err := rep.GetManifest(ctx, id.ID, pol)
if err != nil {
if err := loadPolicyFromManifest(ctx, rep, id.ID, pol); err != nil {
return nil, err
}
pol.Labels = md.Labels
pol.Labels["id"] = string(id.ID)
policies = append(policies, pol)
}
@@ -206,6 +191,15 @@ func (m SubdirectoryPolicyMap) GetPolicyForPath(relativePath string) (*Policy, e
// TreeForSource returns policy Tree for a given source.
func TreeForSource(ctx context.Context, rep repo.Repository, si snapshot.SourceInfo) (*Tree, error) {
pols, err := applicablePoliciesForSource(ctx, rep, si)
if err != nil {
return nil, errors.Wrap(err, "unable to get policies")
}
return BuildTree(pols, DefaultPolicy), nil
}
func applicablePoliciesForSource(ctx context.Context, rep repo.Repository, si snapshot.SourceInfo) (map[string]*Policy, error) {
result := map[string]*Policy{}
pol, _, err := GetEffectivePolicy(ctx, rep, si)
@@ -226,34 +220,47 @@ func TreeForSource(ctx context.Context, rep repo.Repository, si snapshot.SourceI
return nil, errors.Wrapf(err, "unable to find manifests for %v@%v", si.UserName, si.Host)
}
log(ctx).Debugf("found %v policies for %v@%v", si.UserName, si.Host)
log(ctx).Debugf("found %v policies for %v@%v", len(policies), si.UserName, si.Host)
for _, id := range policies {
pol := &Policy{}
em, err := rep.GetManifest(ctx, id.ID, pol)
err := loadPolicyFromManifest(ctx, rep, id.ID, pol)
if err != nil {
return nil, err
}
policyPath := em.Labels["path"]
policyPath := pol.Labels["path"]
if !strings.HasPrefix(policyPath, si.Path+"/") {
rel := nestedRelativePathNormalizedToSlashes(si.Path, policyPath)
if rel == "" {
log(ctx).Debugf("%v is not under %v", policyPath, si.Path)
continue
}
rel, err := filepath.Rel(si.Path, policyPath)
if err != nil {
return nil, errors.Wrap(err, "unable to determine relative path")
}
rel = "./" + rel
log(ctx).Debugf("loading policy for %v (%v)", policyPath, rel)
result[rel] = pol
}
return BuildTree(result, DefaultPolicy), nil
return result, nil
}
func loadPolicyFromManifest(ctx context.Context, rep repo.Repository, id manifest.ID, pol *Policy) error {
md, err := rep.GetManifest(ctx, id, pol)
if err != nil {
if errors.Is(err, manifest.ErrNotFound) {
return ErrPolicyNotFound
}
return err
}
pol.Labels = md.Labels
pol.Labels["id"] = string(md.ID)
return nil
}
func labelsForSource(si snapshot.SourceInfo) map[string]string {
@@ -286,3 +293,65 @@ func labelsForSource(si snapshot.SourceInfo) map[string]string {
}
}
}
func getParentPathOSIndependent(p string) string {
// split into volume (Windows only, e.g. X:) and path using either slash or backslash.
vol, pth := volumeAndPath(p)
last := strings.LastIndexAny(pth, "/\\")
if last == len(pth)-1 && last != 0 {
pth = pth[0:last]
last = strings.LastIndexAny(pth, "/\\")
}
if last < 0 {
return p
}
// special case for root, return root path itself (either slash or backslash-separated)
if last == 0 {
return vol + pth[0:1]
}
return vol + pth[0:last]
}
// volumeAndPath splits path 'p' into Windows-specific volume (e.g. "X:" and path after that starting with either slash or backslash)
func volumeAndPath(p string) (vol, path string) {
if len(p) >= 3 && p[1] == ':' && isSlashOrBackslash(p[2]) {
// "X:\"
return p[0:2], p[2:]
}
return "", p
}
func isSlashOrBackslash(c uint8) bool {
return c == '/' || c == '\\'
}
func isWindowsStylePath(p string) bool {
v, _ := volumeAndPath(p)
return v != ""
}
func trimTrailingSlashOrBackslash(path string) string {
return strings.TrimSuffix(strings.TrimSuffix(path, "/"), "\\")
}
func nestedRelativePathNormalizedToSlashes(parent, child string) string {
isWin := isWindowsStylePath(parent)
parent = trimTrailingSlashOrBackslash(parent)
if !strings.HasPrefix(child, parent+"/") && !strings.HasPrefix(child, parent+"\\") {
return ""
}
p := strings.TrimPrefix(child, parent)[1:]
if isWin {
return strings.ReplaceAll(p, "\\", "/")
}
return p
}

View File

@@ -2,12 +2,170 @@
import (
"context"
"encoding/json"
"fmt"
"sort"
"testing"
"github.com/google/go-cmp/cmp"
"github.com/kopia/kopia/internal/repotesting"
"github.com/kopia/kopia/internal/testlogging"
"github.com/kopia/kopia/snapshot"
)
func TestPolicyManager(t *testing.T) {
func TestPolicyManagerInheritanceTest(t *testing.T) {
ctx := context.Background()
var env repotesting.Environment
defer env.Setup(t).Close(ctx, t)
defaultPolicyWithLabels := policyWithLabels(DefaultPolicy, map[string]string{
"policyType": "global",
"type": "policy",
})
must(t, SetPolicy(ctx, env.Repository, snapshot.SourceInfo{
Host: "host-a",
}, &Policy{
RetentionPolicy: RetentionPolicy{
KeepDaily: intPtr(44),
},
}))
must(t, SetPolicy(ctx, env.Repository, snapshot.SourceInfo{
Host: "host-a",
UserName: "myuser",
Path: "/some/path2",
}, &Policy{
RetentionPolicy: RetentionPolicy{
KeepDaily: intPtr(66),
},
}))
must(t, SetPolicy(ctx, env.Repository, snapshot.SourceInfo{
Host: "host-b",
}, &Policy{
RetentionPolicy: RetentionPolicy{
KeepDaily: intPtr(55),
},
}))
cases := []struct {
sourceInfo snapshot.SourceInfo
wantEffective *Policy
wantSources []snapshot.SourceInfo
}{
{
sourceInfo: GlobalPolicySourceInfo,
wantEffective: defaultPolicyWithLabels,
},
{
sourceInfo: snapshot.SourceInfo{
UserName: "myuser",
Host: "host-c",
Path: "/some/path",
},
wantEffective: policyWithLabels(DefaultPolicy, map[string]string{
"type": "policy",
"policyType": "path",
"hostname": "host-c",
"path": "/some/path",
"username": "myuser",
}),
},
{
sourceInfo: snapshot.SourceInfo{
UserName: "myuser",
Host: "host-a",
Path: "/some/path",
},
wantEffective: policyWithLabels(defaultPolicyWithKeepDaily(t, 44), map[string]string{
"type": "policy",
"policyType": "path",
"hostname": "host-a",
"path": "/some/path",
"username": "myuser",
}),
wantSources: []snapshot.SourceInfo{
{Host: "host-a"},
},
},
{
sourceInfo: snapshot.SourceInfo{
UserName: "myuser",
Host: "host-a",
Path: "/some/path2/nested",
},
wantEffective: policyWithLabels(defaultPolicyWithKeepDaily(t, 66), map[string]string{
"type": "policy",
"policyType": "path",
"hostname": "host-a",
"path": "/some/path2/nested",
"username": "myuser",
}),
wantSources: []snapshot.SourceInfo{
{UserName: "myuser", Path: "/some/path2", Host: "host-a"},
{Host: "host-a"},
},
},
}
for _, tc := range cases {
tc := tc
t.Run(fmt.Sprintf("%v", tc.sourceInfo), func(t *testing.T) {
pol, src, err := GetEffectivePolicy(ctx, env.Repository, tc.sourceInfo)
if err != nil {
t.Fatalf("err: %v", err)
}
if diff := cmp.Diff(pol, tc.wantEffective); diff != "" {
t.Errorf("got: %v", pol)
t.Errorf("want: %v", tc.wantEffective)
t.Errorf("invalid effective policy: %v", diff)
}
var sources []snapshot.SourceInfo
for _, s := range src {
sources = append(sources, s.Target())
}
if diff := cmp.Diff(sources, tc.wantSources); diff != "" {
t.Errorf("got: %v", sources)
t.Errorf("want: %v", tc.wantSources)
t.Errorf("invalid sources: %v", diff)
}
})
}
}
func clonePolicy(t *testing.T, p *Policy) *Policy {
j, err := json.Marshal(p)
if err != nil {
t.Fatalf("unable to marshal JSON: %v", err.Error())
}
p2 := &Policy{}
json.Unmarshal(j, &p2)
return p2
}
func policyWithLabels(p *Policy, l map[string]string) *Policy {
p2 := *p
p2.Labels = l
return &p2
}
func defaultPolicyWithKeepDaily(t *testing.T, keepDaily int) *Policy {
p := clonePolicy(t, DefaultPolicy)
p.RetentionPolicy.KeepDaily = &keepDaily
return p
}
func TestPolicyManagerResolvesConflicts(t *testing.T) {
ctx := context.Background()
var env repotesting.Environment
@@ -36,13 +194,226 @@ func TestPolicyManager(t *testing.T) {
r3 := env.MustOpenAnother(t)
pi, err := GetDefinedPolicy(ctx, r3, sourceInfo)
must(t, err)
if got, want := pi.Target(), sourceInfo; got != want {
t.Errorf("invalid policy target %v, want %v", got, want)
}
if got := *pi.RetentionPolicy.KeepDaily; got != 33 && got != 44 {
t.Errorf("unexpected policy returned")
}
}
func TestParentPathOSIndependent(t *testing.T) {
cases := []struct {
input, want string
}{
{"/", "/"},
{"/x", "/"},
{"/x/", "/"},
{"/x/a", "/x"},
{"/x/a/", "/x"},
{"x:\\", "x:\\"},
{"X:\\Program Files", "X:\\"},
{"X:\\Program Files\\Blah", "X:\\Program Files"},
{"X:/Program Files", "X:/"},
{"X:/Program Files/", "X:/"},
{"X:/Program Files\\", "X:/"},
{"X:/Program Files/Blah", "X:/Program Files"},
{"X:/Program Files/Blah/", "X:/Program Files"},
{"X:/Program Files/Blah/xxx", "X:/Program Files/Blah"},
}
for _, tc := range cases {
if got, want := getParentPathOSIndependent(tc.input), tc.want; got != want {
t.Errorf("invalid value of getParentPathOSIndependent(%q): %q, want %q", tc.input, got, want)
}
}
}
// TestApplicablePoliciesForSource verifies that when we build a policy tree, we pick the appropriate policies
// defined for the subtree regardless of path style (Unix or Windows).
func TestApplicablePoliciesForSource(t *testing.T) {
ctx := testlogging.Context(t)
var env repotesting.Environment
defer env.Setup(t).Close(ctx, t)
setPols := map[snapshot.SourceInfo]*Policy{
// unix-style path names
{Host: "host-a"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(0)},
},
{Host: "host-a", UserName: "myuser", Path: "/home"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(1)},
},
{Host: "host-a", UserName: "myuser", Path: "/home/users"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(2)},
},
// on Unix \ a regular character so the directory name is 'user-with\\backslash'
{Host: "host-a", UserName: "myuser", Path: "/home/users/user-with\\backslash"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(2)},
},
{Host: "host-a", UserName: "myuser", Path: "/home/users/user-with\\backslash/x"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(2)},
},
{Host: "host-a", UserName: "myuser", Path: "/home/users/myuser"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(3)},
},
{Host: "host-a", UserName: "myuser", Path: "/home/users/myuser/dir1"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(4)},
},
{Host: "host-a", UserName: "myuser", Path: "/home/users/myuser/dir2"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(5)},
},
{Host: "host-a", UserName: "myuser", Path: "/home/users/myuser/dir2/a"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(6)},
},
{Host: "host-a", UserName: "myuser", Path: "/home/users/myuser2"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(7)},
},
// windows-style path names with backslash
{Host: "host-b"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(0)},
},
{Host: "host-b", UserName: "myuser", Path: "C:\\"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(1)},
},
{Host: "host-b", UserName: "myuser", Path: "C:\\Users"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(2)},
},
{Host: "host-b", UserName: "myuser", Path: "C:\\Users\\myuser"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(3)},
},
{Host: "host-b", UserName: "myuser", Path: "C:\\Users\\myuser\\dir1"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(4)},
},
{Host: "host-b", UserName: "myuser", Path: "C:\\Users\\myuser\\dir2"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(5)},
},
{Host: "host-b", UserName: "myuser", Path: "C:\\Users\\myuser\\dir2\\a"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(6)},
},
{Host: "host-b", UserName: "myuser", Path: "C:\\Users\\myuser2"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(7)},
},
// windows-style path names with slashes
{Host: "host-c"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(0)},
},
{Host: "host-c", UserName: "myuser", Path: "C:/Users"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(1)},
},
{Host: "host-c", UserName: "myuser", Path: "C:/Users"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(2)},
},
{Host: "host-c", UserName: "myuser", Path: "C:/Users/myuser"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(3)},
},
{Host: "host-c", UserName: "myuser", Path: "C:/Users/myuser/dir1"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(4)},
},
{Host: "host-c", UserName: "myuser", Path: "C:/Users/myuser/dir2"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(5)},
},
{Host: "host-c", UserName: "myuser", Path: "C:/Users/myuser/dir2/a"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(6)},
},
{Host: "host-c", UserName: "myuser", Path: "C:/Users/myuser2"}: {
RetentionPolicy: RetentionPolicy{KeepDaily: intPtr(7)},
},
}
for si, pol := range setPols {
must(t, SetPolicy(ctx, env.Repository, si, pol))
}
cases := []struct {
si snapshot.SourceInfo
wantPaths []string
}{
{snapshot.SourceInfo{Host: "host-a", UserName: "myuser", Path: "/tmp"}, []string{"."}},
{snapshot.SourceInfo{Host: "host-a", UserName: "myuser", Path: "/home/users/myuser"},
[]string{".", "./dir1", "./dir2", "./dir2/a"},
},
{snapshot.SourceInfo{Host: "host-a", UserName: "myuser", Path: "/home/users/myuser2"},
[]string{"."},
},
{snapshot.SourceInfo{Host: "host-a", UserName: "myuser", Path: "/home/users/user-with\\backslash"},
[]string{".", "./x"},
},
{snapshot.SourceInfo{Host: "host-a", UserName: "myuser", Path: "/home"},
[]string{
".",
"./users",
"./users/myuser",
"./users/myuser/dir1",
"./users/myuser/dir2",
"./users/myuser/dir2/a",
"./users/myuser2",
`./users/user-with\backslash`,
`./users/user-with\backslash/x`,
},
},
{snapshot.SourceInfo{Host: "host-a", UserName: "myuser", Path: "/"},
[]string{
".",
"./home",
"./home/users",
"./home/users/myuser",
"./home/users/myuser/dir1",
"./home/users/myuser/dir2",
"./home/users/myuser/dir2/a",
"./home/users/myuser2",
`./home/users/user-with\backslash`,
`./home/users/user-with\backslash/x`,
},
},
{snapshot.SourceInfo{Host: "host-b", UserName: "myuser", Path: "C:\\Temp"}, []string{"."}},
{snapshot.SourceInfo{Host: "host-b", UserName: "myuser", Path: "C:\\Users\\myuser"},
[]string{".", "./dir1", "./dir2", "./dir2/a"},
},
{snapshot.SourceInfo{Host: "host-b", UserName: "myuser", Path: "C:\\"},
[]string{
".",
"./Users",
"./Users/myuser",
"./Users/myuser/dir1",
"./Users/myuser/dir2",
"./Users/myuser/dir2/a",
"./Users/myuser2",
},
},
}
for _, tc := range cases {
tc := tc
t.Run(fmt.Sprintf("%v", tc.si), func(t *testing.T) {
res, err := applicablePoliciesForSource(ctx, env.Repository, tc.si)
if err != nil {
t.Fatalf("error in applicablePoliciesForSource(%v): %v", tc.si, err)
}
var relPaths []string
for k := range res {
relPaths = append(relPaths, k)
}
sort.Strings(relPaths)
if diff := cmp.Diff(relPaths, tc.wantPaths); diff != "" {
t.Errorf("invalid sub-policies %v", diff)
}
})
}
}
func must(t *testing.T, err error) {
t.Helper()

View File

@@ -7,7 +7,7 @@
)
var (
defaultPolicy = &Policy{
defPolicy = &Policy{
FilesPolicy: FilesPolicy{
IgnoreRules: []string{"default"},
},
@@ -84,7 +84,7 @@ func TestBuildTree(t *testing.T) {
".": policyA,
"./foo": policyB,
"./bar/baz/bleh": policyC,
}, defaultPolicy)
}, defPolicy)
dumpTree(n, "root")