Do not realize AntreaNetworkPolicy applied to Pods in other Namespace #4119
Conversation
|
/test-all |
Codecov Report
@@ Coverage Diff @@
## main #4119 +/- ##
==========================================
- Coverage 65.70% 63.22% -2.49%
==========================================
Files 304 304
Lines 46604 46580 -24
==========================================
- Hits 30621 29450 -1171
- Misses 13557 14777 +1220
+ Partials 2426 2353 -73
*This pull request uses carry forward flags. Click here to find out more.
|
b97b7b1 to
891b95b
Compare
|
/test-all |
| intGrp := ig.(*antreatypes.Group) | ||
| if len(intGrp.IPBlocks) > 0 { | ||
| klog.V(2).InfoS("Group with IPBlocks can not be used as AppliedTo", "Group", key) | ||
| return nil |
There was a problem hiding this comment.
could you clarify why this is not an error case, maybe with a small comment?
is that not a case that needs to be reported to the user through the status?
There was a problem hiding this comment.
ClusterNetworkPolicy handles this case in this way. The patch unifies the behavior of ClusterNetworkPolicy and AntreaNetworkPolicy. I don't have strong preference about which action it should take, but I think the following reason may justify ignoring IPBlocks, and I have added it to code comment:
A Group may have child Groups, some of which contain regular Pod selectors and some of which contain IPBlocks.
When the Group is used as AppliedTo, it seems obvious that we should just apply NetworkPolicy to the selected
Pods and ignore the IPBlocks, instead of reporting errors and asking users to remove IPBlocks from child Groups,
as the Group could also be used as AddressGroup.
To keep the behavior consistent regarding IPBlocks, we ignore Groups containing only IPBlocks when it's used as AppliedTo.
| klog.V(2).Infof("Updating existing AppliedToGroup %s with %d Pods and %d External Entities on %d Nodes", | ||
| key, scheduledPodNum, scheduledExtEntityNum, appGroupNodeNames.Len()) |
There was a problem hiding this comment.
maybe grab this opportunity to change this to structured logging?
| }, | ||
| }, | ||
| { | ||
| name: "anp with invalid group", |
There was a problem hiding this comment.
maybe a more verbose name would be useful to understand why the group is invalid in this case: "anp with invalid group selecting pods in multiple Namespaces"
same for the test case below
| } | ||
| return &antreatypes.AppliedToGroup{UID: intGrp.UID, Name: key}, nil | ||
| func (e *ErrNetworkPolicyAppliedToUnsupportedGroup) Error() string { | ||
| return fmt.Sprintf("group %s/%s with Pods in other Namespaces can not be used as AppliedTo", e.namespace, e.groupName) |
There was a problem hiding this comment.
Should "group" be "Group"?
| group, found, _ := n.internalGroupStore.Get(g.Name) | ||
| if found { | ||
| // This AppliedToGroup is derived from a ClusterGroup. | ||
| grp := group.(*antreatypes.Group) | ||
| return n.getInternalGroupWorkloads(grp) | ||
| pods, ees := n.getInternalGroupWorkloads(grp) |
There was a problem hiding this comment.
We could potentially add an optional namespace check in GetEntities used by getInternalGroupWorkloads, so that we can return early and don't need to loop thru all entities again after getting a list of them. Doesn't need to be address in this PR; a TODO will suffice
There was a problem hiding this comment.
Thanks for the suggestion. I moved the namespace check into getInternalGroupWorkloads. I prefer not to add custom checks to GetEntities, which is a generic interface while checking namespace is a too business specific condition. There are other similar business specific checking/filtering and currently all of them are performed on caller side.
When AntreaNetworkPolicy uses Group as AppliedTo, the Group should not select Pods in other Namespaces, otherwise the policy would be applied to other Namespaces. This was prevented by using a validation when creating AppliedToGroup for Group, which ensures that the Group doesn't have a NamespaceSelector. However, the validation could be bypassed by several approaches, the most straightforward one is to use a parent Group as AppliedTo and make one of its child Groups use NamespaceSelector. It's hard to cover all cases if the validation is only in the phase of creating AppliedToGroup because of its dynamic nature. This patch implements a validation when syncing AppliedToGroup. The validation ensures that the AppliedToGroup cannot have any members in other Namespaces if it's derived from a namespaced Group, regardless of the way by which the members are selected. The error encountered when syncing AppliedToGroup will be reflected in the statuses of the NetworkPolicies that use this AppliedToGroup. This patch also unifies the behavior of ClusterNetworkPolicy and AntreaNetworkPolicy when a ClusterGroup/Group used as AppliedTo contains IPBlocks only: it would be treated like empty AppliedTo in both cases. Fixes antrea-io#4116 Signed-off-by: Quan Tian <qtian@vmware.com>
0ff0557
891b95b to
0ff0557
Compare
|
/test-all |
…antrea-io#4119) When AntreaNetworkPolicy uses Group as AppliedTo, the Group should not select Pods in other Namespaces, otherwise the policy would be applied to other Namespaces. This was prevented by using a validation when creating AppliedToGroup for Group, which ensures that the Group doesn't have a NamespaceSelector. However, the validation could be bypassed by several approaches, the most straightforward one is to use a parent Group as AppliedTo and make one of its child Groups use NamespaceSelector. It's hard to cover all cases if the validation is only in the phase of creating AppliedToGroup because of its dynamic nature. This patch implements a validation when syncing AppliedToGroup. The validation ensures that the AppliedToGroup cannot have any members in other Namespaces if it's derived from a namespaced Group, regardless of the way by which the members are selected. The error encountered when syncing AppliedToGroup will be reflected in the statuses of the NetworkPolicies that use this AppliedToGroup. This patch also unifies the behavior of ClusterNetworkPolicy and AntreaNetworkPolicy when a ClusterGroup/Group used as AppliedTo contains IPBlocks only: it would be treated like empty AppliedTo in both cases. Fixes antrea-io#4116 Signed-off-by: Quan Tian <qtian@vmware.com> Signed-off-by: Quan Tian <qtian@vmware.com>
When AntreaNetworkPolicy uses Group as AppliedTo, the Group should not
select Pods in other Namespaces, otherwise the policy would be applied
to other Namespaces. This was prevented by using a validation when
creating AppliedToGroup for Group, which ensures that the Group doesn't
have a NamespaceSelector. However, the validation could be bypassed by
several approaches; the most straightforward one is to use a parent
Group as AppliedTo and make one of its child Groups use
NamespaceSelector.
It's hard to cover all cases if the validation is only in the phase of
creating AppliedToGroup because of its dynamic nature. This patch
implements a validation when syncing AppliedToGroup. The validation
ensures that the AppliedToGroup cannot have any members in other
Namespaces if it's derived from a namespaced Group, regardless of the
way by which the members are selected. The error encountered when
syncing AppliedToGroup will be reflected in the statuses of the
NetworkPolicies that use this AppliedToGroup.
This patch also unifies the behavior of ClusterNetworkPolicy and
AntreaNetworkPolicy when a ClusterGroup/Group used as AppliedTo contains
IPBlocks only: it would be treated like empty AppliedTo in both cases.
Fixes #4116
Signed-off-by: Quan Tian qtian@vmware.com