From 7e6497874a65b79a241d968686cad9ebc1a3d3b0 Mon Sep 17 00:00:00 2001 From: Bryce Soghigian Date: Wed, 10 Jul 2024 10:10:35 -0700 Subject: [PATCH 1/6] test: 1.30.0 --- Makefile-az.mk | 2 +- pkg/providers/imagefamily/bootstrap/aksbootstrap.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile-az.mk b/Makefile-az.mk index 5db0a0864..287e16db7 100755 --- a/Makefile-az.mk +++ b/Makefile-az.mk @@ -47,7 +47,7 @@ az-mkaks: az-mkacr ## Create test AKS cluster (with --vm-set-type AvailabilitySe az-mkaks-cilium: az-mkacr ## Create test AKS cluster (with --network-dataplane cilium, --network-plugin cilium, and --network-plugin-mode overlay) az aks create --name $(AZURE_CLUSTER_NAME) --resource-group $(AZURE_RESOURCE_GROUP) --attach-acr $(AZURE_ACR_NAME) \ --enable-managed-identity --node-count 3 --generate-ssh-keys -o none --network-dataplane cilium --network-plugin azure --network-plugin-mode overlay \ - --enable-oidc-issuer --enable-workload-identity + --enable-oidc-issuer --enable-workload-identity -k 1.30.0 az aks get-credentials --name $(AZURE_CLUSTER_NAME) --resource-group $(AZURE_RESOURCE_GROUP) --overwrite-existing skaffold config set default-repo $(AZURE_ACR_NAME).azurecr.io/karpenter diff --git a/pkg/providers/imagefamily/bootstrap/aksbootstrap.go b/pkg/providers/imagefamily/bootstrap/aksbootstrap.go index f4e9b2260..f1428389c 100644 --- a/pkg/providers/imagefamily/bootstrap/aksbootstrap.go +++ b/pkg/providers/imagefamily/bootstrap/aksbootstrap.go @@ -466,7 +466,7 @@ func (a AKS) applyOptions(nbv *NodeBootstrapVariables) { // Assign Per K8s version kubelet flags minorVersion := semver.MustParse(a.KubernetesVersion).Minor if utils.UseOOTCredential(minorVersion) { - nbv.CredentialProviderDownloadURL = fmt.Sprintf("https://acs-mirror.azureedge.net/cloud-provider-azure/%s/binaries/azure-acr-credential-provider-linux-amd64-v%s.tar.gz", nbv.KubernetesVersion, nbv.KubernetesVersion) + nbv.CredentialProviderDownloadURL = fmt.Sprintf("https://acs-mirror.azureedge.net/cloud-provider-azure/v%s/binaries/azure-acr-credential-provider-linux-amd64-v%s.tar.gz", nbv.KubernetesVersion, nbv.KubernetesVersion) kubeletFlagsBase["--image-credential-provider-config"] = "/var/lib/kubelet/credential-provider-config.yaml" kubeletFlagsBase["--image-credential-provider-bin-dir"] = "/var/lib/kubelet/credential-provider" } else { // Versions Less than 1.30 From 00d806fdd33c089101660090b643279f4034dd2d Mon Sep 17 00:00:00 2001 From: tallaxes <18728999+tallaxes@users.noreply.github.com> Date: Tue, 16 Jul 2024 05:20:11 +0000 Subject: [PATCH 2/6] fix: credential provider URL --- .../imagefamily/bootstrap/aksbootstrap.go | 28 +++++++++++++++++-- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/pkg/providers/imagefamily/bootstrap/aksbootstrap.go b/pkg/providers/imagefamily/bootstrap/aksbootstrap.go index f1428389c..c064a8f7b 100644 --- a/pkg/providers/imagefamily/bootstrap/aksbootstrap.go +++ b/pkg/providers/imagefamily/bootstrap/aksbootstrap.go @@ -420,6 +420,28 @@ func kubeBinaryURL(kubernetesVersion, cpuArch string) string { return fmt.Sprintf("%s/kubernetes/v%s/binaries/kubernetes-node-linux-%s.tar.gz", globalAKSMirror, kubernetesVersion, cpuArch) } +// CredentialProviderURL returns the URL for OOT credential provider, +// or an empty string if OOT provider is not to be used +func CredentialProviderURL(kubernetesVersion, arch string) string { + minorVersion := semver.MustParse(kubernetesVersion).Minor + if minorVersion < 30 { // use from 1.30; 1.29 supports it too, but we have not fully tested it with Karpenter + return "" + } + + // credential provider has its own release outside of k8s version, and there'll be one credential provider binary for each k8s release, + // as credential provider release goes with cloud-provider-azure, not every credential provider release will be picked up unless + // there are CVE or bug fixes. + credentialProviderVersion := "1.29.2" + switch minorVersion { + case 29: + credentialProviderVersion = "1.29.2" + case 30: + credentialProviderVersion = "1.30.0" + } + + return fmt.Sprintf("%s/cloud-provider-azure/v%s/binaries/azure-acr-credential-provider-linux-%s-v%s.tar.gz", globalAKSMirror, credentialProviderVersion, arch, credentialProviderVersion) +} + func (a AKS) applyOptions(nbv *NodeBootstrapVariables) { nbv.KubeCACrt = *a.CABundle nbv.APIServerName = a.APIServerName @@ -464,9 +486,9 @@ func (a AKS) applyOptions(nbv *NodeBootstrapVariables) { }), ",") // Assign Per K8s version kubelet flags - minorVersion := semver.MustParse(a.KubernetesVersion).Minor - if utils.UseOOTCredential(minorVersion) { - nbv.CredentialProviderDownloadURL = fmt.Sprintf("https://acs-mirror.azureedge.net/cloud-provider-azure/v%s/binaries/azure-acr-credential-provider-linux-amd64-v%s.tar.gz", nbv.KubernetesVersion, nbv.KubernetesVersion) + credentialProviderURL := CredentialProviderURL(a.KubernetesVersion, a.Arch) + if credentialProviderURL != "" { // use OOT credential provider + nbv.CredentialProviderDownloadURL = credentialProviderURL kubeletFlagsBase["--image-credential-provider-config"] = "/var/lib/kubelet/credential-provider-config.yaml" kubeletFlagsBase["--image-credential-provider-bin-dir"] = "/var/lib/kubelet/credential-provider" } else { // Versions Less than 1.30 From 6e7db32e1ea82c75f3c2b5494f225b27e263b7b4 Mon Sep 17 00:00:00 2001 From: tallaxes <18728999+tallaxes@users.noreply.github.com> Date: Tue, 16 Jul 2024 05:22:06 +0000 Subject: [PATCH 3/6] test: associated tests --- .../bootstrap/aksbootstrap_test.go | 50 +++++++++++++++++++ pkg/providers/instancetype/suite_test.go | 11 ++-- 2 files changed, 54 insertions(+), 7 deletions(-) diff --git a/pkg/providers/imagefamily/bootstrap/aksbootstrap_test.go b/pkg/providers/imagefamily/bootstrap/aksbootstrap_test.go index 3c501e8fa..ec0cb423f 100644 --- a/pkg/providers/imagefamily/bootstrap/aksbootstrap_test.go +++ b/pkg/providers/imagefamily/bootstrap/aksbootstrap_test.go @@ -58,3 +58,53 @@ func TestKubeBinaryURL(t *testing.T) { }) } } + +func TestGetCredentialProviderURL(t *testing.T) { + tests := []struct { + version string + arch string + url string + }{ + { + version: "1.30.2", + arch: "amd64", + url: fmt.Sprintf("%s/cloud-provider-azure/v1.30.0/binaries/azure-acr-credential-provider-linux-amd64-v1.30.0.tar.gz", globalAKSMirror), + }, + { + version: "1.30.0", + arch: "amd64", + url: fmt.Sprintf("%s/cloud-provider-azure/v1.30.0/binaries/azure-acr-credential-provider-linux-amd64-v1.30.0.tar.gz", globalAKSMirror), + }, + { + version: "1.30.0", + arch: "arm64", + url: fmt.Sprintf("%s/cloud-provider-azure/v1.30.0/binaries/azure-acr-credential-provider-linux-arm64-v1.30.0.tar.gz", globalAKSMirror), + }, + { + version: "1.29.2", + arch: "amd64", + url: "", + }, + { + version: "1.29.0", + arch: "amd64", + url: "", + }, + { + version: "1.29.0", + arch: "arm64", + url: "", + }, + { + version: "1.28.7", + arch: "amd64", + url: "", + }, + } + for _, tt := range tests { + url := CredentialProviderURL(tt.version, tt.arch) + if url != tt.url { + t.Errorf("for version %s expected %s, got %s", tt.version, tt.url, url) + } + } +} diff --git a/pkg/providers/instancetype/suite_test.go b/pkg/providers/instancetype/suite_test.go index 66d2879db..6e03d42de 100644 --- a/pkg/providers/instancetype/suite_test.go +++ b/pkg/providers/instancetype/suite_test.go @@ -27,7 +27,6 @@ import ( "testing" "time" - "github.com/blang/semver/v4" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/samber/lo" @@ -53,6 +52,7 @@ import ( sdkerrors "github.com/Azure/azure-sdk-for-go-extensions/pkg/errors" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/karpenter-provider-azure/pkg/providers/imagefamily" + "github.com/Azure/karpenter-provider-azure/pkg/providers/imagefamily/bootstrap" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute" "github.com/Azure/karpenter-provider-azure/pkg/apis" @@ -1112,15 +1112,12 @@ var _ = Describe("InstanceType Provider", func() { // NOTE: env.Version may differ from the version we get for the apiserver k8sVersion, err := azureEnv.ImageProvider.KubeServerVersion(ctx) Expect(err).To(BeNil()) - parsed := semver.MustParse(k8sVersion) - if utils.UseOOTCredential(parsed.Minor) { + crendetialProviderURL := bootstrap.CredentialProviderURL(k8sVersion, "amd64") + if crendetialProviderURL != "" { Expect(kubeletFlags).ToNot(ContainSubstring("--azure-container-registry-config")) Expect(kubeletFlags).To(ContainSubstring("--image-credential-provider-config=/var/lib/kubelet/credential-provider-config.yaml")) Expect(kubeletFlags).To(ContainSubstring("--image-credential-provider-bin-dir=/var/lib/kubelet/credential-provider")) - Expect(decodedString).To(ContainSubstring( - fmt.Sprintf("https://acs-mirror.azureedge.net/cloud-provider-azure/%s/binaries/azure-acr-credential-provider-linux-amd64-v%s.tar.gz", parsed.String(), parsed.String()), - )) - + Expect(decodedString).To(ContainSubstring(crendetialProviderURL)) } else { Expect(kubeletFlags).To(ContainSubstring("--azure-container-registry-config")) Expect(kubeletFlags).ToNot(ContainSubstring("--image-credential-provider-config")) From 1d45452a624b9d32f23406a0f8f25c651c83f8fe Mon Sep 17 00:00:00 2001 From: tallaxes <18728999+tallaxes@users.noreply.github.com> Date: Tue, 16 Jul 2024 05:22:40 +0000 Subject: [PATCH 4/6] chore: remove unused function --- pkg/utils/utils.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index edb84c7fc..bd6ea31de 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -58,7 +58,3 @@ func MkVMID(resourceGroupName string, vmName string) string { const idFormat = "/subscriptions/subscriptionID/resourceGroups/%s/providers/Microsoft.Compute/virtualMachines/%s" return fmt.Sprintf(idFormat, resourceGroupName, vmName) } - -func UseOOTCredential(minorK8sVersion uint64) bool { - return minorK8sVersion >= 30 -} From 59d9b522d2a18a647e2f2a546db528923bedf0c0 Mon Sep 17 00:00:00 2001 From: tallaxes <18728999+tallaxes@users.noreply.github.com> Date: Tue, 16 Jul 2024 05:23:14 +0000 Subject: [PATCH 5/6] fix: minor unrelated log fix --- pkg/providers/instance/instance.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/providers/instance/instance.go b/pkg/providers/instance/instance.go index dd1fdc74d..4c2ea355f 100644 --- a/pkg/providers/instance/instance.go +++ b/pkg/providers/instance/instance.go @@ -178,7 +178,7 @@ func (p *Provider) List(ctx context.Context) ([]*armcompute.VirtualMachine, erro } func (p *Provider) Delete(ctx context.Context, resourceName string) error { - logging.FromContext(ctx).Debugf("Deleting virtual machine %s and associated resources") + logging.FromContext(ctx).Debugf("Deleting virtual machine %s and associated resources", resourceName) return p.cleanupAzureResources(ctx, resourceName) } From 2e8cfb7a303fd601ff5ddc50d37e3cf7cbb290c5 Mon Sep 17 00:00:00 2001 From: tallaxes <18728999+tallaxes@users.noreply.github.com> Date: Tue, 16 Jul 2024 05:28:02 +0000 Subject: [PATCH 6/6] chore: undo AKS version choice --- Makefile-az.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile-az.mk b/Makefile-az.mk index 287e16db7..5db0a0864 100755 --- a/Makefile-az.mk +++ b/Makefile-az.mk @@ -47,7 +47,7 @@ az-mkaks: az-mkacr ## Create test AKS cluster (with --vm-set-type AvailabilitySe az-mkaks-cilium: az-mkacr ## Create test AKS cluster (with --network-dataplane cilium, --network-plugin cilium, and --network-plugin-mode overlay) az aks create --name $(AZURE_CLUSTER_NAME) --resource-group $(AZURE_RESOURCE_GROUP) --attach-acr $(AZURE_ACR_NAME) \ --enable-managed-identity --node-count 3 --generate-ssh-keys -o none --network-dataplane cilium --network-plugin azure --network-plugin-mode overlay \ - --enable-oidc-issuer --enable-workload-identity -k 1.30.0 + --enable-oidc-issuer --enable-workload-identity az aks get-credentials --name $(AZURE_CLUSTER_NAME) --resource-group $(AZURE_RESOURCE_GROUP) --overwrite-existing skaffold config set default-repo $(AZURE_ACR_NAME).azurecr.io/karpenter