Implement Antrea proxy #772

weiqiangt · 2020-05-31T16:25:03Z

Implement Antrea proxy for handling explicit ClusterIP services in OVS instead of using kube-proxy.
Add unit tests and e2e tests for Antrea proxy.
Add AntreaProxy option in FeatureGate for enabling Antrea proxy in the antrea agent. The default value for Linux is false while a Windows one is true.
Enhance kind tests for running with Antrea proxy.
Fix Windows service network policy not work issue.

Fixes #463.
Signed-off-by: Weiqiang TANG [email protected]

antrea-bot · 2020-05-31T16:25:24Z

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-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-all: to trigger all tests.
/skip-all: to skip all tests.

These commands can only be run by members of the vmware-tanzu organization.

weiqiangt · 2020-06-01T06:15:54Z

/test-conformance

weiqiangt · 2020-06-01T08:27:10Z

/test-networkpolicy

weiqiangt · 2020-06-07T09:00:12Z

/test-windows-conformance

weiqiangt · 2020-06-09T10:03:26Z

/test-all

weiqiangt · 2020-06-10T01:55:09Z

/test-all

antoninbas · 2020-06-11T19:38:50Z

@weiqiangt @jianjuns just wanted to check if the plan was to enable this mode unconditionally starting with the next Antrea release (0.8.0)? Or have a config switch (temporarily or permanently) to enable / disable the feature? Personally I am fine with enabling unconditionally, I don't if a user would have as a requirement that kube-proxy must be used.

This is relevant to @srikartati who is working on conntrack polling for IPFix flow information export.

srikartati · 2020-06-11T20:44:56Z

This is relevant to @srikartati who is working on conntrack polling for IPFix flow information export.

Just to be specific.. looking at supporting Pod-to-Service flows/connections as IPFIX flow records.

jianjuns · 2020-06-11T21:19:56Z

@weiqiangt @jianjuns just wanted to check if the plan was to enable this mode unconditionally starting with the next Antrea release (0.8.0)? Or have a config switch (temporarily or permanently) to enable / disable the feature? Personally I am fine with enabling unconditionally, I don't if a user would have as a requirement that kube-proxy must be used.

I suggest to add a config option, and maybe keep the default to upstream kube-proxy for Linux nodes, until we are confident on the quality and have enough tests.

jianjuns

Just some suggestions about comments improvement.

jianjuns · 2020-06-29T21:14:13Z

build/yamls/antrea.yml

+    # Enable antrea proxy which provides ServiceLB for in-cluster services in antrea agent.
+    # It should be enabled on Windows, otherwise NetworkPolicy will not take effect on
+    # Service traffic.
+    #  AntreaProxy: false


I feel ok to add relatively mature features to the yamls but not all features. Creating a separate doc sounds good.

jianjuns · 2020-06-29T21:52:17Z

pkg/agent/proxy/proxier.go

+	serviceConfig   *config.ServiceConfig
+	// endpointsChanges and serviceChanges contains all changes to endpoints and
+	// services that happened since last syncProxyRules call. For a single object,
+	// changes are accumulated, i.e. previous is state from before all of them,


Maybe you should add the comments about previous/current to endpointsChanges struct or endpointsChangesTracker funcs. Hard to understand from here.

jianjuns · 2020-06-29T21:55:22Z

pkg/agent/proxy/endpoints.go

+	k8sproxy "github.com/vmware-tanzu/antrea/third_party/proxy"
+)
+
+type endpointsChange struct {


I think you should add more comments to this file for structs and funcs. Not easy to understand the code without enough comments.

jianjuns · 2020-06-29T21:58:03Z

pkg/agent/proxy/proxier.go

+	endpointsConfig *config.EndpointsConfig
+	serviceConfig   *config.ServiceConfig
+	// endpointsChanges and serviceChanges contains all changes to endpoints and
+	// services that happened since last syncProxyRules call. For a single object,


I think it would be much better if you can add some comments to explain the event sequence before syncProxyRules.

lzhecheng · 2020-06-30T00:15:39Z

/test-windows-networkpolicy

weiqiangt · 2020-06-30T03:29:03Z

/test-all
/test-windows-networkpolicy

weiqiangt · 2020-06-30T03:44:17Z

/test-networkpolicy

tnqn · 2020-06-30T05:03:31Z

@weiqiangt the PR description about enableProxy is out of date, could you update?

weiqiangt · 2020-06-30T07:48:38Z

/test-all
/test-windows-networkpolicy

lzhecheng · 2020-06-30T10:58:33Z

/test-all

lzhecheng · 2020-06-30T11:47:38Z

/test-conformance
/test-e2e
/test-networkpolicy
/test-windows-networkpolicy

lzhecheng · 2020-06-30T12:17:57Z

/test-e2e
/test-conformance
/test-networkpolicy

antoninbas

Some small comments, mostly because of some typos in recently-added comments in the code. Otherwise LGTM. Glad that this is ready!

antoninbas · 2020-06-30T20:57:38Z

pkg/agent/openflow/pipeline.go

+		endpointIP := net.ParseIP(endpoint.IP()).To4()
+		ipVal := binary.BigEndian.Uint32(endpointIP)
+		portVal := uint16(endpointPort)
+		// TODO: use goto_table instead.


maybe a comment to this effect would have been nice

antoninbas · 2020-06-30T20:57:55Z

pkg/agent/openflow/client.go

+	// affinityTimeout is not zero, it also installs the flow which has a learn
+	// action to maintain the LB decision.
+	// The group with the groupID must be installed before, otherwise the
+	// installation will be failed.


s/will be failed/will fail

antoninbas · 2020-06-30T20:58:40Z

pkg/agent/proxy/endpoints.go

+
+// endpointsChangesTracker tracks Endpoints changes.
+type endpointsChangesTracker struct {
+	// hostname is used to tell whether the Endpoint locates on current Node.


s/locates/is located

antoninbas · 2020-06-30T20:59:04Z

pkg/agent/proxy/endpoints.go

+
+// OnEndpointUpdate updates given Service's Endpoints change map based on the
+// <previous, current> Endpoints pair. It returns true if items changed,
+// otherwise return false.


s/return false/it returns false

antoninbas · 2020-06-30T21:00:06Z

pkg/agent/proxy/endpoints.go

+}
+
+// endpointsToEndpointsMap translates single Endpoints object to EndpointsMap.
+// This function is used for incremental updated of EndpointsMap.


s/updated/update ?

antoninbas · 2020-06-30T21:01:10Z

pkg/agent/proxy/proxier.go

+	// endpointsChanges and serviceChanges contains all changes to endpoints and
+	// services that happened since last syncProxyRules call. For a single object,
+	// changes are accumulated. Once both endpointsChanges and serviceChanges
+	// had been synced, syncProxyRules will start syncing rules to the OVS.


Suggested change

// had been synced, syncProxyRules will start syncing rules to the OVS.

// have been synced, syncProxyRules will start syncing rules to OVS.

antoninbas · 2020-06-30T21:01:45Z

pkg/agent/proxy/proxier.go

+	serviceChanges   *serviceChangesTracker
+	// syncProxyRulesMutex protects internal caches and states.
+	syncProxyRulesMutex sync.Mutex
+	// serviceMap stores services we expected to be installed.


antoninbas · 2020-06-30T21:01:52Z

pkg/agent/proxy/proxier.go

+	serviceMap k8sproxy.ServiceMap
+	// serviceInstalledMap stores services we actually installed.
+	serviceInstalledMap k8sproxy.ServiceMap
+	// endpointsMap stores endpoints we expected to be installed.


antoninbas · 2020-06-30T21:03:31Z

test/e2e/proxy_test.go

+	}
+	defer teardownTest(t, data)
+
+	if enabled, err := proxyEnabled(data); err != nil {


maybe define skipIfProxyDisabled(t, data) in this file to reduce verbosity of this test and the subsequent ones

antoninbas · 2020-06-30T21:12:02Z

test/e2e/proxy_test.go

+)
+
+func proxyEnabled(data *TestData) (bool, error) {
+	key := "resubmit(,40),resubmit(,41)"


as a follow-up to an earlier comment I made, there is already a GetAntreaConfigMap method defined for e2e tests, but I am not sure it's a much better solution than what you are doing at the moment

If we use the configmap method, then we will need to parse the config.
For now, I prefer to keep the current code.
In the future, as we may add more experimental features, we can have a particular function to retrieve featuregayte options in e2e tests. How do you think about this?

Sounds good

jianjuns · 2020-06-30T23:45:31Z

pkg/agent/proxy/endpoints.go

+	return endpointsMap
+}
+
+// Update updates an EndpointsMap base on current changes and returns stale


base -> based

Co-authored-by: Quan Tian <[email protected]> Signed-off-by: Weiqiang TANG <[email protected]>

weiqiangt · 2020-07-01T01:14:25Z

/test-all
/test-windows-networkpolicy

antoninbas

LGTM, I believe all my comments have been addressed

lzhecheng · 2020-07-01T01:53:18Z

/test-conformance
/test-windows-networkpolicy

lzhecheng · 2020-07-01T02:27:04Z

/test-windows-networkpolicy

tnqn

All comments should have been addressed now, I'm going to merge this after remaining tests pass to give other feature developers to address the conflicts, which might need some efforts given all of them reply on openflow implementation..

