Add controller to antrea-agent for implementing Egress #2026

tnqn · 2021-04-02T18:25:33Z

The controller watches Egress and EgressGroup API and calls openflow
client and route client to enforce an Egress.

Co-authored-by: ceclinux ceclinux@users.noreply.github.com

For #1924

tnqn · 2021-04-02T18:32:41Z

@jianjuns I haven't rebased on main branch and added any unit test. Create this PR for you to review the main code and the e2e code earlier. Please ignore changes other than the below files:

pkg/agent/controller/egress/egress_controller.go
test/e2e/egress_test.go

I will keep updating while you are reviewing.

jianjuns · 2021-04-02T23:41:55Z

pkg/agent/controller/egress/egress_controller.go

+		egressIPStates:       map[string]*egressIPState{},
+		egressBindings:       map[string]*egressBinding{},
+		localIPDetector:      &localIPDetector{},
+		idAllocator:          newIDAllocator(256),


Define a constant for the maximum number of SNAT IPs. I feel maybe 100 is good enough.

Will do. But since we allocate 8 bits for the mark, no harm to allow 255 IPs?

The only thing is it consumes more mem.

jianjuns · 2021-04-02T23:45:51Z

pkg/agent/controller/egress/egress_controller.go

+
+type localIPDetector struct{}
+
+func (d *localIPDetector) IsLocalIP(ip string) (bool, error) {


We can start from this, but later we should add periodic IP detection for the case SNAT IP is added after Egress is received.

Sure, I thought about it. We could use "ip monitor address" to watch the IP Address event so that we don't need to list addresses everytime we check if an IP is local or not, and don't need to do polling for IP change.
Will add a TODO if I can't make it in this release.

jianjuns · 2021-04-02T23:52:26Z

pkg/agent/controller/egress/egress_controller.go

+		}
+	}
+	// Ensure datapath is configured properly.
+	if !ipState.datapathInstalled {


So, datapathInstalled is just to catch the previous failures in flow/rule installation?

Yes, and to avoid extra external calls in later syncs.

jianjuns · 2021-04-02T23:53:23Z

pkg/agent/controller/egress/egress_controller.go

+		if err := c.ofClient.InstallSNATMarkFlows(ipState.egressIP, ipState.mark); err != nil {
+			return 0, fmt.Errorf("error installing SNAT mark flows for IP %s: %v", ipState.egressIP, err)
+		}
+		if err := c.routeClient.AddSNATRule(ipState.egressIP, ipState.mark); err != nil {


Is it a problem to call InstallSNATMarkFlows() again in retry, after AddSNATRule() fails?

I thought adding same openflow rule is idempotent, no? or I could have two flags to record state of snat rule and iptables rule separately.

OVS side should be idempotent. But how about our flow cache?

I think updating flow cache just overrides it so should be ok. Anyway I switched to 2 separate flags to not have assumption on the implementation.

jianjuns · 2021-04-02T23:55:44Z

pkg/agent/controller/egress/egress_controller.go

+
+// getOrAllocateMark gets or allocates a mark for a local Egress. Multiple Egresses can share same Egress IP.
+// The mark is per Egress IP, not per Egress.
+func (c *Controller) getOrAllocateMark(egressName, egressIP string) (uint32, error) {


This func also installs flows and iptables rule. Call it realizeEgress?

jianjuns · 2021-04-02T23:56:09Z

pkg/agent/controller/egress/egress_controller.go

+	return true
+}
+
+// getOrAllocateMark gets or allocates a mark for a local Egress. Multiple Egresses can share same Egress IP.


same -> the same

If we install flows/iptables rule in the func, add comments about these.

jianjuns · 2021-04-03T00:05:53Z

pkg/agent/controller/egress/egress_controller.go

+	}
+	// Ensure datapath is configured properly.
+	if !ipState.datapathInstalled {
+		if err := c.ofClient.InstallSNATMarkFlows(ipState.egressIP, ipState.mark); err != nil {


Could we install flows / rule in the caller with the lock released?

It may introduce extra external calls. For example, if two new Egresses have same Egress IP, and the first one doesn't configure the datapath with the lock, the other one will also try to configure the datapath, unless we introduce per-IP locks. However, even if we add per-IP locks, at least iptables calls will be serial anyway. Do you think it's worthwhile?

The per-IP lock will be necessary as there could be race condition when an Egress starts to use an EgressIP while another Egress stops to use the same EgressIP.

jianjuns · 2021-04-03T00:08:00Z

pkg/agent/controller/egress/egress_controller.go

+		return nil
+	}
+	if ipState.mark != 0 {
+		if err := c.ofClient.UninstallSNATMarkFlows(ipState.mark); err != nil {


Same here - can we release lock before uninstall flows/rule?

jianjuns · 2021-04-03T00:09:13Z

pkg/agent/controller/egress/egress_controller.go

+	return nil
+}
+
+func (c *Controller) getEgressState(egressName string) (*egressState, bool) {


I assume you have considered this, but just a remainder - make sure access to the returned EgressState without lock, will not create any sync issue.

Yes, I have considered this. The mutex is to protect the map, not the egressState item. The workqueue guarantees an Egress will only be processed by a single worker at any time. So the returned EgressState has no race condition. Maybe I could add this in the comment to be clear.

Comments will be good.

Added to the comment of the mutex.

tnqn

@jianjuns thanks for the review

tnqn · 2021-04-03T06:37:09Z

pkg/agent/controller/egress/egress_controller.go

+
+type localIPDetector struct{}
+
+func (d *localIPDetector) IsLocalIP(ip string) (bool, error) {


Sure, I thought about it. We could use "ip monitor address" to watch the IP Address event so that we don't need to list addresses everytime we check if an IP is local or not, and don't need to do polling for IP change.
Will add a TODO if I can't make it in this release.

tnqn · 2021-04-03T06:42:01Z

pkg/agent/controller/egress/egress_controller.go

+		egressIPStates:       map[string]*egressIPState{},
+		egressBindings:       map[string]*egressBinding{},
+		localIPDetector:      &localIPDetector{},
+		idAllocator:          newIDAllocator(256),


Will do. But since we allocate 8 bits for the mark, no harm to allow 255 IPs?

tnqn · 2021-04-03T06:42:25Z

pkg/agent/controller/egress/egress_controller.go

+	return true
+}
+
+// getOrAllocateMark gets or allocates a mark for a local Egress. Multiple Egresses can share same Egress IP.


tnqn · 2021-04-03T06:42:30Z

pkg/agent/controller/egress/egress_controller.go

+
+// getOrAllocateMark gets or allocates a mark for a local Egress. Multiple Egresses can share same Egress IP.
+// The mark is per Egress IP, not per Egress.
+func (c *Controller) getOrAllocateMark(egressName, egressIP string) (uint32, error) {


tnqn · 2021-04-03T14:42:05Z

pkg/agent/controller/egress/egress_controller.go

+		}
+	}
+	// Ensure datapath is configured properly.
+	if !ipState.datapathInstalled {


Yes, and to avoid extra external calls in later syncs.

tnqn · 2021-04-03T15:13:37Z

pkg/agent/controller/egress/egress_controller.go

+	}
+	// Ensure datapath is configured properly.
+	if !ipState.datapathInstalled {
+		if err := c.ofClient.InstallSNATMarkFlows(ipState.egressIP, ipState.mark); err != nil {


It may introduce extra external calls. For example, if two new Egresses have same Egress IP, and the first one doesn't configure the datapath with the lock, the other one will also try to configure the datapath, unless we introduce per-IP locks. However, even if we add per-IP locks, at least iptables calls will be serial anyway. Do you think it's worthwhile?

tnqn · 2021-04-03T15:16:46Z

pkg/agent/controller/egress/egress_controller.go

+		if err := c.ofClient.InstallSNATMarkFlows(ipState.egressIP, ipState.mark); err != nil {
+			return 0, fmt.Errorf("error installing SNAT mark flows for IP %s: %v", ipState.egressIP, err)
+		}
+		if err := c.routeClient.AddSNATRule(ipState.egressIP, ipState.mark); err != nil {


I thought adding same openflow rule is idempotent, no? or I could have two flags to record state of snat rule and iptables rule separately.

tnqn · 2021-04-03T15:17:44Z

pkg/agent/controller/egress/egress_controller.go

+		return nil
+	}
+	if ipState.mark != 0 {
+		if err := c.ofClient.UninstallSNATMarkFlows(ipState.mark); err != nil {


tnqn · 2021-04-03T15:28:38Z

pkg/agent/controller/egress/egress_controller.go

+	return nil
+}
+
+func (c *Controller) getEgressState(egressName string) (*egressState, bool) {


Yes, I have considered this. The mutex is to protect the map, not the egressState item. The workqueue guarantees an Egress will only be processed by a single worker at any time. So the returned EgressState has no race condition. Maybe I could add this in the comment to be clear.

jianjuns · 2021-04-05T16:43:23Z

pkg/agent/controller/egress/local_ip_detector.go

+type localIPDetector struct{}
+
+// IsLocalIP checks if the provided IP is configured on the Node.
+// TODO: Instead of listing all IP addresses every time, it can maintain a cache and subscribe the IP address change to


We need notification to controller for changes too, so controller can handle IP add/delete and update DP correctly.

I found golang API to watch ip address, will update this interface soon.

jianjuns · 2021-04-05T16:44:23Z

pkg/agent/controller/egress/egress_controller.go

+	// Disable resyncing.
+	resyncPeriod time.Duration = 0
+	// maxEgressIPsPerNode is the maximum number of Egress IPs can be configured on a Node.
+	maxEgressIPsPerNode = 256


I think 0 is for default IP, so the max number should be 255?

Thanks, will fix. And switch to a lazy initialization mode to save memory

jianjuns · 2021-04-05T16:49:54Z

pkg/agent/controller/egress/egress_controller.go

+	return nil
+}
+
+func (c *Controller) getEgressState(egressName string) (*egressState, bool) {


Comments will be good.

tnqn · 2021-04-05T18:12:38Z

/test-all

jianjuns · 2021-04-06T16:14:04Z

@tnqn : is this PR ready for final review and merge (it still has WIP in title)?

tnqn · 2021-04-06T16:38:33Z

@jianjuns It's ready for review. I have made all changes except adding the verification in CRD schema. I have to add a simple one to enable e2e tests. Do you want me to add a comprehensive one in this PR? or you want to have another PR?

jianjuns · 2021-04-06T16:40:32Z

We can do another PR.

tnqn · 2021-04-06T16:56:23Z

/test-all

pkg/agent/controller/egress/local_ip_detector_linux.go

codecov-io · 2021-04-07T03:21:35Z

Codecov Report

Merging #2026 (2926972) into main (5752f96) will decrease coverage by 6.35%.
The diff coverage is 37.62%.

@@            Coverage Diff             @@
##             main    #2026      +/-   ##
==========================================
- Coverage   61.57%   55.22%   -6.36%     
==========================================
  Files         265      268       +3     
  Lines       19770    20177     +407     
==========================================
- Hits        12174    11142    -1032     
- Misses       6333     7850    +1517     
+ Partials     1263     1185      -78

Flag	Coverage Δ
kind-e2e-tests	`41.20% <0.00%> (-11.67%)`	⬇️
unit-tests	`41.58% <37.62%> (-0.13%)`	⬇️

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

Impacted Files	Coverage Δ
...agent/controller/egress/local_ip_detector_linux.go	`0.00% <0.00%> (ø)`
pkg/features/antrea_features.go	`16.66% <ø> (ø)`
pkg/agent/controller/egress/egress_controller.go	`40.79% <40.79%> (ø)`
pkg/agent/controller/egress/id_allocator.go	`100.00% <100.00%> (ø)`
pkg/apis/controlplane/helper.go	`25.00% <0.00%> (-75.00%)`	⬇️
pkg/apis/controlplane/v1beta2/helper.go	`25.00% <0.00%> (-75.00%)`	⬇️
pkg/controller/networkpolicy/mutate.go	`0.00% <0.00%> (-71.77%)`	⬇️
pkg/controller/networkpolicy/store/group.go	`5.00% <0.00%> (-70.00%)`	⬇️
pkg/controller/networkpolicy/validate.go	`0.00% <0.00%> (-69.47%)`	⬇️
pkg/k8s/name.go	`33.33% <0.00%> (-66.67%)`	⬇️
... and 67 more

test/e2e/egress_test.go

The controller watches Egress and EgressGroup API and calls openflow client and route client to enforce an Egress. Co-authored-by: ceclinux <ceclinux@users.noreply.github.com>

tnqn · 2021-04-07T10:52:10Z

/test-all

tnqn · 2021-04-07T10:55:07Z

@jianjuns I verified the feature on IPv6 cluster and it worked. Will make the e2e test support IPv6 with another PR.

tnqn · 2021-04-07T14:37:49Z

/test-all

jianjuns

Good to know IPv6 works too!

vmwclabot added the cla-not-required label Apr 2, 2021

tnqn force-pushed the egress-agent branch from 9e4c4d9 to 989c51d Compare April 2, 2021 18:29

jianjuns reviewed Apr 3, 2021

View reviewed changes

tnqn commented Apr 3, 2021

View reviewed changes

tnqn force-pushed the egress-agent branch from 989c51d to 4023740 Compare April 5, 2021 16:06

jianjuns reviewed Apr 5, 2021

View reviewed changes

tnqn force-pushed the egress-agent branch 3 times, most recently from 766c3bf to 5fdb6d2 Compare April 5, 2021 18:11

tnqn force-pushed the egress-agent branch from 5fdb6d2 to d2fba79 Compare April 6, 2021 15:57

tnqn force-pushed the egress-agent branch from d2fba79 to c9b08b1 Compare April 6, 2021 16:31

tnqn changed the title ~~WIP: Add controller to antrea-agent for implementing Egress~~ Add controller to antrea-agent for implementing Egress Apr 6, 2021

jianjuns reviewed Apr 6, 2021

View reviewed changes

pkg/agent/controller/egress/local_ip_detector_linux.go Outdated Show resolved Hide resolved

pkg/agent/controller/egress/local_ip_detector_linux.go Show resolved Hide resolved

jianjuns previously approved these changes Apr 7, 2021

View reviewed changes

test/e2e/egress_test.go Show resolved Hide resolved

Add controller to antrea-agent for implementing Egress

a95af83

The controller watches Egress and EgressGroup API and calls openflow client and route client to enforce an Egress. Co-authored-by: ceclinux <ceclinux@users.noreply.github.com>

tnqn dismissed jianjuns’s stale review via a95af83 April 7, 2021 09:24

tnqn force-pushed the egress-agent branch from 7e9b1c6 to a95af83 Compare April 7, 2021 09:24

This was referenced Apr 7, 2021

Egress Policy v1alpha1 implementation #1924

Closed

[WIP] Egress policy #1987

Closed

Improve CRD schema for Egress feature

2926972

jianjuns approved these changes Apr 7, 2021

View reviewed changes

tnqn merged commit e36c4f2 into antrea-io:main Apr 7, 2021


		type localIPDetector struct{}

		func (d *localIPDetector) IsLocalIP(ip string) (bool, error) {

Add controller to antrea-agent for implementing Egress #2026

Add controller to antrea-agent for implementing Egress #2026

Conversation

tnqn commented Apr 2, 2021 • edited

tnqn commented Apr 2, 2021

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

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

tnqn commented Apr 5, 2021

jianjuns commented Apr 6, 2021

tnqn commented Apr 6, 2021

jianjuns commented Apr 6, 2021

tnqn commented Apr 6, 2021

codecov-io commented Apr 7, 2021 • edited

Codecov Report

tnqn commented Apr 7, 2021

tnqn commented Apr 7, 2021

tnqn commented Apr 7, 2021

jianjuns left a comment

Choose a reason for hiding this comment

tnqn commented Apr 2, 2021 •

edited

codecov-io commented Apr 7, 2021 •

edited