Fix network plugin detection #1061

mkimuram · 2021-02-03T00:20:28Z

This PR fixes network plugin detection by:

Making detection logic of each network plugin or cidrs not to fail on internal error to properly fall through,
Adding logic to detect pod cidrs and cluster cidrs for k8s w/o k8s pods(api-server, controller, kube-proxy)

mkimuram · 2021-02-03T00:31:47Z

I've tested by below way and it seems to work well:

[Build environment]

# make build
# make images
# docker save quay.io/submariner/submariner-operator:dev > /tmp/submariner-operator.tar

(Copy bin/subctl and /tmp/submariner-operator.tar to test environment.)

[Test environment (after k3s is deployed)]

# k3s ctr images import submariner-operator.tar
# k3s ctr images tag quay.io/submariner/submariner-operator:dev quay.io/submariner/submariner-operator-dev:dev
# k3s ctr images list | grep submariner

# ./subctl deploy-broker --globalnet --globalnet-cidr-range 169.254.0.0/16

# ./subctl join broker-info.subm --clusterid cl1 --clustercidr 10.44.0.0/16 --servicecidr 10.45.0.0/16 --natt=false --globalnet-cidr 169.254.44.0/24 --image-override submariner-operator=quay.io/submariner/submariner-operator-dev:dev

# kubectl edit deploy -n submariner-operator submariner-operator

(Change imagePullPolicy to Never.)

# watch kubectl get pod -n submariner-operator

# ./subctl show networks

Showing network details for cluster "default":
    Discovered network details:
        Network plugin:  generic
        Service CIDRs:   [10.45.0.0/16]
        Cluster CIDRs:   [10.44.0.0/24]

# iptables -t nat -S POSTROUTING | head -n 2
-P POSTROUTING ACCEPT
-A POSTROUTING -j SUBMARINER-POSTROUTING

# kubectl logs -n submariner-operator -l app=submariner-routeagent
I0202 22:58:31.139620       1 routes_iface.go:121] Found nil gw or dst
I0202 22:58:31.139631       1 gw_transition.go:40] Creating the vxlan interface: vx-submariner on the gateway node
I0202 22:58:31.140418       1 vxlan.go:164] Successfully added the bridge fdb entry 192.168.122.67 00:00:00:00:00:00
I0202 22:58:31.140562       1 vxlan.go:251] Successfully configured rp_filter to loose mode(2) on vx-submariner
I0202 22:58:33.297912       1 handler.go:78] A Node with name "k3s" and addresses []v1.NodeAddress{v1.NodeAddress{Type:"InternalIP", Address:"192.168.122.67"}, v1.NodeAddress{Type:"Hostname", Address:"k3s"}} has been updated
I0202 22:58:33.297953       1 node_handler.go:30] A Node with name "k3s" and addresses []v1.NodeAddress{v1.NodeAddress{Type:"InternalIP", Address:"192.168.122.67"}, v1.NodeAddress{Type:"Hostname", Address:"k3s"}} has been updated
I0202 22:58:38.924489       1 handler.go:78] A Node with name "k3s" and addresses []v1.NodeAddress{v1.NodeAddress{Type:"InternalIP", Address:"192.168.122.67"}, v1.NodeAddress{Type:"Hostname", Address:"k3s"}} has been updated
I0202 22:58:38.924530       1 node_handler.go:30] A Node with name "k3s" and addresses []v1.NodeAddress{v1.NodeAddress{Type:"InternalIP", Address:"192.168.122.67"}, v1.NodeAddress{Type:"Hostname", Address:"k3s"}} has been updated
I0202 23:03:41.255994       1 handler.go:78] A Node with name "k3s" and addresses []v1.NodeAddress{v1.NodeAddress{Type:"InternalIP", Address:"192.168.122.67"}, v1.NodeAddress{Type:"Hostname", Address:"k3s"}} has been updated
I0202 23:03:41.256036       1 node_handler.go:30] A Node with name "k3s" and addresses []v1.NodeAddress{v1.NodeAddress{Type:"InternalIP", Address:"192.168.122.67"}, v1.NodeAddress{Type:"Hostname", Address:"k3s"}} has been updated

pkg/discovery/network/generic.go

tpantelis · 2021-02-03T01:50:24Z

pkg/discovery/network/generic.go

+		}
+	}
+
+	return "", fmt.Errorf("no node with Spec.PodCIDR found")


