Skip to content

add custom options to sshdriver #1073

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

b2vn
Copy link
Contributor

@b2vn b2vn commented Jan 20, 2023

Description

added the possibility to define custom option for the SSHDriver in the environment yaml. The motivation for this change was to be able to overwrite host key algorithms in test context.

Checklist

  • Documentation for the feature
  • Tests for the feature
  • The arguments and description in doc/configuration.rst have been updated
  • Add a section on how to use the feature to doc/usage.rst
  • Add a section on how to use the feature to doc/development.rst
  • PR has been tested
  • Man pages have been regenerated

@codecov
Copy link

codecov bot commented Jan 20, 2023

Codecov Report

Base: 63.2% // Head: 63.2% // Increases project coverage by +0.0% 🎉

Coverage data is based on head (a5642dc) compared to base (170a642).
Patch coverage: 100.0% of modified lines in pull request are covered.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1073   +/-   ##
======================================
  Coverage    63.2%   63.2%           
======================================
  Files         152     152           
  Lines       11346   11352    +6     
======================================
+ Hits         7174    7180    +6     
  Misses       4172    4172           
Impacted Files Coverage Δ
labgrid/driver/sshdriver.py 58.9% <100.0%> (+0.9%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@jluebbe jluebbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -48,6 +49,9 @@ def on_activate(self):
if not self.networkservice.password:
self.ssh_prefix += ["-o", "PasswordAuthentication=no"]

if self.options:
self.ssh_prefix += self.options.split(" ")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.python.org/3/library/shlex.html#shlex.split is probably better in this case.

As some special cases around quoting are not easily handled even with shlex.split, we should support optionally having a list instead of a string. In that case, they could simply be passed along.

@@ -30,6 +30,7 @@ class SSHDriver(CommandMixin, Driver, CommandProtocol, FileTransferProtocol):
bindings = {"networkservice": "NetworkService", }
priorities = {CommandProtocol: 10, FileTransferProtocol: 10}
keyfile = attr.ib(default="", validator=attr.validators.instance_of(str))
options = attr.ib(default="", validator=attr.validators.instance_of(str))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it should be called extra_options, to indicate that it cannot be used to replace the options set by labgrid? @Emantor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, will be done

@b2vn b2vn force-pushed the sshoptions branch 2 times, most recently from a63e430 to 20f4c34 Compare January 26, 2023 11:37
@Emantor Emantor requested a review from jluebbe February 23, 2023 09:10
Emantor
Emantor previously approved these changes Feb 23, 2023
added the possibility to define custom option for the SSHDriver in the environment yaml

Signed-off-by: Nikolaj Rahbel <[email protected]>
@Emantor Emantor added ready for merge Was reviewed, can be merged after tests ran. enhancement labels Feb 23, 2023
self.ssh_prefix += shlex.split(self.extra_options)
elif isinstance(self.extra_options, list) and len(self.extra_options) > 0:
for eo in self.extra_options:
self.ssh_prefix += shlex.split(eo)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we split each option in the list? This makes it impossible to pass options with spaces.

Comment on lines +1486 to +1489
extra_options:
- "-o option1"
- "-o options2"
- "-4"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems inconsistent. Why use an explicit list if not to exactly device the argv elements?

@Bastian-Krause Bastian-Krause added needs author info Requires more information from the PR/Issue author needs rebase Needs a rebase onto the master branch, maintainter could probably not push to submitter branch. and removed ready for merge Was reviewed, can be merged after tests ran. labels Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs author info Requires more information from the PR/Issue author needs rebase Needs a rebase onto the master branch, maintainter could probably not push to submitter branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants