starred/netbox
1
0
Fork 0
mirror of https://github.com/netbox-community/netbox.git synced 2026-01-11 21:10:29 +01:00
Code Issues 273 Packages Projects Releases 10 Wiki Activity

Use redis as the session backend #3408

New Issue
Closed
opened 2025-12-29 18:28:48 +01:00 by adam · 5 comments
adam commented 2025-12-29 18:28:48 +01:00
Owner
Copy Link

Originally created by @tyler-8 on GitHub (Feb 25, 2020).

Environment

  • Python version: 3.6.8
  • NetBox version: 2.7.7

Proposed Functionality

In issue https://github.com/netbox-community/netbox/issues/4119 there's discussion around adding ways to routinely perform cleanup on the session table in the database. However there's a better way IMO that doesn't involve having to keep up with or run cleanup commands. Instead NetBox could use the already-mandatory redis caching to maintain the session information. It's already self-invalidating and shouldn't require any manual intervention to cleanup old data. The Django docs recommend a session cache for optimal performance as well.

Use Case

A user simply adds new configuration parameters to the configuration.py, for example:

REDIS = {
    'webhooks': {
        'HOST': 'redis.example.com',
        'PORT': 1234,
        'PASSWORD': 'foobar',
        'DATABASE': 0,
        'DEFAULT_TIMEOUT': 300,
        'SSL': False,
    },
    'caching': {
        'HOST': 'localhost',
        'PORT': 6379,
        'PASSWORD': '',
        'DATABASE': 1,
        'DEFAULT_TIMEOUT': 300,
        'SSL': False,
    },
    'sessions': {
        'HOST': 'localhost',
        'PORT': 6379,
        'PASSWORD': '',
        'DATABASE': 2,
        'DEFAULT_TIMEOUT': 300,
        'SSL': False,
    }
}

Database Changes

The django_sessions table would no longer be required

External Dependencies

If a custom implementation (one example) isn't desired, there is a plugin available: https://github.com/martinrusev/django-redis-sessions - but I suspect the example provided is more than enough without requiring a plugin.

Originally created by @tyler-8 on GitHub (Feb 25, 2020). <!-- NOTE: IF YOUR ISSUE DOES NOT FOLLOW THIS TEMPLATE, IT WILL BE CLOSED. This form is only for proposing specific new features or enhancements. If you have a general idea or question, please post to our mailing list instead of opening an issue: https://groups.google.com/forum/#!forum/netbox-discuss NOTE: Due to an excessive backlog of feature requests, we are not currently accepting any proposals which significantly extend NetBox's feature scope. Please describe the environment in which you are running NetBox. Be sure that you are running an unmodified instance of the latest stable release before submitting a bug report. --> ### Environment * Python version: 3.6.8<!-- Example: 3.6.9 --> * NetBox version: 2.7.7<!-- Example: 2.7.3 --> <!-- Describe in detail the new functionality you are proposing. Include any specific changes to work flows, data models, or the user interface. --> ### Proposed Functionality In issue https://github.com/netbox-community/netbox/issues/4119 there's discussion around adding ways to routinely perform cleanup on the session table in the database. However there's a better way IMO that doesn't involve having to keep up with or run cleanup commands. Instead NetBox could use the already-mandatory redis caching to maintain the session information. It's already self-invalidating and shouldn't require any manual intervention to cleanup old data. The [Django docs recommend](https://docs.djangoproject.com/en/2.2/topics/http/sessions/#using-cached-sessions) a session cache for optimal performance as well. <!-- Convey an example use case for your proposed feature. Write from the perspective of a NetBox user who would benefit from the proposed functionality and describe how. ---> ### Use Case A user simply adds new configuration parameters to the `configuration.py`, for example: ```python REDIS = { 'webhooks': { 'HOST': 'redis.example.com', 'PORT': 1234, 'PASSWORD': 'foobar', 'DATABASE': 0, 'DEFAULT_TIMEOUT': 300, 'SSL': False, }, 'caching': { 'HOST': 'localhost', 'PORT': 6379, 'PASSWORD': '', 'DATABASE': 1, 'DEFAULT_TIMEOUT': 300, 'SSL': False, }, 'sessions': { 'HOST': 'localhost', 'PORT': 6379, 'PASSWORD': '', 'DATABASE': 2, 'DEFAULT_TIMEOUT': 300, 'SSL': False, } } ``` <!-- Note any changes to the database schema necessary to support the new feature. For example, does the proposal require adding a new model or field? (Not all new features require database changes.) ---> ### Database Changes The `django_sessions` table would no longer be required <!-- List any new dependencies on external libraries or services that this new feature would introduce. For example, does the proposal require the installation of a new Python package? (Not all new features introduce new dependencies.) --> ### External Dependencies If a custom implementation ([one example](https://apirobot.me/posts/tale-about-redis-and-sessions-in-django)) isn't desired, there is a plugin available: https://github.com/martinrusev/django-redis-sessions - but I suspect the example provided is more than enough without requiring a plugin.
adam closed this issue 2025-12-29 18:28:48 +01:00
adam commented 2025-12-29 18:28:49 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Feb 26, 2020):

