From d12e83d4d64b295b8da5c0506d44e94a330ab272 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 11:41:57 +0200 Subject: [PATCH 01/16] sync: tests: rename triggers to expTriggers --- cmd/safcm-remote/sync/files_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/safcm-remote/sync/files_test.go b/cmd/safcm-remote/sync/files_test.go index bd5eac9..6355011 100644 --- a/cmd/safcm-remote/sync/files_test.go +++ b/cmd/safcm-remote/sync/files_test.go @@ -62,7 +62,7 @@ func TestSyncFiles(t *testing.T) { skip bool req safcm.MsgSyncReq prepare func() - triggers []string + expTriggers []string expFiles []ft.File expResp safcm.MsgSyncResp expDbg []string @@ -869,7 +869,7 @@ func TestSyncFiles(t *testing.T) { testutil.AssertEqual(t, "resp", s.resp, tc.expResp) testutil.AssertEqual(t, "triggers", - s.triggers, tc.triggers) + s.triggers, tc.expTriggers) }) } -- 2.44.2 From c9b032a8c37a1c984127bdd76f0928e4bf5f60ce Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 11:42:31 +0200 Subject: [PATCH 02/16] sync: tests: go fmt --- cmd/safcm-remote/sync/files_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/cmd/safcm-remote/sync/files_test.go b/cmd/safcm-remote/sync/files_test.go index 6355011..11d386d 100644 --- a/cmd/safcm-remote/sync/files_test.go +++ b/cmd/safcm-remote/sync/files_test.go @@ -58,15 +58,15 @@ func TestSyncFiles(t *testing.T) { tmpTestFilePath := "/tmp/safcm-sync-files-test-file" tests := []struct { - name string - skip bool - req safcm.MsgSyncReq - prepare func() + name string + skip bool + req safcm.MsgSyncReq + prepare func() expTriggers []string - expFiles []ft.File - expResp safcm.MsgSyncResp - expDbg []string - expErr error + expFiles []ft.File + expResp safcm.MsgSyncResp + expDbg []string + expErr error }{ // NOTE: Also update MsgSyncResp in safcm test cases when -- 2.44.2 From edc7998408e700323fc7b6f8a9537ac16dfc6c13 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:12:22 +0200 Subject: [PATCH 03/16] sync: tests: properly scope err variable --- cmd/safcm-remote/sync/files_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/safcm-remote/sync/files_test.go b/cmd/safcm-remote/sync/files_test.go index 11d386d..3cf7e04 100644 --- a/cmd/safcm-remote/sync/files_test.go +++ b/cmd/safcm-remote/sync/files_test.go @@ -834,7 +834,7 @@ func TestSyncFiles(t *testing.T) { // Create separate test directory for each test case path := filepath.Join(cwd, "testdata", "files-"+tc.name) - err = os.Mkdir(path, 0700) + err := os.Mkdir(path, 0700) if err != nil { t.Fatal(err) } @@ -852,7 +852,7 @@ func TestSyncFiles(t *testing.T) { }) s.setDefaults() - err := s.syncFiles() + err = s.syncFiles() testutil.AssertErrorEqual(t, "err", err, tc.expErr) dbg := res.Wait() // Remove random file names from result @@ -2374,7 +2374,7 @@ file t.Run(tc.name, func(t *testing.T) { // Create separate test directory for each test case path := filepath.Join(cwd, "testdata", "file-"+tc.name) - err = os.Mkdir(path, 0700) + err := os.Mkdir(path, 0700) if err != nil { t.Fatal(err) } @@ -2396,7 +2396,7 @@ file rand.Seed(0) var changed bool - err := s.syncFile(tc.file, &changed) + err = s.syncFile(tc.file, &changed) testutil.AssertErrorEqual(t, "err", err, tc.expErr) dbg := res.Wait() // Remove random file names from result -- 2.44.2 From 116e5ebec76bbdf4f70ef4b5fc3ebb55cafee719 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:13:37 +0200 Subject: [PATCH 04/16] sync: tests: check return value of setDefaults() --- cmd/safcm-remote/sync/files_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/cmd/safcm-remote/sync/files_test.go b/cmd/safcm-remote/sync/files_test.go index 3cf7e04..5787404 100644 --- a/cmd/safcm-remote/sync/files_test.go +++ b/cmd/safcm-remote/sync/files_test.go @@ -850,7 +850,10 @@ func TestSyncFiles(t *testing.T) { s, res := prepareSync(tc.req, &testRunner{ t: t, }) - s.setDefaults() + err = s.setDefaults() + if err != nil { + t.Fatal(err) + } err = s.syncFiles() testutil.AssertErrorEqual(t, "err", err, tc.expErr) @@ -2390,7 +2393,10 @@ file s, res := prepareSync(tc.req, &testRunner{ t: t, }) - s.setDefaults() + err = s.setDefaults() + if err != nil { + t.Fatal(err) + } // Deterministic temporary symlink names rand.Seed(0) -- 2.44.2 From 006a69a50239a27ba7929c513897ef19259e06af Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:14:16 +0200 Subject: [PATCH 05/16] sync: tests: use CreateDirectoryExists() --- cmd/safcm-remote/sync/files_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cmd/safcm-remote/sync/files_test.go b/cmd/safcm-remote/sync/files_test.go index 5787404..2e3ed3e 100644 --- a/cmd/safcm-remote/sync/files_test.go +++ b/cmd/safcm-remote/sync/files_test.go @@ -415,10 +415,7 @@ func TestSyncFiles(t *testing.T) { }, }, func() { - err = os.Chmod(".", 0750) - if err != nil { - panic(err) - } + ft.CreateDirectoryExists(".", 0750) ft.CreateDirectory("dir", 0755) ft.CreateFile("dir/file", "content\n", 0644) }, -- 2.44.2 From 00e0fb787000a8c2c98942dfa0ed7fc1d7c418af Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:15:42 +0200 Subject: [PATCH 06/16] sync: tests: wrap overlong line --- cmd/safcm-remote/sync/filetest/filetest.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cmd/safcm-remote/sync/filetest/filetest.go b/cmd/safcm-remote/sync/filetest/filetest.go index 794e99c..f0c518c 100644 --- a/cmd/safcm-remote/sync/filetest/filetest.go +++ b/cmd/safcm-remote/sync/filetest/filetest.go @@ -34,7 +34,9 @@ type File struct { func WalkDir(basePath string) ([]File, error) { var res []File - err := filepath.WalkDir(basePath, func(path string, d fs.DirEntry, err error) error { + err := filepath.WalkDir(basePath, func(path string, + d fs.DirEntry, err error) error { + if err != nil { return err } -- 2.44.2 From 6089e2fbed6bf995122f331ceffcfc8c7b9dda31 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:15:58 +0200 Subject: [PATCH 07/16] sync: tests: use strict perm for os.WriteFile() in CreateFile() This parameter is modified by the umask. The proper permissions are set by os.Chmod() afterwards. Don't confuse the reader by using a value which is not relevant. --- cmd/safcm-remote/sync/filetest/filetest.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/safcm-remote/sync/filetest/filetest.go b/cmd/safcm-remote/sync/filetest/filetest.go index f0c518c..370ca10 100644 --- a/cmd/safcm-remote/sync/filetest/filetest.go +++ b/cmd/safcm-remote/sync/filetest/filetest.go @@ -97,7 +97,7 @@ func CurrentUserAndGroup() (string, int, string, int) { } func CreateFile(path string, data string, mode fs.FileMode) { - err := os.WriteFile(path, []byte(data), 0644) + err := os.WriteFile(path, []byte(data), 0600) if err != nil { panic(err) } -- 2.44.2 From 778665c07c9cf6f434dbd1ee867fe8b8636f55c0 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:20:39 +0200 Subject: [PATCH 08/16] sync: tests: use "..." instead of `...` for regular strings --- cmd/safcm-remote/sync/services_systemd_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/safcm-remote/sync/services_systemd_test.go b/cmd/safcm-remote/sync/services_systemd_test.go index 0460bb7..f4f7f49 100644 --- a/cmd/safcm-remote/sync/services_systemd_test.go +++ b/cmd/safcm-remote/sync/services_systemd_test.go @@ -224,7 +224,7 @@ LoadError= nil, nil, nil, - []byte(`fake stderr`), + []byte("fake stderr"), }, []error{nil, nil, nil, nil}, []*exec.Cmd{&exec.Cmd{ @@ -421,7 +421,7 @@ LoadError= [][]byte{ nil, nil, - []byte(`fake stderr`), + []byte("fake stderr"), }, []error{ nil, -- 2.44.2 From 6d78cccd689fd81a08851b95a03fea608f497e16 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:24:12 +0200 Subject: [PATCH 09/16] sync: tests: use variable to reduce line wrapping --- cmd/safcm-remote/sync/sync_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/cmd/safcm-remote/sync/sync_test.go b/cmd/safcm-remote/sync/sync_test.go index 6f8025a..52d7435 100644 --- a/cmd/safcm-remote/sync/sync_test.go +++ b/cmd/safcm-remote/sync/sync_test.go @@ -80,10 +80,8 @@ func (r *testRunner) check(method string, cmd *exec.Cmd) ( exp := r.expCmds[0] r.expCmds = r.expCmds[1:] if !reflect.DeepEqual(exp, cmd) { - r.t.Errorf("%s: %s", method, - cmp.Diff(exp, cmd, cmpopts.IgnoreUnexported( - exec.Cmd{}, - bytes.Buffer{}))) + opts := cmpopts.IgnoreUnexported(exec.Cmd{}, bytes.Buffer{}) + r.t.Errorf("%s: %s", method, cmp.Diff(exp, cmd, opts)) } var stdout, stderr []byte -- 2.44.2 From bacf1f2d844cd8b917ad0d38e0d3ba01d74dcae8 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:27:28 +0200 Subject: [PATCH 10/16] sync: include size in binary "diff" --- cmd/safcm-remote/sync/files.go | 9 ++++++--- cmd/safcm-remote/sync/files_test.go | 6 +++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/cmd/safcm-remote/sync/files.go b/cmd/safcm-remote/sync/files.go index 119a353..3e88be2 100644 --- a/cmd/safcm-remote/sync/files.go +++ b/cmd/safcm-remote/sync/files.go @@ -452,13 +452,16 @@ func diffData(oldData []byte, newData []byte) (string, error) { oldBin := !strings.HasPrefix(http.DetectContentType(oldData), "text/") newBin := !strings.HasPrefix(http.DetectContentType(newData), "text/") if oldBin && newBin { - return "Binary files differ, cannot show diff", nil + return fmt.Sprintf("Binary files differ (%d -> %d bytes), "+ + "cannot show diff", len(oldData), len(newData)), nil } if oldBin { - oldData = []byte("\n") + oldData = []byte(fmt.Sprintf("\n", + len(oldData))) } if newBin { - newData = []byte("\n") + newData = []byte(fmt.Sprintf("\n", + len(newData))) } // TODO: difflib shows empty context lines at the end of the file diff --git a/cmd/safcm-remote/sync/files_test.go b/cmd/safcm-remote/sync/files_test.go index 2e3ed3e..27011ac 100644 --- a/cmd/safcm-remote/sync/files_test.go +++ b/cmd/safcm-remote/sync/files_test.go @@ -2239,7 +2239,7 @@ file Group: group, Gid: gid, }, - DataDiff: "Binary files differ, cannot show diff", + DataDiff: "Binary files differ (3 -> 4 bytes), cannot show diff", }, }, }, @@ -2295,7 +2295,7 @@ file Gid: gid, }, DataDiff: `@@ -1,2 +1,2 @@ -- +- +content `, @@ -2355,7 +2355,7 @@ file }, DataDiff: `@@ -1,2 +1,2 @@ -content -+ ++ `, }, -- 2.44.2 From 2d8cd51379f293fe2047c1347b52f8dc7ac3f78d Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:28:33 +0200 Subject: [PATCH 11/16] sync: remove duplicate code in triggerPaths() --- cmd/safcm-remote/sync/triggers.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cmd/safcm-remote/sync/triggers.go b/cmd/safcm-remote/sync/triggers.go index 34aae7d..d201835 100644 --- a/cmd/safcm-remote/sync/triggers.go +++ b/cmd/safcm-remote/sync/triggers.go @@ -51,9 +51,7 @@ func (s *Sync) queueTriggers(file *safcm.File) { // "/" or ".", then the parents and finally path itself). func triggerPaths(path string) []string { sep := string(filepath.Separator) - if path == sep { - return []string{path} - } else if path == "." { + if path == sep || path == "." { return []string{path} } parts := strings.Split(path, sep) -- 2.44.2 From 902f4dd358b7b3f27faccba2e40a04abc760546f Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:29:08 +0200 Subject: [PATCH 12/16] config: tests: replace FullPermToFileMode() with fs constants FullPermToFileMode() is necessary in a few places but the tests should use the common way in Go to set permissions. This is less confusing for the reader. --- cmd/safcm/config/files_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/cmd/safcm/config/files_test.go b/cmd/safcm/config/files_test.go index cc963c9..7ad12d6 100644 --- a/cmd/safcm/config/files_test.go +++ b/cmd/safcm/config/files_test.go @@ -27,8 +27,8 @@ import ( "ruderich.org/simon/safcm/testutil" ) -func chmod(name string, perm int) { - err := os.Chmod(name, FullPermToFileMode(perm)) +func chmod(name string, mode fs.FileMode) { + err := os.Chmod(name, mode) if err != nil { panic(err) } @@ -55,7 +55,7 @@ func TestLoadFiles(t *testing.T) { chmod("files-invalid-perm-dir/files/etc/", 0755) chmod("files-invalid-perm-dir/files/etc/resolv.conf", 0644) chmod("files-invalid-perm-dir-setgid/files", 0755) - chmod("files-invalid-perm-dir-setgid/files/etc/", 02755) + chmod("files-invalid-perm-dir-setgid/files/etc/", 0755|fs.ModeSetgid) chmod("files-invalid-perm-dir-setgid/files/etc/resolv.conf", 0644) chmod("files-invalid-perm-file/files", 0755) chmod("files-invalid-perm-file/files/etc/", 0755) @@ -66,7 +66,8 @@ func TestLoadFiles(t *testing.T) { if !skipInvalidSticky { chmod("files-invalid-perm-file-sticky/files", 0755) chmod("files-invalid-perm-file-sticky/files/etc", 0755) - chmod("files-invalid-perm-file-sticky/files/etc/resolv.conf", 01644) + chmod("files-invalid-perm-file-sticky/files/etc/resolv.conf", + 0644|fs.ModeSticky) } ft.CreateFifo("files-invalid-type/files/invalid", 0644) -- 2.44.2 From e452996e2008d438aa68dedbd206525f507b73d3 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:31:08 +0200 Subject: [PATCH 13/16] config: rewrap line in ResolveHostGroups() --- cmd/safcm/config/groups.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/safcm/config/groups.go b/cmd/safcm/config/groups.go index a1dd83a..571f19b 100644 --- a/cmd/safcm/config/groups.go +++ b/cmd/safcm/config/groups.go @@ -128,8 +128,7 @@ func LoadGroups(cfg *Config, hosts *Hosts) (map[string][]string, error) { return groups, nil } -func ResolveHostGroups(host string, - groups map[string][]string, +func ResolveHostGroups(host string, groups map[string][]string, detectedGroups []string) ([]string, error) { const maxDepth = 100 -- 2.44.2 From f4bae10a4029edfb5db6cb9d305b5d67135409f0 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:31:26 +0200 Subject: [PATCH 14/16] config: use more explicit variable name in ResolveHostGroups() --- cmd/safcm/config/groups.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/safcm/config/groups.go b/cmd/safcm/config/groups.go index 571f19b..c998fb8 100644 --- a/cmd/safcm/config/groups.go +++ b/cmd/safcm/config/groups.go @@ -131,7 +131,7 @@ func LoadGroups(cfg *Config, hosts *Hosts) (map[string][]string, error) { func ResolveHostGroups(host string, groups map[string][]string, detectedGroups []string) ([]string, error) { - const maxDepth = 100 + const maxRecursionDepth = 100 detectedGroupsMap := make(map[string]bool) for _, x := range detectedGroups { @@ -143,7 +143,7 @@ func ResolveHostGroups(host string, groups map[string][]string, // groups). var lookup func(string, int) bool lookup = func(group string, depth int) bool { - if depth > maxDepth { + if depth > maxRecursionDepth { cycle = &group return false } -- 2.44.2 From 77f373a4aa590711155e3af0b768997781f81559 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:32:53 +0200 Subject: [PATCH 15/16] config: return map from TransitivelyDetectedGroups() This is less clean than the original slice of strings. However, it removes unnecessary code as the caller requires a map instead of a slice. --- cmd/safcm/config/groups.go | 10 ++------- cmd/safcm/config/groups_test.go | 36 ++++++++++++++++----------------- cmd/safcm/sync.go | 5 +---- 3 files changed, 21 insertions(+), 30 deletions(-) diff --git a/cmd/safcm/config/groups.go b/cmd/safcm/config/groups.go index c998fb8..466e493 100644 --- a/cmd/safcm/config/groups.go +++ b/cmd/safcm/config/groups.go @@ -193,7 +193,7 @@ func ResolveHostGroups(host string, groups map[string][]string, // TransitivelyDetectedGroups returns all groups which depend on "detected" // groups, either directly or by depending on groups which transitively depend // on "detected" groups. -func TransitivelyDetectedGroups(groups map[string][]string) []string { +func TransitivelyDetectedGroups(groups map[string][]string) map[string]bool { work := make(map[string][]string) for k, v := range groups { work[k] = v @@ -220,11 +220,5 @@ func TransitivelyDetectedGroups(groups map[string][]string) []string { break } } - - var res []string - for x := range detected { - res = append(res, x) - } - sort.Strings(res) - return res + return detected } diff --git a/cmd/safcm/config/groups_test.go b/cmd/safcm/config/groups_test.go index 63c2683..7c3dd48 100644 --- a/cmd/safcm/config/groups_test.go +++ b/cmd/safcm/config/groups_test.go @@ -332,7 +332,7 @@ func TestTransitivelyDetectedGroups(t *testing.T) { tests := []struct { name string groups map[string][]string - exp []string + exp map[string]bool }{ { @@ -351,7 +351,7 @@ func TestTransitivelyDetectedGroups(t *testing.T) { "d", }, }, - nil, + map[string]bool{}, }, { @@ -367,8 +367,8 @@ func TestTransitivelyDetectedGroups(t *testing.T) { "d", }, }, - []string{ - "group-a", + map[string]bool{ + "group-a": true, }, }, @@ -389,8 +389,8 @@ func TestTransitivelyDetectedGroups(t *testing.T) { "d", }, }, - []string{ - "group-a", + map[string]bool{ + "group-a": true, }, }, @@ -418,12 +418,12 @@ func TestTransitivelyDetectedGroups(t *testing.T) { "b", }, }, - []string{ - "group-a", - "group-b", - "group-c", - "group-d", - "group-e", + map[string]bool{ + "group-a": true, + "group-b": true, + "group-c": true, + "group-d": true, + "group-e": true, }, }, @@ -453,12 +453,12 @@ func TestTransitivelyDetectedGroups(t *testing.T) { "b", }, }, - []string{ - "group-a", - "group-b", - "group-c", - "group-d", - "group-e", + map[string]bool{ + "group-a": true, + "group-b": true, + "group-c": true, + "group-d": true, + "group-e": true, }, }, } diff --git a/cmd/safcm/sync.go b/cmd/safcm/sync.go index 77e83c4..6358f0c 100644 --- a/cmd/safcm/sync.go +++ b/cmd/safcm/sync.go @@ -226,10 +226,7 @@ func MainSync(args []string) error { func hostsToSync(names []string, allHosts *config.Hosts, allGroups map[string][]string) ([]*config.Host, error) { - detectedMap := make(map[string]bool) - for _, x := range config.TransitivelyDetectedGroups(allGroups) { - detectedMap[x] = true - } + detectedMap := config.TransitivelyDetectedGroups(allGroups) const detectedErr = ` -- 2.44.2 From 632ad7c77e8e057b8dc653aadda749767acb51e1 Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 12:34:44 +0200 Subject: [PATCH 16/16] config: tests: fix typo in group name --- cmd/safcm/config/groups_test.go | 2 +- cmd/safcm/testdata/group-invalid-missing/groups.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/safcm/config/groups_test.go b/cmd/safcm/config/groups_test.go index 7c3dd48..2c9658b 100644 --- a/cmd/safcm/config/groups_test.go +++ b/cmd/safcm/config/groups_test.go @@ -186,7 +186,7 @@ func TestLoadGroups(t *testing.T) { &Config{}, &Hosts{}, nil, - fmt.Errorf("groups.yaml: group \"1group2\": member \"does-not-exist\" not found"), + fmt.Errorf("groups.yaml: group \"group2\": member \"does-not-exist\" not found"), }, { "../testdata/group-invalid-name", diff --git a/cmd/safcm/testdata/group-invalid-missing/groups.yaml b/cmd/safcm/testdata/group-invalid-missing/groups.yaml index be700b7..1e5d818 100644 --- a/cmd/safcm/testdata/group-invalid-missing/groups.yaml +++ b/cmd/safcm/testdata/group-invalid-missing/groups.yaml @@ -1,2 +1,2 @@ -1group2: +group2: - does-not-exist -- 2.44.2