Browse code

libnetwork: don't use strings.Fields() to improve performance

While looking at this code, I noticed that we were wasting quite some resources
by first constructing a string, only to split it again (with `strings.Fields()`)
into a string slice.

Some conversions were also happening multiple times (int to string, IP-address to
string, etc.)

Setting up networking is known to be costing a considerable amount of time when
starting containers, and while this may only be a small part of that, it doesn't
hurt to save some resources (and readability of the code isn't significantly
impacted).

For example, benchmarking the `redirector()` code before/after:

BenchmarkParseOld-4 137646 8398 ns/op 4192 B/op 75 allocs/op
BenchmarkParseNew-4 629395 1762 ns/op 2362 B/op 24 allocs/op

Average over 10 runs:

benchstat old.txt new.txt

name old time/op new time/op delta
Parse-4 8.43µs ± 2% 1.79µs ± 3% -78.76% (p=0.000 n=9+8)

name old alloc/op new alloc/op delta
Parse-4 4.19kB ± 0% 2.36kB ± 0% -43.65% (p=0.000 n=10+10)

name old allocs/op new allocs/op delta
Parse-4 75.0 ± 0% 24.0 ± 0% -68.00% (p=0.000 n=10+10)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Sebastiaan van Stijn authored on 2022/04/20 21:43:07
Showing 1 changed files
... ...
@@ -381,7 +381,7 @@ func programIngress(gwIP net.IP, ingressPorts []*PortConfig, isDelete bool) erro
381 381
 			return fmt.Errorf("could not write to %s: %v", path, err)
382 382
 		}
383 383
 
384
-		ruleArgs := strings.Fields(fmt.Sprintf("-m addrtype --src-type LOCAL -o %s -j MASQUERADE", oifName))
384
+		ruleArgs := []string{"-m", "addrtype", "--src-type", "LOCAL", "-o", oifName, "-j", "MASQUERADE"}
385 385
 		if !iptable.Exists(iptables.Nat, "POSTROUTING", ruleArgs...) {
386 386
 			if err := iptable.RawCombinedOutput(append([]string{"-t", "nat", "-I", "POSTROUTING"}, ruleArgs...)...); err != nil {
387 387
 				return fmt.Errorf("failed to add ingress localhost POSTROUTING rule for %s: %v", oifName, err)
... ...
@@ -389,7 +389,7 @@ func programIngress(gwIP net.IP, ingressPorts []*PortConfig, isDelete bool) erro
389 389
 		}
390 390
 	}
391 391
 
392
-	//Filter the ingress ports until port rules start to be added/deleted
392
+	// Filter the ingress ports until port rules start to be added/deleted
393 393
 	filteredPorts := filterPortConfigs(ingressPorts, isDelete)
394 394
 	rollbackRules := make([][]string, 0, len(filteredPorts)*3)
395 395
 	var portErr error
... ...
@@ -405,52 +405,52 @@ func programIngress(gwIP net.IP, ingressPorts []*PortConfig, isDelete bool) erro
405 405
 	}()
406 406
 
407 407
 	for _, iPort := range filteredPorts {
408
+		var (
409
+			protocol      = strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)])
410
+			publishedPort = strconv.FormatUint(uint64(iPort.PublishedPort), 10)
411
+			destination   = net.JoinHostPort(gwIP.String(), publishedPort)
412
+		)
408 413
 		if iptable.ExistChain(ingressChain, iptables.Nat) {
409
-			rule := strings.Fields(fmt.Sprintf("-t nat %s %s -p %s --dport %d -j DNAT --to-destination %s:%d",
410
-				addDelOpt, ingressChain, strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)]), iPort.PublishedPort, gwIP, iPort.PublishedPort))
414
+			rule := []string{"-t", "nat", addDelOpt, ingressChain, "-p", protocol, "--dport", publishedPort, "-j", "DNAT", "--to-destination", destination}
415
+
411 416
 			if portErr = iptable.RawCombinedOutput(rule...); portErr != nil {
412
-				errStr := fmt.Sprintf("set up rule failed, %v: %v", rule, portErr)
417
+				err := fmt.Errorf("set up rule failed, %v: %v", rule, portErr)
413 418
 				if !isDelete {
414
-					return fmt.Errorf("%s", errStr)
419
+					return err
415 420
 				}
416
-				logrus.Infof("%s", errStr)
421
+				logrus.Info(err)
417 422
 			}
418
-			rollbackRule := strings.Fields(fmt.Sprintf("-t nat %s %s -p %s --dport %d -j DNAT --to-destination %s:%d", rollbackAddDelOpt,
419
-				ingressChain, strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)]), iPort.PublishedPort, gwIP, iPort.PublishedPort))
423
+			rollbackRule := []string{"-t", "nat", rollbackAddDelOpt, ingressChain, "-p", protocol, "--dport", publishedPort, "-j", "DNAT", "--to-destination", destination}
420 424
 			rollbackRules = append(rollbackRules, rollbackRule)
421 425
 		}
422 426
 
423 427
 		// Filter table rules to allow a published service to be accessible in the local node from..
424 428
 		// 1) service tasks attached to other networks
425 429
 		// 2) unmanaged containers on bridge networks
426
-		rule := strings.Fields(fmt.Sprintf("%s %s -m state -p %s --sport %d --state ESTABLISHED,RELATED -j ACCEPT",
427
-			addDelOpt, ingressChain, strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)]), iPort.PublishedPort))
430
+		rule := []string{addDelOpt, ingressChain, "-m", "state", "-p", protocol, "--sport", publishedPort, "--state", "ESTABLISHED,RELATED", "-j", "ACCEPT"}
428 431
 		if portErr = iptable.RawCombinedOutput(rule...); portErr != nil {
429
-			errStr := fmt.Sprintf("set up rule failed, %v: %v", rule, portErr)
432
+			err := fmt.Errorf("set up rule failed, %v: %v", rule, portErr)
430 433
 			if !isDelete {
431
-				return fmt.Errorf("%s", errStr)
434
+				return err
432 435
 			}
433
-			logrus.Warnf("%s", errStr)
436
+			logrus.Warn(err)
434 437
 		}
435
-		rollbackRule := strings.Fields(fmt.Sprintf("%s %s -m state -p %s --sport %d --state ESTABLISHED,RELATED -j ACCEPT", rollbackAddDelOpt,
436
-			ingressChain, strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)]), iPort.PublishedPort))
438
+		rollbackRule := []string{rollbackAddDelOpt, ingressChain, "-m", "state", "-p", protocol, "--sport", publishedPort, "--state", "ESTABLISHED,RELATED", "-j", "ACCEPT"}
437 439
 		rollbackRules = append(rollbackRules, rollbackRule)
438 440
 
439
-		rule = strings.Fields(fmt.Sprintf("%s %s -p %s --dport %d -j ACCEPT",
440
-			addDelOpt, ingressChain, strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)]), iPort.PublishedPort))
441
+		rule = []string{addDelOpt, ingressChain, "-p", protocol, "--dport", publishedPort, "-j", "ACCEPT"}
441 442
 		if portErr = iptable.RawCombinedOutput(rule...); portErr != nil {
442
-			errStr := fmt.Sprintf("set up rule failed, %v: %v", rule, portErr)
443
+			err := fmt.Errorf("set up rule failed, %v: %v", rule, portErr)
443 444
 			if !isDelete {
444
-				return fmt.Errorf("%s", errStr)
445
+				return err
445 446
 			}
446
-			logrus.Warnf("%s", errStr)
447
+			logrus.Warn(err)
447 448
 		}
448
-		rollbackRule = strings.Fields(fmt.Sprintf("%s %s -p %s --dport %d -j ACCEPT", rollbackAddDelOpt,
449
-			ingressChain, strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)]), iPort.PublishedPort))
449
+		rollbackRule = []string{rollbackAddDelOpt, ingressChain, "-p", protocol, "--dport", publishedPort, "-j", "ACCEPT"}
450 450
 		rollbackRules = append(rollbackRules, rollbackRule)
451 451
 
452 452
 		if err := plumbProxy(iPort, isDelete); err != nil {
453
-			logrus.Warnf("failed to create proxy for port %d: %v", iPort.PublishedPort, err)
453
+			logrus.Warnf("failed to create proxy for port %s: %v", publishedPort, err)
454 454
 		}
455 455
 	}
456 456
 
... ...
@@ -631,17 +631,20 @@ func fwMarker() {
631 631
 	}
632 632
 
633 633
 	vip := os.Args[2]
634
-	fwMark, err := strconv.ParseUint(os.Args[3], 10, 32)
635
-	if err != nil {
636
-		logrus.Errorf("bad fwmark value(%s) passed: %v", os.Args[3], err)
634
+	fwMark := os.Args[3]
635
+	if _, err := strconv.ParseUint(fwMark, 10, 32); err != nil {
636
+		logrus.Errorf("bad fwmark value(%s) passed: %v", fwMark, err)
637 637
 		os.Exit(3)
638 638
 	}
639 639
 	addDelOpt := os.Args[4]
640 640
 
641
-	rules := [][]string{}
641
+	rules := make([][]string, 0, len(ingressPorts))
642 642
 	for _, iPort := range ingressPorts {
643
-		rule := strings.Fields(fmt.Sprintf("-t mangle %s PREROUTING -p %s --dport %d -j MARK --set-mark %d",
644
-			addDelOpt, strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)]), iPort.PublishedPort, fwMark))
643
+		var (
644
+			protocol      = strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)])
645
+			publishedPort = strconv.FormatUint(uint64(iPort.PublishedPort), 10)
646
+		)
647
+		rule := []string{"-t", "mangle", addDelOpt, "PREROUTING", "-p", protocol, "--dport", publishedPort, "-j", "MARK", "--set-mark", fwMark}
645 648
 		rules = append(rules, rule)
646 649
 	}
647 650
 
... ...
@@ -665,9 +668,9 @@ func fwMarker() {
665 665
 			os.Exit(6)
666 666
 		}
667 667
 
