From e08c9c663d6f0a38836eb7f537fee313ff1dad60 Mon Sep 17 00:00:00 2001 From: Satish Matti Date: Sat, 27 May 2023 00:25:14 -0700 Subject: [PATCH 1/3] Remove remaining node update calls in calico-node startup Followup to https://github.com/projectcalico/calico/pull/7550 Change-Id: I01c8018c9d47401383f05861865ff0c9b44c506e --- node/filesystem/etc/rc.local | 6 ++- node/pkg/lifecycle/startup/startup.go | 47 +++++++++++----------- node/pkg/lifecycle/startup/startup_test.go | 17 ++++---- 3 files changed, 37 insertions(+), 33 deletions(-) diff --git a/node/filesystem/etc/rc.local b/node/filesystem/etc/rc.local index 6ab155aa9ed..24768f9dff2 100755 --- a/node/filesystem/etc/rc.local +++ b/node/filesystem/etc/rc.local @@ -36,8 +36,10 @@ if [ -z "$CALICO_DISABLE_FELIX" ]; then cp -a /etc/service/available/felix /etc/service/enabled/ fi -# Monitor change in node IP addresses and subnets. -cp -a /etc/service/available/monitor-addresses /etc/service/enabled/ +if [ "$CALICO_NETWORKING_BACKEND" != "none" ]; then + # Monitor change in node IP addresses and subnets. + cp -a /etc/service/available/monitor-addresses /etc/service/enabled/ +fi # Enable the allocate tunnel IP service cp -a /etc/service/available/allocate-tunnel-addrs /etc/service/enabled/ diff --git a/node/pkg/lifecycle/startup/startup.go b/node/pkg/lifecycle/startup/startup.go index 77c5058902d..7d7e1c91784 100644 --- a/node/pkg/lifecycle/startup/startup.go +++ b/node/pkg/lifecycle/startup/startup.go @@ -182,25 +182,22 @@ func Run() { } } - // If Calico is running in policy only mode we don't need to write BGP related details to the Node. - if os.Getenv("CALICO_NETWORKING_BACKEND") != "none" { - configureAndCheckIPAddressSubnets(ctx, cli, node, k8sNode) - // Configure the node AS number. - configureASNumber(node) - } - + needsNodeUpdate := configureAndCheckIPAddressSubnets(ctx, cli, node, k8sNode) + // Configure the node AS number. + needsNodeUpdate = configureASNumber(node) || needsNodeUpdate // Populate a reference to the node based on orchestrator node identifiers. - configureNodeRef(node) + needsNodeUpdate = configureNodeRef(node) || needsNodeUpdate + if needsNodeUpdate { + // Apply the updated node resource. + if _, err := CreateOrUpdate(ctx, cli, node); err != nil { + log.WithError(err).Errorf("Unable to set node resource configuration") + utils.Terminate() + } + } // Check expected filesystem ensureFilesystemAsExpected() - // Apply the updated node resource. - if _, err := CreateOrUpdate(ctx, cli, node); err != nil { - log.WithError(err).Errorf("Unable to set node resource configuration") - utils.Terminate() - } - // Configure IP Pool configuration. configureIPPools(ctx, cli, kubeadmConfig) @@ -257,6 +254,11 @@ func getMonitorPollInterval() time.Duration { } func configureAndCheckIPAddressSubnets(ctx context.Context, cli client.Interface, node *libapi.Node, k8sNode *v1.Node) bool { + // If Calico is running in policy only mode we don't need to write BGP related + // details to the Node. + if os.Getenv("CALICO_NETWORKING_BACKEND") == "none" { + return false + } // Configure and verify the node IP addresses and subnets. checkConflicts, err := configureIPsAndSubnets(node, k8sNode, func(incl []string, excl []string, version int) ([]autodetection.Interface, error) { return autodetection.GetInterfaces(net.Interfaces, incl, excl, version) @@ -309,12 +311,6 @@ func configureAndCheckIPAddressSubnets(ctx context.Context, cli client.Interface } func MonitorIPAddressSubnets() { - // If Calico is running in policy only mode we don't need to write BGP - // related details to the Node. - if os.Getenv("CALICO_NETWORKING_BACKEND") == "none" { - log.Info("Skipped monitoring node IP changes when CALICO_NETWORKING_BACKEND=none") - return - } ctx := context.Background() _, cli := calicoclient.CreateClient() nodeName := utils.DetermineNodeName() @@ -369,16 +365,18 @@ func MonitorIPAddressSubnets() { // configureNodeRef will attempt to discover the cluster type it is running on, check to ensure we // have not already set it on this Node, and set it if need be. -func configureNodeRef(node *libapi.Node) { +// Returns true if the node object needs to updated. +func configureNodeRef(node *libapi.Node) bool { orchestrator := "k8s" nodeRef := "" // Sort out what type of cluster we're running on. if nodeRef = os.Getenv("CALICO_K8S_NODE_REF"); nodeRef == "" { - return + return false } node.Spec.OrchRefs = []libapi.OrchRef{{NodeName: nodeRef, Orchestrator: orchestrator}} + return true } // CreateOrUpdate creates the Node if ResourceVersion is not specified, @@ -690,7 +688,8 @@ func evaluateENVBool(envVar string, defaultValue bool) bool { // configureASNumber configures the Node resource with the AS number specified // in the environment, or is a no-op if not specified. -func configureASNumber(node *libapi.Node) { +// Returns true if the node object needs to be updated. +func configureASNumber(node *libapi.Node) bool { // Extract the AS number from the environment asStr := os.Getenv("AS") if asStr != "" { @@ -700,6 +699,7 @@ func configureASNumber(node *libapi.Node) { } else { log.Infof("Using AS number specified in environment (AS=%s)", asNum) node.Spec.BGP.ASNumber = &asNum + return true } } else { if node.Spec.BGP.ASNumber == nil { @@ -708,6 +708,7 @@ func configureASNumber(node *libapi.Node) { log.Infof("Using AS number %s configured in node resource", node.Spec.BGP.ASNumber) } } + return false } // generateIPv6ULAPrefix return a random generated ULA IPv6 prefix as per RFC 4193. The pool diff --git a/node/pkg/lifecycle/startup/startup_test.go b/node/pkg/lifecycle/startup/startup_test.go index 711b1401181..f52c80731f3 100644 --- a/node/pkg/lifecycle/startup/startup_test.go +++ b/node/pkg/lifecycle/startup/startup_test.go @@ -91,7 +91,7 @@ func makeK8sNode(ipv4 string, ipv6 string) *v1.Node { } var _ = DescribeTable("Node IP detection failure cases", - func(networkingBackend string, expectedExitCode int, rrCId string) { + func(networkingBackend string, expectedExitCode int, rrCId string, expectedUpdate bool) { os.Setenv("CALICO_NETWORKING_BACKEND", networkingBackend) os.Setenv("IP", "none") os.Setenv("IP6", "") @@ -113,16 +113,17 @@ var _ = DescribeTable("Node IP detection failure cases", node.Spec.BGP = &libapi.NodeBGPSpec{RouteReflectorClusterID: rrCId} } - _ = configureAndCheckIPAddressSubnets(context.Background(), c, &node, &v1.Node{}) + updated := configureAndCheckIPAddressSubnets(context.Background(), c, &node, &v1.Node{}) + Expect(updated).To(Equal(expectedUpdate)) Expect(my_ec).To(Equal(expectedExitCode)) if rrCId != "" { Expect(node.Spec.BGP).NotTo(BeNil()) } }, - Entry("startup should terminate if IP is set to none and Calico is used for networking", "bird", 1, ""), - Entry("startup should NOT terminate if IP is set to none and Calico is policy-only", "none", 0, ""), - Entry("startup should NOT terminate and BGPSpec shouldn't be set to nil", "none", 0, "rrClusterID"), + Entry("startup should terminate if IP is set to none and Calico is used for networking", "bird", 1, "", false), + Entry("startup should NOT terminate if IP is set to none and Calico is policy-only", "none", 0, "", false), + Entry("startup should NOT terminate and BGPSpec shouldn't be set to nil", "none", 0, "rrClusterID", true), ) var _ = Describe("Default IPv4 pool CIDR", func() { @@ -876,7 +877,7 @@ var _ = Describe("FV tests against a real etcd", func() { os.Setenv(env.key, env.value) } - configureNodeRef(node) + Expect(configureNodeRef(node)).To(Equal(true)) // If we receive an invalid env var then none will be set. if len(node.Spec.OrchRefs) > 0 { ref := node.Spec.OrchRefs[0] @@ -893,7 +894,7 @@ var _ = Describe("FV tests against a real etcd", func() { os.Setenv("CALICO_UNKNOWN_NODE_REF", "node1") node := &libapi.Node{} - configureNodeRef(node) + Expect(configureNodeRef(node)).To(Equal(false)) Expect(node.Spec.OrchRefs).To(HaveLen(0)) }) @@ -902,7 +903,7 @@ var _ = Describe("FV tests against a real etcd", func() { node := &libapi.Node{} node.Spec.OrchRefs = append(node.Spec.OrchRefs, libapi.OrchRef{"node1", "k8s"}) // nolint: vet - configureNodeRef(node) + Expect(configureNodeRef(node)).To(Equal(true)) Expect(node.Spec.OrchRefs).To(HaveLen(1)) }) From 057d956f22d94b16ddc8057614699c8a3c558d32 Mon Sep 17 00:00:00 2001 From: Tobias Giese Date: Wed, 28 Jun 2023 22:39:26 +0200 Subject: [PATCH 2/3] Fix nilpointer if CALICO_NETWORKING_BACKEND=none There is a nil pointer in the configureASNumber function if the networking backend is none (i.e. policy-only mode) Signed-off-by: Tobias Giese --- node/pkg/lifecycle/startup/startup.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/node/pkg/lifecycle/startup/startup.go b/node/pkg/lifecycle/startup/startup.go index 7d7e1c91784..0822eacbba0 100644 --- a/node/pkg/lifecycle/startup/startup.go +++ b/node/pkg/lifecycle/startup/startup.go @@ -690,6 +690,11 @@ func evaluateENVBool(envVar string, defaultValue bool) bool { // in the environment, or is a no-op if not specified. // Returns true if the node object needs to be updated. func configureASNumber(node *libapi.Node) bool { + // If Calico is running in policy only mode we don't need to write BGP related + // details to the Node. + if os.Getenv("CALICO_NETWORKING_BACKEND") == "none" { + return false + } // Extract the AS number from the environment asStr := os.Getenv("AS") if asStr != "" { From da3602f139745dad93524b88eb1904c58732c580 Mon Sep 17 00:00:00 2001 From: Tobias Giese Date: Sat, 1 Jul 2023 14:53:47 +0200 Subject: [PATCH 3/3] Fix startup unit test for policy only mode Policy only mode will no longer update the BGP spec. Thus, the unit test should be fixed as well. Signed-off-by: Tobias Giese --- node/pkg/lifecycle/startup/startup_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/node/pkg/lifecycle/startup/startup_test.go b/node/pkg/lifecycle/startup/startup_test.go index f52c80731f3..b8a04e2039d 100644 --- a/node/pkg/lifecycle/startup/startup_test.go +++ b/node/pkg/lifecycle/startup/startup_test.go @@ -109,21 +109,24 @@ var _ = DescribeTable("Node IP detection failure cases", Expect(err).NotTo(HaveOccurred()) node := libapi.Node{} - if rrCId != "" { + if networkingBackend != "none" && rrCId != "" { node.Spec.BGP = &libapi.NodeBGPSpec{RouteReflectorClusterID: rrCId} } updated := configureAndCheckIPAddressSubnets(context.Background(), c, &node, &v1.Node{}) Expect(updated).To(Equal(expectedUpdate)) Expect(my_ec).To(Equal(expectedExitCode)) - if rrCId != "" { + if networkingBackend != "none" && rrCId != "" { Expect(node.Spec.BGP).NotTo(BeNil()) } + if networkingBackend == "none" { + Expect(node.Spec.BGP).To(BeNil()) + } }, Entry("startup should terminate if IP is set to none and Calico is used for networking", "bird", 1, "", false), Entry("startup should NOT terminate if IP is set to none and Calico is policy-only", "none", 0, "", false), - Entry("startup should NOT terminate and BGPSpec shouldn't be set to nil", "none", 0, "rrClusterID", true), + Entry("startup should NOT terminate and BGPSpec shouldn't be set", "none", 0, "rrClusterID", false), ) var _ = Describe("Default IPv4 pool CIDR", func() {