-
-
Notifications
You must be signed in to change notification settings - Fork 324
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
Add support for user impersonation #797
Conversation
Codecov Report
@@ Coverage Diff @@
## master #797 +/- ##
==========================================
- Coverage 72.01% 71.84% -0.17%
==========================================
Files 55 55
Lines 3730 3747 +17
==========================================
+ Hits 2686 2692 +6
- Misses 1044 1055 +11
Continue to review full report at Codecov.
|
Fixes kube-rs#796 Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
9a83753
to
57b789f
Compare
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
|
||
#[derive(Clone)] | ||
/// Layer that adds a static set of extra headers to each request | ||
pub struct ExtraHeadersLayer { |
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.
I thought we can use Option<SetRequestHeaderLayer>
for user and groups. But I guess this is necessary because impersonate-group: a,b,c
is not supported :(
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.
Yeah.. SRHL also means that we need to encode the exact number of headers into the type signature (or do a bunch of newtyping to hide it).
HeaderName::from_static("impersonate-user"), | ||
HeaderValue::from_str(impersonate_user) | ||
.map_err(http::Error::from) | ||
.map_err(Error::HttpError)?, |
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.
Might be better to add an explicit error for invalid header value with the name.
Error::HttpError
is only used by these
and I think it should be removed eventually.
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.
Agreed, but that also feels like a part of the larger error refactoring.
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.
Yeah, we can do it later.
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.
naming nits, but functionality looks sensible
if let Ok(url) = std::env::var("KUBE_RS_DEBUG_OVERRIDE_URL") { | ||
tracing::warn!(?url, "overriding cluster URL"); | ||
match url.parse() { | ||
Ok(uri) => { | ||
self.cluster_url = uri; | ||
} | ||
Err(err) => { | ||
tracing::warn!( | ||
?url, | ||
error = &err as &dyn std::error::Error, | ||
"failed to parse override cluster URL, ignoring" | ||
); | ||
} | ||
} | ||
} |
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.
you can put the secondary proxyUrl
in the ~/.kube/config
under the Cluster
to have it work everywhere, but i guess you want some safety here for something that is kube only?
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.
nevermind i see you want this as a method to be used with kubectl proxy
rather than kube doing the proxying (which we support already)
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.
Yep, the latter.
There have also been cases where I have wanted to access the cluster in weird ways involving TCP reverse proxies, so this would have been helpful there as well.
Fixes #796
Also ended up adding support for user impersonation at all, because until now those config fields were simply ignored.