fix FQDN machine name mapping on proxy configuration #7672

rjpmestre · 2023-10-11T10:09:01Z

What does this PR change?

It was noticed that configuring a proxy sometimes would fail and just display a generic error.
Analysis can be split in two:

what was causing the problem:
Files uploaded during configuration are stored and moved based on a 'machine name' mapping based on the provided 'Proxy FQDN'.

uyuni/spacewalk/certs-tools/sslToolLib.py

Line 73 in 32465f6

def getMachineName(hostname):

Current solution based on a slightly more generic occurrence:

uyuni/spacewalk/certs-tools/rhn_ssl_tool.py

Line 205 in f1f6202

machinename = '-'.join(machinename.split('-')[:-2])
why was just a generic error
This had to do with our (rhn) custom FileUtils throwing generic RTEs while RhnRuntimeException is expected

uyuni/java/code/src/com/redhat/rhn/common/util/FileUtils.java

Line 130 in 32465f6

public static String readStringFromFile(String path, boolean noLog) {

GUI diff

No difference.

DONE

Documentation

No documentation needed: only internal and user invisible changes
DONE

Test coverage

Unit tests were added
DONE

Links

Fixes: #7610

DONE

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

Re-run test "changelog_test"
Re-run test "backend_unittests_pgsql"
Re-run test "java_pgsql_tests"
Re-run test "schema_migration_test_pgsql"
Re-run test "susemanager_unittests"
Re-run test "javascript_lint"
Re-run test "spacecmd_unittests"

github-actions · 2023-10-11T10:09:33Z

Suggested tests to cover this Pull Request

minssh_salt_install_package
min_deblike_monitoring
srv_power_management
min_salt_mgrcompat_state
srv_sync_channels
proxy_cobbler_pxeboot
allcli_software_channels_dependencies
srv_content_lifecycle
minkvm_guests
srv_task_status_engine
srv_patches_page
srv_logfile
srv_rename_hostname
srv_docker_cve_audit
srv_enable_sync_products
srv_create_repository
srv_cobbler_profile
min_empty_system_profiles
min_ansible_control_node
srv_reportdb
min_retracted_patches
srv_add_rocky8_repositories
srv_organization_credentials
allcli_reboot
min_bootstrap_ssh_key
min_deblike_salt_install_package
buildhost_docker_build_image
minssh_bootstrap_api
srv_advanced_search
srv_cobbler_distro
srv_scc_user_credentials
min_rhlike_ssh
allcli_update_activationkeys
srv_datepicker
min_salt_openscap_audit
srv_change_password
srv_user_preferences
srv_cobbler_sync
min_salt_install_with_staging
allcli_config_channel
min_salt_software_states
srv_channels_add
min_bootstrap_api
min_bootstrap_reactivation
min_rhlike_monitoring
srv_distro_cobbler
min_rhlike_salt_install_package_and_patch
buildhost_osimage_build_image
srv_group_union_intersection
allcli_action_chain
min_salt_pkgset_beacon
buildhost_bootstrap
proxy_branch_network
min_cve_id_new_syntax
allcli_sanity
min_deblike_ssh
srv_check_channels_page
srv_dist_channel_mapping
min_config_state_channel
srv_change_task_schedule
min_salt_minions_page
srv_delete_channel_with_tool
srv_notifications
min_deblike_remote_command
sle_ssh_minion
srv_salt
min_custom_pkg_download_endpoint
allcli_software_channels
srv_maintenance_windows
sle_minion
srv_cobbler_buildiso
minssh_move_from_and_to_proxy
srv_users
srv_push_package
min_docker_api
srv_check_sync_source_packages
allcli_system_group
srv_handle_config_channels_with_ISS_v2
srv_delete_channel_from_ui
srv_restart
srv_power_management_redfish
srv_user_configuration_salt_states
min_bootstrap_negative
min_activationkey
srv_clone_channel_npn
min_rhlike_salt
min_salt_formulas_advanced
srv_handle_software_channels_with_ISS_v2
srv_manage_channels_page
srv_mainpage
min_salt_migration
min_recurring_action
proxy_as_pod_basic_tests
min_salt_minion_details
min_salt_lock_packages
min_deblike_openscap_audit
srv_check_reposync
min_check_patches_install
min_virthost
srv_power_management_api
min_move_from_and_to_proxy
min_project_lotus
min_config_state_channel_subscriptions
srv_payg_ssh_connection
min_timezone
min_salt_user_states
srv_activationkey_api
min_deblike_salt
srv_monitoring
min_deblike_salt_install_with_staging
min_cve_audit
min_rhlike_remote_command
srv_channel_api
proxy_retail_pxeboot_and_mass_import
min_bootstrap_script
min_rhlike_openscap_audit
srv_docker_advanced_content_management
srv_salt_download_endpoint
proxy_register_as_minion_with_script
min_salt_install_package
srv_menu
min_monitoring
min_change_software_channel
min_action_chain
srv_osimage
srv_first_settings
minssh_action_chain
srv_create_activationkey
min_salt_formulas
allcli_overview_systems_details
srv_disable_local_repos_off
min_ssh_tunnel

cbosdo

I like how you document what was the issue in the PR description. Since it's fairly hard to find the PR from a given commit, would you mind adding it to the commit description too? In such cases if there is only one commit GH adds the description to the PR when you create it.

java/code/src/com/redhat/rhn/common/util/FileUtils.java

cbosdo · 2023-10-13T12:19:31Z

java/spacewalk-java.changes.rjpmestre.fix_proxy_config_fqdn_mapping

+- fix FQDN machine name mapping on proxy configuration
+- fix proxy configuration error display


This is for users to read: you don't have to be too specific. Think about you would like to see here as a user that indicates your bug is fixed. And I wouldn't have 2 entries here: one is enough

Got you! To see it as end user is a good hint.

cbosdo · 2023-10-13T12:22:01Z

java/code/src/com/suse/manager/ssl/SSLCertData.java

-        else {
-            return String.join(".");
-        }
+        String[] hostnameParts = this.getCn().replace("*", "_star_").split("\\.");


What I don't understand here is why this is needed... Was the bug only if there is a * in the proxy FDQN? or is that character added somehow? The proxy FQDN should not contain such a character: it's a single name.

I am not sure why you'd use asterisk, ie, specifying/restricting domain in this context.
I wasn't expecting this to be a valid input, but it doesn't even fail any validation. For me, this could even have been a slip.
But the fact they were specifically handled by python's machine name mapping is what made me think asterisks are expected.

As far as i see we have 2 options:

keep the solution, focusing on the original failure when accessing the uploaded files

add a validation to the fqdn inputs 'Proxy FQDN' and 'Parent FQDN' when they contain

what do you think?

Adding validation of the FQDNs couldn't be a bad thing, but I had the issue here even without any invalid one... so I think the truth is somewhere else

cbosdo · 2023-10-14T13:37:46Z

After some quick debugging here, I found that we miss a LOG.error("Failed to generate proxy configuration", e) in

uyuni/java/code/src/com/suse/manager/webui/controllers/ProxyController.java

Line 138 in 7d27393

return json(response, HttpStatus.SC_BAD_REQUEST, e.getMessage());

cbosdo · 2023-10-14T13:58:10Z

After trying again I figured out I was mistyping my CA password. Once set right I got the config generated correctly. Could you please add the missing log and add the verification of the FQDNs on the Java side? I can show you later how to add them on the TSX side if you are interested in that 😜

It was noticed that configuring a proxy sometimes would fail and just display a generic error. Analysis can be split in two: - what was causing the problem Files uploaded during configuration are stored and moved based on a 'machine name' mapping based on the provided 'Proxy FQDN'. https://github.com/uyuni-project/uyuni/blob/32465f6efccd319926ae1897e14d9d20d4d89c39/spacewalk/certs-tools/sslToolLib.py#L73 Current solution based on a slightly more generic occurrence: https://github.com/uyuni-project/uyuni/blob/f1f62028c363f8ea95b0cc080b4fa9eddf9fbaa0/spacewalk/certs-tools/rhn_ssl_tool.py#L205 - why was just a generic error This had to do with our (rhn) custom FileUtils throwing generic RTEs while RhnRuntimeException is expected https://github.com/uyuni-project/uyuni/blob/32465f6efccd319926ae1897e14d9d20d4d89c39/java/code/src/com/redhat/rhn/common/util/FileUtils.java#L130

rjpmestre requested a review from a team as a code owner October 11, 2023 10:09

rjpmestre requested review from lucidd, cbosdo and mbussolotto and removed request for a team and lucidd October 11, 2023 10:09

github-actions bot added java java_lint_checkstyle java_pgsql_tests labels Oct 11, 2023

cbosdo reviewed Oct 13, 2023

View reviewed changes

rjpmestre force-pushed the fix_proxy_config_fqdn_mapping branch from d2ffa4a to 0ba0550 Compare October 17, 2023 10:21

rjpmestre requested a review from a team as a code owner October 17, 2023 10:21

rjpmestre requested review from parlt91 and removed request for a team October 17, 2023 10:21

github-actions bot added ui javascript_lint needs-translation labels Oct 17, 2023

rjpmestre force-pushed the fix_proxy_config_fqdn_mapping branch 2 times, most recently from 4cfeef7 to a09df8b Compare October 17, 2023 10:31

rjpmestre requested a review from cbosdo October 17, 2023 11:16

rjpmestre force-pushed the fix_proxy_config_fqdn_mapping branch from a09df8b to a4071bf Compare October 20, 2023 10:01

rjpmestre force-pushed the fix_proxy_config_fqdn_mapping branch from a4071bf to 966629a Compare October 20, 2023 11:11

cbosdo approved these changes Oct 20, 2023

View reviewed changes

rjpmestre merged commit f7aff89 into uyuni-project:master Oct 20, 2023
15 of 18 checks passed

rjpmestre deleted the fix_proxy_config_fqdn_mapping branch December 18, 2023 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix FQDN machine name mapping on proxy configuration #7672

fix FQDN machine name mapping on proxy configuration #7672

rjpmestre commented Oct 11, 2023 •

edited

Loading

github-actions bot commented Oct 11, 2023

cbosdo left a comment

cbosdo Oct 13, 2023

rjpmestre Oct 13, 2023

cbosdo Oct 13, 2023

rjpmestre Oct 13, 2023 •

edited

Loading

cbosdo Oct 14, 2023

cbosdo commented Oct 14, 2023

cbosdo commented Oct 14, 2023

		- fix FQDN machine name mapping on proxy configuration
		- fix proxy configuration error display

fix FQDN machine name mapping on proxy configuration #7672

fix FQDN machine name mapping on proxy configuration #7672

Conversation

rjpmestre commented Oct 11, 2023 • edited Loading

What does this PR change?

GUI diff

Documentation

Test coverage

Links

Changelogs

Re-run a test

github-actions bot commented Oct 11, 2023

cbosdo left a comment

Choose a reason for hiding this comment

cbosdo Oct 13, 2023

Choose a reason for hiding this comment

rjpmestre Oct 13, 2023

Choose a reason for hiding this comment

cbosdo Oct 13, 2023

Choose a reason for hiding this comment

rjpmestre Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

cbosdo Oct 14, 2023

Choose a reason for hiding this comment

cbosdo commented Oct 14, 2023

cbosdo commented Oct 14, 2023

rjpmestre commented Oct 11, 2023 •

edited

Loading

rjpmestre Oct 13, 2023 •

edited

Loading