I don't think we should return an error here if not found. We should let the caller handle not found as it sees fit (it so happens this is the last resort for the caller but that could change).

pkg/discovery/network/generic.go

pkg/discovery/network/network_suite_test.go

pkg/discovery/network/generic_test.go

mangelajo

see inline comments, but the new service heuristic is good IMO, "cluster-dump" equivalent is what we do on the other heuristics.

mkimuram

@tpantelis

I've fixed almost as suggested. Please see my inline comments.

pkg/discovery/network/generic.go

pkg/discovery/network/generic_test.go

pkg/discovery/network/pods.go

pkg/discovery/network/generic.go

mkimuram · 2021-02-03T23:24:16Z

pkg/discovery/network/generic.go

@@ -89,7 +97,7 @@ func findClusterIPRangeSvcCreation(clientSet kubernetes.Interface) (string, erro

 	// creating invalid svc didn't fail as expected
 	if err == nil {
-		return "", fmt.Errorf("creating invalid service(%v) didn't fail", invalidSvcSpec)
+		return "", nil


Keeping this return error makes unit tests complex. All existing tests for network plugin detection will need to rely on creating svc always return error.

So this means the API server didn't behave as we expected, likely something changed. I think it would be useful to at least log a message:

status.QueueWarningMessage("Could not determine the service IP range via service creation - the expected error was not returned")

Changed it to return an error properly and changed unit tests as well.

mkimuram · 2021-02-03T23:28:01Z

pkg/discovery/network/generic.go

+			},
+		},
+	}
+	_, err := clientSet.CoreV1().Services("submariner-operator").Create(invalidSvcSpec)


Creating svc in submariner-operator namespace might fail if it is called before the namespace is created. It seems to happen even on subctl join command. Other candidate will be kube-system, default, and random namespace created just for this check. Any ideas?

I think we could use empty namespace but, if not, default.

This code is called from both command line and operator.
subctl actually calls it on subctl join before submariner-namespace is created, so it shouldn't create service in submariner-operator namespace. On the other hand, the operator is only allowed to create resources inside submariner-operator namespace, so it shouldn't create service in default namespace.

Therefore, the code was changed to check whether it is running in operator and to create service in the proper namespace.

pkg/discovery/network/canal_test.go

pkg/discovery/network/generic.go

tpantelis · 2021-02-04T00:07:48Z

pkg/discovery/network/generic.go

+			},
+		},
+	}
+	_, err := clientSet.CoreV1().Services("submariner-operator").Create(invalidSvcSpec)


I think we could use empty namespace but, if not, default.

pkg/discovery/network/generic.go

pkg/discovery/network/generic_test.go

pkg/discovery/network/weavenet_test.go

pkg/discovery/network/generic_test.go

pkg/discovery/network/canal_test.go

mkimuram · 2021-02-05T23:31:51Z

@tpantelis

Thank you so much for your review with detailed suggestions. PTAL

tpantelis · 2021-02-06T05:36:49Z

pkg/discovery/network/generic.go

+	}
+
+	// decide which namespace to create the service
+	ns := os.Getenv("WATCH_NAMESPACE")


What is WATCH_NAMESPACE and how is it set?

This is set here as an environment variable of the operator pod. I think that this is used by operator-sdk's GetWatchNamespace.

OK - could you add a comment explaining this? eg "WATCH_NAMESPACE will be present if running in the operator pod".

Another approach is to pass in the NS so we don't rely on the presence of an env var. The operator code implicitly knows its own namespace. I think this would make it clearer and although it would mean passing a param down the chain.

Changed and squashed.

tpantelis · 2021-02-06T05:39:55Z

pkg/discovery/network/generic.go

