diff --git a/pkg/controller/networkpolicy/validate.go b/pkg/controller/networkpolicy/validate.go index 6cd93fb4a67..c700c0828e2 100644 --- a/pkg/controller/networkpolicy/validate.go +++ b/pkg/controller/networkpolicy/validate.go @@ -30,7 +30,6 @@ import ( crdv1alpha1 "antrea.io/antrea/pkg/apis/crd/v1alpha1" crdv1alpha2 "antrea.io/antrea/pkg/apis/crd/v1alpha2" "antrea.io/antrea/pkg/controller/networkpolicy/store" - "antrea.io/antrea/pkg/controller/types" "antrea.io/antrea/pkg/util/env" ) @@ -664,22 +663,21 @@ func validateAntreaGroupSpec(s crdv1alpha2.GroupSpec) (string, bool) { return "", true } -func (g *groupValidator) validateChildGroup(s *crdv1alpha2.ClusterGroup, isUpdate bool) (string, bool) { +func (g *groupValidator) validateChildGroup(s *crdv1alpha2.ClusterGroup) (string, bool) { if len(s.Spec.ChildGroups) > 0 { - if isUpdate { - parentGrps, err := g.networkPolicyController.internalGroupStore.GetByIndex(store.ChildGroupIndex, s.Name) - if err != nil { - return fmt.Sprintf("error retrieving parents of ClusterGroup %s: %v", s.Name, err), false - } - // TODO: relax this constraint when max group nesting level increases. - if len(parentGrps) > 0 { - return fmt.Sprintf("cannot set childGroups for ClusterGroup %s, who has %d parents", s.Name, len(parentGrps)), false - } + parentGrps, err := g.networkPolicyController.internalGroupStore.GetByIndex(store.ChildGroupIndex, s.Name) + if err != nil { + return fmt.Sprintf("error retrieving parents of ClusterGroup %s: %v", s.Name, err), false + } + // TODO: relax this constraint when max group nesting level increases. + if len(parentGrps) > 0 { + return fmt.Sprintf("cannot set childGroups for ClusterGroup %s, who has %d parents", s.Name, len(parentGrps)), false } for _, groupname := range s.Spec.ChildGroups { cg, err := g.networkPolicyController.cgLister.Get(string(groupname)) if err != nil { - return fmt.Sprintf("child group %s does not exist", string(groupname)), false + // the childGroup has not been created yet. + continue } // TODO: relax this constraint when max group nesting level increases. if len(cg.Spec.ChildGroups) > 0 { @@ -697,7 +695,7 @@ func (g *groupValidator) createValidate(curObj interface{}, userInfo authenticat if !allowed { return reason, allowed } - return g.validateChildGroup(curCG, false) + return g.validateChildGroup(curCG) } // updateValidate validates the UPDATE events of ClusterGroup resources. @@ -707,7 +705,7 @@ func (g *groupValidator) updateValidate(curObj, oldObj interface{}, userInfo aut if !allowed { return reason, allowed } - return g.validateChildGroup(curCG, true) + return g.validateChildGroup(curCG) } // deleteValidate validates the DELETE events of ClusterGroup resources. @@ -726,18 +724,5 @@ func (g *groupValidator) deleteValidate(oldObj interface{}, userInfo authenticat } return fmt.Sprintf("ClusterGroup %s is referenced by %d Antrea ClusterNetworkPolicies: %v", oldCG.Name, len(cnps), cnpNameList), false } - // ClusterGroup referenced by other ClusterGroups as childGroup cannot be deleted. - parentGrps, err := g.networkPolicyController.internalGroupStore.GetByIndex(store.ChildGroupIndex, oldCG.Name) - if err != nil { - return fmt.Sprintf("error retrieving parents of ClusterGroup %s: %v", oldCG.Name, err), false - } - if len(parentGrps) > 0 { - parentGrpNameList := make([]string, len(parentGrps)) - for i := range parentGrps { - grpObject := parentGrps[i].(*types.Group) - parentGrpNameList[i] = grpObject.Name - } - return fmt.Sprintf("ClusterGroup %s is referenced by %d ClusterGroups as childGroup: %v", oldCG.Name, len(parentGrps), parentGrpNameList), false - } return "", true } diff --git a/test/e2e/antreapolicy_test.go b/test/e2e/antreapolicy_test.go index fc24ab5bb96..f7bf1617f18 100644 --- a/test/e2e/antreapolicy_test.go +++ b/test/e2e/antreapolicy_test.go @@ -2246,16 +2246,20 @@ func testACNPClusterGroupServiceRefCreateAndUpdate(t *testing.T, data *TestData) func testACNPNestedClusterGroupCreateAndUpdate(t *testing.T, data *TestData) { svc1 := k8sUtils.BuildService("svc1", "x", 80, 80, map[string]string{"app": "a"}, nil) - cg1Name, cg2Name := "cg-svc-x-a", "cg-select-y-b" + cg1Name, cg2Name, cg3Name := "cg-svc-x-a", "cg-select-y-b", "cg-select-y-c" cgBuilder1 := &ClusterGroupV1Alpha3SpecBuilder{} cgBuilder1 = cgBuilder1.SetName(cg1Name).SetServiceReference("x", "svc1") cgBuilder2 := &ClusterGroupV1Alpha3SpecBuilder{} cgBuilder2 = cgBuilder2.SetName(cg2Name). SetNamespaceSelector(map[string]string{"ns": "y"}, nil). SetPodSelector(map[string]string{"pod": "b"}, nil) + cgBuilder3 := &ClusterGroupV1Alpha3SpecBuilder{} + cgBuilder3 = cgBuilder3.SetName(cg3Name). + SetNamespaceSelector(map[string]string{"ns": "y"}, nil). + SetPodSelector(map[string]string{"pod": "c"}, nil) cgNestedName := "cg-nested" cgBuilderNested := &ClusterGroupV1Alpha3SpecBuilder{} - cgBuilderNested = cgBuilderNested.SetName(cgNestedName).SetChildGroups([]string{cg1Name}) + cgBuilderNested = cgBuilderNested.SetName(cgNestedName).SetChildGroups([]string{cg1Name, cg3Name}) builder := &ClusterNetworkPolicySpecBuilder{} builder = builder.SetName("cnp-nested-cg").SetPriority(1.0). @@ -2263,11 +2267,11 @@ func testACNPNestedClusterGroupCreateAndUpdate(t *testing.T, data *TestData) { AddIngress(v1.ProtocolTCP, &p80, nil, nil, nil, nil, nil, nil, nil, false, nil, crdv1alpha1.RuleActionDrop, cgNestedName, "") - // Pods in Namespace z should not allow ingress from Pods backing svc1 (label pod=a) in Namespace x. + // Pods in Namespace z should not allow traffic from Pods backing svc1 (label pod=a) in Namespace x. + // Note that in this testStep cg3 will not be created yet, so even though cg-nested selects cg1 and + // cg3 as childGroups, only members of cg1 will be included as this time. reachability := NewReachability(allPods, Connected) - reachability.Expect(Pod("x/a"), Pod("z/a"), Dropped) - reachability.Expect(Pod("x/a"), Pod("z/b"), Dropped) - reachability.Expect(Pod("x/a"), Pod("z/c"), Dropped) + reachability.ExpectEgressToNamespace("x/a", "z", Dropped) testStep1 := &TestStep{ "Port 80", @@ -2281,15 +2285,11 @@ func testACNPNestedClusterGroupCreateAndUpdate(t *testing.T, data *TestData) { } // Test update "cg-nested" to include "cg-select-y-b" as well. - cgBuilderNested = cgBuilderNested.SetChildGroups([]string{cg1Name, cg2Name}) + cgBuilderNested = cgBuilderNested.SetChildGroups([]string{cg1Name, cg2Name, cg3Name}) // In addition to x/a, all traffic from y/b to Namespace z should also be denied. reachability2 := NewReachability(allPods, Connected) - reachability2.Expect(Pod("x/a"), Pod("z/a"), Dropped) - reachability2.Expect(Pod("x/a"), Pod("z/b"), Dropped) - reachability2.Expect(Pod("x/a"), Pod("z/c"), Dropped) - reachability2.Expect(Pod("y/b"), Pod("z/a"), Dropped) - reachability2.Expect(Pod("y/b"), Pod("z/b"), Dropped) - reachability2.Expect(Pod("y/b"), Pod("z/c"), Dropped) + reachability2.ExpectEgressToNamespace("x/a", "z", Dropped) + reachability2.ExpectEgressToNamespace("y/b", "z", Dropped) // New member in cg-svc-x-a should be reflected in cg-nested as well. cp := []*CustomProbe{ { @@ -2316,7 +2316,24 @@ func testACNPNestedClusterGroupCreateAndUpdate(t *testing.T, data *TestData) { cp, } - testSteps := []*TestStep{testStep1, testStep2} + // In this testStep cg3 is created. It's members should reflect in cg-nested + // and as a result, all traffic from y/c to Namespace z should be denied as well. + reachability3 := NewReachability(allPods, Connected) + reachability3.ExpectEgressToNamespace("x/a", "z", Dropped) + reachability3.ExpectEgressToNamespace("y/b", "z", Dropped) + reachability3.ExpectEgressToNamespace("y/c", "z", Dropped) + testStep3 := &TestStep{ + "Port 80 updated", + reachability3, + nil, + []metav1.Object{cgBuilder3.Get()}, + []int32{80}, + v1.ProtocolTCP, + 0, + nil, + } + + testSteps := []*TestStep{testStep1, testStep2, testStep3} testCase := []*TestCase{ {"ACNP nested ClusterGroup create and update", testSteps}, } diff --git a/test/e2e/clustergroup_test.go b/test/e2e/clustergroup_test.go index 9d512e6d109..bc3e61dbc43 100644 --- a/test/e2e/clustergroup_test.go +++ b/test/e2e/clustergroup_test.go @@ -160,23 +160,6 @@ func testInvalidCGServiceRefWithIPBlock(t *testing.T) { } } -func testInvalidCGChildGroupDoesNotExist(t *testing.T) { - invalidErr := fmt.Errorf("clustergroup childGroup does not exist") - cgName := "child-group-not-exist" - cg := &crdv1alpha3.ClusterGroup{ - ObjectMeta: metav1.ObjectMeta{ - Name: cgName, - }, - Spec: crdv1alpha3.GroupSpec{ - ChildGroups: []crdv1alpha3.ClusterGroupReference{crdv1alpha3.ClusterGroupReference("some-non-existing-cg")}, - }, - } - if _, err := k8sUtils.CreateOrUpdateV1Alpha3CG(cg); err == nil { - // Above creation of CG must fail as it is an invalid spec. - failOnError(invalidErr, t) - } -} - var testChildCGName = "test-child-cg" func createChildCGForTest(t *testing.T) { @@ -249,24 +232,40 @@ func testInvalidCGMaxNestedLevel(t *testing.T) { ChildGroups: []crdv1alpha3.ClusterGroupReference{crdv1alpha3.ClusterGroupReference(testChildCGName)}, }, } - if _, err := k8sUtils.CreateOrUpdateV1Alpha3CG(cg1); err != nil { - // Above creation of CG must succeed as it is a valid spec. - failOnError(err, t) - } cg2 := &crdv1alpha3.ClusterGroup{ ObjectMeta: metav1.ObjectMeta{Name: cgName2}, Spec: crdv1alpha3.GroupSpec{ ChildGroups: []crdv1alpha3.ClusterGroupReference{crdv1alpha3.ClusterGroupReference(cgName1)}, }, } + // Try to create cg-nested-1 first and then cg-nested-2. + // The creation of cg-nested-2 should fail as it breaks the max nested level + if _, err := k8sUtils.CreateOrUpdateV1Alpha3CG(cg1); err != nil { + // Above creation of CG must succeed as it is a valid spec. + failOnError(err, t) + } if _, err := k8sUtils.CreateOrUpdateV1Alpha3CG(cg2); err == nil { - // Above creation of CG must fail as it is an invalid spec. + // Above creation of CG must fail as cg-nested-2 cannot have cg-nested-1 as childGroup. failOnError(invalidErr, t) } // cleanup cg-nested-1 if err := k8sUtils.DeleteV1Alpha3CG(cgName1); err != nil { failOnError(err, t) } + // Try to create cg-nested-2 first and then cg-nested-1. + // The creation of cg-nested-1 should fail as it breaks the max nested level + if _, err := k8sUtils.CreateOrUpdateV1Alpha3CG(cg2); err != nil { + // Above creation of CG must succeed as it is a valid spec. + failOnError(err, t) + } + if _, err := k8sUtils.CreateOrUpdateV1Alpha3CG(cg1); err == nil { + // Above creation of CG must fail as cg-nested-2 cannot have cg-nested-1 as childGroup. + failOnError(invalidErr, t) + } + // cleanup cg-nested-2 + if err := k8sUtils.DeleteV1Alpha3CG(cgName2); err != nil { + failOnError(err, t) + } } func testClusterGroupConversionV1A2AndV1A3(t *testing.T) { @@ -332,7 +331,6 @@ func TestClusterGroup(t *testing.T) { t.Run("Case=ServiceRefWithPodSelectorDenied", func(t *testing.T) { testInvalidCGServiceRefWithPodSelector(t) }) t.Run("Case=ServiceRefWithNamespaceSelectorDenied", func(t *testing.T) { testInvalidCGServiceRefWithNSSelector(t) }) t.Run("Case=ServiceRefWithIPBlockDenied", func(t *testing.T) { testInvalidCGServiceRefWithIPBlock(t) }) - t.Run("Case=InvalidChildGroupName", func(t *testing.T) { testInvalidCGChildGroupDoesNotExist(t) }) }) t.Run("TestGroupClusterGroupValidateChildGroup", func(t *testing.T) { createChildCGForTest(t) diff --git a/test/e2e/legacyclustergroup_test.go b/test/e2e/legacyclustergroup_test.go index f31e0ae76be..598ea4efb7e 100644 --- a/test/e2e/legacyclustergroup_test.go +++ b/test/e2e/legacyclustergroup_test.go @@ -137,23 +137,6 @@ func testLegacyInvalidCGServiceRefWithIPBlock(t *testing.T) { } } -func testLegacyInvalidCGChildGroupDoesNotExist(t *testing.T) { - invalidErr := fmt.Errorf("clustergroup childGroup does not exist") - cgName := "child-group-not-exist" - cg := &legacycorev1alpha2.ClusterGroup{ - ObjectMeta: metav1.ObjectMeta{ - Name: cgName, - }, - Spec: crdv1alpha2.GroupSpec{ - ChildGroups: []crdv1alpha2.ClusterGroupReference{crdv1alpha2.ClusterGroupReference("some-non-existing-cg")}, - }, - } - if _, err := k8sUtils.CreateOrUpdateLegacyCG(cg); err == nil { - // Above creation of CG must fail as it is an invalid spec. - failOnError(invalidErr, t) - } -} - func createLegacyChildCGForTest(t *testing.T) { cg := &legacycorev1alpha2.ClusterGroup{ ObjectMeta: metav1.ObjectMeta{ @@ -261,7 +244,6 @@ func TestLegacyClusterGroup(t *testing.T) { t.Run("Case=LegacyServiceRefWithPodSelectorDenied", func(t *testing.T) { testLegacyInvalidCGServiceRefWithPodSelector(t) }) t.Run("Case=LegacyServiceRefWithNamespaceSelectorDenied", func(t *testing.T) { testLegacyInvalidCGServiceRefWithNSSelector(t) }) t.Run("Case=LegacyServiceRefWithIPBlockDenied", func(t *testing.T) { testLegacyInvalidCGServiceRefWithIPBlock(t) }) - t.Run("Case=LegacyInvalidChildGroupName", func(t *testing.T) { testLegacyInvalidCGChildGroupDoesNotExist(t) }) }) t.Run("TestLegacyGroupClusterGroupValidateChildGroup", func(t *testing.T) {