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

zfs_delegate_admin: fix: zfs allow cannot parse unknown uid/gid #5943

Merged

Conversation

papamoose
Copy link
Contributor

@papamoose papamoose commented Feb 7, 2023

When setting allow permissions for particular users or groups there will be circumstances when that user is not known to the host system.

In that case the output of zfs allow <pool/dataset> looks similar to this:

$ sudo zfs allow tank/test
---- Permissions on tank/test ---------------------------------------
Local+Descendent permissions:
user (unknown: 1002) hold
user zfsuser receive

The fix in this commit removes (unknown:+) from the output leaving only the uid/gid.

This allows the current parser to continue even if the uid/gid is not known.

This situation occurs most often when moving a zpool from one system to another that may not have the same users/groups. Simply adding permissions to a user/group and then deleting the user/group from the system will cause this situation to occur.

SUMMARY

Fixes #5941

Removing (unknown: + ) from line allows the user to remove and permissions to uids or gids even if the host system is unaware of an actual user/group by that name.

Ideally, openzfs could modify the zfs allow command to allow for machine readable output like most of the zfs commands available: e.g. -H -o name,permissions,entity.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

community.general.zfs_delegate_admin

ADDITIONAL INFORMATION

Testing

  tasks:

  - name: remove all perms
    community.general.zfs_delegate_admin:
      name: tank/test
      users: "zfsuser,1002"
      groups: 2000
      permissions: send,hold
      recursive: yes
      state: absent

  - ansible.builtin.command:
      cmd: zfs allow tank/test
    register: o1

  - debug:
      msg: "{{ o1.stdout.split('\n') }}"

  - name: add perm
    community.general.zfs_delegate_admin:
      name: tank/test
      users: "zfsuser,1002"
      groups: 2000
      permissions: send
      recursive: yes
      state: present

  - ansible.builtin.command:
      cmd: zfs allow tank/test
    register: o2

  - debug:
      msg: "{{ o2.stdout.split('\n') }}"

  - name: remove perm
    community.general.zfs_delegate_admin:
      name: tank/test
      users: "zfsuser,1002"
      groups: 2000
      permissions: send
      recursive: yes
      state: absent

  - ansible.builtin.command:
      cmd: zfs allow tank/test
    register: o3

  - debug:
      msg: "{{ o3.stdout.split('\n') }}"
$ ansible-playbook a.yml -l machine-a

PLAY [all] *************************************************************************************************

TASK [Gathering Facts] *************************************************************************************
ok: [machine-a]

TASK [remove all perms] ************************************************************************************
changed: [machine-a]

TASK [ansible.builtin.command] *****************************************************************************
changed: [machine-a]

TASK [debug] ***********************************************************************************************
ok: [machine-a] => {
    "msg": [
        ""
    ]
}

TASK [add perm] ********************************************************************************************
changed: [machine-a]

TASK [ansible.builtin.command] *****************************************************************************
changed: [machine-a]

TASK [debug] ***********************************************************************************************
ok: [machine-a] => {
    "msg": [
        "---- Permissions on tank/test ----------------------------------------",
        "Local+Descendent permissions:",
        "\tuser (unknown: 1002) send",
        "\tuser zfsuser send",
        "\tgroup (unknown: 2000) send"
    ]
}

TASK [remove perm] *****************************************************************************************
changed: [machine-a]

TASK [ansible.builtin.command] *****************************************************************************
changed: [machine-a]

TASK [debug] ***********************************************************************************************
ok: [machine-a] => {
    "msg": [
        ""
    ]
}

PLAY RECAP *************************************************************************************************
machine-a                      : ok=10   changed=6    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug has_issue module module new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review storage labels Feb 7, 2023
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 7, 2023
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Could you please add a changelog fragment and adjust the spacing after commas? Thanks.

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels Feb 7, 2023
When setting allow permissions for particular users or groups
there will be circumstances when that user is not known to the
host system.

In that case the output of `zfs allow <pool/dataset>`
looks similar to this:

  $ sudo zfs allow tank/test
  ---- Permissions on tank/test ---------------------------------------
  Local+Descendent permissions:
    user (unknown: 1002) hold
    user zfsuser receive

The fix in this commit removes ' (unknown: '+')' from the output
leaving only the uid/gid.

This allows the current parser to continue even if the uid/gid
is not known.

This situation occurs most often when moving a zpool from one system
to another that may not have the same users/groups. Simply adding
permissions to a user/group and then deleting the user/group
from the system will cause this situation to occur.
@papamoose papamoose force-pushed the zfs_delegate_admin_unknown_id branch from 4a9b693 to ecdb7c2 Compare February 7, 2023 14:38
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI small_patch Hopefully easy to review needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 7, 2023
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Feb 15, 2023
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

If nobody objects, I'll merge this by this weekend for the next release.

@felixfontein felixfontein merged commit 53f7297 into ansible-collections:main Feb 25, 2023
@patchback
Copy link

patchback bot commented Feb 25, 2023

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/53f729730bcc95c75354b11d6ad5f999f883fe9d/pr-5943

