-
Notifications
You must be signed in to change notification settings - Fork 297
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
Fixes #38028 - Update smart proxy URI methods for load balancer setups #11229
Conversation
URI(setting(SmartProxy::PULP3_FEATURE, 'content_app_url')) | ||
end | ||
|
||
def load_balancer_pulp_content_url | ||
URI::HTTPS.build(host: registration_url.host, path: pulp_content_url.path) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hopefully setting(SmartProxy::PULP3_FEATURE, 'content_app_url')
doesn't contain any crucial information that would be lost by only using its path
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ianballou Would this url be different for a smart proxy that uses an external Pulp? Is that setup supported with load balanced smart proxies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to rely on the path always being /pulp/content
def registration_url | ||
url = self.setting('Registration', 'registration_url').presence || self.url | ||
URI.parse(url).host | ||
URI(url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the bare registration_url
method here so that Foreman can do things to the URI
object it returns, directly (e.g. using its path
and port
).
fa41086
to
4eb82e7
Compare
The Foreman PR is now merged. This should be merged soon since the Foreman change needs it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test with a full load balancer, but the UI is looking fine (by adding mock data) and the updated smart_proxy_extensions code makes sense.
What are the changes introduced in this pull request?
Goes with theforeman/foreman#10382
For content sources that are behind a load balancer:
For non-load balanced content sources, the card will not show the load balancer info:
Considerations taken when implementing this change?
As always, "Smart Proxy" will be changed to "Capsule" downstream by foreman_theme_satellite.
What are the testing steps for this pull request?
Set up a load-balanced smart proxy with
registration_url
set to the fqdn of the load balancer, as outlined in the docsProvision a host with the load balanced smart proxy set as the content source (?)
The host's "registered through" value should be that of the load balancer, not that of the smart proxy behind the load balancer
To test the UI changes without a load balancer, you can do