Dan Kenigsberg has posted comments on this change. Change subject: ceph: Support ephemeral Libvirt secrets ......................................................................
Patch Set 27: Code-Review-1 (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? Without it, this assignment is going to affect all following tests. Even if harmless at the moment, it's impolite for a test to leave side-effects. 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 capable of encoding strings. (I don't assume that the answer is obfuscation, as that does not help security, of course) 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 libvirtError with "secret" unassisgned 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 is not known to the host, it is most probably an Engine bug. 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 successfully removed, and which not (which kinda answers my question on line 68....) 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. We would like to stop linking against minidom and etree. Can you use etree here instead? after all, using vmxml for an xml of something that is NOT a vm is a bit awkward anyway. 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 supply it if self.description is not None 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? 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? I think that if value is not None is better fitting here. 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
