fix(provider/azure): Fix several issues in Azure Load Balancer (#3713)

spinnaker · May 23, 2019 · 2c7177e · 2c7177e
1 parent 06cb8ac
commit 2c7177e
Show file tree

Hide file tree

Showing 14 changed files with 132 additions and 58 deletions.
diff --git a/.../resources/appgateway/ops/converters/DeleteAzureAppGatewayAtomicOperationConverter.groovy b/.../resources/appgateway/ops/converters/DeleteAzureAppGatewayAtomicOperationConverter.groovy
@@ -20,6 +20,10 @@ import com.netflix.spinnaker.clouddriver.azure.AzureOperation
 import com.netflix.spinnaker.clouddriver.azure.common.AzureAtomicOperationConverterHelper
 import com.netflix.spinnaker.clouddriver.azure.resources.appgateway.model.AzureAppGatewayDescription
 import com.netflix.spinnaker.clouddriver.azure.resources.appgateway.ops.DeleteAzureAppGatewayAtomicOperation
+import com.netflix.spinnaker.clouddriver.azure.resources.loadbalancer.model.AzureLoadBalancer
+import com.netflix.spinnaker.clouddriver.azure.resources.loadbalancer.model.AzureLoadBalancerDescription
+import com.netflix.spinnaker.clouddriver.azure.resources.loadbalancer.model.DeleteAzureLoadBalancerDescription
+import com.netflix.spinnaker.clouddriver.azure.resources.loadbalancer.ops.DeleteAzureLoadBalancerAtomicOperation
 import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation
 import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperations
 import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsSupport
@@ -31,10 +35,18 @@ import org.springframework.stereotype.Component
 @Component("deleteAzureAppGatewayDescription")
 class DeleteAzureAppGatewayAtomicOperationConverter extends AbstractAtomicOperationsCredentialsSupport {
   AtomicOperation convertOperation(Map input) {
-    new DeleteAzureAppGatewayAtomicOperation(convertDescription(input))
+    if(input.get("loadBalancerType") == AzureLoadBalancer.AzureLoadBalancerType.AZURE_LOAD_BALANCER.toString()) {
+      new DeleteAzureLoadBalancerAtomicOperation(convertALBDescription(input))
+    } else {
+      new DeleteAzureAppGatewayAtomicOperation(convertDescription(input))
+    }
   }
 
   AzureAppGatewayDescription convertDescription(Map input) {
     AzureAtomicOperationConverterHelper.convertDescription(input, this, AzureAppGatewayDescription) as AzureAppGatewayDescription
   }
+
+  DeleteAzureLoadBalancerDescription convertALBDescription(Map input) {
+    AzureAtomicOperationConverterHelper.convertDescription(input, this, DeleteAzureLoadBalancerDescription) as DeleteAzureLoadBalancerDescription
+  }
 }
diff --git a/...naker/clouddriver/azure/resources/loadbalancer/cache/AzureLoadBalancerCachingAgent.groovy b/...naker/clouddriver/azure/resources/loadbalancer/cache/AzureLoadBalancerCachingAgent.groovy
@@ -189,6 +189,9 @@ class AzureLoadBalancerCachingAgent implements CachingAgent, OnDemandAgent, Acco
 
       loadBalancers.each { AzureLoadBalancerDescription item ->
         AzureLoadBalancerDescription loadBalancer = item
+        // Skip for internal type ALB (Which only serve for connection to VMSS instance)
+        if (loadBalancer.internal) return
+
         String lbKey = getLoadBalancerKey(loadBalancer)
 
         // Search the current OnDemand update map entries and look for a load balancer match

diff --git a/...nnaker/clouddriver/azure/resources/loadbalancer/model/AzureLoadBalancerDescription.groovy b/...nnaker/clouddriver/azure/resources/loadbalancer/model/AzureLoadBalancerDescription.groovy
@@ -34,8 +34,10 @@ class AzureLoadBalancerDescription extends AzureResourceOpsDescription {
   String dnsName
   String cluster
   List<String> serverGroups
+  String trafficEnabledSG
   String appName
   String sessionPersistence
+  boolean internal
   List<AzureLoadBalancerProbe> probes = []
   List<AzureLoadBalancingRule> loadBalancingRules = []
   List<AzureLoadBalancerInboundNATRule> inboundNATRules = []
@@ -92,6 +94,7 @@ class AzureLoadBalancerDescription extends AzureResourceOpsDescription {
     description.createdTime = azureLoadBalancer.tags?.createdTime?.toLong()
     description.tags.putAll(azureLoadBalancer.tags)
     description.region = azureLoadBalancer.location()
+    description.internal = azureLoadBalancer.tags?.internal != null
 
     // Each load balancer backend address pool corresponds to a server group (except the "default_LB_BAP")
     description.serverGroups = []
@@ -106,6 +109,7 @@ class AzureLoadBalancerDescription extends AzureResourceOpsDescription {
       r.probeName = AzureUtilities.getNameFromResourceId(rule?.probe()?.id()) ?: "not-assigned"
       r.persistence = rule.loadDistribution()
       r.idleTimeout = rule.idleTimeoutInMinutes()
+      description.trafficEnabledSG = AzureUtilities.getNameFromResourceId(rule.backendAddressPool().id())
 
       if (rule.protocol() == TransportProtocol.UDP) {
         r.protocol = AzureLoadBalancingRule.AzureLoadBalancingRulesType.UDP

diff --git a/.../clouddriver/azure/resources/loadbalancer/model/DeleteAzureLoadBalancerDescription.groovy b/.../clouddriver/azure/resources/loadbalancer/model/DeleteAzureLoadBalancerDescription.groovy
@@ -20,5 +20,5 @@ import com.netflix.spinnaker.clouddriver.azure.resources.common.AzureResourceOps
 
 class DeleteAzureLoadBalancerDescription extends AzureResourceOpsDescription {
   String loadBalancerName
-  List<String> regions
+  String region
 }
diff --git a/...louddriver/azure/resources/loadbalancer/ops/DeleteAzureLoadBalancerAtomicOperation.groovy b/...louddriver/azure/resources/loadbalancer/ops/DeleteAzureLoadBalancerAtomicOperation.groovy
@@ -42,27 +42,25 @@ class DeleteAzureLoadBalancerAtomicOperation implements AtomicOperation<Void> {
   @Override
   Void operate(List priorOutputs) {
     task.updateStatus(BASE_PHASE, "Initializing Delete Azure Load Balancer Operation...")
-    for (region in description.regions) {
-      task.updateStatus(BASE_PHASE, "Deleting ${description.loadBalancerName} " + "in ${region}...")
+    task.updateStatus(BASE_PHASE, "Deleting ${description.loadBalancerName}...")
 
-      if (!description.credentials) {
-        throw new IllegalArgumentException("Unable to resolve credentials for the selected Azure account.")
-      }
+    if (!description.credentials) {
+      throw new IllegalArgumentException("Unable to resolve credentials for the selected Azure account.")
+    }
 
-      try {
-        String resourceGroupName = AzureUtilities.getResourceGroupName(description.appName, region)
+    try {
+      String resourceGroupName = AzureUtilities.getResourceGroupName(description.appName, description.region)
 
-        description
-          .credentials
-          .networkClient
-          .deleteLoadBalancer(resourceGroupName, description.loadBalancerName)
+      description
+        .credentials
+        .networkClient
+        .deleteLoadBalancer(resourceGroupName, description.loadBalancerName)
 
-        // TODO: check response to ensure operation succeeded
-        task.updateStatus(BASE_PHASE, "Deletion of Azure load balancer ${description.loadBalancerName} in ${region} has succeeded.")
-      } catch (Exception e) {
-        task.updateStatus(BASE_PHASE, "Deletion of load balancer ${description.loadBalancerName} failed: e.message")
-        throw new AtomicOperationException("Failed to delete ${description.name}", [e.message])
-      }
+      // TODO: check response to ensure operation succeeded
+      task.updateStatus(BASE_PHASE, "Deletion of Azure load balancer ${description.loadBalancerName} in ${description.region} has succeeded.")
+    } catch (Exception e) {
+      task.updateStatus(BASE_PHASE, "Deletion of load balancer ${description.loadBalancerName} failed: e.message")
+      throw new AtomicOperationException("Failed to delete ${description.name}", [e.message])
     }
 
     null

diff --git a/...louddriver/azure/resources/loadbalancer/ops/UpsertAzureLoadBalancerAtomicOperation.groovy b/...louddriver/azure/resources/loadbalancer/ops/UpsertAzureLoadBalancerAtomicOperation.groovy
@@ -63,6 +63,7 @@ class UpsertAzureLoadBalancerAtomicOperation implements AtomicOperation<Map> {
 
       if(loadBalancerDescription) {
         description.serverGroups = loadBalancerDescription.serverGroups
+        description.trafficEnabledSG = loadBalancerDescription.trafficEnabledSG
       }
       Deployment deployment = description.credentials.resourceManagerClient.createResourceFromTemplate(
         AzureLoadBalancerResourceTemplate.getTemplate(description),

diff --git a/...pinnaker/clouddriver/azure/resources/servergroup/model/AzureServerGroupDescription.groovy b/...pinnaker/clouddriver/azure/resources/servergroup/model/AzureServerGroupDescription.groovy
@@ -168,8 +168,7 @@ class AzureServerGroupDescription extends AzureResourceOpsDescription implements
     azureSG.loadBalancerName = scaleSet.tags?.loadBalancerName
     azureSG.enableInboundNAT = scaleSet.tags?.enableInboundNAT
     azureSG.appGatewayName = scaleSet.tags?.appGatewayName
-    azureSG.loadBalancerName = scaleSet.tags?.loadBalancerName
-    azureSG.loadBalancerType = azureSG.loadBalancerName != null ? AzureLoadBalancer.AzureLoadBalancerType.AZURE_LOAD_BALANCER.toString() : AzureLoadBalancer.AzureLoadBalancerType.AZURE_APPLICATION_GATEWAY.toString()
+    azureSG.loadBalancerType = azureSG.appGatewayName != null ? AzureLoadBalancer.AzureLoadBalancerType.AZURE_APPLICATION_GATEWAY.toString() : AzureLoadBalancer.AzureLoadBalancerType.AZURE_LOAD_BALANCER.toString()
     azureSG.appGatewayBapId = scaleSet.tags?.appGatewayBapId
     // TODO: appGatewayBapId can be retrieved via scaleSet->networkProfile->networkInterfaceConfigurations->ipConfigurations->ApplicationGatewayBackendAddressPools
     azureSG.subnetId = scaleSet.tags?.subnetId

diff --git a/.../clouddriver/azure/resources/servergroup/ops/CreateAzureServerGroupAtomicOperation.groovy b/.../clouddriver/azure/resources/servergroup/ops/CreateAzureServerGroupAtomicOperation.groovy
@@ -87,6 +87,7 @@ class CreateAzureServerGroupAtomicOperation implements AtomicOperation<Map> {
       // TODO: replace appGatewayName with loadBalancerName
       if (!description.appGatewayName) {
         description.appGatewayName = description.loadBalancerName
+        description.loadBalancerName = null
       }
       def appGatewayDescription = description.credentials.networkClient.getAppGateway(resourceGroupName, description.appGatewayName)
 

diff --git a/...resources/servergroup/ops/CreateAzureServerGroupWithAzureLoadBalancerAtomicOperation.java b/...resources/servergroup/ops/CreateAzureServerGroupWithAzureLoadBalancerAtomicOperation.java
@@ -21,8 +21,10 @@
 import com.netflix.spinnaker.clouddriver.azure.common.AzureUtilities;
 import com.netflix.spinnaker.clouddriver.azure.resources.common.model.AzureDeploymentOperation;
 import com.netflix.spinnaker.clouddriver.azure.resources.common.model.KeyVaultSecret;
+import com.netflix.spinnaker.clouddriver.azure.resources.network.model.AzureVirtualNetworkDescription;
 import com.netflix.spinnaker.clouddriver.azure.resources.network.view.AzureNetworkProvider;
 import com.netflix.spinnaker.clouddriver.azure.resources.servergroup.model.AzureServerGroupDescription;
+import com.netflix.spinnaker.clouddriver.azure.resources.subnet.model.AzureSubnetDescription;
 import com.netflix.spinnaker.clouddriver.azure.templates.AzureServerGroupResourceTemplate;
 import com.netflix.spinnaker.clouddriver.data.task.Task;
 import com.netflix.spinnaker.clouddriver.data.task.TaskRepository;
@@ -67,10 +69,10 @@ public Map operate(List priorOutputs) {
     List<String> errList = new ArrayList<>();
     String resourceGroupName = null;
     String virtualNetworkName = null;
-    String subnetName = null;
     String serverGroupName = null;
     String loadBalancerPoolID = null;
     String inboundNatPoolID = null;
+    String subnetId = null;
 
     try {
 
@@ -116,7 +118,40 @@ public Map operate(List priorOutputs) {
       String loadBalancerName = description.getLoadBalancerName();
 
       virtualNetworkName = description.getVnet();
-      subnetName = description.getSubnet();
+      final String subnetName = description.getSubnet();
+
+      AzureVirtualNetworkDescription vnetDescription =
+          networkProvider.get(
+              description.getAccountName(),
+              description.getRegion(),
+              description.getVnetResourceGroup(),
+              virtualNetworkName);
+
+      if (vnetDescription == null) {
+        throw new RuntimeException(
+            "Selected virtual network " + virtualNetworkName + " does not exist");
+      }
+
+      List<AzureSubnetDescription> subnets = vnetDescription.getSubnets();
+
+      if (subnets == null || subnets.size() == 0) {
+        throw new RuntimeException(
+            "Cannot find any subnets in virtual network " + virtualNetworkName);
+      }
+
+      Optional<AzureSubnetDescription> filteredSubnet =
+          subnets.stream().filter(subnet -> subnet.getName().equals(subnetName)).findFirst();
+
+      if (!filteredSubnet.isPresent()) {
+        throw new RuntimeException(
+            "Selected subnet "
+                + subnetName
+                + " in virtual network "
+                + virtualNetworkName
+                + " is not valid");
+      }
+
+      subnetId = filteredSubnet.get().getResourceId();
 
       getTask()
           .updateStatus(
@@ -183,6 +218,7 @@ public Map operate(List priorOutputs) {
 
       Map<String, Object> templateParameters = new HashMap<>();
 
+      templateParameters.put(AzureServerGroupResourceTemplate.getSubnetParameterName(), subnetId);
       templateParameters.put(
           AzureServerGroupResourceTemplate.getAppGatewayAddressPoolParameterName(),
           loadBalancerPoolID);
@@ -194,7 +230,8 @@ public Map operate(List priorOutputs) {
               description.getCredentials().getDefaultResourceGroup(),
               description.getCredentials().getDefaultKeyVault()));
 
-      if (description.getCredentials().getUseSshPublicKey()) {
+      if (description.getCredentials().getUseSshPublicKey() != null
+          && description.getCredentials().getUseSshPublicKey()) {
         templateParameters.put(
             AzureServerGroupResourceTemplate.getVmSshPublicKeyParameterName(),
             new KeyVaultSecret(
@@ -230,6 +267,7 @@ public Map operate(List priorOutputs) {
 
       if (errList.isEmpty()) {
         getTask().updateStatus(BASE_PHASE, "Deploying server group");
+        serverGroupName = description.getName();
         Deployment deployment =
             description
                 .getCredentials()
@@ -249,7 +287,6 @@ public Map operate(List priorOutputs) {
                 description.getCredentials(),
                 resourceGroupName,
                 deployment.name()));
-        serverGroupName = errList.isEmpty() ? description.getName() : null;
       }
     } catch (Exception e) {
       getTask()

diff --git a/...clouddriver/azure/resources/servergroup/ops/DestroyAzureServerGroupAtomicOperation.groovy b/...clouddriver/azure/resources/servergroup/ops/DestroyAzureServerGroupAtomicOperation.groovy
@@ -18,6 +18,7 @@ package com.netflix.spinnaker.clouddriver.azure.resources.servergroup.ops
 
 import com.netflix.frigga.Names
 import com.netflix.spinnaker.clouddriver.azure.common.AzureUtilities
+import com.netflix.spinnaker.clouddriver.azure.resources.loadbalancer.model.AzureLoadBalancer
 import com.netflix.spinnaker.clouddriver.azure.resources.servergroup.model.AzureServerGroupDescription
 import com.netflix.spinnaker.clouddriver.data.task.Task
 import com.netflix.spinnaker.clouddriver.data.task.TaskRepository
@@ -78,12 +79,26 @@ class DestroyAzureServerGroupAtomicOperation implements AtomicOperation<Void> {
 
         // Clean-up the storrage account, load balancer and the subnet that where attached to the server group
         if (errList.isEmpty()) {
-          // Remove association between server group and the assigned application gateway backend address pool
-          task.updateStatus(BASE_PHASE, "Remove backend address pool in $description.appGatewayName")
-          description
-            .credentials
-            .networkClient
-            .removeAppGatewayBAPforServerGroup(resourceGroupName, serverGroupDescription.appGatewayName, serverGroupDescription.name)
+          if(serverGroupDescription.loadBalancerType == AzureLoadBalancer.AzureLoadBalancerType.AZURE_LOAD_BALANCER.toString()) {
+            task.updateStatus(BASE_PHASE, "Remove backend address pool in $description.loadBalancerName")
+            description
+              .credentials
+              .networkClient
+              .removeLoadBalancerAPforServerGroup(resourceGroupName, serverGroupDescription.loadBalancerName, serverGroupDescription.name)
+
+            task.updateStatus(BASE_PHASE, "Remove NAT pool in $description.loadBalancerName")
+            description
+              .credentials
+              .networkClient
+              .removeLoadBalancerNatPoolPortRangeforServerGroup(resourceGroupName, serverGroupDescription.loadBalancerName, serverGroupDescription.name)
+          }else {
+            // Remove association between server group and the assigned application gateway backend address pool
+            task.updateStatus(BASE_PHASE, "Remove backend address pool in $description.appGatewayName")
+            description
+              .credentials
+              .networkClient
+              .removeAppGatewayBAPforServerGroup(resourceGroupName, serverGroupDescription.appGatewayName, serverGroupDescription.name)
+          }
 
           // Delete storage accounts if any
           serverGroupDescription.storageAccountNames?.each { def storageAccountName ->
@@ -101,7 +116,22 @@ class DestroyAzureServerGroupAtomicOperation implements AtomicOperation<Void> {
             }
           }
 
+          // Delete load balancer attached to server group
+          if (serverGroupDescription.enableInboundNAT) {
+            String loadBalancerName = AzureUtilities.LB_NAME_PREFIX + serverGroupDescription.name
+            task.updateStatus(BASE_PHASE, "Deleting load balancer ${loadBalancerName} " + "in ${region}...")
+            try {
+              description
+                .credentials
+                .networkClient
+                .deleteLoadBalancer(resourceGroupName, loadBalancerName)
 
+              task.updateStatus(BASE_PHASE, "Deletion of Azure load balancer ${loadBalancerName} in ${region} has succeeded.")
+            } catch (Exception e) {
+              task.updateStatus(BASE_PHASE, "Deletion of Azure load balancer ${loadBalancerName} failed: ${e.message}")
+              errList.add("Failed to delete ${loadBalancerName}: ${e.message}")
+            }
+          }
 
           // Delete subnet attached to server group
           if (serverGroupDescription.hasNewSubnet && serverGroupDescription.subnetId) {

diff --git a/...om/netflix/spinnaker/clouddriver/azure/templates/AzureLoadBalancerResourceTemplate.groovy b/...om/netflix/spinnaker/clouddriver/azure/templates/AzureLoadBalancerResourceTemplate.groovy
@@ -78,7 +78,7 @@ class AzureLoadBalancerResourceTemplate {
       virtualNetworkName = AzureUtilities.VNET_NAME_PREFIX + resourceGroupName.toLowerCase()
       publicIPAddressName = AzureUtilities.PUBLICIP_NAME_PREFIX + description.loadBalancerName.toLowerCase()
       loadBalancerFrontEnd = AzureUtilities.LBFRONTEND_NAME_PREFIX + description.loadBalancerName.toLowerCase()
-      loadBalancerBackEnd = DEFAULT_BACKEND_POOL
+      loadBalancerBackEnd = description.trafficEnabledSG ? description.trafficEnabledSG : DEFAULT_BACKEND_POOL
       dnsNameForLBIP = DnsSettings.getUniqueDNSName(description.loadBalancerName.toLowerCase())
       ipConfigName = AzureUtilities.IPCONFIG_NAME_PREFIX + description.loadBalancerName.toLowerCase()
     }
@@ -177,7 +177,7 @@ class AzureLoadBalancerResourceTemplate {
 
     BackEndAddressPool()
     {
-      name = "[variables('loadBalancerBackEnd')]"
+      name = DEFAULT_BACKEND_POOL
     }
 
     BackEndAddressPool(String name)