From db62f64116cf96255da5ad97c68536bcf2b8e07b Mon Sep 17 00:00:00 2001 From: wgrayson Date: Tue, 7 Dec 2021 14:29:21 -0800 Subject: [PATCH] Make Peer validation more comprehensive Signed-off-by: wgrayson --- pkg/controller/networkpolicy/validate.go | 23 ++++- test/e2e/antreapolicy_test.go | 110 +++++++++++++++++++++++ 2 files changed, 129 insertions(+), 4 deletions(-) diff --git a/pkg/controller/networkpolicy/validate.go b/pkg/controller/networkpolicy/validate.go index ec74a77d59d..2cd5e687c2f 100644 --- a/pkg/controller/networkpolicy/validate.go +++ b/pkg/controller/networkpolicy/validate.go @@ -17,6 +17,7 @@ package networkpolicy import ( "encoding/json" "fmt" + "reflect" "regexp" "strconv" "strings" @@ -461,6 +462,11 @@ func (v *antreaPolicyValidator) validateAppliedTo(ingress, egress []crdv1alpha1. if numAppliedToInRules > 0 && (numAppliedToInRules != len(ingress)+len(egress)) { return "appliedTo field should either be set in all rules or in none of them", false } + for _, eachSpecAppliedTo := range specAppliedTo { + if eachSpecAppliedTo.Group != "" && numFieldsSetInPeer(eachSpecAppliedTo) > 1 { + return "group cannot be set with other peers in appliedTo", false + } + } return "", true } @@ -472,10 +478,7 @@ func (v *antreaPolicyValidator) validatePeers(ingress, egress []crdv1alpha1.Rule if peer.NamespaceSelector != nil && peer.Namespaces != nil { return "namespaces and namespaceSelector cannot be set at the same time for a single NetworkPolicyPeer", false } - if peer.Group == "" { - continue - } - if peer.PodSelector != nil || peer.IPBlock != nil || peer.NamespaceSelector != nil { + if peer.Group != "" && numFieldsSetInPeer(peer) > 1 { return "group cannot be set with other peers in rules", false } } @@ -504,6 +507,18 @@ func (v *antreaPolicyValidator) validatePeers(ingress, egress []crdv1alpha1.Rule return "", true } +// numFieldsSetInPeer returns the number of fields in use of a peer. +func numFieldsSetInPeer(peer crdv1alpha1.NetworkPolicyPeer) int { + num := 0 + v := reflect.ValueOf(peer) + for i := 0; i < v.NumField(); i++ { + if !v.Field(i).IsZero() { + num++ + } + } + return num +} + // validateTierForPolicy validates whether a referenced Tier exists. func (v *antreaPolicyValidator) validateTierForPolicy(tier string) (string, bool) { // "tier" must exist before referencing diff --git a/test/e2e/antreapolicy_test.go b/test/e2e/antreapolicy_test.go index 22e4ca4ac25..d69cd2212c4 100644 --- a/test/e2e/antreapolicy_test.go +++ b/test/e2e/antreapolicy_test.go @@ -427,6 +427,46 @@ func testInvalidACNPAppliedToNotSetInAllRules(t *testing.T) { } } +func testInvalidACNPAppliedToGroupSetWithPodSelector(t *testing.T) { + invalidNpErr := fmt.Errorf("invalid Antrea ClusterNetworkPolicy with appliedTo set group with podSelector") + appTo := ACNPAppliedToSpec{ + PodSelector: map[string]string{"pod": "b"}, + Group: "cgA", + } + builder := &ClusterNetworkPolicySpecBuilder{} + builder = builder.SetName("acnp-appto-set-group-with-psel"). + SetPriority(1.0). + SetAppliedToGroup([]ACNPAppliedToSpec{appTo}) + builder = builder.AddIngress(v1.ProtocolTCP, &p80, nil, nil, nil, map[string]string{"pod": "b"}, map[string]string{"ns": "x"}, + nil, nil, false, nil, crdv1alpha1.RuleActionAllow, "", "") + acnp := builder.Get() + log.Debugf("creating ACNP %v", acnp.Name) + if _, err := k8sUtils.CreateOrUpdateACNP(acnp); err == nil { + // Above creation of ACNP must fail as it is an invalid spec. + failOnError(invalidNpErr, t) + } +} + +func testInvalidACNPAppliedToGroupSetWithNSSelector(t *testing.T) { + invalidNpErr := fmt.Errorf("invalid Antrea ClusterNetworkPolicy with appliedTo set group with nsSelector") + appTo := ACNPAppliedToSpec{ + NSSelector: map[string]string{"ns": "x"}, + Group: "cgA", + } + builder := &ClusterNetworkPolicySpecBuilder{} + builder = builder.SetName("acnp-appto-set-group-with-nssel"). + SetPriority(1.0). + SetAppliedToGroup([]ACNPAppliedToSpec{appTo}) + builder = builder.AddIngress(v1.ProtocolTCP, &p80, nil, nil, nil, map[string]string{"pod": "b"}, map[string]string{"ns": "x"}, + nil, nil, false, nil, crdv1alpha1.RuleActionAllow, "", "") + acnp := builder.Get() + log.Debugf("creating ACNP %v", acnp.Name) + if _, err := k8sUtils.CreateOrUpdateACNP(acnp); err == nil { + // Above creation of ACNP must fail as it is an invalid spec. + failOnError(invalidNpErr, t) + } +} + func testInvalidACNPIngressPeerCGSetWithPodSelector(t *testing.T) { cgA := "cgA" selectorA := metav1.LabelSelector{MatchLabels: map[string]string{"foo1": "bar1"}} @@ -489,6 +529,71 @@ func testInvalidACNPIngressPeerCGSetWithIPBlock(t *testing.T) { } } +func testInvalidACNPEgressPeerCGSetWithFQDN(t *testing.T) { + cgA := "cgA" + selectorA := metav1.LabelSelector{MatchLabels: map[string]string{"foo1": "bar1"}} + k8sUtils.CreateCG(cgA, &selectorA, nil, nil) + invalidNpErr := fmt.Errorf("invalid Antrea ClusterNetworkPolicy with group and fqdn in NetworkPolicyPeer set") + builder := &ClusterNetworkPolicySpecBuilder{} + builder = builder.SetName("acnp-egress-group-fqdn-set"). + SetPriority(1.0). + SetAppliedToGroup([]ACNPAppliedToSpec{{PodSelector: map[string]string{"pod": "b"}}}) + builder = builder.AddEgress(v1.ProtocolTCP, &p80, nil, nil, nil, nil, nil, + nil, nil, false, nil, crdv1alpha1.RuleActionAllow, "cgA", "") + builder.Spec.Egress[0].To[0].FQDN = "google.com" + + acnp := builder.Get() + log.Debugf("creating ACNP %v", acnp.Name) + if _, err := k8sUtils.CreateOrUpdateACNP(acnp); err == nil { + // Above creation of ACNP must fail as it is an invalid spec. + failOnError(invalidNpErr, t) + } + failOnError(k8sUtils.CleanCGs(), t) +} + +func testInvalidACNPIngressPeerCGSetWithNamespace(t *testing.T) { + cgA := "cgA" + selectorA := metav1.LabelSelector{MatchLabels: map[string]string{"foo1": "bar1"}} + k8sUtils.CreateCG(cgA, &selectorA, nil, nil) + invalidNpErr := fmt.Errorf("invalid Antrea ClusterNetworkPolicy with group and namespace in NetworkPolicyPeer set") + builder := &ClusterNetworkPolicySpecBuilder{} + builder = builder.SetName("acnp-ingress-group-ns-set"). + SetPriority(1.0). + SetAppliedToGroup([]ACNPAppliedToSpec{{PodSelector: map[string]string{"pod": "b"}}}) + builder = builder.AddIngress(v1.ProtocolTCP, &p80, nil, nil, nil, nil, nil, + nil, nil, true, nil, crdv1alpha1.RuleActionAllow, "cgA", "") + acnp := builder.Get() + log.Debugf("creating ACNP %v", acnp.Name) + if _, err := k8sUtils.CreateOrUpdateACNP(acnp); err == nil { + // Above creation of ACNP must fail as it is an invalid spec. + failOnError(invalidNpErr, t) + } + failOnError(k8sUtils.CleanCGs(), t) +} + +func testInvalidACNPIngressPeerCGSetWithEESelector(t *testing.T) { + cgA := "cgA" + selectorA := metav1.LabelSelector{MatchLabels: map[string]string{"foo1": "bar1"}} + k8sUtils.CreateCG(cgA, &selectorA, nil, nil) + invalidNpErr := fmt.Errorf("invalid Antrea ClusterNetworkPolicy with group and external entity selector in NetworkPolicyPeer set") + builder := &ClusterNetworkPolicySpecBuilder{} + builder = builder.SetName("acnp-ingress-group-eeselector-set"). + SetPriority(1.0). + SetAppliedToGroup([]ACNPAppliedToSpec{{PodSelector: map[string]string{"pod": "b"}}}) + builder = builder.AddIngress(v1.ProtocolTCP, &p80, nil, nil, nil, nil, nil, + nil, nil, true, nil, crdv1alpha1.RuleActionAllow, "cgA", "") + builder.Spec.Ingress[0].From[0].ExternalEntitySelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{"foo2": "bar2"}, + } + acnp := builder.Get() + log.Debugf("creating ACNP %v", acnp.Name) + if _, err := k8sUtils.CreateOrUpdateACNP(acnp); err == nil { + // Above creation of ACNP must fail as it is an invalid spec. + failOnError(invalidNpErr, t) + } + failOnError(k8sUtils.CleanCGs(), t) +} + func testInvalidACNPIngressPeerNamespacesSetWithNSSelector(t *testing.T) { invalidNpErr := fmt.Errorf("invalid Antrea ClusterNetworkPolicy with namespaces and namespaceSelector in NetworkPolicyPeer set") builder := &ClusterNetworkPolicySpecBuilder{} @@ -2989,9 +3094,14 @@ func TestAntreaPolicy(t *testing.T) { t.Run("Case=ACNPIngressPeerCGSetWithIPBlock", func(t *testing.T) { testInvalidACNPIngressPeerCGSetWithIPBlock(t) }) t.Run("Case=ACNPIngressPeerCGSetWithPodSelector", func(t *testing.T) { testInvalidACNPIngressPeerCGSetWithPodSelector(t) }) t.Run("Case=ACNPIngressPeerCGSetWithNSSelector", func(t *testing.T) { testInvalidACNPIngressPeerCGSetWithNSSelector(t) }) + t.Run("Case=ACNPEgressPeerCGSetWithFQDN", func(t *testing.T) { testInvalidACNPEgressPeerCGSetWithFQDN(t) }) + t.Run("Case=ACNPIngressPeerCGSetWithNamespace", func(t *testing.T) { testInvalidACNPIngressPeerCGSetWithNamespace(t) }) + t.Run("Case=ACNPIngressPeerCGSetWithEESelector", func(t *testing.T) { testInvalidACNPIngressPeerCGSetWithEESelector(t) }) t.Run("Case=ACNPIngressPeerNamespaceSetWithNSSelector", func(t *testing.T) { testInvalidACNPIngressPeerNamespacesSetWithNSSelector(t) }) t.Run("Case=ACNPSpecAppliedToRuleAppliedToSet", func(t *testing.T) { testInvalidACNPSpecAppliedToRuleAppliedToSet(t) }) t.Run("Case=ACNPAppliedToNotSetInAllRules", func(t *testing.T) { testInvalidACNPAppliedToNotSetInAllRules(t) }) + t.Run("Case=ACNPAppliedToGroupSetWithPodSelector", func(t *testing.T) { testInvalidACNPAppliedToGroupSetWithPodSelector(t) }) + t.Run("Case=ACNPAppliedToGroupSetWithNSSelector", func(t *testing.T) { testInvalidACNPAppliedToGroupSetWithNSSelector(t) }) t.Run("Case=ANPNoPriority", func(t *testing.T) { testInvalidANPNoPriority(t) }) t.Run("Case=ANPRuleNameNotUniqueDenied", func(t *testing.T) { testInvalidANPRuleNameNotUnique(t) }) t.Run("Case=ANPTierDoesNotExistDenied", func(t *testing.T) { testInvalidANPTierDoesNotExist(t) })