Redefine "readiness" from "serving" to "serving and OIDC authenticator successfully initialized"

Question

Redefine "readiness" from "serving" to "serving and OIDC authenticator successfully initialized"

dlipovetsky opened this issue 5 years ago · 5 comments

I started the proxy while the OIDC provider was unavailable; the proxy reported itself as Ready:

$ kubectl -n authentication logs -l=app=kube-oidc-proxy
I0911 23:39:33.681175       1 proxy.go:51] waiting for oidc provider to become ready...
I0911 23:39:43.681530       1 secure_serving.go:116] Serving securely on [::]:30003
I0911 23:39:43.681628       1 proxy.go:95] proxy ready
E0911 23:39:43.695487       1 oidc.go:232] oidc authenticator: initializing plugin: 503 Service Unavailable: <html>
<head><title>503 Service Temporarily Unavailable</title></head>
<body>
<center><h1>503 Service Temporarily Unavailable</h1></center>
<hr><center>openresty/1.15.8.1</center>
</body>
</html>

$ kubectl -n authentication get deploy -l=app=kube-oidc-proxy
NAME              READY   UP-TO-DATE   AVAILABLE   AGE
kube-oidc-proxy   1/1     1            1           100m

In my view, I think the proxy should report itself as ready only after the OIDC authenticator is initalized successfully.

Answer 1 · 2019-09-12T00:07:00.000Z

The authenticator's only initializer is asychronous, and never gives up, which prevents the proxy from knowing whether the initializer has failed (though its possible to deduce this externally by looking at the proxy's log).

I'd be in favor of adding a synchronous initializer to the proxy or upstream.

Answer 2 · 2019-09-12T00:44:53.000Z

Hi there :)

I have thought about this when writing it and assumed this would come up. The reason it is the way it is is for that exact reason you mentioned. I think we should be able to test the connection ourselves and should be fairly cheap to do periodically
https://github.com/kubernetes/apiserver/blob/e72ec4e02467bb6c5f6792ca183b679109fa2614/plugin/pkg/authenticator/token/oidc/oidc.go#L128

This is definitely something I would like to see.

/kind feature

Answer 3 · 2019-09-12T01:05:04.000Z

Thanks a lot for the feedback @JoshVanL, and for your work on this excellent proxy! 🙂

What do you see as the way forward?

Propose a synchronous initializer to apiserver/plugin/pkg/authenticator/token/oidc upstream
Fork upstream and implement a synchronous initializer
Continue to use the asynchronous initializer, but perform verification on the side
Something else

(If your comment already proposes a way forward, my apologies, I must not understand it completely).

Answer 4 · 2019-09-12T01:33:03.000Z

Cheers!

Propose a synchronous initializer to apiserver/plugin/pkg/authenticator/token/oidc upstream

Judging by their implementation I think they are probably quite intent to keep it this way.

Fork upstream and implement a synchronous initializer

I'm quite keen to not fork upstream unless absolutely necessary and stay aligned.

Continue to use the asynchronous initializer, but perform verification on the side

This is the approach I was thinking of but want to make sure we're not too aggressive with the polling.

@simonswine What are your thoughts?

Answer 5 · 2019-09-13T16:42:14.000Z

I'm quite keen to not fork upstream unless absolutely necessary and stay aligned.

To be able to check the issuer, I think the proxy needs to duplicate a lot of the code in newAuthenticator: validate parameters, configure the transport, etc. Duplicating that code has most of the downsides of forking the oidc auth plugin.

On the other hand, we can add a single method to the oidc auth plugin to allow the proxy to check if the oidc authenticator is initialized. In my view, it's easier to maintain a fork with this method than duplicating most of of newAuthenticator. Of course, I'd like to get that single method upstream 🙂 .