Skip to content

Commit

Permalink
Do not realize AntreaNetworkPolicy applied to Pods in other Namespace (…
Browse files Browse the repository at this point in the history 
…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>
  • Loading branch information
@tnqn @heanlan
tnqn authored and heanlan committed Mar 29, 2023
1 parent 1f5c46e commit f43d688
Show file tree
Hide file tree
Showing 10 changed files with 489 additions and 308 deletions.
67 changes: 9 additions & 58 deletions pkg/controller/networkpolicy/antreanetworkpolicy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"antrea.io/antrea/pkg/apis/controlplane"
crdv1alpha1 "antrea.io/antrea/pkg/apis/crd/v1alpha1"
antreatypes "antrea.io/antrea/pkg/controller/types"
"antrea.io/antrea/pkg/util/k8s"
)

func getANPReference(anp *crdv1alpha1.NetworkPolicy) *controlplane.NetworkPolicyReference {
Expand Down Expand Up @@ -87,35 +86,15 @@ func (n *NetworkPolicyController) processAntreaNetworkPolicy(np *crdv1alpha1.Net
appliedToGroups := map[string]*antreatypes.AppliedToGroup{}
addressGroups := map[string]*antreatypes.AddressGroup{}
rules := make([]controlplane.NetworkPolicyRule, 0, len(np.Spec.Ingress)+len(np.Spec.Egress))
newUnrealizableInternalNetworkPolicy := func(err error) *antreatypes.NetworkPolicy {
return &antreatypes.NetworkPolicy{
SourceRef: &controlplane.NetworkPolicyReference{
Type: controlplane.AntreaNetworkPolicy,
Namespace: np.Namespace,
Name: np.Name,
UID: np.UID,
},
Name: internalNetworkPolicyKeyFunc(np),
UID: np.UID,
Generation: np.Generation,
RealizationError: err,
}
}
// Create AppliedToGroup for each AppliedTo present in AntreaNetworkPolicy spec.
atgs, err := n.processAppliedTo(np.Namespace, np.Spec.AppliedTo)
if err != nil {
return newUnrealizableInternalNetworkPolicy(err), nil, nil
}
atgs := n.processAppliedTo(np.Namespace, np.Spec.AppliedTo)
appliedToGroups = mergeAppliedToGroups(appliedToGroups, atgs...)
// Compute NetworkPolicyRule for Ingress Rule.
for idx, ingressRule := range np.Spec.Ingress {
// Set default action to ALLOW to allow traffic.
services, namedPortExists := toAntreaServicesForCRD(ingressRule.Ports, ingressRule.Protocols)
// Create AppliedToGroup for each AppliedTo present in the ingress rule.
atgs, err := n.processAppliedTo(np.Namespace, ingressRule.AppliedTo)
if err != nil {
return newUnrealizableInternalNetworkPolicy(err), nil, nil
}
atgs := n.processAppliedTo(np.Namespace, ingressRule.AppliedTo)
appliedToGroups = mergeAppliedToGroups(appliedToGroups, atgs...)
peer, ags := n.toAntreaPeerForCRD(ingressRule.From, np, controlplane.DirectionIn, namedPortExists)
addressGroups = mergeAddressGroups(addressGroups, ags...)
Expand All @@ -135,10 +114,7 @@ func (n *NetworkPolicyController) processAntreaNetworkPolicy(np *crdv1alpha1.Net
// Set default action to ALLOW to allow traffic.
services, namedPortExists := toAntreaServicesForCRD(egressRule.Ports, egressRule.Protocols)
// Create AppliedToGroup for each AppliedTo present in the egress rule.
atgs, err := n.processAppliedTo(np.Namespace, egressRule.AppliedTo)
if err != nil {
return newUnrealizableInternalNetworkPolicy(err), nil, nil
}
atgs := n.processAppliedTo(np.Namespace, egressRule.AppliedTo)
appliedToGroups = mergeAppliedToGroups(appliedToGroups, atgs...)
var peer *controlplane.NetworkPolicyPeer
if egressRule.ToServices != nil {
Expand Down Expand Up @@ -179,54 +155,29 @@ func (n *NetworkPolicyController) processAntreaNetworkPolicy(np *crdv1alpha1.Net
return internalNetworkPolicy, appliedToGroups, addressGroups
}

func (n *NetworkPolicyController) processAppliedTo(namespace string, appliedTo []crdv1alpha1.NetworkPolicyPeer) ([]*antreatypes.AppliedToGroup, error) {
func (n *NetworkPolicyController) processAppliedTo(namespace string, appliedTo []crdv1alpha1.NetworkPolicyPeer) []*antreatypes.AppliedToGroup {
var appliedToGroups []*antreatypes.AppliedToGroup
for _, at := range appliedTo {
var atg *antreatypes.AppliedToGroup
if at.Group != "" {
var err error
atg, err = n.createAppliedToGroupForNamespacedGroup(namespace, at.Group)
if err != nil {
return nil, err
}
atg = n.createAppliedToGroupForGroup(namespace, at.Group)
} else {
atg = n.createAppliedToGroup(namespace, at.PodSelector, at.NamespaceSelector, at.ExternalEntitySelector)
}
if atg != nil {
appliedToGroups = append(appliedToGroups, atg)
}
}
return appliedToGroups, nil
return appliedToGroups
}

// ErrNetworkPolicyAppliedToUnsupportedGroup is an error response when
// a Group with IPBlocks or NamespaceSelector is used as AppliedTo.
// a Group with Pods in other Namespaces is used as AppliedTo.
type ErrNetworkPolicyAppliedToUnsupportedGroup struct {
namespace string
groupName string
}

func (e ErrNetworkPolicyAppliedToUnsupportedGroup) Error() string {
return fmt.Sprintf("group %s/%s with IPBlocks or NamespaceSelector can not be used as AppliedTo", e.namespace, e.groupName)
}

func (n *NetworkPolicyController) createAppliedToGroupForNamespacedGroup(namespace, groupName string) (*antreatypes.AppliedToGroup, error) {
// Namespaced group uses NAMESPACE/NAME as the key of the corresponding internal group.
key := k8s.NamespacedName(namespace, groupName)
// Find the internal Group corresponding to this Group.
// There is no need to check if the namespaced group exists in groupLister because its existence will eventually be
// reflected in internalGroupStore.
ig, found, _ := n.internalGroupStore.Get(key)
if !found {
// Internal Group is not found, which means the corresponding namespaced group is either not created yet or not
// processed yet. Once the internal Group is created and processed, the sync worker for internal group will
// re-enqueue the ClusterNetworkPolicy processing which will trigger the creation of AppliedToGroup.
return nil, nil
}
intGrp := ig.(*antreatypes.Group)
if len(intGrp.IPBlocks) > 0 || (intGrp.Selector != nil && intGrp.Selector.NamespaceSelector != nil) {
klog.V(2).InfoS("Group with IPBlocks or NamespaceSelector can not be used as AppliedTo", "Group", key)
return nil, ErrNetworkPolicyAppliedToUnsupportedGroup{namespace: namespace, groupName: groupName}
}
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)
}
77 changes: 0 additions & 77 deletions pkg/controller/networkpolicy/antreanetworkpolicy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package networkpolicy

import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -25,7 +24,6 @@ import (

"antrea.io/antrea/pkg/apis/controlplane"
crdv1alpha1 "antrea.io/antrea/pkg/apis/crd/v1alpha1"
crdv1alpha3 "antrea.io/antrea/pkg/apis/crd/v1alpha3"
antreatypes "antrea.io/antrea/pkg/controller/types"
)

Expand Down Expand Up @@ -620,81 +618,6 @@ func TestDeleteANP(t *testing.T) {
assert.False(t, done)
}

func TestProcessAppliedToGroupsForGroup(t *testing.T) {
selectorA := metav1.LabelSelector{MatchLabels: map[string]string{"foo1": "bar1"}}
cidr := "10.0.0.0/24"
// gA with selector present in cache
gA := crdv1alpha3.Group{
ObjectMeta: metav1.ObjectMeta{Namespace: "nsA", Name: "gA", UID: "uidA"},
Spec: crdv1alpha3.GroupSpec{
PodSelector: &selectorA,
},
}
// gB with IPBlock present in cache
gB := crdv1alpha3.Group{
ObjectMeta: metav1.ObjectMeta{Namespace: "nsB", Name: "gB", UID: "uidB"},
Spec: crdv1alpha3.GroupSpec{
IPBlocks: []crdv1alpha1.IPBlock{
{
CIDR: cidr,
},
},
},
}
// gC not found in cache
gC := crdv1alpha3.Group{
ObjectMeta: metav1.ObjectMeta{Namespace: "nsC", Name: "gC", UID: "uidC"},
Spec: crdv1alpha3.GroupSpec{
NamespaceSelector: &selectorA,
},
}
_, npc := newController()
npc.addGroup(&gA)
npc.addGroup(&gB)
npc.gStore.Add(&gA)
npc.gStore.Add(&gB)
tests := []struct {
name string
namespace string
inputG string
expectedAG *antreatypes.AppliedToGroup
expectedErr error
}{
{
name: "empty-grp-no-result",
namespace: "nsA",
inputG: "",
expectedAG: nil,
},
{
name: "ipblock-grp-no-result",
namespace: "nsB",
inputG: gB.Name,
expectedAG: nil,
expectedErr: ErrNetworkPolicyAppliedToUnsupportedGroup{namespace: "nsB", groupName: gB.Name},
},
{
name: "selector-grp-missing-no-result",
namespace: "nsC",
inputG: gC.Name,
expectedAG: nil,
},
{
name: "selector-grp",
namespace: "nsA",
inputG: gA.Name,
expectedAG: &antreatypes.AppliedToGroup{UID: gA.UID, Name: fmt.Sprintf("%s/%s", gA.Namespace, gA.Name)},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
actualAG, actualErr := npc.createAppliedToGroupForNamespacedGroup(tt.namespace, tt.inputG)
assert.Equal(t, tt.expectedAG, actualAG, "appliedToGroup list does not match")
assert.ErrorIs(t, actualErr, tt.expectedErr)
})
}
}

// util functions for testing.
func getANP() *crdv1alpha1.NetworkPolicy {
p10 := float64(10)
Expand Down
21 changes: 1 addition & 20 deletions pkg/controller/networkpolicy/clusternetworkpolicy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ func (n *NetworkPolicyController) processClusterAppliedTo(appliedTo []crdv1alpha
for _, at := range appliedTo {
var atg *antreatypes.AppliedToGroup
if at.Group != "" {
atg = n.createAppliedToGroupForCG(at.Group)
atg = n.createAppliedToGroupForGroup("", at.Group)
} else if at.Service != nil {
atg = n.createAppliedToGroupForService(at.Service)
} else if at.ServiceAccount != nil {
Expand Down Expand Up @@ -602,22 +602,3 @@ func (n *NetworkPolicyController) processRefGroupOrClusterGroup(g, namespace str
}
return nil, ipb
}

func (n *NetworkPolicyController) createAppliedToGroupForCG(clusterGroupName string) *antreatypes.AppliedToGroup {
// Find the internal Group corresponding to this ClusterGroup
// There is no need to check if the ClusterGroup exists in clusterGroupLister because its existence will eventually
// be reflected in internalGroupStore.
ig, found, _ := n.internalGroupStore.Get(clusterGroupName)
if !found {
// Internal Group was not found. Once the internal Group is created, the sync
// worker for internal group will re-enqueue the ClusterNetworkPolicy processing
// which will trigger the creation of AddressGroup.
return nil
}
intGrp := ig.(*antreatypes.Group)
if len(intGrp.IPBlocks) > 0 {
klog.V(2).InfoS("ClusterGroup with IPBlocks will not be processed as AppliedTo", "ClusterGroup", clusterGroupName)
return nil
}
return &antreatypes.AppliedToGroup{UID: intGrp.UID, Name: clusterGroupName}
}
67 changes: 0 additions & 67 deletions pkg/controller/networkpolicy/clusternetworkpolicy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1795,73 +1795,6 @@ func TestProcessRefGroupOrClusterGroup(t *testing.T) {
}
}

func TestProcessAppliedToGroupsForCGs(t *testing.T) {
selectorA := metav1.LabelSelector{MatchLabels: map[string]string{"foo1": "bar1"}}
cidr := "10.0.0.0/24"
// cgA with selector present in cache
cgA := crdv1alpha3.ClusterGroup{
ObjectMeta: metav1.ObjectMeta{Name: "cgA", UID: "uidA"},
Spec: crdv1alpha3.GroupSpec{
NamespaceSelector: &selectorA,
},
}
// cgB with IPBlock present in cache
cgB := crdv1alpha3.ClusterGroup{
ObjectMeta: metav1.ObjectMeta{Name: "cgB", UID: "uidB"},
Spec: crdv1alpha3.GroupSpec{
IPBlocks: []crdv1alpha1.IPBlock{
{
CIDR: cidr,
},
},
},
}
// cgC not found in cache
cgC := crdv1alpha3.ClusterGroup{
ObjectMeta: metav1.ObjectMeta{Name: "cgC", UID: "uidC"},
Spec: crdv1alpha3.GroupSpec{
NamespaceSelector: &selectorA,
},
}
_, npc := newController()
npc.addClusterGroup(&cgA)
npc.addClusterGroup(&cgB)
npc.cgStore.Add(&cgA)
npc.cgStore.Add(&cgB)
tests := []struct {
name string
inputCG string
expectedAG *antreatypes.AppliedToGroup
}{
{
name: "empty-cgs-no-result",
inputCG: "",
expectedAG: nil,
},
{
name: "ipblock-cgs-no-result",
inputCG: cgB.Name,
expectedAG: nil,
},
{
name: "selector-cgs-missing-no-result",
inputCG: cgC.Name,
expectedAG: nil,
},
{
name: "selector-cgs",
inputCG: cgA.Name,
expectedAG: &antreatypes.AppliedToGroup{UID: cgA.UID, Name: cgA.Name},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
actualAG := npc.createAppliedToGroupForCG(tt.inputCG)
assert.Equal(t, tt.expectedAG, actualAG, "appliedToGroup list does not match")
})
}
}

// util functions for testing.

func getCNP() *crdv1alpha1.ClusterNetworkPolicy {
Expand Down
29 changes: 29 additions & 0 deletions pkg/controller/networkpolicy/crd_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,35 @@ func (n *NetworkPolicyController) createAppliedToGroupForService(service *v1alph
return appliedToGroup
}

// createAppliedToGroupForGroup creates an AppliedToGroup object corresponding to a ClusterGroup or a Group.
// The namespace parameter is only provided when the group is namespace scoped.
func (n *NetworkPolicyController) createAppliedToGroupForGroup(namespace, group string) *antreatypes.AppliedToGroup {
// Cluster group uses NAME and Namespaced group uses NAMESPACE/NAME as the key of the corresponding internal group.
key := k8s.NamespacedName(namespace, group)
// Find the internal Group corresponding to this ClusterGroup/Group.
// There is no need to check if the ClusterGroup/Group exists in clusterGroupLister/groupLister because its
// existence will eventually be reflected in internalGroupStore.
ig, found, _ := n.internalGroupStore.Get(key)
if !found {
// Internal Group was not found. Once the internal Group is created, the sync worker for internal group will
// re-enqueue the ClusterNetworkPolicy/AntreaNetworkPolicy processing which will call this method again. So it's
// fine to ignore NotFound case.
return nil
}
intGrp := ig.(*antreatypes.Group)
// 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.
if len(intGrp.IPBlocks) > 0 {
klog.V(2).InfoS("Group with IPBlocks can not be used as AppliedTo", "Group", key)
return nil
}
return &antreatypes.AppliedToGroup{UID: intGrp.UID, Name: key}
}

// getTierPriority retrieves the priority associated with the input Tier name.
// If the Tier name is empty, by default, the lowest priority Application Tier
// is returned.
Expand Down
Loading

0 comments on commit f43d688

Please sign in to comment.