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.

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

Support multiple load_balancer entries #111

Closed
svrc opened this issue Mar 25, 2016 · 15 comments · Fixed by cloudfoundry/docs-bosh#761
Closed

Support multiple load_balancer entries #111

svrc opened this issue Mar 25, 2016 · 15 comments · Fixed by cloudfoundry/docs-bosh#761

Comments

@svrc
Copy link

svrc commented Mar 25, 2016

Hi all, here's a scenario , let me know what you think of it

  1. It is good to have split horizon DNS for a CF instance, i.e. on the devbox you have a named/bind9 server that resolves *.cf.azurelovecf.com to 10.10.16.4 inside the VNet, and the Public IP outside the VNet. This avoids hairpin NAT routing to the load balancer public IP address which sometimes can be slower and flaky than remaining within the VNet.
  2. In a high availability enterprise scenario, we should use an Azure Load Balancer created ahead of time and have it load balance to two HAproxy instances annotated with the load_balancer cloud property.
  3. However an azure load balancer can only have a Public OR a Private IP address, it cannot have both.
  4. Therefore I think it makes sense to be able to have two (or more?) load_balancer entries, one for a Private load balancer and one for a Public load balancer.

Let me know if this is a reasonable enhancement. Right now as workaround, I'd need to create two resource pools and jobs for haproxy.

@AbelHu
Copy link
Contributor

AbelHu commented Mar 25, 2016

@svrc-pivotal It is a good idea. Let us do some investigation.

@AbelHu
Copy link
Contributor

AbelHu commented Apr 7, 2016

Related to Azure/azure-powershell#2042

@AbelHu
Copy link
Contributor

AbelHu commented Jul 26, 2016

@svrc-pivotal Now Azure has supported to create one external LB and one internal LB in the same NIC. Could you have a try?

@AbelHu
Copy link
Contributor

AbelHu commented Aug 10, 2016

@svrc-pivotal Need to correct. Azure can support to create create one external LB and one internal LB in the same NIC but the port rules cannot be same. We are still investigating.

@gossion
Copy link
Contributor

gossion commented Aug 18, 2016

There is a question and answer at this link about multiple load balancers, saying that:

  1. Azure NICs can participate in multiple load-balancer: one external and one internal.
  2. LB rules on the external and internal load-balancers cannot use the same backend port.
  3. Only the primary NIC can be used in load-balancer backend pools.

I had tried to create rule tcp 22:22 for both external and internal load balancers and assigned those two lbs to the same VM, I got error code "RulesUseSameBackendPortProtocolAndIPConfig" when creating the VM.

Hi @svrc-pivotal, are you expecting to use same backend port in load balancers in CF? Azure does not support such feature now.

@svrc
Copy link
Author

svrc commented Aug 19, 2016

Hi @gossion yeah the idea was that I could reduce the number of HA proxy instances.. they would use the same backend port. I guess we'll have to wait to see if Azure improves LBs to enable the same backend ports.

@gossion
Copy link
Contributor

gossion commented Sep 21, 2017

Hi @svrc-pivotal ,

Now in Azure the secondary NICs can also be used in LB backend pools, so this feature is doable. However, the secondary NICs can't be probed by LB because by default the outbound traffic will go to gateway of primary NIC. In order to make LB work with the secondary NICs we need to configure a policy route in the VM, which means we need a bosh release to configure the network additionally. I had a PR in networking-release, trying to implement the policy routing for this scenario, but it is not reviewed / merged yet.

It looks like this is not a common scenario, and the issue has last for a long time. Not sure if you have already solved the issue by any other methods, so my question is - do you still need this feature?

@svrc
Copy link
Author

svrc commented Sep 26, 2017

@gossion At the moment we deploy separate VMs if we need separate load balancers. I have at least two large Azure customers deploying separate VMs for internal vs. external facing HAproxies for example.

I guess my point was that Azure is the only cloud I know (out of AWS, OpenStack, GCP, vSphere) that requires multiple NICs for multiple load balancers, so this should be a common case. Multi-NIC could be a workaround but seems too early.

