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.

Sign up for GitHub

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

Estebanreyl/admin enabled fixes #341

Merged
merged 21 commits into from
Aug 11, 2018
Merged

Estebanreyl/admin enabled fixes #341

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
ad59f03
Fixes crashing of registry opening
Jul 30, 2018
8b5e95c
Eliminated ACR Admin enabled requirement and fixed this for appservic…
Jul 30, 2018
7a75c96
Fixed obscure bug that prevented proper showing of the location of ea…
Jul 31, 2018
949437b
Removed confusing methods for potential future use.
Jul 31, 2018
ce3e2e1
Small naming and style improvements
Jul 31, 2018
e58cfa7
Removed unused imports
Aug 1, 2018
803a8ef
Small style updates
Aug 1, 2018
f9b4bd1
Fixes crashing of registry opening
Jul 30, 2018
dc47180
Eliminated ACR Admin enabled requirement and fixed this for appservic…
Jul 30, 2018
06226e1
Fixed obscure bug that prevented proper showing of the location of ea…
Jul 31, 2018
3ebe79a
Removed confusing methods for potential future use.
Jul 31, 2018
39cc9cf
Small naming and style improvements
Jul 31, 2018
790cac6
Removed unused imports
Aug 1, 2018
0c2377d
Small style updates
Aug 1, 2018
2c67a8c
Merge branch 'estebanreyl/admin-enabled-fixes' of https://github.com/…
Aug 8, 2018
0596c30
Updated Error checking
Aug 8, 2018
b28431b
Fix error handling
Aug 9, 2018
253083d
user canceled
Aug 9, 2018
ff38b2c
Adds parseError
Aug 10, 2018
07a42cb
Merge remote-tracking branch 'upstream/master' into estebanreyl/admin…
Aug 10, 2018
ba24c82
added ext.outputchannel back
Aug 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 50 additions & 7 deletions explorer/deploy/webAppCreator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { ContainerRegistryManagementClient } from 'azure-arm-containerregistry';
import { Registry } from 'azure-arm-containerregistry/lib/models';
import { ResourceManagementClient, ResourceModels, SubscriptionModels } from 'azure-arm-resource';
import { Subscription } from 'azure-arm-resource/lib/subscription/models';
import WebSiteManagementClient = require('azure-arm-website');
import * as fs from 'fs';
import * as path from 'path';
import * as vscode from 'vscode';
import * as WebSiteModels from '../../node_modules/azure-arm-website/lib/models';
import { reporter } from '../../telemetry/telemetry';
Expand Down Expand Up @@ -172,9 +173,16 @@ class ResourceGroupStep extends WebAppCreatorStepBase {
resourceGroups = results[0];
locations = results[1];
resourceGroups.forEach(rg => {
let locationDisplayName: string;
try {
locationDisplayName = locations.find(l => l.name.toLowerCase() === rg.location.toLowerCase()).displayName;
} catch (error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What error is occurring? Null ref? I'd rather avoid exceptions than catch and recover.

Copy link
Author

Choose a reason for hiding this comment

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

Exactly, I could also store the result in a variable and check if it is available before assigning the value, say :
const location : Location = locations.find(l => l.name.toLowerCase() === rg.location.toLowerCase());

if(location){
locationDisplayName = location.displayName;
}else{
...
}

Alternatively I could try changing the location acquisition by subscription strategy and do so by overall locations which would prevent the error in the grand majority of the very rare cases although I suspect this would have problems in cases when there is a private azure location. Also, is there a specific reason as to prefer avoiding rather than catch and recover?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's preferable, thanks. Exceptions should be for things that are exceptional. Really you should never see a null ref exception, it's completely avoidable with proper checking. Having the location not match is a fully normal event that we just need to handle.

(Also, having a bunch of exceptions throw all over the place is darn annoying when you're trying to debug.)

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, I'll try to follow that paradigm.

//Less desirable formatting but allows this to work (Only occurs when a location is not part of the locations for a given subscription)
locationDisplayName = rg.location;
}
quickPickItems.push({
label: rg.name,
description: `(${locations.find(l => l.name.toLowerCase() === rg.location.toLowerCase()).displayName})`,
description: locationDisplayName,
detail: '',
data: rg
});
Expand Down Expand Up @@ -408,13 +416,23 @@ class WebsiteStep extends WebAppCreatorStepBase {
private _serverUserName: string;
private _serverPassword: string;
private _imageName: string;
private _imageSubscription: Subscription;
private _registry: Registry;

constructor(wizard: WizardBase, azureAccount: AzureAccountWrapper, context: AzureImageNode | DockerHubImageNode) {
super(wizard, 'Create Web App', azureAccount);

this._serverUrl = context.serverUrl;
this._serverPassword = context.password;
this._serverUserName = context.userName;
if (context instanceof DockerHubImageNode) {
this._serverPassword = context.password;
this._serverUserName = context.userName;
} else if (context instanceof AzureImageNode) {
this._imageSubscription = context.subscription;
this._registry = context.registry;
} else {
throw Error(`Invalid context, cannot deploy to Azure App services from ${context}`);
}

this._imageName = context.label;

}
Expand Down Expand Up @@ -447,6 +465,12 @@ class WebsiteStep extends WebAppCreatorStepBase {
await vscode.window.showWarningMessage(nameAvailability.message);
}
}
try {
await this.acquireRegistryLoginCredentials();
} catch (error) {
//Admin was not enabled, cannot proceed
throw new Error(('Admin not enabled'));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: double parens

Copy link
Contributor

Choose a reason for hiding this comment

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

You're making an assumption about why this failed - it could fail for any number of reasons, including network out - this message could mislead users.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should simply not have the try/catch here - let the exception bubble up (sync up with my PR I mentioned above).

}

let linuxFXVersion: string;
if (this._serverUrl.length > 0) {
Expand Down Expand Up @@ -476,7 +500,6 @@ class WebsiteStep extends WebAppCreatorStepBase {
const subscription = this.getSelectedSubscription();
const rg = this.getSelectedResourceGroup();
const websiteClient = new WebSiteManagementClient(this.azureAccount.getCredentialByTenantId(subscription.tenantId), subscription.subscriptionId);

// If the plan is also newly created, its resource ID won't be available at this step's prompt stage, but should be available now.
if (!this._website.serverFarmId) {
this._website.serverFarmId = this.getSelectedAppServicePlan().id;
Expand Down Expand Up @@ -505,7 +528,6 @@ class WebsiteStep extends WebAppCreatorStepBase {
"DOCKER_REGISTRY_SERVER_PASSWORD": this._serverPassword
}
};

}

await websiteClient.webApps.updateApplicationSettings(rg.name, this._website.name, appSettings);
Expand Down Expand Up @@ -543,4 +565,25 @@ class WebsiteStep extends WebAppCreatorStepBase {
}
}

//Implements new Service principal model for ACR container registries while maintaining old admin enabled use
private async acquireRegistryLoginCredentials(): Promise<void> {
if (this._serverPassword && this._serverUserName) { return; }

if (this._registry.adminUserEnabled) {
const client = new ContainerRegistryManagementClient(this.azureAccount.getCredentialByTenantId(this._imageSubscription.tenantId), this._imageSubscription.subscriptionId);
const resourceGroup: string = this._registry.id.slice(this._registry.id.search('resourceGroups/') + 'resourceGroups/'.length, this._registry.id.search('/providers/'));
try {
let creds = await client.registries.listCredentials(resourceGroup, this._registry.name);
Copy link

Choose a reason for hiding this comment

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

move this to _util file inside a new function listRegistryCredentials ?

Copy link
Author

Choose a reason for hiding this comment

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

While this makes sense to me conceptually I am honestly having trouble with how to go about it, I could create a function say in azureUtils that takes an azure account, a subscription, and a registry to do this and while this seems reasonable, and maybe I am biased because of PR #332 , it doesn't seem to follow the existing pattern of this file and seems more like something to add to my unified credentials that would look odd anywhere else. It seems most reasonable to me to add it to the Unified credentials branch and leave this like it is, at some point it may be good to refactor this code around that other branch however but that goes for the overall file,

Copy link

Choose a reason for hiding this comment

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

Sounds good, you can consider refactoring in the other branch. Also, you are right about the overall file in general.

this._serverPassword = creds.passwords[0].value;
this._serverUserName = creds.username;
return;
} catch (error) {
vscode.window.showErrorMessage(error.message);
Copy link

Choose a reason for hiding this comment

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

await?

Copy link
Author

@estebanreyl estebanreyl Aug 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In this case await pauses execution until the user clicks to exit the error message window which is not a guarantee, additionally it makes no difference as the error is shown when ready and execution ends nonetheless.

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need to both show a message and throw an error - the error should get displayed by the caller, is that not happening?

One issue is that this code was copied from elsewhere and we need to actually update it to use the shared code in https://github.com/Microsoft/vscode-azuretools, so maybe that needs to be done before integrating these changes.

Copy link
Author

Choose a reason for hiding this comment

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

This does not happen indeed, in fact there is no visible information about an error occurring if I propagate it all the way to the code I didn't write, although I could modify it to act in this way. Additionally, while I agree that we should update the original code for the whole deploy to app services, this PR is essentially an important bugfix which by the nature of the bug broke the existing app services functionality and thus required me to solve a small number of issues on that side to keep existing functionality post the PR. Would it not be best to integrate this functional code with the added bugfixes before attempting the necessary refactoring that appears to me as less pressing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Added #358 to track that we want telemetry for such failures.

BTW, with my merge of #350, the caller should now be catching and handling (telemetry and error messages) anything you throw from commands. That should now be the correct way to handle things (let exceptions bubble up), except for cases like there where you need to continue.

Copy link
Author

Choose a reason for hiding this comment

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

sweet, Ill update accordingly

throw new Error(('Azure Container Registry Admin is not enabled'));
}
}
vscode.window.showErrorMessage('Azure App service currently only supports running images from Azure Container Registries with admin enabled');
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, you can't make that assumption. Does the error that you get back in this scenario have a particular code that you can check for in order to throw a more specific error in this case?

Copy link
Author

@estebanreyl estebanreyl Aug 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I could check for whether the list failed because of admin not enabled specifically and then throw accordingly. As for the Azure App service does not support... etc I have to make the assumption as the only way to identify this error is to go into the azure portal and note that your web app fails due to admin not being enabled, no error is actually given to the user from the API as long as credentials are valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused - you're throwing this error specifically when you get an exception. Can you not check for the error code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just throw (use the longer message), don't show a message.

throw new Error(('Azure Container Registry Admin is not enabled'));
}

}
47 changes: 22 additions & 25 deletions explorer/models/azureRegistryNodes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,9 @@ export class AzureRegistryNode extends NodeBase {
this._azureAccount = azureAccount;
}

public password: string;
public registry: ContainerModels.Registry;
public subscription: SubscriptionModels.Subscription;
public type: RegistryType;
public userName: string;

public getTreeItem(): vscode.TreeItem {
return {
Expand Down Expand Up @@ -96,12 +94,10 @@ export class AzureRegistryNode extends NodeBase {
node = new AzureRepositoryNode(repositories[i], "azureRepositoryNode");
node.accessTokenARC = accessTokenARC;
node.azureAccount = element.azureAccount;
node.password = element.password;
node.refreshTokenARC = refreshTokenARC;
node.registry = element.registry;
node.repository = element.label;
node.subscription = element.subscription;
node.userName = element.userName;
repoNodes.push(node);
}
}
Expand All @@ -127,12 +123,10 @@ export class AzureRepositoryNode extends NodeBase {

public accessTokenARC: string;
public azureAccount: AzureAccount
public password: string;
public refreshTokenARC: string;
public registry: ContainerModels.Registry;
public repository: string;
public subscription: SubscriptionModels.Subscription;
public userName: string;

public getTreeItem(): vscode.TreeItem {
return {
Expand Down Expand Up @@ -202,23 +196,28 @@ export class AzureRepositoryNode extends NodeBase {
// tslint:disable-next-line:prefer-for-of // Grandfathered in
for (let i = 0; i < tags.length; i++) {
pool.addTask(async () => {
let data = await request.get('https://' + element.repository + '/v2/' + element.label + `/manifests/${tags[i]}`, {
auth: {
bearer: accessTokenARC
}
});

//Acquires each image's manifest to acquire build time.
let manifest = JSON.parse(data);
node = new AzureImageNode(`${element.label}:${tags[i]}`, 'azureImageNode');
node.azureAccount = element.azureAccount;
node.password = element.password;
node.registry = element.registry;
node.serverUrl = element.repository;
node.subscription = element.subscription;
node.userName = element.userName;
node.created = moment(new Date(JSON.parse(manifest.history[0].v1Compatibility).created)).fromNow();
imageNodes.push(node);
let data: string;
try {
data = await request.get('https://' + element.repository + '/v2/' + element.label + `/manifests/${tags[i]}`, {
auth: {
bearer: accessTokenARC
}
});
} catch (error) {
vscode.window.showErrorMessage(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

vscode.window.showErrorMessage(parseError(error).message);

}

if (data) {
//Acquires each image's manifest to acquire build time.
let manifest = JSON.parse(data);
node = new AzureImageNode(`${element.label}:${tags[i]}`, 'azureImageNode');
node.azureAccount = element.azureAccount;
node.registry = element.registry;
node.serverUrl = element.repository;
node.subscription = element.subscription;
node.created = moment(new Date(JSON.parse(manifest.history[0].v1Compatibility).created)).fromNow();
imageNodes.push(node);
}
});
}
await pool.runAll();
Expand All @@ -242,11 +241,9 @@ export class AzureImageNode extends NodeBase {

public azureAccount: AzureAccount
public created: string;
public password: string;
public registry: ContainerModels.Registry;
public serverUrl: string;
public subscription: SubscriptionModels.Subscription;
public userName: string;

public getTreeItem(): vscode.TreeItem {
let displayName: string = this.label;
Expand Down
24 changes: 12 additions & 12 deletions explorer/models/registryRootNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,43 +133,43 @@ export class RegistryRootNode extends NodeBase {
const subs: SubscriptionModels.Subscription[] = this.getFilteredSubscriptions();

const subPool = new AsyncPool(MAX_CONCURRENT_SUBSCRIPTON_REQUESTS);
let subsAndRegistries: { 'subscription': SubscriptionModels.Subscription, 'registries': ContainerModels.RegistryListResult, 'client': any }[] = [];
let subsAndRegistries: { 'subscription': SubscriptionModels.Subscription, 'registries': ContainerModels.RegistryListResult }[] = [];
//Acquire each subscription's data simultaneously
// tslint:disable-next-line:prefer-for-of // Grandfathered in
for (let i = 0; i < subs.length; i++) {
subPool.addTask(async () => {
const client = new ContainerRegistryManagement(this.getCredentialByTenantId(subs[i].tenantId), subs[i].subscriptionId);
subsAndRegistries.push({
'subscription': subs[i],
'registries': await client.registries.list(),
'client': client
});
let regs: ContainerModels.Registry[];
try {
regs = await client.registries.list();
subsAndRegistries.push({
'subscription': subs[i],
'registries': regs
});
} catch (error) {
vscode.window.showErrorMessage(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let caller show error.

Copy link
Contributor

Choose a reason for hiding this comment

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

and other places.

Let me know if the caller is not showing the error. There are further changes around command registration and error handling that I intend to make.

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean handle this within asyncpool? I could do that so its a single location for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it as is for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Except change to show parseError(error).message

}
});
}
await subPool.runAll();

const regPool = new AsyncPool(MAX_CONCURRENT_REQUESTS);
// tslint:disable-next-line:prefer-for-of // Grandfathered in
for (let i = 0; i < subsAndRegistries.length; i++) {
const client = subsAndRegistries[i].client;
const registries = subsAndRegistries[i].registries;
const subscription = subsAndRegistries[i].subscription;

//Go through the registries and add them to the async pool
// tslint:disable-next-line:prefer-for-of // Grandfathered in
for (let j = 0; j < registries.length; j++) {
if (registries[j].adminUserEnabled && !registries[j].sku.tier.includes('Classic')) {
const resourceGroup: string = registries[j].id.slice(registries[j].id.search('resourceGroups/') + 'resourceGroups/'.length, registries[j].id.search('/providers/'));
if (!registries[j].sku.tier.includes('Classic')) {
regPool.addTask(async () => {
let creds = await client.registries.listCredentials(resourceGroup, registries[j].name);
let iconPath = {
light: path.join(__filename, '..', '..', '..', '..', 'images', 'light', 'Registry_16x.svg'),
dark: path.join(__filename, '..', '..', '..', '..', 'images', 'dark', 'Registry_16x.svg')
};
let node = new AzureRegistryNode(registries[j].loginServer, 'azureRegistryNode', iconPath, this._azureAccount);
node.type = RegistryType.Azure;
node.password = creds.passwords[0].value;
node.userName = creds.username;
node.subscription = subscription;
node.registry = registries[j];
azureRegistryNodes.push(node);
Expand Down