Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing REST API. #743

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

lspgn
Copy link

@lspgn lspgn commented Jun 26, 2017

Hello,
This PR implements the REST transport by HTTP POST with requests.
See Juniper doc here.

New flags:

  • rest: boolean / Enable REST API
  • ssl_verify: boolean / Do not check TLS certificate
  • schema: string / "http" or "https"
  • path: string / "/rpc" URL path of the API

Exception handling.

@jnpr-community-netdev
Copy link

Can one of the admins verify this patch?

@lspgn
Copy link
Author

lspgn commented Jun 26, 2017

Example for testing:

from pprint import pprint

from jnpr.junos.transport.rest import Rest
from jnpr.junos import Device

dev = Device(host='192.168.1.x', port = '8080', user='user', password='pass', rest = True, ssl_verify = False, schema = 'http')
dev.open()
pprint( dev.facts )
dev.close()

@coveralls
Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage decreased (-2.6%) to 95.807% when pulling d817360 on lspgn:louis/rest-transport into 335277c on Juniper:master.

@lspgn lspgn force-pushed the louis/rest-transport branch 2 times, most recently from aa49b1e to 0bc121d Compare June 26, 2017 15:31
@coveralls
Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage decreased (-2.6%) to 95.806% when pulling aa49b1e on lspgn:louis/rest-transport into 335277c on Juniper:master.

@coveralls
Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage decreased (-2.6%) to 95.806% when pulling 0bc121d on lspgn:louis/rest-transport into 335277c on Juniper:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.6%) to 95.806% when pulling 0bc121d on lspgn:louis/rest-transport into 335277c on Juniper:master.

@coveralls
Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage decreased (-2.6%) to 95.806% when pulling ce03da0 on lspgn:louis/rest-transport into 335277c on Juniper:master.

@mirceaulinic
Copy link
Contributor

mirceaulinic commented Jun 26, 2017

This pull request addresses the subject proposed in #729, then discussed and agreed privately with @vnitinv.

For complete disclosure, REST presents a couple of advantages for long-standing sessions, but not limited to. Not only that reconnect is not required anymore, but as discussed during our conversation, it is good for many other reasons: a long-standing netconf session over SSH triggered some weird bugs in Junos which we want to avoid - waiting for them to be reproduced, acknowledged then solved takes extremely long. For example, one of my favourites was when executing 100 commits over the same session caused a memory leak and the router crashed. Or, the config DB couldn't be unlocked although the commit was successful (that was a race condition keeping the juniper.config open) and many other bizarre behaviors.

In all these cases, it requires restarting the netconf session. But if we use restconf instead, hence won't have a continuous session like that, probably we won't see weird Junos bugs anymore.

This does not constitute a replacement by any means: the default transport is and will forever be NETCONF over SSH. This is just an alternative, unfortunately not tackled yet by any existing library, although Junos devices leverage RESTCONF even since version 15.

Given that the document structure is exactly the same, it does make sense to re-use features already nicely implemented in this library.

Looking forward to your feedback!

New transport.
New flags:
* rest: boolean / Enable REST API
* ssl_verify: boolean / Do not check TLS certificate
* schema: string / "http" or "https"
* path: string / "/rpc" URL path of the API
Exception handling
@coveralls
Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage decreased (-2.6%) to 95.806% when pulling 0348735 on lspgn:louis/rest-transport into 335277c on Juniper:master.

@coveralls
Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage decreased (-2.6%) to 95.806% when pulling 8560672 on lspgn:louis/rest-transport into 335277c on Juniper:master.

@stacywsmith
Copy link
Contributor

