Nir Soffer has posted comments on this change. Change subject: ceph: Support ephemeral Libvirt secrets ......................................................................
Patch Set 27: (9 comments) https://gerrit.ovirt.org/#/c/40712/27/tests/vmSecretTests.py File tests/vmSecretTests.py: Line 185: Line 186: def test_register_libvirt_error(self): Line 187: def fail(xml): Line 188: raise vmfakelib.Error(libvirt.VIR_ERR_INTERNAL_ERROR) Line 189: self.connection.secretDefineXML = fail > why don't you use monkeypatch for these settings? Because it is not needed. self.connection is created in setUp(), each test is using a new connection object. Line 190: res = secret.register([make_secret()]) Line 191: self.assertEqual(res, response.error("secretRegisterErr")) Line 192: Line 193: def test_register_unexpected_error(self): https://gerrit.ovirt.org/#/c/40712/27/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 3996: # Line 3997: # @usageID: Specifies a unique usage id for the current usage type, as Line 3998: # documented in https://libvirt.org/formatsecret.html Line 3999: # Line 4000: # @password: Base64 encoded value of this secret > Why do we want to use base64? our transports {json,xml}rpc are already capa This is the way ceph keys are generated by ceph, and stored in the engine db. We can decoded them at the engine side and send the binary data, but I don't see any advantage. Line 4001: # Line 4002: # @description: #optional A human-readable description of this secret Line 4003: # Line 4004: # Since: 4.17.0 https://gerrit.ovirt.org/#/c/40712/27/vdsm/virt/secret.py File vdsm/virt/secret.py: Line 36: logging.warning("Attempt to register invalid secret: %s", e) Line 37: return response.error("secretBadRequestErr") Line 38: Line 39: try: Line 40: con = libvirtconnection.get() > please take it out of the try-block. get() can end up raising a libvirtErro Right, will fix in next version. Line 41: for secret in secrets: Line 42: logging.info("Registering secret %s", secret) Line 43: secret.register(con) Line 44: except libvirt.libvirtError as e: Line 64: virsecret = con.secretLookupByUUIDString(sec_uuid) Line 65: except libvirt.libvirtError as e: Line 66: if e.get_error_code() != libvirt.VIR_ERR_NO_SECRET: Line 67: raise Line 68: logging.debug("No such secret %r", sec_uuid) > Why do we swallow this error? If Engine requested to remove a secret that i No, we don't expect engine to know the what are the configured secrets on the machine. For example, engine could register some secrets, and then loose the reply from vdsm, so it can never know if the secrets are defined or not. But engine must have the power to remove the secrets after such event. Having a way to unregister the secrets engines knows about even if the host does not know about some of them is a must. Engine asked to remove the secrets, and when this function returns with success code, these secrets are removed. We log missing secrets to make it easier to debug engine management code - nothing is wallowed. Line 69: else: Line 70: virsecret.undefine() Line 71: except libvirt.libvirtError as e: Line 72: logging.error("Could not unregister secrets: %s", e) Line 68: logging.debug("No such secret %r", sec_uuid) Line 69: else: Line 70: virsecret.undefine() Line 71: except libvirt.libvirtError as e: Line 72: logging.error("Could not unregister secrets: %s", e) > there's a problem in this API - Engine cannot tell which secret(s) were suc Engine does not care about partial results, and we don't provide a way to list current secrets. Even if we did provide such api, engine cannot depend on it since the secret value is private to libvirt. If the operation failed, engine must retry until it succeeds. Line 73: return response.error("secretUnregisterErr") Line 74: Line 75: return response.success() Line 76: Line 96: virsecret = con.secretDefineXML(self.toxml()) Line 97: virsecret.setValue(self.password.value) Line 98: Line 99: def toxml(self): Line 100: secret = vmxml.Element('secret', ephemeral="yes", private="yes") > oh dear, more minidom dependencies. I can, will update the next patch. Line 101: if self.description: Line 102: secret.appendChildWithArgs('description', text=self.description) Line 103: secret.appendChildWithArgs('uuid', text=self.uuid) Line 104: usage = vmxml.Element('usage', type=self.usage_type) Line 97: virsecret.setValue(self.password.value) Line 98: Line 99: def toxml(self): Line 100: secret = vmxml.Element('secret', ephemeral="yes", private="yes") Line 101: if self.description: > Let us allow the empty string description, if someone was inclined to suppl Why should be care about empty description? I don't see any value in this element: <description/> Line 102: secret.appendChildWithArgs('description', text=self.description) Line 103: secret.appendChildWithArgs('uuid', text=self.uuid) Line 104: usage = vmxml.Element('usage', type=self.usage_type) Line 105: usage.appendChildWithArgs(self._USAGE_TYPES[self.usage_type], Line 106: text=self.usage_id) Line 107: secret.appendChild(usage) Line 108: return secret.toxml() Line 109: Line 110: def __str__(self): > why don't you use your own lovely password.ProtecedPassword? I'm using it, password is protected. Actually the comment is misleading, we can safely include the password, but it is not very helpful. Line 111: # Note: self.password is intentionally not displayed Line 112: return ("Secret(uuid={self.uuid}, " Line 113: "usage_type={self.usage_type}, " Line 114: "usage_id={self.usage_id}, " Line 135: return value Line 136: Line 137: Line 138: def _get_required(params, name): Line 139: value = params.get(name) > a "required" param can never have the value False? Right, will fix in next version. Line 140: if not value: Line 141: raise ValueError("Missing or empty required property %r" % name) -- 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: 27 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> Gerrit-Reviewer: Adam Litke <[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
