Skip to content
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

Add Global params for host obfuscation and migrate off settings #921

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

chris1984
Copy link
Member

@chris1984 chris1984 commented Nov 8, 2024

What are the changes introduced in this pull request?

  • Created a global parameter for obfuscate_inventory_hostnames and obfuscate_inventory_ips and set them to false which is the default value for insights-client
  • Created a DB migration to set the values of the global parameters to the value of the settings for obfuscate_inventory_hostnames and obfuscate_inventory_ips and deletes the settings from the plugin and DB
  • Removed the UI toggles
  • Updated UI and JS tests to use the new parameters
  • These parameters will be used for the REX job template for existing hosts to configure host obfuscation and for global registration.

@chris1984 chris1984 requested a review from parthaa November 8, 2024 20:38
@chris1984 chris1984 force-pushed the obfuscate-settings branch 3 times, most recently from 3e407b8 to efbca09 Compare November 8, 2024 20:55
@chris1984 chris1984 marked this pull request as ready for review November 8, 2024 21:06
Copy link
Member

@ShimShtein ShimShtein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will need to add a UI message on the inventory page about the ability to obfuscate using the parameter. Otherwise it will feel to the user that the setting just disappeared without replacement.

Comment on lines +4 to +5
CommonParameter.find_or_create_by!(name: 'obfuscate_inventory_hostnames', value: false)
CommonParameter.find_or_create_by!(name: 'obfuscate_inventory_ips', value: false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CommonParameter.find_or_create_by!(name: 'obfuscate_inventory_hostnames', value: false)
CommonParameter.find_or_create_by!(name: 'obfuscate_inventory_ips', value: false)
hostname_setting = CommonParameter.find_or_create_by!(name: 'obfuscate_inventory_hostnames', value: false)
ip_setting = CommonParameter.find_or_create_by!(name: 'obfuscate_inventory_ips', value: false)

You can skip the find_by later in the code

@@ -61,7 +61,7 @@ def obfuscate_hostname?(host)
insights_client_setting = ActiveModel::Type::Boolean.new.cast(insights_client_setting)
return insights_client_setting unless insights_client_setting.nil?

Setting[:obfuscate_inventory_hostnames]
CommonParameter.find_by(name: 'obfuscate_inventory_hostnames')&.value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should look for a value on the host, not just the common parameter, similar to

param_value = host.host_param(InsightsCloud.enable_client_param)

@@ -79,7 +79,7 @@ def obfuscate_ips?(host)
insights_client_setting = ActiveModel::Type::Boolean.new.cast(insights_client_setting)
return insights_client_setting unless insights_client_setting.nil?

Setting[:obfuscate_inventory_ips]
CommonParameter.find_by(name: 'obfuscate_inventory_ips')&.value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a parameter, you have to look up the value on the host object itself:

param_value = host.host_param(InsightsCloud.enable_client_param)

@@ -114,7 +114,7 @@ def hostname_match
foreman_hostname = ForemanRhCloud.foreman_host&.name
if bash_hostname == foreman_hostname
fqdn(ForemanRhCloud.foreman_host)
elsif Setting[:obfuscate_inventory_hostnames]
elsif CommonParameter.find_by(name: 'obfuscate_inventory_hostnames')&.value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a parameter, you have to look up the value on the host object itself:

param_value = host.host_param(InsightsCloud.enable_client_param)

@@ -23,8 +23,8 @@ class MetadataGeneratorTest < ActiveSupport::TestCase
end

test 'generates an empty report with hidden hostname and ip' do
Setting[:obfuscate_inventory_hostnames] = true
Setting[:obfuscate_inventory_ips] = true
CommonParameter.find_or_create_by!(name: 'obfuscate_inventory_ips', value: true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the parameter exists, I think this method will not reset its value.

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