Conversation
There was a problem hiding this comment.
Pull request overview
Improves AKS Flex bootstrap and related tooling to correctly respect non-default Azure CLI profiles (notably when users set AZURE_CONFIG_DIR), avoiding accidental use of the default profile during cluster creation, node bootstrap, and Cilium install.
Changes:
- Make default Azure subscription ID resolution read
clouds.configfromAZURE_CONFIG_DIR(with fallback to$HOME/.azure). - Make kubeadm defaults pick the tenant ID from the Azure CLI profile under
AZURE_CONFIG_DIRwhen creatingAzureCLICredential. - Install Cilium in AKS BYOCNI mode using kubeconfig-derived API server host/port, and update node bootstrap apt install flags.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/pkg/util/config/config.go | Honor AZURE_CONFIG_DIR when reading clouds.config for default subscription ID. |
| plugin/pkg/util/config/config_test.go | Adds test coverage for AZURE_CONFIG_DIR subscription resolution. |
| cli/internal/config/nodebootstrap/assets/script.sh.tmpl | Adjusts apt install command to allow changing held packages. |
| cli/internal/config/nodebootstrap/script_test.go | Updates expected rendered script line for apt install flag change. |
| cli/internal/config/configcmd/defaults.go | Uses Azure CLI profile tenant (from azureProfile.json) when building Azure CLI credential for kubeadm defaults. |
| cli/internal/config/configcmd/defaults_test.go | Adds test coverage for tenant ID resolution from AZURE_CONFIG_DIR profile. |
| cli/internal/aks/deploy/cilium.go | Switches to BYOCNI-oriented Cilium install flow and derives API server host/port from kubeconfig. |
| cli/internal/aks/deploy/cilium_test.go | Adds test coverage for kubeconfig API server parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
cli/internal/aks/deploy/cilium.go
Outdated
| k8sServiceHost, k8sServicePort, err := kubeconfigAPIServer(kubeconfigFile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| cmd := exec.CommandContext( | ||
| ctx, | ||
| "cilium", "install", | ||
| "--set", "azure.resourceGroup="+cfg.ResourceGroupName, | ||
| "--kubeconfig", kubeconfigFile, | ||
| "--context", cfg.ClusterName+"-admin", | ||
| "--namespace", "kube-system", | ||
| "--datapath-mode", "aks-byocni", | ||
| "--helm-set", "aksbyocni.enabled=true", | ||
| "--helm-set", "cluster.name="+cfg.ClusterName, | ||
| "--helm-set", "operator.replicas=1", | ||
| "--helm-set", "kubeProxyReplacement=true", | ||
| "--helm-set", "k8sServiceHost="+k8sServiceHost, | ||
| "--helm-set", "k8sServicePort="+k8sServicePort, |
| return cfg | ||
| } | ||
|
|
||
| func azureConfigTenantID() string { |
There was a problem hiding this comment.
this is actually easier to just extract the tenant id via getting the command output of az account show --query 'tenantId' -o tsv. This can avoid adding hard dependency on az cli's internal state.
There was a problem hiding this comment.
and we should move this function to pkg/util/config @copilot
cli/internal/aks/deploy/cilium.go
Outdated
| return cmd.Run() | ||
| } | ||
|
|
||
| func kubeconfigAPIServer(kubeconfigFile string) (string, string, error) { |
There was a problem hiding this comment.
we can potentitally use implementations under
aks-flex/plugin/pkg/util/k8s/k8s.go
Line 48 in 2a5060a
…ve kubeconfig/k8s helpers to shared packages (#55) * Initial plan * Address review feedback: use az CLI for tenant ID, move utility functions to shared packages Co-authored-by: bcho <1975118+bcho@users.noreply.github.com> * Use az CLI for subscription ID; fix indentation in APIServerFromKubeconfigFile Co-authored-by: bcho <1975118+bcho@users.noreply.github.com> * Add contextName parameter to APIServerFromKubeconfigFile; pass context in cilium deploy Co-authored-by: bcho <1975118+bcho@users.noreply.github.com> * Extract clusterContext variable in deployCilium; reuse across API server lookup and cilium install flags Co-authored-by: bcho <1975118+bcho@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: bcho <1975118+bcho@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Initial plan * Run go mod tidy across all submodules (cli, karpenter, plugin) Co-authored-by: bcho <1975118+bcho@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: bcho <1975118+bcho@users.noreply.github.com>
Summary
AZURE_CONFIG_DIRwhen deriving the default Azure subscription IDWhy
AKS Flex cluster creation and node bootstrap could silently use the wrong Azure CLI profile when users isolate multiple Azure accounts with different
AZURE_CONFIG_DIRfolders. That broke Flex cluster creation and external node bootstrap for clusters created from a non-default Azure profile.Validation
go test ./internal/config/... ./internal/aks/deploy/...incligo test ./pkg/util/config/...inplugin