I'm not sold on the value of this approach given the drawbacks:

  1. It requires yet another configuration change. (Seems like we had a difficult enough time with the caching change under v2.7.)
  2. It requires introducing (or replicating) a new dependency.
  3. It removes the ability to capture session data in a database snapshot. (Probably not a big deal but I'm sure someone will complain.)
  4. This brings Redis front-and-center in any troubleshooting process. Currently, a Redis failure would affect only webhooks and/or caching, but if we rely on it for session storage a user won't even be able to authenticate if Redis is unavailable. This is likely to frustrate troubleshooting efforts in the event of an outage.

It just doesn't seem like a worthwhile investment of effort if the primary issue (#4119) can be resolved through less disruptive means.

@jeremystretch commented on GitHub (Feb 26, 2020): I'm not sold on the value of this approach given the drawbacks: 1. It requires yet another configuration change. (Seems like we had a difficult enough time with the caching change under v2.7.) 2. It requires introducing (or replicating) a new dependency. 3. It removes the ability to capture session data in a database snapshot. (_Probably_ not a big deal but I'm sure someone will complain.) 4. This brings Redis front-and-center in any troubleshooting process. Currently, a Redis failure would affect only webhooks and/or caching, but if we rely on it for session storage a user won't even be able to authenticate if Redis is unavailable. This is likely to frustrate troubleshooting efforts in the event of an outage. It just doesn't seem like a worthwhile investment of effort if the primary issue (#4119) can be resolved through less disruptive means.
adam commented 2025-12-29 18:28:49 +01:00
Author
Owner
Copy Link

@tyler-8 commented on GitHub (Feb 26, 2020):

  1. Most of the issues I saw were people blindly copy/pasting the example config using port 1234 for the webhooks config rather than using the real port their server was on - though I'm sure there were other problems people encountered
  2. The code for the new session handling is basically just modifying SessionBase from Django - it's not particularly complicated as the redis package handles the connection bits already.
from django.contrib.sessions.backends.base import SessionBase
from django.utils.functional import cached_property
from redis import Redis
class SessionStore(SessionBase):
    @cached_property
    def _connection(self):
        return Redis(
            host='127.0.0.1',
            port='6379',
            db=0,
            decode_responses=True
        )
    def load(self):
        # Loads the session data by the session key.
        # Returns dictionary.
        return self._connection.hgetall(self.session_key)
    def exists(self, session_key):
        # Checks whether the session key already exists
        # in the database or not.
        return self._connection.exists(session_key)
    def create(self):
        # Creates a new session in the database.
        self._session_key = self._get_new_session_key()
        self.save(must_create=True)
        self.modified = True
    def save(self, must_create=False):
        # Saves the session data. If `must_create` is True,
        # creates a new session object. Otherwise, only updates
        # an existing object and doesn't create one.
        if self.session_key is None:
            return self.create()
        data = self._get_session(no_load=must_create)
        session_key = self._get_or_create_session_key()
        self._connection.hmset(session_key, data)
        self._connection.expire(session_key, self.get_expiry_age())
    def delete(self, session_key=None):
        # Deletes the session data under the session key.
        if session_key is None:
            if self.session_key is None:
                return
            session_key = self.session_key
        self._connection.delete(session_key)
    @classmethod
    def clear_expired(cls):
        # There is no need to remove expired sessions by hand
        # because Redis can do it automatically when
        # the session has expired.
        # We set expiration time in `save` method.
  1. I can't imagine that would be missed by anyone as that data is ephemeral and having snapshots of it doesn't particularly help - users just have to log in again.
  2. Correct me if I'm wrong but I think the entire app already fails to launch if redis can't be connected to (as witnessed by people using the wrong port mentioned in item 1). I think that ship has sailed, to be frank.
  3. A wise man once said it was worth exploring. ;)
@tyler-8 commented on GitHub (Feb 26, 2020): 1. Most of the issues I saw were people blindly copy/pasting the example config using port `1234` for the `webhooks` config rather than using the real port their server was on - though I'm sure there were other problems people encountered 2. The code for the new session handling is basically just modifying `SessionBase` from Django - it's not particularly complicated as the `redis` package handles the connection bits already. ```python from django.contrib.sessions.backends.base import SessionBase from django.utils.functional import cached_property from redis import Redis class SessionStore(SessionBase): @cached_property def _connection(self): return Redis( host='127.0.0.1', port='6379', db=0, decode_responses=True ) def load(self): # Loads the session data by the session key. # Returns dictionary. return self._connection.hgetall(self.session_key) def exists(self, session_key): # Checks whether the session key already exists # in the database or not. return self._connection.exists(session_key) def create(self): # Creates a new session in the database. self._session_key = self._get_new_session_key() self.save(must_create=True) self.modified = True def save(self, must_create=False): # Saves the session data. If `must_create` is True, # creates a new session object. Otherwise, only updates # an existing object and doesn't create one. if self.session_key is None: return self.create() data = self._get_session(no_load=must_create) session_key = self._get_or_create_session_key() self._connection.hmset(session_key, data) self._connection.expire(session_key, self.get_expiry_age()) def delete(self, session_key=None): # Deletes the session data under the session key. if session_key is None: if self.session_key is None: return session_key = self.session_key self._connection.delete(session_key) @classmethod def clear_expired(cls): # There is no need to remove expired sessions by hand # because Redis can do it automatically when # the session has expired. # We set expiration time in `save` method. ``` 3. I can't imagine that would be missed by anyone as that data is ephemeral and having snapshots of it doesn't particularly help - users just have to log in again. 4. Correct me if I'm wrong but I think the entire app already fails to launch if redis can't be connected to (as witnessed by people using the wrong port mentioned in item 1). I think that ship has sailed, to be frank. 5. A wise man [once said](https://github.com/netbox-community/netbox/issues/3050#issuecomment-487207603) it was worth exploring. ;)
adam commented 2025-12-29 18:28:49 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Feb 26, 2020):

  1. I can't imagine that would be missed by anyone as that data is ephemeral and having snapshots of it doesn't particularly help - users just have to log in again.

In it's current form, yes. But in the future we may rely on session data for more than just authentication. (#3294 may or may not be relevant here.)

  1. Correct me if I'm wrong but I think the entire app already fails to launch if redis can't be connected to (as witnessed by people using the wrong port mentioned in item 1). I think that ship has sailed, to be frank.

This was changed just recently, particularly because it was a huge impediment to troubleshooting. I don't know the change itself was captured in an issue, but #4173 was implemented to provide a "softer" error message when Redis is not reachable. (Edit: Here's the change.)

  1. A wise man once said it was worth exploring. ;)

Which is what we're doing now. :)

@jeremystretch commented on GitHub (Feb 26, 2020): > 3. I can't imagine that would be missed by anyone as that data is ephemeral and having snapshots of it doesn't particularly help - users just have to log in again. In it's _current_ form, yes. But in the future we may rely on session data for more than just authentication. (#3294 may or may not be relevant here.) > 4. Correct me if I'm wrong but I think the entire app already fails to launch if redis can't be connected to (as witnessed by people using the wrong port mentioned in item 1). I think that ship has sailed, to be frank. This was changed just recently, particularly because it was a huge impediment to troubleshooting. I don't know the change itself was captured in an issue, but #4173 was implemented to provide a "softer" error message when Redis is not reachable. (Edit: Here's [the change](https://github.com/netbox-community/netbox/commit/ec0f45e20dffde593f157b789a922aca1c9fe64a#diff-584c6a52264f324179c1b809bb3c632d).) > 5. A wise man once said it was worth exploring. ;) Which is what we're doing now. :)
adam commented 2025-12-29 18:28:49 +01:00
Author
Owner
Copy Link

@tyler-8 commented on GitHub (Feb 26, 2020):

In it's current form, yes. But in the future we may rely on session data for more than just authentication. (#3294 may or may not be relevant here.)

