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

Added auth and standard logging with templating and file rolling #52

Merged
merged 19 commits into from
May 1, 2019

Conversation

MisterWil
Copy link
Contributor

@MisterWil MisterWil commented Feb 10, 2019

Description

Created a logging package to handle logging standard, authentication, and request logs with templating. This package is roughly the same as the stdlib log package but with some additional handling for customizing the template of the logger output.

Added configuration options for outputting logs to a file with support for log rolling using the lumberjack library.

Motivation and Context

Done to meet the requirements for issue #45.

How Has This Been Tested?

Despite many files being impacted, most of those changes were to just replace existing Printf/Fatal logging lines to run through the new logging package.

Testing via the standard unit tests provided, modifying only the logging_handler test to point to the new constants and changed constructor.

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.

@MisterWil MisterWil requested a review from a team February 10, 2019 17:04
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
options.go Show resolved Hide resolved
@terala
Copy link

terala commented Mar 6, 2019

I am ready with a pull request to use zap for logging. This would log in json, suitable for logging apps like splunk and elastic. Should I go ahead with submitting the pull request? It would change all if these.

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.

If you could rebase and fix the conflicts and fix the couple of comments I've made I'm happy to get this merged! Thanks for your work so far :)

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
@JoelSpeed JoelSpeed mentioned this pull request Mar 13, 2019
3 tasks
@JoelSpeed
Copy link
Member

@MisterWil Do you have any time to complete this?

@MisterWil
Copy link
Contributor Author

MisterWil commented Apr 12, 2019

Sorry for the delay. This just got lost in the shuffle. I believe I've resolved all of the merge conflicts, added the changes you requested, and all tests pass. :-)

ETA: Shoot, I just noticed it looks like a bungled the merge somehow and blew up the commit log. I was trying to fix some problems with my environment at the same time and I have no idea what happened.

@MisterWil MisterWil changed the base branch from master to kubernetes April 12, 2019 20:14
@MisterWil MisterWil changed the base branch from kubernetes to master April 12, 2019 20:14
@MisterWil
Copy link
Contributor Author

Alright I managed to fix it. I'm not sure what happened but editing this PR to point to a different branch and then pointing it back to master fixed the issue. Please let me know if anything else needs to be done before this can be pulled in - sorry again for the delay.

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 below, looks like there are a few semi-colons instead of colons in error messages, unless you had intended this?

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
oauthproxy.go Outdated Show resolved Hide resolved
oauthproxy.go Outdated Show resolved Hide resolved
oauthproxy.go Outdated Show resolved Hide resolved
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, let's get this merged and take it for a spin!

@JoelSpeed JoelSpeed merged commit 93b7d31 into oauth2-proxy:master May 1, 2019
@MisterWil MisterWil deleted the enhanced_logging branch May 1, 2019 14:47
Jing-ze pushed a commit to Jing-ze/oauth2-proxy that referenced this pull request Nov 19, 2024
T-vK pushed a commit to T-vK/oauth2-proxy that referenced this pull request Nov 20, 2024
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