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
Bug 1880941: kube-apiserver: log non-probe requests before ready #356
Bug 1880941: kube-apiserver: log non-probe requests before ready #356
Conversation
2d3461f
to
dfac60a
Compare
dfac60a
to
0f31fd1
Compare
// ignore connections to local IP. Those clients better know what they are doing. | ||
if pth := "/" + strings.TrimLeft(r.URL.Path, "/"); pth != "/readyz" && pth != "/healthz" && pth != "/livez" { | ||
if isLocal(r) { | ||
klog.V(2).Infof("Loopback request to %q (user agent %q). This client probably does not watch /readyz and might get inconsistent answers.", r.URL.Path, r.UserAgent()) |
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.
why not as klog.Warningf
?
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.
copy & paste. But I agree.
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.
Actually, no. V(2).Info
is just right. We still have few clients that make use of this (disaster recovery for example). It's not a warning, just an info.
0f31fd1
to
e60aead
Compare
5ec3d27
to
1962108
Compare
@@ -275,6 +284,7 @@ func TestAuthenticationAuditAnnotationsDefaultChain(t *testing.T) { | |||
RequestInfoResolver: &request.RequestInfoFactory{}, | |||
RequestTimeout: 10 * time.Second, | |||
LongRunningFunc: func(_ *http.Request, _ *request.RequestInfo) bool { return false }, | |||
Serializer: serializer.NewCodecFactory(scheme), |
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.
not needed anymore?
@@ -100,3 +91,45 @@ func WithLateConnectionFilter(handler http.Handler) http.Handler { | |||
handler.ServeHTTP(w, r) | |||
}) | |||
} | |||
|
|||
// WithNonReadyRequestLogging rejects the request until the process has been ready once. | |||
func WithNonReadyRequestLogging(handler http.Handler, hasBeenReadyCh <-chan struct{}, s runtime.NegotiatedSerializer) http.Handler { |
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.
s runtime.NegotiatedSerializer
not used ?
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.
not anymore
028da8a
to
395964f
Compare
/retest |
staging/src/k8s.io/apiserver/pkg/server/patch_genericapiserver.go
Outdated
Show resolved
Hide resolved
0bf1538
to
31e7ad4
Compare
/retest |
Broken base images 😞 |
31e7ad4
to
68c745d
Compare
68c745d
to
8ca9c03
Compare
/retest |
1 similar comment
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: p0lyn0mial, sttts 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 |
@sttts: This pull request references Bugzilla bug 1880941, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
@sttts: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged:
These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Bugzilla bug 1880941 has not been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Log and create NonReadyRequests event.