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

Experimental: Use zfs permissions instead of zfs shell and sudo #32

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MichaelHierweck
Copy link
Contributor

Experimental: Use zfs permissions instead of zfs shell and sudo
Note: pr23 and pr24 have to be merged before.

@Rudd-O
Copy link
Owner

Rudd-O commented Dec 28, 2020

Can you look at the merge conflict fix and see if the code is adequate for merging? Thanks.

@@ -59,25 +59,25 @@ def __init__(self,host="localhost", trust=False, sshcipher=None, properties=None
def _get_poolset(self):
if self._dirty:
properties = [ 'creation' ] + self._properties
stdout2 = subprocess.check_output(self.command + ["zfs", "list", "-Hpr", "-o", ",".join( ['name'] + properties ), "-t", "all"])
stdout2 = subprocess.check_output(self.command + ["/sbin/zfs", "list", "-Hpr", "-o", ",".join( ['name'] + properties ), "-t", "all"])
Copy link
Owner

Choose a reason for hiding this comment

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

You should not encode the path to zfs. Let the PATH resolution find it.

@@ -5,6 +5,18 @@
| <img width="164" height="164" title="" alt="" src="doc/bitcoin.png" /> |
| [1Cw9nZu9ygknussPofMWCzmSMveusTbQvN](bitcoin:1Cw9nZu9ygknussPofMWCzmSMveusTbQvN) |

## Experimental version of zfs-tools:
Copy link
Owner

Choose a reason for hiding this comment

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

We need to maintain support for zfs-shell since not everyone is ready to migrate to ZFS permissions, and people deploy from this repo.

If needed, we can add to zfs-shell some sort of canary command that the client can then query, and if that fails, revert back to using zfs-shell instead of ZFS permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First of all, sorry, the last weeks I was too busy with other things .

I wonder how we could make zfs-shell optional.

Case "zfs-shell":
zfs-shell should be installed and registered as a shell. We might even need the sudo rules.
ztools should invoke zfs-shell as a helper script.

Case "zfs-permissions":
zfs-shell should not be installed (or at least) not registered. Sudo rules should not be installed or at least not activated.
ztools should invoke zpool and zfs directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about creating two Debian packages out of a single source package, e.g. "zfs-tools" and optional "zfs-shell" (providing zfs-shell and sudo rules). zfs-tools could suggest or even recommend zfs-shell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Rudd-O
Copy link
Owner

Rudd-O commented Apr 29, 2021

Happy to take a look at a second round!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants