fix(akeyless): stop reporting API failures as a missing item (#6753)

DescribeItem unmarshalled the API error body into a local struct and
assigned the result back to err. Every field of that struct was optional,
so any JSON error body unmarshalled cleanly, err became nil, and the
zero-value item was returned. GetSecretByType then saw an item with no
name and returned ErrItemNotExists, so 401, 403 and 500 all reached the
caller as "item does not exist".

That is not only a misleading message. ErrItemNotExists drives control
flow: SecretExists reports the secret as absent, and PushSecret takes it
as a signal to create rather than update. An authorization failure was
therefore being turned into a create attempt.

Key the decision on the HTTP status instead. Measured against
api.akeyless.io: a caller permitted to know an item is missing gets 404
NotFound, while a caller without permission gets 401 UnauthorizedAccess
whether or not the item exists. So 404 keeps ErrItemNotExists and leaves
SecretExists and PushSecret behaving exactly as before, and every other
status now surfaces the body Akeyless returned, which already names the
reason and links to the account audit log. Nothing matches on message
text.

The local Item struct existed only for that unmarshal and is removed.
It was exported, so this is technically a breaking change, but the
provider packages are not supported for consumption outside
external-secrets.

The package-level apiErr this code reads from is shared mutable state
across concurrent reconciles; that is tracked separately in #6747 and
left alone here.

Fixes: external-secrets/external-secrets#5394

Signed-off-by: Alexander Chernov <alexander@chernov.it>
Co-authored-by: Jean-Philippe Evrard <jean-philippe.evrard+rochepub@external.roche.com>

providers/v1/akeyless/akeyless.go

@@ -84,13 +84,6 @@ type Akeyless struct {
 	url    string
 }
 
-// Item represents an item in the Akeyless Vault.
-type Item struct {
-	ItemName    string `json:"item_name"`
-	ItemType    string `json:"item_type"`
-	LastVersion int32  `json:"last_version"`
-}
-
 type akeylessVaultInterface interface {
 	GetSecretByType(ctx context.Context, secretName string, version int32) (string, error)
 	TokenFromSecretRef(ctx context.Context) (string, error)

providers/v1/akeyless/akeyless_api.go

@@ -23,6 +23,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net/http"
 	"os"
 	"strings"
 
@@ -152,11 +153,10 @@ func (a *akeylessBase) DescribeItem(ctx context.Context, itemName string) (*akey
 	metrics.ObserveAPICall(constants.ProviderAKEYLESSSM, constants.CallAKEYLESSSMDescribeItem, err)
 	var apiErr akeyless.GenericOpenAPIError
 	if errors.As(err, &apiErr) {
-		var item *Item
-		err = json.Unmarshal(apiErr.Body(), &item)
-		if err != nil {
-			return nil, fmt.Errorf("can't describe item: %v, error: %v", itemName, string(apiErr.Body()))
+		if res.StatusCode == http.StatusNotFound {
+			return nil, ErrItemNotExists
 		}
+		return nil, fmt.Errorf("can't describe item: %v, error: %v", itemName, string(apiErr.Body()))
 	}
 	if err != nil {
 		return nil, fmt.Errorf("can't describe item: %w", err)

providers/v1/akeyless/akeyless_test.go

@@ -20,6 +20,8 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/http"
+	"net/http/httptest"
 	"strings"
 	"testing"
 
@@ -596,3 +598,110 @@ func TestCapabilities(t *testing.T) {
 	p := &Provider{}
 	require.Equal(t, esv1.SecretStoreReadWrite, p.Capabilities())
 }
+
+// newDescribeItemServer serves a single canned response for /describe-item and
+// returns an akeylessBase wired to it.
+func newDescribeItemServer(t *testing.T, status int, body string) *akeylessBase {
+	t.Helper()
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(status)
+		_, _ = w.Write([]byte(body))
+	}))
+	t.Cleanup(srv.Close)
+	return &akeylessBase{
+		RestAPI: akeyless.NewAPIClient(&akeyless.Configuration{
+			Servers: []akeyless.ServerConfiguration{{URL: srv.URL}},
+		}).V2Api,
+	}
+}
+
+func describeItemCtx() context.Context {
+	return context.WithValue(context.Background(), aKeylessToken, "t-test-token")
+}
+
+// TestDescribeItemMapsAPIStatus pins which Akeyless responses mean "absent".
+// Akeyless answers 404 only for a caller allowed to know an item is missing,
+// and 401 for one that is not, so the HTTP status is the discriminator. The
+// wording of the error body is not part of the contract and is not matched on.
+func TestDescribeItemMapsAPIStatus(t *testing.T) {
+	const notFoundBody = `{"error":"failed to obtain item description: Desc: Failed to get item. ` +
+		`Status 404 Not Found, Error: NotFound. Message: account id: acc-x, access id: p-y. ` +
+		`failed to obtain item /some/item"}`
+	const deniedBody = `{"error":"failed to obtain item description: Desc: Failed to get item. ` +
+		`Status 401 Unauthorized, Error: UnauthorizedAccess. Message: account id: acc-x, ` +
+		`access id: p-y. unauthorized access for access id p-y"}`
+
+	tests := []struct {
+		name        string
+		status      int
+		body        string
+		notExists   bool
+		wantMessage string
+	}{
+		{
+			name:      "404 means the item is absent",
+			status:    http.StatusNotFound,
+			body:      notFoundBody,
+			notExists: true,
+		},
+		{
+			name:        "401 is an authorization failure, not an absent item",
+			status:      http.StatusUnauthorized,
+			body:        deniedBody,
+			wantMessage: "UnauthorizedAccess",
+		},
+		{
+			name:        "5xx is a server failure, not an absent item",
+			status:      http.StatusInternalServerError,
+			body:        `{"error":"internal server error"}`,
+			wantMessage: "internal server error",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			a := newDescribeItemServer(t, tt.status, tt.body)
+
+			item, err := a.DescribeItem(describeItemCtx(), "/some/item")
+
+			require.Error(t, err)
+			require.Nil(t, item)
+			if tt.notExists {
+				require.ErrorIs(t, err, ErrItemNotExists)
+				return
+			}
+			// The caller must not mistake this for an absent item, and the
+			// operator needs the reason Akeyless gave.
+			require.NotErrorIs(t, err, ErrItemNotExists)
+			require.Contains(t, err.Error(), tt.wantMessage)
+		})
+	}
+}
+
+// TestGetSecretByTypeSurfacesAuthFailure covers the path from the bug report:
+// a denied describe used to reach the caller as ErrItemNotExists, which made
+// SecretExists report absence and PushSecret attempt a create.
+func TestGetSecretByTypeSurfacesAuthFailure(t *testing.T) {
+	a := newDescribeItemServer(t, http.StatusUnauthorized,
+		`{"error":"Status 401 Unauthorized, Error: UnauthorizedAccess. Message: sub claim mismatch"}`)
+
+	_, err := a.GetSecretByType(describeItemCtx(), "/some/item", 0)
+
+	require.Error(t, err)
+	require.NotErrorIs(t, err, ErrItemNotExists)
+	require.Contains(t, err.Error(), "UnauthorizedAccess")
+}
+
+// TestDescribeItemSuccess guards the happy path, since the fix reorders the
+// error branch that precedes it.
+func TestDescribeItemSuccess(t *testing.T) {
+	a := newDescribeItemServer(t, http.StatusOK,
+		`{"item_name":"/some/item","item_type":"STATIC_SECRET","last_version":3}`)
+
+	item, err := a.DescribeItem(describeItemCtx(), "/some/item")
+
+	require.NoError(t, err)
+	require.Equal(t, "/some/item", item.GetItemName())
+	require.Equal(t, "STATIC_SECRET", item.GetItemType())
+}