Re: [libvirt] [PATCH] storage: escape ipv6 for ceph mon hosts to librados
Hosts for rbd are ceph monitor daemons. These have fixed IP addresses, so they are often referenced by IP rather than hostname for convenience, or to avoid relying on DNS. Using IPv4 addresses as the host name works already, but IPv6 addresses require rbd-specific
If you include the escaping in the XML, does it currently work? <host name='[2205::192:168:205:141]' port='6789'/>
Yes, It works.
escaping because the colon is used as an option separator in the string passed to librados.
Escape these colons, and enclose the IPv6 address in square brackets so it is distinguished from the port, which is currently mandatory.
Signed-off-by: Yi Li <yili@winhong.com> --- docs/schemas/storagepool.rng | 5 ++++- src/storage/storage_backend_rbd.c | 13 ++++++++++--- tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml | 13 +++++++++++++ tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml | 16 ++++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 5 files changed, 44 insertions(+), 4 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index f8c968e..3056563 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -268,9 +268,16 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, source->hosts[i].name); } else if (source->hosts[i].name != NULL && source->hosts[i].port) { - virBufferAsprintf(&mon_host, "%s:%d,", - source->hosts[i].name, - source->hosts[i].port); + /* assume host containing : is ipv6 */ + if (strchr(source->hosts[i].name, ':')) {
if (virSocketAddrNumericFamily(listenAddress) == AF_INET6)
By using this helper function, we won't try to escape an address that is already escaped.
Also, instead of copying the whole virBuffer call twice, it would be nicer to assign the format to a temporary variable like we do in qemuMigrationDstPrepare
Jano
Good point. I'd sending a v2.
+ virBufferAsprintf(&mon_host, "[%s]:%d,", + source->hosts[i].name, + source->hosts[i].port); + } else { + virBufferAsprintf(&mon_host, "%s:%d,", + source->hosts[i].name, + source->hosts[i].port); + } } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("received malformed monitor, check the XML definition"));
On Sun, Apr 28, 2019 at 1:10 PM winhong-yili <yili@winhong.com> wrote:
Hosts for rbd are ceph monitor daemons. These have fixed IP addresses, so they are often referenced by IP rather than hostname for convenience, or to avoid relying on DNS. Using IPv4 addresses as the host name works already, but IPv6 addresses require rbd-specific
If you include the escaping in the XML, does it currently work? <host name='[2205::192:168:205:141]' port='6789'/>
escaping because the colon is used as an option separator in the string passed to librados.
Escape these colons, and enclose the IPv6 address in square brackets so it is distinguished from the port, which is currently mandatory.
Signed-off-by: Yi Li <yili@winhong.com> --- docs/schemas/storagepool.rng | 5 ++++- src/storage/storage_backend_rbd.c | 13 ++++++++++--- tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml | 13 +++++++++++++ tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml | 16 ++++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 5 files changed, 44 insertions(+), 4 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index f8c968e..3056563 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -268,9 +268,16 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, source->hosts[i].name); } else if (source->hosts[i].name != NULL && source->hosts[i].port) { - virBufferAsprintf(&mon_host, "%s:%d,", - source->hosts[i].name, - source->hosts[i].port); + /* assume host containing : is ipv6 */ + if (strchr(source->hosts[i].name, ':')) {
if (virSocketAddrNumericFamily(listenAddress) == AF_INET6)
By using this helper function, we won't try to escape an address that is already escaped.
Also, instead of copying the whole virBuffer call twice, it would be nicer to assign the format to a temporary variable like we do in qemuMigrationDstPrepare
Jano
+ virBufferAsprintf(&mon_host, "[%s]:%d,", + source->hosts[i].name, + source->hosts[i].port); + } else { + virBufferAsprintf(&mon_host, "%s:%d,", + source->hosts[i].name, + source->hosts[i].port); + } } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("received malformed monitor, check the XML definition"));
From 4af765cb6ee87eb7a131901057a8b6d0e859ac63 Mon Sep 17 00:00:00 2001 From: Yi Li <yili@winhong.com> Date: Sun, 28 Apr 2019 10:29:53 +0800 Subject: [PATCH v2] storage: escape ipv6 for ceph mon hosts to librados
Hosts for rbd are ceph monitor daemons. These have fixed IP addresses, so they are often referenced by IP rather than hostname for convenience, or to avoid relying on DNS. Using IPv4 addresses as the host name works already, but IPv6 addresses require rbd-specific escaping because the colon is used as an option separator in the string passed to librados. Escape these colons, and enclose the IPv6 address in square brackets so it is distinguished from the port, which is currently mandatory. Signed-off-by: Yi Li <yili@winhong.com> --- docs/schemas/storagepool.rng | 5 ++++- src/storage/storage_backend_rbd.c | 15 ++++++++++++--- tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml | 13 +++++++++++++ tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml | 16 ++++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 5 files changed, 46 insertions(+), 4 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 3ca8e79..976a02b 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -305,7 +305,10 @@ <oneOrMore> <element name='host'> <attribute name='name'> - <text/> + <choice> + <ref name="dnsName"/> + <ref name="ipAddr"/> + </choice> </attribute> <optional> <attribute name='port'> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index f8c968e..d6d7356 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -261,6 +261,7 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, VIR_DEBUG("Found %zu RADOS cluster monitors in the pool configuration", source->nhost); + /* combine host and port into portal */ for (i = 0; i < source->nhost; i++) { if (source->hosts[i].name != NULL && !source->hosts[i].port) { @@ -268,9 +269,17 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, source->hosts[i].name); } else if (source->hosts[i].name != NULL && source->hosts[i].port) { - virBufferAsprintf(&mon_host, "%s:%d,", - source->hosts[i].name, - source->hosts[i].port); + const char *incFormat; + if (virSocketAddrNumericFamily(source->hosts[i].name) == AF_INET6) { + /* IPv6 address must be escaped in brackets on the cmd line */ + incFormat = "[%s]:%d"; + } else { + /* listenAddress is a hostname or IPv4 */ + incFormat = "%s:%d"; + } + virBufferAsprintf(&mon_host, incFormat, + source->hosts[i].name, + source->hosts[i].port); } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("received malformed monitor, check the XML definition")); diff --git a/tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml b/tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml new file mode 100644 index 0000000..0744b33 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml @@ -0,0 +1,13 @@ +<pool type='rbd'> + <name>ceph</name> + <uuid>47c1faee-0207-e741-f5ae-d9b019b98fe2</uuid> + <source> + <name>rbd</name> + <host name='localhost' port='6789'/> + <host name='localhost' port='6790'/> + <host name='2205::192:168:205:141' port='6789'/> + <auth username='admin' type='ceph'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> + </source> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml b/tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml new file mode 100644 index 0000000..cc2a379 --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml @@ -0,0 +1,16 @@ +<pool type='rbd'> + <name>ceph</name> + <uuid>47c1faee-0207-e741-f5ae-d9b019b98fe2</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <host name='localhost' port='6789'/> + <host name='localhost' port='6790'/> + <host name='2205::192:168:205:141' port='6789'/> + <name>rbd</name> + <auth type='ceph' username='admin'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> + </source> +</pool> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 2ae514f..b6f4cb4 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -95,6 +95,7 @@ mymain(void) DO_TEST("pool-zfs-sourcedev"); DO_TEST("pool-rbd"); #ifdef WITH_STORAGE_RBD + DO_TEST("pool-rbd-ipv6"); DO_TEST("pool-rbd-refresh-volume-allocation"); DO_TEST("pool-rbd-ns-configopts"); #endif -- 2.7.5 On Sun, Apr 28, 2019 at 1:21 PM Yi Li <yiliworking@gmail.com> wrote:
Hosts for rbd are ceph monitor daemons. These have fixed IP addresses, so they are often referenced by IP rather than hostname for convenience, or to avoid relying on DNS. Using IPv4 addresses as the host name works already, but IPv6 addresses require rbd-specific
If you include the escaping in the XML, does it currently work? <host name='[2205::192:168:205:141]' port='6789'/>
Yes, It works.
escaping because the colon is used as an option separator in the string passed to librados.
Escape these colons, and enclose the IPv6 address in square brackets so it is distinguished from the port, which is currently mandatory.
Signed-off-by: Yi Li <yili@winhong.com> --- docs/schemas/storagepool.rng | 5 ++++- src/storage/storage_backend_rbd.c | 13 ++++++++++--- tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml | 13 +++++++++++++ tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml | 16 ++++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 5 files changed, 44 insertions(+), 4 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index f8c968e..3056563 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -268,9 +268,16 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, source->hosts[i].name); } else if (source->hosts[i].name != NULL && source->hosts[i].port) { - virBufferAsprintf(&mon_host, "%s:%d,", - source->hosts[i].name, - source->hosts[i].port); + /* assume host containing : is ipv6 */ + if (strchr(source->hosts[i].name, ':')) {
if (virSocketAddrNumericFamily(listenAddress) == AF_INET6)
By using this helper function, we won't try to escape an address that is already escaped.
Also, instead of copying the whole virBuffer call twice, it would be nicer to assign the format to a temporary variable like we do in qemuMigrationDstPrepare
Jano
Good point. I'd sending a v2.
+ virBufferAsprintf(&mon_host, "[%s]:%d,", + source->hosts[i].name, + source->hosts[i].port); + } else { + virBufferAsprintf(&mon_host, "%s:%d,", + source->hosts[i].name, + source->hosts[i].port); + } } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("received malformed monitor, check the XML definition"));
On Sun, Apr 28, 2019 at 1:10 PM winhong-yili <yili@winhong.com> wrote:
Hosts for rbd are ceph monitor daemons. These have fixed IP addresses, so they are often referenced by IP rather than hostname for convenience, or to avoid relying on DNS. Using IPv4 addresses as the host name works already, but IPv6 addresses require rbd-specific
If you include the escaping in the XML, does it currently work? <host name='[2205::192:168:205:141]' port='6789'/>
escaping because the colon is used as an option separator in the string passed to librados.
Escape these colons, and enclose the IPv6 address in square brackets so it is distinguished from the port, which is currently mandatory.
Signed-off-by: Yi Li <yili@winhong.com> --- docs/schemas/storagepool.rng | 5 ++++- src/storage/storage_backend_rbd.c | 13 ++++++++++--- tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml | 13 +++++++++++++ tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml | 16 ++++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 5 files changed, 44 insertions(+), 4 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index f8c968e..3056563 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -268,9 +268,16 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, source->hosts[i].name); } else if (source->hosts[i].name != NULL && source->hosts[i].port) { - virBufferAsprintf(&mon_host, "%s:%d,", - source->hosts[i].name, - source->hosts[i].port); + /* assume host containing : is ipv6 */ + if (strchr(source->hosts[i].name, ':')) {
if (virSocketAddrNumericFamily(listenAddress) == AF_INET6)
By using this helper function, we won't try to escape an address that is already escaped.
Also, instead of copying the whole virBuffer call twice, it would be nicer to assign the format to a temporary variable like we do in qemuMigrationDstPrepare
Jano
+ virBufferAsprintf(&mon_host, "[%s]:%d,", + source->hosts[i].name, + source->hosts[i].port); + } else { + virBufferAsprintf(&mon_host, "%s:%d,", + source->hosts[i].name, + source->hosts[i].port); + } } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("received malformed monitor, check the XML definition"));
Oops, I forgot about this patch. On Sun, Apr 28, 2019 at 01:25:05PM +0800, Yi Li wrote:
From 4af765cb6ee87eb7a131901057a8b6d0e859ac63 Mon Sep 17 00:00:00 2001 From: Yi Li <yili@winhong.com> Date: Sun, 28 Apr 2019 10:29:53 +0800 Subject: [PATCH v2] storage: escape ipv6 for ceph mon hosts to librados
Hosts for rbd are ceph monitor daemons. These have fixed IP addresses, so they are often referenced by IP rather than hostname for convenience, or to avoid relying on DNS. Using IPv4 addresses as the host name works already, but IPv6 addresses require rbd-specific escaping because the colon is used as an option separator in the string passed to librados.
Escape these colons, and enclose the IPv6 address in square brackets so it is distinguished from the port, which is currently mandatory.
Signed-off-by: Yi Li <yili@winhong.com> --- docs/schemas/storagepool.rng | 5 ++++- src/storage/storage_backend_rbd.c | 15 ++++++++++++--- tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml | 13 +++++++++++++ tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml | 16 ++++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 5 files changed, 46 insertions(+), 4 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-ipv6.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-ipv6.xml
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index f8c968e..d6d7356 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -261,6 +261,7 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, VIR_DEBUG("Found %zu RADOS cluster monitors in the pool configuration", source->nhost);
+ /* combine host and port into portal */ for (i = 0; i < source->nhost; i++) { if (source->hosts[i].name != NULL && !source->hosts[i].port) { @@ -268,9 +269,17 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, source->hosts[i].name); } else if (source->hosts[i].name != NULL && source->hosts[i].port) { - virBufferAsprintf(&mon_host, "%s:%d,", - source->hosts[i].name, - source->hosts[i].port); + const char *incFormat; + if (virSocketAddrNumericFamily(source->hosts[i].name) == AF_INET6) { + /* IPv6 address must be escaped in brackets on the cmd line */ + incFormat = "[%s]:%d"; + } else { + /* listenAddress is a hostname or IPv4 */ + incFormat = "%s:%d"; + } + virBufferAsprintf(&mon_host, incFormat, + source->hosts[i].name, + source->hosts[i].port);
The indentation here is off.
} else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("received malformed monitor, check the XML definition"));
Reviewed-by: Ján Tomko <jtomko@redhat.com> and finally pushed. Jano
participants (2)
-
Ján Tomko -
Yi Li