From 687fe2d092bede0da057a74774116912833dafaf Mon Sep 17 00:00:00 2001 From: Khaja Omer Date: Thu, 14 Mar 2024 14:11:17 -0500 Subject: [PATCH 1/7] [Bug-Fix] - Improved error handling by using linodego provided method to unwrap the error in a linodego.Error{} struct --- cloud/services/loadbalancers.go | 4 ++-- controller/linodecluster_controller.go | 1 - devbox.lock | 16 ++++++++-------- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/cloud/services/loadbalancers.go b/cloud/services/loadbalancers.go index a281ed7bf..a44074a2c 100644 --- a/cloud/services/loadbalancers.go +++ b/cloud/services/loadbalancers.go @@ -60,8 +60,8 @@ func CreateNodeBalancer(ctx context.Context, clusterScope *scope.ClusterScope, l logger.Info("Failed to create Linode NodeBalancer", "error", err.Error()) // Already exists is not an error - apiErr := linodego.Error{} - if errors.As(err, &apiErr) && apiErr.Code != http.StatusFound { + apiErr := linodego.NewError(err) + if apiErr.Code != http.StatusFound { return nil, err } diff --git a/controller/linodecluster_controller.go b/controller/linodecluster_controller.go index 748aaf5ea..cc621b5af 100644 --- a/controller/linodecluster_controller.go +++ b/controller/linodecluster_controller.go @@ -165,7 +165,6 @@ func (r *LinodeClusterReconciler) reconcileCreate(ctx context.Context, logger lo linodeNB, err := services.CreateNodeBalancer(ctx, clusterScope, logger) if err != nil { setFailureReason(clusterScope, cerrs.CreateClusterError, err, r) - return err } diff --git a/devbox.lock b/devbox.lock index 41de68583..9babb9b7a 100644 --- a/devbox.lock +++ b/devbox.lock @@ -101,23 +101,23 @@ } } }, - "golangci-lint@1.56.1": { - "last_modified": "2024-02-12T13:06:46Z", - "resolved": "github:NixOS/nixpkgs/2d627a2a704708673e56346fcb13d25344b8eaf3#golangci-lint", + "golangci-lint@latest": { + "last_modified": "2024-02-24T23:06:34Z", + "resolved": "github:NixOS/nixpkgs/9a9dae8f6319600fa9aebde37f340975cab4b8c0#golangci-lint", "source": "devbox-search", - "version": "1.56.1", + "version": "1.56.2", "systems": { "aarch64-darwin": { - "store_path": "/nix/store/4f7s164lywkq0nzv2r80sj7z0kjvw7vs-golangci-lint-1.56.1" + "store_path": "/nix/store/fs44z0nysjiwl2sj2m9x70dbrx2lbyhh-golangci-lint-1.56.2" }, "aarch64-linux": { - "store_path": "/nix/store/dyv5cy8inj2f3m1ymh5jqisjrss53y7w-golangci-lint-1.56.1" + "store_path": "/nix/store/bs18c2bx56b6xnpf49wb5kzi3vynbqmx-golangci-lint-1.56.2" }, "x86_64-darwin": { - "store_path": "/nix/store/v57g47gzxgbwj6193n5zrhzrdrp53g1s-golangci-lint-1.56.1" + "store_path": "/nix/store/1xhl4b3nk3npq70d651vg4bamfg8xyga-golangci-lint-1.56.2" }, "x86_64-linux": { - "store_path": "/nix/store/5w4vqr92y2rs826z10lcnkhlv61bcxny-golangci-lint-1.56.1" + "store_path": "/nix/store/kc58bqdmjdc6mfilih5pywprs7r7lxrw-golangci-lint-1.56.2" } } }, From 3c21b0a8b452ad6a1d4d36372b11757d469441b6 Mon Sep 17 00:00:00 2001 From: Khaja Omer Date: Thu, 14 Mar 2024 14:16:20 -0500 Subject: [PATCH 2/7] Adding more fixes for other files --- cloud/services/object_storage_buckets.go | 9 ++++----- controller/linodecluster_controller.go | 5 ++--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/cloud/services/object_storage_buckets.go b/cloud/services/object_storage_buckets.go index cd2c327b9..50e2d6b50 100644 --- a/cloud/services/object_storage_buckets.go +++ b/cloud/services/object_storage_buckets.go @@ -2,7 +2,6 @@ package services import ( "context" - "errors" "fmt" "net/http" @@ -18,8 +17,8 @@ func EnsureObjectStorageBucket(ctx context.Context, bScope *scope.ObjectStorageB bScope.Bucket.Spec.Cluster, bScope.Bucket.Name, ) - linodeErr := &linodego.Error{} - if errors.As(err, linodeErr) && linodeErr.StatusCode() != http.StatusNotFound { + linodeErr := linodego.NewError(err) + if linodeErr.StatusCode() != http.StatusNotFound { return nil, fmt.Errorf("failed to get bucket from cluster %s: %w", bScope.Bucket.Spec.Cluster, err) } if bucket != nil { @@ -109,8 +108,8 @@ func RevokeObjectStorageKeys(ctx context.Context, bScope *scope.ObjectStorageBuc func revokeObjectStorageKey(ctx context.Context, bScope *scope.ObjectStorageBucketScope, keyID int) error { if err := bScope.LinodeClient.DeleteObjectStorageKey(ctx, keyID); err != nil { - linodeErr := &linodego.Error{} - if errors.As(err, linodeErr) && linodeErr.StatusCode() != http.StatusNotFound { + linodeErr := linodego.NewError(err) + if linodeErr.StatusCode() != http.StatusNotFound { bScope.Logger.Error(err, "Failed to revoke access key", "id", keyID) return fmt.Errorf("failed to revoke access key: %w", err) diff --git a/controller/linodecluster_controller.go b/controller/linodecluster_controller.go index cc621b5af..8c1332ede 100644 --- a/controller/linodecluster_controller.go +++ b/controller/linodecluster_controller.go @@ -18,7 +18,6 @@ package controller import ( "context" - "errors" "fmt" "net/http" "time" @@ -200,8 +199,8 @@ func (r *LinodeClusterReconciler) reconcileDelete(ctx context.Context, logger lo logger.Info("Failed to delete Linode NodeBalancer", "error", err.Error()) // Not found is not an error - apiErr := linodego.Error{} - if errors.As(err, &apiErr) && apiErr.Code != http.StatusNotFound { + apiErr := linodego.NewError(err) + if apiErr.Code != http.StatusNotFound { setFailureReason(clusterScope, cerrs.DeleteClusterError, err, r) return err From 1b3cbb711e78af436ec4e55c1888949a858b92d6 Mon Sep 17 00:00:00 2001 From: Khaja Omer Date: Thu, 14 Mar 2024 14:30:48 -0500 Subject: [PATCH 3/7] Fixes --- cloud/services/object_storage_buckets.go | 8 +++++--- controller/linodecluster_controller.go | 6 ++++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/cloud/services/object_storage_buckets.go b/cloud/services/object_storage_buckets.go index 50e2d6b50..647301bed 100644 --- a/cloud/services/object_storage_buckets.go +++ b/cloud/services/object_storage_buckets.go @@ -17,9 +17,11 @@ func EnsureObjectStorageBucket(ctx context.Context, bScope *scope.ObjectStorageB bScope.Bucket.Spec.Cluster, bScope.Bucket.Name, ) - linodeErr := linodego.NewError(err) - if linodeErr.StatusCode() != http.StatusNotFound { - return nil, fmt.Errorf("failed to get bucket from cluster %s: %w", bScope.Bucket.Spec.Cluster, err) + if err != nil { + linodeErr := linodego.NewError(err) + if linodeErr.StatusCode() != http.StatusNotFound { + return nil, fmt.Errorf("failed to get bucket from cluster %s: %w", bScope.Bucket.Spec.Cluster, err) + } } if bucket != nil { bScope.Logger.Info("Bucket exists") diff --git a/controller/linodecluster_controller.go b/controller/linodecluster_controller.go index 8c1332ede..c93fb1f58 100644 --- a/controller/linodecluster_controller.go +++ b/controller/linodecluster_controller.go @@ -167,6 +167,12 @@ func (r *LinodeClusterReconciler) reconcileCreate(ctx context.Context, logger lo return err } + if linodeNB == nil { + err := fmt.Errorf("NodeBalancer created was nil") + setFailureReason(clusterScope, cerrs.CreateClusterError, err, r) + return err + } + clusterScope.LinodeCluster.Spec.Network.NodeBalancerID = &linodeNB.ID linodeNBConfig, err := services.CreateNodeBalancerConfig(ctx, clusterScope, logger) From 9b227b5db5f7260a16205f0d43227d2a98cb3aeb Mon Sep 17 00:00:00 2001 From: Khaja Omer Date: Thu, 14 Mar 2024 15:33:30 -0500 Subject: [PATCH 4/7] Minor update --- controller/linodecluster_controller.go | 11 ++++------- controller/linodeobjectstoragebucket_controller.go | 5 ++++- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/controller/linodecluster_controller.go b/controller/linodecluster_controller.go index c93fb1f58..30bc390d2 100644 --- a/controller/linodecluster_controller.go +++ b/controller/linodecluster_controller.go @@ -162,13 +162,10 @@ func setFailureReason(clusterScope *scope.ClusterScope, failureReason cerrs.Clus func (r *LinodeClusterReconciler) reconcileCreate(ctx context.Context, logger logr.Logger, clusterScope *scope.ClusterScope) error { linodeNB, err := services.CreateNodeBalancer(ctx, clusterScope, logger) - if err != nil { - setFailureReason(clusterScope, cerrs.CreateClusterError, err, r) - return err - } - - if linodeNB == nil { - err := fmt.Errorf("NodeBalancer created was nil") + if err != nil || linodeNB == nil { + if err == nil { + err = fmt.Errorf("nodeBalancer created was nil") + } setFailureReason(clusterScope, cerrs.CreateClusterError, err, r) return err } diff --git a/controller/linodeobjectstoragebucket_controller.go b/controller/linodeobjectstoragebucket_controller.go index a781a6d44..81f1a9ba3 100644 --- a/controller/linodeobjectstoragebucket_controller.go +++ b/controller/linodeobjectstoragebucket_controller.go @@ -145,7 +145,10 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context } bucket, err := services.EnsureObjectStorageBucket(ctx, bScope) - if err != nil { + if err != nil || bucket == nil { + if err == nil { + err = fmt.Errorf("bucket created is nil") + } bScope.Logger.Error(err, "Failed to ensure bucket exists") r.setFailure(bScope, err) From 27ed75e538afa059da5d01c2297804b7d87e8f4f Mon Sep 17 00:00:00 2001 From: Khaja Omer Date: Thu, 14 Mar 2024 16:18:13 -0500 Subject: [PATCH 5/7] Reverting to old way --- controller/linodecluster_controller.go | 11 +++++++---- controller/linodeobjectstoragebucket_controller.go | 12 ++++++++---- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/controller/linodecluster_controller.go b/controller/linodecluster_controller.go index 30bc390d2..c57084e7b 100644 --- a/controller/linodecluster_controller.go +++ b/controller/linodecluster_controller.go @@ -162,10 +162,13 @@ func setFailureReason(clusterScope *scope.ClusterScope, failureReason cerrs.Clus func (r *LinodeClusterReconciler) reconcileCreate(ctx context.Context, logger logr.Logger, clusterScope *scope.ClusterScope) error { linodeNB, err := services.CreateNodeBalancer(ctx, clusterScope, logger) - if err != nil || linodeNB == nil { - if err == nil { - err = fmt.Errorf("nodeBalancer created was nil") - } + if err != nil { + setFailureReason(clusterScope, cerrs.CreateClusterError, err, r) + return err + } + + if linodeNB == nil { + err = fmt.Errorf("nodeBalancer created was nil") setFailureReason(clusterScope, cerrs.CreateClusterError, err, r) return err } diff --git a/controller/linodeobjectstoragebucket_controller.go b/controller/linodeobjectstoragebucket_controller.go index 81f1a9ba3..0778fe5b9 100644 --- a/controller/linodeobjectstoragebucket_controller.go +++ b/controller/linodeobjectstoragebucket_controller.go @@ -145,15 +145,19 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context } bucket, err := services.EnsureObjectStorageBucket(ctx, bScope) - if err != nil || bucket == nil { - if err == nil { - err = fmt.Errorf("bucket created is nil") - } + if err != nil { bScope.Logger.Error(err, "Failed to ensure bucket exists") r.setFailure(bScope, err) return err } + + if bucket == nil { + err = fmt.Errorf("bucket created is nil") + bScope.Logger.Error(err, "Failed to ensure bucket exists") + r.setFailure(bScope, err) + } + bScope.Bucket.Status.Hostname = util.Pointer(bucket.Hostname) bScope.Bucket.Status.CreationTime = &metav1.Time{Time: *bucket.Created} From b339cf13ad9bc188d8caaf47606d07483fa84011 Mon Sep 17 00:00:00 2001 From: Khaja Omer Date: Thu, 14 Mar 2024 16:41:29 -0500 Subject: [PATCH 6/7] Using util.IgnoreLinodeAPIError() instead of linodego.NewError() --- cloud/services/loadbalancers.go | 18 ++++++------------ cloud/services/object_storage_buckets.go | 19 +++++++------------ controller/linodecluster_controller.go | 15 ++++----------- controller/linodemachine_controller.go | 3 +-- 4 files changed, 18 insertions(+), 37 deletions(-) diff --git a/cloud/services/loadbalancers.go b/cloud/services/loadbalancers.go index a44074a2c..106283a34 100644 --- a/cloud/services/loadbalancers.go +++ b/cloud/services/loadbalancers.go @@ -56,18 +56,12 @@ func CreateNodeBalancer(ctx context.Context, clusterScope *scope.ClusterScope, l Tags: tags, } - if linodeNB, err = clusterScope.LinodeClient.CreateNodeBalancer(ctx, createConfig); err != nil { - logger.Info("Failed to create Linode NodeBalancer", "error", err.Error()) - - // Already exists is not an error - apiErr := linodego.NewError(err) - if apiErr.Code != http.StatusFound { - return nil, err - } - - if linodeNB != nil { - logger.Info("Linode NodeBalancer already exists", "existing", linodeNB.Label) - } + linodeNB, err = clusterScope.LinodeClient.CreateNodeBalancer(ctx, createConfig) + if util.IgnoreLinodeAPIError(err, http.StatusNotFound) != nil { + return nil, err + } + if linodeNB != nil { + logger.Info("Linode NodeBalancer already exists", "existing", linodeNB.Label) } return linodeNB, nil diff --git a/cloud/services/object_storage_buckets.go b/cloud/services/object_storage_buckets.go index 647301bed..8240ad232 100644 --- a/cloud/services/object_storage_buckets.go +++ b/cloud/services/object_storage_buckets.go @@ -9,6 +9,7 @@ import ( utilerrors "k8s.io/apimachinery/pkg/util/errors" "github.com/linode/cluster-api-provider-linode/cloud/scope" + "github.com/linode/cluster-api-provider-linode/util" ) func EnsureObjectStorageBucket(ctx context.Context, bScope *scope.ObjectStorageBucketScope) (*linodego.ObjectStorageBucket, error) { @@ -17,11 +18,8 @@ func EnsureObjectStorageBucket(ctx context.Context, bScope *scope.ObjectStorageB bScope.Bucket.Spec.Cluster, bScope.Bucket.Name, ) - if err != nil { - linodeErr := linodego.NewError(err) - if linodeErr.StatusCode() != http.StatusNotFound { - return nil, fmt.Errorf("failed to get bucket from cluster %s: %w", bScope.Bucket.Spec.Cluster, err) - } + if util.IgnoreLinodeAPIError(err, http.StatusNotFound) != nil { + return nil, fmt.Errorf("failed to get bucket from cluster %s: %w", bScope.Bucket.Spec.Cluster, err) } if bucket != nil { bScope.Logger.Info("Bucket exists") @@ -109,13 +107,10 @@ func RevokeObjectStorageKeys(ctx context.Context, bScope *scope.ObjectStorageBuc } func revokeObjectStorageKey(ctx context.Context, bScope *scope.ObjectStorageBucketScope, keyID int) error { - if err := bScope.LinodeClient.DeleteObjectStorageKey(ctx, keyID); err != nil { - linodeErr := linodego.NewError(err) - if linodeErr.StatusCode() != http.StatusNotFound { - bScope.Logger.Error(err, "Failed to revoke access key", "id", keyID) - - return fmt.Errorf("failed to revoke access key: %w", err) - } + err := bScope.LinodeClient.DeleteObjectStorageKey(ctx, keyID) + if util.IgnoreLinodeAPIError(err, http.StatusNotFound) != nil { + bScope.Logger.Error(err, "Failed to revoke access key", "id", keyID) + return fmt.Errorf("failed to revoke access key: %w", err) } bScope.Logger.Info("Revoked access key", "id", keyID) diff --git a/controller/linodecluster_controller.go b/controller/linodecluster_controller.go index c57084e7b..aa636ce6b 100644 --- a/controller/linodecluster_controller.go +++ b/controller/linodecluster_controller.go @@ -23,7 +23,6 @@ import ( "time" "github.com/go-logr/logr" - "github.com/linode/linodego" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" utilerrors "k8s.io/apimachinery/pkg/util/errors" @@ -201,16 +200,10 @@ func (r *LinodeClusterReconciler) reconcileDelete(ctx context.Context, logger lo return nil } - if err := clusterScope.LinodeClient.DeleteNodeBalancer(ctx, *clusterScope.LinodeCluster.Spec.Network.NodeBalancerID); err != nil { - logger.Info("Failed to delete Linode NodeBalancer", "error", err.Error()) - - // Not found is not an error - apiErr := linodego.NewError(err) - if apiErr.Code != http.StatusNotFound { - setFailureReason(clusterScope, cerrs.DeleteClusterError, err, r) - - return err - } + err := clusterScope.LinodeClient.DeleteNodeBalancer(ctx, *clusterScope.LinodeCluster.Spec.Network.NodeBalancerID) + if util.IgnoreLinodeAPIError(err, http.StatusNotFound) != nil { + setFailureReason(clusterScope, cerrs.DeleteClusterError, err, r) + return err } conditions.MarkFalse(clusterScope.LinodeCluster, clusterv1.ReadyCondition, clusterv1.DeletedReason, clusterv1.ConditionSeverityInfo, "Load balancer deleted") diff --git a/controller/linodemachine_controller.go b/controller/linodemachine_controller.go index 74fa36b6d..298f2e827 100644 --- a/controller/linodemachine_controller.go +++ b/controller/linodemachine_controller.go @@ -372,8 +372,7 @@ func (r *LinodeMachineReconciler) reconcileUpdate( } if linodeInstance, err = machineScope.LinodeClient.GetInstance(ctx, *machineScope.LinodeMachine.Spec.InstanceID); err != nil { - err = util.IgnoreLinodeAPIError(err, http.StatusNotFound) - if err != nil { + if util.IgnoreLinodeAPIError(err, http.StatusNotFound) != nil { logger.Error(err, "Failed to get Linode machine instance") } else { logger.Info("Instance not found, let's create a new one") From 4995b4375c3ee411680a290e462d4fba09f32a44 Mon Sep 17 00:00:00 2001 From: Khaja Omer Date: Thu, 14 Mar 2024 16:48:30 -0500 Subject: [PATCH 7/7] Fix lint err --- controller/linodeobjectstoragebucket_controller.go | 1 + 1 file changed, 1 insertion(+) diff --git a/controller/linodeobjectstoragebucket_controller.go b/controller/linodeobjectstoragebucket_controller.go index 0778fe5b9..4679c1c8e 100644 --- a/controller/linodeobjectstoragebucket_controller.go +++ b/controller/linodeobjectstoragebucket_controller.go @@ -156,6 +156,7 @@ func (r *LinodeObjectStorageBucketReconciler) reconcileApply(ctx context.Context err = fmt.Errorf("bucket created is nil") bScope.Logger.Error(err, "Failed to ensure bucket exists") r.setFailure(bScope, err) + return err } bScope.Bucket.Status.Hostname = util.Pointer(bucket.Hostname)