Open slurmdbd.conf to full customization; Enable slurmdbd purge by default#214
Open slurmdbd.conf to full customization; Enable slurmdbd purge by default#214elelaysh wants to merge 3 commits into
Conversation
Also create the ohpc_nodegroups_computed variable, to share computed values per nodegroup between multiple places of the role. If a nodegroup is in ohpc_nodegroups_computed it has at least one host. It stores - first_host: name of the first host in the nodegroup; use `hostvars[computed.first_host]` to access its hostvars - inventory_group_name: name of the inventory group for this nodegroup; use `groups[computed.inventory_group_name]` to access the host list - ram_mb: memory per node in the nodegroup - def_mem_per_cpu: computed DefMemPerCPU for this nodegroup as the RAM/vCPU ratio.
- convert hardcoded values to defaults - introduce openhpc_slurmdbd_config to let appliances override it - generic templating to convert boolean to yes/no and rest as string (quoting strings is not necessary)
Set it to 2 years. Purges will run on the first day of each month
There was a problem hiding this comment.
Code Review
This pull request refactors the generation of slurmdbd.conf to be fully customizable through Ansible variables and centralizes the logic for computing node properties for slurm.conf. It also enables database purging by default. However, the full customization of slurmdbd.conf introduces a configuration injection vulnerability because keys and values are not validated or escaped for newlines, and proper quoting for values containing special characters is missing. Furthermore, a potential issue exists with how a complex variable is constructed, which could lead to errors, and there's a minor issue with a missing newline in a template.
| ohpc_nodegroups_computed: > | ||
| { | ||
| {% for nodegroup in openhpc_nodegroups %} | ||
| {% set inventory_group_name = openhpc_cluster_name ~ '_' ~ nodegroup.name %} | ||
| {% set inventory_group_hosts = groups.get(inventory_group_name, []) %} | ||
| {% if inventory_group_hosts | length > 0 %} | ||
| {% set play_group_hosts = inventory_group_hosts | intersect (play_hosts) %} | ||
| {% set first_host = play_group_hosts | first | mandatory('Inventory group "' ~ inventory_group_name ~ '" contains no hosts in this play - was --limit used?') %} | ||
| {% set first_host_hv = hostvars[first_host] %} | ||
| {% set ram_mb = (first_host_hv['ansible_memory_mb']['real']['total'] * (nodegroup.ram_multiplier | default(openhpc_ram_multiplier))) | int %} | ||
| {{ nodegroup.name | to_json }}: { | ||
| "inventory_group_name": {{ inventory_group_name | to_json }}, | ||
| "first_host": {{ first_host | to_json }}, | ||
| "ram_mb": {{ ram_mb }}, | ||
| "def_mem_per_cpu": {{ (ram_mb / first_host_hv['ansible_processor_vcpus']) | int }}, | ||
| }, | ||
| {% endif %} | ||
| {% endfor %} | ||
| } |
There was a problem hiding this comment.
The current implementation for constructing ohpc_nodegroups_computed builds a string that is supposed to be a YAML/JSON dictionary. This is fragile and can fail due to a trailing comma if the last item in the loop meets the condition, which would result in invalid YAML. A more robust approach is to build a list of dictionary-item strings and then join them. This avoids manual string formatting and ensures valid output without a trailing comma.
ohpc_nodegroups_computed: >
{
{% set items = [] %}
{% for nodegroup in openhpc_nodegroups %}
{% set inventory_group_name = openhpc_cluster_name ~ '_' ~ nodegroup.name %}
{% set inventory_group_hosts = groups.get(inventory_group_name, []) %}
{% if inventory_group_hosts | length > 0 %}
{% set play_group_hosts = inventory_group_hosts | intersect (play_hosts) %}
{% set first_host = play_group_hosts | first | mandatory('Inventory group "' ~ inventory_group_name ~ '" contains no hosts in this play - was --limit used?') %}
{% set first_host_hv = hostvars[first_host] %}
{% set ram_mb = (first_host_hv['ansible_memory_mb']['real']['total'] * (nodegroup.ram_multiplier | default(openhpc_ram_multiplier))) | int %}
{% capture item_str %}
{{ nodegroup.name | to_json }}: {
"inventory_group_name": {{ inventory_group_name | to_json }},
"first_host": {{ first_host | to_json }},
"ram_mb": {{ ram_mb }},
"def_mem_per_cpu": {{ (ram_mb / first_host_hv['ansible_processor_vcpus']) | int }}
}
{% endcapture %}
{% set _ = items.append(item_str) %}
{% endif %}
{% endfor %}
{{ items | join(',\n') }}
}| {% for key, val in openhpc_slurmdbd_merged_config.items() | sort %} | ||
| {% if val is not none and val != 'omit' %} | ||
| {{ key }}={{ 'yes' if val is sameas true else ('no' if val is sameas false else val|string) }} |
There was a problem hiding this comment.
The loop used to generate slurmdbd.conf is vulnerable to configuration injection. If any of the keys or values in openhpc_slurmdbd_merged_config contain newline characters, an attacker who can control these variables can inject arbitrary configuration directives into the slurmdbd.conf file. This could lead to unauthorized configuration changes, such as modifying authentication settings or log file locations. Additionally, values containing special characters like # or spaces are not properly quoted, which can lead to parsing errors or truncated values (e.g., in passwords).
| {% if val is not none and val != 'omit' %} | ||
| {{ key }}={{ 'yes' if val is sameas true else ('no' if val is sameas false else val|string) }} | ||
| {% endif %} | ||
| {% endfor %} No newline at end of file |
Builds on #209