Change in vdsm[master]: vdsm-tool: roll out self signed certificates in vdsm-tool.

mtayer at redhat.com mtayer at redhat.com
Sun Aug 17 21:38:55 UTC 2014


mooli tayer has posted comments on this change.

Change subject: vdsm-tool: roll out self signed certificates in vdsm-tool.
......................................................................


Patch Set 1:

(8 comments)

http://gerrit.ovirt.org/#/c/31562/1/lib/vdsm/tool/configurators/certificates.py
File lib/vdsm/tool/configurators/certificates.py:

Line 45:     def getName(self):
Line 46:         return 'certificates'
Line 47: 
Line 48:     def getServices(self):
Line 49:         return []
> this should be at base, no need to override
Right! Done.
Line 50: 
Line 51:     def validate(self):
Line 52:         return self._certsExists()
Line 53: 


Line 59:                     'vdsm-gencerts.sh'
Line 60:                 ),
Line 61:                 CA_FILE,
Line 62:                 KEY_FILE,
Line 63:                 CERT_FILE,
> still strange we do not pass the CA key file
I don't know. 
Further inspection needed...
Line 64:             ),
Line 65:             raw=True,
Line 66:         )
Line 67:         sys.stdout.write(out)


Line 71: 
Line 72:     def configure(self):
Line 73:         self._exec_vdsm_gencerts()
Line 74:         if isOvirtNode():
Line 75:             validate_ovirt_certs.validate_ovirt_certs()
About this code, I went over it and I Really don't understand
why we need it and why it makes sense.

@Yaniv, @Douglas, @Dan?

I can dig history some more if no one remembers.
Line 76: 
Line 77:     def isconfigured(self):
Line 78:         if self._certsExists():
Line 79:             ret = CONFIGURED


Line 73:         self._exec_vdsm_gencerts()
Line 74:         if isOvirtNode():
Line 75:             validate_ovirt_certs.validate_ovirt_certs()
Line 76: 
Line 77:     def isconfigured(self):
> why not doing something like:
cool. done.
Line 78:         if self._certsExists():
Line 79:             ret = CONFIGURED
Line 80:         else:
Line 81:             ret = NOT_CONFIGURED


Line 78:         if self._certsExists():
Line 79:             ret = CONFIGURED
Line 80:         else:
Line 81:             ret = NOT_CONFIGURED
Line 82:         return ret
> return CONFIGURED if self._certsExists() else NOT_CONFIGURED ?
Done
Line 83: 
Line 84:     def removeConf(self):
Line 85:         pass
Line 86: 


Line 81:             ret = NOT_CONFIGURED
Line 82:         return ret
Line 83: 
Line 84:     def removeConf(self):
Line 85:         pass
> remove the cert files..
Well, according to Alon we should not.

Also If we do remove, we have to know it's a file 

vdsm-tool put there. It's not right to remove a file

put manually or by host deploy... 


@Alon?
Line 86: 
Line 87:     def _certsExists(self):
Line 88:         return all(os.path.isfile(f) for f in [
Line 89:             CA_FILE,


Line 88:         return all(os.path.isfile(f) for f in [
Line 89:             CA_FILE,
Line 90:             CERT_FILE,
Line 91:             KEY_FILE
Line 92:         ])
> I think enough is to check for CERT_FILE... but...
I'm not sure.

I that because we assume if we have cert we have all?

Or because cert alone is somehow enough?

Why not check all?


http://gerrit.ovirt.org/#/c/31562/1/lib/vdsm/tool/configurators/libvirt.py
File lib/vdsm/tool/configurators/libvirt.py:

Line 74:         self.removeConf()
Line 75: 
Line 76:         config.read(self._getFile('VDSM_CONF'))
Line 77:         vdsmConfiguration = {
Line 78:             'ssl_enabled': config.getboolean('vars', 'ssl'),
> why not to leave the validation for the cert files?
well... I thought about this...

since libvirt now requires 'certificates',
I think its the responsibility of the certs module

to roll out certs and to fail if it did not succeed.

Generally when we will have deps it's enough to depend on 

something and you don't have to check but you can relay on 

your dependent module.

That is atleast how I see things...
Line 79:             'sanlock_enabled': SANLOCK_ENABLED,
Line 80:             'libvirt_selinux': LIBVIRT_SELINUX
Line 81:         }
Line 82: 


-- 
To view, visit http://gerrit.ovirt.org/31562
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I53d927e49920e8c45d02718f2b6ef5dff63eead0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list