[PATCH master 0/9] A bunch of import/export improvements

Michael Hanselmann

unread,

Jun 11, 2010, 1:14:15 PM6/11/10

to ganeti...@googlegroups.com

This patch series introduces a bunch of import/export improvements.

Michael Hanselmann (9):
utils: Add function to validate TCP/UDP port
import/export: Validate remote host/port
import/export: Limit max length of socat options
import/export daemon: Simplify command building
import/export daemon: Add support for a magic prefix
backend: Add support for import/export magic
qa_rapi: Test inter-cluster instance move script
Disable compression for all intra-cluster imports/exports
Use import/export magic for backup/import and inter-cluster moves

Michael Hanselmann

unread,

Jun 11, 2010, 1:14:22 PM6/11/10

to ganeti...@googlegroups.com

---
lib/backend.py | 3 +++
lib/masterd/instance.py | 7 +++++++
lib/objects.py | 2 ++
3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/lib/backend.py b/lib/backend.py
index ffee337..6d31576 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -2696,6 +2696,9 @@ def StartImportExportDaemon(mode, opts, host, port, instance, ieio, ieioargs):
if opts.compress:
cmd.append("--compress=%s" % opts.compress)

+ if opts.magic:
+ cmd.append("--magic=%s" % opts.magic)
+
if exp_size is not None:
cmd.append("--expected-size=%s" % exp_size)

diff --git a/lib/masterd/instance.py b/lib/masterd/instance.py
index f80c6bb..fd30e4a 100644
--- a/lib/masterd/instance.py
+++ b/lib/masterd/instance.py
@@ -209,6 +209,13 @@ class _DiskImportExportBase(object):
self._daemon.progress_eta)

@property
+ def magic(self):
+ """Returns the magic value for this import/export.
+
+ """
+ return self._opts.magic
+
+ @property
def active(self):
"""Determines whether this transport is still active.

diff --git a/lib/objects.py b/lib/objects.py
index f954081..19c6d8f 100644
--- a/lib/objects.py
+++ b/lib/objects.py
@@ -1032,12 +1032,14 @@ class ImportExportOptions(ConfigObject):
@ivar key_name: X509 key name (None for cluster certificate)
@ivar ca_pem: Remote peer CA in PEM format (None for cluster certificate)
@ivar compress: Compression method (one of L{constants.IEC_ALL})
+ @ivar magic: Used to ensure the connection goes to the right disk

"""
__slots__ = [
"key_name",
"ca_pem",
"compress",
+ "magic",
]

--
1.7.0.4

Michael Hanselmann

unread,

Jun 11, 2010, 1:14:24 PM6/11/10

to ganeti...@googlegroups.com

Tests have shown that usually we're CPU-bound for intra-cluster
imports/exports. Disabling compression will help with this.

