Enable setting VNet peering properties by nprokopic · Pull Request #3340 · kubernetes-sigs/cluster-api-provider-azure

nprokopic · 2023-03-21T17:25:00Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR enables setting additional properties for VNet peering (AllowForwardedTraffic, AllowGatewayTransit, AllowVirtualNetworkAccess and UseRemoteGateways).

One use case for setting these properties to non-default values is to, for example, configure VPN gateway transit for virtual network peering.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3183

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Enable setting VNet peering properties (`AllowForwardedTraffic`, `AllowGatewayTransit`, `AllowVirtualNetworkAccess` and `UseRemoteGateways`)

willie-yao

This looks great, thanks for working on this @nprokopic !

Just one comment from my end: would you be able to add a unit test for VnetPeeringSpecs() in cluster_test.go?

Jont828

Thanks for the contribution! As @willie-yao said, we might want to add some tests in cluster_tests.go, and here are a couple points from me.

Jont828 · 2023-03-24T22:25:20Z

+	AllowVirtualNetworkAccess *bool `json:"allowVirtualNetworkAccess,omitempty"`
+
+	// UseRemoteGateways specifies if remote gateways can be used on this virtual network.
+	//


Can probably remove this extra line

Jont828 · 2023-03-24T22:25:42Z

+
+	// UseRemoteGateways specifies if remote gateways can be used on this virtual network.
+	//
+	// If the flag is set to true, and allowGatewayTransit on remote peering is also true, virtual network will use


Suggested change

// If the flag is set to true, and allowGatewayTransit on remote peering is also true, virtual network will use

// If the flag is set to true, and allowGatewayTransit on remote peering is also set to true, the virtual network will use

Jont828 · 2023-03-24T22:26:06Z

+	// UseRemoteGateways specifies if remote gateways can be used on this virtual network.
+	//
+	// If the flag is set to true, and allowGatewayTransit on remote peering is also true, virtual network will use
+	// gateways of remote virtual network for transit. Only one peering can have this flag set to true. This flag cannot


Suggested change

// gateways of remote virtual network for transit. Only one peering can have this flag set to true. This flag cannot

// the gateways of the remote virtual network for transit. Only one peering can have this flag set to true. This flag cannot

Jont828 · 2023-03-24T22:28:00Z

+		RemoteVnetName:            "spoke-vnet",
+		RemoteResourceGroup:       "spoke-group",
+		SubscriptionID:            "sub1",
+		AllowForwardedTraffic:     to.BoolPtr(true),


Suggested change

AllowForwardedTraffic: to.BoolPtr(true),

AllowForwardedTraffic: pointer.Bool(true),

I think we want to switch to the "k8s.io/utils/pointer" library

nprokopic · 2023-03-30T14:59:55Z

This looks great, thanks for working on this @nprokopic !

Just one comment from my end: would you be able to add a unit test for VnetPeeringSpecs() in cluster_test.go?

Thanks for the review! Sure, I will add a unit test for VnetPeeringSpecs() in cluster_test.go, missed it when I looked for unit tests to add. Will address other comments as well and update the PR.

nprokopic · 2023-03-31T12:30:50Z

@Jont828 @willie-yao I have added missing tests and also addressed other suggestions from the review. All tests are 🟢 :)

Can we add this PR to the v1.9 milestone?

Jont828 · 2023-04-03T16:44:18Z

/lgtm

This looks great! Unless there are any objections, I don't see why not.

k8s-ci-robot · 2023-04-03T16:44:25Z

LGTM label has been added.

Details

Git tree hash: 324aa8ade4d393ac71aa222ab473486159927d13

willie-yao · 2023-04-03T16:46:11Z

/lgtm

Thanks for all the great work on this!

willie-yao · 2023-04-03T16:46:35Z

/milestone v1.9

k8s-ci-robot · 2023-04-03T16:46:36Z

@willie-yao: You must be a member of the kubernetes-sigs/cluster-api-provider-azure-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Cluster API Provider Azure Maintainers and have them propose you as an additional delegate for this responsibility.

Details

In response to this:

/milestone v1.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nawazkh · 2023-04-04T23:17:35Z

Sweet! Thanks for adding this! 🚀

nprokopic · 2023-04-05T14:05:50Z

@willie-yao: You must be a member of the kubernetes-sigs/cluster-api-provider-azure-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Cluster API Provider Azure Maintainers and have them propose you as an additional delegate for this responsibility.

Can somebody from @kubernetes-sigs/cluster-api-provider-azure-maintainers help out here?

mboersma · 2023-04-05T16:08:56Z

/milestone v1.9

CecileRobertMichon · 2023-04-05T16:22:14Z

/approve

Thanks all for the reviews 🚀

k8s-ci-robot · 2023-04-05T16:22:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from Jont828 and willie-yao March 21, 2023 17:25

nprokopic force-pushed the more-peering-properties-clean branch 2 times, most recently from 5660020 to 5dc706b Compare March 22, 2023 15:19

willie-yao reviewed Mar 24, 2023

View reviewed changes

Jont828 reviewed Mar 24, 2023

View reviewed changes

Enable setting VNet peering properties

82d0936

nprokopic force-pushed the more-peering-properties-clean branch from 5dc706b to 82d0936 Compare March 31, 2023 11:16

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 31, 2023

This was referenced Apr 3, 2023

Enable setting peering properties giantswarm/cluster-api-provider-azure#16

Merged

Improve CAPZ upstream VNet Peering giantswarm/roadmap#2041

Closed

k8s-ci-robot assigned Jont828 Apr 3, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 3, 2023

k8s-ci-robot assigned willie-yao Apr 3, 2023

k8s-ci-robot added this to the v1.9 milestone Apr 5, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2023

k8s-ci-robot merged commit 8fefb1a into kubernetes-sigs:main Apr 5, 2023

jackfrancis mentioned this pull request Jan 31, 2024

Add Jonathan Tong to maintainers #4526

Merged

4 tasks

Gacko deleted the more-peering-properties-clean branch February 13, 2025 15:28

	// If the flag is set to true, and allowGatewayTransit on remote peering is also true, virtual network will use
	// If the flag is set to true, and allowGatewayTransit on remote peering is also set to true, the virtual network will use

	// gateways of remote virtual network for transit. Only one peering can have this flag set to true. This flag cannot
	// the gateways of the remote virtual network for transit. Only one peering can have this flag set to true. This flag cannot

	AllowForwardedTraffic: to.BoolPtr(true),
	AllowForwardedTraffic: pointer.Bool(true),

Conversation

nprokopic commented Mar 21, 2023

Uh oh!

willie-yao left a comment

Choose a reason for hiding this comment

Uh oh!

Jont828 left a comment

Choose a reason for hiding this comment

Uh oh!

Jont828 Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

Jont828 Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

Jont828 Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

Jont828 Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

nprokopic commented Mar 30, 2023

Uh oh!

nprokopic commented Mar 31, 2023

Uh oh!

Jont828 commented Apr 3, 2023

Uh oh!

k8s-ci-robot commented Apr 3, 2023

Uh oh!

willie-yao commented Apr 3, 2023

Uh oh!

willie-yao commented Apr 3, 2023

Uh oh!

k8s-ci-robot commented Apr 3, 2023

Uh oh!

nawazkh commented Apr 4, 2023

Uh oh!

nprokopic commented Apr 5, 2023

Uh oh!

mboersma commented Apr 5, 2023

Uh oh!

CecileRobertMichon commented Apr 5, 2023

Uh oh!

k8s-ci-robot commented Apr 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants