Improve E2E identity retries and logging for MIWI and SP setup

.pipelines/ci.yml

@@ -410,8 +410,14 @@ stages:
               export SKIP_MIWI_ROLE_ASSIGNMENT=true
               . ./hack/devtools/local_dev_env.sh
               echo "Creating MIWI env file"
-              create_miwi_env_file
+              create_miwi_env_file || exit 1
               . ./env
+              for required_var in MOCK_MSI_CLIENT_ID MOCK_MSI_OBJECT_ID MOCK_MSI_TENANT_ID MOCK_MSI_CERT; do
+                if [ -z "${!required_var:-}" ]; then
+                  echo "##vso[task.logissue type=error] ${required_var} is empty after create_miwi_env_file."
+                  exit 1
+                fi
+              done
               az acr login --name arosvcdev
               deploy_e2e_db
               register_sub
@@ -428,10 +434,18 @@ stages:
               fi
             displayName: ⚙️  Run E2E Test Suite for MIWI
           - bash: |
-              . ./env
-              echo "deleting mock msi service principal $MOCK_MSI_OBJECT_ID"
-              az ad sp delete --id $MOCK_MSI_OBJECT_ID
+              if [ -f ./env ]; then
+                . ./env
+              fi
+              sp_identifier="${MOCK_MSI_OBJECT_ID:-${MOCK_MSI_CLIENT_ID:-}}"
+              if [ -z "${sp_identifier}" ]; then
+                echo "skipping mock msi service principal cleanup because no service principal identifier is available"
+                exit 0
+              fi
+              echo "deleting mock msi service principal ${sp_identifier}"
+              az ad sp delete --id "${sp_identifier}"
             displayName: Delete Mock MSI Service Principal
+            condition: always()
 
           # Log the output from the services in case of failure
           - bash: |

hack/devtools/local_dev_env.sh

@@ -66,8 +66,43 @@ get_mock_msi_tenantID() {
     echo "$1" | jq -r .tenant
 }
 
+require_non_empty_value() {
+    local value="${1}"
+    local name="${2}"
+
+    if [[ -z "${value}" ]]; then
+        echo "ERROR: ${name} is empty." >&2
+        return 1
+    fi
+}
+
+get_service_principal_object_id() {
+    local servicePrincipalID="${1}"
+    local objectID=""
+    local attempt=1
+    local maxAttempts=12
+
+    while [[ "${attempt}" -le "${maxAttempts}" ]]; do
+        if objectID=$(az ad sp show --id "${servicePrincipalID}" --query id --output tsv 2>/dev/null) && [[ -n "${objectID}" ]]; then
+            echo "${objectID}"
+            return 0
+        fi
+
+        if [[ "${attempt}" -eq "${maxAttempts}" ]]; then
+            break
+        fi
+
+        echo "INFO: Service principal object ID not available yet for ${servicePrincipalID} (attempt ${attempt}/${maxAttempts}), retrying..." >&2
+        sleep 10
+        attempt=$((attempt + 1))
+    done
+
+    echo "ERROR: Failed to resolve service principal object ID for ${servicePrincipalID} after ${maxAttempts} attempts." >&2
+    return 1
+}
+
 get_mock_msi_objectID() {
-    az ad sp list --all --filter "appId eq '$1'" --output json | jq -r ".[] | .id"
+    get_service_principal_object_id "$1"
 }
 
 get_mock_msi_cert() {
@@ -97,7 +132,7 @@ get_platform_workloadIdentity_role_sets() {
 assign_role_to_identity() {
     local objectId=$1
     local roleId=$2
-    
+
     local scope="/subscriptions/${AZURE_SUBSCRIPTION_ID}/resourceGroups/${CLUSTER_RESOURCEGROUP}"
     local roles
 
@@ -108,7 +143,10 @@ assign_role_to_identity() {
 
     if [[ "$roles" == "" ]] || [[ "$roles" == "[]" ]] ; then
         echo "INFO: Assigning role to identity: ${objectId}"
-        az role assignment create --assignee-object-id "${objectId}" --assignee-principal-type "ServicePrincipal" --role "${roleId}"  --scope "${scope}" --output json
+        if ! az role assignment create --assignee-object-id "${objectId}" --assignee-principal-type "ServicePrincipal" --role "${roleId}"  --scope "${scope}" --output json; then
+            echo "ERROR: Failed to assign role ${roleId} to identity: ${objectId}" >&2
+            return 1
+        fi
         echo ""
     else
         echo "INFO: Role already assigned to identity: ${objectId}"
@@ -124,7 +162,10 @@ create_platform_identity_and_assign_role() {
 
     if ! identity=$(az identity show --name "${identityName}" --resource-group "${CLUSTER_RESOURCEGROUP}" --subscription "${AZURE_SUBSCRIPTION_ID}" --output json 2>/dev/null); then
         echo "INFO: Creating platform identity for operator: ${operatorName}"
-        identity=$(az identity create --name "${identityName}" --resource-group "${CLUSTER_RESOURCEGROUP}" --subscription "${AZURE_SUBSCRIPTION_ID}" --output json)
+        if ! identity=$(az identity create --name "${identityName}" --resource-group "${CLUSTER_RESOURCEGROUP}" --subscription "${AZURE_SUBSCRIPTION_ID}" --output json); then
+            echo "ERROR: Failed to create platform identity for operator: ${operatorName}" >&2
+            return 1
+        fi
     fi
 
     # Extract the client ID, principal Id, resource ID and name from the result
@@ -142,7 +183,7 @@ create_platform_identity_and_assign_role() {
     # Storage Operator don't require access to customer BYO virtual network
     if [[ "${operatorName}" != "StorageOperator" ]]; then
 
-        assign_role_to_identity "${principalId}" "${roleDefinitionId}"
+        assign_role_to_identity "${principalId}" "${roleDefinitionId}" || return 1
     fi
 }
 
@@ -159,23 +200,25 @@ setup_platform_identity() {
         operatorName=$(echo "$role" | jq -r '.operatorName')
         roleDefinitionId=$(echo "$role" | jq -r '.roleDefinitionId' | awk -F'/' '{print $NF}')
 
-        create_platform_identity_and_assign_role "${operatorName}" "${roleDefinitionId}"
+        create_platform_identity_and_assign_role "${operatorName}" "${roleDefinitionId}" || return 1
 
     done <<< "$platformWorkloadIdentityRoles"
 
     # Create the cluster identity
     echo "INFO: Creating cluster identity under RG ($CLUSTER_RESOURCEGROUP) and Sub Id ($AZURE_SUBSCRIPTION_ID)"
     echo ""
 
-    create_platform_identity_and_assign_role "Cluster" "ef318e2a-8334-4a05-9e4a-295a196c6a6e"
+    create_platform_identity_and_assign_role "Cluster" "ef318e2a-8334-4a05-9e4a-295a196c6a6e" || return 1
 }
 
 cluster_msi_role_assignment() {
     local clusterMSIAppID="${1}"
     local FEDERATED_CREDENTIAL_ROLE_ID="ef318e2a-8334-4a05-9e4a-295a196c6a6e"
     local clusterMSIObjectID
 
-    clusterMSIObjectID=$(az ad sp show --id "${clusterMSIAppID}" --query '{objectId: id}' --output json | jq -r .objectId)
+    if ! clusterMSIObjectID=$(get_service_principal_object_id "${clusterMSIAppID}"); then
+        return 1
+    fi
 
     echo "INFO: Assigning role to cluster MSI: ${clusterMSIAppID}"
     assign_role_to_identity "${clusterMSIObjectID}" "${FEDERATED_CREDENTIAL_ROLE_ID}"
@@ -184,15 +227,23 @@ cluster_msi_role_assignment() {
 create_miwi_env_file() {
     echo "INFO: Creating default env config file for managed/workload identity development..."
 
-    mockMSI=$(create_mock_msi)
+    if ! mockMSI=$(create_mock_msi); then
+        echo "ERROR: Failed to create mock MSI service principal." >&2
+        return 1
+    fi
     mockClientID=$(get_mock_msi_clientID "$mockMSI")
+    require_non_empty_value "${mockClientID}" "mock MSI client ID" || return 1
     mockTenantID=$(get_mock_msi_tenantID "$mockMSI")
+    require_non_empty_value "${mockTenantID}" "mock MSI tenant ID" || return 1
     mockCert=$(get_mock_msi_cert "$mockMSI")
-    mockObjectID=$(get_mock_msi_objectID "$mockClientID")
-
+    require_non_empty_value "${mockCert}" "mock MSI certificate" || return 1
+    if ! mockObjectID=$(get_mock_msi_objectID "$mockClientID"); then
+        return 1
+    fi
+
     if [[ $SKIP_MIWI_ROLE_ASSIGNMENT != "true" ]]; then
-      setup_platform_identity
-      cluster_msi_role_assignment "${mockClientID}"
+      setup_platform_identity || return 1
+      cluster_msi_role_assignment "${mockClientID}" || return 1
     fi
 
     cat >> env <<EOF
@@ -219,7 +270,7 @@ EOF
 
 
 ask_to_create_Azure_deployment() {
-    
+
     local answer
     read -r -p "Create Azure deployment in the current subscription ($AZURE_SUBSCRIPTION_ID)? (y / n / l (list existing deployments)) " answer
 

hack/devtools/msi.sh

@@ -26,12 +26,17 @@ if [[ -z "$sp" ]]; then
     exit 1
 fi
 mockClientID=$(get_mock_msi_clientID "$sp")
+require_non_empty_value "$mockClientID" "mock MSI client ID" || exit 1
 mockTenantID=$(get_mock_msi_tenantID "$sp")
+require_non_empty_value "$mockTenantID" "mock MSI tenant ID" || exit 1
 base64EncodedCert=$(get_mock_msi_cert "$sp")
-mockObjectID=$(get_mock_msi_objectID "$mockClientID")
+require_non_empty_value "$base64EncodedCert" "mock MSI certificate" || exit 1
+if ! mockObjectID=$(get_mock_msi_objectID "$mockClientID"); then
+    exit 1
+fi
 
-setup_platform_identity
-cluster_msi_role_assignment "${mockClientID}"
+setup_platform_identity || exit 1
+cluster_msi_role_assignment "${mockClientID}" || exit 1
 
 # Print the extracted values
 echo "Cluster MSI Client ID: $mockClientID"

pkg/frontend/adminactions/delete_managedresource.go

@@ -37,22 +37,28 @@ func (a *azureActions) ResourceDeleteAndWait(ctx context.Context, resourceID str
 	}
 
 	apiVersion := azureclient.APIVersion(strings.ToLower(idParts.ResourceType.String()))
+	resourceType := strings.ToLower(idParts.ResourceType.String())
 
 	_, err = a.resources.GetByID(ctx, resourceID, apiVersion)
 	if err != nil {
 		return err
 	}
 
 	// FrontendIPConfiguration cannot be deleted with DeleteByIDAndWait (DELETE method is invalid on frontendIPConfiguration resourceID)
-	if idParts.ResourceType.String() == "Microsoft.Network/loadBalancers/frontendIPConfigurations" {
+	if resourceType == "microsoft.network/loadbalancers/frontendipconfigurations" {
 		return a.deleteFrontendIPConfiguration(ctx, resourceID, idParts.ResourceGroupName, idParts.Parent.Name)
 	}
 
 	// HealthProbes cannot be deleted with DeleteByIDAndWait either.
-	if idParts.ResourceType.String() == "Microsoft.Network/loadBalancers/probes" {
+	if resourceType == "microsoft.network/loadbalancers/probes" {
 		return a.deleteHealthProbe(ctx, resourceID, idParts.ResourceGroupName, idParts.Parent.Name)
 	}
 
+	// LoadBalancingRules must be removed via an update to the parent load balancer.
+	if resourceType == "microsoft.network/loadbalancers/loadbalancingrules" {
+		return a.deleteLoadBalancingRule(ctx, resourceID, idParts.ResourceGroupName, idParts.Parent.Name)
+	}
+
 	return a.resources.DeleteByIDAndWait(ctx, resourceID, apiVersion)
 }
 
@@ -83,3 +89,17 @@ func (a *azureActions) deleteHealthProbe(ctx context.Context, resourceID string,
 
 	return a.loadBalancers.CreateOrUpdateAndWait(ctx, rg, loadBalancerName, lb.LoadBalancer, nil)
 }
+
+func (a *azureActions) deleteLoadBalancingRule(ctx context.Context, resourceID string, rg string, loadBalancerName string) error {
+	lb, err := a.loadBalancers.Get(ctx, rg, loadBalancerName, nil)
+	if err != nil {
+		return err
+	}
+
+	err = loadbalancer.RemoveLoadBalancingRule(&lb.LoadBalancer, resourceID)
+	if err != nil {
+		return err
+	}
+
+	return a.loadBalancers.CreateOrUpdateAndWait(ctx, rg, loadBalancerName, lb.LoadBalancer, nil)
+}

pkg/frontend/adminactions/delete_managedresource_test.go

@@ -102,6 +102,67 @@ var originalLB = armnetwork.LoadBalancer{
 	Location: &location,
 }
 
+func loadBalancerWithRuleReferences() armnetwork.LoadBalancer {
+	rule80ID := "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/clusterRG/providers/Microsoft.Network/loadBalancers/infraID/loadBalancingRules/ae3506385907e44eba9ef9bf76eac973-TCP-80"
+	rule443ID := "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/clusterRG/providers/Microsoft.Network/loadBalancers/infraID/loadBalancingRules/ae3506385907e44eba9ef9bf76eac973-TCP-443"
+
+	return armnetwork.LoadBalancer{
+		SKU: &armnetwork.LoadBalancerSKU{
+			Name: pointerutils.ToPtr(armnetwork.LoadBalancerSKUNameStandard),
+		},
+		Properties: &armnetwork.LoadBalancerPropertiesFormat{
+			FrontendIPConfigurations: []*armnetwork.FrontendIPConfiguration{
+				{
+					Name: pointerutils.ToPtr("ae3506385907e44eba9ef9bf76eac973"),
+					ID:   pointerutils.ToPtr("/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/clusterRG/providers/Microsoft.Network/loadBalancers/infraID/frontendIPConfigurations/ae3506385907e44eba9ef9bf76eac973"),
+					Properties: &armnetwork.FrontendIPConfigurationPropertiesFormat{
+						LoadBalancingRules: []*armnetwork.SubResource{
+							{ID: pointerutils.ToPtr("ae3506385907e44eba9ef9bf76eac973-TCP-80")},
+							{ID: pointerutils.ToPtr("ae3506385907e44eba9ef9bf76eac973-TCP-443")},
+						},
+					},
+				},
+			},
+			BackendAddressPools: []*armnetwork.BackendAddressPool{
+				{
+					Name: pointerutils.ToPtr("test-backend-pool"),
+					Properties: &armnetwork.BackendAddressPoolPropertiesFormat{
+						LoadBalancingRules: []*armnetwork.SubResource{
+							{ID: pointerutils.ToPtr("ae3506385907e44eba9ef9bf76eac973-TCP-80")},
+							{ID: pointerutils.ToPtr("ae3506385907e44eba9ef9bf76eac973-TCP-443")},
+						},
+					},
+				},
+			},
+			Probes: []*armnetwork.Probe{
+				{
+					Name: pointerutils.ToPtr("testProbeInUse"),
+					Properties: &armnetwork.ProbePropertiesFormat{
+						Port: pointerutils.ToPtr(int32(8443)),
+						LoadBalancingRules: []*armnetwork.SubResource{
+							{ID: pointerutils.ToPtr("ae3506385907e44eba9ef9bf76eac973-TCP-80")},
+							{ID: pointerutils.ToPtr("ae3506385907e44eba9ef9bf76eac973-TCP-443")},
+						},
+					},
+				},
+			},
+			LoadBalancingRules: []*armnetwork.LoadBalancingRule{
+				{
+					Name: pointerutils.ToPtr("ae3506385907e44eba9ef9bf76eac973-TCP-80"),
+					ID:   pointerutils.ToPtr(rule80ID),
+				},
+				{
+					Name: pointerutils.ToPtr("ae3506385907e44eba9ef9bf76eac973-TCP-443"),
+					ID:   pointerutils.ToPtr(rule443ID),
+				},
+			},
+		},
+		Name:     &infraID,
+		Type:     pointerutils.ToPtr("Microsoft.Network/loadBalancers"),
+		Location: &location,
+	}
+}
+
 func TestDeleteManagedResource(t *testing.T) {
 	// Run tests
 	for _, tt := range []struct {
@@ -286,6 +347,58 @@ func TestDeleteManagedResource(t *testing.T) {
 				}, nil).Return(nil)
 			},
 		},
+		{
+			name:        "load balancing rule delete",
+			resourceID:  "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/clusterRG/providers/Microsoft.Network/loadBalancers/infraID/loadBalancingRules/ae3506385907e44eba9ef9bf76eac973-TCP-80",
+			expectedErr: "",
+			mocks: func(resources *mock_features.MockResourcesClient, loadBalancers *mock_armnetwork.MockLoadBalancersClient) {
+				lbFixture := loadBalancerWithRuleReferences()
+				resources.EXPECT().GetByID(gomock.Any(), "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/clusterRG/providers/Microsoft.Network/loadBalancers/infraID/loadBalancingRules/ae3506385907e44eba9ef9bf76eac973-TCP-80", "2020-08-01").Return(mgmtfeatures.GenericResource{}, nil)
+				loadBalancers.EXPECT().Get(gomock.Any(), "clusterRG", "infraID", nil).Return(armnetwork.LoadBalancersClientGetResponse{LoadBalancer: lbFixture}, nil)
+				loadBalancers.EXPECT().CreateOrUpdateAndWait(gomock.Any(), clusterRG, infraID, gomock.AssignableToTypeOf(armnetwork.LoadBalancer{}), nil).DoAndReturn(
+					func(ctx context.Context, resourceGroupName, loadBalancerName string, parameters armnetwork.LoadBalancer, options *armnetwork.LoadBalancersClientBeginCreateOrUpdateOptions) error {
+						expectedRemainingRuleID := "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/clusterRG/providers/Microsoft.Network/loadBalancers/infraID/loadBalancingRules/ae3506385907e44eba9ef9bf76eac973-TCP-443"
+
+						if parameters.Properties == nil {
+							t.Fatal("expected load balancer properties to be set")
+						}
+						if len(parameters.Properties.LoadBalancingRules) != 1 || parameters.Properties.LoadBalancingRules[0].ID == nil || *parameters.Properties.LoadBalancingRules[0].ID != expectedRemainingRuleID {
+							t.Fatalf("expected exactly one remaining top-level load balancing rule %s, got %#v", expectedRemainingRuleID, parameters.Properties.LoadBalancingRules)
+						}
+						if len(parameters.Properties.FrontendIPConfigurations) != 1 || parameters.Properties.FrontendIPConfigurations[0].Properties == nil ||
+							len(parameters.Properties.FrontendIPConfigurations[0].Properties.LoadBalancingRules) != 1 ||
+							parameters.Properties.FrontendIPConfigurations[0].Properties.LoadBalancingRules[0].ID == nil ||
+							*parameters.Properties.FrontendIPConfigurations[0].Properties.LoadBalancingRules[0].ID != "ae3506385907e44eba9ef9bf76eac973-TCP-443" {
+							t.Fatalf("expected frontend IP config references to retain only the remaining rule, got %#v", parameters.Properties.FrontendIPConfigurations)
+						}
+						if len(parameters.Properties.BackendAddressPools) != 1 || parameters.Properties.BackendAddressPools[0].Properties == nil ||
+							len(parameters.Properties.BackendAddressPools[0].Properties.LoadBalancingRules) != 1 ||
+							parameters.Properties.BackendAddressPools[0].Properties.LoadBalancingRules[0].ID == nil ||
+							*parameters.Properties.BackendAddressPools[0].Properties.LoadBalancingRules[0].ID != "ae3506385907e44eba9ef9bf76eac973-TCP-443" {
+							t.Fatalf("expected backend pool references to retain only the remaining rule, got %#v", parameters.Properties.BackendAddressPools)
+						}
+						if len(parameters.Properties.Probes) != 1 || parameters.Properties.Probes[0].Properties == nil ||
+							len(parameters.Properties.Probes[0].Properties.LoadBalancingRules) != 1 ||
+							parameters.Properties.Probes[0].Properties.LoadBalancingRules[0].ID == nil ||
+							*parameters.Properties.Probes[0].Properties.LoadBalancingRules[0].ID != "ae3506385907e44eba9ef9bf76eac973-TCP-443" {
+							t.Fatalf("expected probe references to retain only the remaining rule, got %#v", parameters.Properties.Probes)
+						}
+						return nil
+					},
+				)
+			},
+		},
+		{
+			name:        "load balancing rule delete with mixed-case resource type",
+			resourceID:  "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/clusterRG/providers/Microsoft.Network/loadBalancers/infraID/LoadBalancingRules/ae3506385907e44eba9ef9bf76eac973-TCP-80",
+			expectedErr: "",
+			mocks: func(resources *mock_features.MockResourcesClient, loadBalancers *mock_armnetwork.MockLoadBalancersClient) {
+				lbFixture := loadBalancerWithRuleReferences()
+				resources.EXPECT().GetByID(gomock.Any(), "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/clusterRG/providers/Microsoft.Network/loadBalancers/infraID/LoadBalancingRules/ae3506385907e44eba9ef9bf76eac973-TCP-80", "2020-08-01").Return(mgmtfeatures.GenericResource{}, nil)
+				loadBalancers.EXPECT().Get(gomock.Any(), "clusterRG", "infraID", nil).Return(armnetwork.LoadBalancersClientGetResponse{LoadBalancer: lbFixture}, nil)
+				loadBalancers.EXPECT().CreateOrUpdateAndWait(gomock.Any(), clusterRG, infraID, gomock.AssignableToTypeOf(armnetwork.LoadBalancer{}), nil).Return(nil)
+			},
+		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			controller := gomock.NewController(t)