Unfortunately, I do not feel that the Junos REST API makes an appropriate transport for PyEZ. There are a few issues:

  1. I don't believe the Junos REST API is fully supported and tested on all Junos platforms. It was introduced in MX platforms in Junos 14.2, but even if another platform is running a Junos release >= 14.2 I don't believe that automatically implies support for the REST API.

  2. The REST API does not currently provide any mechanism for specifying an RPC timeout and has a hard-coded RPC timeout of ~5 seconds. If an RPC takes longer than ~5 seconds to generate a response, then an HTTP 500 - Internal server error response code is returned.

  3. Each HTTP(S) GET or POST request is a separate connection to the device and does not maintain state from the previous request. While it is possible to combine more than one RPC in a single POST, we can't take advantage of that in the PyEZ library where users control what RPCs are invoked when. The biggest impact is that it's impossible to support configuration locks (aka configure exclusive) or private configuration databases (aka configure private). The lock is released, private config database is destroyed, when the NETCONF connection is closed at the end of HTTP(S) request. It's unwise for any automation tool to configure a device using the shared configuration database, and it's impossible to support locks or private databases when using REST as a PyEZ transport.

While I understand your motivation for wanting to avoid bugs you've encountered with long-lived NETCONF connections, these drawbacks to the Junos REST API mean that features which PyEZ users expect and depend upon can not be supported. It would be a support nightmare trying to continually explain these limitations to users. An alternative path is for the user to control when a NETCONF session is opened and closed by simply calling open() and close() as needed. i.e. The user can implement a happy medium between extremely long-lived NETCONF connections and the one-connection-per-RPC model of the REST API.

@mirceaulinic
Copy link
Contributor

mirceaulinic commented Jun 28, 2017

I don't believe the Junos REST API is fully supported and tested on all Junos platforms. It was introduced in MX platforms in Junos 14.2, but even if another platform is running a Junos release >= 14.2 I don't believe that automatically implies support for the REST API.

That's why documentation has been invented.

The REST API does not currently provide any mechanism for specifying an RPC timeout and has a hard-coded RPC timeout of ~5 seconds. If an RPC takes longer than ~5 seconds to generate a response, then an HTTP 500 - Internal server error response code is returned.

I belive this is what Juniper has to address. Last time I checked you were a Juniper employee - why not take this chance to forward this feedback to your colleagues?

The biggest impact is that it's impossible to support configuration locks (aka configure exclusive) or private configuration databases (aka configure private).

I learnt that it's very dangerous and infelicitous to use words such as "impossible" in any tech area - everything is possible. I'm very suprised that an engineer with your experience says that.
This one in particular would be pretty easy to solve actually.

It would be a support nightmare trying to continually explain these limitations to users.

In the worst case, if we'd need to throw your statements in a document, we don't need to explain this continually. Again: there's something called documentation and there are people out there like to use it.

With these said, I consider that none of your arguments above is actually valid.

