better support of ipv6-only clusters: prevent creation of default-ipv4-pool during calico-node startup

[kruftik]

Description

Current calico-node startup logic supports creation of default ip-pools both for ipv4 and ipv6 stacks if the ones do not exist. However, it does not take into account accordingly the ipv6-only clusters (without ipv4 addresses on any interfaces at all): as soon as default-ipv4 pool being created, every calico-node instance begin log flooding with something like:

felix/route_table.go 1016: Failed to get link attributes error=interface not present ifaceRegex="^vxlan.calico$" ipVersion=0x4 tableIndex=254

This PR mitigate such an issue: in addition to NO_DEFAULT_POOLS environment variable, CALICO_IPV4POOL_CIDR=none and CALICO_IPV6POOL_CIDR=none special values are added.

Release Note

In order to prevent default IP-pools creation, `CALICO_IPV4POOL_CIDR`=`none` and `CALICO_IPV6POOL_CIDR`=`none` environment variable special values are now supported.

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

• docs-pr-required: This change requires a change to the documentation that has not been completed yet.
• docs-completed: This change has all necessary documentation completed.
• docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

• release-note-required: This PR has user-facing changes. Most PRs should have this label.
• release-note-not-required: This PR has no user-facing changes.

Other optional labels:

• cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
• needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

[CLAassistant]

All committers have signed the CLA.

[mazdakn]

/sem-approve

[caseydavenport]

I wonder if instead of introducing new env vars for this, we should follow the pattern we used for CALICO_NETWORKING_BACKEND and support a none option for the existing IPV4POOL_CIDR variable.

That said, I am curious why using NO_DEFAULT_POOLS and then creating an IPv6 pool out-of-band isn't viable here? That would be my preferred solution here, since in general I would like us to move away from using calico/node to create IP pools - configuring APIs from a collection of env vars makes it hard to support all of the necessary use cases. For now though, calico/node is where default IP pools are created, so I'm not completely against merging something like this as a stopgap.

[kruftik]

@caseydavenport ,

we should follow the pattern we used for CALICO_NETWORKING_BACKEND and support a none option for the existing IPV4POOL_CIDR variable.

yeah, it's a better option. I will refactor the PR into such an approach.

[kruftik]

@caseydavenport , done. Could you look into?

[caseydavenport]

Apologies for the delay here - this looks good to me!

I do think we need a pair of unit tests to verify this behavior works - here's an example of a similar test you can base it on:

calico/node/pkg/lifecycle/startup/startup_test.go

Lines 455 to 477 in 8d31b26

	It("should properly handle NO_DEFAULT_POOLS env variable", func() {
	// Create clients for test.
	cfg, err := apiconfig.LoadClientConfigFromEnvironment()
	Expect(err).NotTo(HaveOccurred())
	c, err := client.New(*cfg)
	Expect(err).NotTo(HaveOccurred())
	be, err := backend.NewClient(*cfg)
	Expect(err).NotTo(HaveOccurred())
	err = be.Clean()
	Expect(err).NotTo(HaveOccurred())
	// Set the env variables specified.
	os.Setenv("NO_DEFAULT_POOLS", "true")
	// Run the UUT.
	configureIPPools(ctx, c, kubeadmConfig)
	// Get the IPPool list.
	poolList, err := c.IPPools().List(ctx, options.ListOptions{})
	Expect(err).NotTo(HaveOccurred())
	Expect(poolList.Items).To(BeEmpty(), "Environment %#v", os.Environ())
	})

[caseydavenport]

/sem-approve

[mazdakn]

@sridhartigera This might be related to your recent works.

[caseydavenport]

@kruftik is this still on your radar? The only thing remaining is a unit test (see my last comment)

[kruftik]

@kruftik is this still on your radar? The only thing remaining is a unit test (see my last comment)

seems like done

[caseydavenport]

/sem-approve