+	// decide which namespace to create the service
+	ns := os.Getenv("WATCH_NAMESPACE")
+	// use "submariner-operator" if WATCH_NAMESPACE is "submariner-operator" (in operator)
+	if ns != "submariner-operator" {


There's a constant for this in root.go.

It seems that importing constant from pkg/subctl/cmd to pkg/discovery/network/generic.go makes import cycle not allowed error. Is it OK to just define constant in pkg/discovery/network/generic.go or do we need to move the constant to the other place to share it?

Note that as explained, subctl create the operator pod in the namespace and pass the env variable via the downward API (metadata.namespace). Then inside the operator, it checks whether env variable is set to check if it runs inside the operator. So, this constant is not directly shared across these functions.

If we pass the NS into the function as described above, this becomes moot.

tpantelis · 2021-02-06T05:53:43Z

pkg/discovery/network/generic.go

+
+	// creating invalid service didn't fail as expected
+	if err == nil {
+		return "", fmt.Errorf("creating invalid service(%v) didn't fail", invalidSvcSpec)


I prefer this the way you had it, ie not return error here or on L128. This makes it consistent with the other discovery methods and, on join, will result in the user being prompted. But, as I noted earlier, I think it's worth logging a message as the API server didn't behave as expected.

The callers of discoverGenericNetwork are as follows:

[Operator] Reconcile discoverNetwork getClusterNetwork Discover discoverGenericNetwork [Command line] getNetworkDetails Discover discoverGenericNetwork if err != nil { status.QueueWarningMessage(fmt.Sprintf("Error trying to discover network details: %s", err)) }

So, as for command line case, the error is already been passed to status.QueueWarningMessage.

And, other functions called from discoverGenericNetwork, like findClusterIPRangeFromApiserver actually returns error and not catching it with status.QueueWarningMessage.

Therefore, the code should be already as you expected.

My point was about whether it should return an error in this case, ie should we treat it like we didn't find the data similar to the other discovery methods. So for subclt join, this would at least allow it to prompt the user as a last resort. After all, this is just another attempt to find the data. This seems reasonable but it would be beneficial to print a warning. @mangelajo WDYT?

Waiting for @mangelajo 's comment. But I'm now inclined to return nil and log error here, only for this case.

Meanwhile, I would like to confirm that it is safe to call status.QueueWarningMessage outside CLI's context, or from operator. It looks like a package under pkg/internal/cli and I'm not familiar with its implementation details.

Unfortunately status.QueueWarningMessage wouldn't be appropriate in the operator. The operator uses a different logging API. We could pass in a function to log warnings.

Added logging code in controllers/submariner/submariner_networkdiscovery.go for operator to log it (See the first commit). Also, re-organized and squashed commits to review easier.

tpantelis · 2021-02-06T06:01:03Z

pkg/discovery/network/canal_test.go

 		})
 	})

-	When("There is a kubeapi pod at least ", func() {
+	When("There is a kube-api pod at least ", func() {


Suggested change

When("There is a kube-api pod at least ", func() {

When("There is a kube-api pod", func() {

sridhargaddam · 2021-02-10T09:04:24Z

pkg/discovery/network/generic_test.go

+		var clusterNet *ClusterNetwork
+
+		BeforeEach(func() {
+			clusterNet = testDiscoverGenericWith(
 				fakePod("kube-controller", []string{"kube-controller", "--cluster-ABCD=1.2.3.4"}, []v1.EnvVar{}),


Suggested change

fakePod("kube-controller", []string{"kube-controller", "--cluster-ABCD=1.2.3.4"}, []v1.EnvVar{}),

fakePod("kube-controller-manager", []string{"kube-controller-manager", "--cluster-ABCD=1.2.3.4"}, []v1.EnvVar{}),

In the code, we are actually checking for component kube-controller-manager and this test-case is about validating the arguments (i.e., no expected params).

Thank you for pointing it out. Fixed.

sridhargaddam · 2021-02-10T09:07:06Z

pkg/discovery/network/generic_test.go

+		var clusterNet *ClusterNetwork
+
+		BeforeEach(func() {
+			clusterNet = testDiscoverGenericWith(
 				fakePod("kube-controller", []string{"kube-api", "--cluster-ABCD=1.2.3.4"}, []v1.EnvVar{}),


Suggested change

fakePod("kube-controller", []string{"kube-api", "--cluster-ABCD=1.2.3.4"}, []v1.EnvVar{}),

fakePod("kube-apiserver", []string{"kube-apiserver", "--cluster-ABCD=1.2.3.4"}, []v1.EnvVar{}),

Fixed as well.

Signed-off-by: Masaki Kimura <masaki.kimura@hitachivantara.com>

sridhargaddam

Thanks for the PR and also addressing the comments @mkimuram

mangelajo

This looks good to me now, the additional heuristics will make this much more robust. Thank you @mkimuram .

mangelajo · 2021-02-15T09:43:20Z

@tpantelis have an eye, I belive mkimura handled all your comments.

tpantelis · 2021-02-15T15:36:11Z

@tpantelis have an eye, I belive mkimura handled all your comments.

Mostly. So we're going to fail fast on join if the service creation API doesn't fail with the CIDR info as expected. I'm fine with that but I'd like to make the messages clearer and add a comment wrt to the WATCH_NAMESPACE. I can push a follow-up PR.

mkimuram requested review from mangelajo, Oats87, skitt and tpantelis as code owners February 3, 2021 00:20

tpantelis requested changes Feb 3, 2021

View reviewed changes

mangelajo reviewed Feb 3, 2021

View reviewed changes

mkimuram force-pushed the issue/1116v3 branch 2 times, most recently from 3866d7a to d89fbc6 Compare February 3, 2021 23:01

mkimuram commented Feb 3, 2021

View reviewed changes

tpantelis requested changes Feb 4, 2021

View reviewed changes

Base automatically changed from master to devel February 4, 2021 09:37

mkimuram force-pushed the issue/1116v3 branch from d89fbc6 to a6ef93b Compare February 5, 2021 23:13

tpantelis reviewed Feb 6, 2021

View reviewed changes

mkimuram force-pushed the issue/1116v3 branch from ed5a74c to 06ad059 Compare February 9, 2021 00:50

sridhargaddam reviewed Feb 10, 2021

View reviewed changes

mkimuram added 6 commits February 10, 2021 15:49

Log error on failure in discovering network plugin in operator

4295ae9

Signed-off-by: Masaki Kimura <masaki.kimura@hitachivantara.com>

Clearly make findPod not return error when no pod found

863f37d

Signed-off-by: Masaki Kimura <masaki.kimura@hitachivantara.com>

Refactor discoverGenericNetwork

62ba069

Signed-off-by: Masaki Kimura <masaki.kimura@hitachivantara.com>

Apply weave's service cidr detection logic to canal

62c8624

Signed-off-by: Masaki Kimura <masaki.kimura@hitachivantara.com>

Add fall-back logic to detect pod cidrs from node spec

0dea170

Signed-off-by: Masaki Kimura <masaki.kimura@hitachivantara.com>

Add fall-back logic to detect service cidrs from invalid svc creation

317aaf6

Signed-off-by: Masaki Kimura <masaki.kimura@hitachivantara.com>

mkimuram force-pushed the issue/1116v3 branch from 06ad059 to 317aaf6 Compare February 10, 2021 17:23

sridhargaddam approved these changes Feb 10, 2021

View reviewed changes

Merge branch 'devel' into issue/1116v3

ed9aa37

mangelajo approved these changes Feb 15, 2021

View reviewed changes

mangelajo requested a review from tpantelis February 15, 2021 09:42

tpantelis approved these changes Feb 15, 2021

View reviewed changes

tpantelis added 2 commits February 15, 2021 10:37

Merge branch 'devel' into issue/1116v3

597bff5

Merge branch 'devel' into issue/1116v3

c62e1e5

tpantelis merged commit e4fac24 into submariner-io:devel Feb 15, 2021

tpantelis mentioned this pull request Feb 16, 2021

Update service creation error parsing messages #1092

Merged

nyechiel added this to the 0.9-m1 milestone Feb 16, 2021

mkimuram mentioned this pull request Feb 7, 2022

REQUEST: New Membership request for mkimuram submariner-io/submariner#1688

Closed

6 tasks

	When("There is a kube-api pod at least ", func() {
	When("There is a kube-api pod", func() {

	fakePod("kube-controller", []string{"kube-controller", "--cluster-ABCD=1.2.3.4"}, []v1.EnvVar{}),
	fakePod("kube-controller-manager", []string{"kube-controller-manager", "--cluster-ABCD=1.2.3.4"}, []v1.EnvVar{}),

	fakePod("kube-controller", []string{"kube-api", "--cluster-ABCD=1.2.3.4"}, []v1.EnvVar{}),
	fakePod("kube-apiserver", []string{"kube-apiserver", "--cluster-ABCD=1.2.3.4"}, []v1.EnvVar{}),

Fix network plugin detection #1061

Fix network plugin detection #1061

Conversation

mkimuram commented Feb 3, 2021

mkimuram commented Feb 3, 2021

Choose a reason for hiding this comment

mangelajo left a comment

Choose a reason for hiding this comment

mkimuram left a 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

Choose a reason for hiding this comment

mkimuram commented Feb 5, 2021

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sridhargaddam left a comment

Choose a reason for hiding this comment

mangelajo left a comment

Choose a reason for hiding this comment

mangelajo commented Feb 15, 2021

tpantelis commented Feb 15, 2021 • edited Loading

tpantelis commented Feb 15, 2021 •

edited

Loading