Added the NLBTask For Adding And Removing VMs from Load Balancer #3086
Conversation
| @@ -0,0 +1,92 @@ | |||
| { | |||
| "id": "e94f1750-a6a8-11e6-be69-bdf37a7b15d8", | |||
| "name": "NLBTask", | |||
There was a problem hiding this comment.
Change name to AzureNLBManagement
| { | ||
| "id": "e94f1750-a6a8-11e6-be69-bdf37a7b15d8", | ||
| "name": "NLBTask", | ||
| "friendlyName": "NetworkLoadBalancer Task", |
| "id": "e94f1750-a6a8-11e6-be69-bdf37a7b15d8", | ||
| "name": "NLBTask", | ||
| "friendlyName": "NetworkLoadBalancer Task", | ||
| "description": "Add/Remove a virtual machine's nic to the Load Balancer", |
There was a problem hiding this comment.
Add/Remove a Azure virtual machine's network interface to Load Balancer
| "friendlyName": "NetworkLoadBalancer Task", | ||
| "description": "Add/Remove a virtual machine's nic to the Load Balancer", | ||
| "author": "Microsoft Corporation", | ||
| "helpMarkDown": "Replace with markdown to show in help", |
There was a problem hiding this comment.
Update the help Mark down properly. Give a Fwd Link.
| "category": "Utility", | ||
| "visibility": [ | ||
| "Build", | ||
| "Release" |
| "messages": { | ||
| "CouldNotFetchNicDetails": "Could Not Fetch NIC Configuration details for the VM : %s", | ||
| "NicDetailsFetched": "NIC Configuration Details obtained for VM : %s", | ||
| "GettingAllNetworkInterfacesInTheResourceGroup": "Getting All Network Interfaces In The Resource Group : %s", |
There was a problem hiding this comment.
The messages start with Caps for every word. Have the user facing messages corrected with appropriate casing.
| "messages": { | ||
| "CouldNotFetchNicDetails": "Could Not Fetch NIC Configuration details for the VM : %s", | ||
| "NicDetailsFetched": "NIC Configuration Details obtained for VM : %s", | ||
| "GettingAllNetworkInterfacesInTheResourceGroup": "Getting All Network Interfaces In The Resource Group : %s", |
There was a problem hiding this comment.
The messages start with Caps for every word. Have the user facing messages corrected with appropriate casing. Do it for all relevant messages here.
| if (operation == "Add") { | ||
| tl._writeLine("Adding VM to LB"); | ||
| var lb = await nlbUtility.getLoadBalancer(SPN, endpointUrl, resourceGroupName, loadBalancerName); | ||
| obj = lb.properties.backendAddressPools; |
There was a problem hiding this comment.
Don't call it obj. Name the variable meaningfully.
| obj = lb.properties.backendAddressPools; | ||
| } | ||
| else { | ||
| tl._writeLine("Removing VM from LB"); |
There was a problem hiding this comment.
Again, if this is user facing message, this should come from resource in task.json. Modify the message similar to the comment given above for Add.
| return ipv4Address; | ||
| } | ||
|
|
||
| async function getNetworkInterface(SPN, endpointUrl: string, resourceGroupName: string) { |
There was a problem hiding this comment.
This logic won't scale if a VM has multiple IPv4 valid NIC cards. Would it be better to list the NICs for selected resource group in drop down and let user pick which NIC he needs to be added or removed?
| deferred.resolve(JSON.parse(body).access_token); | ||
| } | ||
| else { | ||
| deferred.reject("Could not fetch access token. Request ended with status code : " + response.statusCode); |
There was a problem hiding this comment.
Can u print response.statusMessage too ?
| export async function getNetworkInterfaces(SPN, endpointUrl: string, resourceGroupName: string) { | ||
|
|
||
| var deferred = Q.defer<any>(); | ||
| var restUrl = "https://management.azure.com/subscriptions/" + SPN.subscriptionId + "/resourceGroups/" + resourceGroupName + "/providers/Microsoft.Network/networkInterfaces?api-version=" + azureApiVersion; |
There was a problem hiding this comment.
Azure Rest call may provide nextLink attribute, which then need to be queried to get the actual result.
| export async function getLoadBalancer(SPN, endpointUrl: string, resourceGroupName: string, name: string) { | ||
|
|
||
| var deferred = Q.defer<any>(); | ||
| var restUrl = "https://management.azure.com/subscriptions/" + SPN.subscriptionId + "/resourceGroups/" + resourceGroupName + "/providers/Microsoft.Network/loadBalancers/" + name + "?api-version=" + azureApiVersion; |
There was a problem hiding this comment.
Please check for Azure Rest Call , NextLink attribute.
| "friendlyName": "Azure Network Load Balancer (Preview)", | ||
| "description": "Connect/Disconnect an Azure virtual machine's network interface to a Load Balancer's backend address pool", | ||
| "author": "Microsoft Corporation", | ||
| "helpMarkDown": "Replace with markdown to show in help", |
| "Patch": "0" | ||
| }, | ||
| "minimumAgentVersion": "1.95.0", | ||
| "instanceNameFormat": "Azure Network Load Balancer: $(Action) VMs from $(LoadBalancer)", |
There was a problem hiding this comment.
Check with PM on the resource here. This should be meaningful for both connect and disconnect actions.
| "required": true, | ||
| "options": { | ||
| "AutoDetectNic": "Auto select primary network interface", | ||
| "CustomNic": "Custom - virtual machine : network interface" |
There was a problem hiding this comment.
Is Custom just good enough. Do we need the mapping format in this drop down label?
| "Disconnect": "Disconnect Network Interface", | ||
| "Connect": "Connect Network Interface" | ||
| }, | ||
| "helpMarkDown": "Connect: Adds the virtual machine’s network interface to load balancer backend pool. So that it starts receiving network traffic based on the load balancing rules for the load balancer resource.\n\nDisconnect: Removes the virtual machine’s network interface from the load balancer’s backend pool. So that it stops receiving network traffic." |
There was a problem hiding this comment.
Can the help mark down for connect and Disconnect be in same order as the Options list.
| }, | ||
| "defaultValue": "AutoDetectNic", | ||
| "helpMarkDown": "Custom: Specify mapping virtual machine and the network interface associated with the virtual machine to be added/removed from load balancer backend pool. The format to specify is: `<virtual machine1>:<network interface name1> <virtual machine2>:<network interface name2>`… and so on.\n\nAuto select: Will select the primary network interface of the virtual machine to perform the Action of connecting/disconnecting from load balancer's backend address pool." | ||
| }, |
There was a problem hiding this comment.
Can autodetect be first in the help markdown. That being default option in drop down, let's match the order.
| tl.debug("Retrying after " + sleepTime/1000 + " sec"); | ||
| setTimeout(putNetworkInterface, sleepTime); | ||
| } | ||
| else { |
There was a problem hiding this comment.
Can you correct the formatting here.
| "Authorization": 'Bearer ' + accessToken | ||
| }; | ||
|
|
||
| tl._writeLine("Setting the network interface"); |
There was a problem hiding this comment.
If it is not a debug statement, this has to be localized.
| } | ||
| } | ||
| else { | ||
| // Handle custom vm:nic mapping |
There was a problem hiding this comment.
How is the VM name given in input helpful? We don't seem to make use of that input for ensuring the NIC belongs to the VM. Our logic is to enumerate nics and compare mac.
| else { | ||
| // Handle custom vm:nic mapping | ||
| var nicVmMap = {}; | ||
| var temp = vmNicMapping.split(" "); |
There was a problem hiding this comment.
You have to validate that vmNicMapping input is in right format. Any improper input with wrong : or wrong spaces can lead to failure below while you use the split out as index to arrays.
| @@ -0,0 +1,102 @@ | |||
| import * as tl from 'vsts-task-lib/task'; | |||
| var accessToken = await getAccessToken(SPN, endpointUrl); | ||
|
|
||
| var requestHeader = { | ||
| authorization: 'Bearer ' + accessToken |
There was a problem hiding this comment.
Since you are bypassing REST Client and using HTTP client, you are missing out filling the Accept header.
| var accessToken = await getAccessToken(SPN, endpointUrl); | ||
|
|
||
| var requestHeader = { | ||
| authorization: 'Bearer ' + accessToken |
There was a problem hiding this comment.
Since you are bypassing REST Client and using HTTP client, you are missing out filling the Accept header. This comment is applicable to all other places as well.
| var accessToken = await getAccessToken(SPN, endpointUrl); | ||
|
|
||
| var requestHeader = { | ||
| authorization: 'Bearer ' + accessToken |
There was a problem hiding this comment.
Since you are bypassing REST Client and using HTTP client, you are missing out filling the Accept header. This comment is applicable to all other places as well. If the issue of using REST client is only in SetNetworkInterface, why can't you use REST client in other places?
| var networkInterfaces = os.networkInterfaces(); | ||
| Object.keys(networkInterfaces).forEach( (interfaceName) => { | ||
| networkInterfaces[interfaceName].forEach( (interFace) => { | ||
| if (interFace.family !== 'IPv4' || interFace.internal !== false) { |
There was a problem hiding this comment.
Do we need IPV4 check anymore, given that you are depending on MAC.
No description provided.