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

Add ability to have proxy rewrite API_ROOT w/ header #5013

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/scripts/install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ if [ "$TEST" = "azure" ]; then
- ./azurite:/etc/pulp\
command: "azurite-blob --blobHost 0.0.0.0 --cert /etc/pulp/azcert.pem --key /etc/pulp/azkey.pem"' vars/main.yaml
sed -i -e '$a azure_test: true\
pulp_scenario_settings: {"domain_enabled": true}\
pulp_scenario_settings: {"api_root_rewrite_header": "X-API-Root", "domain_enabled": true}\
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering, can we infer the original relative path the user sent this request to from the request object?
Or is there some apache/nginx rewrite we going on we cannot see anymore?
(I was wondering, because drf used to use the request object to create uris, where we dumbed it down to do hrefs.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think so, unless the reverse proxy sends along that info inside some header we are completely clueless where the original path came from.

pulp_scenario_env: {"otel_bsp_max_export_batch_size": 1, "otel_bsp_max_queue_size": 1, "otel_exporter_otlp_endpoint": "http://localhost:4318", "otel_exporter_otlp_protocol": "http/protobuf", "pulp_otel_enabled": "true"}\
' vars/main.yaml
fi
Expand Down
5 changes: 3 additions & 2 deletions .github/workflows/scripts/pre_before_script.sh
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
set -euv



if [ "$TEST" = "azure" ]; then
cmd_stdin_prefix bash -c "cat > /etc/nginx/pulp/api_root_rewrite.conf" < pulpcore/tests/functional/assets/api_root_rewrite.conf
Copy link
Member

Choose a reason for hiding this comment

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

Is this related to your recent oci images change with a templated nginx config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it is not.

cmd_prefix bash -c "s6-rc -d change nginx"
cmd_prefix bash -c "s6-rc -u change nginx"
cmd_stdin_prefix bash -c "cat > /var/lib/pulp/scripts/otel_server.py" < pulpcore/tests/functional/assets/otel_server.py
cmd_user_prefix nohup python3 /var/lib/pulp/scripts/otel_server.py &
fi
2 changes: 2 additions & 0 deletions CHANGES/4207.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Added new setting ``API_ROOT_REWRITE_HEADER`` that when specified allows the API_ROOT to be rewritten
per request based on the header's value.
2 changes: 2 additions & 0 deletions CHANGES/plugin_api/4207.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Added new ``reverse`` method that handles Pulp specific url formatting. Plugins should update
instances of ``django.urls.reverse`` and ``rest_framework.reverse`` to this new Pulp one.
15 changes: 6 additions & 9 deletions pulpcore/app/models/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,16 @@
from django.core.validators import MinValueValidator
from django.db import models, transaction
from django.db.models import F, Func, Q, Value
from django.urls import reverse
from django_lifecycle import AFTER_UPDATE, BEFORE_DELETE, hook
from rest_framework.exceptions import APIException

from pulpcore.app.util import (
batch_qs,
get_url,
get_view_name_for_model,
get_domain,
get_domain_pk,
cache_key,
reverse,
)
from pulpcore.constants import ALL_KNOWN_CONTENT_CHECKSUMS, PROTECTED_REPO_VERSION_MESSAGE
from pulpcore.download.factory import DownloaderFactory
Expand Down Expand Up @@ -1254,8 +1253,7 @@ class RepositoryVersionContentDetails(models.Model):
)
count = models.IntegerField()

@property
def content_href(self):
def get_content_href(self, request=None):
Copy link
Member

Choose a reason for hiding this comment

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

This always feels like request would be the perfect candidate for a context var...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think I should do so in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

I am a little worried that we are changing a public interface here. But is this class actually public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The interface isn't changed though, further down I re-create the content_href property and have it call this "new" method.

"""
Generate URLs for the content types added, removed, or present in the RepositoryVersion.

Expand All @@ -1274,19 +1272,16 @@ def content_href(self):
ctypes = {c.get_pulp_type(): c for c in repository_model.CONTENT_TYPES}
ctype_model = ctypes[self.content_type]
ctype_view = get_view_name_for_model(ctype_model, "list")
kwargs = {}
if settings.DOMAIN_ENABLED:
kwargs["pulp_domain"] = get_domain().name
try:
ctype_url = reverse(ctype_view, kwargs=kwargs)
ctype_url = reverse(ctype_view, request=request)
except django.urls.exceptions.NoReverseMatch:
# We've hit a content type for which there is no viewset.
# There's nothing we can do here, except to skip it.
return

repository_view = get_view_name_for_model(repository_model, "list")

repository_url = reverse(repository_view, kwargs=kwargs)
repository_url = reverse(repository_view, request=request)
rv_href = (
repository_url
+ str(self.repository_version.repository_id)
Expand All @@ -1300,3 +1295,5 @@ def content_href(self):
partial_url_str = "{base}?repository_version_removed={rv_href}"
full_url = partial_url_str.format(base=ctype_url, rv_href=rv_href)
return full_url

content_href = property(get_content_href)
11 changes: 3 additions & 8 deletions pulpcore/app/response.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
from django.conf import settings
from rest_framework.response import Response
from rest_framework.reverse import reverse
from pulpcore.app.util import reverse


class OperationPostponedResponse(Response):
Expand All @@ -24,9 +23,7 @@ def __init__(self, task, request):
request (rest_framework.request.Request): Request used to generate the pulp_href urls
"""
kwargs = {"pk": task.pk}
if settings.DOMAIN_ENABLED:
kwargs["pulp_domain"] = request.pulp_domain.name
resp = {"task": reverse("tasks-detail", kwargs=kwargs, request=None)}
resp = {"task": reverse("tasks-detail", kwargs=kwargs, request=request)}
super().__init__(data=resp, status=202)


Expand All @@ -50,7 +47,5 @@ def __init__(self, task_group, request):
request (rest_framework.request.Request): Request used to generate the pulp_href urls
"""
kwargs = {"pk": task_group.pk}
if settings.DOMAIN_ENABLED:
kwargs["pulp_domain"] = request.pulp_domain.name
resp = {"task_group": reverse("task-groups-detail", kwargs=kwargs, request=None)}
resp = {"task_group": reverse("task-groups-detail", kwargs=kwargs, request=request)}
super().__init__(data=resp, status=202)
31 changes: 16 additions & 15 deletions pulpcore/app/serializers/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
get_viewset_for_model,
get_request_without_query_params,
get_domain,
reverse,
)


Expand All @@ -44,17 +45,17 @@
# Field mixins


def _reverse(reverse, request, obj):
def _reverse(obj):
"""Include domain-path in reverse call if DOMAIN_ENABLED."""

if settings.DOMAIN_ENABLED:

@functools.wraps(reverse)
def _patched_reverse(viewname, args=None, kwargs=None, **extra):
def _patched_reverse(viewname, request=None, args=None, kwargs=None, **extra):
kwargs = kwargs or {}
domain_name = obj.pulp_domain.name if hasattr(obj, "pulp_domain") else "default"
kwargs["pulp_domain"] = domain_name
return reverse(viewname, args=args, kwargs=kwargs, **extra)
return reverse(viewname, request=request, args=args, kwargs=kwargs, **extra)

return _patched_reverse

Expand All @@ -65,9 +66,9 @@ class HrefFieldMixin:
"""A mixin to configure related fields to generate relative hrefs."""

def get_url(self, obj, view_name, request, *args, **kwargs):
# Removes the request from the arguments to display relative hrefs.
self.reverse = _reverse(self.reverse, request, obj)
return super().get_url(obj, view_name, None, *args, **kwargs)
# Use the Pulp reverse method to display relative hrefs.
self.reverse = _reverse(obj)
return super().get_url(obj, view_name, request, *args, **kwargs)


class _MatchingRegexViewName(object):
Expand Down Expand Up @@ -165,14 +166,19 @@ class RelatedResourceField(RelatedField):
Specific implementation requires the model to be defined in the Meta:.
"""

def repo_ver_url(self, repo_ver):
def repo_ver_url(self, repo_ver, request=None):
repo_model = Repository.get_model_for_pulp_type(repo_ver.repository.pulp_type)
view_name = get_view_name_for_model(repo_model, "detail")
obj = PKDomainObject(pk=repo_ver.repository.pk, pulp_domain=self.context["pulp_domain"])
repo_url = self.get_url(obj, view_name, request=None, format=None)
repo_url = self.get_url(obj, view_name, request=request, format=None)
return f"{repo_url}versions/{repo_ver.number}/"

def to_representation(self, data):
# query parameters can be ignored because we are looking just for 'pulp_href'; still,
# we need to use the request object due to contextual references required by some
# serializers
request = get_request_without_query_params(self.context)

# Try to use optimized lookup to avoid DB if content_type has been already been fetched
if GenericRelationModel.content_type.is_cached(data):
model = data.content_type.model_class()
Expand All @@ -181,7 +187,7 @@ def to_representation(self, data):
repo_ver_mapping = self.context.get("repo_ver_mapping")
if repo_ver_mapping is not None:
if repo_ver := repo_ver_mapping.get(data.object_id):
return self.repo_ver_url(repo_ver)
return self.repo_ver_url(repo_ver, request=request)
return None
else:
try:
Expand All @@ -196,7 +202,7 @@ def to_representation(self, data):
else:
obj = PKObject(pk=data.object_id)
try:
return self.get_url(obj, view_name, request=None, format=None)
return self.get_url(obj, view_name, request=request, format=None)
except NoReverseMatch:
pass

Expand All @@ -210,11 +216,6 @@ def to_representation(self, data):
except AttributeError:
pass