We can close this issue if it doesn't look like Azure LBs will ever allow a single NIC to expose to multiple load balancers, as it's a larger concern than just this CPI :)

@svrc svrc closed this as completed Sep 26, 2017
@svrc
Copy link
Author

svrc commented Nov 21, 2018

This should work with standard LB skus and maybe should be revisited, especially considering SNAT with standard LBs requires a public LB with unused inbound rules to denote SNAT outbound IPs/ports (scenario 2 here - https://docs.microsoft.com/en-us/azure/load-balancer/load-balancer-outbound-connections ).

I could forsee a case where if using standard LBs that every VM in a BOSH deployment gets a public LB assigned for outbound SNAT purposes, and then a subset of VMs require a second LBs for internal purposes. (Or is is preferred to mix internal/external frontends on the same LB in a standard case? admittedly I don't know)

@svrc svrc reopened this Nov 21, 2018
@daviddob
Copy link

Any updates regarding supporting multiple LBs attached to the same VM orchestrating via BOSH? We currently are accomplishing this using a separate piece of automation to attach the VMs after deployment to the second LB (one internal, one external), however it would be nice to simplify and eliminate the need for out of band changes.

@bosh-admin-bot
Copy link

This issue was marked as Stale because it has been open for 21 days without any activity. If no activity takes place in the coming 7 days it will automatically be close. To prevent this from happening remove the Stale label or comment below.

@daviddob
Copy link

Not stale - waiting on updates from the BOSH team.

@bosh-admin-bot
Copy link

This issue was marked as Stale because it has been open for 21 days without any activity. If no activity takes place in the coming 7 days it will automatically be close. To prevent this from happening remove the Stale label or comment below.

@bosh-admin-bot
Copy link

This issue was closed because it has been labeled Stale for 7 days without subsequent activity. Feel free to re-open this issue at any time by commenting below.

@MSSedusch
Copy link
Contributor

this issue should be fixed with PR #638 which was released with v37.6.0.

Justin-W added a commit to Justin-W/docs-bosh that referenced this issue Dec 10, 2021
…s related to the following Azure CPI PRs/issues:

- cloudfoundry/bosh-azure-cpi-release#651 (Fixes )
- cloudfoundry/bosh-azure-cpi-release#541 (Fixed cloudfoundry/bosh-azure-cpi-release#328)
- cloudfoundry/bosh-azure-cpi-release#638 (Fixed cloudfoundry/bosh-azure-cpi-release#111)

Note: This commit assumes that PR cloudfoundry#651 will be included in a new `v37.7.0` release of the Azure CPI (in order to properly document the CPI version requirements of new features). If PR cloudfoundry#651 is included in a different CPI version, then this PR will need to be amended appropriately.
Justin-W added a commit to Justin-W/docs-bosh that referenced this issue Dec 10, 2021
…s related to the following Azure CPI PRs/issues:

- cloudfoundry/bosh-azure-cpi-release#651 (Fixes cloudfoundry/bosh-azure-cpi-release#644)
- cloudfoundry/bosh-azure-cpi-release#541 (Fixed cloudfoundry/bosh-azure-cpi-release#328)
- cloudfoundry/bosh-azure-cpi-release#638 (Fixed cloudfoundry/bosh-azure-cpi-release#111)

Note: This commit assumes that PR cloudfoundry#651 will be included in a new `v37.7.0` release of the Azure CPI (in order to properly document the CPI version requirements of new features). If PR cloudfoundry#651 is included in a different CPI version, then this PR will need to be amended appropriately.
Justin-W added a commit to Justin-W/docs-bosh that referenced this issue Dec 15, 2021
…ample of" sections for alternate Load Balancer configurations, for new and existing features related to the following Azure CPI PRs/issues:

- cloudfoundry/bosh-azure-cpi-release#651 (Fixes cloudfoundry/bosh-azure-cpi-release#644)
- cloudfoundry/bosh-azure-cpi-release#541 (Fixed cloudfoundry/bosh-azure-cpi-release#328)
- cloudfoundry/bosh-azure-cpi-release#638 (Fixed cloudfoundry/bosh-azure-cpi-release#111)

Note: This commit assumes that PR cloudfoundry#651 will be included in a new `v37.7.0` release of the Azure CPI (in order to properly document the CPI version requirements of new features). If PR cloudfoundry#651 is included in a different CPI version, then this PR will need to be amended appropriately.
Justin-W added a commit to Justin-W/docs-bosh that referenced this issue Dec 15, 2021
…y configurations, for new and existing features related to the following Azure CPI PRs/issues:

- cloudfoundry/bosh-azure-cpi-release#651 (Fixes cloudfoundry/bosh-azure-cpi-release#644)
- cloudfoundry/bosh-azure-cpi-release#541 (Fixed cloudfoundry/bosh-azure-cpi-release#328)
- cloudfoundry/bosh-azure-cpi-release#638 (Fixed cloudfoundry/bosh-azure-cpi-release#111)

Note: This commit assumes that PR cloudfoundry#651 will be included in a new `v37.7.0` release of the Azure CPI (in order to properly document the CPI version requirements of new features). If PR cloudfoundry#651 is included in a different CPI version, then this PR will need to be amended appropriately.
Justin-W added a commit to Justin-W/bosh-azure-cpi-release that referenced this issue Jan 12, 2022
…foundry#111 and cloudfoundry#328, in preparation for cloudfoundry#644)

Note: There was already a high degree of symmetry (of both functionality and implementation) between the CPI's support for LBs and AGWs. However, previous CPI enhancements (for example: cloudfoundry#541 for cloudfoundry#111 and cloudfoundry#638 for cloudfoundry#328) had reduced this symmetry to some degree, and also caused some divergence between the names of some methods and variables (vs their implementations/values), which would make it harder to both implement and review the new functionality for cloudfoundry#644, and also harder to maintain the symmetry between the LB and AGW functionality.

These changes include:

- refactoring of existing code/implementation. E.g. Renamed some vars and methods to more accurately reflect their current values/behavior.
- minor configuration API enhancements. E.g. Allow the `load_balancer` config to be configured as an Array, instead of the (previously-undocumented, but already-supported) comma-delimited string of LB names, to support multiple LB configs which differ by more than just the LB name (which now enables the functionality of cloudfoundry#111 and cloudfoundry#328 to be used more independently within a single config).
- adding multiple new/missing unit tests, which cover existing functionality (previously added by cloudfoundry#541 for cloudfoundry#111 and cloudfoundry#638 for cloudfoundry#328) which was previously not covered/tested by the unit test suite.

---

COMMIT HISTORY:

Modified the `VMCloudProps._parse_load_balancer_config` method to allow the `load_balancer` config to be configured as an Array.
Note: When specifying an Array of LBs, each element of the Array should be a String and/or Hash of the same format which was previously (and still is) valid when configuring a single LB as a non-Array.

Refactoring: Minor refactoring in the `VMManager._get_load_balancers` method.

Added additional data type validation to the `VMCloudProps._parse_load_balancer_config` method.

Modified the `VMCloudProps._parse_load_balancer_config` method to return `nil` when the `load_balancer` config is omitted/missing.

Added additional specs for the `VMCloudProps._parse_load_balancer_config` method.

Refactoring: Renamed the `VMCloudProps.load_balancer` attribute.

Refactoring of the `VMCloudProps._parse_load_balancer_config` and `VMManager._get_load_balancers` methods' implementations.

Refactoring: Cleanup of the `VMCloudProps._parse_load_balancer_config` method's implementation.

Refactoring: Minor cleanup of the `VMManager._get_load_balancers` method's implementation.

Renamed a key within the Hash returned by the `AzureClient.parse_network_interface` method.

Fixed the `AzureClient.parse_network_interface` method to correctly return all of the NIC's load_balancers' info (instead of only the first LB's info).

Refactoring: Minor refactoring in the `AzureClient.create_network_interface` method.

Refactoring: Renamed some vars in the `AzureClient.create_network_interface` method.

Refactoring: Renamed a key within the `nic_params` Hash (from "load_balancer" to "load_balancers"), and fixed inaccurate doc comments for that key.

Fixed a comment typo.

Refactoring: Renamed a method and some vars: vm_manager_network.rb.
Justin-W added a commit to Justin-W/bosh-azure-cpi-release that referenced this issue Jan 12, 2022
Note: This increases/restores the symmetry between the LB and AGW functionality (and configuration), by enhancing AGW support with new (for AGWs) functionality which had already been previously supported for LBs (by cloudfoundry#541 for cloudfoundry#111 and cloudfoundry#638 for cloudfoundry#328). Restoring this level of symmetry partially resolves cloudfoundry#644.

These changes include:

- refactoring of existing code/implementation (and tests). E.g. Renamed some vars and methods to more accurately reflect their current values/behavior.
- significant configuration API enhancements, to enable the (previously LB-only) functionality of cloudfoundry#111 and cloudfoundry#328 to be used with both LB and/or AGW configs. E.g. Allow the `application_gateway` config to be configured as a Hash; allow the `application_gateway` config to be configured as an Array (new in this PR for both LBs and AGWs); added support for `application_gateway/resource_roup_name` config.

---

SQUASHED COMMIT HISTORY:

issue-644: multi-AGW: Refactored the `AzureClient.get_application_gateway_by_name` method to make the `resource_group_name` param a positional param (instead of an optional keyword param).
Note: This change was made so that the `AzureClient.get_application_gateway_by_name` method's signature is consistent with the signatures of the other `AzureClient.get_*` methods in requiring the `resource_group_name` param to be passed as the first positional param.

issue-644: multi-AGW: Modified the `VMCloudProps._parse_application_gateway_config` method to allow the `application_gateway` config to be configured as an Array.
Note: When specifying an Array of AGWs, each element of the Array should be a String and/or Hash of the same format which was previously (and still is) valid when configuring a single AGW (as a non-Array).

issue-644: multi-AGW: Modified the `VMCloudProps._parse_application_gateway_config` method to accept either String or Hash config (instead of String only).

issue-644: multi-AGW: Fixed the `AzureClient.parse_network_interface` method to correctly return all of the NIC's application_gateways' info (instead of only the first LB's info).
NOTE: All unit test specs are now passing again.

issue-644: multi-AGW: Update `VMManager._get_application_gateways` method to support multi-AGW (and also to use the new `ApplicationGatewayConfig.resource_group_name` property/data).

issue-644: multi-AGW: Modified the `VMCloudProps._parse_application_gateway_config` method to init `ApplicationGatewayConfig.resource_group_name` as `nil`, unless a value is explicitly configured. (See code comment for details.)

issue-644: multi-AGW: Update `AzureClient.parse_network_interface` method to use the new `resource_group_name` keyword param of the `AzureClient.get_application_gateway_by_name` method.

issue-644: multi-AGW: Added a new, optional `resource_group_name` keyword param to the `AzureClient.get_application_gateway_by_name` method.

issue-644: multi-AGW: Updated `AzureClient.create_network_interface` method to support multi-AGW.

Renamed a spec example in the Integration test suite, to make the meaning clearer: application_gateway_spec.rb.

COMMENTS: Added/updated comments and doc comments: AzureClient, VMManager.

issue-644: multi-AGW: Renamed a key within the `nic_params` Hash (from "application_gateway" to "application_gateways").

issue-644: multi-AGW: Rename and update `VMManager._get_application_gateways` method

issue-644: multi-AGW: Renamed various vars/keys (from `application_gateway` to `application_gateways`)

issue-644: multi-AGW: Added additional specs for the `VMCloudProps._parse_application_gateway_config` method.

issue-644: multi-AGW: Add new `VMCloudProps._parse_application_gateway_config` method.
NOTE: This change is part of a multi-commit set of incremental changes. Some of the existing unit test specs are now failing. They will be fixed/updated by the time the incremental changes are completed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants