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.

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

Request for comment: Introduce UserData struct in a rule #48

Closed
sbezverk opened this issue Aug 14, 2019 · 3 comments · Fixed by #49
Closed

Request for comment: Introduce UserData struct in a rule #48

sbezverk opened this issue Aug 14, 2019 · 3 comments · Fixed by #49

Comments

@sbezverk
Copy link
Contributor

sbezverk commented Aug 14, 2019

Currently if the process which created a specific rule in nftables restart there is no easy way to associate it with this rule consumer.
Example when kube-proxy creates iptables rule for a service or an endpoint, even if kube-proxy restarts it can compute this association from a rule.
To achieve something similar with nftables I propose to add UserData struct as a part of Rule struct. Rule's UserData is preserved in the kernel so it can reliably retrieved even if the process which created a specific rule was restarted.
I propose:

type Rule struct {
	Table    *Table
	Chain    *Chain
	RuleID   uint32    < ----- Remove
	Position uint64
	Handle   uint64
	Exprs    []expr.Any
        UserData *UserData  < ----- Add this struct
}

type UserData struct {
        RuleID   uint32   < -------- Move here to preserve current functionality
        Data .   []byte
}

RuleID must be moved inside of a UserData struct as currently RuleID already uses UserData but just for itself. With this approach UserData will carry RuleID and byte slice which can be use to store any arbitrary data, example json with details on a service or endpoint which is using this rule.

I will do the implementation, but I would your opinion about it.

@stapelberg WDYT??

@stapelberg
Copy link
Collaborator

As per https://sourcegraph.com/github.com/google/nftables@master/-/blob/rule.go#L34:2&tab=references, your nftableslib is the only public user of RuleID currently.

Hence, I think we don’t need to jump through hoops to preserve compatibility, and can favor a cleaner design, where RuleID is dropped entirely and the caller persists the RuleID in UserData if desired.

Does that work?

@sbezverk
Copy link
Contributor Author

Thank you for your feedback. Unfortunately no, I do use SecureID inside nftableslib just at the point when rule gets created, it is needed to get newly created rule's handle, and I would not want to use UserData.Data as higher level app, example kube-proxy might use it with some encoding which nftableslib has no visibility or knowledge. As you can see eliminating RuleID completely, poses some major problem for me.

@sbezverk
Copy link
Contributor Author

After more thorough consideration, I think it will work. I am pushing updated PR with no RuleID.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants