Change in vdsm[master]: ceph: Support ephemeral Libvirt secrets

danken at redhat.com danken at redhat.com
Thu Jun 25 10:33:58 UTC 2015


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 <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Daniel Erez <derez at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list