Skip to content

OCPEDGE-2934: feat: add macAddress as alternative identifier for fencing credentials #1916

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.

Sign up for GitHub

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

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

OCPEDGE-2934: feat: add macAddress as alternative identifier for fencing credentials #1916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions agent/05_agent_configure.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,9 @@ function configure_node() {
AGENT_NODES_IPSV6+=("$ipv6")
fi
AGENT_NODES_MACS+=("$node_mac")
if [[ "$node_type" == "master" ]]; then
AGENT_MASTER_MACS+=("$node_mac")

@lucaconsalvi lucaconsalvi Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: AGENT_MASTER_MACS is populated here in configure_node() (called by get_static_ips_and_macs()), but the alternative code path — get_baremetal_ips_and_macs() — never declares or populates this array.

Since generate_cluster_manifests() unconditionally serializes it ("${AGENT_MASTER_MACS[@]}"), and the script runs with set -euxo pipefail, this will crash with unbound variable on every NODES_PLATFORM=baremetal deployment — even with the default hostname mode.

Suggested fix — mirror the AGENT_MASTER_HOSTNAMES pattern in get_baremetal_ips_and_macs():

  • Add AGENT_MASTER_MACS=() to the initialization block
  • Add AGENT_MASTER_MACS+=("$mac") inside the if (( i < NUM_MASTERS )) block

Also worth adding AGENT_MASTER_MACS=() to the init block in get_static_ips_and_macs() to match the established pattern.

fi
if [[ ! -z "${BOND_PRIMARY_INTERFACE:-}" ]]; then
# For a bond, a random mac is added for the 2nd interface
AGENT_NODES_MACS+=("$(sudo virsh domiflist "${cluster_name}" | grep "${BAREMETAL_NETWORK_NAME}" | grep -v "${node_mac}" | awk '{print $5}')")
Expand All @@ -152,6 +155,7 @@ function get_static_ips_and_macs() {
AGENT_NODES_IPSV6=()
AGENT_NODES_MACS=()
AGENT_NODES_HOSTNAMES=()
AGENT_MASTER_MACS=()
AGENT_EXTRA_WORKERS_IPS=()
AGENT_EXTRA_WORKERS_IPSV6=()
AGENT_EXTRA_WORKERS_MACS=()
Expand Down Expand Up @@ -188,6 +192,7 @@ function get_baremetal_ips_and_macs() {
AGENT_NODES_MACS=()
AGENT_NODES_HOSTNAMES=()
AGENT_MASTER_HOSTNAMES=()
AGENT_MASTER_MACS=()
AGENT_EXTRA_WORKERS_IPS=()
AGENT_EXTRA_WORKERS_IPSV6=()
AGENT_EXTRA_WORKERS_MACS=()
Expand Down Expand Up @@ -219,6 +224,7 @@ function get_baremetal_ips_and_macs() {
AGENT_NODES_MACS+=("$mac")
AGENT_NODES_HOSTNAMES+=("$hostname")
AGENT_MASTER_HOSTNAMES+=("$hostname")
AGENT_MASTER_MACS+=("$mac")
else
AGENT_EXTRA_WORKERS_IPS+=("$ip")
AGENT_EXTRA_WORKERS_MACS+=("$mac")
Expand Down Expand Up @@ -361,6 +367,8 @@ function generate_cluster_manifests() {

master_hostnames=$(printf '%s,' "${AGENT_MASTER_HOSTNAMES[@]}")
export AGENT_MASTER_HOSTNAMES_STR=${master_hostnames::-1}
master_macs=$(printf '%s,' "${AGENT_MASTER_MACS[@]}")
export AGENT_MASTER_MACS_STR=${master_macs::-1}
master_bmc_usernames=$(printf '%s,' "${AGENT_MASTER_BMC_USERNAMES[@]}")
export AGENT_MASTER_BMC_USERNAMES_STR=${master_bmc_usernames::-1}
master_bmc_passwords=$(printf '%s,' "${AGENT_MASTER_BMC_PASSWORDS[@]}")
Expand Down
17 changes: 11 additions & 6 deletions agent/roles/manifests/templates/install-config_baremetal_yaml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,23 @@ controlPlane:
replicas: {{ num_masters }}
{% if enable_two_node_fencing %}
{% set master_hostnames = agent_master_hostnames.split(',') %}
{% set master_macs = agent_master_macs.split(',') %}
{% set master_bmc_addresses = agent_master_bmc_addresses.split(',') %}
{% set master_bmc_usernames = agent_master_bmc_usernames.split(',') %}
{% set master_bmc_passwords = agent_master_bmc_passwords.split(',') %}
{% set master_bmc_verify_cas = agent_master_bmc_verify_cas.split(',') %}
fencing:
credentials:
{% for hostname in master_hostnames %}
- hostname: {{hostname}}
address: {{ master_bmc_addresses[loop.index0] }}
username: {{ master_bmc_usernames[loop.index0] }}
password: {{ master_bmc_passwords[loop.index0] }}
certificateVerification: {{ 'Disabled' if bmc_verify_cas[loop.index0] == "False" else 'Enabled' }}
{% for i in range(master_hostnames | length) %}
{% if fencing_credential_identifier == "macAddress" %}
- macAddress: {{ master_macs[i] }}
{% else %}
- hostname: {{ master_hostnames[i] }}
{% endif %}
address: {{ master_bmc_addresses[i] }}
username: {{ master_bmc_usernames[i] }}
password: {{ master_bmc_passwords[i] }}
certificateVerification: {{ 'Disabled' if master_bmc_verify_cas[i] == "False" else 'Enabled' }}
{% endfor %}
{% endif %}
fips: {{ fips_mode }}
Expand Down
2 changes: 2 additions & 0 deletions agent/roles/manifests/vars/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ cluster_subnet_v4: "{{ lookup('env', 'CLUSTER_SUBNET_V4') }}"
cluster_subnet_v6: "{{ lookup('env', 'CLUSTER_SUBNET_V6') }}"
enable_local_registry: "{{ lookup('env', 'ENABLE_LOCAL_REGISTRY') != '' }}"
enable_two_node_fencing: "{{ lookup('env', 'ENABLE_TWO_NODE_FENCING', default='') == 'true' }}"
fencing_credential_identifier: "{{ lookup('env', 'FENCING_CREDENTIAL_IDENTIFIER', default='hostname') }}"
external_subnet_v4: "{{ lookup('env', 'EXTERNAL_SUBNET_V4') }}"
external_subnet_v6: "{{ lookup('env', 'EXTERNAL_SUBNET_V6') }}"
external_subnet_v4_prefixlen: "{{ lookup('env', 'EXTERNAL_SUBNET_V4') | ansible.utils.ipaddr('prefix') }}"
Expand Down Expand Up @@ -73,6 +74,7 @@ service_subnet_v4: "{{ lookup('env', 'SERVICE_SUBNET_V4') }}"
service_subnet_v6: "{{ lookup('env', 'SERVICE_SUBNET_V6') }}"
version: "{{ lookup('env', 'VERSION') }}"
agent_master_hostnames: "{{ lookup('env', 'AGENT_MASTER_HOSTNAMES_STR') }}"
agent_master_macs: "{{ lookup('env', 'AGENT_MASTER_MACS_STR') }}"
agent_master_bmc_addresses: "{{ lookup('env', 'AGENT_MASTER_BMC_ADDRESSES_STR') }}"
agent_master_bmc_passwords: "{{ lookup('env', 'AGENT_MASTER_BMC_PASSWORDS_STR') }}"
agent_master_bmc_usernames: "{{ lookup('env', 'AGENT_MASTER_BMC_USERNAMES_STR') }}"
Expand Down
8 changes: 8 additions & 0 deletions common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,14 @@ if [[ -z ${AGENT_E2E_TEST_SCENARIO:-} ]] && [[ ${NUM_ARBITERS} -eq 0 ]] && [[ ${
export ENABLE_TWO_NODE_FENCING="true"
fi

# Controls whether fencing credentials use "hostname" or "macAddress" to identify nodes
export FENCING_CREDENTIAL_IDENTIFIER=${FENCING_CREDENTIAL_IDENTIFIER:-hostname}

@lucaconsalvi lucaconsalvi Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: Any value other than the exact string macAddress silently falls through to hostname mode in both code paths. A typo like macaddress or mac would be silently ignored.

The BMC_DRIVER validation just below establishes a precedent for early validation — worth adding a similar guard:


if [[ "${FENCING_CREDENTIAL_IDENTIFIER}" != "hostname" ]] && [[ "${FENCING_CREDENTIAL_IDENTIFIER}" != "macAddress" ]]; then
printf "FENCING_CREDENTIAL_IDENTIFIER must be 'hostname' or 'macAddress', got '%s'\n" "${FENCING_CREDENTIAL_IDENTIFIER}"
exit 1
fi

# Only redfish BMC driver is supported for two node fencing
if [[ "${BMC_DRIVER}" != "redfish" ]] && [[ "${ENABLE_TWO_NODE_FENCING:-}" == "true" ]]; then
printf "Only redfish BMC driver is supported for Two Node Fencing deployments: BMC_DRIVER=%s, ENABLE_TWO_NODE_FENCING=%s" "${BMC_DRIVER}" "${ENABLE_TWO_NODE_FENCING}"
Expand Down
8 changes: 8 additions & 0 deletions config_example.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,14 @@ set -x
#export ARBITER_DISK=50
#export ARBITER_VCPU=2

# FENCING_CREDENTIAL_IDENTIFIER -
# Controls the identifier used in fencing credentials for Two Node Fencing.
# Set to "hostname" to identify nodes by hostname (default), or "macAddress"
# to identify nodes by their boot MAC address.
# Default: "hostname"
#
#export FENCING_CREDENTIAL_IDENTIFIER=hostname

# WORKER_HOSTNAME_FORMAT -
# Set a custom hostname format for workers. This is a format string that should
# include one %d field, which will be replaced with the number of the node.
Expand Down
26 changes: 18 additions & 8 deletions utils.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,19 +468,29 @@ function node_map_to_install_config_fencing_credentials() {
credentials:
EOF
for ((idx=0; idx < NUM_MASTERS ; idx++)); do
# shellcheck disable=SC2059
hostname="$(printf "$MASTER_HOSTNAME_FORMAT" ${idx})"
# IP V6 and DualStack will force FQDN hostname for the VMs, we need to update
# this here to correctly set the hostname for the fencing credentials.
if [[ $IP_STACK != 'v4' ]]; then
hostname="${hostname}.${CLUSTER_DOMAIN}"
fi
username=$(node_val ${idx} "driver_info.username")
password=$(node_val ${idx} "driver_info.password")
address=$(node_val ${idx} "driver_info.address")

if [[ "${FENCING_CREDENTIAL_IDENTIFIER}" == "macAddress" ]]; then
identifier_key="macAddress"
identifier_value=$(node_val ${idx} "ports[0].address")

@lucaconsalvi lucaconsalvi Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: node_val uses jq -r, which returns the literal string "null" with exit code 0 when a JSON path doesn't exist. If the node JSON lacks ports[0].address, the generated YAML would contain - macAddress: null — which YAML parsers interpret as a null value.

Consider adding a guard:

Also — the original code had a helpful comment explaining why the FQDN append is needed for IPv6/DualStack. That "why" was lost in the refactor. Worth restoring above the if [[ $IP_STACK != 'v4' ]] check.

if [[ "${identifier_value}" == "null" || -z "${identifier_value}" ]]; then
printf "ports[0].address not found for node %d in %s\n" "${idx}" "${NODES_FILE}" >&2
exit 1
fi
else
# shellcheck disable=SC2059
identifier_value="$(printf "$MASTER_HOSTNAME_FORMAT" ${idx})"
# IPv6 and DualStack force FQDN hostnames for VMs
if [[ $IP_STACK != 'v4' ]]; then
identifier_value="${identifier_value}.${CLUSTER_DOMAIN}"
fi
identifier_key="hostname"
fi

cat <<EOF
- hostname: ${hostname}
- ${identifier_key}: ${identifier_value}
address: ${address}
username: ${username}
password: ${password}
Expand Down