Mutate empty Tier set in Antrea-native policies to default tier (#1567)

pkg/controller/networkpolicy/mutate.go

@@ -39,7 +39,7 @@ func NewNetworkPolicyMutator(networkPolicyController *NetworkPolicyController) *
 	}
 }
 
-// Mutate function mutate a Antrea Policy object
+// Mutate function mutates an Antrea-native policy object
 func (m *NetworkPolicyMutator) Mutate(ar *admv1.AdmissionReview) *admv1.AdmissionResponse {
 	var result *metav1.Status
 	var msg string
@@ -67,7 +67,7 @@ func (m *NetworkPolicyMutator) Mutate(ar *admv1.AdmissionReview) *admv1.Admissio
 				return GetAdmissionResponseForErr(err)
 			}
 		}
-		msg, allowed, patch = m.mutateAntreaPolicyRuleName(op, curACNP.Spec.Ingress, curACNP.Spec.Egress)
+		msg, allowed, patch = m.mutateAntreaPolicy(op, curACNP.Spec.Ingress, curACNP.Spec.Egress, curACNP.Spec.Tier)
 	case "NetworkPolicy":
 		klog.V(2).Info("Mutating Antrea NetworkPolicy CRD")
 		var curANP, oldANP secv1alpha1.NetworkPolicy
@@ -83,7 +83,7 @@ func (m *NetworkPolicyMutator) Mutate(ar *admv1.AdmissionReview) *admv1.Admissio
 				return GetAdmissionResponseForErr(err)
 			}
 		}
-		msg, allowed, patch = m.mutateAntreaPolicyRuleName(op, curANP.Spec.Ingress, curANP.Spec.Egress)
+		msg, allowed, patch = m.mutateAntreaPolicy(op, curANP.Spec.Ingress, curANP.Spec.Egress, curANP.Spec.Tier)
 	}
 
 	if msg != "" {
@@ -99,19 +99,28 @@ func (m *NetworkPolicyMutator) Mutate(ar *admv1.AdmissionReview) *admv1.Admissio
 	}
 }
 
-// mutateAntreaPolicyRuleName mutates names of rules of an Antrea NetworkPolicy CRD.
+// mutateAntreaPolicy mutates names of rules of an Antrea NetworkPolicy CRD.
 // If users didn't specify the name of an ingress or egress rule,
