ARO-9391 Remerge original PR with additional fixes

Author: rajdeepc2792

go.sum

@@ -991,8 +991,8 @@ github.com/phpdave11/gofpdf v1.4.2/go.mod h1:zpO6xFn9yxo3YLyMvW8HcKWVdbNqgIfOOp2
 github.com/phpdave11/gofpdi v1.0.12/go.mod h1:vBmVV0Do6hSBHC8uKUQ71JGW+ZGQq74llk/7bXwjDoI=
 github.com/phpdave11/gofpdi v1.0.13/go.mod h1:vBmVV0Do6hSBHC8uKUQ71JGW+ZGQq74llk/7bXwjDoI=
 github.com/pierrec/lz4/v4 v4.1.15/go.mod h1:gZWDp/Ze/IJXGXf23ltt2EXimqmTUXEy0GFuRQyBid4=
-github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 h1:KoWmjvw+nsYOo29YJK9vDA65RGE3NrOnUtO7a+RF9HU=
-github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8/go.mod h1:HKlIX3XHQyzLZPlr7++PzdhaXEj94dEiJgZDTsxEqUI=
+github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c h1:+mdjkGKdHQG3305AYmdv1U2eRNDiU2ErMBj1gwrq8eQ=
+github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c/go.mod h1:7rwL4CYBLnjLxUqIJNnCWiEdr3bn6IUYi15bNlnbCCU=
 github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA=
 github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
 github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
@@ -1366,7 +1366,6 @@ golang.org/x/sys v0.0.0-20210510120138-977fb7262007/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20210514084401-e8d321eab015/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20210603125802-9665404d3644/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.0.0-20210616045830-e2b7044e8c71/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20210616094352-59db8d763f22/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20210806184541-e5e7981a1069/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=

pkg/storage/azure/azure.go

