Add capability to define version when using eos_command with httpapi

carlbuchmann commented 4 years ago

SUMMARY

Add the capability to specify the data model version with eos_command when using httpapi connectivity method.

ISSUE TYPE

Feature Idea

COMPONENT NAME

httpapi/eos.py -> https://github.com/ansible-collections/arista.eos/blob/main/plugins/httpapi/eos.py
modules/eos_command.py -> https://github.com/ansible-collections/arista.eos/blob/main/plugins/modules/eos_command.py

ADDITIONAL INFORMATION

For example, a new data models are developed, or to pin to a specific version of the data model it might be desirable to provide eAPI with the desired version.

Here's an example when interacting directly with eAPI:

Request w/version: 1:

{
  "jsonrpc": "2.0",
  "method": "runCmds",
  "params": {
    "format": "json",
    "timestamps": false,
    "autoComplete": false,
    "expandAliases": false,
    "includeErrorDetail": false,
    "cmds": [
      "show ip route summary"
    ],
    "version": 1
  },
  "id": "EapiExplorer-1"
}

response:

{
  "jsonrpc": "2.0",
  "id": "EapiExplorer-1",
  "result": [
    {
      "maskLen": {
        "8": 2,
        "31": 6,
        "32": 27
      },
      "totalRoutes": 35,
      "ospfv3Counts": {
        "ospfv3Total": 0,
        "ospfv3InterArea": 0,
        "ospfv3External1": 0,
        "ospfv3External2": 0,
        "ospfv3NssaExternal1": 0,
        "ospfv3NssaExternal2": 0,
        "ospfv3IntraArea": 0
      },
      "staticNexthopGroup": 0,
      "bgpCounts": {
        "bgpExternal": 12,
        "bgpTotal": 13,
        "bgpLocal": 0,
        "bgpInternal": 1
      },
      "attached": 6,
      "rip": 0,
      "static": 0,
      "vcs": 0,
      "connected": 6,
      "isisCounts": {
        "isisLevel1": 0,
        "isisLevel2": 0,
        "isisTotal": 0
      },
      "aggregate": 0,
      "dynamicPolicy": 0,
      "internal": 10,
      "ospfCounts": {
        "ospfInterArea": 0,
        "ospfExternal2": 0,
        "ospfExternal1": 0,
        "nssaExternal1": 0,
        "nssaExternal2": 0,
        "ospfIntraArea": 0,
        "ospfTotal": 0
      }
    }
  ]
}

Request with version: latest:

{
  "jsonrpc": "2.0",
  "method": "runCmds",
  "params": {
    "format": "json",
    "timestamps": false,
    "autoComplete": false,
    "expandAliases": false,
    "includeErrorDetail": false,
    "cmds": [
      "show ip route summary"
    ],
    "version": "latest"
  },
  "id": "EapiExplorer-1"
}

response - note new key: protoModelStatus:

