Introducing NodePortLocal in Antrea Agent #1459

monotosh-avi · 2020-10-29T16:54:08Z

This commit introduces NodePortLocal (NPL) in Antrea agent.
With NPL, a Pod port can be directly reached from external network through a port of the node on which the pod is running.
NPL programs IPTABLES rules to send incoming traffic on a specific port of a node to a POD port.
This information is exposed through annotation of the POD for consumption of other agents. An example of the pod annotation is given below.
metadata:
annotations:
npl.k8s.io/endpoints: '[{"podport":"8080","nodeip":"10.102.47.229","nodeport":"40002"}]'

To use this feature, following changes are required in Antrea configuration:

Set NodePortLocal: true under featureGates in antrea-agent.conf in Antrea Configmap.
In the Antrea Agent DaemonSet configure the port range for NPL (NPL_PORT_RANGE) in the env. e.g.
- name: NPL_PORT_RANGE
value: 40000-46000
Add capability for pod update in antrea-agent cluster role

Note: NPL agent is currently not supported in Windows platform

Upcoming changes in future commits:

Label based service filtering to select backend pods for NPL
Option to select port range for NPL though Configmap
Bootup sync : Compare programmed IPTABLES rules with pod annotation during bootup and add/delete new rules only if required.
- Currently we are deleting all the rules and adding new rules for all the pods while booting up Antrea agent.

Signed-off-by: Hemant Shaw [email protected]
Signed-off-by: Manu Dilip Shah [email protected]
Signed-off-by: Monotosh Das [email protected]
Signed-off-by: Shubham Chauhan [email protected]
Signed-off-by: Sudipta Biswas [email protected]

antrea-bot · 2020-10-29T16:54:21Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy 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.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

codecov-io · 2020-10-29T17:30:28Z

Codecov Report

Merging #1459 (b7f2021) into master (9d3d10b) will decrease coverage by 1.66%.
The diff coverage is 45.57%.

@@            Coverage Diff             @@
##           master    #1459      +/-   ##
==========================================
- Coverage   63.31%   61.65%   -1.67%     
==========================================
  Files         170      193      +23     
  Lines       14250    16961    +2711     
==========================================
+ Hits         9023    10458    +1435     
- Misses       4292     5445    +1153     
- Partials      935     1058     +123

