Skip to content

chore: log requests upon retrieval #1140

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

Merged
merged 8 commits into from
Aug 24, 2023
Merged

chore: log requests upon retrieval #1140

merged 8 commits into from
Aug 24, 2023

Conversation

RTann
Copy link
Collaborator

@RTann RTann commented Mar 29, 2023

This came up when looking into potential network issues between Central and Scanner. These logs will let us know about all incoming traffic to the HTTP and gRPC endpoints which are exposed by Scanner.

@ghost
Copy link

ghost commented Mar 29, 2023

Images are ready for the commit at 1ccc31e.

To use the images, use the tag 2.30.x-52-g1ccc31e529.

@dcaravel dcaravel self-requested a review August 22, 2023 19:00
@RTann RTann force-pushed the ross/server-logging branch from 524c2ce to 679c526 Compare August 22, 2023 19:24
@RTann RTann changed the title log requests upon retrieval chore: log requests upon retrieval Aug 22, 2023
@RTann RTann force-pushed the ross/server-logging branch 2 times, most recently from f371323 to fbb7c9c Compare August 22, 2023 22:56
Copy link
Contributor

@dcaravel dcaravel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the logs in the artifacts of the associated CI, made a few general comments.

Certainly an improvement.

Comment on lines +206 to +210
imageName := "unknown"
if req.GetImage() != "" {
imageName = req.GetImage()
}
logrus.Infof("Fetching vulnerabilities for %s", imageName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add to the backlog modifying the invocations from Central/Sensor to add the image name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 18 to 26
return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
peerInfo, exists := peer.FromContext(ctx)
if !exists {
return nil, status.Error(codes.InvalidArgument, "unable to parse peer information from request context")
}

logrus.WithFields(map[string]interface{}{
"Method": info.FullMethod,
}).Infof("Received gRPC request from %v", peerInfo.Addr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert on grpc-gateway - but looks like it supports adding X-Forwarded-For header to the context

Guessing should be able to extract the Metadata from the context, something like:

addrs := metadata.ValueFromIncomingContext(ctx, "X-Forwarded-For")

And then append the addrs to this log

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't seem like we can rely on this:

{"Event":"Received gRPC request from unknown","HEADERS":{":authority":["localhost:8443"],"content-type":["application/grpc"],"user-agent":["grpc-go/1.57.0"]},"Level":"info","Location":"logging_interceptor.go:28","Method":"/scannerV1.ImageScanService/GetImageComponents","Time":"2023-08-23 22:25:36.923706"}

Going to revert back to the original but leave out the peer's address because it added no value

@RTann RTann force-pushed the ross/server-logging branch 3 times, most recently from 604ba3f to 1ccc31e Compare August 24, 2023 02:22
@RTann RTann force-pushed the ross/server-logging branch from 1ccc31e to 526adf6 Compare August 24, 2023 03:04
@RTann RTann enabled auto-merge (squash) August 24, 2023 03:04
@RTann RTann merged commit 980f4eb into master Aug 24, 2023
@RTann RTann deleted the ross/server-logging branch August 24, 2023 03:16
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.

2 participants