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

Specify Load Balancer in Resource Groups other than default #328

Closed
leochu opened this issue Nov 7, 2017 · 2 comments · Fixed by cloudfoundry/docs-bosh#761
Closed

Specify Load Balancer in Resource Groups other than default #328

leochu opened this issue Nov 7, 2017 · 2 comments · Fixed by cloudfoundry/docs-bosh#761

Comments

@leochu
Copy link

leochu commented Nov 7, 2017

Support load balancers in resource groups other than the default resource group.

https://github.com/cloudfoundry-incubator/bosh-azure-cpi-release/blob/33f3e636244ec4ae937596a8f17b38e0bd1cc002/src/bosh_azure_cpi/lib/cloud/azure/azure_client2.rb#L1199-L1202

@gossion
Copy link
Contributor

gossion commented Nov 13, 2017

It is doable to add resource_group_name as a parameter to get_load_balancer_by_name, now the question now is where to place this resource_group_name.

A suggestion is to reuse resource_group_name specified in cloud-properties of networks since load balancer is part of network resource, so load balancer, virutal network, security group are put in the same resource group. Config example as below. Azure CPI will search load balancer in this resource group `RG1' and bind VM to it, if it can't be found, CPI will then search it in the default resource group.

    networks:
    - name: private
      type: manual
      subnets:
      - range: 10.0.0.0/24
        gateway: 10.0.0.1
        dns: [168.63.129.16]
        cloud_properties:
          resource_group_name: RG1   ### Reuse this resource group. Load balancer & security group & virtual network will be put in this resource group `RG1' together
          virtual_network_name: boshvnet-crp
          subnet_name: Bosh

    resource_pools:
    - name: resource_with_lb
      ...
      network: private
      cloud_properties:
        load_balancer: cf-lb
        instance_type: Standard_D1

@leochu, are you okay with this design?

@andyliuliming
Copy link
Contributor

PR: #541
fixed this. please wait for the next release of CPI.
config it like this:
vm_extensions:

  • cloud_properties:
    resource_group_name: ((cfcr_resource_group_name))
    load_balancer:
    resource_group_name: ((cfcr_resource_group_name))
    name: ((cfcr_master_loadbalancer_name))
    security_group:
    resource_group_name: ((cfcr_resource_group_name))
    name: ((cfcr_master_security_group))
    name: ((deployment_name))-master-cloud-properties

  • type: replace
    path: /networks/-
    value:
    name: cfcr
    subnets:

    • azs:
      • z1
      • z2
      • z3
        gateway: ((cfcr_internal_gw))
        range: ((cfcr_subnet_cidr))
        reserved:
      • ((cfcr_internal_gw))/30
        cloud_properties:
        virtual_network_name: ((cfcr_vnet_name))
        subnet_name: ((cfcr_subnet))
        security_group:
        resource_group_name: ((cfcr_resource_group_name))
        name: ((cfcr_master_security_group))
        type: dynamic

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.

5 participants