user management does not work under Synology

aogier commented 7 years ago

Description of Issue/Question

Hi, on a Synology NAS I cannot manage users with user virtual module because no useradd command is present. While the installed version is pretty old, it does seem the same on develop branch.

I'd be happy to implement it, but I'm not sure on which choice between patch current module and writing a new would be better given that current useradd virtual match is quite large, matching against kernel grain. What do you think ?

Thank you, regards

Steps to Reproduce Issue

# salt-call --local user.add name=hello
[INFO    ] Executing command ['useradd', '-m', 'hello'] in directory '/root'
Error running 'user.add': Unable to run command ['useradd', '-m', 'hello'] with the context
 {...}, reason: [Errno 2] No such file or directory

Versions Report

           Salt: 2015.5.3
         Python: 2.7.13 (default, Dec 18 2016, 21:23:50)
         Jinja2: 2.7.3
       M2Crypto: 0.25.1
 msgpack-python: 0.4.8
   msgpack-pure: Not Installed
       pycrypto: 2.6.1
        libnacl: Not Installed
         PyYAML: 3.12
          ioflo: Not Installed
          PyZMQ: 16.0.2
           RAET: Not Installed
            ZMQ: 4.1.6
           Mako: Not Installed
        Tornado: Not Installed

gtmanfred commented 7 years ago

I would match against the kernel grain, and write a new module, that is compatible with the linux useradd module, so that it can still be used with the user state.

Thanks! Daniel

aogier commented 7 years ago

Hello Daniel, thank you for the feedback. From your answer I understand you'd go with another __virtual__ module that implement a user state interface synology-specific.
However, I cannot let it "win" over useradd implementation because latter match is still too large and win because Synology also have kernel=Linux, or maybe I'm not able to find a deterministic method to let one win over the other.
Apart user-provided virtual module override feature, given that Synology is a "first class citizen" on Saltstack (see here and here) I'd like to code that diversion for an eventual upstream merge.

What do you think ? Many thanks, ciao !

gtmanfred commented 7 years ago

That sounds good to me!

On Mon, Aug 7, 2017 at 7:29 AM, Alessandro Ogier notifications@github.com wrote:

Hello Daniel, thank you for the feedback. From your answer I understand you'd go with another virtual module that implement a user state interface synology-specific. However, I cannot let it "win" over useradd implementation because latter match is still too large and win https://github.com/saltstack/salt/blob/82582f0304683fbc9bd35a89eca5ddf52d91419c/salt/modules/useradd.py#L40 because Synology also have kernel=Linux, or maybe I'm not able to find a deterministic method to let one win over the other. Apart user-provided virtual module override feature https://docs.saltstack.com/en/latest/ref/modules/#virtual-function, given that Synology is a "first class citizen" on Saltstack (see here https://github.com/saltstack/salt/blob/82582f0304683fbc9bd35a89eca5ddf52d91419c/salt/grains/core.py#L1135 and here https://github.com/saltstack/salt/blob/82582f0304683fbc9bd35a89eca5ddf52d91419c/salt/grains/core.py#L1511) I'd like to code that diversion for an eventual upstream merge.

What do you think ? Many thanks, ciao !

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/saltstack/salt/issues/42739#issuecomment-320663467, or mute the thread https://github.com/notifications/unsubscribe-auth/AAssoZTCJlyigpPnxGXfCMoDtjhO8_Mnks5sVxEigaJpZM4Otii1 .

aogier commented 7 years ago

Hi, I've just implemented group/useradd for Synology platforms, relevant integration tests are passing.
I've however encountered a couple of issue I'd like to implement in a salty way:

code repetitions: I'd like to refactor code in an utils package (see for example here and here) but I cannot understand how to access __salt__ magic variable from there. Tried various solutions but no luck so far
coping with unsupported features: while returning False seems a common approach for entirely unsupported methods on some platform, I'm not sure how to deal with functions that are indeed supported but with only a subset of their parameters. For example, user.rename "root" parameters get ignored on AIX platform. It is the correct way to cope with that, or better to explicitly tell that platform does not support a particular state parameter ?

Thank you, ciao !

gtmanfred commented 7 years ago

__salt__ is not available in __utils__

You can see what is loaded in each module by checking salt.loader

https://github.com/saltstack/salt/blob/2017.7/salt/loader.py#L341

As for point 2, I don't think there is really any guidelines except that you should make sure to make a note of it in the docstring for the function. Whichever way you think is best.

aogier commented 7 years ago

ok thank you, I'm beginning to understand this magic. Still, there's another problem (this time I think real) accessing those dunders: taking as an example this line I've wrote a simple condition that checks for the 'os_family' grain. After many failed jenkins tests because KeyError I've placed a debug print and a divide by zero (you can see it here), here's the output on jenkins: grains is an empty dict.
This lead to a couple issues:

we never tests this method with system == True :P
I really don't know how to access grains in modules, and I don't have a valid example :-(

Can you help me please ? Many thanks !

gtmanfred commented 7 years ago

I think the __virtual__ function may be run twice. Once before the __grains__ dict is populated, and once after, so you may try using __grains__.get('os_family') instead so that there isn't a stack trace?

Just a thought.

aogier commented 7 years ago

the strange thing is that getting kernel grain during __virtual__ does not seem to raise any exception (and it works, in fact, since module win group virtual name). When I get os_family in add() implementation I need it, but maybe this quirk is only test related ?

thank you !

gtmanfred commented 7 years ago

That is odd. I can't think of a reason for it.

On Tue, Aug 22, 2017 at 9:05 AM, Alessandro Ogier notifications@github.com wrote:

the strange thing is that getting kernel grain during virtual does not seem to raise any exception https://github.com/aogier/salt/blob/ad2aae429c03de3d24022e6b87d87dcf88bf0088/salt/modules/groupadd.py#L33 (and it works, in fact, since module win group virtual name). When I get os_family in add() implementation I need it, but maybe this quirk is only test related ?

thank you !

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/saltstack/salt/issues/42739#issuecomment-324055538, or mute the thread https://github.com/notifications/unsubscribe-auth/AAssodtO1D_uoOudmWNZsd5265AYH_asks5sau46gaJpZM4Otii1 .

aogier commented 7 years ago

Ok, first of all I'll try to reproduce aganist a vanilla develop branch by just implementing an integration test that should fail because no grains on already existing code. Then we will see :)

aogier commented 7 years ago

I looked in the wrong place, failing tests are unit not integrations and the culprit is grains patching like here or here. I'll add Synology support and we should be ok !

gtmanfred commented 7 years ago

ahh, that would do it!

On Wed, Aug 23, 2017 at 8:35 AM, Alessandro Ogier notifications@github.com wrote:

I looked in the wrong place, failing tests are unit not integrations and the culprit is grains patching like here https://github.com/saltstack/salt/blob/b3aa9ab394a0028cc59a2d38d902f4136f6f869f/tests/unit/modules/test_groupadd.py#L38 or here https://github.com/saltstack/salt/blob/b3aa9ab394a0028cc59a2d38d902f4136f6f869f/tests/unit/modules/test_groupadd.py#L130. I'll add Synology support and we should be ok !

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/saltstack/salt/issues/42739#issuecomment-324354667, or mute the thread https://github.com/notifications/unsubscribe-auth/AAssoV2IUXohWrBUZDmgxnmWSmwg52v2ks5sbDjHgaJpZM4Otii1 .

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

saltstack / salt