Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: rule: add String() method #108

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+70 −0
Open

WIP: rule: add String() method #108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
nftables.test
8 changes: 8 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,12 @@ the data types/API will be identified as more functionality is added.

Contributions are very welcome!

### Testing Changes

Run the following commands to test your changes:

```bash
go test ./...
go test -c github.com/google/nftables
sudo ./nftables.test -test.v -run_system_tests
```
43 changes: 43 additions & 0 deletions expr/verdict.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ import (
"golang.org/x/sys/unix"
)

const (
NFT_DROP = 0
NFT_ACCEPT = 1
NFT_STOLEN = 2
NFT_QUEUE = 3
NFT_REPEAT = 4
NFT_STOP = 5
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we not have these declared before? Where are they canonically defined in the upstream sources? Can you add a link please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we not have these declared before? Where are they canonically defined in the upstream sources? Can you add a link please?

There is no definition. They are defined with the following:

const (
    VerdictReturn VerdictKind = iota - 5
    VerdictGoto
    VerdictJump
    VerdictBreak
    VerdictContinue
    VerdictDrop
    VerdictAccept
    VerdictStolen
    VerdictQueue
    VerdictRepeat
    VerdictStop
)

I am looking in https://github.com/torvalds/linux/blob/47ec5303d73ea344e84f46660fff693c57641386/include/uapi/linux/netfilter/nf_tables.h#L64-L70 ... yet to discover.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another reference: https://github.com/torvalds/linux/blob/b71acb0e372160167bf6d5500b88b30b52ccef6e/include/uapi/linux/netfilter.h#L10-L17

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stapelberg , in retrospect, after finding https://git.netfilter.org/libnftnl/tree/src/utils.c#n182, I say was mimicking this function 😄

const char *nftnl_verdict2str(uint32_t verdict)
{
	switch (verdict) {
	case NF_ACCEPT:
		return "accept";
	case NF_DROP:
		return "drop";
	case NF_STOLEN:
		return "stolen";
	case NF_QUEUE:
		return "queue";
	case NF_REPEAT:
		return "repeat";
	case NF_STOP:
		return "stop";
	case NFT_RETURN:
		return "return";
	case NFT_JUMP:
		return "jump";
	case NFT_GOTO:
		return "goto";
	case NFT_CONTINUE:
		return "continue";
	case NFT_BREAK:
		return "break";
	default:
		return "unknown";
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stapelberg , also the NF_STOLEN comes from https://git.netfilter.org/libnftnl/tree/include/linux/netfilter.h#n9

/* Responses from hook functions. */
#define NF_DROP 0
#define NF_ACCEPT 1
#define NF_STOLEN 2
#define NF_QUEUE 3
#define NF_REPEAT 4
#define NF_STOP 5
#define NF_MAX_VERDICT NF_STOP

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After having another look, I think this duplicates the verdict codes we already have defined here, no?

nftables/expr/verdict.go

Lines 39 to 54 in c25e4f6

type VerdictKind int64
// Verdicts, as per netfilter.h and netfilter/nf_tables.h.
const (
VerdictReturn VerdictKind = iota - 5
VerdictGoto
VerdictJump
VerdictBreak
VerdictContinue
VerdictDrop
VerdictAccept
VerdictStolen
VerdictQueue
VerdictRepeat
VerdictStop
)

Why not just use e.g. VerdictReturn in the switch statement below in String()?

)

// This code assembles the verdict structure, as expected by the
// nftables netlink API.
// For further information, consult:
Expand Down Expand Up @@ -126,3 +135,37 @@ func (e *Verdict) unmarshal(data []byte) error {
}
return ad.Err()
}

func (e *Verdict) String() string {
var v string
switch e.Kind {
case unix.NFT_RETURN:
v = "return" // -0x5
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What did you want to express with these comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What did you want to express with these comments?

@stapelberg , just a note as to what the id is for the return.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you follow my other suggestion above regarding using the Verdict* names, I think it’d be cleaner to not duplicate these values (we only want a single place to update, not multiple). If people are inclined to find out the value, they should jump to the definition using their editor.

case unix.NFT_GOTO:
v = "goto" // -0x4
case unix.NFT_JUMP:
v = "jump" // NFT_JUMP = -0x3
case unix.NFT_BREAK:
v = "break" // NFT_BREAK = -0x2
case unix.NFT_CONTINUE:
v = "continue" // NFT_CONTINUE = -0x1
case NFT_DROP:
v = "drop"
case NFT_ACCEPT:
v = "accept"
case NFT_STOLEN:
v = "stolen"
case NFT_QUEUE:
v = "queue"
case NFT_REPEAT:
v = "repeat"
case NFT_STOP:
v = "stop"
default:
v = fmt.Sprintf("verdict %v", e.Kind)
}
if e.Chain != "" {
return v + " " + e.Chain
}
return v
}
15 changes: 15 additions & 0 deletions nftables_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,12 +252,27 @@ func TestRuleOperations(t *testing.T) {
expr.VerdictDrop,
}

wantStrings := []string{
"queue",
"accept",
"queue",
"accept",
"drop",
"drop",
}

for i, r := range rules {
rr, _ := r.Exprs[0].(*expr.Verdict)

if rr.Kind != want[i] {
t.Fatalf("bad verdict kind at %d", i)
}

if rr.String() != wantStrings[i] {
t.Fatalf("bad verdict string at %d: %s (received) vs. %s (expected)", i, rr.String(), wantStrings[i])
}

t.Logf("%s", rr)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove debugging left-over?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove debugging left-over?

@stapelberg , 👍 will do prior to removing WIP.

}
}

Expand Down
3 changes: 3 additions & 0 deletions nftables_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
go test ./...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this file from the commit?

Making testing a little easier is a good idea in general, but I’d like to avoid establishing precedent of shell scripts :)

Maybe a “make test” target in a Makefile in a separate PR would be the best way forward?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this file from the commit?

@stapelberg , 👍 will do prior to removing WIP.

Maybe a “make test” target in a Makefile in a separate PR would be the best way forward?

@stapelberg , that would be nice! I agree.

go test -c github.com/google/nftables
sudo ./nftables.test -test.v -run_system_tests