support impersonation via auth_options and in kubectl config #600
Conversation
lib/kubeclient.rb
Outdated
| @headers[:'Impersonate-Uid'] = auth_as_uid | ||
| end | ||
| @auth_options[:as_user_extra]&.each do |k, v| | ||
| raise ArgumentError, "Kubeclient does not support impesonate extra field with multiple values" if v.count > 1 |
There was a problem hiding this comment.
@cben what do you think about this error? this may introduce regression on users side. Should we print just some warning instead and fallback to trimming arrays with > 1 value to just the first value?
There was a problem hiding this comment.
Just for context, as mentioned in the PR summary, this is because Faraday does not support multiple headers with same name, however the Kube API is expecting those to be requested as duplicate headers (with same name, and each different value). It does not recognize joining the value as comma separated lists.
There was a problem hiding this comment.
yikes. doesn't the RFC require "aside from the well-known exception noted below, a sender MUST NOT generate multiple field lines with the same name in a message (whether in the headers or trailers) or append a field line when a field line of the same name already exists in the message, unless that field's definition allows multiple field line values to be recombined as a comma-separated list" ? (with the grandfathered exception being Set-Cookie)
Is there any plan to fix that in kubernetes API? (not that this helps us much here)
Is this supported better in Faraday 2?
There was a problem hiding this comment.
@cben I've opened issue in faraday and actually it is down at the http libraries. Neither ruby's net/http nor exconn supports that. Maybe other client library does (my tip would be typhoeues, but did not test). Anyway I guess as kubeclient we don't want to impose the faraday adapter used on the user? Since user may set global adapter for Faraday that would also be used by kubeclient.
So I think there is nothing to do, except implementing own client? Or maybe forcing specific faraday adapter in kubeclient requests.
There was a problem hiding this comment.
There is also some issue in kubernetes around this problem, from using http proxies, that often reconstruct headers by joining them with comma, which breaks the semantic. But no fix seems to be on the way.
There was a problem hiding this comment.
I see, thanks for the pointers. It's been open for several years, and even if k8s add a new interface in future, it's useful for kubeclient to be able to function against existing k8s versions. 👍
Compatibility on existing kubeconfig files is a good question, but given that previous behavior was a bug (impersonation settings were ignored), I'm not worried much about it.
Also, we're in the weird state that master branch has breaking changes since 4.y releases, but no 5.y releases were made yet, sparing the question of how exactly to semver it 😉
- I like that
Configsupports multiple values but onlyClientwill complain ✔️
There was a problem hiding this comment.
Should we print just some warning instead and fallback to trimming arrays with > 1 value to just the first value?
Not now. It would be incorrect & result will depend on order listed in kubeconfig.
If we do this invisibly, people may keep depending on it, making it hard to revert such a workaround...
I prefer merging with it correctly raising an error, and waiting for people to complain. Then they are in position to change their kubeconfigs, OR add custom trimming in their code between Config and Client — OR convince us the hassle is sufficient to bake the trimming kludge into kubeclient...
What about watches?Watches still use https://github.com/httprb/http gem (#488 having some unresolved issues).
Otherwise LGTM 🚀 |
|
@cben thanks for the review. I've added the changelog and unit test for watch. Please take a look. |
DocX
changed the title
|
Thanks @cben. Is there any plan to publish new release with the update? |
|
I need to make progress on my 5.0 changelog backlog to get master branch releasable... Alternatively, do you want to work on backporting this to |
|
Ok, no problem. Just asking. It is not super critical for us, so we can wait (and we can always install gem from git if we need to) |
This PR is rebase and update of #524 to current master version
We tested it and it works as expected
Ruby http clients (including adapters for Faraday) are not able to handle multiple headers of same name, which is how the kube API expects to receive the list of multiple groups or extra fields. To avoid any confusion, added explicit error when trying to configure client with multivalue groups. This may brake some applications, if user is not aware of the impersonation setting and were ignoring it until now