-
Notifications
You must be signed in to change notification settings - Fork 8
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
basic callhome functionality #31
base: main
Are you sure you want to change the base?
Conversation
Wow. I always tried to keep call home in mind when designing the transport mechanisms but I didn't put much thought into it, but this actually worked out to be really nice. Thanks for the PR. I wait until this is out of draft to review but I am happy with the general direction here! |
callhome.go
Outdated
"net" | ||
) | ||
|
||
type TransportType uint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general i would rather see this layered where a transport is constructed and passed into the Callhome server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct me if I'm wrong, as per your implementation, creating a transport involves passing in a connection (ssh or tls or whatever depending on the desired transport). That is fine for the classical netconf connection that is initiated by the netconf-client device but callhome works the other way around (i.e. is the netconf-server initiating the connection by opening a TCP connection to the callhome server).
The transport (and the subsequent netconf connection) in case of callhome shall be created on top of the incoming TCP connection, therefore I cannot pass the transport to the callhome server, otherwise it would mean that the connection has already been established defeating the purpose of callhome. Does this make sense to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% on board. You couldn't pass a Transport
object in, but we could create a new interface called CallHomeTransport
or TransportHandler
(I am bad at naming that would take a active TCP session and create a TLS or SSH session out of it. This way we are not adding transportation logic inside the core netconf
package.
So i guess my point I was trying to make is that we shouldn't have a TransportationType
that gets switched to do the right thing. There should be an interface that gets passed into the CallHomeServer that is able to create the transport after establishing an incoming connection and thus breaking the hard coupling between the server and the transport logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely agree on that. I updated the PR with a new commit that:
- introduce the CallHomeTransport interface and implements it for TLS and SSH for end user simplicity
- allows handling multiple clients connection
So far implementation is based on client IP address (need to wrap my mind around to understand how I can expand this nicely to manage host keys also) and it is still pretty basic.
This said, I tested with a couple of netconf devices attempting SSH callhome (one of those was configured, the other one was not), this is what you get in the output running the callhome_ssh example:
2023/04/12 18:55:50 callhome server listening on: 0.0.0.0:4339
handling connection from: 192.168.121.17:2386
error handling callhome connection from address: 192.168.121.17:2386, error: no client configured for address 192.168.121.17:2386
handling connection from: 192.168.121.230:3457
handling connection from: 192.168.121.17:2387
error handling callhome connection from address: 192.168.121.17:2387, error: no client configured for address 192.168.121.17:2387
2023/04/12 18:56:03 Config:
<sensitive config xml but I swear it responded correctly :) >
Wanted to check out with you before moving on.
Maybe Callhome could be a package per se or would you rather keep it in the main netconf package?
Any hint on if and how you want to handle logging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Callhome could be a package per se or would you rather keep it in the main netconf package?
For me package consideration is mostly around external dependencies (thus ssh and tls being their own package). Transport is it's own package for cylical redundancy reasons. Adding callhome to the netconf
package seems like the right place.
I am not a big fan of Go modules that are a lot of different packages netconf.CallHomeServer
is a nicer fully qualified name for an object than callhome.Server
in my opinion.
Any hint on if and how you want to handle logging?
Logging I am trying to hold off as long as possible to see if it is needed (it probably is). Even then it should be optional, interface based, and off by default.
For callhome it may be better to have handlers/callbacks for anything that a user may want to do something with (including logging)? I haven't put a lot of thought into it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the PR, I now make use of two channels in the CallHomeServer struct for the consumer to retrieve connecting CallHome clients and intercept errors, it seems a nicer way to handle it to me.
Do you have any suggestion/features you want to add to this PR?
callhome.go
Outdated
/* | ||
CallHome implements netconf callhome procedure as specified in RFC 8071 | ||
*/ | ||
type CallHome struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit. CallHomeServer
callhome.go
Outdated
opt(ch) | ||
} | ||
|
||
if ch.configTLS == nil && ch.configSSH == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general if an option is mandatory then (by definition) it's not an option. In general i try to avoid runtime errors like this based on functional options.
In my opinion It's better to provide multiple functions to construct the object with positional arguments than to do runtime checks like this
transport/ssh/ssh.go
Outdated
@@ -46,6 +46,16 @@ func Dial(ctx context.Context, network, addr string, config *ssh.ClientConfig) ( | |||
return newTransport(client, true) | |||
} | |||
|
|||
// DialWithConn is same as Dial but creates the transport on top of input net.Conn | |||
func DialWithConn(addr string, config *ssh.ClientConfig, conn net.Conn) (*Transport, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to add bunch of "Dial*" type functions like I did in github.com/Juniper/netconf
ssh.NewTransport
should fit everything that that Dial
cannot. This logic can just but stuffed into the session creation in callhome.go.
If for some reason it becomes very popular that sessions (outside of callhome) need to contruct already from a net.Con we can look at potentially adding this back, but for now I want the API surface to be small.
Easier to add functions in a minor version then it is to remove them in a major verion.
callhome.go
Outdated
} | ||
|
||
// handleConnection upgrade input net.Conn to establish a netconf session | ||
func (ch *CallHome) handleConnection(conn net.Conn) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This echo the sentiments above about passing in a transport into a callhome server.
In general the library has been designed in layers in which github.com/nemith/netconf
is transport agnostic with transport logic being in github.com/nemith/transport
. This also allows transports to be defined outside of this module (for example there is a draft rfc for netconf over quic).
callhome.go
Outdated
transport TransportType | ||
configSSH *ssh.ClientConfig | ||
configTLS *tls.Config | ||
session *Session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing about a callhome server is that it should support multiple clients. This should be a map (protected with a mutex probably) and the CallHomeServer
object should have ways of obtaining a particular client based on some sort of identifier.
Looking at the RFC indentifiers could be the the certificate FP (tls) or host key (ssh) and or ipaddress/port pair?
Hey @nemith, how do we move forward with this PR? You want to track this in a proposal as I've done with notifications? |
I think the functionality is fine however I am marching to a 1.0 release with a stable API and don’t want to be changing the api that much later on. So I want to spend some real effort in making sure the api is right. Is this something you need for your use case? If so I can prioritize it. If it’s not maybe it can wait as I am actively testing this package inside Roblox’s network and fixing things as we go along. We aren’t using or have any plans for call home. |
Unfortunately Callhome is a mandatory feature for the project I'm working on. If it helps you following the flow I can put down the proposal with some code snippets. |
Ok let’s prioritize it. I can propose some api changes and possibly do a call home branch if needed.
Sent from Proton Mail for iOS
…On Tue, Jun 13, 2023 at 11:36, Giacomo Cortesi ***@***.***(mailto:On Tue, Jun 13, 2023 at 11:36, Giacomo Cortesi <<a href=)> wrote:
> I think the functionality is fine however I am marching to a 1.0 release with a stable API and don’t want to be changing the api that much later on. So I want to spend some real effort in making sure the api is right.
>
> Is this something you need for your use case? If so I can prioritize it. If it’s not maybe it can wait as I am actively testing this package inside Roblox’s network and fixing things as we go along. We aren’t using or have any plans for call home.
Unfortunately Callhome is a mandatory feature for the project I'm working on.
I can double check the implementation and see if we can apply the same handler approach we used for notifications also for handling callhome clients connections.
If it helps you following the flow I can put down the proposal with some code snippets.
—
Reply to this email directly, [view it on GitHub](#31 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AACVJMHQ2TUYPFVNGMA7RHDXLB3GJANCNFSM6AAAAAAVLI6VGI).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
So re-reading the RFC an thinking this through a bit.
I need to think thought this all bit more and see how the API should roughtly be. It probably won't be much different from what you have but there is a lot to concider to properly support this. The other option is for really simple use cases there isn't much to a callhome server and it could easily be implemented outside of the netconf package. Perhaps really is what is needed is functions for TLS and SSH that take a conn and give back a transport? This may be the simpliest fix for now? Thoughts? |
Agreed, address (and port) is already configurable through the WithAddress CallHomeOption.
This shall be already possible by passing a proper TLSCallHomeTransport configuration
This is already possible by passing a proper SSHCallHomeTransport, e.g. :
I need to think about RESTCONF a little bit, never took it into account so far.
I can try to rethink the current implementation that uses the two channels:
Definitely, current implementation does not allow for customising session creation with options, that needs to be changed for sure (maybe adding them to CallHomeClientConfig struct? or just by allowing to setup a notification handler after session creation?)
Client identification is subtle, it's currently based on IP address only and I agree it is limited.
Thank u for taking the time to look deeper in something you don't really need for yourself 🥇
mmm even though to create the transport we still need the initial client configuration, meaning the callhome server need to know whether to build a SSH or TLS transport on top of the TCP connection so it doesn't solve the identifier issue if I well understand. |
This a super basic callhome functionality based on rfc8071. Tested with real netconf device (in SSH mode), didn't test with TLS.
Opening as a DRAFT, if it is something you may be interested in maybe we can have some discussion on how to proceed further.