separate route tables for node and control-plane

[nader-ziada]

What this PR does / why we need it:

• add different route tables to for node and control-plane

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 #718

Special notes for your reviewer:

• we need to confirm this is a valid requirements, there are some discussion on the issue if we need two route tables. I had done most of the code as part of the route table refactoring, so making a PR anyway

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:

Add separate route tables for node and control-plane

[nader-ziada]

/hold

for validation of the requirement

[nader-ziada]

@CecileRobertMichon @jsturtevant do we still think we need this feature?

[CecileRobertMichon]

@CecileRobertMichon @jsturtevant do we still think we need this feature?

I think so? it's more correct this way at least, even if we're not leveraging the route tables yet.

[jsturtevant]

agreed, it is closer to the the way the public API is designed. The other alternative is to modify the API to only allow a single route table.

@aramase Do you know of scenarios where control plane would have a different route table than the worker nodes?

[nader-ziada]

ok in that case ready for review
/hold cancel

[aramase]

@aramase Do you know of scenarios where control plane would have a different route table than the worker nodes?

I don't see a scenario where we would need a different route table for control plane and worker nodes. I haven't had a chance to look at the PR completely, but when the control plane/worker node is bootstrapped is the RouteTableName in azure.json going to contain the control plane/worker node RT name populated appropriately?

[CecileRobertMichon]

@aramase azure.json uses the node route table as "routeTableName" see https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/controllers/helpers.go#L228

BTW we should change that to get the actual NodeRouteTable().Name @nader-ziada, otherwise, it will break if the use uses a different name.

[nader-ziada]

@aramase azure.json uses the node route table as "routeTableName" see https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/controllers/helpers.go#L228

BTW we should change that to get the actual NodeRouteTable().Name @nader-ziada, otherwise, it will break if the use uses a different name.

we only create one azure.json secret for machines, so control plane and worker machines will get the same, right?

[CecileRobertMichon]

we only create one azure.json secret for machines, so control plane and worker machines will get the same, right?

we do create one per MD/Machine so technically they could have different ones (although right now the controller sets the same thing regardless of machine role). We are going to have to differ between the two as part of #921 anyways. What I'm not 100% sure about is why would you ever want you azure.json routeTable value to be the control plane route table?

[nader-ziada]

/test pull-cluster-api-provider-azure-e2e

[nader-ziada]

rebased with ipv6

[nader-ziada]

ready for review @CecileRobertMichon @devigned

[CecileRobertMichon]

I think we should consider not creating a control plane route table by default and only creating it if the user specifies one since the default use case doesn't require one

[nader-ziada]

so still use node route table unless the user specifies their own name of control plane route table?

[CecileRobertMichon]

yes, the route table is attached to the subnet and the cloud provider cares about the NodeSubnet so it should only care about the NodeRouteTable. This behavior is just more correct because it doesn't ignore the Spec for route table on the control plane subnet and allows more advanced users to customize their cluster. But the default should stay the same, ie. Node subnet has a route table, control plane subnet doesn't, and the azure.json file has the node route table as routeTableName on all nodes.

[CecileRobertMichon]

we also need to update https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/controllers/helpers.go#L226

[k8s-ci-robot]

@nader-ziada: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-azure-apidiff	5e08e50	link	/test pull-cluster-api-provider-azure-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[nader-ziada]

@CecileRobertMichon @devigned ready for another review. Thanks

[CecileRobertMichon]

/lgtm

[devigned]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devigned

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [devigned]

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