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

[Serial-Console]: az serial-console connect: Change to use different region for url calls when custom storage account firewalls are enabled #5398

Merged
merged 19 commits into from
Oct 21, 2022

Conversation

rhkodiak
Copy link
Contributor


This changed introduces the ability when using custom storage accounts with the firewall enabled to use the region (i.e. westcentralUS) when connecting to serial-console. If the VIirtual Machine and custom storage account live in the same region then the user gets a Forbidden message when trying to connect to serial-console. These changes allow the code to find the pairing region and connect to serial-console through that region url.

Screenshot 2022-09-28 101831

.

This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

az serial-console connect

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
The precondition is to put your code inside this repository and upgrade the version in the pull request but do not modify src/index.json.

@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Sep 28, 2022
@ghost
Copy link

ghost commented Sep 28, 2022

Thank you for your contribution rhkodiak! We will review the pull request and get back to you soon.

@ghost ghost added the Auto-Assign Auto assign by bot label Sep 28, 2022
@ghost ghost requested review from evelyn-ys, calvinhzy and yonzhan September 28, 2022 15:21
@ghost ghost assigned evelyn-ys Sep 28, 2022
@ghost ghost added this to the Sep 2022 (2022-10-12) - For Ignite milestone Sep 28, 2022
@ghost ghost added the Storage label Sep 28, 2022
@ghost ghost requested a review from jiasli September 28, 2022 15:21
@ghost ghost assigned jiasli Sep 28, 2022
@ghost ghost added the Account label Sep 28, 2022
@yonzhan
Copy link
Collaborator

yonzhan commented Sep 28, 2022

Serial-Console

@jiasli jiasli removed their assignment Oct 6, 2022
@wangzelin007
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@brendank310
Copy link

@yonzhan: @rhkodiak has run the tests successfully locally, is there some environmental disparity that causes the CI checks to fail?

@yonzhan yonzhan requested a review from wangzelin007 October 13, 2022 14:53
@wangzelin007
Copy link
Member

@rhkodiak
@brendank310

https://dev.azure.com/azclitools/public/_build/results?buildId=8724&view=logs&j=59e8686e-5e74-514d-6cad-f7d66c66b425&t=59602323-6717-504d-f2cc-5c81369f935f&l=1003

According to the error log, it shows that we need use virtual machines == 2022-08-01,
But in your recording files virtual Machines version is 2022-03-01,

Latest profile in Azure CLI:
https://github.com/Azure/azure-cli/blob/c1c805f5680f27b17412acf6fd53228b50edb8b2/src/azure-cli-core/azure/cli/core/profiles/_shared.py#L153

Your recording file's virtual machines version:

uri: https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/cli_test_serialconsole000001/providers/Microsoft.Compute/virtualMachines/cli000003?$expand=instanceView&api-version=2022-03-01

Here are the suggestions:

  1. git pull the latest code from Azure/azure-cli
  2. azdev setup -c
  3. merge the latest code into your branch
  4. rerun all the tests and make sure API version is 2022-08-01

@rhkodiak
Copy link
Contributor Author

@wangzelin007
@brendank310

Thank you for the help! I applied what you suggested and the tests are passing now. I couldn’t tell from the error logs what the problem was so much appreciated.

The PR is ready for your review and approval.

@wangzelin007 wangzelin007 requested review from kairu-ms and removed request for jiasli and evelyn-ys October 18, 2022 01:17
@wangzelin007 wangzelin007 assigned kairu-ms and unassigned evelyn-ys Oct 18, 2022
error_message, recommendation=recommendation)
else:
if result.boot_diagnostics is not None:
print(result.boot_diagnostics)
Copy link
Member

Choose a reason for hiding this comment

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

Is this print for debug? Do you want to keep it in release version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That print was for debug purposes and I would like to keep it in the release version if possible. Is there a different way to implement that for debug purposes?

Copy link
Member

Choose a reason for hiding this comment

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

Please use logger.debug() then. This would be print only when --debug parameter specified

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the code to use the logger.debug() option instead of print

Copy link
Member

Choose a reason for hiding this comment

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

Please import logger from knack:

from knack.log import get_logger
logger = get_logger(__name__)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the missing import and kicked off the build again

Changed the print statement to use the logger.debug() option to only output the boot_diagnostics section for debugging
Added the logger import to correct the build failure
@evelyn-ys evelyn-ys merged commit ec69d77 into Azure:main Oct 21, 2022
@azclibot
Copy link
Collaborator

[Release] Update index.json for extension [ serial-console ] : https://dev.azure.com/azclitools/internal/_build/results?buildId=10355&view=results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Assign Auto assign by bot customer-reported Issues that are reported by GitHub users external to the Azure organization. Serial Console
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants