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

feat: add entrypoint script, which can create a user and related database, to the full Docker image #1722

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

Conversation

tauu
Copy link
Contributor

@tauu tauu commented Jun 27, 2023

This PR adds an entrypoint script to the full container image. This script creates a database and a user which is granted readwrite access to this database. The intention of this change is to facilitate deployments. It allows to define a user without admin priveleges to be used by other services directly in e.g. a docker-compose.yml oder a helm values.yml file and create it automatically. Currently only an admin user is available during the deployment, but using an admin user for services accessing the database is not a preferred option.

The credentials of the to be crated user as well as the name of the database can be set using the environment variables:

  • IMMUDB_USER
  • IMMUDB_PASSWORD
  • IMMUDB_PASSWORD_FILE
  • IMMUDB_DATABASE

Thereby IMMUDB_PASSWORD_FILE specifies a file that contains the password.

@jeroiraz
Copy link
Contributor

thanks @tauu!

@SimoneLazzaris could you please check it?

@SimoneLazzaris
Copy link
Collaborator

I get the point of the PR but I have two issues:

  • everything would be much simpler if immuadmin were better designed for scripting. As it is, it seems that the main usage model is the interactive one, and that force scripts to use weird trick in order to use it non-interactively.
    Some work on that front would also be a welcome addition.
  • this init script starts immudb, adds users and database, then stop it and then start it again. I think it would be more pratical to spawn a child process that waits for immudb to get online, create users and database and then exits, so you can start immudb just one time.

@tauu
Copy link
Contributor Author

tauu commented Jun 30, 2023

Regarding the two issues:

  • I very much get, that a non interactive workflow for immuadmin would be preferable (and would have made my life creating the script a lot easier). If this is considered a requirement to get it merged, I can certainly look into that and hopefully add command line options to provide credentials for performing the operations without a prior login / confirming passwords.
  • The intention for starting an immudb instance and stopping it after the setup, is to prevent other services from connecting to it during the setup. If another service depends on the to be created user/database, it may fail to start. Therefore this temporary instance only listens on localhost, which should prevents the container from being considered ready during the setup and depended services should not start before the setup completes. To be fair considering the time it takes to create the database and user, it currently seems unlikely that a service will connect before the setup completes. Nevertheless it is possible and if the setup script is extended later on by e.g. also loading initialisation data / sql it might very well take longer and make it more likely. This behaviour is also in line with the process performed by entrypoint scripts of other databases as e.g. mysql/mariadb. The temporary server is also only used the first time the container starts.

What are your thoughts regarding the just described intention for using a temporary server for the setup process?

tauu added 22 commits July 8, 2023 01:46
If a command requires a logged in client, a pre run is set for the the
major level (e.g. user, database) otherwise the command will just inherit the
default from the root command.
Reusing the same command line struct for all command unifies the
initialization of immuclient. Furthermore it enables the shell to reuse
the same connection for all commands.
@tauu
Copy link
Contributor Author

tauu commented Jul 14, 2023

As suggested some work to make immuadmin more scriptable has been added to the PR. This turned into a quite significant number of changes to make all commands consistently scriptable. The major changes made are as follows.

  • Added flags to specify all necessary information for connecting to immudb directly in on command line.
    • --username (default immudb) for the user
    • --password-file (no default) for the a file containing the corresponding password
    • --database (default defaultdb) for the database
    • --dir (no default) program directory
  • Replace using the deprecated client.Connect / client.Disconnect api with using the new client.OpenSession / client.CloseSession api (see commandline.connect / commandline.disconnect functions)
  • Change logic for connecting to immudb and remove the requirement of using login / logout commands. New logic (see commadline.connect ):
    • If the command requires a connection to an immudb instance, try to open a session in the pre run function.
    • Credentials and database for the session are read from the flags.
    • If the --password-file flag is set, read the password from the file.
    • Otherwise prompt the user to enter the password manually.
  • Add the --new-password-file flag to the user create command. If set, the password for the new use is read from this file, instead of requesting the user to enter one.
  • Add the --non-interactive / -y flag. If it is set, immuadmin requires passwords to be specified using command line flags. If not set, immuadmin exits and instructs the user to set them. Furthermore if a confirmation from the user is requested (as e.g. in the backup command), the request will always return a positive answer without any user interaction (see askYN function)
  • As the old interactive workflow of using immuadmin login, executing commands and finishing with immuadmin logout is not compatible with the OpenSession & CloseSession api (as far as I can tell at least), this workflow was replaced with an interactive shell powered by go-prompt. The shell is started by running immuadmin shell and accepts any of the regulars commands including login and logout. The same session will be used for all commands, until a new one is created by issuing a login/logout command.

In addition a few things had to be adjusted in immuadmin for being able to implement the shell:

  • All commands inherit the PersistentPostRun function from the root command that closes any open session.
  • If a command requires a connection with an immudb instance, the connection is always established in the major level PerstientPreRunE function. E.g. for the user list command, the user command has the PersistentPreRunE function set with the connect function. These pre run functions are set to nil when executed by the shell, to prevent unnecessary reinitializations.
  • The backup and hot-backup command lines are changed to embed a pointer to main command line struct instead of creating a new one. (This way all commands can use the same session if called from the shell.)
  • All commands are now consistently returning errors in the RunE function. Beforehand some commands did it and some directly exited the program. As the commands should not exit the program when run in a shell, this behaviour was changed to return errors.

For the new flags as well as the new connection handling, unit tests were created and included. Most of the new tests are in commandline_test.go and verify that the new flags work as intended.

As a lot of changes were made through out the immuadmin code, the already existing tests for immuadmin were reenabled. A lot of them seem to have been commented out. To reenable them, the test setup has been changed and unified to create a test immudb server and a commandline struct the same way for each test. Apart from changing the setup part of the tests, no functional part was changed. Currently all tests pass.

The initial entrypoint script was also adjusted to use immuadmin using the newly introduced features and simplifies the script.

@SimoneLazzaris is this in line with your suggestion to make immuadmin more scriptable? I am looking forward to a review of the PR and apologise in advance for the rathe big number of changed lines. Getting immuadmin consistently scriptable required more changes than expected. 😅

@tauu
Copy link
Contributor Author

tauu commented Sep 14, 2023

May I inquire about the status of this PR? Is there something holding it or can I do something to facilitate handling it? :)

@jeroiraz
Copy link
Contributor

May I inquire about the status of this PR? Is there something holding it or can I do something to facilitate handling it? :)

Hi @tauu , we'll start the review of newer changes tomorrow

cc: @SimoneLazzaris

@tauu
Copy link
Contributor Author

tauu commented Nov 15, 2023

How is the review going? Can I be of any assistance?

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.

None yet

3 participants