Task #3407
open
Prepare appliance for roles
Added by Boris Polák 11 months ago. Updated 6 months ago.
90%
Description
Hub should be able to load user roles and info from LDAP/CAS
- add roles
- full name (first + last)
Updated by Peter Štrunc 7 months ago
Imeplemnted permissions attribute to our LDAP + added versioning mechanism.
PR for ldap docker is here https://github.com/bcvsolutions/openldap-docker/pull/6
@fiserp could you check it out please?
IdM module is still in progress.
Updated by Petr Fišer 7 months ago
- The trap in the entrypoint.sh is not working properly. I believe it is because of the
SIG-naming of signals in combination with running the script as plainsh. If you write it liketrap 'handle_signal' TERM INTthen it should register properly. - Not sure if the trap will work anyway, because
/container/tool/runis a python script which may or may not(!) propagate signals further. Needs further testing. - There is some directory missing:
openldap | BCV INIT ldiff directory not found: /bcv/changefiles/, or rather, inconsistent with the value of$CHANGEFILES_PATH.- Which means that changescripts are not located at all...? How come that testing did not catch this?
- Changefiles should be ended
.ldifand not.ldiff. It is a difference between proper syntax marking in the editor and a wall-of-text. - It would be better to have
ou=versions,..object inside the config store, not the user store. Unless there is specific reason for it...? - Why is the schema object for
iamVersionStorrageobjectclass created in the entrypoint script? It could be a changescript in itself... or not? - Target container version is to be
1.4.0-r3, because the1.4.0is version of openldap and that remains unchanged. - The
tail -f /dev/nullat the end of entrypoint is pretty funky. I think using shell'swaitcommand is better. May need to change the executing shell fromshtobashbut that should not be a problem. - There is no documentation of what all this does. That one-liner in the readme feels more like a trolling attempt.
Updated by Peter Štrunc 6 months ago
- Status changed from In Progress to Needs feedback
- Assignee changed from Peter Štrunc to Petr Fišer
- % Done changed from 80 to 90
Hey @fiserp, thanks for the helpful feedback. I managed to fix all of the mentioned issues, here are some notes:
- The trap in the entrypoint.sh is not working properly. I believe it is because of the `SIG-` naming of signals in combination with running the script as plain `sh`. If you write it like `trap 'handle_signal' TERM INT` then it should register properly.
- It seems that the issue was with ENTRYPOINT syntax, because the script was not running under pid 1. When i changed it to array notation, it propagates as expected. Also there was an issue with how i tested the container stopping, because i only did ctrl+c on docker compose up process, which was not sending the signal in the first place.
- Not sure if the trap will work anyway, because `/container/tool/run` is a python script which may or may not(!) propagate signals further. Needs further testing.
- Here i needed to add another wait after killing the run process and now it seems to be working like a charm
openldap | entrypoint.sh(1)-+-pstree(1750)---{pstree}(1752)
openldap | |-run(8)-+-runsvdir(1586)-+-runsv(1589)-+-cron(1598)---{cron}(1616)
openldap | | | | `-{runsv}(1595)
openldap | | | |-runsv(1591)-+-slapd(1601)-+-{slapd}(1621)
openldap | | | | | |-{slapd}(1625)
openldap | | | | | |-{slapd}(1626)
openldap | | | | | `-{slapd}(1630)
openldap | | | | `-{runsv}(1596)
openldap | | | |-runsv(1593)-+-syslog-ng(1600)---{syslog-ng}(1617)
openldap | | | | `-{runsv}(1597)
openldap | | | `-{runsvdir}(1588)
openldap | | `-{run}(10)
openldap | `-{entrypoint.sh}(7)
openldap | Killing process 8
openldap | 65480876 daemon: shutdown requested and initiated.
openldap | 65480876 slapd shutdown: waiting for 0 operations/tasks to finish
openldap | Nov 5 21:26:14 openldap syslog-ng[1600]: syslog-ng shutting down; version='3.19.1'
openldap | 65480876 slapd stopped.
openldap | *** Killing all processes...
openldap | entrypoint.sh(1)-+-pstree(1763)---{pstree}(1765)
openldap | `-{entrypoint.sh}(7)
- There is some directory missing: `openldap | BCV INIT ldiff directory not found: /bcv/changefiles/` , or rather, inconsistent with the value of `$CHANGEFILES_PATH`. Which means that changescripts are not located at all...? How come that testing did not catch this?
- Yeah, this was last minute refactoring and it slipped under my nose. Fixed
- Changefiles should be ended `.ldif` and not `.ldiff`. It is a difference between proper syntax marking in the editor and a wall-of-text.
- Fixed
- It would be better to have `ou=versions,..` object inside the config store, not the user store. Unless there is specific reason for it...?
- I did try it, but I am getting the following error: Object class violation (65). I have troubleshooted the error and it seems that by default cn=config is not suited for objects with structural object classes. At least thats what smart people on the internet say https://www.openldap.net/lists/openldap-technical/201009/msg00351.html. I also havent found anything about it here https://www.zytrax.com/books/ldap/ch6/slapd-config.html. So i propose maintaining it under dc=xxx,dc=yyy as it is now.
- Why is the schema object for `iamVersionStorrage` objectclass created in the entrypoint script? It could be a changescript in itself... or not?
- At the time of writing that part, the function performOperation did not support cn=config modification. Now it may be done and i moved it there. The ldapadd command itself needs to be in the script though, because the dn of versions object is computed from env variable.
- Target container version is to be `1.4.0-r3`, because the `1.4.0` is version of openldap and that remains unchanged.
- Fixed
- The `tail -f /dev/null` at the end of entrypoint is pretty funky. I think using shell's `wait` command is better. May need to change the executing shell from `sh` to `bash` but that should not be a problem.
- Changed to wait
- There is no documentation of what all this does. That one-liner in the readme feels more like a trolling attempt.
- I get why you would feel like this, but be sure that this was not my intent. My reasoning was that i felt LDAP versioning feature was mainly to work in the background as someting, that users of the container would not need to interract with, hence the brief note in the readme. I tried to add plenty of comments in the code for someone who would need to delve more deeply into what is being done there, or maybe fix a bug. That being said, i wrote more descriptive documentation of this feature and added it to the README. Let me know, whether you find it sufficient or not.
I recreated the PR here https://github.com/bcvsolutions/openldap-docker/pull/7
I would appreciate if you could look at it again. Thanks in advance.
Updated by Petr Fišer 6 months ago
@sourek
I read through it and it looks quite nice. Thanks. :)
Could you give me some clarification on the following?- Changescripts are applied in no particular order (well, in alphabetical order to be exact, but it is not enforced anywhere). Right?
- If that's true, then changescripts are usable only for adding isolated entries. Not suitable for modifying something that already exists. Because we are not able to enforce their order.
- What do I need to do to write a changescript?
- Nothing on this in the docs and nothing obvious in the code.
- There seems to be a possibility to re-run a changescript. Do we really want that? The Flyway-like approach is more appealing to me.
- The
handle_signaldoes not need towaitif it does not containexit. This should work:handle_signal() { # Kill the /container/tool/run process echo "Killing process $RUN_PID" kill -TERM $RUN_PID }
- Grepping the ldapsearch output is inherently unsafe.
- When there are long values, the ldif output will be line-wrapped to 80 chars or so and your grep will fail. There is an option
-o ldif-wrap=noto the ldapsearch to prevent wrapping. - In case the attribute value contains non-ASCII characters, its output representation will be base64-encoded which is denoted by
::in the output:attributeName:: attributeValueInBase64instead ofattributeName: attributeValue.
- When there are long values, the ldif output will be line-wrapped to 80 chars or so and your grep will fail. There is an option
load_passwordsshould notexportthe password in a variable because it makes it visible for everybody down the process tree. IMO theexportis unnecessary.
However, I will probably need to do some structural changes to properly integrate it with appliance ecosystem (I will implement them, no worries). It boils down mainly to proper upgrade process on already running deployments.
So there is probably no reason for you fixing the aforementioned bugs. But, please, clarify on those two questions I had.
Once I have this info, I can take it from there and build on top of it.
Updated by Peter Štrunc 6 months ago
Hey @fiserp,
both points are valid.
Regarding the changescript order, i did not consider script ordering for first version of this feature since i did not need it.
As for the changescript writing guide, i will add the doc to README, but let me know if i should do that since you intent to change this behavior.
When i was testing handle_signal, the script ended immediately instead of waiting for all stuff to finish. This was the reason i added the wait, but maybe i did something wrong.
Updated by Petr Fišer 6 months ago
Ticket moved to https://redmine.bcvsolutions.eu/issues/27573 .