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

gnsi.accounting.SessionInfo is lacking in various ways #98

Closed
haussli opened this issue Jul 18, 2023 · 3 comments · Fixed by #105
Closed

gnsi.accounting.SessionInfo is lacking in various ways #98

haussli opened this issue Jul 18, 2023 · 3 comments · Fixed by #105

Comments

@haussli
Copy link
Contributor

haussli commented Jul 18, 2023

I presume the local/remote fields are for the connection that triggered the accounting message.

system_address should either be local_address, matching local_port, or both system_address and local_address should exist, the latter being the address that the client used as the destination address (which could be any interface on the device). These fields might hold the same address, which is fine, and clearly having the loopback ("system address") of the device (or VRF) is useful.

What is SessionInfo.layer4_proto? Is that OSI layer 4, so TCP/UDP/SCTP? Add comments.

It is not clear what channel_id contains. Looking at ssh docs/etc, a channel id appears to be an integer; rfc4254 S3. Is this correct? The field should be commented appropriately for what the field contains, imiho.

@morrowc
Copy link
Contributor

morrowc commented Jul 21, 2023

I agree that we shouldn't have mixed system-address and local-port :(
It seems reasonable to add 'local-address', and require that both system-address (probably loopback-like) and local-address both be filled in, in the message.

Comments for the ports/protocols seem reasonable, yes.

As to channel-id, I was actually thinking (I believe) that this was the gRPC channel id:

https://grpc.io/docs/what-is-grpc/core-concepts/#channels

upon re-reading that, though, it looks like 'channel' is actually the whole of the connection :( not the sub path/channel I was expecting, and which I think does, actually, exist. So, removing channel-id seems safe.

haussli added a commit to haussli/gnsi that referenced this issue Jul 26, 2023
haussli added a commit to haussli/gnsi that referenced this issue Jul 26, 2023
@haussli
Copy link
Contributor Author

haussli commented Jul 26, 2023

OK. #105 addresses the first two bits. I guessed at the intended meaning of the layer4_proto field.

@haussli
Copy link
Contributor Author

haussli commented Jul 26, 2023

I am struggling with channel_id. It seems to be nonsense for gRPC, as a gRPC channel is not about multiplexing. gRPC does not appear to have any multiplexing abilities per-se; it is synchronous though.

For ssh, it would make sense (see https://datatracker.ietf.org/doc/html/rfc4254#section-5.1), but I can not think of reason that it would be relevant, except if one wanted to correlate accounting for each channel this could be the "receiver channel" from the struct mentioned in the rfc.

haussli added a commit to haussli/gnsi that referenced this issue Jul 26, 2023
morrowc pushed a commit that referenced this issue Aug 1, 2023
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 a pull request may close this issue.

2 participants