Signed-off-by: Weiqiang TANG <[email protected]>

vmwclabot added the cla-not-required label May 31, 2020

weiqiangt force-pushed the weiqiangt/antrea-proxy branch 3 times, most recently from 296b664 to d522aa7 Compare June 5, 2020 06:38

weiqiangt force-pushed the weiqiangt/antrea-proxy branch 5 times, most recently from 7fb5813 to a33d19c Compare June 9, 2020 08:17

weiqiangt changed the title ~~[WIP] ANTREA PROXY~~ Implement Antrea proxy Jun 9, 2020

weiqiangt marked this pull request as ready for review June 9, 2020 08:18

weiqiangt force-pushed the weiqiangt/antrea-proxy branch from a33d19c to c207764 Compare June 9, 2020 08:21

weiqiangt linked an issue Jun 9, 2020 that may be closed by this pull request

Provide service loadbalancing function to Pods via openflow in antrea-agent #463

Closed

3 tasks

weiqiangt force-pushed the weiqiangt/antrea-proxy branch from c207764 to 042161e Compare June 9, 2020 09:59

weiqiangt force-pushed the weiqiangt/antrea-proxy branch from 042161e to 14db0c5 Compare June 9, 2020 14:13

moshe010 mentioned this pull request Jun 12, 2020

Support Ovs offload #786

Merged

weiqiangt force-pushed the weiqiangt/antrea-proxy branch 5 times, most recently from 5b55336 to c97d15a Compare June 15, 2020 05:17

weiqiangt force-pushed the weiqiangt/antrea-proxy branch from a0f6639 to 1f7353d Compare June 29, 2020 17:45

jianjuns reviewed Jun 29, 2020

View reviewed changes

weiqiangt force-pushed the weiqiangt/antrea-proxy branch from 1f7353d to d82e51e Compare June 30, 2020 03:27

weiqiangt force-pushed the weiqiangt/antrea-proxy branch from d82e51e to c7ab11b Compare June 30, 2020 04:08

tnqn previously approved these changes Jun 30, 2020

View reviewed changes

antoninbas previously approved these changes Jun 30, 2020

View reviewed changes

jianjuns reviewed Jun 30, 2020

View reviewed changes

Implement antrea proxy

6a82265

Co-authored-by: Quan Tian <[email protected]> Signed-off-by: Weiqiang TANG <[email protected]>

weiqiangt dismissed stale reviews from antoninbas and tnqn via 6a82265 July 1, 2020 01:11

weiqiangt force-pushed the weiqiangt/antrea-proxy branch from c7ab11b to 6a82265 Compare July 1, 2020 01:11

weiqiangt closed this Jul 1, 2020

weiqiangt reopened this Jul 1, 2020

antoninbas approved these changes Jul 1, 2020

View reviewed changes

tnqn approved these changes Jul 1, 2020

View reviewed changes

tnqn merged commit c7d34f8 into antrea-io:master Jul 1, 2020

GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Sep 22, 2020

Implement antrea proxy (antrea-io#772)

534b4b9

Signed-off-by: Weiqiang TANG <[email protected]>

GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Sep 23, 2020

Implement antrea proxy (antrea-io#772)

67493bc

Signed-off-by: Weiqiang TANG <[email protected]>

weiqiangt deleted the weiqiangt/antrea-proxy branch December 24, 2020 06:41

	// had been synced, syncProxyRules will start syncing rules to the OVS.
	// have been synced, syncProxyRules will start syncing rules to OVS.

Implement Antrea proxy #772

Implement Antrea proxy #772

Conversation

weiqiangt commented May 31, 2020 • edited Loading

antrea-bot commented May 31, 2020

weiqiangt commented Jun 1, 2020

weiqiangt commented Jun 1, 2020

weiqiangt commented Jun 7, 2020

weiqiangt commented Jun 9, 2020

weiqiangt commented Jun 10, 2020

antoninbas commented Jun 11, 2020

srikartati commented Jun 11, 2020

jianjuns commented Jun 11, 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

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

lzhecheng commented Jun 30, 2020

weiqiangt commented Jun 30, 2020

weiqiangt commented Jun 30, 2020

tnqn commented Jun 30, 2020

weiqiangt commented Jun 30, 2020

lzhecheng commented Jun 30, 2020

lzhecheng commented Jun 30, 2020

lzhecheng commented Jun 30, 2020

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

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

weiqiangt commented Jul 1, 2020

antoninbas left a comment

Choose a reason for hiding this comment

lzhecheng commented Jul 1, 2020

lzhecheng commented Jul 1, 2020

tnqn left a comment

Choose a reason for hiding this comment

weiqiangt commented May 31, 2020 •

edited

Loading