Some versions of OpenSSL, depending on the build options, also
compress transparently. This will need further work in Ganeti.
---
lib/masterd/instance.py | 7 ++-----
1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/masterd/instance.py b/lib/masterd/instance.py
index fd30e4a..0a8d3a2 100644
--- a/lib/masterd/instance.py
+++ b/lib/masterd/instance.py
@@ -996,11 +996,8 @@ def TransferInstanceData(lu, feedback_fn, src_node, dest_node, dest_ip,
each transfer

"""
- # Compress only if transfer is to another node
- if src_node == dest_node:
- compress = constants.IEC_NONE
- else:
- compress = constants.IEC_GZIP
+ # Disable compression for all moves as these are all within the same cluster
+ compress = constants.IEC_NONE

logging.debug("Source node %s, destination node %s, compression '%s'",
src_node, dest_node, compress)
--
1.7.0.4

Michael Hanselmann

unread,

Jun 11, 2010, 1:14:19 PM6/11/10

to ganeti...@googlegroups.com

Signed-off-by: Michael Hanselmann <han...@google.com>
---
lib/impexpd/__init__.py | 6 ++++++
test/ganeti.impexpd_unittest.py | 19 +++++++++++++++++++
2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/lib/impexpd/__init__.py b/lib/impexpd/__init__.py
index 02b7bb9..b1078db 100644
--- a/lib/impexpd/__init__.py
+++ b/lib/impexpd/__init__.py
@@ -79,6 +79,8 @@ BUFSIZE = 1024 * 1024
SOCAT_TCP_OPTS = ["keepalive", "keepidle=60", "keepintvl=10", "keepcnt=5"]
SOCAT_OPENSSL_OPTS = ["verify=1", "cipher=HIGH", "method=TLSv1"]

+SOCAT_OPTION_MAXLEN = 400
+
(PROG_OTHER,
PROG_SOCAT,
PROG_DD,
@@ -168,6 +170,10 @@ class CommandBuilder(object):

for i in [addr1, addr2]:
for value in i:
+ if len(value) > SOCAT_OPTION_MAXLEN:
+ raise errors.GenericError("Socat option longer than %s"
+ " characters: %r" %
+ (SOCAT_OPTION_MAXLEN, value))
if "," in value:
raise errors.GenericError("Comma not allowed in socat option"
" value: %r" % value)
diff --git a/test/ganeti.impexpd_unittest.py b/test/ganeti.impexpd_unittest.py
index 7832033..0126a5f 100755
--- a/test/ganeti.impexpd_unittest.py
+++ b/test/ganeti.impexpd_unittest.py
@@ -111,6 +111,25 @@ class TestCommandBuilder(unittest.TestCase):
builder = impexpd.CommandBuilder(mode, opts, 1, 2, 3)
self.assertRaises(errors.GenericError, builder.GetCommand)

+ def testOptionLengthError(self):
+ testopts = [
+ CmdBuilderConfig(bind="0.0.0.0" + ("A" * impexpd.SOCAT_OPTION_MAXLEN),
+ port=1234, ca="/tmp/ca"),
+ CmdBuilderConfig(host="localhost", port=1234,
+ ca="/tmp/ca" + ("B" * impexpd.SOCAT_OPTION_MAXLEN)),
+ CmdBuilderConfig(host="localhost", port=1234,
+ key="/tmp/key" + ("B" * impexpd.SOCAT_OPTION_MAXLEN)),
+ ]
+
+ for opts in testopts:
+ for mode in [constants.IEM_IMPORT, constants.IEM_EXPORT]:
+ builder = impexpd.CommandBuilder(mode, opts, 1, 2, 3)
+ self.assertRaises(errors.GenericError, builder.GetCommand)
+
+ opts.host = "localhost" + ("A" * impexpd.SOCAT_OPTION_MAXLEN)
+ builder = impexpd.CommandBuilder(constants.IEM_EXPORT, opts, 1, 2, 3)
+ self.assertRaises(errors.GenericError, builder.GetCommand)
+
def testModeError(self):
mode = "foobarbaz"

--
1.7.0.4

Michael Hanselmann

unread,

Jun 11, 2010, 1:14:16 PM6/11/10

to ganeti...@googlegroups.com

Upon sending signals, ESRCH can be reported when the target no
longer exists.

Signed-off-by: Michael Hanselmann <han...@google.com>
---

daemons/import-export | 7 ++++++-
lib/backend.py | 7 ++++++-
lib/impexpd/__init__.py | 1 +
lib/utils.py | 18 ++++++++++++------
4 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/daemons/import-export b/daemons/import-export
index 870f9a0..d0444f5 100755
--- a/daemons/import-export
+++ b/daemons/import-export
@@ -459,7 +459,12 @@ class ChildProcess(subprocess.Popen):

"""
logging.info("Sending signal %s to child process", signum)
- os.killpg(self.pid, signum)
+ try:
+ os.killpg(self.pid, signum)
+ except EnvironmentError, err:
+ # Ignore ESRCH
+ if err.errno != errno.ESRCH:
+ raise

def ForceQuit(self):
"""Ensure child process is no longer running.
diff --git a/lib/backend.py b/lib/backend.py
index edf6518..ffee337 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -2764,7 +2764,12 @@ def AbortImportExport(name):
if pid:
logging.info("Import/export %s is running with PID %s, sending SIGTERM",
name, pid)
- os.kill(pid, signal.SIGTERM)
+ try:
+ os.kill(pid, signal.SIGTERM)
+ except EnvironmentError, err:
+ # Ignore ESRCH
+ if err.errno != errno.ESRCH:
+ raise

def CleanupImportExport(name):
diff --git a/lib/impexpd/__init__.py b/lib/impexpd/__init__.py
index bcd4dab..02b7bb9 100644
--- a/lib/impexpd/__init__.py
+++ b/lib/impexpd/__init__.py
@@ -344,6 +344,7 @@ class ChildIOProcessor(object):
raise

# Process no longer exists
+ logging.debug("dd exited")
self._dd_pid = None

return True
diff --git a/lib/utils.py b/lib/utils.py
index 1ef35cb..cc1d828 100644
--- a/lib/utils.py
+++ b/lib/utils.py
@@ -2339,12 +2339,18 @@ def KillProcess(pid, signal_=signal.SIGTERM, timeout=30,
"""
def _helper(pid, signal_, wait):
"""Simple helper to encapsulate the kill/waitpid sequence"""
- os.kill(pid, signal_)
- if wait:
- try:
- os.waitpid(pid, os.WNOHANG)
- except OSError:
- pass
+ try:
+ os.kill(pid, signal_)
+ except EnvironmentError, err:
+ # Ignore ESRCH (No such process)
+ if err.errno != errno.ESRCH:
+ raise
+ else:
+ if wait:
+ try:
+ os.waitpid(pid, os.WNOHANG)
+ except OSError:
+ pass

if pid <= 0:
# kill with pid=0 == suicide
--
1.7.0.4

Michael Hanselmann

unread,

Jun 11, 2010, 1:14:17 PM6/11/10

to ganeti...@googlegroups.com

---
lib/utils.py | 25 +++++++++++++++++++++++++
test/ganeti.utils_unittest.py | 26 ++++++++++++++++++++++++++
2 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/lib/utils.py b/lib/utils.py
index cc1d828..af58032 100644
--- a/lib/utils.py
+++ b/lib/utils.py
@@ -81,6 +81,8 @@ X509_SIGNATURE = re.compile(r"^%s:\s*(?P<salt>%s+)/(?P<sign>%s+)$" %
HEX_CHAR_RE, HEX_CHAR_RE),
re.S | re.I)

+_VALID_PORT_RE = re.compile("^[-_.a-zA-Z0-9]{1,128}$")
+
# Structure definition for getsockopt(SOL_SOCKET, SO_PEERCRED, ...):
# struct ucred { pid_t pid; uid_t uid; gid_t gid; };
#
@@ -1155,6 +1157,29 @@ class HostInfo:
return hostname

+def ValidatePort(port):
+ """Validate the given port.
+
+ @type port: number or string
+ @param port: Port specification
+
+ """
+ try:
+ numport = int(port)
+ except (ValueError, TypeError):
+ # Non-numeric port
+ valid = _VALID_PORT_RE.match(port)
+ else:
+ # Numeric port (protocols other than TCP or UDP might need adjustments
+ # here)
+ valid = (numport >= 0 and numport < (1 << 16))
+
+ if not valid:
+ raise errors.OpPrereqError("Invalid port '%s'" % port, errors.ECODE_INVAL)
+
+ return port
+
+
def GetHostInfo(name=None):
"""Lookup host name and raise an OpPrereqError for failures"""

diff --git a/test/ganeti.utils_unittest.py b/test/ganeti.utils_unittest.py
index 68ef826..cf0abb1 100755
--- a/test/ganeti.utils_unittest.py
+++ b/test/ganeti.utils_unittest.py
@@ -1903,6 +1903,32 @@ class TestHostInfo(unittest.TestCase):
HostInfo.NormalizeName(value)

+class TestValidatePort(unittest.TestCase):
+ def testValid(self):
+ testports = [
+ 0, 1, 2, 3, 1024, 65000, 65534, 65535,
+ "ganeti",
+ "gnt-masterd",
+ "HELLO_WORLD_SVC",
+ "hello.world.1",
+ "0", "80", "1111", "65535",
+ ]
+
+ for port in testports:
+ self.assertEqual(utils.ValidatePort(port), port)
+
+ def testInvalid(self):
+ testports = [
+ -15756, -1, 65536, 133428083,
+ "", "Hello World!", "!", "'", "\"", "\t", "\n", "`",
+ "-8546", "-1", "65536",
+ (129 * "A"),
+ ]
+
+ for port in testports:
+ self.assertRaises(OpPrereqError, utils.ValidatePort, port)
+
+
class TestParseAsn1Generalizedtime(unittest.TestCase):
def test(self):
# UTC
--
1.7.0.4

Michael Hanselmann

unread,

Jun 11, 2010, 1:14:25 PM6/11/10

to ganeti...@googlegroups.com

This should prevent bugs in our code from accidentally overwriting
disks.
---
lib/cmdlib.py | 12 ++--
lib/masterd/instance.py | 108 ++++++++++++++++++++----------
test/ganeti.masterd.instance_unittest.py | 16 +++--
3 files changed, 88 insertions(+), 48 deletions(-)

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 8dbb1de..b9983e4 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -9143,13 +9143,13 @@ class LUExportInstance(LogicalUnit):
disk_info = []
for idx, disk_data in enumerate(self.op.target_node):
try:
- (host, port) = masterd.instance.CheckRemoteExportDiskInfo(cds, idx,
- disk_data)
+ (host, port, magic) = \
+ masterd.instance.CheckRemoteExportDiskInfo(cds, idx, disk_data)
except errors.GenericError, err:
raise errors.OpPrereqError("Target info for disk %s: %s" % (idx, err),
errors.ECODE_INVAL)

- disk_info.append((host, port))
+ disk_info.append((host, port, magic))

assert len(disk_info) == len(self.op.target_node)
self.dest_disk_info = disk_info
@@ -9241,10 +9241,8 @@ class LUExportInstance(LogicalUnit):
OpenSSL.crypto.dump_certificate(OpenSSL.crypto.FILETYPE_PEM,
self.dest_x509_ca)

- opts = objects.ImportExportOptions(key_name=key_name,
- ca_pem=dest_ca_pem)
-
- (fin_resu, dresults) = helper.RemoteExport(opts, self.dest_disk_info,
+ (fin_resu, dresults) = helper.RemoteExport(self.dest_disk_info,
+ key_name, dest_ca_pem,
timeouts)
finally:
helper.Cleanup()
diff --git a/lib/masterd/instance.py b/lib/masterd/instance.py
index 0a8d3a2..e5a549b 100644
--- a/lib/masterd/instance.py
+++ b/lib/masterd/instance.py
@@ -815,7 +815,7 @@ class ImportExportLoop:

class _TransferInstCbBase(ImportExportCbBase):
def __init__(self, lu, feedback_fn, instance, timeouts, src_node, src_cbs,
- dest_node, dest_ip, export_opts):
+ dest_node, dest_ip):
"""Initializes this class.

"""
@@ -829,7 +829,6 @@ class _TransferInstCbBase(ImportExportCbBase):
self.src_cbs = src_cbs
self.dest_node = dest_node
self.dest_ip = dest_ip
- self.export_opts = export_opts

class _TransferInstSourceCb(_TransferInstCbBase):
@@ -888,11 +887,12 @@ class _TransferInstDestCb(_TransferInstCbBase):
assert self.src_cbs
assert dtp.src_export is None
assert dtp.dest_import
+ assert dtp.export_opts

self.feedback_fn("%s is now listening, starting export" % dtp.data.name)

# Start export on source node
- de = DiskExport(self.lu, self.src_node, self.export_opts,
+ de = DiskExport(self.lu, self.src_node, dtp.export_opts,
self.dest_ip, ie.listen_port,
self.instance, dtp.data.src_io, dtp.data.src_ioargs,
self.timeouts, self.src_cbs, private=dtp)
@@ -952,7 +952,7 @@ class DiskTransfer(object):

class _DiskTransferPrivate(object):
- def __init__(self, data, success):
+ def __init__(self, data, success, export_opts):
"""Initializes this class.

@type data: L{DiskTransfer}
@@ -960,12 +960,12 @@ class _DiskTransferPrivate(object):

"""
self.data = data
+ self.success = success
+ self.export_opts = export_opts

self.src_export = None
self.dest_import = None

- self.success = success
-
def RecordResult(self, success):
"""Updates the status.

@@ -975,6 +975,25 @@ class _DiskTransferPrivate(object):
self.success = self.success and success

+def _GetInstDiskMagic(base, instance_name, index):
+ """Computes the magic value for a disk export or import.
+
+ @type base: string
+ @param base: Random seed value (can be the same for all disks of a transfer)
+ @type instance_name: string
+ @param instance_name: Name of instance
+ @type index: number
+ @param index: Disk index
+
+ """
+ h = compat.sha1_hash()
+ h.update(str(constants.RIE_VERSION))
+ h.update(base)
+ h.update(instance_name)
+ h.update(str(index))
+ return h.hexdigest()
+
+

def TransferInstanceData(lu, feedback_fn, src_node, dest_node, dest_ip,

instance, all_transfers):
"""Transfers an instance's data from one node to another.
@@ -1002,25 +1021,28 @@ def TransferInstanceData(lu, feedback_fn, src_node, dest_node, dest_ip,

logging.debug("Source node %s, destination node %s, compression '%s'",
src_node, dest_node, compress)

- opts = objects.ImportExportOptions(key_name=None, ca_pem=None,
- compress=compress)
-
timeouts = ImportExportTimeouts(constants.DISK_TRANSFER_CONNECT_TIMEOUT)
src_cbs = _TransferInstSourceCb(lu, feedback_fn, instance, timeouts,
- src_node, None, dest_node, dest_ip, opts)
+ src_node, None, dest_node, dest_ip)
dest_cbs = _TransferInstDestCb(lu, feedback_fn, instance, timeouts,
- src_node, src_cbs, dest_node, dest_ip, opts)
+ src_node, src_cbs, dest_node, dest_ip)

all_dtp = []

+ base_magic = utils.GenerateSecret(6)
+
ieloop = ImportExportLoop(lu)
try:
- for transfer in all_transfers:
+ for idx, transfer in enumerate(all_transfers):
if transfer:
feedback_fn("Exporting %s from %s to %s" %
(transfer.name, src_node, dest_node))

