proposal: RPC Call API

nemith commented

2023-05-11 16:50:56 +00:00

(Migrated from github.com)

Problem

Right now there are two ways to issue a rpc request. There is a low level Do method which takes a RPCMsg and returns a RPCReplyMsg and there is a higher level Call request that takes in a request and response as any and does xml marshaling directly on them. It also converts errors to Go errors.

Both have some issues

Do:

A RPCMsg is given as input however the only field that isn't overwritten is Operation which is just the innerxml field on the message. What is the point in exposing the entire message. You can set a MessageID but it would be overwritten on send.

Call:

Errors are converted to Go errors, but errors in NETCONF have two severity levels: "warning" and "error". Raising warning's as Go errors seems wrong (and is what is done today). We can filter out the warning's but then they are inaccessible and your forced to use Do (which may not be a bad thing).

On top of that some NETCONF servers like Junos like sending informational items as warnings when everything is ok.

Proposal

Combine both Do and Call into a single method called Call with the following signature:
```
func (s *Session) Call(ctx context.Context, req any) (*Reply, error)
```
Add a method to Reply (just renamed RPCReplyMsg) to xml decode a response.
```
 func (r *Reply)  Decode(value any) error 
```
Note this may not be needed as it will just be a thin wrapper around xml.Decode?
Unexport RPCMsg (and probably just rename to request). There is nothing much that an end-user needs here.
Have an option (on Session or on Call?) for handling error messages. Can allow the user to determine if we should convert all error messages to Go errors, Just errors with severity of error, or none.

Other considerations

I want to get this done before cutting a beta to keep the core api as stable as possible.

Given this API should we still have the core RFC6241 operations as methods on (s *Session) like GetConfig, etc? Or should they just be objects that are sent via Call?

## Problem Right now there are two ways to issue a rpc request. There is a low level [`Do`](https://github.com/nemith/netconf/blob/main/session.go#L273) method which takes a `RPCMsg` and returns a `RPCReplyMsg` and there is a higher level [`Call`](https://github.com/nemith/netconf/blob/main/session.go#L273) request that takes in a request and response as `any` and does xml marshaling directly on them. It also converts errors to Go errors. Both have some issues `Do`: * A RPCMsg is given as input however the only field that isn't overwritten is Operation which is just the `innerxml` field on the message. What is the point in exposing the entire message. You can set a `MessageID` but it would be overwritten on send. `Call`: * Errors are converted to Go errors, but errors in NETCONF have two severity levels: "warning" and "error". Raising warning's as Go errors seems wrong (and is what is done today). We can filter out the warning's but then they are inaccessible and your forced to use `Do` (which may not be a bad thing). On top of that some NETCONF servers like Junos like sending informational items as warnings when everything is ok. ## Proposal 1. Combine both `Do` and `Call` into a single method called `Call` with the following signature: ```go func (s *Session) Call(ctx context.Context, req any) (*Reply, error) ``` 2. Add a method to `Reply` (just renamed RPCReplyMsg) to xml decode a response. ```go func (r *Reply) Decode(value any) error ``` **Note this may not be needed as it will just be a thin wrapper around xml.Decode?** 3. Unexport `RPCMsg` (and probably just rename to `request`). There is nothing much that an end-user needs here. 4. Have an option (on `Session` or on `Call`?) for handling error messages. Can allow the user to determine if we should convert all error messages to Go errors, Just errors with severity of `error`, or none. ## Other considerations I want to get this done before cutting a beta to keep the core api as stable as possible. Given this API should we still have the core RFC6241 operations as methods on (s *Session) like GetConfig, etc? Or should they just be objects that are sent via `Call`?

nemith commented

2023-05-11 17:28:28 +00:00

(Migrated from github.com)

@rumenvasilev, @GiacomoCortesi

GiacomoCortesi commented

2023-05-15 10:06:02 +00:00

(Migrated from github.com)

I agree with the proposal, I would probably rather pass the GetConfig etc. objects in the Call method.

I have an additional comment, there are circumstances in which we want to subscribe to a particolar netconf stream for receiving notifications.

The consumer of the library shall be able to receive these notifications when they arrive, in my fork I did that by creating a buffered notifications channel in the session object. The consumer can therefore listen on the channel and get back the notifications as soon as they are received.

So I was wondering if you are thinking about this possibility and whether or not you want to include it in the first beta release.

I agree with the proposal, I would probably rather pass the GetConfig etc. objects in the Call method. I have an additional comment, there are circumstances in which we want to subscribe to a particolar netconf stream for receiving notifications. The consumer of the library shall be able to receive these notifications when they arrive, in my fork I did that by creating a buffered notifications channel in the session object. The consumer can therefore listen on the channel and get back the notifications as soon as they are received. So I was wondering if you are thinking about this possibility and whether or not you want to include it in the first beta release.

nemith commented

2023-05-15 12:06:29 +00:00

(Migrated from github.com)

I agree with the proposal, I would probably rather pass the GetConfig etc. objects in the Call method.

This was my original design but decided to go with the session model but now that we’ve started to roll this out in prod it seems better to just have objects.

I have an additional comment, there are circumstances in which we want to subscribe to a particolar netconf stream for receiving notifications.

We don’t have an immediate need for notifications but I did want to support it. I think it should be able to be supported in the first beta release.

Instead of a buffer I would like to add a handler function. If you wanted you could have a handler that utilized a buffer. If the notification function is nil will will just drop the notifications.

> I agree with the proposal, I would probably rather pass the GetConfig etc. objects in the Call method. This was my original design but decided to go with the session model but now that we’ve started to roll this out in prod it seems better to just have objects. > I have an additional comment, there are circumstances in which we want to subscribe to a particolar netconf stream for receiving notifications. We don’t have an immediate need for notifications but I did want to support it. I think it should be able to be supported in the first beta release. Instead of a buffer I would like to add a handler function. If you wanted you could have a handler that utilized a buffer. If the notification function is nil will will just drop the notifications.

nemith commented

2023-05-15 12:07:14 +00:00

(Migrated from github.com)

Notification support could be added later without API changed (just additions)

GiacomoCortesi commented

2023-05-16 07:50:31 +00:00

(Migrated from github.com)

I see, since I'm extensively using netconf notifications it would be a very nice addition for the project I'm working on.

So if you are willing to get any contributions on that I'd be glad to help.

I see, since I'm extensively using netconf notifications it would be a very nice addition for the project I'm working on. So if you are willing to get any contributions on that I'd be glad to help.

nemith commented

2023-05-16 19:13:37 +00:00

(Migrated from github.com)

Potential example of doing an RPC with the proposed call method

func main() {
	s := netconfig.Open(t)

	reply, err := s.Call(context.Background(), netconf.GetConfig{Datastore: netconf.Running})
	if err != nil {
		return er
	}

	if reply.Err() != nil {
		return reply.Err()
	} 

	var configReply netconf.ConfigReply
	if err != reply.Decode(&configReply); err != nil {
		return err
	}
	fmt.Println(configReply.Config)
}

Potential example of doing an RPC with the proposed call method ```go func main() { s := netconfig.Open(t) reply, err := s.Call(context.Background(), netconf.GetConfig{Datastore: netconf.Running}) if err != nil { return er } if reply.Err() != nil { return reply.Err() } var configReply netconf.ConfigReply if err != reply.Decode(&configReply); err != nil { return err } fmt.Println(configReply.Config) } ```

nemith commented

2023-05-16 19:15:49 +00:00

(Migrated from github.com)

@GiacomoCortesi

So if you are willing to get any contributions on that I'd be glad to help.

Contributions are welcome, but for new features a proposal about the design would probably be beneficial before you code it up.

@GiacomoCortesi > So if you are willing to get any contributions on that I'd be glad to help. Contributions are welcome, but for new features a proposal about the design would probably be beneficial before you code it up.

nemith commented

2023-08-29 15:45:35 +00:00

(Migrated from github.com)

Closed with #60

Rows
Columns

proposal: RPC Call API #44

Problem

Proposal

Other considerations