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

azurerm_lb_backend_address_pool: support for backend_address #10291

Merged
merged 2 commits into from
Feb 4, 2021

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Jan 23, 2021

  • Support outbound_rules for both ds and resource
  • Support backend_address for both ds and resource
  • Use the backend address pool dedicated endpoint in data source and resource Read callback to make the code easier to maintain

Note that the backend address pool API is kind of confusing, for which I've submit Azure/azure-rest-api-specs#11234. However, after offline ping with service team, it turns out they implement Standard sku LB and Basic sku LB in quite different way. They are not probably gonna change current API behavior in turns of backend address pool. See my comment for more details.

Because of this fact, the implementation in this PR (mostly for CreateUpdate/Delete) will first check the LB sku, then call towards different endpoints accordingly.

Supersedes #8920

Test Result

💢 make testacc TEST=./azurerm/internal/services/loadbalancer TESTARGS='-run TestAccAzureRMLoadBalancerBackEndAddressPool_'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/loadbalancer -v -run TestAccAzureRMLoadBalancerBackEndAddressPool_ -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMLoadBalancerBackEndAddressPool_basicSkuBasic
=== PAUSE TestAccAzureRMLoadBalancerBackEndAddressPool_basicSkuBasic
=== RUN   TestAccAzureRMLoadBalancerBackEndAddressPool_standardSkuNIC
=== PAUSE TestAccAzureRMLoadBalancerBackEndAddressPool_standardSkuNIC
=== RUN   TestAccAzureRMLoadBalancerBackEndAddressPool_standardSkuIPBasic
=== PAUSE TestAccAzureRMLoadBalancerBackEndAddressPool_standardSkuIPBasic
=== RUN   TestAccAzureRMLoadBalancerBackEndAddressPool_standardSkuIPUpdate
=== PAUSE TestAccAzureRMLoadBalancerBackEndAddressPool_standardSkuIPUpdate
=== RUN   TestAccAzureRMLoadBalancerBackEndAddressPool_requiresImport
=== PAUSE TestAccAzureRMLoadBalancerBackEndAddressPool_requiresImport
=== RUN   TestAccAzureRMLoadBalancerBackEndAddressPool_BasicSkuRemoval
=== PAUSE TestAccAzureRMLoadBalancerBackEndAddressPool_BasicSkuRemoval
=== CONT  TestAccAzureRMLoadBalancerBackEndAddressPool_basicSkuBasic
=== CONT  TestAccAzureRMLoadBalancerBackEndAddressPool_requiresImport
=== CONT  TestAccAzureRMLoadBalancerBackEndAddressPool_standardSkuIPBasic
=== CONT  TestAccAzureRMLoadBalancerBackEndAddressPool_standardSkuIPUpdate
=== CONT  TestAccAzureRMLoadBalancerBackEndAddressPool_standardSkuNIC
=== CONT  TestAccAzureRMLoadBalancerBackEndAddressPool_BasicSkuRemoval
--- PASS: TestAccAzureRMLoadBalancerBackEndAddressPool_standardSkuNIC (204.34s)
--- PASS: TestAccAzureRMLoadBalancerBackEndAddressPool_standardSkuIPBasic (216.80s)
--- PASS: TestAccAzureRMLoadBalancerBackEndAddressPool_basicSkuBasic (266.50s)
--- PASS: TestAccAzureRMLoadBalancerBackEndAddressPool_requiresImport (283.75s)
--- PASS: TestAccAzureRMLoadBalancerBackEndAddressPool_BasicSkuRemoval (332.49s)
--- PASS: TestAccAzureRMLoadBalancerBackEndAddressPool_standardSkuIPUpdate (428.01s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/loadbalancer        428.057s

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Aside from a few minor comments this looks good

…s as is covered by `backend_ip_configurations`
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @magodo - LGTM 👍

@katbyte katbyte added this to the v2.46.0 milestone Feb 4, 2021
@katbyte katbyte merged commit 357fe2d into hashicorp:master Feb 4, 2021
katbyte added a commit that referenced this pull request Feb 4, 2021
@JRolfe-Gen
Copy link

JRolfe-Gen commented Feb 5, 2021

This broke my current configuration. I'm declaring the backend address pool

resource azurerm_lb_backend_address_pool inside_lb_backend {
  resource_group_name = azurerm_resource_group.rg.name
  loadbalancer_id     = azurerm_lb.inside_lb.id
  name                = "BackEnd"
}

and then later associated IP's with the pool after those are created.

resource azurerm_network_interface_backend_address_pool_association example {
  for_each                = toset(var.palo_nodes_be)
  network_interface_id    = lookup(azurerm_network_interface.inside[each.value], "id")
  ip_configuration_name   = "${var.root_name}-${var.location}-${var.environment}-${each.value}-inside"
  backend_address_pool_id = var.inside_lb_backend.id
}

and now terraform is trying to remove the IP's from inside_lb_backend
**** PLAN OUTPUT ****

# module.region.azurerm_lb_backend_address_pool.inside_lb_backend will be updated in-place
  ~ resource "azurerm_lb_backend_address_pool" "inside_lb_backend" {
        backend_ip_configurations = [
            "/subscriptions/<mysubid>/resourceGroups/<myrg>/providers/Microsoft.Network/networkInterfaces/<ip1>/ipConfigurations/ip1-1-inside",
            "/subscriptions/<mysubid>/resourceGroups/<myrg>/providers/Microsoft.Network/networkInterfaces/<ip2/ipConfigurations/ip2-2-inside",
        ]
        id                        = "/subscriptions/<mysubid>/resourceGroups/<myrg>/providers/Microsoft.Network/loadBalancers/<mylb>/backendAddressPools/BackEnd"
        load_balancing_rules      = [
            "/subscriptions/<mysubid>/resourceGroups/<myrg>/providers/Microsoft.Network/loadBalancers/<mylb>/loadBalancingRules/LBRule",
        ]
        loadbalancer_id           = "/subscriptions/<mysubid>/resourceGroups/<myrg>/providers/Microsoft.Network/loadBalancers/<mylb>"
        name                      = "BackEnd"
        outbound_rules            = []
        resource_group_name       = "<myrg>"

      - backend_address {
          - name = "addressid1" -> null
        }
      - backend_address {
          - name = "addressid2" -> null
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

****ERROR on EXECUTE ****
Error: creating/updating Load Balancer Backend Address Pool "Load Balancer Backend Address Pool: (Backend Address Pool Name "BackEnd" / Load Balancer Name "" / Resource Group "")": network.LoadBalancerBackendAddressPoolsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="ModificationOfNICIpConfigBackendPoolNotSupported" Message="Operation on backend pool /subscriptions//resourceGroups//providers/Microsoft.Network/loadBalancers//backendAddressPools/BackEnd not allowed since it adds/modifies/deletes backend address pool members that are specified with a network interface IP configuration." Details=[]

@etaham
Copy link

etaham commented Feb 5, 2021

I was in the middle of reporting the same issue - this seems to want to destroy azurerm_network_interface_backend_address_pool_association based pool members.

It looks like the Azure API returns the azurerm_network_interface_backend_address_pool_association nic's as members of the backendAddressPools array (according to an ARM export:

{
    "$schema": "https://schema.management.azure.com/schemas/2015-01-01/deploymentTemplate.json#",
    "contentVersion": "1.0.0.0",
    ...
       "resources": [
        {
            "type": "Microsoft.Network/loadBalancers",
            "apiVersion": "2020-05-01",
            "name": "name",
         ....
            "sku": {
                "name": "Standard"
            },
            "properties": {
                ...
                "backendAddressPools": [
                    {
                        "name": "bepool",
                        "properties": {
                            "loadBalancerBackendAddresses": [
                                {
                                    "name": "<resourceGroupName>_<NicName><IpConfigName>",
                                    "properties": {}
                                },
                                ...
                            ]
                        }
                    }
                ],
        ...
   ]
  ....
}

@Doctorbal
Copy link

This broke my current configuration. I'm declaring the backend address pool

resource azurerm_lb_backend_address_pool inside_lb_backend {
  resource_group_name = azurerm_resource_group.rg.name
  loadbalancer_id     = azurerm_lb.inside_lb.id
  name                = "BackEnd"
}

and then later associated IP's with the pool after those are created.

resource azurerm_network_interface_backend_address_pool_association example {
  for_each                = toset(var.palo_nodes_be)
  network_interface_id    = lookup(azurerm_network_interface.inside[each.value], "id")
  ip_configuration_name   = "${var.root_name}-${var.location}-${var.environment}-${each.value}-inside"
  backend_address_pool_id = var.inside_lb_backend.id
}

and now terraform is trying to remove the IP's from inside_lb_backend
**** PLAN OUTPUT ****

# module.region.azurerm_lb_backend_address_pool.inside_lb_backend will be updated in-place
  ~ resource "azurerm_lb_backend_address_pool" "inside_lb_backend" {
        backend_ip_configurations = [
            "/subscriptions/<mysubid>/resourceGroups/<myrg>/providers/Microsoft.Network/networkInterfaces/<ip1>/ipConfigurations/ip1-1-inside",
            "/subscriptions/<mysubid>/resourceGroups/<myrg>/providers/Microsoft.Network/networkInterfaces/<ip2/ipConfigurations/ip2-2-inside",
        ]
        id                        = "/subscriptions/<mysubid>/resourceGroups/<myrg>/providers/Microsoft.Network/loadBalancers/<mylb>/backendAddressPools/BackEnd"
        load_balancing_rules      = [
            "/subscriptions/<mysubid>/resourceGroups/<myrg>/providers/Microsoft.Network/loadBalancers/<mylb>/loadBalancingRules/LBRule",
        ]
        loadbalancer_id           = "/subscriptions/<mysubid>/resourceGroups/<myrg>/providers/Microsoft.Network/loadBalancers/<mylb>"
        name                      = "BackEnd"
        outbound_rules            = []
        resource_group_name       = "<myrg>"

      - backend_address {
          - name = "addressid1" -> null
        }
      - backend_address {
          - name = "addressid2" -> null
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

****ERROR on EXECUTE ****
Error: creating/updating Load Balancer Backend Address Pool "Load Balancer Backend Address Pool: (Backend Address Pool Name "BackEnd" / Load Balancer Name "" / Resource Group "")": network.LoadBalancerBackendAddressPoolsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="ModificationOfNICIpConfigBackendPoolNotSupported" Message="Operation on backend pool /subscriptions//resourceGroups//providers/Microsoft.Network/loadBalancers//backendAddressPools/BackEnd not allowed since it adds/modifies/deletes backend address pool members that are specified with a network interface IP configuration." Details=[]

I am having the exact same issue now.

@magodo
Copy link
Collaborator Author

magodo commented Feb 5, 2021

Hi all Thank you for reporting this and really sorry for introducing this regression... I've submit a quick fix for this in #10481.
For now, you'd better roll back to using the version v2.45.0 or before.

@ghost
Copy link

ghost commented Feb 5, 2021

This has been released in version 2.46.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.46.0"
}
# ... other configuration ...

katbyte pushed a commit that referenced this pull request Feb 5, 2021
…10481)

Fix the regression caused by #10291, that the associated NIC will be cleared by LB BAP
@magodo
Copy link
Collaborator Author

magodo commented Feb 5, 2021

Update: v2.46.1 will contains this fix.

@ghost
Copy link

ghost commented Mar 6, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Mar 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants