[libvirt] [PATCH v5 0/5] Support CHAP authentication for iscsi pool
This is a reworking and reposting of the authentication patches originally posted as part of my v3 reworking of Osier's original patches, see: https://www.redhat.com/archives/libvir-list/2013-July/msg00894.html Patch 7/7 was noted to be incorrect since the authentication was in the wrong place in the backend driver, see v3: https://www.redhat.com/archives/libvir-list/2013-July/msg00901.html and followups from my v2 posting: https://www.redhat.com/archives/libvir-list/2013-July/msg00910.html NOTE: Patch 4 (https://www.redhat.com/archives/libvir-list/2013-July/msg00960.html) was scrapped. Adjustments from previous patches 1. Using the same logic as Direct iSCSI, add the pool authentication info from the iSCSI pool to virDomainDiskDef's during qemuTranslateDiskSourcePool 2. Refactor the secret fetching code into one function for easier use 3. Refactor the volume building code into one function for easier use. This is where the magic to add the auth information to the disk occurs and now that it's part of the disk definition from the pool it will just 'be there' 4. Restore original intent to use startPool callback in order to authenticate the pool. Added a check for NULL connection. This just stops autostarting pools from getting authentication. This still needs to be fixed/resolved, but there just wasn't time in this release for that. 5. Split the rbd authentication code into it's own patch rather than being part of the iscsi patch 4. John Ferlan (5): qemu: Add source pool auth info to virDomainDiskDef for iSCSI qemu: Create a common qemuGetSecretString qemu_common: Create qemuBuildVolumeString() to process storage pool storage: Support "chap" authentication for iscsi pool Adjust 'ceph' authentication secret usage for rbd pool. src/qemu/qemu_command.c | 265 ++++++++++++++++++++---------------- src/qemu/qemu_conf.c | 55 ++++++++ src/storage/storage_backend_iscsi.c | 111 ++++++++++++++- src/storage/storage_backend_rbd.c | 21 ++- 4 files changed, 329 insertions(+), 123 deletions(-) -- 1.8.1.4
During qemuTranslateDiskSourcePool() execution, if the srcpool has been defined with authentication information, then for iSCSI pools copy the authentication and host information to virDomainDiskDef. --- src/qemu/qemu_conf.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 6e6163f..3e7b78a 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1186,6 +1186,58 @@ cleanup: return ret; } +static int +qemuTranslateDiskSourcePoolAuth(virDomainDiskDefPtr def, + virStoragePoolDefPtr pooldef) +{ + int ret = -1; + + /* Only necessary when authentication set */ + if (pooldef->source.authType == VIR_STORAGE_POOL_AUTH_NONE) { + ret = 0; + goto cleanup; + } + + /* Copy the authentication information from the storage pool + * into the virDomainDiskDef + */ + if (pooldef->source.authType == VIR_STORAGE_POOL_AUTH_CHAP) { + if (VIR_STRDUP(def->auth.username, + pooldef->source.auth.chap.username) < 0) + goto cleanup; + if (pooldef->source.auth.chap.secret.uuidUsable) { + def->auth.secretType = VIR_DOMAIN_DISK_SECRET_TYPE_UUID; + memcpy(def->auth.secret.uuid, + pooldef->source.auth.chap.secret.uuid, + VIR_UUID_BUFLEN); + } else { + if (VIR_STRDUP(def->auth.secret.usage, + pooldef->source.auth.chap.secret.usage) < 0) + goto cleanup; + def->auth.secretType = VIR_DOMAIN_DISK_SECRET_TYPE_USAGE; + } + } else if (pooldef->source.authType == VIR_STORAGE_POOL_AUTH_CEPHX) { + if (VIR_STRDUP(def->auth.username, + pooldef->source.auth.cephx.username) < 0) + goto cleanup; + if (pooldef->source.auth.cephx.secret.uuidUsable) { + def->auth.secretType = VIR_DOMAIN_DISK_SECRET_TYPE_UUID; + memcpy(def->auth.secret.uuid, + pooldef->source.auth.cephx.secret.uuid, + VIR_UUID_BUFLEN); + } else { + if (VIR_STRDUP(def->auth.secret.usage, + pooldef->source.auth.cephx.secret.usage) < 0) + goto cleanup; + def->auth.secretType = VIR_DOMAIN_DISK_SECRET_TYPE_USAGE; + } + } + ret = 0; + +cleanup: + return ret; +} + int qemuTranslateDiskSourcePool(virConnectPtr conn, virDomainDiskDefPtr def) @@ -1254,6 +1306,9 @@ qemuTranslateDiskSourcePool(virConnectPtr conn, if (!(def->src = virStorageVolGetPath(vol))) goto cleanup; } + + if (qemuTranslateDiskSourcePoolAuth(def, pooldef) < 0) + goto cleanup; } else { if (!(def->src = virStorageVolGetPath(vol))) goto cleanup; -- 1.8.1.4
On 07/22/2013 10:31 PM, John Ferlan wrote:
During qemuTranslateDiskSourcePool() execution, if the srcpool has been defined with authentication information, then for iSCSI pools copy the authentication and host information to virDomainDiskDef. --- src/qemu/qemu_conf.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
ACK Jan
Make the secret fetching code common for qemuBuildRBDString() and qemuBuildDriveURIString() using the virDomainDiskDef. --- src/qemu/qemu_command.c | 157 +++++++++++++++++++++++++----------------------- 1 file changed, 81 insertions(+), 76 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4a49d81..5bd8e87 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2457,6 +2457,61 @@ qemuSafeSerialParamValue(const char *value) return 0; } +static char * +qemuGetSecretString(virConnectPtr conn, + const char *scheme, + bool encoded, + int diskSecretType, + char *username, + unsigned char *uuid, char *usage, + virSecretUsageType secretUsageType) +{ + size_t secret_size; + virSecretPtr sec = NULL; + char *secret = NULL; + + /* look up secret */ + switch (diskSecretType) { + case VIR_DOMAIN_DISK_SECRET_TYPE_UUID: + sec = virSecretLookupByUUID(conn, uuid); + break; + case VIR_DOMAIN_DISK_SECRET_TYPE_USAGE: + sec = virSecretLookupByUsage(conn, secretUsageType, usage); + break; + } + + if (!sec) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s username '%s' specified but secret not found"), + scheme, username); + goto cleanup; + } + + secret = (char *)conn->secretDriver->secretGetValue(sec, &secret_size, 0, + VIR_SECRET_GET_VALUE_INTERNAL_CALL); + if (!secret) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("could not get value of the secret for username %s"), + username); + goto cleanup; + } + + if (encoded) { + char *base64 = NULL; + + base64_encode_alloc(secret, secret_size, &base64); + VIR_FREE(secret); + if (!base64) { + virReportOOMError(); + goto cleanup; + } + secret = base64; + } + +cleanup: + virObjectUnref(sec); + return secret; +} static int qemuBuildRBDString(virConnectPtr conn, @@ -2465,9 +2520,7 @@ qemuBuildRBDString(virConnectPtr conn, { size_t i; int ret = 0; - virSecretPtr sec = NULL; char *secret = NULL; - size_t secret_size; if (strchr(disk->src, ':')) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -2478,47 +2531,23 @@ qemuBuildRBDString(virConnectPtr conn, virBufferEscape(opt, ',', ",", "rbd:%s", disk->src); if (disk->auth.username) { + virBufferEscape(opt, '\\', ":", ":id=%s", disk->auth.username); - /* look up secret */ - switch (disk->auth.secretType) { - case VIR_DOMAIN_DISK_SECRET_TYPE_UUID: - sec = virSecretLookupByUUID(conn, - disk->auth.secret.uuid); - break; - case VIR_DOMAIN_DISK_SECRET_TYPE_USAGE: - sec = virSecretLookupByUsage(conn, - VIR_SECRET_USAGE_TYPE_CEPH, - disk->auth.secret.usage); - break; - } + /* Get the secret string using the virDomainDiskDef + * NOTE: qemu/librbd wants it base64 encoded + */ + if (!(secret = qemuGetSecretString(conn, "rbd", true, + disk->auth.secretType, + disk->auth.username, + disk->auth.secret.uuid, + disk->auth.secret.usage, + VIR_SECRET_USAGE_TYPE_CEPH))) + goto error; - if (sec) { - char *base64 = NULL; - secret = (char *)conn->secretDriver->secretGetValue(sec, &secret_size, 0, - VIR_SECRET_GET_VALUE_INTERNAL_CALL); - if (secret == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("could not get the value of the secret for username %s"), - disk->auth.username); - goto error; - } - /* qemu/librbd wants it base64 encoded */ - base64_encode_alloc(secret, secret_size, &base64); - if (!base64) { - virReportOOMError(); - goto error; - } - virBufferEscape(opt, '\\', ":", - ":key=%s:auth_supported=cephx\\;none", - base64); - VIR_FREE(base64); - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s username '%s' specified but secret not found"), - "rbd", disk->auth.username); - goto error; - } + virBufferEscape(opt, '\\', ":", + ":key=%s:auth_supported=cephx\\;none", + secret); } else { virBufferAddLit(opt, ":auth_supported=none"); } @@ -2544,7 +2573,6 @@ qemuBuildRBDString(virConnectPtr conn, cleanup: VIR_FREE(secret); - virObjectUnref(sec); return ret; @@ -2863,13 +2891,11 @@ error: static int qemuBuildDriveURIString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr opt, - const char *scheme, virSecretUsageType secretType) + const char *scheme, virSecretUsageType secretUsageType) { int ret = -1; int port = 0; - virSecretPtr sec = NULL; char *secret = NULL; - size_t secret_size; char *tmpscheme = NULL; char *volimg = NULL; @@ -2909,39 +2935,19 @@ qemuBuildDriveURIString(virConnectPtr conn, virAsprintf(&sock, "socket=%s", disk->hosts->socket) < 0) goto cleanup; - if (disk->auth.username && secretType != VIR_SECRET_USAGE_TYPE_NONE) { - /* look up secret */ - switch (disk->auth.secretType) { - case VIR_DOMAIN_DISK_SECRET_TYPE_UUID: - sec = virSecretLookupByUUID(conn, - disk->auth.secret.uuid); - break; - case VIR_DOMAIN_DISK_SECRET_TYPE_USAGE: - sec = virSecretLookupByUsage(conn, secretType, - disk->auth.secret.usage); - break; - } - - if (sec) { - secret = (char *)conn->secretDriver->secretGetValue(sec, &secret_size, 0, - VIR_SECRET_GET_VALUE_INTERNAL_CALL); - if (secret == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("could not get the value of the secret for username %s"), - disk->auth.username); - ret = -1; - goto cleanup; - } - if (virAsprintf(&user, "%s:%s", disk->auth.username, secret) < 0) - goto cleanup; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s username '%s' specified but secret not found"), - scheme, disk->auth.username); - ret = -1; + if (disk->auth.username && secretUsageType != VIR_SECRET_USAGE_TYPE_NONE) { + /* Get the secret string using the virDomainDiskDef */ + if (!(secret = qemuGetSecretString(conn, scheme, false, + disk->auth.secretType, + disk->auth.username, + disk->auth.secret.uuid, + disk->auth.secret.usage, + secretUsageType))) + goto cleanup; + if (virAsprintf(&user, "%s:%s", disk->auth.username, secret) < 0) goto cleanup; - } } + uri.scheme = tmpscheme; /* gluster+<transport> */ uri.server = disk->hosts->name; uri.user = user; @@ -2959,7 +2965,6 @@ cleanup: VIR_FREE(tmpscheme); VIR_FREE(volimg); VIR_FREE(sock); - virObjectUnref(sec); VIR_FREE(secret); VIR_FREE(user); -- 1.8.1.4
On 07/22/2013 10:31 PM, John Ferlan wrote:
Make the secret fetching code common for qemuBuildRBDString() and qemuBuildDriveURIString() using the virDomainDiskDef. --- src/qemu/qemu_command.c | 157 +++++++++++++++++++++++++----------------------- 1 file changed, 81 insertions(+), 76 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4a49d81..5bd8e87 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2478,47 +2531,23 @@ qemuBuildRBDString(virConnectPtr conn,
virBufferEscape(opt, ',', ",", "rbd:%s", disk->src); if (disk->auth.username) { + virBufferEscape(opt, '\\', ":", ":id=%s", disk->auth.username); - /* look up secret */ - switch (disk->auth.secretType) { - case VIR_DOMAIN_DISK_SECRET_TYPE_UUID: - sec = virSecretLookupByUUID(conn, - disk->auth.secret.uuid); - break; - case VIR_DOMAIN_DISK_SECRET_TYPE_USAGE: - sec = virSecretLookupByUsage(conn, - VIR_SECRET_USAGE_TYPE_CEPH, - disk->auth.secret.usage); - break; - } + /* Get the secret string using the virDomainDiskDef
trailing whitespace ^ ACK Jan
Split out into its own separate routine --- src/qemu/qemu_command.c | 108 ++++++++++++++++++++++++++++-------------------- 1 file changed, 64 insertions(+), 44 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5bd8e87..ac275c3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3037,6 +3037,68 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr op return 0; } +static int +qemuBuildVolumeString(virConnectPtr conn, + virDomainDiskDefPtr disk, + virBufferPtr opt) +{ + int ret = -1; + + switch (disk->srcpool->voltype) { + case VIR_STORAGE_VOL_DIR: + if (!disk->readonly) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot create virtual FAT disks in read-write mode")); + goto cleanup; + } + if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) + virBufferEscape(opt, ',', ",", "file=fat:floppy:%s,", disk->src); + else + virBufferEscape(opt, ',', ",", "file=fat:%s,", disk->src); + break; + case VIR_STORAGE_VOL_BLOCK: + if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("tray status 'open' is invalid for " + "block type volume")); + goto cleanup; + } + if (disk->srcpool->pooltype == VIR_STORAGE_POOL_ISCSI) { + if (disk->srcpool->mode == + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) { + if (qemuBuildISCSIString(conn, disk, opt) < 0) + goto cleanup; + virBufferAddChar(opt, ','); + } else if (disk->srcpool->mode == + VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) { + virBufferEscape(opt, ',', ",", "file=%s,", disk->src); + } + } else { + virBufferEscape(opt, ',', ",", "file=%s,", disk->src); + } + break; + case VIR_STORAGE_VOL_FILE: + if (disk->auth.username) { + if (qemuBuildISCSIString(conn, disk, opt) < 0) + goto cleanup; + virBufferAddChar(opt, ','); + } else { + virBufferEscape(opt, ',', ",", "file=%s,", disk->src); + } + break; + case VIR_STORAGE_VOL_NETWORK: + /* Keep the compiler quite, qemuTranslateDiskSourcePool already + * reported the unsupported error. + */ + break; + } + + ret = 0; + +cleanup: + return ret; +} + char * qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskDefPtr disk, @@ -3191,50 +3253,8 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, break; } } else if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) { - switch (disk->srcpool->voltype) { - case VIR_STORAGE_VOL_DIR: - if (!disk->readonly) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot create virtual FAT disks in read-write mode")); - goto error; - } - if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) - virBufferEscape(&opt, ',', ",", "file=fat:floppy:%s,", - disk->src); - else - virBufferEscape(&opt, ',', ",", "file=fat:%s,", disk->src); - break; - case VIR_STORAGE_VOL_BLOCK: - if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("tray status 'open' is invalid for " - "block type volume")); - goto error; - } - - if (disk->srcpool->pooltype == VIR_STORAGE_POOL_ISCSI) { - if (disk->srcpool->mode == - VIR_DOMAIN_DISK_SOURCE_POOL_MODE_DIRECT) { - if (qemuBuildISCSIString(conn, disk, &opt) < 0) - goto error; - virBufferAddChar(&opt, ','); - } else if (disk->srcpool->mode == - VIR_DOMAIN_DISK_SOURCE_POOL_MODE_HOST) { - virBufferEscape(&opt, ',', ",", "file=%s,", disk->src); - } - } else { - virBufferEscape(&opt, ',', ",", "file=%s,", disk->src); - } - break; - case VIR_STORAGE_VOL_FILE: - virBufferEscape(&opt, ',', ",", "file=%s,", disk->src); - break; - case VIR_STORAGE_VOL_NETWORK: - /* Keep the compiler quite, qemuTranslateDiskSourcePool already - * reported the unsupported error. - */ - break; - } + if (qemuBuildVolumeString(conn, disk, &opt) < 0) + goto error; } else { if ((disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK) && (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) { -- 1.8.1.4
On 07/22/2013 10:31 PM, John Ferlan wrote:
Split out into its own separate routine --- src/qemu/qemu_command.c | 108 ++++++++++++++++++++++++++++-------------------- 1 file changed, 64 insertions(+), 44 deletions(-)
ACK, just code movement. Jan
Although the XML for CHAP authentication with plain "password" was introduced long ago, the function was never implemented. This patch replaces the login/password mechanism by following the 'ceph' (or RBD) model of using a 'username' with a 'secret' which has the authentication information. This patch performs the authentication during startPool() processing of pools with an authType of VIR_STORAGE_POOL_AUTH_CHAP specified for iSCSI pools. There are two types of CHAP configurations supported for iSCSI authentication: * Initiator Authentication Forward, one-way; The initiator is authenticated by the target. * Target Authentication Reverse, Bi-directional, mutual, two-way; The target is authenticated by the initiator; This method also requires Initiator Authentication This only supports the "Initiator Authentication". (I don't have any enterprise iSCSI env for testing, only have a iSCSI target setup with tgtd, which doesn't support "Target Authentication"). "Discovery authentication" is not supported by tgt yet too. So this only setup the session authentication by executing 3 iscsiadm commands, E.g: % iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \ "node.session.auth.authmethod" -v "CHAP" --op update % iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \ "node.session.auth.username" -v "Jim" --op update % iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \ "node.session.auth.password" -v "Jimsecret" --op update --- src/storage/storage_backend_iscsi.c | 111 +++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 54bcd14..388d6ed 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -32,6 +32,8 @@ #include <unistd.h> #include <sys/stat.h> +#include "datatypes.h" +#include "driver.h" #include "virerror.h" #include "storage_backend_scsi.h" #include "storage_backend_iscsi.h" @@ -39,6 +41,7 @@ #include "virlog.h" #include "virfile.h" #include "vircommand.h" +#include "virobject.h" #include "virrandom.h" #include "virstring.h" @@ -658,9 +661,112 @@ virStorageBackendISCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; } +static int +virStorageBackendISCSINodeUpdate(const char *portal, + const char *target, + const char *name, + const char *value) +{ + virCommandPtr cmd = NULL; + int status; + int ret = -1; + + cmd = virCommandNewArgList(ISCSIADM, + "--mode", "node", + "--portal", portal, + "--target", target, + "--op", "update", + "--name", name, + "--value", value, + NULL); + + if (virCommandRun(cmd, &status) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to update '%s' of node mode for target '%s'"), + name, target); + goto cleanup; + } + + ret = 0; +cleanup: + virCommandFree(cmd); + return ret; +} static int -virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendISCSISetAuth(const char *portal, + virConnectPtr conn, + virStoragePoolDefPtr def) +{ + virSecretPtr secret = NULL; + unsigned char *secret_value = NULL; + virStoragePoolAuthChap chap; + int ret = -1; + + if (def->source.authType == VIR_STORAGE_POOL_AUTH_NONE) + return 0; + + if (def->source.authType != VIR_STORAGE_POOL_AUTH_CHAP) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("iscsi pool only supports 'chap' auth type")); + return -1; + } + + if (!conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("iscsi 'chap' authentication requires connection")); + return -1; + } + + chap = def->source.auth.chap; + if (chap.secret.uuidUsable) + secret = virSecretLookupByUUID(conn, chap.secret.uuid); + else + secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_ISCSI, + chap.secret.usage); + + if (secret) { + size_t secret_size; + secret_value = + conn->secretDriver->secretGetValue(secret, &secret_size, 0, + VIR_SECRET_GET_VALUE_INTERNAL_CALL); + if (!secret_value) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("could not get the value of the secret " + "for username %s"), chap.username); + goto cleanup; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("username '%s' specified but secret not found"), + chap.username); + goto cleanup; + } + + if (virStorageBackendISCSINodeUpdate(portal, + def->source.devices[0].path, + "node.session.auth.authmethod", + "CHAP") < 0 || + virStorageBackendISCSINodeUpdate(portal, + def->source.devices[0].path, + "node.session.auth.username", + chap.username) < 0 || + virStorageBackendISCSINodeUpdate(portal, + def->source.devices[0].path, + "node.session.auth.password", + (const char *)secret_value) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virObjectUnref(secret); + VIR_FREE(secret_value); + return ret; +} + +static int +virStorageBackendISCSIStartPool(virConnectPtr conn, virStoragePoolObjPtr pool) { char *portal = NULL; @@ -699,6 +805,9 @@ virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, NULL, NULL) < 0) goto cleanup; + if (virStorageBackendISCSISetAuth(portal, conn, pool->def) < 0) + goto cleanup; + if (virStorageBackendISCSIConnection(portal, pool->def->source.initiator.iqn, pool->def->source.devices[0].path, -- 1.8.1.4
Il 22/07/2013 22:31, John Ferlan ha scritto:
Although the XML for CHAP authentication with plain "password" was introduced long ago, the function was never implemented. This patch replaces the login/password mechanism by following the 'ceph' (or RBD) model of using a 'username' with a 'secret' which has the authentication information.
This patch performs the authentication during startPool() processing of pools with an authType of VIR_STORAGE_POOL_AUTH_CHAP specified for iSCSI pools.
There are two types of CHAP configurations supported for iSCSI authentication:
* Initiator Authentication Forward, one-way; The initiator is authenticated by the target.
* Target Authentication Reverse, Bi-directional, mutual, two-way; The target is authenticated by the initiator; This method also requires Initiator Authentication
This only supports the "Initiator Authentication". (I don't have any enterprise iSCSI env for testing, only have a iSCSI target setup with tgtd, which doesn't support "Target Authentication").
"Discovery authentication" is not supported by tgt yet too. So this only setup the session authentication by executing 3 iscsiadm commands, E.g:
% iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \ "node.session.auth.authmethod" -v "CHAP" --op update
% iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \ "node.session.auth.username" -v "Jim" --op update
% iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \ "node.session.auth.password" -v "Jimsecret" --op update --- src/storage/storage_backend_iscsi.c | 111 +++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 54bcd14..388d6ed 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -32,6 +32,8 @@ #include <unistd.h> #include <sys/stat.h>
+#include "datatypes.h" +#include "driver.h" #include "virerror.h" #include "storage_backend_scsi.h" #include "storage_backend_iscsi.h" @@ -39,6 +41,7 @@ #include "virlog.h" #include "virfile.h" #include "vircommand.h" +#include "virobject.h" #include "virrandom.h" #include "virstring.h"
@@ -658,9 +661,112 @@ virStorageBackendISCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; }
+static int +virStorageBackendISCSINodeUpdate(const char *portal, + const char *target, + const char *name, + const char *value) +{ + virCommandPtr cmd = NULL; + int status; + int ret = -1; + + cmd = virCommandNewArgList(ISCSIADM, + "--mode", "node", + "--portal", portal, + "--target", target, + "--op", "update", + "--name", name, + "--value", value, + NULL); + + if (virCommandRun(cmd, &status) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to update '%s' of node mode for target '%s'"), + name, target); + goto cleanup; + } + + ret = 0; +cleanup: + virCommandFree(cmd); + return ret; +}
static int -virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendISCSISetAuth(const char *portal, + virConnectPtr conn, + virStoragePoolDefPtr def) +{ + virSecretPtr secret = NULL; + unsigned char *secret_value = NULL; + virStoragePoolAuthChap chap; + int ret = -1; + + if (def->source.authType == VIR_STORAGE_POOL_AUTH_NONE) + return 0; + + if (def->source.authType != VIR_STORAGE_POOL_AUTH_CHAP) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("iscsi pool only supports 'chap' auth type")); + return -1; + } + + if (!conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("iscsi 'chap' authentication requires connection")); + return -1; + }
Should this be more precisely "not supported for autostarted pools"?
+ + chap = def->source.auth.chap; + if (chap.secret.uuidUsable) + secret = virSecretLookupByUUID(conn, chap.secret.uuid); + else + secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_ISCSI, + chap.secret.usage); + + if (secret) { + size_t secret_size; + secret_value = + conn->secretDriver->secretGetValue(secret, &secret_size, 0, + VIR_SECRET_GET_VALUE_INTERNAL_CALL); + if (!secret_value) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("could not get the value of the secret " + "for username %s"), chap.username); + goto cleanup; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("username '%s' specified but secret not found"), + chap.username); + goto cleanup; + } + + if (virStorageBackendISCSINodeUpdate(portal, + def->source.devices[0].path, + "node.session.auth.authmethod", + "CHAP") < 0 || + virStorageBackendISCSINodeUpdate(portal, + def->source.devices[0].path, + "node.session.auth.username", + chap.username) < 0 || + virStorageBackendISCSINodeUpdate(portal, + def->source.devices[0].path, + "node.session.auth.password", + (const char *)secret_value) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virObjectUnref(secret); + VIR_FREE(secret_value); + return ret; +} + +static int +virStorageBackendISCSIStartPool(virConnectPtr conn, virStoragePoolObjPtr pool) { char *portal = NULL; @@ -699,6 +805,9 @@ virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, NULL, NULL) < 0) goto cleanup;
+ if (virStorageBackendISCSISetAuth(portal, conn, pool->def) < 0) + goto cleanup; + if (virStorageBackendISCSIConnection(portal, pool->def->source.initiator.iqn, pool->def->source.devices[0].path,
On 07/22/2013 10:31 PM, John Ferlan wrote:
--- src/storage/storage_backend_iscsi.c | 111 +++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-)
I can confirm this works, but it's a shame it doesn't work on autostart. ACK if you clarify the error. Jan
On 07/23/2013 10:18 AM, Ján Tomko wrote:
On 07/22/2013 10:31 PM, John Ferlan wrote:
--- src/storage/storage_backend_iscsi.c | 111 +++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-)
I can confirm this works, but it's a shame it doesn't work on autostart.
ACK if you clarify the error.
Jan
The autostart changes require getting a connection to the secret driver which I felt may take more time than I had to figure out how to get to work properly... In any case, I adjusted the message as follows (same in 5/5): diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_i index 388d6ed..ee8dd2e 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -714,7 +714,8 @@ virStorageBackendISCSISetAuth(const char *portal, if (!conn) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("iscsi 'chap' authentication requires connection")); + _("iscsi 'chap' authentication not supported " + "for autostarted pools")); return -1; } John
On Tue, Jul 23, 2013 at 10:47:46AM -0400, John Ferlan wrote:
On 07/23/2013 10:18 AM, Ján Tomko wrote:
On 07/22/2013 10:31 PM, John Ferlan wrote:
--- src/storage/storage_backend_iscsi.c | 111 +++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-)
I can confirm this works, but it's a shame it doesn't work on autostart.
ACK if you clarify the error.
Jan
The autostart changes require getting a connection to the secret driver which I felt may take more time than I had to figure out how to get to work properly...
In any case, I adjusted the message as follows (same in 5/5):
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_i index 388d6ed..ee8dd2e 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -714,7 +714,8 @@ virStorageBackendISCSISetAuth(const char *portal,
if (!conn) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("iscsi 'chap' authentication requires connection")); + _("iscsi 'chap' authentication not supported " + "for autostarted pools")); return -1; }
I noticed that the nwfilter already unconditionally calls virConnectOpen("qemu://system"); so we're already in fact suffering from the problem with autostart having a qemu dependency. Given this, I'd support a patch which simply did conn = virConnectOpen(privilege ? "qemu:///system" : "qemu:///session"); in storageDriverAutostart, provided that we ignore any errors from virConnectOpen, and fallback to use NULL for the connection in that case. Obviously this is something we'll still need to fix properly in a future release, but at least it'll make autostart of storage pools with auth work in the common case in the short term for this release. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/23/2013 10:56 AM, Daniel P. Berrange wrote: <...snip...>
+++ b/src/storage/storage_backend_iscsi.c @@ -714,7 +714,8 @@ virStorageBackendISCSISetAuth(const char *portal,
if (!conn) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("iscsi 'chap' authentication requires connection")); + _("iscsi 'chap' authentication not supported " + "for autostarted pools")); return -1; }
I noticed that the nwfilter already unconditionally calls virConnectOpen("qemu://system"); so we're already in fact suffering from the problem with autostart having a qemu dependency.
Given this, I'd support a patch which simply did
conn = virConnectOpen(privilege ? "qemu:///system" : "qemu:///session");
in storageDriverAutostart, provided that we ignore any errors from virConnectOpen, and fallback to use NULL for the connection in that case.
Obviously this is something we'll still need to fix properly in a future release, but at least it'll make autostart of storage pools with auth work in the common case in the short term for this release.
Regards, Daniel
So resurrect the following: https://www.redhat.com/archives/libvir-list/2013-July/msg00962.html John
On Tue, Jul 23, 2013 at 11:50:57AM -0400, John Ferlan wrote:
On 07/23/2013 10:56 AM, Daniel P. Berrange wrote: <...snip...>
+++ b/src/storage/storage_backend_iscsi.c @@ -714,7 +714,8 @@ virStorageBackendISCSISetAuth(const char *portal,
if (!conn) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("iscsi 'chap' authentication requires connection")); + _("iscsi 'chap' authentication not supported " + "for autostarted pools")); return -1; }
I noticed that the nwfilter already unconditionally calls virConnectOpen("qemu://system"); so we're already in fact suffering from the problem with autostart having a qemu dependency.
Given this, I'd support a patch which simply did
conn = virConnectOpen(privilege ? "qemu:///system" : "qemu:///session");
in storageDriverAutostart, provided that we ignore any errors from virConnectOpen, and fallback to use NULL for the connection in that case.
Obviously this is something we'll still need to fix properly in a future release, but at least it'll make autostart of storage pools with auth work in the common case in the short term for this release.
Regards, Daniel
So resurrect the following:
https://www.redhat.com/archives/libvir-list/2013-July/msg00962.html
Yep, ACK to that patch, but add "XXX Remove hardcoding of QEMU URI" as a comment just before the virConnectOpen call. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/23/2013 12:03 PM, Daniel P. Berrange wrote:
On Tue, Jul 23, 2013 at 11:50:57AM -0400, John Ferlan wrote:
On 07/23/2013 10:56 AM, Daniel P. Berrange wrote: <...snip...>
+++ b/src/storage/storage_backend_iscsi.c @@ -714,7 +714,8 @@ virStorageBackendISCSISetAuth(const char *portal,
if (!conn) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("iscsi 'chap' authentication requires connection")); + _("iscsi 'chap' authentication not supported " + "for autostarted pools")); return -1; }
I noticed that the nwfilter already unconditionally calls virConnectOpen("qemu://system"); so we're already in fact suffering from the problem with autostart having a qemu dependency.
Given this, I'd support a patch which simply did
conn = virConnectOpen(privilege ? "qemu:///system" : "qemu:///session");
in storageDriverAutostart, provided that we ignore any errors from virConnectOpen, and fallback to use NULL for the connection in that case.
Obviously this is something we'll still need to fix properly in a future release, but at least it'll make autostart of storage pools with auth work in the common case in the short term for this release.
Regards, Daniel
So resurrect the following:
https://www.redhat.com/archives/libvir-list/2013-July/msg00962.html
Yep, ACK to that patch, but add "XXX Remove hardcoding of QEMU URI" as a comment just before the virConnectOpen call.
Daniel
OK - I resurrected the patch, added the comment, and pushed. John
On 07/23/2013 04:56 PM, Daniel P. Berrange wrote:
On Tue, Jul 23, 2013 at 10:47:46AM -0400, John Ferlan wrote:
On 07/23/2013 10:18 AM, Ján Tomko wrote:
On 07/22/2013 10:31 PM, John Ferlan wrote:
--- src/storage/storage_backend_iscsi.c | 111 +++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-)
I can confirm this works, but it's a shame it doesn't work on autostart.
ACK if you clarify the error.
Jan
The autostart changes require getting a connection to the secret driver which I felt may take more time than I had to figure out how to get to work properly...
In any case, I adjusted the message as follows (same in 5/5):
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_i index 388d6ed..ee8dd2e 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -714,7 +714,8 @@ virStorageBackendISCSISetAuth(const char *portal,
if (!conn) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("iscsi 'chap' authentication requires connection")); + _("iscsi 'chap' authentication not supported " + "for autostarted pools")); return -1; }
I noticed that the nwfilter already unconditionally calls virConnectOpen("qemu://system"); so we're already in fact suffering from the problem with autostart having a qemu dependency.
Given this, I'd support a patch which simply did
conn = virConnectOpen(privilege ? "qemu:///system" : "qemu:///session");
in storageDriverAutostart, provided that we ignore any errors from virConnectOpen, and fallback to use NULL for the connection in that case.
Obviously this is something we'll still need to fix properly in a future release, but at least it'll make autostart of storage pools with auth work in the common case in the short term for this release.
Both secret and qemu drivers are registered after the storage driver on libvirtd startup, so autostarting these pools will only work on storage driver reload. On libvirtd startup it fails with: qemuConnectOpen:1033 : internal error qemu state driver is not active (And it seems nwfilter only opens the qemu:// connection on reload) Jan
On 24/07/13 16:25, Ján Tomko wrote:
On 07/23/2013 04:56 PM, Daniel P. Berrange wrote:
On Tue, Jul 23, 2013 at 10:47:46AM -0400, John Ferlan wrote:
On 07/23/2013 10:18 AM, Ján Tomko wrote:
On 07/22/2013 10:31 PM, John Ferlan wrote:
--- src/storage/storage_backend_iscsi.c | 111 +++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-)
I can confirm this works, but it's a shame it doesn't work on autostart.
ACK if you clarify the error.
Jan
The autostart changes require getting a connection to the secret driver which I felt may take more time than I had to figure out how to get to work properly...
In any case, I adjusted the message as follows (same in 5/5):
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_i index 388d6ed..ee8dd2e 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -714,7 +714,8 @@ virStorageBackendISCSISetAuth(const char *portal,
if (!conn) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("iscsi 'chap' authentication requires connection")); + _("iscsi 'chap' authentication not supported " + "for autostarted pools")); return -1; } I noticed that the nwfilter already unconditionally calls virConnectOpen("qemu://system"); so we're already in fact suffering from the problem with autostart having a qemu dependency.
Given this, I'd support a patch which simply did
conn = virConnectOpen(privilege ? "qemu:///system" : "qemu:///session");
in storageDriverAutostart, provided that we ignore any errors from virConnectOpen, and fallback to use NULL for the connection in that case.
Obviously this is something we'll still need to fix properly in a future release, but at least it'll make autostart of storage pools with auth work in the common case in the short term for this release.
Both secret and qemu drivers are registered after the storage driver on libvirtd startup, so autostarting these pools will only work on storage driver reload. On libvirtd startup it fails with: qemuConnectOpen:1033 : internal error qemu state driver is not active
oh, that's bad, fortunately we are just entering the freezing. nwfilter is also loaded before qemu driver too, but it only creates the connection when reloading. changing to load the storage and secret modules after qemu module should be okay, unless there are other depedancies between those modules in the middle.
(And it seems nwfilter only opens the qemu:// connection on reload)
conn = virConnectOpen("qemu:///system");
Jan
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 24, 2013 at 10:25:06AM +0200, Ján Tomko wrote:
On 07/23/2013 04:56 PM, Daniel P. Berrange wrote:
On Tue, Jul 23, 2013 at 10:47:46AM -0400, John Ferlan wrote:
On 07/23/2013 10:18 AM, Ján Tomko wrote:
On 07/22/2013 10:31 PM, John Ferlan wrote:
--- src/storage/storage_backend_iscsi.c | 111 +++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-)
I can confirm this works, but it's a shame it doesn't work on autostart.
ACK if you clarify the error.
Jan
The autostart changes require getting a connection to the secret driver which I felt may take more time than I had to figure out how to get to work properly...
In any case, I adjusted the message as follows (same in 5/5):
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_i index 388d6ed..ee8dd2e 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -714,7 +714,8 @@ virStorageBackendISCSISetAuth(const char *portal,
if (!conn) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("iscsi 'chap' authentication requires connection")); + _("iscsi 'chap' authentication not supported " + "for autostarted pools")); return -1; }
I noticed that the nwfilter already unconditionally calls virConnectOpen("qemu://system"); so we're already in fact suffering from the problem with autostart having a qemu dependency.
Given this, I'd support a patch which simply did
conn = virConnectOpen(privilege ? "qemu:///system" : "qemu:///session");
in storageDriverAutostart, provided that we ignore any errors from virConnectOpen, and fallback to use NULL for the connection in that case.
Obviously this is something we'll still need to fix properly in a future release, but at least it'll make autostart of storage pools with auth work in the common case in the short term for this release.
Both secret and qemu drivers are registered after the storage driver on libvirtd startup, so autostarting these pools will only work on storage driver reload. On libvirtd startup it fails with: qemuConnectOpen:1033 : internal error qemu state driver is not active
(And it seems nwfilter only opens the qemu:// connection on reload)
Oh damn, yes, that pretty much dooms us. We can't change the order of the drivers either, because autostarting of QEMU guests, requires that the storage pools be autostarted already. To fix this would require that we split virStateInitialize into two parts, virStateInitialize() and virStateAutoStart(). That's too big a change todo for this release, but we could do it for next release without too much trouble. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/24/2013 05:57 AM, Daniel P. Berrange wrote:
On Wed, Jul 24, 2013 at 10:25:06AM +0200, Ján Tomko wrote: <...snip...>
Both secret and qemu drivers are registered after the storage driver on libvirtd startup, so autostarting these pools will only work on storage driver reload. On libvirtd startup it fails with: qemuConnectOpen:1033 : internal error qemu state driver is not active
(And it seems nwfilter only opens the qemu:// connection on reload)
Oh damn, yes, that pretty much dooms us. We can't change the order of the drivers either, because autostarting of QEMU guests, requires that the storage pools be autostarted already.
To fix this would require that we split virStateInitialize into two parts, virStateInitialize() and virStateAutoStart(). That's too big a change todo for this release, but we could do it for next release without too much trouble.
Daniel
Could we just do it for storage driver? It seems you are indicating that the following would suffice, right? Or does this problem go deeper? John diff --git a/src/driver.h b/src/driver.h index cc03e9f..b416902 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1801,6 +1801,9 @@ typedef int void *opaque); typedef int +(*virDrvStateAutoStart)(void); + +typedef int (*virDrvStateCleanup)(void); typedef int @@ -1815,6 +1818,7 @@ typedef virStateDriver *virStateDriverPtr; struct _virStateDriver { const char *name; virDrvStateInitialize stateInitialize; + virDrvStateAutoStart stateAutoStart; virDrvStateCleanup stateCleanup; virDrvStateReload stateReload; virDrvStateStop stateStop; diff --git a/src/libvirt.c b/src/libvirt.c index 444c1c3..e0bd7aa 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -808,7 +808,11 @@ virRegisterStateDriver(virStateDriverPtr driver) * @callback: callback to invoke to inhibit shutdown of the daemon * @opaque: data to pass to @callback * - * Initialize all virtualization drivers. + * Initialize all virtualization drivers. Accomplished in two phases, + * the first being state and structure initialization followed by any + * auto start supported by the driver. This is done to ensure dependencies + * that some drivers may have will exist. Such as the storage driver's need + * to use the secret driver. * * Returns 0 if all succeed, -1 upon any failure. */ @@ -836,6 +840,22 @@ int virStateInitialize(bool privileged, } } } + + for (i = 0; i < virStateDriverTabCount; i++) { + if (virStateDriverTab[i]->stateAutoStart) { + VIR_DEBUG("Running global auto start for %s state driver", + virStateDriverTab[i]->name); + if (virStateDriverTab[i]->stateAutoStart() < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Auto start for %s driver failed: %s"), + virStateDriverTab[i]->name, + err && err->message ? err->message : + _("Unknown problem")); + if (virStateDriverTab[i]->stateCleanup) + ignore_value(virStateDriverTab[i]->stateCleanup()); + return -1; + } + } + } return 0; } diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4f0c631..390e196 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -182,7 +182,6 @@ storageStateInitialize(bool privileged, driverState->configDir, driverState->autostartDir) < 0) goto error; - storageDriverAutostart(driverState); storageDriverUnlock(driverState); return 0; @@ -195,6 +194,20 @@ error: } /** + * storageStateAutoStart: + * + * Function to auto start the storage driver + */ +static int +storageStateAutoStart(void) +{ + storageDriverLock(driverState); + storageDriverAutostart(driverState); + storageDriverUnlock(driverState); + return 0; +} + +/** * storageStateReload: * * Function to restart the storage driver, it will recheck the configuration @@ -2599,6 +2612,7 @@ static virStorageDriver storageDriver = { static virStateDriver stateDriver = { .name = "Storage", .stateInitialize = storageStateInitialize, + .stateAutoStart = storageStateAutoStart, .stateCleanup = storageStateCleanup, .stateReload = storageStateReload, };
On 07/24/2013 10:06 PM, John Ferlan wrote:
On 07/24/2013 05:57 AM, Daniel P. Berrange wrote:
On Wed, Jul 24, 2013 at 10:25:06AM +0200, Ján Tomko wrote: <...snip...>
Both secret and qemu drivers are registered after the storage driver on libvirtd startup, so autostarting these pools will only work on storage driver reload. On libvirtd startup it fails with: qemuConnectOpen:1033 : internal error qemu state driver is not active
(And it seems nwfilter only opens the qemu:// connection on reload)
Oh damn, yes, that pretty much dooms us. We can't change the order of the drivers either, because autostarting of QEMU guests, requires that the storage pools be autostarted already.
To fix this would require that we split virStateInitialize into two parts, virStateInitialize() and virStateAutoStart(). That's too big a change todo for this release, but we could do it for next release without too much trouble.
Daniel
Could we just do it for storage driver? It seems you are indicating that the following would suffice, right? Or does this problem go deeper?
Doing it only for the storage driver would move the pool autostart after QEMU guests autostart and the guests wouldn't be able to use the pools. Jan
On Thu, Jul 25, 2013 at 08:10:46AM +0200, Ján Tomko wrote:
On 07/24/2013 10:06 PM, John Ferlan wrote:
On 07/24/2013 05:57 AM, Daniel P. Berrange wrote:
On Wed, Jul 24, 2013 at 10:25:06AM +0200, Ján Tomko wrote: <...snip...>
Both secret and qemu drivers are registered after the storage driver on libvirtd startup, so autostarting these pools will only work on storage driver reload. On libvirtd startup it fails with: qemuConnectOpen:1033 : internal error qemu state driver is not active
(And it seems nwfilter only opens the qemu:// connection on reload)
Oh damn, yes, that pretty much dooms us. We can't change the order of the drivers either, because autostarting of QEMU guests, requires that the storage pools be autostarted already.
To fix this would require that we split virStateInitialize into two parts, virStateInitialize() and virStateAutoStart(). That's too big a change todo for this release, but we could do it for next release without too much trouble.
Daniel
Could we just do it for storage driver? It seems you are indicating that the following would suffice, right? Or does this problem go deeper?
Doing it only for the storage driver would move the pool autostart after QEMU guests autostart and the guests wouldn't be able to use the pools.
Yep, we'd need todo it for all the drivers at once to preseve the ordering. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Update virStorageBackendRBDOpenRADOSConn() to use the internal API to the secret driver in order to get the secret value instead of the external virSecretGetValue() path. Without the flag VIR_SECRET_GET_VALUE_INTERNAL_CALL there is no way to get the value of private secret. This also requires ensuring there is a connection which wasn't true for for the refreshPool() path calls from storageDriverAutostart() prior to adding support for the connection to a qemu driver. It seems calls to virSecretLookupByUUIDString() and virSecretLookupByUsage() from the refreshPool() path would have failed with no way to find the secret - that is theoretically speaking since the 'conn' was NULL the failure would have been "failed to find the secret". --- src/storage/storage_backend_rbd.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index badbdac..70121bf 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -23,6 +23,7 @@ #include <config.h> +#include "datatypes.h" #include "virerror.h" #include "storage_backend_rbd.h" #include "storage_conf.h" @@ -71,6 +72,12 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr, goto cleanup; } + if (!conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'ceph' authentication requires connection")); + return -1; + } + if (pool->def->source.auth.cephx.secret.uuidUsable) { virUUIDFormat(pool->def->source.auth.cephx.secret.uuid, secretUuid); VIR_DEBUG("Looking up secret by UUID: %s", secretUuid); @@ -88,7 +95,17 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr, goto cleanup; } - secret_value = virSecretGetValue(secret, &secret_value_size, 0); + secret_value = conn->secretDriver->secretGetValue(secret, &secret_value_size, 0, + VIR_SECRET_GET_VALUE_INTERNAL_CALL); + + if (!secret_value) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("could not get the value of the secret " + "for username %s"), + pool->def->source.auth.cephx.username); + goto cleanup; + } + base64_encode_alloc((char *)secret_value, secret_value_size, &rados_key); memset(secret_value, 0, secret_value_size); @@ -257,7 +274,7 @@ cleanup: return ret; } -static int virStorageBackendRBDRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, +static int virStorageBackendRBDRefreshPool(virConnectPtr conn, virStoragePoolObjPtr pool) { size_t max_size = 1024; -- 1.8.1.4
On 07/22/2013 10:31 PM, John Ferlan wrote:
Update virStorageBackendRBDOpenRADOSConn() to use the internal API to the secret driver in order to get the secret value instead of the external virSecretGetValue() path. Without the flag VIR_SECRET_GET_VALUE_INTERNAL_CALL there is no way to get the value of private secret.
This also requires ensuring there is a connection which wasn't true for for the refreshPool() path calls from storageDriverAutostart() prior to adding support for the connection to a qemu driver. It seems calls to virSecretLookupByUUIDString() and virSecretLookupByUsage() from the refreshPool() path would have failed with no way to find the secret - that is theoretically speaking since the 'conn' was NULL the failure would have been "failed to find the secret". --- src/storage/storage_backend_rbd.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index badbdac..70121bf 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -23,6 +23,7 @@
#include <config.h>
+#include "datatypes.h" #include "virerror.h" #include "storage_backend_rbd.h" #include "storage_conf.h" @@ -71,6 +72,12 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr, goto cleanup; }
+ if (!conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'ceph' authentication requires connection"));
ACK if you change the error to mention autostart, as Paolo suggested in his reply to 4/5. Jan
On 07/22/2013 04:31 PM, John Ferlan wrote:
This is a reworking and reposting of the authentication patches originally posted as part of my v3 reworking of Osier's original patches, see: <...snip...>
John Ferlan (5): qemu: Add source pool auth info to virDomainDiskDef for iSCSI qemu: Create a common qemuGetSecretString qemu_common: Create qemuBuildVolumeString() to process storage pool storage: Support "chap" authentication for iscsi pool Adjust 'ceph' authentication secret usage for rbd pool.
src/qemu/qemu_command.c | 265 ++++++++++++++++++++---------------- src/qemu/qemu_conf.c | 55 ++++++++ src/storage/storage_backend_iscsi.c | 111 ++++++++++++++- src/storage/storage_backend_rbd.c | 21 ++- 4 files changed, 329 insertions(+), 123 deletions(-)
I updated 2/5 to remove the space, adjusted the messages in 4/5 & 5/5 to indicate not supported for autostarted pools and pushed the above changes. Thanks for the reviews. Now to see what it would take for a secretdriver connection... John
participants (5)
-
Daniel P. Berrange -
John Ferlan -
Ján Tomko -
Osier Yang -
Paolo Bonzini