Initial implementation of the smart_proxy app #1
Conversation
evgeni
force-pushed
the
cleanup
branch
13 times, most recently
from
9e995fd to
95c47b1
Compare
| operation_id="version_read", | ||
| ) | ||
| def get(self, request): | ||
| data = {"version": get_plugin_config("smart_proxy").version} |
There was a problem hiding this comment.
This makes Foreman issue a warning as the version obviously doesn't match the Foreman version…
There was a problem hiding this comment.
I thought Foreman is supposed to work with "older" versions of the smart proxy...
There was a problem hiding this comment.
It does! Doesn't forbid it to issue useless warnings ;)
There was a problem hiding this comment.
This makes Foreman issue a warning as the version obviously doesn't match the Foreman version…
Yes, I've been complaining about that a few times: we need a better way to determine compatibility than version numbers.
| "content_app_url": urljoin(host, settings.CONTENT_PATH_PREFIX), | ||
| "username": settings.SMART_PROXY_AUTH_USERNAME, | ||
| "password": settings.SMART_PROXY_AUTH_PASSWORD, | ||
| "client_authentication": settings.SMART_PROXY_AUTH_METHODS, |
There was a problem hiding this comment.
I was wondering if we could derive these values from settings. We set some values (https://github.com/theforeman/puppet-pulpcore/blob/8288f3736a1543417eba6322e7013d0f230bd85e/templates/settings.py.erb#L52-L59) so perhaps that works.
I think you should check if settings.AUTHENTICATION_BACKENDS and derive values based on that.
There was a problem hiding this comment.
I thought about that, but then wasn't sure how to get from pulpcore.app.authentication.PulpNoCreateRemoteUserBackend to client_certificate -- does the one imply the other?
Looking at https://pulpproject.org/pulpcore/docs/admin/guides/auth/external/, PulpNoCreateRemoteUserBackend does imply external auth, but the kind of external is up to the user. (OTOH, we're in the Foreman-domain here, and we probably can imply cert?)
There was a problem hiding this comment.
The non-cert option is mostly interesting for development setups, but I think it's highly questionable if it makes sense in this plugin.
In the Ruby based implementation we assume a trusted (authenticated) channel from Foreman to Foreman Proxy. The v2 features API (which exposes the settings) is normally only available if you are authenticated (typically using client certs).
If you take that to this, you either make the username/password available without authentication (effectively disabling authentication) or you have authentication but then don't need the credentials.
What you can do is to return [] if pulpcore.app.authentication.PulpNoCreateRemoteUserBackend isn't in there, but realistically the current implementation is probably OK.
There was a problem hiding this comment.
Is this related to pulp/pulpcore#5438 which we could not bring forward until openapi 3.1.
There was a problem hiding this comment.
Sorta, but also not really :)
We have a Pulp running somewhere, and we need to tell the "client" (Katello) how to get there and how to auth there.
Today we do this by talking to the smart proxy and that returns a mostly static setting hash (pulp_url, client_authentication are the most important here) that gets used in other parts of the app.
So yeah, if the APIdoc would announce "you can use certs here", that would get rid of that particular line, the whole concept of "someone tells katello how pulp is configured" remains (I'd argue the way we do discovery of services, like pulp, is not optimal)
evgeni
force-pushed
the
cleanup
branch
3 times, most recently
from
68043c3 to
6163060
Compare
evgeni
force-pushed
the
cleanup
branch
3 times, most recently
from
1f17b46 to
5b436b7
Compare
|
I think I've addressed everything that I could address without going tooo crazy for a POC :) |
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
I think you can summarize the PR now as "Initial implementation" because IMHO it's no longer really trivial, but still very nice basis.
Other than the version check that Foreman warns about, I think this looks really nice. Even uncovered some other bugs along the way.
|
Also as a general note looking forward: this will not be compatible with Foreman's model of HA Smart Proxy, but I think that's a feature. If you need Pulp to be HA, run it HA instead of syncing it from the Foreman side. |
evgeni
changed the title
You could say it's bug-for-bug compatible with the current implementation :) |
Not really, because you can set the Pulp URL to the loadbalancer's URL. Your current implementation doesn't allow that. So there is only one API URL that both clients and Foreman talk to, instead of some control path URL and public URL. |
Huh? You can set |
|
Some clients (like |
No description provided.