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

Log successful kube-probe requests at Trace lvl #421

Merged
merged 1 commit into from
Mar 31, 2021

Conversation

dnwe
Copy link
Contributor

@dnwe dnwe commented Nov 5, 2020

When deployed and running in Kubernetes, step-ca will output a log statement every 5 seconds at INFO level from the ongoing readiness/liveness probe requests. This PR changes the behaviour so that successful kube-probe requests will only be logged at Trace level by default.

Fixes #420

@codecov-io
Copy link

Codecov Report

Merging #421 into master will decrease coverage by 0.82%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #421      +/-   ##
==========================================
- Coverage   76.33%   75.50%   -0.83%     
==========================================
  Files          92       97       +5     
  Lines        9109     9271     +162     
==========================================
+ Hits         6953     7000      +47     
- Misses       1773     1880     +107     
- Partials      383      391       +8     
Impacted Files Coverage Δ
logging/handler.go 66.66% <100.00%> (ø)
logging/logger.go 0.00% <0.00%> (ø)
logging/responselogger.go 50.00% <0.00%> (ø)
logging/context.go 0.00% <0.00%> (ø)
logging/clf.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98a5aa5...34fbb28. Read the comment docs.

@maraino
Copy link
Contributor

maraino commented Nov 9, 2020

I understand your issue, the main problem is that right now we don't have a way to define the log level. And those logs will be lost, not only that, I can change my user agent to hide my activity.

I have to discuss this internally.

@maraino
Copy link
Contributor

maraino commented Nov 9, 2020

The logging package is gonna be replaced by this package, and I've added an issue to be able to configure the debug level and to be able to hide liveness/readiness probes (smallstep/logging#2)

@dopey
Copy link
Contributor

dopey commented Nov 18, 2020

Hey @dnwe thanks again for the submission! We spent some time discussing this morning. As @maraino we're planning on switching up the logger soon which should make this type of thing easier. But we're not exactly sure when that will happen. Prioritization is hard 🤕

With regards to this submission we'd be open to pulling it in to fix the short term need but we'd like to see a few changes:

  1. Condition on the Path /health rather than the user agent.
  2. Put the whole section in a IF clause that's triggered by an environment variable e.g. STEP_*. This way users will need to explicitly ask to remove the logs.

If you're open to making those changes, we'll happily pull this in. With the caveat that sometime in the future this implementation will likely change.

@dnwe
Copy link
Contributor Author

dnwe commented Nov 25, 2020

@dopey sure those recommended changes make sense 👍 I'll take a look at updating the PR

Adds `STEP_LOGGER_ONLY_TRACE_HEALTH_ENDPOINT` environment variable to
opt-in to only logging successful /health requests at the Trace level

Fixes smallstep#420

Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>
@dnwe dnwe force-pushed the only-trace-for-kube-probe branch from 34fbb28 to 9ee4441 Compare March 9, 2021 14:53
@dnwe
Copy link
Contributor Author

dnwe commented Mar 9, 2021

@dopey apologies I'd forgotten about this open PR, I whipped up a draft replacement based on the discussion above, but wasn't sure what environment-variable naming you were looking for. I proposed STEP_LOGGER_ONLY_TRACE_HEALTH_ENDPOINT but feel free to suggest changes if you had a preferred one in mind

@dopey dopey self-requested a review March 16, 2021 17:20
Copy link
Contributor

@dopey dopey left a comment

Choose a reason for hiding this comment

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

lgtm

@dopey
Copy link
Contributor

dopey commented Mar 31, 2021

Hey @dnwe thanks for the PR! Merging.

@dopey dopey merged commit cdbdd74 into smallstep:master Mar 31, 2021
@dnwe dnwe deleted the only-trace-for-kube-probe branch April 7, 2021 10:03
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.

step-ca frequently logging HTTP 200 requests from kubernetes liveness/readiness probes
4 participants