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

Upgrade TF azurerm v2 and NatGateway pubIP migration #192

Merged
merged 1 commit into from Nov 23, 2020

Conversation

dkistner
Copy link
Member

@dkistner dkistner commented Nov 16, 2020

How to categorize this PR?

/area open-source
/kind technical-debt
/priority normal
/platform azure

What this PR does / why we need it:
This PR contains migration logic to switch the azurerm Terraform provider version from v1.x.x to version v2.x.x (major version change).

In detail the following changes in the Terraform are required:

  • Add features field to the Terraform manifest
  • Remove the route table and network security group references from the azurerm_subnet resource. There are meanwhile associations resources in place, but we need to keep the resource references before we can migrate. Before this migration can be applied all Shoots Infrastructure need to be reconciled by a Gardener Azure extension which contains this PR: Prepare upgrade to Terraform azurerm v2.x.x #161 (shipped with v1.14.0)
  • The azurerm_subnet resource field address_prefix has been moved to address_prefixes
  • Added the migration steps for the Gardener managed NAT IP proposed by @ialidzhikov. The TODO(natipmigration) changes can be removed in future release when all Shoot Infrastructure are reconciled with this change. These changes are required to unblock also NatGateway integration – step 2 #54

This PR can only be merged when a new Terraformer with this PR gardener/terraformer#54 is in.

Special notes for your reviewer:
Create an Infrastructure with NatGateway (with an Azure extension not including this PR and an azurerm tf provider of version v1.x.x). Then reconcile the existing Infrastructure an Azure extension which contains these changes.

This Terraformer image can be used for testing: dominickistner/terraformer:azurerm-2360

Release note:

The Gardener Azure provider extension is now using the `azurerm` Terraform provider in version `v2.x.x`
Operators need to ensure that all Azure Shoot `Infrastructure` resource are reconciled with the Gardener Azure provider extension `v1.14.0` (contains a PR which is a prerequisite for this Terraform migration https://github.com/gardener/gardener-extension-provider-azure/pull/161) before applying this version. Please have a special look on hibernated clusters as their `Infrastructure` is might not reconciled for a while.
Terraformer uses now the azurerm provider in version v2.36.0

/invite @ialidzhikov
/invite @kon-angelo

@gardener-robot gardener-robot added kind/api-change API change with impact on API users needs/second-opinion Needs second review by someone else labels Nov 16, 2020
@gardener-robot gardener-robot added area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly needs/review Needs review platform/azure Microsoft Azure platform/infrastructure priority/normal size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Nov 16, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 16, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 16, 2020
@dkistner
Copy link
Member Author

@ialidzhikov Could you have a look? I will remove the draft of this pr gardener/terraformer#54 when we have reviewed this one here.

@dkistner
Copy link
Member Author

dkistner commented Nov 16, 2020

toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit

CI/Concourse seems to pull to much from dockerhub

@ialidzhikov ialidzhikov added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 17, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 17, 2020
Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

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

I played with it locally and the migration worked well. Looks good generally, thank you! Minor nits inline.

Before this migration can be applied all Shoots Infrastructure need to be reconciled by a Gardener Azure extension which contains this PR: #161 (shipped with v1.14.0)

There is one corner case that Infrastructures are no longer reconciled for hibernated Shoot - see gardener/gardener#2258. So technically there might be an Infrastructure of hibernated Shoot that is not reconciled by provider-azure@v1.14.0.

pkg/internal/infrastructure/terraform.go Outdated Show resolved Hide resolved
pkg/internal/infrastructure/terraform.go Outdated Show resolved Hide resolved
pkg/internal/infrastructure/terraform.go Outdated Show resolved Hide resolved
@gardener-robot gardener-robot removed the needs/review Needs review label Nov 17, 2020
@ialidzhikov
Copy link
Member

We can also run the infrastructure integration test when there is a release of terraformer. Or alternatively you can also update chart/images.yaml with terraformer v1.5.0-dev-b85b19c9d8ce07882aa1b382e26e4560e04b67cc (I am not sure whether the component descriptor will like this)?

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 18, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 18, 2020
@dkistner
Copy link
Member Author

I run the infra integration tests locally with my custom terraformer dominickistner/terraformer:azurerm-2360 before I opened the pr. Those succeed.
Btw: Addressed also the requested changes. PTAL.

@ialidzhikov
Copy link
Member

/lgtm

Before this migration can be applied all Shoots Infrastructure need to be reconciled by a Gardener Azure extension which contains this PR: #161 (shipped with v1.14.0)

There is one corner case that Infrastructures are no longer reconciled for hibernated Shoot - see gardener/gardener#2258. So technically there might be an Infrastructure of hibernated Shoot that is not reconciled by provider-azure@v1.14.0.

@dkistner , what is our last agreement about this? Manual detection of such clusters and manual trigger for reconcile? If that is the case, will you take care about this?

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/second-opinion Needs second review by someone else labels Nov 19, 2020
@gardener-robot gardener-robot added needs/second-opinion Needs second review by someone else and removed reviewed/lgtm Has approval for merging labels Nov 23, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 23, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 23, 2020
kon-angelo
kon-angelo previously approved these changes Nov 23, 2020
@kon-angelo
Copy link
Contributor

docker's rate limit strikes again

@ialidzhikov
Copy link
Member

docker's rate limit strikes again

Let's switch the base image to eu.gcr.io/gardener-project/3rd/golang:1.15.3. With the g/g@v1.13 vendoring, we will have gardener/gardener#3189 in place.

@ialidzhikov
Copy link
Member

/test-single

@testmachinery
Copy link

testmachinery bot commented Nov 23, 2020

Testrun: e2e-5j4c2
Workflow: e2e-5j4c2-wf
Phase: Succeeded

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| infrastructure-test | infrastructure-test | Succeeded | 17m45s   |
+---------------------+---------------------+-----------+----------+

@dkistner dkistner marked this pull request as ready for review November 23, 2020 14:08
@dkistner dkistner requested review from a team as code owners November 23, 2020 14:08
@ialidzhikov
Copy link
Member

/needs rebase

@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Nov 23, 2020
@gardener-robot gardener-robot removed the needs/rebase Needs git rebase label Nov 23, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 23, 2020
Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/second-opinion Needs second review by someone else labels Nov 23, 2020
Copy link
Contributor

@kon-angelo kon-angelo left a comment

Choose a reason for hiding this comment

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

/lgtm

@ialidzhikov ialidzhikov merged commit 113bc09 into gardener:master Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related kind/api-change API change with impact on API users kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) platform/azure Microsoft Azure platform/infrastructure reviewed/lgtm Has approval for merging size/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants