golinter changes + linter script

cluster-autoscaler/cloudprovider/azure/.golangci.yml

@@ -0,0 +1,155 @@
+linters-settings:
+  depguard:
+    # list-type: denylist
+    # packages:
+    #   # logging is allowed only by logutils.Log, logrus
+    #   # is allowed to use only in logutils package
+    #   - github.com/sirupsen/logrus
+    # packages-with-error-message:
+    #   - github.com/sirupsen/logrus: "logging is allowed only by logutils.Log"
+  dupl:
+    threshold: 100
+  funlen:
+    lines: 100
+    statements: 50
+  goconst:
+    min-len: 2
+    min-occurrences: 3
+  gocritic:
+    enabled-tags:
+      - diagnostic
+      - experimental
+      - opinionated
+      - performance
+      - style
+    disabled-checks:
+      - dupImport # https://github.com/go-critic/go-critic/issues/845
+      - ifElseChain
+      - octalLiteral
+      - whyNoLint
+      - wrapperFunc
+  gocyclo:
+    min-complexity: 20
+  goimports:
+    local-prefixes: k8s.io/autoscaler/cluster-autoscaler/cloudprovider/azure
+  govet:
+    check-shadowing: true
+    settings:
+      printf:
+        funcs:
+          - (github.com/golangci/golangci-lint/pkg/logutils.Log).Infof
+          - (github.com/golangci/golangci-lint/pkg/logutils.Log).Warnf
+          - (github.com/golangci/golangci-lint/pkg/logutils.Log).Errorf
+          - (github.com/golangci/golangci-lint/pkg/logutils.Log).Fatalf
+  lll:
+    line-length: 140
+  misspell:
+    locale: US
+  nolintlint:
+    # allow-leading-space: true # don't require machine-readable nolint directives (i.e. with no leading space)
+    allow-unused: false # report any unused nolint directives
+    require-explanation: false # don't require an explanation for nolint directives
+    require-specific: false # don't require nolint directives to be specific about which linter is being skipped
+
+linters:
+  disable-all: true
+  enable:
+    - bodyclose
+    - depguard
+    - dogsled
+    - dupl
+    - errcheck
+    - exportloopref
+    - funlen
+    - gochecknoinits
+    - goconst
+    - gocritic
+    - gocyclo
+    - gofmt
+    - goimports
+    - goprintffuncname
+    - gosec
+    # #- gosimple
+    # #- govet
+    - ineffassign
+    - lll
+    - misspell
+    - nakedret
+    - noctx
+    # #- staticcheck
+    - stylecheck
+    - typecheck
+    - unconvert
+    - unparam
+    - unused
+    - whitespace
+
+  # don't enable:
+  # - asciicheck
+  # - scopelint
+  # - gomnd
+  # - gochecknoglobals
+  # - gocognit
+  # - godot
+  # - godox
+  # - goerr113
+  # - interfacer
+  # - maligned
+  # - nestif
+  # - prealloc
+  # - testpackage
+  # - revive
+  # - wsl
+  # - structcheck - https://github.com/golangci/golangci-lint/issues/1841 (deprecated)
+  # - deadcode - deprecated (since v1.49.0) due to: The owner seems to have abandoned the linter
+  # - varcheck - deprecated (since v1.49.0) due to: The owner seems to have abandoned the linter
+
+issues:
+  # Excluding configuration per-path, per-linter, per-text and per-source
+  exclude-rules:
+    # https://github.com/go-critic/go-critic/issues/926
+    - linters:
+        - gocritic
+      text: "unnecessaryDefer:"
+
+    # tests can be more readable with long procedures over jumping between funcs
+    - path: _test\.go
+      linters:
+        - funlen
+
+    # use of init() in this test file is essential
+    - path: azure_scale_set_test.go
+      linters:
+        - gochecknoinits
+
+    # it's okay to use math/rand instead of crypto/rand when speed matters more than safety (often in tests)
+    - linters:
+        - gosec
+      text: "Use of weak random number generator"
+
+    # this is a test-case for which we need error message to be capitalized.
+    - path: azure_kubernetes_service_pool_test.go
+      text: "ST1005: error strings should not be capitalized"
+
+    # NodeGroup() has Id() as a method.
+    - path: azure_scale_set.go
+      text: "ST1003: method Id should be ID"
+    - path: azure_agent_pool.go
+      text: "ST1003: method Id should be ID"
+    - path: azure_kubernetes_service_pool.go
+      text: "ST1003: method Id should be ID"
+
+    # opts cannot be passed as pointer here to avoid changes in files other than azure/.
+    - path: azure_cloud_provider.go
+      text: "hugeParam"
+
+run:
+  skip-dirs:
+    - vendor
+
+# golangci.com configuration
+# https://github.com/golangci/golangci/wiki/Configuration
+service:
+  golangci-lint-version: 1.55.x # use the fixed version to not introduce new linters unexpectedly
+  prepare:
+    - echo "here I can run custom commands, but no preparation needed for this repo"

