Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: storage account 403 error when creating new tre env #4406

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ShakutaiGit
Copy link
Collaborator

@ShakutaiGit ShakutaiGit commented Feb 26, 2025

Resolves #4405

What is being addressed

  • Ensuring role propagation before execution: The script now properly waits for both Storage Account Contributor and Storage Blob Data Contributor roles to be assigned before attempting storage operations.
  • Refactoring role assignment check: Instead of checking a single role, the script now verifies both roles are assigned before proceeding, reducing failures due to role propagation delays.

Testing:

…ntributor" and "Storage Account Contributor" roles
Copy link

github-actions bot commented Feb 26, 2025

Unit Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 4c26cde.

♻️ This comment has been updated with latest results.

@ShakutaiGit ShakutaiGit changed the title Refactor role assignment check to validate both "Storage Blob Data Co… Fix: storage account 403 error when creating new tre env. Feb 26, 2025
@ShakutaiGit ShakutaiGit changed the title Fix: storage account 403 error when creating new tre env. Fix: storage account 403 error when creating new tre env Feb 26, 2025
@ShakutaiGit
Copy link
Collaborator Author

/test

Copy link

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/13545863577 (with refid ecb751e8)

(in response to this comment from @ShakutaiGit)

@ShakutaiGit
Copy link
Collaborator Author

/test

Copy link

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/13547308284 (with refid ecb751e8)

(in response to this comment from @ShakutaiGit)

@ShakutaiGit ShakutaiGit self-assigned this Feb 26, 2025
…enhance storage container creation with retry logic
@ShakutaiGit ShakutaiGit marked this pull request as ready for review February 26, 2025 21:27
@ShakutaiGit
Copy link
Collaborator Author

/test

Copy link

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/13553941503 (with refid ecb751e8)

(in response to this comment from @ShakutaiGit)

@ShakutaiGit
Copy link
Collaborator Author

/test

Copy link

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/13559362952 (with refid ecb751e8)

(in response to this comment from @ShakutaiGit)

Copy link
Member

@marrobi marrobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of comments. Thanks for looking into making this more robust.

az role assignment list --assignee "$USER_OBJECT_ID" --role "Storage Blob Data Contributor" --scope "/subscriptions/$ARM_SUBSCRIPTION_ID/resourceGroups/$TF_VAR_mgmt_resource_group_name/providers/Microsoft.Storage/storageAccounts/$TF_VAR_mgmt_storage_account_name" --query "[].id" --output tsv
check_role_assignments() {
local sbdc
sbdc=$(az role assignment list \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion from copilot, not tested, but makes sense to have a timeout of some sort...

# Function to check if the role assignments exist
check_role_assignments() {
  local roles
  roles=$(az role assignment list \
    --assignee "$USER_OBJECT_ID" \
    --scope "/subscriptions/$ARM_SUBSCRIPTION_ID/resourceGroups/$TF_VAR_mgmt_resource_group_name/providers/Microsoft.Storage/storageAccounts/$TF_VAR_mgmt_storage_account_name" \
    --query "[?roleDefinitionName=='Storage Blob Data Contributor' || roleDefinitionName=='Storage Account Contributor'].roleDefinitionName" --output tsv)
  
  if [[ $roles == *"Storage Blob Data Contributor"* && $roles == *"Storage Account Contributor"* ]]; then
    echo "both"
  fi
}

# Wait for the role assignment to be applied with a timeout
echo -e "\n\e[34m»»» ⏳ \e[96mWaiting for role assignment to be applied\e[0m..."
timeout=300  # 5 minutes timeout
start_time=$(date +%s)

while [ -z "$(check_role_assignments)" ]; do
  echo "Waiting for role assignment..."
  sleep 10
  current_time=$(date +%s)
  elapsed_time=$((current_time - start_time))
  if [ $elapsed_time -ge $timeout ]; then
    echo "ERROR: Timeout waiting for role assignments."
    exit 1
  fi
done
echo "Role assignment applied."

echo "Waiting for role assignment..."
sleep 10
done
echo "Role assignment applied."

sleep 30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to sleep if we are retrying below?

@@ -38,29 +38,67 @@ if [ -n "${ARM_CLIENT_ID:-}" ]; then
else
USER_OBJECT_ID=$(az ad signed-in-user show --query id --output tsv)
fi

az role assignment create --assignee "$USER_OBJECT_ID" \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you work out why this is required? I thought the object ID that creates the storage account should have sufficient permissions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bootstrap script fails due to AuthorizationPermissionMismatch (403) on Storage Account
3 participants