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: