-
Notifications
You must be signed in to change notification settings - Fork 31
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
fix(oauth): use Kubernetes client to query OAuth server #882
Conversation
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.
Code looks good. Haven't had a chance to build and test this just yet.
Reviewing this reminded me that I had a branch in progress some time ago where I was working on creating a single long-lived OpenShiftClient
instance, since that only depends on the service account token which shouldn't be changing within the lifecycle of a Cryostat container instance. There should be no need to create a new client instance for every request, and this would also apply to the new OkHttpClient
instance, too. WDYT? This can/should be done as a separate refactoring, I think.
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.
Built, deployed, and tested - everything looks like it works now.
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.
Code looks good to me. My quicklab cluster failed to install yesterday - trying again today.
I think that definitely makes sense for the requests using Cryostat's service account. We'd still need separate clients for the requests using the user's token, but I think this would still be a nice savings. |
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.
Tested, looks good
This PR uses the underlying HTTP client of the Fabric8 Kubernetes Client to make the GET request to query OAuth server metadata from the API server. The HTTP client is already configured to trust the cluster's root CA in order to talk to the API server as we do elsewhere in Cryostat [1].
We could instead add the CA cert bundle to our trust store at startup, but I'm concerned there may be edge cases where the CA bundle isn't mounted in the expected place. Leaving this to the Fabric8 client should give us less to think about. There's also some documentation that sounds like we should avoid trusting the CA bundle for anything other than talking to Kubernetes endpoints [2].
OkHttp and Jackson Databind are already dependencies of the Fabric8 Kubernetes Client, so we're not adding anything new.
In the tests, I had to remove the check that the TokenProvider is only called once. Some code paths now result in calling this twice. Maybe there's a better way to do this.
Fixes: #881
[1] https://github.com/fabric8io/kubernetes-client#configuring-the-client
[2] https://kubernetes.io/docs/tasks/tls/managing-tls-in-a-cluster/#trusting-tls-in-a-cluster