Multi networking support in the L4 ILB controller #2013

mmamczur · 2023-03-15T15:42:44Z

This PR contains commits implementing multi networking support for L4 ILBs

k8s-ci-robot · 2023-03-15T15:42:53Z

Hi @mmamczur. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

bowei · 2023-03-20T06:05:36Z

/ok-to-test

bowei · 2023-03-20T06:46:00Z

/hold

bowei · 2023-03-20T06:46:05Z

/assign

mmamczur · 2023-03-24T09:51:28Z

I've added another PR that has the commits from here but up to changes in the NEG controller only #2023

code-elinka · 2023-05-18T13:30:30Z

/assign @cezarygerard
/assign @code-elinka

cezarygerard · 2023-05-21T23:37:27Z

pkg/backends/regional_ig_linker_test.go

@@ -229,7 +230,7 @@ func createBackendService(t *testing.T, sp utils.ServicePort, backendPool *Backe
 	t.Helper()
 	namespacedName := types.NamespacedName{Name: "service.Name", Namespace: "service.Namespace"}
 	protocol := string(apiv1.ProtocolTCP)
-	if _, err := backendPool.EnsureL4BackendService(sp.BackendName(), hcLink, protocol, string(apiv1.ServiceAffinityNone), string(cloud.SchemeExternal), namespacedName); err != nil {
+	if _, err := backendPool.EnsureL4BackendService(sp.BackendName(), hcLink, protocol, string(apiv1.ServiceAffinityNone), string(cloud.SchemeExternal), namespacedName, network.NetworkInfo{IsDefault: true}); err != nil {


there are 6 operations happening in this 1 line

can you create variables for args in EnsureL4BackendService?

extracted most of them

pkg/firewalls/firewalls_l4_test.go

cezarygerard · 2023-05-21T23:44:07Z

pkg/l4lb/l4controller.go

@@ -227,6 +237,10 @@ func (l4c *L4Controller) processServiceCreateOrUpdate(service *v1.Service) *load
 	if err != nil {
 		return &loadbalancers.L4ILBSyncResult{Error: err}
 	}
+	network, err := network.ServiceNetwork(service, l4c.networkLister, l4c.gkeNetworkParamSetLister, l4c.ctx.Cloud, klog.TODO())


why you needed to pass the logger there?
looks weird

it was a request from Swetha in the changes for NEG controller. They have this scoped log there and want to use it in the code they call. This function is shared in both controllers so I now have to pass some logger

pkg/loadbalancers/forwarding_rules_test.go

cezarygerard · 2023-05-21T23:56:15Z

pkg/loadbalancers/forwarding_rules_test.go

@@ -323,3 +396,108 @@ func TestL4EnsureIPv4ForwardingRuleAddressAlreadyInUse(t *testing.T) {
 	require.Error(t, err)
 	assert.True(t, utils.IsIPConfigurationError(err))
 }
+
+func TestL4EnsureIPv4ForwardingRule(t *testing.T) {


I wonder if creating 2 separate tests would be more readable (instead of table test)

the logic changed a bit so that the code always uses the value from network info - no need for 2 cases anymore. I removed the table

cezarygerard · 2023-05-23T16:18:40Z

that looks good to me

do we need a review from @bowei @swetharepakula OR @aojea ?

mmamczur · 2023-05-24T07:26:52Z

that looks good to me

do we need a review from @bowei @swetharepakula OR @aojea ?

I don't think so, this touches only L4. But everyone is welcome to review

cezarygerard · 2023-05-24T14:50:03Z

/lgtm

k8s-ci-robot · 2023-05-24T14:50:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cezarygerard, mmamczur

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~~ [cezarygerard]

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

cezarygerard · 2023-05-25T09:19:20Z

@bowei I am unholding this change
it does not affect L7
the feature is behind the flags
and we want to move forward with testing

/unhold

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 15, 2023

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 15, 2023

k8s-ci-robot requested review from aojea and kl52752 March 15, 2023 15:43

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2023

mmamczur force-pushed the multi-networking branch from 7954f06 to d094c18 Compare March 16, 2023 14:45

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2023

mmamczur force-pushed the multi-networking branch from d094c18 to ed16f49 Compare March 17, 2023 15:53

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 20, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 20, 2023

k8s-ci-robot assigned bowei Mar 20, 2023

mmamczur force-pushed the multi-networking branch from ed16f49 to 0555ca6 Compare March 20, 2023 08:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2023

mmamczur force-pushed the multi-networking branch from 0555ca6 to 061c480 Compare March 21, 2023 16:25

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2023

mmamczur force-pushed the multi-networking branch 2 times, most recently from 2f2f14d to 280e0e3 Compare March 24, 2023 09:49

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2023

mmamczur force-pushed the multi-networking branch 2 times, most recently from df633ce to 19a11eb Compare April 7, 2023 12:36

mmamczur force-pushed the multi-networking branch 4 times, most recently from 71266b9 to 8ae07d2 Compare May 16, 2023 14:19

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 16, 2023

mmamczur force-pushed the multi-networking branch from 8ae07d2 to d0dce71 Compare May 18, 2023 10:08

mmamczur changed the title ~~Multi networking support in NEG and L4 ILB controllers~~ Multi networking support in the L4 ILB controller May 18, 2023

mmamczur marked this pull request as ready for review May 18, 2023 10:16

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 18, 2023

k8s-ci-robot requested a review from code-elinka May 18, 2023 10:16

k8s-ci-robot assigned cezarygerard and code-elinka May 18, 2023

cezarygerard reviewed May 21, 2023

View reviewed changes

Multi Network support for the ILB L4 controller.

1255994

mmamczur force-pushed the multi-networking branch from d0dce71 to 1255994 Compare May 23, 2023 14:15

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 23, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 24, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2023

k8s-ci-robot merged commit 56f9a89 into kubernetes:master May 25, 2023

mmamczur mentioned this pull request Aug 9, 2023

REQUEST: New membership for mmamczur kubernetes/org#4372

Closed

9 tasks

mmamczur deleted the multi-networking branch April 10, 2024 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi networking support in the L4 ILB controller #2013

Multi networking support in the L4 ILB controller #2013

mmamczur commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023

bowei commented Mar 20, 2023

bowei commented Mar 20, 2023

bowei commented Mar 20, 2023

mmamczur commented Mar 24, 2023 •

edited

Loading

code-elinka commented May 18, 2023

cezarygerard May 21, 2023

mmamczur May 23, 2023

cezarygerard May 21, 2023

mmamczur May 23, 2023

cezarygerard May 21, 2023

mmamczur May 23, 2023

cezarygerard commented May 23, 2023

mmamczur commented May 24, 2023

cezarygerard commented May 24, 2023

k8s-ci-robot commented May 24, 2023

cezarygerard commented May 25, 2023

Multi networking support in the L4 ILB controller #2013

Multi networking support in the L4 ILB controller #2013

Conversation

mmamczur commented Mar 15, 2023

k8s-ci-robot commented Mar 15, 2023

bowei commented Mar 20, 2023

bowei commented Mar 20, 2023

bowei commented Mar 20, 2023

mmamczur commented Mar 24, 2023 • edited Loading

code-elinka commented May 18, 2023

cezarygerard May 21, 2023

Choose a reason for hiding this comment

mmamczur May 23, 2023

Choose a reason for hiding this comment

cezarygerard May 21, 2023

Choose a reason for hiding this comment

mmamczur May 23, 2023

Choose a reason for hiding this comment

cezarygerard May 21, 2023

Choose a reason for hiding this comment

mmamczur May 23, 2023

Choose a reason for hiding this comment

cezarygerard commented May 23, 2023

mmamczur commented May 24, 2023

cezarygerard commented May 24, 2023

k8s-ci-robot commented May 24, 2023

cezarygerard commented May 25, 2023

mmamczur commented Mar 24, 2023 •

edited

Loading