Backported as #6085

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Feb 25, 2023
When setting allow permissions for particular users or groups
there will be circumstances when that user is not known to the
host system.

In that case the output of `zfs allow <pool/dataset>`
looks similar to this:

  $ sudo zfs allow tank/test
  ---- Permissions on tank/test ---------------------------------------
  Local+Descendent permissions:
    user (unknown: 1002) hold
    user zfsuser receive

The fix in this commit removes ' (unknown: '+')' from the output
leaving only the uid/gid.

This allows the current parser to continue even if the uid/gid
is not known.

This situation occurs most often when moving a zpool from one system
to another that may not have the same users/groups. Simply adding
permissions to a user/group and then deleting the user/group
from the system will cause this situation to occur.

(cherry picked from commit 53f7297)
@patchback
Copy link

patchback bot commented Feb 25, 2023

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/53f729730bcc95c75354b11d6ad5f999f883fe9d/pr-5943

Backported as #6086

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@felixfontein
Copy link
Collaborator

@papamoose thank you very much for your contribution!

patchback bot pushed a commit that referenced this pull request Feb 25, 2023
When setting allow permissions for particular users or groups
there will be circumstances when that user is not known to the
host system.

In that case the output of `zfs allow <pool/dataset>`
looks similar to this:

  $ sudo zfs allow tank/test
  ---- Permissions on tank/test ---------------------------------------
  Local+Descendent permissions:
    user (unknown: 1002) hold
    user zfsuser receive

The fix in this commit removes ' (unknown: '+')' from the output
leaving only the uid/gid.

This allows the current parser to continue even if the uid/gid
is not known.

This situation occurs most often when moving a zpool from one system
to another that may not have the same users/groups. Simply adding
permissions to a user/group and then deleting the user/group
from the system will cause this situation to occur.

(cherry picked from commit 53f7297)
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Feb 25, 2023
felixfontein pushed a commit that referenced this pull request Feb 25, 2023
…llow cannot parse unknown uid/gid (#6086)

zfs_delegate_admin: fix: zfs allow cannot parse unknown uid/gid (#5943)

When setting allow permissions for particular users or groups
there will be circumstances when that user is not known to the
host system.

In that case the output of `zfs allow <pool/dataset>`
looks similar to this:

  $ sudo zfs allow tank/test
  ---- Permissions on tank/test ---------------------------------------
  Local+Descendent permissions:
    user (unknown: 1002) hold
    user zfsuser receive

The fix in this commit removes ' (unknown: '+')' from the output
leaving only the uid/gid.

This allows the current parser to continue even if the uid/gid
is not known.

This situation occurs most often when moving a zpool from one system
to another that may not have the same users/groups. Simply adding
permissions to a user/group and then deleting the user/group
from the system will cause this situation to occur.

(cherry picked from commit 53f7297)

Co-authored-by: Phil Kauffman <philip@kauffman.me>
felixfontein pushed a commit that referenced this pull request Feb 25, 2023
…llow cannot parse unknown uid/gid (#6085)

zfs_delegate_admin: fix: zfs allow cannot parse unknown uid/gid (#5943)

When setting allow permissions for particular users or groups
there will be circumstances when that user is not known to the
host system.

In that case the output of `zfs allow <pool/dataset>`
looks similar to this:

  $ sudo zfs allow tank/test
  ---- Permissions on tank/test ---------------------------------------
  Local+Descendent permissions:
    user (unknown: 1002) hold
    user zfsuser receive

The fix in this commit removes ' (unknown: '+')' from the output
leaving only the uid/gid.

This allows the current parser to continue even if the uid/gid
is not known.

This situation occurs most often when moving a zpool from one system
to another that may not have the same users/groups. Simply adding
permissions to a user/group and then deleting the user/group
from the system will cause this situation to occur.

(cherry picked from commit 53f7297)

Co-authored-by: Phil Kauffman <philip@kauffman.me>
@github-actions
Copy link

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

@papamoose papamoose deleted the zfs_delegate_admin_unknown_id branch February 25, 2023 19:38
jikamens pushed a commit to jikamens/community.general that referenced this pull request Feb 25, 2023
…ble-collections#5943)

When setting allow permissions for particular users or groups
there will be circumstances when that user is not known to the
host system.

In that case the output of `zfs allow <pool/dataset>`
looks similar to this:

  $ sudo zfs allow tank/test
  ---- Permissions on tank/test ---------------------------------------
  Local+Descendent permissions:
    user (unknown: 1002) hold
    user zfsuser receive

The fix in this commit removes ' (unknown: '+')' from the output
leaving only the uid/gid.

This allows the current parser to continue even if the uid/gid
is not known.

This situation occurs most often when moving a zpool from one system
to another that may not have the same users/groups. Simply adding
permissions to a user/group and then deleting the user/group
from the system will cause this situation to occur.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug has_issue module module new_contributor Help guide this first time contributor plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging storage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zfs_delegate_admin.py: def current_perms: unable to parse unknown uid and gid
3 participants