Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove child group existence validation for ClusterGroup #2443

Merged
merged 2 commits into from
Aug 2, 2021
Merged

Remove child group existence validation for ClusterGroup #2443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8a8cd4
Remove child group existence validation for ClusterGroup
Dyanngg Jul 21, 2021
d0b5321
Add E2E testcase for childGroup creation after parent
Dyanngg Jul 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 12 additions & 27 deletions pkg/controller/networkpolicy/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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 {
Expand All @@ -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.
Expand All @@ -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.
Expand All @@ -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
}
45 changes: 31 additions & 14 deletions test/e2e/antreapolicy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2246,28 +2246,32 @@ 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).
SetAppliedToGroup([]ACNPAppliedToSpec{{NSSelector: map[string]string{"ns": "z"}}}).
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",
Expand All @@ -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{
{
Expand All @@ -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},
}
Expand Down
44 changes: 21 additions & 23 deletions test/e2e/clustergroup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down
18 changes: 0 additions & 18 deletions test/e2e/legacyclustergroup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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) {
Expand Down