Enable IPSec encryption of tunnel traffic #209

jianjuns · 2019-12-09T02:31:47Z

Create a separate OVS tunnel port for each remote Node.
Change L3 forwarding flows to support outputting to the IPSec tunnel port.
Add yamls to enable IPSec encryption and add a new container in the Agent DS to run IPSec daemons.

Will add e2e tests in the same PR.

antrea-bot · 2019-12-09T02:31:49Z

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

jianjuns · 2019-12-09T02:32:52Z

@wenyingd: I changed the L3 and L2 tables as we discussed offline. Please check.

build/yamls/patches/ipsec/ipsecContainer.yml

pkg/agent/openflow/client.go

pkg/agent/agent.go

wenyingd · 2019-12-09T03:49:56Z

pkg/agent/openflow/client_test.go

@@ -37,7 +37,7 @@ func installNodeFlows(ofClient Client, cacheKey string) (int, error) {
 	gwMAC, _ := net.ParseMAC("AA:BB:CC:DD:EE:FF")
 	IP, IPNet, _ := net.ParseCIDR("10.0.1.1/24")
 	peerNodeIP := net.ParseIP("192.168.1.1")
-	err := ofClient.InstallNodeFlows(hostName, gwMAC, IP, *IPNet, peerNodeIP)
+	err := ofClient.InstallNodeFlows(hostName, gwMAC, IP, *IPNet, peerNodeIP, 0)


If you just want to not block original UT, you could use types.DefaultTunOFPort for parameter of ofport, or the case might fail.

Seems the test does not fail with 0. But I could change to types.DefaultTunOFPort.

hack/generate-manifest.sh

antrea-bot · 2019-12-19T07:10:19Z

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

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

jianjuns · 2019-12-20T02:12:20Z

I still plan to add e2e tests before merging the PR, but the current comments are ready for review.

jianjuns · 2019-12-22T07:07:01Z

/test-e2e

jianjuns · 2019-12-23T07:14:29Z

/test-e2e

pkg/agent/agent.go

pkg/agent/controller/noderoute/node_route_controller.go

tnqn · 2019-12-23T14:43:47Z

pkg/agent/controller/noderoute/node_route_controller.go

@@ -45,6 +48,8 @@ const (
 	maxRetryDelay = 300 * time.Second
 	// Default number of workers processing a node change
 	defaultWorkers = 4
+
+	ovsExternalIDNodeName = "node-name"


Now ovs external ID constants are spread in files, so do ParseXXXInterfaceConfig. Do you think we could put them together and closer to interfaceCache?

Then where you suggest to put them (and buildXXXExternalIDs)? I do not think externalIDs belong to InterfaceStore, which is just a data store for interface data, and should not know anything about OVS.

No strong opinion, just felt a place having all external ID constants and the parsing functions might be convenient to know what data antrea persist in ovsdb and save class like AgentInitializer to depend on all packages that have parsing function. I didn't mean putting them in interfaceCache, but a separate package or file closer to interfaceCache as they are the things stored and read in interfaceCache.

I personally do not like to create too many small pkgs, but do not have a good idea which existing pkg for them.

I was considering a separate file in pkg/agent/interfacestore. But I'm not sure if it makes sense in other perspectives like the interface scope you mentioned. I'm fine with keeping them as they are until we are clear that the other way were more appropriate.

pkg/agent/interfacestore/types.go

tnqn · 2019-12-23T14:49:32Z

pkg/agent/util/net.go

+// multiple calls to GenerateContainerInterfaceName with the same parameters
+// return the same value). The output has the length of interfaceNameLength(15).
+// The probability of collision should be neglectable.
+func GenerateContainerInterfaceName(podName string, podNamespace string) string {


Would it cause existing interface names change? Would upgrading be affected?

Maybe I should first ask which format you like.

To handle upgrade, we could change to use Pod name + Namespace name to be the key.

Would not supporting upgrade make users hard to use new features and get bugs in their clusters fixed? I think handling upgrade in code to consider previously used key might be complicated and hard to describe the compatibility in doc when we want to remove it. Do you think we can just use major release number to indicate compatibility like some software do? If it's 0.2.0 to 0.3.0, maybe we should avoid changes that break compatibility and leave it to major release change if it's necessary.

I think it depends on how many deployments will use 0.2 and 0.1 and would expect upgrade without impact to existing Pods. If we want to make sure nothing breaks across versions, guess we need to add upgrade tests.

But for this change there is another way to handle it - if we believe not safe to use the generated interface name as the key we could instead use Pod + NS or Node name to be the key for InterfaceStore.

Agree it's ok to ignore existing Pods at this stage.
If it's just changing key of InterfaceStore, I think it doesn't break compatibility? My concern was GenerateContainerInterfaceName is used in setupInterfaces for host veth name, not sure what would happen if it changes. Anyway, I agree it's fine to not handle upgrade case until we declare it and test it.

Thought over. Maybe let me revert the name change in this PR, and we could make a separate one to change it.
Do you support the new format I used?

Is the difference keeping "-" in the name and different hashing value? For the former, I'm not sure why @wenyingd removed it and whether it's related to compatibility when names contain "-". If not, I'm fine with both.
@wenyingd could you confirm?

I restored the original format.

jianjuns · 2019-12-24T05:21:43Z

/test-e2e

For each remote Node, create a separate tunnel port and interface on the OVS bridge, and set the remote IP and PSK options to the interface. Refactore InterfaceStore structs and funcs to add tunnel interface attributes and group InterfaceConfig fields by interface types. For packets to a tunnel, change the l3Forwarding table flow to set the output port to the tunnel interface, and resubmit to the l2ForwardingOutput table (bypassing the l2ForwardingCalcTable and the ingress rules tables). When IPSec is enabled, for every remote Node set the output port to the Node's tunnel interface, and also add a separate classification flow in the in port classification table for the tunnel.

Change Makefile to generate antrea-ipsec.yml that deploys Antrea with the IPSec encryption enabled. Add a config option to antrea-agent.conf for enabling IPSec encryption. Add yaml patches for creating a K8s Secret to save IKE PSK and adding an environment variable to the antrea-agent container to pass the Secret value.

jianjuns · 2019-12-24T06:41:37Z

/test-e2e

jianjuns · 2019-12-24T07:28:46Z

/test-e2e

tnqn

LGTM, just a question about duplicate error logs.

tnqn · 2019-12-24T14:25:42Z

pkg/agent/controller/noderoute/node_route_controller.go

+		if err := c.ovsBridgeClient.DeletePort(interfaceConfig.PortUUID); err != nil {
+			klog.Errorf("Failed to delete OVS tunnel port %s for Node %s: %v",
+				interfaceConfig.InterfaceName, nodeName, err)
+			return fmt.Errorf("failed to delete OVS tunnel port for Node %s", nodeName)


This is not unified before, but want to discuss it here, finally these errors will be logged in processNextWorkItem:

klog.Errorf("Error syncing Node %s, requeuing. Error: %v", key, err)

Do you think we can remove all klog.Errorf to avoid duplicate error logs? I'm thinking maybe we should only use klog.Errorf when the code is going to swallow the error and not pop up it, what do you think?

Yes, I thought about it too. Just feel might be easier to debug if log closer to where the failure happens, and probably log with more details there.
For example, if we do not log here, should we always append the previously returned error to the new error and return to the caller?

Yes, if not log here, previously returned error should be appended, like L246 (I think it's the most common way to return error in Go). That would guarantee no information will be missing. I raised it because there were only little difference between the error logs and the returned errors, while there should be no reason to not include details in returned errors too.

Ok. Here I still feel easier to debug if log closer to the failure. But no strong opinion. If you believe we should always log in the upper layer, I could change.

Just personal opinion, not strong too. Feel feel to ignore.

jianjuns · 2019-12-24T19:08:24Z

/test-conformance

jianjuns · 2019-12-24T21:23:33Z

/test-e2e

edwardbadboy · 2019-12-25T02:10:33Z

/test-conformance
/test-networkpolicy

edwardbadboy · 2019-12-25T06:48:35Z

/test-e2e

edwardbadboy · 2019-12-25T07:01:16Z

/test-e2e

Add a test that redeloys Antrea with IPSec tunnel enabled, and tests connectivity between two Pods on two different Nodes.

jianjuns · 2019-12-25T22:26:10Z

/test-e2e

tnqn · 2019-12-26T05:30:25Z

/test-all

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

jianjuns requested a review from wenyingd December 9, 2019 02:32

jianjuns force-pushed the ipsec branch from cc7e264 to 8cdabbf Compare December 9, 2019 02:35

jianjuns commented Dec 9, 2019

View reviewed changes

build/yamls/patches/ipsec/ipsecContainer.yml Outdated Show resolved Hide resolved

wenyingd reviewed Dec 9, 2019

View reviewed changes

jianjuns force-pushed the ipsec branch 2 times, most recently from 2c95a45 to f812877 Compare December 10, 2019 21:45

antoninbas reviewed Dec 12, 2019

View reviewed changes

hack/generate-manifest.sh Outdated Show resolved Hide resolved

wenyingd mentioned this pull request Dec 13, 2019

Add documentation for Antrea's OVS pipeline #206

Merged

jianjuns force-pushed the ipsec branch 4 times, most recently from c5ba8ec to a83fd04 Compare December 19, 2019 07:10

jianjuns changed the title ~~WIP: Enable IPSec encryption of tunnel traffic~~ Enable IPSec encryption of tunnel traffic Dec 20, 2019

jianjuns force-pushed the ipsec branch from a83fd04 to 615c121 Compare December 20, 2019 02:07

jianjuns force-pushed the ipsec branch 2 times, most recently from 57be6d7 to c81ce7c Compare December 22, 2019 07:06

jianjuns force-pushed the ipsec branch from c81ce7c to 9f3ac6a Compare December 23, 2019 07:13

tnqn reviewed Dec 23, 2019

View reviewed changes

jianjuns force-pushed the ipsec branch from 9f3ac6a to 6631277 Compare December 24, 2019 05:00

jianjuns added 2 commits December 23, 2019 22:21

jianjuns force-pushed the ipsec branch from 6631277 to 9fb1d48 Compare December 24, 2019 06:22

jianjuns force-pushed the ipsec branch from 9fb1d48 to d827a3f Compare December 24, 2019 06:50

tnqn reviewed Dec 24, 2019

View reviewed changes

jianjuns force-pushed the ipsec branch from d827a3f to e8be4c8 Compare December 25, 2019 22:06

Add end-to-end tests for IPSec tunnel

cab626f

Add a test that redeloys Antrea with IPSec tunnel enabled, and tests connectivity between two Pods on two different Nodes.

jianjuns force-pushed the ipsec branch from e8be4c8 to cab626f Compare December 25, 2019 22:25

tnqn approved these changes Dec 26, 2019

View reviewed changes

jianjuns merged commit a5d9c42 into master Dec 27, 2019

jianjuns deleted the ipsec branch December 27, 2019 00:21

jianjuns mentioned this pull request Jan 22, 2020

Support IPSec ESP for tunnel traffic #62

Closed

4 tasks

Enable IPSec encryption of tunnel traffic #209

Enable IPSec encryption of tunnel traffic #209

Conversation

jianjuns commented Dec 9, 2019

antrea-bot commented Dec 9, 2019

jianjuns commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antrea-bot commented Dec 19, 2019

jianjuns commented Dec 20, 2019

jianjuns commented Dec 22, 2019

jianjuns commented Dec 23, 2019

Choose a reason for hiding this comment

jianjuns Dec 24, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns commented Dec 24, 2019

jianjuns commented Dec 24, 2019

jianjuns commented Dec 24, 2019

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

jianjuns commented Dec 24, 2019

jianjuns commented Dec 24, 2019

edwardbadboy commented Dec 25, 2019

edwardbadboy commented Dec 25, 2019

edwardbadboy commented Dec 25, 2019

jianjuns commented Dec 25, 2019

tnqn commented Dec 26, 2019

jianjuns Dec 24, 2019 •

edited

Loading