[PATCH storage v2 7/7] zfsnvme: restore ACLs before publishing target

[Joaquin Varela <joaquinvarela@neatech.ar>]

Keep the subsystem unreachable while reconstructing derived configfs
state. Restore every configured Host NQN, its DH-HMAC-CHAP key and
namespace before linking the subsystem into any NVMe/TCP port.

This prevents reconnecting initiators from treating a transient host-not-
allowed response as a terminal controller failure after a target reboot.

Signed-off-by: Joaquin Varela <joaquinvarela@neatech.ar>
---
 src/PVE/Storage/LunCmd/NVMET.pm  | 47 +++++++++++++++++++++-----
 src/PVE/Storage/ZFSNVMePlugin.pm | 58 ++++++++++++++++++++++++++++++--
 src/test/zfsnvme_test.pm         | 40 +++++++++++++++++++++-
 3 files changed, 132 insertions(+), 13 deletions(-)

diff --git a/src/PVE/Storage/LunCmd/NVMET.pm b/src/PVE/Storage/LunCmd/NVMET.pm
index 7a902da..c5b9203 100644
--- a/src/PVE/Storage/LunCmd/NVMET.pm
+++ b/src/PVE/Storage/LunCmd/NVMET.pm
@@ -112,8 +112,7 @@ allocate_port_id() {
 }
 
 ensure_port() {
-    local nqn="$1"
-    local spec="$2"
+    local spec="$1"
     local family address service id port
 
     IFS=, read -r family address service <<<"$spec"
@@ -137,10 +136,6 @@ ensure_port() {
         printf '%s\n' "$service" >"$port/addr_trsvcid"
     fi
 
-    port="$ROOT/ports/$id"
-    if [[ ! -e "$port/subsystems/$nqn" ]]; then
-        ln -s "$ROOT/subsystems/$nqn" "$port/subsystems/$nqn"
-    fi
     printf '%s\n' "$id"
 }
 
@@ -148,7 +143,7 @@ ensure_target() {
     local nqn="$1"
     local pool="$2"
     shift 2
-    local spec id port desired_ports=''
+    local spec
 
     validate_nqn "$nqn"
     validate_pool "$pool"
@@ -156,7 +151,31 @@ ensure_target() {
     ensure_subsystem "$nqn"
 
     for spec in "$@"; do
-        id="$(ensure_port "$nqn" "$spec")"
+        ensure_port "$spec" >/dev/null
+    done
+}
+
+publish_target() {
+    local nqn="$1"
+    shift
+    local spec id port desired_ports=''
+
+    validate_nqn "$nqn"
+    (($# >= 1)) || die "at least one NVMe/TCP portal is required"
+    [[ -d "$ROOT/subsystems/$nqn" ]] || die "NVMe subsystem does not exist"
+
+    # The subsystem only becomes reachable after every namespace, host ACL and
+    # authentication key has been restored. Publishing it earlier makes an
+    # initiator treat a transient "host not allowed" response as permanent and
+    # remove the controller, failing queued I/O despite ctrl_loss_tmo.
+    for spec in "$@"; do
+        IFS=, read -r family address service <<<"$spec"
+        id="$(find_port "$family" "$address" "$service" || true)"
+        [[ -n "$id" ]] || die "NVMe/TCP portal '$spec' is not configured"
+        port="$ROOT/ports/$id"
+        if [[ ! -e "$port/subsystems/$nqn" ]]; then
+            ln -s "$ROOT/subsystems/$nqn" "$port/subsystems/$nqn"
+        fi
         desired_ports="$desired_ports $id"
     done
 
@@ -523,6 +542,7 @@ shift || true
 
 case "$mode" in
     ensure-target) ensure_target "$@" ;;
+    publish-target) publish_target "$@" ;;
     ensure-host) ensure_host "$@" ;;
     allow-host) allow_host "$@" ;;
     create) create_volume "$@" ;;
