Return the http status code information in the error text for the "Managers" (#258)

The various manager APIs all had their own error handling code, which would result in them occasionally obscuring the actual HTTP response error. 

We're now respecting the HTTP status codes and anything >= 400 causes us to fail with an actual error with text, rather than an empty error.

Fixes #229

Author: richardpark-msft

queue_manager.go

@@ -5,7 +5,6 @@ import (
 	"encoding/xml"
 	"errors"
 	"io/ioutil"
-	"net/http"
 	"strings"
 	"time"
 
@@ -262,7 +261,7 @@ func (qm *QueueManager) Delete(ctx context.Context, name string) error {
 	res, err := qm.entityManager.Delete(ctx, "/"+name)
 	defer closeRes(ctx, res)
 
-	return err
+	return checkForError(ctx, err, res)
 }
 
 // Put creates or updates a Service Bus Queue
@@ -309,8 +308,7 @@ func (qm *QueueManager) Put(ctx context.Context, name string, opts ...QueueManag
 	res, err := qm.entityManager.Put(ctx, "/"+name, reqBytes, mw...)
 	defer closeRes(ctx, res)
 
-	if err != nil {
-		tab.For(ctx).Error(err)
+	if err := checkForError(ctx, err, res); err != nil {
 		return nil, err
 	}
 
@@ -346,8 +344,7 @@ func (qm *QueueManager) List(ctx context.Context, options ...ListQueuesOption) (
 	res, err := qm.entityManager.Get(ctx, basePath)
 	defer closeRes(ctx, res)
 
-	if err != nil {
-		tab.For(ctx).Error(err)
+	if err := checkForError(ctx, err, res); err != nil {
 		return nil, err
 	}
 
@@ -378,15 +375,10 @@ func (qm *QueueManager) Get(ctx context.Context, name string) (*QueueEntity, err
 	res, err := qm.entityManager.Get(ctx, name)
 	defer closeRes(ctx, res)
 
-	if err != nil {
-		tab.For(ctx).Error(err)
+	if err := checkForError(ctx, err, res); err != nil {
 		return nil, err
 	}
 
-	if res.StatusCode == http.StatusNotFound {
-		return nil, ErrNotFound{EntityPath: res.Request.URL.Path}
-	}
-
 	b, err := ioutil.ReadAll(res.Body)
 	if err != nil {
 		tab.For(ctx).Error(err)

subscription_manager.go

@@ -213,7 +213,7 @@ func (sm *SubscriptionManager) Delete(ctx context.Context, name string) error {
 	res, err := sm.entityManager.Delete(ctx, sm.getResourceURI(name))
 	defer closeRes(ctx, res)
 
-	return err
+	return checkForError(ctx, err, res)
 }
 
 // Put creates or updates a Service Bus Topic
@@ -260,7 +260,7 @@ func (sm *SubscriptionManager) Put(ctx context.Context, name string, opts ...Sub
 	res, err := sm.entityManager.Put(ctx, sm.getResourceURI(name), reqBytes, mw...)
 	defer closeRes(ctx, res)
 
-	if err != nil {
+	if err := checkForError(ctx, err, res); err != nil {
 		return nil, err
 	}
 
@@ -295,7 +295,7 @@ func (sm *SubscriptionManager) List(ctx context.Context, options ...ListSubscrip
 	res, err := sm.entityManager.Get(ctx, basePath)
 	defer closeRes(ctx, res)
 
-	if err != nil {
+	if err := checkForError(ctx, err, res); err != nil {
 		return nil, err
 	}
 
@@ -325,14 +325,10 @@ func (sm *SubscriptionManager) Get(ctx context.Context, name string) (*Subscript
 	res, err := sm.entityManager.Get(ctx, sm.getResourceURI(name))
 	defer closeRes(ctx, res)
 
-	if err != nil {
+	if err := checkForError(ctx, err, res); err != nil {
 		return nil, err
 	}
 
-	if res.StatusCode == http.StatusNotFound {
-		return nil, ErrNotFound{EntityPath: res.Request.URL.Path}
-	}
-
 	b, err := ioutil.ReadAll(res.Body)
 	if err != nil {
 		return nil, err
@@ -361,14 +357,10 @@ func (sm *SubscriptionManager) ListRules(ctx context.Context, subscriptionName s
 	res, err := sm.entityManager.Get(ctx, sm.getRulesResourceURI(subscriptionName))
 	defer closeRes(ctx, res)
 
-	if err != nil {
+	if err := checkForError(ctx, err, res); err != nil {
 		return nil, err
 	}
 
-	if res.StatusCode == http.StatusNotFound {
-		return nil, ErrNotFound{EntityPath: res.Request.URL.Path}
-	}
-
 	b, err := ioutil.ReadAll(res.Body)
 	if err != nil {
 		return nil, err
@@ -473,7 +465,7 @@ func (sm *SubscriptionManager) DeleteRule(ctx context.Context, subscriptionName,
 	res, err := sm.entityManager.Delete(ctx, sm.getRuleResourceURI(subscriptionName, ruleName))
 	defer closeRes(ctx, res)
 
-	return err
+	return checkForError(ctx, err, res)
 }
 
 func ruleEntryToEntity(entry *ruleEntry) *RuleEntity {

subscription_test.go

@@ -26,13 +26,15 @@ import (
 	"context"
 	"encoding/xml"
 	"fmt"
+	"os"
 	"strings"
 	"sync"
 	"testing"
 	"time"
 
 	"github.com/Azure/azure-amqp-common-go/v3/uuid"
 	"github.com/Azure/azure-sdk-for-go/services/servicebus/mgmt/2015-08-01/servicebus"
+	"github.com/joho/godotenv"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
@@ -458,7 +460,12 @@ func (suite *serviceBusSuite) testSubscriptionManager(tests map[string]func(cont
 					defer func(sName string) {
 						ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout)
 						defer cancel()
-						if !suite.NoError(sm.Delete(ctx, sName)) {
+
+						err = sm.Delete(ctx, sName)
+
+						if !IsErrNotFound(err) && !suite.NoError(err) {
+							// not all tests actually create a subscription (some of these tests are
+							// basically unittests)
 							suite.Fail(err.Error())
 						}
 					}(subName)
@@ -785,3 +792,130 @@ func checkZeroSubscriptionMessages(ctx context.Context, t *testing.T, topic *Top
 
 	assert.Fail(t, "message count never reached zero")
 }
+
+func TestErrorMessagesWithMissingPrivileges(t *testing.T) {
+	_ = godotenv.Load()
+
+	// we're obscuring the HTTP errors coming back from the service and
+	// we shouldn't. Just testing some of the common scenarios with a
+	// connection string that lacks Manage privileges.
+	lowPrivCS := os.Getenv("SERVICEBUS_CONNECTION_STRING_NO_MANAGE")
+	normalCS := os.Getenv("SERVICEBUS_CONNECTION_STRING")
+
+	if lowPrivCS == "" || normalCS == "" {
+		t.Skip("Need both SERVICEBUS_CONNECTION_STRING_NO_MANAGE and SERVICEBUS_CONNECTION_STRING")
+	}
+
+	nanoSeconds := time.Now().UnixNano()
+
+	topicName := fmt.Sprintf("topic-%d", nanoSeconds)
+	queueName := fmt.Sprintf("queue-%d", nanoSeconds)
+	subName := "subscription1"
+	ruleName := "rule"
+
+	// create some entities that we need (there's a diff between something not being
+	// found and something failing because of lack of authorization)
+	cleanup := func() func() {
+		ns, err := NewNamespace(NamespaceWithConnectionString(normalCS))
+		qm := ns.NewQueueManager()
+
+		_, err = qm.Put(context.Background(), queueName)
+		require.NoError(t, err)
+
+		tm := ns.NewTopicManager()
+		_, err = tm.Put(context.Background(), topicName)
+		require.NoError(t, err)
+
+		sm, err := ns.NewSubscriptionManager(topicName)
+		require.NoError(t, err)
+
+		_, err = sm.Put(context.Background(), subName)
+		require.NoError(t, err)
+
+		_, err = sm.PutRule(context.Background(), subName, ruleName, TrueFilter{})
+		require.NoError(t, err)
+
+		return func() {
+			require.NoError(t, tm.Delete(context.Background(), topicName)) // should delete the subscription
+			require.NoError(t, qm.Delete(context.Background(), queueName))
+		}
+	}()
+	defer cleanup()
+
+	ns, err := NewNamespace(NamespaceWithConnectionString(lowPrivCS))
+	require.NoError(t, err)
+
+	ctx := context.Background()
+	wg := sync.WaitGroup{}
+	wg.Add(3)
+
+	go func() {
+		defer wg.Done()
+
+		qm := ns.NewQueueManager()
+
+		_, err = qm.Get(ctx, "not-found-queue")
+		require.True(t, IsErrNotFound(err))
+
+		_, err = qm.Get(ctx, queueName)
+		require.EqualError(t, err, "request failed: 401 Unauthorized")
+
+		_, err = qm.List(ctx)
+		require.EqualError(t, err, "request failed: 401 Unauthorized")
+
+		_, err = qm.Put(ctx, "canneverbecreated")
+		require.EqualError(t, err, "request failed: 401 Unauthorized")
+
+		err = qm.Delete(ctx, queueName)
+		require.EqualError(t, err, "request failed: 401 Unauthorized")
+	}()
+
+	go func() {
+		defer wg.Done()
+
+		tm := ns.NewTopicManager()
+
+		_, err = tm.Get(ctx, "not-found-topic")
+		require.True(t, IsErrNotFound(err))
+
+		_, err = tm.Get(ctx, topicName)
+		require.EqualError(t, err, "request failed: 401 Unauthorized")
+
+		_, err = tm.Put(ctx, "canneverbecreated")
+		require.Contains(t, err.Error(), "error code: 401, Details: Authorization failed for specified action")
+
+		_, err = tm.List(ctx)
+		require.Contains(t, err.Error(), "error code: 401, Details: Manage,EntityRead claims required for this operation")
+
+		err = tm.Delete(ctx, topicName)
+		require.Contains(t, err.Error(), "request failed: 401 Unauthorized")
+	}()
+
+	go func() {
+		defer wg.Done()
+
+		sm, err := ns.NewSubscriptionManager(topicName)
+		require.NoError(t, err)
+
+		_, err = sm.Get(ctx, "not-found-subscription")
+		require.Contains(t, err.Error(), "request failed: 401 SubCode=40100: Unauthorized")
+
+		_, err = sm.Get(ctx, subName)
+		require.Contains(t, err.Error(), "request failed: 401 SubCode=40100: Unauthorized")
+
+		_, err = sm.Put(ctx, subName)
+		require.Contains(t, err.Error(), "request failed: 401 SubCode=40100: Unauthorized")
+
+		err = sm.Delete(ctx, subName)
+		require.Contains(t, err.Error(), "request failed: 401 SubCode=40100: Unauthorized")
+
+		_, err = sm.List(ctx)
+		require.Contains(t, err.Error(), "request failed: 401 SubCode=40100: Unauthorized")
+
+		_, err = sm.ListRules(ctx, subName)
+		require.Contains(t, err.Error(), "request failed: 401 SubCode=40100: Unauthorized")
+
+	}()
+
+	wg.Wait()
+}

topic_manager.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/xml"
 	"errors"
+	"fmt"
 	"io/ioutil"
 	"net/http"
 	"time"
@@ -93,7 +94,7 @@ func (tm *TopicManager) Delete(ctx context.Context, name string) error {
 	res, err := tm.entityManager.Delete(ctx, "/"+name)
 	defer closeRes(ctx, res)
 
-	return err
+	return checkForError(ctx, err, res)
 }
 
 // Put creates or updates a Service Bus Topic
@@ -200,15 +201,10 @@ func (tm *TopicManager) Get(ctx context.Context, name string) (*TopicEntity, err
 	res, err := tm.entityManager.Get(ctx, name)
 	defer closeRes(ctx, res)
 
-	if err != nil {
-		tab.For(ctx).Error(err)
+	if err := checkForError(ctx, err, res); err != nil {
 		return nil, err
 	}
 
-	if res.StatusCode == http.StatusNotFound {
-		return nil, ErrNotFound{EntityPath: res.Request.URL.Path}
-	}
-
 	b, err := ioutil.ReadAll(res.Body)
 	if err != nil {
 		tab.For(ctx).Error(err)
@@ -322,3 +318,25 @@ func TopicWithMessageTimeToLive(window *time.Duration) TopicManagementOption {
 		return nil
 	}
 }
+
+func checkForError(ctxForLogging context.Context, err error, res *http.Response) error {
+	if err != nil {
+		tab.For(ctxForLogging).Error(err)
+		return err
+	}
+
+	// check the response as well
+	if res.StatusCode == http.StatusNotFound {
+		err := ErrNotFound{EntityPath: res.Request.URL.Path}
+		tab.For(ctxForLogging).Error(err)
+		return err
+	}
+
+	if res.StatusCode >= 400 {
+		err := fmt.Errorf("request failed: %s", res.Status)
+		tab.For(ctxForLogging).Error(err)
+		return err
+	}
+
+	return nil
+}