-
Notifications
You must be signed in to change notification settings - Fork 97
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
Ambiguous error when submitting invalid Azure subscription ID #6302
Labels
good first issue
Good for newcomers
Comments
Notes from triage:
|
5 tasks
AaronCrawfis
referenced
this issue
Jun 21, 2023
# Description Today we accept any string for `--azure-subscription-id`, which can be confusing if a user tries to use a subscription name instead of a subscription ID. We also don't have further downstream checks so this results in a really bad error for users when they do this. This PR adds CLI validation to make sure the supplied value is a valid UUID (_but doesn't check if it's a valid Azure subscription, that'll come later_). There is still work to do to add additional validation, which will come as part of a future PR ## Issue reference <!-- We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation. --> Partially addresses #5757 ## Checklist Please make sure you've completed the relevant tasks for this PR, out of the following list: * [x] Code compiles correctly * [x] Adds necessary unit tests for change * [x] Adds necessary E2E tests for change * [x] Unit tests passing * [x] Extended the documentation / Created issue for it ## Auto-generated summary <!-- GitHub Copilot for docs will auto-generate a summary of the PR --> <!-- copilot:all --> ### <samp>🤖 Generated by Copilot at 6876cc0</samp> ### Summary 🔄🆕✅ <!-- 1. 🔄 - This emoji represents the change in the `Validate` function to use the `RequireAzureSubscriptionId` function instead of directly getting the flag value. 2. 🆕 - This emoji represents the addition of the new function `RequireAzureSubscriptionId` to the `cli` package to validate Azure subscription ID flags for commands. 3. ✅ - This emoji represents the addition and update of test cases for Azure subscription ID validation in `update_test`. --> Added a new function to validate Azure subscription ID flags for `env update` and other commands. Updated tests to cover the validation logic. > _Sing, O Muse, of the skillful code review_ > _That brought new order to the update command_ > _And made it heed the azure flag, the true_ > _And valid sign of the subscription's hand._ ### Walkthrough * Import and use `uuid.Parse` and `AzureSubscriptionIdFlag` to validate Azure subscription ID flag value in `RequireAzureSubscriptionId` function ([link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-bf4e917446c73565ee6967f28bdd8e619c0d4fa40b215f195b840c021dd36ce9L25-R28), [link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-bf4e917446c73565ee6967f28bdd8e619c0d4fa40b215f195b840c021dd36ce9R264-R278)) * Use `RequireAzureSubscriptionId` function in `Validate` function of `update` command to ensure valid Azure subscription ID before updating environment ([link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-5666f913c4913108ef57c2f23dec3f4b6a4fc6bc653f2b0c51982b9619fb6e12L140-R140)) * Add test case to check error when Azure subscription ID flag value is invalid in `Test_Validate` function of `update_test` file ([link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-c8b9a3315a934ca0d8e0b750b2872603b1c504239f5d310251e87e5fb6a8e4a2L68-R78)) * Modify existing test case to use valid Azure subscription ID flag value in `Test_Validate` function of `update_test` file ([link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-c8b9a3315a934ca0d8e0b750b2872603b1c504239f5d310251e87e5fb6a8e4a2L78-R87))
nithyatsu
referenced
this issue
Jun 21, 2023
# Description Today we accept any string for `--azure-subscription-id`, which can be confusing if a user tries to use a subscription name instead of a subscription ID. We also don't have further downstream checks so this results in a really bad error for users when they do this. This PR adds CLI validation to make sure the supplied value is a valid UUID (_but doesn't check if it's a valid Azure subscription, that'll come later_). There is still work to do to add additional validation, which will come as part of a future PR ## Issue reference <!-- We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation. --> Partially addresses #5757 ## Checklist Please make sure you've completed the relevant tasks for this PR, out of the following list: * [x] Code compiles correctly * [x] Adds necessary unit tests for change * [x] Adds necessary E2E tests for change * [x] Unit tests passing * [x] Extended the documentation / Created issue for it ## Auto-generated summary <!-- GitHub Copilot for docs will auto-generate a summary of the PR --> <!-- copilot:all --> ### <samp>🤖 Generated by Copilot at 6876cc0</samp> ### Summary 🔄🆕✅ <!-- 1. 🔄 - This emoji represents the change in the `Validate` function to use the `RequireAzureSubscriptionId` function instead of directly getting the flag value. 2. 🆕 - This emoji represents the addition of the new function `RequireAzureSubscriptionId` to the `cli` package to validate Azure subscription ID flags for commands. 3. ✅ - This emoji represents the addition and update of test cases for Azure subscription ID validation in `update_test`. --> Added a new function to validate Azure subscription ID flags for `env update` and other commands. Updated tests to cover the validation logic. > _Sing, O Muse, of the skillful code review_ > _That brought new order to the update command_ > _And made it heed the azure flag, the true_ > _And valid sign of the subscription's hand._ ### Walkthrough * Import and use `uuid.Parse` and `AzureSubscriptionIdFlag` to validate Azure subscription ID flag value in `RequireAzureSubscriptionId` function ([link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-bf4e917446c73565ee6967f28bdd8e619c0d4fa40b215f195b840c021dd36ce9L25-R28), [link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-bf4e917446c73565ee6967f28bdd8e619c0d4fa40b215f195b840c021dd36ce9R264-R278)) * Use `RequireAzureSubscriptionId` function in `Validate` function of `update` command to ensure valid Azure subscription ID before updating environment ([link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-5666f913c4913108ef57c2f23dec3f4b6a4fc6bc653f2b0c51982b9619fb6e12L140-R140)) * Add test case to check error when Azure subscription ID flag value is invalid in `Test_Validate` function of `update_test` file ([link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-c8b9a3315a934ca0d8e0b750b2872603b1c504239f5d310251e87e5fb6a8e4a2L68-R78)) * Modify existing test case to use valid Azure subscription ID flag value in `Test_Validate` function of `update_test` file ([link](https://github.com/project-radius/radius/pull/5758/files?diff=unified&w=0#diff-c8b9a3315a934ca0d8e0b750b2872603b1c504239f5d310251e87e5fb6a8e4a2L78-R87))
/assign |
Hello @jasonviviano - Have checked the scenario, error is not triggered in rad version - 0.39.0. have attached screenshot for reference. |
@ckansara16 - Thanks for validating! We can resolve the issue. cc - @willtsai |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Bug information
Steps to reproduce (required)
rad env update default --azure-subscription-id asdfadsfasdfas --azure-resource-group <resource-group>
Observed behavior (required)
Error displayed in terminal:
Error displayed inside of the Kubernetes deployment pod:
Screenshot:
Desired behavior (required)
Better error output or subscription GUID checking
Workaround (optional)
N/A
System information
rad Version (required)
Operating system (required)
GitHub Codespace
Additional context
N/A
AB#8309
AB#9507
AB#13539
The text was updated successfully, but these errors were encountered: