Repository: kubermatic/machine-controller
Pull request #1537 (2023-01-23 11:02:05)

Anexia Provider: Return TerminalError on 401 and 403 engine responses
**What this PR does / why we need it**:
Return `TerminalError` on 401 and 403 engine responses to signal invalid token configuration.

**What type of PR is this?**
/kind cleanup
<!--
Add one of the following kinds:
/kind bug
/kind cleanup
/kind documentation
/kind feature
/kind design

Optionally add one or more of the following kinds if applicable:
/kind api-change
/kind deprecation
/kind failing-test
/kind flake
/kind regression
/kind chore
-->

**Special notes for your reviewer**:
Please cherry-pick to v1.45 and v1.55.

**Does this PR introduce a user-facing change? Then add your Release Note here**:
<!--
Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
NONE
```

**Documentation**:
<!--
Please do one of the following options:
- Add a link to the existing documentation
- Add a link to the kubermatic/docs pull request
- If no documentation change is applicable then add:
  - TBD (documentation will be added later)
  - NONE (no documentation needed for this PR)
-->
```documentation
NONE
```


kubermatic-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubermatic/machine-controller/pull/1537#" title="Author self-approved">marioreggiori</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign moadqassem for approval by writing `/assign @moadqassem` in a comment. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubermatic%2Fmachine-controller).

<details open>
Needs approval from an approver in each of these files:

- **[pkg/cloudprovider/provider/OWNERS](https://github.com/kubermatic/machine-controller/blob/main/pkg/cloudprovider/provider/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["moadqassem"]} -->
kubermatic-bot: Hi @marioreggiori. Thanks for your PR.

I'm waiting for a [kubermatic](https://github.com/orgs/kubermatic/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://github.com/orgs/kubermatic/people) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=kubermatic%2Fmachine-controller).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

ahmedwaleedmalik: /ok-to-test
ahmedwaleedmalik: /cherry-pick release/v1.45
kubermatic-bot: @ahmedwaleedmalik: once the present PR merges, I will cherry-pick it on top of release/v1.45 in a new PR and assign it to you.

<details>

In response to [this](https://github.com/kubermatic/machine-controller/pull/1537#issuecomment-1400738344):

>/cherry-pick release/v1.45


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
ahmedwaleedmalik: /cherry-pick release/v1.55
kubermatic-bot: @ahmedwaleedmalik: once the present PR merges, I will cherry-pick it on top of release/v1.55 in a new PR and assign it to you.

<details>

In response to [this](https://github.com/kubermatic/machine-controller/pull/1537#issuecomment-1400738599):

>/cherry-pick release/v1.55


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
kubermatic-bot: LGTM label has been added.  <details>Git tree hash: bd6c1180bbfa1434ec07ad0ec0e4c22ebe0f8f3e</details>
kubermatic-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubermatic/machine-controller/pull/1537#pullrequestreview-1266060449" title="Approved">ahmedwaleedmalik</a>*, *<a href="https://github.com/kubermatic/machine-controller/pull/1537#" title="Author self-approved">marioreggiori</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubermatic%2Fmachine-controller).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/cloudprovider/provider/OWNERS](https://github.com/kubermatic/machine-controller/blob/main/pkg/cloudprovider/provider/OWNERS)~~ [ahmedwaleedmalik]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
kubermatic-bot: @marioreggiori: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-machine-controller-lint | ea41c2c46bfa045f90908e510648edf0f71d43be | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/1537/pull-machine-controller-lint/1617579400310558720) | true | `/test pull-machine-controller-lint`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=1537)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
ahmedwaleedmalik: /lgtm cancel

Lint errors
kubermatic-bot: LGTM label has been added.  <details>Git tree hash: cbde07487c9628f91c4bc60dd4b552ed2a6df2b1</details>
kubermatic-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubermatic/machine-controller/pull/1537#pullrequestreview-1266886415" title="Approved">ahmedwaleedmalik</a>*, *<a href="https://github.com/kubermatic/machine-controller/pull/1537#" title="Author self-approved">marioreggiori</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubermatic%2Fmachine-controller).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/cloudprovider/provider/OWNERS](https://github.com/kubermatic/machine-controller/blob/main/pkg/cloudprovider/provider/OWNERS)~~ [ahmedwaleedmalik]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/cloudprovider/provider/anexia/provider.go b/pkg/cloudprovider/provider/anexia/provider.go
index b8976330..1d7a8180 100644
--- a/pkg/cloudprovider/provider/anexia/provider.go
+++ b/pkg/cloudprovider/provider/anexia/provider.go
@@ -27,6 +27,8 @@ import (
 	"sync"
 	"time"
 
+	"go.anx.io/go-anxcloud/pkg/api"
+	"go.anx.io/go-anxcloud/pkg/client"
 	anxclient "go.anx.io/go-anxcloud/pkg/client"
 	anxaddr "go.anx.io/go-anxcloud/pkg/ipam/address"
 	"go.anx.io/go-anxcloud/pkg/vsphere"
@@ -119,7 +121,7 @@ func (p *provider) Create(ctx context.Context, machine *clusterv1alpha1.Machine,
 	// provision machine
 	err = provisionVM(ctx, client)
 	if err != nil {
-		return nil, err
+		return nil, anexiaErrorToTerminalError(err, "failed waiting for vm provisioning")
 	}
 	return p.Get(ctx, machine, data)
 }
@@ -431,7 +433,7 @@ func (p *provider) Get(ctx context.Context, machine *clusterv1alpha1.Machine, pd
 	if status.InstanceID == "" {
 		progress, err := vsphereAPI.Provisioning().Progress().Get(ctx, status.ProvisioningID)
 		if err != nil {
-			return nil, fmt.Errorf("failed to get provisioning progress: %w", err)
+			return nil, anexiaErrorToTerminalError(err, "failed to get provisioning progress")
 		}
 		if len(progress.Errors) > 0 {
 			return nil, fmt.Errorf("vm provisioning had errors: %s", strings.Join(progress.Errors, ","))
@@ -458,7 +460,7 @@ func (p *provider) Get(ctx context.Context, machine *clusterv1alpha1.Machine, pd
 
 	info, err := vsphereAPI.Info().Get(timeoutCtx, status.InstanceID)
 	if err != nil {
-		return nil, fmt.Errorf("failed get machine info: %w", err)
+		return nil, anexiaErrorToTerminalError(err, "failed getting machine info")
 	}
 	instance.info = &info
 
@@ -587,3 +589,23 @@ func updateMachineStatus(machine *clusterv1alpha1.Machine, status anxtypes.Provi
 
 	return nil
 }
+
+func anexiaErrorToTerminalError(err error, msg string) error {
+	var httpError api.HTTPError
+	if errors.As(err, &httpError) && (httpError.StatusCode() == http.StatusForbidden || httpError.StatusCode() == http.StatusUnauthorized) {
+		return cloudprovidererrors.TerminalError{
+			Reason:  common.InvalidConfigurationMachineError,
+			Message: "Request was rejected due to invalid credentials",
+		}
+	}
+
+	var responseError *client.ResponseError
+	if errors.As(err, &responseError) && (responseError.ErrorData.Code == http.StatusForbidden || responseError.ErrorData.Code == http.StatusUnauthorized) {
+		return cloudprovidererrors.TerminalError{
+			Reason:  common.InvalidConfigurationMachineError,
+			Message: "Request was rejected due to invalid credentials",
+		}
+	}
+
+	return fmt.Errorf("%s: %w", msg, err)
+}


Test patch:
diff --git a/pkg/cloudprovider/provider/anexia/provider_test.go b/pkg/cloudprovider/provider/anexia/provider_test.go
index 08cec325..5daa367c 100644
--- a/pkg/cloudprovider/provider/anexia/provider_test.go
+++ b/pkg/cloudprovider/provider/anexia/provider_test.go
@@ -21,17 +21,24 @@ import (
 	"encoding/json"
 	"errors"
 	"net/http"
+	"net/http/httptest"
+	"net/url"
 	"testing"
 	"time"
 
 	"github.com/gophercloud/gophercloud/testhelper"
+	"go.anx.io/go-anxcloud/pkg/api"
+	corev1 "go.anx.io/go-anxcloud/pkg/apis/core/v1"
+	"go.anx.io/go-anxcloud/pkg/client"
 	anxclient "go.anx.io/go-anxcloud/pkg/client"
+	"go.anx.io/go-anxcloud/pkg/core"
 	"go.anx.io/go-anxcloud/pkg/ipam/address"
 	"go.anx.io/go-anxcloud/pkg/vsphere/provisioning/progress"
 	"go.anx.io/go-anxcloud/pkg/vsphere/provisioning/vm"
 
 	"github.com/kubermatic/machine-controller/pkg/apis/cluster/v1alpha1"
 	clusterv1alpha1 "github.com/kubermatic/machine-controller/pkg/apis/cluster/v1alpha1"
+	cloudprovidererrors "github.com/kubermatic/machine-controller/pkg/cloudprovider/errors"
 	anxtypes "github.com/kubermatic/machine-controller/pkg/cloudprovider/provider/anexia/types"
 	cloudprovidertypes "github.com/kubermatic/machine-controller/pkg/cloudprovider/types"
 
@@ -346,3 +353,88 @@ func TestUpdateStatus(t *testing.T) {
 	testhelper.AssertEquals(t, true, called)
 	testhelper.AssertNoErr(t, err)
 }
+
+func Test_anexiaErrorToTerminalError(t *testing.T) {
+	forbiddenMockHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusForbidden)
+		_, err := w.Write([]byte(`{"error": {"code": 403}}`))
+		testhelper.AssertNoErr(t, err)
+	})
+
+	unauthorizedMockHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusUnauthorized)
+		_, err := w.Write([]byte(`{"error": {"code": 401}}`))
+		testhelper.AssertNoErr(t, err)
+	})
+
+	legacyClientRun := func(url string) error {
+		client, err := client.New(client.BaseURL(url), client.IgnoreMissingToken(), client.ParseEngineErrors(true))
+		testhelper.AssertNoErr(t, err)
+		_, err = core.NewAPI(client).Location().List(context.TODO(), 1, 1, "", "")
+		return err
+	}
+
+	apiClientRun := func(url string) error {
+		client, err := api.NewAPI(api.WithClientOptions(
+			client.BaseURL(url),
+			client.IgnoreMissingToken(),
+		))
+		testhelper.AssertNoErr(t, err)
+		return client.Get(context.TODO(), &corev1.Location{Identifier: "foo"})
+	}
+
+	testCases := []struct {
+		name        string
+		mockHandler http.HandlerFunc
+		run         func(url string) error
+	}{
+		{
+			name:        "api client returns forbidden",
+			mockHandler: forbiddenMockHandler,
+			run:         apiClientRun,
+		},
+		{
+			name:        "api client returns unauthorized",
+			mockHandler: unauthorizedMockHandler,
+			run:         apiClientRun,
+		},
+		{
+			name:        "legacy client returns forbidden",
+			mockHandler: forbiddenMockHandler,
+			run:         legacyClientRun,
+		},
+		{
+			name:        "legacy client returns unauthorized",
+			mockHandler: unauthorizedMockHandler,
+			run:         legacyClientRun,
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			srv := httptest.NewServer(testCase.mockHandler)
+			defer srv.Close()
+
+			err := anexiaErrorToTerminalError(testCase.run(srv.URL), "foo")
+			if ok, _, _ := cloudprovidererrors.IsTerminalError(err); !ok {
+				t.Errorf("unexpected error %#v, expected TerminalError", err)
+			}
+		})
+	}
+
+	t.Run("api client 404 HTTPError shouldn't convert to TerminalError", func(t *testing.T) {
+		err := api.NewHTTPError(http.StatusNotFound, "GET", &url.URL{}, errors.New("foo"))
+		err = anexiaErrorToTerminalError(err, "foo")
+		if ok, _, _ := cloudprovidererrors.IsTerminalError(err); ok {
+			t.Errorf("unexpected error %#v, expected no TerminalError", err)
+		}
+	})
+
+	t.Run("legacy api client unspecific ResponseError shouldn't convert to TerminalError", func(t *testing.T) {
+		var err error = &client.ResponseError{}
+		err = anexiaErrorToTerminalError(err, "foo")
+		if ok, _, _ := cloudprovidererrors.IsTerminalError(err); ok {
+			t.Errorf("unexpected error %#v, expected no TerminalError", err)
+		}
+	})
+}