Initial version of iptables-wrapper in Go #5

rikatz · 2022-05-29T21:55:57Z

Creates the initial version of iptables-wrapper in Go

Related to #4

k8s-ci-robot · 2022-05-29T21:56:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rikatz
To complete the pull request process, please assign thockin after the PR has been reviewed.
You can assign the PR to them by writing /assign @thockin in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

danwinship

Hey, lots of comments but it generally looks good

danwinship · 2022-05-30T13:14:28Z

cmd/iptables-wrapper.go

+)
+
+var (
+	regexChain   = regexp.MustCompile(`(?s):(KUBE-IPTABLES-HINT|KUBE-KUBELET-CANARY)`)


(?s) makes it so that . can match \n, but you aren't using ..
The regexp functions will match within multi-line strings without needing any flags. (?s) and (?m) just affect how you're able to match/not match line boundaries.

danwinship · 2022-05-30T13:15:38Z

cmd/iptables-wrapper.go

+	// As we are going directly to distroless, we can assume the binary will be at
+	// `/usr/sbin`


Suggested change

// As we are going directly to distroless, we can assume the binary will be at

// `/usr/sbin`

// As we only support distroless, we can assume the binary will be at `/usr/sbin`

danwinship · 2022-05-30T13:16:17Z

cmd/iptables-wrapper.go

+	iptablesNFTPath           = "/usr/sbin/xtables-nft-multi"
+	iptablesLegacyPath        = "/usr/sbin/xtables-legacy-multi"
+	iptablesNFTMode    string = "nft"
+	iptablesLegacyMode string = "legacy"


(weird to use string on some of the constants but not others)

danwinship · 2022-05-30T13:16:45Z

cmd/iptables-wrapper.go

+Assuming that when assembling the container the following binaries will be linked to
+this wrapper:
+* iptables
+* iptables-wrapper


weird to put iptables-wrapper in this list...

danwinship · 2022-05-30T13:17:27Z

cmd/iptables-wrapper.go

+1) This binary will try to detect which iptables mode was being used by kubelet,
+calling `xtables-<mode>-multi` and checking if the kubelet rules exists in this path


Suggested change

1) This binary will try to detect which iptables mode was being used by kubelet,

calling `xtables-<mode>-multi` and checking if the kubelet rules exists in this path

1) This binary will try to detect which iptables mode was being used by kubelet,

calling `xtables-<mode>-multi` and checking if the kubelet rules exists in this

iptables mode.

danwinship · 2022-05-30T13:31:01Z

cmd/iptables-wrapper.go

+	outNft, err := exec.Command(iptablesNFTPath, "iptables-save",
+		"-t", "mangle").Output()


You can put this on one line... kube code generally seems to wrap at 90 or 100 characters or so

danwinship · 2022-05-30T13:32:22Z

cmd/iptables-wrapper.go

+	if hasKubernetesChains(outNft) || hasKubernetesChains(outNftv6) {
+		return iptablesNFTMode, nil
+	}


If you have to make separate calls anyway you might as well just do the hasKubernetesChains(outNft) after the ipv4 iptables-save, and not even bother doing the ipv6 save if you find the right chains in the ipv4 one.

(and then you only need a single out variable)

danwinship · 2022-05-30T13:34:38Z

cmd/iptables-wrapper.go

+	// Per @danwinship comment, we can deprecate the logic of getting NFT vs Legacy
+	// rules and assume if it's not NFT, then it's legacy and the rules should have
+	// been created in Legacy


This comment only makes sense if you know that the shell script version does something different.

You also dropped the comment from the shell script version that explains why we don't use "-t" here:

# Check for kubernetes 1.17-or-later with iptables-legacy. We # can't pass "-t mangle" to iptables-legacy-save because it would # cause the kernel to create that table if it didn't already # exist, which we don't want. So we have to grab all the rules

I'd put that here, and then at the end of the function you can have a comment about "the shell script version has further checks here for older versions of kubernetes, but the binary version doesn't support that" or something

danwinship · 2022-05-30T13:35:42Z

cmd/iptables-wrapper.go

+	return "", errors.New("no rule could be detected")
+


the shell version never returns an error; if it can't detect, it just guesses. (Specifically, it guesses "nft".)

danwinship · 2022-05-30T13:37:10Z

cmd/iptables-wrapper.go

+	"bytes"
+	"errors"
+	"fmt"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"regexp"


I don't know how much the final binary size is affected by the imports, but you might want to experiment with that.

In particular, you could probably get rid of "regexp" and just use strings.Contains...

k8s-triage-robot · 2022-08-28T14:23:20Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

rikatz · 2022-08-28T15:30:55Z

/lifecycle active

k8s-triage-robot · 2022-11-26T15:33:03Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-12-26T16:21:25Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2023-01-25T17:10:50Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2023-01-25T17:10:55Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 29, 2022

k8s-ci-robot requested review from danwinship and dcbw May 29, 2022 21:56

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 29, 2022

Initial version of iptables-wrapper in Go

093be42

rikatz force-pushed the wrapper-in-go branch from 0f9ed36 to 093be42 Compare May 29, 2022 21:57

danwinship reviewed May 30, 2022

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 28, 2022

k8s-ci-robot added lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 28, 2022

k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. labels Nov 26, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 26, 2022

k8s-ci-robot closed this Jan 25, 2023

jaxesn mentioned this pull request Feb 6, 2023

Supporting hosts configured to use iptables-nft in kube-proxy aws/eks-distro#1729

Closed

g-gaston mentioned this pull request Feb 10, 2023

Convert iptables-wrapper to a static go program #6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial version of iptables-wrapper in Go #5

Initial version of iptables-wrapper in Go #5

rikatz commented May 29, 2022 •

edited

Loading

k8s-ci-robot commented May 29, 2022

danwinship left a comment

danwinship May 30, 2022

danwinship May 30, 2022

danwinship May 30, 2022

danwinship May 30, 2022

danwinship May 30, 2022

danwinship May 30, 2022

danwinship May 30, 2022 •

edited

Loading

danwinship May 30, 2022

danwinship May 30, 2022

danwinship May 30, 2022

k8s-triage-robot commented Aug 28, 2022

rikatz commented Aug 28, 2022

k8s-triage-robot commented Nov 26, 2022

k8s-triage-robot commented Dec 26, 2022

k8s-triage-robot commented Jan 25, 2023

k8s-ci-robot commented Jan 25, 2023

		// As we are going directly to distroless, we can assume the binary will be at
		// `/usr/sbin`

	// As we are going directly to distroless, we can assume the binary will be at
	// `/usr/sbin`
	// As we only support distroless, we can assume the binary will be at `/usr/sbin`

		1) This binary will try to detect which iptables mode was being used by kubelet,
		calling `xtables-<mode>-multi` and checking if the kubelet rules exists in this path

		outNft, err := exec.Command(iptablesNFTPath, "iptables-save",
		"-t", "mangle").Output()

Initial version of iptables-wrapper in Go #5

Initial version of iptables-wrapper in Go #5

Conversation

rikatz commented May 29, 2022 • edited Loading

k8s-ci-robot commented May 29, 2022

danwinship 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

Choose a reason for hiding this comment

danwinship May 30, 2022 • edited Loading

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

k8s-triage-robot commented Aug 28, 2022

rikatz commented Aug 28, 2022

k8s-triage-robot commented Nov 26, 2022

k8s-triage-robot commented Dec 26, 2022

k8s-triage-robot commented Jan 25, 2023

k8s-ci-robot commented Jan 25, 2023

rikatz commented May 29, 2022 •

edited

Loading

danwinship May 30, 2022 •

edited

Loading