diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index 6d9aca7ced..ded4ffcc47 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -1,6 +1,8 @@ package translation import ( + "fmt" + "github.com/Azure/azure-container-networking/log" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" "github.com/Azure/azure-container-networking/npm/util" @@ -230,7 +232,7 @@ func parseNSSelector(selector *metav1.LabelSelector) []labelSelector { // parsePodSelector parses podSelector and returns slice of labelSelector object // which includes operator, setType, ipset name and its members slice. // Members slice exists only if setType is only NestedLabelOfPod. -func parsePodSelector(selector *metav1.LabelSelector) ([]labelSelector, error) { +func parsePodSelector(policyKey string, selector *metav1.LabelSelector) ([]labelSelector, error) { parsedSelectors := newParsedSelectors() // #1. MatchLabels @@ -257,7 +259,8 @@ func parsePodSelector(selector *metav1.LabelSelector) ([]labelSelector, error) { setType = ipsets.KeyValueLabelOfPod } else { // "(!) + matchKey + : + multiple matchVals" case - setName = req.Key + // see caveat in definition of TranslatedIPSet for why the policy key must be included in the set name + setName = fmt.Sprintf("%s-%s", policyKey, req.Key) for _, val := range req.Values { setName = util.GetIpSetFromLabelKV(setName, val) members = append(members, util.GetIpSetFromLabelKV(req.Key, val)) diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index f04f7da58d..49c2707cfe 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -216,8 +216,8 @@ func ipBlockRule(policyName, ns string, direction policies.Direction, matchType // PodSelector translates podSelector of NetworkPolicyPeer field in networkpolicy object to translatedIPSet and SetInfo. // This function is called only when the NetworkPolicyPeer has namespaceSelector field. -func podSelector(matchType policies.MatchType, selector *metav1.LabelSelector) ([]*ipsets.TranslatedIPSet, []policies.SetInfo, error) { - podSelectors, err := parsePodSelector(selector) +func podSelector(policyKey string, matchType policies.MatchType, selector *metav1.LabelSelector) ([]*ipsets.TranslatedIPSet, []policies.SetInfo, error) { + podSelectors, err := parsePodSelector(policyKey, selector) if err != nil { return nil, nil, err } @@ -241,8 +241,8 @@ func podSelector(matchType policies.MatchType, selector *metav1.LabelSelector) ( // podSelectorWithNS translates podSelector of spec and NetworkPolicyPeer in networkpolicy object to translatedIPSet and SetInfo. // This function is called only when the NetworkPolicyPeer does not have namespaceSelector field. -func podSelectorWithNS(ns string, matchType policies.MatchType, selector *metav1.LabelSelector) ([]*ipsets.TranslatedIPSet, []policies.SetInfo, error) { - podSelectorIPSets, podSelectorList, err := podSelector(matchType, selector) +func podSelectorWithNS(policyKey, ns string, matchType policies.MatchType, selector *metav1.LabelSelector) ([]*ipsets.TranslatedIPSet, []policies.SetInfo, error) { + podSelectorIPSets, podSelectorList, err := podSelector(policyKey, matchType, selector) if err != nil { return nil, nil, err } @@ -397,7 +397,7 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire // #2.3 handle podSelector and port if exist if peer.PodSelector != nil && peer.NamespaceSelector == nil { - podSelectorIPSets, podSelectorList, err := podSelectorWithNS(npmNetPol.Namespace, matchType, peer.PodSelector) + podSelectorIPSets, podSelectorList, err := podSelectorWithNS(npmNetPol.PolicyKey, npmNetPol.Namespace, matchType, peer.PodSelector) if err != nil { return err } @@ -418,7 +418,7 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire } // #2.4 handle namespaceSelector and podSelector and port if exist - podSelectorIPSets, podSelectorList, err := podSelector(matchType, peer.PodSelector) + podSelectorIPSets, podSelectorList, err := podSelector(npmNetPol.PolicyKey, matchType, peer.PodSelector) if err != nil { return err } @@ -542,7 +542,7 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy) (*policies.NPMNetworkPol // podSelector in spec.PodSelector is common for ingress and egress. // Process this podSelector first. var err error - npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.Namespace, policies.EitherMatch, &npObj.Spec.PodSelector) + npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.PolicyKey, npmNetPol.Namespace, policies.EitherMatch, &npObj.Spec.PodSelector) if err != nil { return nil, err } diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index 21a396c55d..c096addad1 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -536,6 +536,8 @@ func TestIPBlockRule(t *testing.T) { func TestPodSelector(t *testing.T) { matchType := policies.DstMatch + policyKey := "test-ns/test-policy" + policyKeyWithDash := policyKey + "-" tests := []struct { name string namespace string @@ -687,14 +689,14 @@ func TestPodSelector(t *testing.T) { }, podSelectorIPSets: []*ipsets.TranslatedIPSet{ ipsets.NewTranslatedIPSet("k0:v0", ipsets.KeyValueLabelOfPod), - ipsets.NewTranslatedIPSet("k1:v10:v11", ipsets.NestedLabelOfPod, []string{"k1:v10", "k1:v11"}...), + ipsets.NewTranslatedIPSet(policyKeyWithDash+"k1:v10:v11", ipsets.NestedLabelOfPod, []string{"k1:v10", "k1:v11"}...), ipsets.NewTranslatedIPSet("k1:v10", ipsets.KeyValueLabelOfPod), ipsets.NewTranslatedIPSet("k1:v11", ipsets.KeyValueLabelOfPod), ipsets.NewTranslatedIPSet("k2", ipsets.KeyLabelOfPod), }, podSelectorList: []policies.SetInfo{ policies.NewSetInfo("k0:v0", ipsets.KeyValueLabelOfPod, included, matchType), - policies.NewSetInfo("k1:v10:v11", ipsets.NestedLabelOfPod, included, matchType), + policies.NewSetInfo(policyKeyWithDash+"k1:v10:v11", ipsets.NestedLabelOfPod, included, matchType), policies.NewSetInfo("k2", ipsets.KeyLabelOfPod, nonIncluded, matchType), }, }, @@ -708,9 +710,10 @@ func TestPodSelector(t *testing.T) { var podSelectorList []policies.SetInfo var err error if tt.namespace == "" { - podSelectorIPSets, podSelectorList, err = podSelector(tt.matchType, tt.labelSelector) + podSelectorIPSets, podSelectorList, err = podSelector(policyKey, tt.matchType, tt.labelSelector) } else { - podSelectorIPSets, podSelectorList, err = podSelectorWithNS(tt.namespace, tt.matchType, tt.labelSelector) + // technically, the policyKey prefix would contain the namespace, but it might not for these tests + podSelectorIPSets, podSelectorList, err = podSelectorWithNS(policyKey, tt.namespace, tt.matchType, tt.labelSelector) } require.NoError(t, err) require.Equal(t, tt.podSelectorIPSets, podSelectorIPSets) @@ -1668,7 +1671,7 @@ func TestIngressPolicy(t *testing.T) { ACLPolicyID: tt.npmNetPol.ACLPolicyID, } var err error - npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.Namespace, policies.EitherMatch, tt.targetSelector) + npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.PolicyKey, npmNetPol.Namespace, policies.EitherMatch, tt.targetSelector) require.NoError(t, err) splitPolicyKey := strings.Split(npmNetPol.PolicyKey, "/") require.Len(t, splitPolicyKey, 2, "policy key must include name") @@ -2060,7 +2063,7 @@ func TestEgressPolicy(t *testing.T) { ACLPolicyID: tt.npmNetPol.ACLPolicyID, } var err error - npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.Namespace, policies.EitherMatch, tt.targetSelector) + npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.PolicyKey, npmNetPol.Namespace, policies.EitherMatch, tt.targetSelector) require.NoError(t, err) splitPolicyKey := strings.Split(npmNetPol.PolicyKey, "/") require.Len(t, splitPolicyKey, 2, "policy key must include name") diff --git a/npm/pkg/dataplane/dataplane.go b/npm/pkg/dataplane/dataplane.go index f78b91d5e8..9bad183f3d 100644 --- a/npm/pkg/dataplane/dataplane.go +++ b/npm/pkg/dataplane/dataplane.go @@ -41,11 +41,12 @@ type DataPlane struct { stopChannel <-chan struct{} } +// TODO this struct could be made unexported type NPMEndpoint struct { - Name string - ID string - IP string - // TODO: check it may use PolicyKey instead of Policy name + Name string + ID string + IP string + PodKey string // Map with Key as Network Policy name to to emulate set // and value as struct{} for minimal memory consumption NetPolReference map[string]struct{} @@ -73,7 +74,6 @@ func NewDataPlane(nodeName string, ioShim *common.IOShim, cfg *Config, stopChann } // BootupDataplane cleans the NPM sets and policies in the dataplane and performs initialization. -// TODO rename this function to BootupDataplane func (dp *DataPlane) BootupDataplane() error { // NOTE: used to create an all-namespaces set, but there's no need since it will be created by the control plane return dp.bootupDataPlane() //nolint:wrapcheck // unnecessary to wrap error @@ -225,7 +225,6 @@ func (dp *DataPlane) AddPolicy(policy *policies.NPMNetworkPolicy) error { if err != nil { return fmt.Errorf("[DataPlane] error while applying dataplane: %w", err) } - // TODO calculate endpoints to apply policy on endpointList, err := dp.getEndpointsToApplyPolicy(policy) if err != nil { return err @@ -321,8 +320,6 @@ func (dp *DataPlane) createIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n } } - // TODO is there a possibility for a list set of selector referencing rule ipset? - // if so this below addition would throw an error because rule ipsets are not created // Check if any list sets are provided with members to add for _, set := range sets { // Check if any CIDR block IPSets needs to be applied @@ -362,11 +359,9 @@ func (dp *DataPlane) deleteIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n } } - // Check if any list sets are provided with members to add - // TODO for nested IPsets check if we are safe to remove members - // if k1:v0:v1 is created by two network policies - // and both have same members - // then we should not delete k1:v0:v1 members ( special case for nested ipsets ) + // Check if any list sets are provided with members to delete + // NOTE: every translated member will be deleted, even if the member is part of the same set in another policy + // see the definition of TranslatedIPSet for how to avoid this situation for _, set := range sets { // Check if any CIDR block IPSets needs to be applied setType := set.Metadata.Type diff --git a/npm/pkg/dataplane/ipsets/ipset.go b/npm/pkg/dataplane/ipsets/ipset.go index be604e70c9..b87b21d689 100644 --- a/npm/pkg/dataplane/ipsets/ipset.go +++ b/npm/pkg/dataplane/ipsets/ipset.go @@ -105,6 +105,10 @@ func (setType SetType) getSetKind() SetKind { // 2. NestedLabelOfPod IPSet from multi value labels // Members field holds member ipset names for NestedLabelOfPod and ip address ranges // for CIDRBlocks IPSet +// Caveat: if a list set with translated members is referenced in multiple policies, +// then it must have a different ipset name for each policy. Otherwise, deleting the policy +// will result in removing the translated members from the set even if another policy requires +// those members. See dataplane.go for more details. type TranslatedIPSet struct { Metadata *IPSetMetadata // Members holds member ipset names for NestedLabelOfPod and ip address ranges