Skip to content

improve proxy healthz checking #321

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: sig-auth-acceptance
Choose a base branch
from
Open

improve proxy healthz checking #321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions cmd/kube-rbac-proxy/app/kube-rbac-proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,11 +226,12 @@ func Run(cfg *server.KubeRBACProxyConfig) error {
return err
}

var stoppedCh, listenerStoppedCh <-chan struct{}
go func() {
defer wg.Done()
defer cancel()

stoppedCh, listenerStoppedCh, err := cfg.SecureServing.Serve(mux, 10*time.Second, serverCtx.Done())
stoppedCh, listenerStoppedCh, err = cfg.SecureServing.Serve(mux, 10*time.Second, serverCtx.Done())
if err != nil {
klog.Errorf("%v", err)
return
Expand All @@ -244,7 +245,9 @@ func Run(cfg *server.KubeRBACProxyConfig) error {
// we need a second listener in order to serve proxy-specific endpoints
// on a different port (--proxy-endpoints-port)
proxyEndpointsMux := http.NewServeMux()
proxyEndpointsMux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte("ok")) })
proxyEndpointsMux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) {
Copy link
Collaborator

@ibihim ibihim Nov 18, 2024

Choose a reason for hiding this comment

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

I think the previous /healthz was completely fine. When the proxy go routine would finish, it would also cancel the health endpoint.

What you added might be worth to be used as liveness or whatever you call it. But the question is, if this makes sense or if this is just testing go std lib. Maybe we could ask @enj, if this makes sense.

Copy link

@enj enj Dec 20, 2024

Choose a reason for hiding this comment

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

I am unsure of exactly what this trying to do, but I do think that you need something better than just "always return ok." For example, I would at a minimum expect a check that asserts that the upstream is working. I don't know if checking this specific listener is the correct approach though. From the YAML below I think this is acting as a readiness check and not a health check?

Copy link
Collaborator Author

@stlaz stlaz Jan 3, 2025

Choose a reason for hiding this comment

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

I was trying to set up a few checks that make sure that the other listener is capable of responding to requests.

I am not sure we want to add an upstream healthz check here - the proxy info port is supposed to serve information about the proxy itself. The healthz of upstream should probably be checked directly at upstream (or should be accessed via a permissive proxied path).

Copy link
Collaborator

@ibihim ibihim Mar 20, 2025

Choose a reason for hiding this comment

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

The previous health check is fine. This would be more like a readiness check?

At the beginning I had the opinion that @enj had, that we would need to "somehow check upstream", but your arguments are also right: this is the concern of upstream itself.
Now, I have no strong opinions. I see the reason behind a connection check and I am still not sure if adds enough usefulness.

proxyHealtzCheck(w, cfg.SecureServing.Listener.Addr().String(), listenerStoppedCh, stoppedCh)
})

if err := wg.Add(1); err != nil {
return err
Expand Down Expand Up @@ -376,3 +379,31 @@ func copyHeaderIfSet(inReq *http.Request, outReq *http.Request, headerKey string
outReq.Header.Add(headerKey, v)
}
}

func proxyHealtzCheck(w http.ResponseWriter, localProxyAddr string, listenerStoppedChan, stoppedChan <-chan struct{}) {
select {
case <-stoppedChan:
http.Error(w, "the proxying port serving logic has stopped", http.StatusServiceUnavailable)
return
case <-listenerStoppedChan:
http.Error(w, "listener stopped", http.StatusServiceUnavailable)
return
default:
}

// we need the tls.Dialer otherwise the server would log EOF for TLS handshakes
// since the connection would be cut before that was ever attempted
dialer := tls.Dialer{NetDialer: &net.Dialer{}, Config: &tls.Config{InsecureSkipVerify: true}} // the cert's not likely to have loopback IP SAN
dialCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()

// we just knock on the other listener as we don't want to trigger proxying to upstream
conn, err := dialer.DialContext(dialCtx, "tcp", localProxyAddr)
if err != nil {
http.Error(w, "failed to connect to the proxying listener", http.StatusInternalServerError)
klog.Errorf("failed to connect to the proxying listener: %v", err)
return
}
_ = conn.Close()
_, _ = w.Write([]byte("ok"))
}
44 changes: 44 additions & 0 deletions test/e2e/basics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,3 +519,47 @@ func testIgnorePaths(client kubernetes.Interface) kubetest.TestSuite {
}.Run(t)
}
}

func testHealthz(client kubernetes.Interface) kubetest.TestSuite {
Copy link

@enj enj Dec 20, 2024

Choose a reason for hiding this comment

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

A test for the failure case would make it more clear what this is checking.

return func(t *testing.T) {
command := `curl --connect-timeout 5 -v -s -k --fail https://kube-rbac-proxy.default.svc.cluster.local:8643/healthz`

kubetest.Scenario{
Name: "healtz check",
Description: "check that proxy /healthz works as expected",

Given: kubetest.Actions(
kubetest.CreatedManifests(
client,
"basics/clusterRole.yaml",
"basics/clusterRoleBinding.yaml",
"basics/deployment.yaml",
"basics/service.yaml",
"basics/serviceAccount.yaml",
),
),
When: kubetest.Actions(
kubetest.PodsAreReady(
client,
1,
"app=kube-rbac-proxy",
),
kubetest.ServiceIsReady(
client,
"kube-rbac-proxy",
),
),
Then: kubetest.Actions(
kubetest.ClientLogsContain(
client,
command,
[]string{
"{ [2 bytes data]",
"ok",
},
nil,
),
),
}.Run(t)
}
}
8 changes: 8 additions & 0 deletions test/e2e/basics/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,20 @@ spec:
image: quay.io/brancz/kube-rbac-proxy:local
args:
- "--secure-port=8443"
- "--proxy-endpoints-port=8643"
- "--upstream=http://127.0.0.1:8081/"
- "--authentication-skip-lookup"
- "--v=10"
ports:
- containerPort: 8443
name: https
- containerPort: 8643
name: proxy
readinessProbe:
httpGet:
scheme: HTTPS
port: 8643
path: healthz
- name: prometheus-example-app
image: quay.io/brancz/prometheus-example-app:v0.1.0
args:
Expand Down
3 changes: 3 additions & 0 deletions test/e2e/basics/service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,8 @@ spec:
- name: https
port: 8443
targetPort: https
- name: proxy
port: 8643
targetPort: proxy
selector:
app: kube-rbac-proxy
1 change: 1 addition & 0 deletions test/e2e/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ func TestMain(m *testing.M) {
func Test(t *testing.T) {
tests := map[string]kubetest.TestSuite{
"Basics": testBasics(client),
"Healthz": testHealthz(client),
"H2CUpstream": testH2CUpstream(client),
"IdentityHeaders": testIdentityHeaders(client),
"ClientCertificates": testClientCertificates(client),
Expand Down
Loading