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

Add silence-ping-logging flag #178

Merged
merged 11 commits into from
Jul 19, 2019
Merged

Add silence-ping-logging flag #178

merged 11 commits into from
Jul 19, 2019

Conversation

karlskewes
Copy link
Contributor

@karlskewes karlskewes commented Jun 5, 2019

Description

  1. Convert hard coded ping check path /ping to a new Options PingPath string so its value can be passed to other functions.
    Default remains unchanged at: "/ping"
  2. Add new Options ExcludeLoggingPaths string to contain csv list of url.Path's that should not be logged.
    Default nil = don't exclude any paths from logging.
  3. Add new Options SilencePingLogging bool that adds PingPath to the ExcludeLoggingPaths list, in effect silencing logging of requests to /ping.
    Default false = don't silence logging of requests to /ping.

An alternative implementation could be to extend the loggingHandler struct with Options?
Not sure, open to feedback and happy to revise as preferred.

Motivation and Context

Resolve issue: #167
Kubernetes health checking /ping endpoint creates unnecessary (for us) log entries.

How Has This Been Tested?

Kubernetes 1.13 and 1.14 with Google Groups OAUTH.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@karlskewes karlskewes requested a review from a team June 5, 2019 03:33
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Added a few comments inline, generally looks good but what do you think to making this ExcludePath -> ExcludedPaths, making it a list rather than a single item?

Might even be worth converting the list to a map[string]struct{} inside the logger once initialised to make looking up excluded paths easier/quicker 🤔

docs/configuration/configuration.md Outdated Show resolved Hide resolved
logger/logger.go Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
@JoelSpeed
Copy link
Member

An alternative implementation could be to extend the loggingHandler struct with Options?

I like this approach btw, it's something I'm trying to move towards with our other interfaces too

@karlskewes
Copy link
Contributor Author

Did some tests in cluster with --exclude-logging-paths=/ping,/path/to/file and --silence-ping-logging.

Generate traffic (note I'm enforcing ro file system hence write error):

$ kubectl exec -it oauth2-proxy-7585fcfc7-tn9nz sh
/ $ wget localhost:4180/ping
Connecting to localhost:4180 (127.0.0.1:4180)
wget: can't open 'ping': Read-only file system
/ $ wget localhost:4180/ping1
Connecting to localhost:4180 (127.0.0.1:4180)
wget: server returned error: HTTP/1.1 403 Forbidden
/ $ wget localhost:4180/path/to/file
Connecting to localhost:4180 (127.0.0.1:4180)
wget: server returned error: HTTP/1.1 403 Forbidden
/ $

Pod logs:

$ kubectl logs -f oauth2-proxy-7585fcfc7-tn9nz
<snip>
[2019/06/21 22:48:17] [http.go:92] HTTP: listening on 0.0.0.0:4180
[2019/06/21 22:48:44] [oauthproxy.go:679] Error loading cookied session: Cookie "_dev_oauth2_proxy_rl" not present
127.0.0.1 - - [2019/06/21 22:48:44] localhost:4180 GET - "/ping1" HTTP/1.1 "Wget" 403 2535 0.001
[2019/06/21 22:48:51] [oauthproxy.go:679] Error loading cookied session: Cookie "_dev_oauth2_proxy_rl" not present

@karlskewes
Copy link
Contributor Author

An alternative implementation could be to extend the loggingHandler struct with Options?

I like this approach btw, it's something I'm trying to move towards with our other interfaces too

Happy to amend, do you have an example interface in mind?
Did we want to pass the whole Options struct?

docs/configuration/configuration.md Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
@JoelSpeed
Copy link
Member

@kskewes Could you fix up the merge conflicts and we can try get this merged this week? Thanks

Add ability to silence logging of requests to /ping endpoint, reducing
log clutter

Pros:
- Don't have to change all handlers to set/not set silent ping logging
- Don't have to duplicate `loggingHandler` (this could be preferable yet)

Cons:
- Leaking oauth2proxy logic into `package logger`
- Defining default pingPath in two locations

Alternative:
- Add generic exclude path to `logger.go` and pass in `/ping`.
Useful for excluding /ping endpoint to reduce log volume.
This is somewhat more verbose than a simple bool to disable logging of
the `/ping` endpoint.

Perhaps better to add `-silence-ping-logging` bool flag to `options.go` and
pass in the `/ping` endpoint as part of `logger` declaration in `options.go`.

Could be extended into a slice of paths similar to go-gin's `SkipPaths`:
https://github.com/gin-gonic/gin/blob/master/logger.go#L46
Can probably slim down the `ExcludePath` tests.
- Add `ping-path` option to enable switching on and passing to `logger.go`
  Default remains unchanged at: `"/ping"`
- Add note in configuration.md about silence flag taking precedence

Potential tests:
- `options.go` sets `logger.SetExcludePath` based on silence flag?
- Changing `PingPath` reflected in router?
- `logger.go` convert slice of paths to map for quicker lookup
- `options.go` combines csv paths and pingpath into slice
@karlskewes
Copy link
Contributor Author

Please hold whilst I verify the rebase in our clusters.

@karlskewes
Copy link
Contributor Author

Working for us subject to resolving this: #212

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

One little thing and then we can merge

options.go Outdated Show resolved Hide resolved
Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks @kskewes

@karlskewes
Copy link
Contributor Author

Thanks for patience and picking that up!

@JoelSpeed JoelSpeed merged commit 8635391 into oauth2-proxy:master Jul 19, 2019
Jing-ze pushed a commit to Jing-ze/oauth2-proxy that referenced this pull request Nov 19, 2024
Signed-off-by: bitliu <bitliu@tencent.com>
T-vK pushed a commit to T-vK/oauth2-proxy that referenced this pull request Nov 20, 2024
Avoid unnecessary pod restart on each helm chart version
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.

3 participants