Nir Soffer has posted comments on this change. Change subject: ceph: Support ephemeral Libvirt secrets ......................................................................
Patch Set 8: (10 comments) https://gerrit.ovirt.org/#/c/40712/8/tests/vmSecretTests.py File tests/vmSecretTests.py: Line 1: # > the tests looks very nice and comprehensive. I want to keep the fakes simple as possible - even the stupid fakes you see here had a bug found only when writing the tests for the fakes. I will check the option to move the fakes to vmfakelib, and merge the fake connection with what we have there. Line 2: # Copyright 2015 Red Hat, Inc. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 55: raise FakeLibvirtError(libvirt.VIR_ERR_NO_SECRET) Line 56: return self.secrets[sec_uuid] Line 57: Line 58: Line 59: class FakeLibvitSecret(object): > typo: should be libvirt Will fix Line 60: Line 61: def __init__(self, con, uuid, xml): Line 62: self.con = con Line 63: self.uuid = uuid https://gerrit.ovirt.org/#/c/40712/8/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 3896: # @key: The authentication key for this secret Line 3897: # Line 3898: # @ephemeral: #optional This secret must only be kept in memory, never Line 3899: # stored persistently Line 3900: # (True if not specfied) > Do we have cases on which we DON'T want an ephemeral secret? No, Daniel and me decided to drop this option - we don't need to support it and we don't want to. Line 3901: # Line 3902: # @private: #optional The value of the secret must not be revealed to any Line 3903: # caller of libvirt, nor to any other node Line 3904: # (True if not specified) Line 3900: # (True if not specfied) Line 3901: # Line 3902: # @private: #optional The value of the secret must not be revealed to any Line 3903: # caller of libvirt, nor to any other node Line 3904: # (True if not specified) > Same, do we have some clients which DON'T want a private secret? Same, will be removed in next version. Line 3905: # Line 3906: # @description: #optional A human-readable description of this secret Line 3907: # Line 3908: # Since: 4.17.0 https://gerrit.ovirt.org/#/c/40712/8/vdsm/virt/secret.py File vdsm/virt/secret.py: Line 1: # > this file should be added to vdsm.spec.in Will add Line 2: # Copyright 2015 Red Hat, Inc. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 45: logging.error("Could not register secret %s: %s", secret, e) Line 46: return response.error("secretRegisterErr") Line 47: Line 48: # TODO: Use response.success when it is available Line 49: return {'status': doneCode} > response.success() was recently (earlier this day!) merged Great, will replace. Line 50: Line 51: Line 52: def unregister(uuids): Line 53: try: Line 48: # TODO: Use response.success when it is available Line 49: return {'status': doneCode} Line 50: Line 51: Line 52: def unregister(uuids): > It's probably worth to invoke unregister for all secrets on host upon vdsm I will add another internal function to remove ovirt secrets (name|volume|target starts match ovirt/uuid/uuid). We can call this on startup. Line 53: try: Line 54: uuids = [str(uuid.UUID(s)) for s in uuids] Line 55: except ValueError as e: Line 56: logging.warning("Attempt to unregister invalid uuid %s: %s" % Line 87: Line 88: def __init__(self, params): Line 89: self.uuid = str(uuid.UUID(_get_required(params, "uuid"))) Line 90: self.usage = SecretUsage(_get_required(params, "usage")) Line 91: self._key = _get_required(params, "key") > shouldn't we use "value" instead, to accommodate to libvirt's format (https Ok, lets change the dictionary key also to value. Line 92: self.ephemeral = params.get("ephemeral", True) Line 93: self.private = params.get("private", True) Line 94: self.description = params.get("description") Line 95: Line 88: def __init__(self, params): Line 89: self.uuid = str(uuid.UUID(_get_required(params, "uuid"))) Line 90: self.usage = SecretUsage(_get_required(params, "usage")) Line 91: self._key = _get_required(params, "key") Line 92: self.ephemeral = params.get("ephemeral", True) > I guess we can drop these for now and just use the default values ('yes'), Dropping in next version. Line 93: self.private = params.get("private", True) Line 94: self.description = params.get("description") Line 95: Line 96: def register(self, con): Line 144: Line 145: def _get_required(params, name): Line 146: value = params.get(name) Line 147: if not value: Line 148: raise ValueError("Missing or empty required property %r" % name) > I'm under the impression that KeyError could be equally good and simpler. W All the validation functions and the builtin uuid.UUID() fail with ValueError. I want one error meaning invalid input, for reporting secretBadRequestErr in the api level. Also Key error is incorrect - I want to fail when the key exists but the value is empty. -- To view, visit https://gerrit.ovirt.org/40712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2e9ee33a7447ee07b0c82cf5a80d1f9b470663bb Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
