feat(node): Configuration revamp (SetupOS integration) #3270
Conversation
| /// Write SetupOS or HostOS systemd network configuration. | ||
| /// Requires superuser permissions to run `ipmitool` and write to the systemd directory | ||
| /// TODO(NODE-1466): Consolidate generate_network_config_new_config and generate_network_config | ||
| pub fn generate_network_config_new_config( | ||
| network_settings: &NetworkSettings, | ||
| generated_mac: &MacAddr6, | ||
| output_directory: &Path, | ||
| ) -> Result<()> { | ||
| eprintln!("Generating IPv6 address"); | ||
|
|
||
| match &network_settings.ipv6_config { | ||
| Ipv6Config::RouterAdvertisement => { | ||
| Err(anyhow!("IC-OS router advertisement is not yet supported")) | ||
| } | ||
| Ipv6Config::Fixed(_) => Err(anyhow!("Fixed IP configuration is not yet supported")), | ||
| Ipv6Config::Deterministic(ipv6_config) => { | ||
| let ipv6_address = generated_mac.calculate_slaac(&ipv6_config.prefix)?; | ||
| eprintln!("Using IPv6 address: {ipv6_address}"); | ||
|
|
||
| generate_systemd_config_files_new_config( | ||
| output_directory, | ||
| ipv6_config, | ||
| Some(generated_mac), | ||
| &ipv6_address, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
HostOS still uses the same network crate, so I've created generate_network_config_new_config for SetupOS to use while HostOS will continue to use the untouched generate_network_config function.
I've made a similar change with generate_systemd_config_files_new_config / generate_systemd_config_files in systemd.rs
Once HostOS has been updated to integrate into the new config, these functions will be consolidated.
| if [[ -v ipv4_address && -n ${ipv4_address} && -v ipv4_prefix_length && -n ${ipv4_prefix_length} && -v ipv4_gateway && -n ${ipv4_gateway} && -v domain && -n ${domain} ]]; then | ||
| if [[ -n ${ipv4_address} && -n ${ipv4_prefix_length} && -n ${ipv4_gateway} ]]; then | ||
| echo " IPv4 Address: ${ipv4_address}" | ||
| echo " IPv4 Prefix Length: ${ipv4_prefix_length}" | ||
| echo " IPv4 Gateway: ${ipv4_gateway}" | ||
| echo " Domain name : ${domain}" | ||
| fi | ||
| if [[ -n ${domain_name} ]]; then | ||
| echo " Domain name : ${domain_name}" |
There was a problem hiding this comment.
This change to make domain_name valid without IPv4 could be moved into a separate PR, if anyone wishes so
| function validate_node_reward() { | ||
| read_variables | ||
| if [[ -z "$node_reward_type" ]]; then | ||
| log_and_halt_installation_on_error 1 "Configuration error: node_reward_type is not set" | ||
| fi | ||
|
|
||
| if [[ ! "$node_reward_type" =~ ^type[0-9]+(\.[0-9])?$ ]]; then | ||
| log_and_halt_installation_on_error 1 "Configuration error: node_reward_type is invalid: ${node_reward_type}" | ||
| fi | ||
|
|
||
| echo "Valid node reward type: ${node_reward_type}" | ||
| } |
There was a problem hiding this comment.
node_rewarde_type validation has already been moved directly into the config tool
|
Passed hourly and bare metal installation test! |
rs/ic_os/config/src/main.rs
Outdated
| if let Some(ref node_reward_type) = node_reward_type { | ||
| let node_reward_type_pattern = Regex::new(r"^type[0-9]+(\.[0-9])?$")?; | ||
| if !node_reward_type_pattern.is_match(node_reward_type) { | ||
| anyhow::bail!( | ||
| "Invalid node_reward_type '{}'. It must match the pattern ^type[0-9]+(\\.[0-9])?$", | ||
| node_reward_type | ||
| ); | ||
| } | ||
| } else { | ||
| println!("Node reward type is not set. Skipping validation."); | ||
| } |
There was a problem hiding this comment.
node_reward_type is now an optional config.ini field, hence this change (CC: @sasa-tomic)
| # Arguments: | ||
| # $1 - JSON path to the desired value (e.g., '.icos_settings.nns_urls') | ||
| # Note: If the key is not found, this function will return null. | ||
| function get_config_value() { |
There was a problem hiding this comment.
I remember already seeing a function for reading config values in a previous PR (but can't find it anymore), is there a difference?
There was a problem hiding this comment.
Unfortunately, yes, we need a config.sh file for SetupOS and another for HostOS/GuestOS. See the full configuration PR with both config.sh files: https://github.com/dfinity/ic/pull/1563/files#diff-8760e39edecd962b0efb7167e82feeaa695bcf45af62f0dc1fcdd01a35b058a0
The only difference is the path of the CONFIG_FILE. SetupOS's path is /var/ic/config/config.json because the config object is generated at runtime, and SetupOS boots as readonly, so its config object is written to var.
| log_and_halt_installation_on_error "1" "use_nns_public_key set to true but not found." | ||
| fi | ||
| else | ||
| log_and_halt_installation_on_error "1" "use_nns_public_key must be set to true." |
There was a problem hiding this comment.
Do I understand correctly that we always trigger this when use_nns_public_key is false? Why do we even allow to configure it if it must be true (just for backwards compatibility?)?
There was a problem hiding this comment.
Yes, currently, use_nns_public_key must be true for a successful install. The thought process was that later, when we introduce SEV, we will want to lock down using a custom NNS public key as a dev feature and hard-code the real key for production.
For context: https://dfinity.atlassian.net/browse/NODE-953
| generated_mac: &MacAddr6, | ||
| output_directory: &Path, | ||
| ) -> Result<()> { | ||
| eprintln!("Generating IPv6 address"); |
There was a problem hiding this comment.
When do we log to stderr and when do we log to stdout? E.g. above we log to stdout.
There was a problem hiding this comment.
Honestly, I don't know. Perhaps we should ticket this because I know we use one vs the other in multiple places and sometimes both in the same script for no discernible reason.
NODE-1360
The IC-OS tool has been created, but not yet used by the IC-OS: #1539
This PR integrates the config tool into SetupOS. The config tool is utilized for config sanitization, organization, and access.
Note that the old config is still being passed to HostOS, so this PR should have no impact on HostOS or GuestOS