-
Notifications
You must be signed in to change notification settings - Fork 706
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
Switch CheckNamespaceExist to return a connect error as expected by client #6319
Conversation
…lient. Signed-off-by: Michael Nelson <minelson@vmware.com>
✅ Deploy Preview for kubeapps-dev canceled.
|
### Description of the change Follows on from #6319, cleaning up the resources plugin so that it uses the connect error. ### Applicable issues - ref #6269 ### Additional information PRs will follow for the helm and flux plugins, before removing the original status error code. Signed-off-by: Michael Nelson <minelson@vmware.com>
Hi @absoludity , thanks for the quick work on this! 🚀 |
No problem - it's great to be fixing things people need :) I'm waiting for the development image to be published so you can verify the fix, then after than @ppbaena and the team will chat about the next release. |
@RGPosadas Can you please test the fix by running with this kubeapps-apis image sha, and verify whether the issue is fixed or continues? Thanks. |
@absoludity Just confirmed that the auth loop is no longer blocking authenticated users. I am still getting correct 403s on the default namespace (and other non-authorized resources), and getting valid 2XX for the correct resources on login |
Great, thanks for the confirmation. I'll chat with Pepe about a release soon :) |
Sounds good! |
Hi @RGPosadas , we chatted last night about this. I'll aim to finish the related work (switching over all the other call-sites that are sending the old encoded errors, even though they don't cause problems like this call-site did) and release once that work is done, hopefully later next week. |
Gotcha, thanks for the update. Will keep an eye out for the next 2 weeks 🚀 |
Description of the change
When we switched to connect-grpc, we continued sending standard gRPC error codes from Kubeapps-APIs, and without more information, the connect-gRPC machinery was converting these to code unknown.
When we check that a user has authenticated successfully by getting the default namespace after the login dance, either an OK or a forbidden means the request was authenticated, unauthenticated means it was not. But instead the error was coming back with an unknown code and so Kubeapps was not recognising this as authenticated. This was affecting logins for users who do not have permission to get the default namespace (see #6269 for the workaround).
I reproduced this locally (though with token auth) and so before this change, the gRPC request to CheckNamespaceExists came bace with what looks like a permission denied response - but the grpc-status code is 2 - which is unknown:
After this change, the error arrives as a PermissionDenied as expected:
which Kubeapps happily recognises and assumes the user is now authenticated.
Benefits
People can login without requiring RBAC to get the default namespace.
Possible drawbacks
None
Applicable issues
Additional information
Although I was glad to find the fix here, I'm still uncertain why our CI integration tests did not pick this up, given that the intention is that we check login with an unprivileged user. I need to investigate that further.
Also, this is a second attempt at fixing this after #6312 ended up touching too many call-sites. I'll then followup switching other call-sites and removing the old code.