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

Follow ups for host firewall support of endpoint routes #15942

Merged
merged 4 commits into from
May 10, 2021

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Apr 29, 2021

See commits for details.

Updates: #13121.

@pchaigno pchaigno added release-note/misc This PR makes changes that have no direct user impact. area/host-firewall Impacts the host firewall or the host endpoint. needs-backport/1.10 labels Apr 29, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.0-rc2 Apr 29, 2021
@pchaigno pchaigno force-pushed the hostfw-ep-route-followups branch from 5a1a886 to 87f8c17 Compare May 6, 2021 11:23
The host firewall is now supported when running with endpoint routes
enabled. We can remove the note from the getting started guide.

Signed-off-by: Paul Chaignon <paul@cilium.io>
Detail why we won't loop when jumping from bpf_lxc to bpf_host and back
for ingress to the pods with host firewall enabled.

Suggested-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Paul Chaignon <paul@cilium.io>
When endpoint routes are enabled, the host firewall requires the
remote-node identity to be enabled as well to function properly.
The host identity is used to detect traffic that should be sent through
bpf_host on egress/ingress of bpf_lxc. If the remote-node identity is
disabled, the host identity then also matches remote-node IPs.

Reported-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Paul Chaignon <paul@cilium.io>
HOST_EP_ID was defined as a static data variable in a695f53 ("Endpoint
for host") because the ID of the host endpoint wasn't known at the time
we generated the datapath header files. Commit 81f0626 ("datapath: Include
the host endpoint ID in all endpoint headers") changed this by making
the host endpoint ID available globally to the agent via the node
package and by setting the ID sooner on startup.

We can therefore define the HOST_EP_ID as a normal constant now, without
needing static data. That in turns allows us to revert commit 04cbfa1
("bpf: Lift constraint for inline asm of static tail calls").

We however still use a template-specific value in bpf_host for the BPF
program injected in the policy map (see TEMPLATE_HOST_EP_ID). We could
instead directly use HOST_EP_ID for the program's name, but we would
then have to ignore that symbol when replacing symbols in the ELF [1].
To ignore that symbol we would also have to ignore similar symbols
corresponding to pod endpoints (i.e., all "1/xxxx" symbols), opening the
door to silent errors. It's probably not worth it since there's no
benefit to using HOST_EP_ID for the program's name directly.

1 - https://github.com/cilium/cilium/blob/v1.10.0-rc0/pkg/datapath/loader/cache.go#L39
Suggested-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the hostfw-ep-route-followups branch from 87f8c17 to 35f215e Compare May 7, 2021 16:42
@pchaigno
Copy link
Member Author

pchaigno commented May 7, 2021

test-me-please

@pchaigno pchaigno marked this pull request as ready for review May 7, 2021 19:03
@pchaigno pchaigno requested review from a team May 7, 2021 19:03
@pchaigno pchaigno requested review from a team as code owners May 7, 2021 19:03
@pchaigno pchaigno requested review from jibi and qmonnet May 7, 2021 19:03
@pchaigno
Copy link
Member Author

GKE hit known flake #16004, k8s-1.16-kernel-netnext hit known flakes #13071 and #15455, and runtime failed with known flake #16040. Other tests are passing and reviewers are in. Marking as ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 10, 2021
@ti-mo ti-mo merged commit bf48876 into cilium:master May 10, 2021
@pchaigno pchaigno deleted the hostfw-ep-route-followups branch May 10, 2021 14:07
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.0-rc2 May 12, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.0-rc2 May 13, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.0-rc2 May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/host-firewall Impacts the host firewall or the host endpoint. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.10.0-rc2
Backport done to v1.10
Development

Successfully merging this pull request may close these issues.

None yet

6 participants