@@ -14,6 +14,7 @@ import (
 	"testing"
 
 	"github.com/Azure/azure-pipeline-go/pipeline"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
 	"github.com/Azure/go-autorest/autorest"
 	"github.com/Azure/go-autorest/autorest/mocks"
 	"github.com/google/go-cmp/cmp"
@@ -34,6 +35,32 @@ import (
 	"github.com/openshift/cluster-image-registry-operator/pkg/envvar"
 )
 
+const mockTenantID = "00000000-0000-0000-0000-000000000000"
+
+type testDoer struct {
+	response   *http.Response
+	body       string
+	statusCode int
+	header     http.Header
+}
+
+// Do implements the Doer interface for mocking.
+// Do accepts the passed policy request and body, then appends the response and emits it.
+func (td *testDoer) Do(r *policy.Request) (resp *http.Response, err error) {
+	// Helps in emitting sequential Responses for the same client
+	if td.response != nil {
+		return r.Next()
+	}
+	resp = &http.Response{
+		StatusCode: td.statusCode,
+		Request:    r.Raw(),
+		Body:       io.NopCloser(bytes.NewBufferString(td.body)),
+		Header:     td.header,
+	}
+	td.response = resp
+	return resp, nil
+}
+
 func TestGetConfig(t *testing.T) {
 	for _, tt := range []struct {
 		name    string
@@ -260,7 +287,7 @@ func findEnvVar(envvars envvar.List, name string) *envvar.EnvVar {
 	return nil
 }
 
-func TestConfigEnv(t *testing.T) {
+func TestConfigEnvNonAzureStackHub(t *testing.T) {
 	ctx := context.Background()
 
 	cr := &imageregistryv1.Config{}
@@ -289,6 +316,7 @@ func TestConfigEnv(t *testing.T) {
 		Data: map[string][]byte{
 			"azure_subscription_id": []byte("subscription_id"),
 			"azure_client_id":       []byte("client_id"),
+			"azure_tenant_id":       []byte(mockTenantID),
 			"azure_client_secret":   []byte("client_secret"),
 			"azure_resourcegroup":   []byte("resourcegroup"),
 		},
@@ -304,16 +332,12 @@ func TestConfigEnv(t *testing.T) {
 	sender.AppendResponse(mocks.NewResponseWithContent(`{"keys":[{"value":"firstKey"}]}`))
 	sender.AppendResponse(mocks.NewResponseWithContent(`{"keys":[{"value":"firstKey"}]}`))
 
-	httpSender := pipeline.FactoryFunc(func(next pipeline.Policy, po *pipeline.PolicyOptions) pipeline.PolicyFunc {
-		return func(ctx context.Context, request pipeline.Request) (pipeline.Response, error) {
-			return pipeline.NewHTTPResponse(mocks.NewResponseWithContent(`{}`)), nil
-		}
-	})
-
 	d := NewDriver(ctx, config, &listers.StorageListers)
 	d.authorizer = authorizer
 	d.sender = sender
-	d.httpSender = httpSender
+	d.policies = []policy.Policy{
+		&testDoer{statusCode: http.StatusCreated},
+	}
 	err := d.CreateStorage(cr)
 	if err != nil {
 		t.Fatal(err)
@@ -339,7 +363,7 @@ func TestConfigEnv(t *testing.T) {
 	}
 }
 
-func TestConfigEnvWorkloadIdentity(t *testing.T) {
+func TestConfigEnvWorkloadIdentityNonAzureStackHub(t *testing.T) {
 	ctx := context.Background()
 
 	config := &imageregistryv1.ImageRegistryConfigStorageAzure{}
@@ -382,16 +406,15 @@ func TestConfigEnvWorkloadIdentity(t *testing.T) {
 	sender.AppendResponse(mocks.NewResponseWithContent(`{"name":"account"}`))
 	sender.AppendResponse(mocks.NewResponseWithContent(`{"keys":[{"value":"firstKey"}]}`))
 	sender.AppendResponse(mocks.NewResponseWithContent(`{"keys":[{"value":"firstKey"}]}`))
-	httpSender := pipeline.FactoryFunc(func(next pipeline.Policy, po *pipeline.PolicyOptions) pipeline.PolicyFunc {
-		return func(ctx context.Context, request pipeline.Request) (pipeline.Response, error) {
-			return pipeline.NewHTTPResponse(mocks.NewResponseWithContent(`{}`)), nil
-		}
-	})
 
 	d := NewDriver(ctx, config, &listers.StorageListers)
 	d.authorizer = authorizer
 	d.sender = sender
-	d.httpSender = httpSender
+	d.policies = []policy.Policy{
+		&testDoer{
+			statusCode: http.StatusAccepted,
+		},
+	}
 
 	envvars, err := d.ConfigEnv()
 	if err != nil {
@@ -1154,7 +1177,7 @@ func Test_containerExists(t *testing.T) {
 	}
 }
 
-func Test_storageManagementState(t *testing.T) {
+func Test_storageManagementStateNonAzureStackHub(t *testing.T) {
 	builder := cirofake.NewFixturesBuilder()
 	builder.AddInfraConfig(&configv1.Infrastructure{
 		ObjectMeta: metav1.ObjectMeta{
@@ -1178,17 +1201,20 @@ func Test_storageManagementState(t *testing.T) {
 		Data: map[string][]byte{
 			"azure_subscription_id": []byte("subscription_id"),
 			"azure_client_id":       []byte("client_id"),
+			"azure_tenant_id":       []byte(mockTenantID),
 			"azure_client_secret":   []byte("client_secret"),
 			"azure_resourcegroup":   []byte("resourcegroup"),
 		},
 	})
 	listers := builder.BuildListers()
+	containerNotFoundHeader := http.Header{}
+	containerNotFoundHeader.Add("x-ms-error-code", "ContainerNotFound")
 
 	for _, tt := range []struct {
 		name           string
 		registryConfig *imageregistryv1.Config
 		mockResponses  []*http.Response
-		httpSender     func(int) func(_ context.Context, _ pipeline.Request) (pipeline.Response, error)
+		policies       []policy.Policy
 		err            string
 		checkFn        func(*imageregistryv1.Config)
 	}{
@@ -1206,6 +1232,11 @@ func Test_storageManagementState(t *testing.T) {
 					t.Error("unexpected empty container")
 				}
 			},
+			policies: []policy.Policy{
+				&testDoer{
+					statusCode: http.StatusCreated,
+				},
+			},
 		},
 		{
 			name: "user providing container and account name (both already exist)",
@@ -1234,6 +1265,11 @@ func Test_storageManagementState(t *testing.T) {
 					t.Errorf("container has changed to %s", cr.Spec.Storage.Azure.Container)
 				}
 			},
+			policies: []policy.Policy{
+				&testDoer{
+					statusCode: http.StatusOK,
+				},
+			},
 		},
 		{
 			name: "user providing container and account name (both don't exist)",
@@ -1247,19 +1283,6 @@ func Test_storageManagementState(t *testing.T) {
 					},
 				},
 			},
-			httpSender: func(req int) func(_ context.Context, _ pipeline.Request) (pipeline.Response, error) {
-				if req == 0 {
-					return func(_ context.Context, _ pipeline.Request) (pipeline.Response, error) {
-						r := mocks.NewResponseWithStatus("", http.StatusNotFound)
-						r.Header = map[string][]string{}
-						r.Header.Add("x-ms-error-code", "ContainerNotFound")
-						return pipeline.NewHTTPResponse(r), nil
-					}
-				}
-				return func(_ context.Context, _ pipeline.Request) (pipeline.Response, error) {
-					return pipeline.NewHTTPResponse(mocks.NewResponseWithContent(`{}`)), nil
-				}
-			},
 			checkFn: func(cr *imageregistryv1.Config) {
 				if cr.Spec.Storage.ManagementState != imageregistryv1.StorageManagementStateManaged {
 					t.Errorf("expected to be managed, %q instead", cr.Spec.Storage.ManagementState)
@@ -1271,6 +1294,15 @@ func Test_storageManagementState(t *testing.T) {
 					t.Errorf("container has changed to %s", cr.Spec.Storage.Azure.Container)
 				}
 			},
+			policies: []policy.Policy{
+				&testDoer{
+					statusCode: http.StatusNotFound,
+					header:     containerNotFoundHeader,
+				},
+				&testDoer{
+					statusCode: http.StatusCreated,
+				},
+			},
 		},
 		{
 			name: "user providing container and account name (only account name exists)",
@@ -1295,23 +1327,19 @@ func Test_storageManagementState(t *testing.T) {
 					t.Errorf("container has changed to %s", cr.Spec.Storage.Azure.Container)
 				}
 			},
-			httpSender: func(req int) func(_ context.Context, _ pipeline.Request) (pipeline.Response, error) {
-				if req == 0 {
-					return func(_ context.Context, _ pipeline.Request) (pipeline.Response, error) {
-						r := mocks.NewResponseWithStatus("", http.StatusNotFound)
-						r.Header = map[string][]string{}
-						r.Header.Add("x-ms-error-code", "ContainerNotFound")
-						return pipeline.NewHTTPResponse(r), nil
-					}
-				}
-				return func(_ context.Context, _ pipeline.Request) (pipeline.Response, error) {
-					return pipeline.NewHTTPResponse(mocks.NewResponseWithContent(`{}`)), nil
-				}
-			},
 			mockResponses: []*http.Response{
 				mocks.NewResponseWithContent(`{"nameAvailable":false}`),
 				mocks.NewResponseWithContent(`{"keys":[{"value":"firstKey"}]}`),
 			},
+			policies: []policy.Policy{
+				&testDoer{
+					statusCode: http.StatusNotFound,
+					header:     containerNotFoundHeader,
+				},
+				&testDoer{
+					statusCode: http.StatusCreated,
+				},
+			},
 		},
 		{
 			name: "do not overwrite management state already set by user",
@@ -1333,6 +1361,9 @@ func Test_storageManagementState(t *testing.T) {
 					t.Error("unexpected empty container")
 				}
 			},
+			policies: []policy.Policy{
+				&testDoer{statusCode: http.StatusCreated},
+			},
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
@@ -1360,23 +1391,9 @@ func Test_storageManagementState(t *testing.T) {
 			)
 			drv.authorizer = autorest.NullAuthorizer{}
 			drv.sender = sender
-
-			var requestCounter int
-			drv.httpSender = pipeline.FactoryFunc(
-				func(_ pipeline.Policy, _ *pipeline.PolicyOptions) pipeline.PolicyFunc {
-					defer func() {
-						requestCounter++
-					}()
-
-					if tt.httpSender != nil {
-						return tt.httpSender(requestCounter)
-					}
-
-					return func(_ context.Context, _ pipeline.Request) (pipeline.Response, error) {
-						return pipeline.NewHTTPResponse(mocks.NewResponseWithContent(`{}`)), nil
-					}
-				},
-			)
+			if tt.policies != nil {
+				drv.policies = tt.policies
+			}
 
 			if err := drv.CreateStorage(tt.registryConfig); err != nil {
 				if len(tt.err) == 0 {