Flag	Coverage Δ
e2e-tests	`46.21% <38.28%> (?)`
kind-e2e-tests	`51.54% <37.23%> (-3.85%)`	⬇️
unit-tests	`40.39% <36.77%> (-0.88%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/antrea-agent/agent.go	`0.00% <0.00%> (ø)`
.../agent/apiserver/handlers/networkpolicy/handler.go	`58.33% <ø> (ø)`
...gent/controller/noderoute/node_route_controller.go	`60.98% <0.00%> (+14.51%)`	⬆️
pkg/agent/nodeportlocal/k8s/annotations.go	`0.00% <0.00%> (ø)`
pkg/agent/nodeportlocal/k8s/controller.go	`0.00% <0.00%> (ø)`
pkg/agent/nodeportlocal/portcache/port_table.go	`0.00% <0.00%> (ø)`
pkg/agent/nodeportlocal/rules/iptable_rule.go	`0.00% <0.00%> (ø)`
pkg/agent/nodeportlocal/rules/rules.go	`0.00% <0.00%> (ø)`
pkg/agent/nodeportlocal/util/parse_port.go	`0.00% <0.00%> (ø)`
pkg/agent/proxy/proxier.go	`77.20% <ø> (+2.39%)`	⬆️
... and 159 more

antoninbas

some initial comments

I'm not 100% sure (would like to hear from others) but it seems maybe the entire npl code could go under pkg/agent/controller to match other parts of the agent's implementation.

antoninbas · 2020-10-29T20:01:39Z

pkg/agent/nplagent/nplagent_test.go

+	"testing"
+	"time"
+
+	"github.com/onsi/gomega"


we don't use gomega for unit tests in this repo, we use the Go testing framework directly + github.com/stretchr/testify for test assertions

Sure. I'll check it out.

antoninbas · 2020-10-29T20:05:53Z

pkg/agent/nplagent/lib/constants.go

+package lib
+
+const (
+	NPLEPAnnotation = "npl.k8s.io/endpoints"


I am not an expert there, but it seems wrong to use k8s.io for an Antrea-specific feature. The K8s documentation states:

The kubernetes.io/ and k8s.io/ prefixes are reserved for Kubernetes core components.

It seems that it should be npl.antrea.tanzu.vmware.com to be consistent with the Antrea APIs

Agreed. Or for short, we can consider antrea or antrea.io too.

antoninbas · 2020-10-29T20:09:51Z

pkg/agent/nplagent/lib/constants.go

+
+const (
+	NPLEPAnnotation = "npl.k8s.io/endpoints"
+	IPTableRule     = "IPTABLE"


Based on usage in pkg/agent/nplagent/rules/rules.go, it seems that this should be an enum or you should define a type for this. For example:

type NPLRuleImplementation string const NPLRuleImplementationIptable NPLRuleImplementation = "Iptable"

antoninbas · 2020-10-29T20:14:48Z

pkg/agent/nplagent/rules/rules.go

+func Initrules(args ...string) PodPortRules {
+	var ruletype string
+	if len(args) == 0 {
+		// By default use iptable
+		ruletype = nplutils.IPTableRule
+	} else {
+		ruletype = args[0]
+	}
+	switch ruletype {
+	case "IPTABLE":
+		ipt, err := iptables.New()
+		if err != nil {
+			return nil
+		}
+		iptRule := IPTableRule{
+			name:  "NPL",
+			table: ipt,
+		}
+
+		iptRule.DeleteAllRules()
+		iptRule.CreateChains()
+		return &iptRule
+	}
+	return nil
+}


maybe I'm missing something, but this seems to really defeat the point of having an interface.

there should be a NewIPTableRule constructor and the client should be responsible for calling Init on the returned object, which would in turn take care of creating new chains, etc if necessary.

Ideally we should be able to figure out the type of implementation for PodPortRules from the underneath system's capability or configuration specified. In this case, as we are only supporting one kind of implementation we don't need to do that.
Though, I get your point about adding an abstraction. I'll update it.

antoninbas · 2020-10-29T20:16:04Z

pkg/agent/nplagent/rules/iptable_rule.go

+	"k8s.io/klog"
+)
+
+type IPTableRule struct {


don't know if this is a great name, this doesn't seem to represent a single rule

antoninbas · 2020-10-29T20:21:30Z

pkg/agent/nplagent/bootstrap/npl_agent_init_windows.go

+	klog.Errorf("NodePortLocal is currently not supported in Windows Platform ")
+	return errors.New("Windows Platform not supported fot NPL")


same comment as in another place: do not log and return the same error, it will end up being logged twice

antoninbas · 2020-10-29T20:22:08Z

pkg/agent/nplagent/bootstrap/npl_agent_init.go

@@ -0,0 +1,43 @@
+// +build !windows


I feel like there is no need for a bootstrap package here, these files can go in `pkg/agent/nplagent/ directly

Makes Sense. I'll move the file.

antoninbas · 2020-10-29T20:29:57Z

pkg/agent/nplagent/rules/iptable_rule.go

+// CreateChains : Create the chain NODE-PORT-LOCAL in NAT table
+// All DNAT rules for NPL would be added in this chain
+func (ipt *IPTableRule) CreateChains() error {
+	exists, err := ipt.table.Exists("nat", "NODE-PORT-LOCAL")


"NODE-PORT-LOCAL" should maybe be a constant (nodePortLocalChain)

antoninbas · 2020-10-29T20:31:07Z

pkg/agent/nplagent/rules/rules.go

+	AddRule(port int, podip string) (bool, error)
+	DeleteRule(port int, podip string) (bool, error)
+	SyncState(podPort map[int]string) bool
+	GetAllRules(podPort map[int]string) (bool, error)
+	DeleteAllRules() (bool, error)


I don't understand why most of these interface functions return both a boolean (to indicate success) and an error, instead of just an error?

Make sense. I'll just return one value here.

antoninbas · 2020-10-29T20:37:08Z

pkg/agent/nplagent/portcache/port_table.go

+	pt.tableLock.RLock()
+	defer pt.tableLock.RUnlock()
+	for i := pt.StartPort; i <= pt.EndPort; i++ {
+		if _, ok := pt.Table[i]; !ok && nplutils.IsPortAvailable(i) {


I'm trying to understand the usefulness of the IsPortAvailable port here. Once we select a port number for NPL and install the iptables NAT rule, is there anything preventing a process running on the host to bind to this port as well?

We do a unix socket bind in IsPortAvailable(). So, at least we should be able to check if any other process is already using this port and skip it.

jianjuns

Yes, currently Agent K8s controllers are all under pkg/agent/controller, but I can understand if a feature is big you might not want to put all code under controller/. Is it possible to keep only K8s controller code in controller/?
@tnqn

pkg/agent/nplagent/bootstrap/npl_agent_init_windows.go

build/yamls/antrea.yml

monotosh-avi · 2020-10-30T17:35:53Z

I have added a commit with the changes requested

jianjuns

Sorry, have not reviewed the majority of the code yet. Some comments for the commit message:

This commit introduces NodePortLocal (NPL) in Antrea agent.
With NPL, a pod pod port can be directly reached from external network through a port of the node on which the pod is running.

"a pod pod port" -> "a Pod port"?
You used both "pod" and "POD". I suggest to use Pod (Antrea code/documentation capitalize the first letter of a K8s resource type, like NetworkPolicy, Service, Namespace, etc.).

NPL programs IPTABLE rules to send incoming traffic on a specific port of a node to a POD port.

IPTABLE -> IPTABLES or iptables

This information is exposed through annotation of the POD for consumption of other agents. An example of the pod annotation is given bellow.

bellow -> below

metadata:
annotations:
npl.k8s.io/endpoints: '[{"podport":"8080","nodeip":"10.102.47.229","nodeport":"40002"}]'
More details about Node-Port-Local can be found in the design document: https://docs.google.com/document/d/1SoAKBvbbt2QMnGmt53E8ja0ZxCv_T4r1zLVnMzpykTg

I suggest not to add google doc links to commit messages or code, as the links can become invalid.

To use this feature, following changes are required in Antrea configuration:

Set NodePortLocal: true under featureGates in antrea-agent.conf in Antrea Configmap.

In the Antrea Agent DaemonSet configure the port range for NPL (NPL_PORT_RANGE) in the env. e.g.
- name: NPL_PORT_RANGE
value: 40000-46000

Add capability for pod update in antrea-agent cluster role

Note: NPL agent is currently not supported in Windows platform

Upcoming changes in future commits:

Label based service filtering to select backend pods for NPL

Option to select port range for NPL though Configmap

Bootup sync : Compare programmed IPtable rules with pod annotation during bootup and add/delete new rules in iptable only if required.

IPTable/iptable -> IPTABLES or iptables

- Currently we are deleting all the rules and adding new rules for all the pods while booting up Antrea agent.

build/yamls/base/conf/antrea-agent.conf

pkg/features/antrea_features.go

pkg/agent/nplagent/rules/rules.go

pkg/agent/nplagent/k8s/annotations.go

pkg/agent/nplagent/lib/constants.go

jianjuns · 2020-11-03T06:08:57Z

pkg/agent/nplagent/lib/constants.go

+package lib
+
+const (
+	NPLEPAnnotation    = "npl.antrea.io"


This could be in k8s/annotation.go? Does it need to be public?

We are using this in tests. I have moved this to k8s/annotations.go, but keeping it public.

pkg/agent/nplagent/lib/constants.go

pkg/agent/nplagent/lib/utils.go

antrea-bot · 2020-11-03T13:32:13Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy 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.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-all-features-conformance: to trigger conformance tests with all alpha features enabled.
/skip-all-features-conformance: to skip conformance tests with all alpha features enabled.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

monotosh-avi

Thanks Jianjun for reviewing. I have updated with a commit.

pkg/features/antrea_features.go

pkg/agent/nplagent/rules/rules.go

pkg/agent/nplagent/lib/utils.go

monotosh-avi · 2020-11-03T12:43:24Z

pkg/agent/nplagent/lib/constants.go

+package lib
+
+const (
+	NPLEPAnnotation    = "npl.antrea.io"


We are using this in tests. I have moved this to k8s/annotations.go, but keeping it public.

pkg/agent/nplagent/lib/constants.go

pkg/agent/nplagent/k8s/annotations.go

build/yamls/base/conf/antrea-agent.conf

pkg/agent/nplagent/rules/rules.go

pkg/agent/nplagent/lib/constants.go

pkg/agent/nplagent/rules/iptable_rule.go

pkg/agent/nplagent/k8s/k8s_controller.go

monotosh-avi

Updated with the requested changes.
I'll wait for the review of @tnqn for more changes.

pkg/agent/nplagent/rules/iptable_rule.go

tnqn

Haven't finished review yet.

tnqn · 2020-11-05T17:04:48Z

build/yamls/base/conf/antrea-agent.conf

@@ -97,3 +100,6 @@ featureGates:
 # the flow collector.
 # Flow export frequency should be greater than or equal to 1.
 #flowExportFrequency: 12
+
+# Provide the port range used by NodePortLocal for programming IPTABLES rules. This is only used if NodePortLocal is enabled.
+#nplPortRange: 40000=41000


Is it more common to use "-" as the connector of port range? https://github.com/kubernetes/kubernetes/blob/eca53507be3d9008ff6d4cb776c02733a1ea898b/cmd/kube-apiserver/app/options/options.go#L224
https://docs.docker.com/engine/reference/run/#expose-incoming-ports

tnqn · 2020-11-05T17:06:27Z

cmd/antrea-agent/agent.go

+	if features.DefaultFeatureGate.Enabled(features.NodePortLocal) {
+		err := npl.InitializeNPLAgent(k8sClient, informerFactory, o.config.NPLPortRange)
+		if err != nil {
+			klog.Warningf("Failed to start NPL agent: %v", err)


Should the process exit with error if it fails to initialize a requested feature?

Makes Sense. I'll add it.

tnqn · 2020-11-05T17:12:54Z

pkg/agent/nplagent/k8s/annotations.go

+type NPLEPAnnotation struct {
+	PodPort  string `json:"Podport"`
+	NodeIP   string `json:"Nodeip"`
+	NodePort string `json:"Nodeport"`


The json keys are normally in this style: podPort, nodeIP, nodePort.
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/core/v1/types.go#L61

I'll update it.

tnqn · 2020-11-05T17:14:19Z

pkg/agent/nplagent/k8s/annotations.go

+}
+
+func Stringify(serialize interface{}) string {
+	json_marshalled, _ := json.Marshal(serialize)


jsonMarshalled to keep golang style?

I'll change it.

tnqn · 2020-11-05T17:22:28Z

pkg/agent/nplagent/k8s/annotations.go

+
+// RemoveNPLAnnotationFromPods : Removes npl annotations from all pods
+func (c *Controller) RemoveNPLAnnotationFromPods() {
+	podList, err := c.KubeClient.CoreV1().Pods("").List(context.TODO(), metav1.ListOptions{})


This will be expensive, in large scale, a single such call will cause considerable overhead to kube-apiserver.
Currently antrea-agent only lists Pods of this Node once on start, and it uses "resourceVersion=0" in the options to avoid reading from etcd, which caused performance issue in scale clusters. See #1044 for details.
I think if it's one time job, it could reuse the result of https://github.com/vmware-tanzu/antrea/blob/714af4df1c826131075d4dc65d50f4c6f7049902/pkg/agent/cniserver/server.go#L606 which lists Pods of this Node only.

Yes, this is one time operation only. I'll follow your suggestion.

tnqn · 2020-11-05T17:27:56Z

pkg/agent/nplagent/k8s/annotations.go

+		klog.Infof("Removing all NPL annotation from pod: %s, ns: %s", pod.Name, pod.Namespace)
+		delete(podAnnotation, NPLAnnotationStr)
+		pod.Annotations = podAnnotation
+		c.KubeClient.CoreV1().Pods(pod.Namespace).Update(context.TODO(), &pod, metav1.UpdateOptions{})


Will there be problem if the update fails?

There will be a problem in that Case. I'll add a log here.

tnqn · 2020-11-05T17:29:07Z

pkg/agent/nplagent/k8s/annotations.go

+		klog.Warningf("Unable to update pod %s with annotation: %+v", pod.Name, err)
+		return err
+	}
+	klog.Infof("Successfully updated pod %s %s annotation", pod.Name, pod.Namespace)


Suggested change

klog.Infof("Successfully updated pod %s %s annotation", pod.Name, pod.Namespace)

klog.Infof("Successfully updated pod %s/%s annotation", pod.Namespace, pod.Name)

to be more readable and natural?

@tnqn I have updated incorporating the changes you suggested. Please let me know further comments for the other files.

monotosh-avi · 2020-11-12T15:11:13Z

@tnqn @antoninbas @jianjuns Let me know about the further changes required. After that, I'll raise another PR to add changes required for supporting service selector.

cmd/antrea-agent/agent.go

pkg/agent/nplagent/k8s/annotations.go

tnqn · 2020-11-12T15:42:25Z

pkg/agent/nplagent/k8s/annotations.go

+		current = pod.Annotations
+	}
+
+	klog.Infof("Building annotation for pod: %s\tport: %s --> %s:%s", pod.Name, containerPort, nodeIP, nodePort)


Is the log temporary for development or supposed to be V(0)? not sure if it's too verbose to be default level.

Sure changed to V(0)

pkg/agent/nplagent/k8s/annotations.go

tnqn · 2020-11-12T17:10:06Z

pkg/agent/nplagent/k8s/store.go

+	defer o.Unlock()
+	ports := []int32{}
+	if !o.IsPodInStore(pod.Name) {
+		o.podPortsMap[pod.Name] = ports


in the end of this function, the pod will be inserted to the map anyway, why do we insert an empty slice here?

tnqn · 2020-11-12T17:11:42Z

pkg/agent/nplagent/k8s/store.go

+		}
+	}
+	return false
+}


I didn't see the 3 methods in this file are used anywhere, is the code completed?

You are right. I was planning to use this when we do a sync during boot up. This is not required now. I have removed this file

tnqn · 2020-11-12T17:12:19Z

pkg/agent/nplagent/lib/constants.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package lib


could we add this file only when we need one?

sure removed

tnqn · 2020-11-12T17:12:41Z

pkg/agent/nplagent/lib/utils.go

+	"k8s.io/klog"
+)
+
+// HasElem : check if a slice contains an element


Suggested change

// HasElem : check if a slice contains an element

// HasElem checks if a slice contains an element.

tnqn · 2020-11-12T17:12:54Z

pkg/agent/nplagent/lib/utils.go

+	return false
+}
+
+// ParsePortsRange : parse port range and check if valid


monotosh-avi

Thanks a lot Quan for your detailed review. I have addressed most of the points except a few which I'm not sure about / planning to take care in next PR

monotosh-avi · 2020-11-13T12:14:11Z

pkg/agent/nplagent/k8s/annotations.go

+	})
+	if err != nil {
+		klog.Warningf("Unable to list Pods, err: %v", err)
+		return


We need this during boot up.
In the next PR, I'll add the change to compare existing iptbales rules with pod annotations and apply only required changes during boot up. Then we can remove this function.

monotosh-avi · 2020-11-14T04:07:30Z

pkg/agent/nplagent/lib/utils.go

+	return false
+}
+
+// ParsePortsRange : parse port range and check if valid


monotosh-avi · 2020-11-14T04:07:34Z

pkg/agent/nplagent/lib/utils.go

+	"k8s.io/klog"
+)
+
+// HasElem : check if a slice contains an element


monotosh-avi · 2020-11-14T04:08:05Z

pkg/agent/nplagent/lib/constants.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package lib


sure removed

monotosh-avi · 2020-11-14T04:12:16Z

pkg/agent/nplagent/k8s/store.go

+		}
+	}
+	return false
+}


You are right. I was planning to use this when we do a sync during boot up. This is not required now. I have removed this file

pkg/agent/nplagent/k8s/handlers.go

pkg/agent/nplagent/k8s/k8s_controller.go

monotosh-avi · 2020-11-14T06:49:28Z

pkg/agent/nplagent/k8s/store.go

+	objOnce.Do(func() {
+		objStore = ObjectStore{}
+	})
+	return &objStore


removed this file now

antoninbas

some more nits, otherwise LGTM

I think @tnqn should review as well before we merge

antoninbas · 2021-01-05T23:45:59Z

cmd/antrea-agent/config.go

@@ -120,4 +120,6 @@ type AgentConfig struct {
 	// Flow export frequency should be greater than or equal to 1.
 	// Defaults to "12".
 	FlowExportFrequency uint `yaml:"flowExportFrequency,omitempty"`
+	// Provide the port range used by NodePortLocal for programming IPTABLES rules. This is only used if NodePortLocal is enabled.


could we match the comment from build/yamls/base/conf/antrea-agent.conf?

antoninbas · 2021-01-05T23:47:10Z

pkg/agent/nodeportlocal/k8s/annotations.go

+		klog.Warningf("Unable to update Pod %s with annotation: %+v", pod.Name, err)
+		return err
+	}
+	klog.V(2).Infof("Successfully updated Pod %s/%s annotation", pod.Namespace, pod.Name)


Suggested change

klog.V(2).Infof("Successfully updated Pod %s/%s annotation", pod.Namespace, pod.Name)

klog.V(2).Infof("Successfully updated annotation for Pod %s/%s", pod.Namespace, pod.Name)

antoninbas · 2021-01-05T23:52:30Z

pkg/agent/nodeportlocal/k8s/controller.go

+			return
+		}
+	}
+	podKey := pod.Namespace + "/" + pod.Name


maybe define a local helper function for that, e.g. func podKeyFunc(pod *corev1.Pod) string

antoninbas · 2021-01-05T23:53:02Z

pkg/agent/nodeportlocal/k8s/controller.go

+	return nil
+}
+
+// HandleDeletePod Removes rules from port table and


s/Removes/removes

antoninbas · 2021-01-05T23:53:24Z

pkg/agent/nodeportlocal/k8s/controller.go

+			if !c.portTable.RuleExists(podIP, int(cport.ContainerPort)) {
+				err = c.addRuleForPod(newPod)
+				if err != nil {
+					return fmt.Errorf("Failed to add rule for Pod %s/%s: %s", newPod.Namespace, newPod.Name, err.Error())


s/Failed/failed for consistency

same below

antoninbas · 2021-01-05T23:53:39Z

pkg/agent/nodeportlocal/npl_agent_init.go

+const resyncPeriod = 60 * time.Minute
+
+// InitializeNPLAgent initializes the NodePortLocal (NPL) agent.
+// It initializes the port table cache to keep rack of Node ports available for use by NPL,


s/keep rack/keep track

antoninbas · 2021-01-05T23:55:20Z

pkg/agent/nodeportlocal/nplagent_test.go

+	var data string
+	var found bool
+
+	a := assert.New(t)


nit: I think the style should match for the assert & require assertions

so you can define r := require.New(t) as well, or switch to using assert.<Func>(t, ...)

antoninbas · 2021-01-05T23:55:47Z

pkg/agent/nodeportlocal/rules/iptable_rule.go

+	if err != nil {
+		return fmt.Errorf("IPTABLES rule creation failed for NPL with error: %v", err)
+	}
+	klog.Infof("successfully added rule for pod %s: %d", podip, port)


antoninbas · 2021-01-05T23:56:05Z

pkg/agent/nodeportlocal/rules/iptable_rule.go

+}
+
+// AddRule appends a DNAT rule in NodePortLocalChain chain of NAT table
+func (ipt *IPTableRules) AddRule(port int, podip string) error {


suggest renaming podip to podIP

antoninbas · 2021-01-06T00:03:31Z

pkg/agent/nodeportlocal/portcache/port_table.go

+	tableLock    sync.RWMutex
+}
+
+func NewPortTable(start, end int) (*PortTable, bool) {


idea for a future PR: we can do the same thing as kube-proxy for NodePorts and actually "hold" the port by binding a socket to it and keeping it alive

see https://github.com/kubernetes/kubernetes/blob/86f8c3ee91b6faec437f97e3991107747d7fc5e8/pkg/proxy/iptables/proxier.go#L1664

I know you had something like this originally, but IIRC you were just trying to bind to the port to check if it was available at that point in time, and not actually reserving the port indefinitely

Sure Antonin. I'll take care of this in next PR.

- Updated logs and comments - Added a check for existence of NPL chain before deleting all rules from iptables

build/yamls/base/conf/antrea-agent.conf

cmd/antrea-agent/config.go

cmd/antrea-agent/options.go

pkg/agent/nodeportlocal/k8s/annotations.go

pkg/agent/nodeportlocal/k8s/controller.go

pkg/agent/nodeportlocal/nplagent_test.go

tnqn · 2021-01-06T16:39:38Z

pkg/agent/nodeportlocal/portcache/port_table.go

+	return nil
+}
+
+func (pt *PortTable) getFreePort(podip string, podport int) int {


Suggested change

func (pt *PortTable) getFreePort(podip string, podport int) int {

func (pt *PortTable) getFreePort(podIP string, podPort int) int {

and so do below

pkg/agent/nodeportlocal/rules/rules.go

pkg/features/antrea_features.go

jianjuns · 2021-01-06T19:28:00Z

pkg/agent/nodeportlocal/k8s/handlers.go

-	podIP := pod.Status.PodIP
-	if podIP == "" {
-		klog.Infof("IP address not found for pod: %s/%s", pod.Namespace, pod.Name)
+	klog.Infof("Got delete event for pod: %s", key)


Do you want to generate an INFO log message for every Pod add/delete event? If not, consider adding a verbosity level, like klog.V(2).Infof().

Would it be fine for delete events to have logs ? @antoninbas had suggested this.

pkg/features/antrea_features.go

tnqn

LGTM, thanks for addressing my comments. One last comment left.

tnqn · 2021-01-08T05:55:50Z

pkg/agent/nodeportlocal/k8s/controller.go

+				}
+				assignPodAnnotation(newPod, newPod.Status.HostIP, port, nodePort)
+
+				if err != nil {


this is duplicate with L196?

Thanks, Corrected.

tnqn

LGTM

tnqn · 2021-01-08T07:38:47Z

/test-all

monotosh-avi · 2021-01-12T03:57:23Z

/jenkins-networkpolicy

monotosh-avi · 2021-01-12T03:59:54Z

/test-networkpolicy

This commit introduces NodePortLocal (NPL) in Antrea agent. With NPL, a Pod port can be directly reached from external network through a port of the Node on which the Pod is running. NPL programs IPTABLE rules to send incoming Node traffic to the target Pod port. This information is exposed through an annotation on the Pod object, for consumption by other entities, such as external load-balancers. An example of the pod annotation is given bellow: metadata: annotations: npl.k8s.io/endpoints: '[{"podport":"8080","nodeip":"10.102.47.229","nodeport":"40002"}]' To use this feature, following changes are required in the Antrea configuration: - Enable the "NodePortLocal" featureGate in the antrea-agent configuration - If desired, change the value of "nplPortRange" in the antrea-agent configuration When using this feature, the Antrea Agent need to mutate Pod objects, which is why access to the Pod API was added to the antrea-agent ClusterRole. Note: NPL agent is currently not supported in Windows platform Future changes: - Label based service filtering to select backend pods for NPL - Sync on start: compare programmed iptables rules with pod annotation during bootup and add/delete new rules in iptable only if required. Currently we are deleting all the rules and adding new rules for all the pods while booting up Antrea agent. - Add e2e test - Add documentation See #969 Signed-off-by: Hemant Shaw <[email protected]> Signed-off-by: Manu Dilip Shah <[email protected]> Signed-off-by: Monotosh Das <[email protected]> Signed-off-by: Shubham Chauhan <[email protected]> Signed-off-by: Sudipta Biswas <[email protected]>

Currently, we delete all the iptable rules when the antrea-agent is restarted. This commit does the following: - Evaluates the iptable rules in the Node Port Local chain and check if a rule is required. If a pod got deleted while the antrea-agent was restarting, we don't need that rule and has to be deleted. - The above evaluation logic also creates a cache for all the pods retrived from the iptable rules. - Evaluates the kubernetes Pods and checks if a rule has to be added for a Pod. Part of TODO as mentioned [here](antrea-io#1459).

Currently we delete all the iptable rules when the antrea-agent is restarted. This commit does the following: - Evaluates the iptable rules in the Node Port Local chain and check if a rule is required. If a Pod got deleted while the antrea-agent was restarting, we don't need that rule and has to be deleted. - The above evaluation logic also creates a cache for all the pods retrived from the iptable rules. - Evaluates the kubernetes Pods, and for each Pod, it checks if its part of the above cache. If yes, only the Pod's annotations needs to be verified. If not, a new Node port has to be allocated and a rule has to be added. One of the TODOs as mentioned [here](antrea-io#1459).

When the Antrea Agent restarts, instead of removing the NPL annotations from all Pods before starting the NPL Controller, we reconcile existing Pod annotations with iptables rules using iptables-restore. We also re-build the port table / port cache. We then start running the NPL Controller. If the Pod NPL annotations are no longer up-to-date (e.g. because a Service selector changed), it will be handled when the Controller is running, just like any normal update. This is a follow-up work item from #1459.

vmwclabot added the cla-not-required label Oct 29, 2020

monotosh-avi requested review from antoninbas and jianjuns October 29, 2020 16:55

antoninbas reviewed Oct 29, 2020

View reviewed changes

antoninbas requested a review from tnqn October 29, 2020 20:38

jianjuns reviewed Oct 29, 2020

View reviewed changes

pkg/agent/nplagent/bootstrap/npl_agent_init_windows.go Outdated Show resolved Hide resolved

jianjuns reviewed Oct 29, 2020

View reviewed changes

build/yamls/antrea.yml Outdated Show resolved Hide resolved

jianjuns reviewed Nov 3, 2020

View reviewed changes

monotosh-avi commented Nov 3, 2020

View reviewed changes

monotosh-avi force-pushed the monotosh-npl branch from 4a996b0 to 736bc8b Compare November 3, 2020 13:38

jianjuns reviewed Nov 3, 2020

View reviewed changes

monotosh-avi commented Nov 5, 2020

View reviewed changes

pkg/agent/nplagent/rules/iptable_rule.go Outdated Show resolved Hide resolved

tnqn reviewed Nov 5, 2020

View reviewed changes

antoninbas added this to the Antrea v0.12.0 release milestone Nov 5, 2020

monotosh-avi force-pushed the monotosh-npl branch 6 times, most recently from 5b01a1f to 327e9bf Compare November 12, 2020 10:38

tnqn reviewed Nov 12, 2020

View reviewed changes

monotosh-avi commented Nov 14, 2020

View reviewed changes

monotosh-avi force-pushed the monotosh-npl branch 2 times, most recently from 21490da to 78fc586 Compare November 17, 2020 03:47

monotosh-avi force-pushed the monotosh-npl branch from 0c10946 to 1aafc92 Compare January 5, 2021 05:46

antoninbas mentioned this pull request Jan 5, 2021

Rename Antrea APIs from *.antrea.tanzu.vmware.com to *.antrea.io #1715

Closed

antoninbas reviewed Jan 6, 2021

View reviewed changes

Update incorporating changes in NPL for review comments - part14

7252076

- Updated logs and comments - Added a check for existence of NPL chain before deleting all rules from iptables

monotosh-avi force-pushed the monotosh-npl branch from 1cdac8b to 7252076 Compare January 6, 2021 14:46

tnqn reviewed Jan 6, 2021

View reviewed changes

monotosh-avi force-pushed the monotosh-npl branch from 8505169 to af48ec9 Compare January 6, 2021 18:29

jianjuns reviewed Jan 6, 2021

View reviewed changes

pkg/features/antrea_features.go Outdated Show resolved Hide resolved

monotosh-avi force-pushed the monotosh-npl branch from af48ec9 to a947e41 Compare January 7, 2021 05:55

tnqn reviewed Jan 8, 2021

View reviewed changes

Update incorporating changes in NPL for review comments - part15

b7f2021

monotosh-avi force-pushed the monotosh-npl branch from a947e41 to b7f2021 Compare January 8, 2021 06:40

tnqn approved these changes Jan 8, 2021

View reviewed changes

antoninbas merged commit 90719c3 into antrea-io:master Jan 12, 2021

hemantavi mentioned this pull request Jan 18, 2021

Handle restartability for Node Port Local #1758

Merged

antoninbas mentioned this pull request Feb 9, 2021

NodePort Local in Antrea #969

Closed

		klog.Errorf("NodePortLocal is currently not supported in Windows Platform ")
		return errors.New("Windows Platform not supported fot NPL")

	klog.Infof("Successfully updated pod %s %s annotation", pod.Name, pod.Namespace)
	klog.Infof("Successfully updated pod %s/%s annotation", pod.Namespace, pod.Name)

	// HasElem : check if a slice contains an element
	// HasElem checks if a slice contains an element.

	klog.V(2).Infof("Successfully updated Pod %s/%s annotation", pod.Namespace, pod.Name)
	klog.V(2).Infof("Successfully updated annotation for Pod %s/%s", pod.Namespace, pod.Name)

	func (pt *PortTable) getFreePort(podip string, podport int) int {
	func (pt *PortTable) getFreePort(podIP string, podPort int) int {

Introducing NodePortLocal in Antrea Agent #1459

Introducing NodePortLocal in Antrea Agent #1459

Conversation

monotosh-avi commented Oct 29, 2020 • edited Loading

antrea-bot commented Oct 29, 2020

codecov-io commented Oct 29, 2020 • edited Loading

Codecov Report

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

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

jianjuns left a comment

Choose a reason for hiding this comment

monotosh-avi commented Oct 30, 2020

jianjuns 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

antrea-bot commented Nov 3, 2020

monotosh-avi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monotosh-avi left a comment

Choose a reason for hiding this comment

tnqn 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

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

monotosh-avi commented Nov 12, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monotosh-avi 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

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

monotosh-avi commented Oct 29, 2020 •

edited

Loading

codecov-io commented Oct 29, 2020 •

edited

Loading