Skip to content
This repository has been archived by the owner on Apr 7, 2020. It is now read-only.

Azure: Allow to deploy cluster in existing vNets #371

Merged
merged 1 commit into from
Oct 24, 2019
Merged

Azure: Allow to deploy cluster in existing vNets #371

merged 1 commit into from
Oct 24, 2019

Conversation

dkistner
Copy link
Contributor

@dkistner dkistner commented Oct 15, 2019 •
edited by rfranzke
Loading

Allow to deploy Azure Shoot clusters in an existing vNet which exists in a different resource group.

‼️ [DONE] This PR need a new MCM release with this PR integrated: gardener/machine-controller-manager#344 -> #382

What this PR does / why we need it:
We need to support Azure cluster deployed in existing vnets.

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

Special notes for your reviewer:

Release note:

Gardener Azure provider supports now deploying clusters in existing vNets which are located in a different resource group than the Shoot resources.

@dkistner dkistner requested a review from a team as a code owner October 15, 2019 14:33
@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 Oct 15, 2019
@gardener-robot-ci-3 gardener-robot-ci-3 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 Oct 15, 2019
@dkistner
Copy link
Contributor Author

I opened this PR as work in progress to do some review upfront.
Currently some validation and test are missing.

@dkistner dkistner added the reviewed/do-not-merge Has no approval for merging, may not be merged as it may break things or be of poor quality label Oct 15, 2019
rfranzke
rfranzke reviewed Oct 16, 2019
View reviewed changes
Copy link
Contributor

@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 good overall, having a few minor suggestions.

...ers/provider-azure/charts/internal/cloud-provider-config/templates/cloud-provider-config.tpl Show resolved Hide resolved
controllers/provider-azure/pkg/internal/infrastructure/terraform.go Outdated Show resolved Hide resolved
controllers/provider-azure/charts/internal/azure-infra/values.yaml Outdated Show resolved Hide resolved
@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) 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 Oct 17, 2019
@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 Oct 18, 2019
@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 Oct 18, 2019
@dkistner
Copy link
Contributor Author

Tests and validations were added meanwhile.

@dkistner
Copy link
Contributor Author

Meanwhile there is a MCM release with gardener/machine-controller-manager#344 included. It is also already used by the extension: #382

@AndreasBurger, @rfranzke Could you please have another look? Thank you :)

@dkistner dkistner changed the title [WIP] Azure: Allow to deploy cluster in existing vNets Azure: Allow to deploy cluster in existing vNets Oct 23, 2019
@dkistner dkistner removed the reviewed/do-not-merge Has no approval for merging, may not be merged as it may break things or be of poor quality label Oct 23, 2019
@rfranzke
Copy link
Contributor

Please rebase and fix the CI issues

@gardener-robot-ci-2 gardener-robot-ci-2 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 Oct 23, 2019
rfranzke
rfranzke reviewed Oct 24, 2019
View reviewed changes
Copy link
Contributor

@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, small question on the object meta. ptal

controllers/provider-azure/pkg/apis/azure/types_infrastructure.go Outdated
@@ -23,6 +23,7 @@ import (
// InfrastructureConfig infrastructure configuration resource
type InfrastructureConfig struct {
metav1.TypeMeta
metav1.ObjectMeta
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we now get object meta here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use the Infrastructure resource namespace as name for the Shoot resource group. We need to ensure via validation that the existing vNet is not in the Shoot resource group. See https://github.com/gardener/gardener-extensions/pull/371/files#diff-847ec4dd7a0cc53cfc92b04acd2dd206R70

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I don't understand. The Infrastructure resource is defined in Gardener's extension API, and here you define an embedded InfrastructureConfig resource that is part of the Infrastructure object. Why does it have to have the namespace again? That's not convenient for the end-user and not needed. Can't you simply pass the namespace name to your validation function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I agree. I have modified it to pass the shoot resourceGroup name (=shoot technical name) to the Validation function. Thank you!

controllers/provider-azure/pkg/apis/azure/v1alpha1/types_infrastructure.go Outdated
@@ -23,7 +23,8 @@ import (

// InfrastructureConfig infrastructure configuration resource
type InfrastructureConfig struct {
metav1.TypeMeta `json:",inline"`
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we now get object meta here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above.

@dkistner
Azure: Allow to deploy cluster in existing vNets
c712637 
Allow to deploy Azure Shoot clusters in an existing vNet which exists in a different resource group.
@gardener-robot-ci-2 gardener-robot-ci-2 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 Oct 24, 2019
@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 Oct 24, 2019
@rfranzke rfranzke merged commit eb1060d into gardener-attic:master Oct 24, 2019
@dkistner dkistner deleted the az-vnet-rg branch December 10, 2019 14:39
@rfranzke rfranzke mentioned this pull request Jan 30, 2020
Open
2 tasks
@ghost ghost added the platform/azure Microsoft Azure platform/infrastructure label Mar 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rfranzke rfranzke rfranzke approved these changes

@AndreasBurger AndreasBurger Awaiting requested review from AndreasBurger

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) platform/azure Microsoft Azure platform/infrastructure
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