TZ from env, client log, alpine 3.14, path DB changed. #13

edgd1er · 2021-10-15T00:01:16Z

May be too many change for a single PR. this is still a work in progress as I wish to run as user py-kms with specific uid/gid.
Please note that also database path has been changed to allow volume or file mounting: /db/pykms_database.db

3 python libs updates due to security fix.

simonmicro

Additionally could you think about merging start and entrypoint.py? As the user should be able to execute the scripts as one package - also the entrypoint should not try to change the uid/gui except he has the permissions for that.

docker/start.py

simonmicro · 2021-10-16T12:22:29Z

docker/start.py

+import subprocess
+import time
+
+LTIME = '/etc/localtime'


And this is used for... ?

simonmicro · 2021-10-16T12:24:12Z

docker/start.py

+  '-e': 'EPID'
+}
+enableSQLITE = os.getenv('SQLITE', 'false').lower() == 'true'
+dbPath = os.path.join('/db/pykms_database.db')


Okay, that is wrong - os.path.join() needs to take the path elements as single arguments to ensure cross-platform behavior.

Additionally please do not put it into root - the docker user should have to perms to do that anyways.

docker/start.py

simonmicro · 2021-10-16T12:28:25Z

docker/start.py

+      client_cmd.append(os.environ.get('LOGSIZE'))
+
+    if log_level.lower() in ['info', 'debug']:
+      print("Starting a dummy activation to ensure the database file is created", flush=True)


Remove the flush=True please. We should instead set the environment variable PYTHONUNBUFFERED to 1.

I was not sure what was best. I guessed flushed= true had only a local impact. python unbeferred has a more global impact, but is you wish.

@simonmicro I agree with @edgd1er I am not sure that setting PYTHONUNBUFFERED to 1 is a good idea, but we could probably meet in the middle somewhere by adding a switch to change buffering behaviour of print() for those who want it.

Is the change to buffering behaviour to stop log output from being garbled?

simonmicro · 2021-10-16T12:29:43Z

docker/start.py

+      command.append(arg)
+      command.append(os.environ.get(env))
+
+  os.makedirs(os.path.dirname(dbPath), exist_ok=True)


Whoops, that's probably on me: Move that into the if enableSQLite: please

docker/start.py

simonmicro · 2021-10-16T12:33:42Z

docker/entrypoint.py

+    os.symlink(os.path.join('/usr/share/zoneinfo/', tz), LTIME)
+
+
+LTIME = '/etc/localtime'


Sooo, you define a global variable after using it? And expect implicit binding? Could you at least make it more readable by adding global to the function or pass it directly?

I guess, that was before I changed all. for the moment, it is still a work in progress. It should be corrected.

simonmicro · 2021-10-16T12:36:17Z

docker/entrypoint.py

+  gid = user_grp_db_entries.gr_gid
+  new_gid = int(os.getenv('GID', str(gid)))
+  new_uid = int(os.getenv('UID', str(uid)))
+  os.chown("/home/py-kms", new_uid, new_uid)


Is this recursive?

no recursivity here, just get the env value or a default one if none provided.

simonmicro · 2021-10-16T12:37:00Z

docker/entrypoint.py

+  os.chown("/home/py-kms", new_uid, new_uid)
+  os.chown("/db/pykms_database.db", new_uid, new_uid)
+  if gid != new_gid:
+    print("Setting gid to " + str(new_gid), flush=True)


I've seen you added conditional logging below - do you also intend to do that here? Please be consistent.

simonmicro · 2021-10-18T19:03:49Z

Oh, this is still WIP? Sorry for already taking a look into it! I'll mark the PR as such.

edgd1er · 2021-10-27T17:17:37Z

you may have a look to changes if you wish.

simonmicro · 2021-10-30T09:33:51Z

