libnet/d/bridge: move iptRule to iptables pkg by akerouanton · Pull Request #49125 · moby/moby

akerouanton · 2024-12-18T09:10:26Z

- What I did

This moves the iptRule struct from the bridge package to the iptables pkg.

- How to verify it

Existing tests, plus a new unit test.

thaJeztah · 2024-12-18T09:50:57Z

Looks like the linter is unhappy (because use of unkeyed literals); not sure if you're using GoLand, but that one can "fill in all keys" for you;

libnetwork/drivers/bridge/setup_ip_tables_linux_test.go:418:23: composites: github.com/docker/docker/libnetwork/iptables.Rule struct literal uses unkeyed fields (govet)
				{tc.wantIPv4Masq, iptables.Rule{iptables.IPv4, iptables.Nat, "POSTROUTING", []string{"-s", maskedBrIPv4.String(), "!", "-o", br, "-j", "MASQUERADE"}}},
				                  ^
libnetwork/drivers/bridge/setup_ip_tables_linux_test.go:419:23: composites: github.com/docker/docker/libnetwork/iptables.Rule struct literal uses unkeyed fields (govet)
				{tc.wantIPv4Snat, iptables.Rule{iptables.IPv4, iptables.Nat, "POSTROUTING", []string{"-s", maskedBrIPv4.String(), "!", "-o", br, "-j", "SNAT", "--to-source", hostIPv4.String()}}},
				                  ^
libnetwork/drivers/bridge/setup_ip_tables_linux_test.go:420:23: composites: github.com/docker/docker/libnetwork/iptables.Rule struct literal uses unkeyed fields (govet)
				{tc.wantIPv6Masq, iptables.Rule{iptables.IPv6, iptables.Nat, "POSTROUTING", []string{"-s", maskedBrIPv6.String(), "!", "-o", br, "-j", "MASQUERADE"}}},
				                  ^
libnetwork/drivers/bridge/setup_ip_tables_linux_test.go:421:23: composites: github.com/docker/docker/libnetwork/iptables.Rule struct literal uses unkeyed fields (govet)
				{tc.wantIPv6Snat, iptables.Rule{iptables.IPv6, iptables.Nat, "POSTROUTING", []string{"-s", maskedBrIPv6.String(), "!", "-o", br, "-j", "SNAT", "--to-source", hostIPv6.String()}}},
				                  ^

This moves the iptRule struct from the bridge package to the iptables pkg. Signed-off-by: Albin Kerouanton <[email protected]>

thaJeztah · 2024-12-19T10:15:17Z