{
  "jsonrpc": "2.0",
  "id": "EapiExplorer-1",
  "result": [
    {
      "vrfs": {
        "default": {
          "maskLen": {
            "8": 2,
            "31": 6,
            "32": 27
          },
          "totalRoutes": 35,
          "ospfv3Counts": {
            "ospfv3Total": 0,
            "ospfv3InterArea": 0,
            "ospfv3External1": 0,
            "ospfv3External2": 0,
            "ospfv3NssaExternal1": 0,
            "ospfv3NssaExternal2": 0,
            "ospfv3IntraArea": 0
          },
          "staticNexthopGroup": 0,
          "bgpCounts": {
            "bgpExternal": 12,
            "bgpTotal": 13,
            "bgpLocal": 0,
            "bgpInternal": 1
          },
          "attached": 6,
          "rip": 0,
          "staticNonPersistent": 0,
          "staticPersistent": 0,
          "static": 0,
          "vcs": 0,
          "connected": 6,
          "isisCounts": {
            "isisLevel1": 0,
            "isisLevel2": 0,
            "isisTotal": 0
          },
          "aggregate": 0,
          "dynamicPolicy": 0,
          "internal": 10,
          "ospfCounts": {
            "ospfInterArea": 0,
            "ospfExternal2": 0,
            "ospfExternal1": 0,
            "nssaExternal1": 0,
            "nssaExternal2": 0,
            "ospfIntraArea": 0,
            "ospfTotal": 0
          }
        }
      },
      "protoModelStatus": {
        "configuredProtoModel": "multi-agent",
        "operatingProtoModel": "multi-agent"
      }
    }
  ]

Suggested implementation:

eos_command:
  commands:
    - command: "show ip route summary"
      output: json
      version: latest

Qalthos commented 3 years ago

This is a good idea, I'm just not sure how best to go about it.

I feel like this ought to just be implemented as a var on eos' httpapi plugin... there shouldn't be any reason to involve eos_command in picking the version. But blindly following that var might break existing modules that aren't expecting anything but v1 output... and it would be nice to allow modules to opt into higher versions of the API at their discretion.

The best solution I can figure is adding version=1 as a parameter to the httpapi plugin's get() method (and wiring it up down to request_builder()), but also adding a eos_prefer_eapi_version option (or something like that) to the plugin itself. modules calling get() can request specific versions of the API, or can explicitly send None to indicate that anything is fine and to use the value in eos_prefer_eapi_version (which would probably only get used by eos_command).

I don't know if that is the best solution, nor do I know if it is even better than the proposed module parameter, but it does avoid adding a connection-specific parameter to a module that really shouldn't care about the connection being used while giving the option to other modules as well without changing the argspec.

carlbuchmann commented 3 years ago

@Qalthos that makes total sense and I like the option of having it available for other modules. I'll get other Arista team members to review and comment.

With what you are suggesting, each task can request a different version if required correct? Would the eos_prefer_eapi_version key reside under the module like this?:

- name: Gathter ip route summary and ArBGP state
  eos_command:
    commands: "show ip route summary"
  eos_prefer_eapi_version: "latest"  
  register: ip_route_summary
  tags:
    - bgp_check

Note by default the EOS CLI outputs the latest version of the data model when using | json - see output example below comparing | json to | json version 1. One suggestion I would have is making network_cli and httpapi consistent we would set the version to: version=latest

DC1-BL1A#show ip route summary | json
{
    "vrfs": {
        "default": {
            "maskLen": {
                "8": 2,
                "31": 7,
                "32": 36
            },
            "totalRoutes": 45,
            "ospfv3Counts": {
                "ospfv3Total": 0,
                "ospfv3InterArea": 0,
                "ospfv3External1": 0,
                "ospfv3External2": 0,
                "ospfv3NssaExternal1": 0,
                "ospfv3NssaExternal2": 0,
                "ospfv3IntraArea": 0
            },
            "staticNexthopGroup": 0,
            "bgpCounts": {
                "bgpExternal": 19,
                "bgpTotal": 20,
                "bgpLocal": 0,
                "bgpInternal": 1
            },
            "attached": 7,
            "rip": 0,
            "staticNonPersistent": 0,
            "staticPersistent": 0,
            "static": 0,
            "vcs": 0,
            "connected": 7,
            "isisCounts": {
                "isisLevel1": 0,
                "isisLevel2": 0,
                "isisTotal": 0
            },
            "aggregate": 0,
            "dynamicPolicy": 0,
            "internal": 11,
            "ospfCounts": {
                "ospfInterArea": 0,
                "ospfExternal2": 0,
                "ospfExternal1": 0,
                "nssaExternal1": 0,
                "nssaExternal2": 0,
                "ospfIntraArea": 0,
                "ospfTotal": 0
            }
        }
    },
    "protoModelStatus": {
        "configuredProtoModel": "multi-agent",
        "operatingProtoModel": "multi-agent"
    }
}
DC1-BL1A#show ip route summary | json version 1
{
    "maskLen": {
        "8": 2,
        "31": 7,
        "32": 36
    },
    "totalRoutes": 45,
    "isis": 0,
    "ospfv3Counts": {
        "ospfv3Total": 0,
        "ospfv3InterArea": 0,
        "ospfv3External1": 0,
        "ospfv3External2": 0,
        "ospfv3NssaExternal1": 0,
        "ospfv3NssaExternal2": 0,
        "ospfv3IntraArea": 0
    },
    "staticNexthopGroup": 0,
    "bgpCounts": {
        "bgpExternal": 19,
        "bgpTotal": 20,
        "bgpLocal": 0,
        "bgpInternal": 1
    },
    "attached": 7,
    "rip": 0,
    "static": 0,
    "vcs": 0,
    "connected": 7,
    "isisCounts": {
        "isisLevel1": 0,
        "isisLevel2": 0,
        "isisTotal": 0
    },
    "aggregate": 0,
    "dynamicPolicy": 0,
    "internal": 11,
    "ospfCounts": {
        "ospfInterArea": 0,
        "ospfExternal2": 0,
        "ospfExternal1": 0,
        "nssaExternal1": 0,
        "nssaExternal2": 0,
        "ospfIntraArea": 0,
        "ospfTotal": 0
    }
}
DC1-BL1A#

ClausHolbechArista commented 3 years ago

IMO the Request for returning a specific version of data is the same as the the Request for returning JSON vs Text, so the original proposal of requesting it per Command makes sense to me.

eos_command:
  commands:
    - command: "show ip route summary"
      output: json
      version: latest

I am not sure I follow the point on implementation, but if it is a problem to implement different versions per command, we could move it out one level. On EOS the commands are updated one-by-one to newer versions, so keeping the flexibility here would be good.

titom73 commented 3 years ago

All network modules are using the same type of syntax:

 <vendor_collection_module>_command:
    commands: 
      - < cmd1 >
      - < cmd2 >
      - ...
    < display|output >: < format >

The output statement is at the same level as commands and moving it to another place would make eos_command very different than any other modules.

My personal thought would be to go with @carlbuchmann approach as it is closer to the structure of the current modules.

Qalthos commented 3 years ago

Would the eos_prefer_eapi_version key reside under the module like this?:

- name: Gathter ip route summary and ArBGP state
  eos_command:
    commands: "show ip route summary"
  eos_prefer_eapi_version: "latest"  
  register: ip_route_summary
  tags:
    - bgp_check

No, it would be a plugin option set by any of env var, inventory var, or play or task var. The task equivalent would be

- name: Gathter ip route summary and ArBGP state
  eos_command:
    commands: "show ip route summary"
  vars:
    eos_prefer_eapi_version: "latest"  
  register: ip_route_summary
  tags:
    - bgp_check

That said...

Note by default the EOS CLI outputs the latest version of the data model when using | json - see output example below comparing | json to | json version 1. One suggestion I would have is making network_cli and httpapi consistent we would set the version to: version=latest

This is the detail I was missing here. If the version isn't transport-specific, then we should definitely follow what CLI does and also expose the option to both connections. So add option json_version with default latest to eos_command, document that it only has an effect on output: json and add some kind of version parameter to the get methods of cliconf and httpapi.

The point is to not have weird connection-specific functionality at the module level where it doesn't belong. If it's not actually connection-specific, there's not really an issue, as long as we're clear what the option is for.

carlbuchmann commented 3 years ago

This is the outcome of the discussion with @GomathiselviS and @titom73

Agreed on the proposed data model

agree on data model:

eos_command:
  commands:
    - command: "show ip route summary"
      output: json
      version: < version number | default -> latest >

Make this connection agnostic and have a new version parameter available for both network_cli and httpapi transport.

@Qalthos need your input and guidance here:

Changes in eos_command.py:

extend function def to_cli(obj): to included version variable, default to latest.
create new function def to_httpapi(obj) to capture version variable.

Changes in httpapi/eoy.py:

update request_builder() method to support version parameter, default to latest to match network_cli (currently hardcoded to "1").

Qalthos commented 3 years ago

@Qalthos need your input and guidance here:

Changes in eos_command.py:
* extend function `def to_cli(obj):` to included version variable, default to latest.

to_cli is unused, and should probably be removed to avoid confusion. Ideally you want to avoid sending | json strings at all to maintain compatibility with eapi that doesn't understand that, and to_cli does the opposite of that.

Instead, after calling the common parse_commands function, I would loop over the resulting command dictionaries and assign the version value there if necessary.

Then, in _get_command_with_output() in cliconf, you can rejoin the version value with | json if needed, and in httpapi update send_request to use the version value if needed.

manuwelakanade commented 3 years ago

Hi @Qalthos , @carlbuchmann, @GomathiselviS : Went through the discussion in this thread and based on that, I'm proposing the following solution:

Add a 'version' argument to eos_argument_spec which will make it available to use for all the network_modules (for httpapi as well as cli) - This is in network module_utils
In eos_command module, after the parse_commands call, loop through the commands and for each command, add the command variable command['version']
Update send_request and _get_command_with_output to include the 'version' value. *** A draft PR is raised with above changes: https://github.com/ansible-collections/arista.eos/pull/258

With the above changes, the task will look like this:

      arista.eos.eos_command:
          commands:
              - command: show ip route summary
                 output: json
          version: 1

This way, the version parameter can also be made available for other arista modules if required. And it will be available with both httpapi and cliconf connections.

Questions: If the above approach is acceptable, should the version parameter be plugged only in the eos_command module or also in all the other modules?

KonikaChaurasiya-GSLab commented 3 years ago

Hi @Qalthos , @carlbuchmann, @GomathiselviS : I have raised a drraft PR of changes so far. https://github.com/ansible-collections/arista.eos/pull/258.

ansible-collections / arista.eos