Report NetworkPolicy destined for Services as invalid when proxyAll is disabled #5591

hongliangl · 2023-10-17T16:34:07Z

When AntreaProxy proxyAll is not enabled, the following usages are invalid:

For an ingress NetworkPolicy, which is applied to Services. Note that,
such ingress NetworkPolicy can be only applied to NodePort Services. As
a result, proxyAll should be also enabled.
For an egress NetworkPolicy, which is to Services. Note that, headless
Services are not supported.

Signed-off-by: Hongliang Liu [email protected]

tnqn · 2023-10-23T09:39:53Z

pkg/agent/controller/networkpolicy/cache.go

@@ -863,6 +870,11 @@ func (c *ruleCache) GetCompletedRule(ruleID string) (completedRule *CompletedRul

 	r := obj.(*rule)

+	if !c.antreaProxyEnabled && r.Direction == v1beta.DirectionOut && len(r.To.ToServices) != 0 {
+		klog.V(2).InfoS("Field `ToServices` can be only used when AntreaProxy is enabled", "ruleID", ruleID)
+		return nil, false, false


We support reporting realization failure to NetworkPolicy condition. The error should be reported to antrea-controller to update the condition, instead of ignoring the rule without any error.

Do we have any existing methods to report the message like this from agent to controller currently?

We haven't done it before in agent but I think the method is there, just change v1beta2.NetworkPolicyStatus to reflect the failure. It could be generic for any failure we get from the reconciler.
BTW, I think cache.go is not a good place to perform the validation. It's implementation specific so should be validated by reconciler.go

pkg/agent/controller/networkpolicy/status_controller.go

antoninbas · 2023-10-31T22:39:11Z

pkg/agent/controller/networkpolicy/status_controller_test.go

+						NodeName:           testNode1,
+						Generation:         1,
+						RealizationFailure: true,
+						Message:            `[{"rule_id":"2e8b15c49b690893","reason":"AntreaProxy is not enabled"}]`,


IIRC, this Message will be included in the Status for the network policy CR, using a NetworkPolicyCondition.
Should we make some transformation in the controller, given that the rule ID is not meaningful to users?

Should we make some transformation in the controller, given that the rule ID is not meaningful to users?

You are right, the rule ID is not meaningful to users. Do you mean that we should provide more information in the message? Like Service name and namespace? Or something else?

antoninbas · 2023-10-31T22:58:33Z

pkg/agent/controller/networkpolicy/networkpolicy_controller.go

@@ -646,6 +646,14 @@ func (c *Controller) syncRule(key string) error {
 		return nil
 	}

+	if !c.antreaProxyEnabled && rule.isToServicesPolicyRule() {


Should we handle both cases (applied to service / selects services as peers with toServices) separately?
It would make sense to me because:

the first case (appliedTo) doesn't only require AntreaProxy, I think it also requires proxyAll? Should we add this to the validation?

we may be able to generate a better error message

isToServicesPolicyRule is not a great function name IMO; the name implies that we only consider the second case (toServices)

the first case (appliedTo) doesn't only require AntreaProxy, I think it also requires proxyAll? Should we add this to the validation?

The feature applied to Service should only require AntreaProxy. It is added in this #2755. Do you mean that we should add validation here

antrea/pkg/agent/controller/networkpolicy/networkpolicy_controller.go

Line 621 in 9e84a68

rule, effective, realizable := c.ruleCache.GetCompletedRule(key)

If so, for the first case, the expected result of realizable should be false. But it is a little strange that adding antreaProxyEnabled member to struct ruleCache.

we may be able to generate a better error message

Do you mean that we should generate more detailed error log here?

isToServicesPolicyRule is not a great function name IMO; the name implies that we only consider the second case (toServices)

How about isAppliedToServiceIngressPolicyRule() and isToServicesEgressPolicyRule()?

I think the appliedTo case (different from toServices as an egress rule) was added in #3997, and does require ProxyAll?
@tnqn am I missing something?

@antoninbas Sorry for updating late. I have verified it with @GraysonWu. It does require proxyAll for appliedTo case.

Yes, I believe both cases need AntreaProxy, and AppliedToService also needs proxyAll.

pkg/agent/controller/networkpolicy/status_controller.go

tnqn · 2023-11-16T10:19:25Z

pkg/agent/controller/networkpolicy/networkpolicy_controller.go

+	if !(c.antreaProxyEnabled && c.proxyAllEnabled) && rule.isAppliedToServicesIngressPolicyRule() {
+		klog.InfoS("Ingress policy rule applied to Services is valid only when both AntreaProxy and proxyAll are enabled, skipping", "ruleID", key)
+		if c.statusManagerEnabled {
+			c.statusManager.SetRuleAsInvalid(key, rule.PolicyUID, rule.PolicyName, rule.Name, "AntreaProxy or ProxyAll is not enabled")
+		}
+		return nil
+	}
+	if !c.antreaProxyEnabled && rule.isToServicesEgressPolicyRule() {
+		klog.InfoS("Egress policy rule to Services is valid only when AntreaProxy is enabled, skipping", "ruleID", key)
+		if c.statusManagerEnabled {
+			c.statusManager.SetRuleAsInvalid(key, rule.PolicyUID, rule.PolicyName, rule.Name, "AntreaProxy is not enabled")
+		}
+		return nil
+	}


I think we shouldn't add these implementatic specific checks in controller. Such special handling could eventually make syncRule extremely long and very specific to implementation, especially when the controller is extended to support more scenarios, like Node policy.

It should be the reconciler instead of the controller to do such check as it's the one knowing how to implement the rule. The reconciler should return an error to controller when it's unable to implement it and the controller should just report the error in a generic way to antrea-controller, without the need to know what's the error. In this way, the workflow can be reused by any realization failure in the future.

Just found I commented the same before: #5591 (comment)

Do you meant that we should do such checks in reconciler.go. If a rule is invalid, it should return an error, then we report the error to antrea-controller through statusController? If so, do we still need to requeue the rule? If statusController is not enabled, what should we do? Just log something?

tnqn · 2023-11-16T10:22:29Z

pkg/agent/controller/networkpolicy/status_controller.go

+	// invalidRules keeps track of the invalid NetworkPolicy rules.
+	invalidRules cache.Indexer


Could we change realizedRules to track both realized and failed rules? otherwise you will have to ensure the two storage are exclusive. We could also change SetRuleRealization to support setting both success and failure.

Make sense, I'll try that.

…s disabled When AntreaProxy is not enabled, the following usages are invalid: - For an ingress NetworkPolicy, which is applied to Services. Note that, such ingress NetworkPolicy can be only applied to NodePort Services. As a result, proxyAll should be also enabled. - For an egress NetworkPolicy, which is to Services. Note that, headless Services are not supported. Signed-off-by: Hongliang Liu <[email protected]>

…s disabled When proxyAll is not enabled, the following usages are invalid: - For an ingress NetworkPolicy, which is applied to Services. Note that, such ingress NetworkPolicy can be only applied to NodePort Services. As a result, proxyAll should be also enabled. - For an egress NetworkPolicy, which is to Services. Note that, headless Services are not supported. Signed-off-by: Hongliang Liu <[email protected]>

github-actions · 2024-04-09T00:04:01Z

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

github-actions · 2024-07-15T00:04:39Z

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

luolanzone · 2024-10-14T01:49:49Z

@hongliangl is this still a valid change? please reopen it if you plan to move forward.

hongliangl mentioned this pull request Oct 17, 2023

Promote feature gate AntreaProxy to GA #5401

Merged

hongliangl requested a review from tnqn October 18, 2023 06:30

tnqn reviewed Oct 23, 2023

View reviewed changes

hongliangl force-pushed the 20231017-to-service-validate branch from 9cb23be to 77adfd7 Compare October 30, 2023 10:32

hongliangl changed the title ~~Validate the egress Antrea policy rule with field ToServices~~ Report invalid egress NetworkPolicy for Services when AntreaProxy is disabled Oct 30, 2023

hongliangl requested a review from tnqn October 30, 2023 10:33

hongliangl added this to the Antrea v1.15 release milestone Oct 30, 2023

hongliangl force-pushed the 20231017-to-service-validate branch from 77adfd7 to daeba6f Compare October 30, 2023 11:13

hongliangl requested a review from antoninbas October 31, 2023 01:24

hongliangl force-pushed the 20231017-to-service-validate branch from daeba6f to 5ff60c2 Compare October 31, 2023 02:43

hongliangl changed the title ~~Report invalid egress NetworkPolicy for Services when AntreaProxy is disabled~~ Report invalid ingress NetworkPolicy destined for Services when AntreaProxy is disabled Oct 31, 2023

hongliangl force-pushed the 20231017-to-service-validate branch from 5ff60c2 to 6041faf Compare October 31, 2023 03:33

hongliangl changed the title ~~Report invalid ingress NetworkPolicy destined for Services when AntreaProxy is disabled~~ Report NetworkPolicy destined for Services as invalid when AntreaProxy is disabled Oct 31, 2023

antoninbas reviewed Oct 31, 2023

View reviewed changes

hongliangl force-pushed the 20231017-to-service-validate branch 3 times, most recently from da42ea1 to 143fc44 Compare November 1, 2023 07:46

hongliangl requested review from antoninbas and GraysonWu November 1, 2023 08:05

tnqn reviewed Nov 16, 2023

View reviewed changes

hongliangl force-pushed the 20231017-to-service-validate branch from 143fc44 to 24f4c1a Compare November 17, 2023 03:57

hongliangl changed the title ~~Report NetworkPolicy destined for Services as invalid when AntreaProxy is disabled~~ Report NetworkPolicy destined for Services as invalid when proxyAll is disabled Nov 17, 2023

hongliangl requested a review from tnqn November 17, 2023 04:00

hongliangl mentioned this pull request Jan 5, 2024

Add support for NodeNetworkPolicy datapath #5658

Merged

luolanzone modified the milestones: Antrea v1.15 release, Antrea v1.16 release Jan 9, 2024

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 9, 2024

hongliangl removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 15, 2024

hongliangl removed this from the Antrea v2.0 release milestone Apr 15, 2024

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 15, 2024

github-actions bot closed this Oct 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report NetworkPolicy destined for Services as invalid when proxyAll is disabled #5591

Report NetworkPolicy destined for Services as invalid when proxyAll is disabled #5591

hongliangl commented Oct 17, 2023 •

edited

Loading

tnqn Oct 23, 2023

hongliangl Oct 23, 2023

tnqn Oct 24, 2023

antoninbas Oct 31, 2023

hongliangl Nov 1, 2023

antoninbas Oct 31, 2023

hongliangl Nov 1, 2023 •

edited

Loading

antoninbas Nov 1, 2023

hongliangl Nov 2, 2023

tnqn Nov 16, 2023

tnqn Nov 16, 2023

tnqn Nov 16, 2023

hongliangl Nov 16, 2023

tnqn Nov 16, 2023

hongliangl Nov 16, 2023

github-actions bot commented Apr 9, 2024

github-actions bot commented Jul 15, 2024

luolanzone commented Oct 14, 2024 •

edited

Loading

		// invalidRules keeps track of the invalid NetworkPolicy rules.
		invalidRules cache.Indexer

Report NetworkPolicy destined for Services as invalid when proxyAll is disabled #5591

Report NetworkPolicy destined for Services as invalid when proxyAll is disabled #5591

Conversation

hongliangl commented Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 9, 2024

github-actions bot commented Jul 15, 2024

luolanzone commented Oct 14, 2024 • edited Loading

hongliangl commented Oct 17, 2023 •

edited

Loading

hongliangl Nov 1, 2023 •

edited

Loading

luolanzone commented Oct 14, 2024 •

edited

Loading