Skip to content

[AKS] CLI should tolerate empty kubeconfig#14914

Merged
arrownj merged 1 commit intoAzure:devfrom
matthchr:fix-aks-bug
Sep 7, 2020
Merged

[AKS] CLI should tolerate empty kubeconfig#14914
arrownj merged 1 commit intoAzure:devfrom
matthchr:fix-aks-bug

Conversation

@matthchr
Copy link
Member

Description
Fix bug #13846

Testing Guide
Run az aks get-credentials --resource-group <rg> --name <name> on an AKS cluster while your local kubeconfig is empty:

$ kubectl config view
apiVersion: v1
clusters: null
contexts: null
current-context: ""
kind: Config
preferences: {}
users: null

This checklist is used to make sure that common guidelines for a pull request are followed.

@matthchr matthchr requested a review from arrownj as a code owner August 25, 2020 18:48
@matthchr matthchr changed the title AKS CLI should tolerate empty kubeconfig [AKS] CLI should tolerate empty kubeconfig Aug 25, 2020
@matthchr
Copy link
Member Author

Not sure what the right component prefix is supposed to be here...

@arrownj arrownj self-assigned this Aug 26, 2020
if not addition.get(key, False):
return
if existing[key] is None:
if not existing.get(key):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

existing[key] == None / 0 / [] / "" / {} will all trigger this condition, is this the expected behavior ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert on what "valid" vs "invalid" kubeconfigs look like, so I don't know for sure.

I think at the very least a missing key should be treated the same as an empty key (before, a missing key triggered an exception).

I was matching the line above on 1214 which was was doing the same thing (not checking specifically for == False or something.

Looking further on in the code, if we don't overwrite None / 0 / "" the code below will blow up I think as we'll try to iterate over it assuming that it's a dict.

@yungezz yungezz added the AKS az aks/acs/openshift label Aug 26, 2020
@Azure Azure deleted a comment from yonzhan Aug 26, 2020
@yungezz yungezz added this to the S175 - For Ignite milestone Aug 26, 2020
@arrownj
Copy link
Contributor

arrownj commented Aug 26, 2020

Add @zqingqing1 @tjprescott to review.

@tjprescott tjprescott removed their request for review August 26, 2020 15:29
Copy link
Member

@zqingqing1 zqingqing1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

@matthchr
Copy link
Member Author

@arrownj @zqingqing1 - is there more I need to do here? If not can we merge this?

@arrownj arrownj merged commit e8ecc9c into Azure:dev Sep 7, 2020
@matthchr matthchr deleted the fix-aks-bug branch September 8, 2020 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AKS az aks/acs/openshift

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants