Adding google cloud auth per exec #328

gnunicorn · 2020-10-07T16:10:11Z

Looks like the latest gcloud-setup uses some exec-path-features in the kubectl config system, that is not yet implemented. This PR implements it. If there is not access-token nor id-token given, but a command specified, that command is run with the configured parameters and the output is searched (considered to be json) with the path configured. The token is then used. Errors are thrown as appropriate.

Example kubectl config:

apiVersion: v1
clusters:
- cluster:
    certificate-authority-data: XXXXXXX
    server: https://36.XXX.XXX.XX
  name: generic_name
contexts:
- context:
    cluster: generic_name
    user: generic_name
  name: generic_name
current-context: generic_name
kind: Config
preferences: {}
users:
- name: generic_name
  user:
    auth-provider:
      config:
        cmd-args: config config-helper --format=json
        cmd-path: /opt/google-cloud-sdk/bin/gcloud
        expiry-key: '{.credential.token_expiry}'
        token-key: '{.credential.access_token}'
      name: gcp

clux

left some comments

clux · 2020-10-07T16:52:44Z

kube/src/config/file_config.rs

+                    }
+                } else {
+                    return Err(ConfigError::AuthExec(format!("no token or command provided. Authoring mechanism {:} not supported", provider.name)).into());
+                }


isn't this else case happening when token.is_some(), don't you need another is_none check?

good catch, this is supposed to be the else for the previous block. will fix.

clux · 2020-10-07T17:03:35Z

kube/src/config/file_config.rs

+                                        format!("Can't lookup {:?} in result of command: not an object", item)
+                                    ).into());
+                                }
+                            }


Hah, hacky. This at least makes it easy to understand, but it probably won't work well if people use a more complicated JSONPath selector?

How would you feel like using https://crates.io/crates/jsonpath_lib ? It looks like a good swap-in.
We can probably benefit from selector helpers using that anyway in the API.

didn't want to add any dependency before sending it up, but if that is fine with you I am happy to add this.

Please, this feels like one of those libraries that matches very well anyway :-)

clux · 2020-10-07T17:09:20Z

Hey! Thanks a lot for this. Will definitely want this in here. Just left some comments on a few things.

Also have one request if it's possible; a way to test it. I know it's not great to have to have a gcp cluster for testing, so maybe a test config embedded in that file which causes a shellout to a local script that just echoes some static json?

gnunicorn · 2020-10-07T17:19:57Z

Also have one request if it's possible; a way to test it. I know it's not great to have to have a gcp cluster for testing, so maybe a test config embedded in that file which causes a shellout to a local script that just echoes some static json?

Good idea. How about just using echo as the command and then a static json as the param? Does that work on all supported platforms? Does this target windows? I could make a linux-only-test if that's okay.

clux · 2020-10-07T17:27:37Z

Also have one request if it's possible; a way to test it. I know it's not great to have to have a gcp cluster for testing, so maybe a test config embedded in that file which causes a shellout to a local script that just echoes some static json?

Good idea. How about just using echo as the command and then a static json as the param? Does that work on all supported platforms? Does this target windows? I could make a linux-only-test if that's okay.

Yeah, echo is a good idea with static json. cargo test on CI is primarily for linux so this is fine.

gnunicorn · 2020-10-08T07:37:26Z

@clux addressed your suggestions: now uses the jsonpath-lib and has a test for the feature.

do you mind publishing a release soon after? I'd like to use this is a cli and it would be nice to not depend on git for it :) .

kube/src/config/file_config.rs

clux · 2020-10-08T11:09:26Z

@clux addressed your suggestions: now uses the jsonpath-lib and has a test for the feature.

do you mind publishing a release soon after? I'd like to use this is a cli and it would be nice to not depend on git for it :) .

Awesome, tyvm! I only left a few small questions there for my own sanity. But error handling and code looks great.
Would be happy to release ASAP once merged.

gnunicorn · 2020-10-08T12:23:13Z

so, I found one more quirk ... even though I get an access_token immediately, it sometimes refuses to use that with an error message, telling me to try again later – and seconds after it worked. Not sure, if we can/want to do anything about that ...