-// mutateAntreaPolicyRuleName will auto-generate a name for this rule.
-func (m *NetworkPolicyMutator) mutateAntreaPolicyRuleName(op admv1.Operation, ingress, egress []secv1alpha1.Rule) (string, bool, []byte) {
+// mutateAntreaPolicy will auto-generate a name for this rule. In
+// addition to the rule names, it also mutates the Tier field to the default
+// tier name if it is unset.
+func (m *NetworkPolicyMutator) mutateAntreaPolicy(op admv1.Operation, ingress, egress []secv1alpha1.Rule, tier string) (string, bool, []byte) {
 	allowed := true
 	reason := ""
 	var patch []byte
 	switch op {
 	case admv1.Create, admv1.Update:
+		// Mutate Antrea-native policy rule names.
 		ingressRulePaths, ingressRuleNames := generateRuleNames("ingress", ingress)
 		egressRulePaths, egressRuleNames := generateRuleNames("egress", egress)
-
-		genPatch, err := createReplacePatch(append(ingressRulePaths, egressRulePaths...), append(ingressRuleNames, egressRuleNames...))
+		allPaths := append(ingressRulePaths, egressRulePaths...)
+		allValues := append(ingressRuleNames, egressRuleNames...)
+		// Mutate empty tier name to the name of the Default application Tier.
+		if tier == "" {
+			allPaths = append(allPaths, fmt.Sprintf("/spec/tier"))
+			allValues = append(allValues, defaultTierName)
+		}
+		genPatch, err := createReplacePatch(allPaths, allValues)
 		if err != nil {
 			allowed = false
 			reason = "unable to generate mutating patch"

pkg/controller/networkpolicy/tier.go

@@ -40,15 +40,17 @@ var (
 	// baselineTierPriority maintains the priority for the system generated baseline Tier.
 	// This is the tier that will be enforced after K8s NetworkPolicies.
 	baselineTierPriority = int32(253)
+	// defaultTierName maintains the name of the default Tier in Antrea.
+	defaultTierName = "application"
 	// priorityMap maintains the Tier priority associated with system generated
 	// Tier names.
 	priorityMap = map[string]int32{
-		"baseline":    baselineTierPriority,
-		"application": defaultTierPriority,
-		"platform":    int32(150),
-		"networkops":  int32(100),
-		"securityops": int32(50),
-		"emergency":   int32(5),
+		"baseline":      baselineTierPriority,
+		defaultTierName: defaultTierPriority,
+		"platform":      int32(150),
+		"networkops":    int32(100),
+		"securityops":   int32(50),
+		"emergency":     int32(5),
 	}
 	// staticTierSet maintains the names of the static tiers such that they can
 	// be converted to corresponding Tier CRD names.
@@ -66,10 +68,10 @@ var (
 		},
 		{
 			ObjectMeta: metav1.ObjectMeta{
-				Name: "application",
+				Name: defaultTierName,
 			},
 			Spec: secv1alpha1.TierSpec{
-				Priority:    priorityMap["application"],
+				Priority:    priorityMap[defaultTierName],
 				Description: "[READ-ONLY]: System generated default Application Tier",
 			},
 		},

pkg/controller/networkpolicy/validate.go

@@ -129,7 +129,7 @@ func (v *NetworkPolicyValidator) validateAntreaPolicy(op admv1.Operation, tier s
 			reason = "rules names are not unique, or policy has duplicate rules, or collision occurred in generated rule names"
 			break
 		}
-		// "tier" must exist before referencing
+		// "tier" must exist before referencing.
 		if tier == "" || staticTierSet.Has(tier) {
 			// Empty Tier name corresponds to default Tier
 			break

test/e2e/antreapolicy_test.go

@@ -47,8 +47,9 @@ const (
 	// provide enough time for policies to be enforced & deleted by the CNI plugin.
 	networkPolicyDelay = 2 * time.Second
 	// audit log directory on Antrea Agent
-	logDir      = "/var/log/antrea/networkpolicy/"
-	logfileName = "np.log"
+	logDir          = "/var/log/antrea/networkpolicy/"
+	logfileName     = "np.log"
+	defaultTierName = "application"
 )
 
 func failOnError(err error, t *testing.T) {
@@ -140,6 +141,42 @@ func cleanupDefaultDenyNPs(k8s *KubernetesUtils, namespaces []string) error {
 	return nil
 }
 
+func testMutateACNPNoTier(t *testing.T) {
+	invalidNpErr := fmt.Errorf("ACNP tier not mutated to default tier")
+	builder := &ClusterNetworkPolicySpecBuilder{}
+	builder = builder.SetName("acnp-no-tier").
+		SetAppliedToGroup(map[string]string{"pod": "a"}, nil, nil, nil).
+		SetPriority(10.0)
+	acnp := builder.Get()
+	log.Debugf("creating ACNP %v", acnp.Name)
+	acnp, err := k8sUtils.CreateOrUpdateCNP(acnp)
+	if err != nil {
+		failOnError(fmt.Errorf("ACNP create failed %v", err), t)
+	}
+	if acnp.Spec.Tier != defaultTierName {
+		failOnError(invalidNpErr, t)
+	}
+	failOnError(k8sUtils.CleanCNPs(), t)
+}
+
+func testMutateANPNoTier(t *testing.T) {
+	invalidNpErr := fmt.Errorf("ANP tier not mutated to default tier")
+	builder := &AntreaNetworkPolicySpecBuilder{}
+	builder = builder.SetName("x", "anp-no-tier").
+		SetAppliedToGroup(map[string]string{"pod": "a"}, nil).
+		SetPriority(10.0)
+	anp := builder.Get()
+	log.Debugf("creating ANP %v", anp.Name)
+	anp, err := k8sUtils.CreateOrUpdateANP(anp)
+	if err != nil {
+		failOnError(fmt.Errorf("ANP create failed %v", err), t)
+	}
+	if anp.Spec.Tier != defaultTierName {
+		failOnError(invalidNpErr, t)
+	}
+	failOnError(k8sUtils.CleanANPs([]string{anp.Namespace}), t)
+}
+
 func testInvalidACNPNoPriority(t *testing.T) {
 	invalidNpErr := fmt.Errorf("invalid Antrea ClusterNetworkPolicy accepted")
 	builder := &ClusterNetworkPolicySpecBuilder{}
@@ -855,12 +892,17 @@ func TestAntreaPolicy(t *testing.T) {
 		t.Run("Case=ACNPNoPriority", func(t *testing.T) { testInvalidACNPNoPriority(t) })
 	})
 
+	t.Run("TestGroupMutateAntreaNativePolicies", func(t *testing.T) {
+		t.Run("Case=ACNPNoTierSetDefaultTier", func(t *testing.T) { testMutateACNPNoTier(t) })
+		t.Run("Case=ANPNoTierSetDefaultTier", func(t *testing.T) { testMutateANPNoTier(t) })
+	})
+
 	t.Run("TestGroupDefaultDENY", func(t *testing.T) {
 		// testcases below require default deny k8s NetworkPolicies to work
 		applyDefaultDenyToAllNamespaces(k8sUtils, namespaces)
 		t.Run("Case=CNPAllowXBtoA", func(t *testing.T) { testCNPAllowXBtoA(t) })
 		t.Run("Case=CNPAllowXBtoYA", func(t *testing.T) { testCNPAllowXBtoYA(t) })
-		t.Run("Case=CNPPrioirtyOverrideDefaultDeny", func(t *testing.T) { testCNPPriorityOverrideDefaultDeny(t) })
+		t.Run("Case=CNPPriorityOverrideDefaultDeny", func(t *testing.T) { testCNPPriorityOverrideDefaultDeny(t) })
 		cleanupDefaultDenyNPs(k8sUtils, namespaces)
 	})