From 584d2bf052ba0cc24a942c788172cc49f0f5267a Mon Sep 17 00:00:00 2001 From: Robin Deeboonchai Date: Sun, 15 Sep 2024 00:31:19 -0700 Subject: [PATCH] refactor: abandon ARM_AUTH_METHOD in favor of DefaultAzureCredentials --- go.mod | 2 +- karpenter-values-template.yaml | 2 - pkg/auth/autorest_auth.go | 59 ------------ pkg/auth/config.go | 22 +---- pkg/auth/config_test.go | 58 ------------ pkg/auth/cred.go | 26 ------ pkg/auth/cred_test.go | 90 ------------------- pkg/operator/operator.go | 3 +- pkg/providers/instance/azure_client.go | 5 +- pkg/providers/instance/skuclient/skuclient.go | 9 +- 10 files changed, 14 insertions(+), 262 deletions(-) delete mode 100644 pkg/auth/autorest_auth.go delete mode 100644 pkg/auth/cred_test.go diff --git a/go.mod b/go.mod index 37ddfaa1c..8a9bbf8a4 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,6 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork v1.1.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resourcegraph/armresourcegraph v0.9.0 github.com/Azure/go-autorest/autorest v0.11.29 - github.com/Azure/go-autorest/autorest/adal v0.9.24 github.com/Azure/go-autorest/autorest/to v0.4.0 github.com/Azure/skewer v0.0.19 github.com/Pallinder/go-randomdata v1.2.0 @@ -59,6 +58,7 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/storage/armstorage v1.5.0 // indirect github.com/Azure/go-autorest v14.2.0+incompatible // indirect + github.com/Azure/go-autorest/autorest/adal v0.9.24 // indirect github.com/Azure/go-autorest/autorest/date v0.3.0 // indirect github.com/Azure/go-autorest/autorest/mocks v0.4.2 // indirect github.com/Azure/go-autorest/autorest/validation v0.3.1 // indirect diff --git a/karpenter-values-template.yaml b/karpenter-values-template.yaml index 0deb08e90..1d2515908 100644 --- a/karpenter-values-template.yaml +++ b/karpenter-values-template.yaml @@ -38,8 +38,6 @@ controller: value: "" - name: AZURE_NODE_RESOURCE_GROUP value: ${AZURE_RESOURCE_GROUP_MC} - - name: ARM_AUTH_METHOD - value: "workload-identity" serviceAccount: name: ${KARPENTER_SERVICE_ACCOUNT_NAME} annotations: diff --git a/pkg/auth/autorest_auth.go b/pkg/auth/autorest_auth.go deleted file mode 100644 index de26665f4..000000000 --- a/pkg/auth/autorest_auth.go +++ /dev/null @@ -1,59 +0,0 @@ -/* -Portions Copyright (c) Microsoft Corporation. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package auth - -import ( - "fmt" - - "github.com/Azure/go-autorest/autorest" - "github.com/Azure/go-autorest/autorest/adal" - "github.com/Azure/go-autorest/autorest/azure" - "k8s.io/klog/v2" - - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" - "github.com/jongio/azidext/go/azidext" -) - -func NewAuthorizer(config *Config, env *azure.Environment) (autorest.Authorizer, error) { - // TODO (charliedmcb): need to get track 2 support for the skewer API, and align all auth under workload identity in the same way within cred.go - if config.ArmAuthMethod == authMethodWorkloadIdentity { - klog.V(2).Infoln("auth: using workload identity for new authorizer") - cred, err := azidentity.NewDefaultAzureCredential(nil) - if err != nil { - return nil, fmt.Errorf("default cred: %w", err) - } - return azidext.NewTokenCredentialAdapter(cred, []string{azidext.DefaultManagementScope}), nil - } - - if config.ArmAuthMethod == authMethodSysMSI { - klog.V(2).Infoln("auth: using system assigned MSI to retrieve access token") - msiEndpoint, err := adal.GetMSIVMEndpoint() - if err != nil { - return nil, fmt.Errorf("getting the managed service identity endpoint: %w", err) - } - - token, err := adal.NewServicePrincipalTokenFromMSI( - msiEndpoint, - env.ServiceManagementEndpoint) - if err != nil { - return nil, fmt.Errorf("retrieve service principal token: %w", err) - } - return autorest.NewBearerAuthorizer(token), nil - } - - return nil, fmt.Errorf("auth: unsupported auth method %s", config.ArmAuthMethod) -} diff --git a/pkg/auth/config.go b/pkg/auth/config.go index 4c638a550..2d2b7d5df 100644 --- a/pkg/auth/config.go +++ b/pkg/auth/config.go @@ -25,12 +25,6 @@ import ( "github.com/Azure/go-autorest/autorest/azure" ) -const ( - // auth methods - authMethodSysMSI = "system-assigned-msi" - authMethodWorkloadIdentity = "workload-identity" -) - const ( // from azure_manager vmTypeVMSS = "vmss" @@ -60,11 +54,6 @@ type Config struct { ResourceGroup string `json:"resourceGroup" yaml:"resourceGroup"` VMType string `json:"vmType" yaml:"vmType"` - // ArmAuthMethod determines how to authorize requests for the Azure cloud. - // Valid options are "system-assigned-msi" and "workload-identity" - // The default is "workload-identity". - ArmAuthMethod string `json:"armAuthMethod" yaml:"armAuthMethod"` - // Managed identity for Kubelet (not to be confused with Azure cloud authorization) KubeletIdentityClientID string `json:"kubeletIdentityClientID" yaml:"kubeletIdentityClientID"` @@ -102,6 +91,7 @@ func (cfg *Config) GetAzureClientConfig(authorizer autorest.Authorizer, env *azu } func (cfg *Config) Build() error { + // May require more than this behind the scenes: https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/azidentity/README.md#defaultazurecredential cfg.Cloud = strings.TrimSpace(os.Getenv("ARM_CLOUD")) cfg.Location = strings.TrimSpace(os.Getenv("LOCATION")) cfg.ResourceGroup = strings.TrimSpace(os.Getenv("ARM_RESOURCE_GROUP")) @@ -110,7 +100,6 @@ func (cfg *Config) Build() error { cfg.VMType = strings.ToLower(os.Getenv("ARM_VM_TYPE")) cfg.ClusterName = strings.TrimSpace(os.Getenv("AZURE_CLUSTER_NAME")) cfg.NodeResourceGroup = strings.TrimSpace(os.Getenv("AZURE_NODE_RESOURCE_GROUP")) - cfg.ArmAuthMethod = strings.TrimSpace(os.Getenv("ARM_AUTH_METHOD")) cfg.KubeletIdentityClientID = strings.TrimSpace(os.Getenv("ARM_KUBELET_IDENTITY_CLIENT_ID")) return nil @@ -121,11 +110,6 @@ func (cfg *Config) Default() error { if cfg.VMType == "" { cfg.VMType = vmTypeVMSS } - - if cfg.ArmAuthMethod == "" { - cfg.ArmAuthMethod = authMethodWorkloadIdentity - } - return nil } @@ -145,9 +129,5 @@ func (cfg *Config) Validate() error { } } - if cfg.ArmAuthMethod != authMethodSysMSI && cfg.ArmAuthMethod != authMethodWorkloadIdentity { - return fmt.Errorf("unsupported authorization method: %s", cfg.ArmAuthMethod) - } - return nil } diff --git a/pkg/auth/config_test.go b/pkg/auth/config_test.go index 071a3fb8c..fbcd7a82f 100644 --- a/pkg/auth/config_test.go +++ b/pkg/auth/config_test.go @@ -41,7 +41,6 @@ func TestBuildAzureConfig(t *testing.T) { ResourceGroup: "my-rg", NodeResourceGroup: "my-node-rg", VMType: "vmss", - ArmAuthMethod: "workload-identity", }, wantErr: false, env: map[string]string{ @@ -60,7 +59,6 @@ func TestBuildAzureConfig(t *testing.T) { ResourceGroup: "my-rg", NodeResourceGroup: "my-node-rg", VMType: "vm", - ArmAuthMethod: "workload-identity", }, wantErr: false, env: map[string]string{ @@ -73,60 +71,6 @@ func TestBuildAzureConfig(t *testing.T) { "ARM_VM_TYPE": "vm", }, }, - { - name: "bogus ARM_AUTH_METHOD", - expected: nil, - wantErr: true, - env: map[string]string{ - "ARM_RESOURCE_GROUP": "my-rg", - "ARM_SUBSCRIPTION_ID": "12345", - "AZURE_NODE_RESOURCE_GROUP": "my-node-rg", - "AZURE_SUBNET_ID": "12345", - "AZURE_SUBNET_NAME": "my-subnet", - "AZURE_VNET_NAME": "my-vnet", - "ARM_AUTH_METHOD": "foo", // this is not a supported value - }, - }, - { - name: "auth method msi", - expected: &Config{ - SubscriptionID: "12345", - ResourceGroup: "my-rg", - NodeResourceGroup: "my-node-rg", - VMType: "vmss", - ArmAuthMethod: "system-assigned-msi", - }, - wantErr: false, - env: map[string]string{ - "ARM_RESOURCE_GROUP": "my-rg", - "ARM_SUBSCRIPTION_ID": "12345", - "AZURE_NODE_RESOURCE_GROUP": "my-node-rg", - "AZURE_SUBNET_ID": "12345", - "AZURE_SUBNET_NAME": "my-subnet", - "AZURE_VNET_NAME": "my-vnet", - "ARM_AUTH_METHOD": "system-assigned-msi", - }, - }, - { - name: "auth method workload identity", - expected: &Config{ - SubscriptionID: "12345", - ResourceGroup: "my-rg", - NodeResourceGroup: "my-node-rg", - VMType: "vmss", - ArmAuthMethod: "workload-identity", - }, - wantErr: false, - env: map[string]string{ - "ARM_RESOURCE_GROUP": "my-rg", - "ARM_SUBSCRIPTION_ID": "12345", - "AZURE_NODE_RESOURCE_GROUP": "my-node-rg", - "AZURE_SUBNET_ID": "12345", - "AZURE_SUBNET_NAME": "my-subnet", - "AZURE_VNET_NAME": "my-vnet", - "ARM_AUTH_METHOD": "workload-identity", - }, - }, { name: "valid kubelet identity", expected: &Config{ @@ -134,7 +78,6 @@ func TestBuildAzureConfig(t *testing.T) { ResourceGroup: "my-rg", NodeResourceGroup: "my-node-rg", VMType: "vmss", - ArmAuthMethod: "system-assigned-msi", KubeletIdentityClientID: "11111111-2222-3333-4444-555555555555", }, wantErr: false, @@ -145,7 +88,6 @@ func TestBuildAzureConfig(t *testing.T) { "AZURE_SUBNET_ID": "12345", "AZURE_SUBNET_NAME": "my-subnet", "AZURE_VNET_NAME": "my-vnet", - "ARM_AUTH_METHOD": "system-assigned-msi", "ARM_KUBELET_IDENTITY_CLIENT_ID": "11111111-2222-3333-4444-555555555555", }, }, diff --git a/pkg/auth/cred.go b/pkg/auth/cred.go index 8dafe3a68..86724974c 100644 --- a/pkg/auth/cred.go +++ b/pkg/auth/cred.go @@ -18,13 +18,10 @@ package auth import ( "context" - "fmt" "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" - "k8s.io/klog/v2" "knative.dev/pkg/logging" ) @@ -60,26 +57,3 @@ func (w *expireEarlyTokenCredential) GetToken(ctx context.Context, options polic token.ExpiresOn = twoHoursFromNow return token, nil } - -// NewCredential provides a token credential for msi and service principal auth -func NewCredential(cfg *Config) (azcore.TokenCredential, error) { - if cfg == nil { - return nil, fmt.Errorf("failed to create credential, nil config provided") - } - - if cfg.ArmAuthMethod == authMethodWorkloadIdentity { - klog.V(2).Infoln("cred: using workload identity for new credential") - return azidentity.NewDefaultAzureCredential(nil) - } - - if cfg.ArmAuthMethod == authMethodSysMSI { - klog.V(2).Infoln("cred: using system assigned MSI for new credential") - msiCred, err := azidentity.NewManagedIdentityCredential(nil) - if err != nil { - return nil, err - } - return msiCred, nil - } - - return nil, fmt.Errorf("cred: unsupported auth method: %s", cfg.ArmAuthMethod) -} diff --git a/pkg/auth/cred_test.go b/pkg/auth/cred_test.go deleted file mode 100644 index 8235b0a5c..000000000 --- a/pkg/auth/cred_test.go +++ /dev/null @@ -1,90 +0,0 @@ -/* -Portions Copyright (c) Microsoft Corporation. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package auth - -import ( - "reflect" - "testing" - - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" -) - -func TestNewCredential(t *testing.T) { - tests := []struct { - name string - cfg *Config - want reflect.Type - wantErr bool - wantErrStr string - }{ - { - name: "nil Config", - cfg: nil, - want: nil, - wantErr: true, - wantErrStr: "failed to create credential, nil config provided", - }, - { - name: "unsupported auth method", - cfg: &Config{ - ArmAuthMethod: "unsupported", - }, - want: nil, - wantErr: true, - wantErrStr: "cred: unsupported auth method: unsupported", - }, - { - name: "empty auth method", - cfg: &Config{}, - want: nil, - wantErr: true, - wantErrStr: "cred: unsupported auth method: ", - }, - { - name: "auth method system-assigned-msi", - cfg: &Config{ - ArmAuthMethod: authMethodSysMSI, - }, - want: reflect.TypeOf(&azidentity.ManagedIdentityCredential{}), - wantErr: false, - }, - { - name: "auth method workload-identity", - cfg: &Config{ - ArmAuthMethod: authMethodWorkloadIdentity, - }, - want: reflect.TypeOf(&azidentity.DefaultAzureCredential{}), - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := NewCredential(tt.cfg) - if (err != nil) != tt.wantErr { - t.Errorf("NewCredential() error = %v, wantErr %v", err, tt.wantErr) - return - } - if err != nil && err.Error() != tt.wantErrStr { - t.Errorf("NewCredential() error = %v, wantErrStr %v", err, tt.wantErrStr) - return - } - if reflect.TypeOf(got) != tt.want { - t.Errorf("NewCredential() = %v, want %v", reflect.TypeOf(got), tt.want) - } - }) - } -} diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index 36cfc2fc6..ea83a34cf 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -32,6 +32,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/Azure/karpenter-provider-azure/pkg/apis" "github.com/Azure/karpenter-provider-azure/pkg/auth" azurecache "github.com/Azure/karpenter-provider-azure/pkg/cache" @@ -169,7 +170,7 @@ func getCABundle(restConfig *rest.Config) (*string, error) { } func getVnetGUID(cfg *auth.Config, subnetID string) (string, error) { - creds, err := auth.NewCredential(cfg) + creds, err := azidentity.NewDefaultAzureCredential(nil) if err != nil { return "", err } diff --git a/pkg/providers/instance/azure_client.go b/pkg/providers/instance/azure_client.go index e1efa118a..62a41f99b 100644 --- a/pkg/providers/instance/azure_client.go +++ b/pkg/providers/instance/azure_client.go @@ -20,6 +20,7 @@ import ( "context" "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute" armcomputev5 "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork" @@ -108,11 +109,11 @@ func CreateAZClient(ctx context.Context, cfg *auth.Config) (*AZClient, error) { } func NewAZClient(ctx context.Context, cfg *auth.Config, env *azure.Environment) (*AZClient, error) { - cred, err := auth.NewCredential(cfg) + defaultAzureCred, err := azidentity.NewDefaultAzureCredential(nil) if err != nil { return nil, err } - cred = auth.NewTokenWrapper(cred) + cred := auth.NewTokenWrapper(defaultAzureCred) opts := armopts.DefaultArmOpts() extensionsClient, err := armcompute.NewVirtualMachineExtensionsClient(cfg.SubscriptionID, cred, opts) if err != nil { diff --git a/pkg/providers/instance/skuclient/skuclient.go b/pkg/providers/instance/skuclient/skuclient.go index c69e304d0..4015c3556 100644 --- a/pkg/providers/instance/skuclient/skuclient.go +++ b/pkg/providers/instance/skuclient/skuclient.go @@ -22,9 +22,11 @@ import ( "time" "github.com/Azure/azure-sdk-for-go/profiles/latest/compute/mgmt/compute" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/Azure/go-autorest/autorest/azure" "github.com/Azure/karpenter-provider-azure/pkg/auth" "github.com/Azure/skewer" + "github.com/jongio/azidext/go/azidext" klog "k8s.io/klog/v2" ) @@ -48,11 +50,14 @@ func (sc *skuClient) updateInstance() { sc.mu.RLock() defer sc.mu.RUnlock() - authorizer, err := auth.NewAuthorizer(sc.cfg, sc.env) + // Create a new authorizer for the sku client + // TODO (charliedmcb): need to get track 2 support for the skewer API + cred, err := azidentity.NewDefaultAzureCredential(nil) if err != nil { - klog.V(5).Infof("Error creating authorizer for sku client: %s", err) + klog.V(5).Infof("Error creating authorizer for sku client: default cred: %s", err) return } + authorizer := azidext.NewTokenCredentialAdapter(cred, []string{azidext.DefaultManagementScope}) azClientConfig := sc.cfg.GetAzureClientConfig(authorizer, sc.env) azClientConfig.UserAgent = auth.GetUserAgentExtension()