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

Host endpoint #10994

Merged
merged 9 commits into from May 14, 2020
Merged

Host endpoint #10994

merged 9 commits into from May 14, 2020

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Apr 15, 2020

This pull request adds the special host endpoint (needed for subsequent host network policies PR). See commit messages for details. As a summary:

  1. Consolidate host datapath (bpf_netdev and bpf_hostdev_ingress) into single file bpf_host.
  2. Remove FROM_HOST macro to be able to compile from Go side.
  3. Extract source ID resolution from handle_ipv{4,6} to reduce complexity.
  4. Add pkg/maps/callsmap package for constants of internal calls map.
  5. Refactor code with new, shared createEndpoint function.
  6. Main commit. Add special endpoint for host.
  7. Add BPF_V6_NODEPORT and BPF_V6_DIRECT_ROUTING to allow loading IPv6 addresses as either static data or a macro.
  8. Fix restore on up/downgrade paths (needed because of header file rename).

@pchaigno pchaigno added the release-note/misc This PR makes changes that have no direct user impact. label Apr 15, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 15, 2020
@coveralls
Copy link

coveralls commented Apr 15, 2020

Coverage Status

Coverage decreased (-0.1%) to 37.026% when pulling 1cc2f9c on pr/pchaigno/host-fw into 454eae1 on master.

@pchaigno pchaigno force-pushed the pr/pchaigno/host-fw branch 9 times, most recently from 28e7496 to 35f2213 Compare April 21, 2020 21:37
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Series looks great, thanks!

Some nits and remarks, mostly low-level.
Should we maybe add something to the Documentation/? Documentation/policy/intro.rst and Documentation/policy/language.rst look like good candidates.

contrib/vagrant/start.sh Outdated Show resolved Hide resolved
contrib/vagrant/start.sh Outdated Show resolved Hide resolved
Documentation/cmdref/cilium-agent.md Outdated Show resolved Hide resolved
bpf/bpf_netdev.c Outdated Show resolved Hide resolved
bpf/bpf_netdev.c Outdated Show resolved Hide resolved
bpf/bpf_netdev.c Outdated Show resolved Hide resolved
bpf/bpf_netdev.c Outdated Show resolved Hide resolved
bpf/bpf_netdev.c Outdated Show resolved Hide resolved
daemon/cmd/daemon_main.go Outdated Show resolved Hide resolved
examples/policies/l3/host/host-policy.json Outdated Show resolved Hide resolved
@pchaigno pchaigno force-pushed the pr/pchaigno/host-fw branch 2 times, most recently from e3705b0 to 48f70f4 Compare April 24, 2020 15:28
@pchaigno pchaigno marked this pull request as ready for review April 24, 2020 22:56
@pchaigno pchaigno requested a review from a team April 24, 2020 22:56
@pchaigno pchaigno requested a review from a team as a code owner April 24, 2020 22:56
@pchaigno pchaigno requested a review from a team April 24, 2020 22:56
@pchaigno pchaigno requested review from a team as code owners April 24, 2020 22:56
@pchaigno pchaigno requested review from a team April 24, 2020 22:56
@joestringer joestringer added kind/feature This introduces new functionality. release-note/major This PR introduces major new functionality to Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Apr 25, 2020
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Exciting stuff! I only got about 3-4 commits in as it's a pretty big PR, will come back later.

Is there any chance we can further split this PR? The Github UI is painfully slow to interact with for this PR in its current state.

bpf/bpf_netdev.c Show resolved Hide resolved
bpf/Makefile Outdated Show resolved Hide resolved
bpf/init.sh Outdated Show resolved Hide resolved
bpf/init.sh Outdated Show resolved Hide resolved
daemon/cmd/daemon_main.go Show resolved Hide resolved
pkg/endpoint/endpoint.go Outdated Show resolved Hide resolved
pkg/endpoint/endpoint.go Outdated Show resolved Hide resolved
pkg/endpointmanager/manager.go Outdated Show resolved Hide resolved
pkg/endpointmanager/manager.go Outdated Show resolved Hide resolved
pkg/endpointmanager/manager.go Outdated Show resolved Hide resolved
@joestringer joestringer added the upgrade-impact This PR has potential upgrade or downgrade impact. label Apr 25, 2020
@pchaigno
Copy link
Member Author

@joe I've addressed (or answered) your comments. Please note I had to add a commit (bpf: Introduce BPF_V6_NODEPORT to load NodePort IPv6 addresses) to fix bpf_xdp loading in bpf/init.sh because we sometimes define the NodePort IPv6 address as static data (for bpf_host) and sometimes as a macro (for bpf_xdp).

@pchaigno pchaigno requested a review from joestringer May 12, 2020 15:28
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

:shipit:

bpf/init.sh Outdated Show resolved Hide resolved
@aanm aanm removed the dont-merge/bad-bot To prevent MLH from marking ready-to-merge. label May 13, 2020
@pchaigno
Copy link
Member Author

pchaigno commented May 13, 2020

Failed with:

Suite-k8s-1.18.K8sFQDNTest Restart Cilium validate that FQDN is still working
Suite-k8s-1.18.K8sDatapathConfig Encapsulation Check connectivity with VXLAN encapsulation
Suite-k8s-1.18.K8sDatapathConfig ManagedEtcd Check connectivity with managed etcd

retest-4.9

Suite-k8s-1.18.K8sHealthTest checks cilium-health status between nodes

@pchaigno pchaigno force-pushed the pr/pchaigno/host-fw branch 2 times, most recently from b489494 to 11821ee Compare May 14, 2020 10:02
@pchaigno pchaigno requested a review from a team as a code owner May 14, 2020 10:02
bpf_netdev.c is renamed to bpf_host and bpf_hostdev_ingress is included,
in preparation for the host endpoint that requires a single object file.

Signed-off-by: Paul Chaignon <paul@cilium.io>
For the host firewall, we need to have a single bpf_host object file
containing both the from-host and the from-netdev paths, to load it from
the Go side. We therefore turn FROM_HOST into a variable set to true or
false depending on the entry point (i.e., from_host() vs. from_netdev()).

The other option (to avoid the increased code complexity in datapath) is
to let the Go side handle several object files and header files for the
special host endpoint. That second option is likely to just move the
increase code complexity to the Go side.

The CILIUM_CALL_IPV{4,6}_FROM_LXC entry points (and handle_ipv{4,6}) are
broken into CILIUM_CALL_IPV{4,6}_FROM_LXC and
CILIUM_CALL_IPV{4,6}_FROM_HOST based on the value of from_host. This
change allows the compiler to perform dead code elimination on from_host
and reduce the program sizes to <4096.

Signed-off-by: Paul Chaignon <paul@cilium.io>
handle_ipv{4,6} have the highest complexity as reported by the BPF
verifier. We can extract the source ID resolution from these programs
and execute it before the tail calls. This comes at zero cost in term of
metadata slots since we already use the same metadata slot to transfer the
source identity from the proxy across the tail call.

Signed-off-by: Paul Chaignon <paul@cilium.io>
The package contains a single constant definition right now but will
have more with the host endpoint in later commits.

Signed-off-by: Paul Chaignon <paul@cilium.io>
This function is also going to be used to create the host endpoint in
subsequent commits.

Signed-off-by: Paul Chaignon <paul@cilium.io>
This commit adds a special Cilium endpoint on each node to represent the
node itself. It takes the reserved security ID 1. The special endpoint is
created by endpoint.CreateLocalNodeEndpoint on daemon startup if that
endpoint wasn't already restored.

When the datapath for the special endpoint is regenerated, it loads
bpf_host.o instead of bpf_lxc.o. bpf_host enforces policies for both
traffic to/from pods and traffic to/from the outside world. It follows
the same template generation path as traditional bpf_lxc files.

ENABLE_EXTRA_HOST_DEV was only defined in init.sh for the from_host path.
It is now defined from the Go side for all paths, and we check whether
from_host is true in the C code instead.

Signed-off-by: Paul Chaignon <paul@cilium.io>
We define IPV6_NODEPORT and IPV6_DIRECT_ROUTING both from Golang (for
bpf_netdev) and init.sh (for bpf_xdp). On Go side it is defined as static
data whose value is patched before loading. In init.sh the values are
given as simple macros. IPv6 addresses defined as static data rely on
BPF_V6(), but BPF_V6() doesn't support macros.

Thus, we define BPF_V6_NODEPORT() and BPF_V6_DIRECT_ROUTING() which will
fallback to BPF_V6 in case of static data and just copy the value
otherwise.

For IPV6_DIRECT_ROUTING, this workaround can be removed once we move
it's definition to node_config.h. For IPV6_NODEPORT, a better, long-term
fix would be to load the bpf_xdp program from Golang.

Signed-off-by: Paul Chaignon <paul@cilium.io>
Because we changed the C header filenames (from lxc_config.h to
ep_config.h), endpoints don't get properly restored on upgrades and
downgrades.

This commit fixes the up/downgrade paths by 1) creating a lxc_config.h
symlink for new ep_config.h header files and 2) renaming the header file
and creating the symlink when restoring endpoints on an upgrade. On a
downgrade, Cilium will pick up the lxc_config.h symlink.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno
Copy link
Member Author

This last push fixes a couple bugs:

  • An incorrect warning message found by @aanm: d2d2315#diff-52900f79a52483fa5b629cc6bcf0d1a8R153. We need to check the symlink doesn't already exists before creating it.
  • Incorrect IPCACHE{4,6}_PREFIXES for the host endpoint, with help from @joestringer. See the new, last commit, endpoint: Use CIDR prefixes from Daemon for host endpoint.
  • An incorrect logic reversion in security ID resolution: 2c87074#diff-cb45ec3dbb54bef19894165ec1a2582eR313. This was !from_host and needs to be from_host because, in init.sh, ENABLE_EXTRA_HOST_DEV was only defined for the from-host path.

Because the host endpoint relies on bpf_host instead of bpf_lxc, it
needs the CIDR prefixes maintained by the Daemon instead of
endpoint-specific CIDR prefixes. The former are already printed in
netdev_config.h but the host endpoint datapath doesn't use that header
file anymore. Instead, we need to print the appropriate CIDR prefixes
(the Daemon's) into the host endpoint's ep_config.h header file.

We therefore also need to regenerate the host endpoint whenever CIDR
policies change, as we already do for the base programs.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno
Copy link
Member Author

test-me-please

@pchaigno
Copy link
Member Author

  • Runtime-4.9 hit known CI bug CI: 4.9 Ginkgo-CI-Tests-runtime-Pipeline broken #11512.
  • K8s-1.18-kernel-4.9 hit what looks like a flake (K8sHealthTest checks cilium-health status between nodes). I couldn't find any warning/error/oddity related to the host endpoint in logs or in bugtool. There are some Unable to restore endpoint, ignoring messages due to a missing interface; not sure they are related. I've tried running the test in a loop locally and am unable to reproduce.

@aanm aanm merged commit ad6ef2a into master May 14, 2020
1.8.0 automation moved this from In progress to Merged May 14, 2020
@pchaigno pchaigno deleted the pr/pchaigno/host-fw branch May 14, 2020 14:24
@pchaigno pchaigno added the area/host-firewall Impacts the host firewall or the host endpoint. label Jul 20, 2020
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. kind/feature This introduces new functionality. release-note/major This PR introduces major new functionality to Cilium. upgrade-impact This PR has potential upgrade or downgrade impact.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

7 participants