mattmcspirit / azurestack

Azure Stack Resources
80 stars 41 forks source link

Validate Azure AD credentials at start (offline download method) #65

Closed gijs007 closed 5 years ago

gijs007 commented 5 years ago

I recently ran the script with the offline download method and noticed that the script failed after the extraction part. The issue was that I used the wrong Azure AD credentials.

I also noticed that a lot of validation steps are executed when the script starts. It would be great if the Azure AD login credentials are also validated at the start, so that these can be corrected immediately if they are incorrect.

mattmcspirit commented 5 years ago

Hey - I've looked into this, and it's not possible for me to move the login validation earlier. The reason is, to validate the Azure AD credentials, I need the PowerShell modules installed. I've just tested on a freshly deployed ASDK host (I haven't run my script on this yet) and Login-AzureRmAccount, Add-AzureRmAccount etc, aren't there, so i can't test the accounts earlier in the process as I need to install PowerShell first. In an online deployment, this involves downloading and installing PowerShell directly, and in a deployment where you provide the .ZIP file, it installs from the extracted ZIP, which takes a few minutes also.

Many of the validation steps you mention, are there to ensure that things are less likely to fail later in the script, and some of them take only seconds to perform - the longest steps in the initial stage are focused on installing PowerShell, which I can't get around, and installing the SqlLocalDB to track progress. Both of these only take a few mins, and only run once, so even if you have the wrong login and it fails, the next time you run, you'll be at the login validation step almost immediately.

Good feedback though, and if I can change this in the future, I will, but for now, I'll close it out.

gijs007 commented 5 years ago

I get your point. However when we register to Azure we are doing a partly online installation. Hence you could opt to download the modules required to check this ;)

mattmcspirit commented 5 years ago

The most important step of all of the validation, is the SQL DB install, which tracks progress. If I do anything before this step, I can't track it in the DB as it hasn't been created, and so if a step like installing PowerShell is successful, every time someone runs the script, it's going to try to install again over the top, and I've seen unpredictable behavior when that happens. It doesn't matter whether the modules are coming from the Internet, or the ZIP, for the best reliability (that I've tested so far, and I've done MANY test runs!) it currently works best where it is I'm afraid.

With the script, I've got to solve for the majority I'm afraid - you're more than welcome to fork the repo and move things around in the script, and then use your version if you prefer - that's the beauty of GitHub!

gijs007 commented 5 years ago

If you say that doesn't work well, then I'll tale your word for it :)

I'm curious though, is the SQL DB install for progress executed after the extraction of the .zip file? The main thing is the Azure Ad credentials are currently checked after the extraction of the .zip file. The extraction can take some time, hence why it's better to check the credentials before it (as soon as possible in the process).

mattmcspirit commented 5 years ago

The ZIP contains the SqlLocalDB MSI installer, so is installed and configured after the ZIP is extracted. Admittedly, this takes time, but its only performed once.

Let's say the ZIP, SQL and PS installations take a total of 10 mins. If you have the wrong password, you find out after 10 mins. When you rerun, all of those steps are not re-performed, so you find out after less than 1 minute.

Contrast that with what you're proposing - you check the password after installing Azure RM PowerShell, which lets say, takes 2 mins total - so you find out your password is wrong after 2 mins. You then rerun with the correct password, and after 10-12 mins, you're still at the same point you would have been earlier, with the ZIP extracted, SQL installed, and PS installed.

I understand you're gaining a convenience to know that you're using the correct password to begin with, but it may be too risky to implement that at an earlier stage in the process, based on everything else that needs to happen in the early stages.

gijs007 commented 5 years ago

Agreed it doesn't make much sense as long as there is a risk that rerunning the script fails.

The only thing I can think of is to extract only the files needed for the SQLlocalDB and the modules. Afterwards the script can extract all the other files (ISO, appservices, etc). Not sure if this makes sense or if it's worth the effort though.

mattmcspirit commented 5 years ago

PowerShell's Expand-Archive command is an all or nothing I'm afraid, I can't just extract select files - I'd need a separate zip file just for PowerShell I'm afraid, and then it just gets more complicated to maintain and manage.

I'll investigate for future, but for now, I'll leave it as it is.

gijs007 commented 5 years ago

After doing a quick Google I came across a Stackoverflow question, regarding extracting single files with PowerShell: https://stackoverflow.com/questions/32526175/extract-a-certain-file-from-zip-via-powershell-seems-like-not-to-look-in-sub-fol

Apparently NetFramework 4.5 and newer versions have a native feature which supports extracting a single file: https://docs.microsoft.com/en-us/dotnet/api/system.io.compression.zipfileextensions.extracttofile?redirectedfrom=MSDN&view=netframework-4.7.2#System_IO_Compression_ZipFileExtensions_ExtractToFile_System_IO_Compression_ZipArchiveEntry_System_String_System_Boolean_

Perhaps it's possible to use the NetFramework feature in PowerShell?

mattmcspirit commented 5 years ago

You really are keen to have me do the login check earlier :)

Perhaps as an interim step, I'll ask people to open a browser and validate their login details before running the script, after all, providing the wrong credentials is a PEBKAC issue :-)

I'll investigate for future - it would require quite a lot of validation work and re-testing unfortunately.

gijs007 commented 5 years ago

I simply see it as a thinking out of the box exercise ;)

To be fair it's not a priority "issue", just a suggestion for improvement. If it takes a lot of work, it's not worth making this change.

mattmcspirit commented 5 years ago

Agreed - and I'll only know how much work, when I start the work... :-)