Adds project_openldap plugin #696
Conversation
|
|
||
| #### GID: | ||
|
|
||
| - **NOTE: A starting GID number to increment posixgroup GIDs is required. Consider this carefully before going into project to avoid clashes or problems.** |
There was a problem hiding this comment.
are there any protections against clashes? could it just check if a GID already exists and step over it?
There was a problem hiding this comment.
Using 8000 as the start GID, this is project 11 (pk=11), so 8000+11 is the resultant gidNumber.
I guess stepping over wouldn't work
There was a problem hiding this comment.
no protection at the moment ... so say your site might expect 3000 projects (overestimate) within the coldfront instance's lifetime (which is possibly directly coupled to a resource's lifetime .. eg. an HPC cluster that last 5-7 years) ... then you'd have to leave that contiguous block of 3k
- perhaps this will be a future enhancement
- currently at our site we have GIDs spaced apart by a large margin
a concern with non-contiguous might be harder to fix if it goes awry (or at least think about the state as an admin under pressure) ... e.g. in a situation where you require a re-sync or something like that .. also contiguous is easy for operators ... project 49 will have START GID + 49 ...
There was a problem hiding this comment.
it could also just be an error in the sync script
There was a problem hiding this comment.
To address this I just enabled the slapo-unique openldap overlay/module, which will give the sync script an error whenever it tries to assign a duplicate uidNumber/gidNumber/uid.
There was a problem hiding this comment.
dn: cn=module{0},cn=config
changetype: modify
add: olcModuleLoad
olcModuleLoad: unique
ldapmodify -Y EXTERNAL -H ldapi:/// -f ~/load-unique.ldif
dn: olcOverlay=unique,olcDatabase={1}mdb,cn=config
objectClass: olcOverlayConfig
objectClass: olcUniqueConfig
olcOverlay: unique
olcUniqueAttribute: uid
olcUniqueAttribute: uidNumber
olcUniqueAttribute: gidNumber
ldapmodify -Y EXTERNAL -H ldapi:/// -f ~/configure-unique.ldif
|
|
||
| The aforemention signals trigger functions in tasks.py, these inturn use functions in ``utils.py`` to accomplish the action required. Examples are: | ||
|
|
||
| - create a per project OU and per project posixgroup within a Projects OU - **new project** |
There was a problem hiding this comment.
what's the motivation for creating a per-project OU?
There was a problem hiding this comment.
flexibility and further demarcation ... im thinking about adding a project's allocations underneath the per project OU - though that is just one approach for an allocation_openldap plugin
ds-04
force-pushed
the
project_openldap
branch
3 times, most recently
from
0c82b76 to
50fa30a
Compare
There was a problem hiding this comment.
This is a lot of really great content!
I left comments mostly concerning unhandled error cases, code maintaiability, spelling, and ways to structure the logic more in line with Django's conventions.
One of the other maintainers will be coming in (hopefully) soon with a more macro-level review concering functionality testing and checking to make sure this works well within coldfront as a whole. If you have any questions please feel free to @ me and I will get back to you as soon as I can!
|
|
||
| This plugin makes use of django signals within Coldfront to push project information to OpenLDAP. The main motivation for this is so projects and their members can be represented inside a _PosixGroup_, with usage of _memberUid_ (for members). Having this information in OpenLDAP facilitates the usage of filesystem quotas and other activities on a project basis. | ||
|
|
||
| **The motivation for using this plugin, is that you want Coldfront and it's WebUI to be the source of truth**. This might be in constrast to another operating modality where information is [generally] imported from another system or IdP into Coldfront. |
There was a problem hiding this comment.
"contrast" not "constrast"
| **The motivation for using this plugin, is that you want Coldfront and it's WebUI to be the source of truth**. This might be in constrast to another operating modality where information is [generally] imported from another system or IdP into Coldfront. | ||
| - You will still need some means of creating/registering users in your OpenLDAP, e.g. a user registration portal - that is not covered/provided here. If you are just testing, you might add these to the DIT with ldifs. | ||
|
|
||
| - **NOTE: The plugin doesn't write a Coldfront project's allocation(s) into OpenLDAP. It is expected a seperate plugin will do this.** |
There was a problem hiding this comment.
"separate" not "seperate"
|
|
||
| #### actions | ||
|
|
||
| The aforemention signals trigger functions in tasks.py, these inturn use functions in ``utils.py`` to accomplish the action required. Examples are: |
There was a problem hiding this comment.
"aforementioned" not "aforemention"
"in turn" not "inturn"
|
|
||
| - Projects get created in the Projects OU. A per project OU and a per project posixgroup is created. | ||
| - Per project OU's get moved to Archive OU or deleted on Coldfront WebUI archive action. | ||
| - **If no Archive OU** is setup by the site adminsitrator, then **projects** (per project OUs) are **deleted on archival**. |
There was a problem hiding this comment.
"administrator" not "adminsitrator"
| ```mermaid | ||
| %%{init: {'theme': 'forest', 'themeVariables': { 'fontSize': '20px', 'fontFamily': 'Inter'}}}%% |
There was a problem hiding this comment.
I love mermaid diagrams!
The only issue here is that the arrows are not being rendered on github's mermaid renderer. I think if you add 'darkMode': true to the theme variables that it will fix that. Otherwise might have to change the theme.
Reference I found: https://github.com/Gordonby/MermaidTheming/blob/ed362bee7950f9b3a84967a31551b2f7abebd664/README.md?plain=1#L61C1-L61C110
| def handle_project_removal_if_needed(project, project_ou_dn, sync=False): | ||
| if project.status_id not in [ | ||
| PROJECT_STATUS_CHOICE_NEW, | ||
| PROJECT_STATUS_CHOICE_ACTIVE, | ||
| ]: | ||
| # archive OU not defined, so remove this project | ||
| if PROJECT_OPENLDAP_REMOVE_PROJECT and not PROJECT_OPENLDAP_ARCHIVE_OU: | ||
| if not sync: | ||
| print( | ||
| f"{project_ou_dn} <<< WARNING WE EXPECTED THIS TO BE REMOVED IN OPENLDAP - PROJECT_OPENLDAP_ARCHIVE_OU NOT is set" | ||
| ) | ||
| print("Sync is required to make this change, please supply: -s or --sync") | ||
| if sync: | ||
| try: | ||
| remove_project(project) | ||
| print(f"Removed inactive project {project.project_code} from OpenLDAP - SYNC is {sync}") | ||
| except Exception as e: | ||
| print(f"Exception removing {project.project_code}, DN: {project_ou_dn} in OpenLDAP: {e}") | ||
|
|
There was a problem hiding this comment.
Consider breaking down large functions like this to reduce complexity. You can also use the inversion of your conditionals and quick returns to make the code more readable and less nested. For example:
| def handle_project_removal_if_needed(project, project_ou_dn, sync=False): | |
| if project.status_id not in [ | |
| PROJECT_STATUS_CHOICE_NEW, | |
| PROJECT_STATUS_CHOICE_ACTIVE, | |
| ]: | |
| # archive OU not defined, so remove this project | |
| if PROJECT_OPENLDAP_REMOVE_PROJECT and not PROJECT_OPENLDAP_ARCHIVE_OU: | |
| if not sync: | |
| print( | |
| f"{project_ou_dn} <<< WARNING WE EXPECTED THIS TO BE REMOVED IN OPENLDAP - PROJECT_OPENLDAP_ARCHIVE_OU NOT is set" | |
| ) | |
| print("Sync is required to make this change, please supply: -s or --sync") | |
| if sync: | |
| try: | |
| remove_project(project) | |
| print(f"Removed inactive project {project.project_code} from OpenLDAP - SYNC is {sync}") | |
| except Exception as e: | |
| print(f"Exception removing {project.project_code}, DN: {project_ou_dn} in OpenLDAP: {e}") | |
| def handle_project_removal_if_needed(project, project_ou_dn, sync=False): | |
| if project.status_id in [PROJECT_STATUS_CHOICE_NEW, PROJECT_STATUS_CHOICE_ACTIVE]: | |
| return # inverted conditional and return quickly instead | |
| # archive OU not defined, so remove this project | |
| if not PROJECT_OPENLDAP_REMOVE_PROJECT and PROJECT_OPENLDAP_ARCHIVE_OU: | |
| return # inverted conditional and return quickly instead | |
| if not sync: | |
| print( | |
| f"{project_ou_dn} <<< WARNING WE EXPECTED THIS TO BE REMOVED IN OPENLDAP - PROJECT_OPENLDAP_ARCHIVE_OU NOT is set" | |
| ) | |
| print("Sync is required to make this change, please supply: -s or --sync") | |
| else: | |
| try: | |
| remove_project(project) | |
| print(f"Removed inactive project {project.project_code} from OpenLDAP - SYNC is {sync}") | |
| except Exception as e: | |
| print(f"Exception removing {project.project_code}, DN: {project_ou_dn} in OpenLDAP: {e}") |
| def handle_description_update( | ||
| project, | ||
| ldapsearch_project_result=False, | ||
| ldapsearch_project_result_archive=False, | ||
| project_dn="", | ||
| archive_dn="", | ||
| sync=False, | ||
| write_to_archive=False, | ||
| ): | ||
| new_description = construct_project_posixgroup_description(project) # supply project_obj | ||
|
|
||
| if project.status_id in [PROJECT_STATUS_CHOICE_NEW, PROJECT_STATUS_CHOICE_ACTIVE]: | ||
| # fetch current description from project_dn | ||
| fetched_description = ldapsearch_get_project_description(project_dn) | ||
| if new_description == fetched_description: | ||
| print("Description is up-to-date.") | ||
| if new_description != fetched_description: | ||
| if sync: | ||
| update_project_posixgroup_in_openldap(project_dn, new_description, write=True) | ||
| print(f"{new_description}") | ||
| else: | ||
| # line up description output | ||
| print(f"OLD openldap_description is {fetched_description}") | ||
| print(f"NEW openldap_description will be {new_description}") | ||
| print("SYNC required to update OpenLDAP description") | ||
|
|
||
| if project.status_id in [PROJECT_STATUS_CHOICE_ARCHIVED]: | ||
| # fetch current description from archive DN | ||
| fetched_description = ldapsearch_get_project_description(archive_dn) | ||
| if new_description == fetched_description: | ||
| print("Description is up-to-date.") | ||
| if new_description != fetched_description: | ||
| if not sync: | ||
| # line up description output | ||
| print(f"OLD openldap_description is {fetched_description}") | ||
| print(f"NEW openldap_description will be {new_description}") | ||
| print("SYNC required to update OpenLDAP description") | ||
| if sync and not write_to_archive: | ||
| print("CANNOT Modify descrption in OpenLDAP for this archived project") | ||
| print("WRITE_TO_ARCHIVE is required to make changes, please supply: -z or --write_archive") | ||
| if sync and write_to_archive: | ||
| update_project_posixgroup_in_openldap(archive_dn, new_description, write=True) | ||
| print(f"{new_description}") |
There was a problem hiding this comment.
Some of the deep nesting here is not necessary, pooling more information onto a single level of conditional can make the code more clear in this instance. Something similar to the following:
| def handle_description_update( | |
| project, | |
| ldapsearch_project_result=False, | |
| ldapsearch_project_result_archive=False, | |
| project_dn="", | |
| archive_dn="", | |
| sync=False, | |
| write_to_archive=False, | |
| ): | |
| new_description = construct_project_posixgroup_description(project) # supply project_obj | |
| if project.status_id in [PROJECT_STATUS_CHOICE_NEW, PROJECT_STATUS_CHOICE_ACTIVE]: | |
| # fetch current description from project_dn | |
| fetched_description = ldapsearch_get_project_description(project_dn) | |
| if new_description == fetched_description: | |
| print("Description is up-to-date.") | |
| if new_description != fetched_description: | |
| if sync: | |
| update_project_posixgroup_in_openldap(project_dn, new_description, write=True) | |
| print(f"{new_description}") | |
| else: | |
| # line up description output | |
| print(f"OLD openldap_description is {fetched_description}") | |
| print(f"NEW openldap_description will be {new_description}") | |
| print("SYNC required to update OpenLDAP description") | |
| if project.status_id in [PROJECT_STATUS_CHOICE_ARCHIVED]: | |
| # fetch current description from archive DN | |
| fetched_description = ldapsearch_get_project_description(archive_dn) | |
| if new_description == fetched_description: | |
| print("Description is up-to-date.") | |
| if new_description != fetched_description: | |
| if not sync: | |
| # line up description output | |
| print(f"OLD openldap_description is {fetched_description}") | |
| print(f"NEW openldap_description will be {new_description}") | |
| print("SYNC required to update OpenLDAP description") | |
| if sync and not write_to_archive: | |
| print("CANNOT Modify descrption in OpenLDAP for this archived project") | |
| print("WRITE_TO_ARCHIVE is required to make changes, please supply: -z or --write_archive") | |
| if sync and write_to_archive: | |
| update_project_posixgroup_in_openldap(archive_dn, new_description, write=True) | |
| print(f"{new_description}") | |
| def handle_description_update( | |
| project, | |
| ldapsearch_project_result=False, | |
| ldapsearch_project_result_archive=False, | |
| project_dn="", | |
| archive_dn="", | |
| sync=False, | |
| write_to_archive=False, | |
| ): | |
| project_status_is_current: bool = project.status_id in [PROJECT_STATUS_CHOICE_NEW,PROJECT_STATUS_CHOICE_ACTIVE] | |
| project_status_is_archived: bool = project.status_id in [PROJECT_STATUS_CHOICE_ARCHIVED] | |
| if not project_status_is_current or project_status_is_archived | |
| return | |
| fetched_description = ldapsearch_get_project_description(project_dn) | |
| new_description = construct_project_posixgroup_description(project) # supply project_obj | |
| should_update: bool = sync and (project_status_is_current or (project_status_is_archived and write_to_archive)) | |
| if new_description == fetched_description: | |
| print("Description is up-to-date.") | |
| elif should_update: | |
| update_project_posixgroup_in_openldap(project_dn, new_description, write=True) | |
| print(f"{new_description}") | |
| elif project_status_is_archived and sync: | |
| print("CANNOT Modify description in OpenLDAP for this archived project") | |
| print("WRITE_TO_ARCHIVE is required to make changes, please supply: -z or --write_archive") | |
| elif project_status_is_current or project_status_is_archived: | |
| print(f"OLD openldap_description is {fetched_description}") | |
| print(f"NEW openldap_description will be {new_description}") | |
| print("SYNC required to update OpenLDAP description") |
there might be more that can be done with this function still but I think I got the idea across.
If you have never come across the concept look up 'never nesting'. This will help to make the code more understandable so that whoever is updating it in the future (probably you in two months) has an easier time. This also makes it easier for maintainers to review as it makes the control flow and the intentions of the code clearer.
There are other functions in this file that this applies to as well.
| def local_get_openldap_members(dn): | ||
| entries = ldapsearch_get_project_memberuids(dn) | ||
| members = [] | ||
| for entry in entries: | ||
| members.extend(entry.memberUid.values) | ||
| return tuple(members) |
There was a problem hiding this comment.
This loop could run into a NoneType exception because of ldapsearch_get_project_memberuids. Either add a guard clause if you get None returned to you or modify ldapsearch_get_project_memberuids to give [] when it reaches an exception.
There was a problem hiding this comment.
will action good spot
| sys.exit(1) | ||
|
|
There was a problem hiding this comment.
use django's command error unless you have a good reason to not have django handle exiting this command.
| sys.exit(1) | ||
| # 2) --- END --- |
There was a problem hiding this comment.
use django's command error unless you have a good reason to not have django handle exiting this command.
ds-04
force-pushed
the
project_openldap
branch
2 times, most recently
from
f30dc27 to
3c07f9c
Compare
|
Two force pushes, first changes ... then again i got caught by ruff versioning in my local environment... The changes address the vast majority of the comments and improvements flagged These issues were intentionally NOT addressed at this point:
They are all valid and good points ... I'm just a bit concerned about trying to cram it all in right now. @Eric-Butcher thanks for the expert review, it is appreciated @Eric-Butcher please can you cast you eye over the sync management command in particular, post these changes 26.06.25 thanks |
| self.stdout.write("ERROR: Unrecognized project status - HALTING") | ||
| raise CommandError |
There was a problem hiding this comment.
You can put the error message right in the CommandError and have Django take care of it for you.
| self.stdout.write("ERROR: Unrecognized project status - HALTING") | |
| raise CommandError | |
| raise CommandError("Unrecognized project status - HALTING") |
| if not ldapsearch_project_result and not ldapsearch_project_result_archive: | ||
| self.stdout.write( | ||
| "WARNING: sync_members (method) ldapsearch are both False for project OU and archive OU for project: {project.project_code}" | ||
| ) | ||
| raise CommandError |
There was a problem hiding this comment.
I think I missed this before. Is this supposed to be an f-string? You can also put the error message right inside the CommandError.
| if not ldapsearch_project_result and not ldapsearch_project_result_archive: | |
| self.stdout.write( | |
| "WARNING: sync_members (method) ldapsearch are both False for project OU and archive OU for project: {project.project_code}" | |
| ) | |
| raise CommandError | |
| if not ldapsearch_project_result and not ldapsearch_project_result_archive: | |
| raise CommandError(f"sync_members (method) ldapsearch are both False for project OU and archive OU for project: {project.project_code}") |
|
I just checked the sync command file again and except for a couple things, I do not have anything new to mention. Everything looks a lot nicer! |
Signed-off-by: David Simpson <>
|
thanks .. rectified, please double check files changed listing before merge |
This PR adds the
project_openldapColdfront plugin.A comprehensive
README.mdis provided both in the main plugin directory and the management command directory.Also included are mermaid diagrams (inside main plugin
README.mdand separate file in the management command directory). There are two diagrams one showing OpenLDAP DIT and the other showing the main syncer function (management command related).Management commands: