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

The Debug Toolbar has stopped showing under Docker #1854

Closed
epicserve opened this issue Nov 1, 2023 · 17 comments · Fixed by #1887
Closed

The Debug Toolbar has stopped showing under Docker #1854

epicserve opened this issue Nov 1, 2023 · 17 comments · Fixed by #1887

Comments

@epicserve
Copy link

It seems like the snippet of code in the documentation for using Docker has stopped working.

image

That snippet of code will set INTERNAL_IPS to ['172.27.0.1', '127.0.0.1', '10.0.2.2'], however, inside debug_toolbar.middleware.show_toolbar(), request.META.get("REMOTE_ADDR") returns '192.168.65.1', which is why it ends up not showing the Debug Toolbar.

This snippet of code used to work. I'm guessing something must of changed in a recent version of Docker to cause it to not get the correct IP address. I'm not sure at this point how to get the IP dynamically because even ifconfig shows the 172.27.01 IP address instead of the 192.168.65.1 IP address.

Versions:

  • Docker version 24.0.6
  • Docker Compose version v2.22.0-desktop.2
  • macOS Sonoma version 14.0
  • django-debug-toolba v3.8.1
  • django v4.2.5
@matthiask
Copy link
Member

If you're absolutely sure that nobody else will access your development server I still prefer the following snippet:

if DEBUG:
    # `debug` is only True in templates if the vistor IP is in INTERNAL_IPS.
    INTERNAL_IPS = type("c", (), {"__contains__": lambda *a: True})()

There's no need to determine the IP address, all you have to do is to make INTERNAL_IPS contain everything by defining a __contains__ method which always returns true.

That being said, it would be nice if there was a copy-pasteable and somewhat safer fix for this.

@blopker
Copy link
Contributor

blopker commented Nov 8, 2023

Can confirm the original code for finding the internal IP has stopped working. I've also just allowed all IPs when debug is on. From what I can tell, something like socket.gethostbyname('host.docker.internal') is supposed to return the right IP, but doesn't. In my case, the right IP is 192.168.65.1, but host.docker.internal returns 192.168.65.254. Maybe allowing just that 192.168.65.* prefix would be more secure?

@epicserve
Copy link
Author

@blopker, great find! I'm guessing that the last number will always be 1. Maybe something the following would always work!

".".join(socket.gethostbyname('host.docker.internal').rsplit('.')[:-1]) + ".1"

@matthiask
Copy link
Member

I doubt there's anything which is always going to work, whatever docker/podman/etc. does, except for my INTERNAL_IPS hack.

I'm slowly leaning towards documenting the hack with a big big caveat. Opinions?

@blopker
Copy link
Contributor

blopker commented Dec 18, 2023

As a user, this has tripped me up every single time I start a new project. I get the need for making sure the toolbar isn't shown in production, but since Docker is such a common development environment, I can't help but think there's a better way to accomplish this?

Maybe this snippet can be included in this library somehow? Then if users are on Docker they can just import a single function and add it to INTERNAL_IPS? The snippet could be more robust then, like checking for DEBUG is True.

But yeah, in lieu of that, documentation is always welcome. A bit more explanation about why the snippet is needed, plus what it's actually doing would be great. Maybe a date/Docker version on when it was last known to work as well?

@tim-schilling
Copy link
Contributor

@matthiask I'm less sure we should even be supporting an INTERNAL_IPS out of the box. That feels like an invitation to utilize the toolbar in a remote environment. Perhaps instead we should default to checking that the user is a super user instead?

@epicserve
Copy link
Author

I don't even have DjDT installed in production, but I know not everyone does it that way.

Checking if someone has superuser permissions wouldn't work for us because we sometimes impersonate regular users for debugging and still need DjDT.

I don't know the history ... why isn't going off what DEBUG is set to enough? Isn't it recommended that you don't run a Django project with DEBUG set to True in production?

The simplest thing to do would be just going off of DEBUG and calling it a day. 🙂

@blopker
Copy link
Contributor

blopker commented Dec 19, 2023

One issue is that INTERNAL_IPS (https://docs.djangoproject.com/en/5.0/ref/settings/#internal-ips) does other things besides enable the toolbar.

Specifically: "Allow the debug() context processor to add some variables to the template context.", I suspect (but haven't looked) that the toolbar uses that information internally. However, if the toolbar duplicated this context processor, which itself is pretty small, it could get around that. See https://github.com/django/django/blob/08306bad57761b5eb176894649ac7d4e735c52dd/django/template/context_processors.py#L41

@matthiask
Copy link
Member

@matthiask I'm less sure we should even be supporting an INTERNAL_IPS out of the box. That feels like an invitation to utilize the toolbar in a remote environment. Perhaps instead we should default to checking that the user is a super user instead?

I'm not sure. I do not have to do anything right now to use the toolbar, and this change would definitely mean I'd have to customize the callback locally, since I use the toolbar also when I'm not authenticated (locally) to quickly see if a particular view requires too many SQL queries for example.

@tim-schilling
Copy link
Contributor

I'm brainstorming now. We have some extra development bandwidth. Maybe we provide various out of the box solutions for the person to configure.

DEBUG_TOOLBAR_CONFIG = {
    "SHOW_TOOLBAR_CALLBACK": "debug_toolbar.show_toolbar.allow_super_users"
}

Then maybe we have:

  • debug_toolbar.show_toolbar.restrict_to_internal_ips
  • debug_toolbar.show_toolbar.allow_all
  • debug_toolbar.show_toolbar.magic_local_docker_fix

You'll still need to change something regardless of what we do. And from what this issue sounds like, the docker integration today is already broken.

@matthiask
Copy link
Member

matthiask commented Feb 15, 2024

The thing which is broken right now is the documentation I think? I can see the usefulness of adding a few additional callbacks of course, but when we only evaluate DEBUG and INTERNAL_IPS we're following Django's lead and I still don't think it's a bad default. People using Docker are probably a bit more advanced already and can for better or worse be expected to add a line or three to their projects, or not?

@epicserve
Copy link
Author

To me, it's simple. Update the documentation and close this issue. Then, it might be good to start planning a more permanent solution and better DX. With Docker/Docker Compose becoming more the preferred development environment, it would be nice if DjDT worked out of the box by just adding it to INSTALLED_APPS. To me, that's the ideal DX to shoot for.

@tim-schilling
Copy link
Contributor

People using Docker are probably a bit more advanced already and can for better or worse be expected to add a line or three to their projects, or not?

I disagree that people using docker are typically more advanced than those that aren't. Docker is one of those technologies that some folks think everyone should know. That leads to at least some folks having it thrust on them when they arguably should be focused on learning the basics of the language and framework they're using.

I know enough of docker to get a service up and running locally for development. I'd be a bit lost trying to solve this particular problem though.

@epicserve
Copy link
Author

I agree with @tim-schilling. I manage a team of engineers; some of the people on the team are advanced, and some are not because that isn't their focus. Again, if we're thinking about the ideal DX, it would be great if the only requirement was adding DjDT to INSTALLED_APPS.

tim-schilling added a commit to tim-schilling/django-debug-toolbar that referenced this issue Feb 17, 2024
@blopker
Copy link
Contributor

blopker commented Feb 17, 2024

Great discussion here. Thought I would add from my own experience.

First, I do think the docs should be updated with the @epicserve change. Happy to leave it at that and move on, until at least Docker (or some other new development thing) breaks the solution again.

However, it does seem like it will break again, and I think it's an inherent problem with INTERNAL_IPS. This is because the increasing complexity of our development environments has broken assumptions around what an 'internal ip' means. Django itself only really uses it to enable the debug context processor. While I do see that usage as an invitation to use it for other debug solutions as well, I don't think IPs should be used at all.

As a developer, I'd prefer to have a simple toggle that I control, so I can make decisions on when features of my apps are enabled, or not. I don't want to have to dig into why something broke (or worse, got enabled) when my IP routing changed. I would much prefer to just have a boolean config for the toolbar that just does what I expect. If I want the toolbar to run on a server, great, that should be OK.

My preferred solution:

Ideally, I'd want the toolbar to be enabled after 1) adding it to INSTALLED_APPS and 2) DEBUG is True. I think only requiring the toolbar to be in INSTALLED_APPS is too dangerous and adds to the config complexity of small apps. Since Django's debug mode already can lead to major security issues, gating the toolbar on DEBUG only adds a bit of convince to a potential attack. Also, it's what I would expect as a long time Django user.

However, this is a breaking change. Since INTERNAL_IPS is no longer respected the toolbar may show up when it didn't before. That's bad. If we aren't looking to take on a major version bump, I'd propose adding something like a DEBUG_TOOLBAR boolean config. This would override/ignore the INTERNAL_IPS check. Then to update the docs to make the new config the preferred way, possibly suggesting that users set DEBUG_TOOLBAR = DEBUG. If the config isn't present, then fallback to INTERNAL_IPS.

Hope this makes sense!

tim-schilling added a commit to tim-schilling/django-debug-toolbar that referenced this issue Feb 18, 2024
@epicserve
Copy link
Author

@blopker, sorry I wasn't clear. I was assuming DEBUG set True as a given for DjDT to be enabled.

Why couldn't you just go off of INTERNAL_IPS, if that value is set, otherwise use the DEBUG_TOOLBAR value for checking if it should be enabled. I don't see that as being a breaking change.

@blopker
Copy link
Contributor

blopker commented Feb 19, 2024

Great, thanks for clarifying. I guess it's a question around order of precedence for the settings. I'll go through the 3 types of users I can think of:

  1. New users to the library
  2. Old users to the library that read changelogs and want to move off the Docker hacks
  3. Old users that just want to update and not change anything

I've created a toy test case with these scenarios. First, here's the should_enable function:

def should_enable(settings, request):
    if "debug_toolbar" not in settings.INSTALLED_APPS:
        return False

    if settings.DEBUG is not True:
        return False

    if settings.DEBUG_TOOLBAR is True:
        return True

    if settings.INTERNAL_IPS:
        return request.META["REMOTE_ADDR"] in settings.INTERNAL_IPS

    return True

It prioritizes DEBUG_TOOLBAR over INTERNAL_IPS, which I think is the area of discussion. This lets the dev chose to enable the toolbar without INTERNAL_IPS being correct. I like this because INTERNAL_IPS toggles other functionality in Django that devs may want, but with the option to toggle the toolbar independently.

The full toy script with tests is here: https://gist.github.com/blopker/8186a79792d39022e33eccb094c433ea

Does that make sense? Are important test cases missing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants