Fixes #19037: Refactor the system techniques by component #1652

Fdall · 2021-03-17T14:56:03Z

https://issues.rudder.io/issues/19037

ncharles

this is really nicer than what we have
some minor changes request but this is a great step in the right direction

ncharles · 2021-03-18T13:38:43Z

techniques/systemSettings/systemRelay/1.0/apache/main.cf

+      "pass1" expression => "any";
+  methods:
+    pass3::
+      "any" usebundle => enable_reporting;


why are you enabling reporting ? it should already be enabled

ncharles · 2021-03-18T13:38:56Z

techniques/systemSettings/systemRelay/1.0/apache/main.cf

+    redhat::
+      "apache_service"   string => "httpd";
+
+  classes:


it doesn't seem useful here to set up the classes

ncharles · 2021-03-18T13:48:33Z

techniques/systemSettings/systemRelay/1.0/apache/rudder_system_apache_networks_check.cf

+      "server_ip_found" expression => regcmp("^[0-9.]+$|^[0-9a-fA-F:]+:[0-9a-fA-F:]+$", "${policy_server_ip}");
+
+      # Restart apache at the end of the technique if needed
+      "system_restart_apache" expression => "${allowed_network_prefix}_repaired|${remote_run_prefix}_repaired",


i'd rather have an even more specific name:
"rudder_server_system_restart_apache", or something like that, to minimize classes colision risks

ncharles · 2021-03-18T13:49:46Z

techniques/systemSettings/systemRelay/1.0/common/reload_rudder_services.cf

+      "restart_jetty_password" usebundle => _method_reporting_context("Reload rudder services", "None");
+    rudder_system_restart_jetty::
+      "restart_jetty_password" usebundle => service_restart("${jetty_service_name}");
+    rudder_system_restart_apache::


as sad earlier, i'd like something more specific, like "rudder_server_system_restart_apache"

ncharles · 2021-03-18T13:52:10Z

techniques/systemSettings/systemRelay/1.0/apache/rudder_system_apache_password_check_dav.cf

+     "technique_name"         string => "server-roles";
+     "report_string"          string => "Apache WebDAV user and password";
+
+      "webdav_pwd_cmd"        string => "${htpasswd_bin} -b ${g.rudder_base}/etc/htpasswd-webdav ${g.davuser} ${g.davpw}";


identation is not consistent in this block

ncharles · 2021-03-18T14:02:29Z

techniques/systemSettings/systemRelay/1.0/common/reload_rudder_services.cf

+    rudder_system_restart_jetty::
+      "restart_jetty_password" usebundle => service_restart("${jetty_service_name}");
+    rudder_system_restart_apache::
+      "restart_jetty_password" usebundle => service_reload("${apache_service}");


Suggested change

"restart_jetty_password" usebundle => service_reload("${apache_service}");

"restart_apache_password" usebundle => service_reload("${apache_service}");

ncharles · 2021-03-18T14:02:54Z

techniques/systemSettings/systemRelay/1.0/common/reload_rudder_services.cf

+
+    any::
+      "jetty_prefix"         string => canonify("service_restart_${jetty_service_name}");
+      "apache_prefix"         string => canonify("service_restart_${apache_service_name}");


Suggested change

"apache_prefix" string => canonify("service_restart_${apache_service_name}");

"apache_prefix" string => canonify("service_reload_${apache_service_name}");

ncharles · 2021-03-18T14:05:20Z

techniques/systemSettings/systemRelay/1.0/common/reload_rudder_services.cf

+      # Reporting
+      "report_error"   usebundle  => rudder_common_report("${technique_name}", "result_error", "${server_roles_common.directiveId}", "${component_name}", "None", "${report_string}"),
+                       ifvarclass => "result_error";
+      "report_repaired" usebundle => rudder_common_report("${technique_name}", "result_error", "${server_roles_common.directiveId}", "${component_name}", "None", "${report_string}"),


Suggested change

"report_repaired" usebundle => rudder_common_report("${technique_name}", "result_error", "${server_roles_common.directiveId}", "${component_name}", "None", "${report_string}"),

"report_repaired" usebundle => rudder_common_report("${technique_name}", "result_repaired", "${server_roles_common.directiveId}", "${component_name}", "None", "${report_string}"),

ncharles · 2021-03-18T14:05:39Z

techniques/systemSettings/systemRelay/1.0/common/reload_rudder_services.cf

+      "restart_jetty_password" usebundle => enable_reporting;
+
+      # Reporting
+      "report_error"   usebundle  => rudder_common_report("${technique_name}", "result_error", "${server_roles_common.directiveId}", "${component_name}", "None", "${report_string}"),


where is the report_string defined ?

ncharles · 2021-03-18T14:07:29Z

techniques/systemSettings/systemRelay/1.0/relayd/relayd.cf

+bundle agent rudder_system_relayd_configuration {
+  vars:
+      "config_dir"         string => "${g.rudder_base}/etc/relayd";
+      "config_file"        string => "${config_dir}/main.conf2";


Suggested change

"config_file" string => "${config_dir}/main.conf2";

"config_file" string => "${config_dir}/main.conf";

amousset · 2021-03-18T14:11:27Z

techniques/systemSettings/systemRelay/1.0/apache/main.cf

+      "any" usebundle => service_enabled("${apache_service}");
+
+      # Configure relayd
+      "any" usebundle => rudder_system_relayd_configuration;


relayd should not be part of an apache bundle IMHO

Sure, do you think I should make two different techniques (one apache and one relayd) or keep one "relay" technique with an apache and a relayd bundle?

amousset · 2021-03-18T14:14:30Z

techniques/systemSettings/systemRelay/1.0/common/reload_rudder_services.cf

+      "restart_jetty_password" usebundle => disable_reporting;
+      "restart_jetty_password" usebundle => _method_reporting_context("Reload rudder services", "None");
+    rudder_system_restart_jetty::
+      "restart_jetty_password" usebundle => service_restart("${jetty_service_name}");


no need to mention _password here, we do not need to leak this information across bundles, knowing we need to restart it is enough

Fdall · 2021-03-30T14:25:23Z

Commit modified

Fixes #19037: Refactor the system techniques by component

Fdall · 2021-03-30T14:26:55Z

PR updated with a new commit

Fixes #19037: Refactor the system techniques by component

Fdall · 2021-03-30T14:32:03Z

PR updated with a new commit

…component Fixes #19037: Refactor the system techniques by component

Fdall · 2021-03-30T14:32:30Z

PR updated with a new commit

Fdall · 2021-03-31T08:53:35Z

Migrating the changes to #1655

Fdall requested a review from amousset March 17, 2021 14:56

Fdall added the Trigger test label Mar 17, 2021

ncharles requested changes Mar 18, 2021

View reviewed changes

amousset reviewed Mar 18, 2021

View reviewed changes

Fixes #19037: Refactor the system techniques by component

4a38e55

Fdall force-pushed the arch_19037/refactor_the_system_techniques_by_component branch from b9c7cdc to 4a38e55 Compare March 30, 2021 14:25

Fdall added Trigger test and removed Trigger test labels Mar 30, 2021

fixup! Fixes #19037: Refactor the system techniques by component

76516e6

Fixes #19037: Refactor the system techniques by component

Fdall added Trigger test and removed Trigger test labels Mar 30, 2021

fixup! fixup! Fixes #19037: Refactor the system techniques by component

59d01cd

Fixes #19037: Refactor the system techniques by component

Fdall added Trigger test and removed Trigger test labels Mar 30, 2021

fixup! fixup! fixup! Fixes #19037: Refactor the system techniques by …

9107ce6

…component Fixes #19037: Refactor the system techniques by component

Fdall added Trigger test and removed Trigger test labels Mar 30, 2021

Fdall added the qa: Can't merge label Mar 31, 2021

Fdall closed this Mar 31, 2021

Fdall added Trigger test and removed Trigger test labels Apr 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #19037: Refactor the system techniques by component #1652

Fixes #19037: Refactor the system techniques by component #1652

Fdall commented Mar 17, 2021

ncharles left a comment

ncharles Mar 18, 2021

ncharles Mar 18, 2021

ncharles Mar 18, 2021

ncharles Mar 18, 2021

ncharles Mar 18, 2021

ncharles Mar 18, 2021

ncharles Mar 18, 2021

ncharles Mar 18, 2021

ncharles Mar 18, 2021

ncharles Mar 18, 2021

amousset Mar 18, 2021

Fdall Mar 18, 2021

amousset Mar 18, 2021

Fdall commented Mar 30, 2021

Fdall commented Mar 30, 2021

Fdall commented Mar 30, 2021

Fdall commented Mar 30, 2021

Fdall commented Mar 31, 2021

	"restart_jetty_password" usebundle => service_reload("${apache_service}");
	"restart_apache_password" usebundle => service_reload("${apache_service}");

	"apache_prefix" string => canonify("service_restart_${apache_service_name}");
	"apache_prefix" string => canonify("service_reload_${apache_service_name}");

	"report_repaired" usebundle => rudder_common_report("${technique_name}", "result_error", "${server_roles_common.directiveId}", "${component_name}", "None", "${report_string}"),
	"report_repaired" usebundle => rudder_common_report("${technique_name}", "result_repaired", "${server_roles_common.directiveId}", "${component_name}", "None", "${report_string}"),

	"config_file" string => "${config_dir}/main.conf2";
	"config_file" string => "${config_dir}/main.conf";

Fixes #19037: Refactor the system techniques by component #1652

Fixes #19037: Refactor the system techniques by component #1652

Conversation

Fdall commented Mar 17, 2021

ncharles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fdall commented Mar 30, 2021

Fdall commented Mar 30, 2021

Fdall commented Mar 30, 2021

Fdall commented Mar 30, 2021

Fdall commented Mar 31, 2021