-
-// Exists returns true if the rule exists in the kernel.
-func (r iptRule) Exists() bool {
-	return iptables.GetIptable(r.ipv).Exists(r.table, r.chain, r.args...)


I was slightly trying to grasp the reason behind this change; because previously this was non-exported and internal to the bridge driver. Is the intent to make this type more widely used, and used as an abstraction for iptables + nftables?

In that case, I'm wondering if it's currently the right abstraction, or at least, I'm wondering if all fields it defines are "portable";

Should there be an interface defined instead (providing Exists(), Append(), Insert(), Delete())

The fields to be unexported, and instead having a constructor (iptables.NewRule(...))

Alternatively, the iptables package to have Delete(rule), Insert(rule) functions (which could potentially become an abstraction?)

Do you have more context to this change?

The end goal is to decouple the bridge driver from iptables.

I have a wip branch where I'm moving iptables rules into a new internal package. There will be transitional commits where both the bridge package and the new fwipt pacakge will need this struct.

iptRule is currently the best iptables abstraction we have. It'd be great to converge on that and drop some of the alternative functions we've (eg. ProgramRule).

and used as an abstraction for iptables + nftables?

No, we won't use the same abstraction for both, they're too different. So, no need to worry about 'portability'.

iptRule is currently the best iptables abstraction we have

Yeah, the args feels like a leaky abstraction; the combination of args (doing the actual work) with some defined fields, is a bit odd 😂

thaJeztah · 2024-12-19T10:21:58Z

-		}
-		rule := iptRule{ipv: ipv, table: iptables.Mangle, chain: "POSTROUTING", args: args}
+		}}
 		if err := appendOrDelChainRule(rule, "SCTP CHECKSUM", enable); err != nil {


If we're moving, would it make sense to put the comment as a field in the Rule? Possibly the error-message generating could then be fully delegated to the rule.Append and rule.Delete functions, because they both know whether something was added or deleted that failed, and they would have the comment related to it;

moby/libnetwork/drivers/bridge/setup_ip_tables_linux.go

Lines 544 to 555 in 40f58b3

func appendOrDelChainRule(rule iptRule, ruleDescr string, append bool) error {

operation := "disable"

fn := rule.Delete

if append {

operation = "enable"

fn = rule.Append

}

if err := fn(); err != nil {

return fmt.Errorf("Unable to %s %s rule: %w", operation, ruleDescr, err)

}

return nil

}

Good idea!

I originally thought about adding a Comment field to automatically add -m comment --comment <COMMENT> to iptables rules, but that's quite involved because we need to handle 'migration' (eg. we should not create duplicate rules, one with and one without a comment). But what you're proposing sounds good too.

However, that will need a bit more work as not all rules are currently inserted by an helper that takes a comment, and some comments might be too unspecific to be really helpful when debugging.

Since this PR doesn't deal with comments, I'd prefer to address that in a follow-up.

thaJeztah · 2024-12-19T12:47:55Z

+// Exists returns true if the rule exists in the kernel.
+func (r Rule) Exists() bool {


Note; I went looking where Exists() was used outside of this type itself (i.e., as part of functions like Append()), and it looks like it's only used in tests to assert that some actions happened.

If we're gonna work on follow-ups, we could look if we need Exists() to be part of this interface, or if it would be a dedicated test-utility. We could still split it between a non-exported one (for the internal use) and an exported one, to make it easier to discover where the exported one is used.

thaJeztah

LGTM

akerouanton added status/2-code-review area/networking Networking kind/refactor PR's that refactor, or clean-up code area/networking/firewalling Networking labels Dec 18, 2024

akerouanton added this to the 28.0.0 milestone Dec 18, 2024

akerouanton requested review from robmry and thaJeztah December 18, 2024 09:10

akerouanton self-assigned this Dec 18, 2024

libnet/d/bridge: move iptRule to iptables pkg

ec8a5b0

This moves the iptRule struct from the bridge package to the iptables pkg. Signed-off-by: Albin Kerouanton <[email protected]>

akerouanton force-pushed the move-iptRule-to-ipt-pkg branch from 89de986 to ec8a5b0 Compare December 18, 2024 10:01

robmry approved these changes Dec 18, 2024

View reviewed changes

thaJeztah reviewed Dec 19, 2024

View reviewed changes

thaJeztah approved these changes Dec 19, 2024

View reviewed changes

thaJeztah merged commit 4e3202a into moby:master Dec 19, 2024

akerouanton deleted the move-iptRule-to-ipt-pkg branch December 19, 2024 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libnet/d/bridge: move iptRule to iptables pkg#49125

libnet/d/bridge: move iptRule to iptables pkg#49125
thaJeztah merged 1 commit intomoby:masterfrom
akerouanton:move-iptRule-to-ipt-pkg

akerouanton commented Dec 18, 2024

Uh oh!

thaJeztah commented Dec 18, 2024

Uh oh!

thaJeztah Dec 19, 2024

Uh oh!

akerouanton Dec 19, 2024

Uh oh!

thaJeztah Dec 19, 2024

Uh oh!

thaJeztah Dec 19, 2024

Uh oh!

akerouanton Dec 19, 2024 •

edited

Loading

Uh oh!

thaJeztah Dec 19, 2024

Uh oh!

thaJeztah left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	func appendOrDelChainRule(rule iptRule, ruleDescr string, append bool) error {
	operation := "disable"
	fn := rule.Delete
	if append {
	operation = "enable"
	fn = rule.Append
	}
	if err := fn(); err != nil {
	return fmt.Errorf("Unable to %s %s rule: %w", operation, ruleDescr, err)
	}
	return nil
	}

		// Exists returns true if the rule exists in the kernel.
		func (r Rule) Exists() bool {

Conversation

akerouanton commented Dec 18, 2024

Uh oh!

thaJeztah commented Dec 18, 2024

Uh oh!

thaJeztah Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

akerouanton Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

akerouanton Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

akerouanton Dec 19, 2024 •

edited

Loading