OCPBUGS-9404: azure: skip LB creation when not needed #7063

r4f4 · 2023-04-04T10:24:24Z

When one creates an IPI Azure cluster with an internal publishing method, it creates a standard load balancer with an empty definition. This load balancer doesn't serve a purpose since the configuration is completely empty. Because it doesn't have a public IP address and backend pools it's not providing any outbound connectivity, and there are no frontend IP configurations for ingress connectivity to the cluster.

openshift-ci-robot · 2023-04-04T10:24:29Z

@r4f4: This pull request references Jira Issue OCPBUGS-9404, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

When one creates an IPI Azure cluster with an internal publishing method, it creates a standard load balancer with an empty definition. This load balancer doesn't serve a purpose since the configuration is completely empty. Because it doesn't have a public IP address and backend pools it's not providing any outbound connectivity, and there are no frontend IP configurations for ingress connectivity to the cluster.

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.

r4f4 · 2023-04-04T10:25:03Z

/jira refresh

openshift-ci-robot · 2023-04-04T10:25:11Z

@r4f4: This pull request references Jira Issue OCPBUGS-9404, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jinyunma

In response to this:

/jira refresh

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.

r4f4 · 2023-04-04T22:27:18Z

/hold
Needs #7064 for the CI jobs to run.

zaneb · 2023-04-05T05:08:44Z

/retest
/hold cancel

patrickdillon

When one creates an IPI Azure cluster with an internal publishing method, it creates a standard load balancer with an empty definition. This load balancer doesn't serve a purpose since the configuration is completely empty. Because it doesn't have a public IP address and backend pools it's not providing any outbound connectivity, and there are no frontend IP configurations for ingress connectivity to the cluster.

It looks like the way we provide outbound connectivity to VMs when the install is private and not using user-defined routing, is by attaching an outbound rule to the load balancer:

installer/data/data/azure/vnet/public-lb.tf

Lines 146 to 157 in ebabc74

    
           resource "azurerm_lb_outbound_rule" "public_lb_outbound_rule_v4" { 
        
             count = var.use_ipv4 && var.azure_private && ! var.azure_outbound_user_defined_routing ? 1 : 0 
        
             name                    = "outbound-rule-v4" 
        
             loadbalancer_id         = azurerm_lb.public.id 
        
             backend_address_pool_id = azurerm_lb_backend_address_pool.public_lb_pool_v4[0].id 
        
             protocol                = "All" 
        
             frontend_ip_configuration { 
        
               name = local.public_lb_frontend_ip_v4_configuration_name 
        
             } 
        
           }

Which leaves me with the question: how are VMs getting outbound connectivity in this scenario?

I was under the impression that we could only remove the LB if using user-defined routing.

data/data/azure/vnet/public-lb.tf

patrickdillon · 2023-04-17T15:05:02Z

We don't have any CI testing that performs a private install. I am floating openshift/release#38408 as an option. Have you done any local testing of this? I'm not sure what the expectations are for the cloud provider.

r4f4 · 2023-04-17T15:14:14Z

We don't have any CI testing that performs a private install. I am floating openshift/release#38408 as an option. Have you done any local testing of this? I'm not sure what the expectations are for the cloud provider.

It was pre-merge tested by @jinyunma before I addressed your comments. I haven't found the time to retest it yet after the update.

patrickdillon · 2023-04-17T15:25:52Z

We don't have any CI testing that performs a private install. I am floating openshift/release#38408 as an option. Have you done any local testing of this? I'm not sure what the expectations are for the cloud provider.

It was pre-merge tested by @jinyunma before I addressed your comments. I haven't found the time to retest it yet after the update.

Excellent! And sorry I missed that pre-merge testing in the bug. I'm not concerned about testing the recent update. Thank you!

patrickdillon · 2023-04-17T15:36:55Z

@jinyunma Thanks for doing the pre-merge testing on this. I see that the testing was done with user-defined routing. That makes sense and it seems like we should be able to remove the LB in that case.

Are you able to test without user-defined routing? I believe in that case we still need to keep the LB, because the installer applies it's own rule to enable outbound internet access. If the install succeeds with the current changes, we want to double check to make sure Azure is not enabling default outbound access.

jinyunma · 2023-04-18T08:55:11Z

That makes sense and it seems like we should be able to remove the LB in that case.

yes, external LB is removed on fully private cluster (fully private cluster ( publish: Internal + outboundType: UserDefinedRouting), and no function impact.

Are you able to test without user-defined routing? I believe in that case we still need to keep the LB, because the installer applies it's own rule to enable outbound internet access. If the install succeeds with the current changes, we want to double check to make sure Azure is not enabling default outbound access.

pre-merge testing on the latest update, install private cluster successfully ( publish: Internal wihtout user-defined routing), external LB is created, and outbound rules is explicitly defined, using the frontend IP address of external LB for outbound.

# az network lb list -g jima7673-18b-slv6v-rg -otable
Location    Name                         ProvisioningState    ResourceGroup          ResourceGuid
----------  ---------------------------  -------------------  ---------------------  ------------------------------------
eastus      jima7673-18b-slv6v           Succeeded            jima7673-18b-slv6v-rg  8d5b10d7-9b15-4961-8968-00673ba6de33
eastus      jima7673-18b-slv6v-internal  Succeeded            jima7673-18b-slv6v-rg  a547ee2b-701c-4ada-9048-79df4b31ceac

# az network lb show --name jima7673-18b-slv6v -g jima7673-18b-slv6v-rg --query outboundRules
[
  {
    "allocatedOutboundPorts": 1024,
    "backendAddressPool": {
      "id": "/subscriptions/53b8f551-f0fc-4bea-8cba-6d1fefd54c8a/resourceGroups/jima7673-18b-slv6v-rg/providers/Microsoft.Network/loadBalancers/jima7673-18b-slv6v/backendAddressPools/jima7673-18b-slv6v",
      "resourceGroup": "jima7673-18b-slv6v-rg"
    },
    "enableTcpReset": false,
    "etag": "W/\"5b6a9854-dfa1-4972-999f-3475b70623e2\"",
    "frontendIpConfigurations": [
      {
        "id": "/subscriptions/53b8f551-f0fc-4bea-8cba-6d1fefd54c8a/resourceGroups/jima7673-18b-slv6v-rg/providers/Microsoft.Network/loadBalancers/jima7673-18b-slv6v/frontendIPConfigurations/public-lb-ip-v4",
        "resourceGroup": "jima7673-18b-slv6v-rg"
      }
    ],
    "id": "/subscriptions/53b8f551-f0fc-4bea-8cba-6d1fefd54c8a/resourceGroups/jima7673-18b-slv6v-rg/providers/Microsoft.Network/loadBalancers/jima7673-18b-slv6v/outboundRules/outbound-rule-v4",
    "idleTimeoutInMinutes": 4,
    "name": "outbound-rule-v4",
    "protocol": "All",
    "provisioningState": "Succeeded",
    "resourceGroup": "jima7673-18b-slv6v-rg",
    "type": "Microsoft.Network/loadBalancers/outboundRules"
  }
]

r4f4 · 2023-05-11T15:14:03Z

@patrickdillon any other concerns?

data/data/azure/vnet/public-lb.tf

patrickdillon · 2023-06-22T13:50:17Z

/approve

openshift-ci · 2023-06-22T13:57:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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:

~~data/data/azure/OWNERS~~ [patrickdillon]

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

When one creates an IPI Azure cluster with an `internal` publishing method, it creates a standard load balancer with an empty definition. This load balancer doesn't serve a purpose since the configuration is completely empty. Because it doesn't have a public IP address and backend pools it's not providing any outbound connectivity, and there are no frontend IP configurations for ingress connectivity to the cluster.

r4f4 · 2023-06-22T13:59:58Z

Update: no functional changes, just reusing a tf variable.

jhixson74 · 2023-07-06T21:22:13Z

/lgtm

openshift-ci-robot · 2023-07-07T00:31:19Z

/retest-required

Remaining retests: 0 against base HEAD 30838d9 and 2 for PR HEAD 4c9d13d in total

openshift-ci-robot · 2023-07-07T03:31:27Z

/retest-required

Remaining retests: 0 against base HEAD a58142c and 1 for PR HEAD 4c9d13d in total

openshift-ci-robot · 2023-07-07T23:31:17Z

/retest-required

Remaining retests: 0 against base HEAD f8975eb and 0 for PR HEAD 4c9d13d in total

openshift-ci-robot · 2023-07-08T03:31:07Z

/hold

Revision 4c9d13d was retested 3 times: holding

r4f4 · 2023-07-10T14:27:10Z

/hold cancel

openshift-ci-robot · 2023-07-10T14:31:12Z

/retest-required

Remaining retests: 0 against base HEAD ff1f452 and 2 for PR HEAD 4c9d13d in total

openshift-ci-robot · 2023-07-10T22:31:13Z

/retest-required

Remaining retests: 0 against base HEAD 5409f2e and 1 for PR HEAD 4c9d13d in total

openshift-ci-robot · 2023-07-11T02:31:12Z

/retest-required

Remaining retests: 0 against base HEAD a4604b0 and 0 for PR HEAD 4c9d13d in total

openshift-ci · 2023-07-11T06:59:32Z

@r4f4: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-azurestack	`4c9d13d`	link	false	`/test e2e-azurestack`
ci/prow/e2e-azure-ovn-shared-vpc	`4c9d13d`	link	false	`/test e2e-azure-ovn-shared-vpc`

Full PR test history. Your PR dashboard.

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.

openshift-ci-robot · 2023-07-11T07:31:07Z

/hold

Revision 4c9d13d was retested 3 times: holding

r4f4 · 2023-07-11T16:29:02Z

/hold cancel

openshift-ci-robot · 2023-07-11T16:48:51Z

/retest-required

Remaining retests: 0 against base HEAD 3bc9b24 and 2 for PR HEAD 4c9d13d in total

openshift-ci-robot · 2023-07-12T19:28:22Z

@r4f4: Jira Issue OCPBUGS-9404: All pull requests linked via external trackers have merged:

openshift/installer#7063

Jira Issue OCPBUGS-9404 has been moved to the MODIFIED state.

In response to this:

When one creates an IPI Azure cluster with an internal publishing method, it creates a standard load balancer with an empty definition. This load balancer doesn't serve a purpose since the configuration is completely empty. Because it doesn't have a public IP address and backend pools it's not providing any outbound connectivity, and there are no frontend IP configurations for ingress connectivity to the cluster.

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.

r4f4 · 2023-07-12T19:41:36Z

/cherry-pick release-4.13

openshift-cherrypick-robot · 2023-07-12T19:43:16Z

@r4f4: new pull request created: #7322

In response to this:

/cherry-pick release-4.13

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.

openshift-ci bot requested review from jinyunma, jhixson74 and mtulio April 4, 2023 10:25

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 5, 2023

patrickdillon reviewed Apr 12, 2023

View reviewed changes

data/data/azure/vnet/public-lb.tf Show resolved Hide resolved

r4f4 force-pushed the azure-udr-lb branch from e2fa744 to 6b76ac1 Compare April 12, 2023 19:09

barbacbd reviewed May 11, 2023

View reviewed changes

data/data/azure/vnet/public-lb.tf Outdated Show resolved Hide resolved

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2023

r4f4 force-pushed the azure-udr-lb branch from 6b76ac1 to 4c9d13d Compare June 22, 2023 13:59

openshift-ci bot assigned jhixson74 Jul 6, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2023

openshift-merge-robot merged commit d3dc62b into openshift:master Jul 12, 2023
21 of 23 checks passed

openshift-cherrypick-robot mentioned this pull request Jul 12, 2023

[release-4.13] OCPBUGS-16124: azure: skip LB creation when not needed #7322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-9404: azure: skip LB creation when not needed #7063

OCPBUGS-9404: azure: skip LB creation when not needed #7063

r4f4 commented Apr 4, 2023

openshift-ci-robot commented Apr 4, 2023

r4f4 commented Apr 4, 2023

openshift-ci-robot commented Apr 4, 2023

r4f4 commented Apr 4, 2023

zaneb commented Apr 5, 2023

patrickdillon left a comment

patrickdillon commented Apr 17, 2023

r4f4 commented Apr 17, 2023

patrickdillon commented Apr 17, 2023

patrickdillon commented Apr 17, 2023

jinyunma commented Apr 18, 2023

r4f4 commented May 11, 2023

patrickdillon commented Jun 22, 2023

openshift-ci bot commented Jun 22, 2023

r4f4 commented Jun 22, 2023

jhixson74 commented Jul 6, 2023

openshift-ci-robot commented Jul 7, 2023

openshift-ci-robot commented Jul 7, 2023

openshift-ci-robot commented Jul 7, 2023

openshift-ci-robot commented Jul 8, 2023

r4f4 commented Jul 10, 2023

openshift-ci-robot commented Jul 10, 2023

openshift-ci-robot commented Jul 10, 2023

openshift-ci-robot commented Jul 11, 2023

openshift-ci bot commented Jul 11, 2023 •

edited

openshift-ci-robot commented Jul 11, 2023

r4f4 commented Jul 11, 2023

openshift-ci-robot commented Jul 11, 2023

openshift-ci-robot commented Jul 12, 2023

r4f4 commented Jul 12, 2023

openshift-cherrypick-robot commented Jul 12, 2023

	resource "azurerm_lb_outbound_rule" "public_lb_outbound_rule_v4" {
	count = var.use_ipv4 && var.azure_private && ! var.azure_outbound_user_defined_routing ? 1 : 0

	name = "outbound-rule-v4"
	loadbalancer_id = azurerm_lb.public.id
	backend_address_pool_id = azurerm_lb_backend_address_pool.public_lb_pool_v4[0].id
	protocol = "All"

	frontend_ip_configuration {
	name = local.public_lb_frontend_ip_v4_configuration_name
	}
	}

OCPBUGS-9404: azure: skip LB creation when not needed #7063

OCPBUGS-9404: azure: skip LB creation when not needed #7063

Conversation

r4f4 commented Apr 4, 2023

openshift-ci-robot commented Apr 4, 2023

r4f4 commented Apr 4, 2023

openshift-ci-robot commented Apr 4, 2023

r4f4 commented Apr 4, 2023

zaneb commented Apr 5, 2023

patrickdillon left a comment

Choose a reason for hiding this comment

patrickdillon commented Apr 17, 2023

r4f4 commented Apr 17, 2023

patrickdillon commented Apr 17, 2023

patrickdillon commented Apr 17, 2023

jinyunma commented Apr 18, 2023

r4f4 commented May 11, 2023

patrickdillon commented Jun 22, 2023

openshift-ci bot commented Jun 22, 2023

r4f4 commented Jun 22, 2023

jhixson74 commented Jul 6, 2023

openshift-ci-robot commented Jul 7, 2023

openshift-ci-robot commented Jul 7, 2023

openshift-ci-robot commented Jul 7, 2023

openshift-ci-robot commented Jul 8, 2023

r4f4 commented Jul 10, 2023

openshift-ci-robot commented Jul 10, 2023

openshift-ci-robot commented Jul 10, 2023

openshift-ci-robot commented Jul 11, 2023

openshift-ci bot commented Jul 11, 2023 • edited

openshift-ci-robot commented Jul 11, 2023

r4f4 commented Jul 11, 2023

openshift-ci-robot commented Jul 11, 2023

openshift-ci-robot commented Jul 12, 2023

r4f4 commented Jul 12, 2023

openshift-cherrypick-robot commented Jul 12, 2023

openshift-ci bot commented Jul 11, 2023 •

edited