Verify rsync protocol version match prior to proceeding - Rebased on current master. #71

stephenwithav · 2020-07-01T23:29:15Z

sync.Version() proceeds with a warning that there may be a version
mismatch if it timesout.

syncCmd.version assumes rsync is present in the path.

wsep integration is currently unverified. Verification is next.

Potentially closes #65.

`sync.Version()` proceeds with a warning that there may be a version mismatch if it timesout. `syncCmd.version` assumes rsync is present in the path. `wsep.RemoteExecer` isn't available publicly on GitHub, so I took a best guess with it and `wsep.ExitError`.

Passing `s.ReadLine` to `strings.Split` was a mistake since it returns three values. Having access to `wsep` helped the compiler find the bugs.

stephenwithav · 2020-07-02T00:27:03Z

This builds locally fine.

The CI bug is inaccessible to me. Any tips, @scsmithr?

scsmithr

I know it's kind of tough to be contributing changes that you don't have a good way of testing, but I think we're close.

And you can ignore the ci failure, it's just failing because there isn't a license for you on that service. Everything compiled fine on my end.

scsmithr · 2020-07-02T00:48:39Z

cmd/coder/sync.go

+// See https://lxadm.com/Rsync_exit_codes#List_of_standard_rsync_exit_codes.
+var IncompatRsync = errors.New("rsync: exit status 2")
+var StreamErrRsync = errors.New("rsync: exit status 12")


Probably leftover from a rebase, these were removed in master.

scsmithr · 2020-07-02T01:23:57Z

cmd/coder/sync.go

+func (s *syncCmd) version() string {
+	cmd := exec.Command("rsync", "--version")
+	stdout, err := cmd.StdoutPipe()
+	if err != nil {
+		log.Fatal(err)
+	}
+
+	if err := cmd.Start(); err != nil {
+		log.Fatal(err)
+	}
+
+	r := bufio.NewReader(stdout)
+	if err := cmd.Wait(); err != nil {
+		log.Fatal(err)
+	}
+
+	firstLine, err := r.ReadString('\n')
+	if err != nil {
+		return ""
+	}
+
+	versionString := strings.Split(firstLine, "protocol version ")
+
+	return versionString[1]


The way this is laid out, version will always return an empty string. Reading from r will always error, because the pipe will be closed before we start reading from the command.

I would suggest replacing reading the output of the command with:

out, err := cmd.CombinedOutput() if err != nil { log.Fatal(err) }

And I would replace reading the first line with:

firstLine, err := bytes.NewBuffer(out).ReadString('\n') if err != nil { log.Fatal(err) }

scsmithr · 2020-07-02T01:27:45Z

internal/sync/sync.go

+	if err != nil {
+		return "", err
+	}
+	r := bufio.NewReader(process.Stdout())


We have the same issue here with the reader never reading anything.

I think the best solution here would be to replace this reader with:

buf := &bytes.Buffer{} io.Copy(buf, process.Stdout())

And buf can be used down below to read the first line.

@scsmithr Do you want the remote check to log.Fatal fail if we can't retrieve it?

I think I prefer the behavior you had, where we just return the error and warn the user.

The remote check still has that.

scsmithr · 2020-07-02T14:24:16Z

internal/sync/sync.go

+		return "", err
+	}
+	buf := &bytes.Buffer{}
+	go io.Copy(buf, process.Stdout())


I would remove the go here. io.Copy will will terminate as expected once the command completes.

Having a goroutine here can be racy, because there's no guarantees that it runs or completes the copy before we start reading from the buffer.

scsmithr

Two very minor issues and we should be good to go.

scsmithr · 2020-07-02T14:46:56Z

internal/sync/sync.go

