refactor: add receiver to iptables and create interface #2421
Conversation
QxBytes
force-pushed
the
alew/iptables
branch
from
eb89d40 to
25d5eab
Compare
QxBytes
force-pushed
the
alew/iptables
branch
2 times, most recently
from
343ceee to
f26f229
Compare
QxBytes
force-pushed
the
alew/iptables
branch
from
bbbf7c5 to
25ef4c7
Compare
| @@ -0,0 +1,9 @@ | |||
| package network | |||
There was a problem hiding this comment.
this should be in iptable package and export interface type
There was a problem hiding this comment.
No, that's not how we do interfaces in Go. The interface exists to abstract the behavior that your code is using from the backing implementation of that behavior at your usage of that behavior. This package only needs this subset of behavior, so the interface should be defined in this package.
There was a problem hiding this comment.
https://go.dev/doc/effective_go#interfaces
"If something can do this, it can be used here."
Don't predefine an interface that simply matches a type.
There was a problem hiding this comment.
i guess iptables package used in other components like cns i guess.. if its just used in network package, then i agree
QxBytes
force-pushed
the
alew/iptables
branch
from
d9c7fff to
7ed2c6e
Compare
| type Client struct{} | ||
|
|
||
| func NewClient() *Client { | ||
| return &Client{} | ||
| } |
There was a problem hiding this comment.
there are no fields (at all) being set in this struct, or other initialization magic - we don't need a constructor and can make the struct directly ... := &iptables.Client{}
| _, err := client.plClient.ExecuteCommand(enableIPForwardCmd) | ||
| if err != nil { | ||
| return errors.Wrap(err, "enable ipforwarding command failed") | ||
| } |
There was a problem hiding this comment.
hm, I wish we didn't have this in the codebase already... I generally don't think that it's our responsibility to configure the host like this.
There was a problem hiding this comment.
talked with @QxBytes offline and clarified standard practice is to define interface in consumer side
QxBytes
dismissed stale reviews from tamilmani1989, vipul-21, and rbtr
via
eaad01c
QxBytes
force-pushed
the
alew/iptables
branch
2 times, most recently
from
e640d0a to
43b4726
Compare
QxBytes
force-pushed
the
alew/iptables
branch
from
43b4726 to
d621c22
Compare
* Move network utils functions with iptables to new file * Add receiver to iptables and create interface * Resolve conflicts from rebasing * Add changes for building on windows * Address linter issues * Address windows linter issues * Invert if condition for linter nesting * Scope iptables interfaces to package * Rename iptables client to avoid stuttering * Move EnableIPForwarding to snat linux * Rename ipTablesClientInterface to ipTablesClient * Address linter issues from moving enable ip forwarding function * Rename after rebase
* Move network utils functions with iptables to new file * Add receiver to iptables and create interface * Resolve conflicts from rebasing * Add changes for building on windows * Address linter issues * Address windows linter issues * Invert if condition for linter nesting * Scope iptables interfaces to package * Rename iptables client to avoid stuttering * Move EnableIPForwarding to snat linux * Rename ipTablesClientInterface to ipTablesClient * Address linter issues from moving enable ip forwarding function * Rename after rebase
Reason for Change:
The iptables package is currently a set of functions without a receiver. Because of this, any code that uses the iptables package has a dependency on it, and proper unit tests cannot be written. This PR will add a receiver to the iptables methods and create an interface in the network folder so that in the future, unit tests can mock the iptables interface and methods that use iptables can be tested. This PR should not change the functionality of the codebase (for example, existing tests, if they use iptables, should still use a non-mocked version of the new iptables client).
Issue Fixed:
See above.
Requirements:
Notes:
Newfunction (ex:Client{ }) has been checked to ensure either an iptablesClient is passed in if the struct now requires aniptablesClient, or is part of testing code.