clux · 2020-10-08T15:40:56Z

so, I found one more quirk ... even though I get an access_token immediately, it sometimes refuses to use that with an error message, telling me to try again later – and seconds after it worked. Not sure, if we can/want to do anything about that ...

Does that error cause a whole re-authentication cycle? We don't write to our local config, but kubectl probably updates the config. We might need some mechanism for updating the local kube config for it. We could raise an issue to deal with this (actually serialize the kube config ourselves). If it causes a re-auth then that is a bit of a problem. If it can be fixed with waiting on a condition (preferable to sleeping) then that would be best.

Is there something you need to wait for in gcp before it's valid?

clux · 2020-10-08T15:42:42Z

At any rate, I am happy to merge this as it stands and make a release today. Though would be good to keep track of any upcoming work needed for avoiding re-authing. Have linked #84 which seem covered by this PR.

clux · 2020-10-08T20:54:37Z

I'll merge it for now anyway, we can tackle the other things when we get to it. Thanks again for your work!

clux · 2020-10-08T21:10:03Z

Released in 0.43.0 :-)

Adding google cloud auth per exec

e6c9b88

clux reviewed Oct 7, 2020

View reviewed changes

gnunicorn added 3 commits October 8, 2020 09:09

add tests ensuring the cmd data is extracted properly

9ea84f7

fix indent

ac3a483

switching to jsonpath_lib

e1def7a

clux approved these changes Oct 8, 2020

View reviewed changes

kube/src/config/file_config.rs Show resolved Hide resolved

kube/src/config/file_config.rs Show resolved Hide resolved

clux linked an issue Oct 8, 2020 that may be closed by this pull request

Drop implicit kind->resource conversion #284

Closed

4 tasks

clux removed a link to an issue Oct 8, 2020

Drop implicit kind->resource conversion #284

Closed

4 tasks

clux linked an issue Oct 8, 2020 that may be closed by this pull request

Error loading kube config: Missing GOOGLE_APPLICATION_CREDENTIALS #84

Closed

clux merged commit 0a32e19 into kube-rs:master Oct 8, 2020

clux mentioned this pull request Oct 8, 2020

Error loading kube config: Missing GOOGLE_APPLICATION_CREDENTIALS #84

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding google cloud auth per exec #328

Adding google cloud auth per exec #328

gnunicorn commented Oct 7, 2020

clux left a comment

clux Oct 7, 2020

gnunicorn Oct 7, 2020

clux Oct 7, 2020

gnunicorn Oct 7, 2020

clux Oct 7, 2020

clux commented Oct 7, 2020

gnunicorn commented Oct 7, 2020 •

edited

Loading

clux commented Oct 7, 2020

gnunicorn commented Oct 8, 2020 •

edited

Loading

clux commented Oct 8, 2020

gnunicorn commented Oct 8, 2020

clux commented Oct 8, 2020

clux commented Oct 8, 2020

clux commented Oct 8, 2020

clux commented Oct 8, 2020

Adding google cloud auth per exec #328

Adding google cloud auth per exec #328

Conversation

gnunicorn commented Oct 7, 2020

clux left a comment

Choose a reason for hiding this comment

clux Oct 7, 2020

Choose a reason for hiding this comment

gnunicorn Oct 7, 2020

Choose a reason for hiding this comment

clux Oct 7, 2020

Choose a reason for hiding this comment

gnunicorn Oct 7, 2020

Choose a reason for hiding this comment

clux Oct 7, 2020

Choose a reason for hiding this comment

clux commented Oct 7, 2020

gnunicorn commented Oct 7, 2020 • edited Loading

clux commented Oct 7, 2020

gnunicorn commented Oct 8, 2020 • edited Loading

clux commented Oct 8, 2020

gnunicorn commented Oct 8, 2020

clux commented Oct 8, 2020

clux commented Oct 8, 2020

clux commented Oct 8, 2020

clux commented Oct 8, 2020

gnunicorn commented Oct 7, 2020 •

edited

Loading

gnunicorn commented Oct 8, 2020 •

edited

Loading