Repository: istio/istio
File: tools/istio-iptables/pkg/dependencies/implementation_linux.go (Go)

Code before:
		parsedVer, parseErr = parseIptablesVer(verStdOut.String())
		if parseErr != nil {
			return IptablesVersion{}, fmt.Errorf("iptables version %q is not a valid version string: %v", verStdOut.Bytes(), parseErr)
		}
		// Legacy will have no marking or 'legacy', so just look for nf_tables
		isNft = strings.Contains(verStdOut.String(), "nf_tables")
	} else {
		log.Warnf("found iptables binary %s, but it does not appear to support the '--version' flag, assuming very old legacy version", iptablesSaveBin)
		// Some really old iptables-legacy-save versions (1.6.1, ubuntu bionic) don't support any arguments at all, including `--version`
		// So if we get here, we found `iptables-save` in PATH, but it's too outdated to understand `--version`.
		//
		// We can eventually remove this.
		//
		// So assume it's legacy/an unknown version, but assume we can use it since it's in PATH
		parsedVer = utilversion.MustParseGeneric("0.0.0")
		isNft = false
	}

	// `filter` should ALWAYS exist if kernel support for this version is locally present,
	// so try to add a no-op rule to that table, and make sure it doesn't fail - if it does, we can't use this version
	// as the host is missing kernel support for it.
	// (255 is a nonexistent protocol number, IANA reserved, see `cat /etc/protocols`, so this rule will never match anything)
	// should use the wait flag if supported to avoid lock contention whenever possible
	needLock := !isNft && !parsedVer.LessThan(IptablesRestoreLocking)
	if needLock {
		testRuleAdd = append(testRuleAdd, "--wait=30")
	}
	testCmd := exec.Command(iptablesBin, testRuleAdd...)

	var testStdErr bytes.Buffer
	testCmd.Stderr = &testStdErr
	testRes := testCmd.Run()
	// If we can't add a rule to the basic `filter` table, we can't use this binary - bail out.
	// Otherwise, delete the no-op rule and carry on with other checks
	if testRes != nil || strings.Contains(testStdErr.String(), "does not exist") {
		return IptablesVersion{}, fmt.Errorf("iptables binary %s has no loaded kernel support and cannot be used, err: %v out: %s",
			iptablesBin, testRes, testStdErr.String())
	}

	testRuleDel := append(make([]string, 0, len(testRuleAdd)), testRuleAdd...)
	testRuleDel[2] = "-D"

	testCmd = exec.Command(iptablesBin, testRuleDel...)
	_ = testCmd.Run()

	// if binary seems to exist, check the dump of rules in our netns, and see if any rules exist there
	// Note that this is highly dependent on context.
	// new pod netns? probably no rules. Hostnetns? probably rules
	// So this is mostly just a "hint"/heuristic as to which version we should be using, if more than one binary is present.
	// `xx-save` should return _no_ output (0 lines) if no rules are defined in this netns for that binary variant.
	// `xx-save` should return at least 3 output lines if at least one rule is defined in this netns for that binary variant.

Review comment (suggestion):
The `DetectIptablesVersion()` method calls `shouldUseBinaryForContext()` API couple of times (f.e., for legacy, nft, and plain binaries). Here we are modifying the global variable `testRuleAdd` by appending `--wait=30` for each invocation. Please fix it by making it a local copy.

Code after:
		parsedVer, parseErr = parseIptablesVer(verStdOut.String())
		if parseErr != nil {
			return IptablesVersion{}, fmt.Errorf("iptables version %q is not a valid version string: %v", verStdOut.Bytes(), parseErr)
		}
		// Legacy will have no marking or 'legacy', so just look for nf_tables
		isNft = strings.Contains(verStdOut.String(), "nf_tables")
	} else {
		log.Warnf("found iptables binary %s, but it does not appear to support the '--version' flag, assuming very old legacy version", iptablesSaveBin)
		// Some really old iptables-legacy-save versions (1.6.1, ubuntu bionic) don't support any arguments at all, including `--version`
		// So if we get here, we found `iptables-save` in PATH, but it's too outdated to understand `--version`.
		//
		// We can eventually remove this.
		//
		// So assume it's legacy/an unknown version, but assume we can use it since it's in PATH
		parsedVer = utilversion.MustParseGeneric("0.0.0")
		isNft = false
	}

	// `filter` should ALWAYS exist if kernel support for this version is locally present,
	// so try to add a no-op rule to that table, and make sure it doesn't fail - if it does, we can't use this version
	// as the host is missing kernel support for it.
	// (255 is a nonexistent protocol number, IANA reserved, see `cat /etc/protocols`, so this rule will never match anything)
	// should use the wait flag if supported to avoid lock contention whenever possible
	needLock := !isNft && !parsedVer.LessThan(IptablesRestoreLocking)
	testArgs := append(make([]string, 0, len(testRuleAdd)+1), testRuleAdd...)
	if needLock {
		testArgs = append(testArgs, "--wait=30")
	}
	testCmd := exec.Command(iptablesBin, testArgs...)

	var testStdErr bytes.Buffer
	testCmd.Stderr = &testStdErr
	testRes := testCmd.Run()
	// If we can't add a rule to the basic `filter` table, we can't use this binary - bail out.
	// Otherwise, delete the no-op rule and carry on with other checks
	if testRes != nil || strings.Contains(testStdErr.String(), "does not exist") {
		return IptablesVersion{}, fmt.Errorf("iptables binary %s has no loaded kernel support and cannot be used, err: %v out: %s",
			iptablesBin, testRes, testStdErr.String())
	}

	testRuleDel := append(make([]string, 0, len(testArgs)), testArgs...)
	testRuleDel[2] = "-D"

	testCmd = exec.Command(iptablesBin, testRuleDel...)
	_ = testCmd.Run()

	// if binary seems to exist, check the dump of rules in our netns, and see if any rules exist there
	// Note that this is highly dependent on context.
	// new pod netns? probably no rules. Hostnetns? probably rules
	// So this is mostly just a "hint"/heuristic as to which version we should be using, if more than one binary is present.
	// `xx-save` should return _no_ output (0 lines) if no rules are defined in this netns for that binary variant.