[Junos] Make _load_candidate() support loading of XML files with headers that have encoding information

[hkam40k]

Hello,

Using the JUNOSDriver, trying to load an XML file that has a header with encoding information present (<?xml version='1.0' encoding='utf-8'?>) will raise the following exception in method _load_candidate:

ValueError: Unicode strings with encoding declaration are not supported. Please use bytes input or XML fragments without declaration.

The error is particularly unfortunate, because when downloading an XML conf from a device, the XML output will contain the header plus encoding (at least on some models), meaning the conf cannot be directly loaded back as a restore operation, for example.

The error happens because before parsing the XML using lxml, the contents are previously already read into a string, after which the encoding information is useless:

with open(filename) as f:
    configuration = f.read()
# ...
configuration = etree.XML(configuration)

The fix would be either to strip the header or the encoding information from the string contents before calling etree.XML(), or better, load the XML from the file handle as bytes, so that the encoding information has effect:

with open(filename) as f:
    configuration = etree.parse(f).getroot()

The above also works without the header present, or if the header has no encoding information.

In the proposed fix, if filename is given, parsing the file as XML is attempted first (while rectifying the above issue), since even if the file is not XML, it bails out very fast, and then continues with the previous workflow of reading the content, and deciphering its format by inspecting its contents.

Some of the code is also removed out of the last try block, since any exceptions thrown by the moved code aren't handled in the catch block anyway (the try seems to be intended more for errors originating from interacting with the device).

[mirceaulinic]

I don't think it's best to always try to load any file. The format should be still detected correctly by the _detect_config_format helper, so why not just update the code below (line 260) to

            fmt = self._detect_config_format(configuration)

            if fmt == "xml":
-               configuration = etree.XML(configuration)
+               configuration = etree.parse(f).getroot()

[hkam40k]

For this to work the f file handle needs to be kept around, and the way to load the XML is a bit different when loading from file or loading from string (etree.parse(fh).getroot() or etree.XML(conf_string)), so I don't see a clean way to handle all cases in a simple way.

It could be modified though to first read the file, detect format, and parse XML from file handle only when approriate. In that case I would do something like this:

    def _load_candidate(self, filename, config, overwrite):
        if filename is None:
            configuration = config
            fmt = self._detect_config_format(configuration)
            if fmt == "xml":
                # Note: configuration cannot have encoding information, since it is already in string-format.
                configuration = etree.XML(configuration)
        else:
            with open(filename) as f:
                configuration = f.read()
                fmt = self._detect_config_format(configuration)
                if fmt == "xml":
                    # XML header encoding is evaluated, if present.
                    configuration = etree.parse(f).getroot()

        if (
            not self.lock_disable
            and not self.session_config_lock
            and not self.config_private
        ):
            # if not locked during connection time, will try to lock
            self._lock()

        try:
            if self.config_private:
                try:
                    self.device.rpc.open_configuration(private=True, normalize=True)
                except RpcError as err:
                    if str(err) == "uncommitted changes will be discarded on exit":
                        pass

            self.device.cu.load(
                configuration,
                format=fmt,
                overwrite=overwrite,
                ignore_warning=self.ignore_warning,
            )
        except ConfigLoadError as e:
            if self.config_replace:
                raise ReplaceConfigException(e.errs)
            else:
                raise MergeConfigException(e.errs)

What do you think?

[mirceaulinic]

Yes, let's try this way.