Rewrite init script #178

2021-06-12T18:32:57Z

justusbunsi commented

2021-06-12 18:32:57 +00:00

(Migrated from gitea.com)

These changes rewrite the init script to be error aware, informative and have a bit more security awareness.

During rewrite several hidden bugs could be identified and fixed, such as:

LDAP configuration options interpreted by the shell before passed to command
Finding multiple ldap ids instead of one during lookup when their names are almost identical
e.g. _my-ldap-auth and my-ldap-auth
Properly filter auth sources by their types to prevent unintended type converting attempts that fail

In addition to that the script is a bit cleaner. Some commands do not exist anymore and would cause false-positive errors during script execution.

Helps for: #149

These changes rewrite the init script to be error aware, informative and have a bit more security awareness. During rewrite several hidden bugs could be identified and fixed, such as: - LDAP configuration options interpreted by the shell before passed to command - Finding multiple ldap ids instead of one during lookup when their names are almost identical e.g. `_my-ldap-auth` and `my-ldap-auth` - Properly filter auth sources by their types to prevent unintended type converting attempts that fail In addition to that the script is a bit cleaner. Some commands do not exist anymore and would cause false-positive errors during script execution. Helps for: #149

justusbunsi commented

2021-06-12 18:35:51 +00:00

(Migrated from gitea.com)

I've tested all changes for every possibility I could think of (probably not all ?). Even switching between both available images (root, rootless) to verify interoperability.

Some things (not really related to my changes) didn't work (properly):

Using not-active, skip-tls-verify, allow-deactivate-all, synchronize-users and attributes-in-bind for LDAP configuration. They are boolean values and would be passed like --not-active '' which breaks the command for some reason.
~~Same for use-custom-urls for OAuth command.~~ (Edit: incorrect statement)

~~Should I open separate issues for this or do we fix boolean option passing in this PR as well since they would break the init script?~~ (Edit: Yes, fix in this PR as well.)

I've tested all changes for every possibility I could think of (probably not all ?). Even switching between both available images (root, rootless) to verify interoperability. Some things (not really related to my changes) didn't work (properly): Using _not-active_, _skip-tls-verify_, _allow-deactivate-all_, _synchronize-users_ and _attributes-in-bind_ for LDAP configuration. They are boolean values and would be passed like `--not-active ''` which breaks the command for some reason. ~~Same for `use-custom-urls` for OAuth command.~~ (Edit: incorrect statement) ~~Should I open separate issues for this or do we fix boolean option passing in this PR as well since they would break the init script?~~ (Edit: Yes, fix in this PR as well.)

justusbunsi (Migrated from gitea.com) reviewed 2021-06-12 18:37:38 +00:00

justusbunsi commented

2021-06-14 17:06:40 +00:00

(Migrated from gitea.com)

I just re-read the issue #149 and am sure that this PR will not fully fix it. The repeated failure on init container due to unavailable db is done and the script is more verbose. But right now no credentials are used for the db check.

luhahn commented

2021-06-15 20:30:17 +00:00

(Migrated from gitea.com)

really like what you did here, will test this in a few days

luhahn (Migrated from gitea.com) approved these changes 2021-06-29 08:22:34 +00:00

luhahn (Migrated from gitea.com) left a comment

Tested this PR in different clusters, with existing PVCs and new PVCs.
Looks good :)

Tested this PR in different clusters, with existing PVCs and new PVCs. Looks good :)

justusbunsi commented

2021-06-29 18:50:58 +00:00

(Migrated from gitea.com)

@luhahn Done. Ready for another review.

techknowlogick (Migrated from gitea.com) approved these changes 2021-06-29 20:09:08 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: lunny/helm-chart#178