patch kubeconfig if token cannot be deleted via api #12962

juanvallejo · 2017-02-14T23:32:43Z

Fixes #7011
Related bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1422252

Deletes a token in a kubeconfig user stanza even if the token could not be deleted via the API.

cc @openshift/cli-review @stevekuznetsov @deads2k

juanvallejo · 2017-02-14T23:34:28Z

[test]

stevekuznetsov

Not sure the logic makes sense to me -- oc logout should always remove the token from your .kube/config but should delete it using the API as a best-effort attempt.

If this does end up being the accepted logic, you should break this out into a separate method at the very least, four levels of nested logic smells and and doesn't read well for someone trying to glance over RunLogout().

juanvallejo · 2017-02-15T14:45:31Z

@stevekuznetsov thanks for the feedback, I extracted out existing code to patch config into a separate function, which is now being called before attempting to delete using api. PTAL

stevekuznetsov · 2017-02-15T15:05:39Z

pkg/cmd/cli/cmd/login/logout.go

+	// before deleting using api.
+	tokenDeleted, err := o.deleteTokenFromConfig()
+	if err != nil {
+		return err


Is the ordering here important? Not sure we want to be in a situation where we have deleted the token from config but failed before trying to delete it using the API.

stevekuznetsov · 2017-02-15T15:07:09Z

pkg/cmd/cli/cmd/login/logout.go

 			value.Token = ""
 			newConfig.AuthInfos[key] = value
+			tokenDeleted = true


Doesn't this just mean we modified the in-memory copy of the config? Whereas the kclientcmd.ModifyConfig call later is what actually modifies the config on disk?

juanvallejo · 2017-02-15T15:52:57Z

re[test]

stevekuznetsov · 2017-02-15T17:53:02Z

pkg/cmd/cli/cmd/login/logout.go

-		return err
+	errs := []error{}
+	serverErr := client.OAuthAccessTokens().Delete(token)
+	if serverErr != nil {


NewAggregate does this for you

I need to not append nil to the errs slice at the end in order to print a successful message or not by checking if len(errs) == 0 ...

Just check if the output of Flatten is nil or not

stevekuznetsov · 2017-02-15T17:54:27Z

pkg/cmd/cli/cmd/login/logout.go

+		fmt.Fprintf(o.ErrOut, "%s\n\n", "An error ocurred deleting the token on the server. The token has been removed from your local configuration.")
+	}
+
+	if len(errs) > 0 {


Flatten does this for you

juanvallejo · 2017-02-15T18:34:03Z

pkg/cmd/cli/cmd/login/logout.go

+	} else if serverErr != nil {
+		// if token was not able to be deleted using api but was successfully removed
+		// from client config, print brief explanation that token was still removed locally
+		fmt.Fprintf(o.ErrOut, "%s\n\n", "An error ocurred deleting the token on the server. The token has been removed from your local configuration.")


Although this also applies to cases outside of being logged in with a serviceaccount token, I am not sure how I feel about returning an error message everytime a user tries to log out when using a serviceaccount token. cc @fabianofranz

I would make it a log with low level, but not print it by default.

@fabianofranz Thanks, updated this line to glog instead. It will still print the server error (if any) however, just in case it is unrelated to the token not being found:

$ oc logout --loglevel=1 I0216 09:42:03.042611 13554 logout.go:137] An error ocurred deleting the token on the server. The token has been removed from your local configuration. Error from server (NotFound): oauthaccesstokens "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJkZWZhdWx0Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9zZWNyZXQubmFtZSI6ImRlZmF1bHQtdG9rZW4tNjU3bjQiLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlcnZpY2UtYWNjb3VudC5uYW1lIjoiZGVmYXVsdCIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50LnVpZCI6IjVmMzk0Y2I0LWYzYWYtMTFlNi1iNDUxLTUwN2I5ZGFjOTZlMSIsInN1YiI6InN5c3RlbTpzZXJ2aWNlYWNjb3VudDpkZWZhdWx0OmRlZmF1bHQifQ.c7TiHikwCsobsMTIYhB6O-FjRca6TJmccpN4jem4oKWh9UTE8SxjqnCtrjZe_-zJ0eZ-GovMgiaa5L0xcilcvnj7htKeT3XeycKcUzxYCGJj884UL-Ouu4dh0-vJElvEMu6AlpwxmaES99-aJbghaTOssIIVFQdP9-AsYA2iVS1BBbeCWLw7yrE1gRXVhSjRhntpUwr-OBGO7fV8_ky96FjmUltADw0cynZr6P_NN_DWZYrEhaK7TuAbc-14K9uzLqcAiG_RLJfR7fOUvjPBp9kQDfQ13PczYhvMhXxluMma7WyXOrsB9ia2D2dXJ2SIA4a6tuL_8K_7UPaiO-an9Q" not found

smarterclayton · 2017-02-15T18:48:58Z

Flake was

12:14:21 TASK [openshift-registry : Deploy latest configuration of registry DC] *********
12:14:21 Wednesday 15 February 2017  17:14:21 +0000 (0:00:00.565)       0:14:48.331 **** 
12:14:21 fatal: [ci-prtest590-ig-m-2w6c]: FAILED! => {"changed": true, "cmd": ["oc", "deploy", "docker-registry", "--latest"], "delta": "0:00:00.199192", "end": "2017-02-15 12:14:21.602776", "failed": true, "rc": 1, "start": "2017-02-15 12:14:21.403584", "stderr": "Flag --latest has been deprecated, use 'oc rollout latest' instead\nerror: #2 is already in progress (Running).\nOptionally, you can cancel this deployment using 'oc rollout cancel dc/docker-registry-2'.", "stdout": "", "stdout_lines": [], "warnings": []}

@Kargakis @mfojtik failed when deploying, looks familiar but has never happened in gce before

fabianofranz · 2017-02-17T14:34:58Z

@juanvallejo legit failure in https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/281/

juanvallejo · 2017-02-17T14:47:35Z

@fabianofranz thanks!

juanvallejo · 2017-02-17T15:18:48Z

re[test]

fabianofranz · 2017-02-17T18:48:19Z

@stevekuznetsov anything else here?

stevekuznetsov · 2017-02-17T19:33:43Z

pkg/cmd/cli/cmd/login/logout.go

+	} else if serverErr != nil {
+		// if token was not able to be deleted using api but was successfully removed
+		// from client config, log brief explanation that token was still removed locally
+		glog.V(1).Infof("%s\n\n", "An error ocurred deleting the token on the server. The token has been removed from your local configuration.")


You're doing a glog.V(1).Infof with the flattened error below, so you'll always get a log about the delete failure. You should just do this here:

if configErr := deleteTokenFromConfig(*o.StartingKubeConfig, o.PathOptions, token); configErr == nil { glog.V(1).Info("Removed token from your local configuration.\n\n") } errs = append(errs, configErr)

stevekuznetsov · 2017-02-17T19:34:58Z

pkg/cmd/cli/cmd/login/logout.go


-	for key, value := range newConfig.AuthInfos {
-		if value.Token == token {
+	return nil


The logic now is -- we fail to delete from API server, and we fail to delete from config, but we print the logout message and return nil so we exit successfully. Are both of these operations meant to be best-effort?

stevekuznetsov · 2017-02-17T19:35:32Z

pkg/cmd/cli/cmd/login/logout.go

 			// don't break, its possible that more than one user stanza has the same token.
 		}
 	}

-	if err := kclientcmd.ModifyConfig(o.PathOptions, newConfig, true); err != nil {
+	if err := kclientcmd.ModifyConfig(pathOptions, config, true); err != nil {


Don't do

if err != nil { return err } return nil

just do

return err

This is outstanding @juanvallejo

juanvallejo · 2017-02-17T21:26:15Z

@stevekuznetsov

The logic now is -- we fail to delete from API server, and we fail to delete from config, but we print the logout message and return nil so we exit successfully. Are both of these operations meant to be best-effort?

Updated this to print Logged out user ... as long as there is no configError. Also, I return configError if there is one, since the server error gets logged regardless. PTAL

juanvallejo · 2017-02-17T22:41:29Z

re[test]

stevekuznetsov · 2017-02-20T13:38:10Z

pkg/cmd/cli/cmd/login/logout.go

-	if err := client.OAuthAccessTokens().Delete(token); err != nil {
-		return err
+	serverErr := client.OAuthAccessTokens().Delete(token)
+	if serverErr != nil {


You don't need to leak serverErr out of this scope anymore

Thanks, fixed

remove token from config on error / success from api req log server error to prevent failure on sa logout

juanvallejo · 2017-02-20T16:46:52Z

@stevekuznetsov thanks for the feedback, review comments addressed

stevekuznetsov · 2017-02-20T17:58:24Z

pkg/cmd/cli/cmd/login/logout.go

-	newConfig := *o.StartingKubeConfig
+	configErr := deleteTokenFromConfig(*o.StartingKubeConfig, o.PathOptions, token)
+	if configErr == nil {
+		glog.V(1).Infof("Removed token from your local configuration.\n\n")


Don't \n in glog

stevekuznetsov

One small nit, otherwise LGTM

openshift-bot · 2017-02-20T18:37:33Z

Evaluated for origin test up to 3c0eb53

openshift-bot · 2017-02-20T20:04:47Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/365/) (Base Commit: 3f36d2e)

fabianofranz · 2017-02-20T20:12:23Z

glog line breaks addressed, [merge].

openshift-bot · 2017-02-20T20:13:27Z

Evaluated for origin merge up to 3c0eb53

openshift-bot · 2017-02-21T07:29:15Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/365/) (Base Commit: 3ef5377) (Image: devenv-rhel7_5945)

stevekuznetsov suggested changes Feb 15, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/delete-sa-token-kubeconfig-logout branch 3 times, most recently from b8347c2 to 44902a7 Compare February 15, 2017 14:44

stevekuznetsov suggested changes Feb 15, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/delete-sa-token-kubeconfig-logout branch from a6ab352 to c93f914 Compare February 15, 2017 15:52

juanvallejo force-pushed the jvallejo/delete-sa-token-kubeconfig-logout branch from 0168440 to 350cf33 Compare February 15, 2017 17:51

stevekuznetsov suggested changes Feb 15, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/delete-sa-token-kubeconfig-logout branch from 350cf33 to 5af4679 Compare February 15, 2017 18:30

juanvallejo commented Feb 15, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/delete-sa-token-kubeconfig-logout branch 4 times, most recently from de163de to 93976dd Compare February 16, 2017 16:31

juanvallejo force-pushed the jvallejo/delete-sa-token-kubeconfig-logout branch 2 times, most recently from e973706 to f37ba7b Compare February 17, 2017 14:46

stevekuznetsov suggested changes Feb 17, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/delete-sa-token-kubeconfig-logout branch 4 times, most recently from 7a16da2 to 1d346e7 Compare February 17, 2017 21:35

juanvallejo force-pushed the jvallejo/delete-sa-token-kubeconfig-logout branch from 1d346e7 to 98d990d Compare February 18, 2017 04:06

stevekuznetsov reviewed Feb 20, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/delete-sa-token-kubeconfig-logout branch from 98d990d to a280ea5 Compare February 20, 2017 14:46

patch kubeconfig if token cannot be deleted via api

bf1f4e2

remove token from config on error / success from api req log server error to prevent failure on sa logout

juanvallejo force-pushed the jvallejo/delete-sa-token-kubeconfig-logout branch from a280ea5 to bf1f4e2 Compare February 20, 2017 16:45

stevekuznetsov reviewed Feb 20, 2017

View reviewed changes

stevekuznetsov approved these changes Feb 20, 2017

View reviewed changes

Removed line breaks in glog messages

3c0eb53

openshift-bot merged commit a16f4c6 into openshift:master Feb 21, 2017

juanvallejo deleted the jvallejo/delete-sa-token-kubeconfig-logout branch February 21, 2017 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

patch kubeconfig if token cannot be deleted via api #12962

patch kubeconfig if token cannot be deleted via api #12962

juanvallejo commented Feb 14, 2017 •

edited

Loading

juanvallejo commented Feb 14, 2017

stevekuznetsov left a comment

juanvallejo commented Feb 15, 2017

stevekuznetsov Feb 15, 2017

stevekuznetsov Feb 15, 2017

juanvallejo commented Feb 15, 2017

stevekuznetsov Feb 15, 2017

juanvallejo Feb 15, 2017

stevekuznetsov Feb 15, 2017

stevekuznetsov Feb 15, 2017

juanvallejo Feb 15, 2017

fabianofranz Feb 16, 2017

juanvallejo Feb 16, 2017

smarterclayton commented Feb 15, 2017

fabianofranz commented Feb 17, 2017

juanvallejo commented Feb 17, 2017

juanvallejo commented Feb 17, 2017

fabianofranz commented Feb 17, 2017

stevekuznetsov Feb 17, 2017

stevekuznetsov Feb 17, 2017

stevekuznetsov Feb 17, 2017

stevekuznetsov Feb 20, 2017

juanvallejo commented Feb 17, 2017

juanvallejo commented Feb 17, 2017

stevekuznetsov Feb 20, 2017

juanvallejo Feb 20, 2017

juanvallejo commented Feb 20, 2017

stevekuznetsov Feb 20, 2017

stevekuznetsov left a comment

openshift-bot commented Feb 20, 2017

openshift-bot commented Feb 20, 2017

fabianofranz commented Feb 20, 2017

openshift-bot commented Feb 20, 2017

openshift-bot commented Feb 21, 2017 •

edited

Loading

patch kubeconfig if token cannot be deleted via api #12962

patch kubeconfig if token cannot be deleted via api #12962

Conversation

juanvallejo commented Feb 14, 2017 • edited Loading

juanvallejo commented Feb 14, 2017

stevekuznetsov left a comment

Choose a reason for hiding this comment

juanvallejo commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Feb 15, 2017

fabianofranz commented Feb 17, 2017

juanvallejo commented Feb 17, 2017

juanvallejo commented Feb 17, 2017

fabianofranz commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Feb 17, 2017

juanvallejo commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Feb 20, 2017

Choose a reason for hiding this comment

stevekuznetsov left a comment

Choose a reason for hiding this comment

openshift-bot commented Feb 20, 2017

openshift-bot commented Feb 20, 2017

fabianofranz commented Feb 20, 2017

openshift-bot commented Feb 20, 2017

openshift-bot commented Feb 21, 2017 • edited Loading

juanvallejo commented Feb 14, 2017 •

edited

Loading

openshift-bot commented Feb 21, 2017 •

edited

Loading