Skip to content

Conversation

@jeremylenz
Copy link
Member

@jeremylenz jeremylenz commented Nov 20, 2024

What are the changes introduced in this pull request?

Goes with theforeman/foreman#10382

  1. Update smart proxy methods so that provisioning can use correct URLs in sub-man config.
  2. Update the "Registration details" card to properly show load-balanced smart proxies, and also add the "Registered to" value to match what is shown in Hammer.

For content sources that are behind a load balancer:
image

For non-load balanced content sources, the card will not show the load balancer info:
image

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 docs
Provision 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

diff --git a/app/models/katello/concerns/smart_proxy_extensions.rb b/app/models/katello/concerns/smart_proxy_extensions.rb
index 0311b4422a..944c875997 100644
--- a/app/models/katello/concerns/smart_proxy_extensions.rb
+++ b/app/models/katello/concerns/smart_proxy_extensions.rb
@@ -138,6 +138,7 @@ module Katello
 
       def registration_host
         registration_url.host
+        pulp_primary? ? registration_url.host : "loadbalancer.example.com"
       end

Comment on lines 602 to +607
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
Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Member

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

Comment on lines +134 to +136
def registration_url
url = self.setting('Registration', 'registration_url').presence || self.url
URI.parse(url).host
URI(url)
Copy link
Member Author

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

@jeremylenz
Copy link
Member Author

The Foreman PR is now merged. This should be merged soon since the Foreman change needs it.

Copy link
Member

@ianballou ianballou left a 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.

@ianballou ianballou merged commit 7ba19bd into Katello:master Dec 5, 2024
25 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants