Add network package containing the NetworkInfo struct #2078

mmamczur · 2023-04-17T13:01:27Z

This struct holds the networking URLs that are now used when creating GCE resources (this commit contains changes for NEGs).
Currently only the default network information is always populated but it will be different for multi-networked services.

Why adding to PortInfo/ServicePort:

The controller creates syncers per service port. On update a diff is made with already used Ports vs the ones from the update. Adding the network to PortInfo will make the diff recognize any network change, so it will be possible to shut down the syncers for old network. Updating network for a running syncer would be difficult
there can be only one network set for the service so it could make sense to keep it on a service level but this introduced more complicated logic of handling syncers. It is possible that changing network won't be supported but still I find this solution cleaner.

k8s-ci-robot · 2023-04-17T13:01:37Z

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.

cezarygerard · 2023-04-20T13:04:16Z

pkg/network/network.go

+// ServiceNetwork determines the network data to be used for the LB resources.
+// This function currently returns only the default network but will provide
+// secondary networks information for multi-networked services in the future.
+func ServiceNetwork(_ *apiv1.Service, cloudProvider cloudNetworkProvider) (*NetworkInfo, error) {


ServiceNetwork => NewNetworkInfo?

I could rename it but the main point of this func is to figure out the network values for the service, with funcs named like NewAbc I soft of expect them to not do that much besides creating the struct

with multinetwork all logic to get the GCE VPC and subnetwork will go here

fair enough, let see how it evolves and decide later

pkg/neg/controller.go

cezarygerard · 2023-04-20T13:11:03Z

pkg/neg/controller_test.go

@@ -1030,6 +1010,7 @@ func TestMergeCSMPortInfoMap(t *testing.T) {
 	defer controller.stop()
 	n1s1 := newTestServiceCus(t, controller, "namespace1", "service1", []int32{80, 90})
 	n2s1 := newTestServiceCus(t, controller, "namespace2", "service1", []int32{90})
+	defaultNetwork := &network.NetworkInfo{K8sNetwork: "default"}


you can declare this defaultNetwork var at the very top of the file

in the

var ( ... )

section

pkg/neg/manager_test.go

cezarygerard · 2023-04-20T13:16:33Z

pkg/neg/syncers/transaction.go

@@ -393,6 +399,7 @@ func (s *transactionSyncer) ensureNetworkEndpointGroups() error {
 			s.recorder,
 			s.NegSyncerKey.GetAPIVersion(),
 			s.customName,
+			s.networkInfo,


why ensureNetworkEndpointGroup is not part of transactionSyncer 😲 ?

NWM...

Michał, this is just random comment I had reading the code.
you can skip it

cezarygerard · 2023-04-20T13:23:32Z

pkg/neg/syncers/utils.go

 		if networkEndpointType != negtypes.NonGCPPrivateEndpointType &&
 			// Only perform the following checks when the NEGs are not Non-GCP NEGs.
 			// Non-GCP NEGs do not have associated network and subnetwork.
-			(!utils.EqualResourceIDs(neg.Network, cloud.NetworkURL()) ||
-				!utils.EqualResourceIDs(neg.Subnetwork, cloud.SubnetworkURL())) {
+			(!utils.EqualResourceIDs(neg.Network, networkInfo.NetworkURL) ||
+				!utils.EqualResourceIDs(neg.Subnetwork, networkInfo.SubnetworkURL)) {


this if should be a method

if isNonGCPNEG(networkEndpointType, neg, networkInfo) {

...

ehhh...

Michał, this is just random comment I had reading the code.
you can skip it

pkg/neg/syncers/utils_test.go

cezarygerard · 2023-04-20T13:27:26Z

overall I like the idea of NetworkInfo struct

bowei · 2023-04-20T22:29:39Z

/ok-to-test

cezarygerard · 2023-04-24T09:02:14Z

/lgtm

This struct holds the networking URLs that are now used when creating GCE resources (this commit contains changes for NEGs). Currently only the default network information is always populated but it will be different for multi-networked services.

cezarygerard · 2023-04-24T11:40:12Z

/lgtm

k8s-ci-robot · 2023-04-24T11:40:22Z

[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

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 Apr 17, 2023

k8s-ci-robot requested review from aojea and code-elinka April 17, 2023 13:01

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 17, 2023

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 17, 2023

mmamczur force-pushed the multi-networking-network-info branch from 267d4ab to f86def6 Compare April 19, 2023 10:32

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 19, 2023

cezarygerard reviewed Apr 20, 2023

View reviewed changes

mmamczur force-pushed the multi-networking-network-info branch from f86def6 to c10c64d Compare April 20, 2023 14:05

mmamczur marked this pull request as ready for review April 20, 2023 16:37

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 Apr 20, 2023

k8s-ci-robot assigned cezarygerard Apr 24, 2023

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 24, 2023

mmamczur force-pushed the multi-networking-network-info branch from c10c64d to bcb2ea9 Compare April 24, 2023 10:26

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 24, 2023

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

k8s-ci-robot merged commit 8c7d784 into kubernetes:master Apr 24, 2023

mmamczur deleted the multi-networking-network-info branch July 25, 2023 07:02

mmamczur mentioned this pull request Aug 9, 2023

REQUEST: New membership for mmamczur kubernetes/org#4372

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add network package containing the NetworkInfo struct #2078

Add network package containing the NetworkInfo struct #2078

mmamczur commented Apr 17, 2023

k8s-ci-robot commented Apr 17, 2023

cezarygerard Apr 20, 2023

mmamczur Apr 20, 2023

cezarygerard Apr 20, 2023

cezarygerard Apr 20, 2023

cezarygerard Apr 20, 2023

cezarygerard Apr 20, 2023

cezarygerard Apr 20, 2023

cezarygerard Apr 20, 2023

cezarygerard commented Apr 20, 2023

bowei commented Apr 20, 2023

cezarygerard commented Apr 24, 2023

cezarygerard commented Apr 24, 2023

k8s-ci-robot commented Apr 24, 2023

Add network package containing the NetworkInfo struct #2078

Add network package containing the NetworkInfo struct #2078

Conversation

mmamczur commented Apr 17, 2023

k8s-ci-robot commented Apr 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezarygerard commented Apr 20, 2023

bowei commented Apr 20, 2023

cezarygerard commented Apr 24, 2023

cezarygerard commented Apr 24, 2023

k8s-ci-robot commented Apr 24, 2023