From c3816d5c4b60a908fe1311040c1f126432966772 Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Wed, 7 Jun 2023 20:16:22 +0800 Subject: [PATCH] Fix status report when no-op changes are applied to Antrea-native policies 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 --- pkg/agent/controller/networkpolicy/cache.go | 8 ++++-- .../networkpolicy/networkpolicy_controller.go | 9 +++--- test/e2e/antreapolicy_test.go | 28 ++++++++++++++++++- 3 files changed, 36 insertions(+), 9 deletions(-) diff --git a/pkg/agent/controller/networkpolicy/cache.go b/pkg/agent/controller/networkpolicy/cache.go index 7665e88462d..f61fcd227f8 100644 --- a/pkg/agent/controller/networkpolicy/cache.go +++ b/pkg/agent/controller/networkpolicy/cache.go @@ -758,7 +758,7 @@ 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() @@ -766,8 +766,10 @@ func (c *ruleCache) UpdateNetworkPolicy(policy *v1beta.NetworkPolicy) bool { 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 index 8cad7f77d7c..c65f69b8c55 100644 --- 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 index 53e551c57bc..7659b449ad7 100644 --- 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) {