- dtp = _DiskTransferPrivate(transfer, True)
+ magic = _GetInstDiskMagic(base_magic, instance.name, idx)
+ opts = objects.ImportExportOptions(key_name=None, ca_pem=None,
+ compress=compress, magic=magic)
+
+ dtp = _DiskTransferPrivate(transfer, True, opts)

di = DiskImport(lu, dest_node, opts, instance,
transfer.dest_io, transfer.dest_ioargs,
@@ -1224,13 +1246,15 @@ class ExportInstanceHelper:

return (fin_resu, dresults)

- def RemoteExport(self, opts, disk_info, timeouts):
+ def RemoteExport(self, disk_info, key_name, dest_ca_pem, timeouts):
"""Inter-cluster instance export.

- @type opts: L{objects.ImportExportOptions}
- @param opts: Import/export daemon options
@type disk_info: list
@param disk_info: Per-disk destination information
+ @type key_name: string
+ @param key_name: Name of X509 key to use
+ @type dest_ca_pem: string
+ @param dest_ca_pem: Destination X509 CA in PEM format
@type timeouts: L{ImportExportTimeouts}
@param timeouts: Timeouts for this import

@@ -1243,8 +1267,12 @@ class ExportInstanceHelper:

ieloop = ImportExportLoop(self._lu)
try:
- for idx, (dev, (host, port)) in enumerate(zip(instance.disks,
- disk_info)):
+ for idx, (dev, (host, port, magic)) in enumerate(zip(instance.disks,
+ disk_info)):
+ opts = objects.ImportExportOptions(key_name=key_name,
+ ca_pem=dest_ca_pem,
+ magic=magic)
+
self._feedback_fn("Sending disk %s to %s:%s" % (idx, host, port))
finished_fn = compat.partial(self._TransferFinished, idx)
ieloop.Add(DiskExport(self._lu, instance.primary_node,
@@ -1323,9 +1351,9 @@ class _RemoteImportCb(ImportExportCbBase):
host = self._external_address

disks = []
- for idx, port in enumerate(self._daemon_port):
+ for idx, (port, magic) in enumerate(self._daemon_port):
disks.append(ComputeRemoteImportDiskInfo(self._cds, self._salt,
- idx, host, port))
+ idx, host, port, magic))

assert len(disks) == self._disk_count

@@ -1344,7 +1372,7 @@ class _RemoteImportCb(ImportExportCbBase):

assert self._daemon_port[idx] is None

- self._daemon_port[idx] = ie.listen_port
+ self._daemon_port[idx] = (ie.listen_port, ie.magic)

self._CheckAllListening()

@@ -1393,6 +1421,8 @@ def RemoteImport(lu, feedback_fn, instance, source_x509_ca, cds, timeouts):
source_ca_pem = OpenSSL.crypto.dump_certificate(OpenSSL.crypto.FILETYPE_PEM,
source_x509_ca)

+ magic_base = utils.GenerateSecret(6)
+
# Create crypto key
result = lu.rpc.call_x509_cert_create(instance.primary_node,
constants.RIE_CERT_VALIDITY)
@@ -1404,10 +1434,6 @@ def RemoteImport(lu, feedback_fn, instance, source_x509_ca, cds, timeouts):
x509_cert = OpenSSL.crypto.load_certificate(OpenSSL.crypto.FILETYPE_PEM,
x509_cert_pem)

- # Import daemon options
- opts = objects.ImportExportOptions(key_name=x509_key_name,
- ca_pem=source_ca_pem)
-
# Sign certificate
signed_x509_cert_pem = \
utils.SignX509Certificate(x509_cert, cds, utils.GenerateSecret(8))
@@ -1418,6 +1444,13 @@ def RemoteImport(lu, feedback_fn, instance, source_x509_ca, cds, timeouts):
ieloop = ImportExportLoop(lu)
try:
for idx, dev in enumerate(instance.disks):
+ magic = _GetInstDiskMagic(magic_base, instance.name, idx)
+
+ # Import daemon options
+ opts = objects.ImportExportOptions(key_name=x509_key_name,
+ ca_pem=source_ca_pem,
+ magic=magic)
+
ieloop.Add(DiskImport(lu, instance.primary_node, opts, instance,
constants.IEIO_SCRIPT, (dev, idx),
timeouts, cbs, private=(idx, )))
@@ -1480,7 +1513,7 @@ def CheckRemoteExportHandshake(cds, handshake):
return None

-def _GetRieDiskInfoMessage(disk_index, host, port):
+def _GetRieDiskInfoMessage(disk_index, host, port, magic):
"""Returns the hashed text for import/export disk information.

@type disk_index: number
@@ -1489,9 +1522,11 @@ def _GetRieDiskInfoMessage(disk_index, host, port):
@param host: Hostname
@type port: number
@param port: Daemon port
+ @type magic: string
+ @param magic: Magic value

"""
- return "%s:%s:%s" % (disk_index, host, port)
+ return "%s:%s:%s:%s" % (disk_index, host, port, magic)

def CheckRemoteExportDiskInfo(cds, disk_index, disk_info):
@@ -1506,23 +1541,24 @@ def CheckRemoteExportDiskInfo(cds, disk_index, disk_info):

"""
try:
- (host, port, hmac_digest, hmac_salt) = disk_info
+ (host, port, magic, hmac_digest, hmac_salt) = disk_info
except (TypeError, ValueError), err:
raise errors.GenericError("Invalid data: %s" % err)

- if not (host and port):
- raise errors.GenericError("Missing destination host or port")
+ if not (host and port and magic):
+ raise errors.GenericError("Missing destination host, port or magic")

- msg = _GetRieDiskInfoMessage(disk_index, host, port)
+ msg = _GetRieDiskInfoMessage(disk_index, host, port, magic)

if not utils.VerifySha1Hmac(cds, msg, hmac_digest, salt=hmac_salt):
raise errors.GenericError("HMAC is wrong")

return (utils.HostInfo.NormalizeName(host),
- utils.ValidatePort(port))
+ utils.ValidatePort(port),
+ magic)

