akarneliuk / pygnmi

The pure Python implementation of the gNMI client.
https://training.karneliuk.com
BSD 3-Clause "New" or "Revised" License
129 stars 44 forks source link

Default to json_ietf if the target only supports that #95

Closed candlerb closed 2 years ago

candlerb commented 2 years ago

I am working with IOS-XR 7.2.2. It doesn't accept the "json" encoding, it requires "json_ietf". This is clear from the capabilities response:

>>> gc.capabilities()["supported_encodings"]
['json_ietf', 'ascii', 'proto']

However, if I don't specify the encoding explicitly, pygnmi still chooses json by default, and the request fails:

>>> gc.get(path=['openconfig-interfaces:interfaces'])
GRPC ERROR Host: 192.168.0.2:57400, Error: gNMI: unsupported get-request encoding: JSON
Traceback (most recent call last):
...

Therefore, I have to set the encoding explicitly on every request, which is inconvenient (and easy to forget):

         result = gc.get(path=['openconfig-interfaces:interfaces'],
                         encoding='json_ietf')

AFAICS from source, get() has a hard-coded default of json for the encoding argument.

There is a choose_encoding() function, but any explicit value of requested_encoding passed through takes precedence - therefore, the encoding json will always be chosen.

I think this can be improved. Suggestion:

To be backwards-compatible, you'd need to prefer 'json' over 'json_ietf'. Perhaps pass a list of default_encodings to choose_encoding, instead of a single value? Try them in turn, and pick the first that is supported.

akarneliuk commented 2 years ago

Hey @candlerb ,

The solution to your request is simply yo make capabilities() request before doing get. Doing so will create the private dictionary, which will store the supported encodings and the the function choose_encoding will pick up the correct one, whether it is json or json_ietf. So, simply update your code as this:

with gNMIclient(...) as gc:
    gc.capabilities()
    result = gc.get(path=['openconfig-interfaces:interfaces'])

It shall solve your use case of setting the encoding manually.

Let me know if that works.

Best, Anton

candlerb commented 2 years ago

That's exactly what I've done, and it doesn't work.

akarneliuk commented 2 years ago

Thanks @candlerb,

That's not what I would expect. Can you please run the code with debug=True and post here the output?

Best, Anton

candlerb commented 2 years ago

Sure. Here's the code:

# Modules
from pygnmi.client import gNMIclient
import json, sys

# Variables
host = ('192.168.0.2', '57400')
username = 'brian-candler'
password = 'XXXXXXXX'

# Body
if __name__ == '__main__':
    with gNMIclient(target=host, username=username, password=password, insecure=True, debug=True) as gc:
         gc.capabilities()["supported_encodings"]
         result = gc.get(path=['/interfaces/interface[name="Bundle-Ether2"]/state/counters'],
                         #encoding='json_ietf',
         )
         print(result)

Here's the result:

GRPC Target
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
192.168.0.2:57400
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------

GRPC Channel options
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
[]
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------

gNMI request
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------

-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------

gNMI response
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
supported_models {
  name: "Cisco-IOS-XR-ipv4-io-oper"
  organization: "Cisco Systems, Inc."
  version: "2019-10-01"
}
<< snip lots more >>
supported_models {
  name: "SNMP-COMMUNITY-MIB"
  organization: "Cisco Systems, Inc."
  version: "2003-08-06"
}
supported_encodings: JSON_IETF
supported_encodings: ASCII
supported_encodings: PROTO
gNMI_version: "0.7.0"

-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------

gNMI request
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
prefix {
}
path {
  elem {
    name: "interfaces"
  }
  elem {
    name: "interface"
    key {
      key: "name"
      value: "\"Bundle-Ether2\""
    }
  }
  elem {
    name: "state"
  }
  elem {
    name: "counters"
  }
}

-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------

GRPC ERROR Host: 192.168.0.2:57400, Error: gNMI: unsupported get-request encoding: JSON
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/site-packages/pygnmi/client.py", line 386, in get
    gnmi_message_response = self.__stub.Get(gnmi_message_request, metadata=self.__metadata)
  File "/usr/local/lib/python3.9/site-packages/grpc/_channel.py", line 946, in __call__
    return _end_unary_response_blocking(state, call, False, None)
  File "/usr/local/lib/python3.9/site-packages/grpc/_channel.py", line 849, in _end_unary_response_blocking
    raise _InactiveRpcError(state)
grpc._channel._InactiveRpcError: <_InactiveRpcError of RPC that terminated with:
    status = StatusCode.UNIMPLEMENTED
    details = "gNMI: unsupported get-request encoding: JSON"
    debug_error_string = "{"created":"@1661098625.944299000","description":"Error received from peer ipv4:192.168.0.2:57400","file":"src/core/lib/surface/call.cc","file_line":967,"grpc_message":"gNMI: unsupported get-request encoding: JSON","grpc_status":12}"
>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/brian/2022/src/test-netconf/test4-cisco.py", line 14, in <module>
    result = gc.get(path=['/interfaces/interface[name="Bundle-Ether2"]/state/counters'],
  File "/usr/local/lib/python3.9/site-packages/pygnmi/client.py", line 464, in get
    raise gNMIException(f"GRPC ERROR Host: {self.__target_path}, Error: {err.details()}", err)
pygnmi.client.gNMIException: GRPC ERROR Host: 192.168.0.2:57400, Error: gNMI: unsupported get-request encoding: JSON
candlerb commented 2 years ago

I think I can see from code inspection what's happening.

Firstly, the get() method applies default value 'json' for its encoding parameter here:

    def get(self, prefix: str = "", path: list = None,
            target: str = None, datatype: str = 'all',
            encoding: str = 'json'):
            ^^^^^^^^^^^^^^^^^^^^^^

This value is then passed as requested_encoding here:

        pb_encoding = choose_encoding(collected_capabilities=self.__capabilities,
                                      default_encoding="json",
                                      requested_encoding=encoding)
                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^

choose_encoding() honours any non-empty value of requested_encoding unconditionally, without consulting the capabilities, here:

    if requested_encoding:
        if requested_encoding.upper() in Encoding.keys():
            result = Encoding.Value(name=requested_encoding.upper())

        else:
            raise ValueError(f'Subscribe encoding {requested_encoding} is out of allowed ranges.')

Hence if you don't provide an encoding, "json" is always used.

candlerb commented 2 years ago

Here is a proof-of-concept patch (not submitted as PR in case you want to do it differently):

--- /usr/local/lib/python3.9/site-packages/pygnmi/client.py.orig    2022-08-21 17:45:31.000000000 +0100
+++ /usr/local/lib/python3.9/site-packages/pygnmi/client.py 2022-08-21 18:06:11.000000000 +0100
@@ -307,7 +307,7 @@

     def get(self, prefix: str = "", path: list = None,
             target: str = None, datatype: str = 'all',
-            encoding: str = 'json'):
+            encoding: str = None):
         """
         Collecting the information about the resources from defined paths.

@@ -468,7 +468,7 @@
             raise gNMIException(f'Collection of Get information failed: {e}', e)

     def set(self, delete: list = None, replace: list = None,
-            update: list = None, encoding: str = 'json',
+            update: list = None, encoding: str = None,
             prefix: str = "", target: str = None):
         """
         Changing the configuration on the destination network elements.
@@ -611,7 +611,7 @@
             raise gNMIException(f"Set failed: {e}", e)

-    def set_with_retry(self, delete: list = None, replace: list = None, update: list = None, encoding: str = 'json', retry_delay: int = 3):
+    def set_with_retry(self, delete: list = None, replace: list = None, update: list = None, encoding: str = None, retry_delay: int = 3):
         """
         Performs a set and retries (once) after a temporary failure with StatusCode.FAILED_PRECONDITION
         """
@@ -1218,8 +1218,8 @@
         else:
             raise ValueError(f'Subscribe encoding {requested_encoding} is out of allowed ranges.')

-    else:
-        if collected_capabilities and 'supported_encodings' in collected_capabilities and\
+    elif default_encoding:
+        if not collected_capabilities or not 'supported_encodings' in collected_capabilities or\
                 default_encoding in collected_capabilities['supported_encodings']:
             result = Encoding.Value(name=default_encoding.upper())

Note: the reason I had to change the logic in choose_encoding is because otherwise it was backwards incompatible: the default became JSON_IETF if you hadn't called gc.capabilities() (that is, my test program still worked without calling gc.capabilities)

This is turn is because, even though the call to choose_encoding explicitly passes default_encoding="json", the value is ignored if you haven't collected the capabilities:

        if collected_capabilities and 'supported_encodings' in collected_capabilities and\
                default_encoding in collected_capabilities['supported_encodings']:
            result = Encoding.Value(name=default_encoding.upper())

Therefore it does nothing, and falls back to JSON_IETF:

    # Default encoding equals to JSON_IETF in Protobuf format
    result = 4

The logic now says: use the supplied default_encoding, unless you know for sure (due to collected capabilities) that it isn't allowed.

Hence if you pass default_encoding="json", it will use it, unless you've called collected_capabilities and "json" isn't in the set, in which case it will default to "json_ietf".

I've tested this. My test program now doesn't crash if I call gc.capabilities(), but does crash if I do.

candlerb commented 2 years ago

Oops: that patch doesn't work for set(), because set doesn't use choose_encoding. (And if encoding is None, then you get an error that None doesn't have method upper())

Do you think set() should do the automatic encoding selection? Otherwise I need to use gc.set(encoding='json_ietf', ...) everywhere.

akarneliuk commented 2 years ago

Hey @candlerb ,

Use for the time being encoding, please. I have some ideas how to make more resilient and user friendly, which I plan to test tomorrow.

Best, Anton

akarneliuk commented 2 years ago

In fact, implemented in #97 . Can you please test and let me know?

candlerb commented 2 years ago

Yes, 0.8.9 works. Thank you!

I guess the only downside is the forced 'capabilities' request. If you just wanted to connect, make one small change and disconnect, and you knew the device capabilities already, then you have some extra network traffic. But I don't think it's a big deal.

I also tested the pygnmicli --gni-timeout option.

akarneliuk commented 2 years ago

There's no free coffee, i think. I would assume that time needed for capabilities request is negligible. Otherwise, you won't have possibility to automatically detect the encoding.

candlerb commented 2 years ago

Sure. I was just wondering if it had been an intentional decision originally to leave it up to the user to decide whether or not to request capabilities. Obviously, if they don't, it's up to them to choose the right features.