668
-		ruleParams := strings.Fields(fmt.Sprintf("-m ipvs --ipvs -d %s -j SNAT --to-source %s", subnet, eIP))
668
+		ruleParams := []string{"-m", "ipvs", "--ipvs", "-d", subnet.String(), "-j", "SNAT", "--to-source", eIP.String()}
669 669
 		if !iptable.Exists("nat", "POSTROUTING", ruleParams...) {
670
-			rule := append(strings.Fields("-t nat -A POSTROUTING"), ruleParams...)
670
+			rule := append([]string{"-t", "nat", "-A", "POSTROUTING"}, ruleParams...)
671 671
 			rules = append(rules, rule)
672 672
 
673 673
 			err := os.WriteFile("/proc/sys/net/ipv4/vs/conntrack", []byte{'1', '\n'}, 0644)
... ...
@@ -678,7 +681,7 @@ func fwMarker() {
678 678
 		}
679 679
 	}
680 680
 
681
-	rule := strings.Fields(fmt.Sprintf("-t mangle %s INPUT -d %s/32 -j MARK --set-mark %d", addDelOpt, vip, fwMark))
681
+	rule := []string{"-t", "mangle", addDelOpt, "INPUT", "-d", vip + "/32", "-j", "MARK", "--set-mark", fwMark}
682 682
 	rules = append(rules, rule)
683 683
 
684 684
 	for _, rule := range rules {
... ...
@@ -742,20 +745,25 @@ func redirector() {
742 742
 		logrus.Errorf("Failed to parse endpoint IP %s: %v", os.Args[2], err)
743 743
 		os.Exit(3)
744 744
 	}
745
+	ipAddr := eIP.String()
745 746
 
746
-	rules := [][]string{}
747
+	rules := make([][]string, 0, len(ingressPorts)*3) // 3 rules per port
747 748
 	for _, iPort := range ingressPorts {
748
-		rule := strings.Fields(fmt.Sprintf("-t nat -A PREROUTING -d %s -p %s --dport %d -j REDIRECT --to-port %d",
749
-			eIP.String(), strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)]), iPort.PublishedPort, iPort.TargetPort))
750
-		rules = append(rules, rule)
751
-		// Allow only incoming connections to exposed ports
752
-		iRule := strings.Fields(fmt.Sprintf("-I INPUT -d %s -p %s --dport %d -m conntrack --ctstate NEW,ESTABLISHED -j ACCEPT",
753
-			eIP.String(), strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)]), iPort.TargetPort))
754
-		rules = append(rules, iRule)
755
-		// Allow only outgoing connections from exposed ports
756
-		oRule := strings.Fields(fmt.Sprintf("-I OUTPUT -s %s -p %s --sport %d -m conntrack --ctstate ESTABLISHED -j ACCEPT",
757
-			eIP.String(), strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)]), iPort.TargetPort))
758
-		rules = append(rules, oRule)
749
+		var (
750
+			protocol      = strings.ToLower(PortConfig_Protocol_name[int32(iPort.Protocol)])
751
+			publishedPort = strconv.FormatUint(uint64(iPort.PublishedPort), 10)
752
+			targetPort    = strconv.FormatUint(uint64(iPort.TargetPort), 10)
753
+		)
754
+
755
+		rules = append(rules,
756
+			[]string{"-t", "nat", "-A", "PREROUTING", "-d", ipAddr, "-p", protocol, "--dport", publishedPort, "-j", "REDIRECT", "--to-port", targetPort},
757
+
758
+			// Allow only incoming connections to exposed ports
759
+			[]string{"-I", "INPUT", "-d", ipAddr, "-p", protocol, "--dport", targetPort, "-m", "conntrack", "--ctstate", "NEW,ESTABLISHED", "-j", "ACCEPT"},
760
+
761
+			// Allow only outgoing connections from exposed ports
762
+			[]string{"-I", "OUTPUT", "-s", ipAddr, "-p", protocol, "--sport", targetPort, "-m", "conntrack", "--ctstate", "ESTABLISHED", "-j", "ACCEPT"},
763
+		)
759 764
 	}
760 765
 
761 766
 	ns, err := netns.GetFromPath(os.Args[1])
... ...
@@ -783,9 +791,9 @@ func redirector() {
783 783
 
784 784
 	// Ensure blocking rules for anything else in/to ingress network
785 785
 	for _, rule := range [][]string{
786
-		{"-d", eIP.String(), "-p", "sctp", "-j", "DROP"},
787
-		{"-d", eIP.String(), "-p", "udp", "-j", "DROP"},
788
-		{"-d", eIP.String(), "-p", "tcp", "-j", "DROP"},
786
+		{"-d", ipAddr, "-p", "sctp", "-j", "DROP"},
787
+		{"-d", ipAddr, "-p", "udp", "-j", "DROP"},
788
+		{"-d", ipAddr, "-p", "tcp", "-j", "DROP"},
789 789
 	} {
790 790
 		if !iptable.ExistsNative(iptables.Filter, "INPUT", rule...) {
791 791
 			if err := iptable.RawCombinedOutputNative(append([]string{"-A", "INPUT"}, rule...)...); err != nil {