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

AUTH_LDAP_MIRROR_GROUPS ldap_config tweak #448

Merged
merged 7 commits into from
Apr 20, 2021
Merged

AUTH_LDAP_MIRROR_GROUPS ldap_config tweak #448

merged 7 commits into from
Apr 20, 2021

Conversation

ryanmerolle
Copy link
Contributor

@ryanmerolle ryanmerolle commented Feb 22, 2021

Related Issue: Fixes #445

New Behavior

If AUTH_LDAP_MIRROR_GROUPS not set in environment variables, it will not be set in ldap_config.py, allowing for ldap config shown in the new example configuration\ldap\extra.py

Contrast to Current Behavior

If AUTH_LDAP_MIRROR_GROUPS not set in environment variables, it is set to empty in ldap_config.py

Discussion: Benefits and Drawbacks

See #445

Changes to the Wiki

TBD, likely will suggest something with regards to language around the #343 function and the example configuration\ldap\extra.py file included.

Proposed Release Note Entry

  • Tweaked ldap_config.py to allow for a AUTH_LDAP_MIRROR_GROUPS list of groups to be specified.

Double Check

  • I have read the comments and followed the PR template.
  • I have explained my PR according to the information in the comments.
  • My PR targets the develop branch.

cimnine
cimnine previously approved these changes Feb 23, 2021
Copy link
Collaborator

@cimnine cimnine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the example file!

configuration/ldap/ldap_config.py Outdated Show resolved Hide resolved
configuration/ldap/extra.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@cimnine cimnine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant to click Comment, not Approve.

@cimnine cimnine dismissed their stale review February 23, 2021 17:35

Clicked wrong button.

@cimnine cimnine added the enhancement The issue describes an enhancement that we would like to implement in the future. label Feb 23, 2021
@cimnine cimnine added this to the 1.1.0 milestone Feb 23, 2021
@cimnine cimnine linked an issue Feb 23, 2021 that may be closed by this pull request
@ryanmerolle
Copy link
Contributor Author

I'll build from develop locally and test when I get a chance since #450 is only in develop at this point. Give me a day or two.

@cimnine
Copy link
Collaborator

cimnine commented Mar 4, 2021

Do you think you have a chance to look into this anytime soon*ish? No hurry, I just want to know whether I should wait for it or otherwise postpone this change to the next release.

@ryanmerolle
Copy link
Contributor Author

I will have this tested today or tomorrow since this directly is tied to a new deployment I have to have in place by tomorrow evening.

@ryanmerolle
Copy link
Contributor Author

I tested as you suggested by mounting the volume to pull in the develop version of docker/configuration.docker.py from #450

The extra.py still works as it did before. IE if I do not comment out the variables set in ldap.py that overlap with what I declare in extra.py, the settings declared in extra.py that overlap do not take. All the other non-overlapping variables take no matter which file they are declared in.

@ryanmerolle
Copy link
Contributor Author

I say bump my PRs to the next release. No need to further hold up here.

@cimnine cimnine modified the milestones: 1.1.0, next Mar 5, 2021
@tobiasge
Copy link
Member

tobiasge commented Mar 5, 2021

I tested as you suggested by mounting the volume to pull in the develop version of docker/configuration.docker.py from #450

The extra.py still works as it did before. IE if I do not comment out the variables set in ldap.py that overlap with what I declare in extra.py, the settings declared in extra.py that overlap do not take. All the other non-overlapping variables take no matter which file they are declared in.

If you still have this problem you can do some more tests like this:
Mount modified versions of the configurations files into the container (with docker-compose.override.yml):

version: '3.4'
services:
  netbox:
    ports:
      - "8000:8080"
    volumes:
      - ./docker/configuration.docker.py:/opt/netbox/netbox/netbox/configuration.py:z,ro
      - ./docker/ldap_config.docker.py:/opt/netbox/netbox/netbox/ldap_config.py:z,ro

Modify the method __getattr__(name) in docker/configuration.docker.py and docker/ldap_config.docker.py to this:

def __getattr__(name):
    for config in _loaded_configurations:
        try:
            attr = getattr(config, name)
            print(f"Returning '{name}' from '{config}'")
            return attr
        except:
            print(f"'{name}' not found in '{config}'")
            pass
    raise AttributeError

This will produce lines (a lot, basically for every settings access) like these:

netbox_1         | 'MAINTENANCE_MODE' not found in '<module '' from '/etc/netbox/config/extra.py'>'
netbox_1         | Returning 'MAINTENANCE_MODE' from '<module '' from '/etc/netbox/config/configuration.py'>'

Then you can see from where the values are loaded.

@cimnine
Copy link
Collaborator

cimnine commented Apr 6, 2021

@ryanmerolle , just checking what's the status here, as I lost track 🙈

@cimnine cimnine added the awaiting answer There is still some open discussion. label Apr 6, 2021
@ryanmerolle
Copy link
Contributor Author

I was quite distracted the past month with the birth of my daughter. I plan to dig this back up along with some of my other netbox-docker items in the next week.

@cimnine
Copy link
Collaborator

cimnine commented Apr 6, 2021

Congratulations, I hope she and your family are all well! There's no rush, just good to know that you're still on it.

@cimnine cimnine modified the milestones: Version 1.2.0, next Apr 19, 2021
@ryanmerolle
Copy link
Contributor Author

instead of making a feature branch in my fork I developed in the develop branch. Once this PR gets merged or rejected I will tackle #461

configuration/ldap/extra.py Outdated Show resolved Hide resolved
configuration/ldap/ldap_config.py Outdated Show resolved Hide resolved
@ryanmerolle
Copy link
Contributor Author

Apologies. I forgot to commit my changes. This should be fixed now @cimnine

@cimnine cimnine modified the milestones: next, 1.2.0 Apr 20, 2021
@cimnine
Copy link
Collaborator

cimnine commented Apr 20, 2021

Apologies. I forgot to commit my changes. This should be fixed now @cimnine

Happens to all of us 😅

@cimnine cimnine merged commit f2731d3 into netbox-community:develop Apr 20, 2021
@cimnine cimnine mentioned this pull request Apr 23, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting answer There is still some open discussion. enhancement The issue describes an enhancement that we would like to implement in the future.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update AUTH_LDAP_MIRROR_GROUPS handling
3 participants