Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more sshcmd tests #1952

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@tcely
Copy link

commented Aug 14, 2018

What is the purpose of this change? What does it change?

Provide more test data to hopefully catch coding errors.

Was the change discussed in an issue or in the forum before?

Not that I am aware of.

Checklist

  • I have read the Contribution Guidelines
  • I have added tests for all changes in this PR
  • I have added documentation for the changes (in the manual)
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
  • I have run gofmt on the code in all commits
  • All commit messages are formatted in the same style as the other commits in the repo
  • I'm done, this Pull Request is ready for review

@tcely tcely referenced this pull request Aug 14, 2018

Closed

Add more sshcmd tests #1943

4 of 7 tasks complete

@tcely tcely force-pushed the tcely:improve-sshcmd branch 2 times, most recently from 850a91f to 05ffb10 Aug 14, 2018

@codecov-io

This comment has been minimized.

Copy link

commented Aug 14, 2018

Codecov Report

Merging #1952 into master will decrease coverage by 4.31%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1952      +/-   ##
==========================================
- Coverage   50.56%   46.25%   -4.32%     
==========================================
  Files         170      170              
  Lines       13258    13271      +13     
==========================================
- Hits         6704     6138     -566     
- Misses       5540     6173     +633     
+ Partials     1014      960      -54
Impacted Files Coverage Δ
internal/backend/sftp/sftp.go 62.68% <88.23%> (+1.08%) ⬆️
internal/backend/b2/b2.go 0% <0%> (-78.54%) ⬇️
internal/backend/swift/swift.go 0% <0%> (-78.45%) ⬇️
internal/backend/gs/gs.go 0% <0%> (-73.61%) ⬇️
internal/backend/azure/azure.go 0% <0%> (-69.46%) ⬇️
internal/backend/swift/config.go 36.95% <0%> (-54.35%) ⬇️
internal/backend/s3/s3.go 62.4% <0%> (+2.25%) ⬆️
internal/archiver/file_saver.go 85.96% <0%> (+4.38%) ⬆️
internal/archiver/blob_saver.go 100% <0%> (+4.93%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49d95e9...46397bc. Read the comment docs.

@tcely tcely force-pushed the tcely:improve-sshcmd branch from 05ffb10 to 75b6d82 Aug 14, 2018

@tcely tcely force-pushed the tcely:improve-sshcmd branch from 86cbaeb to 46397bc Aug 14, 2018

@fd0

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

Hm, I'd like to confirm something: are host names with a colon in them a thing? Do people use this, did you encounter a host name with a colon in it in the wild?

I'm sorry to say this, but the solution you implemented is very complicated, and I'd rather like to avoid such complexity. People can always work around such issues by defining a Host in their ~/.ssh/config like this:

Host foo
    Hostname ho:st
    User us:er
    Port 22222

And then use restic -r sftp:user@foo instead.

@fd0

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

Besides, the code as implemented here would not be used: we're using colons in restic already for splitting the host name and directory part in the repository setting. Calling restic with a repository sftp:us:er@ho:st10022:/file leads to the following sftp.Config struct (you can check by writing a debug log):

$ ./restic -r sftp:us:er@ho:st:10022:/file init
[...]opening sftp repository at sftp.Config{User:"", Host:"us", Path:"er@ho:st:10022:/file", Layout:"", Command:""}

So we would have to change the parsing code for the repository definition also. Which makes it even more complicated :(

}
args = []string{host}
if port != "" {
tryport, err := net.LookupPort("tcp", port)

This comment has been minimized.

Copy link
@fd0

fd0 Aug 14, 2018

Member

Hm, I don't understand. Why is this needed? It adds a feature (being able to specify a port name instead of a port number)?

This comment has been minimized.

Copy link
@tcely

tcely Aug 15, 2018

Author

It's not adding that feature. What it is doing is restricting the values that are accepted as ports.

I'd prefer not passing something we assume is a port just to have ssh kick back the error when it fails to find ssh3 in /etc/services or says that 75022 is not a valid port number.

@@ -30,6 +30,21 @@ var sshcmdTests = []struct {
"ssh",
[]string{"host", "-p", "10022", "-l", "user", "-s", "sftp"},

This comment has been minimized.

Copy link
@fd0

fd0 Aug 14, 2018

Member

I think we should remove this test (and support for passing a host:port string to buildSSHCommand(). That caused us problems in the past, to the point that we removed parsing the port at all, and added hints to the manual how users can work around this: https://restic.readthedocs.io/en/latest/030_preparing_a_new_repo.html#sftp

This comment has been minimized.

Copy link
@tcely

tcely Aug 15, 2018

Author

I don't think this is a step that should be taken. Again, making the repository parsing more robust is my preference.

All of this becomes less important when restic has it's own configuration file support. However, even then when you have to run restic as different users or on many systems, being able to specify all of the configuration in a single command line is much better. I don't like the idea of needing to ensure a specific configuration across many files or users is properly setup before running restic operations.

That documentation should include a working example of -o sftp.command="foobar" and possibly also the default sftp command that's built.

The options to be built appear to be these:

  • -o sftp.command="ssh host -p 22 -l user -s sftp"
  • -o sftp.command="ssh host -p 22 -s sftp"
  • -o sftp.command="ssh host -l user -s sftp"
  • -o sftp.command="ssh host -s sftp"

I know I recently went looking for how to get restic to use krssh and this information would have been helpful then. I ended up creating a wrapper named ssh then changing the PATH restic was using so that this wrapper was found first.

@fd0

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

I'm sorry, I think the comments I wrote come across as very negative. I'm criticizing the code, not your contribution! Thank you very much for trying to improve restic!

It'd be helpful for me to understand what your motivation for this change is, so I'm looking forward to your answers for the questions I asked above. :)

@tcely

This comment has been minimized.

Copy link
Author

commented Aug 15, 2018

@fd0

Hostnames with a colon are unusual in my experience, but IPv6 makes heavy use of them and the naive splitting on ':' precludes specifying the IPv6 address as the host to connect to. FYI, this is typically [::1]:22 with the IPv6 address and port.

Usernames on Unix-like systems don't contain colons either. Not everything we connect to with sftp is guaranteed to be Unix-like though. BTW, the work around you gave won't work as the first user specified wins in ssh land and providing sftp:user@foo means a command line with -l user is built.

I tried not to make this overly complicated, but in my opinion the parsing of the repository specification should be made more robust too. My suggestion is to outlaw colon characters in both the sftp marker at the beginning of repository specification and in the path at the end.

The parsing then becomes something along these lines:

  1. find the first colon, the beginning up to the first colon is the backend type
  2. find the last colon, the position after that colon to the end is used to set Config.Path
  3. split the substring between those two colon positions on '@' if it exists to set Config.User
  4. the remainder is used to set Config.Host
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.