Repository: Azure/ARO-RP
Pull request #2328 (2022-08-16 15:39:57)

Persist tags on ensureResourceGroup
### Which issue this PR addresses:

Fixes [this](https://msazure.visualstudio.com/AzureRedHatOpenShift/_workitems/edit/15272241/)

### What this PR does / why we need it:

Currently, if we PUCM a cluster with tags set on the cluster resource group, it will remove them.  We do allow tagging of the clsuter resource group, so we shouldn't be removing these.  

### Test plan for issue:

Unit tests & tested locally. 
### Is there any documentation that needs to be updated for this PR?

no, 


Patch:
diff --git a/.pipelines/generate-release-notes-tagged.yml b/.pipelines/build-and-push-images-tagged.yml
similarity index 62%
rename from .pipelines/generate-release-notes-tagged.yml
rename to .pipelines/build-and-push-images-tagged.yml
index 2a51aa736..3fd229c3a 100644
--- a/.pipelines/generate-release-notes-tagged.yml
+++ b/.pipelines/build-and-push-images-tagged.yml
@@ -1,19 +1,23 @@
-# Azure DevOps Pipeline for generating release notes
+# Azure DevOps Pipeline building rp images and pushing to int acr
 trigger: none
 pr: none
 
 variables:
+- template: vars.yml
 - name: TAG
+- group: PROD CI Credentials
 
 jobs:
-- job: Generate_release_notes
+- job: Build_and_push_images
   condition: startsWith(variables['build.sourceBranch'], 'refs/tags/v2')
-  displayName: Generate release notes
+  displayName: Build release
   pool:
     name: ARO-CI
+    demands: go-1.17
 
   steps:
   - template: ./templates/template-checkout.yml
+  - template: ./templates/template-az-cli-login.yml
     parameters:
       azureDevOpsJSONSPN: $(aro-v4-ci-devops-spn)
 
@@ -27,6 +31,19 @@ jobs:
       ## set the variable
       echo "##vso[task.setvariable variable=TAG]${TAG}"
 
+  - template: ./templates/template-push-images-to-acr-tagged.yml
+    parameters:
+      rpImageACR: $(RP_IMAGE_ACR)
+      imageTag: $(TAG)
+
+  - template: ./templates/template-az-cli-logout.yml
+  - script: |
+      cp -a --parents aro "$(Build.ArtifactStagingDirectory)"
+    displayName: Copy artifacts
+  - task: PublishBuildArtifacts@1
+    displayName: Publish Artifacts
+    name: aro_deployer
+
   - script: |
       set -xe
       MESSAGE="$(git for-each-ref refs/tags/${TAG} --format='%(contents)')"
diff --git a/.pipelines/onebranch/pipeline.buildrp.official.yml b/.pipelines/onebranch/pipeline.buildrp.official.yml
index d42b70d89..74cae8d1e 100644
--- a/.pipelines/onebranch/pipeline.buildrp.official.yml
+++ b/.pipelines/onebranch/pipeline.buildrp.official.yml
@@ -13,8 +13,7 @@ pr: none
 
 variables:
   Cdp_Definition_Build_Count: $[counter('', 0)] # needed for onebranch.pipeline.version task https://aka.ms/obpipelines/versioning
-  ONEBRANCH_AME_ACR_LOGIN: cdpxb8e9ef87cd634085ab141c637806568c00.azurecr.io
-  LinuxContainerImage: $(ONEBRANCH_AME_ACR_LOGIN)/b8e9ef87-cd63-4085-ab14-1c637806568c/official/ubi8/go-toolset:1.17.7-13 # Docker image which is used to build the project https://aka.ms/obpipelines/containers
+  LinuxContainerImage: cdpxlinux.azurecr.io/user/aro/ubi8-gotoolset-1.17.7-13:20220526   # Docker image which is used to build the project https://aka.ms/obpipelines/containers
   Debian_Frontend: noninteractive
 
 resources:
diff --git a/.pipelines/onebranch/pipeline.buildrp.pullrequest.yml b/.pipelines/onebranch/pipeline.buildrp.pullrequest.yml
index 6503245ea..1b3474e6e 100644
--- a/.pipelines/onebranch/pipeline.buildrp.pullrequest.yml
+++ b/.pipelines/onebranch/pipeline.buildrp.pullrequest.yml
@@ -13,8 +13,7 @@ pr: none
 
 variables:
   Cdp_Definition_Build_Count: $[counter('', 0)]  # needed for onebranch.pipeline.version task https://aka.ms/obpipelines/versioning
-  ONEBRANCH_AME_ACR_LOGIN: cdpxb8e9ef87cd634085ab141c637806568c00.azurecr.io
-  LinuxContainerImage: $(ONEBRANCH_AME_ACR_LOGIN)/b8e9ef87-cd63-4085-ab14-1c637806568c/official/ubi8/go-toolset:1.17.7-13 # Docker image which is used to build the project https://aka.ms/obpipelines/containers
+  LinuxContainerImage: cdpxlinux.azurecr.io/user/aro/ubi8-gotoolset-1.17.7-13:20220526 # Docker image which is used to build the project https://aka.ms/obpipelines/containers
   Debian_Frontend: noninteractive
 
 resources:
diff --git a/.pipelines/onebranch/templates/template-buildrp-builddocker.yml b/.pipelines/onebranch/templates/template-buildrp-builddocker.yml
index fe6bbe53b..f01894bb1 100644
--- a/.pipelines/onebranch/templates/template-buildrp-builddocker.yml
+++ b/.pipelines/onebranch/templates/template-buildrp-builddocker.yml
@@ -3,10 +3,9 @@ steps:
   displayName: Build Multi Stage Dockerfile
   inputs:
     repositoryName: aro-rp
-    dockerFileRelPath: ./Dockerfile.aro-multistage
+    dockerFileRelPath: ./Dockerfile.aro-multistage-onebranch
     dockerFileContextPath: ./
-    registry: cdpxb8e9ef87cd634085ab141c637806568c00.azurecr.io
-    arguments: --build-arg REGISTRY=registry.access.redhat.com
+    registry: cdpxlinux.azurecr.io
     saveImageToPath: aro-rp.tar
     buildkit: 1
     enable_network: true
diff --git a/Dockerfile.aro-multistage-onebranch b/Dockerfile.aro-multistage-onebranch
new file mode 100644
index 000000000..a032cddb0
--- /dev/null
+++ b/Dockerfile.aro-multistage-onebranch
@@ -0,0 +1,22 @@
+# Uses a multi-stage container build to build the RP in OneBranch.
+#
+# TODO:
+# OneBranch pipelines currently pull from CDPx which is deprecated. As a temporary fix before
+# we migrate to a new solution, this multistage dockerfile uses the same image in both steps to avoid
+# needing to pull images that aren't hosted in CDPx.
+FROM cdpxlinux.azurecr.io/user/aro/ubi8-gotoolset-1.17.7-13:20220526 AS builder
+ENV GOOS=linux \
+    GOPATH=/go/
+WORKDIR ${GOPATH}/src/github.com/Azure/ARO-RP
+USER root
+RUN yum update -y
+COPY . ${GOPATH}/src/github.com/Azure/ARO-RP/
+RUN make aro && make e2e.test
+
+FROM cdpxlinux.azurecr.io/user/aro/ubi8-gotoolset-1.17.7-13:20220526
+USER root
+RUN yum -y update && yum -y clean all
+COPY --from=builder /go/src/github.com/Azure/ARO-RP/aro /go/src/github.com/Azure/ARO-RP/e2e.test /usr/local/bin/
+ENTRYPOINT ["aro"]
+EXPOSE 2222/tcp 8080/tcp 8443/tcp 8444/tcp 8445/tcp
+USER 1000
diff --git a/docs/releases.md b/docs/releases.md
index d3e1cf858..2de0cef0e 100644
--- a/docs/releases.md
+++ b/docs/releases.md
@@ -23,16 +23,15 @@ you are tagging the right commit with the changes you want to release.
 
 ## Release pipeline
 
-Currently the release is done manually via the [EV2 pipelines](https://msazure.visualstudio.com/AzureRedHatOpenShift/_wiki/wikis/ARO.wiki/233405/Performing-Release).
+Currently the release is done manually via the [pipeline](https://github.com/Azure/ARO-RP/blob/master/.pipelines/build-and-push-images-tagged.yml).
+The pipeline does not have any parameter, instead the pipeline is started on the `tag` as illustrated on the image.
 
+![Start pipelines with tag](img/pipelines.png "Aro Monitor Architecture")
 
-### Release page
-
-The generate release notes pipeline does not have any parameter, instead the pipeline is started on the `tag` as illustrated on the image.
 
-![Start pipelines with tag](img/pipelines.png "Aro Monitor Architecture")
+### Release page
 
-Once the release notes pipeline is finished, a new item is added to the [GitHub release page](https://github.com/Azure/ARO-RP/releases) with
+Once the release is built, new item is added to the [GitHub release page](https://github.com/Azure/ARO-RP/releases) with
 the following format:
 
 ```
@@ -45,6 +44,11 @@ ${{tag annotation}}
 - hash commit message
 - hash commit message
 
+## Assets:
+
+- aro binary
+- code zipped
+
 ```
 
 The title of the release is the used `git tag`. The description is extracted
diff --git a/pkg/cluster/deploystorage.go b/pkg/cluster/deploystorage.go
index e6fb43ccc..98634c5e7 100644
--- a/pkg/cluster/deploystorage.go
+++ b/pkg/cluster/deploystorage.go
@@ -12,7 +12,6 @@ import (
 	"strings"
 
 	mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
-	mgmtfeatures "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-07-01/features"
 	"github.com/Azure/go-autorest/autorest"
 	"github.com/Azure/go-autorest/autorest/azure"
 	"github.com/Azure/go-autorest/autorest/to"
@@ -45,16 +44,19 @@ func (m *manager) ensureInfraID(ctx context.Context) (err error) {
 func (m *manager) ensureResourceGroup(ctx context.Context) error {
 	resourceGroup := stringutils.LastTokenByte(m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID, '/')
 
-	group := mgmtfeatures.ResourceGroup{
-		Location:  &m.doc.OpenShiftCluster.Location,
-		ManagedBy: &m.doc.OpenShiftCluster.ID,
+	// Retain the existing resource group configuration (such as tags) if it exists
+	group, err := m.resourceGroups.Get(ctx, resourceGroup)
+	if err != nil {
+		if detailedErr, ok := err.(autorest.DetailedError); !ok || detailedErr.StatusCode != http.StatusNotFound {
+			return err
+		}
 	}
+
+	group.Location = &m.doc.OpenShiftCluster.Location
+	group.ManagedBy = &m.doc.OpenShiftCluster.ID
+
+	// HACK: set purge=true on dev clusters so our purger wipes them out since there is not deny assignment in place
 	if m.env.IsLocalDevelopmentMode() {
-		// grab tags so we do not accidently remove them on createOrUpdate, set purge tag to true for dev clusters
-		rg, err := m.resourceGroups.Get(ctx, resourceGroup)
-		if err == nil {
-			group.Tags = rg.Tags
-		}
 		if group.Tags == nil {
 			group.Tags = map[string]*string{}
 		}
@@ -64,7 +66,7 @@ func (m *manager) ensureResourceGroup(ctx context.Context) error {
 	// According to https://stackoverflow.microsoft.com/a/245391/62320,
 	// re-PUTting our RG should re-create RP RBAC after a customer subscription
 	// migrates between tenants.
-	_, err := m.resourceGroups.CreateOrUpdate(ctx, resourceGroup, group)
+	_, err = m.resourceGroups.CreateOrUpdate(ctx, resourceGroup, group)
 
 	var serviceError *azure.ServiceError
 	// CreateOrUpdate wraps DetailedError wrapping a *RequestError (if error generated in ResourceGroup CreateOrUpdateResponder at least)


Test patch:
diff --git a/pkg/cluster/deploystorage_test.go b/pkg/cluster/deploystorage_test.go
index 85c3912d8..3c0d895ff 100644
--- a/pkg/cluster/deploystorage_test.go
+++ b/pkg/cluster/deploystorage_test.go
@@ -5,7 +5,9 @@ package cluster
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"net/http"
 	"strings"
 	"testing"
 
@@ -25,7 +27,7 @@ import (
 	testdatabase "github.com/Azure/ARO-RP/test/database"
 )
 
-func TestCreateAndUpdateErrors(t *testing.T) {
+func TestEnsureResourceGroup(t *testing.T) {
 	ctx := context.Background()
 	clusterID := "test-cluster"
 	resourceGroupName := "fakeResourceGroup"
@@ -37,41 +39,126 @@ func TestCreateAndUpdateErrors(t *testing.T) {
 		ManagedBy: &clusterID,
 	}
 
+	groupWithTags := group
+	groupWithTags.Tags = map[string]*string{
+		"yeet": to.StringPtr("yote"),
+	}
+
 	disallowedByPolicy := autorest.NewErrorWithError(&azure.RequestError{
 		ServiceError: &azure.ServiceError{Code: "RequestDisallowedByPolicy"},
 	}, "", "", nil, "")
 
 	for _, tt := range []struct {
 		name    string
-		result  mgmtfeatures.ResourceGroup
-		mocks   func(*mock_features.MockResourceGroupsClient, interface{})
+		mocks   func(*mock_features.MockResourceGroupsClient, *mock_env.MockInterface)
 		wantErr string
 	}{
 		{
-			name: "ResourceGroup creation was fine",
-			mocks: func(rg *mock_features.MockResourceGroupsClient, result interface{}) {
+			name: "success - rg doesn't exist",
+			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				rg.EXPECT().
+					Get(gomock.Any(), resourceGroupName).
+					Return(group, autorest.DetailedError{StatusCode: http.StatusNotFound})
+
+				rg.EXPECT().
+					CreateOrUpdate(gomock.Any(), resourceGroupName, group).
+					Return(group, nil)
+
+				env.EXPECT().
+					IsLocalDevelopmentMode().
+					Return(false)
+
+				env.EXPECT().
+					EnsureARMResourceGroupRoleAssignment(gomock.Any(), gomock.Any(), resourceGroupName).
+					Return(nil)
+			},
+		},
+		{
+			name: "success - rg doesn't exist and localdev mode tags set",
+			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				groupWithLocalDevTags := group
+				groupWithLocalDevTags.Tags = map[string]*string{
+					"purge": to.StringPtr("true"),
+				}
+				rg.EXPECT().
+					Get(gomock.Any(), resourceGroupName).
+					Return(group, autorest.DetailedError{StatusCode: http.StatusNotFound})
+
+				rg.EXPECT().
+					CreateOrUpdate(gomock.Any(), resourceGroupName, groupWithLocalDevTags).
+					Return(groupWithLocalDevTags, nil)
+
+				env.EXPECT().
+					IsLocalDevelopmentMode().
+					Return(true)
+
+				env.EXPECT().
+					EnsureARMResourceGroupRoleAssignment(gomock.Any(), gomock.Any(), resourceGroupName).
+					Return(nil)
+			},
+		},
+		{
+			name: "success - rg exists and maintain tags",
+			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				rg.EXPECT().
+					Get(gomock.Any(), resourceGroupName).
+					Return(groupWithTags, nil)
+
 				rg.EXPECT().
-					CreateOrUpdate(ctx, resourceGroupName, group).
-					Return(result, nil)
+					CreateOrUpdate(gomock.Any(), resourceGroupName, groupWithTags).
+					Return(groupWithTags, nil)
+
+				env.EXPECT().
+					IsLocalDevelopmentMode().
+					Return(false)
+
+				env.EXPECT().
+					EnsureARMResourceGroupRoleAssignment(gomock.Any(), gomock.Any(), resourceGroupName).
+					Return(nil)
 			},
 		},
 		{
-			name: "ResourceGroup creation failed with RequestDisallowedByPolicy",
-			mocks: func(rg *mock_features.MockResourceGroupsClient, result interface{}) {
+			name: "fail - get rg returns generic error",
+			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
 				rg.EXPECT().
-					CreateOrUpdate(ctx, resourceGroupName, group).
-					Return(result, disallowedByPolicy)
+					Get(gomock.Any(), resourceGroupName).
+					Return(group, errors.New("generic error"))
+			},
+			wantErr: "generic error",
+		},
+		{
+			name: "fail - CreateOrUpdate returns requestdisallowedbypolicy",
+			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				rg.EXPECT().
+					Get(gomock.Any(), resourceGroupName).
+					Return(group, autorest.DetailedError{StatusCode: http.StatusNotFound})
+
+				rg.EXPECT().
+					CreateOrUpdate(gomock.Any(), resourceGroupName, group).
+					Return(group, disallowedByPolicy)
+
+				env.EXPECT().
+					IsLocalDevelopmentMode().
+					Return(false)
 			},
 			wantErr: `400: DeploymentFailed: : Deployment failed. Details: : : {"code":"RequestDisallowedByPolicy","message":"","target":null,"details":null,"innererror":null,"additionalInfo":null}`,
 		},
 		{
-			name: "ResourceGroup creation failed with other error",
-			mocks: func(rg *mock_features.MockResourceGroupsClient, result interface{}) {
+			name: "fail - CreateOrUpdate returns generic error",
+			mocks: func(rg *mock_features.MockResourceGroupsClient, env *mock_env.MockInterface) {
+				rg.EXPECT().
+					Get(gomock.Any(), resourceGroupName).
+					Return(group, autorest.DetailedError{StatusCode: http.StatusNotFound})
+
 				rg.EXPECT().
-					CreateOrUpdate(ctx, resourceGroupName, group).
-					Return(result, fmt.Errorf("Any other error"))
+					CreateOrUpdate(gomock.Any(), resourceGroupName, group).
+					Return(group, errors.New("generic error"))
+
+				env.EXPECT().
+					IsLocalDevelopmentMode().
+					Return(false)
 			},
-			wantErr: "Any other error",
+			wantErr: "generic error",
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
@@ -79,12 +166,10 @@ func TestCreateAndUpdateErrors(t *testing.T) {
 			defer controller.Finish()
 
 			resourceGroupsClient := mock_features.NewMockResourceGroupsClient(controller)
-			tt.mocks(resourceGroupsClient, tt.result)
-
 			env := mock_env.NewMockInterface(controller)
+			tt.mocks(resourceGroupsClient, env)
+
 			env.EXPECT().Location().AnyTimes().Return(location)
-			env.EXPECT().EnsureARMResourceGroupRoleAssignment(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(nil)
-			env.EXPECT().IsLocalDevelopmentMode().Return(false)
 
 			m := &manager{
 				log:            logrus.NewEntry(logrus.StandardLogger()),