Avoid Django's ALLOWED_HOSTS check
#3651
Conversation
jourdanrodrigues
changed the title
ALLOWED_HOSTS check
instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py
Outdated
Show resolved
Hide resolved
...nstrumentation-django/src/opentelemetry/instrumentation/django/middleware/otel_middleware.py
Outdated
Show resolved
Hide resolved
| url = "{}://{}{}".format( | ||
| request.scheme, request._get_raw_host(), request.path | ||
| ) |
There was a problem hiding this comment.
Will this break anyone who was relying on the previous behavior of request.build_absolute_uri("?")
There was a problem hiding this comment.
In my experience (not proof of anything, more as an example), host checks are intentional, not an accidental feature of any particular lib (reason why I'm proposing this).
The consequence of these changes is that the host won't be checked anymore, everything else works normally.
I can put these behind an environment variable, Django settings or just the current code under that method abstraction so that it can more easily be overridden, without having to copy over the whole code.
Let me know what you think!
There was a problem hiding this comment.
I defer to the python maintainers on how they would want this configurable, if they do. Thanks for the explanation!
There was a problem hiding this comment.
It's not clear to me why the previous implementation appended a ? to the URL being checked, but I'm wondering if we should keep it to minimize changes in behavior. What do you think @jourdanrodrigues?
Description
Django (eventually) calls
HttpRequest.get_hostwhen callingHttpRequest.build_absolute_uri, which we use in the middleware.In the context I'm at, we have a case for setting up a health check as early as possible in a middleware that needs to bypass that check since Kubernetes hits the containers directly via IP.
I can put the middleware after the health check one but would be good to also instrument it to collect traces if that ever fails.
These changes will bring up the implementation of
build_absolute_uriwithout any of the checks.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py, all tests underTestMiddleware.Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.