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

gerrod3 · 2024-02-01T06:27:04Z

mdellweg · 2024-02-01T09:17:48Z

pulpcore/app/models/repository.py

@@ -1254,8 +1254,7 @@ class RepositoryVersionContentDetails(models.Model):
    )
    count = models.IntegerField()

-    @property
-    def content_href(self):
+    def get_content_href(self, request=None):


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

Do you think I should do so in this PR?

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

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

mdellweg · 2024-02-02T09:32:40Z

pulpcore/app/models/repository.py

@@ -1254,8 +1254,7 @@ class RepositoryVersionContentDetails(models.Model):
    )
    count = models.IntegerField()

-    @property
-    def content_href(self):
+    def get_content_href(self, request=None):


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

pulpcore/app/path_api_urls.py

mdellweg · 2024-02-02T09:38:01Z

pulpcore/app/path_api_urls.py

+PATH_API_ROOT = "<path:api_root>/" + API_ROOT.split(settings.API_ROOT[1:])[1]
+dup_urls = special_views + docs_and_status
+for router in all_routers:
+    dup_urls.extend(router.urls)


Do we really need to dupe the url patterns? Can't we just also handle the default paths with the global match pattern?

Also do plugins with individual urlpattern opt into this? Do we get the whole "domain enabled" story all over again?

Now using a global pattern placed into V3_API_ROOT. Plugins that use this for extending their urlpatterns will automatically get support for this feature.

mdellweg · 2024-02-02T09:40:11Z

pulpcore/app/serializers/base.py

-        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


At least some consolidating codepaths. ;)

Might have to change this back to a patched reverse (albeit using our new Pulp reverse). The tests are failing because super().get_url doesn't accept kwargs to pass onto reverse, so the domain can't be specified. For serializing fields we need to know if the object has a domain relationship to use the correct domain name, the requests domain is not good enough.

mdellweg · 2024-02-02T09:42:40Z

pulpcore/app/settings.py

@@ -113,6 +114,7 @@
    "django.contrib.messages.middleware.MessageMiddleware",
    "django.middleware.clickjacking.XFrameOptionsMiddleware",
    "pulpcore.middleware.DomainMiddleware",
+    "pulpcore.middleware.APIRootRewriteMiddleware",


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

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.

mdellweg · 2024-03-19T08:46:44Z

.github/workflows/scripts/install.sh

@@ -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}\


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.)

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.

fixes: pulp#4207

[noissue]

mdellweg · 2024-04-04T10:08:18Z

.github/workflows/scripts/pre_before_script.sh

 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


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

No it is not.

gerrod3 · 2024-05-10T15:30:20Z

@newswangerd If you have any free time would love to hear feedback on the design of this feature and if it accomplishes what you need.

mdellweg reviewed Feb 1, 2024

View reviewed changes

mdellweg reviewed Feb 2, 2024

View reviewed changes

gerrod3 marked this pull request as draft February 6, 2024 14:01

gerrod3 force-pushed the proxy-base-path branch from c28b04d to a9c3bcb Compare March 18, 2024 13:36

github-actions bot added the multi-commit label Mar 18, 2024

gerrod3 force-pushed the proxy-base-path branch 5 times, most recently from bfe1c84 to 89bc79e Compare March 18, 2024 23:46

mdellweg reviewed Mar 19, 2024

View reviewed changes

gerrod3 force-pushed the proxy-base-path branch 4 times, most recently from d2e8f2f to 3fcde86 Compare March 25, 2024 14:55

gerrod3 marked this pull request as ready for review March 25, 2024 14:56

Add ability to have proxy rewrite API_ROOT w/ header

0bf2deb

fixes: pulp#4207

gerrod3 force-pushed the proxy-base-path branch 7 times, most recently from a2745ad to ba404f7 Compare April 4, 2024 06:14

rework url routing

bb71385

[noissue]

gerrod3 force-pushed the proxy-base-path branch from ba404f7 to bb71385 Compare April 4, 2024 06:44

mdellweg reviewed Apr 4, 2024

View reviewed changes

goosemania requested a review from newswangerd May 15, 2024 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gerrod3 commented Feb 1, 2024 •

edited

mdellweg Feb 1, 2024

gerrod3 Feb 2, 2024

mdellweg Feb 2, 2024

gerrod3 Feb 2, 2024

mdellweg Feb 2, 2024

mdellweg Feb 2, 2024

mdellweg Feb 2, 2024

gerrod3 Mar 19, 2024

mdellweg Feb 2, 2024

gerrod3 Mar 19, 2024

mdellweg Feb 2, 2024

gerrod3 Feb 2, 2024

mdellweg Mar 19, 2024

gerrod3 Mar 19, 2024

mdellweg Apr 4, 2024

gerrod3 Apr 4, 2024

gerrod3 commented May 10, 2024

		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

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

Are you sure you want to change the base?

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

Conversation

gerrod3 commented Feb 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerrod3 commented May 10, 2024

gerrod3 commented Feb 1, 2024 •

edited