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

proxy access log #2301

Merged
merged 3 commits into from
Jul 15, 2021
Merged

proxy access log #2301

merged 3 commits into from
Jul 15, 2021

Conversation

butonic
Copy link
Member

@butonic butonic commented Jul 14, 2021

We now use a dedicated middleware to log all requests, regardless of routing selector outcome. The log now includes the remote address and the selected routing policy.

@butonic butonic requested review from refs and C0rby July 14, 2021 11:44
@IljaN
Copy link
Member

IljaN commented Jul 14, 2021

Could we log request bodies in Debug mode?

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic
Copy link
Member Author

butonic commented Jul 14, 2021

Could we log request bodies in Debug mode?

how much of it? 100 bytes, kb or mb? I think if you want to log all the request bodies adding a real proxy would make more sense? or what use case do you have in mind?

@IljaN
Copy link
Member

IljaN commented Jul 14, 2021

what use case do you have in mind?

Mainly debugging json and webdav bodies, headers. Yeah I didn't have file-uploads in mind so this might be not viable. Maybe based on content-type. But maybe this is to complicated or should be an additional param.

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@sonarcloud
Copy link

sonarcloud bot commented Jul 14, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@butonic butonic self-assigned this Jul 14, 2021
@butonic butonic added Type:Bug Category:Defect Existing functionality is not working as expected labels Jul 14, 2021
@butonic
Copy link
Member Author

butonic commented Jul 15, 2021

@refs we might be able to log the routing policy by making the selector decision in a middleware and putting it in a context. Then the proxy handler can read it from the context. but since that is a little bigger refactoring I'd defer that to another PR. The routing policy is logged in debug mode. We at least now hale a proper access log.

wrap := middleware.NewWrapResponseWriter(w, r.ProtoMajor)
next.ServeHTTP(wrap, r)

logger.Info().
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the log be called before next.ServeHTTP?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, the log needs to know the duration so it has to happen after next.ServeHTTP(wrap, r)

next.ServeHTTP(wrap, r)

logger.Info().
Str("proto", r.Proto).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Str("proto", r.Proto).
Str("protocol", r.Proto).

@refs
Copy link
Member

refs commented Jul 15, 2021

GET THIS IN, THIS IS GOOD NEWS!

@refs refs merged commit 6f37b38 into master Jul 15, 2021
@refs refs deleted the proxy-access-log branch July 15, 2021 11:42
ownclouders pushed a commit that referenced this pull request Jul 15, 2021
Merge: afa9afd 8c7f1f0
Author: Alex Unger <6905948+refs@users.noreply.github.com>
Date:   Thu Jul 15 13:42:08 2021 +0200

    Merge pull request #2301 from owncloud/proxy-access-log

    proxy access log
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Defect Existing functionality is not working as expected Type:Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants