Impossible to create a chain with a DROP default policy about nftables HOT 6 CLOSED

Thanks for the detailed report!

I like this approach the most:

• We could create a new type in chain.go, called PolicyVerdict, that would be a uint32. We also make two const, PolicyDrop and PolicyAccept, which would be defined as equal to iota (so 0 and 1, exactly the netfilter numbers for DROP and ACCEPT.

Wouldn’t we have the same problem that we currently have if DROP maps to 0? Shouldn’t we use 1+x as our value space to distinguish the empty value from an explicitly set value?

This solution makes things easily understandable without any extra import, allow additional documentation about the Policy field (currently there isn't any doc about that field beside its definition) but is kind of redundant with the VerdictKind type already existing in expr submodule.

I’m not too worried about that redundancy. Verdicts are used in different contexts than Policies, so they can have a separate type.

from nftables.

You indeed have a point about DROP mapping to 0, which makes it impossible to make the difference between an explicitely defined DROP policy and an undefined one.

I personally dislike the "hacky" approach of +1/-1, as it tends to get confusing for the end user dev and also can cause problems if you forget for a minute that it's here while working onto improving the type.

I'd suggest making the Policy field in Chain a pointer upon our new type. It will come with the drawback that the user cannot simply use our new type constants directly, (because you'd need to take the address of the constant through &, which isn't possible because of limitations of the language itself), but at the same time it'll ensure the difference between uninitialised Policy field (which would be nil) and initialised field set to DROP (which would be a pointer to a myType which value would be 0 after dereferencing it).

Also, for the sake of keeping the same formatting as other types in chain.go, I'll name the new type ChainPolicy. It sounds better than PolicyVerdict and fits the Chain_somethingsomething_ format that is already used.

Here's few snipets of how I imagine the changes in the existing code and how it'd impact the use

Definition:

// ChainPolicy defines what this chain default policy will be.
type ChainPolicy uint32

// Possible ChainPolicy values.
const (
	ChainPolicyDrop ChainPolicy = iota
	ChainPolicyAccept
)

// A Chain contains Rules. See also
// https://wiki.nftables.org/wiki-nftables/index.php/Configuring_chains
type Chain struct {
	Name     string
	Table    *Table
	Hooknum  ChainHook
	Priority ChainPriority
	Type     ChainType
	Policy   *ChainPolicy
}

That way we just have to check if it's nil or not, and change the way Policy is used in Chain related functions so it dereferences the pointer when the value is needed.

Effectively, the user would have to create his chains that way with those modifications:

var defaultPolicy = nftables.ChainPolicyDrop

conn.AddTable(&nftables.Table{
		Family: nftables.TableFamilyIPv4,
		Name: "filter",
	})

conn.AddChain(&nftables.Chain{
		Name: "forward",
		Table: filterTable,
		Type: nftables.ChainTypeFilter,
		Hooknum: nftables.ChainHookForward,
		Priority: nftables.ChainPriorityFilter,
		Policy: &defaultPolicy,
	})

I don't see any other way to keep the user space values aligned with the kernel space values, while solving our undefined != defined at DROP issue.

Let me know if you have any other idea or feedback on this one.

from nftables.

I’m a bit torn on using pointers. We don’t consistently do that in our current data structures, so introducing that here creates a bit of a mismatch.

On the other hand, I understand your concern about the constants not matching, which can be very confusing when debugging, depending on how you print data/values.

Perhaps using a pointer is the lesser evil after all.

from nftables.

There could be another approach but it also has its own pros and cons.

The MySQL connector for go deals with non-nullable types by creating its own types, such has this NullInt32.

Pros:

• Structs have their fields initialised by their default values. In our case, the uint32 would be initialised to 0, and the bool to false, which makes the check of an explicitly defined policy very easy.
• DROP will be equal to 0, ACCEPT to 1, so we don't need to use the +1/-1 solution.
• It won't use pointers, which, I agree, can get confusing on devs who come from more high level languages/networking world and have reduced knowledge on how (de)referencing works. It should save some of us a lot of headaches.

Cons:

• We can't create constant structs (language limitation) BUT there would be a workaround were we could call a function that returns us an ACCEPT/DROP struct (so we can imagine something like nftables.ChainDefaultPolicy(nftables.ChainPolicyDrop) that returns a struct initialised with uint32 at 0 and bool at true).
• This approach is probably harder to understand at first even if its usability is better than pointers.

from nftables.

Thanks for the thorough research.

I think the pointer solution is the most appealing over all, if only because some of our tables already have a *Table pointer field.

from nftables.

I'm closing this as PR has been merged.

from nftables.