Trigger reconciling NetworkPolicy rules upon CNI Add #222

tnqn · 2019-12-13T07:18:44Z

Currently only Pods that have IPs are included in AppliedToGroups. It
makes a Pod's NetworkPolicies not enforced until it has IP assigned in
kube-apiserver, which always introduces a time slot that the Pod is not
isolated by NetworkPolicies created before it.

To enforce NetworkPolicies to Pods, antrea-agent doesn't need PodIPs
from kube-apiserver but the OFPort and IP from internal InterfaceCache.
This patch uses a channel to receive Pod update events from CNIServer
and notify NetworkPolicyController to reconcile rules related to the
updated Pods.

Fixes #197

antrea-bot · 2019-12-13T07:18:46Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests. This command can only be run by members of the vmware-tanzu organization
/skip-e2e: to skip e2e tests. This command can only be run by members of the vmware-tanzu organization

tnqn · 2019-12-13T07:19:03Z

/test-e2e

tnqn · 2019-12-13T10:17:32Z

/test-e2e

antoninbas

I added a couple questions. I think it would be good to add to this PR description which assumptions (if any) we make regarding ordering of events. We can add this information later in developer documentation.

antoninbas · 2019-12-13T19:38:43Z

pkg/agent/controller/networkpolicy/cache.go

+				for group, podSet := range c.podSetByGroup {
+					_, exists := podSet[pod]
+					if exists {
+						c.onAppliedToGroupUpdate(group)


Maybe a stupid question. Are we assuming that by the time the CNI server notifies us the channel and we execute this code, we have already received the created / updated relevant AppliedToGroups from the Antrea NP controller? If yes, it may be worth documenting in the comment for this function (and explaining why that is the case). If no, then I think I would benefit from an explanation, because in my mind the sequence of events is as follows:

Antrea NP controller is notified by apiserver that a new Pod is being created. The IP address is not known yet, but we update / create AppliedToGroups as needed based on existing Network Policies.

Agent NP controller is notified by Antrea NP controller, flows cannot be installed yet because IP address is not known.

CNIAdd is invoked, Pod networking is configured, IP address is determined. CNI server can notify Agent NP controller using the podUpdates channel.

processPodUpdates processes the notification from the CNI server. The IP address is now retrievable from the interface store, and we can install NP flows.

So I guess my question is: is this the correct sequence of events, are we assuming that 2) happens before 3) (and if yes why can we make this assumption)?

The NetworkPolicy implementation is all asynchronous from Antrea Controller to Antrea Agent with best-effort. Your understanding of the sequence of events is correct, but we don't assume it must happen, just it should be that case with a great possibility.
It's because the way Kubernetes works: kube-scheduler schedules a Pod on a Node by setting its its NodeName field via API, then kubelet and antrea-controller can receive the Pod update event at almost the same time, and kubelet starts to create containers for the Pod and antrea-controller starts to update the Group info and notify antrea-agent. It's observed that the former is slower than the latter so the PR works in that case. But it's hard to say it's consistent, due to different network condition, antrea-controller workload, kubelet workload, and cluster scale.
This PR is to enhance the best-effort by triggering the realization as early as possible. Even if a NetworkPolicy is created before a Pod, we can't say antrea-controller must receive the Policy from APIServer first as they are two API connections. But this slight difference should make no difference to users as they should reach eventual consistency in a very short time.
I will add comments to explain which situation this processing will help.

antoninbas · 2019-12-13T19:48:29Z

pkg/agent/cniserver/server.go

@@ -403,6 +406,10 @@ func (s *CNIServer) CmdAdd(ctx context.Context, request *cnipb.CniCmdRequest) (
 		klog.Errorf("Failed to configure container %s interface: %v", cniConfig.ContainerId, err)
 		return s.configInterfaceFailureResponse(err), nil
 	}
+
+	// Notify the Pod update event to required components.
+	s.podUpdates <- v1beta1.PodReference{Name: podName, Namespace: podNamespace}


so as a follow-up to your comment there: #197 (comment)

we are assuming that it takes less time to 1) notify on the channel + reconcile rules + install flows than it takes for 2) kubelet to receive the response from the CNI and create the workload container

If 1) is very fast, wouldn't it be acceptable to just block until we know it has completed before returning from CNI Add? This way we would have a guarantee that known network policies are enforced before the workload container is created. I know that we don't have a notification mechanism to determine when flows have been installed, but that's something we may have in the future. The only issue I see is we need to know the OF port for NP flows, which means we cannot start installing flows until we have added the new port to the vSwitch.

Even we block there there is no guarantee Controller already computes and pushes all policies applied to the Pod. As most event processing is already async, I prefer to keep the async behaviors here too.

Agree @jianjuns. Also, as I explained in #222 (comment), it's not even guaranteed antrea-controller will receive the network policy and handle it first unless we sort events of different resources which makes it more complicated. I think the most important thing is whether it's worth to do it if it makes no difference to users.

jianjuns · 2019-12-16T05:07:29Z

pkg/agent/cniserver/server.go

@@ -403,6 +406,10 @@ func (s *CNIServer) CmdAdd(ctx context.Context, request *cnipb.CniCmdRequest) (
 		klog.Errorf("Failed to configure container %s interface: %v", cniConfig.ContainerId, err)
 		return s.configInterfaceFailureResponse(err), nil
 	}
+
+	// Notify the Pod update event to required components.
+	s.podUpdates <- v1beta1.PodReference{Name: podName, Namespace: podNamespace}


Even we block there there is no guarantee Controller already computes and pushes all policies applied to the Pod. As most event processing is already async, I prefer to keep the async behaviors here too.

jianjuns · 2019-12-16T05:15:50Z

pkg/controller/networkpolicy/networkpolicy_controller.go

@@ -1080,8 +1080,8 @@ func (n *NetworkPolicyController) syncAppliedToGroup(key string) error {
 	// Retrieve all Pods matching the podSelector.
 	pods, err = n.podLister.Pods(appliedToGroup.Selector.Namespace).List(selector)
 	for _, pod := range pods {
-		if pod.Status.PodIP == "" {
-			// No need to process Pod when IPAddress is unset.
+		if pod.Spec.NodeName == "" {


If typically Controller always receives a Pod event without NodeName (and IP), maybe we should optimize to ignore the event in updatePod()?

I do not mean we need to make the change in this PR.

makes sense, I would create an issue to track the optimization as there could be more cases: if nodeName is not set, no need to trigger appliedToGroup; if PodIP is not set, no need to trigger addressGroup.

tnqn · 2019-12-16T12:31:14Z

/test-e2e

jianjuns

LGTM. Added one minor comment and one question.

pkg/agent/controller/networkpolicy/reconciler.go

tnqn · 2019-12-17T03:23:54Z

/test-e2e

pkg/agent/controller/networkpolicy/reconciler.go

Currently only Pods that have IPs are included in AppliedToGroups. It makes a Pod's NetworkPolicies not enforced until it has IP assigned in kube-apiserver, which always introduces a time slot that the Pod is not isolated by NetworkPolicies created before it. To enforce NetworkPolicies to Pods, antrea-agent doesn't need PodIPs from kube-apiserver but the OFPort and IP from internal InterfaceCache. This patch uses a channel to receive Pod update events from CNIServer and notify NetworkPolicyController to reconcile rules related to the updated Pods.

tnqn · 2019-12-18T02:31:39Z

/test-e2e

vmwclabot added the cla-not-required label Dec 13, 2019

tnqn force-pushed the egress-enhance branch 2 times, most recently from 24f9f44 to f2a6c5e Compare December 13, 2019 07:32

antoninbas reviewed Dec 13, 2019

View reviewed changes

jianjuns reviewed Dec 16, 2019

View reviewed changes

tnqn mentioned this pull request Dec 16, 2019

Optimize to ignore Pod events without NodeName (and IP) in updatePod() #231

Closed

tnqn force-pushed the egress-enhance branch from f2a6c5e to d432425 Compare December 16, 2019 08:05

jianjuns reviewed Dec 16, 2019

View reviewed changes

pkg/agent/controller/networkpolicy/reconciler.go Show resolved Hide resolved

pkg/agent/controller/networkpolicy/reconciler.go Show resolved Hide resolved

tnqn force-pushed the egress-enhance branch from d432425 to ade73f9 Compare December 17, 2019 03:16

jianjuns previously approved these changes Dec 17, 2019

View reviewed changes

pkg/agent/controller/networkpolicy/reconciler.go Show resolved Hide resolved

tnqn dismissed jianjuns’s stale review via 8ce8c23 December 18, 2019 02:31

tnqn force-pushed the egress-enhance branch from ade73f9 to 8ce8c23 Compare December 18, 2019 02:31

jianjuns approved these changes Dec 18, 2019

View reviewed changes

tnqn merged commit 791e2ba into antrea-io:master Dec 18, 2019

tnqn deleted the egress-enhance branch December 18, 2019 05:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trigger reconciling NetworkPolicy rules upon CNI Add #222

Trigger reconciling NetworkPolicy rules upon CNI Add #222

tnqn commented Dec 13, 2019 •

edited

Loading

antrea-bot commented Dec 13, 2019

tnqn commented Dec 13, 2019

tnqn commented Dec 13, 2019

antoninbas left a comment

antoninbas Dec 13, 2019

tnqn Dec 16, 2019

antoninbas Dec 13, 2019

jianjuns Dec 16, 2019

tnqn Dec 16, 2019

jianjuns Dec 16, 2019

jianjuns Dec 16, 2019

tnqn Dec 16, 2019

tnqn commented Dec 16, 2019

jianjuns left a comment

tnqn commented Dec 17, 2019

tnqn commented Dec 18, 2019

Trigger reconciling NetworkPolicy rules upon CNI Add #222

Trigger reconciling NetworkPolicy rules upon CNI Add #222

Conversation

tnqn commented Dec 13, 2019 • edited Loading

antrea-bot commented Dec 13, 2019

tnqn commented Dec 13, 2019

tnqn commented Dec 13, 2019

antoninbas left a 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

Choose a reason for hiding this comment

tnqn commented Dec 16, 2019

jianjuns left a comment

Choose a reason for hiding this comment

tnqn commented Dec 17, 2019

tnqn commented Dec 18, 2019

tnqn commented Dec 13, 2019 •

edited

Loading