cluster-autoscaler/cloudprovider/azure/azure_agent_pool.go

@@ -26,8 +26,6 @@ import (
 
 	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2022-08-01/compute"
 	"github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2017-05-10/resources"
-	azStorage "github.com/Azure/azure-sdk-for-go/storage"
-	"github.com/Azure/go-autorest/autorest/to"
 
 	apiv1 "k8s.io/api/core/v1"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
@@ -82,9 +80,11 @@ func (as *AgentPool) initialize() error {
 	ctx, cancel := getContextWithCancel()
 	defer cancel()
 
-	template, err := as.manager.azClient.deploymentsClient.ExportTemplate(ctx, as.manager.config.ResourceGroup, as.manager.config.Deployment)
+	template, err := as.manager.azClient.deploymentsClient.ExportTemplate(ctx, as.manager.config.ResourceGroup,
+		as.manager.config.Deployment)
 	if err != nil {
-		klog.Errorf("deploymentsClient.ExportTemplate(%s, %s) failed: %v", as.manager.config.ResourceGroup, as.manager.config.Deployment, err)
+		klog.Errorf("deploymentsClient.ExportTemplate(%s, %s) failed: %v", as.manager.config.ResourceGroup,
+			as.manager.config.Deployment, err)
 		return err
 	}
 
@@ -145,7 +145,7 @@ func (as *AgentPool) getVMsFromCache() ([]compute.VirtualMachine, error) {
 }
 
 // GetVMIndexes gets indexes of all virtual machines belonging to the agent pool.
-func (as *AgentPool) GetVMIndexes() ([]int, map[int]string, error) {
+func (as *AgentPool) GetVMIndexes() (sortedIndexes []int, indexToVM map[int]string, err error) {
 	klog.V(6).Infof("GetVMIndexes: starts for as %v", as)
 
 	instances, err := as.getVMsFromCache()
@@ -155,23 +155,23 @@ func (as *AgentPool) GetVMIndexes() ([]int, map[int]string, error) {
 	klog.V(6).Infof("GetVMIndexes: got instances, length = %d", len(instances))
 
 	indexes := make([]int, 0)
-	indexToVM := make(map[int]string)
+	indexToVM = make(map[int]string)
 	for _, instance := range instances {
 		index, err := GetVMNameIndex(instance.StorageProfile.OsDisk.OsType, *instance.Name)
 		if err != nil {
 			return nil, nil, err
 		}
 
 		indexes = append(indexes, index)
-		resourceID, err := convertResourceGroupNameToLower("azure://" + *instance.ID)
+		resourceID, err := convertResourceGroupNameToLower(azurePrefix + *instance.ID)
 		if err != nil {
 			return nil, nil, err
 		}
 		indexToVM[index] = resourceID
 	}
 
-	sortedIndexes := sort.IntSlice(indexes)
-	sortedIndexes.Sort()
+	sortedIndexes = sort.IntSlice(indexes)
+	sort.Ints(sortedIndexes)
 	return sortedIndexes, indexToVM, nil
 }
 
@@ -216,7 +216,8 @@ func (as *AgentPool) getAllSucceededAndFailedDeployments() (succeededAndFailedDe
 	defer cancel()
 
 	deploymentsFilter := "provisioningState eq 'Succeeded' or provisioningState eq 'Failed'"
-	succeededAndFailedDeployments, err = as.manager.azClient.deploymentsClient.List(ctx, as.manager.config.ResourceGroup, deploymentsFilter, nil)
+	succeededAndFailedDeployments, err = as.manager.azClient.deploymentsClient.List(ctx, as.manager.config.ResourceGroup,
+		deploymentsFilter, nil)
 	if err != nil {
 		klog.Errorf("getAllSucceededAndFailedDeployments: failed to list succeeded or failed deployments with error: %v", err)
 		return nil, err
@@ -258,10 +259,14 @@ func (as *AgentPool) deleteOutdatedDeployments() (err error) {
 	errList := make([]error, 0)
 	for _, deployment := range toBeDeleted {
 		klog.V(4).Infof("deleteOutdatedDeployments: starts deleting outdated deployment (%s)", *deployment.Name)
-		_, err := as.manager.azClient.deploymentsClient.Delete(ctx, as.manager.config.ResourceGroup, *deployment.Name)
+		resp, err := as.manager.azClient.deploymentsClient.Delete(ctx, as.manager.config.ResourceGroup, *deployment.Name)
+
 		if err != nil {
 			errList = append(errList, err)
 		}
+		if resp != nil && resp.Body != nil {
+			resp.Body.Close()
+		}
 	}
 
 	return utilerrors.NewAggregate(errList)
@@ -317,11 +322,17 @@ func (as *AgentPool) IncreaseSize(delta int) error {
 	}
 	ctx, cancel := getContextWithCancel()
 	defer cancel()
-	klog.V(3).Infof("Waiting for deploymentsClient.CreateOrUpdate(%s, %s, %v)", as.manager.config.ResourceGroup, newDeploymentName, newDeployment)
-	resp, err := as.manager.azClient.deploymentsClient.CreateOrUpdate(ctx, as.manager.config.ResourceGroup, newDeploymentName, newDeployment)
+	klog.V(3).Infof("Waiting for deploymentsClient.CreateOrUpdate(%s, %s, %v)", as.manager.config.ResourceGroup,
+		newDeploymentName, newDeployment)
+	resp, err := as.manager.azClient.deploymentsClient.CreateOrUpdate(ctx, as.manager.config.ResourceGroup,
+		newDeploymentName, newDeployment)
+	if resp != nil && resp.Body != nil {
+		defer resp.Body.Close()
+	}
 	isSuccess, realError := isSuccessHTTPResponse(resp, err)
 	if isSuccess {
-		klog.V(3).Infof("deploymentsClient.CreateOrUpdate(%s, %s, %v) success", as.manager.config.ResourceGroup, newDeploymentName, newDeployment)
+		klog.V(3).Infof("deploymentsClient.CreateOrUpdate(%s, %s, %v) success", as.manager.config.ResourceGroup,
+			newDeploymentName, newDeployment)
 
 		// Update cache after scale success.
 		as.curSize = int64(expectedSize)
@@ -404,7 +415,7 @@ func (as *AgentPool) DeleteInstances(instances []*azureRef) error {
 	}
 
 	for _, instance := range instances {
-		name, err := resourceName((*instance).Name)
+		name, err := resourceName((instance).Name)
 		if err != nil {
 			klog.Errorf("Get name for instance %q failed: %v", *instance, err)
 			return err
@@ -478,13 +489,13 @@ func (as *AgentPool) Nodes() ([]cloudprovider.Instance, error) {
 
 	nodes := make([]cloudprovider.Instance, 0, len(instances))
 	for _, instance := range instances {
-		if len(*instance.ID) == 0 {
+		if *instance.ID == "" {
 			continue
 		}
 
 		// To keep consistent with providerID from kubernetes cloud provider, convert
 		// resourceGroupName in the ID to lower case.
-		resourceID, err := convertResourceGroupNameToLower("azure://" + *instance.ID)
+		resourceID, err := convertResourceGroupNameToLower(azurePrefix + *instance.ID)
 		if err != nil {
 			return nil, err
 		}
@@ -494,28 +505,6 @@ func (as *AgentPool) Nodes() ([]cloudprovider.Instance, error) {
 	return nodes, nil
 }
 
-func (as *AgentPool) deleteBlob(accountName, vhdContainer, vhdBlob string) error {
-	ctx, cancel := getContextWithCancel()
-	defer cancel()
-
-	storageKeysResult, rerr := as.manager.azClient.storageAccountsClient.ListKeys(ctx, as.manager.config.SubscriptionID, as.manager.config.ResourceGroup, accountName)
-	if rerr != nil {
-		return rerr.Error()
-	}
-
-	keys := *storageKeysResult.Keys
-	client, err := azStorage.NewBasicClientOnSovereignCloud(accountName, to.String(keys[0].Value), as.manager.env)
-	if err != nil {
-		return err
-	}
-
-	bs := client.GetBlobService()
-	containerRef := bs.GetContainerReference(vhdContainer)
-	blobRef := containerRef.GetBlobReference(vhdBlob)
-
-	return blobRef.Delete(&azStorage.DeleteBlobOptions{})
-}
-
 // deleteVirtualMachine deletes a VM and any associated OS disk
 func (as *AgentPool) deleteVirtualMachine(name string) error {
 	ctx, cancel := getContextWithCancel()
@@ -564,55 +553,17 @@ func (as *AgentPool) deleteVirtualMachine(name string) error {
 	}
 	klog.V(2).Infof("VirtualMachine %s/%s removed", as.manager.config.ResourceGroup, name)
 
-	if len(nicName) > 0 {
-		klog.Infof("deleting nic: %s/%s", as.manager.config.ResourceGroup, nicName)
-		interfaceCtx, interfaceCancel := getContextWithCancel()
-		defer interfaceCancel()
-		rerr := as.manager.azClient.interfacesClient.Delete(interfaceCtx, as.manager.config.ResourceGroup, nicName)
-		klog.Infof("waiting for nic deletion: %s/%s", as.manager.config.ResourceGroup, nicName)
-		_, realErr := checkResourceExistsFromRetryError(rerr)
-		if realErr != nil {
-			return realErr
-		}
-		klog.V(2).Infof("interface %s/%s removed", as.manager.config.ResourceGroup, nicName)
+	if deleteNicErr := deleteNic(as.manager.azClient.interfacesClient, nicName, as.manager.config.ResourceGroup); deleteNicErr != nil {
+		return deleteNicErr
 	}
 
 	if vhd != nil {
-		accountName, vhdContainer, vhdBlob, err := splitBlobURI(*vhd.URI)
-		if err != nil {
-			return err
-		}
-
-		klog.Infof("found os disk storage reference: %s %s %s", accountName, vhdContainer, vhdBlob)
-
-		klog.Infof("deleting blob: %s/%s", vhdContainer, vhdBlob)
-		if err = as.deleteBlob(accountName, vhdContainer, vhdBlob); err != nil {
-			_, realErr := checkResourceExistsFromError(err)
-			if realErr != nil {
-				return realErr
-			}
-			klog.V(2).Infof("Blob %s/%s removed", as.manager.config.ResourceGroup, vhdBlob)
-		}
+		return deleteVhdBlob(as.manager.azClient.storageAccountsClient, vhd, as.manager.env, as.manager.config.ResourceGroup,
+			as.manager.config.SubscriptionID)
 	} else if managedDisk != nil {
-		if osDiskName == nil {
-			klog.Warningf("osDisk is not set for VM %s/%s", as.manager.config.ResourceGroup, name)
-		} else {
-			klog.Infof("deleting managed disk: %s/%s", as.manager.config.ResourceGroup, *osDiskName)
-			disksCtx, disksCancel := getContextWithCancel()
-			defer disksCancel()
-			rerr := as.manager.azClient.disksClient.Delete(disksCtx, as.manager.config.SubscriptionID, as.manager.config.ResourceGroup, *osDiskName)
-			_, realErr := checkResourceExistsFromRetryError(rerr)
-			if realErr != nil {
-				return realErr
-			}
-			klog.V(2).Infof("disk %s/%s removed", as.manager.config.ResourceGroup, *osDiskName)
-		}
+		return deleteManagedDisk(as.manager.azClient.disksClient, managedDisk, osDiskName, name, as.manager.config.ResourceGroup,
+			as.manager.config.SubscriptionID)
 	}
 
 	return nil
 }
-
-// getAzureRef gets AzureRef for the as.
-func (as *AgentPool) getAzureRef() azureRef {
-	return as.azureRef
-}