+
+	err = process.Wait()
+	if code, ok := err.(wsep.ExitError); ok {
+		return "", fmt.Errorf("Version check exit status: %v", code)


Suggested change

return "", fmt.Errorf("Version check exit status: %v", code)

return "", fmt.Errorf("version check exit status: %v", code)

scsmithr · 2020-07-02T14:47:13Z

internal/sync/sync.go

+		return "", fmt.Errorf("Version check exit status: %v", code)
+	}
+	if err != nil {
+		return "", fmt.Errorf("Server version mismatch")


I'd prefer returning the error here instead.

scsmithr · 2020-07-02T15:00:10Z

internal/sync/sync.go

+	if _, ok := err.(wsep.ExitError); ok {
+		return "", err
+	}


We may as well remove this branch.

scsmithr · 2020-07-02T15:02:02Z

cmd/coder/sync.go

@@ -29,6 +33,23 @@ func (cmd *syncCmd) RegisterFlags(fl *pflag.FlagSet) {
 	fl.BoolVarP(&cmd.init, "init", "i", false, "do initial transfer and exit")
 }

+// Returns local rsync protocol version as a string.


Apologies for missing this earlier in the review. These comments should start with the function/method name. So this would be "version returns local rsync protocol version as a string."

(And same for the other Version method)

No problem. I appreciate you teaching me the style you prefer prior to the interview next week so if I get the job, I'm already style trained.

scsmithr

Nice work, appreciate the contribution.

stephenwithav added 3 commits July 1, 2020 19:23

Add context to sync.Version.

5538af6

Correct syncCmd.version, sync.Version.

54cecb2

Passing `s.ReadLine` to `strings.Split` was a mistake since it returns three values. Having access to `wsep` helped the compiler find the bugs.

scsmithr suggested changes Jul 2, 2020

View reviewed changes

Update local, remote version checks to avoid blanks.

35ee1de

stephenwithav force-pushed the add-version-check-locally branch from 223551b to 35ee1de Compare July 2, 2020 13:40

stephenwithav mentioned this pull request Jul 2, 2020

Check that rsync protocol versions match before initiating transfer #65

Closed

scsmithr reviewed Jul 2, 2020

View reviewed changes

Remove go from io.Copy in internal/sync/sync.go#Version

21976c6

scsmithr reviewed Jul 2, 2020

View reviewed changes

Return error directly in internal/sync/sync.go#Version

d853066

stephenwithav force-pushed the add-version-check-locally branch from 7ec1868 to d853066 Compare July 2, 2020 14:56

scsmithr reviewed Jul 2, 2020

View reviewed changes

Update [vV]ersion comment strings; remove extraneous error check.

e8abef2

scsmithr approved these changes Jul 2, 2020

View reviewed changes

scsmithr merged commit c8cec8e into coder:master Jul 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify rsync protocol version match prior to proceeding - Rebased on current master. #71

Verify rsync protocol version match prior to proceeding - Rebased on current master. #71

stephenwithav commented Jul 1, 2020

stephenwithav commented Jul 2, 2020

scsmithr left a comment

scsmithr Jul 2, 2020

scsmithr Jul 2, 2020

stephenwithav Jul 2, 2020

scsmithr Jul 2, 2020

stephenwithav Jul 2, 2020

stephenwithav Jul 2, 2020 •

edited

Loading

scsmithr Jul 2, 2020

stephenwithav Jul 2, 2020

scsmithr Jul 2, 2020

stephenwithav Jul 2, 2020

scsmithr left a comment

scsmithr Jul 2, 2020

scsmithr Jul 2, 2020

scsmithr Jul 2, 2020

scsmithr Jul 2, 2020

stephenwithav Jul 2, 2020

scsmithr left a comment

	return "", fmt.Errorf("Version check exit status: %v", code)
	return "", fmt.Errorf("version check exit status: %v", code)

Verify rsync protocol version match prior to proceeding - Rebased on current master. #71

Verify rsync protocol version match prior to proceeding - Rebased on current master. #71

Conversation

stephenwithav commented Jul 1, 2020

stephenwithav commented Jul 2, 2020

scsmithr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenwithav Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scsmithr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scsmithr left a comment

Choose a reason for hiding this comment

stephenwithav Jul 2, 2020 •

edited

Loading