-def ComputeRemoteImportDiskInfo(cds, salt, disk_index, host, port):
+def ComputeRemoteImportDiskInfo(cds, salt, disk_index, host, port, magic):
"""Computes the signed disk information for a remote import.

@type cds: string
@@ -1535,8 +1571,10 @@ def ComputeRemoteImportDiskInfo(cds, salt, disk_index, host, port):
@param host: Hostname
@type port: number
@param port: Daemon port
+ @type magic: string
+ @param magic: Magic value

"""
- msg = _GetRieDiskInfoMessage(disk_index, host, port)
+ msg = _GetRieDiskInfoMessage(disk_index, host, port, magic)
hmac_digest = utils.Sha1Hmac(cds, msg, salt=salt)
- return (host, port, hmac_digest, salt)
+ return (host, port, magic, hmac_digest, salt)
diff --git a/test/ganeti.masterd.instance_unittest.py b/test/ganeti.masterd.instance_unittest.py
index 2daa1a4..28a799e 100755
--- a/test/ganeti.masterd.instance_unittest.py
+++ b/test/ganeti.masterd.instance_unittest.py
@@ -102,9 +102,9 @@ class TestRieDiskInfo(unittest.TestCase):
def test(self):
cds = "bbf46ea9a"
salt = "ee5ad9"
- di = ComputeRemoteImportDiskInfo(cds, salt, 0, "node1", 1234)
+ di = ComputeRemoteImportDiskInfo(cds, salt, 0, "node1", 1234, "mag111")
self.assertEqual(CheckRemoteExportDiskInfo(cds, 0, di),
- ("node1", 1234))
+ ("node1", 1234, "mag111"))

for i in range(1, 100):
# Wrong disk index
@@ -116,12 +116,12 @@ class TestRieDiskInfo(unittest.TestCase):
salt = "drK5oYiHWD"

for host in [",", "...", "Hello World", "`", "!", "#", "\\"]:
- di = ComputeRemoteImportDiskInfo(cds, salt, 0, host, 1234)
+ di = ComputeRemoteImportDiskInfo(cds, salt, 0, host, 1234, "magic")
self.assertRaises(errors.OpPrereqError,
CheckRemoteExportDiskInfo, cds, 0, di)

for port in [-1, 792825908, "HelloWorld!", "`#", "\\\"", "_?_"]:
- di = ComputeRemoteImportDiskInfo(cds, salt, 0, "localhost", port)
+ di = ComputeRemoteImportDiskInfo(cds, salt, 0, "localhost", port, "magic")
self.assertRaises(errors.OpPrereqError,
CheckRemoteExportDiskInfo, cds, 0, di)

@@ -136,11 +136,15 @@ class TestRieDiskInfo(unittest.TestCase):

# No host/port
self.assertRaises(errors.GenericError, CheckRemoteExportDiskInfo,
- cds, 0, ("", 0, "", ""))
+ cds, 0, ("", 1234, "magic", "", ""))
+ self.assertRaises(errors.GenericError, CheckRemoteExportDiskInfo,
+ cds, 0, ("host", 0, "magic", "", ""))
+ self.assertRaises(errors.GenericError, CheckRemoteExportDiskInfo,
+ cds, 0, ("host", 1234, "", "", ""))

# Wrong hash
self.assertRaises(errors.GenericError, CheckRemoteExportDiskInfo,
- cds, 0, ("nodeX", 123, "fakehash", "xyz"))
+ cds, 0, ("nodeX", 123, "magic", "fakehash", "xyz"))

class TestFormatProgress(unittest.TestCase):
--
1.7.0.4

Guido Trotter

unread,

Jun 14, 2010, 5:49:39 AM6/14/10

to Michael Hanselmann, ganeti...@googlegroups.com

On Fri, Jun 11, 2010 at 6:14 PM, Michael Hanselmann <han...@google.com> wrote:

Hi,

How are these all valid TCP/UDP ports? I think this function behaves
in a quite unexpected way.
Should we rather reject strings? Or do a services lookup if the port
is a string? (perhaps improving/integrating with the other utils
function that does that)

thanks,

Guido

Michael Hanselmann

unread,

Jun 14, 2010, 6:43:20 AM6/14/10

to Guido Trotter, ganeti...@googlegroups.com

Am 14. Juni 2010 10:49 schrieb Guido Trotter <ultr...@google.com>:
>> --- a/test/ganeti.utils_unittest.py
>> +++ b/test/ganeti.utils_unittest.py

