diff --git a/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go b/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go index e1053a25cdcd..fe6e15b37c86 100644 --- a/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go +++ b/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go @@ -194,11 +194,6 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques // Lookup the cluster the config owner is associated with cluster, err := util.GetClusterByName(ctx, r.Client, configOwner.GetNamespace(), configOwner.ClusterName()) if err != nil { - if errors.Cause(err) == util.ErrNoCluster { - log.Info(fmt.Sprintf("%s does not belong to a cluster yet, waiting until it's part of a cluster", configOwner.GetKind())) - return ctrl.Result{}, nil - } - if apierrors.IsNotFound(err) { log.Info("Cluster does not exist yet, waiting until it is created") return ctrl.Result{}, nil diff --git a/cmd/clusterctl/client/cluster/template.go b/cmd/clusterctl/client/cluster/template.go index 872fd4eed6c2..7216d187025b 100644 --- a/cmd/clusterctl/client/cluster/template.go +++ b/cmd/clusterctl/client/cluster/template.go @@ -303,10 +303,12 @@ func getGitHubClient(ctx context.Context, configVariablesClient config.Variables return github.NewClient(authenticatingHTTPClient), nil } +var errRateLimit = errors.New("rate limit for github api has been reached. Please wait one hour or get a personal API token and assign it to the GITHUB_TOKEN environment variable") + // handleGithubErr wraps error messages. func handleGithubErr(err error, message string, args ...interface{}) error { if _, ok := err.(*github.RateLimitError); ok { - return errors.New("rate limit for github api has been reached. Please wait one hour or get a personal API token and assign it to the GITHUB_TOKEN environment variable") + return errRateLimit } - return errors.Wrapf(err, message, args...) + return fmt.Errorf("%s: %w", fmt.Sprintf(message, args...), err) } diff --git a/cmd/clusterctl/client/cluster/template_test.go b/cmd/clusterctl/client/cluster/template_test.go index 79c6a809711f..9679e5322c07 100644 --- a/cmd/clusterctl/client/cluster/template_test.go +++ b/cmd/clusterctl/client/cluster/template_test.go @@ -29,6 +29,7 @@ import ( "github.com/google/go-github/v53/github" . "github.com/onsi/gomega" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -517,6 +518,43 @@ func Test_templateClient_GetFromURL(t *testing.T) { } } +func Test_handleGithubErr(t *testing.T) { + tests := []struct { + name string + err error + message string + args []any + want error + }{ + { + name: "Return error", + err: errors.New("error"), + message: "message %s and %s", + args: []any{"arg1", "arg2"}, + want: fmt.Errorf("message arg1 and arg2: %w", errors.New("error")), + }, + { + name: "Return RateLimitError", + err: &github.RateLimitError{ + Response: &http.Response{ + StatusCode: http.StatusForbidden, + }, + }, + message: "", + args: nil, + want: errRateLimit, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + got := handleGithubErr(tt.err, tt.message, tt.args...) + g.Expect(got.Error()).To(Equal(tt.want.Error())) + }) + } +} + func mustParseURL(rawURL string) *url.URL { rURL, err := url.Parse(rawURL) if err != nil { diff --git a/cmd/clusterctl/client/repository/repository_github.go b/cmd/clusterctl/client/repository/repository_github.go index bf772a9b04b2..7f01a0f953a7 100644 --- a/cmd/clusterctl/client/repository/repository_github.go +++ b/cmd/clusterctl/client/repository/repository_github.go @@ -50,7 +50,8 @@ const ( ) var ( - errNotFound = errors.New("404 Not Found") + errNotFound = errors.New("404 Not Found") + errRateLimit = errors.New("rate limit for github api has been reached. Please wait one hour or get a personal API token and assign it to the GITHUB_TOKEN environment variable") // Caches used to limit the number of GitHub API calls. @@ -319,7 +320,7 @@ func (g *gitHubRepository) getVersions(ctx context.Context) ([]string, error) { if listReleasesErr != nil { retryError = g.handleGithubErr(listReleasesErr, "failed to get the list of releases") // Return immediately if we are rate limited. - if _, ok := listReleasesErr.(*github.RateLimitError); ok { + if errors.Is(retryError, errRateLimit) { return false, retryError } return false, nil @@ -334,7 +335,7 @@ func (g *gitHubRepository) getVersions(ctx context.Context) ([]string, error) { if listReleasesErr != nil { retryError = g.handleGithubErr(listReleasesErr, "failed to get the list of releases") // Return immediately if we are rate limited. - if _, ok := listReleasesErr.(*github.RateLimitError); ok { + if errors.Is(retryError, errRateLimit) { return false, retryError } return false, nil @@ -384,7 +385,7 @@ func (g *gitHubRepository) getReleaseByTag(ctx context.Context, tag string) (*gi return false, retryError } // Return immediately if we are rate limited. - if _, ok := getReleasesErr.(*github.RateLimitError); ok { + if errors.Is(retryError, errRateLimit) { return false, retryError } return false, nil @@ -466,7 +467,7 @@ func (g *gitHubRepository) downloadFilesFromRelease(ctx context.Context, release if downloadReleaseError != nil { retryError = g.handleGithubErr(downloadReleaseError, "failed to download file %q from %q release", *release.TagName, fileName) // Return immediately if we are rate limited. - if _, ok := downloadReleaseError.(*github.RateLimitError); ok { + if errors.Is(retryError, errRateLimit) { return false, retryError } return false, nil @@ -500,12 +501,13 @@ func (g *gitHubRepository) downloadFilesFromRelease(ctx context.Context, release // handleGithubErr wraps error messages. func (g *gitHubRepository) handleGithubErr(err error, message string, args ...interface{}) error { if _, ok := err.(*github.RateLimitError); ok { - return errors.New("rate limit for github api has been reached. Please wait one hour or get a personal API token and assign it to the GITHUB_TOKEN environment variable") + return errRateLimit } - if ghErr, ok := err.(*github.ErrorResponse); ok { - if ghErr.Response.StatusCode == http.StatusNotFound { - return errNotFound - } + + var ghErr *github.ErrorResponse + if errors.As(err, &ghErr) && ghErr.Response.StatusCode == http.StatusNotFound { + return errNotFound } - return errors.Wrapf(err, message, args...) + + return fmt.Errorf("%s: %w", fmt.Sprintf(message, args...), err) } diff --git a/cmd/clusterctl/client/repository/repository_github_test.go b/cmd/clusterctl/client/repository/repository_github_test.go index 07eed06cc85e..ed689e9a8d4e 100644 --- a/cmd/clusterctl/client/repository/repository_github_test.go +++ b/cmd/clusterctl/client/repository/repository_github_test.go @@ -26,6 +26,7 @@ import ( "github.com/google/go-github/v53/github" . "github.com/onsi/gomega" + "github.com/pkg/errors" "k8s.io/utils/ptr" clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3" @@ -1108,3 +1109,54 @@ func Test_gitHubRepository_releaseNotFound(t *testing.T) { }) } } + +func Test_handleGithubErr(t *testing.T) { + tests := []struct { + name string + err error + message string + args []any + want error + }{ + { + name: "Return error", + err: errors.New("error"), + message: "message %s and %s", + args: []any{"arg1", "arg2"}, + want: fmt.Errorf("message arg1 and arg2: %w", errors.New("error")), + }, + { + name: "Return RateLimitError", + err: &github.RateLimitError{ + Response: &http.Response{ + StatusCode: http.StatusForbidden, + }, + }, + message: "", + args: nil, + want: errRateLimit, + }, + { + name: "Return ErrorResponse", + err: &github.ErrorResponse{ + Response: &http.Response{ + StatusCode: http.StatusNotFound, + }, + }, + message: "", + args: nil, + want: errNotFound, + }, + } + + gRepo := &gitHubRepository{} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + got := gRepo.handleGithubErr(tt.err, tt.message, tt.args...) + g.Expect(got.Error()).To(Equal(tt.want.Error())) + }) + } +} diff --git a/controlplane/kubeadm/internal/controllers/status.go b/controlplane/kubeadm/internal/controllers/status.go index 878d77ca9c3d..a1cbd8dafa9b 100644 --- a/controlplane/kubeadm/internal/controllers/status.go +++ b/controlplane/kubeadm/internal/controllers/status.go @@ -23,7 +23,6 @@ import ( "strings" "time" - "github.com/pkg/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/utils/ptr" @@ -100,7 +99,7 @@ func (r *KubeadmControlPlaneReconciler) updateStatus(ctx context.Context, contro workloadCluster, err := controlPlane.GetWorkloadCluster(ctx) if err != nil { - return errors.Wrap(err, "failed to create remote cluster client") + return fmt.Errorf("failed to create remote cluster client: %w", err) } status, err := workloadCluster.ClusterStatus(ctx) if err != nil { diff --git a/exp/internal/controllers/machinepool_controller_noderef.go b/exp/internal/controllers/machinepool_controller_noderef.go index 364206e10625..32909381330f 100644 --- a/exp/internal/controllers/machinepool_controller_noderef.go +++ b/exp/internal/controllers/machinepool_controller_noderef.go @@ -97,7 +97,7 @@ func (r *MachinePoolReconciler) reconcileNodeRefs(ctx context.Context, s *scope) // Get the Node references. nodeRefsResult, err := r.getNodeReferences(ctx, mp.Spec.ProviderIDList, mp.Spec.MinReadySeconds, s.nodeRefMap) if err != nil { - if err == errNoAvailableNodes { + if errors.Is(err, errNoAvailableNodes) { log.Info("Cannot assign NodeRefs to MachinePool, no matching Nodes") // No need to requeue here. Nodes emit an event that triggers reconciliation. return ctrl.Result{}, nil diff --git a/exp/internal/controllers/machinepool_controller_phases.go b/exp/internal/controllers/machinepool_controller_phases.go index 26279c7ce8a1..2f76d6dd5080 100644 --- a/exp/internal/controllers/machinepool_controller_phases.go +++ b/exp/internal/controllers/machinepool_controller_phases.go @@ -346,7 +346,7 @@ func (r *MachinePoolReconciler) reconcileInfrastructure(ctx context.Context, s * // Get and set Status.Replicas from the infrastructure provider. err = util.UnstructuredUnmarshalField(infraConfig, &mp.Status.Replicas, "status", "replicas") if err != nil { - if err != util.ErrUnstructuredFieldNotFound { + if !errors.Is(err, util.ErrUnstructuredFieldNotFound) { return ctrl.Result{}, errors.Wrapf(err, "failed to retrieve replicas from infrastructure provider for MachinePool %q in namespace %q", mp.Name, mp.Namespace) } } diff --git a/internal/controllers/cluster/cluster_controller_phases.go b/internal/controllers/cluster/cluster_controller_phases.go index e5475e9a19eb..6fcd5ccb934e 100644 --- a/internal/controllers/cluster/cluster_controller_phases.go +++ b/internal/controllers/cluster/cluster_controller_phases.go @@ -415,7 +415,7 @@ func (r *Reconciler) reconcileKubeconfig(ctx context.Context, s *scope) (ctrl.Re switch { case apierrors.IsNotFound(err): if err := kubeconfig.CreateSecret(ctx, r.Client, cluster); err != nil { - if err == kubeconfig.ErrDependentCertificateNotFound { + if errors.Is(err, kubeconfig.ErrDependentCertificateNotFound) { log.Info("Could not find secret for cluster, requeuing", "Secret", secret.ClusterCA) return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } diff --git a/internal/controllers/clusterresourceset/clusterresourceset_controller.go b/internal/controllers/clusterresourceset/clusterresourceset_controller.go index 8cfa04fab9ad..1e4b385e9560 100644 --- a/internal/controllers/clusterresourceset/clusterresourceset_controller.go +++ b/internal/controllers/clusterresourceset/clusterresourceset_controller.go @@ -313,7 +313,7 @@ func (r *Reconciler) ApplyClusterResourceSet(ctx context.Context, cluster *clust for i, resource := range clusterResourceSet.Spec.Resources { unstructuredObj, err := r.getResource(ctx, resource, cluster.GetNamespace()) if err != nil { - if err == ErrSecretTypeNotSupported { + if errors.Is(err, ErrSecretTypeNotSupported) { v1beta1conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedV1Beta1Condition, addonsv1.WrongSecretTypeV1Beta1Reason, clusterv1.ConditionSeverityWarning, err.Error()) conditions.Set(clusterResourceSet, metav1.Condition{ Type: addonsv1.ClusterResourceSetResourcesAppliedCondition, diff --git a/internal/controllers/machine/drain/drain.go b/internal/controllers/machine/drain/drain.go index 1236eceacc12..b8cfd340cd3d 100644 --- a/internal/controllers/machine/drain/drain.go +++ b/internal/controllers/machine/drain/drain.go @@ -375,7 +375,7 @@ evictionLoop: // Ensure the causes are also included in the error message. // Before: "Cannot evict pod as it would violate the pod's disruption budget." // After: "Cannot evict pod as it would violate the pod's disruption budget. The disruption budget nginx needs 20 healthy pods and has 20 currently" - if ok := errors.As(err, &statusError); ok { + if errors.As(err, &statusError) { errorMessage := statusError.Status().Message if statusError.Status().Details != nil { var causes []string diff --git a/internal/controllers/machine/machine_controller_noderef.go b/internal/controllers/machine/machine_controller_noderef.go index 0835d41252f3..d5610abc51f1 100644 --- a/internal/controllers/machine/machine_controller_noderef.go +++ b/internal/controllers/machine/machine_controller_noderef.go @@ -83,12 +83,11 @@ func (r *Reconciler) reconcileNode(ctx context.Context, s *scope) (ctrl.Result, // Even if Status.NodeRef exists, continue to do the following checks to make sure Node is healthy node, err := r.getNode(ctx, remoteClient, *machine.Spec.ProviderID) if err != nil { - if err == ErrNodeNotFound { + if errors.Is(err, ErrNodeNotFound) { if !s.machine.DeletionTimestamp.IsZero() { // Tolerate node not found when the machine is being deleted. return ctrl.Result{}, nil } - // While a NodeRef is set in the status, failing to get that node means the node is deleted. // If Status.NodeRef is not set before, node still can be in the provisioning state. if machine.Status.NodeRef != nil {