Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cilium: fixes transparent encryption #11974

Merged
merged 2 commits into from Jun 9, 2020
Merged

cilium: fixes transparent encryption #11974

merged 2 commits into from Jun 9, 2020

Conversation

jrfastab
Copy link
Contributor

@jrfastab jrfastab commented Jun 9, 2020

Fix a couple issues with transparent encryption. First ensure encryption mark field is not replaced with source identity. If packet is being encrypted it is a remote packet and source identity in the mark field is not needed. Next on kernels without FIB support we may still have a encryption interface. By using the interface with redirect encryption will continue to work even when an explicit route has not been configured. With above two fixes GKE using standard configuration from guides will work correctly.

Finally, on GKE systems and a local cluster the endpoint k8s watcher was reporting a zero key. By doing an explicit assignment this is resolved. I'm not entirely clear what is happening here but its clear the event has a correct encryption key identifier and the post translated even drops the encryption key id. By doing the assignment directly the key is now correct. TBD understand golang parts here.

fix transparent encryption related bugs

Encryption fixes, when setting the encrypt ctx->mark field we need to
skip resetting it with the source identity. Its not needed in the
encryption case anywasy because we already checked the endpoint is
remote before encoding encryption signal.

Next if fib lookup is not available we will discover the route at
init time and encode it in the ENCRYPT_IFACE define. If this field
is non-zero we should use it. Otherwise in some configurations
where there is not a route to egress in the main routing table
the packet will be dropped.

Fixes: 86db0fd ("cilium: encryption, use fib_lookup to rewrite dmac/smac")
Fixes: f25d8b9 ("bpf: Preserve source identity for hairpin via stack")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
@jrfastab jrfastab requested review from a team June 9, 2020 07:17
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@jrfastab
Copy link
Contributor Author

jrfastab commented Jun 9, 2020

test-focus K8sDatapathConfig.*Check connectivity with transparent encryption"

@@ -684,7 +686,7 @@ func ConvertToCiliumEndpoint(obj interface{}) interface{} {
Labels: nil,
Annotations: nil,
},
Encryption: &concreteObj.Status.Encryption,
Encryption: &encryptVal,
Copy link
Member

Choose a reason for hiding this comment

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

Is this change really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand it at the moment but its causing encryption key to be zero without it on my GKE cluster.

@coveralls
Copy link

coveralls commented Jun 9, 2020

Coverage Status

Coverage decreased (-0.02%) to 36.992% when pulling 50b0c70 on fix-mark-ipsec into 55fccf6 on master.

We observed in the K8sWatcher for "ciliumendpoints" the call
ConvertToCiliumEndpointAddFunc was taking an endpoint event with a
valid Encryption field and converting it to '0'.

To fix we can make the translation more explicit.

Fixes: 720c0b0 ("pkg/k8s: do not DeepCopy when converting to CiliumEndpoint")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: André Martins <andre@cilium.io>
@aanm
Copy link
Member

aanm commented Jun 9, 2020

test-me-please

@aanm aanm added needs-backport/1.8 release-note/misc This PR makes changes that have no direct user impact. labels Jun 9, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.0 Jun 9, 2020
@jrfastab
Copy link
Contributor Author

jrfastab commented Jun 9, 2020

Issue filed for K8s-1.11-Kernel-netnext, that appears to be unrelated those tests are running without encryption.

#11985

@jrfastab
Copy link
Contributor Author

jrfastab commented Jun 9, 2020

retest-net-next

Copy link
Contributor Author

@jrfastab jrfastab left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@joestringer joestringer merged commit b41bbc0 into master Jun 9, 2020
1.8.0 automation moved this from In progress to Merged Jun 9, 2020
@joestringer joestringer deleted the fix-mark-ipsec branch June 9, 2020 18:24
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.5 Jun 9, 2020
Copy link
Contributor Author

@jrfastab jrfastab left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.5 Jun 11, 2020
@aanm aanm mentioned this pull request Jun 11, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 11, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 11, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 12, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.5 Jun 12, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.5 Jun 12, 2020
@aanm aanm added release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.7.5
Backport done to v1.7
1.8.0
  
Merged
1.8.0
Backport done to v1.8
Development

Successfully merging this pull request may close these issues.

None yet

5 participants