>> +class TestValidatePort(unittest.TestCase):
>> + def testValid(self):
>> + testports = [
>> + 0, 1, 2, 3, 1024, 65000, 65534, 65535,
>> + "ganeti",
>> + "gnt-masterd",
>> + "HELLO_WORLD_SVC",
>> + "hello.world.1",
>> + "0", "80", "1111", "65535",
>
> How are these all valid TCP/UDP ports? I think this function behaves
> in a quite unexpected way.

The function never specifically says TCP/UDP (even though other
protocols, as mentioned in a comment, might need small adjustments).
For the usecase of this function, verifying ports for socat, all those
ports are valid (whether they're actually used is another thing).
Please check the services(5) manpage if you want to know more about
service names. The main point of this function is to disallow
potentially malicious ports (e.g. "1234`rm -rf /`") in case there's a
bug in the shell quoting.

Michael

Guido Trotter

unread,

Jun 14, 2010, 7:01:57 AM6/14/10

to Michael Hanselmann, ganeti...@googlegroups.com

Ok, then can we please call it ValidateService or ValidateServiceName
? (which is ok if it's just a port number).
Using "port" might lead to confusion.

Thanks,
Guido

Michael Hanselmann

unread,

Jun 14, 2010, 7:28:20 AM6/14/10

to ganeti...@googlegroups.com

The hostname and port received from the remote cluster should
be validated, just in case.
---
daemons/import-export | 11 +++++++++++
lib/cmdlib.py | 12 ++++++++++--
lib/masterd/instance.py | 7 ++++---
test/ganeti.masterd.instance_unittest.py | 14 ++++++++++++++
test/import-export_unittest.bash | 17 +++++++++++++++--
5 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/daemons/import-export b/daemons/import-export
index d0444f5..9fdd8fb 100755
--- a/daemons/import-export
+++ b/daemons/import-export
@@ -40,6 +40,7 @@ import math
from ganeti import constants
from ganeti import cli
from ganeti import utils
+from ganeti import errors
from ganeti import serializer
from ganeti import objects
from ganeti import locking
@@ -401,6 +402,16 @@ def ParseOptions():
# Won't return
parser.error("Invalid mode: %s" % mode)

+ # Normalize and check parameters
+ if options.host is not None:
+ try:
+ options.host = utils.HostInfo.NormalizeName(options.host)
+ except errors.OpPrereqError, err:
+ parser.error("Invalid hostname '%s': %s" % (options.host, err))
+
+ if options.port is not None:
+ options.port = utils.ValidatePort(options.port)
+
if (options.exp_size is not None and
options.exp_size != constants.IE_CUSTOM_SIZE):
try:
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index ab1adb2..8dbb1de 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -9100,6 +9100,7 @@ class LUExportInstance(LogicalUnit):
_CheckNodeNotDrained(self, self.dst_node.name)

self._cds = None
+ self.dest_disk_info = None
self.dest_x509_ca = None

elif self.export_mode == constants.EXPORT_MODE_REMOTE:
@@ -9139,13 +9140,20 @@ class LUExportInstance(LogicalUnit):
self.dest_x509_ca = cert

# Verify target information
+ disk_info = []

for idx, disk_data in enumerate(self.op.target_node):
try:

- masterd.instance.CheckRemoteExportDiskInfo(cds, idx, disk_data)
+ (host, port) = masterd.instance.CheckRemoteExportDiskInfo(cds, idx,
+ disk_data)

except errors.GenericError, err:
raise errors.OpPrereqError("Target info for disk %s: %s" % (idx, err),
errors.ECODE_INVAL)

+ disk_info.append((host, port))
+
+ assert len(disk_info) == len(self.op.target_node)
+ self.dest_disk_info = disk_info
+
else:
raise errors.ProgrammerError("Unhandled export mode %r" %
self.export_mode)
@@ -9236,7 +9244,7 @@ class LUExportInstance(LogicalUnit):
opts = objects.ImportExportOptions(key_name=key_name,
ca_pem=dest_ca_pem)

- (fin_resu, dresults) = helper.RemoteExport(opts, self.op.target_node,
+ (fin_resu, dresults) = helper.RemoteExport(opts, self.dest_disk_info,

timeouts)
finally:
helper.Cleanup()
diff --git a/lib/masterd/instance.py b/lib/masterd/instance.py

index 04806da..f80c6bb 100644
--- a/lib/masterd/instance.py
+++ b/lib/masterd/instance.py
@@ -1239,8 +1239,8 @@ class ExportInstanceHelper:

ieloop = ImportExportLoop(self._lu)
try:
- for idx, (dev, (host, port, _, _)) in enumerate(zip(instance.disks,
- disk_info)):
+ for idx, (dev, (host, port)) in enumerate(zip(instance.disks,
+ disk_info)):

self._feedback_fn("Sending disk %s to %s:%s" % (idx, host, port))
finished_fn = compat.partial(self._TransferFinished, idx)
ieloop.Add(DiskExport(self._lu, instance.primary_node,

@@ -1514,7 +1514,8 @@ def CheckRemoteExportDiskInfo(cds, disk_index, disk_info):

if not utils.VerifySha1Hmac(cds, msg, hmac_digest, salt=hmac_salt):
raise errors.GenericError("HMAC is wrong")

- return (host, port)
+ return (utils.HostInfo.NormalizeName(host),
+ utils.ValidatePort(port))

def ComputeRemoteImportDiskInfo(cds, salt, disk_index, host, port):

diff --git a/test/ganeti.masterd.instance_unittest.py b/test/ganeti.masterd.instance_unittest.py
index 066da7d..2daa1a4 100755
--- a/test/ganeti.masterd.instance_unittest.py
+++ b/test/ganeti.masterd.instance_unittest.py
@@ -111,6 +111,20 @@ class TestRieDiskInfo(unittest.TestCase):
self.assertRaises(errors.GenericError, CheckRemoteExportDiskInfo,
cds, i, di)

+ def testInvalidHostPort(self):
+ cds = "3ZoJY8KtGJ"
+ salt = "drK5oYiHWD"
+
+ for host in [",", "...", "Hello World", "`", "!", "#", "\\"]:
+ di = ComputeRemoteImportDiskInfo(cds, salt, 0, host, 1234)
+ self.assertRaises(errors.OpPrereqError,
+ CheckRemoteExportDiskInfo, cds, 0, di)
+
+ for port in [-1, 792825908, "HelloWorld!", "`#", "\\\"", "_?_"]:
+ di = ComputeRemoteImportDiskInfo(cds, salt, 0, "localhost", port)
+ self.assertRaises(errors.OpPrereqError,
+ CheckRemoteExportDiskInfo, cds, 0, di)
+
def testCheckErrors(self):
cds = "0776450535a"
self.assertRaises(errors.GenericError, CheckRemoteExportDiskInfo,
diff --git a/test/import-export_unittest.bash b/test/import-export_unittest.bash
index 96a055f..bbb5e02 100755
--- a/test/import-export_unittest.bash
+++ b/test/import-export_unittest.bash
@@ -108,8 +108,21 @@ $impexpd $src_statusfile >/dev/null 2>&1 &&
$impexpd $src_statusfile invalidmode >/dev/null 2>&1 &&
err "daemon-util succeeded with invalid mode"

-$impexpd $src_statusfile import --compression=rot13 >/dev/null 2>&1 &&
- err "daemon-util succeeded with invalid compression"
+for mode in import export; do
+ $impexpd $src_statusfile $mode --compression=rot13 >/dev/null 2>&1 &&
+ err "daemon-util succeeded with invalid compression"
+
+ for host in '' ' ' ' s p a c e' ... , foo.example.net... \
+ 'some"evil"name' 'x\ny\tmoo'; do
+ $impexpd $src_statusfile $mode --host="$host" >/dev/null 2>&1 &&
+ err "daemon-util succeeded with invalid host '$host'"
+ done
+
+ for port in '' ' ' -1234 'some ` port " here'; do
+ $impexpd $src_statusfile $mode --port="$port" >/dev/null 2>&1 &&
+ err "daemon-util succeeded with invalid port '$port'"
+ done
+done

upto 'Generate test data'
cat $(get_testfile proc_drbd8.txt) $(get_testfile cert1.pem) > $testdata
--
1.7.0.4

Guido Trotter

unread,

Jun 14, 2010, 8:08:36 AM6/14/10

to Michael Hanselmann, ganeti...@googlegroups.com

On Mon, Jun 14, 2010 at 12:28 PM, Michael Hanselmann <han...@google.com> wrote:
> The hostname and port received from the remote cluster should
> be validated, just in case.

This is not the same patch, did something happen?
Also, small question: if the remote cluster passes a service name, how
can we guarantee it exists on the local cluster?
Should we look it up in the services db? Or should we enforce a
numeric port to be passed?

thanks,

Guido

Michael Hanselmann

unread,

Jun 14, 2010, 8:18:17 AM6/14/10

to Guido Trotter, ganeti...@googlegroups.com

Am 14. Juni 2010 13:08 schrieb Guido Trotter <ultr...@google.com>:
> On Mon, Jun 14, 2010 at 12:28 PM, Michael Hanselmann <han...@google.com> wrote:
>> The hostname and port received from the remote cluster should
>> be validated, just in case.
>
> This is not the same patch, did something happen?

Yes, something must've gone wrong, sorry. Will resend the correct patch.

> Also, small question: if the remote cluster passes a service name, how
> can we guarantee it exists on the local cluster?
> Should we look it up in the services db? Or should we enforce a
> numeric port to be passed?

They get what they ask for: breakage. Don't try to overdesign this
function. It's simply validation and so far the port is always taken
from the import-export daemon (which takes it from socat) and sent to
the remote cluster.

Michael

Michael Hanselmann

unread,

Jun 14, 2010, 8:19:07 AM6/14/10

to ganeti...@googlegroups.com

---
lib/utils.py | 26 ++++++++++++++++++++++++++
test/ganeti.utils_unittest.py | 26 ++++++++++++++++++++++++++
2 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/lib/utils.py b/lib/utils.py
index 49b5bc9..809489e 100644

--- a/lib/utils.py
+++ b/lib/utils.py
@@ -81,6 +81,8 @@ X509_SIGNATURE = re.compile(r"^%s:\s*(?P<salt>%s+)/(?P<sign>%s+)$" %
HEX_CHAR_RE, HEX_CHAR_RE),
re.S | re.I)

+_VALID_SERVICE_NAME_RE = re.compile("^[-_.a-zA-Z0-9]{1,128}$")

+
# Structure definition for getsockopt(SOL_SOCKET, SO_PEERCRED, ...):
# struct ucred { pid_t pid; uid_t uid; gid_t gid; };
#

@@ -1155,6 +1157,30 @@ class HostInfo:
return hostname

+def ValidateServiceName(name):
+ """Validate the given service name.

+
+ @type port: number or string
+ @param port: Port specification
+
+ """
+ try:

+ numport = int(name)
+ except (ValueError, TypeError):
+ # Non-numeric service name
+ valid = _VALID_SERVICE_NAME_RE.match(name)

+ else:
+ # Numeric port (protocols other than TCP or UDP might need adjustments
+ # here)
+ valid = (numport >= 0 and numport < (1 << 16))
+
+ if not valid:

+ raise errors.OpPrereqError("Invalid service name '%s'" % name,
+ errors.ECODE_INVAL)
+
+ return name

+
+
def GetHostInfo(name=None):
"""Lookup host name and raise an OpPrereqError for failures"""

diff --git a/test/ganeti.utils_unittest.py b/test/ganeti.utils_unittest.py

index 9955d3b..f679341 100755

--- a/test/ganeti.utils_unittest.py
+++ b/test/ganeti.utils_unittest.py
@@ -1903,6 +1903,32 @@ class TestHostInfo(unittest.TestCase):
HostInfo.NormalizeName(value)

+class TestValidateServiceName(unittest.TestCase):
+ def testValid(self):
+ testnames = [

+ 0, 1, 2, 3, 1024, 65000, 65534, 65535,
+ "ganeti",
+ "gnt-masterd",
+ "HELLO_WORLD_SVC",
+ "hello.world.1",
+ "0", "80", "1111", "65535",

+ ]
+
+ for name in testnames:
+ self.assertEqual(utils.ValidateServiceName(name), name)
+
+ def testInvalid(self):
+ testnames = [

+ -15756, -1, 65536, 133428083,
+ "", "Hello World!", "!", "'", "\"", "\t", "\n", "`",
+ "-8546", "-1", "65536",
+ (129 * "A"),
+ ]
+

+ for name in testnames:
+ self.assertRaises(OpPrereqError, utils.ValidateServiceName, name)

Manuel Franceschini

unread,

Jun 14, 2010, 9:06:46 AM6/14/10

to Michael Hanselmann, ganeti...@googlegroups.com

On Mon, Jun 14, 2010 at 2:19 PM, Michael Hanselmann <han...@google.com> wrote:
> ---
> lib/utils.py | 26 ++++++++++++++++++++++++++
> test/ganeti.utils_unittest.py | 26 ++++++++++++++++++++++++++
> 2 files changed, 52 insertions(+), 0 deletions(-)
>
> diff --git a/lib/utils.py b/lib/utils.py
> index 49b5bc9..809489e 100644
> --- a/lib/utils.py
> +++ b/lib/utils.py
> @@ -81,6 +81,8 @@ X509_SIGNATURE = re.compile(r"^%s:\s*(?P<salt>%s+)/(?P<sign>%s+)$" %
> HEX_CHAR_RE, HEX_CHAR_RE),
> re.S | re.I)
>
> +_VALID_SERVICE_NAME_RE = re.compile("^[-_.a-zA-Z0-9]{1,128}$")
> +
> # Structure definition for getsockopt(SOL_SOCKET, SO_PEERCRED, ...):
> # struct ucred { pid_t pid; uid_t uid; gid_t gid; };
> #
> @@ -1155,6 +1157,30 @@ class HostInfo:
> return hostname
>
>
> +def ValidateServiceName(name):
> + """Validate the given service name.
> +
> + @type port: number or string
> + @param port: Port specification

s/port/name/

Cheers, Manuel.

Guido Trotter

unread,

Jun 14, 2010, 9:30:58 AM6/14/10

to Michael Hanselmann, ganeti...@googlegroups.com

That's the point, so it's always an integer, now, correct? Wouldn't
requiring an integer inside the port numbers range make things
simpler, and avoid overdesign even more?
Is there any case where a string is needed, currently, or where you
foresee a string being used in the very near future?

Guido

Michael Hanselmann

unread,

Jun 14, 2010, 9:49:38 AM6/14/10

to Guido Trotter, ganeti...@googlegroups.com

Am 14. Juni 2010 14:30 schrieb Guido Trotter <ultr...@google.com>:
> That's the point, so it's always an integer, now, correct? Wouldn't
> requiring an integer inside the port numbers range make things
> simpler, and avoid overdesign even more?

So instead of having a generic function (which is already written
including unittests) which can be used to verify all service names or
ports, you'd rather have a very restricted test which can not be used
with names and spend more time to reduce functionality again?

> Is there any case where a string is needed, currently, or where you
> foresee a string being used in the very near future?

The import-export is quite flexible and could be used for other tasks.
Currently it only accepts an integer as the port.

In case it's not obvious: This function is not restricted for use in
import/export. If you want, you can write a unittest to check
constants.DAEMONS_PORTS with it …

Michael

Guido Trotter

unread,

Jun 14, 2010, 10:00:19 AM6/14/10

to Michael Hanselmann, ganeti...@googlegroups.com

On Mon, Jun 14, 2010 at 2:49 PM, Michael Hanselmann <han...@google.com> wrote:
>
> So instead of having a generic function (which is already written
> including unittests) which can be used to verify all service names or
> ports, you'd rather have a very restricted test which can not be used
> with names and spend more time to reduce functionality again?
>

Yes, I'm starting to prefer simple code that does exactly what we need
to do instead of generic code that does what we need plus other things
which aren't quite needed but you never know.
Especially since by the time someone will get to use them, most of the
time we'd have forgotten about them anyway.

>> Is there any case where a string is needed, currently, or where you
>> foresee a string being used in the very near future?
>
> The import-export is quite flexible and could be used for other tasks.

Do we plan to use it for other tasks? If so, which? If not, perhaps we
should have built it less flexible and simpler, if possible! :)
But this is not related to the patch at hand, so let's not get into
it. If you resent with the changed name the patch is LGTM, for me,
although I thought that just passing/verifying a port was more than
enough. :)

> In case it's not obvious: This function is not restricted for use in
> import/export. If you want, you can write a unittest to check
> constants.DAEMONS_PORTS with it …

It's clear it can be used that way. Not very useful, but clear. :)

Thanks,

Guido

Michael Hanselmann

unread,

Jun 14, 2010, 10:05:32 AM6/14/10

to Manuel Franceschini, ganeti...@googlegroups.com

Am 14. Juni 2010 14:06 schrieb Manuel Franceschini <live...@google.com>:
> On Mon, Jun 14, 2010 at 2:19 PM, Michael Hanselmann <han...@google.com> wrote:
>> +def ValidateServiceName(name):
>> + """Validate the given service name.
>> +
>> + @type port: number or string
>> + @param port: Port specification
>
> s/port/name/

Done:

--- a/lib/utils.py
+++ b/lib/utils.py
@@ -1160,8 +1160,8 @@ class HostInfo:
def ValidateServiceName(name):

"""Validate the given service name.

- @type port: number or string
- @param port: Port specification
+ @type name: number or string
+ @param name: Service name or port specification

"""
try:

Michael

Guido Trotter

unread,

Jun 14, 2010, 10:26:52 AM6/14/10

to Michael Hanselmann, ganeti...@googlegroups.com

On Fri, Jun 11, 2010 at 6:14 PM, Michael Hanselmann <han...@google.com> wrote:

LGTM

Although, should it be optional, or should we enable it by default?

Thanks,

Guido

Guido Trotter

unread,

Jun 14, 2010, 10:29:47 AM6/14/10

to Michael Hanselmann, ganeti...@googlegroups.com

On Fri, Jun 11, 2010 at 6:14 PM, Michael Hanselmann <han...@google.com> wrote:

> Signed-off-by: Michael Hanselmann <han...@google.com>

LGTM

Thanks,
Guido

Guido Trotter

unread,

Jun 14, 2010, 10:31:19 AM6/14/10

to Michael Hanselmann, ganeti...@googlegroups.com

On Fri, Jun 11, 2010 at 6:14 PM, Michael Hanselmann <han...@google.com> wrote:

> Tests have shown that usually we're CPU-bound for intra-cluster
> imports/exports. Disabling compression will help with this.
>
> Some versions of OpenSSL, depending on the build options, also
> compress transparently. This will need further work in Ganeti.

LGTM

Thanks,

Guido

Guido Trotter

unread,

Jun 14, 2010, 10:32:42 AM6/14/10

to Michael Hanselmann, ganeti...@googlegroups.com

On Fri, Jun 11, 2010 at 6:14 PM, Michael Hanselmann <han...@google.com> wrote:

> This should prevent bugs in our code from accidentally overwriting
> disks.

LGTM

Thanks,

Guido

Michael Hanselmann

unread,

Jun 14, 2010, 10:39:34 AM6/14/10

to Guido Trotter, ganeti...@googlegroups.com

Am 14. Juni 2010 15:26 schrieb Guido Trotter <ultr...@google.com>:
> Although, should it be optional, or should we enable it by default?

The magic value must be generated by the calling code (which controls
boths sides of an import/export), hence it cannot really be enabled by
default. After this patch series it's used by all imports/exports.

Michael

Guido Trotter

unread,

Jun 14, 2010, 11:21:25 AM6/14/10

to Michael Hanselmann, ganeti...@googlegroups.com

Ah, ok, fair enough

Thanks,

Guido

Reply all

Reply to author

Forward