Skip to content
This repository was archived by the owner on Jun 29, 2022. It is now read-only.

Add encryption to in-cluster pod traffic#911

Merged
knrt10 merged 6 commits intomasterfrom
knrt10/encypting-cluster-traffic
Sep 30, 2020
Merged

Add encryption to in-cluster pod traffic#911
knrt10 merged 6 commits intomasterfrom
knrt10/encypting-cluster-traffic

Conversation

@knrt10
Copy link
Contributor

@knrt10 knrt10 commented Sep 7, 2020

Calico v3.15+ support wireguard and if enabled it encrypts in-cluster
pod traffic across nodes.

Signed-off-by: knrt10 kautilya@kinvolk.io

Release Notes

  • delete default FelixConfiguration while upgrading from 0.4.1
  • network_mtu changed from CNI network MTU to Physical network MTU.
  • bpfLogLevel will be briefly set from Info to Off.
  • Following ports will be opened on all hosts for the upgrade time between calico chart upgrade and calico-host-protection chart:
  - 22/tcp
  - 68/udp
  - 179/tcp
  - 2379/tcp
  - 2380/tcp
  - 5473/tcp
  - 6443/tcp
  - 6666/tcp
  - 6667/tcp

@knrt10 knrt10 requested a review from invidian September 7, 2020 08:30
@knrt10 knrt10 force-pushed the knrt10/encypting-cluster-traffic branch from 6cd5126 to beb2608 Compare September 7, 2020 09:01
@knrt10 knrt10 changed the title Add encryption to cluster traffic Add encryption to in-cluster pod traffic Sep 7, 2020
@knrt10 knrt10 linked an issue Sep 7, 2020 that may be closed by this pull request
@knrt10 knrt10 requested review from iaguis and removed request for johananl and surajssd September 7, 2020 09:40
@knrt10 knrt10 force-pushed the knrt10/encypting-cluster-traffic branch 2 times, most recently from a58f31e to 8b50f72 Compare September 7, 2020 11:59
@iaguis
Copy link
Contributor

iaguis commented Sep 7, 2020

We should also change the MTU size when enabling this as mentioned here.

Would encryption be picked up by the Calico chart on lokoctl apply? Would the MTU?

@knrt10 knrt10 force-pushed the knrt10/encypting-cluster-traffic branch from 8b50f72 to 7f07add Compare September 7, 2020 12:24
@knrt10 knrt10 force-pushed the knrt10/encypting-cluster-traffic branch from 7f07add to 51687a4 Compare September 7, 2020 12:49
@invidian
Copy link
Member

invidian commented Sep 7, 2020

Would encryption be picked up by the Calico chart on lokoctl apply? Would the MTU?

If encryption is enabled and Calico does not align MTU by default, then I guess we can do it automatically?

@knrt10 knrt10 force-pushed the knrt10/encypting-cluster-traffic branch 5 times, most recently from c526052 to 2239711 Compare September 8, 2020 12:30
@knrt10 knrt10 requested a review from invidian September 8, 2020 12:51
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.

@knrt10 knrt10 force-pushed the knrt10/encypting-cluster-traffic branch from 2239711 to ef0e9bb Compare September 8, 2020 14:10
@knrt10 knrt10 requested a review from invidian September 8, 2020 14:11
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one more question, as I'm not sure if we don't change the existing behavior with failsafe ports.

@knrt10 knrt10 force-pushed the knrt10/encypting-cluster-traffic branch from ef0e9bb to ed891c6 Compare September 8, 2020 14:56
@johananl
Copy link
Member

I think we should make it a habit to include a "how to test" section in PRs. I just tried using this feature using Flatcar stable which doesn't work because the kernel is too old.

Copy link
Member

@johananl johananl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The encryption seems to work fine provided that you don't use stable. Nice job @knrt10.

I've left a couple of comments.

@knrt10 knrt10 dismissed stale reviews from ipochi and invidian via cab4983 September 29, 2020 07:03
@knrt10 knrt10 force-pushed the knrt10/encypting-cluster-traffic branch from 6cef80f to cab4983 Compare September 29, 2020 07:03
@knrt10 knrt10 requested review from invidian and ipochi September 29, 2020 07:05
@knrt10 knrt10 force-pushed the knrt10/encypting-cluster-traffic branch 2 times, most recently from be1b98d to e4758b4 Compare September 30, 2020 06:55
@knrt10 knrt10 requested a review from johananl September 30, 2020 06:56
@knrt10 knrt10 force-pushed the knrt10/encypting-cluster-traffic branch 2 times, most recently from db862da to 7cb540b Compare September 30, 2020 11:22
@knrt10 knrt10 requested a review from invidian September 30, 2020 11:23
Felixconfig should be part of calico charts and not
calico-host-protection. This change is required for encrypting
in-cluster pod traffic.

This also introduces failsafeInboundHostPorts as a variable.

Signed-off-by: knrt10 <kautilya@kinvolk.io>
@knrt10 knrt10 force-pushed the knrt10/encypting-cluster-traffic branch 2 times, most recently from 844d1ca to a5a5eb4 Compare September 30, 2020 12:19
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some last nits, otherwise LGTM.

knrt10 added 5 commits September 30, 2020 18:31
Previously we used CNI network MTU in terraform, but now using default
as Physical network MTU.

Signed-off-by: knrt10 <kautilya@kinvolk.io>
Signed-off-by: knrt10 <kautilya@kinvolk.io>
Calico v3.15+ support WireGuard and if enabled it encrypts in-cluster
pod traffic across nodes.

Signed-off-by: knrt10 <kautilya@kinvolk.io>
Signed-off-by: knrt10 <kautilya@kinvolk.io>
Signed-off-by: knrt10 <kautilya@kinvolk.io>
@knrt10 knrt10 force-pushed the knrt10/encypting-cluster-traffic branch from a5a5eb4 to 7f891d8 Compare September 30, 2020 13:04
@knrt10 knrt10 requested a review from invidian September 30, 2020 13:05
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some commit structure nits, no blockers from merging really.

Nice work @knrt10, LGTM ✔️


disk_iops = var.disk_iops

network_mtu = 1480
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this commit could be merged with the one changing the code.


enable_tls_bootstrap = true

encrypt_pod_traffic = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs can be committed with the implementation.

Copy link
Contributor

@iaguis iaguis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We should mention in the release notes that we've changed the meaning of the network_mtu parameter.

@knrt10
Copy link
Contributor Author

knrt10 commented Sep 30, 2020

Thank you all for the reviews. Merging now.

@knrt10 knrt10 merged commit 2d55a8b into master Sep 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider encrypting in-cluster traffic

6 participants