-
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
Add Except for Antrea-native ipBlock #6658
base: main
Are you sure you want to change the base?
Conversation
c840536
to
cd59fed
Compare
cc5df40
to
1153d8c
Compare
Signed-off-by: Dyanngg <dingyang@vmware.com>
1153d8c
to
413edae
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.
ipNet := &group.IPNets[idx] | ||
if ipNet.Contains(ip) { | ||
for _, ipNet := range group.IPNets { | ||
if ipNet != nil && ipNet.Contains(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.
just out-of-curiosity, is the nil
case possible here (I am not asking to change the code even if this case is not possible in theory)?
} | ||
return antreaIPBlock, nil | ||
} | ||
|
||
// computeEffectiveIPNetForIPBlocks calculates the list of net.IPNet CIDRs after the | ||
// "except" CIDRs are subtracted from each corresponding ipBlock. | ||
func computeEffectiveIPNetForIPBlocks(ipBlocks []crdv1beta1.IPBlock) []*net.IPNet { |
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.
this function should be unit tested independently
controlplaneIPNet, _ := cidrStrToIPNet(cidr) | ||
controlplaneIPNetExcept1, _ := cidrStrToIPNet(exceptCIDR1) | ||
controlplaneIPNetExcept2, _ := cidrStrToIPNet(exceptCIDR2) | ||
_, controlplaneIPNetDiff, _ := net.ParseCIDR("10.0.0.192/26") |
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.
can we use a slightly more complicated case, where the computed controlplaneIPNetDiff
is not a single cidr?
I notice that the except cidrs you have are either at the very beginning or at the very end of the IP block, which means we only ever have a single resulting cidr
if ipb.CIDR == "" { | ||
return "field 'cidr' is required in an ipBlock", false | ||
} |
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.
We can keep this, but I assume this is guaranteed by the OpenAPI spec?
@@ -653,6 +653,105 @@ func TestValidateAntreaClusterNetworkPolicy(t *testing.T) { | |||
operation: admv1.Create, | |||
expectedReason: "group cannot be set with other peers in rules", | |||
}, | |||
{ | |||
name: "acnp-rule-ipblock-invalid-ipv4-cidr", |
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.
here as well, we can keep these 2 tests but this should be guaranteed by the cidr
format in the OpenAPI spec?
// There are three situations of a Pod's IP(s): | ||
// 1. Only one IPv4 address. | ||
// 2. Only one IPv6 address. | ||
// 3. One IPv4 and one IPv6 address, and we don't know the order in list. | ||
// We need to add all IP(s) of Pods as CIDR to IPBlock. |
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.
the comment doesn't match what the function is doing
SetAppliedToGroup([]ACNPAppliedToSpec{{PodSelector: map[string]string{"pod": "a"}, NSSelector: map[string]string{"ns": getNS("y")}}}). | ||
AddEgress(ProtocolTCP, &p80, nil, nil, nil, nil, nil, nil, nil, map[string]string{"pod": "a"}, nil, map[string]string{"ns": getNS("x")}, nil, nil, nil, nil, nil, | ||
crdv1beta1.RuleActionDrop, "", "egress-drop-xa", nil) | ||
// Make sure that the except IPs in the previous policy excluded from the drop rule but not explicitly allowed |
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 I understand this comment
as far as I can tell, there is nothing "explicitly allowed", and we are just adding an extra drop rule to supplement the previous one and drop one extra connection (y/a -> x/a)
ipBlock1 = append(ipBlock1, crdv1beta1.IPBlock{CIDR: genCIDR(podXCIP[i])}) | ||
ipBlock2 = append(ipBlock2, crdv1beta1.IPBlock{CIDR: genCIDR(podZAIP[i])}) | ||
for _, ips := range [][]string{podXAIP, podXBIP, podXCIP} { | ||
if ipb := genIPBlock(ips[i]); ipb != nil { |
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 feel like the fact that genIPBlock
can return nil
is just making the test code more verbose
// 2. Only one IPv6 address. | ||
// 3. One IPv4 and one IPv6 address, and we don't know the order in list. | ||
// We need to add all IP(s) of Pods as CIDR to IPBlock. | ||
func genIPBlock(ip string) *crdv1beta1.IPBlock { |
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.
s/genIPBlock/genIPBlockForIP
|
||
// genIPBlockWithExceptIPs generates ipBlocks which contains all the IP addresses in the | ||
// provided IPs' address family(s), except for the addresses in the input slice. | ||
func genIPBlockWithExceptIPs(except []string) []*crdv1beta1.IPBlock { |
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.
s/genIPBlockWithExceptIPs/genIPBlockForAllIPsExcept
Fixes #6428
This PR adds an "except" field for all ipBlocks in Antrea-native policies and groups. Users can exclude certain CIDRs from the ipBlock.cidr in all resources that support ipBlocks, including AntreaClusterNetworkPolicy, AntreaNetworkPolicy, ClusterGroup and Group. Group membership and IP association query logic are also updated to accommodate this change. Documentation will follow in a separate PR.