SystemRage / py-kms

KMS Server Emulator written in Python
The Unlicense
2.04k stars 618 forks source link

Specifying client count causes error & program exit #75

Closed Dreikasehoch closed 3 years ago

Dreikasehoch commented 3 years ago

Expected Behavior

When starting pykms_Server.py with the client count flag (either -c or --client-count), the provided client count value should be honored by the application and everything should run as usual.

Actual Behavior

No matter what value is given for client-count, the application writes an error message to console then exits. e.g. for the case where -c 30 is given, the application will print argument `-c/--client-count`: invalid with: '30'. Exiting...

This is a tricky problem for Docker users since there's no option to skip the -c flag with the current Dockerfile.

Steps to Reproduce the Problem

  1. Create a new python environment with all the needed prerequisites.
  2. Clone the repo
  3. Start py-kms using /usr/bin/python3 pykms_Server.py :: 1688 -c 30

Specifications

lepoarro commented 3 years ago

Same problem here with latest docker (pykmsorg/py-kms:latest) version running on Unraid argument '-c/--client-count': invalid with: '26'. Exiting.... Previous version worked flawlessly!

libots commented 3 years ago

As a temporary fix for Docker users, you can specify the entrypoint manually (removing the -c flag). I added the following to my docker-compose.yaml: entrypoint: ["/bin/sh","-c","/usr/bin/python3 pykms_Server.py :: 1688 -l 1033 -a 120 -r 10080 -w RANDOM -V INFO -F /var/log/py3-kms.log"], which has it working now.

lepoarro commented 3 years ago

Under Unraid OS you can't really manipulate Docker environment the way @libots proposed. What you can do is edit/change environment variables and that's just about it. Right now I got it working in small Debian VM by directly cloning from github and running the python script without the -c flag. I really hope they fix the Dockerfile.

cfredericksen commented 3 years ago

I added the following to my k8s pykms config to override the ENTRYPOINT.

    command: ["/bin/sh"]
    args: ["-c", "/usr/bin/python3 pykms_Server.py :: 1688 -l 1033 -a 120 -r 10080 -w RANDOM -V INFO -F /var/log/py3-kms.log"]
Dreikasehoch commented 3 years ago

So the issue seems to come down to this sanity check on the input params where it iterates over a concatenated array of input argument values for clientcount, timeoutidle, etc; doing a quick check for null values or data of a wrong type. https://github.com/SystemRage/py-kms/blob/9d9a3639e186734e0650a8bec5bc2a7033295cc9/py-kms/pykms_Server.py#L397-L399 Sadly, the statement will always evaluate to True and panic out since the datatype of clientcount is a string as specified below instead of an int as expected in the check. https://github.com/SystemRage/py-kms/blob/9d9a3639e186734e0650a8bec5bc2a7033295cc9/py-kms/pykms_Server.py#L196-L197

Has a conversion from string to int for input arguments been lost recently? These segments of code are multiple months old and I can't think of why they'd work until just recently.

simonmicro commented 3 years ago

Hmm, we recently moved to only support Python3 - maybe the old Python2 instance does not had that problem, but since we changed the python version all people are updating their clients and therefore may have found a undiscovered bug. Maybe the used libs changed. But the possibility for that is not very high. Fell free to fix it - I currently have no time for further investigation (sadly)...

srepac commented 3 years ago

Old version of py-kms used type=int

parser.add_argument("-c", "--client-count", dest = srv_options['count']['des'] , default = srv_options['count']['def' ], help = srv_options['count']['help'], type = int)

really that should be the only change that needs to be done to fix this. I made the edit but someone needs to approve it. We need this fixed asap for docker users.

spitfire commented 3 years ago

Seems to still be an issue with docker - See #80

simonmicro commented 3 years ago

@Dreikasehoch @spitfire Fixed (both in repo and in docker). Please close this issue now.

Dreikasehoch commented 3 years ago

The fix looks good, thanks!