In regards to #3294, IMO - user preferences (beyond 'remember me', or settings that are truly relevant to only the single session) should really get their own session-independent storage mechanism (like a database table). Even with database sessions, they are still set to expire at some point (though the row may exist, it is no longer valid and a new one is created in its place).

Using the cache for the session will still follow the configured expiration timers, still allowing for storage of extra data in the session, while removing the need to routinely cleanup expired sessions as this will happen automatically without NetBox intervention. Redis does have some semblance of persistence (I believe out of the box) so even in the event of a cache-server reboot, it's not necessarily going to wipe all session data - which again, the worst thing to happen there is the front-end user logs in again and possibly has to tick a box or two to re-establish a setting - API users wouldn't be affected of course.

This was changed just recently...

Good to hear - so it's a fair concern, just not sure how big of a concern it should ultimately be. If most of the problems were related to incorrect configuration and the previous code that caused the app to bomb when not connecting then we're really no different than relying on an external database vs an external cache server. Reading the Django docs I get the sense that their hierarchy of session management (at least in terms of performance & flexibility) is something like file < memory < database < cache. The docs do talk about a way to do a cache + db for extra persistence but that path would likely complicate the redis cache implementation (since the built-in way involves memcached).

Which is what we're doing now. :)

Why can't you just blindly accept my FR?!11 /s

In any event - I think this FR could close the loop on #4119 while also providing a small performance bump but definitely understand if the appetite isn't there quite yet to go down this route.

@tyler-8 commented on GitHub (Feb 26, 2020): > In it's current form, yes. But in the future we may rely on session data for more than just authentication. (#3294 may or may not be relevant here.) In regards to #3294, IMO - user preferences (beyond 'remember me', or settings that are truly relevant to only the single session) should really get their own session-independent storage mechanism (like a database table). Even with database sessions, they are still set to expire at some point (though the row may exist, it is no longer valid and a new one is created in its place). Using the cache for the session will still follow the configured expiration timers, still allowing for storage of extra data in the session, while removing the need to routinely cleanup expired sessions as this will happen automatically without NetBox intervention. Redis does have some semblance of [persistence](https://redis.io/topics/persistence) (I believe out of the box) so even in the event of a cache-server reboot, it's not necessarily going to wipe all session data - which again, the worst thing to happen there is the front-end user logs in again and possibly has to tick a box or two to re-establish a setting - API users wouldn't be affected of course. > This was changed just recently... Good to hear - so it's a fair concern, just not sure how big of a concern it should ultimately be. If most of the problems were related to incorrect configuration and the previous code that caused the app to bomb when not connecting then we're really no different than relying on an external database vs an external cache server. Reading the Django docs I get the sense that their hierarchy of session management (at least in terms of performance & flexibility) is something like `file < memory < database < cache`. The docs do talk about a way to do a `cache + db` for extra persistence but that path would likely complicate the redis cache implementation (since the built-in way involves memcached). > Which is what we're doing now. :) Why can't you just blindly accept my FR?!11 /s In any event - I think this FR could close the loop on #4119 while also providing a small performance bump but definitely understand if the appetite isn't there quite yet to go down this route.
adam commented 2025-12-29 18:28:50 +01:00
Author
Owner
Copy Link

@jeremystretch commented on GitHub (Mar 3, 2020):

After some discussion with the other maintainers, the consensus is to keep the existing session store as it is, but open its configuration under the upcoming plugins framework (see #3351). This ensures that we are not on the hook to maintain or force the installation of a separate session backend.

@jeremystretch commented on GitHub (Mar 3, 2020): After some discussion with the other maintainers, the consensus is to keep the existing session store as it is, but open its configuration under the upcoming plugins framework (see #3351). This ensures that we are not on the hook to maintain or force the installation of a separate session backend.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
beta
breaking change
complexity: high
complexity: low
complexity: medium
needs milestone
netbox
pending closure
plugin candidate
pull-request
Mirrored from GitHub Pull Request
 severity: high
severity: low
severity: medium
status: accepted
status: backlog
status: blocked
status: duplicate
status: needs owner
status: needs triage
status: revisions needed
status: under review
topic: GraphQL
topic: Internationalization
topic: OpenAPI
topic: UI/UX
topic: cabling
topic: event rules
topic: htmx navigation
topic: industrialization
topic: migrations
topic: plugins
topic: scripts
topic: templating
topic: testing
type: bug
type: deprecation
type: documentation
type: feature
type: housekeeping
type: translation
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adam
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/netbox#3408
Delete Branch "%!s()"

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?