Okay, I'm still not 100% percent conform with the new start.py but whatever - let's ignore that. But still please integrate 4ee742d and 677916a of the current upstream, because this makes the script at least some sort more readable (and you have somehow dropped the os.makedirs() instruction. After that: I'll hit "merge" faster than you can comment it! 😄

simonmicro · 2021-10-30T09:35:22Z

docker/start.py

+      command.append(os.environ.get(env))
+
+  if enableSQLITE:
+    loggersrv.info("Storing database file to %s" % dbPath)


Please add 4ee742d

simonmicro · 2021-10-30T09:36:43Z

docker/start.py

+def start_kms_client():
+  if not os.path.isfile(dbPath):
+    # Start a dummy activation to ensure the database file is created
+    client_cmd = [PYTHON3, '-u', 'pykms_Client.py', os.environ.get('IP', "0.0.0.0"), os.environ.get('PORT', 1688),


And also here 4ee742d

https://www.python.org/dev/peps/pep-0008/#function-and-variable-names

edgd1er · 2021-10-30T10:45:58Z

changes are implemented. as a side-note, I'm not sure being able to change listening port for kms and websqlite is very useful. Being in a container, ports are reassigned, except when network is inhost-mode.

Matthew-Beckett · 2021-10-30T16:48:21Z

changes are implemented. as a side-note, I'm not sure being able to change listening port for kms and websqlite is very useful. Being in a container, ports are reassigned, except when network is inhost-mode.

Can you expand on that part about the port reassignment? Docker ports are configurable and static in my experience.

edgd1er · 2021-10-30T17:39:14Z

changes are implemented. as a side-note, I'm not sure being able to change listening port for kms and websqlite is very useful. Being in a container, ports are reassigned, except when network is in host-mode.

Can you expand on that part about the port reassignment? Docker ports are configurable and static in my experience.

for me, internal port = port exposed inside the container, external port is the port exposed on the host.

when using bridge mode, an "external" port is mapped to the "internal" port: docker run -p 1080:8080 ... => access through host_ip:1080
when using host mode, only the exposed port(s), formerly the internal one is used: docker run -d --network host .... => access through host_ip:8080

As far as I know, most usage are in bridge mode to have network isolation, as such, changing internal ports are useless.
kms_server has port assignement as it may also be executed without docker, and then ports assignements might be useful.

websqlite is defined only in the container, defining internal port (inside the container) has no use when running in bridge mode.

The question: do we need to give the opportunity to define (and maintain) the "internals" ports ( kms, web sqlite) feature, knowing most of the time, the container will be running in bridge mode ?

I might be wrong assuming host mode is rarely used.

Matthew-Beckett · 2021-10-30T17:41:08Z

changes are implemented. as a side-note, I'm not sure being able to change listening port for kms and websqlite is very useful. Being in a container, ports are reassigned, except when network is in host-mode.

Can you expand on that part about the port reassignment? Docker ports are configurable and static in my experience.

for me, internal port = port exposed inside the container, external port is the port exposed on the host.

when using bridge mode, an "external" port is mapped to the "internal" port: docker run -p 1080:8080 ... => access through host_ip:1080

when using host mode, only the exposed port(s), formerly the internal one is used: docker run -d --network host .... => access through host_ip:8080

As far as I know, most usage are in bridge mode to have network isolation, as such, changing internal ports are useless.

kms_server has port assignement as it may also be executed without docker, and then ports assignements might be useful.

websqlite is defined only in the container, defining internal port (inside the container) has no use when running in bridge mode.

The question: do we need to give the opportunity to define (and maintain) the "internals" ports ( kms, web sqlite) feature, knowing most of the time, the container will be running in bridge mode ?

I might be wrong assuming host mode is rarely used.

I understand what you mean now, I agree with you.

simonmicro · 2021-10-31T14:10:48Z

It is just the same philosophy for the ports as also for your uid/gid reassignment. After all docker allows you to remap our default docker uid of 1000 to any other arbitrary id as you need. But beside that the port specification could be really interesting in case you have to use the host networking mode - e.g. in case you run on a swarm and do not want to utilize load-balancing and be only available on an specific node.

simonmicro · 2021-10-31T14:14:35Z

Okay, let's hope this does not break anything!

simonmicro · 2021-10-31T14:20:00Z

Aaand the Dockerfile is broken. @edgd1er please fix this and test it this time.

edgd1er force-pushed the master branch 4 times, most recently from ff3c0ca to 776dcba Compare October 15, 2021 21:20

simonmicro requested changes Oct 16, 2021

View reviewed changes

This was linked to issues Oct 18, 2021

Something is broken in TZ... #10

Closed

Set own UID/GID in Docker #15

Closed

simonmicro marked this pull request as draft October 18, 2021 19:06

edgd1er force-pushed the master branch from 776dcba to f79f02b Compare October 22, 2021 21:39

edgd1er added 4 commits October 25, 2021 22:05

lint dockerfile, set TZ and UID/GID, have client log.

f9e7df6

start as root, change uid/gid, drop priv, run server/client

d2394cd

use unbuffered output globally.

6d71244

add proper logger to entrypoint and start

5f7ef13

edgd1er force-pushed the master branch from f79f02b to 5f7ef13 Compare October 26, 2021 07:59

add healthcheck with netcap checking port

a564822

edgd1er marked this pull request as ready for review October 27, 2021 17:16

Matthew-Beckett assigned edgd1er Oct 29, 2021

Matthew-Beckett added the enhancement New feature or request label Oct 29, 2021

add missing setting to timzeone.

2979e78

simonmicro self-requested a review October 30, 2021 09:17

simonmicro requested changes Oct 30, 2021

View reviewed changes

var added as requested and renamed as per PEP-008

51e2210

https://www.python.org/dev/peps/pep-0008/#function-and-variable-names

simonmicro merged commit 70975bb into Py-KMS-Organization:master Oct 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TZ from env, client log, alpine 3.14, path DB changed. #13

TZ from env, client log, alpine 3.14, path DB changed. #13

edgd1er commented Oct 15, 2021

simonmicro left a comment

simonmicro Oct 16, 2021

simonmicro Oct 16, 2021

simonmicro Oct 16, 2021

simonmicro Oct 16, 2021

edgd1er Oct 17, 2021

Matthew-Beckett Oct 29, 2021

simonmicro Oct 16, 2021

simonmicro Oct 16, 2021

edgd1er Oct 17, 2021

simonmicro Oct 16, 2021

edgd1er Oct 17, 2021

simonmicro Oct 16, 2021

simonmicro commented Oct 18, 2021

edgd1er commented Oct 27, 2021

simonmicro commented Oct 30, 2021

simonmicro Oct 30, 2021

simonmicro Oct 30, 2021

edgd1er commented Oct 30, 2021

Matthew-Beckett commented Oct 30, 2021

edgd1er commented Oct 30, 2021

Matthew-Beckett commented Oct 30, 2021

simonmicro commented Oct 31, 2021

simonmicro commented Oct 31, 2021

simonmicro commented Oct 31, 2021

		os.symlink(os.path.join('/usr/share/zoneinfo/', tz), LTIME)


		LTIME = '/etc/localtime'

TZ from env, client log, alpine 3.14, path DB changed. #13

TZ from env, client log, alpine 3.14, path DB changed. #13

Conversation

edgd1er commented Oct 15, 2021

simonmicro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonmicro commented Oct 18, 2021

edgd1er commented Oct 27, 2021

simonmicro commented Oct 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgd1er commented Oct 30, 2021

Matthew-Beckett commented Oct 30, 2021

edgd1er commented Oct 30, 2021

Matthew-Beckett commented Oct 30, 2021

simonmicro commented Oct 31, 2021

simonmicro commented Oct 31, 2021

simonmicro commented Oct 31, 2021