Skip to content
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

config: Support importing config using Azure API #39

Merged
merged 5 commits into from
Jul 11, 2023

Conversation

mqasimsarfraz
Copy link
Member

@mqasimsarfraz mqasimsarfraz commented Jul 6, 2023

This change introduces a fallback mechanism to import config in case Kubernetes API isn't available.

Fixes: #37

IAM changes

Since we will using Azure API to fetch config information, we will be needing new permission actions in subscription the cluster is running:

current:

  • Microsoft.Compute/virtualMachineScaleSets/virtualmachines/runCommand/action

new

  • Microsoft.Compute/virtualMachineScaleSets/virtualmachines/runCommand/action
  • Microsoft.Compute/virtualMachineScaleSets/virtualMachines/read
  • Microsoft.Compute/virtualMachineScaleSets/read

Testing done

Added an integration tests and:

$ KUBECONFIG=foo ./kubectl-aks  config import -s foo -g foo -c foo

@mqasimsarfraz mqasimsarfraz force-pushed the qasim/config-import-azure-api branch 4 times, most recently from f3989d9 to a9c816f Compare July 6, 2023 13:58
@mqasimsarfraz mqasimsarfraz requested a review from blanquicet July 7, 2023 11:10
@mqasimsarfraz mqasimsarfraz force-pushed the qasim/config-import-azure-api branch from c084c94 to c2968c0 Compare July 10, 2023 14:09
Copy link
Member

@blanquicet blanquicet left a comment

Choose a reason for hiding this comment

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

Looks great. I tested and it generated indeed the same file. I only have some concerns about when the Azure API mechanism is used.

test/integration/integration_test.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
cmd/config.go Outdated Show resolved Hide resolved
cmd/config.go Outdated Show resolved Hide resolved
cmd/config.go Outdated Show resolved Hide resolved
cmd/utils/utils.go Outdated Show resolved Hide resolved
cmd/config.go Show resolved Hide resolved
test/integration/integration_test.go Outdated Show resolved Hide resolved
@mqasimsarfraz mqasimsarfraz force-pushed the qasim/config-import-azure-api branch from c2968c0 to 4391143 Compare July 11, 2023 09:58
@mqasimsarfraz mqasimsarfraz requested a review from blanquicet July 11, 2023 11:28
Copy link
Member

@blanquicet blanquicet left a comment

Choose a reason for hiding this comment

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

Some nits but LGTM!

cmd/config.go Outdated Show resolved Hide resolved
cmd/config.go Outdated Show resolved Hide resolved
"github.com/briandowns/spinner"
)

var DefaultSpinner = spinner.New(spinner.CharSets[9], 50*time.Millisecond)
Copy link
Member

Choose a reason for hiding this comment

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

It is a lot slower in az 😅

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 agree I got bit aggressive with that 🙈 seems like they are using 200 ms frequency so I will switch to the same!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Feels much better! 😄

@mqasimsarfraz mqasimsarfraz force-pushed the qasim/config-import-azure-api branch from 4391143 to bf9e028 Compare July 11, 2023 15:08
@mqasimsarfraz
Copy link
Member Author

Thanks of the reviews! I will merge this one!

@mqasimsarfraz mqasimsarfraz merged commit 909e71b into main Jul 11, 2023
@mqasimsarfraz mqasimsarfraz deleted the qasim/config-import-azure-api branch July 11, 2023 15:37
@blanquicet
Copy link
Member

I just realized that we didn't update the documentation with this alternative for config.

@mqasimsarfraz
Copy link
Member Author

mqasimsarfraz commented Jul 14, 2023

I just realized that we didn't update the documentation with this alternative for config.

Yeah, I opened a follow up PR with other improvement: https://github.com/Azure/kubectl-aks/pull/41/files

I see that PR title isn't reflecting that 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFE] Support importing configuration regardless the status of the k8s control plan
2 participants