-
Notifications
You must be signed in to change notification settings - Fork 363
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
Update iptables builder #6381
Update iptables builder #6381
Conversation
/skip-all |
@@ -31,6 +31,7 @@ import ( | |||
"antrea.io/antrea/pkg/agent/config" | |||
"antrea.io/antrea/pkg/agent/route" | |||
"antrea.io/antrea/pkg/agent/types" | |||
ipsetutil "antrea.io/antrea/pkg/agent/util/ipset" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not rename the import here, especially since there is no conflict and we do not do it for "antrea.io/antrea/pkg/util/ip"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed the import since the function there is a variable called ipset
where the import is used. I have removed the renaming and added a global variable var ipsetTypeHashIP = ipset.HashIP
to avoid duplicated ipset
.
pkg/agent/util/iptables/builder.go
Outdated
func (b *iptablesRuleBuilder) MatchIPSetSrc(ipset string) IPTablesRuleBuilder { | ||
if ipset == "" { | ||
func (b *iptablesRuleBuilder) MatchIPSrc(ip string) IPTablesRuleBuilder { | ||
if ip == "" || ip == "0.0.0.0" || ip == "::" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is really meaningful. For example, "0.0.0.0" doesn't usually mean "match any IP address". This is different from the meaning of CIDR "0.0.0.0/0".
IMO, we should not add these new methods (MatchIPSrc
, MatchIPDst
). I didn't see a compelling reason to do it. Instead of MatchIPSrc("192.168.77.100")
, we can just do MatchCIDRSrc("192.168.77.100/32")
, or even MatchCIDRSrc("192.168.77.100")
. From an iptables perspective this is the same, and we don't do any input validation anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I have removed these two methods and added extra checks to skip 0.0.0.0
or ::
to MatchCIDRSrc
and MatchCIDRDst
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not exactly what I meant. "0.0.0.0"
and "::"
don't usually mean "match any IP address of that family" AFIK. So I don't think we should handle them in a special way. If someone wants to explicitly match any IP for some reason, they should use the proper CIDR ("0.0.0.0/0"
/ "::/0"
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, 0.0.0.0
or ::
means "matching nonthing IP address. Thanks for pointing out this. 0.0.0.0/0
or ::/0
should be used to match any IPv4 or IPv6 addresses.
f118d76
to
385a32d
Compare
This PR adds more methods to build an iptables entries: - `SetTargetDNATToDst`, setting DNAT destination IP and port. This PR also updates the methods: - `MatchCIDRSrc`, supporting an IP or CIDR as source. - `MatchCIDRDst`, supporting an IP or CIDR as destination. - `MatchIPSetSrc`, supporting ipset type of hashIPPort as destination IP and port. - `MatchIPSetDst`, supporting ipset type of hashIPPort as source IP and port. - Rename `MatchDstPort` to `MatchPortDst` to be consistent with other method. - Rename `MatchSrcPort` to `MatchPortSrc` to be consistent with other method. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
385a32d
to
e621be3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-all |
Needed by #6308
This PR adds more methods to build an iptables entries:
SetTargetDNATToDst
, setting DNAT destination IP and port.This PR also updates the methods:
MatchCIDRSrc
, supporting an IP or CIDR as source.MatchCIDRDst
, supporting an IP or CIDR as destination.MatchIPSetSrc
, supporting ipset type of hashIPPort as destination IP and port.MatchIPSetDst
, supporting ipset type of hashIPPort as source IP and port.MatchDstPort
toMatchPortDst
to be consistent with other method.MatchSrcPort
toMatchPortSrc
to be consistent with other method.