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.

Sign up for GitHub

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

Jump to bottom

NatGateway integration – step 1 #50

Merged
merged 1 commit into from
Mar 17, 2020
Merged

NatGateway integration – step 1 #50

merged 1 commit into from
Mar 17, 2020

Conversation

dkistner
Copy link
Member

@dkistner dkistner commented Mar 11, 2020 •
edited
Loading

Azure NatGateway integration for zoned clusters only and without option to attach user provided public ip(s)/range(s) to the NatGateway.

✅ We need a terraformer version included which contain this pr: gardener/terraformer#35

ℹ️ This PR contains also a little boy scout rule actions for the terraform unit tests

What this PR does / why we need it:
see #43

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

Special notes for your reviewer:

Release note:

Gardener Azure provider extension support now Shoot cluster which have an Azure NatGateway attached to its worker subnet.

@dkistner dkistner added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Mar 11, 2020
@dkistner dkistner requested review from a team as code owners March 11, 2020 12:12
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) 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 Mar 11, 2020
@rfranzke
Copy link
Member

❌ Do not merge! We need a terraformer version included which contain this pr: gardener/terraformer#35

Have you validated that the new Azure TF plugin version doesn't break anything old? If yes, feel free to trigger a new release to unblock this PR :)

rfranzke
rfranzke requested changes Mar 12, 2020
View reviewed changes
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

I understand that it is possible to switch from "zoned w/o NAT GW <-> zoned w/ NAT GW" clusters and vice versa, right? Is this assumption correct and will it work properly (having existing zoned cluster w/o NAT GW in mind that might want to switch (and maybe later switch back)).

hack/api-reference/config.md Outdated Show resolved Hide resolved
pkg/apis/azure/v1alpha1/types_infrastructure.go Outdated Show resolved Hide resolved
docs/usage-as-end-user.md Outdated Show resolved Hide resolved
pkg/apis/azure/validation/infrastructure.go Outdated Show resolved Hide resolved
pkg/apis/azure/validation/infrastructure.go Outdated Show resolved Hide resolved
pkg/internal/infrastructure/terraform.go Outdated Show resolved Hide resolved
charts/internal/azure-infra/values.yaml Outdated Show resolved Hide resolved
charts/internal/azure-infra/templates/main.tf Show resolved Hide resolved
charts/internal/azure-infra/templates/main.tf Outdated Show resolved Hide resolved
charts/internal/azure-infra/templates/main.tf Outdated Show resolved Hide resolved
@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 Mar 16, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 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 Mar 16, 2020
@dkistner
Copy link
Member Author

dkistner commented Mar 16, 2020 •
edited
Loading

❌ Do not merge! We need a terraformer version included which contain this pr: gardener/terraformer#35

Have you validated that the new Azure TF plugin version doesn't break anything old? If yes, feel free to trigger a new release to unblock this PR :)

Yes, I have validated everything seems still working with the new azurerm Terraform version. I will trigger soon a new release of the Terraformer.

I understand that it is possible to switch from "zoned w/o NAT GW <-> zoned w/ NAT GW" clusters and vice versa, right? Is this assumption correct and will it work properly (having existing zoned cluster w/o NAT GW in mind that might want to switch (and maybe later switch back)).

Yep, exactly. You can turn on and off again the NatGateway for zoned clusters. Existing zoned cluster will stay untouched and use still the Standard LoadBalancer to route egress until a user decide to enable the NatGateway.

I also updated the PR. PTAL when the new Terraformer version is included.

rfranzke
rfranzke requested changes Mar 17, 2020
View reviewed changes
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

Looks great, just two minor nits in the documentation, then it's good to go in!

docs/usage-as-end-user.md Outdated Show resolved Hide resolved
pkg/apis/azure/v1alpha1/types_infrastructure.go Outdated Show resolved Hide resolved
@dkistner
NatGateway integration – step 1
051faf3 
Azure NatGateway integration for zoned clusters only and without option to attach user provided public ip(s)/range(s) to the NatGateway.
Furthermore the NatGateway is not yet deployed zone redundant.
@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 Mar 17, 2020
@dkistner
Copy link
Member Author

@rfranzke done. Thanks for the review. I have addressed the requested changes.

@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 Mar 17, 2020
@dkistner dkistner removed the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Mar 17, 2020
rfranzke
rfranzke approved these changes Mar 17, 2020
View reviewed changes
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

/lgtm

@dkistner dkistner merged commit 7b12651 into master Mar 17, 2020
@dkistner dkistner deleted the az-nat-step1 branch March 17, 2020 09:58
@dkistner dkistner mentioned this pull request Mar 18, 2020
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfranzke rfranzke rfranzke approved these changes

Assignees
No one assigned
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dkistner @rfranzke @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3