Fix status report when no-op changes are applied to Antrea-native pol…

…icies When no-op changes are applied to Antrea-native policies, such as adding or removing a non-existing group, there will be no datapath change but the generation has changed. The agent must report it has realized the latest generation even if it does nothing, otherwise the policy would stay realizing. Signed-off-by: Quan Tian <[email protected]>
antrea-io · Jun 7, 2023 · c3816d5 · c3816d5
1 parent c003098
commit c3816d5
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 9 deletions.
diff --git a/pkg/agent/controller/networkpolicy/cache.go b/pkg/agent/controller/networkpolicy/cache.go
@@ -758,16 +758,18 @@ func (c *ruleCache) AddNetworkPolicy(policy *v1beta.NetworkPolicy) {
 	c.updateNetworkPolicyLocked(policy)
 }
 
-// UpdateNetworkPolicy updates a cached *v1beta.NetworkPolicy and returns whether there is any rule change.
+// UpdateNetworkPolicy updates a cached *v1beta.NetworkPolicy and returns whether any rule or the generation changes.
 // The added rules and removed rules will be regarded as dirty.
 func (c *ruleCache) UpdateNetworkPolicy(policy *v1beta.NetworkPolicy) bool {
 	c.policyMapLock.Lock()
 	defer c.policyMapLock.Unlock()
 	return c.updateNetworkPolicyLocked(policy)
 }
 
-// updateNetworkPolicyLocked returns whether there is any rule change.
+// updateNetworkPolicyLocked returns whether any rule or the generation changes.
 func (c *ruleCache) updateNetworkPolicyLocked(policy *v1beta.NetworkPolicy) bool {
+	oldPolicy, exists := c.policyMap[string(policy.UID)]
+	generationUpdated := !exists || oldPolicy.Generation != policy.Generation
 	c.policyMap[string(policy.UID)] = policy
 	existingRules, _ := c.rules.ByIndex(policyIndex, string(policy.UID))
 	ruleByID := map[string]interface{}{}
@@ -810,7 +812,7 @@ func (c *ruleCache) updateNetworkPolicyLocked(policy *v1beta.NetworkPolicy) bool
 		c.dirtyRuleHandler(ruleID)
 		anyRuleUpdate = true
 	}
-	return anyRuleUpdate
+	return anyRuleUpdate || generationUpdated
 }
 
 // DeleteNetworkPolicy deletes a cached *v1beta.NetworkPolicy.

diff --git a/pkg/agent/controller/networkpolicy/networkpolicy_controller.go b/pkg/agent/controller/networkpolicy/networkpolicy_controller.go
@@ -252,11 +252,10 @@ func NewNetworkPolicyController(antreaClientGetter agent.AntreaClientProvider,
 					policy.SourceRef.ToString())
 				return nil
 			}
-			anyRuleUpdate := c.ruleCache.UpdateNetworkPolicy(policy)
-			// If there is any rule update, we ensure statusManager will resync the policy's status once, in case that
-			// the added/deleted/updated rule is not effective, in which case the rule's realization status is not
-			// changed but the whole policy's generation is changed.
-			if c.statusManagerEnabled && anyRuleUpdate && policy.SourceRef.Type != v1beta2.K8sNetworkPolicy {
+			updated := c.ruleCache.UpdateNetworkPolicy(policy)
+			// If any rule or the generation changes, we ensure statusManager will resync the policy's status once, in
+			// case the changes don't cause any actual rule update but the whole policy's generation is changed.
+			if c.statusManagerEnabled && updated && policy.SourceRef.Type != v1beta2.K8sNetworkPolicy {
 				c.statusManager.Resync(policy.UID)
 			}
 			return nil

diff --git a/test/e2e/antreapolicy_test.go b/test/e2e/antreapolicy_test.go
@@ -4560,13 +4560,39 @@ func TestAntreaPolicyStatusWithAppliedToPerRule(t *testing.T) {
 	anp.Spec.Ingress = anp.Spec.Ingress[0:1]
 	_, err = data.crdClient.CrdV1alpha1().NetworkPolicies(anp.Namespace).Update(context.TODO(), anp, metav1.UpdateOptions{})
 	assert.NoError(t, err)
-	checkANPStatus(t, data, anp, crdv1alpha1.NetworkPolicyStatus{
+	anp = checkANPStatus(t, data, anp, crdv1alpha1.NetworkPolicyStatus{
 		Phase:                crdv1alpha1.NetworkPolicyRealized,
 		ObservedGeneration:   2,
 		CurrentNodesRealized: 1,
 		DesiredNodesRealized: 1,
 		Conditions:           networkpolicy.GenerateNetworkPolicyCondition(nil),
 	})
+
+	// Add a non-existing group.
+	// Although nothing will be changed in datapath, the policy's status should be realized with the latest generation.
+	anp.Spec.Ingress[0].AppliedTo = append(anp.Spec.Ingress[0].AppliedTo, crdv1alpha1.AppliedTo{Group: "foo"})
+	_, err = data.crdClient.CrdV1alpha1().NetworkPolicies(anp.Namespace).Update(context.TODO(), anp, metav1.UpdateOptions{})
+	assert.NoError(t, err)
+	anp = checkANPStatus(t, data, anp, crdv1alpha1.NetworkPolicyStatus{
+		Phase:                crdv1alpha1.NetworkPolicyRealized,
+		ObservedGeneration:   3,
+		CurrentNodesRealized: 1,
+		DesiredNodesRealized: 1,
+		Conditions:           networkpolicy.GenerateNetworkPolicyCondition(nil),
+	})
+
+	// Delete the non-existing group.
+	// Although nothing will be changed in datapath, the policy's status should be realized with the latest generation.
+	anp.Spec.Ingress[0].AppliedTo = anp.Spec.Ingress[0].AppliedTo[0:1]
+	_, err = data.crdClient.CrdV1alpha1().NetworkPolicies(anp.Namespace).Update(context.TODO(), anp, metav1.UpdateOptions{})
+	assert.NoError(t, err)
+	checkANPStatus(t, data, anp, crdv1alpha1.NetworkPolicyStatus{
+		Phase:                crdv1alpha1.NetworkPolicyRealized,
+		ObservedGeneration:   4,
+		CurrentNodesRealized: 1,
+		DesiredNodesRealized: 1,
+		Conditions:           networkpolicy.GenerateNetworkPolicyCondition(nil),
+	})
 }
 
 func TestAntreaPolicyStatusWithAppliedToUnsupportedGroup(t *testing.T) {