Updated error message when download using MSI fails (#164)

* Updated error messaging for Linux clients.

* Updated manifest.xml version

* fixed formatting with gofmt

* Added comma to user readable string

* fixed error messages for test

misc/manifest.xml

@@ -2,7 +2,7 @@
 <ExtensionImage xmlns="http://schemas.microsoft.com/windowsazure">
   <ProviderNameSpace>Microsoft.Azure.Extensions</ProviderNameSpace>
   <Type>CustomScript</Type>
-  <Version>2.1.3</Version>
+  <Version>2.1.4</Version>
   <Label>Microsoft Azure Custom Script Extension for Linux Virtual Machines</Label>
   <HostingResources>VmRole</HostingResources>
   <MediaLink></MediaLink>

pkg/download/downloader.go

@@ -17,6 +17,11 @@ type Downloader interface {
 	GetRequest() (*http.Request, error)
 }
 
+const (
+	MsiDownload404ErrorString = "please ensure that the blob location in the fileUri setting exists, and the specified Managed Identity has read permissions to the storage blob"
+	MsiDownload403ErrorString = "please ensure that the specified Managed Identity has read permissions to the storage blob"
+)
+
 var (
 	// httpClient is the default client to be used in downloading files from
 	// Internet. http.Get() uses a client without timeouts (http.DefaultClient)
@@ -56,8 +61,11 @@ func Download(d Downloader) (int, io.ReadCloser, error) {
 	err = fmt.Errorf("unexpected status code: actual=%d expected=%d", resp.StatusCode, http.StatusOK)
 	switch d.(type) {
 	case *blobWithMsiToken:
-		if resp.StatusCode == http.StatusNotFound {
-			return resp.StatusCode, nil, errors.Wrapf(err, "please ensure that the blob location in the fileUri setting exists and the specified Managed Identity has read permissions to the storage blob")
+		switch resp.StatusCode {
+		case http.StatusNotFound:
+			return resp.StatusCode, nil, errors.Wrapf(err, MsiDownload404ErrorString)
+		case http.StatusForbidden:
+			return resp.StatusCode, nil, errors.Wrapf(err, MsiDownload403ErrorString)
 		}
 	}
 	return resp.StatusCode, nil, err

pkg/download/downloader_test.go

@@ -3,9 +3,11 @@ package download_test
 import (
 	"errors"
 	"fmt"
+	"github.com/Azure/azure-extension-foundation/msi"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 
 	"github.com/Azure/custom-script-extension-linux/pkg/download"
@@ -60,6 +62,28 @@ func TestDownload_statusOKSucceeds(t *testing.T) {
 	require.NotNil(t, body)
 }
 
+func TestDowload_msiDownloaderErrorMessage(t *testing.T) {
+	var mockMsiProvider download.MsiProvider = func() (msi.Msi, error) {
+		return msi.Msi{AccessToken: "fakeAccessToken"}, nil
+	}
+	srv := httptest.NewServer(httpbin.GetMux())
+	defer srv.Close()
+
+	msiDownloader404 := download.NewBlobWithMsiDownload(srv.URL+"/status/404", mockMsiProvider)
+
+	returnCode, body, err := download.Download(msiDownloader404)
+	require.True(t, strings.Contains(err.Error(), download.MsiDownload404ErrorString), "error string doesn't contain the correct message")
+	require.Nil(t, body, "body is not nil for failed download")
+	require.Equal(t, 404, returnCode, "return code was not 404")
+
+	msiDownloader403 := download.NewBlobWithMsiDownload(srv.URL+"/status/403", mockMsiProvider)
+	returnCode, body, err = download.Download(msiDownloader403)
+	require.True(t, strings.Contains(err.Error(), download.MsiDownload403ErrorString), "error string doesn't contain the correct message")
+	require.Nil(t, body, "body is not nil for failed download")
+	require.Equal(t, 403, returnCode, "return code was not 403")
+
+}
+
 func TestDownload_retrievesBody(t *testing.T) {
 	srv := httptest.NewServer(httpbin.GetMux())
 	defer srv.Close()

pkg/download/retry_test.go

@@ -1,8 +1,10 @@
 package download_test
 
 import (
+	"github.com/Azure/azure-extension-foundation/msi"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 	"time"
 
@@ -92,6 +94,30 @@ func TestRetriesWith_SwitchDownloaderOn404(t *testing.T) {
 	require.Equal(t, d200.timesCalled, 4)
 }
 
+func TestRetriesWith_SwitchDownloaderThenFailWithCorretErrorMessage(t *testing.T) {
+	svr := httptest.NewServer(httpbin.GetMux())
+	defer svr.Close()
+	var mockMsiProvider download.MsiProvider = func() (msi.Msi, error) {
+		return msi.Msi{AccessToken: "fakeAccessToken"}, nil
+	}
+
+	d404 := mockDownloader{0, svr.URL + "/status/404"}
+	msiDownloader403 := download.NewBlobWithMsiDownload(svr.URL+"/status/403", mockMsiProvider)
+	resp, err := download.WithRetries(nopLog(), []download.Downloader{&d404, msiDownloader403}, func(d time.Duration) { return })
+	require.NotNil(t, err, "download with retries should fail")
+	require.Nil(t, resp, "response body should be nil for failed download with retries")
+	require.Equal(t, d404.timesCalled, 1)
+	require.True(t, strings.Contains(err.Error(), download.MsiDownload403ErrorString), "error string doesn't contain the correct message")
+
+	d404 = mockDownloader{0, svr.URL + "/status/404"}
+	msiDownloader404 := download.NewBlobWithMsiDownload(svr.URL+"/status/404", mockMsiProvider)
+	resp, err = download.WithRetries(nopLog(), []download.Downloader{&d404, msiDownloader404}, func(d time.Duration) { return })
+	require.NotNil(t, err, "download with retries should fail")
+	require.Nil(t, resp, "response body should be nil for failed download with retries")
+	require.Equal(t, d404.timesCalled, 1)
+	require.True(t, strings.Contains(err.Error(), download.MsiDownload404ErrorString), "error string doesn't contain the correct message")
+}
+
 // Test Utilities:
 
 type mockDownloader struct {