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.

Sign up for GitHub

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

Jump to bottom

Fix encryption in IPAM ENI mode #14924

Merged
merged 4 commits into from Feb 12, 2021
Merged

Fix encryption in IPAM ENI mode #14924

merged 4 commits into from Feb 12, 2021

Conversation

aditighag
Copy link
Member

@aditighag aditighag commented Feb 10, 2021 •
edited

The PR consists of two main fixes to make encryption functional on EKS -

  • Add routing rules for cilium_host ENI secondary IP address
  • Attach bpf_network program to the correct interface for decryption to work

Following is out of scope of the PR, and will be addressed in follow-up PRs to simplify backporting :

  • Refactor loader.Reinitialize(...)
  • Review back-and-forth type conversions for RoutingInfo
  • Writing datapath header defines (IPV4_ENCRYPT_IFACE and ENCRYPT_IFACE)

Release note

Make pod-to-pod encryption functional in the IPAM ENI mode.

@aditighag
ipam: Rename ENI Master field to PrimaryMAC
cbbad61 
Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag requested review from a team February 10, 2021 08:09
@aditighag aditighag requested a review from a team as a code owner February 10, 2021 08:09
@aditighag aditighag requested a review from a team February 10, 2021 08:09
@aditighag aditighag requested a review from a team as a code owner February 10, 2021 08:09
@aditighag aditighag requested review from a team and nathanjsweet February 10, 2021 08:09
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 10, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Feb 10, 2021
@aditighag aditighag marked this pull request as draft February 10, 2021 08:10
@aditighag aditighag added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Feb 10, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 10, 2021
@aditighag aditighag marked this pull request as ready for review February 10, 2021 17:22
@aditighag
Copy link
Member Author

test-me-please

christarazi
christarazi reviewed Feb 10, 2021
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Nice! Core idea of the PR seems good to me. I have some questions on how we handle dependencies / importing packages.

daemon/cmd/ipam.go Outdated Show resolved Hide resolved
pkg/node/address.go Outdated Show resolved Hide resolved
pkg/datapath/loader/base.go Outdated Show resolved Hide resolved
@aditighag
Copy link
Member Author

@aditighag would you mind sending the docs changes out as a separate PR? I think the rest was almost ready to merge, but for the docs we may need to iterate a few times (and none of that will require triggering CI). Also would be good to specifically get John's input on the docs side as I think there are some extra missing pieces that we haven't yet taken into account.

Yeah, that makes sense. We'll need to iterate over the document once endpoint-routes is set to to true by default anyway.

@aditighag aditighag removed the request for review from a team February 11, 2021 18:41
@aditighag
Copy link
Member Author

test-me-please

@aditighag aditighag mentioned this pull request Feb 11, 2021
Merged
@aditighag
Copy link
Member Author

test-me-please

@joestringer
Copy link
Member

GKE job hit known flake #14915 , otherwise CI looks good.

@nathanjsweet nathanjsweet merged commit 2c5e192 into cilium:master Feb 12, 2021
1.10.0 automation moved this from In progress to Done Feb 12, 2021
christarazi
christarazi reviewed Feb 12, 2021
View reviewed changes
Copy link
Member

@christarazi christarazi 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! Mostly minor nits to followup on; not urgent

daemon/cmd/ipam.go
routingInfo, err = linuxrouting.NewRoutingInfo(result.GatewayIP, result.CIDRs,
result.PrimaryMAC, result.InterfaceNumber, option.Config.EnableIPv4Masquerade)
if err != nil {
err = fmt.Errorf("failed to create router info %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

"failed to create router info: %w" (colon)

pkg/datapath/linux/routing/routing.go
Comment on lines +199 to +200
// SetupRules installs routing rules based on the passed attributes. It accounts
// for option.Config.EgressMultiHomeIPRuleCompat while configuring the rules.
Copy link
Member

Choose a reason for hiding this comment

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

Might make sense to clarify this to say routing rules for Azure / ENI modes

pkg/datapath/linux/routing/routing.go
@@ -195,6 +196,44 @@ func Delete(ip net.IP, compat bool) error {
return nil
}

// SetupRules installs routing rules based on the passed attributes. It accounts
// for option.Config.EgressMultiHomeIPRuleCompat while configuring the rules.
func SetupRules(from, to *net.IPNet, mac string, ifaceNum int) error {
Copy link
Member

Choose a reason for hiding this comment

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

Might also make sense to call this SetupEgressRules

pkg/datapath/linux/routing/routing.go
Comment on lines +218 to +224
return route.ReplaceRule(route.Rule{
Priority: prio,
From: from,
To: to,
Table: tableId,
})
}
Copy link
Member

Choose a reason for hiding this comment

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

You could now use this function to replace where Configure() creates egress rules (just a few lines above)

pkg/datapath/linux/routing/routing.go
tableId int
)

if option.Config.EgressMultiHomeIPRuleCompat {
Copy link
Member

Choose a reason for hiding this comment

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

If we converge the egress rule creation with Configure(), then we can leverage the fact that Configure() already takes in a compat bool which represents this option, so we can get rid of this line and replace it with:

if compat {

pkg/datapath/linux/routing/routing.go
func retrieveIfaceIdxFromMAC(mac string) (int, error) {
iface, err := retrieveIfaceFromMAC(mac)
if err != nil {
err = fmt.Errorf("failed to get iface index with MAC %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

"failed to get iface index with MAC %w" ->

"failed to get iface index by MAC: %w"

pkg/datapath/loader/base.go
info := node.GetRouterInfo()
cidrs := info.GetIPv4CIDRs()
routerIP := net.IPNet{
IP: nodeAddressing.IPv4().Router(),
Copy link
Member

Choose a reason for hiding this comment

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

Were you able to call node.GetInternalIP() to get rid of the additional param to this function?

pkg/datapath/loader/base.go
mac := info.GetMac()
iface, err := linuxrouting.RetrieveIfaceNameFromMAC(mac.String())
if err != nil {
log.WithError(err).WithField("mac", mac).Fatal("Failed to set encrypt interface in the ENI ipam mode")
Copy link
Member

Choose a reason for hiding this comment

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

logfields.MAC

@pchaigno pchaigno mentioned this pull request Feb 12, 2021
Merged
@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.7 Feb 12, 2021
@pchaigno pchaigno mentioned this pull request Feb 12, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.5 Feb 12, 2021
@aditighag aditighag added the area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. label Feb 12, 2021
@joestringer joestringer moved this from In progress to Done in 1.10.0 Feb 12, 2021
@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.7 Feb 15, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.5 Feb 15, 2021
@joestringer joestringer mentioned this pull request Feb 17, 2021
Merged
@aditighag aditighag mentioned this pull request Feb 24, 2021
Merged
@christarazi christarazi mentioned this pull request Mar 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi christarazi left review comments

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@joestringer joestringer joestringer approved these changes

@jrajahalme jrajahalme Awaiting requested review from jrajahalme

Assignees

@nathanjsweet nathanjsweet

@joestringer joestringer

@christarazi christarazi

@jrajahalme jrajahalme

Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.10.0
Done
1.8.7
Backport done to v1.8
1.9.5
Backport done to v1.9
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@aditighag @joestringer @nathanjsweet @christarazi @pchaigno @aanm @jrajahalme