Change in vdsm[master]: register: Add --ca-file

danken at redhat.com danken at redhat.com
Sun Aug 16 13:19:56 UTC 2015


Dan Kenigsberg has posted comments on this change.

Change subject: register: Add --ca-file
......................................................................


Patch Set 2: Code-Review-1

(4 comments)

https://gerrit.ovirt.org/#/c/44576/2/lib/vdsm/tool/register.py
File lib/vdsm/tool/register.py:

Line 119:         self.logger.debug("VDSM UUID: {uuid_provided}".format(
Line 120:                           uuid_provided=self.vdsm_uuid))
Line 121: 
Line 122:         if ca_file is None:
Line 123:             self.temp_ca_file = True
why do you need this boolean? having ca_engine=None is enough to tell that we do not need to store the ca cert.
Line 124:             self.ca_engine = None
Line 125:         else:
Line 126:             self.temp_ca_file = False
Line 127:             self.ca_engine = ca_file


Line 207:         cert_validation -- SSL cert will be verified
Line 208: 
Line 209:         Returns: Content of http request
Line 210:         """
Line 211:         if self.check_fqdn and cert_validation:
is this related to this patch? I think it deserves its own justification
Line 212:             cert_validation = self.ca_engine
Line 213:         else:
Line 214:             cert_validation = False
Line 215: 


Line 294:         """
Line 295:         self.logger.info("Collecting CA data from Engine...")
Line 296: 
Line 297:         if self.ca_engine is None:
Line 298:             ca_dir = "/tmp"
never use /tmp directly. tempdir knows which path should be used for temporary files.
Line 299:         else:
Line 300:             ca_dir = os.path.dirname(self.ca_engine)
Line 301: 
Line 302:         if self.ca_engine and os.path.exists(self.ca_engine):


Line 387:         """
Line 388:         self._execute_http_request(self.url_reg)
Line 389:         self.logger.info("Registration completed, host is pending approval"
Line 390:                          " on Engine: {e}".format(e=self.engine_fqdn))
Line 391: 
this cleanup does not seem related to execute_registration.

I find this logic very confusing. download_ca should download the ca cert to a temporary directory.

If the user asks to persist it, a special function would copy the file and persist it.

Immediately after use, the temp dir should be removed.
Line 392:         if self.temp_ca_file:
Line 393:             os.unlink(self.ca_engine)
Line 394: 
Line 395: 


-- 
To view, visit https://gerrit.ovirt.org/44576
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie24793674569107148c832f0395807586044b95e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list