-
Notifications
You must be signed in to change notification settings - Fork 991
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 #38029 - Update subscription_manager_setup to use correct URLs for load-balanced smart proxies #10382
Fixes #38029 - Update subscription_manager_setup to use correct URLs for load-balanced smart proxies #10382
Conversation
…for load-balanced smart proxies
@@ -88,11 +88,11 @@ fi | |||
end | |||
elsif @subman_setup_scenario == 'provisioning' | |||
if plugin_present?('katello') | |||
server_hostname = @host.content_source.rhsm_url.host | |||
server_hostname = @host.content_source.registration_url.host |
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.
Using registration_url.host
here ensures that it uses the hostname of the load balancer, not of the underlying smart proxy.
server_port = @host.content_source.rhsm_url.port | ||
server_prefix = @host.content_source.rhsm_url.path | ||
repo_ca_cert = "$KATELLO_SERVER_CA_CERT" | ||
rhsm_baseurl = @host.content_source.pulp_content_url | ||
rhsm_baseurl = @host.content_source.load_balancer_pulp_content_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 load_balancer_pulp_content_url
which is the same as pulp_content_url
except it uses registration_url
as the base instead of setting(SmartProxy::PULP3_FEATURE, 'content_app_url')
. Hopefully that doesn't break anything.
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.
The changes make sense to me. I want to clarify: With this, don't we have to make any UI changes, as was discussed in the Slack thread?
app/views/unattended/provisioning_templates/snippet/subscription_manager_setup.erb
Show resolved
Hide resolved
Correct, this will resolve the issue without any UI changes. The host "registered through" will be correct again, so Hammer will display the correct "Registered To" value. However, the "Registration Details" card in the UI can be additionally improved - I kinda forgot since I was distracted by getting to the root of the issue :) I can update the UI card to display load balancer status and "registered through" value. I can do it either in the existing Katello PR or in a separate one. thoughts? |
If the effort is small, I vote to do it in one PR. |
Updated Katello/katello#11229 👍 |
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.
The code change looks good to me; let's wait for QE ACK
Couldn't apply the patch due to some conflicts, but tested by adding the changes manually. |
Thanks @jeremylenz @shubhamsg199 |
The
subscription_manager_setup
snippet, when used in provisioning, was setting the wrong URLs for subscription-manager for load-balanced smart proxy setups. This attempts to fix this by usingregistration_url
, which should correctly reflect the load balancer FQDN when the smart proxy is set up correctly with load balancing, and will reflect the smart proxy FQDN when it's not.Goes with Katello/katello#11229