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
⚠️ Implement secure diagnostics (metrics, pprof, log level changes) #9264
Conversation
b6cdb3b
to
362a49d
Compare
8a2de8c
to
d74ee1f
Compare
d74ee1f
to
caa59bc
Compare
This PR is now ready for review. I will finish the open todos once we have consensus on the changes |
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.
This is great work.
Code looks good to me, haven't had time to test it locally yet though. Will look to set it up over the next couple of days. Happy to approve given others have been able to test it.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini, killianmuldoon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Oh and thanks for the docs! They'll come in useful 😄 |
cc @bavarianbidi Just fyi given previous work/interest on metrics. Just in case CAPI or CR metrics are still interesting for you |
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.
one nit
6627c77
to
fc6f48e
Compare
@chrischdi PTAL :) |
Signed-off-by: Stefan Büringer buringerst@vmware.com
fc6f48e
to
500fc1f
Compare
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.
/lgtm
LGTM label has been added. Git tree hash: 83d875ee147eb33e2020c2672e73dff6a7e90bff
|
/hold cancel |
Thx everyone for the reviews!! |
Signed-off-by: Stefan Büringer buringerst@vmware.com
What this PR does / why we need it:
Changes to flags
--metrics-bind-addr
flag--diagnostics-address
(default::8443
)--insecure-diagnostics
(default:false
)With the following behavior
--metrics-bind-addr=<addr>
is set:--diagnostics-address=<addr> --insecure-diagnostics
is set--metrics-bind-addr
--diagnostics-address
is setDefault behavior:
--metrics-bind-addr
set: insecure serving on the specified address (as before)CAPI_DIAGNOSTICS_ADDRESS=localhost:8080
&CAPI_INSECURE_DIAGNOSTICS=true
: insecure serving on localhost:8080This should allow a smooth transition as folks can set either the env vars for clusterctl or
--metrics-bind-addr
directly on the binary to preserve the current behaviorIt is now possible to easily and securely expose metrics in production. Additionally, the pprof endpoint can now be also always enabled and it is possible to change log levels dynamically.
This should make it a lot easier to debug Cluster API in production.
Open TODOs:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #9289