add cmd-drop-env to AuthProviderConfig #1074
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1074 +/- ##
==========================================
+ Coverage 72.43% 72.47% +0.04%
==========================================
Files 65 65
Lines 4759 4763 +4
==========================================
+ Hits 3447 3452 +5
+ Misses 1312 1311 -1
|
| @@ -360,9 +360,14 @@ fn token_from_gcp_provider(provider: &AuthProviderConfig) -> Result<ProviderToke | |||
| // Command-based token source | |||
| if let Some(cmd) = provider.config.get("cmd-path") { | |||
| let params = provider.config.get("cmd-args").cloned().unwrap_or_default(); | |||
|
|
|||
| let drop_env = provider.config.get("cmd-drop-env").cloned().unwrap_or_default(); | |||
There was a problem hiding this comment.
This is basically just a freestyle property through the upstream map: https://github.com/kubernetes/client-go/blob/7697067af71046b18e03dbda04e01a5bb17f9809/tools/clientcmd/api/types.go#L167-L172 right?
I'm OK with it, but would add a comment that it's a non-standard property that's not used in client-go/kubectl if there's no upstream equivalent for it.
|
I think this is OK. It's kind of a hidden property that's impossible to detect if we break it, and if upstream adds it we will likely want to conform, but I guess if that happens we can always throw in a warning message for half a year to try to help. |
|
Thank you. Added a note. |
|
Minor nit, plus DCO needs to be fixed then happy to merge! |
aviramha
force-pushed
the
command_based_token_drop_env
branch
from
852fcc9
to
2ef5edc
Compare
Co-authored-by: Eirik A <sszynrae@gmail.com> Signed-off-by: Aviram Hassan <aviramyhassan@gmail.com>
aviramha
force-pushed
the
command_based_token_drop_env
branch
from
2ef5edc
to
e3eb17c
Compare
Fixed |
This is a follow up after #1059 . We found out that
AuthProviderConfigdoes pretty much the same (leaking env vars) but in a different implementation.This is pretty much a draft in my opinion, not sure if cleanest approach. I'm thinking maybe Config should have this as a field instead and it is shared among all implementations that use
Command..