# query parameters can be ignored because we are looking just for 'pulp_href'; still,
# we need to use the request object due to contextual references required by some
# serializers
request = get_request_without_query_params(self.context)

viewset = get_viewset_for_model(data.content_object)
serializer = viewset.serializer_class(data.content_object, context={"request": request})
return serializer.data.get("pulp_href")
Expand Down
9 changes: 3 additions & 6 deletions pulpcore/app/serializers/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,10 @@
from django.conf import settings
from rest_framework import serializers
from rest_framework.fields import empty
from rest_framework.reverse import reverse

from pulpcore.app import models
from pulpcore.app.serializers import DetailIdentityField, IdentityField, RelatedField
from pulpcore.app.util import get_domain
from pulpcore.app.util import reverse


def relative_path_validator(relative_path):
Expand Down Expand Up @@ -179,13 +178,11 @@ def to_representation(self, value):
"""
ret = {}
kwargs = {}
if settings.DOMAIN_ENABLED:
domain = get_domain()
kwargs["pulp_domain"] = domain.name
for content_artifact in value:
if content_artifact.artifact_id:
kwargs["pk"] = content_artifact.artifact_id
url = reverse("artifacts-detail", kwargs=kwargs, request=None)
request = self.context.get("request")
url = reverse("artifacts-detail", kwargs=kwargs, request=request)
else:
url = None
ret[content_artifact.relative_path] = url
Expand Down
6 changes: 5 additions & 1 deletion pulpcore/app/serializers/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -375,9 +375,13 @@ def to_representation(self, obj):

"""
to_return = {"added": {}, "removed": {}, "present": {}}
request = self.context.get("request")
for count_detail in obj.counts.all():
count_type = count_detail.get_count_type_display()
item_dict = {"count": count_detail.count, "href": count_detail.content_href}
item_dict = {
"count": count_detail.count,
"href": count_detail.get_content_href(request=request),
}
to_return[count_type][count_detail.content_type] = item_dict

return to_return
Expand Down
9 changes: 3 additions & 6 deletions pulpcore/app/serializers/task.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
from gettext import gettext as _

from django.conf import settings
from django.urls import reverse
from rest_framework import serializers

from pulpcore.app import models
Expand All @@ -15,7 +13,7 @@
TaskGroupStatusCountField,
)
from pulpcore.constants import TASK_STATES
from pulpcore.app.util import get_domain
from pulpcore.app.util import reverse


class CreatedResourceSerializer(RelatedResourceField):
Expand Down Expand Up @@ -95,9 +93,8 @@ def get_created_by(self, obj):
if task_user_map := self.context.get("task_user_mapping"):
if user_id := task_user_map.get(str(obj.pk)):
kwargs = {"pk": user_id}
if settings.DOMAIN_ENABLED:
kwargs["pulp_domain"] = get_domain().name
return reverse("users-detail", kwargs=kwargs)
request = self.context.get("request")
return reverse("users-detail", kwargs=kwargs, request=request)
return None

class Meta:
Expand Down
10 changes: 8 additions & 2 deletions pulpcore/app/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@

# API Root
API_ROOT = "/pulp/"
API_ROOT_REWRITE_HEADER = None

# Application definition

Expand Down Expand Up @@ -113,6 +114,7 @@
"django.contrib.messages.middleware.MessageMiddleware",
"django.middleware.clickjacking.XFrameOptionsMiddleware",
"pulpcore.middleware.DomainMiddleware",
"pulpcore.middleware.APIRootRewriteMiddleware",
Copy link
Member

Choose a reason for hiding this comment

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

Didn't you only want to include this when configured?
I saw the raising of MiddlewareNotUsed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's part of django's middelware api: https://docs.djangoproject.com/en/5.0/topics/http/middleware/#marking-middleware-as-unused. You list it here and then on launch the middleware's init will determine if it should be used or not.

]

AUTHENTICATION_BACKENDS = [
Expand Down Expand Up @@ -509,7 +511,11 @@
finally:
connection.close()

settings.set("V3_API_ROOT", settings.API_ROOT + "api/v3/") # Not user configurable
settings.set("V3_DOMAIN_API_ROOT", settings.API_ROOT + "<slug:pulp_domain>/api/v3/")
if settings.API_ROOT_REWRITE_HEADER:
api_root = "/<path:api_root>/"
else:
api_root = settings.API_ROOT
settings.set("V3_API_ROOT", api_root + "api/v3/") # Not user configurable
settings.set("V3_DOMAIN_API_ROOT", api_root + "<slug:pulp_domain>/api/v3/")
settings.set("V3_API_ROOT_NO_FRONT_SLASH", settings.V3_API_ROOT.lstrip("/"))
settings.set("V3_DOMAIN_API_ROOT_NO_FRONT_SLASH", settings.V3_DOMAIN_API_ROOT.lstrip("/"))