@@ -616,10 +636,15 @@ sub get_base($scfg) {
     return '/dev/zvol';
 }
 
-sub ensure_target($scfg, $hostnqn, $portals) {
+sub ensure_target($scfg, $portals) {
     my $nqn = $scfg->{subsysnqn};
 
     remote_call($scfg, 15, 'ensure-target', $nqn, $scfg->{pool}, $portals->@*);
+}
+
+sub ensure_host($scfg, $hostnqn) {
+    my $nqn = $scfg->{subsysnqn};
+
     remote_call($scfg, 10, 'ensure-host', $nqn, $hostnqn);
 }
 
@@ -657,6 +682,10 @@ sub reconcile($scfg) {
     remote_call($scfg, 30, 'reconcile', $scfg->{subsysnqn}, $scfg->{pool});
 }
 
+sub publish_target($scfg, $portals) {
+    remote_call($scfg, 15, 'publish-target', $scfg->{subsysnqn}, $portals->@*);
+}
+
 sub delete_target($scfg) {
     remote_call($scfg, 15, 'delete-target', $scfg->{subsysnqn});
 }
diff --git a/src/PVE/Storage/ZFSNVMePlugin.pm b/src/PVE/Storage/ZFSNVMePlugin.pm
index 54c406d..eedc67d 100644
--- a/src/PVE/Storage/ZFSNVMePlugin.pm
+++ b/src/PVE/Storage/ZFSNVMePlugin.pm
@@ -19,6 +19,7 @@ my $nvme = '/usr/sbin/nvme';
 my $secret_dir = '/etc/pve/priv/storage';
 my $runtime_dir = '/run/pve-storage';
 my $max_paths = 16;
+my $max_hosts = 64;
 
 my $RE_NQN = qr{
     \A
@@ -140,11 +141,41 @@ sub parse_nvme_host_ifaces($value, $noerr = undef) {
     return $result;
 }
 
+sub parse_nvme_host_nqns($value, $noerr = undef) {
+    my $result = [];
+    my $seen = {};
+
+    for my $hostnqn (split(/,/, $value // '')) {
+        $hostnqn = trim($hostnqn);
+        if (!verify_nvme_nqn($hostnqn, 1) || $seen->{$hostnqn}++) {
+            return undef if $noerr;
+            die "invalid or duplicate NVMe host NQN '$hostnqn'\n";
+        }
+        push $result->@*, $hostnqn;
+        if (scalar($result->@*) > $max_hosts) {
+            return undef if $noerr;
+            die "at most $max_hosts NVMe host NQNs are supported\n";
+        }
+    }
+
+    if (!$result->@*) {
+        return undef if $noerr;
+        die "at least one NVMe host NQN is required\n";
+    }
+
+    return $result;
+}
+
 my sub verify_nvme_host_ifaces($value, $noerr = undef) {
     return undef if !parse_nvme_host_ifaces($value, $noerr);
     return $value;
 }
 
+my sub verify_nvme_host_nqns($value, $noerr = undef) {
+    return undef if !parse_nvme_host_nqns($value, $noerr);
+    return $value;
+}
+
 sub _configured_portals($scfg) {
     my $portals = parse_nvme_portals($scfg->{'nvme-portals'});
     my $ifaces = parse_nvme_host_ifaces($scfg->{'nvme-host-ifaces'});
@@ -172,6 +203,7 @@ sub _validate_local_ifaces($portals) {
 PVE::JSONSchema::register_format('pve-storage-nvme-nqn', \&verify_nvme_nqn);
 PVE::JSONSchema::register_format('pve-storage-nvme-portals', \&verify_nvme_portals);
 PVE::JSONSchema::register_format('pve-storage-nvme-host-ifaces', \&verify_nvme_host_ifaces);
+PVE::JSONSchema::register_format('pve-storage-nvme-host-nqns', \&verify_nvme_host_nqns);
 
 sub type($class) {
     return 'zfsnvme';
@@ -205,6 +237,13 @@ sub properties($class) {
             format => 'pve-storage-nvme-host-ifaces',
             maxLength => 512,
         },
+        'nvme-host-nqns' => {
+            description =>
+                "Comma-separated /etc/nvme/hostnqn values for every cluster node allowed to use this storage.",
+            type => 'string',
+            format => 'pve-storage-nvme-host-nqns',
+            maxLength => 8192,
+        },
         'dhchap-key' => {
             description => "NVMe DH-HMAC-CHAP key in secret representation format.",
             type => 'string',
@@ -261,6 +300,7 @@ sub options($class) {
         subsysnqn => { fixed => 1 },
         'nvme-portals' => { fixed => 1 },
         'nvme-host-ifaces' => { optional => 1 },
+        'nvme-host-nqns' => { optional => 1 },
         pool => { fixed => 1 },
         blocksize => { fixed => 1 },
         sparse => { optional => 1 },
@@ -306,6 +346,8 @@ sub check_config($class, $section_id, $config, $create, $skip_schema_check) {
     } elsif (defined($config->{'nvme-host-ifaces'})) {
         parse_nvme_host_ifaces($config->{'nvme-host-ifaces'});
     }
+    parse_nvme_host_nqns($config->{'nvme-host-nqns'})
+        if defined($config->{'nvme-host-nqns'});
     _validate_fail_fast_timeout($config, $create ? 600 : undef);
     return $class->SUPER::check_config($section_id, $config, $create, $skip_schema_check);
 }
@@ -392,6 +434,7 @@ my sub delete_secret($storeid) {
 
 sub on_add_hook($class, $storeid, $scfg, %sensitive) {
     _configured_portals($scfg);
+    parse_nvme_host_nqns($scfg->{'nvme-host-nqns'});
     _assert_unique_target($storeid, $scfg);
     set_secret($storeid, $sensitive{'dhchap-key'});
     return;
@@ -402,6 +445,7 @@ sub on_update_hook_full($class, $storeid, $scfg, $update, $delete, $sensitive) {
     delete @prospective{$delete->@*} if $delete;
     verify_nvme_nqn($prospective{subsysnqn});
     _configured_portals(\%prospective);
+    parse_nvme_host_nqns($prospective{'nvme-host-nqns'});
     _validate_fail_fast_timeout(\%prospective, 600);
     _assert_unique_target($storeid, \%prospective);
 
@@ -667,15 +711,23 @@ sub activate_storage($class, $storeid, $scfg, $cache = undef) {
     my $hostid = file_read_firstline('/etc/nvme/hostid')
         // die "missing /etc/nvme/hostid\n";
     verify_nvme_nqn($hostnqn);
+    my $hostnqns = parse_nvme_host_nqns($scfg->{'nvme-host-nqns'});
+    my $local_host_is_allowed = grep { $_ eq $hostnqn } $hostnqns->@*;
+    die "local NVMe host NQN '$hostnqn' is missing from nvme-host-nqns\n"
+        if !$local_host_is_allowed;
     my $key = get_secret($storeid);
     my $target_portals = [
         map { "$_->{family},$_->{address},$_->{port}" } $portals->@*
     ];
 
-    PVE::Storage::LunCmd::NVMET::ensure_target($scfg, $hostnqn, $target_portals);
-    PVE::Storage::LunCmd::NVMET::set_host_key($scfg, $hostnqn, $key);
-    PVE::Storage::LunCmd::NVMET::allow_host($scfg, $hostnqn);
+    PVE::Storage::LunCmd::NVMET::ensure_target($scfg, $target_portals);
+    for my $allowed_hostnqn ($hostnqns->@*) {
+        PVE::Storage::LunCmd::NVMET::ensure_host($scfg, $allowed_hostnqn);
+        PVE::Storage::LunCmd::NVMET::set_host_key($scfg, $allowed_hostnqn, $key);
+        PVE::Storage::LunCmd::NVMET::allow_host($scfg, $allowed_hostnqn);
+    }
     PVE::Storage::LunCmd::NVMET::reconcile($scfg);
+    PVE::Storage::LunCmd::NVMET::publish_target($scfg, $target_portals);
 
     my $config_path =
         write_runtime_config($storeid, $scfg, $hostnqn, $hostid, $key, $portals);
diff --git a/src/test/zfsnvme_test.pm b/src/test/zfsnvme_test.pm
index d64b149..3d69ec7 100644
--- a/src/test/zfsnvme_test.pm
+++ b/src/test/zfsnvme_test.pm
@@ -26,6 +26,18 @@ ok(
     'rejects an invalid NQN',
 );
 
+my $hostnqn_a = 'nqn.2014-08.org.nvmexpress:uuid:12345678-1234-1234-1234-123456789abc';
+my $hostnqn_b = 'nqn.2014-08.org.nvmexpress:uuid:abcdefab-abcd-abcd-abcd-abcdefabcdef';
+is_deeply(
+    PVE::Storage::ZFSNVMePlugin::parse_nvme_host_nqns("$hostnqn_a,$hostnqn_b"),
+    [$hostnqn_a, $hostnqn_b],
+    'parses the complete cluster NVMe host allow-list',
+);
+ok(
+    !PVE::Storage::ZFSNVMePlugin::parse_nvme_host_nqns("$hostnqn_a,$hostnqn_a", 1),
+    'rejects duplicate NVMe host NQNs',
+);
+
 is_deeply(
     PVE::Storage::ZFSNVMePlugin::parse_nvme_portals(
         '10.90.1.11:4420,[fd00::11]:4421,10.90.2.11',
@@ -421,6 +433,7 @@ is($@, '', 'fast I/O fail remains valid with infinite controller reconnect');
             {
                 subsysnqn => 'nqn.2026-07.example:test',
                 'nvme-portals' => '10.90.1.11,10.90.2.11',
+                'nvme-host-nqns' => $hostnqn_a,
             },
             { 'nvme-host-ifaces' => 'ens20,ens21' },
             undef,
@@ -445,7 +458,6 @@ PVE::Storage::LunCmd::NVMET::ensure_target(
         subsysnqn => 'nqn.2026-07.example:test',
         pool => 'tank/pve-nvme',
     },
-    'nqn.2014-08.org.nvmexpress:uuid:12345678-1234-1234-1234-123456789abc',
     ['ipv4,10.90.1.11,4420'],
 );
 like(
@@ -453,6 +465,32 @@ like(
     qr/current_model.*current_serial.*refusing to take over existing NVMe subsystem/s,
     'target reconcile verifies model and deterministic serial before taking over a subsystem',
 );
+my ($ensure_port_body) = $helper_calls[0]->{input} =~ /^ensure_port\(\) \{\n(?<body>.*?)^\}/ms;
+ok(defined($ensure_port_body), 'remote helper contains the port preparation function');
+unlike(
+    $ensure_port_body,
+    qr{ln[ ]-s},
+    'target setup does not publish a subsystem before ACL and namespace reconciliation',
+);
+like(
+    $helper_calls[0]->{input},
+    qr{^publish_target\(\).*?ln[ ]-s}ms,
+    'the remote helper exposes the subsystem only in its publish operation',
+);
+
+@helper_calls = ();
+PVE::Storage::LunCmd::NVMET::publish_target(
+    {
+        server => '192.0.2.10',
+        subsysnqn => 'nqn.2026-07.example:test',
+    },
+    ['ipv4,10.90.1.11,4420'],
+);
+like(
+    join(' ', $helper_calls[0]->{cmd}->@*),
+    qr/publish-target/,
+    'publishing the fully reconciled target is an explicit final operation',
+);
 
 my ($key_cmd, %key_opts);
 $nvmet_mock->redefine(
-- 
2.54.0.windows.1