-
Notifications
You must be signed in to change notification settings - Fork 3
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
Log network activity in rVASP #145
Conversation
looks like the tests are failing - I will need to look into these. |
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.
Glad you were able to import the package, I had a comment about one more thing we need to add to ensure that the activity handler starts up.
@@ -40,6 +41,8 @@ func (s *Server) fetchSigningKey(peer *peers.Peer) (key *rsa.PublicKey, err erro | |||
} | |||
|
|||
if peer.SigningKey() == nil { | |||
// send key exchange activity to network activity handler | |||
activity.KeyExchange().Add() |
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.
Nice! We will also need to add a call to activity.Start()
when we're starting the rVASP server, (in New()
of rvasp.go).
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.
Looks good, I had one small thing we should address but then we can merge this in.
pkg/rvasp/rvasp.go
Outdated
@@ -88,6 +89,7 @@ func New(conf *config.Config) (s *Server, err error) { | |||
s.peers.Connect(grpc.WithTransportCredentials(insecure.NewCredentials())) | |||
} | |||
s.updates = NewUpdateManager() | |||
activity.Start(conf.Activity) |
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.
Doesn't this return an error? If so, it should be handled like the other service errors here so that we're not confused if there's bad configuration and it doesn't start.
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.
good point - I have made the change
This PR resolves SC-20306 that asked to log network activity in rVASP. This includes calls to Search, Lookup, and KeyExchanage. This is currently pulling from the main branch of the directory repo.