@stacywsmith please let us know if your backing off attempt reflects your own view (which - from past experiences - seems to count more that community's demands) or Juniper's view.

@ydnath
Copy link
Member

ydnath commented Jun 28, 2017

@mirceaulinic As with any technology area, priorities are assessed continuously. The focus is always on what is used by the customers and is important to them. There is a fine balance between working on the frequently used feature set and over-engineering in every possible way. Open source projects also strive for the same.

If the RESTCONF paradigm is the way forward for networking customers, this project would definitely consider the feedback and be enhanced accordingly.

I would wait for comments from the community about the need for RESTCONF support in this project.

@mirceaulinic
Copy link
Contributor

mirceaulinic commented Jun 28, 2017

The focus is always on what is used by the customers and is important to them.

WRONG: open source does not involve customers. Open source is about community-driven projects - it's a huge difference that network vendors, unfortunately, fail to understand. This is not a TAC case, this is a common effort of everyone, where the community has the power, not your private business priorities. Otherwise, is just another masked way of selling your products, which is the opposite. The community is constituted equally by customers, but also passionate engineers that may not buy your products. If you're interested in helping only the customers, it means you're driven by other values.

If the RESTCONF paradigm is the way forward for networking customers,

Yes, we are a big customer having this need (and submitting the changes required for the features, not begging Juniper for help). Are you backing off this need?

this project would definitely consider the feedback and be enhanced accordingly.

If not implemented & used, how would you expect to receive the feedback?

@pklimai
Copy link
Contributor

pklimai commented Jun 29, 2017

I wonder if this can be added but explicitly marked as experimental/unsupported?

@vnitinv
Copy link
Contributor

vnitinv commented Jun 30, 2017

@mirceaulinic First of all thanks for all your contribution to the open source projects/community and this pull request. I also do understand we need to keep up the expectation of user likes you.

I'm very suprised that an engineer with your experience says that.

Now the idea of this PR can be debated/supported (as other are even doing) but not the kind of comment you put for my team member.
We were very clear and even open in accepting the issues we have with junos REST feature in the public domain. We only put view and didnt close the PR.
We first want the backend layer to be hardened to support any off box client in full-fledged manner. And we already started this discussion internally just after our call (and not after your comments) and I must tell you we take your (and others users) feeedback seriously.

We also discussed over first call that nc in junos-eznc is meant for netconf, so core of this module is netconf only. Adding RestConf in this module we can discuss with this pull request. If someone would have created a parallel module like junos-eznc for restconf, I dont think anyone would had any concerns. PyEZ do support different transport but the application layer runs is Netconf only.

WRONG: open source does not involve customers. Open source is about community-driven projects - it's a huge difference that network vendors, unfortunately, fail to understand. This is not a TAC case, this is a common effort of everyone, where the community has the power, not your private business priorities. Otherwise, is just another masked way of selling your products, which is the opposite. The community is constituted equally by customers, but also passionate engineers that may not buy your products. If you're interested in helping only the customers, it means you're driven by other values.

Ideology wise this is right. But why do you think you contribute to SaltStack/NAPALM or other networking modules only and not other n numbers of Python modules. Or why do a company pay someone just to maintain open sources only. No one give free lunch in this world.

We all need to work together to make this project better. So lets discuss on core idea of the pull request to take it to final conclusion. You can unicast me if you still have any concern.

@mirceaulinic
Copy link
Contributor

mirceaulinic commented Jun 30, 2017

I disagree, but it's pointless to debate this here, it's not healthy.
However, there's something I'd like to clarify and maybe it could help you too to see the problem from more than one single side:

Or why do a company pay someone just to maintain open sources only. No one give free lunch in this world.

Again, it is not about comanies, employees or other financial implications all the time - it's also about passions, very often in our spare time. You might not believe, but it's actually true, and I will give you an example. Because you reminded about NAPALM, let's take the case of its creator, David (which in case he'll ever read these, I apologize I'm using his name to exemplify): he created NAPALM while working at Spotify. Despite the fact that he doesn't work there for over one year, and his current employer does not depend on NAPALM, he's still a very active user and maintainer, helping the community as much as possible on his own time, not when he's paid by the employer. Why? You can find the answers above. There are many other examples.
I hope you'll take this as a constructive feedback, and hopefully, network vendors will understand that at some point: when you decide to open source something you assume that it will belong to the community from there on, not limiting its spectrum to customer needs only, as closed software does. Otherwise, it's just dust in the eyes...
BTW: I'm writing this in my spare time too, way after the office hours. #justsaying

Regarding this PR: let us know what you decide, but please bring some valid arguments, at least (didn't hear any so far).

@ydnath
Copy link
Member

ydnath commented Jul 3, 2017

@mirceaulinic Thanks for the patience. We are debating this internally and will share the feedback/decisions once available. We may reach out to you in the case of any clarification.

@jejenone
Copy link

Disclaimer: I work with @lspgn and @mirceaulinic, I am therefore biased.

@stacywsmith I understand your arguments, they are certainly valid in terms of the quality of the user experience you want to achieve with that library. However I'd like to make a few remarks.

Rest API (not sure at this stage if we can call it RestCONF) has been created by Juniper for a reason: this is what users want. Users don't like the SSH transport layer and usually prefer https, and users don't like XML and usually prefer JSON.
Turns out, as @mirceaulinic said, that long-lived sessions are not easy to deal with for network management. This is especially true when you have an expanded network, in regions with high packet loss and latency. My guess is that it's another reason why this REST API exists.

All of these NETCONF attributes are relics of the past, and also explain why NETCONF is having a hard time taking off. Maybe it's time to admit that NETCONF in its current form has to die.

It just makes sense to me that this library supports REST API as a courtesy to JunOS users and customers, even if labelled as experimental.

@jnpr-community-netdev
Copy link

Autobot: Would an admin like to run functional tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants