NatGateway integration – step 2

[dkistner]

Allow users to attach their own zonal public ip addresses to the NatGateway of a Shoot cluster.
Allow also to decide in which zone the NatGateway should be deployed to. User provided public ips needs to be zonal and in the same zone as the NatGateway.

Attaching Public IP Prefixes to the NatGateway will currently not be supported as there is no association resource to attach IP Prefixes to the NatGateway in the azurerm Terraform provider. Attaching IP Prefixes is currently only possible inline on the NatGateway resource, but as we were forced to write complicated migration logic for public ips when we moved to azurerm v2 Terraform provider, we don't want to repeat that for IP Prefixes.

❌ Do not merge until hashicorp/terraform-provider-azurerm#6052 is fixed and a new Terraformer is integrated that contain a version of the azurerm provider which incorporate the fix for the mentioned issue.

What this PR does / why we need it:
Part 2 of #43

Which issue(s) this PR fixes:
Fixes partly #43

Release note:

It is now possible to attach user provided public ips to the NatGateway of an Azure Shoot and to specify to which zone the NatGateway of the Azure Shoot should be deployed to. Only zonal public ips can be attached to a NatGateway and they need to be in the same zone like the NatGateway. You can find more information [here](https://github.com/gardener/gardener-extension-provider-azure/blob/master/docs/usage-as-end-user.md#infrastructureconfig).

Changing the zone NatGateway via `.spec.provider.infrastructureConfig.networks.natGateway.zone` while using a managed NatGateway public ip will lead to a recreation of the public ip and you will most likely end up with a different public ip. That mean egress connections will use then a different public ip as before. You can find more information [here](https://github.com/gardener/gardener-extension-provider-azure/blob/master/docs/usage-as-end-user.md#infrastructureconfig).

[rfranzke]

Looks good, a few minor suggestions. :)

[rfranzke]

Converted to a draft PR as its dependencies are not yet resolved. Please mark as 'ready for review' once they are resolved.

[rfranzke]

hashicorp/terraform-provider-azurerm#6052 seems now fixed as of hashicorp/terraform-provider-azurerm#6450.

[dkistner]

Nice! Let's wait until there is a new release of the azurerm terraform provider. I will try it out then and update the pr accordingly.

[dkistner]

Now there is an release which contain the fix https://github.com/terraform-providers/terraform-provider-azurerm/blob/v2.12.0/CHANGELOG.md#2120-may-28-2020
I will try out.

[dkistner]

So have tried it out. It does not work without introducing a new association resource for the public ip to nat gateway instead of using the internal public ip association of the nat gateway resource.

The migration from the internal association to association resource does not work, see here: hashicorp/terraform-provider-azurerm#7167
Seems it can be solved by importing the public ip resource to the Terraform state.

[ialidzhikov]

The migration from the internal association to association resource does not work, see here: terraform-providers/terraform-provider-azurerm#7167
Seems it can be solved by importing the public ip resource to the Terraform state.

I checked hashicorp/terraform-provider-azurerm#7167 and it is quite unfortunate.
The thing I am wondering is whether we can have

resource "azurerm_nat_gateway" "nat" {
  name                    = "{{ required "clusterName is required" .Values.clusterName }}-nat-gateway"
  location                = "{{ required "azure.region is required" .Values.azure.region }}"
  {{ if .Values.create.resourceGroup -}}
  resource_group_name     = azurerm_resource_group.rg.name
  {{- else -}}
  resource_group_name     = data.azurerm_resource_group.rg.name
  {{- end }}
  sku_name                = "Standard"
  {{ if .Values.natGateway.migrateToPublicIPAssociation -}}
  public_ip_address_ids   = []
  {{- end }}
}

resource "azurerm_nat_gateway_public_ip_association" "nat-ip-association" {
   nat_gateway_id       = "${azurerm_nat_gateway.nat.id}"
   public_ip_address_id = "${azurerm_public_ip.natip.id}"
}

I guess we could have in the Infrastructure status field like migratedToPublicIPAssociation that indicates whether the infra migrated to azurerm_nat_gateway_public_ip_association.
In the first apply run, the terraformer would run with migrateToPublicIPAssociation: true which will remove for a short the public IP addresses from the nat gateway (because of public_ip_address_ids = []) but then it will add them once again because of the azurerm_nat_gateway_public_ip_association resource. After successful run, we could set the status.migratedToPublicIPAssociation=true and then pass in the subsequent runs migrateToPublicIPAssociation=false to the terraformer chart.
I don't know exactly what is the complexity of the terraform import way but this approach sounds easier to implement? WDYT?

[dkistner]

@ialidzhikov I had the same idea and already implemented that almost exactly like you proposed. The issue here is that there will be a time during the first and the second reconciliation where no public ip is assigned to the NatGateway. In this period of time there is no ip assigned to the NatGateway and it can't be used to route egress traffic which require a stable ip. For some workloads this would mean a serious disruption.

Can we somehow influence that an infrastructure resource get reconciled multiple times even after the previous reconcile succeed? I didn't find a way to archive that. Therefore I rather thought in the direction of import support.

I think having a generic way to support Terraform imports for the Gardener extensions could also help in the future with other migration scenarios.

[ialidzhikov]

@ialidzhikov I had the same idea and already implemented that almost exactly like you proposed. The issue here is that there will be a time during the first and the second reconciliation where no public ip is assigned to the NatGateway. In this period of time there is no ip assigned to the NatGateway and it can't be used to route egress traffic which require a stable ip. For some workloads this would mean a serious disruption.

Hmm, why there would be 2 reconciliations. Setting public_ip_address_ids to [] and introduction of azurerm_nat_gateway_public_ip_association happen in the same infra reconciliation - one after other in the terraform execution. So the public ip should be removed and then associated again after few seconds.

1. This is what we have now:

resource "azurerm_public_ip" "natip" {
  name                = "nat-ip"
  location            = "westeurope"
  resource_group_name = "${azurerm_resource_group.rg.name}"
  allocation_method   = "Static"
  sku                 = "Standard"
}

resource "azurerm_nat_gateway" "nat" {
  name                    = "nat-gateway"
  location                = "westeurope"
  resource_group_name     = "${azurerm_resource_group.rg.name}"
  sku_name                = "Standard"
  public_ip_address_ids   = ["${azurerm_public_ip.natip.id}"]
}

2. This is how migration run looks like

resource "azurerm_public_ip" "natip" {
  name                = "nat-ip"
  location            = "westeurope"
  resource_group_name = "${azurerm_resource_group.rg.name}"
  allocation_method   = "Static"
  sku                 = "Standard"
}

resource "azurerm_nat_gateway" "nat" {
  name                    = "nat-gateway"
  location                = "westeurope"
  resource_group_name     = "${azurerm_resource_group.rg.name}"
  sku_name                = "Standard"
- public_ip_address_ids   = ["${azurerm_public_ip.natip.id}"]
+ public_ip_address_ids   = []
}

+resource "azurerm_nat_gateway_public_ip_association" "nat-ip-association" {
+  nat_gateway_id       = "${azurerm_nat_gateway.nat.id}"
+  public_ip_address_id = "${azurerm_public_ip.natip.id}"
+}

3. This is how a normal run after the migration looks like:

resource "azurerm_nat_gateway" "nat" {
  name                    = "nat-gateway"
  location                = "westeurope"
  resource_group_name     = "${azurerm_resource_group.rg.name}"
  sku_name                = "Standard"
- public_ip_address_ids   = []
}

resource "azurerm_nat_gateway_public_ip_association" "nat-ip-association" {
   nat_gateway_id       = "${azurerm_nat_gateway.nat.id}"
   public_ip_address_id = "${azurerm_public_ip.natip.id}"
}

[CLAassistant]

All committers have signed the CLA.

[dkistner]

Thanks @ialidzhikov
I have addressed the requested changes.

[ialidzhikov]

/test

[testmachinery]

Testrun: e2e-wsmbr
Workflow: e2e-wsmbr-wf
Phase: Succeeded

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| infrastructure-test | infrastructure-test | Succeeded | 25m19s   |
+---------------------+---------------------+-----------+----------+

[ialidzhikov]

/lgtm
/needs second-opinion

[dkistner]

@kon-angelo Thanks for the review. I have addressed the requested changes.

[kon-angelo]

/lgtm