-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Mo: add option to turn off stream logs when access log is set to off #3701
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3701 +/- ##
==========================================
+ Coverage 51.95% 52.35% +0.40%
==========================================
Files 59 59
Lines 16762 16880 +118
==========================================
+ Hits 8708 8838 +130
+ Misses 7755 7747 -8
+ Partials 299 295 -4 ☔ View full report in Codecov by Sentry. |
Hi @Binsabbar Thanks for submitting this pull request. We're currently reviewing your proposed changes with the team to investigate the impact of this and determine if we need to use another config map key for the stream block. We'll let you know as soon as possible. Thanks for your patience. |
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.
Seems reasonable to me. @Binsabbar can you please update your commit message and PR title following the guidelines? Thanks!
Sure I will work on that |
@lucacome @haywoodsh I am having a second though about how I implemented this. Currently there are two variables to control
In my PR I used I can think of two options:
what do you think? (I will need some directions on implementing option 2) |
HI @brianehlert , can you provide me with a preferred option? and in case of option 2, can you provide me with direction? |
I think I found out how to do it and introduce a new I will work on this one this week, and update my MR. I think introducing a new variable is cleaner |
Hi @Binsabbar apologies for the delay on our responding to this. Since this would be a new There is currently a PR open which will be re-structuring our docs. I would wait for this PR to be merged before making any docs related changes (This PR is expected to be merged today 🤞 ) #4620 |
This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
We recently merged a PR that changed logging behaviour. We interested in brining this functionality from http to stream. We have an issue for it here #6171. Are you still interested in doing this? |
Proposed changes
During DoS attack the stream logs still writes logs, unlike when it is in HTTP, where the DoS automatically turn off access log based on the value of
$loggable
used in dos access log.Since there is already an option to turn off
access_log
for default http server, it was not added for default stream server. I used the same variable to also turn off the stream access log.If a new variable is preferred, please let me know.
Checklist
Before creating a PR, run through this checklist and mark each as complete.