New autodetection method KubernetesInternalIP added #1242

hanamantagoudvk · 2021-10-11T09:19:46Z

Description

New autodetection method KubernetesInternalIP / KubernetesInternalIP6 being introduced so that calico can always pick the node IP deterministically.

Fixes projectcalico/calico#4870

Fixes projectcalico/calico#5090

CLAassistant · 2021-10-11T09:19:55Z

All committers have signed the CLA.

caseydavenport · 2021-10-19T21:42:39Z

Tagging for v3.22

hanamantagoudvk · 2021-10-21T12:37:38Z

@caseydavenport : Could you please prioritize it for v3.21 ? Our customer deployment needs this solution.

pkg/lifecycle/startup/startup.go

caseydavenport

First pass - thanks for the PR! This is mostly looking good but I think we need to make a few structural changes to reduce the scale impact that a periodic node query on every node in the cluster can have. We'll also need unit tests for this in order to merge.

Let me know if you have any questions!

pkg/lifecycle/startup/startup.go

caseydavenport · 2021-11-02T16:14:29Z

pkg/lifecycle/startup/startup.go

+	var config *rest.Config
+	var k8sNode *v1.Node
+	var err error
+	if config, err = rest.InClusterConfig(); err != nil {


This code needs to work on non-kubernetes clusters as well - I'd suggest modeling after the logic above: https://github.com/projectcalico/node/pull/1242/files#diff-e481557bd1e2fed4cea80458ebb02e18797fdf49c42944f4a8d6c23cbfbec3f9R130

modified as per suggestion

caseydavenport · 2021-11-02T16:15:19Z

pkg/lifecycle/startup/startup.go

@@ -772,6 +809,27 @@ func autoDetectCIDRBySkipInterface(ifaceRegexes []string, version int) *cnet.IPN
 	return cidr
 }

+// autoDetectUsingK8sInternalIP reads K8s Node InternalIP.
+func autoDetectUsingK8sInternalIP(version int, k8sNode *v1.Node) *cnet.IPNet {


should probably put this in the autodetection package along with the other methods.

It would require moving the getLocalCIDR function with it, but I think that's OK

modified as per suggestion

pkg/lifecycle/startup/startup.go

caseydavenport · 2021-11-09T22:59:30Z

@hanamantagoudvk just checking in on this one - did you see my latest comments?

hanamantagoudvk · 2021-11-10T15:05:08Z

@hanamantagoudvk just checking in on this one - did you see my latest comments?

@caseydavenport : Sorry for the delay. Uploaded the new patchset. I am still not able to run UT cases in my setup.
I get some error related to setup. I am working on UT part.

tomastigera

I have nothing major against this change, tbh I like that it is coming, we did consider it for eBPF and then we went different way. So great! 👍 I will leave it to @caseydavenport to approve.

pkg/lifecycle/startup/autodetection/autodetection_methods.go

tomastigera · 2021-11-17T00:12:58Z

pkg/lifecycle/startup/startup_test.go

@@ -78,6 +100,32 @@ func makeNode(ipv4 string, ipv6 string) *libapi.Node {
 	return n
 }

+// makeK8sNode creates an v1.Node with some Addresses populated.
+func makeK8sNode(ipv4 string, ipv6 string) *v1.Node {


this function is used only with ipv4 ...

same comment as below

tomastigera · 2021-11-17T00:19:50Z

pkg/lifecycle/startup/startup_test.go

+		ip6, ip6net, _ := net.ParseCIDR(ipv6)
+		// Guard against nil here in case we pass in an empty string for IPv6.
+		if ip6 != nil {
+			ip6net.IP = ip6.IP
+		}
+		ip6Addr = ip6net.String()


why don't we do this for ipv4? TBH this is test code and we can assume that the test passes in valid addresses, nothing that changes dynamically. It would be more interesting to test different permutations, mix with other address types etc.

yes i want to add various combinations with IPv4 and IPv6 addresses, but not able to mock the GetInterfaces method inside autoDetectUsingK8sInternalIP. Due to that , configureIPsAndSubnets always returns false

pkg/lifecycle/startup/startup_test.go

caseydavenport

I mostly don't understand why we're now passing DEFAULT_INTERFACES_TO_EXCLUDE as an argument through several layers of call stack, especially since it's not used in the new autodetection method. We should revert that.

Otherwise this is looking good.

pkg/lifecycle/startup/autodetection/autodetection_methods.go

caseydavenport · 2021-11-19T17:35:06Z

pkg/lifecycle/startup/startup.go

 	}

-	configureAndCheckIPAddressSubnets(ctx, cli, node)
+	configureAndCheckIPAddressSubnets(ctx, cli, node, k8sNode, DEFAULT_INTERFACES_TO_EXCLUDE)


I think we should revert this change to pass DEFAULT_INTERFACES_TO_EXCLUDE all the way through the call stack of the autodetection functions. It seems to be an extra argument just to pass a constant value.

pkg/lifecycle/startup/startup_test.go

caseydavenport · 2021-11-19T17:49:01Z

/sem-approve

caseydavenport · 2021-11-19T20:26:21Z

@hanamantagoudvk you've got an import cycle looks like:

package command-line-arguments01:39
	imports github.com/projectcalico/node/pkg/lifecycle/startup01:39
	imports github.com/projectcalico/node/pkg/lifecycle/startup/autodetection01:39
	imports github.com/projectcalico/node/pkg/lifecycle/startup: import cycle not allowed01:39
Makefile:281: recipe for target 'dist/bin//calico-node-amd64' failed

hanamantagoudvk · 2021-11-20T06:40:20Z

package command-line-arguments01:39
	imports github.com/projectcalico/node/pkg/lifecycle/startup01:39
	imports github.com/projectcalico/node/pkg/lifecycle/startup/autodetection01:39
	imports github.com/projectcalico/node/pkg/lifecycle/startup: import cycle not allowed01:39
Makefile:281: recipe for target 'dist/bin//calico-node-amd64' failed

@caseydavenport
The reason for above import cycle is DEFAULT_INTERFACES_TO_EXCLUDE . Due to split in code now , DEFAULT_INTERFACES_TO_EXCLUDE has remained in startup package and its getting used in autodetection package. autodetection package imports startup package and vice-versa.

caseydavenport · 2021-11-22T21:51:56Z

The reason for above import cycle is DEFAULT_INTERFACES_TO_EXCLUDE . Due to split in code now , DEFAULT_INTERFACES_TO_EXCLUDE has remained in startup package and its getting used in autodetection package. autodetection package imports startup package and vice-versa.

Should probably just move DEFAULT_INTERFACES_TO_EXCLUDE to the autodetection package then, right?

hanamantagoudvk · 2021-11-23T05:27:19Z

The reason for above import cycle is DEFAULT_INTERFACES_TO_EXCLUDE . Due to split in code now , DEFAULT_INTERFACES_TO_EXCLUDE has remained in startup package and its getting used in autodetection package. autodetection package imports startup package and vice-versa.

Should probably just move DEFAULT_INTERFACES_TO_EXCLUDE to the autodetection package then, right?

@caseydavenport
yes now i have moved DEFAULT_INTERFACES_TO_EXCLUDE into the autodetection pkg.

hanamantagoudvk · 2021-11-28T12:30:29Z

@caseydavenport : Please review it and let me know if anything is pending ? I am not able to see the below errors in General Tests since i dont have access to CI system.

caseydavenport

This LGTM - Thanks for all of the hard work on this @hanamantagoudvk

caseydavenport · 2021-12-07T18:54:00Z

@hanamantagoudvk just a static check failure it looks like:

pkg/lifecycle/startup/startup_test.go:86:5: composites: `k8s.io/api/core/v1.NodeAddress` composite literal uses unkeyed fields (govet)04:51
				{v1.NodeInternalIP, ipv4},04:51
				^04:51
pkg/lifecycle/startup/startup_test.go:87:5: composites: `k8s.io/api/core/v1.NodeAddress` composite literal uses unkeyed fields (govet)04:52
				{v1.NodeInternalIP, ipv6},04:52
				^

hanamantagoudvk · 2021-12-09T17:16:03Z

@caseydavenport , what else is failing now ?

caseydavenport · 2021-12-09T20:26:46Z

I think they were test flakes, kicked off another build.

caseydavenport · 2021-12-09T21:54:53Z

UT for Node IP assignment and conflict checking. Test variations on how IPs are detected. [It] Test with no "IP" env var set 02:25
/go/src/github.com/projectcalico/node/pkg/lifecycle/startup/startup_test.go:90602:25
  Expected02:25
      <bool>: false02:25
  to equal02:25
      <bool>: true02:25
  /go/src/github.com/projectcalico/node/pkg/lifecycle/startup/startup_test.go:902

This test appears to be failing consistently

caseydavenport · 2021-12-10T17:15:56Z

@hanamantagoudvk thanks for sticking with this! Glad we got it merged.

What was the last issue with that test? I couldn't tell because the commits were squashed.

Also, are you planning to do documentation for this PR? If so I can help.

Similarly, we probably want operator support for this feature as well - I can pick that up if you're strapped for time.

hanamantagoudvk · 2021-12-10T17:24:42Z

@caseydavenport , basically ginkgo orders the testcases alphabetically. So the tests i added were run before the existing one and it was setting the IP env variable and not resetting it. So due to that the testcases were failing. In my local setup , it used to pass on and off. So now i put a code to reset those env variables.

Regarding documentation , yes i can do the documentation.

Ya that operator support , you can pick that up.

hanamantagoudvk · 2021-12-12T08:01:05Z

@caseydavenport , regarding documentation , i believe in calico website where different autodetection methods are listed , we need to this new one as well. Other than anywhere else we need to capture this one ?

If i need to modify the calico website content , which repo i need to look at and modify it ?

marvin-tigera added this to the Calico v3.21.0 milestone Oct 11, 2021

marvin-tigera added docs-pr-required release-note-required labels Oct 11, 2021

caseydavenport modified the milestones: Calico v3.21.0, Calico v3.22.0 Oct 19, 2021

tomastigera reviewed Oct 21, 2021

View reviewed changes

pkg/lifecycle/startup/startup.go Outdated Show resolved Hide resolved

hanamantagoudvk force-pushed the vip-issue branch from 630b3cf to dc1220a Compare October 22, 2021 06:01

hanamantagoudvk mentioned this pull request Oct 25, 2021

TCP listen on port 179 on all interface addresses creates security vulnerability projectcalico/calico#4974

Closed

caseydavenport requested changes Oct 26, 2021

View reviewed changes

hanamantagoudvk force-pushed the vip-issue branch from dc1220a to fcce869 Compare October 29, 2021 07:33

caseydavenport requested changes Nov 2, 2021

View reviewed changes

hanamantagoudvk force-pushed the vip-issue branch from fcce869 to 95fae8a Compare November 10, 2021 15:02

hanamantagoudvk force-pushed the vip-issue branch 4 times, most recently from 8928fb7 to be54246 Compare November 15, 2021 08:44

tomastigera reviewed Nov 17, 2021

View reviewed changes

hanamantagoudvk force-pushed the vip-issue branch 2 times, most recently from f7ff901 to 10b46d9 Compare November 19, 2021 11:35

caseydavenport requested changes Nov 19, 2021

View reviewed changes

hanamantagoudvk force-pushed the vip-issue branch from 10b46d9 to 1158d9a Compare November 19, 2021 19:14

caseydavenport mentioned this pull request Nov 19, 2021

Node 'changes IP' and causes loss of connectivity projectcalico/calico#5090

Closed

hanamantagoudvk force-pushed the vip-issue branch from 1158d9a to 4b9eb96 Compare November 23, 2021 05:19

caseydavenport approved these changes Dec 7, 2021

View reviewed changes

hanamantagoudvk force-pushed the vip-issue branch from 4b9eb96 to 5df8bf5 Compare December 8, 2021 05:59

New autodetection method KubernetesInternalIP added

2a36b23

hanamantagoudvk force-pushed the vip-issue branch from 5df8bf5 to 2a36b23 Compare December 10, 2021 07:20

caseydavenport merged commit a8098c7 into projectcalico:master Dec 10, 2021

caseydavenport mentioned this pull request Dec 13, 2021

Use kubernetes Node status' addresses ExternalIP field for autodetecting node-to-node mesh BGP peer ip projectcalico/calico#5154

Open

This was referenced Dec 15, 2021

Updating documentation for new autodetection method kubernetes-internal-ip projectcalico/calico#5185

Closed

Updating documentation for new autodetection method kubernetes-internal-ip projectcalico/calico#5212

Merged

caseydavenport added docs-completed and removed docs-pr-required labels Dec 15, 2021

caseydavenport mentioned this pull request Dec 20, 2021

Support for autodetection Calico node IP from k8s InternalIP tigera/operator#1704

Merged

5 tasks

fasaxc mentioned this pull request May 24, 2022

IP AutoDetectionMethod kubernetes-internal-ip seems to throw last part of IP (corresponding to subnet) projectcalico/calico#6142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New autodetection method KubernetesInternalIP added #1242

New autodetection method KubernetesInternalIP added #1242

hanamantagoudvk commented Oct 11, 2021 •

edited by caseydavenport

Loading

CLAassistant commented Oct 11, 2021 •

edited

Loading

caseydavenport commented Oct 19, 2021

hanamantagoudvk commented Oct 21, 2021

caseydavenport left a comment

caseydavenport Nov 2, 2021

hanamantagoudvk Nov 10, 2021

caseydavenport Nov 2, 2021

caseydavenport Nov 2, 2021

hanamantagoudvk Nov 10, 2021

caseydavenport commented Nov 9, 2021

hanamantagoudvk commented Nov 10, 2021

tomastigera left a comment

tomastigera Nov 17, 2021

hanamantagoudvk Nov 18, 2021

tomastigera Nov 17, 2021

hanamantagoudvk Nov 18, 2021

caseydavenport left a comment

caseydavenport Nov 19, 2021

caseydavenport commented Nov 19, 2021

caseydavenport commented Nov 19, 2021

hanamantagoudvk commented Nov 20, 2021

caseydavenport commented Nov 22, 2021

hanamantagoudvk commented Nov 23, 2021

hanamantagoudvk commented Nov 28, 2021 •

edited

Loading

caseydavenport left a comment

caseydavenport commented Dec 7, 2021

hanamantagoudvk commented Dec 9, 2021

caseydavenport commented Dec 9, 2021

caseydavenport commented Dec 9, 2021

caseydavenport commented Dec 10, 2021

hanamantagoudvk commented Dec 10, 2021

hanamantagoudvk commented Dec 12, 2021

New autodetection method KubernetesInternalIP added #1242

New autodetection method KubernetesInternalIP added #1242

Conversation

hanamantagoudvk commented Oct 11, 2021 • edited by caseydavenport Loading

Description

CLAassistant commented Oct 11, 2021 • edited Loading

caseydavenport commented Oct 19, 2021

hanamantagoudvk commented Oct 21, 2021

caseydavenport 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

caseydavenport commented Nov 9, 2021

hanamantagoudvk commented Nov 10, 2021

tomastigera 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

caseydavenport left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caseydavenport commented Nov 19, 2021

caseydavenport commented Nov 19, 2021

hanamantagoudvk commented Nov 20, 2021

caseydavenport commented Nov 22, 2021

hanamantagoudvk commented Nov 23, 2021

hanamantagoudvk commented Nov 28, 2021 • edited Loading

caseydavenport left a comment

Choose a reason for hiding this comment

caseydavenport commented Dec 7, 2021

hanamantagoudvk commented Dec 9, 2021

caseydavenport commented Dec 9, 2021

caseydavenport commented Dec 9, 2021

caseydavenport commented Dec 10, 2021

hanamantagoudvk commented Dec 10, 2021

hanamantagoudvk commented Dec 12, 2021

hanamantagoudvk commented Oct 11, 2021 •

edited by caseydavenport

Loading

CLAassistant commented Oct 11, 2021 •

edited

Loading

hanamantagoudvk commented Nov 28, 2021 •

edited

Loading