Skip to content

Enable NSGs shared between multiple machines#77

Merged
aiyengar2 merged 1 commit intorancher:masterfrom
aiyengar2:new_nsg_requirements
May 21, 2020
Merged

Enable NSGs shared between multiple machines#77
aiyengar2 merged 1 commit intorancher:masterfrom
aiyengar2:new_nsg_requirements

Conversation

@aiyengar2
Copy link
Copy Markdown

@aiyengar2 aiyengar2 commented Mar 27, 2020

This PR allows users of the Azure driver to submit an NSG name or ARM resource identifier to be used by multiple machines that are provisioned via Rancher Machine. If a user does not submit an NSG name, machine will default to the legacy option of creating an NSG per machine.

Related Issues: rancher/rancher#25342, rancher/rancher#11674, rancher/rancher#17181, rancher/rancher#22147, rancher/rancher#24449

@aiyengar2 aiyengar2 requested a review from a team March 27, 2020 20:49
@aiyengar2
Copy link
Copy Markdown
Author

A point of consideration for pooled NSGs in this commit:

In the pooled NSG case, we currently create one network interface per NSG and attach the network interface to the NSG referenced by the driver flag. As a result, when we have x nodes spun up in our node pool, there are x network interfaces (all part of the same subnet) that are attached to the single network security group.

An alternative to this design would be to have the one NSG attached directly to the subnet, as suggested in rancher/rancher#17181 and rancher/rancher#11674. However, I chose not to include that design due to the following two edge cases:

  1. Two node templates sharing the same subnet will be forced to share the same NSG / open port values.
  2. Cleanup logic (detailed here: https://github.com/rancher/machine/blob/master/drivers/azure/azure.go#L462-L480) currently deletes network security groups before deleting the subnet. Therefore, when cleaning up the network security group, it would not be clear whether the network security group is still in use by other nodes (as we haven't attempted to clean up the subnet yet) or this is the last node to be deleted. So to facilitate this logic, it would require a rewrite in the way we handle cleanup logic for nodes, which could have unintended consequences with legacy versions of machine currently running.

Would love to hear if there are any opposing opinions / suggested improvements for this design!

@aiyengar2
Copy link
Copy Markdown
Author

@aiyengar2 aiyengar2 force-pushed the new_nsg_requirements branch from 2c82443 to 9b8a945 Compare April 14, 2020 19:18
@aiyengar2 aiyengar2 requested review from luthermonson, mrajashree and superseb and removed request for a team, mrajashree and superseb April 14, 2020 22:24
@aiyengar2 aiyengar2 force-pushed the new_nsg_requirements branch 2 times, most recently from 559965e to c889076 Compare April 19, 2020 22:58
@aiyengar2 aiyengar2 requested a review from mrajashree April 19, 2020 23:05
@aiyengar2 aiyengar2 force-pushed the new_nsg_requirements branch from c889076 to 628ea6c Compare April 21, 2020 23:01
Comment thread drivers/azure/azureutil/azureutil.go Outdated
@aiyengar2 aiyengar2 force-pushed the new_nsg_requirements branch from 628ea6c to 976ae4a Compare April 27, 2020 04:23
Comment thread drivers/azure/azureutil/securityrules.go Outdated
Comment thread drivers/azure/azureutil/securityrules.go Outdated
Comment thread drivers/azure/azureutil/securityrules.go Outdated
Comment thread drivers/azure/azure.go
Comment thread drivers/azure/azureutil/securityrules.go Outdated
Comment thread drivers/azure/util.go Outdated
Comment thread drivers/azure/azureutil/securityrules.go Outdated
@aiyengar2 aiyengar2 force-pushed the new_nsg_requirements branch 3 times, most recently from 6cb4e78 to aad464d Compare May 14, 2020 20:27
@aiyengar2
Copy link
Copy Markdown
Author

PR has been simplified based on clarified requirements from #77 (comment) and is ready for re-review!

Double checked that the expected use cases provision correctly:

  • Creating 1 NSG per node when d.NSG is not specified / is empty (legacy)
  • Creating 1 NSG per pool when d.NSG is specified and is not an ARM resource identifier (new default)
  • Using an existing NSG when provided in the ARM resource identifier format
  • Creating a second node in the legacy case with an updated node template creates a brand new NSG with the updated rules. The older node's NSG is not updated, as expected.
  • Creating a second node in the pooled case with an updated node template attaches itself to the pooled NSG and updates the rules accordingly.

@deniseschannon
Copy link
Copy Markdown

rancher/rancher#24449

luthermonson
luthermonson previously approved these changes May 18, 2020
mrajashree
mrajashree previously approved these changes May 18, 2020
Comment thread drivers/azure/azure.go
Comment thread drivers/azure/azure.go Outdated
This commit allows users of the Azure driver to submit an NSG name or ARM resource identifier to be used by multiple machines that are provisioned via Rancher Machine. If a user does not submit an NSG name, machine will default to the legacy option of creating an NSG per machine.
@aiyengar2 aiyengar2 dismissed stale reviews from mrajashree and luthermonson via bfde51e May 21, 2020 17:35
@aiyengar2 aiyengar2 force-pushed the new_nsg_requirements branch from aad464d to bfde51e Compare May 21, 2020 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants