lunny/helm-chart
1
0
Fork 1
Code Issues36 Pull Requests14 Actions Packages Projects Releases93 Wiki Activity

Rewrite init script #178

Merged
justusbunsi merged 1 commits from refs/pull/178/head into master 2021-06-29 20:09:16 +00:00
Conversation 7 Commits 1 Files Changed 2 +11
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:

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 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.

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

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.

@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.
No Reviewers
luhahn
techknowlogick
No Label
kind/enhancement
kind/refactor
No Milestone Release 4.0.0
No project
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: lunny/helm-chart#178
No description provided.
Delete Branch "refs/pull/178/head"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?